[PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The DWMAC 4.10 supports the same enhanced addressing mode as later
generations. Parse this capability from the hardware feature registers
and set the EAME (Enhanced Addressing Mode Enable) bit when necessary.

Thierry

Thierry Reding (2):
  net: stmmac: Only enable enhanced addressing mode when needed
  net: stmmac: Support enhanced addressing mode for DWMAC 4.10

 drivers/net/ethernet/stmicro/stmmac/dwmac4.h  |  1 +
 .../ethernet/stmicro/stmmac/dwmac4_descs.c    |  4 ++--
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  | 22 +++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.h  |  3 +++
 .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    |  5 ++++-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  6 +++++
 include/linux/stmmac.h                        |  1 +
 7 files changed, 39 insertions(+), 3 deletions(-)

-- 
2.23.0

[PATCH v3 1/2] net: stmmac: Only enable enhanced addressing mode when needed

[Thierry Reding]

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

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

[PATCH v3 2/2] net: stmmac: Support enhanced addressing mode for DWMAC 4.10

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The address width of the controller can be read from hardware feature
registers much like on XGMAC. Add support for parsing the ADDR64 field
so that the DMA mask can be set accordingly.

This avoids getting swiotlb involved for DMA on Tegra186 and later.

Also make sure that the upper 32 bits of the DMA address are written to
the DMA descriptors when enhanced addressing mode is used. Similarily,
for each channel, the upper 32 bits of the DMA descriptor ring's base
address also need to be programmed to make sure the correct memory can
be fetched when the DMA descriptor ring is located beyond the 32-bit
boundary.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v3:
- unconditionally write upper 32 bits

Changes in v2:
- also program the upper 32 bits of the DMA descriptor base address for
  each channel

 drivers/net/ethernet/stmicro/stmmac/dwmac4.h  |  1 +
 .../ethernet/stmicro/stmmac/dwmac4_descs.c    |  4 ++--
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  | 22 +++++++++++++++++++
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.h  |  3 +++
 4 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
index 2ed11a581d80..f634fa09dffc 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4.h
@@ -183,6 +183,7 @@ enum power_event {
 #define GMAC_HW_HASH_TB_SZ		GENMASK(25, 24)
 #define GMAC_HW_FEAT_AVSEL		BIT(20)
 #define GMAC_HW_TSOEN			BIT(18)
+#define GMAC_HW_ADDR64			GENMASK(15, 14)
 #define GMAC_HW_TXFIFOSIZE		GENMASK(10, 6)
 #define GMAC_HW_RXFIFOSIZE		GENMASK(4, 0)
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
index dbde23e7e169..d546041d2fcd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
@@ -431,8 +431,8 @@ static void dwmac4_get_addr(struct dma_desc *p, unsigned int *addr)
 
 static void dwmac4_set_addr(struct dma_desc *p, dma_addr_t addr)
 {
-	p->des0 = cpu_to_le32(addr);
-	p->des1 = 0;
+	p->des0 = cpu_to_le32(lower_32_bits(addr));
+	p->des1 = cpu_to_le32(upper_32_bits(addr));
 }
 
 static void dwmac4_clear(struct dma_desc *p)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
index 3ed5508586ef..8439dd84f786 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c
@@ -79,6 +79,7 @@ static void dwmac4_dma_init_rx_chan(void __iomem *ioaddr,
 	value = value | (rxpbl << DMA_BUS_MODE_RPBL_SHIFT);
 	writel(value, ioaddr + DMA_CHAN_RX_CONTROL(chan));
 
+	writel(upper_32_bits(dma_rx_phy), ioaddr + DMA_CHAN_RX_BASE_ADDR_HI(chan));
 	writel(lower_32_bits(dma_rx_phy), ioaddr + DMA_CHAN_RX_BASE_ADDR(chan));
 }
 
@@ -97,6 +98,7 @@ static void dwmac4_dma_init_tx_chan(void __iomem *ioaddr,
 
 	writel(value, ioaddr + DMA_CHAN_TX_CONTROL(chan));
 
+	writel(upper_32_bits(dma_tx_phy), ioaddr + DMA_CHAN_TX_BASE_ADDR_HI(chan));
 	writel(lower_32_bits(dma_tx_phy), ioaddr + DMA_CHAN_TX_BASE_ADDR(chan));
 }
 
@@ -132,6 +134,9 @@ static void dwmac4_dma_init(void __iomem *ioaddr,
 	if (dma_cfg->aal)
 		value |= DMA_SYS_BUS_AAL;
 
+	if (dma_cfg->eame)
+		value |= DMA_SYS_BUS_EAME;
+
 	writel(value, ioaddr + DMA_SYS_BUS_MODE);
 }
 
@@ -354,6 +359,23 @@ static void dwmac4_get_hw_feature(void __iomem *ioaddr,
 	dma_cap->hash_tb_sz = (hw_cap & GMAC_HW_HASH_TB_SZ) >> 24;
 	dma_cap->av = (hw_cap & GMAC_HW_FEAT_AVSEL) >> 20;
 	dma_cap->tsoen = (hw_cap & GMAC_HW_TSOEN) >> 18;
+
+	dma_cap->addr64 = (hw_cap & GMAC_HW_ADDR64) >> 14;
+	switch (dma_cap->addr64) {
+	case 0:
+		dma_cap->addr64 = 32;
+		break;
+	case 1:
+		dma_cap->addr64 = 40;
+		break;
+	case 2:
+		dma_cap->addr64 = 48;
+		break;
+	default:
+		dma_cap->addr64 = 32;
+		break;
+	}
+
 	/* RX and TX FIFO sizes are encoded as log2(n / 128). Undo that by
 	 * shifting and store the sizes in bytes.
 	 */
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h
index b66da0237d2a..5299fa1001a3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.h
@@ -65,6 +65,7 @@
 #define DMA_SYS_BUS_MB			BIT(14)
 #define DMA_AXI_1KBBE			BIT(13)
 #define DMA_SYS_BUS_AAL			BIT(12)
+#define DMA_SYS_BUS_EAME		BIT(11)
 #define DMA_AXI_BLEN256			BIT(7)
 #define DMA_AXI_BLEN128			BIT(6)
 #define DMA_AXI_BLEN64			BIT(5)
@@ -91,7 +92,9 @@
 #define DMA_CHAN_CONTROL(x)		DMA_CHANX_BASE_ADDR(x)
 #define DMA_CHAN_TX_CONTROL(x)		(DMA_CHANX_BASE_ADDR(x) + 0x4)
 #define DMA_CHAN_RX_CONTROL(x)		(DMA_CHANX_BASE_ADDR(x) + 0x8)
+#define DMA_CHAN_TX_BASE_ADDR_HI(x)	(DMA_CHANX_BASE_ADDR(x) + 0x10)
 #define DMA_CHAN_TX_BASE_ADDR(x)	(DMA_CHANX_BASE_ADDR(x) + 0x14)
+#define DMA_CHAN_RX_BASE_ADDR_HI(x)	(DMA_CHANX_BASE_ADDR(x) + 0x18)
 #define DMA_CHAN_RX_BASE_ADDR(x)	(DMA_CHANX_BASE_ADDR(x) + 0x1c)
 #define DMA_CHAN_TX_END_ADDR(x)		(DMA_CHANX_BASE_ADDR(x) + 0x20)
 #define DMA_CHAN_RX_END_ADDR(x)		(DMA_CHANX_BASE_ADDR(x) + 0x28)
-- 
2.23.0

Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Florian Fainelli]

On 9/20/19 10:00 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The DWMAC 4.10 supports the same enhanced addressing mode as later
> generations. Parse this capability from the hardware feature registers
> and set the EAME (Enhanced Addressing Mode Enable) bit when necessary.

Do you think these two patches should have companion Fixes: tag? They
are definitively bug fixes, but maybe you would also want those to be
back ported to -stable trees?

> 
> Thierry
> 
> Thierry Reding (2):
>   net: stmmac: Only enable enhanced addressing mode when needed
>   net: stmmac: Support enhanced addressing mode for DWMAC 4.10
> 
>  drivers/net/ethernet/stmicro/stmmac/dwmac4.h  |  1 +
>  .../ethernet/stmicro/stmmac/dwmac4_descs.c    |  4 ++--
>  .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  | 22 +++++++++++++++++++
>  .../net/ethernet/stmicro/stmmac/dwmac4_dma.h  |  3 +++
>  .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    |  5 ++++-
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  6 +++++
>  include/linux/stmmac.h                        |  1 +
>  7 files changed, 39 insertions(+), 3 deletions(-)
> 


-- 
Florian

Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1543 bytes --]

On Fri, Sep 20, 2019 at 10:02:28AM -0700, Florian Fainelli wrote:
> On 9/20/19 10:00 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The DWMAC 4.10 supports the same enhanced addressing mode as later
> > generations. Parse this capability from the hardware feature registers
> > and set the EAME (Enhanced Addressing Mode Enable) bit when necessary.
> 
> Do you think these two patches should have companion Fixes: tag? They
> are definitively bug fixes, but maybe you would also want those to be
> back ported to -stable trees?

I wouldn't really consider these bug fixes. They're more along the lines
of feature additions. The driver previously didn't support EAME and that
was fine. The fact that it never worked under specific circumstances is
not a bug or regression, really.

Thierry

> > Thierry Reding (2):
> >   net: stmmac: Only enable enhanced addressing mode when needed
> >   net: stmmac: Support enhanced addressing mode for DWMAC 4.10
> > 
> >  drivers/net/ethernet/stmicro/stmmac/dwmac4.h  |  1 +
> >  .../ethernet/stmicro/stmmac/dwmac4_descs.c    |  4 ++--
> >  .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  | 22 +++++++++++++++++++
> >  .../net/ethernet/stmicro/stmmac/dwmac4_dma.h  |  3 +++
> >  .../ethernet/stmicro/stmmac/dwxgmac2_dma.c    |  5 ++++-
> >  .../net/ethernet/stmicro/stmmac/stmmac_main.c |  6 +++++
> >  include/linux/stmmac.h                        |  1 +
> >  7 files changed, 39 insertions(+), 3 deletions(-)
> > 
> 
> 
> -- 
> Florian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[David Miller]

From: Thierry Reding <thierry.reding@gmail.com>
Date: Fri, 20 Sep 2019 19:00:34 +0200

> From: Thierry Reding <treding@nvidia.com>
> 
> The DWMAC 4.10 supports the same enhanced addressing mode as later
> generations. Parse this capability from the hardware feature registers
> and set the EAME (Enhanced Addressing Mode Enable) bit when necessary.

This looks like an enhancement and/or optimization rather than a bug fix.

Also, you're now writing to the high 32-bits unconditionally, even when
it will always be zero because of 32-bit addressing.  That looks like
a step backwards to me.

I'm not applying this.

RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Jose Abreu]

From: David Miller <davem@davemloft.net>
Date: Sep/24/2019, 20:45:08 (UTC+00:00)

> From: Thierry Reding <thierry.reding@gmail.com>
> Date: Fri, 20 Sep 2019 19:00:34 +0200
> 
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The DWMAC 4.10 supports the same enhanced addressing mode as later
> > generations. Parse this capability from the hardware feature registers
> > and set the EAME (Enhanced Addressing Mode Enable) bit when necessary.
> 
> This looks like an enhancement and/or optimization rather than a bug fix.

Agree.

> Also, you're now writing to the high 32-bits unconditionally, even when
> it will always be zero because of 32-bit addressing.  That looks like
> a step backwards to me.

Don't agree. As per previous discussions and as per my IP knowledge, if 
EAME is not enabled / not supported the register can still be written. 
This is not fast path and will not impact any remaining operation. Can 
you please explain what exactly is the concern about this ?

Anyway, this is an important feature for performance so I hope Thierry 
re-submits this once -next opens and addressing the review comments.

---
Thanks,
Jose Miguel Abreu

Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1075 bytes --]

On Tue, Sep 24, 2019 at 09:45:08PM +0200, David Miller wrote:
> From: Thierry Reding <thierry.reding@gmail.com>
> Date: Fri, 20 Sep 2019 19:00:34 +0200
> 
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The DWMAC 4.10 supports the same enhanced addressing mode as later
> > generations. Parse this capability from the hardware feature registers
> > and set the EAME (Enhanced Addressing Mode Enable) bit when necessary.
> 
> This looks like an enhancement and/or optimization rather than a bug fix.
> 
> Also, you're now writing to the high 32-bits unconditionally, even when
> it will always be zero because of 32-bit addressing.  That looks like
> a step backwards to me.
> 
> I'm not applying this.

Sounds like you would prefer v2 of this:

	https://patchwork.ozlabs.org/project/netdev/list/?series=129768&state=*

While v2 didn't have a cover letter, it did write the upper 32 bits
conditionally.

Do you want to pick that up instead, or do you want me to send out a v4
with the cover letter from v3 and the patches from v2?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[David Miller]

From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Wed, 25 Sep 2019 10:44:53 +0000

> From: David Miller <davem@davemloft.net>
> Date: Sep/24/2019, 20:45:08 (UTC+00:00)
> 
>> From: Thierry Reding <thierry.reding@gmail.com>
>> Date: Fri, 20 Sep 2019 19:00:34 +0200
>> 
>> Also, you're now writing to the high 32-bits unconditionally, even when
>> it will always be zero because of 32-bit addressing.  That looks like
>> a step backwards to me.
> 
> Don't agree. As per previous discussions and as per my IP knowledge, if 
> EAME is not enabled / not supported the register can still be written. 
> This is not fast path and will not impact any remaining operation. Can 
> you please explain what exactly is the concern about this ?
> 
> Anyway, this is an important feature for performance so I hope Thierry 
> re-submits this once -next opens and addressing the review comments.

Perhaps I misunderstand the context, isn't this code writing the
descriptors for every packet?

RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Jose Abreu]

From: David Miller <davem@davemloft.net>
Date: Sep/25/2019, 12:33:53 (UTC+00:00)

> From: Jose Abreu <Jose.Abreu@synopsys.com>
> Date: Wed, 25 Sep 2019 10:44:53 +0000
> 
> > From: David Miller <davem@davemloft.net>
> > Date: Sep/24/2019, 20:45:08 (UTC+00:00)
> > 
> >> From: Thierry Reding <thierry.reding@gmail.com>
> >> Date: Fri, 20 Sep 2019 19:00:34 +0200
> >> 
> >> Also, you're now writing to the high 32-bits unconditionally, even when
> >> it will always be zero because of 32-bit addressing.  That looks like
> >> a step backwards to me.
> > 
> > Don't agree. As per previous discussions and as per my IP knowledge, if 
> > EAME is not enabled / not supported the register can still be written. 
> > This is not fast path and will not impact any remaining operation. Can 
> > you please explain what exactly is the concern about this ?
> > 
> > Anyway, this is an important feature for performance so I hope Thierry 
> > re-submits this once -next opens and addressing the review comments.
> 
> Perhaps I misunderstand the context, isn't this code writing the
> descriptors for every packet?

No, its just setting up the base address for the descriptors which is 
done in open(). The one that's in the fast path is the tail address, 
which is always the lower 32 bits.

---
Thanks,
Jose Miguel Abreu

RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Jose Abreu]

From: Jose Abreu <joabreu@synopsys.com>
Date: Sep/25/2019, 12:41:04 (UTC+00:00)

> From: David Miller <davem@davemloft.net>
> Date: Sep/25/2019, 12:33:53 (UTC+00:00)
> 
> > From: Jose Abreu <Jose.Abreu@synopsys.com>
> > Date: Wed, 25 Sep 2019 10:44:53 +0000
> > 
> > > From: David Miller <davem@davemloft.net>
> > > Date: Sep/24/2019, 20:45:08 (UTC+00:00)
> > > 
> > >> From: Thierry Reding <thierry.reding@gmail.com>
> > >> Date: Fri, 20 Sep 2019 19:00:34 +0200
> > >> 
> > >> Also, you're now writing to the high 32-bits unconditionally, even when
> > >> it will always be zero because of 32-bit addressing.  That looks like
> > >> a step backwards to me.
> > > 
> > > Don't agree. As per previous discussions and as per my IP knowledge, if 
> > > EAME is not enabled / not supported the register can still be written. 
> > > This is not fast path and will not impact any remaining operation. Can 
> > > you please explain what exactly is the concern about this ?
> > > 
> > > Anyway, this is an important feature for performance so I hope Thierry 
> > > re-submits this once -next opens and addressing the review comments.
> > 
> > Perhaps I misunderstand the context, isn't this code writing the
> > descriptors for every packet?
> 
> No, its just setting up the base address for the descriptors which is 
> done in open(). The one that's in the fast path is the tail address, 
> which is always the lower 32 bits.

Oops, sorry. Indeed it's done in refill operation in function 
dwmac4_set_addr() for rx/tx which is fast path so you do have a point 
that I was not seeing. Thanks for bringing this up!

Now, the point would be:
	a) Is it faster to have an condition check in dwmac4_set_addr(), or
	b) Always write to descs the upper 32 bits. Which always exists in the 
IP and is a standard write to memory.

---
Thanks,
Jose Miguel Abreu

Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[David Miller]

From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Wed, 25 Sep 2019 11:41:04 +0000

> From: David Miller <davem@davemloft.net>
> Date: Sep/25/2019, 12:33:53 (UTC+00:00)
> 
>> From: Jose Abreu <Jose.Abreu@synopsys.com>
>> Date: Wed, 25 Sep 2019 10:44:53 +0000
>> 
>> > From: David Miller <davem@davemloft.net>
>> > Date: Sep/24/2019, 20:45:08 (UTC+00:00)
>> > 
>> >> From: Thierry Reding <thierry.reding@gmail.com>
>> >> Date: Fri, 20 Sep 2019 19:00:34 +0200
>> >> 
>> >> Also, you're now writing to the high 32-bits unconditionally, even when
>> >> it will always be zero because of 32-bit addressing.  That looks like
>> >> a step backwards to me.
>> > 
>> > Don't agree. As per previous discussions and as per my IP knowledge, if 
>> > EAME is not enabled / not supported the register can still be written. 
>> > This is not fast path and will not impact any remaining operation. Can 
>> > you please explain what exactly is the concern about this ?
>> > 
>> > Anyway, this is an important feature for performance so I hope Thierry 
>> > re-submits this once -next opens and addressing the review comments.
>> 
>> Perhaps I misunderstand the context, isn't this code writing the
>> descriptors for every packet?
> 
> No, its just setting up the base address for the descriptors which is 
> done in open(). The one that's in the fast path is the tail address, 
> which is always the lower 32 bits.

Aha, ok, yes then initializing both parts unconditionally is fine.

Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Florian Fainelli]

On 9/25/19 4:46 AM, Jose Abreu wrote:
> From: Jose Abreu <joabreu@synopsys.com>
> Date: Sep/25/2019, 12:41:04 (UTC+00:00)
> 
>> From: David Miller <davem@davemloft.net>
>> Date: Sep/25/2019, 12:33:53 (UTC+00:00)
>>
>>> From: Jose Abreu <Jose.Abreu@synopsys.com>
>>> Date: Wed, 25 Sep 2019 10:44:53 +0000
>>>
>>>> From: David Miller <davem@davemloft.net>
>>>> Date: Sep/24/2019, 20:45:08 (UTC+00:00)
>>>>
>>>>> From: Thierry Reding <thierry.reding@gmail.com>
>>>>> Date: Fri, 20 Sep 2019 19:00:34 +0200
>>>>>
>>>>> Also, you're now writing to the high 32-bits unconditionally, even when
>>>>> it will always be zero because of 32-bit addressing.  That looks like
>>>>> a step backwards to me.
>>>>
>>>> Don't agree. As per previous discussions and as per my IP knowledge, if 
>>>> EAME is not enabled / not supported the register can still be written. 
>>>> This is not fast path and will not impact any remaining operation. Can 
>>>> you please explain what exactly is the concern about this ?
>>>>
>>>> Anyway, this is an important feature for performance so I hope Thierry 
>>>> re-submits this once -next opens and addressing the review comments.
>>>
>>> Perhaps I misunderstand the context, isn't this code writing the
>>> descriptors for every packet?
>>
>> No, its just setting up the base address for the descriptors which is 
>> done in open(). The one that's in the fast path is the tail address, 
>> which is always the lower 32 bits.
> 
> Oops, sorry. Indeed it's done in refill operation in function 
> dwmac4_set_addr() for rx/tx which is fast path so you do have a point 
> that I was not seeing. Thanks for bringing this up!
> 
> Now, the point would be:
> 	a) Is it faster to have an condition check in dwmac4_set_addr(), or
> 	b) Always write to descs the upper 32 bits. Which always exists in the 
> IP and is a standard write to memory.

The way I would approach it (as done in bcmgenet.c) is that if the
platform both has CONFIG_PHYS_ADDR_T_64BIT=y and supports > 32-bits
addresses, then you write the upper 32-bits otherwise, you do not. Given
you indicate that the registers are safe to write regardless, then maybe
just the check on CONFIG_PHYS_ADDR_T_64BIT is enough for your case. The
rationale in my case is that register writes to on-chip descriptors are
fairly expensive (~200ns per operation) and get in the hot-path.

The CONFIG_PHYS_ADDR_T_64BIT check addresses both native 64-bit
platforms (e.g.: ARM64) and those that do support LPAE (ARM LPAE for
instance).
-- 
Florian

Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3251 bytes --]

On Wed, Sep 25, 2019 at 10:31:13AM -0700, Florian Fainelli wrote:
> On 9/25/19 4:46 AM, Jose Abreu wrote:
> > From: Jose Abreu <joabreu@synopsys.com>
> > Date: Sep/25/2019, 12:41:04 (UTC+00:00)
> > 
> >> From: David Miller <davem@davemloft.net>
> >> Date: Sep/25/2019, 12:33:53 (UTC+00:00)
> >>
> >>> From: Jose Abreu <Jose.Abreu@synopsys.com>
> >>> Date: Wed, 25 Sep 2019 10:44:53 +0000
> >>>
> >>>> From: David Miller <davem@davemloft.net>
> >>>> Date: Sep/24/2019, 20:45:08 (UTC+00:00)
> >>>>
> >>>>> From: Thierry Reding <thierry.reding@gmail.com>
> >>>>> Date: Fri, 20 Sep 2019 19:00:34 +0200
> >>>>>
> >>>>> Also, you're now writing to the high 32-bits unconditionally, even when
> >>>>> it will always be zero because of 32-bit addressing.  That looks like
> >>>>> a step backwards to me.
> >>>>
> >>>> Don't agree. As per previous discussions and as per my IP knowledge, if 
> >>>> EAME is not enabled / not supported the register can still be written. 
> >>>> This is not fast path and will not impact any remaining operation. Can 
> >>>> you please explain what exactly is the concern about this ?
> >>>>
> >>>> Anyway, this is an important feature for performance so I hope Thierry 
> >>>> re-submits this once -next opens and addressing the review comments.
> >>>
> >>> Perhaps I misunderstand the context, isn't this code writing the
> >>> descriptors for every packet?
> >>
> >> No, its just setting up the base address for the descriptors which is 
> >> done in open(). The one that's in the fast path is the tail address, 
> >> which is always the lower 32 bits.
> > 
> > Oops, sorry. Indeed it's done in refill operation in function 
> > dwmac4_set_addr() for rx/tx which is fast path so you do have a point 
> > that I was not seeing. Thanks for bringing this up!
> > 
> > Now, the point would be:
> > 	a) Is it faster to have an condition check in dwmac4_set_addr(), or
> > 	b) Always write to descs the upper 32 bits. Which always exists in the 
> > IP and is a standard write to memory.
> 
> The way I would approach it (as done in bcmgenet.c) is that if the
> platform both has CONFIG_PHYS_ADDR_T_64BIT=y and supports > 32-bits
> addresses, then you write the upper 32-bits otherwise, you do not. Given
> you indicate that the registers are safe to write regardless, then maybe
> just the check on CONFIG_PHYS_ADDR_T_64BIT is enough for your case. The
> rationale in my case is that register writes to on-chip descriptors are
> fairly expensive (~200ns per operation) and get in the hot-path.
> 
> The CONFIG_PHYS_ADDR_T_64BIT check addresses both native 64-bit
> platforms (e.g.: ARM64) and those that do support LPAE (ARM LPAE for
> instance).

I think we actually want CONFIG_DMA_ADDR_T_64BIT here because we're
dealing with addresses returned from the DMA API here.

I can add an additional condition for the upper 32-bit register writes,
something like:

	if (IS_ENABLED(CONFIG_DMA_ADDR_T_64BIT) && priv->dma_cfg->eame)
		...

The compiler should be able to eliminate that as dead code on platforms
that don't support 64-bit DMA addresses, but the code should still be
compiler regardless of the setting, thus increasing the compile
coverage.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10

[Jose Abreu]

From: Thierry Reding <thierry.reding@gmail.com>
Date: Sep/25/2019, 23:46:20 (UTC+00:00)

> On Wed, Sep 25, 2019 at 10:31:13AM -0700, Florian Fainelli wrote:
> > The way I would approach it (as done in bcmgenet.c) is that if the
> > platform both has CONFIG_PHYS_ADDR_T_64BIT=y and supports > 32-bits
> > addresses, then you write the upper 32-bits otherwise, you do not. Given
> > you indicate that the registers are safe to write regardless, then maybe
> > just the check on CONFIG_PHYS_ADDR_T_64BIT is enough for your case. The
> > rationale in my case is that register writes to on-chip descriptors are
> > fairly expensive (~200ns per operation) and get in the hot-path.
> > 
> > The CONFIG_PHYS_ADDR_T_64BIT check addresses both native 64-bit
> > platforms (e.g.: ARM64) and those that do support LPAE (ARM LPAE for
> > instance).
> 
> I think we actually want CONFIG_DMA_ADDR_T_64BIT here because we're
> dealing with addresses returned from the DMA API here.
> 
> I can add an additional condition for the upper 32-bit register writes,
> something like:
> 
> 	if (IS_ENABLED(CONFIG_DMA_ADDR_T_64BIT) && priv->dma_cfg->eame)
> 		...
> 
> The compiler should be able to eliminate that as dead code on platforms
> that don't support 64-bit DMA addresses, but the code should still be
> compiler regardless of the setting, thus increasing the compile
> coverage.

I'm fine with this. Some notes:
a) Do not try to enable dma_cfg->eame if CONFIG_DMA_ADDR_T_64BIT is not 
enabled;
b) You can even add a likely() around priv->dma_cfg->eame check because 
if a given SoC supports 64 bit addressing then its highly probable that 
the IP will also support EAME.

---
Thanks,
Jose Miguel Abreu