[U-Boot,2/8] net: asix: fix operation without eeprom

From: Marcel Ziswiler <marcel.ziswiler@toradex.com>

This patch fixes operation of our on-board AX88772B chip without EEPROM
but with a ethaddr coming from the regular U-Boot environment. This is
a forward port of some remaining parts initially implemented by
Antmicro.

Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
---
 drivers/usb/eth/asix.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

On Thursday, July 02, 2015 at 01:04:47 AM, Marcel Ziswiler wrote:

Hi!

[...]

> @@ -64,8 +67,14 @@
>  	 AX_MEDIUM_AC | AX_MEDIUM_RE)
> 
>  /* AX88772 & AX88178 RX_CTL values */
> +#define AX_RX_CTL_RH2M			0x0200	/* Enable IP header in 
receive
> +						   buffer aligned on 32-bit
> +						   boundary */

The comments need a bit of polishing, though it is not the main problem I have
with this patch. The multiline comments should be like this according to kernel
coding style (to my knowledge):

/*
 * foo
 * bar
 * baz
 */

> +#define AX_RX_CTL_RH1M			0x0100	/* Enable RX-Header mode 
0 */
>  #define AX_RX_CTL_SO			0x0080
>  #define AX_RX_CTL_AB			0x0008
> +#define AX_RX_HEADER_DEFAULT		(AX_RX_CTL_RH1M | \
> +					 AX_RX_CTL_RH2M)
> 
>  #define AX_DEFAULT_RX_CTL	\
>  	(AX_RX_CTL_SO | AX_RX_CTL_AB)
> @@ -426,7 +435,15 @@ static int asix_init(struct eth_device *eth, bd_t *bd)
> 
>  	debug("** %s()\n", __func__);
> 
> -	if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0)
> +	if ((dev->pusb_dev->descriptor.idVendor == 0x0b95) &&
> +	    (dev->pusb_dev->descriptor.idProduct == 0x772b)) {

I don't like hardcoding these constants here (and further down).
I understand that those are AX88792B chips (or whatever the number
is, there's a B at the end and they're not exactly compatible with
the original AX88792), but what about making this a bit more generic?

What I expect is that when AX88792C comes, we'd just add another
if (idVendor == ... ) into this code here with another magic number
and it will become an unmaintainable horror.

Maybe add a function which handles quirks of each revision (B, C, ...)
of the ASIX chip and definitelly define those magic numbers as macros.

> +		if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL |
> +					   AX_RX_HEADER_DEFAULT) < 0)
> +			goto out_err;
> +	} else if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0)
> +		goto out_err;
> +
> +	if (asix_write_hwaddr(eth) < 0)
>  		goto out_err;
> 
>  	do {

[...]

On 2 July 2015 07:50:59 CEST, Marek Vasut <marex@denx.de> wrote:
>On Thursday, July 02, 2015 at 01:04:47 AM, Marcel Ziswiler wrote:
>
>Hi!
>
>[...]
>
>> @@ -64,8 +67,14 @@
>>  	 AX_MEDIUM_AC | AX_MEDIUM_RE)
>> 
>>  /* AX88772 & AX88178 RX_CTL values */
>> +#define AX_RX_CTL_RH2M			0x0200	/* Enable IP header in 
>receive
>> +						   buffer aligned on 32-bit
>> +						   boundary */
>
>The comments need a bit of polishing, though it is not the main problem
>I have
>with this patch.

I was hesitant at first but then decided to submit it anyway to get some feedback on the thematic. So thank you very much!

>The multiline comments should be like this according
>to kernel
>coding style (to my knowledge):
>
>/*
> * foo
> * bar
> * baz
> */

Yeah, sorry. My bad. I since got educated in doing this but stumble over it at times on older patches.

>> +#define AX_RX_CTL_RH1M			0x0100	/* Enable RX-Header mode 
>0 */
>>  #define AX_RX_CTL_SO			0x0080
>>  #define AX_RX_CTL_AB			0x0008
>> +#define AX_RX_HEADER_DEFAULT		(AX_RX_CTL_RH1M | \
>> +					 AX_RX_CTL_RH2M)
>> 
>>  #define AX_DEFAULT_RX_CTL	\
>>  	(AX_RX_CTL_SO | AX_RX_CTL_AB)
>> @@ -426,7 +435,15 @@ static int asix_init(struct eth_device *eth,
>bd_t *bd)
>> 
>>  	debug("** %s()\n", __func__);
>> 
>> -	if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0)
>> +	if ((dev->pusb_dev->descriptor.idVendor == 0x0b95) &&
>> +	    (dev->pusb_dev->descriptor.idProduct == 0x772b)) {
>
>I don't like hardcoding these constants here (and further down).
>I understand that those are AX88792B chips (or whatever the number
>is, there's a B at the end and they're not exactly compatible with
>the original AX88792), but what about making this a bit more generic?

AX88772B actually and yes there seem to be C variants of that same chip out now as well but we haven't gotten our hands on any such yet. I just do remember that ASIX does not take backwards compatibility too serious.

>What I expect is that when AX88792C comes, we'd just add another
>if (idVendor == ... ) into this code here with another magic number
>and it will become an unmaintainable horror.

Understood.

>Maybe add a function which handles quirks of each revision (B, C, ...)
>of the ASIX chip and definitelly define those magic numbers as macros.

Agreed.

>> +		if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL |
>> +					   AX_RX_HEADER_DEFAULT) < 0)
>> +			goto out_err;
>> +	} else if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0)
>> +		goto out_err;
>> +
>> +	if (asix_write_hwaddr(eth) < 0)
>>  		goto out_err;
>> 
>>  	do {
>
>[...]

On Thursday, July 02, 2015 at 08:12:32 AM, Marcel Ziswiler wrote:
> On 2 July 2015 07:50:59 CEST, Marek Vasut <marex@denx.de> wrote:
> >On Thursday, July 02, 2015 at 01:04:47 AM, Marcel Ziswiler wrote:
> >
> >Hi!

Hi!

> >[...]
> >
> >> @@ -64,8 +67,14 @@
> >> 
> >>  	 AX_MEDIUM_AC | AX_MEDIUM_RE)
> >>  
> >>  /* AX88772 & AX88178 RX_CTL values */
> >> 
> >> +#define AX_RX_CTL_RH2M			0x0200	/* Enable IP header in
> >
> >receive
> >
> >> +						   buffer aligned on 32-bit
> >> +						   boundary */
> >
> >The comments need a bit of polishing, though it is not the main problem
> >I have
> >with this patch.
> 
> I was hesitant at first but then decided to submit it anyway to get some
> feedback on the thematic. So thank you very much!

No worries :)

> >The multiline comments should be like this according
> >to kernel
> >coding style (to my knowledge):
> >
> >/*
> >
> > * foo
> > * bar
> > * baz
> > */
> 
> Yeah, sorry. My bad. I since got educated in doing this but stumble over it
> at times on older patches.

Yeah, the code in certain areas of U-Boot isn't the pinacle of coding style
excellence for sure.

> >> +#define AX_RX_CTL_RH1M			0x0100	/* Enable RX-Header mode
> >
> >0 */
> >
> >>  #define AX_RX_CTL_SO			0x0080
> >>  #define AX_RX_CTL_AB			0x0008
> >> 
> >> +#define AX_RX_HEADER_DEFAULT		(AX_RX_CTL_RH1M | \
> >> +					 AX_RX_CTL_RH2M)
> >> 
> >>  #define AX_DEFAULT_RX_CTL	\
> >>  
> >>  	(AX_RX_CTL_SO | AX_RX_CTL_AB)
> >> 
> >> @@ -426,7 +435,15 @@ static int asix_init(struct eth_device *eth,
> >
> >bd_t *bd)
> >
> >>  	debug("** %s()\n", __func__);
> >> 
> >> -	if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0)
> >> +	if ((dev->pusb_dev->descriptor.idVendor == 0x0b95) &&
> >> +	    (dev->pusb_dev->descriptor.idProduct == 0x772b)) {
> >
> >I don't like hardcoding these constants here (and further down).
> >I understand that those are AX88792B chips (or whatever the number
> >is, there's a B at the end and they're not exactly compatible with
> >the original AX88792), but what about making this a bit more generic?
> 
> AX88772B actually and yes there seem to be C variants of that same chip out
> now as well but we haven't gotten our hands on any such yet. I just do
> remember that ASIX does not take backwards compatibility too serious.

Yeah, that I can confirm this.

> >What I expect is that when AX88792C comes, we'd just add another
> >if (idVendor == ... ) into this code here with another magic number
> >and it will become an unmaintainable horror.
> 
> Understood.
> 
> >Maybe add a function which handles quirks of each revision (B, C, ...)
> >of the ASIX chip and definitelly define those magic numbers as macros.
> 
> Agreed.

OK, thanks! :-)

[...]

Hi Marcel,

On Wed, Jul 1, 2015 at 6:04 PM, Marcel Ziswiler <marcel@ziswiler.com> wrote:
> From: Marcel Ziswiler <marcel.ziswiler@toradex.com>
>
> This patch fixes operation of our on-board AX88772B chip without EEPROM
> but with a ethaddr coming from the regular U-Boot environment. This is
> a forward port of some remaining parts initially implemented by
> Antmicro.
>
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> ---
>  drivers/usb/eth/asix.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/eth/asix.c b/drivers/usb/eth/asix.c
> index c8697ae..3926d50 100644
> --- a/drivers/usb/eth/asix.c
> +++ b/drivers/usb/eth/asix.c
> @@ -1,5 +1,8 @@
>  /*
>   * Copyright (c) 2011 The Chromium OS Authors.
> + * Copyright (c) 2012-2014 Toradex, Inc.
> + *
> + * Patched for AX88772B by Antmicro Ltd <www.antmicro.com>
>   *
>   * SPDX-License-Identifier:    GPL-2.0+
>   */
> @@ -64,8 +67,14 @@
>          AX_MEDIUM_AC | AX_MEDIUM_RE)
>
>  /* AX88772 & AX88178 RX_CTL values */
> +#define AX_RX_CTL_RH2M                 0x0200  /* Enable IP header in receive
> +                                                  buffer aligned on 32-bit
> +                                                  boundary */

Definitely use checkpatch.pl on this series. I recommend patman.

> +#define AX_RX_CTL_RH1M                 0x0100  /* Enable RX-Header mode 0 */
>  #define AX_RX_CTL_SO                   0x0080
>  #define AX_RX_CTL_AB                   0x0008
> +#define AX_RX_HEADER_DEFAULT           (AX_RX_CTL_RH1M | \
> +                                        AX_RX_CTL_RH2M)

I'm guessing this is ported from the ASIX Linux driver. Perhaps
mention in the log where it came from.

>
>  #define AX_DEFAULT_RX_CTL      \
>         (AX_RX_CTL_SO | AX_RX_CTL_AB)
> @@ -426,7 +435,15 @@ static int asix_init(struct eth_device *eth, bd_t *bd)
>
>         debug("** %s()\n", __func__);
>
> -       if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0)
> +       if ((dev->pusb_dev->descriptor.idVendor == 0x0b95) &&
> +           (dev->pusb_dev->descriptor.idProduct == 0x772b)) {

Please don't use magic numbers. Also, ideally don't create PID hacks.

> +               if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL |
> +                                          AX_RX_HEADER_DEFAULT) < 0)
> +                       goto out_err;
> +       } else if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0)
> +               goto out_err;
> +
> +       if (asix_write_hwaddr(eth) < 0)
>                 goto out_err;
>
>         do {
> @@ -447,6 +464,10 @@ static int asix_init(struct eth_device *eth, bd_t *bd)
>                 goto out_err;
>         }
>
> +       /* Wait some more to avoid timeout on first transfer
> +          (e.g. EHCI timed out on TD - token=0x8008d80) */
> +       udelay(25000);

Is this always needed or is it only helpful to your device?

> +
>         return 0;
>  out_err:
>         return -1;
> @@ -533,6 +554,10 @@ static int asix_recv(struct eth_device *eth)
>                         return -1;
>                 }
>
> +               if ((dev->pusb_dev->descriptor.idVendor == 0x0b95) &&
> +                   (dev->pusb_dev->descriptor.idProduct == 0x772b))

Again, Avoid PID hacks and at least magic numbers.

> +                       buf_ptr += 2;
> +
>                 /* Notify net stack */
>                 net_process_received_packet(buf_ptr + sizeof(packet_len),
>                                             packet_len);

Thanks,
-Joe

Hi Joe

On 8 July 2015 05:55:28 CEST, Joe Hershberger <joe.hershberger@gmail.com> wrote:

>Definitely use checkpatch.pl on this series.

Yeah, it's been late and I somehow forgot about it. I already run v2 about to be posted through it now.

> I recommend patman.

So far I have not used that one yet but thanks for the tip.


>I'm guessing this is ported from the ASIX Linux driver. Perhaps
>mention in the log where it came from.

As mentioned in the log it actually came from Antmicro who did an initial port way back 2011.

>Please don't use magic numbers.

I already got rid of this in v2.

> Also, ideally don't create PID hacks.

Understood, unfortunately I don't know what the effect of this on other ASIX variants is.

>Is this always needed or is it only helpful to your device?

Good question which posting it here I hoped to get some more feedback about.

Cheers

Marcel