| Message ID | 20180116003443.GA10587@ingrassia.epigenesys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Emiliano, On Tue, Jan 16, 2018 at 1:34 AM, Emiliano Ingrassia <ingrassia@epigenesys.com> wrote: > The Odroid-C1+ board is equipped with an RTL8211F ethernet PHY > which supports 10/100/1000 Mbps ethernet. > This patch adds the PHY description to the board DT, > setting the maximum speed to 1 Gbps, and enables the ethernet controller. > > Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com> I have two minor change-requests - with these you can add my: Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > --- > arch/arm/boot/dts/meson8b-odroidc1.dts | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts > index 9ff6ca4e20d0..3766f6fd1882 100644 > --- a/arch/arm/boot/dts/meson8b-odroidc1.dts > +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts > @@ -99,3 +99,35 @@ > &usb1 { > status = "okay"; > }; > + > +ðmac { > + status = "okay"; > + > + snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_HIGH>; > + snps,reset-active-low; > + snps,reset-delays-us = <0 10000 30000>; > + > + pinctrl-0 = <ð_rgmii_pins>; > + pinctrl-names = "default"; > + > + phy-mode = "rgmii"; > + phy-handle = <ð_phy>; > + amlogic,tx-delay-ns = <4>; > + > + mdio { > + compatible = "snps,dwmac-mdio"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + eth_phy: ethernet-phy@0 { > + /* ethernet PHY RTL8211F */ > + compatible = "ethernet-phy-id001c.c916", > + "ethernet-phy-ieee802.3-c22"; could you please remove the "ethernet-phy-id001c.c916" compatible? the Ethernet PHY documentation [0] states: 'If the PHY reports an incorrect ID (or none at all) then the "compatible" list may contain an entry with the correct PHY ID' I have not seen any Amlogic board where the RTL8211F PHY reported an incorrect ID yet On Odroid-C2 we simply add the ID to the comment above: /* Realtek RTL8211F (0x001cc916) */ the "ethernet-phy-ieee802.3-c22" compatible is fine - but I'm also fine if you remove the whole compatible property (like we have it on Odroid-C2) > + reg = <0>; > + max-speed = <1000>; > + eee-broken-1000t; > + interrupt-parent = <&gpio_intc>; could you please add a comment here (which will allow us to specify the IRQs by their GPIO #define later on, without having to calculate what "17" means in the context of Meson8b, as this calculation depends on the SoC) - for example: /* GPIOH_3 */ > + interrupts = <17 IRQ_TYPE_LEVEL_LOW>; > + }; > + }; > +}; > -- > 2.15.1 > many thanks for your work! Regards, Martin [0] https://elixir.free-electrons.com/linux/v4.14/source/Documentation/devicetree/bindings/net/phy.txt#L12
On Tue, 2018-01-16 at 01:34 +0100, Emiliano Ingrassia wrote: > The Odroid-C1+ board is equipped with an RTL8211F ethernet PHY > which supports 10/100/1000 Mbps ethernet. > This patch adds the PHY description to the board DT, > setting the maximum speed to 1 Gbps, and enables the ethernet controller. > > Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com> > --- > arch/arm/boot/dts/meson8b-odroidc1.dts | 32 ++++++++++++++++++++++++++++++++ > 1 file changed, 32 insertions(+) > > diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts > index 9ff6ca4e20d0..3766f6fd1882 100644 > --- a/arch/arm/boot/dts/meson8b-odroidc1.dts > +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts > @@ -99,3 +99,35 @@ > &usb1 { > status = "okay"; > }; > + > +ðmac { > + status = "okay"; > + > + snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_HIGH>; > + snps,reset-active-low; > + snps,reset-delays-us = <0 10000 30000>; > + > + pinctrl-0 = <ð_rgmii_pins>; > + pinctrl-names = "default"; > + > + phy-mode = "rgmii"; > + phy-handle = <ð_phy>; > + amlogic,tx-delay-ns = <4>; > + > + mdio { > + compatible = "snps,dwmac-mdio"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + eth_phy: ethernet-phy@0 { > + /* ethernet PHY RTL8211F */ > + compatible = "ethernet-phy-id001c.c916", > + "ethernet-phy-ieee802.3-c22"; > + reg = <0>; > + max-speed = <1000>; > + eee-broken-1000t; You should test against the issue before adding this flags. Did you have any hang issue while doing iperf test at Gbit speed ? It happens quickly if you are impacted as the c2 is. > + interrupt-parent = <&gpio_intc>; > + interrupts = <17 IRQ_TYPE_LEVEL_LOW>; > + }; > + }; > +};
Hi Martin, thanks for the review. On Tue, Jan 16, 2018 at 11:27:07AM +0100, Martin Blumenstingl wrote: > Hi Emiliano, > > On Tue, Jan 16, 2018 at 1:34 AM, Emiliano Ingrassia > <ingrassia@epigenesys.com> wrote: > > The Odroid-C1+ board is equipped with an RTL8211F ethernet PHY > > which supports 10/100/1000 Mbps ethernet. > > This patch adds the PHY description to the board DT, > > setting the maximum speed to 1 Gbps, and enables the ethernet controller. > > > > Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com> > I have two minor change-requests - with these you can add my: > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > --- > > arch/arm/boot/dts/meson8b-odroidc1.dts | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts > > index 9ff6ca4e20d0..3766f6fd1882 100644 > > --- a/arch/arm/boot/dts/meson8b-odroidc1.dts > > +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts > > @@ -99,3 +99,35 @@ > > &usb1 { > > status = "okay"; > > }; > > + > > +ðmac { > > + status = "okay"; > > + > > + snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_HIGH>; > > + snps,reset-active-low; > > + snps,reset-delays-us = <0 10000 30000>; > > + > > + pinctrl-0 = <ð_rgmii_pins>; > > + pinctrl-names = "default"; > > + > > + phy-mode = "rgmii"; > > + phy-handle = <ð_phy>; > > + amlogic,tx-delay-ns = <4>; > > + > > + mdio { > > + compatible = "snps,dwmac-mdio"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + eth_phy: ethernet-phy@0 { > > + /* ethernet PHY RTL8211F */ > > + compatible = "ethernet-phy-id001c.c916", > > + "ethernet-phy-ieee802.3-c22"; > could you please remove the "ethernet-phy-id001c.c916" compatible? > the Ethernet PHY documentation [0] states: 'If the PHY reports an > incorrect ID (or none at all) then the "compatible" list may contain > an entry with the correct PHY ID' > I have not seen any Amlogic board where the RTL8211F PHY reported an > incorrect ID yet OK, you're correct! I'll test without this and submit a second version. > > On Odroid-C2 we simply add the ID to the comment above: > /* Realtek RTL8211F (0x001cc916) */ > Ok. > the "ethernet-phy-ieee802.3-c22" compatible is fine - but I'm also > fine if you remove the whole compatible property (like we have it on > Odroid-C2) Ok, that is also the default so no need to indicate explicitly. > > > + reg = <0>; > > + max-speed = <1000>; > > + eee-broken-1000t; > > + interrupt-parent = <&gpio_intc>; > could you please add a comment here (which will allow us to specify > the IRQs by their GPIO #define later on, without having to calculate > what "17" means in the context of Meson8b, as this calculation depends > on the SoC) - for example: > /* GPIOH_3 */ > > + interrupts = <17 IRQ_TYPE_LEVEL_LOW>; > > + }; > > + }; > > +}; > > -- > > 2.15.1 > > > Ok, sure! > many thanks for your work! > Thanks for your work and support! > Regards, > Martin > > > [0] https://elixir.free-electrons.com/linux/v4.14/source/Documentation/devicetree/bindings/net/phy.txt#L12 Regards, Emiliano
Hi Jerome, thanks for the review. On Tue, Jan 16, 2018 at 11:31:55AM +0100, Jerome Brunet wrote: > On Tue, 2018-01-16 at 01:34 +0100, Emiliano Ingrassia wrote: > > The Odroid-C1+ board is equipped with an RTL8211F ethernet PHY > > which supports 10/100/1000 Mbps ethernet. > > This patch adds the PHY description to the board DT, > > setting the maximum speed to 1 Gbps, and enables the ethernet controller. > > > > Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com> > > --- > > arch/arm/boot/dts/meson8b-odroidc1.dts | 32 ++++++++++++++++++++++++++++++++ > > 1 file changed, 32 insertions(+) > > > > diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts > > index 9ff6ca4e20d0..3766f6fd1882 100644 > > --- a/arch/arm/boot/dts/meson8b-odroidc1.dts > > +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts > > @@ -99,3 +99,35 @@ > > &usb1 { > > status = "okay"; > > }; > > + > > +ðmac { > > + status = "okay"; > > + > > + snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_HIGH>; > > + snps,reset-active-low; > > + snps,reset-delays-us = <0 10000 30000>; > > + > > + pinctrl-0 = <ð_rgmii_pins>; > > + pinctrl-names = "default"; > > + > > + phy-mode = "rgmii"; > > + phy-handle = <ð_phy>; > > + amlogic,tx-delay-ns = <4>; > > + > > + mdio { > > + compatible = "snps,dwmac-mdio"; > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + eth_phy: ethernet-phy@0 { > > + /* ethernet PHY RTL8211F */ > > + compatible = "ethernet-phy-id001c.c916", > > + "ethernet-phy-ieee802.3-c22"; > > + reg = <0>; > > + max-speed = <1000>; > > + eee-broken-1000t; > > You should test against the issue before adding this flags. > Did you have any hang issue while doing iperf test at Gbit speed ? It happens > quickly if you are impacted as the c2 is. > Of course I tested for this issue. I experiment a high packet loss while pinging the board without it. Should I report this in message log? > > + interrupt-parent = <&gpio_intc>; > > + interrupts = <17 IRQ_TYPE_LEVEL_LOW>; > > + }; > > + }; > > +}; >
On Wed, 2018-01-17 at 15:39 +0100, Emiliano Ingrassia wrote: > Hi Jerome, > > thanks for the review. > > On Tue, Jan 16, 2018 at 11:31:55AM +0100, Jerome Brunet wrote: > > On Tue, 2018-01-16 at 01:34 +0100, Emiliano Ingrassia wrote: > > > The Odroid-C1+ board is equipped with an RTL8211F ethernet PHY > > > which supports 10/100/1000 Mbps ethernet. > > > This patch adds the PHY description to the board DT, > > > setting the maximum speed to 1 Gbps, and enables the ethernet controller. > > > > > > Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com> > > > --- > > > arch/arm/boot/dts/meson8b-odroidc1.dts | 32 ++++++++++++++++++++++++++++++++ > > > 1 file changed, 32 insertions(+) > > > > > > diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts > > > index 9ff6ca4e20d0..3766f6fd1882 100644 > > > --- a/arch/arm/boot/dts/meson8b-odroidc1.dts > > > +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts > > > @@ -99,3 +99,35 @@ > > > &usb1 { > > > status = "okay"; > > > }; > > > + > > > +ðmac { > > > + status = "okay"; > > > + > > > + snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_HIGH>; > > > + snps,reset-active-low; > > > + snps,reset-delays-us = <0 10000 30000>; > > > + > > > + pinctrl-0 = <ð_rgmii_pins>; > > > + pinctrl-names = "default"; > > > + > > > + phy-mode = "rgmii"; > > > + phy-handle = <ð_phy>; > > > + amlogic,tx-delay-ns = <4>; > > > + > > > + mdio { > > > + compatible = "snps,dwmac-mdio"; > > > + #address-cells = <1>; > > > + #size-cells = <0>; > > > + > > > + eth_phy: ethernet-phy@0 { > > > + /* ethernet PHY RTL8211F */ > > > + compatible = "ethernet-phy-id001c.c916", > > > + "ethernet-phy-ieee802.3-c22"; > > > + reg = <0>; > > > + max-speed = <1000>; Do we really this property ? I might be mistaken, but I think it is only useful we you to restrict the available speed. It does not look like it here. > > > + eee-broken-1000t; > > > > You should test against the issue before adding this flags. > > Did you have any hang issue while doing iperf test at Gbit speed ? It happens > > quickly if you are impacted as the c2 is. > > > > Of course I tested for this issue. I experiment a high packet loss > while pinging the board without it. > Should I report this in message log? I wouldn't have asked if you did, so yes. Thx > > > > + interrupt-parent = <&gpio_intc>; > > > + interrupts = <17 IRQ_TYPE_LEVEL_LOW>; > > > + }; > > > + }; > > > +};
On Wed, Jan 17, 2018 at 03:42:48PM +0100, Jerome Brunet wrote: > On Wed, 2018-01-17 at 15:39 +0100, Emiliano Ingrassia wrote: > > Hi Jerome, > > > > thanks for the review. > > > > On Tue, Jan 16, 2018 at 11:31:55AM +0100, Jerome Brunet wrote: > > > On Tue, 2018-01-16 at 01:34 +0100, Emiliano Ingrassia wrote: > > > > The Odroid-C1+ board is equipped with an RTL8211F ethernet PHY > > > > which supports 10/100/1000 Mbps ethernet. > > > > This patch adds the PHY description to the board DT, > > > > setting the maximum speed to 1 Gbps, and enables the ethernet controller. > > > > > > > > Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com> > > > > --- > > > > arch/arm/boot/dts/meson8b-odroidc1.dts | 32 ++++++++++++++++++++++++++++++++ > > > > 1 file changed, 32 insertions(+) > > > > > > > > diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts > > > > index 9ff6ca4e20d0..3766f6fd1882 100644 > > > > --- a/arch/arm/boot/dts/meson8b-odroidc1.dts > > > > +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts > > > > @@ -99,3 +99,35 @@ > > > > &usb1 { > > > > status = "okay"; > > > > }; > > > > + > > > > +ðmac { > > > > + status = "okay"; > > > > + > > > > + snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_HIGH>; > > > > + snps,reset-active-low; > > > > + snps,reset-delays-us = <0 10000 30000>; > > > > + > > > > + pinctrl-0 = <ð_rgmii_pins>; > > > > + pinctrl-names = "default"; > > > > + > > > > + phy-mode = "rgmii"; > > > > + phy-handle = <ð_phy>; > > > > + amlogic,tx-delay-ns = <4>; > > > > + > > > > + mdio { > > > > + compatible = "snps,dwmac-mdio"; > > > > + #address-cells = <1>; > > > > + #size-cells = <0>; > > > > + > > > > + eth_phy: ethernet-phy@0 { > > > > + /* ethernet PHY RTL8211F */ > > > > + compatible = "ethernet-phy-id001c.c916", > > > > + "ethernet-phy-ieee802.3-c22"; > > > > + reg = <0>; > > > > + max-speed = <1000>; > > Do we really this property ? I might be mistaken, but I think it is only useful > we you to restrict the available speed. It does not look like it here. > Ok, the realtek driver has this information in per phy model structures so no need to specify it. > > > > + eee-broken-1000t; > > > > > > You should test against the issue before adding this flags. > > > Did you have any hang issue while doing iperf test at Gbit speed ? It happens > > > quickly if you are impacted as the c2 is. > > > > > > > Of course I tested for this issue. I experiment a high packet loss > > while pinging the board without it. > > Should I report this in message log? > > I wouldn't have asked if you did, so yes. > Thx > > > > > > > + interrupt-parent = <&gpio_intc>; > > > > + interrupts = <17 IRQ_TYPE_LEVEL_LOW>; > > > > + }; > > > > + }; > > > > +}; > Thanks, Emiliano
diff --git a/arch/arm/boot/dts/meson8b-odroidc1.dts b/arch/arm/boot/dts/meson8b-odroidc1.dts index 9ff6ca4e20d0..3766f6fd1882 100644 --- a/arch/arm/boot/dts/meson8b-odroidc1.dts +++ b/arch/arm/boot/dts/meson8b-odroidc1.dts @@ -99,3 +99,35 @@ &usb1 { status = "okay"; }; + +ðmac { + status = "okay"; + + snps,reset-gpio = <&gpio GPIOH_4 GPIO_ACTIVE_HIGH>; + snps,reset-active-low; + snps,reset-delays-us = <0 10000 30000>; + + pinctrl-0 = <ð_rgmii_pins>; + pinctrl-names = "default"; + + phy-mode = "rgmii"; + phy-handle = <ð_phy>; + amlogic,tx-delay-ns = <4>; + + mdio { + compatible = "snps,dwmac-mdio"; + #address-cells = <1>; + #size-cells = <0>; + + eth_phy: ethernet-phy@0 { + /* ethernet PHY RTL8211F */ + compatible = "ethernet-phy-id001c.c916", + "ethernet-phy-ieee802.3-c22"; + reg = <0>; + max-speed = <1000>; + eee-broken-1000t; + interrupt-parent = <&gpio_intc>; + interrupts = <17 IRQ_TYPE_LEVEL_LOW>; + }; + }; +};
The Odroid-C1+ board is equipped with an RTL8211F ethernet PHY which supports 10/100/1000 Mbps ethernet. This patch adds the PHY description to the board DT, setting the maximum speed to 1 Gbps, and enables the ethernet controller. Signed-off-by: Emiliano Ingrassia <ingrassia@epigenesys.com> --- arch/arm/boot/dts/meson8b-odroidc1.dts | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)