[PATCH v3 2/7] net: macb: Add DMA 64-bit address support for macb

Anup Patel anup at brainfault.org
Thu Nov 26 06:38:22 CET 2020


On Thu, Nov 26, 2020 at 10:33 AM Padmarao Begari <padmarao.b at 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.

>
> +    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 <anup at brainfault.org> wrote:
>>
>> On Tue, Nov 10, 2020 at 4:16 PM Padmarao Begari
>> <padmarao.begari at 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.begari at 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.patel at wdc.com>
>>
>> We need to test this patch on SiFive Unleashed as well because
>> it has same ethernet controller.
>>
>> Regards,
>> Anup


More information about the U-Boot mailing list