| Message ID | 20250121044138.2883912-3-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Limit devicetree parameters to hardware capability | expand |
在 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
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.
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 >
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
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;
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(+)