[U-Boot] [PATCH v2 1/6] tsec: use IO accessors for IO accesses

Detlev Zundel dzu at denx.de
Wed Apr 6 14:29:38 CEST 2011 

Hi Andy,

> From: Mingkai Hu <Mingkai.hu at freescale.com>
>
> Signed-off-by: Mingkai Hu <Mingkai.hu at freescale.com>
> Acked-by: Andy Fleming <afleming at freescale.com>
> Signed-off-by: Kumar Gala <galak at kernel.crashing.org>
> ---
> Fixed the commit message's spelling
>
>  drivers/net/tsec.c |  242 +++++++++++++++++++++++++++-------------------------
>  include/tsec.h     |    8 +-
>  2 files changed, 130 insertions(+), 120 deletions(-)

Despite what Kumar says I can't see that patch in mpc85xx/next so let's
see.

> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index 9a91b9e..4ea9813 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c

[...]

> @@ -230,17 +230,18 @@ static int tsec_init(struct eth_device *dev, bd_t * bd)
>  }
>  
>  /* Writes the given phy's reg with value, using the specified MDIO regs */
> -static void tsec_local_mdio_write(volatile tsec_mdio_t *phyregs, uint addr,
> +static void tsec_local_mdio_write(tsec_mdio_t *phyregs, uint addr,
>  		uint reg, uint value)
>  {
>  	int timeout = 1000000;
>  
> -	phyregs->miimadd = (addr << 8) | reg;
> -	phyregs->miimcon = value;
> +	out_be32(&phyregs->miimadd, (addr << 8) | reg);
> +	out_be32(&phyregs->miimcon, value);
>  	asm("sync");

We don't need the sync here anymore, as this is done by the next
in_be32.

>  
>  	timeout = 1000000;
> -	while ((phyregs->miimind & MIIMIND_BUSY) && timeout--) ;
> +	while ((in_be32(&phyregs->miimind) & MIIMIND_BUSY) && timeout--)
> +		;
>  }
>  
>  
> @@ -254,28 +255,28 @@ static void tsec_local_mdio_write(volatile tsec_mdio_t *phyregs, uint addr,
>   * notvalid bit cleared), and the bus to cease activity (miimind
>   * busy bit cleared), and then returns the value
>   */
> -static uint tsec_local_mdio_read(volatile tsec_mdio_t *phyregs,
> -				uint phyid, uint regnum)
> +static uint tsec_local_mdio_read(tsec_mdio_t *phyregs, uint phyid, uint regnum)
>  {
>  	uint value;
>  
>  	/* Put the address of the phy, and the register
>  	 * number into MIIMADD */
> -	phyregs->miimadd = (phyid << 8) | regnum;
> +	out_be32(&phyregs->miimadd, (phyid << 8) | regnum);
>  
>  	/* Clear the command register, and wait */
> -	phyregs->miimcom = 0;
> +	out_be32(&phyregs->miimcom, 0);
>  	asm("sync");

Not needed anymore, done by the next accessor.

>  	/* Initiate a read command, and wait */
> -	phyregs->miimcom = MIIM_READ_COMMAND;
> +	out_be32(&phyregs->miimcom, MIIM_READ_COMMAND);
>  	asm("sync");

Dito.

>  
>  	/* Wait for the the indication that the read is done */
> -	while ((phyregs->miimind & (MIIMIND_NOTVALID | MIIMIND_BUSY))) ;
> +	while ((in_be32(&phyregs->miimind) & (MIIMIND_NOTVALID | MIIMIND_BUSY)))
> +		;
>  
>  	/* Grab the value read from the PHY */
> -	value = phyregs->miimstat;
> +	value = in_be32(&phyregs->miimstat);
>  
>  	return value;
>  }
> @@ -321,18 +322,19 @@ static int init_phy(struct eth_device *dev)
>  {
>  	struct tsec_private *priv = (struct tsec_private *)dev->priv;
>  	struct phy_info *curphy;
> -	volatile tsec_t *regs = priv->regs;
> +	tsec_t *regs = priv->regs;
>  
>  	/* Assign a Physical address to the TBI */
> -	regs->tbipa = CONFIG_SYS_TBIPA_VALUE;
> +	out_be32(&regs->tbipa, CONFIG_SYS_TBIPA_VALUE);
>  	asm("sync");

dito

>  
>  	/* Reset MII (due to new addresses) */
> -	priv->phyregs->miimcfg = MIIMCFG_RESET;
> +	out_be32(&priv->phyregs->miimcfg, MIIMCFG_RESET);
>  	asm("sync");

dito

> -	priv->phyregs->miimcfg = MIIMCFG_INIT_VALUE;
> +	out_be32(&priv->phyregs->miimcfg, MIIMCFG_INIT_VALUE);
>  	asm("sync");

once more

> -	while (priv->phyregs->miimind & MIIMIND_BUSY) ;
> +	while (in_be32(&priv->phyregs->miimind) & MIIMIND_BUSY)
> +		;
>  
>  	/* Get the cmd structure corresponding to the attached
>  	 * PHY */
> @@ -345,7 +347,7 @@ static int init_phy(struct eth_device *dev)
>  		return 0;
>  	}
>  
> -	if (regs->ecntrl & ECNTRL_SGMII_MODE)
> +	if (in_be32(&regs->ecntrl) & ECNTRL_SGMII_MODE)
>  		tsec_configure_serdes(priv);
>  
>  	priv->phyinfo = curphy;
> @@ -838,16 +840,17 @@ static uint mii_parse_dm9161_scsr(uint mii_reg, struct tsec_private * priv)
>  static uint mii_cis8204_fixled(uint mii_reg, struct tsec_private * priv)
>  {
>  	uint phyid;
> -	volatile tsec_mdio_t *regbase = priv->phyregs;
> +	tsec_mdio_t *regbase = priv->phyregs;
>  	int timeout = 1000000;
>  
>  	for (phyid = 0; phyid < 4; phyid++) {
> -		regbase->miimadd = (phyid << 8) | mii_reg;
> -		regbase->miimcon = MIIM_CIS8204_SLEDCON_INIT;
> +		out_be32(&regbase->miimadd, (phyid << 8) | mii_reg);
> +		out_be32(&regbase->miimcon, MIIM_CIS8204_SLEDCON_INIT);
>  		asm("sync");

and again

>  
>  		timeout = 1000000;
> -		while ((regbase->miimind & MIIMIND_BUSY) && timeout--) ;
> +		while ((in_be32(&regbase->miimind) & MIIMIND_BUSY) && timeout--)
> +			;
>  	}
>  
>  	return MIIM_CIS8204_SLEDCON_INIT;

[...]

> @@ -922,44 +925,49 @@ static void init_registers(volatile tsec_t * regs)
>  static void adjust_link(struct eth_device *dev)
>  {
>  	struct tsec_private *priv = (struct tsec_private *)dev->priv;
> -	volatile tsec_t *regs = priv->regs;
> +	tsec_t *regs = priv->regs;
> +	u32 ecntrl, maccfg2;
>  
> -	if (priv->link) {
> -		if (priv->duplexity != 0)
> -			regs->maccfg2 |= MACCFG2_FULL_DUPLEX;
> -		else
> -			regs->maccfg2 &= ~(MACCFG2_FULL_DUPLEX);
> +	if (!priv->link) {
> +		printf("%s: No link.\n", dev->name);
> +		return;
> +	}
>  
> -		switch (priv->speed) {
> -		case 1000:
> -			regs->maccfg2 = ((regs->maccfg2 & ~(MACCFG2_IF))
> -					 | MACCFG2_GMII);
> -			break;
> -		case 100:
> -		case 10:
> -			regs->maccfg2 = ((regs->maccfg2 & ~(MACCFG2_IF))
> -					 | MACCFG2_MII);
> +	/* clear all bits relative with interface mode */
> +	ecntrl = in_be32(&regs->ecntrl);
> +	ecntrl &= ~ECNTRL_R100;
>  
> -			/* Set R100 bit in all modes although
> -			 * it is only used in RGMII mode
> -			 */
> -			if (priv->speed == 100)
> -				regs->ecntrl |= ECNTRL_R100;
> -			else
> -				regs->ecntrl &= ~(ECNTRL_R100);
> -			break;
> -		default:
> -			printf("%s: Speed was bad\n", dev->name);
> -			break;
> -		}
> +	maccfg2 = in_be32(&regs->maccfg2);
> +	maccfg2 &= ~(MACCFG2_IF | MACCFG2_FULL_DUPLEX);
>  
> -		printf("Speed: %d, %s duplex%s\n", priv->speed,
> -		       (priv->duplexity) ? "full" : "half",
> -		       (priv->flags & TSEC_FIBER) ? ", fiber mode" : "");
> +	if (priv->duplexity)
> +		maccfg2 |= MACCFG2_FULL_DUPLEX;
>  
> -	} else {
> -		printf("%s: No link.\n", dev->name);
> +	switch (priv->speed) {
> +	case 1000:
> +		maccfg2 |= MACCFG2_GMII;
> +		break;
> +	case 100:
> +	case 10:
> +		maccfg2 |= MACCFG2_MII;
> +
> +		/* Set R100 bit in all modes although
> +		 * it is only used in RGMII mode
> +		 */
> +		if (priv->speed == 100)
> +			ecntrl |= ECNTRL_R100;
> +		break;
> +	default:
> +		printf("%s: Speed was bad\n", dev->name);
> +		break;
>  	}
> +
> +	out_be32(&regs->ecntrl, ecntrl);
> +	out_be32(&regs->maccfg2, maccfg2);
> +
> +	printf("Speed: %d, %s duplex%s\n", priv->speed,
> +			(priv->duplexity) ? "full" : "half",
> +			(priv->flags & TSEC_FIBER) ? ", fiber mode" : "");
>  }

Hm, this hunk is definitely more than changing to accessors, but if I
read it correctly, it introduces a short-cut way out and thus changes
indentation but does not change functionality.  Still would have been
nice to split this out into a change before and only do whats advertised
in the patch description here.

Cheers
  Detlev

-- 
Wissenschaft ohne Verstand ist doppelte Narrheit.
                                    --- Baltasar Gracian
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

More information about the U-Boot mailing list