RE: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM

[Rafal Ozieblo]

-----Original Message-----
From: Harini Katakam [mailto:harinikatakamlinux@xxxxxxxxx] 
Sent: 17 listopada 2016 13:22
To: Rafal Ozieblo
Cc: harini.katakam@xxxxxxxxxx; nicolas.ferre@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM

> Hi Rafal,
>
> On Thu, Nov 17, 2016 at 5:20 PM, Rafal Ozieblo <rafalo@xxxxxxxxxxx> wrote:
> > Hello,
> > I think, there could a bug in your patch.
> >
> >> +
> >> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> >> +             dmacfg |= GEM_BIT(ADDR64); #endif
> >
> > You enable 64 bit addressing (64b dma bus width) always when appropriate architecture config option is enabled.
> > But there are some legacy controllers which do not support that feature. According Cadence hardware team:
> > "64 bit addressing was added in July 2013. Earlier version do not have it.
> > This feature was enhanced in release August 2014 to have separate upper address values for transmit and receive."
> >
> >> /* Bitfields in NSR */
> >> @@ -474,6 +479,10 @@
> >>  struct macb_dma_desc {
> >  >      u32     addr;
> >>       u32     ctrl; 
> >> +#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
> >> +     u32     addrh;
> >> +     u32     resvd;
> >> +#endif
> >>  };
> >
> > It will not work for legacy hardware. Old descriptor is 2 words wide, the new one is 4 words wide.
> > If you enable CONFIG_ARCH_DMA_ADDR_T_64BIT but hardware doesn't 
> > support it at all, you will miss every second descriptor.
> >
>
> True, this feature is not available in all of Cadence IP versions.
> In fact, the IP version Zynq does not support this. But the one in ZynqMP does.
> So, we enable kernel config for 64 bit DMA addressing for this SoC and hence the driver picks it up. My assumption was that if the legacy IP does not support
> 64 bit addressing, then this DMA option wouldn't be enabled.

What for example with arm64 (which enables CONFIG_ARCH_DMA_ADDR_T_64BIT by default) and legacy IP controller? Or systems with multiple IP, both with and without 64b dma capable?
It might result in unpredictable behavio. (explained below)

> There is a design config register in Cadence IP which is being read to check for 64 bit address support - DMA mask is set based on that.
> But the addition of two descriptor words cannot be based on this runtime check.
> For this reason, all the static changes were placed under this check.

Are you talking about this?

+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+	if (GEM_BFEXT(DBWDEF, gem_readl(bp, DCFG1)) > GEM_DBW32)
+		dma_set_mask(&pdev->dev, DMA_BIT_MASK(44));
+#endif
+

It only sets the maximum address which can be seen on address bus. (its mask exactly)

+static inline void macb_set_addr(struct macb_dma_desc *desc, dma_addr_t addr)
+{
+	desc->addr = (u32)addr;
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+	desc->addrh = (u32)(addr >> 32);
+#endif
+}
+

Because addr is 32b wide, (u32)(addr >> 32) equals 0. IP controller uses 2 words for dma descriptor, so desc->addr from second hardware descriptor will be overwritten by desc->addrh from the first software descriptor.
(same desc->ctrl from second hardware descriptor will be overwritten by desc->resvd). 

Regards,
Harini