Re: [PATCH net v4 3/3] net: stmmac: Specify hardware capability value when FIFO size isn't specified

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Sat, Feb 01, 2025 at 11:14:41AM -0800, Guenter Roeck wrote:
> Hi,
> 
> On Mon, Jan 27, 2025 at 10:38:20AM +0900, Kunihiko Hayashi wrote:
> > When Tx/Rx FIFO size is not specified in advance, the driver checks if
> > the value is zero and sets the hardware capability value in functions
> > where that value is used.
> > 
> > Consolidate the check and settings into function stmmac_hw_init() and
> > remove redundant other statements.
> > 
> > If FIFO size is zero and the hardware capability also doesn't have upper
> > limit values, return with an error message.
> > 
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
> 
> This patch breaks qemu's stmmac emulation, for example for
> npcm750-evb. The error message is:
> 	stmmaceth f0804000.eth: Can't specify Rx FIFO size

Interesting. I looked at QEMU to see whether anything in the Debian
stable version of QEMU might possibly have STMMAC emulation, but
drew a blank... Even trying to find where in QEMU it emulates the
STMMAC. I do see that it does include this, so maybe I can use that
to test some of my stmmac changes. Thanks!

> The setup function called for the emulated hardware is dwmac1000_setup().
> That function does not set the DMA rx or tx fifo size.
> 
> At the same time, the rx and tx fifo size is not provided in the
> devicetree file (nuvoton-npcm750.dtsi), so the failure is obvious.
> 
> I understand that the real hardware may be based on a more recent
> version of the DWMAC IP which provides the DMA tx/rx fifo size, but
> I do wonder: Are the benefits of this patch so substantial that it
> warrants breaking the qemu emulation of this network interface ?

Please see my message sent a while back on an earlier revision of this
patch series. I reviewed the stmmac driver for the fifo sizes and
documented what I found.

https://lore.kernel.org/r/Z4_ZilVFKacuAUE8@shell.armlinux.org.uk

To save clicking on the link, I'll reproduce the relevant part below.
It appears that dwmac1000 has no way to specify the FIFO size, and
thus would have priv->dma_cap.rx_fifo_size and
priv->dma_cap.tx_fifo_size set to zero.

Given the responses, I'm now of the opinion that the patch series is
wrong, and probably should be reverted - I never really understood
the motivation why the series was necessary. It seemed to me to be a
"wouldn't it be nice if" series rather than something that is
functionally necessary.


Here's the extract from my previous email:

Now looking at the defintions:

drivers/net/ethernet/stmicro/stmmac/dwmac4.h:#define GMAC_HW_RXFIFOSIZE        GENMASK(4, 0)
drivers/net/ethernet/stmicro/stmmac/dwxgmac2.h:#define XGMAC_HWFEAT_RXFIFOSIZE GENMASK(4, 0)

So there's a 5-bit bitfield that describes the receive FIFO size for
these two MACs. Then we have:

drivers/net/ethernet/stmicro/stmmac/common.h:#define DMA_HW_FEAT_RXFIFOSIZE    0x00080000       /* Rx FIFO > 2048 Bytes */

which is used here:

drivers/net/ethernet/stmicro/stmmac/dwmac1000_dma.c:    dma_cap->rxfifo_over_2048 = (hw_cap & DMA_HW_FEAT_RXFIFOSIZE) >> 19;

which is only used to print a Y/N value in a debugfs file, otherwise
having no bearing on driver behaviour.

So, I suspect MACs other than xgmac2 or dwmac4 do not have the ability
to describe the hardware FIFO sizes in hardware, thus why there's the
override and no checking of what the platform provided - and doing so
would break the driver. This is my interpretation from the code alone.

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