[net-next,v2,1/2] net: stmmac: Only enable enhanced addressing mode when needed
diff mbox series

Message ID 20190909152546.383-1-thierry.reding@gmail.com
State New
Headers show
Series
  • [net-next,v2,1/2] net: stmmac: Only enable enhanced addressing mode when needed
Related show

Commit Message

Thierry Reding Sept. 9, 2019, 3:25 p.m. UTC
From: Thierry Reding <treding@nvidia.com>

Enhanced addressing mode is only required when more than 32 bits need to
be addressed. Add a DMA configuration parameter to enable this mode only
when needed.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c | 5 ++++-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c  | 6 ++++++
 include/linux/stmmac.h                             | 1 +
 3 files changed, 11 insertions(+), 1 deletion(-)

Comments

Jose Abreu Sept. 9, 2019, 4:07 p.m. UTC | #1
From: Thierry Reding <thierry.reding@gmail.com>
Date: Sep/09/2019, 16:25:45 (UTC+00:00)

> @@ -92,6 +92,7 @@ struct stmmac_dma_cfg {
>  	int fixed_burst;
>  	int mixed_burst;
>  	bool aal;
> +	bool eame;

bools should not be used in struct's, please change to int.

---
Thanks,
Jose Miguel Abreu
Thierry Reding Sept. 9, 2019, 7:11 p.m. UTC | #2
On Mon, Sep 09, 2019 at 04:07:04PM +0000, Jose Abreu wrote:
> From: Thierry Reding <thierry.reding@gmail.com>
> Date: Sep/09/2019, 16:25:45 (UTC+00:00)
> 
> > @@ -92,6 +92,7 @@ struct stmmac_dma_cfg {
> >  	int fixed_burst;
> >  	int mixed_burst;
> >  	bool aal;
> > +	bool eame;
> 
> bools should not be used in struct's, please change to int.

Huh? Since when? "aal" right above it is also bool. Can you provide a
specific rationale for why we shouldn't use bool in structs?

Thierry
Jose Abreu Sept. 10, 2019, 8:32 a.m. UTC | #3
From: Thierry Reding <thierry.reding@gmail.com>
Date: Sep/09/2019, 20:11:27 (UTC+00:00)

> On Mon, Sep 09, 2019 at 04:07:04PM +0000, Jose Abreu wrote:
> > From: Thierry Reding <thierry.reding@gmail.com>
> > Date: Sep/09/2019, 16:25:45 (UTC+00:00)
> > 
> > > @@ -92,6 +92,7 @@ struct stmmac_dma_cfg {
> > >  	int fixed_burst;
> > >  	int mixed_burst;
> > >  	bool aal;
> > > +	bool eame;
> > 
> > bools should not be used in struct's, please change to int.
> 
> Huh? Since when? "aal" right above it is also bool. Can you provide a
> specific rationale for why we shouldn't use bool in structs?

Please see https://lkml.org/lkml/2017/11/21/384.

---
Thanks,
Jose 
Miguel Abreu
Thierry Reding Sept. 10, 2019, 1:54 p.m. UTC | #4
On Tue, Sep 10, 2019 at 08:32:38AM +0000, Jose Abreu wrote:
> From: Thierry Reding <thierry.reding@gmail.com>
> Date: Sep/09/2019, 20:11:27 (UTC+00:00)
> 
> > On Mon, Sep 09, 2019 at 04:07:04PM +0000, Jose Abreu wrote:
> > > From: Thierry Reding <thierry.reding@gmail.com>
> > > Date: Sep/09/2019, 16:25:45 (UTC+00:00)
> > > 
> > > > @@ -92,6 +92,7 @@ struct stmmac_dma_cfg {
> > > >  	int fixed_burst;
> > > >  	int mixed_burst;
> > > >  	bool aal;
> > > > +	bool eame;
> > > 
> > > bools should not be used in struct's, please change to int.
> > 
> > Huh? Since when? "aal" right above it is also bool. Can you provide a
> > specific rationale for why we shouldn't use bool in structs?
> 
> Please see https://lkml.org/lkml/2017/11/21/384.

The context is slightly different here. stmmac_dma_cfg exists once for
each of these ethernet devices in the system, and I would assume that in
the vast majority of cases there's exactly one such device in the system
so the potential size increase is very small. On the other hand, there
are potentially very many struct sched_dl_entity, so the size impact is
multiplied.

Anyway, if you insist I'll rewrite this to use an unsigned int bitfield.

Thierry
Jose Abreu Sept. 10, 2019, 5:25 p.m. UTC | #5
From: Thierry Reding <thierry.reding@gmail.com>
Date: Sep/10/2019, 14:54:27 (UTC+00:00)

> On Tue, Sep 10, 2019 at 08:32:38AM +0000, Jose Abreu wrote:
> > From: Thierry Reding <thierry.reding@gmail.com>
> > Date: Sep/09/2019, 20:11:27 (UTC+00:00)
> > 
> > > On Mon, Sep 09, 2019 at 04:07:04PM +0000, Jose Abreu wrote:
> > > > From: Thierry Reding <thierry.reding@gmail.com>
> > > > Date: Sep/09/2019, 16:25:45 (UTC+00:00)
> > > > 
> > > > > @@ -92,6 +92,7 @@ struct stmmac_dma_cfg {
> > > > >  	int fixed_burst;
> > > > >  	int mixed_burst;
> > > > >  	bool aal;
> > > > > +	bool eame;
> > > > 
> > > > bools should not be used in struct's, please change to int.
> > > 
> > > Huh? Since when? "aal" right above it is also bool. Can you provide a
> > > specific rationale for why we shouldn't use bool in structs?
> > 
> > Please see https://lkml.org/lkml/2017/11/21/384.
> 
> The context is slightly different here. stmmac_dma_cfg exists once for
> each of these ethernet devices in the system, and I would assume that in
> the vast majority of cases there's exactly one such device in the system
> so the potential size increase is very small. On the other hand, there
> are potentially very many struct sched_dl_entity, so the size impact is
> multiplied.
> 
> Anyway, if you insist I'll rewrite this to use an unsigned int bitfield.

For new code I would rather prefer "int" but I guess it's up to David to 
decide this. I'm okay with both options as the check for this usage was 
removed in checkpatch: 
https://lkml.org/lkml/2019/1/10/975

---
Thanks,
Jose Miguel 
Abreu

Patch
diff mbox series

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
index 64956465c030..3e00fd8befcf 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwxgmac2_dma.c
@@ -27,7 +27,10 @@  static void dwxgmac2_dma_init(void __iomem *ioaddr,
 	if (dma_cfg->aal)
 		value |= XGMAC_AAL;
 
-	writel(value | XGMAC_EAME, ioaddr + XGMAC_DMA_SYSBUS_MODE);
+	if (dma_cfg->eame)
+		value |= XGMAC_EAME;
+
+	writel(value, ioaddr + XGMAC_DMA_SYSBUS_MODE);
 }
 
 static void dwxgmac2_dma_init_chan(void __iomem *ioaddr,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 06ccd216ae90..ecd461207dbc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -4497,6 +4497,12 @@  int stmmac_dvr_probe(struct device *device,
 		if (!ret) {
 			dev_info(priv->device, "Using %d bits DMA width\n",
 				 priv->dma_cap.addr64);
+
+			/*
+			 * If more than 32 bits can be addressed, make sure to
+			 * enable enhanced addressing mode.
+			 */
+			priv->plat->dma_cfg->eame = true;
 		} else {
 			ret = dma_set_mask_and_coherent(device, DMA_BIT_MASK(32));
 			if (ret) {
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index 7ad7ae35cf88..d300ac907c76 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -92,6 +92,7 @@  struct stmmac_dma_cfg {
 	int fixed_burst;
 	int mixed_burst;
 	bool aal;
+	bool eame;
 };
 
 #define AXI_BLEN	7