[U-Boot] [U-Boot PATCH v2 10/12] keystone2: net: add keystone ethernet driver

[Tom Rini]

On Thu, Feb 20, 2014 at 12:55:12PM -0500, Murali Karicheri wrote:

> From: Vitaly Andrianov <vitalya at ti.com>
> 
> Ethernet driver configures the CPSW, SGMI and Phy and uses
> the the Navigator APIs. The driver supports 4 Ethernet ports and
> can work with only one port at a time.

First, can we just use things in a "dumb" mode and use the existing CPSW
driver?  Or are there use cases we need to worry about here with a
bigger / more robust network driver?

> Port configurations are defined in board.c.
> 
> Signed-off-by: Vitaly Andrianov <vitalya at ti.com>
> Signed-off-by: Murali Karicheri <m-karicheri2 at ti.com>
> Signed-off-by: WingMan Kwok <w-kwok2 at ti.com>
[snip]
> +/*#define chipLmbd(x,y) _lmbd(x,y) */

Don't add unused code.

> +/* Marvell 88E1111 PHY ID */
> +#define PHY_MARVELL_88E1111		(0x01410cc0)

More on this later.

> +/* EMAC MDIO Registers Structure */
> +struct mdio_regs {
> +	dv_reg		VERSION;
> +	dv_reg		CONTROL;

Why caps?

> +++ b/drivers/net/keystone_net.c

Generic problem, please use phylib.

> +#define debug_emac(fmt, args...) if (emac_dbg) printf(fmt, ##args)

We have debug() already.

> +#ifdef KEYSTONE2_EMAC_GIG_ENABLE
> +#define emac_gigabit_enable(x)	keystone2_eth_gigabit_enable(x)
> +#else
> +#define emac_gigabit_enable(x)	/* no gigabit to enable */
> +#endif

Do we need to do this?

> +static void chip_delay(u32 del)
> +{
> +	volatile unsigned int i;
> +
> +	for (i = 0; i < (del / 8); i++)
> +		;
> +}

Use one of the generic delays please.

> +/* PHY functions for a generic PHY */
> +static int __attribute__((unused)) gen_init_phy(int phy_addr)

Why are we adding tons of functions that we mark as unused?

> +#if CONFIG_GET_LINK_STATUS_ATTEMPTS > 1
> +	int j;
> +
> +	for (j = 0; (j < CONFIG_GET_LINK_STATUS_ATTEMPTS) && (link_state == 0);
> +	     j++) {
> +#endif

New config option, do we really need this as an option?

> +void sgmii_serdes_setup_156p25mhz()
> +{
> +	unsigned int cnt;
> +
> +	reg_rmw(0x0232a000, 0x00800000, 0xffff0000);

Please comment on what we're doing here, and why we aren't using a
struct to describe whatever is at 0x0232a000 ?

> +void sgmii_serdes_shutdown()
> +{
> +	reg_rmw(0x0232bfe0, 0, 3 << 29 | 3 << 13);

Same.

Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20140225/a97bd1cf/attachment.pgp>