| Message ID | d75ef7df-a645-7fdd-491a-f89f70dbea01@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [net-next] net: phy: meson-gxl: support more G12A-internal PHY versions | expand |
On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: > On my SC2-based system the genphy driver was used because the PHY > identifies as 0x01803300. It works normal with the meson g12a > driver after this change. > Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. Hi Heiner Are there any datasheets for these devices? Anything which documents the lower nibble really is a revision? I'm just trying to avoid future problems where we find it is actually a different PHY, needs its own MATCH_EXACT entry, and then we find we break devices using 0x01803302 which we had no idea exists, but got covered by this change. Andrew
On 15.01.2023 17:57, Andrew Lunn wrote: > On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: >> On my SC2-based system the genphy driver was used because the PHY >> identifies as 0x01803300. It works normal with the meson g12a >> driver after this change. >> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. > > Hi Heiner > > Are there any datasheets for these devices? Anything which documents > the lower nibble really is a revision? > > I'm just trying to avoid future problems where we find it is actually > a different PHY, needs its own MATCH_EXACT entry, and then we find we > break devices using 0x01803302 which we had no idea exists, but got > covered by this change. > The SC2 platform inherited a lot from G12A, therefore it's plausible that it's the same PHY. Also the vendor driver for SC2 gives a hint as it has the following compatible for the PHY: compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; But you're right, I can't say for sure as I don't have the datasheets. > Andrew Heiner
Hi Heiner, Le 15/01/2023 à 18:09, Heiner Kallweit a écrit : > On 15.01.2023 17:57, Andrew Lunn wrote: >> On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: >>> On my SC2-based system the genphy driver was used because the PHY >>> identifies as 0x01803300. It works normal with the meson g12a >>> driver after this change. >>> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. >> >> Hi Heiner >> >> Are there any datasheets for these devices? Anything which documents >> the lower nibble really is a revision? >> >> I'm just trying to avoid future problems where we find it is actually >> a different PHY, needs its own MATCH_EXACT entry, and then we find we >> break devices using 0x01803302 which we had no idea exists, but got >> covered by this change. >> > The SC2 platform inherited a lot from G12A, therefore it's plausible > that it's the same PHY. Also the vendor driver for SC2 gives a hint > as it has the following compatible for the PHY: > > compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; > > But you're right, I can't say for sure as I don't have the datasheets. On G12A (& GXL), the PHY ID is set in the MDIO MUX registers, please see: https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36 So you should either add support for the PHY mux in SC2 or check what is in the ETH_PHY_CNTL0 register. Neil > >> Andrew > > Heiner
Hi Heiner, On Mon, 16 Jan 2023 at 00:14, Neil Armstrong <neil.armstrong@linaro.org> wrote: > > Hi Heiner, > > Le 15/01/2023 à 18:09, Heiner Kallweit a écrit : > > On 15.01.2023 17:57, Andrew Lunn wrote: > >> On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: > >>> On my SC2-based system the genphy driver was used because the PHY > >>> identifies as 0x01803300. It works normal with the meson g12a > >>> driver after this change. > >>> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. > >> > >> Hi Heiner > >> > >> Are there any datasheets for these devices? Anything which documents > >> the lower nibble really is a revision? > >> > >> I'm just trying to avoid future problems where we find it is actually > >> a different PHY, needs its own MATCH_EXACT entry, and then we find we > >> break devices using 0x01803302 which we had no idea exists, but got > >> covered by this change. > >> > > The SC2 platform inherited a lot from G12A, therefore it's plausible > > that it's the same PHY. Also the vendor driver for SC2 gives a hint > > as it has the following compatible for the PHY: > > > > compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; > > > > But you're right, I can't say for sure as I don't have the datasheets. > > On G12A (& GXL), the PHY ID is set in the MDIO MUX registers, > please see: > https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36 > > So you should either add support for the PHY mux in SC2 or check > what is in the ETH_PHY_CNTL0 register. > I you want to read the ethernet id, we can use the following tool [0] https://github.com/wkz/mdio-tools om my odroid-n2 it show alarm@odroid-n2:~/mdio-tools$ sudo mdio mdio_mux-0.0 DEV PHY-ID LINK 0x00 0x001cc916 down 0x01 0x001cc916 up -Anand > Neil > > > > >> Andrew > > > > Heiner > > > _______________________________________________ > linux-amlogic mailing list > linux-amlogic@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-amlogic
On 15.01.2023 19:43, Neil Armstrong wrote: > Hi Heiner, > > Le 15/01/2023 à 18:09, Heiner Kallweit a écrit : >> On 15.01.2023 17:57, Andrew Lunn wrote: >>> On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: >>>> On my SC2-based system the genphy driver was used because the PHY >>>> identifies as 0x01803300. It works normal with the meson g12a >>>> driver after this change. >>>> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. >>> >>> Hi Heiner >>> >>> Are there any datasheets for these devices? Anything which documents >>> the lower nibble really is a revision? >>> >>> I'm just trying to avoid future problems where we find it is actually >>> a different PHY, needs its own MATCH_EXACT entry, and then we find we >>> break devices using 0x01803302 which we had no idea exists, but got >>> covered by this change. >>> >> The SC2 platform inherited a lot from G12A, therefore it's plausible >> that it's the same PHY. Also the vendor driver for SC2 gives a hint >> as it has the following compatible for the PHY: >> >> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >> >> But you're right, I can't say for sure as I don't have the datasheets. > > On G12A (& GXL), the PHY ID is set in the MDIO MUX registers, > please see: > https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36 > > So you should either add support for the PHY mux in SC2 or check > what is in the ETH_PHY_CNTL0 register. > Thanks for the hint. I just checked and reading back ETH_PHY_CNTL0 at the end of g12a_enable_internal_mdio() gives me the expected result of 0x33010180. But still the PHY reports 3300. Even if I write some other random value to ETH_PHY_CNTL0, I get 0180/3300 as PHY ID. For u-boot I found the following: https://github.com/khadas/u-boot/blob/khadas-vim4-r-64bit/drivers/net/phy/amlogic.c static struct phy_driver amlogic_internal_driver = { .name = "Meson GXL Internal PHY", .uid = 0x01803300, .mask = 0xfffffff0, .features = PHY_BASIC_FEATURES, .config = &meson_phy_config, .startup = &meson_aml_startup, .shutdown = &genphy_shutdown, }; So it's the same PHY ID I'm seeing in Linux. My best guess is that the following is the case: The PHY compatible string in DT is the following in all cases: compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; Therefore id 0180/3301 is used even if the PHY reports something else. Means it doesn't matter which value you write to ETH_PHY_CNTL0. I reduced the compatible string to compatible = "ethernet-phy-ieee802.3-c22" and this resulted in the actual PHY ID being used. You could change the compatible in dts the same way for any g12a system and I assume you would get 0180/3300 too. Remaining question is why the value in ETH_PHY_CNTL0 is ignored. > Neil > >> >>> Andrew >> >> Heiner >
Hello, On Sun, 2023-01-15 at 21:38 +0100, Heiner Kallweit wrote: > On 15.01.2023 19:43, Neil Armstrong wrote: > > Hi Heiner, > > > > Le 15/01/2023 à 18:09, Heiner Kallweit a écrit : > > > On 15.01.2023 17:57, Andrew Lunn wrote: > > > > On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: > > > > > On my SC2-based system the genphy driver was used because the PHY > > > > > identifies as 0x01803300. It works normal with the meson g12a > > > > > driver after this change. > > > > > Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. > > > > > > > > Hi Heiner > > > > > > > > Are there any datasheets for these devices? Anything which documents > > > > the lower nibble really is a revision? > > > > > > > > I'm just trying to avoid future problems where we find it is actually > > > > a different PHY, needs its own MATCH_EXACT entry, and then we find we > > > > break devices using 0x01803302 which we had no idea exists, but got > > > > covered by this change. > > > > > > > The SC2 platform inherited a lot from G12A, therefore it's plausible > > > that it's the same PHY. Also the vendor driver for SC2 gives a hint > > > as it has the following compatible for the PHY: > > > > > > compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; > > > > > > But you're right, I can't say for sure as I don't have the datasheets. > > > > On G12A (& GXL), the PHY ID is set in the MDIO MUX registers, > > please see: > > https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36 > > > > So you should either add support for the PHY mux in SC2 or check > > what is in the ETH_PHY_CNTL0 register. > > > Thanks for the hint. I just checked and reading back ETH_PHY_CNTL0 at the > end of g12a_enable_internal_mdio() gives me the expected result of 0x33010180. > But still the PHY reports 3300. > Even if I write some other random value to ETH_PHY_CNTL0, I get 0180/3300 > as PHY ID. > > For u-boot I found the following: > > https://github.com/khadas/u-boot/blob/khadas-vim4-r-64bit/drivers/net/phy/amlogic.c > > static struct phy_driver amlogic_internal_driver = { > .name = "Meson GXL Internal PHY", > .uid = 0x01803300, > .mask = 0xfffffff0, > .features = PHY_BASIC_FEATURES, > .config = &meson_phy_config, > .startup = &meson_aml_startup, > .shutdown = &genphy_shutdown, > }; > > So it's the same PHY ID I'm seeing in Linux. > > My best guess is that the following is the case: > > The PHY compatible string in DT is the following in all cases: > compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; > > Therefore id 0180/3301 is used even if the PHY reports something else. > Means it doesn't matter which value you write to ETH_PHY_CNTL0. > > I reduced the compatible string to compatible = "ethernet-phy-ieee802.3-c22" > and this resulted in the actual PHY ID being used. > You could change the compatible in dts the same way for any g12a system > and I assume you would get 0180/3300 too. > > Remaining question is why the value in ETH_PHY_CNTL0 is ignored. I [mis?]read the above as we can't completely rule out Andrew's doubt, as such marking this patch as changed requested. Cheers, Paolo
> > The PHY compatible string in DT is the following in all cases: > > compatible = "ethernet-phy-id0180.3301" This form of compatible has two purposes. 1) You cannot read the PHY ID register during MDIO bus enumeration, generally because you need to turn on GPIOs, clocks, regulators etc, which the MDIO/PHY core does not know how to do. 2) The PHY has bad values in its ID registers, typically because the manufactures messed up. If you have a compatible like this, the ID registers are totally ignored by Linux, and the ID is used to find the driver and tell the driver exactly which of the multiple devices it supports it should assume the device is. So you should use this from of compatible with care. You can easily end up thinking you have a different PHY to what you actually have, which could then result in wrong erratas being applied etc, or even the wrong driver being used. Andrew
On 17.01.2023 14:30, Andrew Lunn wrote: >>> The PHY compatible string in DT is the following in all cases: >>> compatible = "ethernet-phy-id0180.3301" > > This form of compatible has two purposes. > > 1) You cannot read the PHY ID register during MDIO bus enumeration, > generally because you need to turn on GPIOs, clocks, regulators etc, > which the MDIO/PHY core does not know how to do. > > 2) The PHY has bad values in its ID registers, typically because the > manufactures messed up. > > If you have a compatible like this, the ID registers are totally > ignored by Linux, and the ID is used to find the driver and tell the > driver exactly which of the multiple devices it supports it should > assume the device is. > > So you should use this from of compatible with care. You can easily > end up thinking you have a different PHY to what you actually have, > which could then result in wrong erratas being applied etc, or even > the wrong driver being used. > Right. I checked and this compatible was added with 280c17df8fbf ("arm64: dts: meson: g12a: add mdio multiplexer"). compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; The commit message doesn't explain why overriding the PHY ID is needed. Maybe Jerome as author can shed some light on it. At least on my system it's not needed (after setting the PHY ID in the PHY driver to the actual value). Would be interesting to know whether PHY is still detected and driver loaded with this patch on g12 systems. If the genphy driver should be used then the actual PHY ID would be interesting (look for attribute phy_id in sysfs). diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi index 585dd70f6..8af48aff0 100644 --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi @@ -1695,8 +1695,7 @@ int_mdio: mdio@1 { #size-cells = <0>; internal_ephy: ethernet_phy@8 { - compatible = "ethernet-phy-id0180.3301", - "ethernet-phy-ieee802.3-c22"; + compatible = "ethernet-phy-ieee802.3-c22"; interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>; reg = <8>; max-speed = <100>; diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c index c49062ad7..0fd76d49a 100644 --- a/drivers/net/phy/meson-gxl.c +++ b/drivers/net/phy/meson-gxl.c @@ -262,7 +262,7 @@ static struct phy_driver meson_gxl_phy[] = { .suspend = genphy_suspend, .resume = genphy_resume, }, { - PHY_ID_MATCH_EXACT(0x01803301), + PHY_ID_MATCH_EXACT(0x01803300), .name = "Meson G12A Internal PHY", /* PHY_BASIC_FEATURES */ .flags = PHY_IS_INTERNAL,
On 15.01.2023 21:38, Heiner Kallweit wrote: > On 15.01.2023 19:43, Neil Armstrong wrote: >> Hi Heiner, >> >> Le 15/01/2023 à 18:09, Heiner Kallweit a écrit : >>> On 15.01.2023 17:57, Andrew Lunn wrote: >>>> On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: >>>>> On my SC2-based system the genphy driver was used because the PHY >>>>> identifies as 0x01803300. It works normal with the meson g12a >>>>> driver after this change. >>>>> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. >>>> >>>> Hi Heiner >>>> >>>> Are there any datasheets for these devices? Anything which documents >>>> the lower nibble really is a revision? >>>> >>>> I'm just trying to avoid future problems where we find it is actually >>>> a different PHY, needs its own MATCH_EXACT entry, and then we find we >>>> break devices using 0x01803302 which we had no idea exists, but got >>>> covered by this change. >>>> >>> The SC2 platform inherited a lot from G12A, therefore it's plausible >>> that it's the same PHY. Also the vendor driver for SC2 gives a hint >>> as it has the following compatible for the PHY: >>> >>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >>> >>> But you're right, I can't say for sure as I don't have the datasheets. >> >> On G12A (& GXL), the PHY ID is set in the MDIO MUX registers, >> please see: >> https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36 >> >> So you should either add support for the PHY mux in SC2 or check >> what is in the ETH_PHY_CNTL0 register. >> > Thanks for the hint. I just checked and reading back ETH_PHY_CNTL0 at the > end of g12a_enable_internal_mdio() gives me the expected result of 0x33010180. > But still the PHY reports 3300. > Even if I write some other random value to ETH_PHY_CNTL0, I get 0180/3300 > as PHY ID. > > For u-boot I found the following: > > https://github.com/khadas/u-boot/blob/khadas-vim4-r-64bit/drivers/net/phy/amlogic.c > > static struct phy_driver amlogic_internal_driver = { > .name = "Meson GXL Internal PHY", > .uid = 0x01803300, > .mask = 0xfffffff0, > .features = PHY_BASIC_FEATURES, > .config = &meson_phy_config, > .startup = &meson_aml_startup, > .shutdown = &genphy_shutdown, > }; > > So it's the same PHY ID I'm seeing in Linux. > > My best guess is that the following is the case: > > The PHY compatible string in DT is the following in all cases: > compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; > > Therefore id 0180/3301 is used even if the PHY reports something else. > Means it doesn't matter which value you write to ETH_PHY_CNTL0. > > I reduced the compatible string to compatible = "ethernet-phy-ieee802.3-c22" > and this resulted in the actual PHY ID being used. > You could change the compatible in dts the same way for any g12a system > and I assume you would get 0180/3300 too. > > Remaining question is why the value in ETH_PHY_CNTL0 is ignored. > I think I found what's going on. The PHY ID written to SoC register ETH_PHY_CNTL0 isn't effective immediately. It takes a PHY soft reset before it reports the new PHY ID. Would be good to have a comment in the g12a mdio mux code mentioning this constraint. I see no easy way to trigger a soft reset early enough. Therefore it's indeed the simplest option to specify the new PHY ID in the compatible.
On Tue 17 Jan 2023 at 15:51, Heiner Kallweit <hkallweit1@gmail.com> wrote: > On 17.01.2023 14:30, Andrew Lunn wrote: >>>> The PHY compatible string in DT is the following in all cases: >>>> compatible = "ethernet-phy-id0180.3301" >> >> This form of compatible has two purposes. >> >> 1) You cannot read the PHY ID register during MDIO bus enumeration, >> generally because you need to turn on GPIOs, clocks, regulators etc, >> which the MDIO/PHY core does not know how to do. >> >> 2) The PHY has bad values in its ID registers, typically because the >> manufactures messed up. >> >> If you have a compatible like this, the ID registers are totally >> ignored by Linux, and the ID is used to find the driver and tell the >> driver exactly which of the multiple devices it supports it should >> assume the device is. >> >> So you should use this from of compatible with care. You can easily >> end up thinking you have a different PHY to what you actually have, >> which could then result in wrong erratas being applied etc, or even >> the wrong driver being used. >> > > Right. I checked and this compatible was added with > 280c17df8fbf ("arm64: dts: meson: g12a: add mdio multiplexer"). > > compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; > > The commit message doesn't explain why overriding the PHY ID > is needed. Maybe Jerome as author can shed some light on it. Guilty ... I'm afraid git a far better memory than I do. There is no reason for this compatible. It works without it (as explained on other threads) It was a mistake to add it in the first place. Probably a stupid copy/paste. It can (and should) be removed. > > At least on my system it's not needed (after setting the PHY ID > in the PHY driver to the actual value). > > Would be interesting to know whether PHY is still detected > and driver loaded with this patch on g12 systems. > If the genphy driver should be used then the actual PHY ID > would be interesting (look for attribute phy_id in sysfs). > > diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > index 585dd70f6..8af48aff0 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > @@ -1695,8 +1695,7 @@ int_mdio: mdio@1 { > #size-cells = <0>; > > internal_ephy: ethernet_phy@8 { > - compatible = "ethernet-phy-id0180.3301", > - "ethernet-phy-ieee802.3-c22"; > + compatible = "ethernet-phy-ieee802.3-c22"; > interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>; > reg = <8>; > max-speed = <100>; > diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c > index c49062ad7..0fd76d49a 100644 > --- a/drivers/net/phy/meson-gxl.c > +++ b/drivers/net/phy/meson-gxl.c > @@ -262,7 +262,7 @@ static struct phy_driver meson_gxl_phy[] = { > .suspend = genphy_suspend, > .resume = genphy_resume, > }, { > - PHY_ID_MATCH_EXACT(0x01803301), > + PHY_ID_MATCH_EXACT(0x01803300), > .name = "Meson G12A Internal PHY", > /* PHY_BASIC_FEATURES */ > .flags = PHY_IS_INTERNAL,
On Thu 19 Jan 2023 at 23:42, Heiner Kallweit <hkallweit1@gmail.com> wrote: > On 15.01.2023 21:38, Heiner Kallweit wrote: >> On 15.01.2023 19:43, Neil Armstrong wrote: >>> Hi Heiner, >>> >>> Le 15/01/2023 à 18:09, Heiner Kallweit a écrit : >>>> On 15.01.2023 17:57, Andrew Lunn wrote: >>>>> On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: >>>>>> On my SC2-based system the genphy driver was used because the PHY >>>>>> identifies as 0x01803300. It works normal with the meson g12a >>>>>> driver after this change. >>>>>> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. >>>>> >>>>> Hi Heiner >>>>> >>>>> Are there any datasheets for these devices? Anything which documents >>>>> the lower nibble really is a revision? >>>>> >>>>> I'm just trying to avoid future problems where we find it is actually >>>>> a different PHY, needs its own MATCH_EXACT entry, and then we find we >>>>> break devices using 0x01803302 which we had no idea exists, but got >>>>> covered by this change. >>>>> >>>> The SC2 platform inherited a lot from G12A, therefore it's plausible >>>> that it's the same PHY. Also the vendor driver for SC2 gives a hint >>>> as it has the following compatible for the PHY: >>>> >>>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >>>> >>>> But you're right, I can't say for sure as I don't have the datasheets. >>> >>> On G12A (& GXL), the PHY ID is set in the MDIO MUX registers, >>> please see: >>> https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36 >>> >>> So you should either add support for the PHY mux in SC2 or check >>> what is in the ETH_PHY_CNTL0 register. >>> >> Thanks for the hint. I just checked and reading back ETH_PHY_CNTL0 at the >> end of g12a_enable_internal_mdio() gives me the expected result of 0x33010180. >> But still the PHY reports 3300. >> Even if I write some other random value to ETH_PHY_CNTL0, I get 0180/3300 >> as PHY ID. >> >> For u-boot I found the following: >> >> https://github.com/khadas/u-boot/blob/khadas-vim4-r-64bit/drivers/net/phy/amlogic.c >> >> static struct phy_driver amlogic_internal_driver = { >> .name = "Meson GXL Internal PHY", >> .uid = 0x01803300, >> .mask = 0xfffffff0, >> .features = PHY_BASIC_FEATURES, >> .config = &meson_phy_config, >> .startup = &meson_aml_startup, >> .shutdown = &genphy_shutdown, >> }; >> >> So it's the same PHY ID I'm seeing in Linux. >> >> My best guess is that the following is the case: >> >> The PHY compatible string in DT is the following in all cases: >> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >> >> Therefore id 0180/3301 is used even if the PHY reports something else. >> Means it doesn't matter which value you write to ETH_PHY_CNTL0. >> >> I reduced the compatible string to compatible = "ethernet-phy-ieee802.3-c22" >> and this resulted in the actual PHY ID being used. >> You could change the compatible in dts the same way for any g12a system >> and I assume you would get 0180/3300 too. >> >> Remaining question is why the value in ETH_PHY_CNTL0 is ignored. >> > > I think I found what's going on. The PHY ID written to SoC register > ETH_PHY_CNTL0 isn't effective immediately. It takes a PHY soft reset before > it reports the new PHY ID. Would be good to have a comment in the > g12a mdio mux code mentioning this constraint. > > I see no easy way to trigger a soft reset early enough. Therefore it's indeed > the simplest option to specify the new PHY ID in the compatible. This is because (I guess) the PHY only picks ups the ID from the glue when it powers up. After that the values are ignored. Remember the PHY is a "bought" IP, the glue/mux provides the input settings required by the PHY provider. Best would be to trigger an HW reset of PHY from glue after setting the register ETH_PHY_CNTL0. Maybe this patch could help : ? https://gitlab.com/jbrunet/linux/-/commit/ccbb07b0c9eb2de26818eb4f8aa1fd0e5b31e6db.patch I tried this when we debugged the connectivity issue on the g12 earlier this spring. I did not send it because the problem was found to be in stmmac.
On 20.01.2023 11:01, Jerome Brunet wrote: > > On Thu 19 Jan 2023 at 23:42, Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> On 15.01.2023 21:38, Heiner Kallweit wrote: >>> On 15.01.2023 19:43, Neil Armstrong wrote: >>>> Hi Heiner, >>>> >>>> Le 15/01/2023 à 18:09, Heiner Kallweit a écrit : >>>>> On 15.01.2023 17:57, Andrew Lunn wrote: >>>>>> On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: >>>>>>> On my SC2-based system the genphy driver was used because the PHY >>>>>>> identifies as 0x01803300. It works normal with the meson g12a >>>>>>> driver after this change. >>>>>>> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. >>>>>> >>>>>> Hi Heiner >>>>>> >>>>>> Are there any datasheets for these devices? Anything which documents >>>>>> the lower nibble really is a revision? >>>>>> >>>>>> I'm just trying to avoid future problems where we find it is actually >>>>>> a different PHY, needs its own MATCH_EXACT entry, and then we find we >>>>>> break devices using 0x01803302 which we had no idea exists, but got >>>>>> covered by this change. >>>>>> >>>>> The SC2 platform inherited a lot from G12A, therefore it's plausible >>>>> that it's the same PHY. Also the vendor driver for SC2 gives a hint >>>>> as it has the following compatible for the PHY: >>>>> >>>>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >>>>> >>>>> But you're right, I can't say for sure as I don't have the datasheets. >>>> >>>> On G12A (& GXL), the PHY ID is set in the MDIO MUX registers, >>>> please see: >>>> https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36 >>>> >>>> So you should either add support for the PHY mux in SC2 or check >>>> what is in the ETH_PHY_CNTL0 register. >>>> >>> Thanks for the hint. I just checked and reading back ETH_PHY_CNTL0 at the >>> end of g12a_enable_internal_mdio() gives me the expected result of 0x33010180. >>> But still the PHY reports 3300. >>> Even if I write some other random value to ETH_PHY_CNTL0, I get 0180/3300 >>> as PHY ID. >>> >>> For u-boot I found the following: >>> >>> https://github.com/khadas/u-boot/blob/khadas-vim4-r-64bit/drivers/net/phy/amlogic.c >>> >>> static struct phy_driver amlogic_internal_driver = { >>> .name = "Meson GXL Internal PHY", >>> .uid = 0x01803300, >>> .mask = 0xfffffff0, >>> .features = PHY_BASIC_FEATURES, >>> .config = &meson_phy_config, >>> .startup = &meson_aml_startup, >>> .shutdown = &genphy_shutdown, >>> }; >>> >>> So it's the same PHY ID I'm seeing in Linux. >>> >>> My best guess is that the following is the case: >>> >>> The PHY compatible string in DT is the following in all cases: >>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >>> >>> Therefore id 0180/3301 is used even if the PHY reports something else. >>> Means it doesn't matter which value you write to ETH_PHY_CNTL0. >>> >>> I reduced the compatible string to compatible = "ethernet-phy-ieee802.3-c22" >>> and this resulted in the actual PHY ID being used. >>> You could change the compatible in dts the same way for any g12a system >>> and I assume you would get 0180/3300 too. >>> >>> Remaining question is why the value in ETH_PHY_CNTL0 is ignored. >>> >> >> I think I found what's going on. The PHY ID written to SoC register >> ETH_PHY_CNTL0 isn't effective immediately. It takes a PHY soft reset before >> it reports the new PHY ID. Would be good to have a comment in the >> g12a mdio mux code mentioning this constraint. >> >> I see no easy way to trigger a soft reset early enough. Therefore it's indeed >> the simplest option to specify the new PHY ID in the compatible. > > This is because (I guess) the PHY only picks ups the ID from the glue > when it powers up. After that the values are ignored. > I tested and a PHY soft reset is also sufficient to pick up the new PHY ID. Supposedly everything executing the soft reset logic is sufficient: power-up, soft reset, coming out of suspend/power-down > Remember the PHY is a "bought" IP, the glue/mux provides the input > settings required by the PHY provider. > > Best would be to trigger an HW reset of PHY from glue after setting the > register ETH_PHY_CNTL0. > > Maybe this patch could help : ? > https://gitlab.com/jbrunet/linux/-/commit/ccbb07b0c9eb2de26818eb4f8aa1fd0e5b31e6db.patch > Thanks for the hint, I'll look at it and test. > I tried this when we debugged the connectivity issue on the g12 earlier > this spring. I did not send it because the problem was found to be in > stmmac. >
On 20.01.2023 11:22, Heiner Kallweit wrote: > On 20.01.2023 11:01, Jerome Brunet wrote: >> >> On Thu 19 Jan 2023 at 23:42, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >>> On 15.01.2023 21:38, Heiner Kallweit wrote: >>>> On 15.01.2023 19:43, Neil Armstrong wrote: >>>>> Hi Heiner, >>>>> >>>>> Le 15/01/2023 à 18:09, Heiner Kallweit a écrit : >>>>>> On 15.01.2023 17:57, Andrew Lunn wrote: >>>>>>> On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: >>>>>>>> On my SC2-based system the genphy driver was used because the PHY >>>>>>>> identifies as 0x01803300. It works normal with the meson g12a >>>>>>>> driver after this change. >>>>>>>> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. >>>>>>> >>>>>>> Hi Heiner >>>>>>> >>>>>>> Are there any datasheets for these devices? Anything which documents >>>>>>> the lower nibble really is a revision? >>>>>>> >>>>>>> I'm just trying to avoid future problems where we find it is actually >>>>>>> a different PHY, needs its own MATCH_EXACT entry, and then we find we >>>>>>> break devices using 0x01803302 which we had no idea exists, but got >>>>>>> covered by this change. >>>>>>> >>>>>> The SC2 platform inherited a lot from G12A, therefore it's plausible >>>>>> that it's the same PHY. Also the vendor driver for SC2 gives a hint >>>>>> as it has the following compatible for the PHY: >>>>>> >>>>>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >>>>>> >>>>>> But you're right, I can't say for sure as I don't have the datasheets. >>>>> >>>>> On G12A (& GXL), the PHY ID is set in the MDIO MUX registers, >>>>> please see: >>>>> https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36 >>>>> >>>>> So you should either add support for the PHY mux in SC2 or check >>>>> what is in the ETH_PHY_CNTL0 register. >>>>> >>>> Thanks for the hint. I just checked and reading back ETH_PHY_CNTL0 at the >>>> end of g12a_enable_internal_mdio() gives me the expected result of 0x33010180. >>>> But still the PHY reports 3300. >>>> Even if I write some other random value to ETH_PHY_CNTL0, I get 0180/3300 >>>> as PHY ID. >>>> >>>> For u-boot I found the following: >>>> >>>> https://github.com/khadas/u-boot/blob/khadas-vim4-r-64bit/drivers/net/phy/amlogic.c >>>> >>>> static struct phy_driver amlogic_internal_driver = { >>>> .name = "Meson GXL Internal PHY", >>>> .uid = 0x01803300, >>>> .mask = 0xfffffff0, >>>> .features = PHY_BASIC_FEATURES, >>>> .config = &meson_phy_config, >>>> .startup = &meson_aml_startup, >>>> .shutdown = &genphy_shutdown, >>>> }; >>>> >>>> So it's the same PHY ID I'm seeing in Linux. >>>> >>>> My best guess is that the following is the case: >>>> >>>> The PHY compatible string in DT is the following in all cases: >>>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >>>> >>>> Therefore id 0180/3301 is used even if the PHY reports something else. >>>> Means it doesn't matter which value you write to ETH_PHY_CNTL0. >>>> >>>> I reduced the compatible string to compatible = "ethernet-phy-ieee802.3-c22" >>>> and this resulted in the actual PHY ID being used. >>>> You could change the compatible in dts the same way for any g12a system >>>> and I assume you would get 0180/3300 too. >>>> >>>> Remaining question is why the value in ETH_PHY_CNTL0 is ignored. >>>> >>> >>> I think I found what's going on. The PHY ID written to SoC register >>> ETH_PHY_CNTL0 isn't effective immediately. It takes a PHY soft reset before >>> it reports the new PHY ID. Would be good to have a comment in the >>> g12a mdio mux code mentioning this constraint. >>> >>> I see no easy way to trigger a soft reset early enough. Therefore it's indeed >>> the simplest option to specify the new PHY ID in the compatible. >> >> This is because (I guess) the PHY only picks ups the ID from the glue >> when it powers up. After that the values are ignored. >> > I tested and a PHY soft reset is also sufficient to pick up the new PHY ID. > Supposedly everything executing the soft reset logic is sufficient: > power-up, soft reset, coming out of suspend/power-down > > >> Remember the PHY is a "bought" IP, the glue/mux provides the input >> settings required by the PHY provider. >> >> Best would be to trigger an HW reset of PHY from glue after setting the >> register ETH_PHY_CNTL0. >> >> Maybe this patch could help : ? >> https://gitlab.com/jbrunet/linux/-/commit/ccbb07b0c9eb2de26818eb4f8aa1fd0e5b31e6db.patch >> > Thanks for the hint, I'll look at it and test. > >> I tried this when we debugged the connectivity issue on the g12 earlier >> this spring. I did not send it because the problem was found to be in >> stmmac. >> > I tested the patch with a slight modification. Maybe the PHY picks up other settings also on power-up/soft-reset only. Therefore I moved setting ETH_PHY_CNTL2 to before powering up the PHY. Do you think that's needed/better? With this patch the new PHY ID is effective early enough and the right PHY driver is loaded also w/o overriding the PHY ID with a compatible. Based on which version of the patch you prefer, are you going to submit it? Else I can do it too, just let me know. diff --git a/drivers/net/mdio/mdio-mux-meson-g12a.c b/drivers/net/mdio/mdio-mux-meson-g12a.c index 9d21fdf85..7882dcce2 100644 --- a/drivers/net/mdio/mdio-mux-meson-g12a.c +++ b/drivers/net/mdio/mdio-mux-meson-g12a.c @@ -6,6 +6,7 @@ #include <linux/bitfield.h> #include <linux/clk.h> #include <linux/clk-provider.h> +#include <linux/delay.h> #include <linux/device.h> #include <linux/io.h> #include <linux/iopoll.h> @@ -148,6 +149,7 @@ static const struct clk_ops g12a_ephy_pll_ops = { static int g12a_enable_internal_mdio(struct g12a_mdio_mux *priv) { + u32 val; int ret; /* Enable the phy clock */ @@ -159,17 +161,19 @@ static int g12a_enable_internal_mdio(struct g12a_mdio_mux *priv) /* Initialize ephy control */ writel(EPHY_G12A_ID, priv->regs + ETH_PHY_CNTL0); - writel(FIELD_PREP(PHY_CNTL1_ST_MODE, 3) | - FIELD_PREP(PHY_CNTL1_ST_PHYADD, EPHY_DFLT_ADD) | - FIELD_PREP(PHY_CNTL1_MII_MODE, EPHY_MODE_RMII) | - PHY_CNTL1_CLK_EN | - PHY_CNTL1_CLKFREQ | - PHY_CNTL1_PHY_ENB, - priv->regs + ETH_PHY_CNTL1); writel(PHY_CNTL2_USE_INTERNAL | PHY_CNTL2_SMI_SRC_MAC | PHY_CNTL2_RX_CLK_EPHY, priv->regs + ETH_PHY_CNTL2); + val = FIELD_PREP(PHY_CNTL1_ST_MODE, 3) | + FIELD_PREP(PHY_CNTL1_ST_PHYADD, EPHY_DFLT_ADD) | + FIELD_PREP(PHY_CNTL1_MII_MODE, EPHY_MODE_RMII) | + PHY_CNTL1_CLK_EN | + PHY_CNTL1_CLKFREQ; + writel(val, priv->regs + ETH_PHY_CNTL1); + val |= PHY_CNTL1_PHY_ENB; + writel(val, priv->regs + ETH_PHY_CNTL1); + mdelay(10); return 0; }
On Fri 20 Jan 2023 at 11:52, Heiner Kallweit <hkallweit1@gmail.com> wrote: > On 20.01.2023 11:22, Heiner Kallweit wrote: >> On 20.01.2023 11:01, Jerome Brunet wrote: >>> >>> On Thu 19 Jan 2023 at 23:42, Heiner Kallweit <hkallweit1@gmail.com> wrote: >>> >>>> On 15.01.2023 21:38, Heiner Kallweit wrote: >>>>> On 15.01.2023 19:43, Neil Armstrong wrote: >>>>>> Hi Heiner, >>>>>> >>>>>> Le 15/01/2023 à 18:09, Heiner Kallweit a écrit : >>>>>>> On 15.01.2023 17:57, Andrew Lunn wrote: >>>>>>>> On Sun, Jan 15, 2023 at 04:19:37PM +0100, Heiner Kallweit wrote: >>>>>>>>> On my SC2-based system the genphy driver was used because the PHY >>>>>>>>> identifies as 0x01803300. It works normal with the meson g12a >>>>>>>>> driver after this change. >>>>>>>>> Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. >>>>>>>> >>>>>>>> Hi Heiner >>>>>>>> >>>>>>>> Are there any datasheets for these devices? Anything which documents >>>>>>>> the lower nibble really is a revision? >>>>>>>> >>>>>>>> I'm just trying to avoid future problems where we find it is actually >>>>>>>> a different PHY, needs its own MATCH_EXACT entry, and then we find we >>>>>>>> break devices using 0x01803302 which we had no idea exists, but got >>>>>>>> covered by this change. >>>>>>>> >>>>>>> The SC2 platform inherited a lot from G12A, therefore it's plausible >>>>>>> that it's the same PHY. Also the vendor driver for SC2 gives a hint >>>>>>> as it has the following compatible for the PHY: >>>>>>> >>>>>>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >>>>>>> >>>>>>> But you're right, I can't say for sure as I don't have the datasheets. >>>>>> >>>>>> On G12A (& GXL), the PHY ID is set in the MDIO MUX registers, >>>>>> please see: >>>>>> https://elixir.bootlin.com/linux/latest/source/drivers/net/mdio/mdio-mux-meson-g12a.c#L36 >>>>>> >>>>>> So you should either add support for the PHY mux in SC2 or check >>>>>> what is in the ETH_PHY_CNTL0 register. >>>>>> >>>>> Thanks for the hint. I just checked and reading back ETH_PHY_CNTL0 at the >>>>> end of g12a_enable_internal_mdio() gives me the expected result of 0x33010180. >>>>> But still the PHY reports 3300. >>>>> Even if I write some other random value to ETH_PHY_CNTL0, I get 0180/3300 >>>>> as PHY ID. >>>>> >>>>> For u-boot I found the following: >>>>> >>>>> https://github.com/khadas/u-boot/blob/khadas-vim4-r-64bit/drivers/net/phy/amlogic.c >>>>> >>>>> static struct phy_driver amlogic_internal_driver = { >>>>> .name = "Meson GXL Internal PHY", >>>>> .uid = 0x01803300, >>>>> .mask = 0xfffffff0, >>>>> .features = PHY_BASIC_FEATURES, >>>>> .config = &meson_phy_config, >>>>> .startup = &meson_aml_startup, >>>>> .shutdown = &genphy_shutdown, >>>>> }; >>>>> >>>>> So it's the same PHY ID I'm seeing in Linux. >>>>> >>>>> My best guess is that the following is the case: >>>>> >>>>> The PHY compatible string in DT is the following in all cases: >>>>> compatible = "ethernet-phy-id0180.3301", "ethernet-phy-ieee802.3-c22"; >>>>> >>>>> Therefore id 0180/3301 is used even if the PHY reports something else. >>>>> Means it doesn't matter which value you write to ETH_PHY_CNTL0. >>>>> >>>>> I reduced the compatible string to compatible = "ethernet-phy-ieee802.3-c22" >>>>> and this resulted in the actual PHY ID being used. >>>>> You could change the compatible in dts the same way for any g12a system >>>>> and I assume you would get 0180/3300 too. >>>>> >>>>> Remaining question is why the value in ETH_PHY_CNTL0 is ignored. >>>>> >>>> >>>> I think I found what's going on. The PHY ID written to SoC register >>>> ETH_PHY_CNTL0 isn't effective immediately. It takes a PHY soft reset before >>>> it reports the new PHY ID. Would be good to have a comment in the >>>> g12a mdio mux code mentioning this constraint. >>>> >>>> I see no easy way to trigger a soft reset early enough. Therefore it's indeed >>>> the simplest option to specify the new PHY ID in the compatible. >>> >>> This is because (I guess) the PHY only picks ups the ID from the glue >>> when it powers up. After that the values are ignored. >>> >> I tested and a PHY soft reset is also sufficient to pick up the new PHY ID. >> Supposedly everything executing the soft reset logic is sufficient: >> power-up, soft reset, coming out of suspend/power-down >> >> >>> Remember the PHY is a "bought" IP, the glue/mux provides the input >>> settings required by the PHY provider. >>> >>> Best would be to trigger an HW reset of PHY from glue after setting the >>> register ETH_PHY_CNTL0. >>> >>> Maybe this patch could help : ? >>> https://gitlab.com/jbrunet/linux/-/commit/ccbb07b0c9eb2de26818eb4f8aa1fd0e5b31e6db.patch >>> >> Thanks for the hint, I'll look at it and test. >> >>> I tried this when we debugged the connectivity issue on the g12 earlier >>> this spring. I did not send it because the problem was found to be in >>> stmmac. >>> >> > > I tested the patch with a slight modification. Maybe the PHY picks up other > settings also on power-up/soft-reset only. Therefore I moved setting > ETH_PHY_CNTL2 to before powering up the PHY. Do you think that's needed/better? Something in between. The first write to CNTL1 powers off the PHY if it was on. I think I'll change CNTL2 while the PHY is down and then power it back up > > With this patch the new PHY ID is effective early enough and the right > PHY driver is loaded also w/o overriding the PHY ID with a compatible. > > Based on which version of the patch you prefer, are you going to submit it? > Else I can do it too, just let me know. Thanks for testing. I will submit it soon. > > > diff --git a/drivers/net/mdio/mdio-mux-meson-g12a.c b/drivers/net/mdio/mdio-mux-meson-g12a.c > index 9d21fdf85..7882dcce2 100644 > --- a/drivers/net/mdio/mdio-mux-meson-g12a.c > +++ b/drivers/net/mdio/mdio-mux-meson-g12a.c > @@ -6,6 +6,7 @@ > #include <linux/bitfield.h> > #include <linux/clk.h> > #include <linux/clk-provider.h> > +#include <linux/delay.h> > #include <linux/device.h> > #include <linux/io.h> > #include <linux/iopoll.h> > @@ -148,6 +149,7 @@ static const struct clk_ops g12a_ephy_pll_ops = { > > static int g12a_enable_internal_mdio(struct g12a_mdio_mux *priv) > { > + u32 val; > int ret; > > /* Enable the phy clock */ > @@ -159,17 +161,19 @@ static int g12a_enable_internal_mdio(struct g12a_mdio_mux *priv) > > /* Initialize ephy control */ > writel(EPHY_G12A_ID, priv->regs + ETH_PHY_CNTL0); > - writel(FIELD_PREP(PHY_CNTL1_ST_MODE, 3) | > - FIELD_PREP(PHY_CNTL1_ST_PHYADD, EPHY_DFLT_ADD) | > - FIELD_PREP(PHY_CNTL1_MII_MODE, EPHY_MODE_RMII) | > - PHY_CNTL1_CLK_EN | > - PHY_CNTL1_CLKFREQ | > - PHY_CNTL1_PHY_ENB, > - priv->regs + ETH_PHY_CNTL1); > writel(PHY_CNTL2_USE_INTERNAL | > PHY_CNTL2_SMI_SRC_MAC | > PHY_CNTL2_RX_CLK_EPHY, > priv->regs + ETH_PHY_CNTL2); > + val = FIELD_PREP(PHY_CNTL1_ST_MODE, 3) | > + FIELD_PREP(PHY_CNTL1_ST_PHYADD, EPHY_DFLT_ADD) | > + FIELD_PREP(PHY_CNTL1_MII_MODE, EPHY_MODE_RMII) | > + PHY_CNTL1_CLK_EN | > + PHY_CNTL1_CLKFREQ; > + writel(val, priv->regs + ETH_PHY_CNTL1); > + val |= PHY_CNTL1_PHY_ENB; > + writel(val, priv->regs + ETH_PHY_CNTL1); > + mdelay(10); > > return 0; > }
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c index c49062ad7..a36d471b8 100644 --- a/drivers/net/phy/meson-gxl.c +++ b/drivers/net/phy/meson-gxl.c @@ -262,7 +262,7 @@ static struct phy_driver meson_gxl_phy[] = { .suspend = genphy_suspend, .resume = genphy_resume, }, { - PHY_ID_MATCH_EXACT(0x01803301), + PHY_ID_MATCH_MODEL(0x01803301), .name = "Meson G12A Internal PHY", /* PHY_BASIC_FEATURES */ .flags = PHY_IS_INTERNAL,
On my SC2-based system the genphy driver was used because the PHY identifies as 0x01803300. It works normal with the meson g12a driver after this change. Switch to PHY_ID_MATCH_MODEL to cover the different sub-versions. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/net/phy/meson-gxl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)