Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Aug 28, 2017

Conversation

dylad
Copy link
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.

@dylad
Copy link
Member Author

dylad commented May 14, 2017

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

@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

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
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
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

@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
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 ?

Copy link
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
Contributor

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 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
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
Development

Successfully merging this pull request may close these issues.

6 participants