discrepancy while save and restore of debounce registers

discrepancy while save and restore of debounce registers

[Hebbar, Gururaja]

Hi,

I came across a peculiar issue while updating GPIO debounce registers on
OMAP platform.

According to mainline commit ae547354a8ed59f19b57f7e1de9c7816edfc3537

gpio/omap: save and restore debounce registers

GPIO debounce registers need to be saved and restored for proper functioning
of driver.

...
@@ -1363,6 +1369,12 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
        __raw_writel(bank->context.fallingdetect,
                                bank->base + bank->regs->fallingdetect);
        __raw_writel(bank->context.dataout, bank->base + bank->regs->dataout);
+       if (bank->dbck_enable_mask) {
+               __raw_writel(bank->context.debounce, bank->base +
+                                       bank->regs->debounce);
+               __raw_writel(bank->context.debounce_en,
+                                       bank->base + bank->regs->debounce_en);
+       }
 }


Due to copy/paste of this commit into my local tree, I missed the check for 
bank->dbck_enable_mask, and directly restored the saved value from context.

After this, I saw random crashes when accessing different registers (sometimes
its OE register and sometime its DATAOUT register). 

These crashes were seen across 2nd and subsequent suspend/resume.

My doubt/questions are
1. Why should debounce registers be updated only when it's accessed previously?

2. What is the relation between updating debounce registers and crash seen on
others registers? 

Thanks in advance for the support.

Regards
Gururaja

Re: discrepancy while save and restore of debounce registers

[Jon Hunter]

Hi Gururaja,

On 10/17/2012 01:13 AM, Hebbar, Gururaja wrote:
> Hi,
> 
> I came across a peculiar issue while updating GPIO debounce registers on
> OMAP platform.
> 
> According to mainline commit ae547354a8ed59f19b57f7e1de9c7816edfc3537
> 
> gpio/omap: save and restore debounce registers
> 
> GPIO debounce registers need to be saved and restored for proper functioning
> of driver.
> 
> ...
> @@ -1363,6 +1369,12 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
>         __raw_writel(bank->context.fallingdetect,
>                                 bank->base + bank->regs->fallingdetect);
>         __raw_writel(bank->context.dataout, bank->base + bank->regs->dataout);
> +       if (bank->dbck_enable_mask) {
> +               __raw_writel(bank->context.debounce, bank->base +
> +                                       bank->regs->debounce);
> +               __raw_writel(bank->context.debounce_en,
> +                                       bank->base + bank->regs->debounce_en);
> +       }
>  }
> 
> 
> Due to copy/paste of this commit into my local tree, I missed the check for 
> bank->dbck_enable_mask, and directly restored the saved value from context.
> 
> After this, I saw random crashes when accessing different registers (sometimes
> its OE register and sometime its DATAOUT register). 
> 
> These crashes were seen across 2nd and subsequent suspend/resume.
> 
> My doubt/questions are
> 1. Why should debounce registers be updated only when it's accessed previously?

If debounce is not being used by any of the gpios, then there is no need
to restore them as there are no bits set. So this makes sense and saves
a couple register writes.

> 2. What is the relation between updating debounce registers and crash seen on
> others registers? 

This I am not sure about. I gave this a quick try on my omap3430 beagle
board, but I did not see any side-effects from doing this. However, if
you are always restoring the debounce context regardless of whether
debounce is being used, then you could be writing bad values to the
debounce registers as the context variables bank->context.debouce and
bank->context.debouce_en may not initialised. So that is bad. However,
that said I am still not sure how this could cause a crash.

Can you share more details on ...
1. The OMAP platform you are using?
2. What linux distro/environment you are using?
3. If there are any specific steps to reproduce this 100% of the time?

Cheers
Jon

RE: discrepancy while save and restore of debounce registers

[Hebbar, Gururaja]

Jon,

On Thu, Oct 18, 2012 at 02:42:01, Hunter, Jon wrote:
> Hi Gururaja,
> 
> On 10/17/2012 01:13 AM, Hebbar, Gururaja wrote:
> > Hi,
> > 
> > I came across a peculiar issue while updating GPIO debounce registers on
> > OMAP platform.
> > 
> > According to mainline commit ae547354a8ed59f19b57f7e1de9c7816edfc3537
> > 
> > gpio/omap: save and restore debounce registers
> > 
> > GPIO debounce registers need to be saved and restored for proper functioning
> > of driver.
> > 
> > ...
> > @@ -1363,6 +1369,12 @@ static void omap_gpio_restore_context(struct gpio_bank *bank)
> >         __raw_writel(bank->context.fallingdetect,
> >                                 bank->base + bank->regs->fallingdetect);
> >         __raw_writel(bank->context.dataout, bank->base + bank->regs->dataout);
> > +       if (bank->dbck_enable_mask) {
> > +               __raw_writel(bank->context.debounce, bank->base +
> > +                                       bank->regs->debounce);
> > +               __raw_writel(bank->context.debounce_en,
> > +                                       bank->base + bank->regs->debounce_en);
> > +       }
> >  }
> > 
> > 
> > Due to copy/paste of this commit into my local tree, I missed the check for 
> > bank->dbck_enable_mask, and directly restored the saved value from context.
> > 
> > After this, I saw random crashes when accessing different registers (sometimes
> > its OE register and sometime its DATAOUT register). 
> > 
> > These crashes were seen across 2nd and subsequent suspend/resume.
> > 
> > My doubt/questions are
> > 1. Why should debounce registers be updated only when it's accessed previously?
> 
> If debounce is not being used by any of the gpios, then there is no need
> to restore them as there are no bits set. So this makes sense and saves
> a couple register writes.

What I want to know is that other than saving register writes, is there any
other important stuff that specifies this requirement.

> 
> > 2. What is the relation between updating debounce registers and crash seen on
> > others registers? 
> 
> This I am not sure about. I gave this a quick try on my omap3430 beagle
> board, but I did not see any side-effects from doing this. However, if
> you are always restoring the debounce context regardless of whether
> debounce is being used, then you could be writing bad values to the
> debounce registers as the context variables bank->context.debouce and
> bank->context.debouce_en may not initialised. So that is bad. However,
> that said I am still not sure how this could cause a crash.
> 
> Can you share more details on ...

Sorry for missing below details in first post.

> 1. The OMAP platform you are using?

I was trying this on TI AM335x platform (repo below). On AM335x EVM board

http://arago-project.org/git/projects/?p=linux-am33x.git;a=shortlog;
h=refs/heads/v3.2-staging

> 2. What linux distro/environment you are using?

Arago AM335x PSP release (linux 3.2 + am335x patch-set)

> 3. If there are any specific steps to reproduce this 100% of the time?

On top of this tree, try suspend/resume using "echo mem > /syspower/state"

> 
> Cheers
> Jon
> 


Regards, 
Gururaja

Re: discrepancy while save and restore of debounce registers

[Jon Hunter]

Hi Gururaja,

On 10/18/2012 12:31 AM, Hebbar, Gururaja wrote:
> Jon,
> 
> On Thu, Oct 18, 2012 at 02:42:01, Hunter, Jon wrote:

[snip]

>>> My doubt/questions are
>>> 1. Why should debounce registers be updated only when it's accessed previously?
>>
>> If debounce is not being used by any of the gpios, then there is no need
>> to restore them as there are no bits set. So this makes sense and saves
>> a couple register writes.
> 
> What I want to know is that other than saving register writes, is there any
> other important stuff that specifies this requirement.

>From a HW perspective, none that I can see.

>From a SW perspective, yes, as I mentioned if you restore these
registers without first initialising the context variables where these
registers are stored, then you may be restoring unknown values to the
registers and that is bad.

>>> 2. What is the relation between updating debounce registers and crash seen on
>>> others registers? 
>>
>> This I am not sure about. I gave this a quick try on my omap3430 beagle
>> board, but I did not see any side-effects from doing this. However, if
>> you are always restoring the debounce context regardless of whether
>> debounce is being used, then you could be writing bad values to the
>> debounce registers as the context variables bank->context.debouce and
>> bank->context.debouce_en may not initialised. So that is bad. However,
>> that said I am still not sure how this could cause a crash.
>>
>> Can you share more details on ...
> 
> Sorry for missing below details in first post.
> 
>> 1. The OMAP platform you are using?
> 
> I was trying this on TI AM335x platform (repo below). On AM335x EVM board
> 
> http://arago-project.org/git/projects/?p=linux-am33x.git;a=shortlog;
> h=refs/heads/v3.2-staging
> 
>> 2. What linux distro/environment you are using?
> 
> Arago AM335x PSP release (linux 3.2 + am335x patch-set)
> 
>> 3. If there are any specific steps to reproduce this 100% of the time?
> 
> On top of this tree, try suspend/resume using "echo mem > /syspower/state"

I have a beagle-bone but unfortunately, suspend does not appear to be
supported in the mainline kernel yet so I am unable to test this on the
am335x on the mainline.

Have you compared the gpio driver from your v3.2 branch with the current
mainline to see how different this code is? Ideally, it would be good to
test on the mainline kernel for another data point to see if this is
local to your branch.

Cheers
Jon

RE: discrepancy while save and restore of debounce registers

[Hebbar, Gururaja]

On Thu, Oct 18, 2012 at 21:44:56, Hunter, Jon wrote:
> Hi Gururaja,
> 
> On 10/18/2012 12:31 AM, Hebbar, Gururaja wrote:
> > Jon,
> > 
> > On Thu, Oct 18, 2012 at 02:42:01, Hunter, Jon wrote:
> 
> [snip]
> 
> >>> My doubt/questions are
> >>> 1. Why should debounce registers be updated only when it's accessed previously?
> >>
> >> If debounce is not being used by any of the gpios, then there is no need
> >> to restore them as there are no bits set. So this makes sense and saves
> >> a couple register writes.
> > 
> > What I want to know is that other than saving register writes, is there any
> > other important stuff that specifies this requirement.
> 
> From a HW perspective, none that I can see.
> 
> From a SW perspective, yes, as I mentioned if you restore these
> registers without first initialising the context variables where these
> registers are stored, then you may be restoring unknown values to the
> registers and that is bad.
> 
> >>> 2. What is the relation between updating debounce registers and crash seen on
> >>> others registers? 
> >>
> >> This I am not sure about. I gave this a quick try on my omap3430 beagle
> >> board, but I did not see any side-effects from doing this. However, if
> >> you are always restoring the debounce context regardless of whether
> >> debounce is being used, then you could be writing bad values to the
> >> debounce registers as the context variables bank->context.debouce and
> >> bank->context.debouce_en may not initialised. So that is bad. However,
> >> that said I am still not sure how this could cause a crash.
> >>
> >> Can you share more details on ...
> > 
> > Sorry for missing below details in first post.
> > 
> >> 1. The OMAP platform you are using?
> > 
> > I was trying this on TI AM335x platform (repo below). On AM335x EVM board
> > 
> > http://arago-project.org/git/projects/?p=linux-am33x.git;a=shortlog;
> > h=refs/heads/v3.2-staging
> > 
> >> 2. What linux distro/environment you are using?
> > 
> > Arago AM335x PSP release (linux 3.2 + am335x patch-set)
> > 
> >> 3. If there are any specific steps to reproduce this 100% of the time?
> > 
> > On top of this tree, try suspend/resume using "echo mem > /syspower/state"
> 
> I have a beagle-bone but unfortunately, suspend does not appear to be
> supported in the mainline kernel yet so I am unable to test this on the
> am335x on the mainline.

You can try the suspend/resume on BB from Arago tree which I mentioned above

> 
> Have you compared the gpio driver from your v3.2 branch with the current
> mainline to see how different this code is? Ideally, it would be good to
> test on the mainline kernel for another data point to see if this is
> local to your branch.

The mainline kernel omap GPIO driver has changed a lot from current arago
tree (which is at v3.2). Upon comparing is when I found out about the
register access check done before restoring.

> 
> Cheers
> Jon
> 
> 
> 


Regards, 
Gururaja