Message ID | 20190909152546.383-2-thierry.reding@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net-next,v2,1/2] net: stmmac: Only enable enhanced addressing mode when needed | expand |
From: Thierry Reding <thierry.reding@gmail.com> Date: Sep/09/2019, 16:25:46 (UTC+00:00) > @@ -79,6 +79,10 @@ 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)); > > + if (dma_cfg->eame) There is no need for this check. If EAME is not enabled then upper 32 bits will be zero. > + 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 +101,10 @@ static void dwmac4_dma_init_tx_chan(void __iomem *ioaddr, > > writel(value, ioaddr + DMA_CHAN_TX_CONTROL(chan)); > > + if (dma_cfg->eame) Same here. > + 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)); > } Also, please provide a cover letter in next submission. --- Thanks, Jose Miguel Abreu
On Mon, Sep 09, 2019 at 04:05:52PM +0000, Jose Abreu wrote: > From: Thierry Reding <thierry.reding@gmail.com> > Date: Sep/09/2019, 16:25:46 (UTC+00:00) > > > @@ -79,6 +79,10 @@ 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)); > > > > + if (dma_cfg->eame) > > There is no need for this check. If EAME is not enabled then upper 32 > bits will be zero. The idea here was to potentially guard against this register not being available on some revisions. Having the check here would avoid access to the register if the device doesn't support enhanced addressing. > > > + 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 +101,10 @@ static void dwmac4_dma_init_tx_chan(void __iomem *ioaddr, > > > > writel(value, ioaddr + DMA_CHAN_TX_CONTROL(chan)); > > > > + if (dma_cfg->eame) > > Same here. > > > + 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)); > > } > > Also, please provide a cover letter in next submission. Alright, will do. Thierry
From: Thierry Reding <thierry.reding@gmail.com> Date: Sep/09/2019, 20:13:29 (UTC+00:00) > On Mon, Sep 09, 2019 at 04:05:52PM +0000, Jose Abreu wrote: > > From: Thierry Reding <thierry.reding@gmail.com> > > Date: Sep/09/2019, 16:25:46 (UTC+00:00) > > > > > @@ -79,6 +79,10 @@ 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)); > > > > > > + if (dma_cfg->eame) > > > > There is no need for this check. If EAME is not enabled then upper 32 > > bits will be zero. > > The idea here was to potentially guard against this register not being > available on some revisions. Having the check here would avoid access to > the register if the device doesn't support enhanced addressing. I see your point but I don't think there will be any problems unless you have some strange system that doesn't handle the write accesses to unimplemented features properly ... --- Thanks, Jose Miguel Abreu
On 9/10/19 1:35 AM, Jose Abreu wrote: > From: Thierry Reding <thierry.reding@gmail.com> > Date: Sep/09/2019, 20:13:29 (UTC+00:00) > >> On Mon, Sep 09, 2019 at 04:05:52PM +0000, Jose Abreu wrote: >>> From: Thierry Reding <thierry.reding@gmail.com> >>> Date: Sep/09/2019, 16:25:46 (UTC+00:00) >>> >>>> @@ -79,6 +79,10 @@ 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)); >>>> >>>> + if (dma_cfg->eame) >>> >>> There is no need for this check. If EAME is not enabled then upper 32 >>> bits will be zero. >> >> The idea here was to potentially guard against this register not being >> available on some revisions. Having the check here would avoid access to >> the register if the device doesn't support enhanced addressing. > > I see your point but I don't think there will be any problems unless you > have some strange system that doesn't handle the write accesses to > unimplemented features properly ... Is not it then just safer to not do the write to a register that you do not know how the implementation is going to respond to with one of a target abort, timeout, decoding error, just dead lock? Also, would it make sense to consider adding an #ifdef CONFIG_PHYS_ADDR_T_64BIT plus the conditional check so that you can be slightly more optimal in the hot-path here?
From: Florian Fainelli <f.fainelli@gmail.com> Date: Sep/10/2019, 20:01:01 (UTC+00:00) > On 9/10/19 1:35 AM, Jose Abreu wrote: > > From: Thierry Reding <thierry.reding@gmail.com> > > Date: Sep/09/2019, 20:13:29 (UTC+00:00) > > > >> On Mon, Sep 09, 2019 at 04:05:52PM +0000, Jose Abreu wrote: > >>> From: Thierry Reding <thierry.reding@gmail.com> > >>> Date: Sep/09/2019, 16:25:46 (UTC+00:00) > >>> > >>>> @@ -79,6 +79,10 @@ 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)); > >>>> > >>>> + if (dma_cfg->eame) > >>> > >>> There is no need for this check. If EAME is not enabled then upper 32 > >>> bits will be zero. > >> > >> The idea here was to potentially guard against this register not being > >> available on some revisions. Having the check here would avoid access to > >> the register if the device doesn't support enhanced addressing. > > > > I see your point but I don't think there will be any problems unless you > > have some strange system that doesn't handle the write accesses to > > unimplemented features properly ... > > Is not it then just safer to not do the write to a register that you do > not know how the implementation is going to respond to with one of a > target abort, timeout, decoding error, just dead lock? I don't think any of these will ever happen. Notice that this is already been done for a long time in some registers that may not exist in some random HW config and there is also the point that this is a write operation so Slave Error would only get triggered if we did a read. > Also, would it make sense to consider adding an #ifdef > CONFIG_PHYS_ADDR_T_64BIT plus the conditional check so that you can be > slightly more optimal in the hot-path here? Well, this is not hot-path. It's only done in HW open sequence. The hot-path would be set_{rx/tx}_tail_ptr() but that's 32 bits only. --- Thanks, Jose Miguel Abreu
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..e77410f21d7d 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_dma.c @@ -79,6 +79,10 @@ 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)); + if (dma_cfg->eame) + 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 +101,10 @@ static void dwmac4_dma_init_tx_chan(void __iomem *ioaddr, writel(value, ioaddr + DMA_CHAN_TX_CONTROL(chan)); + if (dma_cfg->eame) + 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 +140,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 +365,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)