[RFC PATCH 1/2] net: macb: Correct CAPS mask

[RFC PATCH 1/2] net: macb: Correct CAPS mask

[Harini Katakam]

USRIO and JUMBO CAPS have the same mask.
Fix the same.

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
 drivers/net/ethernet/cadence/macb.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index 36893d8..b6fcf10 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -403,11 +403,11 @@
 #define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII	0x00000004
 #define MACB_CAPS_NO_GIGABIT_HALF		0x00000008
 #define MACB_CAPS_USRIO_DISABLED		0x00000010
+#define MACB_CAPS_JUMBO				0x00000020
 #define MACB_CAPS_FIFO_MODE			0x10000000
 #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
 #define MACB_CAPS_SG_DISABLED			0x40000000
 #define MACB_CAPS_MACB_IS_GEM			0x80000000
-#define MACB_CAPS_JUMBO				0x00000010
 
 /* Bit manipulation macros */
 #define MACB_BIT(name)					\
-- 
2.7.4

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

[Harini Katakam]

This patch adds support for 64 bit addressing and BDs.
-> Enable 64 bit addressing in DMACFG register.
-> Set DMA mask when design config register shows support for 64 bit addr.
-> Add new BD words for higher address when 64 bit DMA support is present.
-> Add and update TBQPH and RBQPH for MSB of BD pointers.
-> Change extraction and updation of buffer addresses to use
64 bit address.
-> In gem_rx extract address in one place insted of two and use a
separate flag for RXUSED.

Signed-off-by: Harini Katakam <harinik@xilinx.com>
---
 drivers/net/ethernet/cadence/macb.c | 62 ++++++++++++++++++++++++++++++-------
 drivers/net/ethernet/cadence/macb.h | 10 ++++++
 2 files changed, 61 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 89c0cfa..6b797e3 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -541,6 +541,14 @@ static void macb_tx_unmap(struct macb *bp, struct macb_tx_skb *tx_skb)
 	}
 }
 
+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
+}
+
 static void macb_tx_error_task(struct work_struct *work)
 {
 	struct macb_queue	*queue = container_of(work, struct macb_queue,
@@ -621,14 +629,17 @@ static void macb_tx_error_task(struct work_struct *work)
 
 	/* Set end of TX queue */
 	desc = macb_tx_desc(queue, 0);
-	desc->addr = 0;
+	macb_set_addr(desc, 0);
 	desc->ctrl = MACB_BIT(TX_USED);
 
 	/* Make descriptor updates visible to hardware */
 	wmb();
 
 	/* Reinitialize the TX desc queue */
-	queue_writel(queue, TBQP, queue->tx_ring_dma);
+	queue_writel(queue, TBQP, (u32)(queue->tx_ring_dma));
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+	queue_writel(queue, TBQPH, (u32)(queue->tx_ring_dma >> 32));
+#endif
 	/* Make TX ring reflect state of hardware */
 	queue->tx_head = 0;
 	queue->tx_tail = 0;
@@ -750,7 +761,7 @@ static void gem_rx_refill(struct macb *bp)
 
 			if (entry == RX_RING_SIZE - 1)
 				paddr |= MACB_BIT(RX_WRAP);
-			bp->rx_ring[entry].addr = paddr;
+			macb_set_addr(&(bp->rx_ring[entry]), paddr);
 			bp->rx_ring[entry].ctrl = 0;
 
 			/* properly align Ethernet header */
@@ -798,7 +809,9 @@ static int gem_rx(struct macb *bp, int budget)
 	int			count = 0;
 
 	while (count < budget) {
-		u32 addr, ctrl;
+		u32 ctrl;
+		dma_addr_t addr;
+		bool rxused;
 
 		entry = macb_rx_ring_wrap(bp->rx_tail);
 		desc = &bp->rx_ring[entry];
@@ -806,10 +819,14 @@ static int gem_rx(struct macb *bp, int budget)
 		/* Make hw descriptor updates visible to CPU */
 		rmb();
 
-		addr = desc->addr;
+		rxused = (desc->addr & MACB_BIT(RX_USED)) ? true : false;
+		addr = MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr));
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+		addr |= ((u64)(desc->addrh) << 32);
+#endif
 		ctrl = desc->ctrl;
 
-		if (!(addr & MACB_BIT(RX_USED)))
+		if (!rxused)
 			break;
 
 		bp->rx_tail++;
@@ -835,7 +852,6 @@ static int gem_rx(struct macb *bp, int budget)
 		netdev_vdbg(bp->dev, "gem_rx %u (len %u)\n", entry, len);
 
 		skb_put(skb, len);
-		addr = MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, addr));
 		dma_unmap_single(&bp->pdev->dev, addr,
 				 bp->rx_buffer_size, DMA_FROM_DEVICE);
 
@@ -1299,7 +1315,7 @@ static unsigned int macb_tx_map(struct macb *bp,
 			ctrl |= MACB_BIT(TX_WRAP);
 
 		/* Set TX buffer descriptor */
-		desc->addr = tx_skb->mapping;
+		macb_set_addr(desc, tx_skb->mapping);
 		/* desc->addr must be visible to hardware before clearing
 		 * 'TX_USED' bit in desc->ctrl.
 		 */
@@ -1422,6 +1438,9 @@ static void gem_free_rx_buffers(struct macb *bp)
 
 		desc = &bp->rx_ring[i];
 		addr = MACB_BF(RX_WADDR, MACB_BFEXT(RX_WADDR, desc->addr));
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+		addr |= ((u64)(desc->addrh) << 32);
+#endif
 		dma_unmap_single(&bp->pdev->dev, addr, bp->rx_buffer_size,
 				 DMA_FROM_DEVICE);
 		dev_kfree_skb_any(skb);
@@ -1547,7 +1566,7 @@ static void gem_init_rings(struct macb *bp)
 
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
 		for (i = 0; i < TX_RING_SIZE; i++) {
-			queue->tx_ring[i].addr = 0;
+			macb_set_addr(&(queue->tx_ring[i]), 0);
 			queue->tx_ring[i].ctrl = MACB_BIT(TX_USED);
 		}
 		queue->tx_ring[TX_RING_SIZE - 1].ctrl |= MACB_BIT(TX_WRAP);
@@ -1694,6 +1713,10 @@ static void macb_configure_dma(struct macb *bp)
 			dmacfg |= GEM_BIT(TXCOEN);
 		else
 			dmacfg &= ~GEM_BIT(TXCOEN);
+
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+		dmacfg |= GEM_BIT(ADDR64);
+#endif
 		netdev_dbg(bp->dev, "Cadence configure DMA with 0x%08x\n",
 			   dmacfg);
 		gem_writel(bp, DMACFG, dmacfg);
@@ -1739,9 +1762,15 @@ static void macb_init_hw(struct macb *bp)
 	macb_configure_dma(bp);
 
 	/* Initialize TX and RX buffers */
-	macb_writel(bp, RBQP, bp->rx_ring_dma);
+	macb_writel(bp, RBQP, (u32)(bp->rx_ring_dma));
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+	macb_writel(bp, RBQPH, (u32)(bp->rx_ring_dma >> 32));
+#endif
 	for (q = 0, queue = bp->queues; q < bp->num_queues; ++q, ++queue) {
-		queue_writel(queue, TBQP, queue->tx_ring_dma);
+		queue_writel(queue, TBQP, (u32)(queue->tx_ring_dma));
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+		queue_writel(queue, TBQPH, (u32)(queue->tx_ring_dma >> 32));
+#endif
 
 		/* Enable interrupts */
 		queue_writel(queue, IER,
@@ -2379,6 +2408,9 @@ static int macb_init(struct platform_device *pdev)
 			queue->IDR  = GEM_IDR(hw_q - 1);
 			queue->IMR  = GEM_IMR(hw_q - 1);
 			queue->TBQP = GEM_TBQP(hw_q - 1);
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+			queue->TBQPH = GEM_TBQPH(hw_q -1);
+#endif
 		} else {
 			/* queue0 uses legacy registers */
 			queue->ISR  = MACB_ISR;
@@ -2386,6 +2418,9 @@ static int macb_init(struct platform_device *pdev)
 			queue->IDR  = MACB_IDR;
 			queue->IMR  = MACB_IMR;
 			queue->TBQP = MACB_TBQP;
+#ifdef CONFIG_ARCH_DMA_ADDR_T_64BIT
+			queue->TBQPH = MACB_TBQPH;
+#endif
 		}
 
 		/* get irq: here we use the linux queue index, not the hardware
@@ -2935,6 +2970,11 @@ static int macb_probe(struct platform_device *pdev)
 		bp->wol |= MACB_WOL_HAS_MAGIC_PACKET;
 	device_init_wakeup(&pdev->dev, bp->wol & MACB_WOL_HAS_MAGIC_PACKET);
 
+#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
+
 	spin_lock_init(&bp->lock);
 
 	/* setup capabilities */
diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
index b6fcf10..aa3aeec 100644
--- a/drivers/net/ethernet/cadence/macb.h
+++ b/drivers/net/ethernet/cadence/macb.h
@@ -66,6 +66,8 @@
 #define MACB_USRIO		0x00c0
 #define MACB_WOL		0x00c4
 #define MACB_MID		0x00fc
+#define MACB_TBQPH		0x04C8
+#define MACB_RBQPH		0x04D4
 
 /* GEM register offsets. */
 #define GEM_NCFGR		0x0004 /* Network Config */
@@ -139,6 +141,7 @@
 
 #define GEM_ISR(hw_q)		(0x0400 + ((hw_q) << 2))
 #define GEM_TBQP(hw_q)		(0x0440 + ((hw_q) << 2))
+#define GEM_TBQPH(hw_q)		(0x04C8)
 #define GEM_RBQP(hw_q)		(0x0480 + ((hw_q) << 2))
 #define GEM_IER(hw_q)		(0x0600 + ((hw_q) << 2))
 #define GEM_IDR(hw_q)		(0x0620 + ((hw_q) << 2))
@@ -249,6 +252,8 @@
 #define GEM_RXBS_SIZE		8
 #define GEM_DDRP_OFFSET		24 /* disc_when_no_ahb */
 #define GEM_DDRP_SIZE		1
+#define GEM_ADDR64_OFFSET	30 /* Address bus width - 64b or 32b */
+#define GEM_ADDR64_SIZE		1
 
 
 /* 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
 };
 
 /* DMA descriptor bitfields */
@@ -777,6 +786,7 @@ struct macb_queue {
 	unsigned int		IDR;
 	unsigned int		IMR;
 	unsigned int		TBQP;
+	unsigned int		TBQPH;
 
 	unsigned int		tx_head, tx_tail;
 	struct macb_dma_desc	*tx_ring;
-- 
2.7.4

Re: [RFC PATCH 1/2] net: macb: Correct CAPS mask

[Nicolas Ferre]

Le 01/08/2016 à 09:20, Harini Katakam a écrit :
> USRIO and JUMBO CAPS have the same mask.
> Fix the same.
> 
> Signed-off-by: Harini Katakam <harinik@xilinx.com>

Hi,
Indeed there's a bug...


> ---
>  drivers/net/ethernet/cadence/macb.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 36893d8..b6fcf10 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -403,11 +403,11 @@
>  #define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII	0x00000004
>  #define MACB_CAPS_NO_GIGABIT_HALF		0x00000008
>  #define MACB_CAPS_USRIO_DISABLED		0x00000010
> +#define MACB_CAPS_JUMBO				0x00000020
>  #define MACB_CAPS_FIFO_MODE			0x10000000
>  #define MACB_CAPS_GIGABIT_MODE_AVAILABLE	0x20000000
>  #define MACB_CAPS_SG_DISABLED			0x40000000
>  #define MACB_CAPS_MACB_IS_GEM			0x80000000
> -#define MACB_CAPS_JUMBO				0x00000010

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Can you please send it independently without the RFC tag in the subject
and with the following tags in the body as well:

Fixes: ce721a702197 ("net: ethernet: cadence-macb: Add disabled usrio caps")
Cc: stable@vger.kernel.org # v4.5+

>  /* Bit manipulation macros */
>  #define MACB_BIT(name)					\
> 

Thanks, bye,
-- 
Nicolas Ferre

Re: [RFC PATCH 1/2] net: macb: Correct CAPS mask

[Harini Katakam]

On Thu, Aug 4, 2016 at 7:37 PM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> Le 01/08/2016 à 09:20, Harini Katakam a écrit :
>> USRIO and JUMBO CAPS have the same mask.
>> Fix the same.
>>
>> Signed-off-by: Harini Katakam <harinik@xilinx.com>
>
> Hi,
> Indeed there's a bug...
>
>
>> ---
>>  drivers/net/ethernet/cadence/macb.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
>> index 36893d8..b6fcf10 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -403,11 +403,11 @@
>>  #define MACB_CAPS_USRIO_DEFAULT_IS_MII_GMII  0x00000004
>>  #define MACB_CAPS_NO_GIGABIT_HALF            0x00000008
>>  #define MACB_CAPS_USRIO_DISABLED             0x00000010
>> +#define MACB_CAPS_JUMBO                              0x00000020
>>  #define MACB_CAPS_FIFO_MODE                  0x10000000
>>  #define MACB_CAPS_GIGABIT_MODE_AVAILABLE     0x20000000
>>  #define MACB_CAPS_SG_DISABLED                        0x40000000
>>  #define MACB_CAPS_MACB_IS_GEM                        0x80000000
>> -#define MACB_CAPS_JUMBO                              0x00000010
>
> Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>
>
> Can you please send it independently without the RFC tag in the subject
> and with the following tags in the body as well:
>
> Fixes: ce721a702197 ("net: ethernet: cadence-macb: Add disabled usrio caps")
> Cc: stable@vger.kernel.org # v4.5+
>

Thanks Nicolas. I'll do that.

Regards,
Harini

>>  /* Bit manipulation macros */
>>  #define MACB_BIT(name)                                       \
>>
>
> Thanks, bye,
> --
> Nicolas Ferre

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

[Rafal Ozieblo]

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.

Regards,
Rafal

 

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

[Harini Katakam]

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.

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.

Regards,
Harini

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

[Rafal Ozieblo]

-----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

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

[Nicolas Ferre]

Le 17/11/2016 à 13:21, Harini Katakam a écrit :
> 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.
> 
> 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

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

[Rafal Ozieblo]

-----Original Message-----
From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com] 
Sent: 17 listopada 2016 14:29
To: Harini Katakam; Rafal Ozieblo
Cc: harini.katakam@xilinx.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

> Le 17/11/2016 à 13:21, Harini Katakam a écrit :
> > 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.
> > 
> > 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.

Regards, 
Rafal Ozieblo

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

[Harini Katakam]

Hi Rafal,

On Thu, Nov 17, 2016 at 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
> -----Original Message-----
> From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]
> Sent: 17 listopada 2016 14:29
> To: Harini Katakam; Rafal Ozieblo
> Cc: harini.katakam@xilinx.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
>
>> Le 17/11/2016 à 13:21, Harini Katakam a écrit :
>> > 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.
>> >
>> > 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?

Regards,
Harini

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

[Rafal Ozieblo]

Hello,

> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]
> Sent: 18 listopada 2016 05:30
> To: Rafal Ozieblo
> Cc: Nicolas Ferre; harini.katakam@xilinx.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 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
> > -----Original Message-----
> > From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]
> > Sent: 17 listopada 2016 14:29
> > To: Harini Katakam; Rafal Ozieblo
> > Cc: harini.katakam@xilinx.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
> >
> >> Le 17/11/2016 à 13:21, Harini Katakam a écrit :
> >> > 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.
> >> >
> >> > 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!

1. 32b and without PTP support:

struct macb_dma_desc {
	u32	addr;
	u32	ctrl;
}

2. 64b and without PTP support:

struct macb_dma_desc {
	u32	addr;
	u32	ctrl;
	u32 	addrh;
	u32  	resvd;
}

3. 32 and PTP support:

struct macb_dma_desc {
	u32	addr;
	u32	ctrl;
	u32	dma_desc_ts_1;
	u32	dma_desc_ts_2;
};

4. 64b and PTP support:

struct macb_dma_desc {
	u32 	addr;
	u32 	ctrl;
	u32 	addrh;
	u32  	resvd;
	u32 	dma_desc_ts_1;
	u32	dma_desc_ts_2;
};

> 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?

I have some ideas but I don’t know whether it is possible to upstream it later on.
IMHO, we need to use the same mechanism in both our cases.

My very first idea to start discussion:
(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()

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.

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.

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

[Nicolas Ferre]

Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit :
> Hello,
> 
>> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]
>> Sent: 18 listopada 2016 05:30
>> To: Rafal Ozieblo
>> Cc: Nicolas Ferre; harini.katakam@xilinx.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 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
>>> -----Original Message-----
>>> From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]
>>> Sent: 17 listopada 2016 14:29
>>> To: Harini Katakam; Rafal Ozieblo
>>> Cc: harini.katakam@xilinx.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
>>>
>>>> Le 17/11/2016 à 13:21, Harini Katakam a écrit :
>>>>> 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.
>>>>>
>>>>> 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://marc.info/?l=linux-netdev&m=147282092309112&w=3
or
https://patchwork.kernel.org/patch/9310989/
and
https://patchwork.kernel.org/patch/9310991/

Regards,

> 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!
> 
> 1. 32b and without PTP support:
> 
> struct macb_dma_desc {
> 	u32	addr;
> 	u32	ctrl;
> }
> 
> 2. 64b and without PTP support:
> 
> struct macb_dma_desc {
> 	u32	addr;
> 	u32	ctrl;
> 	u32 	addrh;
> 	u32  	resvd;
> }
> 
> 3. 32 and PTP support:
> 
> struct macb_dma_desc {
> 	u32	addr;
> 	u32	ctrl;
> 	u32	dma_desc_ts_1;
> 	u32	dma_desc_ts_2;
> };
> 
> 4. 64b and PTP support:
> 
> struct macb_dma_desc {
> 	u32 	addr;
> 	u32 	ctrl;
> 	u32 	addrh;
> 	u32  	resvd;
> 	u32 	dma_desc_ts_1;
> 	u32	dma_desc_ts_2;
> };
> 
>> 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?
> 
> I have some ideas but I don’t know whether it is possible to upstream it later on.
> IMHO, we need to use the same mechanism in both our cases.
> 
> My very first idea to start discussion:
> (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()
> 
> 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.
> 
> 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.
> 


-- 
Nicolas Ferre

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

[Rafal Ozieblo]

>-----Original Message-----         
>From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com] 
>Sent: 18 listopada 2016 10:10                        
>To: Rafal Ozieblo; Harini Katakam; Andrei Pistirica  
>Cc: harini.katakam@xilinx.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      
>                                                                                   
>Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit :                                     
>> Hello,                                                                           
>>                                                                                  
>>> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]                      
>>> Sent: 18 listopada 2016 05:30                                                   
>>> To: Rafal Ozieblo                                                               
>>> Cc: Nicolas Ferre; harini.katakam@xilinx.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 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:      
>>>> -----Original Message-----                                                     
>>>> From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]                           
>>>> Sent: 17 listopada 2016 14:29                                                  
>>>> To: Harini Katakam; Rafal Ozieblo                                              
>>>> Cc: harini.katakam@xilinx.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                                                                
>>>>                                                                                
>>>>> Le 17/11/2016 à 13:21, Harini Katakam a écrit :                               
>>>>>> 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.                                      
>>>>>>                                                                                                   
>>>>>> 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-3Dlinux-2Dnetdev-26m-3D147282092309112-26w-3D3&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=kr_km0MKQBzpkaOt0fkM-FIajN1-pOylzzTjsXi-vak&e=    
>or                                                                                                      
>https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9310989_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=ZbXVD5Lb5zGn7wUKIPYHxagIEKp_vJvrnkRa4qJfmTY&e=                              
>and                                                                                                     
>https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9310991_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=Z0kRqUqh5Is4TmuaY7A4hnpizfdeq3HrgPhyDAMLuA8&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?

Best regards,

Rafal Ozieblo   |   Firmware System Engineer, 
phone nbr.: +48 32 5085469    
www.cadence.com

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

[Harini Katakam]

Hi Rafal,

On Fri, Nov 18, 2016 at 3:00 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
>>-----Original Message-----
>>From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]
>>Sent: 18 listopada 2016 10:10
>>To: Rafal Ozieblo; Harini Katakam; Andrei Pistirica
>>Cc: harini.katakam@xilinx.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
>>
>>Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit :
>>> Hello,
>>>
>>>> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]
>>>> Sent: 18 listopada 2016 05:30
>>>> To: Rafal Ozieblo
>>>> Cc: Nicolas Ferre; harini.katakam@xilinx.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 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
>>>>> -----Original Message-----
>>>>> From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]
>>>>> Sent: 17 listopada 2016 14:29
>>>>> To: Harini Katakam; Rafal Ozieblo
>>>>> Cc: harini.katakam@xilinx.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
>>>>>
>>>>>> Le 17/11/2016 à 13:21, Harini Katakam a écrit :
>>>>>>> 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.
>>>>>>>
>>>>>>> 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-3Dlinux-2Dnetdev-26m-3D147282092309112-26w-3D3&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=kr_km0MKQBzpkaOt0fkM-FIajN1-pOylzzTjsXi-vak&e=
>>or
>>https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9310989_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=ZbXVD5Lb5zGn7wUKIPYHxagIEKp_vJvrnkRa4qJfmTY&e=
>>and
>>https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9310991_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=Z0kRqUqh5Is4TmuaY7A4hnpizfdeq3HrgPhyDAMLuA8&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://lkml.org/lkml/2015/9/11/92
But right now, i'm working on building on top of Andre's changes.

Regards,
Harini

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

[Rafal Ozieblo]

>                                                     
>-----Original Message-----                           
>From: Harini Katakam [mailto:harinikatakamlinux@gmail.com] 
>Sent: 18 listopada 2016 10:44                              
>To: Rafal Ozieblo                                          
>Cc: Nicolas Ferre; Andrei Pistirica; harini.katakam@xilinx.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 Fri, Nov 18, 2016 at 3:00 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:                              
>>>-----Original Message-----                                                                            
>>>From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]                                                  
>>>Sent: 18 listopada 2016 10:10                                                                         
>>>To: Rafal Ozieblo; Harini Katakam; Andrei Pistirica                                                   
>>>Cc: harini.katakam@xilinx.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                                                                                               
>>>                                                                                                      
>>>Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit :                                                        
>>>> Hello,                                                                                              
>>>>                                                                                                     
>>>>> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]                                         
>>>>> Sent: 18 listopada 2016 05:30                                                                      
>>>>> To: Rafal Ozieblo                                                                                  
>>>>> Cc: Nicolas Ferre; harini.katakam@xilinx.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 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:                         
>>>>>> -----Original Message-----                                                                        
>>>>>> From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]                                              
>>>>>> Sent: 17 listopada 2016 14:29                                                                     
>>>>>> To: Harini Katakam; Rafal Ozieblo                                                                 
>>>>>> Cc: harini.katakam@xilinx.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                                                                                   
>>>>>>                                                                                                   
>>>>>>> Le 17/11/2016 à 13:21, Harini Katakam a écrit :                                                  
>>>>>>>> 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.                                    
>>>>>>>>                                                                                                 
>>>>>>>> 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 ? 

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

[Harini Katakam]

Hi Rafal,


> I can’t see a place where you enable extended descriptor for PTP. Did you add support for extended PTP descriptor?
Sorry, that was separate patch in the series:
http://lkml.iu.edu/hypermail/linux/kernel/1509.1/02239.html

> "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 ?
Sure, please mail. Also, please mention the Cadence IP version number you use.

Regards,
Harini

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

[Nicolas Ferre]

Le 18/11/2016 à 10:59, Rafal Ozieblo a écrit :
>>                                                     
>> -----Original Message-----                           
>> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com] 
>> Sent: 18 listopada 2016 10:44                              
>> To: Rafal Ozieblo                                          
>> Cc: Nicolas Ferre; Andrei Pistirica; harini.katakam@xilinx.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 Fri, Nov 18, 2016 at 3:00 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:                              
>>>> -----Original Message-----                                                                            
>>>> From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]                                                  
>>>> Sent: 18 listopada 2016 10:10                                                                         
>>>> To: Rafal Ozieblo; Harini Katakam; Andrei Pistirica                                                   
>>>> Cc: harini.katakam@xilinx.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                                                                                               
>>>>                                                                                                      
>>>> Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit :                                                        
>>>>> Hello,                                                                                              
>>>>>                                                                                                     
>>>>>> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]                                         
>>>>>> Sent: 18 listopada 2016 05:30                                                                      
>>>>>> To: Rafal Ozieblo                                                                                  
>>>>>> Cc: Nicolas Ferre; harini.katakam@xilinx.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 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:                         
>>>>>>> -----Original Message-----                                                                        
>>>>>>> From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]                                              
>>>>>>> Sent: 17 listopada 2016 14:29                                                                     
>>>>>>> To: Harini Katakam; Rafal Ozieblo                                                                 
>>>>>>> Cc: harini.katakam@xilinx.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                                                                                   
>>>>>>>                                                                                                   
>>>>>>>> Le 17/11/2016 à 13:21, Harini Katakam a écrit :                                                  
>>>>>>>>> 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.                                    
>>>>>>>>>                                                                                                 
>>>>>>>>> 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 ? 

You mean "send you and everyone else on the mailing-list" right?
Even if the first initiative has a bit or priority, like Harini's one
ported further by Andrei, we are still eager to share code and ideas to
make this support go forward.

I do think that the steps will be to include generic support for PTP
(without 64bits descriptors) and then plug the 64bits descriptor support
which is an newer extension of it (and definitively more robust and
leading to more accurate behavior, I know). Basically like Harini
suggested: "building on top of Andrei's changes".

Best regards,
-- 
Nicolas Ferre

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

[Andrei Pistirica]

Hi Harini,

On 18.11.2016 10:43, Harini Katakam wrote:
> Hi Rafal,
>
> On Fri, Nov 18, 2016 at 3:00 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
>>> -----Original Message-----
>>> From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]
>>> Sent: 18 listopada 2016 10:10
>>> To: Rafal Ozieblo; Harini Katakam; Andrei Pistirica
>>> Cc: harini.katakam@xilinx.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
>>>
>>> Le 18/11/2016 à 09:59, Rafal Ozieblo a écrit :
>>>> Hello,
>>>>
>>>>> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]
>>>>> Sent: 18 listopada 2016 05:30
>>>>> To: Rafal Ozieblo
>>>>> Cc: Nicolas Ferre; harini.katakam@xilinx.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 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
>>>>>> -----Original Message-----
>>>>>> From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]
>>>>>> Sent: 17 listopada 2016 14:29
>>>>>> To: Harini Katakam; Rafal Ozieblo
>>>>>> Cc: harini.katakam@xilinx.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
>>>>>>
>>>>>>> Le 17/11/2016 à 13:21, Harini Katakam a écrit :
>>>>>>>> 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.
>>>>>>>>
>>>>>>>> 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-3Dlinux-2Dnetdev-26m-3D147282092309112-26w-3D3&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=kr_km0MKQBzpkaOt0fkM-FIajN1-pOylzzTjsXi-vak&e=
>>> or
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9310989_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=ZbXVD5Lb5zGn7wUKIPYHxagIEKp_vJvrnkRa4qJfmTY&e=
>>> and
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_9310991_&d=DgIDaQ&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=MWSZN_jP4BHjuU9UFsrndALGeJqIXzD0WtGCtCg4L5E&m=jUmZzqwn1uRrvcqF3BHCHFC6ZsKoZkU3vT8gJ-7fnsE&s=Z0kRqUqh5Is4TmuaY7A4hnpizfdeq3HrgPhyDAMLuA8&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://lkml.org/lkml/2015/9/11/92
> But right now, i'm working on building on top of Andre's changes.

I'm addressing maintainer's feedback on both patches:
https://patchwork.kernel.org/patch/9310989/
https://patchwork.kernel.org/patch/9310991/

I've done all suggested updates, except the numerous 64bit divisions in 
the frequency adjustment callback. I've implemented a different 
algorithm which uses 2 64bit division, but I couldn't find a way to use 
only 1.

Also, I have a version with timecounter/cyclecounter which shows a much 
better accuracy (less than 100ns level). In my opinion this could be a 
better implementation. What is you opinion about this? Did you try it?

Tuesday I can send an updated version of these patches.

>
> Regards,
> Harini
>

Regards,
Andrei

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

[Harini Katakam]

tHi Andrei,

>> 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://lkml.org/lkml/2015/9/11/92
>> But right now, i'm working on building on top of Andre's changes.
>
>
> I'm addressing maintainer's feedback on both patches:
> https://patchwork.kernel.org/patch/9310989/
> https://patchwork.kernel.org/patch/9310991/
>
> I've done all suggested updates, except the numerous 64bit divisions in the
> frequency adjustment callback. I've implemented a different algorithm which
> uses 2 64bit division, but I couldn't find a way to use only 1.
>
> Also, I have a version with timecounter/cyclecounter which shows a much
> better accuracy (less than 100ns level). In my opinion this could be a
> better implementation. What is you opinion about this? Did you try it?
>

I did not try timecounter on Cadence IP versions later than r1p06.
Because with sub ns precision in HW timestamp, that works better
than SW cyclecounter.
On older Zynq version, yes timecounter is used and is better.

Regards,
Harini

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@cadence.com> wrote:
> Hello,
>
>> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]
>> Sent: 18 listopada 2016 05:30
>> To: Rafal Ozieblo
>> Cc: Nicolas Ferre; harini.katakam@xilinx.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 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
>> > -----Original Message-----
>> > From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]
>> > Sent: 17 listopada 2016 14:29
>> > To: Harini Katakam; Rafal Ozieblo
>> > Cc: harini.katakam@xilinx.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
>> >
>> >> Le 17/11/2016 à 13:21, Harini Katakam a écrit :
>> >> > 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.
>> >> >
>> >> > 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.

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

[Rafal Ozieblo]

>Hi Rafal,                                  
>                                           
>On Fri, Nov 18, 2016 at 2:29 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
>> Hello,                                                                  
>>                                                                         
>>> From: Harini Katakam [mailto:harinikatakamlinux@gmail.com]             
>>> Sent: 18 listopada 2016 05:30                                          
>>> To: Rafal Ozieblo                                                      
>>> Cc: Nicolas Ferre; harini.katakam@xilinx.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 7:05 PM, Rafal Ozieblo <rafalo@cadence.com> wrote:
>>> > -----Original Message-----                                              
>>> > From: Nicolas Ferre [mailto:nicolas.ferre@atmel.com]                    
>>> > Sent: 17 listopada 2016 14:29                                           
>>> > To: Harini Katakam; Rafal Ozieblo                                       
>>> > Cc: harini.katakam@xilinx.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                                                         
>>> >                                                                         
>>> >> Le 17/11/2016 à 13:21, Harini Katakam a écrit :                        
>>> >> > 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.                                    
>>> >> >                                                                                                 
>>> >> > 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