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

[Cyril Chemparathy]

Hi Ben,

[...]
>> +COBJS-$(CONFIG_DRIVER_TI_CPSW) += cpsw.o
> Please don't use the word DRIVER here.  If possible, use something more
> verbose than "CPSW" too.

Will TI_CPSW_SWITCH work better considering that "CPSW" is the name of
the hardware block?

[...]
>> +++ b/drivers/net/cpsw.c
> Please rename this ti_cpsw.c
Agreed.

[...]
>> +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)
>> +};
>> +
> 
> Can this stuff go in a header file?

This stuff is intended to be private within the driver.  I can split
this out into drivers/net/ti_cpsw.h.

[...]
>> diff --git a/include/netdev.h b/include/netdev.h
>> index 94eedfe..009e2f1 100644
>> --- a/include/netdev.h
>> +++ b/include/netdev.h
>> @@ -180,4 +180,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);
>> +};
>> +
> This stuff doesn't belong in this file.  Definitions specific to your
> driver should go in /include/net/ti_cpsw.h (note that you'll be creating
> this directory, but that's where we want driver headers to go moving
> forward.  Also, please call the initialize function
> 'ti_cpsw_initialize()'.  Despite what the readme file recommends, you're
> initializing something, not registering it.  If anything, the 'init()'
> functions are misnamed, but that's another discussion.

Will do.

We will post an updated v2 series shortly, with these changes.

Thanks
Cyril.