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

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

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.

>> 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.
> 
> Huh? I mentioned dwmac1000, not dwmac100.

My above comment is missing the point.
dwmac1000 doesn't have the field of the fifo size in the hardware capability
even if dwmac1000 has get_hw_feature function as you said.
Steven's diff is reasonable to check the feature.

> 
>> 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.
> 
> I'm not sure you're discussing the same issue as the rest of us.
> You seem to be talking about a different pair of structure members
> (queues_to_use) whereas your patches and the problem at hand is with
> the changes made to {tx,rx}_fifo_size.

Sorry for confusion. The patch 3/3 treats FIFO size as per the subject.

Thank you,

---
Best Regards
Kunihiko Hayashi