Re: [PATCH net-next 10/12] net: stmmac: tc956x: add TC956x/QPS615 support

[Andrew Lunn <andrew@lunn.ch>]

On Thu, May 07, 2026 at 05:03:46PM +0100, Daniel Thompson wrote:
> On Fri, May 01, 2026 at 09:04:58PM +0200, Andrew Lunn wrote:
> > > +static struct tc956x_mac_speed mac_speed[] = {
> > > +	{ PHY_INTERFACE_MODE_2500BASEX,	SPEED_2500,  SP_SEL_SGMII_2500M, },
> > > +	{ PHY_INTERFACE_MODE_SGMII,	SPEED_2500,  SP_SEL_SGMII_2500M, },
> > > +	{ PHY_INTERFACE_MODE_SGMII,	SPEED_1000,  SP_SEL_SGMII_1000M, },
> >
> > That looks odd. Some vendors implemented 2500BaseX using SGMII
> > overclocked. But that is not strictly 2500BaseX. Having the 2500BASEX
> > entry suggests you have real 2500BASEX, so why have an SGMII entry
> > with SPEED_2500?
> 
> This is a consequence of the code that uses this lookup table being
> called both during initialization and from the fix_mac_speed() callback.
> 
> During initialization we only have the value in plat->phy_interface to
> go on so we run the lookup table using plat->phy_interface (which is
> typically PHY_INTERFACE_MODE_SGMII) and with the maximum permitted
> speed.

Something sounds wrong here. SGMII only supports 10/100/1G. You should
never be asked to do SGMII at 2500. It should ask for 2500BaseX.

> I haven't got detailed enough notes to allow me to double check but I
> think there were problems completing the initial MAC reset if we didn't
> write something sensible to the hardware during initialization.

> During fix_max_speed() we get told to adopt 2500base-x. Reviewing the
> code I can see we don't propagate that and just use
> plat->phy_interface for fix_mac_speed(). I will fix the code to that
> the requested interface propagates properly to the lookup table but I
> think we would still rely on the SGMII entry to get sane initial values
> to write to the hardware.

Getting sane values into the hardware is good, but 2500 SGMII is not
sane :-(

> > Doesn't that break any other dwxgmac301 in the system? Shouldn't you
> > be making a copy of the global structure, and then making
> > modifications to your copy? mac->dma then points to your copy?
> 
> That's exactly what this code does.
> 
> `*dma = dwxgmac301_dma_ops` is a structure copy, we never take a pointer
> to dwxgmac301_dma_ops (and if we did, dwxgmac301_dma_ops is const so I
> think we'd get a kernel oops if we tried to write to rodata).
> 
> Would to code be easier to read if we dropped the local `dma` variable
> since that would make it clearer that td->dma is not a pointer? More
> like:

> +       /* dwxgmac301_dma_ops needs extending to provide DMA address translation */
> +       td->dma = dwxgmac301_dma_ops;
> +       td->dma.init_rx_chan = tc956x_dma_init_rx_chan;
> +       td->dma.init_tx_chan = tc956x_dma_init_tx_chan;
> +       mac->dma = &dma;

Yes, that is better. I also think it is partially my problem. You
don't often see structure assignments, just pointer assignments. So
i'm somewhat blind to them.

	Andrew