cc2538/periph/gpio: workaround for old defines and some fixes #6773

AnonMall · 2017-03-21T16:54:56Z

Temporary workaround for issue mentioned in #6650. Also some small fixes, as the interrupts haven't been working anymore. Enabling interrupts for PORT_D GPIO pins still causes my board to freeze, but interrupts are now working again for all other ports.

AnonMall · 2017-03-23T14:57:24Z

Fixed some problems in clearing the interrupt register, causing the isr to never end. PORT_D interrupts also work now

smlng · 2017-07-03T10:09:05Z

cpu/cc2538/periph/gpio.c

 #define PORTNUM_SHIFT       (12U)
 #define PIN_MASK            (0x00000007)
 #define MODE_NOTSUP         (0xff)

 static inline cc2538_gpio_t *gpio(gpio_t pin)
 {
-    return (cc2538_gpio_t *)(pin & GPIO_MASK);
+    if(((uint32_t)pin &GPIO_MASK) == 0){


can you explain a bit more why this handling is needed, and what error you see without this fix - and how to reproduce?!

Right now there are two types of pin encoding. The one provided by cpu/cc2538/include/cc2538-gpio.h and the one which is used here, provided by the RIOT gpio interface. The one in cc2538-gpio.h is also used for the pin name defines, which are then used for configuring boards, such as the Re-Mote. Because one encoding does not translate well into the other, SPI Pins for example, are not correctly initialized. The error can be reproduced by simply initializing pins through their pin-names, i.e. GPIO_PD4 and then setting or reading them. The result will be, that nothing happens, as the pin will not be propably addressed. This workaround just checks, whether the old or the new encoding is used. And if the old encoding is detected, it then translates it according to the new gpio interface pin encoding. As I mentioned, this should only be a temporary workaround. The fix would be to adjust cc2538-gpio.h pin encoding and all its functions. I have stated one example in the issue, linked to this pull request. I hope I was able to answer your questions adequately.
Cheers

thanks for clearing things up! So this if ... else is not really needed (and should be fixed otherwise), and the actual fix is correction of PORTNUM_MASK and the interrupt initialization?

Changing PORTNUM_MASK alone does not work unfortunately, as there is different use of both encodings. While the new one uses the upper bits of the encoding for the port address and the lower bits for the pin number, the old encoding style, as in the pin names, holds no information on which address to access and had to be calculated. So the usage of both is different. A more permanent fix would be to change cc2538-gpio.h and make in compatible with this implementation.

smlng · 2017-07-03T12:19:43Z

cpu/cc2538/periph/gpio.c

@@ -26,19 +26,27 @@
 #include "periph/gpio.h"

 #define GPIO_MASK           (0xfffff000)
-#define PORTNUM_MASK        (0x00007000)
+#define PORTNUM_MASK        (0x00003000)


why changing this, I think 0x00007000 is right because PORT_D = 3 = b11 and base address of GPIO_A = 0x400d9000 = b1101 1001 0000 0000 hence

b0000 0011 0000 0000 (=PORT_D << 12) + b1101 1001 0000 0000 (=GPIO_A) = b1101 1100 0000 0000 ^^ -> 0x00003000 (PORTNUM_MASK)

but with the old

= b1101 1100 0000 0000 ^^^ -> 0x00007000 (PORTNUM_MASK)

the new portnum mask would not capture port_d, or does it?

ah, you changed the calculation, too - so this still works. mhm, don't know if I like that.

Just tested it. That fix seems to be a remnant of a previous fix, where the 'PORTNUM_MASK' would give me negative numbers, so I reworked it. But the calculation and the 'PORTNUM_MASK' can remain the same for me, as long as the if-clause is present, in order to work with the old encoding.

smlng · 2017-07-03T12:36:36Z

cpu/cc2538/periph/gpio.c

@@ -174,7 +182,7 @@ static inline void handle_isr(cc2538_gpio_t *gpio, int port_num)
 {
    uint32_t state       = gpio->MIS;
    gpio->IC             = 0x000000ff;
-    gpio->IRQ_DETECT_ACK = 0x000000ff;
+    gpio->IRQ_DETECT_ACK = (0xff << (port_num * 8));


more consistent would even be

gpio->IRQ_DETECT_ACK = (0xff << (port_num * GPIO_BITS_PER_PORT));

Yeah you're right. That's better.

smlng · 2017-07-03T15:27:39Z

@AnonMall can you factor out your fix of int gpio_init_int and make it separate PR? Or, if you don't have time to work on this I can overtake that.

Regarding the other changes aka workaround: I'd rather work on completely fixing the GPIO stuff which means to rework I2C, SPI, and UART to the RIOT GPIO API, but it has to be (or should be) done anyway.

AnonMall · 2017-07-03T15:55:26Z

Just factored it and opened a new PR. #7303

smlng · 2017-07-07T09:52:02Z

@AnonMall please try if #7316 or #7320 (the latter includes the former + rework of all other periphs) resolve this issue #6650 for you and hence make this workaround obsolete.

AnonMall · 2017-07-07T11:27:19Z

@smlng #7320 seems to be working without problems for all periph related things direct gpio operations. But that also means that as long as we don't define them ourselves in our programs, we have to refrain from using pin names, am I right? Also just a small question, which might be more of a RIOT SPI API thing. There is currently no possibilty to use the spi bus chip select of, lets say for example SSI0, as a software chip select right? Because right now they are coded as hardware chip select (at least on the CC2538) with no option in changing that, what leads to the problem, when I want to do spi burst operations or try to access extended register addresses on devices.

smlng · 2017-07-07T11:32:00Z

SPI is not my strong suit maybe @haukepetersen can help here?

Anyhow, my currently open PRs for cc2538 are all about resolving the conflicting GPIO pin definitions and handling between RIOTs and TIs native GPIO API.

I would say open an issue for SPI, stating your use-case as outlined above - and then we'll see howto fix that in a separate PR.

But also, as you have been: thanks for testing!

haukepetersen · 2017-07-11T10:36:30Z

Not sure I understand the question about the SPI CS pin usage correctly. You want to control the SPI CS line manually, is that it? In this case, you simply pass GPIO_UNDEF as cs parameter to the spi_write function, and voilla, you have full control over the CS line outside of the SPI driver.

And by the way, the cc2538`s SPI does currently not support the usage of hardware chip select lines...

AnonMall · 2017-07-19T18:33:43Z

Sorry for the late reply. What I meant is, that while I still can use GPIO_UNDEF for spi_write, in the cpu/cc2538/periph/spi.c spi_init_pins(...) function, there is a chip select pin being programmed for hardware control, which is defined through the boards periph_conf.h. This may be only the case for the cc2538, as I haven't looked into other platforms, but this means, that I am not able to do any software chip select on that same pin, if I don't comment out, either the chip select pin in the periph_conf.h or spi.c. This is nescessary, as I have implemented a CC1200 driver, for which I am going to open a PR soon. The CC1200 has an extended register address set and the hardware controlled chip select is interfering with register access. Maybe I am missing something, for which I would be glad to be enlightend. I hope that I was able to explain my problem a bit more detailed. Thanks in advance.

haukepetersen · 2017-07-20T09:34:46Z

Ah, I think I get it now. Actually, the spi driver code for the cc2538 is broken: the cs pin should never be touched when calling spi_init_pins(). That is what the spi_init_cs() function is explicitly for...

See #7389 for a fix.

Anon Mall added 3 commits March 21, 2017 17:35

cc2538 gpio workaround for old defines and some fixes

65e9bb8

Merge remote-tracking branch 'upstream/master' into gpioFix

64169a9

gpio interrupt register fix

6ff9c6f

PeterKietzmann assigned smlng and A-Paul Mar 28, 2017

PeterKietzmann added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers labels Mar 28, 2017

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 26, 2017

smlng reviewed Jul 3, 2017

View reviewed changes

Anon Mall added 3 commits July 3, 2017 17:39

cc2538 gpio workaround for old defines and some fixes

00a79f0

reverted interrupt changes for factoring

59482d3

reverted port_num

2dca87c

This was referenced Jul 3, 2017

Gpio Interrupt fix for the CC2538 #7303

Merged

cpu, cc2538: gpio cleanup #7320

Closed

AnonMall closed this Jul 7, 2017

smlng mentioned this pull request Jul 20, 2017

cpu, cc2538: adapt periph spi to GPIO API #7313

Closed

AnonMall deleted the gpioFix branch January 31, 2018 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cc2538/periph/gpio: workaround for old defines and some fixes #6773

cc2538/periph/gpio: workaround for old defines and some fixes #6773

AnonMall commented Mar 21, 2017

AnonMall commented Mar 23, 2017

smlng Jul 3, 2017

AnonMall Jul 3, 2017

smlng Jul 3, 2017

AnonMall Jul 3, 2017

smlng Jul 3, 2017

smlng Jul 3, 2017

AnonMall Jul 3, 2017

smlng Jul 3, 2017

AnonMall Jul 3, 2017

smlng commented Jul 3, 2017

AnonMall commented Jul 3, 2017

smlng commented Jul 7, 2017

AnonMall commented Jul 7, 2017 •

edited

Loading

smlng commented Jul 7, 2017

haukepetersen commented Jul 11, 2017

AnonMall commented Jul 19, 2017

haukepetersen commented Jul 20, 2017

cc2538/periph/gpio: workaround for old defines and some fixes #6773

cc2538/periph/gpio: workaround for old defines and some fixes #6773

Conversation

AnonMall commented Mar 21, 2017

AnonMall commented Mar 23, 2017

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

Choose a reason for hiding this comment

smlng commented Jul 3, 2017

AnonMall commented Jul 3, 2017

smlng commented Jul 7, 2017

AnonMall commented Jul 7, 2017 • edited Loading

smlng commented Jul 7, 2017

haukepetersen commented Jul 11, 2017

AnonMall commented Jul 19, 2017

haukepetersen commented Jul 20, 2017

AnonMall commented Jul 7, 2017 •

edited

Loading