| Message ID | 1415876417-28728-14-git-send-email-peter.griffin@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 13 November 2014 11:00:16 Peter Griffin wrote: > + soc { > + usb2_picophy0: usbpicophy@0 { > + compatible = "st,stih407-usb2-phy"; > + reg = <0xf8 0x04>, /* syscfg 5062 */ > + <0xf4 0x04>; /* syscfg 5061 */ > I think the node name for the phy should be "phy@f8" instead of usbpicophy@0 by common convention. I notice that there are some existing instances of this, you can probably change them as well. Linux doesn't normally care about the node names. It also seems that you have put the node in the wrong place, as the reg property apparently refers to a different address space. Did you mean to put this under the syscfg_core node? Arnd
Hi Arnd, Thanks for reviewing. On Thu, 13 Nov 2014, Arnd Bergmann wrote: > On Thursday 13 November 2014 11:00:16 Peter Griffin wrote: > > + soc { > > + usb2_picophy0: usbpicophy@0 { > > + compatible = "st,stih407-usb2-phy"; > > + reg = <0xf8 0x04>, /* syscfg 5062 */ > > + <0xf4 0x04>; /* syscfg 5061 */ > > > > I think the node name for the phy should be "phy@f8" instead of usbpicophy@0 > by common convention. I notice that there are some existing instances of > this, you can probably change them as well. Linux doesn't normally care > about the node names. Ok will fix in v3 > > It also seems that you have put the node in the wrong place, as the reg > property apparently refers to a different address space. Did you mean > to put this under the syscfg_core node? Your correct in that it refers to the syscfg_core address space. However I intentionaly didn't put it under the syscfg_core node. The phy is more unique than most other devices in that in this instance it is only controlled from two syscfg_core registers which happen to be in the same sysconf bank. However most other devices tend to have a combination of some memory mapped registers and also some sysconfig registers which does then create a conflict over where the dt node should live. Currently I can't find an example but there is also no guarentee that a device will not have some configuration bits in syscfg_core and some other bits in syscfg_rear/front registers. The phy for example could have had one register in each, which would make deciding where to put it difficult. So to create coherency / conformity we decided to put all the IP blocks under the soc node. Also its worth pointing if your not already aware that sysconf_core/rear/front isn't really a device, it's just a regmap abstraction of some memory mapped configuration registers where a bunch of seemingly random control bits get stuffed. Of course if you feel strongly about it I can of course change it like you suggested, but that is the reasoning / rationale of why it was done like this initially. regards, Peter.
On Thursday 13 November 2014 14:54:19 Peter Griffin wrote: > > > > > It also seems that you have put the node in the wrong place, as the reg > > property apparently refers to a different address space. Did you mean > > to put this under the syscfg_core node? > > Your correct in that it refers to the syscfg_core address space. > However I intentionaly didn't put it under the syscfg_core node. > > The phy is more unique than most other devices in that in this instance it > is only controlled from two syscfg_core registers which happen to be in the same > sysconf bank. > > However most other devices tend to have a combination of some memory mapped > registers and also some sysconfig registers which does then create a conflict > over where the dt node should live. > > Currently I can't find an example but there is also no guarentee that a > device will not have some configuration bits in syscfg_core and some > other bits in syscfg_rear/front registers. The phy for example could > have had one register in each, which would make deciding where to put > it difficult. > > So to create coherency / conformity we decided to put all the IP blocks under the soc node. > > Also its worth pointing if your not already aware that sysconf_core/rear/front isn't > really a device, it's just a regmap abstraction of some memory mapped configuration > registers where a bunch of seemingly random control bits get stuffed. > > Of course if you feel strongly about it I can of course change it like you suggested, > but that is the reasoning / rationale of why it was done like this initially. The problem is your use of the 'reg' property. If it doesn't refer to the node's address space, it shouldn't be called 'reg'. Please fix the binding. Arnd
Hi Arnd, On Thu, 13 Nov 2014, Arnd Bergmann wrote: > On Thursday 13 November 2014 14:54:19 Peter Griffin wrote: > > > > > > > > It also seems that you have put the node in the wrong place, as the reg > > > property apparently refers to a different address space. Did you mean > > > to put this under the syscfg_core node? > > > > Your correct in that it refers to the syscfg_core address space. > > However I intentionaly didn't put it under the syscfg_core node. > > > > The phy is more unique than most other devices in that in this instance it > > is only controlled from two syscfg_core registers which happen to be in the same > > sysconf bank. > > > > However most other devices tend to have a combination of some memory mapped > > registers and also some sysconfig registers which does then create a conflict > > over where the dt node should live. > > > > Currently I can't find an example but there is also no guarentee that a > > device will not have some configuration bits in syscfg_core and some > > other bits in syscfg_rear/front registers. The phy for example could > > have had one register in each, which would make deciding where to put > > it difficult. > > > > So to create coherency / conformity we decided to put all the IP blocks under the soc node. > > > > Also its worth pointing if your not already aware that sysconf_core/rear/front isn't > > really a device, it's just a regmap abstraction of some memory mapped configuration > > registers where a bunch of seemingly random control bits get stuffed. > > > > Of course if you feel strongly about it I can of course change it like you suggested, > > but that is the reasoning / rationale of why it was done like this initially. > > The problem is your use of the 'reg' property. If it doesn't refer to the > node's address space, it shouldn't be called 'reg'. > > Please fix the binding. I'll appologize in advance as I'm not sure I've understood you correctly, definately your answer has given me a bunch more questions to clarify my understanding... In the case of this particular node, then both reg properties refer to the syscfg_core address space as both <0xf8 0x04> and <0xf4 0x04> are offsets into syscfg_core registers. So moving it under the syscfg_core like you originaly suggested I believe would meet the definition of 'reg' ("describes the address of the device's resources within the address space defined by its parent bus"). Or maybe you meant, if I want to keep the picophy node where it is (under soc), I need to stop using the reg property? The other problem I was describing is a device with some memory mapped registers and some sysconfig registers you can find examples already upstream in stih416.dtsi file with the ethernet0 and miphy365x_phy nodes. Here the first 1/2 reg properties are expressed as a 32 bit physical address and size (meets the spec definition), and the last is expressed as a sysconfig offset and size (which AFAIK doesn't match the spec definition). This change in address space is however documented in the bindings documentation. How should devices which have multiple address spaces ideally be handled? From looking around in the source I see a few different ways people have done this: - 1) Some drivers encode the data statically into the source and make a decision based on compatible string. 2) Some drivers add a couple of extra dt properties to encode the offset (spifsm in stih416) 3) Some drivers mix address spaces in the reg properties (examples given above) 3) Probably some other ways I've not found yet Is there a blessed way of doing this? It would be nice at least for all the drivers in STI to be doing it the same as currently there seems to be a mix of implementations. For the moment I think we can forget the third example in my previous email as it is currently hypothetical. regards, Peter.
On Friday 14 November 2014 09:56:21 Peter Griffin wrote: > > I'll appologize in advance as I'm not sure I've understood you correctly, definately your answer > has given me a bunch more questions to clarify my understanding... > > In the case of this particular node, then both reg properties refer to the syscfg_core address space > as both <0xf8 0x04> and <0xf4 0x04> are offsets into syscfg_core registers. So moving it under > the syscfg_core like you originaly suggested I believe would meet the definition of 'reg' ("describes the > address of the device's resources within the address space defined by its parent bus"). You will also have to add an appropriate 'ranges' property then. > Or maybe you meant, if I want to keep the picophy node where it is (under soc), I need to stop using the reg > property? That would also be possible, just put the register numbers into your 'st,syscfg' property after the phandle, and leave the reg property empty then. > The other problem I was describing is a device with some memory mapped registers and some > sysconfig registers you can find examples already upstream in stih416.dtsi file with the > ethernet0 and miphy365x_phy nodes. > > Here the first 1/2 reg properties are expressed as a 32 bit physical address and size (meets the spec definition), > and the last is expressed as a sysconfig offset and size (which AFAIK doesn't match the spec definition). > This change in address space is however documented in the bindings documentation. > > How should devices which have multiple address spaces ideally be handled? There should at most be one 'reg' address space, so put it under the node whose bus these refer to. Put all offsets that are relative to a syscon link into the phandle that refers to the syscon device, or hardcode the offsets in the driver. > From looking around in the source I see a few different ways people have done this: - > > 1) Some drivers encode the data statically into the source and make a decision based on compatible string. > 2) Some drivers add a couple of extra dt properties to encode the offset (spifsm in stih416) > 3) Some drivers mix address spaces in the reg properties (examples given above) > 3) Probably some other ways I've not found yet > > Is there a blessed way of doing this? It would be nice at least for all the drivers in STI to be doing > it the same as currently there seems to be a mix of implementations. You can do 1 or 2, not 3. Arnd
Hi Arnd, Thanks for taking the time to explain this. On Fri, 14 Nov 2014, Arnd Bergmann wrote: <snip> > > address of the device's resources within the address space defined by its parent bus"). > > You will also have to add an appropriate 'ranges' property then. > > > Or maybe you meant, if I want to keep the picophy node where it is (under soc), I need to stop using the reg > > property? > > That would also be possible, just put the register numbers into your 'st,syscfg' > property after the phandle, and leave the reg property empty then. I think this is the best approach and whay I will do, as it would also extend well to devices with sysconfig registers in different banks. > > > The other problem I was describing is a device with some memory mapped registers and some > > sysconfig registers you can find examples already upstream in stih416.dtsi file with the > > ethernet0 and miphy365x_phy nodes. > > > > Here the first 1/2 reg properties are expressed as a 32 bit physical address and size (meets the spec definition), > > and the last is expressed as a sysconfig offset and size (which AFAIK doesn't match the spec definition). > > This change in address space is however documented in the bindings documentation. > > > > How should devices which have multiple address spaces ideally be handled? > > There should at most be one 'reg' address space, so put it under the node whose > bus these refer to. Put all offsets that are relative to a syscon link into > the phandle that refers to the syscon device, or hardcode the offsets in the > driver. Ok makes sense. > > > From looking around in the source I see a few different ways people have done this: - > > > > 1) Some drivers encode the data statically into the source and make a decision based on compatible string. > > 2) Some drivers add a couple of extra dt properties to encode the offset (spifsm in stih416) > > 3) Some drivers mix address spaces in the reg properties (examples given above) > > 3) Probably some other ways I've not found yet > > > > Is there a blessed way of doing this? It would be nice at least for all the drivers in STI to be doing > > it the same as currently there seems to be a mix of implementations. > > You can do 1 or 2, not 3. Yep got it. One last question, what are the rules about updating ethernet and miphy365 over to using this method for there sysconfig registers? regards, Peter.
On Friday 14 November 2014 13:34:25 Peter Griffin wrote: > > > > > From looking around in the source I see a few different ways people have done this: - > > > > > > 1) Some drivers encode the data statically into the source and make a decision based on compatible string. > > > 2) Some drivers add a couple of extra dt properties to encode the offset (spifsm in stih416) > > > 3) Some drivers mix address spaces in the reg properties (examples given above) > > > 3) Probably some other ways I've not found yet > > > > > > Is there a blessed way of doing this? It would be nice at least for all the drivers in STI to be doing > > > it the same as currently there seems to be a mix of implementations. > > > > You can do 1 or 2, not 3. > > Yep got it. > > One last question, what are the rules about updating ethernet and miphy365 over > to using this method for there sysconfig registers? It depends on whether you have any machines that rely on this for backwards compatibility. If the platform support is still work in progress and you wouldn't be able to run a machine with an upstream kernel yet, just say in the patch description that this breaks compatibility and why it is ok in this particular case. If you have existing users that rely on these properties, make the driver code handle the existing dtb files but print a warning if you encounter them. In the documentation, you should list the new method as required then, but you can mention that the old method might be accepted for backwards compatibility. Arnd
diff --git a/arch/arm/boot/dts/stih410.dtsi b/arch/arm/boot/dts/stih410.dtsi index c05627e..5ade3ed 100644 --- a/arch/arm/boot/dts/stih410.dtsi +++ b/arch/arm/boot/dts/stih410.dtsi @@ -10,5 +10,29 @@ #include "stih407-family.dtsi" #include "stih410-pinctrl.dtsi" / { + soc { + usb2_picophy0: usbpicophy@0 { + compatible = "st,stih407-usb2-phy"; + reg = <0xf8 0x04>, /* syscfg 5062 */ + <0xf4 0x04>; /* syscfg 5061 */ + reg-names = "param", "ctrl"; + #phy-cells = <0>; + st,syscfg = <&syscfg_core>; + resets = <&softreset STIH407_PICOPHY_SOFTRESET>, + <&picophyreset STIH407_PICOPHY0_RESET>; + reset-names = "global", "port"; + }; + usb2_picophy1: usbpicophy@1 { + compatible = "st,stih407-usb2-phy"; + #phy-cells = <0>; + reg = <0xfc 0x04>, /* syscfg 5063 */ + <0xf4 0x04>; /* syscfg 5061 */ + reg-names = "param", "ctrl"; + st,syscfg = <&syscfg_core>; + resets = <&softreset STIH407_PICOPHY_SOFTRESET>, + <&picophyreset STIH407_PICOPHY1_RESET>; + reset-names = "global", "port"; + }; + }; };
This patch adds the dt nodes for the extra usb2 picophys found on the stih410. These two picophys are used in conjunction with the extra ehci/ohci usb controllers also found on the stih410. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> --- arch/arm/boot/dts/stih410.dtsi | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)