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

[PATCH net v2 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 the hardware capabilities, limit to
the values from the capabilities.

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

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 | 43 +++++++++++++------
 1 file changed, 30 insertions(+), 13 deletions(-)

-- 
2.25.1

[PATCH net v2 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.

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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 7bf275f127c9..251a8c15637f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7232,6 +7232,19 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 	if (priv->dma_cap.tsoen)
 		dev_info(priv->device, "TSO supported\n");
 
+	if (priv->plat->rx_queues_to_use > priv->dma_cap.number_rx_queues) {
+		dev_warn(priv->device,
+			 "Number of Rx queues exceeds dma capability (%d)\n",
+			 priv->plat->rx_queues_to_use);
+		priv->plat->rx_queues_to_use = priv->dma_cap.number_rx_queues;
+	}
+	if (priv->plat->tx_queues_to_use > priv->dma_cap.number_tx_queues) {
+		dev_warn(priv->device,
+			 "Number of Tx queues exceeds dma capability (%d)\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 v2 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.

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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 251a8c15637f..da3316e3e93b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -7245,6 +7245,19 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 		priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
 	}
 
+	if (priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) {
+		dev_warn(priv->device,
+			 "Rx FIFO size exceeds dma capability (%d)\n",
+			 priv->plat->rx_fifo_size);
+		priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
+	}
+	if (priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) {
+		dev_warn(priv->device,
+			 "Tx FIFO size exceeds dma capability (%d)\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 v2 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.

This sets the hardware capability value as a default and removes redundant
statements.

Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com>
---
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 21 ++++++-------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index da3316e3e93b..486283c27963 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) {
@@ -7245,13 +7232,17 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
 		priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
 	}
 
-	if (priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) {
+	if (!priv->plat->rx_fifo_size) {
+		priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
+	} else if (priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) {
 		dev_warn(priv->device,
 			 "Rx FIFO size exceeds dma capability (%d)\n",
 			 priv->plat->rx_fifo_size);
 		priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
 	}
-	if (priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) {
+	if (!priv->plat->tx_fifo_size) {
+		priv->plat->tx_fifo_size = priv->dma_cap.tx_fifo_size;
+	} else if (priv->plat->tx_fifo_size > priv->dma_cap.tx_fifo_size) {
 		dev_warn(priv->device,
 			 "Tx FIFO size exceeds dma capability (%d)\n",
 			 priv->plat->tx_fifo_size);
-- 
2.25.1

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

[Furong Xu]

On Tue, 21 Jan 2025 13:41:36 +0900, Kunihiko Hayashi <hayashi.kunihiko@socionext.com> 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.
> 
> 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 | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 7bf275f127c9..251a8c15637f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7232,6 +7232,19 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
>  	if (priv->dma_cap.tsoen)
>  		dev_info(priv->device, "TSO supported\n");
>  
> +	if (priv->plat->rx_queues_to_use > priv->dma_cap.number_rx_queues) {
> +		dev_warn(priv->device,
> +			 "Number of Rx queues exceeds dma capability (%d)\n",
> +			 priv->plat->rx_queues_to_use);
> +		priv->plat->rx_queues_to_use = priv->dma_cap.number_rx_queues;
> +	}
> +	if (priv->plat->tx_queues_to_use > priv->dma_cap.number_tx_queues) {
> +		dev_warn(priv->device,
> +			 "Number of Tx queues exceeds dma capability (%d)\n",
> +			 priv->plat->tx_queues_to_use);

I would prefer print these warnings like this:

dev_warn(priv->device, "Number of Tx queues (%u) exceeds dma capability (%u)\n",
priv->plat->tx_queues_to_use, priv->dma_cap.number_tx_queues);

And number_tx_queues, number_rx_queues are u32, so %u would be better.

This print format change is quite minor. Probably not worth a re-roll since one
can always view DMA capabilities by reading a debugfs entry.

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

[Russell King (Oracle)]

On Tue, Jan 21, 2025 at 01:41:35PM +0900, Kunihiko Hayashi 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 the hardware capabilities, limit to
> the values from the capabilities.
> 
> And this sets hardware capability values if FIFO sizes are not specified
> and removes redundant lines.

I think you also indeed to explain why (and possibly understand) - if
there are hardware capabilities that describe these parameters - it has
been necessary to have them in firmware as well.

There are two scenarios I can think of why these would be duplicated:

1. firmware/platform capabilities are there to correct wrong values
   provided by the hardware.
2. firmware/platform capabilities are there to reduce the parameters
   below hardware maximums.

Which it is affects whether your patch is correct or not, and thus needs
to be mentioned.

Finally, as you are submitting to the net tree, you really need to
describe what has regressed in the driver. To me, this looks like a new
"feature" to validate parameters against the hardware.

Please answer these points in this email thread. Please also include
the explanation in future postings.

Thanks.

-- 
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 v2 2/3] net: stmmac: Limit FIFO size by hardware capability

[Yanteng Si]

在 1/21/25 12:41, 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.
> 
> 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 | 13 +++++++++++++
>   1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 251a8c15637f..da3316e3e93b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -7245,6 +7245,19 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
>   		priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
>   	}
>   

> +	if (priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) {

> +		dev_warn(priv->device,
> +			 "Rx FIFO size exceeds dma capability (%d)\n",
> +			 priv->plat->rx_fifo_size);
> +		priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
I executed grep and found that only dwmac4 and dwxgmac2 have initialized 
dma_cap.rx_fifo_size. Can this code still work properly on hardware 
other than these two?


Thanks,
Yanteng

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

[Russell King (Oracle)]

On Wed, Jan 22, 2025 at 01:14:25AM +0800, Yanteng Si wrote:
> 在 1/21/25 12:41, 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.
> > 
> > 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 | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 251a8c15637f..da3316e3e93b 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -7245,6 +7245,19 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
> >   		priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
> >   	}
> 
> > +	if (priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) {
> 
> > +		dev_warn(priv->device,
> > +			 "Rx FIFO size exceeds dma capability (%d)\n",
> > +			 priv->plat->rx_fifo_size);
> > +		priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
> I executed grep and found that only dwmac4 and dwxgmac2 have initialized
> dma_cap.rx_fifo_size. Can this code still work properly on hardware other
> than these two?

Looking at drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:

        /* Compute minimum number of packets to make FIFO full */
        pkt_count = priv->plat->rx_fifo_size;
        if (!pkt_count)
                pkt_count = priv->dma_cap.rx_fifo_size;

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:

        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;

(in two locations)

It looks to me like the intention is that priv->plat->rx_fifo_size is
supposed to _override_ whatever is in priv->dma_cap.rx_fifo_size.

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 v2 2/3] net: stmmac: Limit FIFO size by hardware capability

[Huacai Chen]

On Wed, Jan 22, 2025 at 1:15 AM Yanteng Si <si.yanteng@linux.dev> wrote:
>
> 在 1/21/25 12:41, 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.
> >
> > 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 | 13 +++++++++++++
> >   1 file changed, 13 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 251a8c15637f..da3316e3e93b 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -7245,6 +7245,19 @@ static int stmmac_hw_init(struct stmmac_priv *priv)
> >               priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
> >       }
> >
>
> > +     if (priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) {
>
> > +             dev_warn(priv->device,
> > +                      "Rx FIFO size exceeds dma capability (%d)\n",
> > +                      priv->plat->rx_fifo_size);
> > +             priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
> I executed grep and found that only dwmac4 and dwxgmac2 have initialized
> dma_cap.rx_fifo_size. Can this code still work properly on hardware
> other than these two?
Agree, this will make my below patch not work again, because
dwmac-loongson is based on dwmac1000.

https://lore.kernel.org/netdev/20250121093703.2660482-1-chenhuacai@loongson.cn/T/#u

Huacai

>
>
> Thanks,
> Yanteng
>

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

[Kunihiko Hayashi]

Hi Russell,

Thank you for your comment.

On 2025/01/21 19:25, Russell King (Oracle) wrote:
> On Tue, Jan 21, 2025 at 01:41:35PM +0900, Kunihiko Hayashi 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 the hardware capabilities, limit to
>> the values from the capabilities.
>>
>> And this sets hardware capability values if FIFO sizes are not specified
>> and removes redundant lines.
> 
> I think you also indeed to explain why (and possibly understand) - if
> there are hardware capabilities that describe these parameters - it has
> been necessary to have them in firmware as well.
> 
> There are two scenarios I can think of why these would be duplicated:
> 
> 1. firmware/platform capabilities are there to correct wrong values
>     provided by the hardware.
> 2. firmware/platform capabilities are there to reduce the parameters
>     below hardware maximums.
> 
> Which it is affects whether your patch is correct or not, and thus needs
> to be mentioned.

I think scenario 2 applies in this case.

The queue values specified in devicetree bindings are defined as the
amount to be "used."

     number of TX queues to be used in the driver

And the fifo sizes specified in devicetree bindings are defined as a
"configurable" size.

     This is used for components that can have configurable receive fifo
     sizes, ...

If the amounts of hardware resources are available from the hardware
capabilities, it must be limited to these amounts because exceeding
that values will cause issues.

Otherwise, since there is no values to show hardware resources, specify
these values to be used directly in devicetree. In this case, the values
will be referenced without checking.

> Finally, as you are submitting to the net tree, you really need to
> describe what has regressed in the driver. To me, this looks like a new
> "feature" to validate parameters against the hardware.

Actually, I think that specifyin an arbitrary (too big) value might 
result in unexcepted behavior, and the limit of these parameters is
determined by:
- the max number that can be managed by software, or
- the amount of hardware resources.

> Please answer these points in this email thread. Please also include
> the explanation in future postings.

I'll pay attention next.

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Kunihiko Hayashi]

Hi,

On 2025/01/22 2:29, Russell King (Oracle) wrote:
> On Wed, Jan 22, 2025 at 01:14:25AM +0800, Yanteng Si wrote:
>> 在 1/21/25 12:41, 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.
>>>
>>> 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 | 13 +++++++++++++
>>>    1 file changed, 13 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> index 251a8c15637f..da3316e3e93b 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> @@ -7245,6 +7245,19 @@ static int stmmac_hw_init(struct stmmac_priv
>>> *priv)
>>>    		priv->plat->tx_queues_to_use = priv->dma_cap.number_tx_queues;
>>>    	}
>>
>>> +	if (priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size) {
>>
>>> +		dev_warn(priv->device,
>>> +			 "Rx FIFO size exceeds dma capability (%d)\n",
>>> +			 priv->plat->rx_fifo_size);
>>> +		priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
>> I executed grep and found that only dwmac4 and dwxgmac2 have initialized
>> dma_cap.rx_fifo_size. Can this code still work properly on hardware other
>> than these two?
> 
> Looking at drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:
> 
>          /* Compute minimum number of packets to make FIFO full */
>          pkt_count = priv->plat->rx_fifo_size;
>          if (!pkt_count)
>                  pkt_count = priv->dma_cap.rx_fifo_size;
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:
> 
>          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;
> 
> (in two locations)
> 
> It looks to me like the intention is that priv->plat->rx_fifo_size is
> supposed to _override_ whatever is in priv->dma_cap.rx_fifo_size.
> 
> 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.

Surely, other MACs than xgmac2 and dwmac4 don't have hardware capability
values, and the variables from the capabilities will have zero.
I can add a check whether a capability value is zero or not like that:

If priv->plat->rx-fifo-size is not specified:

     if (priv->dma_cap.rx_fifo_size)
         priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
     else
         return; (with an error value and a message)

If priv->plat->rx-fifo-size is specified:

     if (priv->dma_cap.rx_fifo_size &&
         priv->plat->rx_fifo_size > priv->dma_cap.rx_fifo_size)
         priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;

Same as others.

Thank you,

---
Best Regards
Kunihiko Hayashi

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

[Russell King (Oracle)]

On Thu, Jan 23, 2025 at 02:25:15PM +0900, Kunihiko Hayashi wrote:
> Hi Russell,
> 
> Thank you for your comment.
> 
> On 2025/01/21 19:25, Russell King (Oracle) wrote:
> > On Tue, Jan 21, 2025 at 01:41:35PM +0900, Kunihiko Hayashi 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 the hardware capabilities, limit to
> > > the values from the capabilities.
> > > 
> > > And this sets hardware capability values if FIFO sizes are not specified
> > > and removes redundant lines.
> > 
> > I think you also indeed to explain why (and possibly understand) - if
> > there are hardware capabilities that describe these parameters - it has
> > been necessary to have them in firmware as well.
> > 
> > There are two scenarios I can think of why these would be duplicated:
> > 
> > 1. firmware/platform capabilities are there to correct wrong values
> >     provided by the hardware.
> > 2. firmware/platform capabilities are there to reduce the parameters
> >     below hardware maximums.
> > 
> > Which it is affects whether your patch is correct or not, and thus needs
> > to be mentioned.
> 
> I think scenario 2 applies in this case.

In light of my other reply
(https://lore.kernel.org/r/Z4_ZilVFKacuAUE8@shell.armlinux.org.uk) I
don't think either of my two above applies, and the driver is designed
to allow platform code to override the hardware value, or to provide
the value if there is no hardware value.

My suggestion, therefore, would be (e.g.):

	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 exceeds dma capability (%d)\n",
			 priv->plat->rx_fifo_size);
		priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
	}

if we still want to limit it to the hardware provided capability, where
that is provided.

-- 
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 v2 0/3] Limit devicetree parameters to hardware capability

[Kunihiko Hayashi]

Hi Russell,

On 2025/01/24 1:31, Russell King (Oracle) wrote:
> On Thu, Jan 23, 2025 at 02:25:15PM +0900, Kunihiko Hayashi wrote:
>> Hi Russell,
>>
>> Thank you for your comment.
>>
>> On 2025/01/21 19:25, Russell King (Oracle) wrote:
>>> On Tue, Jan 21, 2025 at 01:41:35PM +0900, Kunihiko Hayashi 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 the hardware capabilities,
> limit to
>>>> the values from the capabilities.
>>>>
>>>> And this sets hardware capability values if FIFO sizes are not
> specified
>>>> and removes redundant lines.
>>>
>>> I think you also indeed to explain why (and possibly understand) - if
>>> there are hardware capabilities that describe these parameters - it
> has
>>> been necessary to have them in firmware as well.
>>>
>>> There are two scenarios I can think of why these would be duplicated:
>>>
>>> 1. firmware/platform capabilities are there to correct wrong values
>>>      provided by the hardware.
>>> 2. firmware/platform capabilities are there to reduce the parameters
>>>      below hardware maximums.
>>>
>>> Which it is affects whether your patch is correct or not, and thus
> needs
>>> to be mentioned.
>>
>> I think scenario 2 applies in this case.
> 
> In light of my other reply
> (https://lore.kernel.org/r/Z4_ZilVFKacuAUE8@shell.armlinux.org.uk) I
> don't think either of my two above applies, and the driver is designed
> to allow platform code to override the hardware value, or to provide
> the value if there is no hardware value.

I understand. Especially I realized that I had to care about some hardwares
not having these values.

> My suggestion, therefore, would be (e.g.):
> 
> 	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 exceeds dma capability (%d)\n",
> 			 priv->plat->rx_fifo_size);
> 		priv->plat->rx_fifo_size = priv->dma_cap.rx_fifo_size;
> 	}
> 
> if we still want to limit it to the hardware provided capability, where
> that is provided.

Thank you for your suggestion. I also came up with the same code in:
https://lore.kernel.org/all/c2aa354d-1bd5-4fb0-aa8b8-48fcce3c1628@socionext.com/#t

I'll reflect this code to the next.

Thank you,

---
Best Regards
Kunihiko Hayashi