[U-Boot,v2,1/2] TI: netdev: add driver for cpsw ethernet device

From: Cyril Chemparathy <cyril@ti.com>

CPSW is an on-chip ethernet switch that is found on various SoCs from Texas
Instruments.  This patch adds a simple driver (based on the Linux driver) for
this hardware module.

Signed-off-by: Cyril Chemparathy <cyril@ti.com>
---
Changes since v1:
 - Code cleanup and removal of cpsw_eth_set_mac_addr function

 drivers/net/Makefile |    1 +
 drivers/net/cpsw.c   |  862 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/netdev.h     |   29 ++
 3 files changed, 892 insertions(+), 0 deletions(-)
 create mode 100644 drivers/net/cpsw.c

On Thursday 10 November 2011 07:40:28 Chandan Nath wrote:
> +#define mac_hi(mac)    (((mac)[0] << 0) | ((mac)[1] << 8) |    \
> +                        ((mac)[2] << 16) | ((mac)[3] << 24))

looks like __get_unaligned_u32()

> +#define mac_lo(mac)    (((mac)[4] << 0) | ((mac)[5] << 8))

looks like __get_unaligned_u16()
-mike

On Thu, Nov 10, 2011 at 9:26 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On Thursday 10 November 2011 07:40:28 Chandan Nath wrote:
>> +#define mac_hi(mac)    (((mac)[0] << 0) | ((mac)[1] << 8) |    \
>> +                        ((mac)[2] << 16) | ((mac)[3] << 24))
>
> looks like __get_unaligned_u32()
>
>> +#define mac_lo(mac)    (((mac)[4] << 0) | ((mac)[5] << 8))
>
> looks like __get_unaligned_u16()

It's the regular MAC shifting around everyone does.  Are you saying
there's a general cleanup to be done here?

On Thursday 10 November 2011 11:39:49 Tom Rini wrote:
> On Thu, Nov 10, 2011 at 9:26 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> > On Thursday 10 November 2011 07:40:28 Chandan Nath wrote:
> >> +#define mac_hi(mac)    (((mac)[0] << 0) | ((mac)[1] << 8) |    \
> >> +                        ((mac)[2] << 16) | ((mac)[3] << 24))
> > 
> > looks like __get_unaligned_u32()
> > 
> >> +#define mac_lo(mac)    (((mac)[4] << 0) | ((mac)[5] << 8))
> > 
> > looks like __get_unaligned_u16()
> 
> It's the regular MAC shifting around everyone does.  Are you saying
> there's a general cleanup to be done here?

other drivers probably could benefit from using the unaligned helpers.  but it 
isn't something we could generally unify as it requires assumption on 
core/system endian, and the layout of the registers in the peripheral.
-mike

On Thu, Nov 10, 2011 at 6:40 AM, Chandan Nath <chandan.nath@ti.com> wrote:
> From: Cyril Chemparathy <cyril@ti.com>
>
> CPSW is an on-chip ethernet switch that is found on various SoCs from Texas
> Instruments.  This patch adds a simple driver (based on the Linux driver) for
> this hardware module.


Which Linux driver is this based on?


>
> Signed-off-by: Cyril Chemparathy <cyril@ti.com>
> ---
> Changes since v1:
>  - Code cleanup and removal of cpsw_eth_set_mac_addr function
>
>  drivers/net/Makefile |    1 +
>  drivers/net/cpsw.c   |  862 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/netdev.h     |   29 ++
>  3 files changed, 892 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/net/cpsw.c
>
> +
> +#define BIT(x)                 (1 << (x))
> +#define BITMASK(bits)          (BIT(bits) - 1)
> +#define PHY_REG_MASK           0x1f
> +#define PHY_ID_MASK            0x1f

These 4 defines seem very generic


> +struct cpsw_priv {
> +       struct eth_device               *dev;
> +       struct cpsw_platform_data       data;
> +       int                             host_port;
> +
> +       struct cpsw_regs                *regs;
> +       void                            *dma_regs;
> +       struct cpsw_host_regs           *host_port_regs;
> +       void                            *ale_regs;
> +
> +       struct cpdma_desc               descs[NUM_DESCS];
> +       struct cpdma_desc               *desc_free;
> +       struct cpdma_chan               rx_chan, tx_chan;
> +
> +       struct cpsw_slave               *slaves;
> +#define for_each_slave(priv, func, arg...)                     \
> +       do {                                                    \
> +               int idx;                                        \
> +               for (idx = 0; idx < (priv)->data.slaves; idx++) \
> +                       (func)((priv)->slaves + idx, ##arg);    \
> +       } while (0)
> +};


It seems a bit awkward to me to put a complex macro like this inside
the structure definition. After further review, I think it should also
be defined differently, more like the list macros:

#define for_each_slave(slave, priv) \
        for (slave = (priv)->slaves; slave != (priv)->slaves +
(priv)->data.slaves; slave++)

It makes the code more maintainable, as the macro no longer contains
the implicit assumption that "func" has a slave pointer as its first
argument.


> +
> +static inline int cpsw_ale_get_field(u32 *ale_entry, u32 start, u32 bits)
> +{
> +       int idx;
> +
> +       idx    = start / 32;
> +       start -= idx * 32;
> +       idx    = 2 - idx; /* flip */
> +       return (ale_entry[idx] >> start) & BITMASK(bits);
> +}
> +
> +static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32 bits,
> +                                     u32 value)
> +{
> +       int idx;
> +
> +       value &= BITMASK(bits);
> +       idx    = start / 32;
> +       start -= idx * 32;
> +       idx    = 2 - idx; /* flip */
> +       ale_entry[idx] &= ~(BITMASK(bits) << start);
> +       ale_entry[idx] |=  (value << start);
> +}


These two functions seem both very generic, and also over-worked.


> +
> +#define DEFINE_ALE_FIELD(name, start, bits)                            \
> +static inline int cpsw_ale_get_##name(u32 *ale_entry)                  \
> +{                                                                      \
> +       return cpsw_ale_get_field(ale_entry, start, bits);              \
> +}                                                                      \
> +static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value)      \
> +{                                                                      \
> +       cpsw_ale_set_field(ale_entry, start, bits, value);              \
> +}
> +
> +DEFINE_ALE_FIELD(entry_type,           60,     2)
> +DEFINE_ALE_FIELD(mcast_state,          62,     2)
> +DEFINE_ALE_FIELD(port_mask,            64,     3)
> +DEFINE_ALE_FIELD(ucast_type,           66,     2)
> +DEFINE_ALE_FIELD(port_num,             64,     2)
> +DEFINE_ALE_FIELD(blocked,              63,     1)
> +DEFINE_ALE_FIELD(secure,               62,     1)
> +DEFINE_ALE_FIELD(mcast,                        47,     1)
> +
> +/* The MAC address field in the ALE entry cannot be macroized as above */
> +static inline void cpsw_ale_get_addr(u32 *ale_entry, u8 *addr)
> +{
> +       int i;
> +
> +       for (i = 0; i < 6; i++)
> +               addr[i] = cpsw_ale_get_field(ale_entry, 40 - 8*i, 8);
> +}
> +
> +static inline void cpsw_ale_set_addr(u32 *ale_entry, u8 *addr)
> +{
> +       int i;
> +
> +       for (i = 0; i < 6; i++)
> +               cpsw_ale_set_field(ale_entry, 40 - 8*i, 8, addr[i]);
> +}
> +
> +static int cpsw_ale_read(struct cpsw_priv *priv, int idx, u32 *ale_entry)
> +{
> +       int i;
> +
> +       __raw_writel(idx, priv->ale_regs + ALE_TABLE_CONTROL);
> +
> +       for (i = 0; i < ALE_ENTRY_WORDS; i++)
> +               ale_entry[i] = __raw_readl(priv->ale_regs + ALE_TABLE + 4 * i);
> +
> +       return idx;
> +}
> +
> +static int cpsw_ale_write(struct cpsw_priv *priv, int idx, u32 *ale_entry)
> +{
> +       int i;
> +
> +       for (i = 0; i < ALE_ENTRY_WORDS; i++)
> +               __raw_writel(ale_entry[i], priv->ale_regs + ALE_TABLE + 4 * i);
> +
> +       __raw_writel(idx | ALE_TABLE_WRITE, priv->ale_regs + ALE_TABLE_CONTROL);
> +
> +       return idx;
> +}


I don't know how your device works, but it seems odd to me to have to
read out a large table, and modify it, and then write it back out.

I'm also highly suspicious of the overworked bitfield math. Are
standard C bitfields not usable for some reason? It borders on code
obfuscation, IMO. I've spent far too much time, already, just trying
to figure out what that code was doing, and why.


> +static inline void cpsw_ale_port_state(struct cpsw_priv *priv, int port,
> +                                      int val)
> +{
> +       int offset = ALE_PORTCTL + 4 * port;
> +       u32 tmp, mask = 0x3;
> +
> +       tmp  = __raw_readl(priv->ale_regs + offset);
> +       tmp &= ~mask;
> +       tmp |= val & 0x3;

All of this can probably be done with more generic setbits functions,
but at least use "mask" consistently for both the clearing and the
masking of "val".

> +       __raw_writel(tmp, priv->ale_regs + offset);
> +}
> +
> +static struct cpsw_mdio_regs *mdio_regs;


Global variables like this shouldn't be necessary. Especially with the
new PHY API



> +
> +static int cpsw_mdio_read(char *devname, unsigned char phy_id,
> +                         unsigned char phy_reg, unsigned short *data)
> +{

This is the legacy interface for PHY interactions. You should look
into using the new "PHYLIB" code.


> +       u32 reg;
> +
> +       if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK)
> +               return -EINVAL;
> +
> +       wait_for_user_access();
> +       reg = (USERACCESS_GO | USERACCESS_READ | (phy_reg << 21) |
> +              (phy_id << 16));
> +       __raw_writel(reg, &mdio_regs->user[0].access);
> +       reg = wait_for_user_access();
> +
> +       *data = (reg & USERACCESS_ACK) ? (reg & USERACCESS_DATA) : -1;
> +       return (reg & USERACCESS_ACK) ? 0 : -EIO;
> +}
> +
> +static int cpsw_mdio_write(char *devname, unsigned char phy_id,
> +                          unsigned char phy_reg, unsigned short data)

Same comment as above.


> +static void cpsw_mdio_init(char *name, u32 mdio_base, u32 div)
> +{
> +       mdio_regs = (struct cpsw_mdio_regs *)mdio_base;
> +
> +       /* set enable and clock divider */
> +       __raw_writel(div | CONTROL_ENABLE, &mdio_regs->control);
> +
> +       /*
> +        * wait for scan logic to settle:
> +        * the scan time consists of (a) a large fixed component, and (b) a
> +        * small component that varies with the mii bus frequency.  These
> +        * were estimated using measurements at 1.1 and 2.2 MHz on tnetv107x
> +        * silicon.  Since the effect of (b) was found to be largely
> +        * negligible, we keep things simple here.
> +        */
> +       udelay(1000);

This seems sketchy. Is there no way to *know* the bus is ready, other
than to wait?

> +
> +       miiphy_register(name, cpsw_mdio_read, cpsw_mdio_write);


This is the legacy API.


> +}
> +
> +static inline void soft_reset(void *reg)
> +{
> +       __raw_writel(1, reg);
> +       while (__raw_readl(reg) & 1)
> +               ;
> +}


???

Can reg be *any* reg? Use a better name. Also, name "1" something sensible.

Perhaps a cleaner implementation (which might be usable in other
people's code) would be something like:

/* Set a self-clearing bit in a register, and wait for it to clear */
static inline void setbit_and_wait_for_clear32(void *addr, u32 mask)
{
   __raw_writel(val, addr);
  while (__raw_readl(addr) & val)
    ;
}


> +static void cpsw_slave_update_link(struct cpsw_slave *slave,
> +                                  struct cpsw_priv *priv, int *link)
> +{
> +       char *name = priv->dev->name;
> +       int phy_id = slave->data->phy_id;
> +       int speed, duplex;
> +       unsigned short reg;
> +       u32 mac_control = 0;
> +
> +       if (miiphy_read(name, phy_id, MII_BMSR, &reg))
> +               return; /* could not read, assume no link */


Please use phy_read(), with the new API.


> +
> +       if (reg & BMSR_LSTATUS) { /* link up */
> +               speed = miiphy_speed(name, phy_id);
> +               duplex = miiphy_duplex(name, phy_id);
> +
> +               *link = 1;
> +               mac_control = priv->data.mac_control;
> +               if (speed == 1000)
> +                       mac_control |= BIT(7);  /* GIGABITEN    */
> +               if (duplex == FULL)
> +                       mac_control |= BIT(0);  /* FULLDUPLEXEN */

Why not:
    mac_control |= GIGABITEN;

etc



> +static int cpsw_update_link(struct cpsw_priv *priv)
> +{
> +       int link = 0;
> +       for_each_slave(priv, cpsw_slave_update_link, priv, &link);

As I mentioned, there are a lot of similarly-named macros in Linux and
U-boot. Usually they act exactly like for-loops, where you provide an
iterator, and then use it in each iteration:

struct cpsw_slave *slave;

for_each_slave(slave, priv)
  cpsw_slave_update_link(slave, priv, &link);

I think this is much easier to understand and maintain.

> +       return link;

Also, link looks useless. It will always be the value in the last
slave. Is that what you meant to do?


> +}
> +
> +static void cpsw_slave_init(struct cpsw_slave *slave, struct cpsw_priv *priv)
> +{

[...]

> +
> +       priv->data.phy_init(priv->dev->name, slave->data->phy_id);

What does this do? Probably needs to be converted to the newer API.


> +}
> +
> +static struct cpdma_desc *cpdma_desc_alloc(struct cpsw_priv *priv)
> +{
> +       struct cpdma_desc *desc = priv->desc_free;
> +
> +       if (desc)
> +               priv->desc_free = desc_read_ptr(desc, hw_next);
> +       return desc;
> +}
> +
> +static void cpdma_desc_free(struct cpsw_priv *priv, struct cpdma_desc *desc)
> +{
> +       if (desc) {
> +               desc_write(desc, hw_next, priv->desc_free);
> +               priv->desc_free = desc;
> +       }
> +}
> +
> +static int cpdma_submit(struct cpsw_priv *priv, struct cpdma_chan *chan,
> +                       volatile void *buffer, int len)
> +{
> +       struct cpdma_desc *desc, *prev;
> +       u32 mode;
> +
> +       desc = cpdma_desc_alloc(priv);
> +       if (!desc)
> +               return -ENOMEM;
> +
> +       if (len < PKT_MIN)
> +               len = PKT_MIN;
> +
> +       mode = CPDMA_DESC_OWNER | CPDMA_DESC_SOP | CPDMA_DESC_EOP;
> +
> +       desc_write(desc, hw_next,   0);
> +       desc_write(desc, hw_buffer, buffer);
> +       desc_write(desc, hw_len,    len);
> +       desc_write(desc, hw_mode,   mode | len);
> +       desc_write(desc, sw_buffer, buffer);
> +       desc_write(desc, sw_len,    len);
> +
> +       if (!chan->head) {
> +               /* simple case - first packet enqueued */
> +               chan->head = desc;
> +               chan->tail = desc;
> +               chan_write(chan, hdp, desc);
> +               goto done;
> +       }
> +
> +       /* not the first packet - enqueue at the tail */
> +       prev = chan->tail;
> +       desc_write(prev, hw_next, desc);
> +       chan->tail = desc;
> +
> +       /* next check if EOQ has been triggered already */
> +       if (desc_read(prev, hw_mode) & CPDMA_DESC_EOQ)
> +               chan_write(chan, hdp, desc);
> +
> +done:
> +       if (chan->rxfree)
> +               chan_write(chan, rxfree, 1);
> +       return 0;
> +}
> +
> +static int cpdma_process(struct cpsw_priv *priv, struct cpdma_chan *chan,
> +                        void **buffer, int *len)
> +{
> +       struct cpdma_desc *desc = chan->head;
> +       u32 status;
> +
> +       if (!desc)
> +               return -ENOENT;
> +
> +       status  = desc_read(desc, hw_mode);
> +
> +       if (len)
> +               *len = status & 0x7ff;
> +
> +       if (buffer)
> +               *buffer = desc_read_ptr(desc, sw_buffer);
> +
> +       if (status & CPDMA_DESC_OWNER)
> +               return -EBUSY;
> +
> +       chan->head = desc_read_ptr(desc, hw_next);
> +       chan_write(chan, cp, desc);
> +
> +       cpdma_desc_free(priv, desc);
> +       return 0;
> +}
> +
> +static int cpsw_init(struct eth_device *dev, bd_t *bis)
> +{
> +       struct cpsw_priv        *priv = dev->priv;
> +       int i, ret;
> +
> +       priv->data.control(1);

1 what? The "control" function probably needs a better name. And maybe
so does "1".

> +
> +       /* soft reset the controller and initialize priv */
> +       soft_reset(&priv->regs->soft_reset);
> +

See comment about soft_reset, above.

> +
> +       cpsw_ale_port_state(priv, priv->host_port, ALE_PORT_STATE_FORWARD);
> +
> +       cpsw_ale_add_ucast(priv, priv->dev->enetaddr, priv->host_port,
> +                          ALE_SECURE);
> +       cpsw_ale_add_mcast(priv, NetBcastAddr, 1 << priv->host_port);
> +
> +       for_each_slave(priv, cpsw_slave_init, priv);

Same comment as before about the for-loop construct.

[...]

> +       eth_register(dev);
> +
> +       cpsw_mdio_init(dev->name, data->mdio_base, data->mdio_div);

I bet you only need one MDIO bus, and the new PHY API makes that
straightforward.

> +
> +       return 1;
> +}
> diff --git a/include/netdev.h b/include/netdev.h
> index 96c7b9b..740213e 100644
> --- a/include/netdev.h
> +++ b/include/netdev.h
> @@ -185,4 +185,33 @@ struct mv88e61xx_config {
>  int mv88e61xx_switch_initialize(struct mv88e61xx_config *swconfig);
>  #endif /* CONFIG_MV88E61XX_SWITCH */
>
> +#ifdef CONFIG_DRIVER_TI_CPSW
> +
> +struct cpsw_slave_data {
> +       u32             slave_reg_ofs;
> +       u32             sliver_reg_ofs;
> +       int             phy_id;
> +};
> +
> +struct cpsw_platform_data {
> +       u32     mdio_base;
> +       u32     cpsw_base;
> +       int     mdio_div;
> +       int     channels;       /* number of cpdma channels (symmetric) */
> +       u32     cpdma_reg_ofs;  /* cpdma register offset                */
> +       int     slaves;         /* number of slave cpgmac ports         */
> +       u32     ale_reg_ofs;    /* address lookup engine reg offset     */
> +       int     ale_entries;    /* ale table size                       */
> +       u32     host_port_reg_ofs;      /* cpdma host port registers    */
> +       u32     hw_stats_reg_ofs;       /* cpsw hw stats counters       */
> +       u32     mac_control;
> +       struct cpsw_slave_data  *slave_data;
> +       void    (*control)(int enabled);
> +       void    (*phy_init)(char *name, int addr);
> +};
> +
> +int cpsw_register(struct cpsw_platform_data *data);
> +
> +#endif /* CONFIG_DRIVER_TI_CPSW */

Hmmm... nothing here seems like it should be in a generic header like
"netdev.h". Only cpsw_register(), and probably not that, either. I see
there's some Marvell stuff in there, too, which is suspect, but let's
not propagate that issue. There's a cpu_eth_init() and a
board_eth_init(). Surely one of those can call cpsw_register(), and
then you can put the cpsw-specific structures in something like
include/cpsw.h.

Andy

On Thu, Nov 10, 2011 at 3:43 PM, Andy Fleming <afleming@gmail.com> wrote:
> On Thu, Nov 10, 2011 at 6:40 AM, Chandan Nath <chandan.nath@ti.com> wrote:
>> From: Cyril Chemparathy <cyril@ti.com>
>>
>> CPSW is an on-chip ethernet switch that is found on various SoCs from Texas
>> Instruments.  This patch adds a simple driver (based on the Linux driver) for
>> this hardware module.
>
> Which Linux driver is this based on?

drivers/net/cpsw.c, which isn't in mainline (and I didn't go running
about other maintainer trees).  There's a copy over in
http://arago-project.org/git/projects/?p=linux-am33x.git

Andy,

> -----Original Message-----
> From: Andy Fleming [mailto:afleming@gmail.com]
> Sent: Friday, November 11, 2011 4:14 AM
> To: Kumar Nath, Chandan
> Cc: u-boot@lists.denx.de; Chemparathy, Cyril
> Subject: Re: [U-Boot] [PATCH v2 1/2] TI: netdev: add driver for cpsw
> ethernet device
> 
> On Thu, Nov 10, 2011 at 6:40 AM, Chandan Nath <chandan.nath@ti.com>
> wrote:
> > From: Cyril Chemparathy <cyril@ti.com>
> >
> > CPSW is an on-chip ethernet switch that is found on various SoCs from
> Texas
> > Instruments.  This patch adds a simple driver (based on the Linux
> driver) for
> > this hardware module.
> 
> 
> Which Linux driver is this based on?

This is based on linux CPSW driver, which is yet to be up streamed.

> 
> 
> >
> > Signed-off-by: Cyril Chemparathy <cyril@ti.com>
> > ---
> > Changes since v1:
> >  - Code cleanup and removal of cpsw_eth_set_mac_addr function
> >
> >  drivers/net/Makefile |    1 +
> >  drivers/net/cpsw.c   |  862
> ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/netdev.h     |   29 ++
> >  3 files changed, 892 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/net/cpsw.c
> >
> > +
> > +#define BIT(x)                 (1 << (x))
> > +#define BITMASK(bits)          (BIT(bits) - 1)
> > +#define PHY_REG_MASK           0x1f
> > +#define PHY_ID_MASK            0x1f
> 
> These 4 defines seem very generic

I did not find any generic code for these definitions in arch/arm
and so keeping these definitions here.

> 
> 
> > +struct cpsw_priv {
> > +       struct eth_device               *dev;
> > +       struct cpsw_platform_data       data;
> > +       int                             host_port;
> > +
> > +       struct cpsw_regs                *regs;
> > +       void                            *dma_regs;
> > +       struct cpsw_host_regs           *host_port_regs;
> > +       void                            *ale_regs;
> > +
> > +       struct cpdma_desc               descs[NUM_DESCS];
> > +       struct cpdma_desc               *desc_free;
> > +       struct cpdma_chan               rx_chan, tx_chan;
> > +
> > +       struct cpsw_slave               *slaves;
> > +#define for_each_slave(priv, func, arg...)                     \
> > +       do {                                                    \
> > +               int idx;                                        \
> > +               for (idx = 0; idx < (priv)->data.slaves; idx++) \
> > +                       (func)((priv)->slaves + idx, ##arg);    \
> > +       } while (0)
> > +};
> 
> 
> It seems a bit awkward to me to put a complex macro like this inside
> the structure definition. After further review, I think it should also
> be defined differently, more like the list macros:
> 
> #define for_each_slave(slave, priv) \
>         for (slave = (priv)->slaves; slave != (priv)->slaves +
> (priv)->data.slaves; slave++)
> 
> It makes the code more maintainable, as the macro no longer contains
> the implicit assumption that "func" has a slave pointer as its first
> argument.
> 

This complex macro will be separated out and will be defined like list macros as you have
mentioned above.

> 
> > +
> > +static inline int cpsw_ale_get_field(u32 *ale_entry, u32 start, u32
> bits)
> > +{
> > +       int idx;
> > +
> > +       idx    = start / 32;
> > +       start -= idx * 32;
> > +       idx    = 2 - idx; /* flip */
> > +       return (ale_entry[idx] >> start) & BITMASK(bits);
> > +}
> > +
> > +static inline void cpsw_ale_set_field(u32 *ale_entry, u32 start, u32
> bits,
> > +                                     u32 value)
> > +{
> > +       int idx;
> > +
> > +       value &= BITMASK(bits);
> > +       idx    = start / 32;
> > +       start -= idx * 32;
> > +       idx    = 2 - idx; /* flip */
> > +       ale_entry[idx] &= ~(BITMASK(bits) << start);
> > +       ale_entry[idx] |=  (value << start);
> > +}
> 
> 
> These two functions seem both very generic, and also over-worked.
> 

Should this be changed to a different implementation?

> 
> > +
> > +#define DEFINE_ALE_FIELD(name, start, bits)
>    \
> > +static inline int cpsw_ale_get_##name(u32 *ale_entry)
>    \
> > +{
>    \
> > +       return cpsw_ale_get_field(ale_entry, start, bits);
>    \
> > +}
>    \
> > +static inline void cpsw_ale_set_##name(u32 *ale_entry, u32 value)
>    \
> > +{
>    \
> > +       cpsw_ale_set_field(ale_entry, start, bits, value);
>    \
> > +}
> > +
> > +DEFINE_ALE_FIELD(entry_type,           60,     2)
> > +DEFINE_ALE_FIELD(mcast_state,          62,     2)
> > +DEFINE_ALE_FIELD(port_mask,            64,     3)
> > +DEFINE_ALE_FIELD(ucast_type,           66,     2)
> > +DEFINE_ALE_FIELD(port_num,             64,     2)
> > +DEFINE_ALE_FIELD(blocked,              63,     1)
> > +DEFINE_ALE_FIELD(secure,               62,     1)
> > +DEFINE_ALE_FIELD(mcast,                        47,     1)
> > +
> > +/* The MAC address field in the ALE entry cannot be macroized as
> above */
> > +static inline void cpsw_ale_get_addr(u32 *ale_entry, u8 *addr)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < 6; i++)
> > +               addr[i] = cpsw_ale_get_field(ale_entry, 40 - 8*i, 8);
> > +}
> > +
> > +static inline void cpsw_ale_set_addr(u32 *ale_entry, u8 *addr)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < 6; i++)
> > +               cpsw_ale_set_field(ale_entry, 40 - 8*i, 8, addr[i]);
> > +}
> > +
> > +static int cpsw_ale_read(struct cpsw_priv *priv, int idx, u32
> *ale_entry)
> > +{
> > +       int i;
> > +
> > +       __raw_writel(idx, priv->ale_regs + ALE_TABLE_CONTROL);
> > +
> > +       for (i = 0; i < ALE_ENTRY_WORDS; i++)
> > +               ale_entry[i] = __raw_readl(priv->ale_regs + ALE_TABLE
> + 4 * i);
> > +
> > +       return idx;
> > +}
> > +
> > +static int cpsw_ale_write(struct cpsw_priv *priv, int idx, u32
> *ale_entry)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < ALE_ENTRY_WORDS; i++)
> > +               __raw_writel(ale_entry[i], priv->ale_regs + ALE_TABLE
> + 4 * i);
> > +
> > +       __raw_writel(idx | ALE_TABLE_WRITE, priv->ale_regs +
> ALE_TABLE_CONTROL);
> > +
> > +       return idx;
> > +}
> 
> 
> I don't know how your device works, but it seems odd to me to have to
> read out a large table, and modify it, and then write it back out.
> 
> I'm also highly suspicious of the overworked bitfield math. Are
> standard C bitfields not usable for some reason? It borders on code
> obfuscation, IMO. I've spent far too much time, already, just trying
> to figure out what that code was doing, and why.
> 

I will think of some standard C bitfields to make it simpler.

> > +static inline void cpsw_ale_port_state(struct cpsw_priv *priv, int
> port,
> > +                                      int val)
> > +{
> > +       int offset = ALE_PORTCTL + 4 * port;
> > +       u32 tmp, mask = 0x3;
> > +
> > +       tmp  = __raw_readl(priv->ale_regs + offset);
> > +       tmp &= ~mask;
> > +       tmp |= val & 0x3;
> 
> All of this can probably be done with more generic setbits functions,
> but at least use "mask" consistently for both the clearing and the
> masking of "val".
> 

Ok, mask will be used consistently.

> > +       __raw_writel(tmp, priv->ale_regs + offset);
> > +}
> > +
> > +static struct cpsw_mdio_regs *mdio_regs;
> 
> 
> Global variables like this shouldn't be necessary. Especially with the
> new PHY API
>

This global variable is required to access mdio registers across different functions.
 
> 	
> 
> > +
> > +static int cpsw_mdio_read(char *devname, unsigned char phy_id,
> > +                         unsigned char phy_reg, unsigned short
> *data)
> > +{
> 
> This is the legacy interface for PHY interactions. You should look
> into using the new "PHYLIB" code.
> 

Sure, I will be using new "PHYLIB" code instead of this legacy code.

> 
> > +       u32 reg;
> > +
> > +       if (phy_reg & ~PHY_REG_MASK || phy_id & ~PHY_ID_MASK)
> > +               return -EINVAL;
> > +
> > +       wait_for_user_access();
> > +       reg = (USERACCESS_GO | USERACCESS_READ | (phy_reg << 21) |
> > +              (phy_id << 16));
> > +       __raw_writel(reg, &mdio_regs->user[0].access);
> > +       reg = wait_for_user_access();
> > +
> > +       *data = (reg & USERACCESS_ACK) ? (reg & USERACCESS_DATA) : -
> 1;
> > +       return (reg & USERACCESS_ACK) ? 0 : -EIO;
> > +}
> > +
> > +static int cpsw_mdio_write(char *devname, unsigned char phy_id,
> > +                          unsigned char phy_reg, unsigned short
> data)
> 
> Same comment as above.
> 

Yes, I will use "PHYLIB" code instead of legacy code.

> 
> > +static void cpsw_mdio_init(char *name, u32 mdio_base, u32 div)
> > +{
> > +       mdio_regs = (struct cpsw_mdio_regs *)mdio_base;
> > +
> > +       /* set enable and clock divider */
> > +       __raw_writel(div | CONTROL_ENABLE, &mdio_regs->control);
> > +
> > +       /*
> > +        * wait for scan logic to settle:
> > +        * the scan time consists of (a) a large fixed component, and
> (b) a
> > +        * small component that varies with the mii bus frequency.
>  These
> > +        * were estimated using measurements at 1.1 and 2.2 MHz on
> tnetv107x
> > +        * silicon.  Since the effect of (b) was found to be largely
> > +        * negligible, we keep things simple here.
> > +        */
> > +       udelay(1000);
> 
> This seems sketchy. Is there no way to *know* the bus is ready, other
> than to wait?
> 
> > +
> > +       miiphy_register(name, cpsw_mdio_read, cpsw_mdio_write);
> 
> 
> This is the legacy API.
> 

PHYLIB code will be used instead of this.

> 
> > +}
> > +
> > +static inline void soft_reset(void *reg)
> > +{
> > +       __raw_writel(1, reg);
> > +       while (__raw_readl(reg) & 1)
> > +               ;
> > +}
> 
> 
> ???
> 
> Can reg be *any* reg? Use a better name. Also, name "1" something
> sensible.
> 
> Perhaps a cleaner implementation (which might be usable in other
> people's code) would be something like:
> 
> /* Set a self-clearing bit in a register, and wait for it to clear */
> static inline void setbit_and_wait_for_clear32(void *addr, u32 mask)
> {
>    __raw_writel(val, addr);
>   while (__raw_readl(addr) & val)
>     ;
> }
> 
> 

Ok, I will implement this in a cleaner manner.

> > +static void cpsw_slave_update_link(struct cpsw_slave *slave,
> > +                                  struct cpsw_priv *priv, int *link)
> > +{
> > +       char *name = priv->dev->name;
> > +       int phy_id = slave->data->phy_id;
> > +       int speed, duplex;
> > +       unsigned short reg;
> > +       u32 mac_control = 0;
> > +
> > +       if (miiphy_read(name, phy_id, MII_BMSR, &reg))
> > +               return; /* could not read, assume no link */
> 
> 
> Please use phy_read(), with the new API.
>

Ok, I will use phy_read(), instead of miiphy_read.
 
> 
> > +
> > +       if (reg & BMSR_LSTATUS) { /* link up */
> > +               speed = miiphy_speed(name, phy_id);
> > +               duplex = miiphy_duplex(name, phy_id);
> > +
> > +               *link = 1;
> > +               mac_control = priv->data.mac_control;
> > +               if (speed == 1000)
> > +                       mac_control |= BIT(7);  /* GIGABITEN    */
> > +               if (duplex == FULL)
> > +                       mac_control |= BIT(0);  /* FULLDUPLEXEN */
> 
> Why not:
>     mac_control |= GIGABITEN;
> 
> etc
> 

Sure, I will make this change.

> 
> 
> > +static int cpsw_update_link(struct cpsw_priv *priv)
> > +{
> > +       int link = 0;
> > +       for_each_slave(priv, cpsw_slave_update_link, priv, &link);
> 
> As I mentioned, there are a lot of similarly-named macros in Linux and
> U-boot. Usually they act exactly like for-loops, where you provide an
> iterator, and then use it in each iteration:
> 
> struct cpsw_slave *slave;
> 
> for_each_slave(slave, priv)
>   cpsw_slave_update_link(slave, priv, &link);
> 
> I think this is much easier to understand and maintain.
> 
> > +       return link;
> 
> Also, link looks useless. It will always be the value in the last
> slave. Is that what you meant to do?
> 
> 

Ok, I will use this as per your comment.
for_each_slave(slave, priv)
	cpsw_slave_update_link(slave, priv, &link);

Also, I am returning link which returns the value of last slave.

> > +}
> > +
> > +static void cpsw_slave_init(struct cpsw_slave *slave, struct
> cpsw_priv *priv)
> > +{
> 
> [...]
> 
> > +
> > +       priv->data.phy_init(priv->dev->name, slave->data->phy_id);
> 
> What does this do? Probably needs to be converted to the newer API.
> 

I will remove this and new PHYLIB APIs will be used instead.

> 
> > +}
> > +
> > +static struct cpdma_desc *cpdma_desc_alloc(struct cpsw_priv *priv)
> > +{
> > +       struct cpdma_desc *desc = priv->desc_free;
> > +
> > +       if (desc)
> > +               priv->desc_free = desc_read_ptr(desc, hw_next);
> > +       return desc;
> > +}
> > +
> > +static void cpdma_desc_free(struct cpsw_priv *priv, struct
> cpdma_desc *desc)
> > +{
> > +       if (desc) {
> > +               desc_write(desc, hw_next, priv->desc_free);
> > +               priv->desc_free = desc;
> > +       }
> > +}
> > +
> > +static int cpdma_submit(struct cpsw_priv *priv, struct cpdma_chan
> *chan,
> > +                       volatile void *buffer, int len)
> > +{
> > +       struct cpdma_desc *desc, *prev;
> > +       u32 mode;
> > +
> > +       desc = cpdma_desc_alloc(priv);
> > +       if (!desc)
> > +               return -ENOMEM;
> > +
> > +       if (len < PKT_MIN)
> > +               len = PKT_MIN;
> > +
> > +       mode = CPDMA_DESC_OWNER | CPDMA_DESC_SOP | CPDMA_DESC_EOP;
> > +
> > +       desc_write(desc, hw_next,   0);
> > +       desc_write(desc, hw_buffer, buffer);
> > +       desc_write(desc, hw_len,    len);
> > +       desc_write(desc, hw_mode,   mode | len);
> > +       desc_write(desc, sw_buffer, buffer);
> > +       desc_write(desc, sw_len,    len);
> > +
> > +       if (!chan->head) {
> > +               /* simple case - first packet enqueued */
> > +               chan->head = desc;
> > +               chan->tail = desc;
> > +               chan_write(chan, hdp, desc);
> > +               goto done;
> > +       }
> > +
> > +       /* not the first packet - enqueue at the tail */
> > +       prev = chan->tail;
> > +       desc_write(prev, hw_next, desc);
> > +       chan->tail = desc;
> > +
> > +       /* next check if EOQ has been triggered already */
> > +       if (desc_read(prev, hw_mode) & CPDMA_DESC_EOQ)
> > +               chan_write(chan, hdp, desc);
> > +
> > +done:
> > +       if (chan->rxfree)
> > +               chan_write(chan, rxfree, 1);
> > +       return 0;
> > +}
> > +
> > +static int cpdma_process(struct cpsw_priv *priv, struct cpdma_chan
> *chan,
> > +                        void **buffer, int *len)
> > +{
> > +       struct cpdma_desc *desc = chan->head;
> > +       u32 status;
> > +
> > +       if (!desc)
> > +               return -ENOENT;
> > +
> > +       status  = desc_read(desc, hw_mode);
> > +
> > +       if (len)
> > +               *len = status & 0x7ff;
> > +
> > +       if (buffer)
> > +               *buffer = desc_read_ptr(desc, sw_buffer);
> > +
> > +       if (status & CPDMA_DESC_OWNER)
> > +               return -EBUSY;
> > +
> > +       chan->head = desc_read_ptr(desc, hw_next);
> > +       chan_write(chan, cp, desc);
> > +
> > +       cpdma_desc_free(priv, desc);
> > +       return 0;
> > +}
> > +
> > +static int cpsw_init(struct eth_device *dev, bd_t *bis)
> > +{
> > +       struct cpsw_priv        *priv = dev->priv;
> > +       int i, ret;
> > +
> > +       priv->data.control(1);
> 
> 1 what? The "control" function probably needs a better name. And maybe
> so does "1".
> 

This is not required and will be removed from the code.

> > +
> > +       /* soft reset the controller and initialize priv */
> > +       soft_reset(&priv->regs->soft_reset);
> > +
> 
> See comment about soft_reset, above.
> 

Ok, I will address this as above.

> > +
> > +       cpsw_ale_port_state(priv, priv->host_port,
> ALE_PORT_STATE_FORWARD);
> > +
> > +       cpsw_ale_add_ucast(priv, priv->dev->enetaddr, priv-
> >host_port,
> > +                          ALE_SECURE);
> > +       cpsw_ale_add_mcast(priv, NetBcastAddr, 1 << priv->host_port);
> > +
> > +       for_each_slave(priv, cpsw_slave_init, priv);
> 
> Same comment as before about the for-loop construct.
> 

Ok, will address in similar way as above.

> [...]
> 
> > +       eth_register(dev);
> > +
> > +       cpsw_mdio_init(dev->name, data->mdio_base, data->mdio_div);
> 
> I bet you only need one MDIO bus, and the new PHY API makes that
> straightforward.
>

Yes, I will use new PHYLIB APIs.
 
> > +
> > +       return 1;
> > +}
> > diff --git a/include/netdev.h b/include/netdev.h
> > index 96c7b9b..740213e 100644
> > --- a/include/netdev.h
> > +++ b/include/netdev.h
> > @@ -185,4 +185,33 @@ struct mv88e61xx_config {
> >  int mv88e61xx_switch_initialize(struct mv88e61xx_config *swconfig);
> >  #endif /* CONFIG_MV88E61XX_SWITCH */
> >
> > +#ifdef CONFIG_DRIVER_TI_CPSW
> > +
> > +struct cpsw_slave_data {
> > +       u32             slave_reg_ofs;
> > +       u32             sliver_reg_ofs;
> > +       int             phy_id;
> > +};
> > +
> > +struct cpsw_platform_data {
> > +       u32     mdio_base;
> > +       u32     cpsw_base;
> > +       int     mdio_div;
> > +       int     channels;       /* number of cpdma channels
> (symmetric) */
> > +       u32     cpdma_reg_ofs;  /* cpdma register offset
>    */
> > +       int     slaves;         /* number of slave cpgmac ports
>   */
> > +       u32     ale_reg_ofs;    /* address lookup engine reg offset
>   */
> > +       int     ale_entries;    /* ale table size
>   */
> > +       u32     host_port_reg_ofs;      /* cpdma host port registers
>    */
> > +       u32     hw_stats_reg_ofs;       /* cpsw hw stats counters
>   */
> > +       u32     mac_control;
> > +       struct cpsw_slave_data  *slave_data;
> > +       void    (*control)(int enabled);
> > +       void    (*phy_init)(char *name, int addr);
> > +};
> > +
> > +int cpsw_register(struct cpsw_platform_data *data);
> > +
> > +#endif /* CONFIG_DRIVER_TI_CPSW */
> 
> Hmmm... nothing here seems like it should be in a generic header like
> "netdev.h". Only cpsw_register(), and probably not that, either. I see
> there's some Marvell stuff in there, too, which is suspect, but let's
> not propagate that issue. There's a cpu_eth_init() and a
> board_eth_init(). Surely one of those can call cpsw_register(), and
> then you can put the cpsw-specific structures in something like
> include/cpsw.h.
> 

Sure, I will remove this code from netdev.h and will keep in separate cpsw header, i.e. cpsw.h

> Andy