Re: [U-Boot] [PATCH 16/17] aspeed: Add AST2500/AST2400 compatible NIC Driver

[Maxim Sloyko]

On Wed, Mar 22, 2017 at 6:06 AM, Simon Glass  wrote:

> Hi Maxim,
>
> On 21 March 2017 at 17:44, Maxim Sloyko  wrote:
> > Hi Joe,
> >
> > Please see responses inline, simply ACK'ed comments will be addressed
> > in the next version.
> >
> > On Tue, Mar 21, 2017 at 12:32 PM, Joe Hershberger
> >  wrote:
> >> On Thu, Mar 16, 2017 at 4:36 PM, Maxim Sloyko 
> wrote:
> >>> The device that Aspeed uses is basically Faraday FTGMAC100, but with
> >>> some differences here and there. Since I don't have access to a
> properly
> >>> implemented FTGMAC100 though, I can't really test it and so I don't
> >>> feel comfortable claiming compatibility, even though I reused a lot of
> >>> FTGMAC100 driver code.
> >>
> >> I think it would be better to attempt to integrate this driver with
> >> the FTGMAC driver and ask others on the list who have that HW to test
> >> your changes to ensure no regressions. I prefer we have fewer drivers
> >> to maintain.
> >
> > One concern: this driver also performs its clock configuration, which
> > I believe is very specific to the SoC, so to have that compatibility
> > clock configuration needs to be externalized somehow. I don't know
> > what is the best way to do it.
>
> Generally the clock is defined by a DT property in the node, so this
> should work out OK.
>

Well, this device on this SoC needs two different clocks configured, one
for all devices and one device specific. The device speed is also hardware
strapped, so it reads the unrelated register to figure out which rate to
enable. Not to mention, it's still unclear how it's going to be done in
Linux, so somewhere else in this review Tom actually suggested to go non-DT
way with this.

Anyway, I'm going to drop this driver from this series and work this out
separately, just to keep things moving, because it looks like it raises the
largest number of concerns.


>
> Regards,
> Simon
>



-- 
*M*axim *S*loyko
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 16/17] aspeed: Add AST2500/AST2400 compatible NIC Driver

[Simon Glass]

Hi Maxim,

On 21 March 2017 at 17:44, Maxim Sloyko  wrote:
> Hi Joe,
>
> Please see responses inline, simply ACK'ed comments will be addressed
> in the next version.
>
> On Tue, Mar 21, 2017 at 12:32 PM, Joe Hershberger
>  wrote:
>> On Thu, Mar 16, 2017 at 4:36 PM, Maxim Sloyko  wrote:
>>> The device that Aspeed uses is basically Faraday FTGMAC100, but with
>>> some differences here and there. Since I don't have access to a properly
>>> implemented FTGMAC100 though, I can't really test it and so I don't
>>> feel comfortable claiming compatibility, even though I reused a lot of
>>> FTGMAC100 driver code.
>>
>> I think it would be better to attempt to integrate this driver with
>> the FTGMAC driver and ask others on the list who have that HW to test
>> your changes to ensure no regressions. I prefer we have fewer drivers
>> to maintain.
>
> One concern: this driver also performs its clock configuration, which
> I believe is very specific to the SoC, so to have that compatibility
> clock configuration needs to be externalized somehow. I don't know
> what is the best way to do it.

Generally the clock is defined by a DT property in the node, so this
should work out OK.

Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH 16/17] aspeed: Add AST2500/AST2400 compatible NIC Driver

[Maxim Sloyko]

Hi Joe,

Please see responses inline, simply ACK'ed comments will be addressed
in the next version.

On Tue, Mar 21, 2017 at 12:32 PM, Joe Hershberger
 wrote:
> On Thu, Mar 16, 2017 at 4:36 PM, Maxim Sloyko  wrote:
>> The device that Aspeed uses is basically Faraday FTGMAC100, but with
>> some differences here and there. Since I don't have access to a properly
>> implemented FTGMAC100 though, I can't really test it and so I don't
>> feel comfortable claiming compatibility, even though I reused a lot of
>> FTGMAC100 driver code.
>
> I think it would be better to attempt to integrate this driver with
> the FTGMAC driver and ask others on the list who have that HW to test
> your changes to ensure no regressions. I prefer we have fewer drivers
> to maintain.

One concern: this driver also performs its clock configuration, which
I believe is very specific to the SoC, so to have that compatibility
clock configuration needs to be externalized somehow. I don't know
what is the best way to do it.

>
> I'll review what you've got here, and presumably the comments apply to
> either your changes or the FTGMAC driver.
>
>> Signed-off-by: Maxim Sloyko 
>> ---
>>
>>  drivers/net/Kconfig   |   8 +
>>  drivers/net/Makefile  |   1 +
>>  drivers/net/ast_nic.c | 584 
>> ++
>>  drivers/net/ast_nic.h | 198 +
>>  4 files changed, 791 insertions(+)
>>  create mode 100644 drivers/net/ast_nic.c
>>  create mode 100644 drivers/net/ast_nic.h
>>
>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>> index 70e36611ea..6de8b35d9f 100644
>> --- a/drivers/net/Kconfig
>> +++ b/drivers/net/Kconfig
>> @@ -208,4 +208,12 @@ config GMAC_ROCKCHIP
>>   This driver provides Rockchip SoCs network support based on the
>>   Synopsys Designware driver.
>>
>> +config AST_NIC
>> +   bool "Support Aspeed ast2500/ast2400 NIC"
>> +   depends on DM_ETH
>> +   help
>> + This driver provides support for ast2500/ast2400 network devices.
>> + It uses Driver Model and so can support multiple devices on the 
>> same SoC.
>> + The device itself is basically a variation of Faraday FTGMAC100.
>> +
>>  endif # NETDEVICES
>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> index 2493a48b88..792bebb9cc 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -78,3 +78,4 @@ obj-$(CONFIG_FSL_MEMAC) += fm/memac_phy.o
>>  obj-$(CONFIG_VSC9953) += vsc9953.o
>>  obj-$(CONFIG_PIC32_ETH) += pic32_mdio.o pic32_eth.o
>>  obj-$(CONFIG_DWC_ETH_QOS) += dwc_eth_qos.o
>> +obj-$(CONFIG_AST_NIC) += ast_nic.o
>> diff --git a/drivers/net/ast_nic.c b/drivers/net/ast_nic.c
>> new file mode 100644
>> index 00..881d20151c
>> --- /dev/null
>> +++ b/drivers/net/ast_nic.c
>> @@ -0,0 +1,584 @@
>> +/*
>> + * (C) Copyright 2009 Faraday Technology
>> + * Po-Yu Chuang 
>> + *
>> + * (C) Copyright 2010 Andes Technology
>> + * Macpaul Lin 
>> + *
>> + * Copyright 2017 Google Inc
>> + *
>> + * SPDX-License-Identifier:GPL-2.0+
>> + */
>> +
>> +/*
>> + * This device is basically Faraday FTGMAC100, with some differences,
>> + * which do not seem to be very big, but are in very random places, like
>> + * some registers removed and completely different ones put in their place.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
>> +#include 
>> +#endif
>> +#include 
>> +#include 
>> +#include 
>> +#include "ast_nic.h"
>> +
>> +#define ETH_ZLEN   60
>> +#define RBSR_DEFAULT_VALUE 0x640
>> +
>> +#define PKTBUFSTX  4
>> +
>> +#define MAX_PHY_ADDR 32
>> +
>> +struct ast_nic_xdes {
>> +   u32 des[4];
>> +} __aligned(16);
>
> Can you use a constant for this, like ARCH_DMA_MINALIGN?

Ack

>
>> +
>> +struct ast_nic_xdes ast_txdes[PKTBUFSTX];
>> +struct ast_nic_xdes ast_rxdes[PKTBUFSRX];
>
> Any reason these are not static? Also, why globals instead of allocated?

These should be static, yes. The reason for globals is that I could
not get them properly aligned, when I put them into ast_nic_priv
structure.

>
>> +
>> +struct ast_nic_priv {
>> +   struct ast_nic_xdes *txdes;
>> +   struct ast_nic_xdes *rxdes;
>> +   struct ast_nic_regs *regs;
>> +   int tx_index;
>> +   int rx_index;
>> +   int phy_addr;
>> +};
>> +
>> +static int ast_nic_ofdata_to_platdata(struct udevice *dev)
>> +{
>> +   struct ast_nic_priv *priv = dev_get_priv(dev);
>> +   struct eth_pdata *platdata = dev_get_platdata(dev);
>> +
>> +   priv->regs = dev_get_addr_ptr(dev);
>> +   priv->txdes = ast_txdes;
>> +   priv->rxdes = ast_rxdes;
>> +   platdata->iobase = (phys_addr_t)priv->regs;
>> +
>> +   return 0;
>> +}
>> +
>> +static void ast_nic_reset(struct udevice *dev)
>> +{
>> +   struct ast_nic_priv *priv = dev_get_priv(dev);
>> +
>> +   

Re: [U-Boot] [PATCH 16/17] aspeed: Add AST2500/AST2400 compatible NIC Driver

[Joe Hershberger]

On Thu, Mar 16, 2017 at 4:36 PM, Maxim Sloyko  wrote:
> The device that Aspeed uses is basically Faraday FTGMAC100, but with
> some differences here and there. Since I don't have access to a properly
> implemented FTGMAC100 though, I can't really test it and so I don't
> feel comfortable claiming compatibility, even though I reused a lot of
> FTGMAC100 driver code.

I think it would be better to attempt to integrate this driver with
the FTGMAC driver and ask others on the list who have that HW to test
your changes to ensure no regressions. I prefer we have fewer drivers
to maintain.

I'll review what you've got here, and presumably the comments apply to
either your changes or the FTGMAC driver.

> Signed-off-by: Maxim Sloyko 
> ---
>
>  drivers/net/Kconfig   |   8 +
>  drivers/net/Makefile  |   1 +
>  drivers/net/ast_nic.c | 584 
> ++
>  drivers/net/ast_nic.h | 198 +
>  4 files changed, 791 insertions(+)
>  create mode 100644 drivers/net/ast_nic.c
>  create mode 100644 drivers/net/ast_nic.h
>
> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> index 70e36611ea..6de8b35d9f 100644
> --- a/drivers/net/Kconfig
> +++ b/drivers/net/Kconfig
> @@ -208,4 +208,12 @@ config GMAC_ROCKCHIP
>   This driver provides Rockchip SoCs network support based on the
>   Synopsys Designware driver.
>
> +config AST_NIC
> +   bool "Support Aspeed ast2500/ast2400 NIC"
> +   depends on DM_ETH
> +   help
> + This driver provides support for ast2500/ast2400 network devices.
> + It uses Driver Model and so can support multiple devices on the 
> same SoC.
> + The device itself is basically a variation of Faraday FTGMAC100.
> +
>  endif # NETDEVICES
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 2493a48b88..792bebb9cc 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -78,3 +78,4 @@ obj-$(CONFIG_FSL_MEMAC) += fm/memac_phy.o
>  obj-$(CONFIG_VSC9953) += vsc9953.o
>  obj-$(CONFIG_PIC32_ETH) += pic32_mdio.o pic32_eth.o
>  obj-$(CONFIG_DWC_ETH_QOS) += dwc_eth_qos.o
> +obj-$(CONFIG_AST_NIC) += ast_nic.o
> diff --git a/drivers/net/ast_nic.c b/drivers/net/ast_nic.c
> new file mode 100644
> index 00..881d20151c
> --- /dev/null
> +++ b/drivers/net/ast_nic.c
> @@ -0,0 +1,584 @@
> +/*
> + * (C) Copyright 2009 Faraday Technology
> + * Po-Yu Chuang 
> + *
> + * (C) Copyright 2010 Andes Technology
> + * Macpaul Lin 
> + *
> + * Copyright 2017 Google Inc
> + *
> + * SPDX-License-Identifier:GPL-2.0+
> + */
> +
> +/*
> + * This device is basically Faraday FTGMAC100, with some differences,
> + * which do not seem to be very big, but are in very random places, like
> + * some registers removed and completely different ones put in their place.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
> +#include 
> +#endif
> +#include 
> +#include 
> +#include 
> +#include "ast_nic.h"
> +
> +#define ETH_ZLEN   60
> +#define RBSR_DEFAULT_VALUE 0x640
> +
> +#define PKTBUFSTX  4
> +
> +#define MAX_PHY_ADDR 32
> +
> +struct ast_nic_xdes {
> +   u32 des[4];
> +} __aligned(16);

Can you use a constant for this, like ARCH_DMA_MINALIGN?

> +
> +struct ast_nic_xdes ast_txdes[PKTBUFSTX];
> +struct ast_nic_xdes ast_rxdes[PKTBUFSRX];

Any reason these are not static? Also, why globals instead of allocated?

> +
> +struct ast_nic_priv {
> +   struct ast_nic_xdes *txdes;
> +   struct ast_nic_xdes *rxdes;
> +   struct ast_nic_regs *regs;
> +   int tx_index;
> +   int rx_index;
> +   int phy_addr;
> +};
> +
> +static int ast_nic_ofdata_to_platdata(struct udevice *dev)
> +{
> +   struct ast_nic_priv *priv = dev_get_priv(dev);
> +   struct eth_pdata *platdata = dev_get_platdata(dev);
> +
> +   priv->regs = dev_get_addr_ptr(dev);
> +   priv->txdes = ast_txdes;
> +   priv->rxdes = ast_rxdes;
> +   platdata->iobase = (phys_addr_t)priv->regs;
> +
> +   return 0;
> +}
> +
> +static void ast_nic_reset(struct udevice *dev)
> +{
> +   struct ast_nic_priv *priv = dev_get_priv(dev);
> +
> +   setbits_le32(>regs->maccr, MAC_MACCR_SW_RST);
> +   while (readl(>regs->maccr) & MAC_MACCR_SW_RST)
> +   ;

Use  and wait_for_bit()

> +   /*
> +* Only needed for ast2400, for ast2500 this is the no-op,
> +* because the register is marked read-only.
> +*/
> +   setbits_le32(>regs->fear0, MAC_FEAR_NEW_MD_IFACE);
> +}
> +
> +static int ast_nic_phy_read(struct udevice *dev, int phy_addr,
> +   int regnum, u16 *value)
> +{
> +   struct ast_nic_priv *priv = dev_get_priv(dev);
> +   int phycr;
> +   int i;
> +
> +   phycr = MAC_PHYCR_FIRE | MAC_PHYCR_ST_22 | MAC_PHYCR_READ |
> +   (phy_addr << MAC_PHYCR_PHYAD_SHIFT) |
> +   

[U-Boot] [PATCH 16/17] aspeed: Add AST2500/AST2400 compatible NIC Driver

[Maxim Sloyko]

The device that Aspeed uses is basically Faraday FTGMAC100, but with
some differences here and there. Since I don't have access to a properly
implemented FTGMAC100 though, I can't really test it and so I don't
feel comfortable claiming compatibility, even though I reused a lot of
FTGMAC100 driver code.

Signed-off-by: Maxim Sloyko 
---

 drivers/net/Kconfig   |   8 +
 drivers/net/Makefile  |   1 +
 drivers/net/ast_nic.c | 584 ++
 drivers/net/ast_nic.h | 198 +
 4 files changed, 791 insertions(+)
 create mode 100644 drivers/net/ast_nic.c
 create mode 100644 drivers/net/ast_nic.h

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index 70e36611ea..6de8b35d9f 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -208,4 +208,12 @@ config GMAC_ROCKCHIP
  This driver provides Rockchip SoCs network support based on the
  Synopsys Designware driver.
 
+config AST_NIC
+   bool "Support Aspeed ast2500/ast2400 NIC"
+   depends on DM_ETH
+   help
+ This driver provides support for ast2500/ast2400 network devices.
+ It uses Driver Model and so can support multiple devices on the same 
SoC.
+ The device itself is basically a variation of Faraday FTGMAC100.
+
 endif # NETDEVICES
diff --git a/drivers/net/Makefile b/drivers/net/Makefile
index 2493a48b88..792bebb9cc 100644
--- a/drivers/net/Makefile
+++ b/drivers/net/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_FSL_MEMAC) += fm/memac_phy.o
 obj-$(CONFIG_VSC9953) += vsc9953.o
 obj-$(CONFIG_PIC32_ETH) += pic32_mdio.o pic32_eth.o
 obj-$(CONFIG_DWC_ETH_QOS) += dwc_eth_qos.o
+obj-$(CONFIG_AST_NIC) += ast_nic.o
diff --git a/drivers/net/ast_nic.c b/drivers/net/ast_nic.c
new file mode 100644
index 00..881d20151c
--- /dev/null
+++ b/drivers/net/ast_nic.c
@@ -0,0 +1,584 @@
+/*
+ * (C) Copyright 2009 Faraday Technology
+ * Po-Yu Chuang 
+ *
+ * (C) Copyright 2010 Andes Technology
+ * Macpaul Lin 
+ *
+ * Copyright 2017 Google Inc
+ *
+ * SPDX-License-Identifier:GPL-2.0+
+ */
+
+/*
+ * This device is basically Faraday FTGMAC100, with some differences,
+ * which do not seem to be very big, but are in very random places, like
+ * some registers removed and completely different ones put in their place.
+ */
+
+#include 
+#include 
+#include 
+#if defined(CONFIG_MII) || defined(CONFIG_CMD_MII)
+#include 
+#endif
+#include 
+#include 
+#include 
+#include "ast_nic.h"
+
+#define ETH_ZLEN   60
+#define RBSR_DEFAULT_VALUE 0x640
+
+#define PKTBUFSTX  4
+
+#define MAX_PHY_ADDR 32
+
+struct ast_nic_xdes {
+   u32 des[4];
+} __aligned(16);
+
+struct ast_nic_xdes ast_txdes[PKTBUFSTX];
+struct ast_nic_xdes ast_rxdes[PKTBUFSRX];
+
+struct ast_nic_priv {
+   struct ast_nic_xdes *txdes;
+   struct ast_nic_xdes *rxdes;
+   struct ast_nic_regs *regs;
+   int tx_index;
+   int rx_index;
+   int phy_addr;
+};
+
+static int ast_nic_ofdata_to_platdata(struct udevice *dev)
+{
+   struct ast_nic_priv *priv = dev_get_priv(dev);
+   struct eth_pdata *platdata = dev_get_platdata(dev);
+
+   priv->regs = dev_get_addr_ptr(dev);
+   priv->txdes = ast_txdes;
+   priv->rxdes = ast_rxdes;
+   platdata->iobase = (phys_addr_t)priv->regs;
+
+   return 0;
+}
+
+static void ast_nic_reset(struct udevice *dev)
+{
+   struct ast_nic_priv *priv = dev_get_priv(dev);
+
+   setbits_le32(>regs->maccr, MAC_MACCR_SW_RST);
+   while (readl(>regs->maccr) & MAC_MACCR_SW_RST)
+   ;
+   /*
+* Only needed for ast2400, for ast2500 this is the no-op,
+* because the register is marked read-only.
+*/
+   setbits_le32(>regs->fear0, MAC_FEAR_NEW_MD_IFACE);
+}
+
+static int ast_nic_phy_read(struct udevice *dev, int phy_addr,
+   int regnum, u16 *value)
+{
+   struct ast_nic_priv *priv = dev_get_priv(dev);
+   int phycr;
+   int i;
+
+   phycr = MAC_PHYCR_FIRE | MAC_PHYCR_ST_22 | MAC_PHYCR_READ |
+   (phy_addr << MAC_PHYCR_PHYAD_SHIFT) |
+   (regnum << MAC_PHYCR_REGAD_SHIFT);
+
+   writel(phycr, >regs->phycr);
+
+   for (i = 0; i < 10; i++) {
+   phycr = readl(>regs->phycr);
+
+   if ((phycr & MAC_PHYCR_FIRE) == 0) {
+   int data;
+
+   data = readl(>regs->phydata);
+   *value = (data & MAC_PHYDATA_MIIRDATA_MASK) >>
+   MAC_PHYDATA_MIIRDATA_SHIFT;
+
+   return 0;
+   }
+
+   mdelay(10);
+   }
+
+   debug("mdio read timed out\n");
+   return -ETIMEDOUT;
+}
+
+static int ast_nic_phy_write(struct udevice *dev, int phy_addr,
+   int regnum, u16 value)
+{
+   struct ast_nic_priv *priv = dev_get_priv(dev);
+   int phycr;
+   int i;
+
+   phycr = (value << MAC_PHYDATA_MIIWDATA_SHIFT) |
+