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

[Xi Ruoyao]

On Fri, 2025-01-31 at 15:54 +0000, Steven Price wrote:
> 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@xxxxxxx>
> 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@xxxxxxx>
> ---
>  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;
>  		}

Works for me on TH1520 (arch/riscv/boot/dts/thead/th1520.dtsi).

Tested-by: Xi Ruoyao <xry111@xxxxxxxxxxx>

-- 
Xi Ruoyao <xry111@xxxxxxxxxxx>
School of Aerospace Science and Technology, Xidian University