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

[Ben Warren]

  On 8/31/2010 8:58 AM, Cyril Chemparathy wrote:
> 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?
>
Sure.
> [...]
>>> +++ 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.
>
If it's truly private within the driver and will never be needed by 
anybody else, the source file is OK, but since you're going to create a 
new header file in /include/net anyway, maybe you'll want to put some 
stuff there.  Your call...
> [...]
>>> 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.
regards,
Ben