| Message ID | 20220419102709.26432-4-josua@solid-run.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | adin: add support for clock output | expand |
On Tue, Apr 19, 2022 at 01:27:09PM +0300, Josua Mayer wrote: > Since SoM revision 1.9 the PHY has been replaced with an ADIN1300, > add an entry for it next to the original. > > Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com> > Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com> > Signed-off-by: Josua Mayer <josua@solid-run.com> > --- > V1 -> V2: changed dts property name > > arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi > index f86efd0ccc40..d46182095d79 100644 > --- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi > +++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi > @@ -83,6 +83,12 @@ ethernet-phy@4 { > qca,clk-out-frequency = <125000000>; > qca,smarteee-tw-us-1g = <24>; > }; > + > + /* ADIN1300 (som rev 1.9 or later) */ > + ethernet-phy@1 { > + reg = <1>; > + adi,phy-output-clock = "125mhz-free-running"; > + }; There is currently the comment: * The PHY can appear at either address 0 or 4 due to the * configuration (LED) pin not being pulled sufficiently. */ It would be good to add another comment about this PHY at address 1. Andrew
On Thu, Apr 21, 2022 at 02:27:16PM +0200, Andrew Lunn wrote: > On Tue, Apr 19, 2022 at 01:27:09PM +0300, Josua Mayer wrote: > > Since SoM revision 1.9 the PHY has been replaced with an ADIN1300, > > add an entry for it next to the original. > > > > Co-developed-by: Alvaro Karsz <alvaro.karsz@solid-run.com> > > Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com> > > Signed-off-by: Josua Mayer <josua@solid-run.com> > > --- > > V1 -> V2: changed dts property name > > > > arch/arm/boot/dts/imx6qdl-sr-som.dtsi | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi > > index f86efd0ccc40..d46182095d79 100644 > > --- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi > > +++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi > > @@ -83,6 +83,12 @@ ethernet-phy@4 { > > qca,clk-out-frequency = <125000000>; > > qca,smarteee-tw-us-1g = <24>; > > }; > > + > > + /* ADIN1300 (som rev 1.9 or later) */ > > + ethernet-phy@1 { > > + reg = <1>; > > + adi,phy-output-clock = "125mhz-free-running"; > > + }; > > There is currently the comment: > > * The PHY can appear at either address 0 or 4 due to the > * configuration (LED) pin not being pulled sufficiently. > */ > > It would be good to add another comment about this PHY at address 1. There is an issue with this approach. Listing the "possible" PHYs in DT so we can configure them leads to the kernel complaining at boot time with: mdio_bus 2188000.ethernet-1: MDIO device at address 4 is missing. So with this patch, we'll also get: mdio_bus 2188000.ethernet-1: MDIO device at address 1 is missing. which is not great for the user to see. Arguably though, it's down to broken hardware design in the case of the AR8035, since this is caused by insufficient pull on the LED pin to ensure the hardware address is reliable configured. However, adding this for a rev 1.9 uSOM where we know that the PHY is different matter. I think we should be aiming for a new revision of DT for the uSOM with the AR8035 nodes removed and the ADIN added, rather than trying to stuff them all into a single DT and have the kernel complain about not just one missing PHY, but now two. The only other ways around this that I can see would be to have some way to flag in DT that the PHYs are "optional" - if they're not found while probing the hardware, then don't whinge about them. Or have u-boot discover which address the PHY is located, and update the DT blob passed to the kernel to disable the PHY addresses that aren't present. Or edit the DT to update the node name and reg property. Or something along those lines.
> The only other ways around this that I can see would be to have some > way to flag in DT that the PHYs are "optional" - if they're not found > while probing the hardware, then don't whinge about them. Or have > u-boot discover which address the PHY is located, and update the DT > blob passed to the kernel to disable the PHY addresses that aren't > present. Or edit the DT to update the node name and reg property. Or > something along those lines. uboot sounds like the best option. I don't know if we currently support the status property for PHYs. Maybe the .dtsi file should have them all status = "disabled"; and uboot can flip the populated ones to "okay". Or maybe the other way around to handle older bootloaders. Andrew
On Thu, Apr 21, 2022 at 03:30:38PM +0200, Andrew Lunn wrote: > > The only other ways around this that I can see would be to have some > > way to flag in DT that the PHYs are "optional" - if they're not found > > while probing the hardware, then don't whinge about them. Or have > > u-boot discover which address the PHY is located, and update the DT > > blob passed to the kernel to disable the PHY addresses that aren't > > present. Or edit the DT to update the node name and reg property. Or > > something along those lines. > > uboot sounds like the best option. I don't know if we currently > support the status property for PHYs. Maybe the .dtsi file should have > them all status = "disabled"; and uboot can flip the populated ones to > "okay". Or maybe the other way around to handle older bootloaders. ... which would immediately regress the networking on all SolidRun iMX6 platforms when booting "new" DT with existing u-boot, so clearly that isn't a possible solution.
Hi Russell, Am 21.04.22 um 17:20 schrieb Russell King (Oracle): > On Thu, Apr 21, 2022 at 03:30:38PM +0200, Andrew Lunn wrote: >>> The only other ways around this that I can see would be to have some >>> way to flag in DT that the PHYs are "optional" - if they're not found >>> while probing the hardware, then don't whinge about them. Or have >>> u-boot discover which address the PHY is located, and update the DT >>> blob passed to the kernel to disable the PHY addresses that aren't >>> present. Or edit the DT to update the node name and reg property. Or >>> something along those lines. >> uboot sounds like the best option. I don't know if we currently >> support the status property for PHYs. Maybe the .dtsi file should have >> them all status = "disabled"; and uboot can flip the populated ones to >> "okay". Or maybe the other way around to handle older bootloaders. > ... which would immediately regress the networking on all SolidRun iMX6 > platforms when booting "new" DT with existing u-boot, so clearly that > isn't a possible solution. So to summarize - you don't want to see a third phy spamming the console with probe errors ... I think a combination of the suggestions would be doable: - Add the new phy to dt, with status disabled - keep the existing phys unchanged - after probing in u-boot, disable the two old entries, and enable the new one It is not very convenient since that means changes to u-boot are necessary, but it can be done - and won't break existing users only updating Linux. sincerely Josua Mayer
On Wed, Apr 27, 2022 at 10:15:31AM +0300, Josua Mayer wrote: > Hi Russell, > > Am 21.04.22 um 17:20 schrieb Russell King (Oracle): > > On Thu, Apr 21, 2022 at 03:30:38PM +0200, Andrew Lunn wrote: > > > > The only other ways around this that I can see would be to have some > > > > way to flag in DT that the PHYs are "optional" - if they're not found > > > > while probing the hardware, then don't whinge about them. Or have > > > > u-boot discover which address the PHY is located, and update the DT > > > > blob passed to the kernel to disable the PHY addresses that aren't > > > > present. Or edit the DT to update the node name and reg property. Or > > > > something along those lines. > > > uboot sounds like the best option. I don't know if we currently > > > support the status property for PHYs. Maybe the .dtsi file should have > > > them all status = "disabled"; and uboot can flip the populated ones to > > > "okay". Or maybe the other way around to handle older bootloaders. > > ... which would immediately regress the networking on all SolidRun iMX6 > > platforms when booting "new" DT with existing u-boot, so clearly that > > isn't a possible solution. > > So to summarize - you don't want to see a third phy spamming the console > with probe errors ... Exactly - it's bad enough that we have to list two PHYs because the PHY could appear at either address 0 or address 4 depending on the direction of the wind on any given day - and all because the PHY uses the LED pin to determine its address, and which doesn't give a strong enough pull that the address can be reliably determined. Every time the PHY gets a hardware reset, it can change its address. > I think a combination of the suggestions would be doable: > - Add the new phy to dt, with status disabled > - keep the existing phys unchanged > - after probing in u-boot, disable the two old entries, and enable the new > one Exactly. > It is not very convenient since that means changes to u-boot are necessary, > but it can be done - and won't break existing users only updating Linux. We wouldn't have the first problem of needing two PHYs if the hardware had been fixed after I reported the problem...
diff --git a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi index f86efd0ccc40..d46182095d79 100644 --- a/arch/arm/boot/dts/imx6qdl-sr-som.dtsi +++ b/arch/arm/boot/dts/imx6qdl-sr-som.dtsi @@ -83,6 +83,12 @@ ethernet-phy@4 { qca,clk-out-frequency = <125000000>; qca,smarteee-tw-us-1g = <24>; }; + + /* ADIN1300 (som rev 1.9 or later) */ + ethernet-phy@1 { + reg = <1>; + adi,phy-output-clock = "125mhz-free-running"; + }; }; };