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

[Kunihiko Hayashi]

On 2025/02/03 20:32, Kunihiko Hayashi wrote:
> Hi,
>
> On 2025/02/03 18:23, Russell King (Oracle) wrote:
> > On Mon, Feb 03, 2025 at 11:45:05AM +0900, Kunihiko Hayashi wrote:
> > > Hi all,
> > >
> > > On 2025/02/02 5:35, Russell King (Oracle) wrote:
> > > > 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@xxxxxxxxxxxxx>
> > > > >
> > > > > 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
> > >
> > > Sorry for inconvenience.
> > >
> > > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > >
> > > > 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.
> > >
> > > The {tx,rx}_queus_to_use are referenced in stmmac_ethtool.c,
> > stmmac_tc.c,
> > > and stmmac_selftests.c as the number of queues, so I've thought that
> > > these variables should not be non-zero.
> >
> > Huh? We're talking about {tx,rx}_fifo_size, not _queues_to_use.
>
> Sorry, this topic is just about {tx,rx}_fifo_size.
> Above comments are not needed here.
>
> These value are only referenced in stmmac_main.c and stmmac_selftest.c.
> As far as I can see, there is no usage that requires the values to be
> non-zero.

Comment to myself:
This is incorrect as it depends on how the values are used.

Thank you,

---
Best Regards
Kunihiko Hayashi