Skip to content

saml21/gpio: Allow multiple EXTI at the same time#6932

Merged
haukepetersen merged 1 commit intoRIOT-OS:masterfrom
dylad:saml21_multiple_exti
Aug 28, 2017
Merged

saml21/gpio: Allow multiple EXTI at the same time#6932
haukepetersen merged 1 commit intoRIOT-OS:masterfrom
dylad:saml21_multiple_exti

Conversation

@dylad
Copy link
Copy Markdown
Member

@dylad dylad commented Apr 19, 2017

This PR proposes a fix for #6904
To sum up, having one EXTI on saml21 works fine but if you try to enable several exti only the first one works.( all init said it's ok but only the first works)
The problem is any attempt to write into EIC->CONFIG failed after EIC is enabled (EIC->CTRLA.reg = EIC_CTRLA_ENABLE;)
SAML21 datasheet said that EIC->CONFIG registers are protected after EIC is enable. (27.6.2.1. Initialization -> page 473 of saml21 datasheet)

Another solution would be (disable EIC at the end of the function, change EIC->CONFIG reg and re-enable it):

    /* enable clocks for the EIC module */
    MCLK->APBAMASK.reg |= MCLK_APBAMASK_EIC;
    GCLK->PCHCTRL[EIC_GCLK_ID].reg = GCLK_PCHCTRL_CHEN | GCLK_PCHCTRL_GEN_GCLK0;
    /* enable the global EIC interrupt */
    NVIC_EnableIRQ(EIC_IRQn);
    /*Enable pin interrupt */
    EIC->INTFLAG.reg = (1 << exti);
    EIC->INTENSET.reg = (1 << exti);
    /* disable the EIC module*/
    EIC->CTRLA.reg = 0;
    while (EIC->SYNCBUSY.reg & EIC_SYNCBUSY_ENABLE) {}
    /* configure the active flank */
    EIC->CONFIG[exti >> 3].reg &= ~(0xf << ((exti & 0x7) * 4));
    EIC->CONFIG[exti >> 3].reg |=  (flank << ((exti & 0x7) * 4));
    /* enable the EIC module*/
    EIC->CTRLA.reg = EIC_CTRLA_ENABLE;
    while (EIC->SYNCBUSY.reg & EIC_SYNCBUSY_ENABLE) {}
    return 0;

Let me know what you think about it.

@miri64 miri64 requested a review from kaspar030 May 3, 2017 12:10
@dylad
Copy link
Copy Markdown
Member Author

dylad commented May 14, 2017 •
edited
Loading

After looking at cpu/samd21, I'm wordering if this bug is also present on SAM(R/D)21.
I don't have the hardware to test it. Can someone help me to confirm it ?
If the bug is also present, I'll merged GPIO driver into sam0_common and fix the bug.

edit: typos

@dylad dylad mentioned this pull request Jun 2, 2017
Merged
@aabadie aabadie added this to the Release 2017.07 milestone Jun 7, 2017
@aabadie aabadie 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) labels Jun 7, 2017
@photonthunder
Copy link
Copy Markdown

photonthunder commented Jun 8, 2017

I just ran a test using a samd21g18a and two EXTI appeared to work fine with latest master:

    gpio_init_int(GPIO_PIN(PA, 19), GPIO_IN_PU, GPIO_FALLING, samd21ble_isr, _);
    gpio_irq_enable(GPIO_PIN(PA,19));
    gpio_init_int(GPIO_PIN(PB, 8), GPIO_IN_PU, GPIO_FALLING, testpin_isr, _);
    gpio_irq_enable(GPIO_PIN(PB,8));

Let me know if I am missing something or you want me to try something else...

@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 8, 2017

@photonthunder Thanks a lot for testing it !
I just read the SAMD21 & SAML21 datasheet. In fact, this bug only affects the SAML21 family.
CONFIGn registers (and some others) are "enable-protected" for SAML21 (Module EIC must be disabled to write the register) but this is not the case for SAMD21.
Sorry I should have seen that earlier...

@dylad
Copy link
Copy Markdown
Member Author

dylad commented Jun 9, 2017

@photonthunder BTW, did you notice the call to gpio_irq_enable is useless as it is already done in gpio_init_int ?
But I don't know if it's a normal behavior...

@photonthunder
Copy link
Copy Markdown

photonthunder commented Jun 10, 2017

@dylad thanks. I was troubleshooting because I couldn't get it to work initially and through those in. I took them out and it was still functional.

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 22, 2017
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 22, 2017

@dylad, I have no strong opinion on the change and tend to give an ACK. Maybe other maintainers will have comments ? @haukepetersen ?

haukepetersen
haukepetersen approved these changes Aug 28, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@haukepetersen haukepetersen added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 28, 2017
@haukepetersen
Copy link
Copy Markdown
Contributor

haukepetersen commented Aug 28, 2017

verified 2 exti lines simultaneously working on the saml21-xpro and Murdock is happy -> go

@haukepetersen haukepetersen merged commit 10d3948 into RIOT-OS:master Aug 28, 2017
@dylad dylad mentioned this pull request Aug 28, 2017
Closed
@dylad dylad deleted the saml21_multiple_exti branch August 28, 2017 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haukepetersen haukepetersen haukepetersen approved these changes

@kaspar030 kaspar030 Awaiting requested review from kaspar030

Assignees

@jnohlgard jnohlgard

@kaspar030 kaspar030

Labels

CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR 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)

Projects

None yet

Milestone

Release 2017.10

Development

Successfully merging this pull request may close these issues.

6 participants

@dylad @photonthunder @aabadie @haukepetersen @jnohlgard @kaspar030