[PATCH net-next] net: lan969x: correct RGMII port mapping index

[PATCH net-next] net: lan969x: correct RGMII port mapping index

[Robert Marko]

Currently, the lan969x_port_dev_mapping does not check for RGMII ports
and just returns the physical port index.

However, this does not work for RGMII ports as they have dedicated DEVRGMII
register space with an dedicated instance per RGMII port.

So, check if requested port index is an RGMII port and return the correct
DEVRGMII index.

Signed-off-by: Robert Marko <robert.marko@sartura.hr>
---
 drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
index f3a9c71bea36a..bf5320c7a6ca8 100644
--- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
+++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
@@ -142,6 +142,15 @@ static u32 lan969x_get_dev_mode_bit(struct sparx5 *sparx5, int port)
 
 static u32 lan969x_port_dev_mapping(struct sparx5 *sparx5, int port)
 {
+	if (lan969x_port_is_rgmii(port)) {
+		switch (port) {
+		case 28:
+			return 0;
+		case 29:
+			return 1;
+		}
+	}
+
 	if (lan969x_port_is_5g(port)) {
 		switch (port) {
 		case 9:
-- 
2.55.0

Re: [PATCH net-next] net: lan969x: correct RGMII port mapping index

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net: lan969x: the patch does not achieve its stated purpose —
  the new DEVRGMII instance index it returns is never paired with…

--

net: lan969x: correct RGMII port mapping index

This adds an RGMII case to lan969x_port_dev_mapping() so that ports 28 and
29 return DEVRGMII instance indices 0 and 1 instead of the physical port
number. The helper is installed as sparx5_ops::get_port_dev_index and is
read through sparx5_port_dev_index().

> diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> index f3a9c71bea36a..bf5320c7a6ca8 100644
> --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> @@ -142,6 +142,15 @@ static u32 lan969x_get_dev_mode_bit(struct sparx5 *sparx5, int port)
>  
>  static u32 lan969x_port_dev_mapping(struct sparx5 *sparx5, int port)
>  {
> +	if (lan969x_port_is_rgmii(port)) {
> +		switch (port) {
> +		case 28:
> +			return 0;
> +		case 29:
> +			return 1;
> +		}
> +	}
> +

[Severity: High]

Can this new index ever reach DEVRGMII register space?  The value returned
by lan969x_port_dev_mapping() is only read through sparx5_port_dev_index()
and is always combined with a target coming from sparx5_to_high_dev() or
sparx5_to_pcs_dev(), and neither of those has an RGMII case:

drivers/net/ethernet/microchip/sparx5/sparx5_port.h:
static inline u32 sparx5_to_high_dev(struct sparx5 *sparx5, int port)
{
	const struct sparx5_ops *ops = sparx5->data->ops;

	if (ops->is_port_5g(port))
		return TARGET_DEV5G;
	if (ops->is_port_10g(port))
		return TARGET_DEV10G;
	return TARGET_DEV25G;
}

So for lan969x ports 28 and 29 the target stays TARGET_DEV25G (or
TARGET_PCS25G_BR), never TARGET_DEVRGMII.

The base then comes from an unchecked array index:

drivers/net/ethernet/microchip/sparx5/sparx5_main.h:spx5_inst_get() {
	return sparx5->regs[id + tinst];
}

With the new index that is regs[TARGET_DEV25G + 0] and
regs[TARGET_DEV25G + 1].  The lan969x iomap in lan969x_main_iomap[] has no
TARGET_DEV25G or TARGET_PCS25G_BR entries at all, so both slots are NULL.
Before the patch, index 28 resolved to regs[TARGET_DEV25G + 28], which
lands on a mapped (though unrelated) DEV2G5 base.

Does that mean the commit message claim

    So, check if requested port index is an RGMII port and return the correct
    DEVRGMII index.

is not reflected in any code path, since no consumer pairs this index with
TARGET_DEVRGMII?

The real DEVRGMII and HSIO_WRAP accesses compute the instance locally and do
not use this helper:

drivers/net/ethernet/microchip/sparx5/lan969x/lan969x_rgmii.c:
#define RGMII_PORT_IDX(port) ((port)->portno - LAN969X_RGMII_PORT_START_IDX)

used by lan969x_rgmii_port_device_config(), lan969x_rgmii_tx_clk_config(),
lan969x_rgmii_delay_config() and lan969x_rgmii_gpio_config().  Should the
two sources of the same index be folded into one helper instead?

There is also a latent NULL __iomem base introduced here.  In
sparx5_port_init() the devinst and pcsinst pointers are computed for RGMII
ports too:

	devinst = spx5_inst_get(sparx5, devhigh, pix);
	pcsinst = spx5_inst_get(sparx5, pcs, pix);
	...
	if (ops->is_port_rgmii(port->portno))
		return 0; /* RGMII device - nothing more to configure */

They are now NULL rather than a mapped base, and only the is_port_rgmii()
early return keeps them from being dereferenced.  The other consumers of the
pair are gated by unrelated conditions today: sparx5_port_max_tags_set()
runs only after that early return, sparx5_port_disable() and
sparx5_port_pcs_high_set() are reached via sparx5_pcs_config() where
sparx5_phylink_mac_select_pcs() returns NULL for RGMII interface modes, and
the ethtool device stat paths are gated by
sparx5_is_baser(port->conf.portmode).  Would it be safer to also teach the
target selection side about RGMII, for instance returning TARGET_DEVRGMII
for is_port_rgmii() ports, so the index and the target stay consistent?

If this is intended as preparatory work for a later change, could the commit
message say so and name the dependent patch?  A Fixes: tag also seems to be
missing for the addressing bug the changelog describes.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260922125112.2111256-1-robert.marko%40sartura.hr

Re: [PATCH net-next] net: lan969x: correct RGMII port mapping index

[Daniel Machon]

Hi Robert,

> Currently, the lan969x_port_dev_mapping does not check for RGMII ports
> and just returns the physical port index.
> 
> However, this does not work for RGMII ports as they have dedicated DEVRGMII
> register space with an dedicated instance per RGMII port.
> 
> So, check if requested port index is an RGMII port and return the correct
> DEVRGMII index.
> 
> Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> ---
>  drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> index f3a9c71bea36a..bf5320c7a6ca8 100644
> --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> @@ -142,6 +142,15 @@ static u32 lan969x_get_dev_mode_bit(struct sparx5 *sparx5, int port)
> 
>  static u32 lan969x_port_dev_mapping(struct sparx5 *sparx5, int port)
>  {
> +       if (lan969x_port_is_rgmii(port)) {
> +               switch (port) {
> +               case 28:
> +                       return 0;
> +               case 29:
> +                       return 1;
> +               }
> +       }
> +
>         if (lan969x_port_is_5g(port)) {
>                 switch (port) {
>                 case 9:
> --
> 2.55.0
> 

The mapping itself is right, but as far as I can see nothing upstream
uses the returned index to access DEVRGMII. Every caller of
sparx5_port_dev_index() pairs it with sparx5_to_high_dev() or
sparx5_to_pcs_dev(), which never return TARGET_DEVRGMII. Those paths
are also never taken for RGMII ports. The code that does access
DEVRGMII, in lan969x_rgmii.c, computes its own index with
RGMII_PORT_IDX(). Sashiko, correctly points this out.

So I don't think anything is broken today. Did you see a problem on
hardware that this fixes?

FYI, we carry the same change downstream. But there it was added together with
RGMII MTU support, which reads and writes DEVRGMII_MAC_MAXLEN_CFG() using this
index.

/Daniel

Re: [PATCH net-next] net: lan969x: correct RGMII port mapping index

[Robert Marko]

On Mon, Sep 28, 2026 at 11:18 AM Daniel Machon
<daniel.machon@microchip.com> wrote:
>
> Hi Robert,
>
> > Currently, the lan969x_port_dev_mapping does not check for RGMII ports
> > and just returns the physical port index.
> >
> > However, this does not work for RGMII ports as they have dedicated DEVRGMII
> > register space with an dedicated instance per RGMII port.
> >
> > So, check if requested port index is an RGMII port and return the correct
> > DEVRGMII index.
> >
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > ---
> >  drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> > index f3a9c71bea36a..bf5320c7a6ca8 100644
> > --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> > +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> > @@ -142,6 +142,15 @@ static u32 lan969x_get_dev_mode_bit(struct sparx5 *sparx5, int port)
> >
> >  static u32 lan969x_port_dev_mapping(struct sparx5 *sparx5, int port)
> >  {
> > +       if (lan969x_port_is_rgmii(port)) {
> > +               switch (port) {
> > +               case 28:
> > +                       return 0;
> > +               case 29:
> > +                       return 1;
> > +               }
> > +       }
> > +
> >         if (lan969x_port_is_5g(port)) {
> >                 switch (port) {
> >                 case 9:
> > --
> > 2.55.0
> >
>
> The mapping itself is right, but as far as I can see nothing upstream
> uses the returned index to access DEVRGMII. Every caller of
> sparx5_port_dev_index() pairs it with sparx5_to_high_dev() or
> sparx5_to_pcs_dev(), which never return TARGET_DEVRGMII. Those paths
> are also never taken for RGMII ports. The code that does access
> DEVRGMII, in lan969x_rgmii.c, computes its own index with
> RGMII_PORT_IDX(). Sashiko, correctly points this out.
>
> So I don't think anything is broken today. Did you see a problem on
> hardware that this fixes?
>
> FYI, we carry the same change downstream. But there it was added together with
> RGMII MTU support, which reads and writes DEVRGMII_MAC_MAXLEN_CFG() using this
> index.

Hi Andrew,
As pointed out nothing yet is broken, hence no fixes tag.

I am using it downstream as well, for the MTU change support where as
you pointed out RGMII code
uses it to get the correct index.

So, I thought that it would be good idea to send it upstream before
eventually getting around to sending
the MTU change support as well.

Regards,
Robert

>
> /Daniel



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura d.d.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr

Re: [PATCH net-next] net: lan969x: correct RGMII port mapping index

[Daniel Machon]

Hi Robert,

> On Mon, Sep 28, 2026 at 11:18 AM Daniel Machon
> <daniel.machon@microchip.com> wrote:
> >
> > Hi Robert,
> >
> > > Currently, the lan969x_port_dev_mapping does not check for RGMII ports
> > > and just returns the physical port index.
> > >
> > > However, this does not work for RGMII ports as they have dedicated DEVRGMII
> > > register space with an dedicated instance per RGMII port.
> > >
> > > So, check if requested port index is an RGMII port and return the correct
> > > DEVRGMII index.
> > >
> > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > ---
> > >  drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> > > index f3a9c71bea36a..bf5320c7a6ca8 100644
> > > --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> > > +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> > > @@ -142,6 +142,15 @@ static u32 lan969x_get_dev_mode_bit(struct sparx5 *sparx5, int port)
> > >
> > >  static u32 lan969x_port_dev_mapping(struct sparx5 *sparx5, int port)
> > >  {
> > > +       if (lan969x_port_is_rgmii(port)) {
> > > +               switch (port) {
> > > +               case 28:
> > > +                       return 0;
> > > +               case 29:
> > > +                       return 1;
> > > +               }
> > > +       }
> > > +
> > >         if (lan969x_port_is_5g(port)) {
> > >                 switch (port) {
> > >                 case 9:
> > > --
> > > 2.55.0
> > >
> >
> > The mapping itself is right, but as far as I can see nothing upstream
> > uses the returned index to access DEVRGMII. Every caller of
> > sparx5_port_dev_index() pairs it with sparx5_to_high_dev() or
> > sparx5_to_pcs_dev(), which never return TARGET_DEVRGMII. Those paths
> > are also never taken for RGMII ports. The code that does access
> > DEVRGMII, in lan969x_rgmii.c, computes its own index with
> > RGMII_PORT_IDX(). Sashiko, correctly points this out.
> >
> > So I don't think anything is broken today. Did you see a problem on
> > hardware that this fixes?
> >
> > FYI, we carry the same change downstream. But there it was added together with
> > RGMII MTU support, which reads and writes DEVRGMII_MAC_MAXLEN_CFG() using this
> > index.
> 
> Hi Andrew,
> As pointed out nothing yet is broken, hence no fixes tag.
> 
> I am using it downstream as well, for the MTU change support where as
> you pointed out RGMII code
> uses it to get the correct index.
> 
> So, I thought that it would be good idea to send it upstream before
> eventually getting around to sending
> the MTU change support as well.

It wasn't clear to me that this was preparation work for a future feature. As a
standlone patch it does nothing. IDK, to me it would make more sense to just
send it together with the patchset that would actually use it.

/Daniel

> 
> Regards,
> Robert
> 
> >
> > /Daniel
> 
> 
> 
> --
> Robert Marko
> Staff Embedded Linux Engineer
> Sartura d.d.
> Lendavska ulica 16a
> 10000 Zagreb, Croatia
> Email: robert.marko@sartura.hr
> Web: www.sartura.hr

Re: [PATCH net-next] net: lan969x: correct RGMII port mapping index

[Robert Marko]

On Mon, Sep 28, 2026 at 1:04 PM Daniel Machon
<daniel.machon@microchip.com> wrote:
>
> Hi Robert,
>
> > On Mon, Sep 28, 2026 at 11:18 AM Daniel Machon
> > <daniel.machon@microchip.com> wrote:
> > >
> > > Hi Robert,
> > >
> > > > Currently, the lan969x_port_dev_mapping does not check for RGMII ports
> > > > and just returns the physical port index.
> > > >
> > > > However, this does not work for RGMII ports as they have dedicated DEVRGMII
> > > > register space with an dedicated instance per RGMII port.
> > > >
> > > > So, check if requested port index is an RGMII port and return the correct
> > > > DEVRGMII index.
> > > >
> > > > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > > > ---
> > > >  drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> > > > index f3a9c71bea36a..bf5320c7a6ca8 100644
> > > > --- a/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> > > > +++ b/drivers/net/ethernet/microchip/sparx5/lan969x/lan969x.c
> > > > @@ -142,6 +142,15 @@ static u32 lan969x_get_dev_mode_bit(struct sparx5 *sparx5, int port)
> > > >
> > > >  static u32 lan969x_port_dev_mapping(struct sparx5 *sparx5, int port)
> > > >  {
> > > > +       if (lan969x_port_is_rgmii(port)) {
> > > > +               switch (port) {
> > > > +               case 28:
> > > > +                       return 0;
> > > > +               case 29:
> > > > +                       return 1;
> > > > +               }
> > > > +       }
> > > > +
> > > >         if (lan969x_port_is_5g(port)) {
> > > >                 switch (port) {
> > > >                 case 9:
> > > > --
> > > > 2.55.0
> > > >
> > >
> > > The mapping itself is right, but as far as I can see nothing upstream
> > > uses the returned index to access DEVRGMII. Every caller of
> > > sparx5_port_dev_index() pairs it with sparx5_to_high_dev() or
> > > sparx5_to_pcs_dev(), which never return TARGET_DEVRGMII. Those paths
> > > are also never taken for RGMII ports. The code that does access
> > > DEVRGMII, in lan969x_rgmii.c, computes its own index with
> > > RGMII_PORT_IDX(). Sashiko, correctly points this out.
> > >
> > > So I don't think anything is broken today. Did you see a problem on
> > > hardware that this fixes?
> > >
> > > FYI, we carry the same change downstream. But there it was added together with
> > > RGMII MTU support, which reads and writes DEVRGMII_MAC_MAXLEN_CFG() using this
> > > index.
> >
> > Hi Andrew,
> > As pointed out nothing yet is broken, hence no fixes tag.
> >
> > I am using it downstream as well, for the MTU change support where as
> > you pointed out RGMII code
> > uses it to get the correct index.
> >
> > So, I thought that it would be good idea to send it upstream before
> > eventually getting around to sending
> > the MTU change support as well.
>
> It wasn't clear to me that this was preparation work for a future feature. As a
> standlone patch it does nothing. IDK, to me it would make more sense to just
> send it together with the patchset that would actually use it.

That is fine by me.

Regards,
Robert

>
> /Daniel
>
> >
> > Regards,
> > Robert
> >
> > >
> > > /Daniel
> >
> >
> >
> > --
> > Robert Marko
> > Staff Embedded Linux Engineer
> > Sartura d.d.
> > Lendavska ulica 16a
> > 10000 Zagreb, Croatia
> > Email: robert.marko@sartura.hr
> > Web: www.sartura.hr



-- 
Robert Marko
Staff Embedded Linux Engineer
Sartura d.d.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr