> >-----Original Message----- >From: Harini Katakam [mailto:harinikatakamli...@gmail.com] >Sent: 18 listopada 2016 10:44 >To: Rafal Ozieblo >Cc: Nicolas Ferre; Andrei Pistirica; harini.kata...@xilinx.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 Fri, Nov 18, 2016 at 3:00 PM, Rafal Ozieblo <raf...@cadence.com> wrote: > >>>-----Original Message----- >>> >>>From: Nicolas Ferre [mailto:nicolas.fe...@atmel.com] >>> >>>Sent: 18 listopada 2016 10:10 >>> >>>To: Rafal Ozieblo; Harini Katakam; Andrei Pistirica >>> >>>Cc: harini.kata...@xilinx.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 >>> >>> >>> >>>Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit : >>> >>>> Hello, >>>> >>>> >>>> >>>>> From: Harini Katakam [mailto:harinikatakamli...@gmail.com] >>>>> >>>>> Sent: 18 listopada 2016 05:30 >>>>> >>>>> To: Rafal Ozieblo >>>>> >>>>> Cc: Nicolas Ferre; harini.kata...@xilinx.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 7:05 PM, Rafal Ozieblo <raf...@cadence.com> >>>>> wrote: >>>>>> -----Original Message----- >>>>>> >>>>>> From: Nicolas Ferre [mailto:nicolas.fe...@atmel.com] >>>>>> >>>>>> Sent: 17 listopada 2016 14:29 >>>>>> >>>>>> To: Harini Katakam; Rafal Ozieblo >>>>>> >>>>>> Cc: harini.kata...@xilinx.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 >>>>>> >>>>>> >>>>>> >>>>>>> Le 17/11/2016 à 13:21, Harini Katakam a écrit : >>>>>>> >>>>>>>> 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. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> 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. >>>>>>>> >>>>>>> >>>>>>> >>>>>>> We have quite a bunch of options in this driver to determinate what is >>>>>>> the real capacity of the underlying hardware. >>>>>>> >>>>>>> If HW configuration registers are not appropriate, and it seems they >>>>>>> are not, I would advice to simply use the DT compatibility string. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Best regards, >>>>>>> >>>>>>> -- >>>>>>> >>>>>>> Nicolas Ferre >>>>>>> >>>>>> >>>>>> >>>>>> HW configuration registers are appropriate. The issue is that this code >>>>>> doesn’t use the capability bit to switch between different dma >>>>>> descriptors (2 words vs. 4 words). >>>>>> DMA descriptor size is chosen based on kernel configuration, not based >>>>>> on hardware capabilities. >>>>> >>>>> >>>>> HW configuration register does give appropriate information. >>>>> >>>>> But addition of two address words in the macb descriptor structure is a >>>>> static change. >>>>> >>>>> >>>>> +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 >>>>> >>>>> + >>>>> >>>>> >>>>> >>>>> Even if the #ifdef condition here is changed to HW config check, addr and >>>>> addrh are different. >>>>> And "addrh" entry has to be present for 64 bit desc case to be handled >>>>> separately. >>>>> Can you please tell me how you propose change in DMA descriptor >>>>> >>>>> structure from >>>>> >>>>> 4 to 2 or 2 to 4 words *after* reading the DCFG register? >>>>> >>>> >>>> >>>> It is very complex problem. I wrote to you because I faced the same issue. >>>> I'm working on PTP implementation for macb. When PTP is enabled there are >>>> additional two words in buffer descriptor. >>> >>>Moreover, we can use PTP without the 64bits descriptor support (early GEM >>>revisions). >>> >>>BTW, note that there is an initiative ongoing with Andrei to support >>>PTP/1588 on these devices with patches already send: please synchronize with >>>him. >>>https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3D >>>linux-2Dnetdev-26m-3D147282092309112-26w-3D3&d=DgIDaQ&c=aUq983L2pue2Fq >>>KFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtC >>>g4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=kr_km0MKQBzpkaOt >>>0fkM-FIajN1-pOylzzTjsXi-vak&e= >>>or >>>https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel. >>>org_patch_9310989_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_ha >>>XqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3B >>>HCHFC6ZsKoZkU3vT8gJ-7fnsE&s=ZbXVD5Lb5zGn7wUKIPYHxagIEKp_vJvrnkRa4qJfmT >>>Y&e= >>>and >>>https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel. >>>org_patch_9310991_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_ha >>>XqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3B >>>HCHFC6ZsKoZkU3vT8gJ-7fnsE&s=Z0kRqUqh5Is4TmuaY7A4hnpizfdeq3HrgPhyDAMLuA >>>8&e= >>> >>>Regards, >>>-- >>>Nicolas Ferre >> >> Above patch doesn’t use hardware timestamps in descriptors at all. It >> doesn't use appropriate accuracy. >> We have our PTP patch ready, which use timestamps from descriptor. But we >> have not sent it yet because of compatibility issue. >> Of course, we can do it like Harini did: >> >> struct macb_dma_desc { >> u32 addr; >> u32 ctrl; >> #ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT >> u32 addrh; >> u32 resvd; >> #endif >> #if IS_ENABLED(CONFIG_PTP_1588_CLOCK) >> u32 dma_desc_ts_1; >> u32 dma_desc_ts_2; >> #endif >> }; >> >> But in that approach we lose backward compatibility. >> >> What are your suggestion? Can we send patch like it is or wait for some >> common solution with backward compatibility? >> > >Yes, Andre's version of Cadence does not ability to report have RX timestamp. >The version I worked with did. This is the old series I sent: >https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2015_9_11_92&d=DgIFaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=1A1qxdlwY3kMlJ1JUJJL1EqLzWz4AKtfOxZf_vu8bWo&s=NGpEbbC4bYXUZjJ6n0Ud8F8p3fPz5EhTJ1O9-NKwCkQ&e= >But right now, i'm working on building on top of Andre's changes. > >Regards, >Harini > I can’t see a place where you enable extended descriptor for PTP. Did you add support for extended PTP descriptor? "DMA Configuration Register" 0x010: 29 tx_bd_extended_mode_en "Enable TX extended BD mode. See TX BD control register definition for description of feature." 28 rx_bd_extended_mode_en "Enable RX extended BD mode. See RX BD control register definition for description of feature."
Can I send you our patch for comparison ?
