Received-SPF: pass (google.com: domain of linux-kernel+bounces-63834-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Content-Type: multipart/signed;
 boundary=cfa596c0b51d52dd3aecf0f90e9ae3c845fbf778d202ed71698f391c8fdb;
 micalg=pgp-sha256; protocol="application/pgp-signature"
Precedence: bulk
Mime-Version: 1.0
Date: Tue, 13 Feb 2024 16:51:34 +0100
Message-Id: <CZ42KOP6ECUC.EEIHWA147NV9@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>
Subject: Re: [PATCH net-next v2 3/3] net: stmmac: Configure AXI on Tegra234
 MGBE
From: "Thierry Reding" <thierry.reding@gmail.com>
To: "Serge Semin" <fancer.lancer@gmail.com>
References: <20240202-stmmac-axi-config-v2-0-64eab2bab17b@nvidia.com>
 <20240202-stmmac-axi-config-v2-3-64eab2bab17b@nvidia.com>
 <uzzzxx3mv6yoslijhhzdzyossvcvi52jgbulza54uqh2wrm5kd@ddd5o56b2dhu>
In-Reply-To: <uzzzxx3mv6yoslijhhzdzyossvcvi52jgbulza54uqh2wrm5kd@ddd5o56b2dhu>

--cfa596c0b51d52dd3aecf0f90e9ae3c845fbf778d202ed71698f391c8fdb
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8

On Mon Feb 5, 2024 at 1:44 AM CET, Serge Semin wrote:
> On Fri, Feb 02, 2024 at 12:53:35PM +0100, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >=20
> > 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.
> >=20
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >=20
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-tegra.c b/driver=
s/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(str=
uct net_device *ndev, void *mg
> >  	writel(value, mgbe->xpcs + XPCS_WRAP_UPHY_RX_CONTROL);
> >  }
> > =20
> > +static const struct stmmac_axi tegra234_mgbe_axi =3D {
> > +	.axi_wr_osr_lmt =3D 63,
> > +	.axi_rd_osr_lmt =3D 63,
> > +	.axi_blen =3D { 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;
> > =20
> > +	/* setup default AXI configuration */
> > +	res.axi =3D &tegra234_mgbe_axi;
> > +
> >  	plat =3D devm_stmmac_probe_config_dt(pdev, &res);
> >  	if (IS_ERR(plat)) {
> >  		err =3D PTR_ERR(plat);
>
> The entire series can be converted to just a few lines of change:

Sorry for the delay, I missed this reply.

>  	plat =3D devm_stmmac_probe_config_dt(pdev, res.mac);
>  	if (IS_ERR(plat)) {
>  		err =3D PTR_ERR(plat);
>  		goto disable_clks;
>  	}
> +
> +	if (IS_ERR_OR_NULL(plat->axi)) {
> +		plat->axi =3D devm_kzalloc(&pdev->dev, sizeof(*axi), GFP_KERNEL);
> +		if (!plat->axi) {
> +			ret =3D -ENOMEM;
> +			goto disable_clks;
> +		}
> +	} /* else memset plat->axi with zeros if you wish */
> +
> +	plat->axi->axi_wr_osr_lmt =3D 63;
> +	plat->axi->axi_rd_osr_lmt =3D 63;
> +	plat->axi->axi_blen[0] =3D 256;
> =20
>  	plat->has_xgmac =3D 1;
>  	plat->flags |=3D STMMAC_FLAG_TSO_EN;
>  	plat->pmt =3D 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.

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.

Thierry

--cfa596c0b51d52dd3aecf0f90e9ae3c845fbf778d202ed71698f391c8fdb
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAmXLkAcACgkQ3SOs138+
s6HY6g//TyBX07vSPy79ApF01cK74bhK4woNCTlScH1jk/Pky8il9Yw+GrXfnnEk
9d6LiXfIWLHIvPLS4xcvwM0dT/FTTdDjgS41G2X0p06Bn7BGTHVJ+xuPjKh6yRuv
mVbYhtqUJynPHkT9R3OIBKZ6E3nTNGPp8ocpAe82uN6Opl1QI4BRJ8syKYciUcHj
REtkb1HZBXlxYhnWlFCHEsyD+mNdnZ1Z1mD50lauYCdkjhu/GnL8Czt1ScCy8q/s
URZ/Ft5r+6M8aBVyfRfR+r9XalZiRtxksOL2Ova79dlistCGl8S2NNWw7M3vxHPO
HvfhWMHyf0ywdQXoYEhTW/pt98X+fJPtIhnCfSrdOac58+WRSCiKekarM2WkS4ma
lUm4Azh/xK5z4zexVXB6j5Szgb9Hz/u3twxKvnNxu/frAJL7WesujOUy2RR+g2CM
KI3xDH9t2yuWE45vhv2MGAuPEeAtVmG5R2OxV7duuCXHCsY7njs6bZLyT1nX6eC1
ON9D4tsIj6TGo51UPokLNU04KzDB/D2xEqzqeV1GUuEXe/CsJIey76JpFT0UvJry
D1/UtEp+lAczY85TRiILc6z+NqSCq4p3kEqi+txrfr806bkHfBmRz/ZxlZbgrYPZ
ijRgUxzyXZxcL0ci0o8g+YFcoXfTJhD5cTt8NvBoolThfwHM/aY=
=vhC9
-----END PGP SIGNATURE-----

--cfa596c0b51d52dd3aecf0f90e9ae3c845fbf778d202ed71698f391c8fdb--