>Hi Rafal, > >On Fri, Nov 18, 2016 at 2:29 PM, Rafal Ozieblo <raf...@cadence.com> wrote: >> 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. >> 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
It is done very similarly in sdhci driver: http://lxr.free-electrons.com/source/drivers/mmc/host/sdhci.c For example, please look at sdhci_adma_write_desc(). If we change my idea to operate directly on void * instead of macb_dma_desc *, it would be even better. (rx_ring, tx_ring and others), ex.: struct void *rx_ring; struct void *tx_ring; static macb_dma_desc *macb_rx_desc(struct macb *bp, unsigned int index) { index *= macb_dma_desc_get_size(bp); return (macb_dma_desc *)&bp->rx_ring[macb_rx_ring_wrap(bp, index)]; } >> 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. > Best regards, Rafal Ozieblo | Firmware System Engineer, phone nbr.: +48 32 5085469 www.cadence.com
