-----Original Message-----
From: Harini Katakam [mailto:harinikatakamli...@gmail.com]
Sent: 17 listopada 2016 13:22
To: Rafal Ozieblo
Cc: harini.kata...@xilinx.com; nicolas.fe...@atmel.com; netdev@vger.kernel.org;
linux-ker...@vger.kernel.org
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 <raf...@cadence.com> 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
