Received-SPF: pass (google.com: domain of linux-kernel+bounces-71810-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 19 Feb 2024 21:32:43 +0300
From: Serge Semin <fancer.lancer@gmail.com>
To: Thierry Reding <thierry.reding@gmail.com>
Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>, 
	Jose Abreu <joabreu@synopsys.com>, "David S. Miller" <davem@davemloft.net>, 
	Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, 
	Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, 
	linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-tegra@vger.kernel.org, 
	Thierry Reding <treding@nvidia.com>
Subject: Re: [PATCH net-next v3 3/3] net: stmmac: Configure AXI on Tegra234
 MGBE
Message-ID: <xne2i6jwqaptsrd2hjdahxbscysgtj7iabqendyjb75fnrjc5z@js7n7qngtzym>
References: <20240219-stmmac-axi-config-v3-0-fca7f046e6ee@nvidia.com>
 <20240219-stmmac-axi-config-v3-3-fca7f046e6ee@nvidia.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240219-stmmac-axi-config-v3-3-fca7f046e6ee@nvidia.com>

On Mon, Feb 19, 2024 at 05:46:06PM +0100, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Allow the device to use bursts and increase the maximum number of
> outstanding requests to improve performance. Measurements show an
> increase in throughput of around 5x on a 1 Gbps link.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> index bab57d1675df..b6bfa48f279d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c
> @@ -199,6 +199,12 @@ static void mgbe_uphy_lane_bringup_serdes_down(struct net_device *ndev, void *mg
>  	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
>  }
>  
> +static const struct stmmac_axi tegra234_mgbe_axi = {
> +	.axi_wr_osr_lmt = 63,
> +	.axi_rd_osr_lmt = 63,
> +	.axi_blen = { 256, },
> +};
> +
>  static int tegra_mgbe_probe(struct platform_device *pdev)
>  {
>  	struct plat_stmmacenet_data *plat;
> @@ -284,6 +290,9 @@ static int tegra_mgbe_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		goto disable_clks;
>  
> +	/* setup default AXI configuration */
> +	res.axi = &tegra234_mgbe_axi;
> +
>  	plat = devm_stmmac_probe_config_dt(pdev, &res);
>  	if (IS_ERR(plat)) {
>  		err = PTR_ERR(plat);

Let's get back to the v2 discussion:

On Mon Feb 5, 2024 at 1:44 AM CET, Serge Semin wrote:
> The entire series can be converted to just a few lines of change:
>     plat = devm_stmmac_probe_config_dt(pdev, res.mac);
>     if (IS_ERR(plat)) {
>             err = PTR_ERR(plat);
>             goto disable_clks;
>     }
> +
> +   if (IS_ERR_OR_NULL(plat->axi)) {
> +           plat->axi = devm_kzalloc(&pdev->dev, sizeof(*axi), GFP_KERNEL);
> +           if (!plat->axi) {
> +                   ret = -ENOMEM;
> +                   goto disable_clks;
> +           }
> +   } /* else memset plat->axi with zeros if you wish */
> +
> +   plat->axi->axi_wr_osr_lmt = 63;
> +   plat->axi->axi_rd_osr_lmt = 63;
> +   plat->axi->axi_blen[0] = 256;
>  
>     plat->has_xgmac = 1;
>     plat->flags |= STMMAC_FLAG_TSO_EN;
>     plat->pmt = 1;
>
> Please don't overcomplicate the already overcomplicated driver with a
> functionality which can be reached by the default one. In this case
> the easiest way is to let the generic code work and then
> override/replace/fix/etc the retrieved values. Thus there won't be
> need in adding the redundant functionality and keep the generic
> DT-platform code a bit simpler to read.

You responded with:

On Tue, Feb 13, 2024 at 04:51:34PM +0100, Thierry Reding wrote:
> I'm not sure I understand how this is overcomplicating things. The code
> is pretty much unchanged, except that the AXI configuration can now have
> driver-specified defaults before the DT is parsed. Perhaps I need to add
> comments to make that a bit clearer?
> 
> While your version is certainly simpler it has the drawback that it no
> longer allows the platform defaults to be overridden in device tree. I
> would prefer if the defaults can be derived from the compatible string
> but if need be for those defaults to still be overridable from device
> tree.

Currently available functionality is easier to read and understand: by
default the data is retrieved from the DT, if no AXI DT-node found you
can allocate/create your own AXI-configs, if there is AXI DT-node you
can fix it up in whatever way your wish. Thus the default behavior is
straightforward. You on the contrary suggest to add an additional
field to the resources structure which would need to be merged in with
the data retrieved from DT. It makes the stmmac_axi_setup() method and
the entire logic more complex and thus harder to comprehend. The
driver is already overwhelmed with flags and private/platform data
fixing the code here and there (see plat_stmmacenet_data, it's a
madness). So please justify in more details why do you need one more
complexity added instead of:
1. overriding the AXI-configs retrieved from DT,
2. updating DT on your platform
?

-Serge(y)

> 
> -- 
> 2.43.2
>