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

[Harini Katakam]

Hi Rafal,

On Fri, Nov 18, 2016 at 2:29 PM, Rafal Ozieblo <rafalo@xxxxxxxxxxx> wrote:
> 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.
> But hardware might not be compatible with PTP. Therefore I have to change DMA descriptor between 2 and 4 words after reading DCFG register (the same as you between 32b and 64b).
> When we consider both PTP and 64 bits, we end up with 4 different descriptors!

Agree

<snip>
> (Below is kind of pseudo code only to show my idea. All defines like 64B or PTP are omitted for simplicity)
>
> 1. Prepare appropriate structures:
>
> struct macb_dma_desc {
>         u32     addr;
>         u32     ctrl;
> }
>
> struct macb_dma_desc_64 {
>         u32 addrh;
>         u32 resvd;
> }
>
> struct macb_dma_desc_ptp {
>         u32 dma_desc_ts_1;
>         u32     dma_desc_ts_2;
> }
>
> 2. Add hardware support information to macb:
>
> enum macb_hw_cap {
>         HW_CAP_NONE,
>         HW_CAP_64B,
>         HW_CAP_PTP,
>         HW_CAP_64B_PTP,
> };
>
> struct macb {
> // (...)
>         macb_hw_cap hw_cap;
>
> }
>
> 3. Set bp->hw_cap on macb_probe()
>

hw_cap can alreayd be obtained from config structures
based on compatible string.

> 4. Additional function might be helpful:
>
> static unsigned char macb_dma_desc_get_mul(struct macb *bp)
> {
>         switch (bp->hw_cap) {
>                 case HW_CAP_NONE:
>                         return 1;
>                 case HW_CAP_64B:
>                 case HW_CAP_PTP:
>                         return 2;
>                 case HW_CAP_64B_PTP:
>                         return 3;
>         }
> }
>
> 5. Change sizeof struct to function:
> (sizeof(struct macb_dma_desc) change to macb_dma_desc_get_size(bp). It will return sizeof(struct macb_dma_desc) * macb_dma_desc_get_mul(bp).
> There is a hidden assumption that all three structures have the same size.
>
> 6. macb_rx_desc() and macb_tx_desc() will still return struct macb_dma_desc * but descriptor index will be counted in different way, ex.
>
> static struct macb_dma_desc *macb_rx_desc(struct macb *bp, unsigned int index)
> {
>         index *= macb_dma_desc_get_mul(bp);
>         return &bp->rx_ring[macb_rx_ring_wrap(bp, index)];
> }
>
> 7. Two additional functions to dereference PTP and 64b information:
>
> static struct macb_dma_desc_64 *macb_64b_desc(struct macb *bp, struct macb_dma_desc *desc)
> {
>         switch (bp->hw_cap) {
>                 case HW_CAP_64B:
>                 case HW_CAP_64B_PTP:
>                         return (struct macb_dma_desc_64 *)(desc + 1);  // ugly casting
>                 default:
>                         return NULL;
>         }
> }
>
> static struct macb_dma_desc_ptp *macb_ptp_desc(struct macb *bp, struct macb_dma_desc *desc)
> {
>         switch (bp->hw_cap) {
>                 case HW_CAP_PTP:
>                         return (struct macb_dma_desc_ptp *)(desc + 1);
>                 case HW_CAP_64B_PTP:
>                         return (struct macb_dma_desc_ptp *)(desc + 2);
>                 default:
>                         return NULL;
>         }
> }
>
> Whenever you want to reach fields in appropriate descriptor, above function should be used.
>

Theoretically I agree this will work.
But we'll have to try to see how this will affect/slow down
the desc reading.. especially with PTP.

Regards,
Harini

> This is only my very first idea. Of course, we can leave it as it is and say, that old hardware is not support either when PTP enabled or 64b enabled.