[U-Boot] [PATCH v2 1/2] usb: eth: add ASIX AX88179 DRIVER

[Marek Vasut]

On Wednesday, September 10, 2014 at 12:00:29 PM, René Griessl wrote:
> Am 09.09.2014 16:34, schrieb Marek Vasut:
> > On Wednesday, September 03, 2014 at 04:31:20 PM, Rene Griessl wrote:
> >> changes in v2:
> >> 	-added usb_ether.h to change list
> >> 	-added 2nd patch to enable driver for arndale board
> >> 
> >> Signed-off-by: Rene Griessl <rgriessl at cit-ec.uni-bielefeld.de>
> > 
> > I see that in Linux, there is asix_common.c stuff. Can this driver share
> > any parts with drivers/net/ax88* ?
> 
> The asix_common.c includes asix.h. There you see that the common driver
> supports following devices:
> AX88172
> AX88772
> AX88178
> but it is not supporting AX88179 and AX88178a, they have a separate
> driver called ax88179_178a.c
> These two have a different style in accessing MAC registers and PHY

I didn't look deep enough. The 88179 driver is a completely separate driver, not 
sharing any code what-so-ever with the other ASIX driver, yes ?

[...]

> >> +		      buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
> >> +		}
> >> +	else {
> >> +		}
> > 
> > Uh, this check needs some rework, right ? Also, you want to lint your
> > patches with ./scripts/checkpatch.pl tool before resubmitting.
> 
> was OK for ./scripts/checkpatch.pl
> but I can change that

This is rather surprising, but you're right. Please fix this up, the else can be 
dropped and the trailing } can be indented just under the if () clause.

> >> +	return ret;
> >> +}
> >> +
> >> +
> >> +static int asix_read_mac(struct eth_device *eth)
> >> +{
> >> +	struct ueth_data *dev = (struct ueth_data *)eth->priv;
> >> +	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buf, ETH_ALEN);
> >> +
> >> +	if (dev->pusb_dev->descriptor.idProduct == 0x1790) {
> >> +		asix_read_cmd(dev, AX_ACCESS_MAC, 0x10, 6, 6, buf);
> >> +		debug("asix_read_mac() returning 0x%02x%02x%02x%02x%02x%02x\n",
> >> +		      buf[0], buf[1], buf[2], buf[3], buf[4], buf[5]);
> >> +		memcpy(eth->enetaddr, buf, ETH_ALEN);
> >> +		}

Same here.

> >> +	return 0;
> >> +}
> >> +
> >> +
> >> +
> >> +static int asix_basic_reset(struct ueth_data *dev)
> >> +{
> >> +	ALLOC_CACHE_ALIGN_BUFFER(u8, buf, 6);
> > 
> > Why does the buffer need to be aligned here ? It's just a buffer that is
> > not used for DMA, no ?
> > 
> >> +	u16 *tmp16;
> > 
> > Is it because you were getting unaligned accesses , since when the buffer
> > itself was not aligned and you did cast it to u16, the CPU triggered
> > unaligned access ?
> 
> Thats right, if I do not align I get unaligned accesses during USB
> communication.
> Is there a smarter solution for that?

Oh I see. The smart solution would be to add bounce buffer into the USB stack, 
but unless you want to dive into this, this solution would be OK here.