Re: [PATCH 3/3] ARM: dts: gr-peach: Add ETHER pin group

Re: [PATCH 3/3] ARM: dts: gr-peach: Add ETHER pin group

[jacopo mondi]

Hi Geert

On Thu, Oct 05, 2017 at 11:09:40AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Oct 5, 2017 at 10:58 AM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > Add pin configuration subnode for ETHER pin group and enable the interface.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> > --- a/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > +++ b/arch/arm/boot/dts/r7s72100-gr-peach.dts
>
> > @@ -88,3 +110,19 @@
> >
> >         status = "okay";
> >  };
> > +
> > +&ether {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&ether_pins>;
> > +
> > +       status = "okay";
> > +
> > +       reset-gpios = <&port4 2 GPIO_ACTIVE_LOW>;
> > +       reset-delay-us = <5>;
>
> I'm afraid the PHY people (not CCed ;-) will want you to move these reset
> properties to the phy subnode these days, despite
> Documentation/devicetree/bindings/net/mdio.txt...

Extending to:
    +andrew@lunn.ch
    +f.fainelli@gmail.com
    +netdev@vger.kernel.org

To hear from them when and how they like to move those properties and
if we can apply this in this shape or not.

Otherwise, if you think it's the case, I can move them nonetheless.

Thanks
   j
>
> > +
> > +       renesas,no-ether-link;
> > +       phy-handle = <&phy0>;
> > +       phy0: ethernet-phy@0 {
> > +               reg = <0>;
> > +       };
> > +};
>
> 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

Re: [PATCH 3/3] ARM: dts: gr-peach: Add ETHER pin group

[Andrew Lunn]

On Thu, Oct 05, 2017 at 11:39:15AM +0200, jacopo mondi wrote:
> Hi Geert
> 
> On Thu, Oct 05, 2017 at 11:09:40AM +0200, Geert Uytterhoeven wrote:
> > Hi Jacopo,
> >
> > On Thu, Oct 5, 2017 at 10:58 AM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > > Add pin configuration subnode for ETHER pin group and enable the interface.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > > --- a/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > > +++ b/arch/arm/boot/dts/r7s72100-gr-peach.dts
> >
> > > @@ -88,3 +110,19 @@
> > >
> > >         status = "okay";
> > >  };
> > > +
> > > +&ether {
> > > +       pinctrl-names = "default";
> > > +       pinctrl-0 = <&ether_pins>;
> > > +
> > > +       status = "okay";
> > > +
> > > +       reset-gpios = <&port4 2 GPIO_ACTIVE_LOW>;
> > > +       reset-delay-us = <5>;
> >
> > I'm afraid the PHY people (not CCed ;-) will want you to move these reset
> > properties to the phy subnode these days, despite
> > Documentation/devicetree/bindings/net/mdio.txt...

Hi Jocopo

So what is this reset resetting?

The MAC?
The PHY?

    Andrew

Re: [PATCH 3/3] ARM: dts: gr-peach: Add ETHER pin group

[jacopo mondi]

Hi Andrew,

On Thu, Oct 05, 2017 at 03:43:39PM +0200, Andrew Lunn wrote:
> On Thu, Oct 05, 2017 at 11:39:15AM +0200, jacopo mondi wrote:
> > Hi Geert
> >
> > On Thu, Oct 05, 2017 at 11:09:40AM +0200, Geert Uytterhoeven wrote:
> > > Hi Jacopo,
> > >
> > > On Thu, Oct 5, 2017 at 10:58 AM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > > > Add pin configuration subnode for ETHER pin group and enable the interface.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > >
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >
> > > > --- a/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > > > +++ b/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > >
> > > > @@ -88,3 +110,19 @@
> > > >
> > > >         status = "okay";
> > > >  };
> > > > +
> > > > +&ether {
> > > > +       pinctrl-names = "default";
> > > > +       pinctrl-0 = <&ether_pins>;
> > > > +
> > > > +       status = "okay";
> > > > +
> > > > +       reset-gpios = <&port4 2 GPIO_ACTIVE_LOW>;
> > > > +       reset-delay-us = <5>;
> > >
> > > I'm afraid the PHY people (not CCed ;-) will want you to move these reset
> > > properties to the phy subnode these days, despite
> > > Documentation/devicetree/bindings/net/mdio.txt...
>
> Hi Jocopo
>
> So what is this reset resetting?
>
> The MAC?
> The PHY?

The reset line goes from our SoC to LAN8710A PHY chip external reset pin.

Thanks
   j

>
>     Andrew

Re: [PATCH 3/3] ARM: dts: gr-peach: Add ETHER pin group

[Andrew Lunn]

On Thu, Oct 05, 2017 at 05:42:39PM +0200, jacopo mondi wrote:
> Hi Andrew,
> 
> On Thu, Oct 05, 2017 at 03:43:39PM +0200, Andrew Lunn wrote:
> > On Thu, Oct 05, 2017 at 11:39:15AM +0200, jacopo mondi wrote:
> > > Hi Geert
> > >
> > > On Thu, Oct 05, 2017 at 11:09:40AM +0200, Geert Uytterhoeven wrote:
> > > > Hi Jacopo,
> > > >
> > > > On Thu, Oct 5, 2017 at 10:58 AM, Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > > > > Add pin configuration subnode for ETHER pin group and enable the interface.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > > >
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >
> > > > > --- a/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > > > > +++ b/arch/arm/boot/dts/r7s72100-gr-peach.dts
> > > >
> > > > > @@ -88,3 +110,19 @@
> > > > >
> > > > >         status = "okay";
> > > > >  };
> > > > > +
> > > > > +&ether {
> > > > > +       pinctrl-names = "default";
> > > > > +       pinctrl-0 = <&ether_pins>;
> > > > > +
> > > > > +       status = "okay";
> > > > > +
> > > > > +       reset-gpios = <&port4 2 GPIO_ACTIVE_LOW>;
> > > > > +       reset-delay-us = <5>;
> > > >
> > > > I'm afraid the PHY people (not CCed ;-) will want you to move these reset
> > > > properties to the phy subnode these days, despite
> > > > Documentation/devicetree/bindings/net/mdio.txt...
> >
> > Hi Jocopo
> >
> > So what is this reset resetting?
> >
> > The MAC?
> > The PHY?
> 
> The reset line goes from our SoC to LAN8710A PHY chip external reset pin.

So yes, this is a PHY property, and should be in the PHY node.

Documentation/devicetree/bindings/net/mdio.txt does not apply here
anyway. That is for an MDIO binding. This node is an ethernet MAC.

So your binding whats to look something like

        ether: ethernet@e8203000 {
                compatible = "renesas,ether-r7s72100";
                reg = <0xe8203000 0x800>,
                      <0xe8204800 0x200>;
                interrupts = <GIC_SPI 327 IRQ_TYPE_LEVEL_HIGH>;
                clocks = <&mstp7_clks R7S72100_CLK_ETHER>;
                power-domains = <&cpg_clocks>;
                phy-mode = "mii";
		phy-handle = <&phy0>;
                #address-cells = <1>;
                #size-cells = <0>;

                mdio: bus-bus {
                      #address-cells = <1>;
                      #size-cells = <0>;

                      phy0: ethernet-phy@1 {
                            reg = <1>;
                            reset-gpios = <&port4 2 GPIO_ACTIVE_LOW>;
                            reset-delay-us = <5>;
                      };
                };
        };

	Andrew

Re: [PATCH 3/3] ARM: dts: gr-peach: Add ETHER pin group

[jacopo mondi]

Hi Andrew,
   thanks for the suggestion

On Thu, Oct 05, 2017 at 06:48:26PM +0200, Andrew Lunn wrote:
> On Thu, Oct 05, 2017 at 05:42:39PM +0200, jacopo mondi wrote:
> > Hi Andrew,

[snip]

> > > Hi Jocopo
> > >
> > > So what is this reset resetting?
> > >
> > > The MAC?
> > > The PHY?
> >
> > The reset line goes from our SoC to LAN8710A PHY chip external reset pin.
>
> So yes, this is a PHY property, and should be in the PHY node.
>
> Documentation/devicetree/bindings/net/mdio.txt does not apply here
> anyway. That is for an MDIO binding. This node is an ethernet MAC.
>
> So your binding whats to look something like
>
>         ether: ethernet@e8203000 {
>                 compatible = "renesas,ether-r7s72100";
>                 reg = <0xe8203000 0x800>,
>                       <0xe8204800 0x200>;
>                 interrupts = <GIC_SPI 327 IRQ_TYPE_LEVEL_HIGH>;
>                 clocks = <&mstp7_clks R7S72100_CLK_ETHER>;
>                 power-domains = <&cpg_clocks>;
>                 phy-mode = "mii";
> 		          phy-handle = <&phy0>;
>                 #address-cells = <1>;
>                 #size-cells = <0>;
>
>                 mdio: bus-bus {
>                       #address-cells = <1>;
>                       #size-cells = <0>;
>
>                       phy0: ethernet-phy@1 {
>                             reg = <1>;

Why reg = <1> ?
Shouldn't this be 0, or even better with no reg property at all?

        mdio: bus-bus {
              phy-0 {
                  reset-gpios = <&port4 2 GPIO_ACTIVE_LOW>;
                  reset-delay-us = <5>;
              };
        };

Thanks
   j

>                             reset-gpios = <&port4 2 GPIO_ACTIVE_LOW>;
>                             reset-delay-us = <5>;
>                       };
>                 };
>         };
>
> 	Andrew

Re: [PATCH 3/3] ARM: dts: gr-peach: Add ETHER pin group

[Andrew Lunn]

> > So your binding whats to look something like
> >
> >         ether: ethernet@e8203000 {
> >                 compatible = "renesas,ether-r7s72100";
> >                 reg = <0xe8203000 0x800>,
> >                       <0xe8204800 0x200>;
> >                 interrupts = <GIC_SPI 327 IRQ_TYPE_LEVEL_HIGH>;
> >                 clocks = <&mstp7_clks R7S72100_CLK_ETHER>;
> >                 power-domains = <&cpg_clocks>;
> >                 phy-mode = "mii";
> > 		          phy-handle = <&phy0>;
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >
> >                 mdio: bus-bus {
> >                       #address-cells = <1>;
> >                       #size-cells = <0>;
> >
> >                       phy0: ethernet-phy@1 {
> >                             reg = <1>;
> 
> Why reg = <1> ?
> Shouldn't this be 0, or even better with no reg property at all?

This is the address of the PHY on the MDIO bus. There can be up to 32
devices on the bus. I have no idea what address your PHY is using, so
i just picked a value. 0 can be special, so i avoided it.

	Andrew