[PATCH net v4 0/3] Limit devicetree parameters to hardware capability

[PATCH net v4 0/3] Limit devicetree parameters to hardware capability

[Kunihiko Hayashi]

This series includes patches that checks the devicetree properties,
the number of MTL queues and FIFO size values, and if these specified
values exceed the value contained in hardware capabilities, limit to
the values from the capabilities. Do nothing if the capabilities don't
have any specified values.

And this sets hardware capability values if FIFO sizes are not specified
and removes redundant lines.

Changes since v3:
- Match the order of conditions to warning messages

Changes since v2:
- Check if there are hardware capability values
- Add an error message when FIFO size can't be specified
- Change each position of specified values in error messages 

Changes since v1:
- Move the check for FIFO size and MTL queues to initializing phase
- Move zero check lines to initializing phase
- Use hardware capabilities instead of defined values
- Add warning messages if the values exceeds
- Add Fixes: lines

Kunihiko Hayashi (3):
  net: stmmac: Limit the number of MTL queues to hardware capability
  net: stmmac: Limit FIFO size by hardware capability
  net: stmmac: Specify hardware capability value when FIFO size isn't
    specified

 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 57 ++++++++++++++-----
 1 file changed, 44 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH net v4 1/3] net: stmmac: Limit the number of MTL queues to hardware capability

[Kunihiko Hayashi]

The number of MTL queues to use is specified by the parameter
"snps,{tx,rx}-queues-to-use" from stmmac_platform layer.

However, the maximum numbers of queues are constrained by upper limits
determined by the capability of each hardware feature. It's appropriate
to limit the values not to exceed the upper limit values and display
a warning message.

This only works if the hardware capability has the upper limit values.

Fixes: d976a525c371 ("net: stmmac: multiple queues dt configuration")
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7bf275f127c9..97c3d1b89529 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7232,6 +7232,21 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 	if (priv->dma_cap.tsoen)
 		dev_info(priv->device, "TSO supported\n");
 
+	if (priv->dma_cap.number_rx_queues &&
+	    priv->plat->rx_queues_to_use > priv->dma_cap.number_rx_queues) {
+		dev_warn(priv->device,
+			 "Number of Rx queues (%u) exceeds dma capability\n",
+			 priv->plat->rx_queues_to_use);
+		priv->plat->rx_queues_to_use = priv->dma_cap.number_rx_queues;
+	}
+	if (priv->dma_cap.number_tx_queues &&
+	    priv->plat->tx_queues_to_use > priv->dma_cap.number_tx_queues) {
+		dev_warn(priv->device,
+			 "Number of Tx queues (%u) exceeds dma capability\n",
+			 priv->plat->tx_queues_to_use);
+		priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
+	}
+
 	priv->hw->vlan_fail_q_en =
 		(priv->plat->flags & STMMAC_FLAG_VLAN_FAIL_Q_EN);
 	priv->hw->vlan_fail_q = priv->plat->vlan_fail_q;
-- 
2.25.1

[PATCH net v4 2/3] net: stmmac: Limit FIFO size by hardware capability

[Kunihiko Hayashi]

Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from
stmmac_platform layer.

However, these values are constrained by upper limits determined by the
capabilities of each hardware feature. There is a risk that the upper
bits will be truncated due to the calculation, so it's appropriate to
limit them to the upper limit values and display a warning message.

This only works if the hardware capability has the upper limit values.

Fixes: e7877f52fd4a ("stmmac: Read tx-fifo-depth and rx-fifo-depth from the devicetree")
Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 97c3d1b89529..1d0491e15e5b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7247,6 +7247,21 @@ 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) {
+		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) {
+		dev_warn(priv->device,
+			 "Tx FIFO size (%u) exceeds dma capability\n",
+			 priv->plat->tx_fifo_size);
+		priv->plat->tx_fifo_size = priv->dma_cap.tx_fifo_size;
+	}
+
 	priv->hw->vlan_fail_q_en =
 		(priv->plat->flags & STMMAC_FLAG_VLAN_FAIL_Q_EN);
 	priv->hw->vlan_fail_q = priv->plat->vlan_fail_q;
-- 
2.25.1

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

[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>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 35 ++++++++++---------
 1 file changed, 18 insertions(+), 17 deletions(-)

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);
-- 
2.25.1

Re: [PATCH net v4 1/3] net: stmmac: Limit the number of MTL queues to hardware capability

[Yanteng Si]

在 1/27/25 09:38, Kunihiko Hayashi 写道:
> The number of MTL queues to use is specified by the parameter
> "snps,{tx,rx}-queues-to-use" from stmmac_platform layer.
> 
> However, the maximum numbers of queues are constrained by upper limits
> determined by the capability of each hardware feature. It's appropriate
> to limit the values not to exceed the upper limit values and display
> a warning message.
> 
> This only works if the hardware capability has the upper limit values.
> 
> Fixes: d976a525c371 ("net: stmmac: multiple queues dt configuration")
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Yanteng Si <si.yanteng@linux.dev>


Thanks,
Yanteng

Re: [PATCH net v4 2/3] net: stmmac: Limit FIFO size by hardware capability

[Yanteng Si]

在 1/27/25 09:38, Kunihiko Hayashi 写道:
> Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from
> stmmac_platform layer.
> 
> However, these values are constrained by upper limits determined by the
> capabilities of each hardware feature. There is a risk that the upper
> bits will be truncated due to the calculation, so it's appropriate to
> limit them to the upper limit values and display a warning message.
> 
> This only works if the hardware capability has the upper limit values.
> 
> Fixes: e7877f52fd4a ("stmmac: Read tx-fifo-depth and rx-fifo-depth from the devicetree")
> Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
Reviewed-by: Yanteng Si <si.yanteng@linux.dev>

Thanks,
Yanteng

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

[Yanteng Si]

在 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

Re: [PATCH net v4 0/3] Limit devicetree parameters to hardware capability

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Mon, 27 Jan 2025 10:38:17 +0900 you wrote:
> This series includes patches that checks the devicetree properties,
> the number of MTL queues and FIFO size values, and if these specified
> values exceed the value contained in hardware capabilities, limit to
> the values from the capabilities. Do nothing if the capabilities don't
> have any specified values.
> 
> And this sets hardware capability values if FIFO sizes are not specified
> and removes redundant lines.
> 
> [...]

Here is the summary with links:
  - [net,v4,1/3] net: stmmac: Limit the number of MTL queues to hardware capability
    https://git.kernel.org/netdev/net/c/f5fb35a3d6b3
  - [net,v4,2/3] net: stmmac: Limit FIFO size by hardware capability
    https://git.kernel.org/netdev/net/c/044f2fbaa272
  - [net,v4,3/3] net: stmmac: Specify hardware capability value when FIFO size isn't specified
    https://git.kernel.org/netdev/net/c/8865d22656b4

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

[Steven Price]

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,
-- 
2.39.5

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

[Andrew Lunn]

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

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

[Steven Price]

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

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

[Andrew Lunn]

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

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

[Steven Price]

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

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

[Andrew Lunn]

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

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

[Steven Price]

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

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

[Andrew Lunn]

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

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

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

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

[Guenter Roeck]

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

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

[Andrew Lunn]

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

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

[Guenter Roeck]

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

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

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Andrew Lunn]

> 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

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

[Guenter Roeck]

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

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

[Kunihiko Hayashi]

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

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

[Russell King (Oracle)]

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.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Kunihiko Hayashi]

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

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

[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