| Message ID | 20160803053254.7077-1-alban.bedel@avionic-design.de |
|---|---|
| State | Superseded |
| Headers | show |
On 08/03/2016 07:32 AM, Alban Bedel wrote: > Commit 147271209a9d ("net: asix: fix operation without eeprom") > added a special handling for ASIX 88772B that enable another > type of header. This break the driver in DM mode as the extra handling > needed in the receive path is missing. So add the extra handling ? > However this new header mode is not required and only seems to > increase the code complexity, so this patch revert this part of > commit 147271209a9d. Why is it not required ? > Change-Id: I7edc89f5714ead60e5204340ba05caf21b155593 > Fixes: 147271209a9d ("net: asix: fix operation without eeprom") > Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> > --- > drivers/usb/eth/asix.c | 22 +++------------------- > 1 file changed, 3 insertions(+), 19 deletions(-) > > diff --git a/drivers/usb/eth/asix.c b/drivers/usb/eth/asix.c > index ad083cf8ae4a..1c6e967db1a0 100644 > --- a/drivers/usb/eth/asix.c > +++ b/drivers/usb/eth/asix.c > @@ -67,11 +67,8 @@ > AX_MEDIUM_AC | AX_MEDIUM_RE) > > /* AX88772 & AX88178 RX_CTL values */ > -#define AX_RX_CTL_RH2M 0x0200 /* 32-bit aligned RX IP header */ > -#define AX_RX_CTL_RH1M 0x0100 /* Enable RX header format type 1 */ > -#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_RX_CTL_SO 0x0080 > +#define AX_RX_CTL_AB 0x0008 > > #define AX_DEFAULT_RX_CTL \ > (AX_RX_CTL_SO | AX_RX_CTL_AB) > @@ -98,8 +95,6 @@ > #define FLAG_TYPE_AX88772B (1U << 2) > #define FLAG_EEPROM_MAC (1U << 3) /* initial mac address in eeprom */ > > -#define ASIX_USB_VENDOR_ID 0x0b95 > -#define AX88772B_USB_PRODUCT_ID 0x772b > > /* driver private */ > struct asix_private { > @@ -431,15 +426,10 @@ static int asix_init_common(struct ueth_data *dev, uint8_t *enetaddr) > int timeout = 0; > #define TIMEOUT_RESOLUTION 50 /* ms */ > int link_detected; > - u32 ctl = AX_DEFAULT_RX_CTL; > > debug("** %s()\n", __func__); > > - if ((dev->pusb_dev->descriptor.idVendor == ASIX_USB_VENDOR_ID) && > - (dev->pusb_dev->descriptor.idProduct == AX88772B_USB_PRODUCT_ID)) > - ctl |= AX_RX_HEADER_DEFAULT; > - > - if (asix_write_rx_ctl(dev, ctl) < 0) > + if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0) > goto out_err; > > if (asix_write_hwaddr_common(dev, enetaddr) < 0) > @@ -572,12 +562,6 @@ static int asix_recv(struct eth_device *eth) > return -1; > } > > - if ((dev->pusb_dev->descriptor.idVendor == > - ASIX_USB_VENDOR_ID) && > - (dev->pusb_dev->descriptor.idProduct == > - AX88772B_USB_PRODUCT_ID)) > - buf_ptr += 2; > - > /* Notify net stack */ > net_process_received_packet(buf_ptr + sizeof(packet_len), > packet_len); >
On Wed, 3 Aug 2016 09:00:42 +0200 Marek Vasut <marex@denx.de> wrote: > On 08/03/2016 07:32 AM, Alban Bedel wrote: > > Commit 147271209a9d ("net: asix: fix operation without eeprom") > > added a special handling for ASIX 88772B that enable another > > type of header. This break the driver in DM mode as the extra handling > > needed in the receive path is missing. > > So add the extra handling ? I can do that too, but I though u-boot preferred to avoid useless code. > > However this new header mode is not required and only seems to > > increase the code complexity, so this patch revert this part of > > commit 147271209a9d. > > Why is it not required ? It works fine without, since 2012. In fact this change is not even mentioned in the log of commit 147271209a9d, so I really don't know why it was added in the first place. As can be seen in the revert all it does is adding 2 bytes to the USB packets that are then just skipped. Seems pretty useless to me. Alban
On 08/03/2016 11:46 AM, Alban Bedel wrote: > On Wed, 3 Aug 2016 09:00:42 +0200 > Marek Vasut <marex@denx.de> wrote: > >> On 08/03/2016 07:32 AM, Alban Bedel wrote: >>> Commit 147271209a9d ("net: asix: fix operation without eeprom") >>> added a special handling for ASIX 88772B that enable another >>> type of header. This break the driver in DM mode as the extra handling >>> needed in the receive path is missing. >> >> So add the extra handling ? > > I can do that too, but I though u-boot preferred to avoid useless code. Yes, if it is useless. >>> However this new header mode is not required and only seems to >>> increase the code complexity, so this patch revert this part of >>> commit 147271209a9d. >> >> Why is it not required ? > > It works fine without, since 2012. In fact this change is not even > mentioned in the log of commit 147271209a9d, so I really don't know why > it was added in the first place. As can be seen in the revert all it > does is adding 2 bytes to the USB packets that are then just skipped. > Seems pretty useless to me. I would like to get some feedback on this from Marcel, since he added this stuff. > Alban >
On Wed, 2016-08-03 at 15:51 +0200, Marek Vasut wrote: > On 08/03/2016 11:46 AM, Alban Bedel wrote: > > > > On Wed, 3 Aug 2016 09:00:42 +0200 > > Marek Vasut <marex@denx.de> wrote: > > > > > > > > On 08/03/2016 07:32 AM, Alban Bedel wrote: > > > > > > > > Commit 147271209a9d ("net: asix: fix operation without eeprom") > > > > added a special handling for ASIX 88772B that enable another > > > > type of header. This break the driver in DM mode as the extra > > > > handling > > > > needed in the receive path is missing. > > > So add the extra handling ? > > I can do that too, but I though u-boot preferred to avoid useless > > code. > Yes, if it is useless. > > > > > > > > > > > > > > However this new header mode is not required and only seems to > > > > increase the code complexity, so this patch revert this part of > > > > commit 147271209a9d. > > > Why is it not required ? > > It works fine without, since 2012. In fact this change is not even > > mentioned in the log of commit 147271209a9d, so I really don't know > > why > > it was added in the first place. As can be seen in the revert all > > it > > does is adding 2 bytes to the USB packets that are then just > > skipped. > > Seems pretty useless to me. > I would like to get some feedback on this from Marcel, since he added > this stuff. Yes, sorry. I just came back from vacation and started looking into it now. As far as I remember on our hardware without this Ethernet did not quite work reliably. This also means that with driver model so far it does not work for us which I fed back to Simon once but so far this has not been resolved. That fix came from some early U-Boot work done by Antmicro way back and I am missing some of the history. Cheers Marcel > > Alban
On Wed, 3 Aug 2016 15:23:30 +0000 Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote: > On Wed, 2016-08-03 at 15:51 +0200, Marek Vasut wrote: > > On 08/03/2016 11:46 AM, Alban Bedel wrote: > > > > > > On Wed, 3 Aug 2016 09:00:42 +0200 > > > Marek Vasut <marex@denx.de> wrote: > > > > > > > > > > > On 08/03/2016 07:32 AM, Alban Bedel wrote: > > > > > > > > > > Commit 147271209a9d ("net: asix: fix operation without eeprom") > > > > > added a special handling for ASIX 88772B that enable another > > > > > type of header. This break the driver in DM mode as the extra > > > > > handling > > > > > needed in the receive path is missing. > > > > So add the extra handling ? > > > I can do that too, but I though u-boot preferred to avoid useless > > > code. > > Yes, if it is useless. > > > > > > > > > > > > > > > > > > > However this new header mode is not required and only seems to > > > > > increase the code complexity, so this patch revert this part of > > > > > commit 147271209a9d. > > > > Why is it not required ? > > > It works fine without, since 2012. In fact this change is not even > > > mentioned in the log of commit 147271209a9d, so I really don't know > > > why > > > it was added in the first place. As can be seen in the revert all > > > it > > > does is adding 2 bytes to the USB packets that are then just > > > skipped. > > > Seems pretty useless to me. > > I would like to get some feedback on this from Marcel, since he added > > this stuff. > > Yes, sorry. I just came back from vacation and started looking into it > now. As far as I remember on our hardware without this Ethernet did not > quite work reliably. This also means that with driver model so far it > does not work for us which I fed back to Simon once but so far this has > not been resolved. That fix came from some early U-Boot work done by > Antmicro way back and I am missing some of the history. Then I'll do a new patch that just fix the driver model receive path. Alban
On 08/04/2016 11:07 AM, Alban Bedel wrote: > On Wed, 3 Aug 2016 15:23:30 +0000 > Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote: > >> On Wed, 2016-08-03 at 15:51 +0200, Marek Vasut wrote: >>> On 08/03/2016 11:46 AM, Alban Bedel wrote: >>>> >>>> On Wed, 3 Aug 2016 09:00:42 +0200 >>>> Marek Vasut <marex@denx.de> wrote: >>>> >>>>> >>>>> On 08/03/2016 07:32 AM, Alban Bedel wrote: >>>>>> >>>>>> Commit 147271209a9d ("net: asix: fix operation without eeprom") >>>>>> added a special handling for ASIX 88772B that enable another >>>>>> type of header. This break the driver in DM mode as the extra >>>>>> handling >>>>>> needed in the receive path is missing. >>>>> So add the extra handling ? >>>> I can do that too, but I though u-boot preferred to avoid useless >>>> code. >>> Yes, if it is useless. >>> >>>> >>>>> >>>>>> >>>>>> However this new header mode is not required and only seems to >>>>>> increase the code complexity, so this patch revert this part of >>>>>> commit 147271209a9d. >>>>> Why is it not required ? >>>> It works fine without, since 2012. In fact this change is not even >>>> mentioned in the log of commit 147271209a9d, so I really don't know >>>> why >>>> it was added in the first place. As can be seen in the revert all >>>> it >>>> does is adding 2 bytes to the USB packets that are then just >>>> skipped. >>>> Seems pretty useless to me. >>> I would like to get some feedback on this from Marcel, since he added >>> this stuff. >> >> Yes, sorry. I just came back from vacation and started looking into it >> now. As far as I remember on our hardware without this Ethernet did not >> quite work reliably. This also means that with driver model so far it >> does not work for us which I fed back to Simon once but so far this has >> not been resolved. That fix came from some early U-Boot work done by >> Antmicro way back and I am missing some of the history. > > Then I'll do a new patch that just fix the driver model receive path. Hold on. Marcel, can you maybe test if removing this code has any impact on the behavior now ?
On Thu, 2016-08-04 at 11:12 +0200, Marek Vasut wrote: > On 08/04/2016 11:07 AM, Alban Bedel wrote: > > > > On Wed, 3 Aug 2016 15:23:30 +0000 > > Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote: > > > > > > > > On Wed, 2016-08-03 at 15:51 +0200, Marek Vasut wrote: > > > > > > > > On 08/03/2016 11:46 AM, Alban Bedel wrote: > > > > > > > > > > > > > > > On Wed, 3 Aug 2016 09:00:42 +0200 > > > > > Marek Vasut <marex@denx.de> wrote: > > > > > > > > > > > > > > > > > > > > > > > On 08/03/2016 07:32 AM, Alban Bedel wrote: > > > > > > > > > > > > > > > > > > > > > Commit 147271209a9d ("net: asix: fix operation without > > > > > > > eeprom") > > > > > > > added a special handling for ASIX 88772B that enable > > > > > > > another > > > > > > > type of header. This break the driver in DM mode as the > > > > > > > extra > > > > > > > handling > > > > > > > needed in the receive path is missing. > > > > > > So add the extra handling ? > > > > > I can do that too, but I though u-boot preferred to avoid > > > > > useless > > > > > code. > > > > Yes, if it is useless. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > However this new header mode is not required and only > > > > > > > seems to > > > > > > > increase the code complexity, so this patch revert this > > > > > > > part of > > > > > > > commit 147271209a9d. > > > > > > Why is it not required ? > > > > > It works fine without, since 2012. In fact this change is not > > > > > even > > > > > mentioned in the log of commit 147271209a9d, so I really > > > > > don't know > > > > > why > > > > > it was added in the first place. As can be seen in the revert > > > > > all > > > > > it > > > > > does is adding 2 bytes to the USB packets that are then just > > > > > skipped. > > > > > Seems pretty useless to me. > > > > I would like to get some feedback on this from Marcel, since he > > > > added > > > > this stuff. > > > Yes, sorry. I just came back from vacation and started looking > > > into it > > > now. As far as I remember on our hardware without this Ethernet > > > did not > > > quite work reliably. This also means that with driver model so > > > far it > > > does not work for us which I fed back to Simon once but so far > > > this has > > > not been resolved. That fix came from some early U-Boot work done > > > by > > > Antmicro way back and I am missing some of the history. > > Then I'll do a new patch that just fix the driver model receive > > path. > Hold on. Marcel, can you maybe test if removing this code has any > impact > on the behavior now ? Sorry for the delay. I tested Alban's patch now both on Toradex Colibri T20 as well as T30 and its on-module ASIX USB-to-Ethernet chip actually works perfectly aside from the occasional EHCI timed out on TD - token=0x88008d80 Rx: failed to receive: -5 message which last I checked with Simon is still unresolved but was already there long before any of the driver model work started. Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> Tested-on: Colibri T20/T30 on Colibri Evaluation Board
On 08/09/2016 02:14 PM, Marcel Ziswiler wrote: > On Thu, 2016-08-04 at 11:12 +0200, Marek Vasut wrote: >> On 08/04/2016 11:07 AM, Alban Bedel wrote: >>> >>> On Wed, 3 Aug 2016 15:23:30 +0000 >>> Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote: >>> >>>> >>>> On Wed, 2016-08-03 at 15:51 +0200, Marek Vasut wrote: >>>>> >>>>> On 08/03/2016 11:46 AM, Alban Bedel wrote: >>>>>> >>>>>> >>>>>> On Wed, 3 Aug 2016 09:00:42 +0200 >>>>>> Marek Vasut <marex@denx.de> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On 08/03/2016 07:32 AM, Alban Bedel wrote: >>>>>>>> >>>>>>>> >>>>>>>> Commit 147271209a9d ("net: asix: fix operation without >>>>>>>> eeprom") >>>>>>>> added a special handling for ASIX 88772B that enable >>>>>>>> another >>>>>>>> type of header. This break the driver in DM mode as the >>>>>>>> extra >>>>>>>> handling >>>>>>>> needed in the receive path is missing. >>>>>>> So add the extra handling ? >>>>>> I can do that too, but I though u-boot preferred to avoid >>>>>> useless >>>>>> code. >>>>> Yes, if it is useless. >>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> However this new header mode is not required and only >>>>>>>> seems to >>>>>>>> increase the code complexity, so this patch revert this >>>>>>>> part of >>>>>>>> commit 147271209a9d. >>>>>>> Why is it not required ? >>>>>> It works fine without, since 2012. In fact this change is not >>>>>> even >>>>>> mentioned in the log of commit 147271209a9d, so I really >>>>>> don't know >>>>>> why >>>>>> it was added in the first place. As can be seen in the revert >>>>>> all >>>>>> it >>>>>> does is adding 2 bytes to the USB packets that are then just >>>>>> skipped. >>>>>> Seems pretty useless to me. >>>>> I would like to get some feedback on this from Marcel, since he >>>>> added >>>>> this stuff. >>>> Yes, sorry. I just came back from vacation and started looking >>>> into it >>>> now. As far as I remember on our hardware without this Ethernet >>>> did not >>>> quite work reliably. This also means that with driver model so >>>> far it >>>> does not work for us which I fed back to Simon once but so far >>>> this has >>>> not been resolved. That fix came from some early U-Boot work done >>>> by >>>> Antmicro way back and I am missing some of the history. >>> Then I'll do a new patch that just fix the driver model receive >>> path. >> Hold on. Marcel, can you maybe test if removing this code has any >> impact >> on the behavior now ? > > Sorry for the delay. I tested Alban's patch now both on Toradex Colibri > T20 as well as T30 and its on-module ASIX USB-to-Ethernet chip actually > works perfectly aside from the occasional EHCI timed out on TD - > token=0x88008d80 Rx: failed to receive: -5 message which last I checked > with Simon is still unresolved but was already there long before any of > the driver model work started. > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > Tested-on: Colibri T20/T30 on Colibri Evaluation Board > Thanks!
On Tue, 9 Aug 2016 14:32:14 +0200 Marek Vasut <marex@denx.de> wrote: > On 08/09/2016 02:14 PM, Marcel Ziswiler wrote: > > On Thu, 2016-08-04 at 11:12 +0200, Marek Vasut wrote: > >> On 08/04/2016 11:07 AM, Alban Bedel wrote: > >>> > >>> On Wed, 3 Aug 2016 15:23:30 +0000 > >>> Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote: > >>> > >>>> > >>>> On Wed, 2016-08-03 at 15:51 +0200, Marek Vasut wrote: > >>>>> > >>>>> On 08/03/2016 11:46 AM, Alban Bedel wrote: > >>>>>> > >>>>>> > >>>>>> On Wed, 3 Aug 2016 09:00:42 +0200 > >>>>>> Marek Vasut <marex@denx.de> wrote: > >>>>>> > >>>>>>> > >>>>>>> > >>>>>>> On 08/03/2016 07:32 AM, Alban Bedel wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> Commit 147271209a9d ("net: asix: fix operation without > >>>>>>>> eeprom") > >>>>>>>> added a special handling for ASIX 88772B that enable > >>>>>>>> another > >>>>>>>> type of header. This break the driver in DM mode as the > >>>>>>>> extra > >>>>>>>> handling > >>>>>>>> needed in the receive path is missing. > >>>>>>> So add the extra handling ? > >>>>>> I can do that too, but I though u-boot preferred to avoid > >>>>>> useless > >>>>>> code. > >>>>> Yes, if it is useless. > >>>>> > >>>>>> > >>>>>> > >>>>>>> > >>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>> However this new header mode is not required and only > >>>>>>>> seems to > >>>>>>>> increase the code complexity, so this patch revert this > >>>>>>>> part of > >>>>>>>> commit 147271209a9d. > >>>>>>> Why is it not required ? > >>>>>> It works fine without, since 2012. In fact this change is not > >>>>>> even > >>>>>> mentioned in the log of commit 147271209a9d, so I really > >>>>>> don't know > >>>>>> why > >>>>>> it was added in the first place. As can be seen in the revert > >>>>>> all > >>>>>> it > >>>>>> does is adding 2 bytes to the USB packets that are then just > >>>>>> skipped. > >>>>>> Seems pretty useless to me. > >>>>> I would like to get some feedback on this from Marcel, since he > >>>>> added > >>>>> this stuff. > >>>> Yes, sorry. I just came back from vacation and started looking > >>>> into it > >>>> now. As far as I remember on our hardware without this Ethernet > >>>> did not > >>>> quite work reliably. This also means that with driver model so > >>>> far it > >>>> does not work for us which I fed back to Simon once but so far > >>>> this has > >>>> not been resolved. That fix came from some early U-Boot work done > >>>> by > >>>> Antmicro way back and I am missing some of the history. > >>> Then I'll do a new patch that just fix the driver model receive > >>> path. > >> Hold on. Marcel, can you maybe test if removing this code has any > >> impact > >> on the behavior now ? > > > > Sorry for the delay. I tested Alban's patch now both on Toradex Colibri > > T20 as well as T30 and its on-module ASIX USB-to-Ethernet chip actually > > works perfectly aside from the occasional EHCI timed out on TD - > > token=0x88008d80 Rx: failed to receive: -5 message which last I checked > > with Simon is still unresolved but was already there long before any of > > the driver model work started. > > > > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > > Tested-on: Colibri T20/T30 on Colibri Evaluation Board > > Will this be applied for the upcoming release? Alban
On 08/11/2016 10:52 AM, Alban Bedel wrote: > On Tue, 9 Aug 2016 14:32:14 +0200 > Marek Vasut <marex@denx.de> wrote: > >> On 08/09/2016 02:14 PM, Marcel Ziswiler wrote: >>> On Thu, 2016-08-04 at 11:12 +0200, Marek Vasut wrote: >>>> On 08/04/2016 11:07 AM, Alban Bedel wrote: >>>>> >>>>> On Wed, 3 Aug 2016 15:23:30 +0000 >>>>> Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote: >>>>> >>>>>> >>>>>> On Wed, 2016-08-03 at 15:51 +0200, Marek Vasut wrote: >>>>>>> >>>>>>> On 08/03/2016 11:46 AM, Alban Bedel wrote: >>>>>>>> >>>>>>>> >>>>>>>> On Wed, 3 Aug 2016 09:00:42 +0200 >>>>>>>> Marek Vasut <marex@denx.de> wrote: >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On 08/03/2016 07:32 AM, Alban Bedel wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Commit 147271209a9d ("net: asix: fix operation without >>>>>>>>>> eeprom") >>>>>>>>>> added a special handling for ASIX 88772B that enable >>>>>>>>>> another >>>>>>>>>> type of header. This break the driver in DM mode as the >>>>>>>>>> extra >>>>>>>>>> handling >>>>>>>>>> needed in the receive path is missing. >>>>>>>>> So add the extra handling ? >>>>>>>> I can do that too, but I though u-boot preferred to avoid >>>>>>>> useless >>>>>>>> code. >>>>>>> Yes, if it is useless. >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> However this new header mode is not required and only >>>>>>>>>> seems to >>>>>>>>>> increase the code complexity, so this patch revert this >>>>>>>>>> part of >>>>>>>>>> commit 147271209a9d. >>>>>>>>> Why is it not required ? >>>>>>>> It works fine without, since 2012. In fact this change is not >>>>>>>> even >>>>>>>> mentioned in the log of commit 147271209a9d, so I really >>>>>>>> don't know >>>>>>>> why >>>>>>>> it was added in the first place. As can be seen in the revert >>>>>>>> all >>>>>>>> it >>>>>>>> does is adding 2 bytes to the USB packets that are then just >>>>>>>> skipped. >>>>>>>> Seems pretty useless to me. >>>>>>> I would like to get some feedback on this from Marcel, since he >>>>>>> added >>>>>>> this stuff. >>>>>> Yes, sorry. I just came back from vacation and started looking >>>>>> into it >>>>>> now. As far as I remember on our hardware without this Ethernet >>>>>> did not >>>>>> quite work reliably. This also means that with driver model so >>>>>> far it >>>>>> does not work for us which I fed back to Simon once but so far >>>>>> this has >>>>>> not been resolved. That fix came from some early U-Boot work done >>>>>> by >>>>>> Antmicro way back and I am missing some of the history. >>>>> Then I'll do a new patch that just fix the driver model receive >>>>> path. >>>> Hold on. Marcel, can you maybe test if removing this code has any >>>> impact >>>> on the behavior now ? >>> >>> Sorry for the delay. I tested Alban's patch now both on Toradex Colibri >>> T20 as well as T30 and its on-module ASIX USB-to-Ethernet chip actually >>> works perfectly aside from the occasional EHCI timed out on TD - >>> token=0x88008d80 Rx: failed to receive: -5 message which last I checked >>> with Simon is still unresolved but was already there long before any of >>> the driver model work started. >>> >>> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> >>> Tested-on: Colibri T20/T30 on Colibri Evaluation Board >>> > > Will this be applied for the upcoming release? Yeah. Why the hurry though ?
On Thu, 11 Aug 2016 11:26:23 +0200 Marek Vasut <marex@denx.de> wrote: > On 08/11/2016 10:52 AM, Alban Bedel wrote: > > On Tue, 9 Aug 2016 14:32:14 +0200 > > Marek Vasut <marex@denx.de> wrote: > > > >> On 08/09/2016 02:14 PM, Marcel Ziswiler wrote: > >>> On Thu, 2016-08-04 at 11:12 +0200, Marek Vasut wrote: > >>>> On 08/04/2016 11:07 AM, Alban Bedel wrote: > >>>>> > >>>>> On Wed, 3 Aug 2016 15:23:30 +0000 > >>>>> Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote: > >>>>> > >>>>>> > >>>>>> On Wed, 2016-08-03 at 15:51 +0200, Marek Vasut wrote: > >>>>>>> > >>>>>>> On 08/03/2016 11:46 AM, Alban Bedel wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>> On Wed, 3 Aug 2016 09:00:42 +0200 > >>>>>>>> Marek Vasut <marex@denx.de> wrote: > >>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On 08/03/2016 07:32 AM, Alban Bedel wrote: > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> Commit 147271209a9d ("net: asix: fix operation without > >>>>>>>>>> eeprom") > >>>>>>>>>> added a special handling for ASIX 88772B that enable > >>>>>>>>>> another > >>>>>>>>>> type of header. This break the driver in DM mode as the > >>>>>>>>>> extra > >>>>>>>>>> handling > >>>>>>>>>> needed in the receive path is missing. > >>>>>>>>> So add the extra handling ? > >>>>>>>> I can do that too, but I though u-boot preferred to avoid > >>>>>>>> useless > >>>>>>>> code. > >>>>>>> Yes, if it is useless. > >>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> However this new header mode is not required and only > >>>>>>>>>> seems to > >>>>>>>>>> increase the code complexity, so this patch revert this > >>>>>>>>>> part of > >>>>>>>>>> commit 147271209a9d. > >>>>>>>>> Why is it not required ? > >>>>>>>> It works fine without, since 2012. In fact this change is not > >>>>>>>> even > >>>>>>>> mentioned in the log of commit 147271209a9d, so I really > >>>>>>>> don't know > >>>>>>>> why > >>>>>>>> it was added in the first place. As can be seen in the revert > >>>>>>>> all > >>>>>>>> it > >>>>>>>> does is adding 2 bytes to the USB packets that are then just > >>>>>>>> skipped. > >>>>>>>> Seems pretty useless to me. > >>>>>>> I would like to get some feedback on this from Marcel, since he > >>>>>>> added > >>>>>>> this stuff. > >>>>>> Yes, sorry. I just came back from vacation and started looking > >>>>>> into it > >>>>>> now. As far as I remember on our hardware without this Ethernet > >>>>>> did not > >>>>>> quite work reliably. This also means that with driver model so > >>>>>> far it > >>>>>> does not work for us which I fed back to Simon once but so far > >>>>>> this has > >>>>>> not been resolved. That fix came from some early U-Boot work done > >>>>>> by > >>>>>> Antmicro way back and I am missing some of the history. > >>>>> Then I'll do a new patch that just fix the driver model receive > >>>>> path. > >>>> Hold on. Marcel, can you maybe test if removing this code has any > >>>> impact > >>>> on the behavior now ? > >>> > >>> Sorry for the delay. I tested Alban's patch now both on Toradex Colibri > >>> T20 as well as T30 and its on-module ASIX USB-to-Ethernet chip actually > >>> works perfectly aside from the occasional EHCI timed out on TD - > >>> token=0x88008d80 Rx: failed to receive: -5 message which last I checked > >>> with Simon is still unresolved but was already there long before any of > >>> the driver model work started. > >>> > >>> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> > >>> Tested-on: Colibri T20/T30 on Colibri Evaluation Board > >>> > > > > Will this be applied for the upcoming release? > > Yeah. Why the hurry though ? I was just wondering because all the other patches I submitted have been applied but this one still seems to be on hold. Alban
On 08/11/2016 12:28 PM, Alban Bedel wrote: > On Thu, 11 Aug 2016 11:26:23 +0200 > Marek Vasut <marex@denx.de> wrote: > >> On 08/11/2016 10:52 AM, Alban Bedel wrote: >>> On Tue, 9 Aug 2016 14:32:14 +0200 >>> Marek Vasut <marex@denx.de> wrote: >>> >>>> On 08/09/2016 02:14 PM, Marcel Ziswiler wrote: >>>>> On Thu, 2016-08-04 at 11:12 +0200, Marek Vasut wrote: >>>>>> On 08/04/2016 11:07 AM, Alban Bedel wrote: >>>>>>> >>>>>>> On Wed, 3 Aug 2016 15:23:30 +0000 >>>>>>> Marcel Ziswiler <marcel.ziswiler@toradex.com> wrote: >>>>>>> >>>>>>>> >>>>>>>> On Wed, 2016-08-03 at 15:51 +0200, Marek Vasut wrote: >>>>>>>>> >>>>>>>>> On 08/03/2016 11:46 AM, Alban Bedel wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Wed, 3 Aug 2016 09:00:42 +0200 >>>>>>>>>> Marek Vasut <marex@denx.de> wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On 08/03/2016 07:32 AM, Alban Bedel wrote: >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Commit 147271209a9d ("net: asix: fix operation without >>>>>>>>>>>> eeprom") >>>>>>>>>>>> added a special handling for ASIX 88772B that enable >>>>>>>>>>>> another >>>>>>>>>>>> type of header. This break the driver in DM mode as the >>>>>>>>>>>> extra >>>>>>>>>>>> handling >>>>>>>>>>>> needed in the receive path is missing. >>>>>>>>>>> So add the extra handling ? >>>>>>>>>> I can do that too, but I though u-boot preferred to avoid >>>>>>>>>> useless >>>>>>>>>> code. >>>>>>>>> Yes, if it is useless. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> However this new header mode is not required and only >>>>>>>>>>>> seems to >>>>>>>>>>>> increase the code complexity, so this patch revert this >>>>>>>>>>>> part of >>>>>>>>>>>> commit 147271209a9d. >>>>>>>>>>> Why is it not required ? >>>>>>>>>> It works fine without, since 2012. In fact this change is not >>>>>>>>>> even >>>>>>>>>> mentioned in the log of commit 147271209a9d, so I really >>>>>>>>>> don't know >>>>>>>>>> why >>>>>>>>>> it was added in the first place. As can be seen in the revert >>>>>>>>>> all >>>>>>>>>> it >>>>>>>>>> does is adding 2 bytes to the USB packets that are then just >>>>>>>>>> skipped. >>>>>>>>>> Seems pretty useless to me. >>>>>>>>> I would like to get some feedback on this from Marcel, since he >>>>>>>>> added >>>>>>>>> this stuff. >>>>>>>> Yes, sorry. I just came back from vacation and started looking >>>>>>>> into it >>>>>>>> now. As far as I remember on our hardware without this Ethernet >>>>>>>> did not >>>>>>>> quite work reliably. This also means that with driver model so >>>>>>>> far it >>>>>>>> does not work for us which I fed back to Simon once but so far >>>>>>>> this has >>>>>>>> not been resolved. That fix came from some early U-Boot work done >>>>>>>> by >>>>>>>> Antmicro way back and I am missing some of the history. >>>>>>> Then I'll do a new patch that just fix the driver model receive >>>>>>> path. >>>>>> Hold on. Marcel, can you maybe test if removing this code has any >>>>>> impact >>>>>> on the behavior now ? >>>>> >>>>> Sorry for the delay. I tested Alban's patch now both on Toradex Colibri >>>>> T20 as well as T30 and its on-module ASIX USB-to-Ethernet chip actually >>>>> works perfectly aside from the occasional EHCI timed out on TD - >>>>> token=0x88008d80 Rx: failed to receive: -5 message which last I checked >>>>> with Simon is still unresolved but was already there long before any of >>>>> the driver model work started. >>>>> >>>>> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> >>>>> Tested-on: Colibri T20/T30 on Colibri Evaluation Board >>>>> >>> >>> Will this be applied for the upcoming release? >> >> Yeah. Why the hurry though ? > > I was just wondering because all the other patches I submitted have > been applied but this one still seems to be on hold. Well because this one was broken, so I had to throw it away. Please do make sure next time that the stuff builds using buildman.
diff --git a/drivers/usb/eth/asix.c b/drivers/usb/eth/asix.c index ad083cf8ae4a..1c6e967db1a0 100644 --- a/drivers/usb/eth/asix.c +++ b/drivers/usb/eth/asix.c @@ -67,11 +67,8 @@ AX_MEDIUM_AC | AX_MEDIUM_RE) /* AX88772 & AX88178 RX_CTL values */ -#define AX_RX_CTL_RH2M 0x0200 /* 32-bit aligned RX IP header */ -#define AX_RX_CTL_RH1M 0x0100 /* Enable RX header format type 1 */ -#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_RX_CTL_SO 0x0080 +#define AX_RX_CTL_AB 0x0008 #define AX_DEFAULT_RX_CTL \ (AX_RX_CTL_SO | AX_RX_CTL_AB) @@ -98,8 +95,6 @@ #define FLAG_TYPE_AX88772B (1U << 2) #define FLAG_EEPROM_MAC (1U << 3) /* initial mac address in eeprom */ -#define ASIX_USB_VENDOR_ID 0x0b95 -#define AX88772B_USB_PRODUCT_ID 0x772b /* driver private */ struct asix_private { @@ -431,15 +426,10 @@ static int asix_init_common(struct ueth_data *dev, uint8_t *enetaddr) int timeout = 0; #define TIMEOUT_RESOLUTION 50 /* ms */ int link_detected; - u32 ctl = AX_DEFAULT_RX_CTL; debug("** %s()\n", __func__); - if ((dev->pusb_dev->descriptor.idVendor == ASIX_USB_VENDOR_ID) && - (dev->pusb_dev->descriptor.idProduct == AX88772B_USB_PRODUCT_ID)) - ctl |= AX_RX_HEADER_DEFAULT; - - if (asix_write_rx_ctl(dev, ctl) < 0) + if (asix_write_rx_ctl(dev, AX_DEFAULT_RX_CTL) < 0) goto out_err; if (asix_write_hwaddr_common(dev, enetaddr) < 0) @@ -572,12 +562,6 @@ static int asix_recv(struct eth_device *eth) return -1; } - if ((dev->pusb_dev->descriptor.idVendor == - ASIX_USB_VENDOR_ID) && - (dev->pusb_dev->descriptor.idProduct == - AX88772B_USB_PRODUCT_ID)) - buf_ptr += 2; - /* Notify net stack */ net_process_received_packet(buf_ptr + sizeof(packet_len), packet_len);
Commit 147271209a9d ("net: asix: fix operation without eeprom") added a special handling for ASIX 88772B that enable another type of header. This break the driver in DM mode as the extra handling needed in the receive path is missing. However this new header mode is not required and only seems to increase the code complexity, so this patch revert this part of commit 147271209a9d. Change-Id: I7edc89f5714ead60e5204340ba05caf21b155593 Fixes: 147271209a9d ("net: asix: fix operation without eeprom") Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> --- drivers/usb/eth/asix.c | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-)