From: Rafal Ozieblo <rafalo@cadence.com>
To: Harini Katakam <harinikatakamlinux@gmail.com>
CC: "harini.katakam@xilinx.com" <harini.katakam@xilinx.com>,
        "nicolas.ferre@atmel.com" <nicolas.ferre@atmel.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
Thread-Topic: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
Thread-Index: AQHSQM0mlMF9RYMdA0O6HcZ/ouUay6DdGzrQ
Date: Thu, 17 Nov 2016 12:57:23 +0000
Message-ID: <BN3PR07MB2516488A6BB0F208C3DDB0E1C9B10@BN3PR07MB2516.namprd07.prod.outlook.com>
References: <BN3PR07MB2516B2EBCF0AE7079971DBF6C9B10@BN3PR07MB2516.namprd07.prod.outlook.com>
 <CAFcVECJ-ZYjGg-3CbWD2Ka8yKzOokUxS8ZHHOJXa2Z6anuTr7Q@mail.gmail.com>
In-Reply-To: <CAFcVECJ-ZYjGg-3CbWD2Ka8yKzOokUxS8ZHHOJXa2Z6anuTr7Q@mail.gmail.com>
Accept-Language: pl-PL, en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Nov 2016 12:57:23.8745
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: d36035c5-6ce6-4662-a3dc-e762e61ae4c9
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR07MB2516
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id uAHHXO34012343
Content-Length: 3197
Lines: 74

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