| Message ID | 1407511136-26477-3-git-send-email-ezequiel.garcia@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 08, 2014 at 12:18:52PM -0300, Ezequiel Garcia wrote: > This commit adds the required pin muxing for the network interfaces and > the MDIO interface to be properly initialized. For instance, this makes > it possible for a bootloader to initialize and access the network interfaces. > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > --- > arch/arm/boot/dts/armada-370-mirabox.dts | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts > index 097df7d8..c745256 100644 > --- a/arch/arm/boot/dts/armada-370-mirabox.dts > +++ b/arch/arm/boot/dts/armada-370-mirabox.dts > @@ -91,6 +91,8 @@ > }; > > mdio { > + pinctrl-0 = <&mdio_pins>; > + pinctrl-names = "default"; > phy0: ethernet-phy@0 { > reg = <0>; > }; > @@ -100,11 +102,15 @@ > }; > }; > ethernet@70000 { > + pinctrl-0 = <&ge0_pins>; > + pinctrl-names = "default"; Hi Ezequiel Could you put these one level higher in the DT hierarchy? i.e. once in armada-370.dtsi and not in every board .dts file? Andrew
On 08 Aug 07:12 PM, Andrew Lunn wrote: > On Fri, Aug 08, 2014 at 12:18:52PM -0300, Ezequiel Garcia wrote: > > This commit adds the required pin muxing for the network interfaces and > > the MDIO interface to be properly initialized. For instance, this makes > > it possible for a bootloader to initialize and access the network interfaces. > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > --- > > arch/arm/boot/dts/armada-370-mirabox.dts | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts > > index 097df7d8..c745256 100644 > > --- a/arch/arm/boot/dts/armada-370-mirabox.dts > > +++ b/arch/arm/boot/dts/armada-370-mirabox.dts > > @@ -91,6 +91,8 @@ > > }; > > > > mdio { > > + pinctrl-0 = <&mdio_pins>; > > + pinctrl-names = "default"; > > phy0: ethernet-phy@0 { > > reg = <0>; > > }; > > @@ -100,11 +102,15 @@ > > }; > > }; > > ethernet@70000 { > > + pinctrl-0 = <&ge0_pins>; > > + pinctrl-names = "default"; > > Hi Ezequiel > > Could you put these one level higher in the DT hierarchy? i.e. once in > armada-370.dtsi and not in every board .dts file? > Are you sure that's the correct approach? As explained on the cover letter, these MAC MPP mux are not required in the SGMII case. Moreover, this is not the only way to wire the MAC0 and MAC1 controllers, so I even considered pushing down the pinmux to the dts files! The devicetree location of the pinmux is something that still confuses me, so I'm more than open to directions here.
On Fri, Aug 08, 2014 at 03:21:56PM -0300, Ezequiel Garcia wrote: > On 08 Aug 07:12 PM, Andrew Lunn wrote: > > On Fri, Aug 08, 2014 at 12:18:52PM -0300, Ezequiel Garcia wrote: > > > This commit adds the required pin muxing for the network interfaces and > > > the MDIO interface to be properly initialized. For instance, this makes > > > it possible for a bootloader to initialize and access the network interfaces. > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > > --- > > > arch/arm/boot/dts/armada-370-mirabox.dts | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts > > > index 097df7d8..c745256 100644 > > > --- a/arch/arm/boot/dts/armada-370-mirabox.dts > > > +++ b/arch/arm/boot/dts/armada-370-mirabox.dts > > > @@ -91,6 +91,8 @@ > > > }; > > > > > > mdio { > > > + pinctrl-0 = <&mdio_pins>; > > > + pinctrl-names = "default"; > > > phy0: ethernet-phy@0 { > > > reg = <0>; > > > }; > > > @@ -100,11 +102,15 @@ > > > }; > > > }; > > > ethernet@70000 { > > > + pinctrl-0 = <&ge0_pins>; > > > + pinctrl-names = "default"; > > > > Hi Ezequiel > > > > Could you put these one level higher in the DT hierarchy? i.e. once in > > armada-370.dtsi and not in every board .dts file? > > > > Are you sure that's the correct approach? > > As explained on the cover letter, these MAC MPP mux are not required in the > SGMII case. Moreover, this is not the only way to wire the MAC0 and MAC1 > controllers, so I even considered pushing down the pinmux to the dts files! Hi Ezequiel At least at the moment, it seems like RGMII is the norm, and SGMII is the exception. So having the default as RGMII probably makes sense. Boards which don't require it can then override this in there .dts file. If we see this changes with time, we can swap it around. > The devicetree location of the pinmux is something that still confuses me, > so I'm more than open to directions here. Patch #1 comment is not really correct. It defines a number of pin configuration nodes. These nodes can then be used via phandles. What might help is naming these differently. Name them ge[01]_rgmii_pins. It is then clear when they should be used. You can also add ge[0]_sgmii_pins, which would be an empty set. You can then use ge[01]_rgmii_pins as the default in armada-370.dtsi and override them with ge[0]_sgmii_pins for the 370 RD. Experience from Kirkwood is that it also has multiple ways to wire up MAC0 and MAC1, but in practice, nobody ever does. And if they did, it can be overridden in the .dts file. Andrew
On 08 Aug 09:21 PM, Andrew Lunn wrote: > On Fri, Aug 08, 2014 at 03:21:56PM -0300, Ezequiel Garcia wrote: > > On 08 Aug 07:12 PM, Andrew Lunn wrote: > > > On Fri, Aug 08, 2014 at 12:18:52PM -0300, Ezequiel Garcia wrote: > > > > This commit adds the required pin muxing for the network interfaces and > > > > the MDIO interface to be properly initialized. For instance, this makes > > > > it possible for a bootloader to initialize and access the network interfaces. > > > > > > > > Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> > > > > --- > > > > arch/arm/boot/dts/armada-370-mirabox.dts | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts > > > > index 097df7d8..c745256 100644 > > > > --- a/arch/arm/boot/dts/armada-370-mirabox.dts > > > > +++ b/arch/arm/boot/dts/armada-370-mirabox.dts > > > > @@ -91,6 +91,8 @@ > > > > }; > > > > > > > > mdio { > > > > + pinctrl-0 = <&mdio_pins>; > > > > + pinctrl-names = "default"; > > > > phy0: ethernet-phy@0 { > > > > reg = <0>; > > > > }; > > > > @@ -100,11 +102,15 @@ > > > > }; > > > > }; > > > > ethernet@70000 { > > > > + pinctrl-0 = <&ge0_pins>; > > > > + pinctrl-names = "default"; > > > > > > Hi Ezequiel > > > > > > Could you put these one level higher in the DT hierarchy? i.e. once in > > > armada-370.dtsi and not in every board .dts file? > > > > > > > Are you sure that's the correct approach? > > > > As explained on the cover letter, these MAC MPP mux are not required in the > > SGMII case. Moreover, this is not the only way to wire the MAC0 and MAC1 > > controllers, so I even considered pushing down the pinmux to the dts files! > > Hi Ezequiel > > At least at the moment, it seems like RGMII is the norm, and SGMII is > the exception. So having the default as RGMII probably makes > sense. Boards which don't require it can then override this in there > .dts file. If we see this changes with time, we can swap it around. > OK. > > The devicetree location of the pinmux is something that still confuses me, > > so I'm more than open to directions here. > > Patch #1 comment is not really correct. It defines a number of pin > configuration nodes. These nodes can then be used via phandles. What > might help is naming these differently. Name them > ge[01]_rgmii_pins. It is then clear when they should be used. You can > also add ge[0]_sgmii_pins, which would be an empty set. You can then > use ge[01]_rgmii_pins as the default in armada-370.dtsi and override > them with ge[0]_sgmii_pins for the 370 RD. > OK. > Experience from Kirkwood is that it also has multiple ways to wire up > MAC0 and MAC1, but in practice, nobody ever does. And if they did, it > can be overridden in the .dts file. > The above sounds good, although I don't have a strong opinion. If everybody is happy with the approach, I'll prepare a v2 changing the .dtsi with RGMII as the default mux, and overriding it in the A370-RD dts. Jason, Thomas, Greg: Any objections?
Dear Andrew Lunn, On Fri, 8 Aug 2014 21:21:06 +0200, Andrew Lunn wrote: > At least at the moment, it seems like RGMII is the norm, and SGMII is > the exception. So having the default as RGMII probably makes > sense. Boards which don't require it can then override this in there > .dts file. If we see this changes with time, we can swap it around. I personally disagree with this approach. armada-370.dtsi is here to describe what happens at the *SoC* level, not to factorize some random board specific details that appear to be common between a certain number of boards (but not all). So I really, really, would prefer to keep the board-specific details such as which pin muxing is done for Ethernet in each individual .dts file. .dtsi to describe the SoC, .dts to describe the board. It's clear and simple for everyone to understand, especially for new comers. Best regards, Thomas
On 09 Aug 05:21 PM, Thomas Petazzoni wrote: > On Fri, 8 Aug 2014 21:21:06 +0200, Andrew Lunn wrote: > > > At least at the moment, it seems like RGMII is the norm, and SGMII is > > the exception. So having the default as RGMII probably makes > > sense. Boards which don't require it can then override this in there > > .dts file. If we see this changes with time, we can swap it around. > > I personally disagree with this approach. armada-370.dtsi is here to > describe what happens at the *SoC* level, not to factorize some random > board specific details that appear to be common between a certain > number of boards (but not all). > > So I really, really, would prefer to keep the board-specific details > such as which pin muxing is done for Ethernet in each individual .dts > file. > > .dtsi to describe the SoC, .dts to describe the board. It's clear and > simple for everyone to understand, especially for new comers. > Just to clarify, you say we should even have pin mux configuration in each dts, or the configuration in the dtsi and the pin muxing of each device in the dts?
Thomas, On Sat, Aug 09, 2014 at 05:21:49PM +0200, Thomas Petazzoni wrote: > Dear Andrew Lunn, > > On Fri, 8 Aug 2014 21:21:06 +0200, Andrew Lunn wrote: > > > At least at the moment, it seems like RGMII is the norm, and SGMII is > > the exception. So having the default as RGMII probably makes > > sense. Boards which don't require it can then override this in there > > .dts file. If we see this changes with time, we can swap it around. > > I personally disagree with this approach. armada-370.dtsi is here to > describe what happens at the *SoC* level, not to factorize some random > board specific details that appear to be common between a certain > number of boards (but not all). It's my understanding that *possible* pinmux configurations are a characteristic of the SoC. Which configurations are used is an artifact of the board. I don't see the need to list all possible configurations in the SoC dtsi file, but just the ones we have actually seen used. > So I really, really, would prefer to keep the board-specific details > such as which pin muxing is done for Ethernet in each individual .dts > file. So, wouldn't this be the phandle selection? > .dtsi to describe the SoC, .dts to describe the board. It's clear and > simple for everyone to understand, especially for new comers. If you're strongly opposed to describing possible pinmux configurations in the SoC dtsi file, how about an SoC-pinmux.dtsi? I just see the "Thar be dragons ahead" sign by describing pinmux in the board files. Developers adding support for a new board will typically (for better or for worse) start with a dts that's close (eg guruplug for dreamplug), and edit it to fit their needs. During the debugging process, if the pinmux is described in the dts file, they might be more prone to tweak it into a non-possible configuration for that SoC. This is a lot less likely if the possible pinmux configs are described in a dtsi file. The developer would be more likely to try different pinmux phandles in that case. fwiw, the possible pinmux configs are usually (always?) described in SoC datasheets, so I think it logically belongs to the SoC dtsi. thx, Jason.
Dear Jason Cooper, On Sat, 9 Aug 2014 11:57:56 -0400, Jason Cooper wrote: > > I personally disagree with this approach. armada-370.dtsi is here to > > describe what happens at the *SoC* level, not to factorize some random > > board specific details that appear to be common between a certain > > number of boards (but not all). > > It's my understanding that *possible* pinmux configurations are a > characteristic of the SoC. Which configurations are used is an artifact > of the board. > > I don't see the need to list all possible configurations in the SoC dtsi > file, but just the ones we have actually seen used. Absolutely. > > So I really, really, would prefer to keep the board-specific details > > such as which pin muxing is done for Ethernet in each individual .dts > > file. > > So, wouldn't this be the phandle selection? Yes. > > .dtsi to describe the SoC, .dts to describe the board. It's clear and > > simple for everyone to understand, especially for new comers. > > If you're strongly opposed to describing possible pinmux configurations > in the SoC dtsi file, how about an SoC-pinmux.dtsi? I just see the > "Thar be dragons ahead" sign by describing pinmux in the board files. Not at all. Pinmux configurations in the .dtsi files are perfect, as long as those pinmux configurations are related to "SoC" things, which the RGMII pinmux configurations are. However, Andrew was suggesting to move the phandle selection to the .dtsi file, with the argument that most boards use RGMII, so it should be the default. That's what I disagreed with. Best regards, Thomas
> However, Andrew was suggesting to move the phandle selection to > the .dtsi file, with the argument that most boards use RGMII, so it > should be the default. That's what I disagreed with. I'm not too bothered. So place it in the board .dts file. Andrew
diff --git a/arch/arm/boot/dts/armada-370-mirabox.dts b/arch/arm/boot/dts/armada-370-mirabox.dts index 097df7d8..c745256 100644 --- a/arch/arm/boot/dts/armada-370-mirabox.dts +++ b/arch/arm/boot/dts/armada-370-mirabox.dts @@ -91,6 +91,8 @@ }; mdio { + pinctrl-0 = <&mdio_pins>; + pinctrl-names = "default"; phy0: ethernet-phy@0 { reg = <0>; }; @@ -100,11 +102,15 @@ }; }; ethernet@70000 { + pinctrl-0 = <&ge0_pins>; + pinctrl-names = "default"; status = "okay"; phy = <&phy0>; phy-mode = "rgmii-id"; }; ethernet@74000 { + pinctrl-0 = <&ge1_pins>; + pinctrl-names = "default"; status = "okay"; phy = <&phy1>; phy-mode = "rgmii-id";
This commit adds the required pin muxing for the network interfaces and the MDIO interface to be properly initialized. For instance, this makes it possible for a bootloader to initialize and access the network interfaces. Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com> --- arch/arm/boot/dts/armada-370-mirabox.dts | 6 ++++++ 1 file changed, 6 insertions(+)