From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ben Warren <biggerbadderben@gmail.com>
Date: Wed, 22 Apr 2009 22:30:23 -0700
Subject: [U-Boot] [PATCH v6] Marvell MV88E61XX Switch Driver support
In-Reply-To: <1240403013-24534-1-git-send-email-prafulla@marvell.com>
References: 73173D32E9439E4ABB5151606C3E19E201CA07C637@SC-VEXCH1.marvell.com
	<1240403013-24534-1-git-send-email-prafulla@marvell.com>
Message-ID: <49EFFCEF.5000005@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Prafulla,

So close...

Prafulla Wadaskar wrote:
<snip>
> +/* Chip Address mode
> + * The Switch support two modes of operation
> + * 1. single chip mode and
> + * 2. Multi-chip mode
> + * Refer section 9.2 &9.3 in chip datasheet-02 for more details
> + *
> + * By default single chip mode is configured
> + * multichip mode operation can be configured in board header
> + */
> +#ifndef CONFIG_MV88E61XX_MULTICHIP_ADRMODE
> +#define mv88e61xx_wr_phy miiphy_write
> +#define mv88e61xx_rd_phy miiphy_read
> +#else
> +
> +static int mv88e61xx_busychk_multic(u32 devaddr)
> +{
> +	u32 reg = 0;
> +	u32 timeout = MV88E61XX_PHY_TIMEOUT;
> +
> +	/* Poll till SMIBusy bit is clear */
> +	do {
> +		miiphy_read(name, devaddr, 0x0, &reg);
> +		if (timeout-- == 0) {
> +			printf("SMI busy timeout\n");
> +			return -1;
> +		}
> +	} while (reg & BIT15);
> +	return 0;
> +}
> +
> +static void mv88e61xx_wr_phy(char *name, u32 phy_adr, u32 reg_ofs, u16 data)
> +{
> +	u16 reg;
> +	u32 mii_dev_addr;
> +
> +	/* command to read PHY dev address */
> +	if (!miiphy_read(name, 0xEE, 0xEE, &mii_dev_addr)) {
> +		printf("Error..could not read PHY dev address\n");
> +		return;
> +	}
> +	mv88e61xx_busychk_multic(mii_dev_addr);
> +	/* Write data to Switch indirect data register */
> +	miiphy_write(name, mii_dev_addr, 0x1, data);
> +	/* Write command to Switch indirect command register (write) */
> +	miiphy_write(name, mii_dev_addr, 0x0,
> +		     reg_ofs | (phy_adr << 5) | BIT10 | BIT12 | BIT15);
> +}
> +
> +static void mv88e61xx_rd_phy(char *name, u32 phy_adr, u32 reg_ofs, u16 * data)
> +{
> +	u16 reg;
> +	u32 mii_dev_addr;
> +
> +	/* command to read PHY dev address */
> +	if (!miiphy_read(name, 0xEE, 0xEE, &mii_dev_addr)) {
> +		printf("Error..could not read PHY dev address\n");
> +		return;
> +	}
> +	mv88e61xx_busychk_multic(mii_dev_addr);
> +	/* Write command to Switch indirect command register (read) */
> +	miiphy_write(name, mii_dev_addr, 0x0,
> +		     reg_ofs | (phy_adr << 5) | BIT10 | BIT12 | BIT15);
> +	mv88e61xx_busychk_multic(mii_dev_addr);
> +	/* Read data from Switch indirect data register */
> +	miiphy_read(name, mii_dev_addr, 0x1, (u16 *) & data);
> +}
> +#endif /* CONFIG_MV88E61XX_MULTICHIP_ADRMODE */
> +
>   
I apologize for not being more clear here. Please don't put non-trivial 
functions in header files. What I was hoping you'd do was put something 
like this in the header file:

static void mv88e61xx_wr_phy(char *name, u32 phy_adr, u32 reg_ofs, u16 data);
static void mv88e61xx_rd_phy(char *name, u32 phy_adr, u32 reg_ofs, u16 * data);

/* Helpful comment here */
#ifndef CONFIG_MV88E61XX_MULTICHIP_ADRMODE
#define WRITE_PHY miiphy_write
#define READ_PHY miiphy_read
#else
#define WRITE_PHY mv88e61xx_wr_phy
#define READ_PHY mv88e61xx_rd_phy
#endif


and then use these macros throughout. You'll agree that it's much easier 
to follow.

> +#endif /* _MV88E61XX_H */
> diff --git a/include/netdev.h b/include/netdev.h
> index 2794ddd..e6832de 100644
> --- a/include/netdev.h
> +++ b/include/netdev.h
> @@ -59,6 +59,7 @@ int mpc512x_fec_initialize(bd_t *bis);
>  int mpc5xxx_fec_initialize(bd_t *bis);
>  int mpc8220_fec_initialize(bd_t *bis);
>  int mpc82xx_scc_enet_initialize(bd_t *bis);
> +int mv88e61xx_switch_initialize(struct mv88e61xx_config *swconfig);
>  int natsemi_initialize(bd_t *bis);
>  int npe_initialize(bd_t *bis);
>  int ns8382x_initialize(bd_t *bis);
>   

Apart from that, it looks really good. Thanks for all the hard work.

regards,
Ben