| Message ID | 20250121044138.2883912-1-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Limit devicetree parameters to hardware capability | expand |
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.
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
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.
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
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(-)