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

[Steven Price <steven.price@arm.com>]

On 31/01/2025 15:29, Andrew Lunn wrote:
> On Fri, Jan 31, 2025 at 03:03:11PM +0000, Steven Price wrote:
>> On 31/01/2025 14:47, Andrew Lunn wrote:
>>>>> I'm guessing, but in your setup, i assume the value is never written
>>>>> to a register, hence 0 is O.K. e.g. dwmac1000_dma_operation_mode_rx(),
>>>>> the fifosz value is used to determine if flow control can be used, but
>>>>> is otherwise ignored.
>>>>
>>>> I haven't traced the code, but that fits my assumptions too.
>>>
>>> I could probably figure it out using code review, but do you know
>>> which set of DMA operations your hardware uses? A quick look at
>>> dwmac-rk.c i see:
>>>
>>>         /* If the stmmac is not already selected as gmac4,
>>>          * then make sure we fallback to gmac.
>>>          */
>>>         if (!plat_dat->has_gmac4)
>>>                 plat_dat->has_gmac = true;
>>
>> has_gmac4 is false on this board, so has_gmac will be set to true here.
> 
> Thanks. Looking in hwif.c, that means dwmac1000_dma_ops are used.
> 
> dwmac1000_dma_operation_mode_rx() just does a check:
> 
> 	if (rxfifosz < 4096) {
> 		csr6 &= ~DMA_CONTROL_EFC;
> 
> but otherwise does not use the value.
> 
> dwmac1000_dma_operation_mode_tx() appears to completely ignore fifosz.
> 
> So i would say all zero is valid for has_gmac == true, but you might
> gain flow control if a value was passed.
> 
> A quick look at dwmac100_dma_operation_mode_tx() suggests fifosz is
> also ignored, and dwmac100_dma_operation_mode_rx() does not exist. So
> all 0 is also valid for gmac == false, gmac4 == false, and xgmac ==
> false.
> 
> dwmac4_dma_rx_chan_op_mode() does use the value to determine mtl_rx_op
> which gets written to a register. So gmac4 == true looks to need
> values. dwxgmac2_dma_rx_mode() is the same, so xgmac = true also need
> valid values.
> 
> Could you cook up a fix based on this?

The below works for me, I haven't got the hardware to actually test the 
gmac4/xgmac paths:

----8<----
From 1ff2f1d5c35d95b38cdec02a283b039befdff0a4 Mon Sep 17 00:00:00 2001
From: Steven Price <steven.price@arm.com>
Date: Fri, 31 Jan 2025 15:45:50 +0000
Subject: [PATCH] net: stmmac: Allow zero for [tr]x_fifo_size

Commit 8865d22656b4 ("net: stmmac: Specify hardware capability value
when FIFO size isn't specified") modified the behaviour to bail out if
both the FIFO size and the hardware capability were both set to zero.
However devices where has_gmac4 and has_xgmac are both false don't use
the fifo size and that commit breaks platforms for which these values
were zero.

Only warn and error out when (has_gmac4 || has_xgmac) where the values
are used and zero would cause problems, otherwise continue with the zero
values.

Fixes: 8865d22656b4 ("net: stmmac: Specify hardware capability value when FIFO size isn't specified")
Signed-off-by: Steven Price <steven.price@arm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d04543e5697b..821404beb629 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7222,7 +7222,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 	if (!priv->plat->rx_fifo_size) {
 		if (priv->dma_cap.rx_fifo_size) {
 			priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
-		} else {
+		} else if (priv->plat->has_gmac4 || priv->plat->has_xgmac) {
 			dev_err(priv->device, "Can't specify Rx FIFO size\n");
 			return -ENODEV;
 		}
@@ -7236,7 +7236,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 	if (!priv->plat->tx_fifo_size) {
 		if (priv->dma_cap.tx_fifo_size) {
 			priv->plat->tx_fifo_size = priv->dma_cap.tx_fifo_size;
-		} else {
+		} else if (priv->plat->has_gmac4 || priv->plat->has_xgmac) {
 			dev_err(priv->device, "Can't specify Tx FIFO size\n");
 			return -ENODEV;
 		}
-- 
2.39.5