| Message ID | 1403777615-25685-2-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Boris, 2014-06-26 3:13 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: > Add ethernet-phy node and specify phy interrupt (connected to pin PB25). > > The PHY address is not specified here because atmel have 2 different > designs > for its CPU modules: one is connecting PHYAD[0-2] pins to pull up resistors > (Embest design) and the other one is connection PHYAD0 to a pull up > resistor and PHYAD[1-2] to pull down resistors (Ronetix design). > As a result, Ronetix design will have its PHY available at address 0x1 and > Embest design at 0x7. > Let the net PHY core automatically detect the PHY address by scanning the > MDIO bus. I though the compatible string was listed as a required property, but it is not. The 'reg' property however is listed as required, although the of_miodbus_register() works just fine without it, although that is a Linux-specific implementation detail. > > Define board specific delays to apply to RGMII signals. > > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- > arch/arm/boot/dts/sama5d3xcm.dtsi | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/arch/arm/boot/dts/sama5d3xcm.dtsi b/arch/arm/boot/dts/sama5d3xcm.dtsi > index b0b1331..fc68bae 100644 > --- a/arch/arm/boot/dts/sama5d3xcm.dtsi > +++ b/arch/arm/boot/dts/sama5d3xcm.dtsi > @@ -34,6 +34,21 @@ > > macb0: ethernet@f0028000 { > phy-mode = "rgmii"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + ethernet-phy { > + interrupt-parent = <&pioB>; > + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; > + txen-skew-ps = <800>; > + txc-skew-ps = <3000>; > + rxdv-skew-ps = <400>; > + rxc-skew-ps = <3000>; > + rxd0-skew-ps = <400>; > + rxd1-skew-ps = <400>; > + rxd2-skew-ps = <400>; > + rxd3-skew-ps = <400>; > + }; > }; > > pmc: pmc@fffffc00 { > -- > 1.8.3.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Florian, On 26/06/2014 20:15, Florian Fainelli wrote: > Hi Boris, > > 2014-06-26 3:13 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: >> Add ethernet-phy node and specify phy interrupt (connected to pin PB25). >> >> The PHY address is not specified here because atmel have 2 different >> designs >> for its CPU modules: one is connecting PHYAD[0-2] pins to pull up resistors >> (Embest design) and the other one is connection PHYAD0 to a pull up >> resistor and PHYAD[1-2] to pull down resistors (Ronetix design). >> As a result, Ronetix design will have its PHY available at address 0x1 and >> Embest design at 0x7. >> Let the net PHY core automatically detect the PHY address by scanning the >> MDIO bus. > I though the compatible string was listed as a required property, but > it is not. The 'reg' property however is listed as required, although > the of_miodbus_register() works just fine without it, although that is > a Linux-specific implementation detail. Indeed, it's listed in the required property list of the DT binding doc, but the code implement auto detection if reg is missing. However this line [1] clearly shows that specifying the reg property is the preferred way of doing things. I could define 2 different sama5d3xcm.dtsi (sama5d3xcm-ronetix.dtsi and sama5d3xcm-embest.dtsi) to avoid this dirty hack, but then we would have 2 more dtb and the user would have to determine which CPU module he owns to choose the appropriate dtb. If at91, arm-soc and DT maintainers agree with this approach I can definitely propose something. > >> Define board specific delays to apply to RGMII signals. >> >> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> Thanks for your review. Best Regards, Boris [1] http://lxr.free-electrons.com/source/drivers/of/of_mdio.c#L187
On 26/06/2014 22:01, Boris BREZILLON : > Hi Florian, > > On 26/06/2014 20:15, Florian Fainelli wrote: >> Hi Boris, >> >> 2014-06-26 3:13 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: >>> Add ethernet-phy node and specify phy interrupt (connected to pin PB25). >>> >>> The PHY address is not specified here because atmel have 2 different >>> designs >>> for its CPU modules: one is connecting PHYAD[0-2] pins to pull up resistors >>> (Embest design) and the other one is connection PHYAD0 to a pull up >>> resistor and PHYAD[1-2] to pull down resistors (Ronetix design). >>> As a result, Ronetix design will have its PHY available at address 0x1 and >>> Embest design at 0x7. >>> Let the net PHY core automatically detect the PHY address by scanning the >>> MDIO bus. >> I though the compatible string was listed as a required property, but >> it is not. The 'reg' property however is listed as required, although >> the of_miodbus_register() works just fine without it, although that is >> a Linux-specific implementation detail. > > Indeed, it's listed in the required property list of the DT binding doc, > but the code implement auto detection if reg is missing. > However this line [1] clearly shows that specifying the reg property is > the preferred way of doing things. > > I could define 2 different sama5d3xcm.dtsi (sama5d3xcm-ronetix.dtsi and > sama5d3xcm-embest.dtsi) to avoid this dirty hack, > but then we would have 2 more dtb and the user would have to determine > which CPU module he owns to choose the appropriate dtb. > If at91, arm-soc and DT maintainers agree with this approach I can > definitely propose something. Yes Boris, I definitively prefer not to add another .dtsi file for this series if we can avoid it. So, I would push for this "reg-less" solution. If it is chosen, you can add my: Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com> Thanks, bye. >>> Define board specific delays to apply to RGMII signals. >>> >>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> >> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > Thanks for your review. > > Best Regards, > > Boris > > [1] http://lxr.free-electrons.com/source/drivers/of/of_mdio.c#L187 >
2014-07-10 2:07 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: > On Fri, 27 Jun 2014 09:52:56 +0200 > Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > >> On 26/06/2014 22:01, Boris BREZILLON : >> > Hi Florian, >> > >> > On 26/06/2014 20:15, Florian Fainelli wrote: >> >> Hi Boris, >> >> >> >> 2014-06-26 3:13 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: >> >>> Add ethernet-phy node and specify phy interrupt (connected to pin PB25). >> >>> >> >>> The PHY address is not specified here because atmel have 2 different >> >>> designs >> >>> for its CPU modules: one is connecting PHYAD[0-2] pins to pull up resistors >> >>> (Embest design) and the other one is connection PHYAD0 to a pull up >> >>> resistor and PHYAD[1-2] to pull down resistors (Ronetix design). >> >>> As a result, Ronetix design will have its PHY available at address 0x1 and >> >>> Embest design at 0x7. >> >>> Let the net PHY core automatically detect the PHY address by scanning the >> >>> MDIO bus. >> >> I though the compatible string was listed as a required property, but >> >> it is not. The 'reg' property however is listed as required, although >> >> the of_miodbus_register() works just fine without it, although that is >> >> a Linux-specific implementation detail. >> > >> > Indeed, it's listed in the required property list of the DT binding doc, >> > but the code implement auto detection if reg is missing. >> > However this line [1] clearly shows that specifying the reg property is >> > the preferred way of doing things. >> > >> > I could define 2 different sama5d3xcm.dtsi (sama5d3xcm-ronetix.dtsi and >> > sama5d3xcm-embest.dtsi) to avoid this dirty hack, >> > but then we would have 2 more dtb and the user would have to determine >> > which CPU module he owns to choose the appropriate dtb. >> > If at91, arm-soc and DT maintainers agree with this approach I can >> > definitely propose something. >> >> Yes Boris, I definitively prefer not to add another .dtsi file for this >> series if we can avoid it. >> > > Okay, now that I don't specify the reg property I have a bunch of > noisy logs (which is exactly what the developer of of_mdio.c wanted in > order to force people to specify the reg property). > > It seems to be a problem for atmel users (all these logs make them > think there is something wrong with the net device). > > Apart from the dts/dtsi split solution, which Nicolas wants to avoid, I > see two solutions here: > > 1) remove the logs (or use dev_dbg instead of dev_info) from of_mdio.c. > But I'm pretty sure this solution won't be accepted :-). I am fine with using dev_dbg() instead of dev_info() for that sort of messages, provided that you state the rationale of this change (spewing the log console with probing messages) and specify tha the 'reg' property is optional. > > 2) define 2 ethernet phys (one for each possible solution). I tested it > and it works fine (only the available PHY is registered and there is no > noisy logs anymore). One advantage of that solution is that you'll get slightly faster boot times since you won't have to auto-probe for the PHYs on the MDIO bus, the time savings get bigger as you start using higher PHY addresses. > > Could net and DT maintainers help us choose the best solution (or > propose a new one) ? > > Best Regards, > > Boris > > -- > Boris Brezillon, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
On Thu, 10 Jul 2014 08:35:15 -0700 Florian Fainelli <f.fainelli@gmail.com> wrote: > 2014-07-10 2:07 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: > > On Fri, 27 Jun 2014 09:52:56 +0200 > > Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > > > >> On 26/06/2014 22:01, Boris BREZILLON : > >> > Hi Florian, > >> > > >> > On 26/06/2014 20:15, Florian Fainelli wrote: > >> >> Hi Boris, > >> >> > >> >> 2014-06-26 3:13 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: > >> >>> Add ethernet-phy node and specify phy interrupt (connected to pin PB25). > >> >>> > >> >>> The PHY address is not specified here because atmel have 2 different > >> >>> designs > >> >>> for its CPU modules: one is connecting PHYAD[0-2] pins to pull up resistors > >> >>> (Embest design) and the other one is connection PHYAD0 to a pull up > >> >>> resistor and PHYAD[1-2] to pull down resistors (Ronetix design). > >> >>> As a result, Ronetix design will have its PHY available at address 0x1 and > >> >>> Embest design at 0x7. > >> >>> Let the net PHY core automatically detect the PHY address by scanning the > >> >>> MDIO bus. > >> >> I though the compatible string was listed as a required property, but > >> >> it is not. The 'reg' property however is listed as required, although > >> >> the of_miodbus_register() works just fine without it, although that is > >> >> a Linux-specific implementation detail. > >> > > >> > Indeed, it's listed in the required property list of the DT binding doc, > >> > but the code implement auto detection if reg is missing. > >> > However this line [1] clearly shows that specifying the reg property is > >> > the preferred way of doing things. > >> > > >> > I could define 2 different sama5d3xcm.dtsi (sama5d3xcm-ronetix.dtsi and > >> > sama5d3xcm-embest.dtsi) to avoid this dirty hack, > >> > but then we would have 2 more dtb and the user would have to determine > >> > which CPU module he owns to choose the appropriate dtb. > >> > If at91, arm-soc and DT maintainers agree with this approach I can > >> > definitely propose something. > >> > >> Yes Boris, I definitively prefer not to add another .dtsi file for this > >> series if we can avoid it. > >> > > > > Okay, now that I don't specify the reg property I have a bunch of > > noisy logs (which is exactly what the developer of of_mdio.c wanted in > > order to force people to specify the reg property). > > > > It seems to be a problem for atmel users (all these logs make them > > think there is something wrong with the net device). > > > > Apart from the dts/dtsi split solution, which Nicolas wants to avoid, I > > see two solutions here: > > > > 1) remove the logs (or use dev_dbg instead of dev_info) from of_mdio.c. > > But I'm pretty sure this solution won't be accepted :-). > > I am fine with using dev_dbg() instead of dev_info() for that sort of > messages, provided that you state the rationale of this change > (spewing the log console with probing messages) and specify tha the > 'reg' property is optional. > > > > > 2) define 2 ethernet phys (one for each possible solution). I tested it > > and it works fine (only the available PHY is registered and there is no > > noisy logs anymore). > > One advantage of that solution is that you'll get slightly faster boot > times since you won't have to auto-probe for the PHYs on the MDIO bus, > the time savings get bigger as you start using higher PHY addresses. Yes I prefer this solution too, but is it acceptable to define 2 phy nodes even if only one is really available ?
2014-07-10 10:19 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: > On Thu, 10 Jul 2014 08:35:15 -0700 > Florian Fainelli <f.fainelli@gmail.com> wrote: > >> 2014-07-10 2:07 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: >> > On Fri, 27 Jun 2014 09:52:56 +0200 >> > Nicolas Ferre <nicolas.ferre@atmel.com> wrote: >> > >> >> On 26/06/2014 22:01, Boris BREZILLON : >> >> > Hi Florian, >> >> > >> >> > On 26/06/2014 20:15, Florian Fainelli wrote: >> >> >> Hi Boris, >> >> >> >> >> >> 2014-06-26 3:13 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: >> >> >>> Add ethernet-phy node and specify phy interrupt (connected to pin PB25). >> >> >>> >> >> >>> The PHY address is not specified here because atmel have 2 different >> >> >>> designs >> >> >>> for its CPU modules: one is connecting PHYAD[0-2] pins to pull up resistors >> >> >>> (Embest design) and the other one is connection PHYAD0 to a pull up >> >> >>> resistor and PHYAD[1-2] to pull down resistors (Ronetix design). >> >> >>> As a result, Ronetix design will have its PHY available at address 0x1 and >> >> >>> Embest design at 0x7. >> >> >>> Let the net PHY core automatically detect the PHY address by scanning the >> >> >>> MDIO bus. >> >> >> I though the compatible string was listed as a required property, but >> >> >> it is not. The 'reg' property however is listed as required, although >> >> >> the of_miodbus_register() works just fine without it, although that is >> >> >> a Linux-specific implementation detail. >> >> > >> >> > Indeed, it's listed in the required property list of the DT binding doc, >> >> > but the code implement auto detection if reg is missing. >> >> > However this line [1] clearly shows that specifying the reg property is >> >> > the preferred way of doing things. >> >> > >> >> > I could define 2 different sama5d3xcm.dtsi (sama5d3xcm-ronetix.dtsi and >> >> > sama5d3xcm-embest.dtsi) to avoid this dirty hack, >> >> > but then we would have 2 more dtb and the user would have to determine >> >> > which CPU module he owns to choose the appropriate dtb. >> >> > If at91, arm-soc and DT maintainers agree with this approach I can >> >> > definitely propose something. >> >> >> >> Yes Boris, I definitively prefer not to add another .dtsi file for this >> >> series if we can avoid it. >> >> >> > >> > Okay, now that I don't specify the reg property I have a bunch of >> > noisy logs (which is exactly what the developer of of_mdio.c wanted in >> > order to force people to specify the reg property). >> > >> > It seems to be a problem for atmel users (all these logs make them >> > think there is something wrong with the net device). >> > >> > Apart from the dts/dtsi split solution, which Nicolas wants to avoid, I >> > see two solutions here: >> > >> > 1) remove the logs (or use dev_dbg instead of dev_info) from of_mdio.c. >> > But I'm pretty sure this solution won't be accepted :-). >> >> I am fine with using dev_dbg() instead of dev_info() for that sort of >> messages, provided that you state the rationale of this change >> (spewing the log console with probing messages) and specify tha the >> 'reg' property is optional. >> >> > >> > 2) define 2 ethernet phys (one for each possible solution). I tested it >> > and it works fine (only the available PHY is registered and there is no >> > noisy logs anymore). >> >> One advantage of that solution is that you'll get slightly faster boot >> times since you won't have to auto-probe for the PHYs on the MDIO bus, >> the time savings get bigger as you start using higher PHY addresses. > > Yes I prefer this solution too, but is it acceptable to define 2 phy > nodes even if only one is really available ? One or the other machine .dts should have to set the status property accordingly so there is only effectively one PHY declared, assuming this is possible based on your .dtsi layout? If not, provided by the phy-handle properties are correct, I can't see any problem with having an unused PHY specified in DT. -- Florian
On Thu, 10 Jul 2014 10:46:21 -0700 Florian Fainelli <f.fainelli@gmail.com> wrote: > 2014-07-10 10:19 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: > > On Thu, 10 Jul 2014 08:35:15 -0700 > > Florian Fainelli <f.fainelli@gmail.com> wrote: > > > >> 2014-07-10 2:07 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: > >> > On Fri, 27 Jun 2014 09:52:56 +0200 > >> > Nicolas Ferre <nicolas.ferre@atmel.com> wrote: > >> > > >> >> On 26/06/2014 22:01, Boris BREZILLON : > >> >> > Hi Florian, > >> >> > > >> >> > On 26/06/2014 20:15, Florian Fainelli wrote: > >> >> >> Hi Boris, > >> >> >> > >> >> >> 2014-06-26 3:13 GMT-07:00 Boris BREZILLON <boris.brezillon@free-electrons.com>: > >> >> >>> Add ethernet-phy node and specify phy interrupt (connected to pin PB25). > >> >> >>> > >> >> >>> The PHY address is not specified here because atmel have 2 different > >> >> >>> designs > >> >> >>> for its CPU modules: one is connecting PHYAD[0-2] pins to pull up resistors > >> >> >>> (Embest design) and the other one is connection PHYAD0 to a pull up > >> >> >>> resistor and PHYAD[1-2] to pull down resistors (Ronetix design). > >> >> >>> As a result, Ronetix design will have its PHY available at address 0x1 and > >> >> >>> Embest design at 0x7. > >> >> >>> Let the net PHY core automatically detect the PHY address by scanning the > >> >> >>> MDIO bus. > >> >> >> I though the compatible string was listed as a required property, but > >> >> >> it is not. The 'reg' property however is listed as required, although > >> >> >> the of_miodbus_register() works just fine without it, although that is > >> >> >> a Linux-specific implementation detail. > >> >> > > >> >> > Indeed, it's listed in the required property list of the DT binding doc, > >> >> > but the code implement auto detection if reg is missing. > >> >> > However this line [1] clearly shows that specifying the reg property is > >> >> > the preferred way of doing things. > >> >> > > >> >> > I could define 2 different sama5d3xcm.dtsi (sama5d3xcm-ronetix.dtsi and > >> >> > sama5d3xcm-embest.dtsi) to avoid this dirty hack, > >> >> > but then we would have 2 more dtb and the user would have to determine > >> >> > which CPU module he owns to choose the appropriate dtb. > >> >> > If at91, arm-soc and DT maintainers agree with this approach I can > >> >> > definitely propose something. > >> >> > >> >> Yes Boris, I definitively prefer not to add another .dtsi file for this > >> >> series if we can avoid it. > >> >> > >> > > >> > Okay, now that I don't specify the reg property I have a bunch of > >> > noisy logs (which is exactly what the developer of of_mdio.c wanted in > >> > order to force people to specify the reg property). > >> > > >> > It seems to be a problem for atmel users (all these logs make them > >> > think there is something wrong with the net device). > >> > > >> > Apart from the dts/dtsi split solution, which Nicolas wants to avoid, I > >> > see two solutions here: > >> > > >> > 1) remove the logs (or use dev_dbg instead of dev_info) from of_mdio.c. > >> > But I'm pretty sure this solution won't be accepted :-). > >> > >> I am fine with using dev_dbg() instead of dev_info() for that sort of > >> messages, provided that you state the rationale of this change > >> (spewing the log console with probing messages) and specify tha the > >> 'reg' property is optional. > >> > >> > > >> > 2) define 2 ethernet phys (one for each possible solution). I tested it > >> > and it works fine (only the available PHY is registered and there is no > >> > noisy logs anymore). > >> > >> One advantage of that solution is that you'll get slightly faster boot > >> times since you won't have to auto-probe for the PHYs on the MDIO bus, > >> the time savings get bigger as you start using higher PHY addresses. > > > > Yes I prefer this solution too, but is it acceptable to define 2 phy > > nodes even if only one is really available ? > > One or the other machine .dts should have to set the status property > accordingly so there is only effectively one PHY declared, assuming > this is possible based on your .dtsi layout? This is exactly what Nicolas wants to avoid (duplication of dts files to handle Embest and Ronetix designs). > If not, provided by the > phy-handle properties are correct, I can't see any problem with having > an unused PHY specified in DT. Okay, then I'll go for that solution. Thanks, Boris
diff --git a/arch/arm/boot/dts/sama5d3xcm.dtsi b/arch/arm/boot/dts/sama5d3xcm.dtsi index b0b1331..fc68bae 100644 --- a/arch/arm/boot/dts/sama5d3xcm.dtsi +++ b/arch/arm/boot/dts/sama5d3xcm.dtsi @@ -34,6 +34,21 @@ macb0: ethernet@f0028000 { phy-mode = "rgmii"; + #address-cells = <1>; + #size-cells = <0>; + + ethernet-phy { + interrupt-parent = <&pioB>; + interrupts = <25 IRQ_TYPE_EDGE_FALLING>; + txen-skew-ps = <800>; + txc-skew-ps = <3000>; + rxdv-skew-ps = <400>; + rxc-skew-ps = <3000>; + rxd0-skew-ps = <400>; + rxd1-skew-ps = <400>; + rxd2-skew-ps = <400>; + rxd3-skew-ps = <400>; + }; }; pmc: pmc@fffffc00 {
Add ethernet-phy node and specify phy interrupt (connected to pin PB25). The PHY address is not specified here because atmel have 2 different designs for its CPU modules: one is connecting PHYAD[0-2] pins to pull up resistors (Embest design) and the other one is connection PHYAD0 to a pull up resistor and PHYAD[1-2] to pull down resistors (Ronetix design). As a result, Ronetix design will have its PHY available at address 0x1 and Embest design at 0x7. Let the net PHY core automatically detect the PHY address by scanning the MDIO bus. Define board specific delays to apply to RGMII signals. Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com> --- arch/arm/boot/dts/sama5d3xcm.dtsi | 15 +++++++++++++++ 1 file changed, 15 insertions(+)