diff mbox series

[net-next,v2,3/3] net: stmmac: Configure AXI on Tegra234 MGBE

Message ID 20240202-stmmac-axi-config-v2-3-64eab2bab17b@nvidia.com
State New
Headers
Series [net-next,v2,1/3] net: stmmac: Pass resources to DT parsing code |

Commit Message

Thierry Reding Feb. 2, 2024, 11:53 a.m. UTC 
  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(+)

Comments

Serge Semin Feb. 5, 2024, 12:44 a.m. UTC | #1 
On Fri, Feb 02, 2024 at 12:53:35PM +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);

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.

-Serge(y)

> 
> -- 
> 2.43.0
> 
>
Thierry Reding Feb. 13, 2024, 3:51 p.m. UTC | #2 
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>
> > 
> > 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);
>
> The entire series can be converted to just a few lines of change:

Sorry for the delay, I missed this reply.

>  	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.

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
diff mbox series

Patch

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);