Andy,
> -----Original Message-----
> From: Andy Fleming [mailto:aflem...@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.n...@ti.com>
> wrote:
> > From: Cyril Chemparathy <cy...@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 <cy...@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, ®))
> > + 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
