Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
> There is a way of doing this to preserve backwards compatibility I think. > Maybe deprecating the previous DTS phy-mode and replacing it with a new one, > phy-mode-v2 instead? Hi Steve It needs to be a vendor specific property, atheros,phy-mode, for example. Andrew
RE: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
Here are my thoughts. On 22 March 2019 10:21, Michal Vokáč wrote: > Subject: Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use > rgmii-id > > On 22. 03. 19 3:24, Fabio Estevam wrote: > > On Thu, Mar 21, 2019 at 11:15 PM Shawn Guo wrote: > > > >>> Unfortunately, just by looking at the dts files we do not know if a > >>> board uses an AR803x PHY or not, so I am afraid we can not do an > >>> automatic conversion. > >> > >> At least for those we already know? > > > > Yes, I can help preparing a patch that fixes the i.MX boards we know > > use AR8031 PHY. > > AFACT this issue is not AR803x specific. I was hit by the same problem > with QCA833x switch on imx6dl-yapp4. Though, we are currently the only > platform in tree using this chip and Shawn already has the fix in his tree. > > I am not aware of any other PHY drivers that my cause problems. > > $ git log --oneline v5.0..v5.1-rc1 --grep=RGMII --author=Vinod -- drivers/net/ > 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode > a968b5e9d587 net: dsa: qca8k: Enable delay for RGMII_ID mode > 5ecdd77c61c8 net: dsa: qca8k: disable delay for RGMII mode > cd28d1d6e52e net: phy: at803x: Disable phy delay for RGMII mode If this is more widespread, as Michal points out, this might affect the way the fix is approach then? On 21 March 2019 12:43 Lucas Stach wrote: > > So yes, we currently have lots of broken dtb's in mainline and I am > > wondering what is the proper fix here. [...] > but if the DT is clearly broken and fixing it in a backward > compatible way is not really feasible, I would argue that we should > treat a DT bug like any other bug. There is a way of doing this to preserve backwards compatibility I think. Maybe deprecating the previous DTS phy-mode and replacing it with a new one, phy-mode-v2 instead? At the moment, there is only a weak link between DTS and the driver in the kernel which makes finding the error difficult. Also, it seems that partly, the direction of change is caused by software which is pushing alterations in the DTS -- and not the other way round. Regards, Steve
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
On 22. 03. 19 3:24, Fabio Estevam wrote: On Thu, Mar 21, 2019 at 11:15 PM Shawn Guo wrote: Unfortunately, just by looking at the dts files we do not know if a board uses an AR803x PHY or not, so I am afraid we can not do an automatic conversion. At least for those we already know? Yes, I can help preparing a patch that fixes the i.MX boards we know use AR8031 PHY. AFACT this issue is not AR803x specific. I was hit by the same problem with QCA833x switch on imx6dl-yapp4. Though, we are currently the only platform in tree using this chip and Shawn already has the fix in his tree. I am not aware of any other PHY drivers that my cause problems. $ git log --oneline v5.0..v5.1-rc1 --grep=RGMII --author=Vinod -- drivers/net/ 6d4cd041f0af net: phy: at803x: disable delay only for RGMII mode a968b5e9d587 net: dsa: qca8k: Enable delay for RGMII_ID mode 5ecdd77c61c8 net: dsa: qca8k: disable delay for RGMII mode cd28d1d6e52e net: phy: at803x: Disable phy delay for RGMII mode Michal
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
On Thu, Mar 21, 2019 at 11:15 PM Shawn Guo wrote: > > Unfortunately, just by looking at the dts files we do not know if a > > board uses an AR803x PHY or not, so I am afraid we can not do an > > automatic conversion. > > At least for those we already know? Yes, I can help preparing a patch that fixes the i.MX boards we know use AR8031 PHY.
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
On Thu, Mar 21, 2019 at 11:00:18PM -0300, Fabio Estevam wrote: > Hi Shawn, > > On Thu, Mar 21, 2019 at 10:12 PM Shawn Guo wrote: > > > > So yes, we currently have lots of broken dtb's in mainline and I am > > > wondering what is the proper fix here. > > > > So can we have a single patch fixing all broken i.MX DTBs? > > Unfortunately, just by looking at the dts files we do not know if a > board uses an AR803x PHY or not, so I am afraid we can not do an > automatic conversion. At least for those we already know? Shawn
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
Hi Shawn, On Thu, Mar 21, 2019 at 10:12 PM Shawn Guo wrote: > > So yes, we currently have lots of broken dtb's in mainline and I am > > wondering what is the proper fix here. > > So can we have a single patch fixing all broken i.MX DTBs? Unfortunately, just by looking at the dts files we do not know if a board uses an AR803x PHY or not, so I am afraid we can not do an automatic conversion.
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
On Thu, Mar 21, 2019 at 08:17:01AM -0300, Fabio Estevam wrote: > Hi Abel, > > On Thu, Mar 21, 2019 at 5:42 AM Abel Vesa wrote: > > > > It seems we have other boards that need to be fixed and we can not > > > have an old dtb with functional Ethernet with a new kernel. > > > > > > Does anyone know if this issue is AR8031 specific? > > > > I can confirm the same fix is works on imx6sx too. > > imx6sx-sabresd also uses an AR8031 Ethernet PHY. > > I also tested that changing the phy-mode to "rgmii-id" fixes Ethernet > on pico-imx7d with AR8035. > > So yes, we currently have lots of broken dtb's in mainline and I am > wondering what is the proper fix here. So can we have a single patch fixing all broken i.MX DTBs? Shawn
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
Am Donnerstag, den 21.03.2019, 08:17 -0300 schrieb Fabio Estevam: > Hi Abel, > > > On Thu, Mar 21, 2019 at 5:42 AM Abel Vesa wrote: > > > > It seems we have other boards that need to be fixed and we can not > > > have an old dtb with functional Ethernet with a new kernel. > > > > > > Does anyone know if this issue is AR8031 specific? > > > > I can confirm the same fix is works on imx6sx too. > > imx6sx-sabresd also uses an AR8031 Ethernet PHY. > > I also tested that changing the phy-mode to "rgmii-id" fixes Ethernet > on pico-imx7d with AR8035. > > So yes, we currently have lots of broken dtb's in mainline and I am > wondering what is the proper fix here. Fix the DT. It's unfortunate the a kernel and DT bug canceled each other out, but if the DT is clearly broken and fixing it in a backward compatible way is not really feasible, I would argue that we should treat a DT bug like any other bug. Regards, Lucas
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
> I bisected to the original breakage (for the NFS rootfs) back to this commit:
> commit 13d0ab6750b20957ac1466da4e44dc0af746ff28
I don't think that is the correct patch. Yes, it broke here, but that
was a different problem.
I think the real commit is:
6d4cd041f0af ("net: phy: at803x: disable delay only for RGMII mode")
That commit contains a fix for broken behaviour in the PHY driver,
when then also causes broken DTs to fail.
Andrew
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
On Thu, Mar 21, 2019 at 08:17:01AM -0300, Fabio Estevam wrote: > Hi Abel, > > On Thu, Mar 21, 2019 at 5:42 AM Abel Vesa wrote: > > > > It seems we have other boards that need to be fixed and we can not > > > have an old dtb with functional Ethernet with a new kernel. > > > > > > Does anyone know if this issue is AR8031 specific? > > > > I can confirm the same fix is works on imx6sx too. > > imx6sx-sabresd also uses an AR8031 Ethernet PHY. > > I also tested that changing the phy-mode to "rgmii-id" fixes Ethernet > on pico-imx7d with AR8035. > > So yes, we currently have lots of broken dtb's in mainline and I am > wondering what is the proper fix here. > > Does anyone know what was the kernel commit that introduced such regressions? Hi Fabio The problem here is, all the DTs were broken since day 0. However, because the PHY driver was also broken, nobody noticed and it worked. Now that the PHY driver has been fixed, all the bugs in the DTs now become an issue. There is also a need to fix the PHY driver, because there is at least one board which needs it fixed in order to work. When we discussed fixing the PHY driver, we had no idea how many boards would break. So we said, lets try it, and fix up whatever breaks. We can however discuss this again. We can declare both the PHY driver and the DTs terminally broken, and add a vendor proprietary property for the phy-mode, which takes preference over the standard phy-mode if present, and that does the correct thing. Andrew
RE: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
Hi Fabio,
On 21 March 2019 11:17, Fabio Estevam,
> Subject: Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use
> rgmii-id
>
> Hi Abel,
>
> On Thu, Mar 21, 2019 at 5:42 AM Abel Vesa wrote:
>
> > > It seems we have other boards that need to be fixed and we can not
> > > have an old dtb with functional Ethernet with a new kernel.
> > >
> > > Does anyone know if this issue is AR8031 specific?
> >
> > I can confirm the same fix is works on imx6sx too.
>
> imx6sx-sabresd also uses an AR8031 Ethernet PHY.
>
> I also tested that changing the phy-mode to "rgmii-id" fixes Ethernet
> on pico-imx7d with AR8035.
>
> So yes, we currently have lots of broken dtb's in mainline and I am
> wondering what is the proper fix here.
Agreed!
The DT should be an ABI.
> Does anyone know what was the kernel commit that introduced such
> regressions?
I bisected to the original breakage (for the NFS rootfs) back to this commit:
commit 13d0ab6750b20957ac1466da4e44dc0af746ff28
Reverting this commit fixed the problem, but only for the kernel up to that
point: it was a long time ago, and several other fixes were added after that
which meant that by the time the kernel got to v5.0 it was working for me
again.
I bisected the breakage between v5.0 to v5.1-rc1 as this:
commit 3acca1dd17060332cfab15693733cdaf9fba1c90
... which doesn't make sense to me.
Regards,
Steve
---
commit 13d0ab6750b20957ac1466da4e44dc0af746ff28
Author: Heiner Kallweit
Date: Wed Jan 16 08:07:38 2019 +0100
net: phy: check return code when requesting PHY driver module
When requesting the PHY driver module fails we'll bind the genphy
driver later. This isn't obvious to the user and may cause, depending
on the PHY, different types of issues. Therefore check the return code
of request_module(). Note that we only check for failures in loading
the module, not whether a module exists for the respective PHY ID.
v2:
- add comment explaining what is checked and what is not
- return error from phy_device_create() if loading module fails
Signed-off-by: Heiner Kallweit
Reviewed-by: Andrew Lunn
Signed-off-by: David S. Miller
---
commit 3acca1dd17060332cfab15693733cdaf9fba1c90
Author: Heiner Kallweit
Date: Mon Mar 4 19:39:03 2019 +0100
net: dsa: mv88e6xxx: add call to mv88e6xxx_ports_cmode_init to probe for
new DSA framework
In the original patch I missed to add mv88e6xxx_ports_cmode_init()
to the second probe function, the one for the new DSA framework.
Fixes: ed8fe20205ac ("net: dsa: mv88e6xxx: prevent interrupt storm caused
by mv88e6390x_port_set_cmode")
Reported-by: Shaokun Zhang
Suggested-by: Andrew Lunn
Signed-off-by: Heiner Kallweit
Reviewed-by: Andrew Lunn
Signed-off-by: David S. Miller
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
Hi Abel, On Thu, Mar 21, 2019 at 5:42 AM Abel Vesa wrote: > > It seems we have other boards that need to be fixed and we can not > > have an old dtb with functional Ethernet with a new kernel. > > > > Does anyone know if this issue is AR8031 specific? > > I can confirm the same fix is works on imx6sx too. imx6sx-sabresd also uses an AR8031 Ethernet PHY. I also tested that changing the phy-mode to "rgmii-id" fixes Ethernet on pico-imx7d with AR8035. So yes, we currently have lots of broken dtb's in mainline and I am wondering what is the proper fix here. Does anyone know what was the kernel commit that introduced such regressions?
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
On 19-03-20 14:04:42, Fabio Estevam wrote: > Hi Steve, > > [Adding Andrew] > > On Wed, Mar 20, 2019 at 1:03 PM Steve Twiss > wrote: > > > > Hi Fabio, > > > > On 20 March 2019 12:17, Fabio Estevam wrote: > > > Subject: Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use > > > rgmii-id > > > > > > Hi Steve, > > > > > > On Wed, Mar 20, 2019 at 9:06 AM Steve Twiss > > > wrote: > > > > > > > > The PHY used on the Freescale i.MX6Q/DL SABRE boards is qualified as > > > > 'rgmii' instead of 'rgmii-id'. Meaning the RX and TX delays that were > > > > > > This patch declares it as 'rgmii-id', which contradicts the commit log. > > > > The PHY used on the Freescale i.MX6Q/DL SABRE boards is qualified as > > 'rgmii' instead of 'rgmii-id'. > > > > Can that be read two ways? > > I meant the phy-mode is currently qualified as 'rgmii' instead of what it > > should be: 'rgmii-id'. > > Thanks for the clarification. > > > > > [...] > > > > > > This patch fixes the network problems seen on the Freescale i.MX6Q/DL > > > > > > Please provide a Fixes tag. It would be good to know if this fix needs > > > to be applied to older kernels. > > > > I didn't put a Fixes tag in because I find this patch fixes v5.1-rc1 (which > > I see as broken). > > But the patch is not needed in v5.0 because I see that as working. > > So didn't see the need for a Cc: stable. > > It seems we have other boards that need to be fixed and we can not > have an old dtb with functional Ethernet with a new kernel. > > Does anyone know if this issue is AR8031 specific? I can confirm the same fix is works on imx6sx too. > > Thanks
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
Hi Steve, [Adding Andrew] On Wed, Mar 20, 2019 at 1:03 PM Steve Twiss wrote: > > Hi Fabio, > > On 20 March 2019 12:17, Fabio Estevam wrote: > > Subject: Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use > > rgmii-id > > > > Hi Steve, > > > > On Wed, Mar 20, 2019 at 9:06 AM Steve Twiss > > wrote: > > > > > > The PHY used on the Freescale i.MX6Q/DL SABRE boards is qualified as > > > 'rgmii' instead of 'rgmii-id'. Meaning the RX and TX delays that were > > > > This patch declares it as 'rgmii-id', which contradicts the commit log. > > The PHY used on the Freescale i.MX6Q/DL SABRE boards is qualified as > 'rgmii' instead of 'rgmii-id'. > > Can that be read two ways? > I meant the phy-mode is currently qualified as 'rgmii' instead of what it > should be: 'rgmii-id'. Thanks for the clarification. > > [...] > > > > This patch fixes the network problems seen on the Freescale i.MX6Q/DL > > > > Please provide a Fixes tag. It would be good to know if this fix needs > > to be applied to older kernels. > > I didn't put a Fixes tag in because I find this patch fixes v5.1-rc1 (which I > see as broken). > But the patch is not needed in v5.0 because I see that as working. > So didn't see the need for a Cc: stable. It seems we have other boards that need to be fixed and we can not have an old dtb with functional Ethernet with a new kernel. Does anyone know if this issue is AR8031 specific? Thanks
RE: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
Hi Fabio, On 20 March 2019 12:17, Fabio Estevam wrote: > Subject: Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use > rgmii-id > > Hi Steve, > > On Wed, Mar 20, 2019 at 9:06 AM Steve Twiss > wrote: > > > > The PHY used on the Freescale i.MX6Q/DL SABRE boards is qualified as > > 'rgmii' instead of 'rgmii-id'. Meaning the RX and TX delays that were > > This patch declares it as 'rgmii-id', which contradicts the commit log. The PHY used on the Freescale i.MX6Q/DL SABRE boards is qualified as 'rgmii' instead of 'rgmii-id'. Can that be read two ways? I meant the phy-mode is currently qualified as 'rgmii' instead of what it should be: 'rgmii-id'. [...] > > This patch fixes the network problems seen on the Freescale i.MX6Q/DL > > Please provide a Fixes tag. It would be good to know if this fix needs > to be applied to older kernels. I didn't put a Fixes tag in because I find this patch fixes v5.1-rc1 (which I see as broken). But the patch is not needed in v5.0 because I see that as working. So didn't see the need for a Cc: stable. Regards, Steve
Re: [PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
Hi Steve, On Wed, Mar 20, 2019 at 9:06 AM Steve Twiss wrote: > > The PHY used on the Freescale i.MX6Q/DL SABRE boards is qualified as > 'rgmii' instead of 'rgmii-id'. Meaning the RX and TX delays that were This patch declares it as 'rgmii-id', which contradicts the commit log. > previously added by the MAC when required, but are now provided > internally by the PHY (and the MAC should no longer add the RX or TX > delays in this case). > > This patch fixes the network problems seen on the Freescale i.MX6Q/DL Please provide a Fixes tag. It would be good to know if this fix needs to be applied to older kernels.
[PATCH] ARM: dts: imx6qdl-sabresd: change phy-mode to use rgmii-id
The PHY used on the Freescale i.MX6Q/DL SABRE boards is qualified as
'rgmii' instead of 'rgmii-id'. Meaning the RX and TX delays that were
previously added by the MAC when required, but are now provided
internally by the PHY (and the MAC should no longer add the RX or TX
delays in this case).
This patch fixes the network problems seen on the Freescale i.MX6Q/DL
SABRE boards and makes a difference when correctly loading the NFS
rootfs on startup.
Link:
https://lkml.kernel.org/r/1397569821-5530-4-git-send-email-thomas.petazz...@free-electrons.com
Tested-by: Steve Twiss
Tested-by: Adam Thomson
Signed-off-by: Steve Twiss
---
arch/arm/boot/dts/imx6qdl-sabresd.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
index a070506..185fb17 100644
--- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
@@ -202,7 +202,7 @@
&fec {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_enet>;
- phy-mode = "rgmii";
+ phy-mode = "rgmii-id";
phy-reset-gpios = <&gpio1 25 GPIO_ACTIVE_LOW>;
status = "okay";
};
--
1.9.3
