[U-Boot,3/6] net: asix: split out basic reset function

The basic device reset ensures that the device is ready to
service commands and does not need to get redone before each
network operation.

Split out the basic reset from asix_init() and instead call it
from asix_eth_get_info(), so that it only gets called once.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
---
 drivers/usb/eth/asix.c | 44 ++++++++++++++++++++++++++------------------
 1 Datei geändert, 26 Zeilen hinzugefügt(+), 18 Zeilen entfernt(-)

Dear Lucas Stach,

> The basic device reset ensures that the device is ready to
> service commands and does not need to get redone before each
> network operation.
> 
> Split out the basic reset from asix_init() and instead call it
> from asix_eth_get_info(), so that it only gets called once.
> 
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> ---
>  drivers/usb/eth/asix.c | 44 ++++++++++++++++++++++++++------------------
>  1 Datei geändert, 26 Zeilen hinzugefügt(+), 18 Zeilen entfernt(-)
> 
> diff --git a/drivers/usb/eth/asix.c b/drivers/usb/eth/asix.c
> index 8fb7fc8..50cbbbd 100644
> --- a/drivers/usb/eth/asix.c
> +++ b/drivers/usb/eth/asix.c
> @@ -310,55 +310,60 @@ static int mii_nway_restart(struct ueth_data *dev)
>  	return r;
>  }
> 
> -/*
> - * Asix callbacks
> - */
> -static int asix_init(struct eth_device *eth, bd_t *bd)
> +static int asix_basic_reset(struct ueth_data *dev)
>  {
>  	int embd_phy;
> -	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
>  	u16 rx_ctl;
> -	struct ueth_data	*dev = (struct ueth_data *)eth->priv;
> -	int timeout = 0;
> -#define TIMEOUT_RESOLUTION 50	/* ms */
> -	int link_detected;
> -
> -	debug("** %s()\n", __func__);
> 
>  	if (asix_write_gpio(dev,
>  			AX_GPIO_RSE | AX_GPIO_GPO_2 | AX_GPIO_GPO2EN, 5) < 0)
> -		goto out_err;
> +		return -1;

You might want to use proper errno.h here ... like -ETIMEDOUT etc.

> 
>  	/* 0x10 is the phy id of the embedded 10/100 ethernet phy */
>  	embd_phy = ((asix_get_phy_addr(dev) & 0x1f) == 0x10 ? 1 : 0);
>  	if (asix_write_cmd(dev, AX_CMD_SW_PHY_SELECT,
>  				embd_phy, 0, 0, NULL) < 0) {
>  		debug("Select PHY #1 failed\n");
> -		goto out_err;
> +		return -1;
>  	}
> 
>  	if (asix_sw_reset(dev, AX_SWRESET_IPPD | AX_SWRESET_PRL) < 0)
> -		goto out_err;
> +		return -1;
> 
>  	if (asix_sw_reset(dev, AX_SWRESET_CLEAR) < 0)
> -		goto out_err;
> +		return -1;
> 
>  	if (embd_phy) {
>  		if (asix_sw_reset(dev, AX_SWRESET_IPRL) < 0)
> -			goto out_err;
> +			return -1;
>  	} else {
>  		if (asix_sw_reset(dev, AX_SWRESET_PRTE) < 0)
> -			goto out_err;
> +			return -1;
>  	}
> 
>  	rx_ctl = asix_read_rx_ctl(dev);
>  	debug("RX_CTL is 0x%04x after software reset\n", rx_ctl);
>  	if (asix_write_rx_ctl(dev, 0x0000) < 0)
> -		goto out_err;
> +		return -1;
> 
>  	rx_ctl = asix_read_rx_ctl(dev);
>  	debug("RX_CTL is 0x%04x setting to 0x0000\n", rx_ctl);
> 
> +	return 0;
> +}
> +
> +/*
> + * Asix callbacks
> + */
> +static int asix_init(struct eth_device *eth, bd_t *bd)
> +{
> +	struct ueth_data	*dev = (struct ueth_data *)eth->priv;
> +	int timeout = 0;
> +#define TIMEOUT_RESOLUTION 50	/* ms */
> +	int link_detected;
> +
> +	debug("** %s()\n", __func__);
> +
>  	/* Get the MAC address */
>  	if (asix_read_cmd(dev, AX_CMD_READ_NODE_ID,
>  				0, 0, ETH_ALEN, buf) < 0) {
> @@ -635,5 +640,8 @@ int asix_eth_get_info(struct usb_device *dev, struct
> ueth_data *ss, eth->halt = asix_halt;
>  	eth->priv = ss;
> 
> +	if (asix_basic_reset(ss))
> +		return 0;
> +
>  	return 1;
>  }

Best regards,
Marek Vasut

Hi Lucas,

On Wed, Aug 22, 2012 at 5:09 AM, Lucas Stach <dev@lynxeye.de> wrote:
> The basic device reset ensures that the device is ready to
> service commands and does not need to get redone before each
> network operation.
>
> Split out the basic reset from asix_init() and instead call it
> from asix_eth_get_info(), so that it only gets called once.
>
> Signed-off-by: Lucas Stach <dev@lynxeye.de>

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

On Wednesday 22 August 2012 06:09:04 Lucas Stach wrote:
> The basic device reset ensures that the device is ready to
> service commands and does not need to get redone before each
> network operation.
> 
> Split out the basic reset from asix_init() and instead call it
> from asix_eth_get_info(), so that it only gets called once.

i'm afraid this is wrong.  the register step (which asix_eth_get_info is 
afaict) should not touch the hardware (other than to extract the MAC address).  
the init func is supposed to bring up the hardware from scratch since the 
expectation is that the halt func brings it down completely.  if it's not 
really possible to completely "bring down" the hardware, then have the 
asix_init func declare a local static int so that it does the "full" bring up 
only once.

so NAK this patch as is
-mike

Hi Mike,

Am Mittwoch, den 22.08.2012, 23:01 -0400 schrieb Mike Frysinger:
> On Wednesday 22 August 2012 06:09:04 Lucas Stach wrote:
> > The basic device reset ensures that the device is ready to
> > service commands and does not need to get redone before each
> > network operation.
> > 
> > Split out the basic reset from asix_init() and instead call it
> > from asix_eth_get_info(), so that it only gets called once.
> 
> i'm afraid this is wrong.  the register step (which asix_eth_get_info is 
> afaict) should not touch the hardware (other than to extract the MAC address).  
> the init func is supposed to bring up the hardware from scratch since the 
> expectation is that the halt func brings it down completely.  if it's not 
> really possible to completely "bring down" the hardware, then have the 
> asix_init func declare a local static int so that it does the "full" bring up 
> only once.
> 
> so NAK this patch as is
> -mike

I still think the patch does the right thing. Let's look at the call
chain. When doing a usb start of the controller where the ethernet
device is attached we do the following to register the device:
1. probe (just check if we found suitable hardware, don't touch it yet)
2. get_info (at this point we extract the MAC, which means we have to
bring up the hardware at a basic level, to even be able to touch the
registers)
3. write_hwaddr (eth core sets the MAC address of the device, remember
this is only done _once_ for each device and also needs basic init)

Every network operation basically does first a init() and then a halt().
If we would bring down the device here to a point where it's completely
reset, we would also lose the MAC address set in the register step. This
is clearly not what we want, but also we don't want to set the MAC over
and over again with each network operation. So IMHO halt() should only
bring down the device to a state where the current ethernet connection
is gone. Therefore init() only needs to bring up the ethernet connection
from this state. The basic device initialisation (including the MAC)
should be persistent across multiple network operations.

This is the behaviour this patch implements, aside from halt() not
really doing it's job in the asix driver, but that is for another patch.

Thanks,
Lucas