Hi Anup,
On Thu, Nov 26, 2020 at 11:08 AM Anup Patel <a...@brainfault.org> wrote:
> On Thu, Nov 26, 2020 at 10:33 AM Padmarao Begari <padmara...@gmail.com>
> wrote:
> >
> > Hi Anup
> >
> > I have tested the MACB driver patch with the SiFive unleashed board and
> it's not working because of the GEM_DCFG6 register.
> > The SiFive FU540 is supporting DMA address width is 64-bit while reading
> the GEM_DCFG6 register but it supports
> > scatter-gather 32-bit wide bus mastering DMA. The SiFive unleashed board
> supports 32-bit DMA not 64-bit DMA.
>
> Thanks for trying this out.
>
> >
> > We can't check dynamically the support of DMA.
>
> We can still check this capability.
>
> I would suggest to detect 64bit DMA support based on
> compatible string as follows:
> 1. Add MACB compatible string for Microchip polarfire
> in <linux>/Documentation/devicetree/bindings/net/macb.txt
> 2. Use MACB polarfir compatible string to enable 64bit
> DMA feature in U-Boot MACB driver.
>
> This way your patch will work fine for both SiFive Unleashed
> and Microchip Polarfire.
>
>
ok, I will add a MACB compatible string for Microchip PolarFire SoC.
Regards
Padmarao
> >
> > + if (GEM_BFEXT(DAW64, gem_readl(macb, DCFG6)))
> > + macb->hw_dma_cap = HW_DMA_CAP_64B;
> > + else
> > + macb->hw_dma_cap = HW_DMA_CAP_32B;
> >
> > Can we use preprocessor macro(like below) to check DMA support?
> >
> > #ifdef CONFIG_DMA_ADDR_T_64BIT
> > macb->hw_dma_cap = HW_DMA_CAP_64B;
> > #else
> > macb->hw_dma_cap = HW_DMA_CAP_32B;
> > #endif
> >
> > By default CONFIG_DMA_ADDR_T_64BIT is not set, whichever the board
> supports 64-bit DMA then the board config can set the
> CONFIG_DMA_ADDR_T_64BIT.
>
> Please see above suggestion.
>
> Regards,
> Anup
>
> >
> > Regards
> > Padmarao
> >
> > On Sun, Nov 15, 2020 at 5:40 PM Anup Patel <a...@brainfault.org> wrote:
> >>
> >> On Tue, Nov 10, 2020 at 4:16 PM Padmarao Begari
> >> <padmarao.beg...@microchip.com> wrote:
> >> >
> >> > Enable 32-bit or 64-bit DMA in the macb driver based on the design
> >> > config debug6 register of MACB hardware which supports 32-bit or
> >> > 64-bit DMA.
> >> >
> >> > Signed-off-by: Padmarao Begari <padmarao.beg...@microchip.com>
> >> > ---
> >> > drivers/net/macb.c | 121
> +++++++++++++++++++++++++++++++++++++++------
> >> > drivers/net/macb.h | 6 +++
> >> > 2 files changed, 112 insertions(+), 15 deletions(-)
> >> >
> >> > diff --git a/drivers/net/macb.c b/drivers/net/macb.c
> >> > index b80a259ff7..9ccbb7a166 100644
> >> > --- a/drivers/net/macb.c
> >> > +++ b/drivers/net/macb.c
> >> > @@ -83,7 +83,16 @@ struct macb_dma_desc {
> >> > u32 ctrl;
> >> > };
> >> >
> >> > -#define DMA_DESC_BYTES(n) (n * sizeof(struct macb_dma_desc))
> >> > +struct macb_dma_desc_64 {
> >> > + u32 addrh;
> >> > + u32 unused;
> >> > +};
> >> > +
> >> > +#define HW_DMA_CAP_32B 0
> >> > +#define HW_DMA_CAP_64B 1
> >> > +
> >> > +#define DMA_DESC_SIZE 16
> >> > +#define DMA_DESC_BYTES(n) ((n) * DMA_DESC_SIZE)
> >> > #define MACB_TX_DMA_DESC_SIZE (DMA_DESC_BYTES(MACB_TX_RING_SIZE))
> >> > #define MACB_RX_DMA_DESC_SIZE (DMA_DESC_BYTES(MACB_RX_RING_SIZE))
> >> > #define MACB_TX_DUMMY_DMA_DESC_SIZE (DMA_DESC_BYTES(1))
> >> > @@ -133,6 +142,7 @@ struct macb_device {
> >> > #endif
> >> > phy_interface_t phy_interface;
> >> > #endif
> >> > + unsigned short hw_dma_cap;
> >> > };
> >> >
> >> > struct macb_config {
> >> > @@ -307,6 +317,24 @@ static inline void
> macb_invalidate_rx_buffer(struct macb_device *macb)
> >> >
> >> > #if defined(CONFIG_CMD_NET)
> >> >
> >> > +static struct macb_dma_desc_64 *macb_64b_desc(struct macb_dma_desc
> *desc)
> >> > +{
> >> > + return (struct macb_dma_desc_64 *)((void *)desc
> >> > + + sizeof(struct macb_dma_desc));
> >> > +}
> >> > +
> >> > +static void macb_set_addr(struct macb_device *macb, struct
> macb_dma_desc *desc,
> >> > + ulong addr)
> >> > +{
> >> > + struct macb_dma_desc_64 *desc_64;
> >> > +
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B) {
> >> > + desc_64 = macb_64b_desc(desc);
> >> > + desc_64->addrh = upper_32_bits(addr);
> >> > + }
> >> > + desc->addr = lower_32_bits(addr);
> >> > +}
> >> > +
> >> > static int _macb_send(struct macb_device *macb, const char *name,
> void *packet,
> >> > int length)
> >> > {
> >> > @@ -325,8 +353,12 @@ static int _macb_send(struct macb_device *macb,
> const char *name, void *packet,
> >> > macb->tx_head++;
> >> > }
> >> >
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B)
> >> > + tx_head = tx_head * 2;
> >> > +
> >> > macb->tx_ring[tx_head].ctrl = ctrl;
> >> > - macb->tx_ring[tx_head].addr = paddr;
> >> > + macb_set_addr(macb, &macb->tx_ring[tx_head], paddr);
> >> > +
> >> > barrier();
> >> > macb_flush_ring_desc(macb, TX);
> >> > macb_writel(macb, NCR, MACB_BIT(TE) | MACB_BIT(RE) |
> MACB_BIT(TSTART));
> >> > @@ -363,19 +395,28 @@ static void reclaim_rx_buffers(struct
> macb_device *macb,
> >> > unsigned int new_tail)
> >> > {
> >> > unsigned int i;
> >> > + unsigned int count;
> >> >
> >> > i = macb->rx_tail;
> >> >
> >> > macb_invalidate_ring_desc(macb, RX);
> >> > while (i > new_tail) {
> >> > - macb->rx_ring[i].addr &= ~MACB_BIT(RX_USED);
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B)
> >> > + count = i * 2;
> >> > + else
> >> > + count = i;
> >> > + macb->rx_ring[count].addr &= ~MACB_BIT(RX_USED);
> >> > i++;
> >> > if (i > MACB_RX_RING_SIZE)
> >> > i = 0;
> >> > }
> >> >
> >> > while (i < new_tail) {
> >> > - macb->rx_ring[i].addr &= ~MACB_BIT(RX_USED);
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B)
> >> > + count = i * 2;
> >> > + else
> >> > + count = i;
> >> > + macb->rx_ring[count].addr &= ~MACB_BIT(RX_USED);
> >> > i++;
> >> > }
> >> >
> >> > @@ -390,16 +431,25 @@ static int _macb_recv(struct macb_device *macb,
> uchar **packetp)
> >> > void *buffer;
> >> > int length;
> >> > u32 status;
> >> > + u8 flag = false;
> >> >
> >> > macb->wrapped = false;
> >> > for (;;) {
> >> > macb_invalidate_ring_desc(macb, RX);
> >> >
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B)
> >> > + next_rx_tail = next_rx_tail * 2;
> >> > +
> >> > if (!(macb->rx_ring[next_rx_tail].addr &
> MACB_BIT(RX_USED)))
> >> > return -EAGAIN;
> >> >
> >> > status = macb->rx_ring[next_rx_tail].ctrl;
> >> > if (status & MACB_BIT(RX_SOF)) {
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B) {
> >> > + next_rx_tail = next_rx_tail / 2;
> >> > + flag = true;
> >> > + }
> >> > +
> >> > if (next_rx_tail != macb->rx_tail)
> >> > reclaim_rx_buffers(macb,
> next_rx_tail);
> >> > macb->wrapped = false;
> >> > @@ -426,11 +476,22 @@ static int _macb_recv(struct macb_device *macb,
> uchar **packetp)
> >> > *packetp = buffer;
> >> > }
> >> >
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B) {
> >> > + if (!flag)
> >> > + next_rx_tail = next_rx_tail /
> 2;
> >> > + }
> >> > +
> >> > if (++next_rx_tail >= MACB_RX_RING_SIZE)
> >> > next_rx_tail = 0;
> >> > macb->next_rx_tail = next_rx_tail;
> >> > return length;
> >> > } else {
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B) {
> >> > + if (!flag)
> >> > + next_rx_tail = next_rx_tail /
> 2;
> >> > + flag = false;
> >> > + }
> >> > +
> >> > if (++next_rx_tail >= MACB_RX_RING_SIZE) {
> >> > macb->wrapped = true;
> >> > next_rx_tail = 0;
> >> > @@ -718,6 +779,7 @@ static int gmac_init_multi_queues(struct
> macb_device *macb)
> >> > {
> >> > int i, num_queues = 1;
> >> > u32 queue_mask;
> >> > + unsigned long paddr;
> >> >
> >> > /* bit 0 is never set but queue 0 always exists */
> >> > queue_mask = gem_readl(macb, DCFG6) & 0xff;
> >> > @@ -731,10 +793,18 @@ static int gmac_init_multi_queues(struct
> macb_device *macb)
> >> > macb->dummy_desc->addr = 0;
> >> > flush_dcache_range(macb->dummy_desc_dma, macb->dummy_desc_dma
> +
> >> > ALIGN(MACB_TX_DUMMY_DMA_DESC_SIZE, PKTALIGN));
> >> > -
> >> > - for (i = 1; i < num_queues; i++)
> >> > - gem_writel_queue_TBQP(macb, macb->dummy_desc_dma, i -
> 1);
> >> > -
> >> > + paddr = macb->dummy_desc_dma;
> >> > +
> >> > + for (i = 1; i < num_queues; i++) {
> >> > + gem_writel_queue_TBQP(macb, lower_32_bits(paddr), i -
> 1);
> >> > + gem_writel_queue_RBQP(macb, lower_32_bits(paddr), i -
> 1);
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B) {
> >> > + gem_writel_queue_TBQPH(macb,
> upper_32_bits(paddr),
> >> > + i - 1);
> >> > + gem_writel_queue_RBQPH(macb,
> upper_32_bits(paddr),
> >> > + i - 1);
> >> > + }
> >> > + }
> >> > return 0;
> >> > }
> >> >
> >> > @@ -760,6 +830,9 @@ static void gmac_configure_dma(struct macb_device
> *macb)
> >> > dmacfg &= ~GEM_BIT(ENDIA_DESC);
> >> >
> >> > dmacfg &= ~GEM_BIT(ADDR64);
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B)
> >> > + dmacfg |= GEM_BIT(ADDR64);
> >> > +
> >> > gem_writel(macb, DMACFG, dmacfg);
> >> > }
> >> >
> >> > @@ -775,6 +848,12 @@ static int _macb_init(struct macb_device *macb,
> const char *name)
> >> > unsigned long paddr;
> >> > int ret;
> >> > int i;
> >> > + int count;
> >> > +
> >> > + if (GEM_BFEXT(DAW64, gem_readl(macb, DCFG6)))
> >> > + macb->hw_dma_cap = HW_DMA_CAP_64B;
> >> > + else
> >> > + macb->hw_dma_cap = HW_DMA_CAP_32B;
> >> >
> >> > /*
> >> > * macb_halt should have been called at some point before now,
> >> > @@ -786,20 +865,28 @@ static int _macb_init(struct macb_device *macb,
> const char *name)
> >> > for (i = 0; i < MACB_RX_RING_SIZE; i++) {
> >> > if (i == (MACB_RX_RING_SIZE - 1))
> >> > paddr |= MACB_BIT(RX_WRAP);
> >> > - macb->rx_ring[i].addr = paddr;
> >> > - macb->rx_ring[i].ctrl = 0;
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B)
> >> > + count = i * 2;
> >> > + else
> >> > + count = i;
> >> > + macb->rx_ring[count].ctrl = 0;
> >> > + macb_set_addr(macb, &macb->rx_ring[count], paddr);
> >> > paddr += macb->rx_buffer_size;
> >> > }
> >> > macb_flush_ring_desc(macb, RX);
> >> > macb_flush_rx_buffer(macb);
> >> >
> >> > for (i = 0; i < MACB_TX_RING_SIZE; i++) {
> >> > - macb->tx_ring[i].addr = 0;
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B)
> >> > + count = i * 2;
> >> > + else
> >> > + count = i;
> >> > + macb_set_addr(macb, &macb->tx_ring[count], 0);
> >> > if (i == (MACB_TX_RING_SIZE - 1))
> >> > - macb->tx_ring[i].ctrl = MACB_BIT(TX_USED) |
> >> > + macb->tx_ring[count].ctrl = MACB_BIT(TX_USED)
> |
> >> > MACB_BIT(TX_WRAP);
> >> > else
> >> > - macb->tx_ring[i].ctrl = MACB_BIT(TX_USED);
> >> > + macb->tx_ring[count].ctrl = MACB_BIT(TX_USED);
> >> > }
> >> > macb_flush_ring_desc(macb, TX);
> >> >
> >> > @@ -812,8 +899,12 @@ static int _macb_init(struct macb_device *macb,
> const char *name)
> >> > gem_writel(macb, DMACFG, MACB_ZYNQ_GEM_DMACR_INIT);
> >> > #endif
> >> >
> >> > - macb_writel(macb, RBQP, macb->rx_ring_dma);
> >> > - macb_writel(macb, TBQP, macb->tx_ring_dma);
> >> > + macb_writel(macb, RBQP, lower_32_bits(macb->rx_ring_dma));
> >> > + macb_writel(macb, TBQP, lower_32_bits(macb->tx_ring_dma));
> >> > + if (macb->hw_dma_cap & HW_DMA_CAP_64B) {
> >> > + macb_writel(macb, RBQPH,
> upper_32_bits(macb->rx_ring_dma));
> >> > + macb_writel(macb, TBQPH,
> upper_32_bits(macb->tx_ring_dma));
> >> > + }
> >> >
> >> > if (macb_is_gem(macb)) {
> >> > /* Initialize DMA properties */
> >> > diff --git a/drivers/net/macb.h b/drivers/net/macb.h
> >> > index 9b16383eba..72b84ae96e 100644
> >> > --- a/drivers/net/macb.h
> >> > +++ b/drivers/net/macb.h
> >> > @@ -768,5 +768,11 @@
> >> > #define GEM_RX_CSUM_CHECKED_MASK 2
> >> > #define gem_writel_queue_TBQP(port, value, queue_num) \
> >> > writel((value), (port)->regs + GEM_TBQP(queue_num))
> >> > +#define gem_writel_queue_TBQPH(port, value, queue_num) \
> >> > + writel((value), (port)->regs + GEM_TBQPH(queue_num))
> >> > +#define gem_writel_queue_RBQP(port, value, queue_num) \
> >> > + writel((value), (port)->regs + GEM_RBQP(queue_num))
> >> > +#define gem_writel_queue_RBQPH(port, value, queue_num) \
> >> > + writel((value), (port)->regs + GEM_RBQPH(queue_num))
> >> >
> >> > #endif /* __DRIVERS_MACB_H__ */
> >> > --
> >> > 2.17.1
> >> >
> >>
> >> Looks good to me.
> >>
> >> Reviewed-by: Anup Patel <anup.pa...@wdc.com>
> >>
> >> We need to test this patch on SiFive Unleashed as well because
> >> it has same ethernet controller.
> >>
> >> Regards,
> >> Anup
>
