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

[Kunihiko Hayashi <hayashi.kunihiko@socionext.com>]

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@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
>>>
>>> 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@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.
>>>>
>>>
>>> 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