diff mbox

[v5,10/10] arm: dts: genmai: Add ethernet pin group

Message ID 1493281194-5200-11-git-send-email-jacopo+renesas@jmondi.org (mailing list archive)
State Superseded
Delegated to: Simon Horman
Headers show

Commit Message

Jacopo Mondi April 27, 2017, 8:19 a.m. UTC
Add pin configuration subnode for ETHER ethernet controller.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm/boot/dts/r7s72100-genmai.dts | 41 +++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

Comments

Geert Uytterhoeven April 27, 2017, 9:56 a.m. UTC | #1
Hi Jacopo,

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:
> Add pin configuration subnode for ETHER ethernet controller.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/arch/arm/boot/dts/r7s72100-genmai.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai.dts

> @@ -77,6 +105,19 @@
>         status = "okay";
>  };
>
> +&ether {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&ether_pins>;
> +
> +       status = "okay";
> +
> +       renesas,no-ether-link;
> +       phy-handle = <&phy0>;
> +       phy0: ethernet-phy@0 {
> +               reg = <0>;

Shouldn't the interrupt (connected to P1_15) be described?

> +       };
> +};

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Chris Brandt April 27, 2017, 10:48 a.m. UTC | #2
Hi Geert,

On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
> > +&ether {

> > +       pinctrl-names = "default";

> > +       pinctrl-0 = <&ether_pins>;

> > +

> > +       status = "okay";

> > +

> > +       renesas,no-ether-link;

> > +       phy-handle = <&phy0>;

> > +       phy0: ethernet-phy@0 {

> > +               reg = <0>;

> 

> Shouldn't the interrupt (connected to P1_15) be described?



That interrupt pin from the PHY is not used. It did not need to be connected.


Chris
Simon Horman April 28, 2017, 5:22 a.m. UTC | #3
On Thu, Apr 27, 2017 at 10:48:45AM +0000, Chris Brandt wrote:
> Hi Geert,
> 
> On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
> > > +&ether {
> > > +       pinctrl-names = "default";
> > > +       pinctrl-0 = <&ether_pins>;
> > > +
> > > +       status = "okay";
> > > +
> > > +       renesas,no-ether-link;
> > > +       phy-handle = <&phy0>;
> > > +       phy0: ethernet-phy@0 {
> > > +               reg = <0>;
> > 
> > Shouldn't the interrupt (connected to P1_15) be described?
> 
> 
> That interrupt pin from the PHY is not used. It did not need to be connected.

So things are fine as above or should I expect to see v6?
Geert Uytterhoeven April 28, 2017, 7:18 a.m. UTC | #4
Hi Chris,

On Thu, Apr 27, 2017 at 12:48 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
>> > +&ether {
>> > +       pinctrl-names = "default";
>> > +       pinctrl-0 = <&ether_pins>;
>> > +
>> > +       status = "okay";
>> > +
>> > +       renesas,no-ether-link;
>> > +       phy-handle = <&phy0>;
>> > +       phy0: ethernet-phy@0 {
>> > +               reg = <0>;
>>
>> Shouldn't the interrupt (connected to P1_15) be described?
>
> That interrupt pin from the PHY is not used. It did not need to be connected.

But it is connected, according to the schematics.

DT describes hardware, not software limitations.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Linus Walleij April 28, 2017, 8:49 a.m. UTC | #5
On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:

> Add pin configuration subnode for ETHER ethernet controller.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
(...)
> +               pins_bidir {
> +                       pinmux = <RZA1_PINMUX(3, 3, 2)>;/* P3_3 = ET_MDIO  */
> +                       bi-directional;
> +               };

So I'm against merging this until someone explains what "bi-directional"
actually means, electrically speaking. What happens physically on this pin?

I think this just means open drain.

It is dangerous to merge things we don't understand.

Surely someone inside Renesas can answer this question.

Yours,
Linus Walleij
Chris Brandt April 28, 2017, 1:50 p.m. UTC | #6
On Friday, April 28, 2017, Linus Walleij wrote:
> > Add pin configuration subnode for ETHER ethernet controller.

> >

> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> (...)

> > +               pins_bidir {

> > +                       pinmux = <RZA1_PINMUX(3, 3, 2)>;/* P3_3 =

> ET_MDIO  */

> > +                       bi-directional;

> > +               };

> 

> So I'm against merging this until someone explains what "bi-directional"

> actually means, electrically speaking. What happens physically on this

> pin?

> 

> I think this just means open drain.

> 

> It is dangerous to merge things we don't understand.

> 

> Surely someone inside Renesas can answer this question.


I don't think this has anything to do with open drain because you need it for any pin that the peripheral IP block needs to transmit and receive over the same line, regardless of if it's a SDHI, I2C, Ethernet MDIO, etc...
It's more about of allowing the internal IP block signals to get hooked up to the IO pad signals.

Chris
Chris Brandt April 28, 2017, 2:48 p.m. UTC | #7
Hi Geert,

On Friday, April 28, 2017, Geert Uytterhoeven wrote:
> >> Shouldn't the interrupt (connected to P1_15) be described?

> >

> > That interrupt pin from the PHY is not used. It did not need to be

> connected.

> 

> But it is connected, according to the schematics.


P1_15 can be configured as:
 * GPIO_IN
 * AN7
 * AVB_CAPTURE

So...I guess you would 'describe' it as an GPIO-input.


> DT describes hardware, not software limitations.


Describing things is fine, but the kernel code takes the DT and then start configuring things based on it.

For example, on the RSK board, that line is connected to P4_14. P4_14 can be configured as IRQ6...but IRQ6 comes out in 8 different pin choices, and I might want to use one of those other choices, so I don't want to describe P4_14 as an interrupt.

If it was just describing that "pin 15 of the PHY chip is tied to pin Y19 of the RZ/A1H", that's fine because it's hardware connection that's not going to change.
But...the DT also defines the pin muxing...which is a software decision (do I want to get interrupt or just manually poll or simple ignore it).
This is the part of the whole "DT is for hardware description only" that doesn't really make sense to me.


Chris
Linus Walleij May 5, 2017, 12:06 p.m. UTC | #8
On Fri, Apr 28, 2017 at 4:48 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:

I think this was written by me, not Geert...

>> DT describes hardware, not software limitations.
>
> Describing things is fine, but the kernel code takes the DT and then start configuring things based on it.
>
> For example, on the RSK board, that line is connected to P4_14. P4_14 can be configured as IRQ6...but IRQ6 comes out in 8 different pin choices, and I might want to use one of those other choices, so I don't want to describe P4_14 as an interrupt.
>
> If it was just describing that "pin 15 of the PHY chip is tied to pin Y19 of the RZ/A1H", that's fine because it's hardware connection that's not going to change.
> But...the DT also defines the pin muxing...which is a software decision (do I want to get interrupt or just manually poll or simple ignore it).
> This is the part of the whole "DT is for hardware description only" that doesn't really make sense to me.

OK yeah we do hardware description AND configuration.

And we never do interpreted languages.

And then there is a bunch of grayzone things. For example we have
a linux,input binding for connecting keypresses to certain Linux input
codes. That is really grayzone, but very useful.

Yours,
Linus Walleij
Geert Uytterhoeven May 5, 2017, 12:20 p.m. UTC | #9
Hi Linus,

On Fri, May 5, 2017 at 2:06 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Apr 28, 2017 at 4:48 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
>
> I think this was written by me, not Geert...
>
>>> DT describes hardware, not software limitations.

Na, I did write it.

I can understand you sometimes mix up things, esp. when you're
enjoying LLC ;-)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Chris Brandt May 5, 2017, 12:45 p.m. UTC | #10
On Friday, May 05, 2017, Linus Walleij wrote:
> > This is the part of the whole "DT is for hardware description only" that

> doesn't really make sense to me.

> 

> OK yeah we do hardware description AND configuration.

> 

> And we never do interpreted languages.

> 

> And then there is a bunch of grayzone things. For example we have a

> linux,input binding for connecting keypresses to certain Linux input codes.

> That is really grayzone, but very useful.


Ah....

	compatible = "linux,grayzone";


Thanks for the reply. I'll stop ranting now.
Of course, I'll still probably screw it up again on a future patch somehow...

Cheers

Chris
Linus Walleij May 11, 2017, 1:48 p.m. UTC | #11
On Fri, May 5, 2017 at 2:45 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, May 05, 2017, Linus Walleij wrote:
>> > This is the part of the whole "DT is for hardware description only" that
>> doesn't really make sense to me.
>>
>> OK yeah we do hardware description AND configuration.
>>
>> And we never do interpreted languages.
>>
>> And then there is a bunch of grayzone things. For example we have a
>> linux,input binding for connecting keypresses to certain Linux input codes.
>> That is really grayzone, but very useful.
>
> Ah....
>
>         compatible = "linux,grayzone";
>
> Thanks for the reply. I'll stop ranting now.
> Of course, I'll still probably screw it up again on a future patch somehow...

For this driver I remarked in some other thread that we may
end up with some

"renesas,bidirectional"

or so. But yeah, it's kinda grayzone too.

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index f7c512e..328f4c9 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -63,6 +63,34 @@ 
 		pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
 		bi-directional;
 	};
+
+	ether_pins: ether {
+		pins {
+			/* Ethernet on Ports 1,2,3,5 */
+			pinmux = <RZA1_PINMUX(1, 14, 4)>,/* P1_14 = ET_COL  */
+				 <RZA1_PINMUX(5, 9, 2)>, /* P5_9 = ET_MDC   */
+				 <RZA1_PINMUX(3, 4, 2)>, /* P3_4 = ET_RXCLK */
+				 <RZA1_PINMUX(3, 5, 2)>, /* P3_5 = ET_RXER  */
+				 <RZA1_PINMUX(3, 6, 2)>, /* P3_6 = ET_RXDV  */
+				 <RZA1_PINMUX(2, 0, 2)>, /* P2_0 = ET_TXCLK */
+				 <RZA1_PINMUX(2, 1, 2)>, /* P2_1 = ET_TXER  */
+				 <RZA1_PINMUX(2, 2, 2)>, /* P2_2 = ET_TXEN  */
+				 <RZA1_PINMUX(2, 3, 2)>, /* P2_3 = ET_CRS   */
+				 <RZA1_PINMUX(2, 4, 2)>, /* P2_4 = ET_TXD0  */
+				 <RZA1_PINMUX(2, 5, 2)>, /* P2_5 = ET_TXD1  */
+				 <RZA1_PINMUX(2, 6, 2)>, /* P2_6 = ET_TXD2  */
+				 <RZA1_PINMUX(2, 7, 2)>, /* P2_7 = ET_TXD3  */
+				 <RZA1_PINMUX(2, 8, 2)>, /* P2_8 = ET_RXD0  */
+				 <RZA1_PINMUX(2, 9, 2)>, /* P2_9 = ET_RXD1  */
+				 <RZA1_PINMUX(2, 10, 2)>,/* P2_10 = ET_RXD2 */
+				 <RZA1_PINMUX(2, 11, 2)>;/* P2_11 = ET_RXD3 */
+		};
+
+		pins_bidir {
+			pinmux = <RZA1_PINMUX(3, 3, 2)>;/* P3_3 = ET_MDIO  */
+			bi-directional;
+		};
+	};
 };
 
 &extal_clk {
@@ -77,6 +105,19 @@ 
 	status = "okay";
 };
 
+&ether {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ether_pins>;
+
+	status = "okay";
+
+	renesas,no-ether-link;
+	phy-handle = <&phy0>;
+	phy0: ethernet-phy@0 {
+		reg = <0>;
+	};
+};
+
 &i2c2 {
 	status = "okay";
 	clock-frequency = <400000>;