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

[Kunihiko Hayashi]

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.

However, currently the variables are allowed to be zero, so I understand
this patch 3/3 breaks on the chips that hasn't hardware capabilities.

In hwif.c, stmmac_hw[] defines four patterns of hardwares:

"dwmac100"  .gmac=false, .gmac4=false, .xgmac=false, .get_hw_feature = NULL
"dwmac1000" .gmac=true,  .gmac4=false, .xgmac=false, .get_hw_feature = dwmac1000_get_hw_feature()
"dwmac4"    .gmac=false, .gmac4=true,  .xgmac=false, .get_hw_feature = dwmac4_get_hw_feature()
"dwxgmac2"  .gmac=false, .gmac4=false, .xgmac=true , .get_hw_feature = dwxgmac2_get_hw_feature()

As Russell said, the dwmac100 can't get the number of queues from the hardware
capability. And some environments (at least QEMU device that Guenter said)
seems the capability values are zero in spite of dwmac1000.

Since I can't test all of the device patterns, so I appreciate checking each
hardware and finding the issue.

The patch 3/3 includes some cleanup and code reduction, though, I think
it would be better to revert it once.

Thank you,

---
Best Regards
Kunihiko Hayashi