[PATCH linux 0/4] Enable ftgmac100 driver on Aspeed g5 SoCs

[PATCH linux 0/4] Enable ftgmac100 driver on Aspeed g5 SoCs

[Andrew Jeffery]

Hi all,

These patches are already applied to the dev-4.7 branch but hadn't been sent
anywhere for feedback.

The first patch is a work-around that gets the ftgmac100 driver probing on the
AST2500 EVB (or any non-NCSI configuration) in the short term. Longer term we
should probably implement the new MDC/MDIO register interface. Shouldn't (yet)
be sent upstream, but required in some cases around these parts.

Patches two and three had some queries from Joel regarding parameters I've
added to a number of functions. I've done some analysis on whether we can avoid
it, but it seems we cannot. Thoughts there would be appreciated.

Patch four is a trivial one for aspeed-g5.dtsi if you accept the general
approach of patch three.

Andrew Jeffery (4):
  aspeed: Use old MDC/MDIO interface for ftgmac100 driver compat
  ftgmac100: Separate rx page storage from rxdesc
  ftgmac100: Add devicetree option for the Aspeed g5 register interface
  aspeed-g5: Configure ftgmac100s for the Aspeed g5 register interface

 arch/arm/boot/dts/aspeed-g5.dtsi         |  2 +
 arch/arm/mach-aspeed/aspeed.c            |  8 ++++
 drivers/net/ethernet/faraday/ftgmac100.c | 76 +++++++++++++++++++++++---------
 drivers/net/ethernet/faraday/ftgmac100.h |  2 -
 4 files changed, 64 insertions(+), 24 deletions(-)

-- 
2.7.4

[PATCH linux 1/4] aspeed: Use old MDC/MDIO interface for ftgmac100 driver compat

[Andrew Jeffery]

This is a work-around, and we should aim to move to the new interface.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/mach-aspeed/aspeed.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c
index 90c541f32ee8..12f3a044a03c 100644
--- a/arch/arm/mach-aspeed/aspeed.c
+++ b/arch/arm/mach-aspeed/aspeed.c
@@ -82,6 +82,7 @@ static void __init aspeed_dt_init(void)
 
 #define AST_BASE_LPC		0x1E789000 /* LPC Controller */
 #define AST_BASE_SPI		0x1E630000 /* SPI Memory Controller */
+#define AST_BASE_MAC1		0X1E660000 /* MAC 1 */
 #define AST_BASE_SCU		0x1E6E2000 /* System Control Unit (SCU) */
 #define AST_BASE_GPIO		0x1E780000 /* GPIO Controller */
 
@@ -186,8 +187,15 @@ static void __init do_garrison_setup(void)
 
 static void __init do_ast2500evb_setup(void)
 {
+	unsigned long reg;
+
 	/* Reset AHB bridges */
 	writel(0x02, AST_IO(AST_BASE_SCU | 0x04));
+
+	/* Set old MDIO interface */
+	reg = readl(AST_IO(AST_BASE_MAC1 | 0x40));
+	reg &= ~0x80000000;
+	writel(reg, AST_IO(AST_BASE_MAC1 | 0x40));
 }
 
 #define SCU_PASSWORD	0x1688A8A8
-- 
2.7.4

[PATCH linux 2/4] ftgmac100: Separate rx page storage from rxdesc

[Andrew Jeffery]

The ftgmac100 hardware revision in e.g. the Aspeed AST2500 no longer
reserves all bits in RXDES#2 but instead uses the bottom 16 bits to
store MAC frame metadata. Avoid corruption by shifting struct page
pointers out to their own member in struct ftgmac100.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---

Joel asked if we could avoid passing both priv and rxdes to the get/set
helpers.  From a quick investigation the answer appears to be no: If we were to
do so we'd need a consistent way to determine the rxdes of interest.
ftgmac100_rxdes_get_page() has two call sites that do not have a common
approach to computing the rxdes of interest. ftgmac100_rxdes_set_page() has one
call site in ftgmac100_alloc_rx_page(), but the rxdes of interest is passed as
an argument to ftgmac100_alloc_rx_page() and so without cross-function analysis
I don't think we claim that there's a consistent means, or would be in the
future.

 drivers/net/ethernet/faraday/ftgmac100.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index 28fa52ebff31..f7d61c4703cf 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -60,6 +60,8 @@ struct ftgmac100 {
 	struct ftgmac100_descs *descs;
 	dma_addr_t descs_dma_addr;
 
+	struct page *rx_pages[RX_QUEUE_ENTRIES];
+
 	unsigned int rx_pointer;
 	unsigned int tx_clean_pointer;
 	unsigned int tx_pointer;
@@ -350,18 +352,27 @@ static bool ftgmac100_rxdes_ipcs_err(struct ftgmac100_rxdes *rxdes)
 	return rxdes->rxdes1 & cpu_to_le32(FTGMAC100_RXDES1_IP_CHKSUM_ERR);
 }
 
+static struct page **ftgmac100_rxdes_page_slot(struct ftgmac100 *priv,
+					       struct ftgmac100_rxdes *rxdes)
+{
+	return &priv->rx_pages[rxdes - priv->descs->rxdes];
+}
+
 /*
  * rxdes2 is not used by hardware. We use it to keep track of page.
  * Since hardware does not touch it, we can skip cpu_to_le32()/le32_to_cpu().
  */
-static void ftgmac100_rxdes_set_page(struct ftgmac100_rxdes *rxdes, struct page *page)
+static void ftgmac100_rxdes_set_page(struct ftgmac100 *priv,
+				     struct ftgmac100_rxdes *rxdes,
+				     struct page *page)
 {
-	rxdes->rxdes2 = (unsigned int)page;
+	*ftgmac100_rxdes_page_slot(priv, rxdes) = page;
 }
 
-static struct page *ftgmac100_rxdes_get_page(struct ftgmac100_rxdes *rxdes)
+static struct page *ftgmac100_rxdes_get_page(struct ftgmac100 *priv,
+					     struct ftgmac100_rxdes *rxdes)
 {
-	return (struct page *)rxdes->rxdes2;
+	return *ftgmac100_rxdes_page_slot(priv, rxdes);
 }
 
 /******************************************************************************
@@ -510,7 +521,7 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, int *processed)
 
 	do {
 		dma_addr_t map = ftgmac100_rxdes_get_dma_addr(rxdes);
-		struct page *page = ftgmac100_rxdes_get_page(rxdes);
+		struct page *page = ftgmac100_rxdes_get_page(priv, rxdes);
 		unsigned int size;
 
 		dma_unmap_page(priv->dev, map, RX_BUF_SIZE, DMA_FROM_DEVICE);
@@ -788,7 +799,7 @@ static int ftgmac100_alloc_rx_page(struct ftgmac100 *priv,
 		return -ENOMEM;
 	}
 
-	ftgmac100_rxdes_set_page(rxdes, page);
+	ftgmac100_rxdes_set_page(priv, rxdes, page);
 	ftgmac100_rxdes_set_dma_addr(rxdes, map);
 	ftgmac100_rxdes_set_dma_own(rxdes);
 	return 0;
@@ -800,7 +811,7 @@ static void ftgmac100_free_buffers(struct ftgmac100 *priv)
 
 	for (i = 0; i < RX_QUEUE_ENTRIES; i++) {
 		struct ftgmac100_rxdes *rxdes = &priv->descs->rxdes[i];
-		struct page *page = ftgmac100_rxdes_get_page(rxdes);
+		struct page *page = ftgmac100_rxdes_get_page(priv, rxdes);
 		dma_addr_t map = ftgmac100_rxdes_get_dma_addr(rxdes);
 
 		if (!page)
-- 
2.7.4

[PATCH linux 3/4] ftgmac100: Add devicetree option for the Aspeed g5 register interface

[Andrew Jeffery]

Amongst other small changes, the g5 SoCs shift the bits used to signal
EDORR/EDOTR states. Abstract over the difference by making the masks
variable and configuring them from the device tree.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---

It's likely that Joel's query on passing both priv and rxdes/txdes also applied
to this patch. The functions of interest are:

ftgmac100_txdes_reset():

         We can use ftgmac100_current_clean_txdes() internally iff
         ftgmac100_tx_clean_pointer_advance() is not called before
         ftgmac100_txdes_reset(). It probably won't be, but it feels like a
         dangerous assumption.

ftgmac100_rxdes_set_dma_own():

         Has 3 call-sites, two use the current rxdes pointer, but one call-site
         is unclear; this is again in ftgmac100_alloc_rx_page(), where the
         rxdes of interest is passed in as a parameter.

ftgmac100_rxdes_set_end_of_ring():

         There's only one call-site, where the last descriptor in the array is
         marked as the end-of-ring. Here we could just pass priv.

ftgmac100_txdes_set_end_of_ring():

         There's only one call-site, where the last descriptor in the array is
         marked as the end-of-ring. Here we could just pass priv.

Overall, in some cases we can't accomodate the suggestion, in other cases it's
murky, and sometimes we could. However, for consistency, I think it's best we
take the one explicit approach.

 drivers/net/ethernet/faraday/ftgmac100.c | 51 ++++++++++++++++++++++----------
 drivers/net/ethernet/faraday/ftgmac100.h |  2 --
 2 files changed, 36 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
index f7d61c4703cf..9f7ca81718fa 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.c
+++ b/drivers/net/ethernet/faraday/ftgmac100.c
@@ -79,6 +79,9 @@ struct ftgmac100 {
 
 	bool use_ncsi;
 	bool enabled;
+
+	u32 rxdes0_edorr_mask;
+	u32 txdes0_edotr_mask;
 };
 
 static int ftgmac100_alloc_rx_page(struct ftgmac100 *priv,
@@ -268,10 +271,11 @@ static bool ftgmac100_rxdes_packet_ready(struct ftgmac100_rxdes *rxdes)
 	return rxdes->rxdes0 & cpu_to_le32(FTGMAC100_RXDES0_RXPKT_RDY);
 }
 
-static void ftgmac100_rxdes_set_dma_own(struct ftgmac100_rxdes *rxdes)
+static void ftgmac100_rxdes_set_dma_own(const struct ftgmac100 *priv,
+					struct ftgmac100_rxdes *rxdes)
 {
 	/* clear status bits */
-	rxdes->rxdes0 &= cpu_to_le32(FTGMAC100_RXDES0_EDORR);
+	rxdes->rxdes0 &= cpu_to_le32(priv->rxdes0_edorr_mask);
 }
 
 static bool ftgmac100_rxdes_rx_error(struct ftgmac100_rxdes *rxdes)
@@ -309,9 +313,10 @@ static bool ftgmac100_rxdes_multicast(struct ftgmac100_rxdes *rxdes)
 	return rxdes->rxdes0 & cpu_to_le32(FTGMAC100_RXDES0_MULTICAST);
 }
 
-static void ftgmac100_rxdes_set_end_of_ring(struct ftgmac100_rxdes *rxdes)
+static void ftgmac100_rxdes_set_end_of_ring(const struct ftgmac100 *priv,
+					    struct ftgmac100_rxdes *rxdes)
 {
-	rxdes->rxdes0 |= cpu_to_le32(FTGMAC100_RXDES0_EDORR);
+	rxdes->rxdes0 |= cpu_to_le32(priv->rxdes0_edorr_mask);
 }
 
 static void ftgmac100_rxdes_set_dma_addr(struct ftgmac100_rxdes *rxdes,
@@ -402,7 +407,7 @@ ftgmac100_rx_locate_first_segment(struct ftgmac100 *priv)
 		if (ftgmac100_rxdes_first_segment(rxdes))
 			return rxdes;
 
-		ftgmac100_rxdes_set_dma_own(rxdes);
+		ftgmac100_rxdes_set_dma_own(priv, rxdes);
 		ftgmac100_rx_pointer_advance(priv);
 		rxdes = ftgmac100_current_rxdes(priv);
 	}
@@ -473,7 +478,7 @@ static void ftgmac100_rx_drop_packet(struct ftgmac100 *priv)
 		if (ftgmac100_rxdes_last_segment(rxdes))
 			done = true;
 
-		ftgmac100_rxdes_set_dma_own(rxdes);
+		ftgmac100_rxdes_set_dma_own(priv, rxdes);
 		ftgmac100_rx_pointer_advance(priv);
 		rxdes = ftgmac100_current_rxdes(priv);
 	} while (!done && ftgmac100_rxdes_packet_ready(rxdes));
@@ -565,10 +570,11 @@ static bool ftgmac100_rx_packet(struct ftgmac100 *priv, int *processed)
 /******************************************************************************
  * internal functions (transmit descriptor)
  *****************************************************************************/
-static void ftgmac100_txdes_reset(struct ftgmac100_txdes *txdes)
+static void ftgmac100_txdes_reset(const struct ftgmac100 *priv,
+				  struct ftgmac100_txdes *txdes)
 {
 	/* clear all except end of ring bit */
-	txdes->txdes0 &= cpu_to_le32(FTGMAC100_TXDES0_EDOTR);
+	txdes->txdes0 &= cpu_to_le32(priv->txdes0_edotr_mask);
 	txdes->txdes1 = 0;
 	txdes->txdes2 = 0;
 	txdes->txdes3 = 0;
@@ -589,9 +595,10 @@ static void ftgmac100_txdes_set_dma_own(struct ftgmac100_txdes *txdes)
 	txdes->txdes0 |= cpu_to_le32(FTGMAC100_TXDES0_TXDMA_OWN);
 }
 
-static void ftgmac100_txdes_set_end_of_ring(struct ftgmac100_txdes *txdes)
+static void ftgmac100_txdes_set_end_of_ring(const struct ftgmac100 *priv,
+					    struct ftgmac100_txdes *txdes)
 {
-	txdes->txdes0 |= cpu_to_le32(FTGMAC100_TXDES0_EDOTR);
+	txdes->txdes0 |= cpu_to_le32(priv->txdes0_edotr_mask);
 }
 
 static void ftgmac100_txdes_set_first_segment(struct ftgmac100_txdes *txdes)
@@ -710,7 +717,7 @@ static bool ftgmac100_tx_complete_packet(struct ftgmac100 *priv)
 
 	dev_kfree_skb(skb);
 
-	ftgmac100_txdes_reset(txdes);
+	ftgmac100_txdes_reset(priv, txdes);
 
 	ftgmac100_tx_clean_pointer_advance(priv);
 
@@ -801,7 +808,7 @@ static int ftgmac100_alloc_rx_page(struct ftgmac100 *priv,
 
 	ftgmac100_rxdes_set_page(priv, rxdes, page);
 	ftgmac100_rxdes_set_dma_addr(rxdes, map);
-	ftgmac100_rxdes_set_dma_own(rxdes);
+	ftgmac100_rxdes_set_dma_own(priv, rxdes);
 	return 0;
 }
 
@@ -840,6 +847,8 @@ static void ftgmac100_free_buffers(struct ftgmac100 *priv)
 static int ftgmac100_alloc_buffers(struct ftgmac100 *priv)
 {
 	int i;
+	struct ftgmac100_rxdes *rxdes;
+	struct ftgmac100_txdes *txdes;
 
 	priv->descs = dma_zalloc_coherent(priv->dev,
 					  sizeof(struct ftgmac100_descs),
@@ -848,17 +857,19 @@ static int ftgmac100_alloc_buffers(struct ftgmac100 *priv)
 		return -ENOMEM;
 
 	/* initialize RX ring */
-	ftgmac100_rxdes_set_end_of_ring(&priv->descs->rxdes[RX_QUEUE_ENTRIES - 1]);
+	rxdes = &priv->descs->rxdes[RX_QUEUE_ENTRIES - 1];
+	ftgmac100_rxdes_set_end_of_ring(priv, rxdes);
 
 	for (i = 0; i < RX_QUEUE_ENTRIES; i++) {
-		struct ftgmac100_rxdes *rxdes = &priv->descs->rxdes[i];
+		rxdes = &priv->descs->rxdes[i];
 
 		if (ftgmac100_alloc_rx_page(priv, rxdes, GFP_KERNEL))
 			goto err;
 	}
 
 	/* initialize TX ring */
-	ftgmac100_txdes_set_end_of_ring(&priv->descs->txdes[TX_QUEUE_ENTRIES - 1]);
+	txdes = &priv->descs->txdes[TX_QUEUE_ENTRIES - 1];
+	ftgmac100_txdes_set_end_of_ring(priv, txdes);
 	return 0;
 
 err:
@@ -1346,6 +1358,15 @@ static int ftgmac100_probe(struct platform_device *pdev)
 		priv->use_ncsi = false;
 	}
 
+	if (pdev->dev.of_node &&
+	    of_get_property(pdev->dev.of_node, "aspeed-g5-interface", NULL)) {
+		priv->rxdes0_edorr_mask = (1 << 30);
+		priv->txdes0_edotr_mask = (1 << 30);
+	} else {
+		priv->rxdes0_edorr_mask = (1 << 15);
+		priv->txdes0_edotr_mask = (1 << 15);
+	}
+
 	netdev->ethtool_ops = &ftgmac100_ethtool_ops;
 	netdev->netdev_ops = &ftgmac100_netdev_ops;
 	if (pdev->dev.of_node &&
diff --git a/drivers/net/ethernet/faraday/ftgmac100.h b/drivers/net/ethernet/faraday/ftgmac100.h
index 13408d448b05..c258586ce4a4 100644
--- a/drivers/net/ethernet/faraday/ftgmac100.h
+++ b/drivers/net/ethernet/faraday/ftgmac100.h
@@ -189,7 +189,6 @@ struct ftgmac100_txdes {
 } __attribute__ ((aligned(16)));
 
 #define FTGMAC100_TXDES0_TXBUF_SIZE(x)	((x) & 0x3fff)
-#define FTGMAC100_TXDES0_EDOTR		(1 << 15)
 #define FTGMAC100_TXDES0_CRC_ERR	(1 << 19)
 #define FTGMAC100_TXDES0_LTS		(1 << 28)
 #define FTGMAC100_TXDES0_FTS		(1 << 29)
@@ -215,7 +214,6 @@ struct ftgmac100_rxdes {
 } __attribute__ ((aligned(16)));
 
 #define FTGMAC100_RXDES0_VDBC		0x3fff
-#define FTGMAC100_RXDES0_EDORR		(1 << 15)
 #define FTGMAC100_RXDES0_MULTICAST	(1 << 16)
 #define FTGMAC100_RXDES0_BROADCAST	(1 << 17)
 #define FTGMAC100_RXDES0_RX_ERR		(1 << 18)
-- 
2.7.4

[PATCH linux 4/4] aspeed-g5: Configure ftgmac100s for the Aspeed g5 register interface

[Andrew Jeffery]

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/aspeed-g5.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index c86e3d5bfab1..afddbb8d28b4 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -41,6 +41,7 @@
 			compatible = "faraday,ftgmac100";
 			reg = <0x1e660000 0x180>;
 			interrupts = <2>;
+			aspeed-g5-interface;
 			no-hw-checksum;
 			status = "disabled";
 		};
@@ -49,6 +50,7 @@
 			compatible = "faraday,ftgmac100";
 			reg = <0x1e680000 0x180>;
 			interrupts = <3>;
+			aspeed-g5-interface;
 			no-hw-checksum;
 			status = "disabled";
 		};
-- 
2.7.4

Re: [PATCH linux 1/4] aspeed: Use old MDC/MDIO interface for ftgmac100 driver compat

[Joel Stanley]

On Thu, Jun 9, 2016 at 11:44 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> This is a work-around, and we should aim to move to the new interface.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  arch/arm/mach-aspeed/aspeed.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/mach-aspeed/aspeed.c b/arch/arm/mach-aspeed/aspeed.c
> index 90c541f32ee8..12f3a044a03c 100644
> --- a/arch/arm/mach-aspeed/aspeed.c
> +++ b/arch/arm/mach-aspeed/aspeed.c
> @@ -82,6 +82,7 @@ static void __init aspeed_dt_init(void)
>
>  #define AST_BASE_LPC           0x1E789000 /* LPC Controller */
>  #define AST_BASE_SPI           0x1E630000 /* SPI Memory Controller */
> +#define AST_BASE_MAC1          0X1E660000 /* MAC 1 */
>  #define AST_BASE_SCU           0x1E6E2000 /* System Control Unit (SCU) */
>  #define AST_BASE_GPIO          0x1E780000 /* GPIO Controller */
>
> @@ -186,8 +187,15 @@ static void __init do_garrison_setup(void)
>
>  static void __init do_ast2500evb_setup(void)
>  {
> +       unsigned long reg;
> +
>         /* Reset AHB bridges */
>         writel(0x02, AST_IO(AST_BASE_SCU | 0x04));
> +
> +       /* Set old MDIO interface */
> +       reg = readl(AST_IO(AST_BASE_MAC1 | 0x40));
> +       reg &= ~0x80000000;
> +       writel(reg, AST_IO(AST_BASE_MAC1 | 0x40));

I think this is common to all ast2500 platforms. We should put it in a
function that matches on aspeed,ast2500.


>  }
>
>  #define SCU_PASSWORD   0x1688A8A8
> --
> 2.7.4
>

Re: [PATCH linux 3/4] ftgmac100: Add devicetree option for the Aspeed g5 register interface

[Joel Stanley]

On Thu, Jun 9, 2016 at 11:44 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
> @@ -1346,6 +1358,15 @@ static int ftgmac100_probe(struct platform_device *pdev)
>                 priv->use_ncsi = false;
>         }
>
> +       if (pdev->dev.of_node &&
> +           of_get_property(pdev->dev.of_node, "aspeed-g5-interface", NULL)) {

We should get our device tree experts to chime in here.

An alternative could be to use
of_machine_is_compatible("aspeed,ast2500"). Then we don't need extra
properties.

> +               priv->rxdes0_edorr_mask = (1 << 30);
> +               priv->txdes0_edotr_mask = (1 << 30);
> +       } else {
> +               priv->rxdes0_edorr_mask = (1 << 15);
> +               priv->txdes0_edotr_mask = (1 << 15);
> +       }
> +
>         netdev->ethtool_ops = &ftgmac100_ethtool_ops;
>         netdev->netdev_ops = &ftgmac100_netdev_ops;
>         if (pdev->dev.of_node &&

Re: [PATCH linux 3/4] ftgmac100: Add devicetree option for the Aspeed g5 register interface

[Cédric Le Goater]

Hello, 

On 06/10/2016 10:46 AM, Joel Stanley wrote:
> On Thu, Jun 9, 2016 at 11:44 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
>> @@ -1346,6 +1358,15 @@ static int ftgmac100_probe(struct platform_device *pdev)
>>                 priv->use_ncsi = false;
>>         }
>>
>> +       if (pdev->dev.of_node &&
>> +           of_get_property(pdev->dev.of_node, "aspeed-g5-interface", NULL)) {
> 
> We should get our device tree experts to chime in here.
> 
> An alternative could be to use
> of_machine_is_compatible("aspeed,ast2500"). Then we don't need extra
> properties.

yes. That would be better.


So, given that there always has been fixes from Aspeed on the driver in
u-boot (see below), it seems that these EDO bits have been wrong since
the beginning in Linux.

I am wondering what how this was working. We should probably contact 
Aspeed to have some clarification.

C. 


Current SDK (fixed):

./u-boot-v2016.05-ast2500/drivers/net/ftgmac100.h://#define FTGMAC100_TXDES0_EDOTR		(1 << 15)
./u-boot-v2016.05-ast2500/drivers/net/ftgmac100.h:#define FTGMAC100_TXDES0_EDOTR		(1 << 30) //org is 15 ->30
./u-boot-v2016.05-ast2500/drivers/net/ftgmac100.h://#define FTGMAC100_RXDES0_EDORR		(1 << 15)
./u-boot-v2016.05-ast2500/drivers/net/ftgmac100.h:#define FTGMAC100_RXDES0_EDORR		(1 << 30)	//org 15->30
./u-boot-2013.01-aspeed/drivers/net/ftgmac100.h://#define FTGMAC100_TXDES0_EDOTR		(1 << 15)
./u-boot-2013.01-aspeed/drivers/net/ftgmac100.h:#define FTGMAC100_TXDES0_EDOTR		(1 << 30) //org is 15 ->30
./u-boot-2013.01-aspeed/drivers/net/ftgmac100.h://#define FTGMAC100_RXDES0_EDORR		(1 << 15)
./u-boot-2013.01-aspeed/drivers/net/ftgmac100.h:#define FTGMAC100_RXDES0_EDORR		(1 << 30)	//org 15->30

Older SDK (only aspeednic is fixed with a comment)

./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c://#define EDORR   0x00008000  /* Receive End Of Ring */
./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c:#define EDORR   0x40000000  /* Receive End Of Ring */
./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c://#define EDOTR   0x00008000  /* Transmit End Of Ring */
./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c:#define EDOTR   0x40000000  /* Transmit End Of Ring */
./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c:  rx_ring[rxRingSize - 1].status |= cpu_to_le32(EDORR);
./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c:  tx_ring[txRingSize - 1].status |= cpu_to_le32(EDOTR);

./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/ftgmac100.h:#define FTGMAC100_TXDES0_EDOTR		(1 << 15)
./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/ftgmac100.h:#define FTGMAC100_RXDES0_EDORR		(1 << 15)

Mainline (broken):

./u-boot/drivers/net/ftgmac100.h:#define FTGMAC100_TXDES0_EDOTR		(1 << 15)
./u-boot/drivers/net/ftgmac100.h:#define FTGMAC100_RXDES0_EDORR		(1 << 15)

Re: [PATCH linux 3/4] ftgmac100: Add devicetree option for the Aspeed g5 register interface

[Cédric Le Goater]

On 06/10/2016 02:08 PM, Cédric Le Goater wrote:
> Hello, 
> 
> On 06/10/2016 10:46 AM, Joel Stanley wrote:
>> On Thu, Jun 9, 2016 at 11:44 PM, Andrew Jeffery <andrew@aj.id.au> wrote:
>>> @@ -1346,6 +1358,15 @@ static int ftgmac100_probe(struct platform_device *pdev)
>>>                 priv->use_ncsi = false;
>>>         }
>>>
>>> +       if (pdev->dev.of_node &&
>>> +           of_get_property(pdev->dev.of_node, "aspeed-g5-interface", NULL)) {
>>
>> We should get our device tree experts to chime in here.
>>
>> An alternative could be to use
>> of_machine_is_compatible("aspeed,ast2500"). Then we don't need extra
>> properties.
> 
> yes. That would be better.
> 
> 
> So, given that there always has been fixes from Aspeed on the driver in
> u-boot (see below), it seems that these EDO bits have been wrong since
> the beginning in Linux.
> 
> I am wondering what how this was working. We should probably contact 
> Aspeed to have some clarification.

Aspeed said that they have a different design than Faraday and we should 
be using :

	#define FTGMAC100_TXDES0_EDOTR		(1 << 30) 
	#define FTGMAC100_RXDES0_EDORR		(1 << 30)	

for all boards : ast2400 *and* ast2500. 

I gave it a try on a palmetto and indeed, network works fine with the 
Aspeed EDO{T,R}R bits. 
  
Cheers,

C.
 

> Current SDK (fixed):
> 
> ./u-boot-v2016.05-ast2500/drivers/net/ftgmac100.h://#define FTGMAC100_TXDES0_EDOTR		(1 << 15)
> ./u-boot-v2016.05-ast2500/drivers/net/ftgmac100.h:#define FTGMAC100_TXDES0_EDOTR		(1 << 30) //org is 15 ->30
> ./u-boot-v2016.05-ast2500/drivers/net/ftgmac100.h://#define FTGMAC100_RXDES0_EDORR		(1 << 15)
> ./u-boot-v2016.05-ast2500/drivers/net/ftgmac100.h:#define FTGMAC100_RXDES0_EDORR		(1 << 30)	//org 15->30
> ./u-boot-2013.01-aspeed/drivers/net/ftgmac100.h://#define FTGMAC100_TXDES0_EDOTR		(1 << 15)
> ./u-boot-2013.01-aspeed/drivers/net/ftgmac100.h:#define FTGMAC100_TXDES0_EDOTR		(1 << 30) //org is 15 ->30
> ./u-boot-2013.01-aspeed/drivers/net/ftgmac100.h://#define FTGMAC100_RXDES0_EDORR		(1 << 15)
> ./u-boot-2013.01-aspeed/drivers/net/ftgmac100.h:#define FTGMAC100_RXDES0_EDORR		(1 << 30)	//org 15->30
> 
> Older SDK (only aspeednic is fixed with a comment)
> 
> ./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c://#define EDORR   0x00008000  /* Receive End Of Ring */
> ./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c:#define EDORR   0x40000000  /* Receive End Of Ring */
> ./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c://#define EDOTR   0x00008000  /* Transmit End Of Ring */
> ./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c:#define EDOTR   0x40000000  /* Transmit End Of Ring */
> ./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c:  rx_ring[rxRingSize - 1].status |= cpu_to_le32(EDORR);
> ./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/aspeednic.c:  tx_ring[txRingSize - 1].status |= cpu_to_le32(EDOTR);
> 
> ./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/ftgmac100.h:#define FTGMAC100_TXDES0_EDOTR		(1 << 15)
> ./u-boot-github-v2016.03-aspeed-openbmc/drivers/net/ftgmac100.h:#define FTGMAC100_RXDES0_EDORR		(1 << 15)
> 
> Mainline (broken):
> 
> ./u-boot/drivers/net/ftgmac100.h:#define FTGMAC100_TXDES0_EDOTR		(1 << 15)
> ./u-boot/drivers/net/ftgmac100.h:#define FTGMAC100_RXDES0_EDORR		(1 << 15)
> 
> 
> 
> 

Re: [PATCH linux 3/4] ftgmac100: Add devicetree option for the Aspeed g5 register interface

[Joel Stanley]

Hey Cedric,

On Fri, Jun 24, 2016 at 2:39 AM, Cédric Le Goater <clg@kaod.org> wrote:
>> So, given that there always has been fixes from Aspeed on the driver in
>> u-boot (see below), it seems that these EDO bits have been wrong since
>> the beginning in Linux.
>>
>> I am wondering what how this was working. We should probably contact
>> Aspeed to have some clarification.
>
> Aspeed said that they have a different design than Faraday and we should
> be using :
>
>         #define FTGMAC100_TXDES0_EDOTR          (1 << 30)
>         #define FTGMAC100_RXDES0_EDORR          (1 << 30)
>
> for all boards : ast2400 *and* ast2500.
>
> I gave it a try on a palmetto and indeed, network works fine with the
> Aspeed EDO{T,R}R bits.

Nice investigation. Thank you for doing that.

I have sent a revised patch for the kernel that incorporates Andrew's
work with your findings.

 https://lists.ozlabs.org/pipermail/openbmc/2016-June/003876.html
 https://lists.ozlabs.org/pipermail/openbmc/2016-June/003877.html

Please review and add your tags, and I'll update the branch with the fixes.

If you want to do something similar for u-boot then please go ahead.
Otherwise I'll do it on Monday.

Cheers,

Joel

Re: [PATCH linux 3/4] ftgmac100: Add devicetree option for the Aspeed g5 register interface

[Cédric Le Goater]

On 06/24/2016 09:28 AM, Joel Stanley wrote:
> Hey Cedric,
> 
> On Fri, Jun 24, 2016 at 2:39 AM, Cédric Le Goater <clg@kaod.org> wrote:
>>> So, given that there always has been fixes from Aspeed on the driver in
>>> u-boot (see below), it seems that these EDO bits have been wrong since
>>> the beginning in Linux.
>>>
>>> I am wondering what how this was working. We should probably contact
>>> Aspeed to have some clarification.
>>
>> Aspeed said that they have a different design than Faraday and we should
>> be using :
>>
>>         #define FTGMAC100_TXDES0_EDOTR          (1 << 30)
>>         #define FTGMAC100_RXDES0_EDORR          (1 << 30)
>>
>> for all boards : ast2400 *and* ast2500.
>>
>> I gave it a try on a palmetto and indeed, network works fine with the
>> Aspeed EDO{T,R}R bits.
> 
> Nice investigation. Thank you for doing that.

Yes. Well, the Linux driver is consistent with U-Boot now.

> I have sent a revised patch for the kernel that incorporates Andrew's
> work with your findings.
> 
>  https://lists.ozlabs.org/pipermail/openbmc/2016-June/003876.html
>  https://lists.ozlabs.org/pipermail/openbmc/2016-June/003877.html
> 
> Please review and add your tags, and I'll update the branch with the fixes.

done.

> If you want to do something similar for u-boot then please go ahead.

yes. I would like first to revise the u-boot device model I could not
get working in both environment. With the kernel above, that should 
be better.

And then, U-Boot will get some aesthetic surgery, for sure :)

Thanks a lot,

C. 

> Otherwise I'll do it on Monday.
> 
> Cheers,
> 
> Joel
>