Add basic EFM32HG support - CMU/GPIO/USB #874

schnommus · 2018-02-06T08:51:22Z

The open-hardware http://tomu.im/ project uses an EFM32HG, and the USB stack from libopencm3 works great on it. (disclaimer: I don't have any official role in the project, just having fun hacking at it)

The changes in this PR add basic EFM32HG support for CMU, GPIO, USB & WDOG, a lot copied from the existing EFM32LG support.

Main questions/concerns I have are:

There is a bit of code duplication given the similarity of EFM32LG and EFM32HG CMU/USB headers. I opted to separate them so that I didn't have to modify EFM32LG register names (some EFM32LG registers have the same purpose, but slightly different names to EFM32HG-datasheet names)
I fixed an alignment assumption in lib/usb/usb_standard.c but it might be worth reducing this to a compile-time check (as to whether we're running on a Cortex-M0).
I have added code from PR USB MSC: Writes aren't acknowledged #409 which is a bugfix in the MSC USB module. I am happy to remove this if it is deemed out of scope for this task.

I have tested the GPIO, CMU and USB support extensively on hardware, using the examples in my fork here: https://github.com/schnommus/libopencm3-examples - at examples/efm32/efm32hg/tomu-efm32hg309

Thanks!

karlp · 2018-02-06T10:29:27Z

Please to make it easier to review this, use more appropriate commit line tags. Don't prefix common usb code changes with "efm32hg: " for instance.

karlp · 2018-02-06T10:32:24Z

include/libopencm3/efm32/hg/gpio.h

@@ -2,7 +2,6 @@
 * This file is part of the libopencm3 project.
 *
 * Copyright (C) 2015 Kuldeep Singh Dhaka <kuldeepdhaka9@gmail.com>
- * Copyright (C) 2018 Seb Holzapfel <schnommus@gmail.com>


Try and squish this sort of commit back out of existance, rather than introducing code early in a series and promptly deleting it. Just don't add it in the first place.

Sure thing, I should have checked out the other PRs and noticed you were doing per-commit reviews rather than looking at all the changes at once. Cheers!

karlp · 2018-02-06T10:33:10Z

lib/usb/usb_efm32hg.c

+    CMU_LFCLKSEL = (CMU_LFCLKSEL & ~CMU_LFCLKSEL_LFC_MASK) | CMU_LFCLKSEL_LFC_LFRCO;
+    CMU_LFCCLKEN0 |= CMU_LFCCLKEN0_USBLE;
+
+    // Calibrate USB based on communications


Don't mix in c++ comments here. keep them all the same style

This was fixed by a later commit in the chain (and a good reason for me to clean the commit structure up as you suggested!)

karlp · 2018-02-06T10:33:43Z

lib/usb/usb_efm32hg.c

+    USB_PCGCCTL &= ~(USB_PCGCCTL_PWRCLMP | USB_PCGCCTL_RSTPDWNMODULE);
+
+    /* Core Soft Reset */
+    {


no need for these blocks here, we don't use this style at all.

Sure, I'll make that change.

karlp · 2018-02-06T10:34:06Z

lib/usb/usb_efm32hg.c

+    USB_GAHBCFG |= USB_GAHBCFG_GLBLINTRMSK;
+    USB_GINTMSK = USB_GINTMSK_USBRSTMSK |
+                   USB_GINTMSK_ENUMDONEMSK | USB_GINTMSK_IEPINTMSK | USB_GINTMSK_OEPINTMSK
+        /*| USB_GINTMSK_WKUPINTMSK*/;


drop the dead code

This was fixed by a later commit in the chain (and another good reason for me to clean the commit structure up as you suggested!)

schnommus · 2018-02-06T11:38:10Z

Thanks @karlp for the extremely rapid response!
I've just restructured the commit labelling and made the changes you suggested.

karlp · 2018-02-06T12:12:56Z

I haven't looked at this in any real depth yet, these were just some preliminary comments. I tend to look at both, the "whole set" and also each commit. Particularly for large changes like this, it's nice to look at both to see how things have developed. What would be super nice is if you could add a test for efm32 to the usb gadget zero tests. That would give a lot of assurance that it's working as well as other platforms. see the "tests" directory in the main repo

schnommus · 2018-02-06T21:21:36Z

Definitely, it would be nice to have a gadget-zero test. I'll work on adding one for EFM32HG (I don't have a dev board for EFM32LG as well unfortunately). I'll just tack it onto this PR if that's okay? The PR will grow a bit as I'll have to add a timer driver as well - but hopefully it's mostly the same as for EFM32LG.

schnommus · 2018-02-10T23:04:27Z

Alright, @karlp - v2 of this PR has the following additions:

A timer driver (mostly common with EFM32LG)
A gadget0 usb regression test for efm32hg (verified working on a Tomu board)
The fx07_common USB driver has been renamed to dwc_common, as both STM32FX07 and EFM32 chips use the same DesignWare USB IP core.
Commit history has been rewritten so it should be a bit easier to review

Since the USB OTG core driver was modified (so that it worked on CM0 platforms) I have run gadget0 tests on STM32F4 as well to gain some assurance it doesn't break things.

It would be nice to make the EFM32LG USB implementation use this common dwc driver as well, but I don't have an EFM32LG board to test on.

…chips use DesignWare USB IP cores

… platforms

… STM32 and EFM32 chips use the same dwc USB IP cores

…m3#409

schnommus · 2018-02-19T07:48:06Z

Heya @karlp, v3 of this PR has the following changes (based on your feedback):

Moved otg_fs.h/otg_hs.h so they can be accessed by all platforms with DesignWare USB cores
Further segmented USB changes into separate commits (alignment changes for CM0 of particular note)
Fixed comment nits you brought up
Rebased onto latest master (with travis CI)

karlp · 2018-02-19T22:37:17Z

I've just ran the gadget0 tests on my efm32hg starter kit (BRD2012A) and it works flawlessly. I love it when people do that :) Just going to run it on a bunch of other boards now :)

karlp · 2018-02-19T22:51:09Z

perf on f4 before:
read 5324800 bytes in 0:00:07.713160 for 674.1724533135576 kps
wrote 5324800 bytes in 0:00:14.067787 for 369.63880672916076 kps
perf on f4 after:
read 5324800 bytes in 0:00:07.717820 for 673.7653897084929 kps
wrote 5324800 bytes in 0:00:14.073495 for 369.4888867335371 kps

ie, the same. good :)

karlp · 2018-02-19T23:02:49Z

Just FYI, I'm not going to pull in things like "usb-msc: fix write acknowledgement bug in msc driver from PR #409" in the first round.

karlp · 2018-02-19T23:07:15Z

For the record, the efm32hg performance is
read 5324800 bytes in 0:00:18.334538 for 283.61772737333223 kps
wrote 5324800 bytes in 0:00:28.285705 for 183.83844418938824 kps

karlp · 2018-02-19T23:10:03Z

include/libopencm3/efm32/common/gpio_common_hglg.h

+#define GPIO_P_MODEL_MODE6(mode)	GPIO_P_MODEL_MODEx(6, mode)
+
+#define GPIO_P_MODEL_MODE7_MASK		GPIO_P_MODEL_MODEx_MASK(7)
+#define GPIO_P_MODEL_MODE7(mode)	GPIO_P_MODEL_MODEx(7, mode)


I thinkall of this is gross, no-one's ever going to use the GPIO_P_MODEL_MODE7_MASK macros individually, but whatever, I see this is just how the existing lg code was, so it's fine :)

karlp · 2018-02-19T23:16:08Z

include/libopencm3/efm32/hg/cmu.h

+#define CMU_HFPERCLKDIV_HFPERCLKDIV_DIV256	\
+	CMU_HFPERCLKDIV_HFPERCLKDIV_HFCLK256
+#define CMU_HFPERCLKDIV_HFPERCLKDIV_DIV512	\
+	CMU_HFPERCLKDIV_HFPERCLKDIV_HFCLK512


what on earth is this garbage?! I mean, ok, it will get grandfathered, but yuck!

karlp · 2018-02-19T23:47:40Z

include/libopencm3/efm32/hg/usb.h

+ * This file is part of the libopencm3 project.
+ *
+ * Copyright (C) 2015 Kuldeep Singh Dhaka <kuldeepdhaka9@gmail.com>
+ * Copyright (C) 2018 Seb Holzapfel <schnommus@gmail.com>


(nevermind, see it in the .h, was looking for .c)

schnommus · 2018-02-20T21:19:44Z

Awesome, thanks @karlp for the rapid response (as usual!).
Performance numbers appreciated, and the MSC changes being out of scope is fine.
(If I understand correctly) I will stop touching this PR as it's ready to be merged.
Ping me if you need anything else.
Cheers!

karlp · 2018-03-02T22:46:03Z

this has all landed, but I'm about to push up another branch with all the WG/EZR32WG changes too, and I'd like to see about sharing the USB code there too.

karlp reviewed Feb 6, 2018

View reviewed changes

schnommus force-pushed the efm32hg-pr branch from d5519a6 to 7f1e707 Compare February 6, 2018 11:36

schnommus force-pushed the efm32hg-pr branch from 632dd77 to 7bdcb0e Compare February 10, 2018 22:48

Sebastian Holzapfel added 15 commits February 19, 2018 18:30

efm32hg: add basic makefile, interrupts, device information

8787055

doxygen: efm32hg: update doxygen templates to include efm32hg

de38f9c

efm32hg: add memory map

b69e36a

efm32hg/lg: gpio: use common gpio implementation

e38d031

efm32hg/lg: wdog: use common wdog implementation

730be2f

efm32hg/lg: timer: use common timer implementation

909fbb0

efm32hg: cmu: add updated cmu implementation based on efm32lg

8885a59

stm32fx07: usb: rename common implementation as both STM32 and EFM32 …

4296f11

…chips use DesignWare USB IP cores

usb-dwc: handle word-aligned and unaligned buffers, to support armv6m…

29edaed

… platforms

usb-dwc/stm32: relocate common otg_fs, otg_hs implementations as both…

52aeaa5

… STM32 and EFM32 chips use the same dwc USB IP cores

efm32hg: usb: add usb support

7c9e3aa

efm32hg: add linker script for efm32hg309f64

a434f80

tests: gadget0: efm32hg: add gadget0 test for efm32hg

698ddf8

usb-standard: fix alignment assumption for Cortex-M0 platforms

0c83030

usb-msc: fix write acknowledgement bug in msc driver from PR libopenc…

aad71a3

…m3#409

schnommus force-pushed the efm32hg-pr branch from 7bdcb0e to aad71a3 Compare February 19, 2018 07:36

karlp reviewed Feb 19, 2018

View reviewed changes

karlp closed this Mar 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic EFM32HG support - CMU/GPIO/USB #874

Add basic EFM32HG support - CMU/GPIO/USB #874

schnommus commented Feb 6, 2018 •

edited

Loading

karlp commented Feb 6, 2018

karlp Feb 6, 2018

schnommus Feb 6, 2018

karlp Feb 6, 2018

schnommus Feb 6, 2018

karlp Feb 6, 2018

schnommus Feb 6, 2018

karlp Feb 6, 2018

schnommus Feb 6, 2018

schnommus commented Feb 6, 2018

karlp commented Feb 6, 2018

schnommus commented Feb 6, 2018

schnommus commented Feb 10, 2018

schnommus commented Feb 19, 2018

karlp commented Feb 19, 2018

karlp commented Feb 19, 2018

karlp commented Feb 19, 2018

karlp commented Feb 19, 2018

karlp Feb 19, 2018

karlp Feb 19, 2018

karlp Feb 19, 2018

schnommus commented Feb 20, 2018

karlp commented Mar 2, 2018

Add basic EFM32HG support - CMU/GPIO/USB #874

Add basic EFM32HG support - CMU/GPIO/USB #874

Conversation

schnommus commented Feb 6, 2018 • edited Loading

karlp commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schnommus commented Feb 6, 2018

karlp commented Feb 6, 2018

schnommus commented Feb 6, 2018

schnommus commented Feb 10, 2018

schnommus commented Feb 19, 2018

karlp commented Feb 19, 2018

karlp commented Feb 19, 2018

karlp commented Feb 19, 2018

karlp commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schnommus commented Feb 20, 2018

karlp commented Mar 2, 2018

schnommus commented Feb 6, 2018 •

edited

Loading