| Message ID | 20220905005544.994036-1-aahringo@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,wpan-next] atusb: add support for trac feature | expand |
Hi Alexander, aahringo@redhat.com wrote on Sun, 4 Sep 2022 20:55:44 -0400: > This patch adds support for reading the trac register if atusb firmware > reports tx done. There is currently a feature to compare a sequence > number, if the payload is 1 it tells the driver only the sequence number > is available if it's two there is additional the trac status register as > payload. > > Currently the atusb_in_good() function determines if it's a tx done or > rx done if according the payload length. This patch is doing the same > and assumes this behaviour. > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > --- > > Just an RFC, need another weekend to test it. > > drivers/net/ieee802154/atusb.c | 33 ++++++++++++++++++++++++++++----- > 1 file changed, 28 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > index 2c338783893d..95a4a3cdc8a4 100644 > --- a/drivers/net/ieee802154/atusb.c > +++ b/drivers/net/ieee802154/atusb.c > @@ -191,7 +191,7 @@ static void atusb_work_urbs(struct work_struct *work) > > /* ----- Asynchronous USB -------------------------------------------------- */ > > -static void atusb_tx_done(struct atusb *atusb, u8 seq) > +static void atusb_tx_done(struct atusb *atusb, u8 seq, int reason) > { > struct usb_device *usb_dev = atusb->usb_dev; > u8 expect = atusb->tx_ack_seq; > @@ -199,7 +199,10 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > dev_dbg(&usb_dev->dev, "%s (0x%02x/0x%02x)\n", __func__, seq, expect); > if (seq == expect) { > /* TODO check for ifs handling in firmware */ > - ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); > + if (reason == IEEE802154_SUCCESS) > + ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); > + else > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, reason); > } else { > /* TODO I experience this case when atusb has a tx complete > * irq before probing, we should fix the firmware it's an > @@ -215,7 +218,8 @@ static void atusb_in_good(struct urb *urb) > struct usb_device *usb_dev = urb->dev; > struct sk_buff *skb = urb->context; > struct atusb *atusb = SKB_ATUSB(skb); > - u8 len, lqi; > + int result = IEEE802154_SUCCESS; > + u8 len, lqi, trac; > > if (!urb->actual_length) { > dev_dbg(&usb_dev->dev, "atusb_in: zero-sized URB ?\n"); > @@ -224,8 +228,27 @@ static void atusb_in_good(struct urb *urb) > > len = *skb->data; > > - if (urb->actual_length == 1) { > - atusb_tx_done(atusb, len); > + switch (urb->actual_length) { > + case 2: > + trac = TRAC_MASK(*(skb->data + 1)); I've been fighting all night thinking the issues were on the atusb side (it was horribly difficult to get the atusb toolchain up and running, I'll send a patch to update the instructions), in particular because of the data[2] definition which needed to be declared static outside of the functions (see the other mail) and, I guess, because of this beginner error: I was using skb->data[1] but of course it can't work. Anyway, this patch works, I've tested it by: - associating a device (an Arduino Nano running Zephyr, btw) - having the atusb disassociating the nano successfully (trac = 0) - reassociating the nano - powering off the nano - trying to disassociate The disassociation notification transmission fails with a TRAC status NO_ACK. So: Tested-by: Miquel Raynal <miquel.raynal@bootlin.com> I still have the filtering thing to improve, I'm not sure I'll have time to do that this week. I need some time to prepare the slides now that the demo works :) > + switch (trac) { > + case TRAC_SUCCESS: > + case TRAC_SUCCESS_DATA_PENDING: > + /* already IEEE802154_SUCCESS */ > + break; > + case TRAC_CHANNEL_ACCESS_FAILURE: > + result = IEEE802154_CHANNEL_ACCESS_FAILURE; > + break; > + case TRAC_NO_ACK: > + result = IEEE802154_NO_ACK; > + break; > + default: > + result = IEEE802154_SYSTEM_ERROR; > + } > + > + fallthrough; > + case 1: > + atusb_tx_done(atusb, len, result); > return; > } > Thanks, Miquèl
Hi, On Sun, Sep 4, 2022 at 11:22 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Sun, 4 Sep 2022 20:55:44 -0400: > > > This patch adds support for reading the trac register if atusb firmware > > reports tx done. There is currently a feature to compare a sequence > > number, if the payload is 1 it tells the driver only the sequence number > > is available if it's two there is additional the trac status register as > > payload. > > > > Currently the atusb_in_good() function determines if it's a tx done or > > rx done if according the payload length. This patch is doing the same > > and assumes this behaviour. > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > --- > > > > Just an RFC, need another weekend to test it. > > > > drivers/net/ieee802154/atusb.c | 33 ++++++++++++++++++++++++++++----- > > 1 file changed, 28 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > index 2c338783893d..95a4a3cdc8a4 100644 > > --- a/drivers/net/ieee802154/atusb.c > > +++ b/drivers/net/ieee802154/atusb.c > > @@ -191,7 +191,7 @@ static void atusb_work_urbs(struct work_struct *work) > > > > /* ----- Asynchronous USB -------------------------------------------------- */ > > > > -static void atusb_tx_done(struct atusb *atusb, u8 seq) > > +static void atusb_tx_done(struct atusb *atusb, u8 seq, int reason) > > { > > struct usb_device *usb_dev = atusb->usb_dev; > > u8 expect = atusb->tx_ack_seq; > > @@ -199,7 +199,10 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > > dev_dbg(&usb_dev->dev, "%s (0x%02x/0x%02x)\n", __func__, seq, expect); > > if (seq == expect) { > > /* TODO check for ifs handling in firmware */ > > - ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); > > + if (reason == IEEE802154_SUCCESS) > > + ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); > > + else > > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, reason); > > } else { > > /* TODO I experience this case when atusb has a tx complete > > * irq before probing, we should fix the firmware it's an > > @@ -215,7 +218,8 @@ static void atusb_in_good(struct urb *urb) > > struct usb_device *usb_dev = urb->dev; > > struct sk_buff *skb = urb->context; > > struct atusb *atusb = SKB_ATUSB(skb); > > - u8 len, lqi; > > + int result = IEEE802154_SUCCESS; > > + u8 len, lqi, trac; > > > > if (!urb->actual_length) { > > dev_dbg(&usb_dev->dev, "atusb_in: zero-sized URB ?\n"); > > @@ -224,8 +228,27 @@ static void atusb_in_good(struct urb *urb) > > > > len = *skb->data; > > > > - if (urb->actual_length == 1) { > > - atusb_tx_done(atusb, len); > > + switch (urb->actual_length) { > > + case 2: > > + trac = TRAC_MASK(*(skb->data + 1)); > > I've been fighting all night thinking the issues were on the atusb side > (it was horribly difficult to get the atusb toolchain up and running, > I'll send a patch to update the instructions), in particular because of Really? for me it was just apt install avr-gcc libc-avr (or what the debian packages name is). Then the dfu-util and be sure you invoke dfu-util (as root because you might need some udev rules otherwise) when the atusb is booting up. > the data[2] definition which needed to be declared static outside of > the functions (see the other mail) and, I guess, because of this > beginner error: I was using skb->data[1] but of course it can't work. > mhh, I am sorry. I am not sure if I understand what you mean? Does the firmware patch have issues regarding data[2]? > Anyway, this patch works, I've tested it by: > - associating a device (an Arduino Nano running Zephyr, btw) > - having the atusb disassociating the nano successfully (trac = 0) > - reassociating the nano > - powering off the nano > - trying to disassociate > > The disassociation notification transmission fails with a TRAC status > NO_ACK. So: > > Tested-by: Miquel Raynal <miquel.raynal@bootlin.com> > > I still have the filtering thing to improve, I'm not sure I'll have > time to do that this week. I need some time to prepare the slides now > that the demo works :) > ok, no problem. Thanks for testing it. I am happy that if I could help you here a little bit. - Alex
Hi Alexander, aahringo@redhat.com wrote on Mon, 5 Sep 2022 07:16:42 -0400: > Hi, > > On Sun, Sep 4, 2022 at 11:22 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > Hi Alexander, > > > > aahringo@redhat.com wrote on Sun, 4 Sep 2022 20:55:44 -0400: > > > > > This patch adds support for reading the trac register if atusb firmware > > > reports tx done. There is currently a feature to compare a sequence > > > number, if the payload is 1 it tells the driver only the sequence number > > > is available if it's two there is additional the trac status register as > > > payload. > > > > > > Currently the atusb_in_good() function determines if it's a tx done or > > > rx done if according the payload length. This patch is doing the same > > > and assumes this behaviour. > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > --- > > > > > > Just an RFC, need another weekend to test it. > > > > > > drivers/net/ieee802154/atusb.c | 33 ++++++++++++++++++++++++++++----- > > > 1 file changed, 28 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > > index 2c338783893d..95a4a3cdc8a4 100644 > > > --- a/drivers/net/ieee802154/atusb.c > > > +++ b/drivers/net/ieee802154/atusb.c > > > @@ -191,7 +191,7 @@ static void atusb_work_urbs(struct work_struct *work) > > > > > > /* ----- Asynchronous USB -------------------------------------------------- */ > > > > > > -static void atusb_tx_done(struct atusb *atusb, u8 seq) > > > +static void atusb_tx_done(struct atusb *atusb, u8 seq, int reason) > > > { > > > struct usb_device *usb_dev = atusb->usb_dev; > > > u8 expect = atusb->tx_ack_seq; > > > @@ -199,7 +199,10 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > > > dev_dbg(&usb_dev->dev, "%s (0x%02x/0x%02x)\n", __func__, seq, expect); > > > if (seq == expect) { > > > /* TODO check for ifs handling in firmware */ > > > - ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); > > > + if (reason == IEEE802154_SUCCESS) > > > + ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); > > > + else > > > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, reason); > > > } else { > > > /* TODO I experience this case when atusb has a tx complete > > > * irq before probing, we should fix the firmware it's an > > > @@ -215,7 +218,8 @@ static void atusb_in_good(struct urb *urb) > > > struct usb_device *usb_dev = urb->dev; > > > struct sk_buff *skb = urb->context; > > > struct atusb *atusb = SKB_ATUSB(skb); > > > - u8 len, lqi; > > > + int result = IEEE802154_SUCCESS; > > > + u8 len, lqi, trac; > > > > > > if (!urb->actual_length) { > > > dev_dbg(&usb_dev->dev, "atusb_in: zero-sized URB ?\n"); > > > @@ -224,8 +228,27 @@ static void atusb_in_good(struct urb *urb) > > > > > > len = *skb->data; > > > > > > - if (urb->actual_length == 1) { > > > - atusb_tx_done(atusb, len); > > > + switch (urb->actual_length) { > > > + case 2: > > > + trac = TRAC_MASK(*(skb->data + 1)); > > > > I've been fighting all night thinking the issues were on the atusb side > > (it was horribly difficult to get the atusb toolchain up and running, > > I'll send a patch to update the instructions), in particular because of > > Really? for me it was just apt install avr-gcc libc-avr (or what the > debian packages name is). I feel so bad. All the instructions I could find (from 2011) advised to download an old gcc, an old binutils and old avr-lib, to compile everything by hand... 2 out of 3 archives returned a 404 error, the builds were chaotic I had to disable -Werror and fix many issues manually, then I had to fight with binutils assembler failing, I tried 4 different versions before I got it right... I did write a commit to update the instructions but if you say that just downloading the stock pre-compiled binaries worked I am a bit disappointed because the instructions specifically told not to do so. > Then the dfu-util and be sure you invoke > dfu-util (as root because you might need some udev rules otherwise) > when the atusb is booting up. Yes, dfu worked right away, very nice tool. > > the data[2] definition which needed to be declared static outside of > > the functions (see the other mail) and, I guess, because of this > > beginner error: I was using skb->data[1] but of course it can't work. > > > > mhh, I am sorry. I am not sure if I understand what you mean? Does the > firmware patch have issues regarding data[2]? Actually I'm wrong. You did: *(skb->data + 1), which works. I did: skb->data[1], which does not. I thought that my mistake was related to data being a void *, but it's a char * so both should work. I don't know why it failed, maybe my attention level was too low and I missed something else. > > Anyway, this patch works, I've tested it by: > > - associating a device (an Arduino Nano running Zephyr, btw) > > - having the atusb disassociating the nano successfully (trac = 0) > > - reassociating the nano > > - powering off the nano > > - trying to disassociate > > > > The disassociation notification transmission fails with a TRAC status > > NO_ACK. So: > > > > Tested-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > I still have the filtering thing to improve, I'm not sure I'll have > > time to do that this week. I need some time to prepare the slides now > > that the demo works :) > > > > ok, no problem. Thanks for testing it. I am happy that if I could help > you here a little bit. > > - Alex > Thanks, Miquèl
Hi, On Mon, Sep 5, 2022 at 10:19 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > Hi Alexander, > > aahringo@redhat.com wrote on Mon, 5 Sep 2022 07:16:42 -0400: > > > Hi, > > > > On Sun, Sep 4, 2022 at 11:22 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > Hi Alexander, > > > > > > aahringo@redhat.com wrote on Sun, 4 Sep 2022 20:55:44 -0400: > > > > > > > This patch adds support for reading the trac register if atusb firmware > > > > reports tx done. There is currently a feature to compare a sequence > > > > number, if the payload is 1 it tells the driver only the sequence number > > > > is available if it's two there is additional the trac status register as > > > > payload. > > > > > > > > Currently the atusb_in_good() function determines if it's a tx done or > > > > rx done if according the payload length. This patch is doing the same > > > > and assumes this behaviour. > > > > > > > > Signed-off-by: Alexander Aring <aahringo@redhat.com> > > > > --- > > > > > > > > Just an RFC, need another weekend to test it. > > > > > > > > drivers/net/ieee802154/atusb.c | 33 ++++++++++++++++++++++++++++----- > > > > 1 file changed, 28 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > > > index 2c338783893d..95a4a3cdc8a4 100644 > > > > --- a/drivers/net/ieee802154/atusb.c > > > > +++ b/drivers/net/ieee802154/atusb.c > > > > @@ -191,7 +191,7 @@ static void atusb_work_urbs(struct work_struct *work) > > > > > > > > /* ----- Asynchronous USB -------------------------------------------------- */ > > > > > > > > -static void atusb_tx_done(struct atusb *atusb, u8 seq) > > > > +static void atusb_tx_done(struct atusb *atusb, u8 seq, int reason) > > > > { > > > > struct usb_device *usb_dev = atusb->usb_dev; > > > > u8 expect = atusb->tx_ack_seq; > > > > @@ -199,7 +199,10 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) > > > > dev_dbg(&usb_dev->dev, "%s (0x%02x/0x%02x)\n", __func__, seq, expect); > > > > if (seq == expect) { > > > > /* TODO check for ifs handling in firmware */ > > > > - ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); > > > > + if (reason == IEEE802154_SUCCESS) > > > > + ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); > > > > + else > > > > + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, reason); > > > > } else { > > > > /* TODO I experience this case when atusb has a tx complete > > > > * irq before probing, we should fix the firmware it's an > > > > @@ -215,7 +218,8 @@ static void atusb_in_good(struct urb *urb) > > > > struct usb_device *usb_dev = urb->dev; > > > > struct sk_buff *skb = urb->context; > > > > struct atusb *atusb = SKB_ATUSB(skb); > > > > - u8 len, lqi; > > > > + int result = IEEE802154_SUCCESS; > > > > + u8 len, lqi, trac; > > > > > > > > if (!urb->actual_length) { > > > > dev_dbg(&usb_dev->dev, "atusb_in: zero-sized URB ?\n"); > > > > @@ -224,8 +228,27 @@ static void atusb_in_good(struct urb *urb) > > > > > > > > len = *skb->data; > > > > > > > > - if (urb->actual_length == 1) { > > > > - atusb_tx_done(atusb, len); > > > > + switch (urb->actual_length) { > > > > + case 2: > > > > + trac = TRAC_MASK(*(skb->data + 1)); > > > > > > I've been fighting all night thinking the issues were on the atusb side > > > (it was horribly difficult to get the atusb toolchain up and running, > > > I'll send a patch to update the instructions), in particular because of > > > > Really? for me it was just apt install avr-gcc libc-avr (or what the > > debian packages name is). > > I feel so bad. All the instructions I could find (from 2011) advised to > download an old gcc, an old binutils and old avr-lib, to compile > everything by hand... 2 out of 3 archives returned a 404 error, the > builds were chaotic I had to disable -Werror and fix many issues > manually, then I had to fight with binutils assembler failing, I tried > 4 different versions before I got it right... I did write a commit to > update the instructions but if you say that just downloading the stock > pre-compiled binaries worked I am a bit disappointed because the > instructions specifically told not to do so. > I am sorry that you had those problems. > > Then the dfu-util and be sure you invoke > > dfu-util (as root because you might need some udev rules otherwise) > > when the atusb is booting up. > > Yes, dfu worked right away, very nice tool. > > > > the data[2] definition which needed to be declared static outside of > > > the functions (see the other mail) and, I guess, because of this > > > beginner error: I was using skb->data[1] but of course it can't work. > > > > > > > mhh, I am sorry. I am not sure if I understand what you mean? Does the > > firmware patch have issues regarding data[2]? > > Actually I'm wrong. > You did: *(skb->data + 1), which works. > I did: skb->data[1], which does not. > I thought that my mistake was related to data being a void *, but it's > a char * so both should work. I don't know why it failed, maybe my > attention level was too low and I missed something else. I don't see a difference. No idea... - Alex
diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c index 2c338783893d..95a4a3cdc8a4 100644 --- a/drivers/net/ieee802154/atusb.c +++ b/drivers/net/ieee802154/atusb.c @@ -191,7 +191,7 @@ static void atusb_work_urbs(struct work_struct *work) /* ----- Asynchronous USB -------------------------------------------------- */ -static void atusb_tx_done(struct atusb *atusb, u8 seq) +static void atusb_tx_done(struct atusb *atusb, u8 seq, int reason) { struct usb_device *usb_dev = atusb->usb_dev; u8 expect = atusb->tx_ack_seq; @@ -199,7 +199,10 @@ static void atusb_tx_done(struct atusb *atusb, u8 seq) dev_dbg(&usb_dev->dev, "%s (0x%02x/0x%02x)\n", __func__, seq, expect); if (seq == expect) { /* TODO check for ifs handling in firmware */ - ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); + if (reason == IEEE802154_SUCCESS) + ieee802154_xmit_complete(atusb->hw, atusb->tx_skb, false); + else + ieee802154_xmit_error(atusb->hw, atusb->tx_skb, reason); } else { /* TODO I experience this case when atusb has a tx complete * irq before probing, we should fix the firmware it's an @@ -215,7 +218,8 @@ static void atusb_in_good(struct urb *urb) struct usb_device *usb_dev = urb->dev; struct sk_buff *skb = urb->context; struct atusb *atusb = SKB_ATUSB(skb); - u8 len, lqi; + int result = IEEE802154_SUCCESS; + u8 len, lqi, trac; if (!urb->actual_length) { dev_dbg(&usb_dev->dev, "atusb_in: zero-sized URB ?\n"); @@ -224,8 +228,27 @@ static void atusb_in_good(struct urb *urb) len = *skb->data; - if (urb->actual_length == 1) { - atusb_tx_done(atusb, len); + switch (urb->actual_length) { + case 2: + trac = TRAC_MASK(*(skb->data + 1)); + switch (trac) { + case TRAC_SUCCESS: + case TRAC_SUCCESS_DATA_PENDING: + /* already IEEE802154_SUCCESS */ + break; + case TRAC_CHANNEL_ACCESS_FAILURE: + result = IEEE802154_CHANNEL_ACCESS_FAILURE; + break; + case TRAC_NO_ACK: + result = IEEE802154_NO_ACK; + break; + default: + result = IEEE802154_SYSTEM_ERROR; + } + + fallthrough; + case 1: + atusb_tx_done(atusb, len, result); return; }
This patch adds support for reading the trac register if atusb firmware reports tx done. There is currently a feature to compare a sequence number, if the payload is 1 it tells the driver only the sequence number is available if it's two there is additional the trac status register as payload. Currently the atusb_in_good() function determines if it's a tx done or rx done if according the payload length. This patch is doing the same and assumes this behaviour. Signed-off-by: Alexander Aring <aahringo@redhat.com> --- Just an RFC, need another weekend to test it. drivers/net/ieee802154/atusb.c | 33 ++++++++++++++++++++++++++++----- 1 file changed, 28 insertions(+), 5 deletions(-)