| Message ID | 20250127013820.2941044-4-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Limit devicetree parameters to hardware capability | expand |
在 1/27/25 09:38, Kunihiko Hayashi 写道: > 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> Reviewed-by: Yanteng Si <si.yanteng@linux.dev> Thanks, Yanteng
On 27/01/2025 01:38, 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. This patch breaks my Firefly RK3288 board. It appears that all of the following are true: * priv->plat->rx_fifo_size == 0 * priv->dma_cap.rx_fifo_size == 0 * priv->plat->tx_fifo_size == 0 * priv->dma_cap.tx_fifo_size == 0 Simply removing the "return -ENODEV" lines gets this platform working again (and AFAICT matches the behaviour before this patch was applied). I'm not sure whether this points to another bug causing these to all be zero or if this is just an oversight. The below patch gets my board working: -----8<----- From 5097d29181f320875d29da8fc24e6d3ae44db581 Mon Sep 17 00:00:00 2001 From: Steven Price <steven.price@arm.com> Date: Fri, 31 Jan 2025 09:32:17 +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 there are platforms out there (e.g. RK3288) where this is the case which this breaks. Remove the error return and use the dma_cap value as is. 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 | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index d04543e5697b..41c837c91811 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -7220,12 +7220,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 { - dev_err(priv->device, "Can't specify Rx FIFO size\n"); - return -ENODEV; - } + priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size; } else if (priv->dma_cap.rx_fifo_size && priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) { dev_warn(priv->device, @@ -7234,12 +7229,7 @@ static int stmmac_hw_init(struct stmmac_priv *priv) priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size; } 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 { - dev_err(priv->device, "Can't specify Tx FIFO size\n"); - return -ENODEV; - } + priv->plat->tx_fifo_size = priv->dma_cap.tx_fifo_size; } else if (priv->dma_cap.tx_fifo_size && priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) { dev_warn(priv->device,
On Fri, Jan 31, 2025 at 09:46:41AM +0000, Steven Price wrote: > On 27/01/2025 01:38, 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. > > This patch breaks my Firefly RK3288 board. It appears that all of the > following are true: > > * priv->plat->rx_fifo_size == 0 > * priv->dma_cap.rx_fifo_size == 0 > * priv->plat->tx_fifo_size == 0 > * priv->dma_cap.tx_fifo_size == 0 > > Simply removing the "return -ENODEV" lines gets this platform working > again (and AFAICT matches the behaviour before this patch was applied). > I'm not sure whether this points to another bug causing these to > all be zero or if this is just an oversight. The below patch gets my > board working: Thanks for the quick report of the problem. Your 'fix' basically just reverts the patch. Let first try to understand what is going on, and fix the patch. We can do a revert later if we cannot find a better solution. 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. We should determine which versions of stmmac actually need values, and limit the test to those versions. Andrew
On 31/01/2025 14:15, Andrew Lunn wrote: > On Fri, Jan 31, 2025 at 09:46:41AM +0000, Steven Price wrote: >> On 27/01/2025 01:38, 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. >> >> This patch breaks my Firefly RK3288 board. It appears that all of the >> following are true: >> >> * priv->plat->rx_fifo_size == 0 >> * priv->dma_cap.rx_fifo_size == 0 >> * priv->plat->tx_fifo_size == 0 >> * priv->dma_cap.tx_fifo_size == 0 >> >> Simply removing the "return -ENODEV" lines gets this platform working >> again (and AFAICT matches the behaviour before this patch was applied). >> I'm not sure whether this points to another bug causing these to >> all be zero or if this is just an oversight. The below patch gets my >> board working: > > Thanks for the quick report of the problem. > > Your 'fix' basically just reverts the patch. Let first try to > understand what is going on, and fix the patch. We can do a revert > later if we cannot find a better solution. Sure thing - I wasn't entirely sure if the patch was just to 'tidy up' the code (in which case my code keeps the consolidation). I'm not familiar with this area so I'll let you figure out if there's a better solution. > 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. > We should determine which versions of stmmac actually need values, and > limit the test to those versions. If you want me to try out a patch or do any more investigations then just let me know. Thanks, Steve
> > 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; Which suggests there are two variants of the RockChip MAC. Andrew
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. The DT compatible is rockchip,rk3288-gmac Steve > Which suggests there are two variants of the RockChip MAC. > > Andrew
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? Andrew
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; }
On Fri, Jan 31, 2025 at 03:54:16PM +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: This looks sensible. Hopefully Kunihiko can test on more platforms. Andrew
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@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; > } Works for me on TH1520 (arch/riscv/boot/dts/thead/th1520.dtsi). Tested-by: Xi Ruoyao <xry111@xry111.site>
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 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 ? Thanks, Guenter
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 Hi Guenter Please could you try the patch here: https://lore.kernel.org/lkml/915713e1-b67f-4eae-82c6-8dceae8f97a7@arm.com/ Andrew
On 2/1/25 11:21, Andrew Lunn 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 > > Hi Guenter > > Please could you try the patch here: > > https://lore.kernel.org/lkml/915713e1-b67f-4eae-82c6-8dceae8f97a7@arm.com/ > Yes, that works. Thanks, and sorry for the noise. Guenter
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 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 below works for me, I haven't got the hardware to actually test the > gmac4/xgmac paths: Hi Steven I think we have enough testing done, please could you officially submit this patch. Thanks Andrew
On 2/1/25 12: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 > > 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! > Support was added in qemu v9.0.0. See qemu commit 08f787a34c ("hw/net: Add NPCMXXX GMAC device"). It doesn't directly say what the emulated hardware actually is, but the commit message says The following message shows up with the change: Broadcom BCM54612E stmmac-0:00: attached PHY driver [Broadcom BCM54612E] (mii_bus:phy_addr=stmmac-0:00, irq=POLL) stmmaceth f0802000.eth eth0: Link is Up - 1Gbps/Full - flow control rx/tx so that is a bit of a hint. Guenter
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. 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
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. > 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. > 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.
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
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
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 1d0491e15e5b..e3ce2c214c0b 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2375,11 +2375,6 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv) u32 chan = 0; u8 qmode = 0; - if (rxfifosz == 0) - rxfifosz = priv->dma_cap.rx_fifo_size; - if (txfifosz == 0) - txfifosz = priv->dma_cap.tx_fifo_size; - /* Split up the shared Tx/Rx FIFO memory on DW QoS Eth and DW XGMAC */ if (priv->plat->has_gmac4 || priv->plat->has_xgmac) { rxfifosz /= rx_channels_count; @@ -2851,11 +2846,6 @@ static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode, int rxfifosz = priv->plat->rx_fifo_size; int txfifosz = priv->plat->tx_fifo_size; - if (rxfifosz == 0) - rxfifosz = priv->dma_cap.rx_fifo_size; - if (txfifosz == 0) - txfifosz = priv->dma_cap.tx_fifo_size; - /* Adjust for real per queue fifo size */ rxfifosz /= rx_channels_count; txfifosz /= tx_channels_count; @@ -5856,9 +5846,6 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu) const int mtu = new_mtu; int ret; - if (txfifosz == 0) - txfifosz = priv->dma_cap.tx_fifo_size; - txfifosz /= priv->plat->tx_queues_to_use; if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) { @@ -7247,15 +7234,29 @@ static int stmmac_hw_init(struct stmmac_priv *priv) priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues; } - if (priv->dma_cap.rx_fifo_size && - priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) { + 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 { + dev_err(priv->device, "Can't specify Rx FIFO size\n"); + return -ENODEV; + } + } else if (priv->dma_cap.rx_fifo_size && + priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) { dev_warn(priv->device, "Rx FIFO size (%u) exceeds dma capability\n", priv->plat->rx_fifo_size); priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size; } - if (priv->dma_cap.tx_fifo_size && - priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) { + 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 { + dev_err(priv->device, "Can't specify Tx FIFO size\n"); + return -ENODEV; + } + } else if (priv->dma_cap.tx_fifo_size && + priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) { dev_warn(priv->device, "Tx FIFO size (%u) exceeds dma capability\n", priv->plat->tx_fifo_size);
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> --- .../net/ethernet/stmicro/stmmac/stmmac_main.c | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-)