RE: [RFC PATCH 2/2] net: macb: Add 64 bit addressing support for GEM
From: Rafal Ozieblo
Date: Fri Nov 18 2016 - 05:14:13 EST
>
>-----Original Message-----
>From: Harini Katakam [mailto:harinikatakamlinux@xxxxxxxxx]
>Sent: 18 listopada 2016 10:44
>To: Rafal Ozieblo
>Cc: Nicolas Ferre; Andrei Pistirica; harini.katakam@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
>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 <rafalo@xxxxxxxxxxx> wrote:
>>>-----Original Message-----
>>>From: Nicolas Ferre [mailto:nicolas.ferre@xxxxxxxxx]
>>>Sent: 18 listopada 2016 10:10
>>>To: Rafal Ozieblo; Harini Katakam; Andrei Pistirica
>>>Cc: harini.katakam@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
>>>linux-kernel@xxxxxxxxxxxxxxx
>>>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:harinikatakamlinux@xxxxxxxxx]
>>>>> Sent: 18 listopada 2016 05:30
>>>>> To: Rafal Ozieblo
>>>>> Cc: Nicolas Ferre; harini.katakam@xxxxxxxxxx;
>>>>> 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 7:05 PM, Rafal Ozieblo <rafalo@xxxxxxxxxxx> wrote:
>>>>>> -----Original Message-----
>>>>>> From: Nicolas Ferre [mailto:nicolas.ferre@xxxxxxxxx]
>>>>>> Sent: 17 listopada 2016 14:29
>>>>>> To: Harini Katakam; Rafal Ozieblo
>>>>>> Cc: harini.katakam@xxxxxxxxxx; netdev@xxxxxxxxxxxxxxx;
>>>>>> linux-kernel@xxxxxxxxxxxxxxx
>>>>>> 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 <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.
>>>>>>>>
>>>>>>>> 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 ?