sam0_common: Add USB peripheral driver by bergzand · Pull Request #10915 · RIOT-OS/RIOT

bergzand · 2019-01-31T21:56:16Z

Contribution description

This PR adds a USB peripheral driver for the sam0 device class. The driver is much WIP as some details still have to be thought out.

Testing procedure

Nothing yet, a follow-up is required to actually initialize the usb device.

Issues/PRs references

depends on #9830

dylad

some comments for the first round. I'll test on arduino-zero like board soon.

dylad · 2019-02-01T20:09:41Z

cpu/sam0_common/include/sam_usb.h

@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2018 Freie Universität Berlin
+ * Copyright (C) 2018 Inria


You should probably update these copyrights.

Fixed, thanks

dylad · 2019-02-01T20:10:04Z

cpu/sam0_common/include/sam_usb.h

+ * @{
+ *
+ * @file
+ * @brief       [RFC 4122](https://tools.ietf.org/html/rfc4122) UUID functions


Please also update Doxygen

dylad · 2019-02-01T20:11:11Z

cpu/sam0_common/include/sam_usb.h

+#define SAM_USB_NUM_EP      USBDEV_NUM_ENDPOINTS
+
+typedef struct {
+    usbdev_t usbdev;


missing description

On the TODO

dylad · 2019-02-01T20:12:47Z

cpu/sam0_common/usb/usb.c

+    return 2 * num + (dir == USB_EP_DIR_OUT ? 0 : 1);
+}
+
+static inline unsigned _get_ep_num2(usbdev_ep_t *ep)


Why is there two get_ep_num functions ?

Probably just bad naming, but will check.

Refactored these a bit to make more sense

dylad · 2019-02-01T20:23:42Z

cpu/sam0_common/usb/usb.c

+            val = 0x7;
+            break;
+        default:
+            return;


What about a single line to directly compute this value with a nice comment ? :)

Should be possible, but have to think about it a bit, the last case (1023) makes it a bit annoying.

I've been thinking a bit about this. I'd rather keep it this way as signalling error back in case of invalid size is easier to do with the switch case statement

dylad · 2019-02-01T20:24:14Z

cpu/sam0_common/usb/usb.c

+    int res = -ENOTSUP;
+    assert(ep != NULL);
+    switch (opt) {
+        case USBOPT_EP_ENABLE:


add fall-through comment

removed as they are covered by the default option

boards/samr21-xpro/Makefile.dep

dylad

Another small round !

dylad · 2019-02-02T21:12:14Z

cpu/sam0_common/usb/usb.c

+    /* Reset peripheral */
+    USB->DEVICE.CTRLA.reg |= USB_CTRLA_SWRST;
+    while (usb_swrst_syncing()) {}
+    while (USB->DEVICE.CTRLA.bit.SWRST) {}


You can remove this line. The datasheet states that both bit will be clearer once reset is done, so the line above should be enough.

Duplicate something something...

dylad · 2019-02-02T21:13:27Z

cpu/sam0_common/usb/usb.c

+        USB_PADCAL_TRIM((*(uint32_t *)USB_FUSES_TRIM_ADDR >>
+                         USB_FUSES_TRIM_Pos));
+
+    USB->DEVICE.CTRLB.bit.SPDCONF = 0x0;


Replaces 0 by USB_DEVICE_CTRLB_SPDCONF_FS

cpu/sam0_common/usb/usb.c

dylad · 2019-02-02T21:37:01Z

cpu/sam0_common/usb/usb.c

+            ep_reg->EPSTATUSSET.reg = USB_DEVICE_EPSTATUSSET_STALLRQ1;
+        }
+        else {
+            ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSSET_STALLRQ1;


Suggested change

ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSSET_STALLRQ1;

ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSCLR_STALLRQ1;

dylad · 2019-02-02T21:37:28Z

cpu/sam0_common/usb/usb.c

+            ep_reg->EPSTATUSSET.reg = USB_DEVICE_EPSTATUSSET_STALLRQ0;
+        }
+        else {
+            ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSSET_STALLRQ0;


Suggested change

ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSSET_STALLRQ0;

ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSCLR_STALLRQ0;

dylad · 2019-02-02T21:39:25Z

cpu/sam0_common/usb/usb.c

+            }
+            res = sizeof(usbopt_enable_t);
+            break;
+        default:


DEBUG line ?

dylad · 2019-02-02T21:39:46Z

cpu/sam0_common/usb/usb.c

+    UsbDeviceEndpoint *ep_reg = &USB->DEVICE.DeviceEndpoint[ep->num];
+
+    if (ep->dir == USB_EP_DIR_IN) {
+        ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSSET_BK1RDY;


Suggested change

ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSSET_BK1RDY;

ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSCLR_BK1RDY;

dylad · 2019-02-02T21:40:23Z

cpu/sam0_common/usb/usb.c

+        ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSSET_BK1RDY;
+    }
+    else {
+        ep_reg->EPSTATUSSET.reg = USB_DEVICE_EPSTATUSCLR_BK0RDY;


Suggested change

ep_reg->EPSTATUSSET.reg = USB_DEVICE_EPSTATUSCLR_BK0RDY;

ep_reg->EPSTATUSSET.reg = USB_DEVICE_EPSTATUSSET_BK0RDY;

dylad · 2019-02-09T20:34:40Z

cpu/sam0_common/usb/usb.c

+
+    /* Enable USB device */
+    USB->DEVICE.DESCADD.reg = (uint32_t)banks;
+    USB->DEVICE.CTRLA.reg |= USB_CTRLA_ENABLE | USB_CTRLA_RUNSTDBY;


I think you can remove USB_CTRLA_RUNSTDBY for now or make it user defined in periph_conf.h

Removed for now

dylad · 2019-02-09T20:57:18Z

drivers/include/usb/usbdev.h

+struct usbdev {
+    const struct usbdev_driver *driver;     /**< usbdev driver struct   */
+    usbdev_event_cb_t cb;                   /**< Event callback supplied by
+                                              *  upper layer */


What about adding UsbDevice struct from atmel vendor files ? In the current state, this driver works fine with one USB device instance but maybe one day we will have several USB within a MCU.
This way we can replace all USB->DEVICE from the driver by a more generic call.

I assume you want to add that to the sam0_common_usb_t and not to the generic usbdev struct.

I think your point is valid, but I have to check if the current API allows this. If it doesn't, I would assume this to be a shortcomming of the API.

I assume you want to add that to the sam0_common_usb_t and not to the generic usbdev struct.

Yes

Done, I'm not fully content with the current implementation for retrieving the context in the isr, but this can be reworked as soon as we actually have a device with multiple usb peripherals

cpu/sam0_common/usb/usb.c

bergzand · 2019-02-13T18:30:33Z

Rebased and reworked to the current state of #9830

Also fixed a number of issues/suggestions as proposed by @dylad

bergzand · 2019-02-13T18:32:58Z

Removed WIP status btw, ready for review (I hope)

bergzand · 2019-02-25T19:18:13Z

rebased

dylad · 2019-03-15T09:28:50Z

@bergzand i'll give it another round this weekend. Could you rebase please ?

bergzand · 2019-03-18T17:57:59Z

Reworked and rebased!

dylad

@bergzand We're almost ready, just a few comments.

dylad · 2019-03-18T20:02:14Z

cpu/sam0_common/include/sam_usb.h

+/**
+ * USB endpoint buffer space
+ *
+ * @todo global configurable?


still todo ?

Nope, actually is globally configurable now (for a while)

dylad · 2019-03-18T20:10:56Z

cpu/sam0_common/periph/usbdev.c

+        case 512:
+            val = 0x6;
+            break;
+        case 1023:


@bergzand I think I've read somewhere that some endpoint size can be 1023 or 1024 in specific case. Am I wrong ?

1023 bytes is the max size for USB 2.0 full speed isochronous transfers, USB 2.0 high speed is allowed to have transfers of 1024 bytes with isochronous endpoint types.

Alright then we're all good here :)

dylad · 2019-03-18T20:16:02Z

cpu/sam0_common/periph/usbdev.c

+                                   GCLK_CLKCTRL_GEN_GCLK0 |
+                                   (GCLK_CLKCTRL_ID(USB_GCLK_ID)));
+#elif defined(CPU_FAM_SAML21)
+    MCLK->AHBMASK.reg |= MCLK_AHBMASK_USB;


You missed MCLK->APBBMASK for SAML21 family

dylad · 2019-03-18T20:26:01Z

cpu/sam0_common/periph/usbdev.c

+    GCLK->CLKCTRL.reg = (uint32_t)(GCLK_CLKCTRL_CLKEN |
+                                   GCLK_CLKCTRL_GEN_GCLK0 |
+                                   (GCLK_CLKCTRL_ID(USB_GCLK_ID)));
+#elif defined(CPU_FAM_SAML21)


#elif defined(CPU_FAM_SAML21) || defined(CPU_FAM_SAMR30)

dylad · 2019-03-18T20:29:06Z

cpu/sam0_common/periph/usbdev.c

+    gpio_init(GPIO_PIN(PA, 24), GPIO_IN);
+    gpio_init(GPIO_PIN(PA, 25), GPIO_IN);
+    gpio_init_mux(GPIO_PIN(PA, 24), GPIO_MUX_G);
+    gpio_init_mux(GPIO_PIN(PA, 25), GPIO_MUX_G);


You should reused your config struct defined in periph_conf.h here.

So much effort spent on the config struct, only to forget to use it in the implementation…

dylad · 2019-03-18T20:30:27Z

cpu/sam0_common/periph/usbdev.c

+    usbdev->config->device->CTRLB.bit.SPDCONF = USB_DEVICE_CTRLB_SPDCONF_FS;
+    _enable_irq(usbdev);
+
+    gpio_init_int(GPIO_PIN(PA, 7), GPIO_IN_PD, GPIO_BOTH,


same comment as above

dylad · 2019-03-18T20:33:41Z

cpu/sam0_common/periph/usbdev.c

+    sam0_common_usb_t *usbdev = (sam0_common_usb_t*)dev;
+    if (usbdev->connect_change) {
+        usbdev->connect_change = false;
+        if (gpio_read(GPIO_PIN(PA, 7))) {


same comment as above

bergzand · 2019-03-18T20:58:44Z

@dylad fixed your remarks, also uncrustified the file (but should have done that in a separate fixup commit :'( )

dylad · 2019-03-18T21:01:57Z

also uncrustified the file (but should have done that in a separate fixup commit :'( )

I don't mind, I'll test it on hardware asap so we can move forward.

bergzand · 2019-03-18T21:02:34Z

I don't mind, I'll test it on hardware asap so we can move forward.

Thanks, appreciating your reviews!

bergzand · 2019-03-18T21:20:17Z

I messed up the GPIO things :(

bergzand · 2019-03-18T21:24:06Z

I messed up the GPIO things :(

fixed

bergzand · 2019-03-19T20:10:43Z

@dylad I've reworked the driver with the ideas as discussed out-of-band today. Sorry for the force-push, messed up my commit order a bit.

usbdev is extended with suspend and resume event types. I can split these out into a separate PR if you like
the sam0_common usbdev driver makes use of these. Low power modes are unblocked when the device is suspended. Detaching the device from a host is also registered as a "suspend" here.
I've added config for the arduino-zero board.

dylad

Seems like we're close to an end.

boards/samr21-xpro/include/periph_conf.h

dylad · 2019-03-20T21:08:24Z

cpu/sam0_common/periph/usbdev.c

+    return res;
+}
+
+static int _ep_unready(usbdev_ep_t *ep)


nitpick: why _ep_unready and _usbdev_ep_ready ?

Arbitrary, nothing specific, position of the moon etc.

Will fix to _ep_ready

Never mind the above, I remember now, all the static _usbdev_$SOMETHING calls are used in the instantiated usbdev_driver_t struct

dylad · 2019-03-20T21:11:16Z

cpu/sam0_common/periph/usbdev.c

+static void _usbdev_ep_esr(usbdev_ep_t *ep)
+{
+    UsbDeviceEndpoint *ep_reg = _ep_reg_from_ep(ep);
+    signed event = -1;


int32 or int16 ? I'm sure this one will be troublesome outside of ARM.

But this driver is closely tied to an ARM based core…

I can change it to int32_t if you like

But same as this comment, there is not really a need for a specific variable size here, so I'd rather give the compiler more freedom for possible optimizations.

dylad · 2019-03-25T19:59:26Z

drivers/include/periph/usbdev.h

+    /**
+     * @brief USB suspend condition no longer active
+     */
+    USBDEV_EVENT_RESUME,


dump question here:
Is this suppose to be random or alphabetical order ?

Good question. The only "order" I had in mind was something that appeared logical to me. Which at the moment is to group the similar events together as much as possible.

Alright, I don't think the coding guideline force us to change something here. Let's keep it this way.

bergzand · 2019-03-25T20:06:23Z

rebased to current master

dylad

This is my last round. We're are almost good here !

dylad · 2019-03-25T20:09:12Z

cpu/sam0_common/periph/usbdev.c

+    GCLK->CLKCTRL.reg = (uint32_t)(GCLK_CLKCTRL_CLKEN |
+                                   GCLK_CLKCTRL_GEN_GCLK0 |
+                                   (GCLK_CLKCTRL_ID(USB_GCLK_ID)));
+#elif defined(CPU_FAM_SAML21) || defined(CPU_FAM_SAMR30)


I would change this line to

Suggested change

#elif defined(CPU_FAM_SAML21) || defined(CPU_FAM_SAMR30)

#elif defined(CPU_SAML21)

This will prevent us to add new sub-family like the upcoming SAMR34

Huh? Are you sure? I changed this to the current situation based on your comment. I don't mind changing it back to what you are suggesting, just want to make sure that we get this right.

You can go ahead, this is the way to go.

See 4b9b90b

dylad · 2019-03-25T20:19:59Z

cpu/sam0_common/periph/usbdev.c

+        case USB_EP_TYPE_INTERRUPT:
+            type = 0x04;
+            break;
+        case USB_EP_TYPE_NONE:


add default statement at the same time here ?

How about I add an

default: assert(false);

here. Anything else than the types already handled in the switch is an error.

Never mind, I can just change the case USB_EP_TYPE_NONE: to default:

See 279e844

That would be perfect.

dylad · 2019-03-25T20:41:46Z

ACK.
arduino-zero is happy. Please squash !

dylad · 2019-03-26T09:24:24Z

@bergzand
Feel free to squash when all static-tests will be fixed.

bergzand · 2019-03-26T09:42:02Z

@dylad Squashed, all green here! 🎉

dylad · 2019-03-26T09:43:19Z

Here we go !

bergzand · 2019-03-26T09:44:24Z

@dylad Thanks for the review and testing!

bergzand added State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Jan 31, 2019

bergzand requested a review from dylad January 31, 2019 21:56

bergzand mentioned this pull request Jan 31, 2019

USBUS: Initial work towards an USB stack #10916

Merged

1 task

dylad reviewed Feb 1, 2019

View reviewed changes

dylad reviewed Feb 2, 2019

View reviewed changes

boards/samr21-xpro/Makefile.dep Outdated Show resolved Hide resolved

dylad reviewed Feb 2, 2019

View reviewed changes

dylad reviewed Feb 9, 2019

View reviewed changes

cpu/sam0_common/usb/usb.c Outdated Show resolved Hide resolved

bergzand force-pushed the pr/usb/sam0 branch from a843254 to 12044ec Compare February 13, 2019 18:13

bergzand changed the title ~~[WIP] sam0_common: Add USB peripheral driver~~ sam0_common: Add USB peripheral driver Feb 13, 2019

bergzand removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Feb 13, 2019

bergzand force-pushed the pr/usb/sam0 branch from 12044ec to 2483ab5 Compare February 25, 2019 19:17

bergzand force-pushed the pr/usb/sam0 branch from 2483ab5 to 4677289 Compare February 27, 2019 20:00

bergzand mentioned this pull request Mar 18, 2019

samd21: Expose PM states #11202

Merged

bergzand force-pushed the pr/usb/sam0 branch from 4677289 to 1c04329 Compare March 18, 2019 17:57

bergzand added the Area: USB Area: Universal Serial Bus label Mar 18, 2019

dylad reviewed Mar 18, 2019

View reviewed changes

bergzand force-pushed the pr/usb/sam0 branch from bb262fd to b531f44 Compare March 20, 2019 08:28

dylad reviewed Mar 20, 2019

View reviewed changes

dylad added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Mar 25, 2019

dylad reviewed Mar 25, 2019

View reviewed changes

usbdev: extend events with suspend/resume

c697a3b

bergzand force-pushed the pr/usb/sam0 branch from 93917d9 to 449d88f Compare March 25, 2019 20:06

dylad reviewed Mar 25, 2019

View reviewed changes

dylad approved these changes Mar 25, 2019

View reviewed changes

bergzand force-pushed the pr/usb/sam0 branch from 4b9b90b to f169bfd Compare March 25, 2019 20:42

bergzand added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 25, 2019

bergzand force-pushed the pr/usb/sam0 branch from f169bfd to 7bc2096 Compare March 25, 2019 20:47

bergzand added 3 commits March 26, 2019 10:26

sam0_common: Add USB peripheral driver

d780482

samr21-xpro: add usbdev configuration

99962ff

arduino-zero: add usbdev feature

7daf596

bergzand force-pushed the pr/usb/sam0 branch from 4ba8792 to 7daf596 Compare March 26, 2019 09:28

dylad merged commit 93bff48 into RIOT-OS:master Mar 26, 2019

danpetry added this to the Release 2019.04 milestone Mar 26, 2019

bergzand mentioned this pull request Mar 26, 2019

sam0_common: Fix syntax mistake in usbdev driver #11282

Merged

keestux mentioned this pull request Jun 5, 2019

Add usbdev feature to Sodaq boards #11639

Merged

	ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSSET_STALLRQ1;
	ep_reg->EPSTATUSCLR.reg = USB_DEVICE_EPSTATUSCLR_STALLRQ1;

	ep_reg->EPSTATUSSET.reg = USB_DEVICE_EPSTATUSCLR_BK0RDY;
	ep_reg->EPSTATUSSET.reg = USB_DEVICE_EPSTATUSSET_BK0RDY;

	#elif defined(CPU_FAM_SAML21) \|\| defined(CPU_FAM_SAMR30)
	#elif defined(CPU_SAML21)

Conversation

bergzand commented Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dylad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bergzand commented Jan 31, 2019 •

edited

Loading