* [PATCH v3 1/2] net: stmmac: Only enable enhanced addressing mode when needed
2019-09-20 17:00 [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10 Thierry Reding
@ 2019-09-20 17:00 ` Thierry Reding
2019-09-20 17:00 ` [PATCH v3 2/2] net: stmmac: Support enhanced addressing mode for DWMAC 4.10 Thierry Reding
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Thierry Reding @ 2019-09-20 17:00 UTC (permalink / raw)
To: David S . Miller
Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
Florian Fainelli, Jon Hunter, Bitan Biswas, netdev, linux-tegra
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH v3 2/2] net: stmmac: Support enhanced addressing mode for DWMAC 4.10
2019-09-20 17:00 [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10 Thierry Reding
2019-09-20 17:00 ` [PATCH v3 1/2] net: stmmac: Only enable enhanced addressing mode when needed Thierry Reding
@ 2019-09-20 17:00 ` Thierry Reding
2019-09-20 17:02 ` [PATCH v3 0/2] net: stmmac: Enhanced " Florian Fainelli
2019-09-24 19:45 ` David Miller
3 siblings, 0 replies; 15+ messages in thread
From: Thierry Reding @ 2019-09-20 17:00 UTC (permalink / raw)
To: David S . Miller
Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
Florian Fainelli, Jon Hunter, Bitan Biswas, netdev, linux-tegra
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-20 17:00 [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10 Thierry Reding
2019-09-20 17:00 ` [PATCH v3 1/2] net: stmmac: Only enable enhanced addressing mode when needed Thierry Reding
2019-09-20 17:00 ` [PATCH v3 2/2] net: stmmac: Support enhanced addressing mode for DWMAC 4.10 Thierry Reding
@ 2019-09-20 17:02 ` Florian Fainelli
2019-09-21 1:35 ` Thierry Reding
2019-09-24 19:45 ` David Miller
3 siblings, 1 reply; 15+ messages in thread
From: Florian Fainelli @ 2019-09-20 17:02 UTC (permalink / raw)
To: Thierry Reding, David S . Miller
Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, Jon Hunter,
Bitan Biswas, netdev, linux-tegra
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-20 17:02 ` [PATCH v3 0/2] net: stmmac: Enhanced " Florian Fainelli
@ 2019-09-21 1:35 ` Thierry Reding
0 siblings, 0 replies; 15+ messages in thread
From: Thierry Reding @ 2019-09-21 1:35 UTC (permalink / raw)
To: Florian Fainelli
Cc: David S . Miller, Giuseppe Cavallaro, Alexandre Torgue,
Jose Abreu, Jon Hunter, Bitan Biswas, netdev, linux-tegra
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-20 17:00 [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10 Thierry Reding
` (2 preceding siblings ...)
2019-09-20 17:02 ` [PATCH v3 0/2] net: stmmac: Enhanced " Florian Fainelli
@ 2019-09-24 19:45 ` David Miller
2019-09-25 10:44 ` Jose Abreu
2019-09-25 11:17 ` Thierry Reding
3 siblings, 2 replies; 15+ messages in thread
From: David Miller @ 2019-09-24 19:45 UTC (permalink / raw)
To: thierry.reding
Cc: peppe.cavallaro, alexandre.torgue, joabreu, f.fainelli,
jonathanh, bbiswas, netdev, linux-tegra
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.
^ permalink raw reply [flat|nested] 15+ messages in thread* RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-24 19:45 ` David Miller
@ 2019-09-25 10:44 ` Jose Abreu
2019-09-25 11:33 ` David Miller
2019-09-25 11:17 ` Thierry Reding
1 sibling, 1 reply; 15+ messages in thread
From: Jose Abreu @ 2019-09-25 10:44 UTC (permalink / raw)
To: David Miller, thierry.reding
Cc: peppe.cavallaro, alexandre.torgue, f.fainelli, jonathanh,
bbiswas, netdev, linux-tegra
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-25 10:44 ` Jose Abreu
@ 2019-09-25 11:33 ` David Miller
2019-09-25 11:41 ` Jose Abreu
0 siblings, 1 reply; 15+ messages in thread
From: David Miller @ 2019-09-25 11:33 UTC (permalink / raw)
To: Jose.Abreu
Cc: thierry.reding, peppe.cavallaro, alexandre.torgue, f.fainelli,
jonathanh, bbiswas, netdev, linux-tegra
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?
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-25 11:33 ` David Miller
@ 2019-09-25 11:41 ` Jose Abreu
2019-09-25 11:46 ` Jose Abreu
2019-09-25 12:01 ` David Miller
0 siblings, 2 replies; 15+ messages in thread
From: Jose Abreu @ 2019-09-25 11:41 UTC (permalink / raw)
To: David Miller, Jose.Abreu
Cc: thierry.reding, peppe.cavallaro, alexandre.torgue, f.fainelli,
jonathanh, bbiswas, netdev, linux-tegra
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-25 11:41 ` Jose Abreu
@ 2019-09-25 11:46 ` Jose Abreu
2019-09-25 17:31 ` Florian Fainelli
2019-09-25 12:01 ` David Miller
1 sibling, 1 reply; 15+ messages in thread
From: Jose Abreu @ 2019-09-25 11:46 UTC (permalink / raw)
To: David Miller, Jose.Abreu
Cc: thierry.reding, peppe.cavallaro, alexandre.torgue, f.fainelli,
jonathanh, bbiswas, netdev, linux-tegra
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-25 11:46 ` Jose Abreu
@ 2019-09-25 17:31 ` Florian Fainelli
2019-09-25 22:46 ` Thierry Reding
0 siblings, 1 reply; 15+ messages in thread
From: Florian Fainelli @ 2019-09-25 17:31 UTC (permalink / raw)
To: Jose Abreu, David Miller
Cc: thierry.reding, peppe.cavallaro, alexandre.torgue, jonathanh,
bbiswas, netdev, linux-tegra
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-25 17:31 ` Florian Fainelli
@ 2019-09-25 22:46 ` Thierry Reding
2019-09-26 8:22 ` Jose Abreu
0 siblings, 1 reply; 15+ messages in thread
From: Thierry Reding @ 2019-09-25 22:46 UTC (permalink / raw)
To: Florian Fainelli
Cc: Jose Abreu, David Miller, peppe.cavallaro, alexandre.torgue,
jonathanh, bbiswas, netdev, linux-tegra
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-25 22:46 ` Thierry Reding
@ 2019-09-26 8:22 ` Jose Abreu
0 siblings, 0 replies; 15+ messages in thread
From: Jose Abreu @ 2019-09-26 8:22 UTC (permalink / raw)
To: Thierry Reding, Florian Fainelli
Cc: Jose Abreu, David Miller, peppe.cavallaro, alexandre.torgue,
jonathanh, bbiswas, netdev, linux-tegra
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-25 11:41 ` Jose Abreu
2019-09-25 11:46 ` Jose Abreu
@ 2019-09-25 12:01 ` David Miller
1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2019-09-25 12:01 UTC (permalink / raw)
To: Jose.Abreu
Cc: thierry.reding, peppe.cavallaro, alexandre.torgue, f.fainelli,
jonathanh, bbiswas, netdev, linux-tegra
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
2019-09-24 19:45 ` David Miller
2019-09-25 10:44 ` Jose Abreu
@ 2019-09-25 11:17 ` Thierry Reding
1 sibling, 0 replies; 15+ messages in thread
From: Thierry Reding @ 2019-09-25 11:17 UTC (permalink / raw)
To: David Miller
Cc: peppe.cavallaro, alexandre.torgue, joabreu, f.fainelli,
jonathanh, bbiswas, netdev, linux-tegra
[-- 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 --]
^ permalink raw reply [flat|nested] 15+ messages in thread