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

[PATCH net v3 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 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 v3 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..be1e6fa6d557 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->dma_cap.number_rx_queues < priv->plat->rx_queues_to_use) {
+		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->dma_cap.number_tx_queues < priv->plat->tx_queues_to_use) {
+		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 v3 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 be1e6fa6d557..251a812e4e77 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->dma_cap.rx_fifo_size < priv->plat->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->dma_cap.tx_fifo_size < priv->plat->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 v3 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 251a812e4e77..efeefb9eff8b 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->dma_cap.rx_fifo_size < priv->plat->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->dma_cap.rx_fifo_size < priv->plat->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->dma_cap.tx_fifo_size < priv->plat->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->dma_cap.tx_fifo_size < priv->plat->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 v3 1/3] net: stmmac: Limit the number of MTL queues to hardware capability

[Russell King (Oracle)]

On Fri, Jan 24, 2025 at 07:13:57PM +0900, Kunihiko Hayashi wrote:
> 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..be1e6fa6d557 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->dma_cap.number_rx_queues < priv->plat->rx_queues_to_use) {

While this looks "nicer", which of these two do you think reads better
and is easier to understand:

"If priv->dma_cap.number_rx_queues is set, and
 priv->dma_cap.number_rx_queues is less than
 priv->plat->rx_queues_to_use then print a message about
 priv->plat->rx_queues_to_use exceeding priv->dma_cap.number_rx_queues"

"If priv->dma_cap.number_rx_queues is set, and
 priv->plat->rx_queues_to_use is greater than
 priv->dma_cap.number_rx_queues, then print a message about
 priv->plat->rx_queues_to_use exceeding priv->dma_cap.number_rx_queues"

With the former one has to mentally flip the test around in the if
statement to check that it does indeed match the warning that is
printed.

-- 
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 v3 1/3] net: stmmac: Limit the number of MTL queues to hardware capability

[Kunihiko Hayashi]

On 2025/01/24 20:43, Russell King (Oracle) wrote:
> On Fri, Jan 24, 2025 at 07:13:57PM +0900, Kunihiko Hayashi wrote:
>> 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..be1e6fa6d557 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->dma_cap.number_rx_queues < priv->plat->rx_queues_to_use)
> {
> 
> While this looks "nicer", which of these two do you think reads better
> and is easier to understand:
> 
> "If priv->dma_cap.number_rx_queues is set, and
>   priv->dma_cap.number_rx_queues is less than
>   priv->plat->rx_queues_to_use then print a message about
>   priv->plat->rx_queues_to_use exceeding priv->dma_cap.number_rx_queues"
> 
> "If priv->dma_cap.number_rx_queues is set, and
>   priv->plat->rx_queues_to_use is greater than
>   priv->dma_cap.number_rx_queues, then print a message about
>   priv->plat->rx_queues_to_use exceeding priv->dma_cap.number_rx_queues"
> 
> With the former one has to mentally flip the test around in the if
> statement to check that it does indeed match the warning that is
> printed.

This patch focuses only on the conditional lines, however, certainly
it's not good to have the meaning of the message and the condition
reversed.

I'll fix it next.

Thank you,

---
Best Regards
Kunihiko Hayashi