[PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Heiner Kallweit]

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(-)

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,
-- 
2.39.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Andrew Lunn]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Heiner Kallweit]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Neil Armstrong]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Anand Moon]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Heiner Kallweit]

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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Paolo Abeni]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Andrew Lunn]

> > 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Heiner Kallweit]

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,
-- 
2.39.0

>     Andrew


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Jerome Brunet]

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,


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Heiner Kallweit]

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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Jerome Brunet]

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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Heiner Kallweit]

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.
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Heiner Kallweit]

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;
 }
-- 
2.39.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: phy: meson-gxl: support more G12A-internal PHY versions

[Jerome Brunet]

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;
>  }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH net-next] net: mdio: mux-meson-g12a: use devm_clk_get_enabled to simplify the code

[Heiner Kallweit]

Use devm_clk_get_enabled() to simplify the code.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/net/mdio/mdio-mux-meson-g12a.c | 27 ++++++--------------------
 1 file changed, 6 insertions(+), 21 deletions(-)

diff --git a/drivers/net/mdio/mdio-mux-meson-g12a.c b/drivers/net/mdio/mdio-mux-meson-g12a.c
index 4a2e94faf..1c1ed6e11 100644
--- a/drivers/net/mdio/mdio-mux-meson-g12a.c
+++ b/drivers/net/mdio/mdio-mux-meson-g12a.c
@@ -55,7 +55,6 @@ struct g12a_mdio_mux {
 	bool pll_is_enabled;
 	void __iomem *regs;
 	void *mux_handle;
-	struct clk *pclk;
 	struct clk *pll;
 };
 
@@ -302,6 +301,7 @@ static int g12a_mdio_mux_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct g12a_mdio_mux *priv;
+	struct clk *pclk;
 	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
@@ -314,34 +314,21 @@ static int g12a_mdio_mux_probe(struct platform_device *pdev)
 	if (IS_ERR(priv->regs))
 		return PTR_ERR(priv->regs);
 
-	priv->pclk = devm_clk_get(dev, "pclk");
-	if (IS_ERR(priv->pclk))
-		return dev_err_probe(dev, PTR_ERR(priv->pclk),
+	pclk = devm_clk_get_enabled(dev, "pclk");
+	if (IS_ERR(pclk))
+		return dev_err_probe(dev, PTR_ERR(pclk),
 				     "failed to get peripheral clock\n");
 
-	/* Make sure the device registers are clocked */
-	ret = clk_prepare_enable(priv->pclk);
-	if (ret) {
-		dev_err(dev, "failed to enable peripheral clock");
-		return ret;
-	}
-
 	/* Register PLL in CCF */
 	ret = g12a_ephy_glue_clk_register(dev);
 	if (ret)
-		goto err;
+		return ret;
 
 	ret = mdio_mux_init(dev, dev->of_node, g12a_mdio_switch_fn,
 			    &priv->mux_handle, dev, NULL);
-	if (ret) {
+	if (ret)
 		dev_err_probe(dev, ret, "mdio multiplexer init failed\n");
-		goto err;
-	}
 
-	return 0;
-
-err:
-	clk_disable_unprepare(priv->pclk);
 	return ret;
 }
 
@@ -354,8 +341,6 @@ static int g12a_mdio_mux_remove(struct platform_device *pdev)
 	if (priv->pll_is_enabled)
 		clk_disable_unprepare(priv->pll);
 
-	clk_disable_unprepare(priv->pclk);
-
 	return 0;
 }
 
-- 
2.39.0



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: mdio: mux-meson-g12a: use devm_clk_get_enabled to simplify the code

[Jerome Brunet]

On Thu 19 Jan 2023 at 23:56, Heiner Kallweit <hkallweit1@gmail.com> wrote:

> Use devm_clk_get_enabled() to simplify the code.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH net-next] net: mdio: mux-meson-g12a: use devm_clk_get_enabled to simplify the code

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu, 19 Jan 2023 23:56:37 +0100 you wrote:
> Use devm_clk_get_enabled() to simplify the code.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/mdio/mdio-mux-meson-g12a.c | 27 ++++++--------------------
>  1 file changed, 6 insertions(+), 21 deletions(-)

Here is the summary with links:
  - [net-next] net: mdio: mux-meson-g12a: use devm_clk_get_enabled to simplify the code
    https://git.kernel.org/netdev/net-next/c/32e54254bab8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel