[v2] net: mdio: mux-meson-gxl: set 28th bit in eth_reg2

Commit Message

Christian Hewitt March 31, 2025, 7:44 a.m. UTC

From: Da Xue <[e-mail]>

This bit is necessary to enable packets on the interface. Without this
bit set, ethernet behaves as if it is working, but no activity occurs.

The vendor SDK sets this bit along with the PHY_ID bits. U-boot also
sets this bit, but if u-boot is not compiled with networking support
the interface will not work.

Fixes: 9a24e1ff4326 ("net: mdio: add amlogic gxl mdio mux support");
Signed-off-by: Da Xue <[e-mail]>
Signed-off-by: Christian Hewitt <[e-mail]>
---
Resending on behalf of Da Xue who has email sending issues.
Changes since v1 [0]:
- Remove blank line between Fixes and SoB tags
- Submit without mail server mangling the patch
- Minor tweaks to subject line and commit message
- CC to [e-mail]

[0] https://patchwork.kernel.org/project/linux-amlogic/patch/CACqvRUbx-KsrMwCHYQS6eGXBohynD8Q1CQx=8=[e-mail]/

 drivers/net/mdio/mdio-mux-meson-gxl.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Neil Armstrong March 31, 2025, 1:22 p.m. UTC | #1

On 31/03/2025 09:44, Christian Hewitt wrote:
> From: Da Xue <[e-mail]>
> 
> This bit is necessary to enable packets on the interface. Without this
> bit set, ethernet behaves as if it is working, but no activity occurs.
> 
> The vendor SDK sets this bit along with the PHY_ID bits. U-boot also
> sets this bit, but if u-boot is not compiled with networking support
> the interface will not work.
> 
> Fixes: 9a24e1ff4326 ("net: mdio: add amlogic gxl mdio mux support");
> Signed-off-by: Da Xue <[e-mail]>
> Signed-off-by: Christian Hewitt <[e-mail]>
> ---
> Resending on behalf of Da Xue who has email sending issues.
> Changes since v1 [0]:
> - Remove blank line between Fixes and SoB tags
> - Submit without mail server mangling the patch
> - Minor tweaks to subject line and commit message
> - CC to [e-mail]
> 
> [0] https://patchwork.kernel.org/project/linux-amlogic/patch/CACqvRUbx-KsrMwCHYQS6eGXBohynD8Q1CQx=8=[e-mail]/
> 
>   drivers/net/mdio/mdio-mux-meson-gxl.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
> index 00c66240136b..fc5883387718 100644
> --- a/drivers/net/mdio/mdio-mux-meson-gxl.c
> +++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
> @@ -17,6 +17,7 @@
>   #define  REG2_LEDACT		GENMASK(23, 22)
>   #define  REG2_LEDLINK		GENMASK(25, 24)
>   #define  REG2_DIV4SEL		BIT(27)
> +#define  REG2_RESERVED_28	BIT(28)
>   #define  REG2_ADCBYPASS		BIT(30)
>   #define  REG2_CLKINSEL		BIT(31)
>   #define ETH_REG3		0x4
> @@ -65,7 +66,7 @@ static void gxl_enable_internal_mdio(struct gxl_mdio_mux *priv)
>   	 * The only constraint is that it must match the one in
>   	 * drivers/net/phy/meson-gxl.c to properly match the PHY.
>   	 */
> -	writel(FIELD_PREP(REG2_PHYID, EPHY_GXL_ID),
> +	writel(REG2_RESERVED_28 | FIELD_PREP(REG2_PHYID, EPHY_GXL_ID),
>   	       priv->regs + ETH_REG2);
>   
>   	/* Enable the internal phy */

Reviewed-by: Neil Armstrong <[e-mail]>

Andrew Lunn March 31, 2025, 1:43 p.m. UTC | #2

On Mon, Mar 31, 2025 at 07:44:20AM +0000, Christian Hewitt wrote:
> From: Da Xue <[e-mail]>
> 
> This bit is necessary to enable packets on the interface. Without this
> bit set, ethernet behaves as if it is working, but no activity occurs.
> 
> The vendor SDK sets this bit along with the PHY_ID bits. U-boot also
> sets this bit, but if u-boot is not compiled with networking support
> the interface will not work.
> 
> Fixes: 9a24e1ff4326 ("net: mdio: add amlogic gxl mdio mux support");
> Signed-off-by: Da Xue <[e-mail]>
> Signed-off-by: Christian Hewitt <[e-mail]>
> ---
> Resending on behalf of Da Xue who has email sending issues.
> Changes since v1 [0]:
> - Remove blank line between Fixes and SoB tags
> - Submit without mail server mangling the patch
> - Minor tweaks to subject line and commit message
> - CC to [e-mail]
> 
> [0] https://patchwork.kernel.org/project/linux-amlogic/patch/CACqvRUbx-KsrMwCHYQS6eGXBohynD8Q1CQx=8=[e-mail]/
> 
>  drivers/net/mdio/mdio-mux-meson-gxl.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
> index 00c66240136b..fc5883387718 100644
> --- a/drivers/net/mdio/mdio-mux-meson-gxl.c
> +++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
> @@ -17,6 +17,7 @@
>  #define  REG2_LEDACT		GENMASK(23, 22)
>  #define  REG2_LEDLINK		GENMASK(25, 24)
>  #define  REG2_DIV4SEL		BIT(27)
> +#define  REG2_RESERVED_28	BIT(28)

It must have some meaning, it cannot be reserved. So lets try to find
a better name.

	Andrew

Russell King (Oracle) March 31, 2025, 1:53 p.m. UTC | #3

On Mon, Mar 31, 2025 at 03:43:26PM +0200, Andrew Lunn wrote:
> On Mon, Mar 31, 2025 at 07:44:20AM +0000, Christian Hewitt wrote:
> > From: Da Xue <[e-mail]>
> > 
> > This bit is necessary to enable packets on the interface. Without this
> > bit set, ethernet behaves as if it is working, but no activity occurs.
> > 
> > The vendor SDK sets this bit along with the PHY_ID bits. U-boot also
> > sets this bit, but if u-boot is not compiled with networking support
> > the interface will not work.
> > 
> > Fixes: 9a24e1ff4326 ("net: mdio: add amlogic gxl mdio mux support");
> > Signed-off-by: Da Xue <[e-mail]>
> > Signed-off-by: Christian Hewitt <[e-mail]>
> > ---
> > Resending on behalf of Da Xue who has email sending issues.
> > Changes since v1 [0]:
> > - Remove blank line between Fixes and SoB tags
> > - Submit without mail server mangling the patch
> > - Minor tweaks to subject line and commit message
> > - CC to [e-mail]
> > 
> > [0] https://patchwork.kernel.org/project/linux-amlogic/patch/CACqvRUbx-KsrMwCHYQS6eGXBohynD8Q1CQx=8=[e-mail]/
> > 
> >  drivers/net/mdio/mdio-mux-meson-gxl.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
> > index 00c66240136b..fc5883387718 100644
> > --- a/drivers/net/mdio/mdio-mux-meson-gxl.c
> > +++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
> > @@ -17,6 +17,7 @@
> >  #define  REG2_LEDACT		GENMASK(23, 22)
> >  #define  REG2_LEDLINK		GENMASK(25, 24)
> >  #define  REG2_DIV4SEL		BIT(27)
> > +#define  REG2_RESERVED_28	BIT(28)
> 
> It must have some meaning, it cannot be reserved. So lets try to find
> a better name.

Indeed, that was my thoughts as well, but Andrew got his reply in
before I got around to replying!

Da Xue March 31, 2025, 7:09 p.m. UTC | #4

On Mon, Mar 31, 2025 at 9:55 AM Russell King (Oracle)
<[e-mail]> wrote:
>
> On Mon, Mar 31, 2025 at 03:43:26PM +0200, Andrew Lunn wrote:
> > On Mon, Mar 31, 2025 at 07:44:20AM +0000, Christian Hewitt wrote:
> > > From: Da Xue <[e-mail]>
> > >
> > > This bit is necessary to enable packets on the interface. Without this
> > > bit set, ethernet behaves as if it is working, but no activity occurs.
> > >
> > > The vendor SDK sets this bit along with the PHY_ID bits. U-boot also
> > > sets this bit, but if u-boot is not compiled with networking support
> > > the interface will not work.
> > >
> > > Fixes: 9a24e1ff4326 ("net: mdio: add amlogic gxl mdio mux support");
> > > Signed-off-by: Da Xue <[e-mail]>
> > > Signed-off-by: Christian Hewitt <[e-mail]>
> > > ---
> > > Resending on behalf of Da Xue who has email sending issues.
> > > Changes since v1 [0]:
> > > - Remove blank line between Fixes and SoB tags
> > > - Submit without mail server mangling the patch
> > > - Minor tweaks to subject line and commit message
> > > - CC to [e-mail]
> > >
> > > [0] https://patchwork.kernel.org/project/linux-amlogic/patch/CACqvRUbx-KsrMwCHYQS6eGXBohynD8Q1CQx=8=[e-mail]/
> > >
> > >  drivers/net/mdio/mdio-mux-meson-gxl.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
> > > index 00c66240136b..fc5883387718 100644
> > > --- a/drivers/net/mdio/mdio-mux-meson-gxl.c
> > > +++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
> > > @@ -17,6 +17,7 @@
> > >  #define  REG2_LEDACT               GENMASK(23, 22)
> > >  #define  REG2_LEDLINK              GENMASK(25, 24)
> > >  #define  REG2_DIV4SEL              BIT(27)
> > > +#define  REG2_RESERVED_28  BIT(28)
> >
> > It must have some meaning, it cannot be reserved. So lets try to find
> > a better name.
>
> Indeed, that was my thoughts as well, but Andrew got his reply in
> before I got around to replying!

The datasheets don't have much in the way of information about this
register bit. The Amlogic GXL datasheet is notoriously inaccurate.

ETH_REG2 0XC8834558
29:28 R 0x1 reserved

It claims the bit is read only while the BSP hard codes the setting of
this register. I am open to any name for this register bit.
This is the only thing holding up distro netbooting for these very
popular chip family.

>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
>
> _______________________________________________
> linux-amlogic mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Russell King (Oracle) March 31, 2025, 8:30 p.m. UTC | #5

On Mon, Mar 31, 2025 at 03:09:00PM -0400, Da Xue wrote:
> On Mon, Mar 31, 2025 at 9:55 AM Russell King (Oracle)
> <[e-mail]> wrote:
> >
> > On Mon, Mar 31, 2025 at 03:43:26PM +0200, Andrew Lunn wrote:
> > > On Mon, Mar 31, 2025 at 07:44:20AM +0000, Christian Hewitt wrote:
> > > > From: Da Xue <[e-mail]>
> > > >
> > > > This bit is necessary to enable packets on the interface. Without this
> > > > bit set, ethernet behaves as if it is working, but no activity occurs.
> > > >
> > > > The vendor SDK sets this bit along with the PHY_ID bits. U-boot also
> > > > sets this bit, but if u-boot is not compiled with networking support
> > > > the interface will not work.
> > > >
> > > > Fixes: 9a24e1ff4326 ("net: mdio: add amlogic gxl mdio mux support");
> > > > Signed-off-by: Da Xue <[e-mail]>
> > > > Signed-off-by: Christian Hewitt <[e-mail]>
> > > > ---
> > > > Resending on behalf of Da Xue who has email sending issues.
> > > > Changes since v1 [0]:
> > > > - Remove blank line between Fixes and SoB tags
> > > > - Submit without mail server mangling the patch
> > > > - Minor tweaks to subject line and commit message
> > > > - CC to [e-mail]
> > > >
> > > > [0] https://patchwork.kernel.org/project/linux-amlogic/patch/CACqvRUbx-KsrMwCHYQS6eGXBohynD8Q1CQx=8=[e-mail]/
> > > >
> > > >  drivers/net/mdio/mdio-mux-meson-gxl.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
> > > > index 00c66240136b..fc5883387718 100644
> > > > --- a/drivers/net/mdio/mdio-mux-meson-gxl.c
> > > > +++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
> > > > @@ -17,6 +17,7 @@
> > > >  #define  REG2_LEDACT               GENMASK(23, 22)
> > > >  #define  REG2_LEDLINK              GENMASK(25, 24)
> > > >  #define  REG2_DIV4SEL              BIT(27)
> > > > +#define  REG2_RESERVED_28  BIT(28)
> > >
> > > It must have some meaning, it cannot be reserved. So lets try to find
> > > a better name.
> >
> > Indeed, that was my thoughts as well, but Andrew got his reply in
> > before I got around to replying!
> 
> The datasheets don't have much in the way of information about this
> register bit. The Amlogic GXL datasheet is notoriously inaccurate.
> 
> ETH_REG2 0XC8834558
> 29:28 R 0x1 reserved
> 
> It claims the bit is read only while the BSP hard codes the setting of
> this register. I am open to any name for this register bit.
> This is the only thing holding up distro netbooting for these very
> popular chip family.

Which interface mode do we think this affects?

As a suggestion, maybe call it:

REG2_<interfacemode>_EN

and possibly add a comment "This bit is documented as reserved, but
needs to be set so that <interfacemode> can pass traffic. This is
an unofficial name."

Da Xue March 31, 2025, 9:21 p.m. UTC | #6

On Mon, Mar 31, 2025 at 4:30 PM Russell King (Oracle)
<[e-mail]> wrote:
>
> On Mon, Mar 31, 2025 at 03:09:00PM -0400, Da Xue wrote:
> > On Mon, Mar 31, 2025 at 9:55 AM Russell King (Oracle)
> > <[e-mail]> wrote:
> > >
> > > On Mon, Mar 31, 2025 at 03:43:26PM +0200, Andrew Lunn wrote:
> > > > On Mon, Mar 31, 2025 at 07:44:20AM +0000, Christian Hewitt wrote:
> > > > > From: Da Xue <[e-mail]>
> > > > >
> > > > > This bit is necessary to enable packets on the interface. Without this
> > > > > bit set, ethernet behaves as if it is working, but no activity occurs.
> > > > >
> > > > > The vendor SDK sets this bit along with the PHY_ID bits. U-boot also
> > > > > sets this bit, but if u-boot is not compiled with networking support
> > > > > the interface will not work.
> > > > >
> > > > > Fixes: 9a24e1ff4326 ("net: mdio: add amlogic gxl mdio mux support");
> > > > > Signed-off-by: Da Xue <[e-mail]>
> > > > > Signed-off-by: Christian Hewitt <[e-mail]>
> > > > > ---
> > > > > Resending on behalf of Da Xue who has email sending issues.
> > > > > Changes since v1 [0]:
> > > > > - Remove blank line between Fixes and SoB tags
> > > > > - Submit without mail server mangling the patch
> > > > > - Minor tweaks to subject line and commit message
> > > > > - CC to [e-mail]
> > > > >
> > > > > [0] https://patchwork.kernel.org/project/linux-amlogic/patch/CACqvRUbx-KsrMwCHYQS6eGXBohynD8Q1CQx=8=[e-mail]/
> > > > >
> > > > >  drivers/net/mdio/mdio-mux-meson-gxl.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
> > > > > index 00c66240136b..fc5883387718 100644
> > > > > --- a/drivers/net/mdio/mdio-mux-meson-gxl.c
> > > > > +++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
> > > > > @@ -17,6 +17,7 @@
> > > > >  #define  REG2_LEDACT               GENMASK(23, 22)
> > > > >  #define  REG2_LEDLINK              GENMASK(25, 24)
> > > > >  #define  REG2_DIV4SEL              BIT(27)
> > > > > +#define  REG2_RESERVED_28  BIT(28)
> > > >
> > > > It must have some meaning, it cannot be reserved. So lets try to find
> > > > a better name.
> > >
> > > Indeed, that was my thoughts as well, but Andrew got his reply in
> > > before I got around to replying!
> >
> > The datasheets don't have much in the way of information about this
> > register bit. The Amlogic GXL datasheet is notoriously inaccurate.
> >
> > ETH_REG2 0XC8834558
> > 29:28 R 0x1 reserved
> >
> > It claims the bit is read only while the BSP hard codes the setting of
> > this register. I am open to any name for this register bit.
> > This is the only thing holding up distro netbooting for these very
> > popular chip family.
>
> Which interface mode do we think this affects?
>
> As a suggestion, maybe call it:
>
> REG2_<interfacemode>_EN
>
> and possibly add a comment "This bit is documented as reserved, but
> needs to be set so that <interfacemode> can pass traffic. This is
> an unofficial name."

I found this on the zircon kernel:

#define REG2_ETH_REG2_REVERSED (1 << 28)

pregs->Write32(REG2_ETH_REG2_REVERSED | REG2_INTERNAL_PHY_ID, PER_ETH_REG2);

I can respin and call it that.

>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Russell King (Oracle) March 31, 2025, 11:59 p.m. UTC | #7

On Mon, Mar 31, 2025 at 05:21:08PM -0400, Da Xue wrote:
> I found this on the zircon kernel:
> 
> #define REG2_ETH_REG2_REVERSED (1 << 28)
> 
> pregs->Write32(REG2_ETH_REG2_REVERSED | REG2_INTERNAL_PHY_ID, PER_ETH_REG2);
> 
> I can respin and call it that.

Which interface mode is being used, and what is the MAC connected to?

"Reversed" seems to imply that _this_ end is acting as a PHY rather
than the MAC in the link, so I think a bit more information (the above)
is needed to ensure that this is the correct solution.

Da Xue April 1, 2025, 2:44 a.m. UTC | #8

On Mon, Mar 31, 2025 at 7:59 PM Russell King (Oracle)
<[e-mail]> wrote:
>
> On Mon, Mar 31, 2025 at 05:21:08PM -0400, Da Xue wrote:
> > I found this on the zircon kernel:
> >
> > #define REG2_ETH_REG2_REVERSED (1 << 28)
> >
> > pregs->Write32(REG2_ETH_REG2_REVERSED | REG2_INTERNAL_PHY_ID, PER_ETH_REG2);
> >
> > I can respin and call it that.
>
> Which interface mode is being used, and what is the MAC connected to?
>
> "Reversed" seems to imply that _this_ end is acting as a PHY rather
> than the MAC in the link, so I think a bit more information (the above)
> is needed to ensure that this is the correct solution.

The SoC can be connected to an external PHY or use the internal PHY.
In this gxl_enable_internal_mdio case, we are using the internal PHY.

Sorry about leaving this out in the last email and causing another RT.
I'm not very familiar with ethernet underpinnings so I don't want to
use the wrong terms.


>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Patch

diff --git a/drivers/net/mdio/mdio-mux-meson-gxl.c b/drivers/net/mdio/mdio-mux-meson-gxl.c
index 00c66240136b..fc5883387718 100644
--- a/drivers/net/mdio/mdio-mux-meson-gxl.c
+++ b/drivers/net/mdio/mdio-mux-meson-gxl.c
@@ -17,6 +17,7 @@ 
 #define  REG2_LEDACT		GENMASK(23, 22)
 #define  REG2_LEDLINK		GENMASK(25, 24)
 #define  REG2_DIV4SEL		BIT(27)
+#define  REG2_RESERVED_28	BIT(28)
 #define  REG2_ADCBYPASS		BIT(30)
 #define  REG2_CLKINSEL		BIT(31)
 #define ETH_REG3		0x4
@@ -65,7 +66,7 @@  static void gxl_enable_internal_mdio(struct gxl_mdio_mux *priv)
 	 * The only constraint is that it must match the one in
 	 * drivers/net/phy/meson-gxl.c to properly match the PHY.
 	 */
-	writel(FIELD_PREP(REG2_PHYID, EPHY_GXL_ID),
+	writel(REG2_RESERVED_28 | FIELD_PREP(REG2_PHYID, EPHY_GXL_ID),
 	       priv->regs + ETH_REG2);
 
 	/* Enable the internal phy */