From: khilman@ti•com (Kevin Hilman)
To: linux-arm-kernel@lists•infradead.org
Subject: [GIT PULL] gpio/omap: cleanups for v3.5
Date: Mon, 02 Jul 2012 10:37:21 -0700 [thread overview]
Message-ID: <87pq8e6p4e.fsf@ti.com> (raw)
In-Reply-To: <CAC83ZvJWz7tCL9VSQXiLNoYDrg6+=Mkx7xS9hQivKfA_L_4ANw@mail.gmail.com> (Tarun Kanti DebBarma's message of "Mon, 25 Jun 2012 13:37:12 +0530")
"DebBarma, Tarun Kanti" <tarun.kanti@ti•com> writes:
> On Mon, Jun 25, 2012 at 11:48 AM, NeilBrown <neilb@suse•de> wrote:
>> On Thu, 21 Jun 2012 12:04:26 +0530 "DebBarma, Tarun Kanti"
>> <tarun.kanti@ti•com> wrote:
>>
>>> On Thu, Jun 21, 2012 at 8:46 AM, NeilBrown <neilb@suse•de> wrote:
>>> > On Thu, 14 Jun 2012 23:24:10 +0530 "DebBarma, Tarun Kanti"
>>> > <tarun.kanti@ti•com> wrote:
>>> >
>>> >> On Thu, Jun 14, 2012 at 5:45 AM, NeilBrown <neilb@suse•de> wrote:
>>> >> > On Fri, 11 May 2012 17:30:48 -0700 Kevin Hilman <khilman@ti•com> wrote:
>>> >> >
>>> >> >> Hi Grant,
>>> >> >>
>>> >> >> Here's the final round of GPIO cleanups for v3.5. ?This branch is based
>>> >> >> on my for_3.5/fixes/gpio branch you just pulled.
>>> >> >>
>>> >> >> Kevin
>>> >> >
>>> >> > Hi.
>>> >> >
>>> >> > ?I'm not sure if it was this series or the following cleanups which broke
>>> >> > ?things for me, but I've been trying 3.5-rc2 on my GTA04 and the serial
>>> >> > ?console (ttyO2) dies as soon as the omap-gpio driver initialises.
>>> >> >
>>> >> > ?After some digging I came up with this patch to gpio-omap.c
>>> >> >
>>> >> > @@ -1124,6 +1124,9 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev)
>>> >> >
>>> >> > ? ? ? ?platform_set_drvdata(pdev, bank);
>>> >> >
>>> >> > + ? ? ? if (bank->get_context_loss_count)
>>> >> > + ? ? ? ? ? ? ? bank->context_loss_count =
>>> >> > + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>>> >> > ? ? ? ?pm_runtime_enable(bank->dev);
>>> >> > ? ? ? ?pm_runtime_irq_safe(bank->dev);
>>> >> > ? ? ? ?pm_runtime_get_sync(bank->dev);
>>> >> >
>>> >> > which fixes it.
>>> >> >
>>> >> > What was happening ?was that when omap_gpio_probe calls pm_runtime_get_sync,
>>> >> > it calls
>>> >> > ?_od_runtime_resume -> pm_generic_runtime_resume -> omap_gpio_runtime_resume
>>> >> > ?-> omap_gpio_restore_context
>>> >> >
>>> >> > and then the serial port stops.
>>> >> > I reasoned that the context probably hadn't been set up yet, so restoring
>>> >> > from it broke things.
>>> >> > Initialising bank->context_loss_count seems sensible and would ensure that
>>> >> > we didn't try to restore the context until it has actually been lost.
>>> >>
>>> >> I thought the following code exactly does that. That is context_lost_cnt_after
>>> >> would be zero until there is context loss. The bank->context_loss_count is zero
>>> >> at the beginning. So, (context_lost_cnt_after != bank->context_loss_count) would
>>> >> be false and hence context restore should NOT happen? Not sure if I am
>>> >> over looking
>>> >> anything here....
>>> >>
>>> >> omap_gpio_runtime_resume(...)
>>> >> {
>>> >> ...
>>> >> ? ? ? ? if (bank->get_context_loss_count) {
>>> >> ? ? ? ? ? ? ? ? context_lost_cnt_after =
>>> >> ? ? ? ? ? ? ? ? ? ? ? ? bank->get_context_loss_count(bank->dev);
>>> >> ? ? ? ? ? ? ? ? if (context_lost_cnt_after != bank->context_loss_count) {
>>> >> ? ? ? ? ? ? ? ? ? ? ? ? omap_gpio_restore_context(bank);
>>> >> ? ? ? ? ? ? ? ? } else {
>>> >> ? ? ? ? ? ? ? ? ? ? ? ? spin_unlock_irqrestore(&bank->lock, flags);
>>> >> ? ? ? ? ? ? ? ? ? ? ? ? return 0;
>>> >> ? ? ? ? ? ? ? ? }
>>> >> ? ? ? ? }
>>> >> ...
>>> >> }
>>> >
>>> > Hi,
>>> > ?I've looked more closely at this now.
>>> >
>>> > The problem is that the initial context loss count is *not* zero. ?Not always.
>>> > The context loss count is the sum of
>>> >
>>> > ? ? ? ?count = pwrdm->state_counter[PWRDM_POWER_OFF];
>>> > ? ? ? ?count += pwrdm->ret_logic_off_counter;
>>> >
>>> > ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
>>> > ? ? ? ? ? ? ? ?count += pwrdm->ret_mem_off_counter[i];
>>> >
>>> > (from ?pwrdm_get_context_loss_count()).
>>> >
>>> > These are initlialised in _pwrdm_register
>>> >
>>> > ? ? ? ?/* Initialize the powerdomain's state counter */
>>> > ? ? ? ?for (i = 0; i < PWRDM_MAX_PWRSTS; i++)
>>> > ? ? ? ? ? ? ? ?pwrdm->state_counter[i] = 0;
>>> >
>>> > ? ? ? ?pwrdm->ret_logic_off_counter = 0;
>>> > ? ? ? ?for (i = 0; i < pwrdm->banks; i++)
>>> > ? ? ? ? ? ? ? ?pwrdm->ret_mem_off_counter[i] = 0;
>>> >
>>> > ? ? ? ?pwrdm_wait_transition(pwrdm);
>>> > ? ? ? ?pwrdm->state = pwrdm_read_pwrst(pwrdm);
>>> > ? ? ? ?pwrdm->state_counter[pwrdm->state] = 1;
>>> >
>>> >
>>> > What I'm seeing is that for wkup_pwrdm and dpll{3,4,5}_pwrdm,
>>> > the state that pwrdm_read_pwrst returns is PWRDM_POWER_OFF.
>>> > So that state_counter gets initialised to '1', and so the initial
>>> > context_loss_count, which includes that counter, is also '1'.
>>> > I think it is the wkup_pwrdm that covers the GPIOs that are causing problems
>>> > for me.
>>> I just put a log in omap_gpio_probe() to see the value of context_loss_count.
>>> GPIO Bank 0 (WKUP Domain) always shows the count as '1'.
>>>
>>> [ ? ?0.169494] omap_gpio omap_gpio.0: context_loss_count=1
>>> [ ? ?0.170227] gpiochip_add: registered GPIOs 0 to 31 on device: gpio
>>> [ ? ?0.170471] OMAP GPIO hardware version 0.1
>>> [ ? ?0.170623] omap_gpio omap_gpio.1: context_loss_count=0
>>> [ ? ?0.170928] gpiochip_add: registered GPIOs 32 to 63 on device: gpio
>>> [ ? ?0.171295] omap_gpio omap_gpio.2: context_loss_count=0
>>> [ ? ?0.171600] gpiochip_add: registered GPIOs 64 to 95 on device: gpio
>>> [ ? ?0.171936] omap_gpio omap_gpio.3: context_loss_count=0
>>> [ ? ?0.172241] gpiochip_add: registered GPIOs 96 to 127 on device: gpio
>>> [ ? ?0.172576] omap_gpio omap_gpio.4: context_loss_count=0
>>> [ ? ?0.172882] gpiochip_add: registered GPIOs 128 to 159 on device: gpio
>>> [ ? ?0.173217] omap_gpio omap_gpio.5: context_loss_count=0
>>> [ ? ?0.173522] gpiochip_add: registered GPIOs 160 to 191 on device: gpio
>>
>> That's consistent with what I see, and confirm that initialising the
>> context_lost_count to zero isn't always correct.
> I am just wondering if the context_lost_count = 1 for GPIO in WKUP domain
> is expected. In that case we have to add additional logic in runtime callbacks
> to skip context restore/save for WKUP domain GPIOs.
> But let's hear what Kevin says.
I think the original patch from Neil looks right.
Note that we would need something like this in the case where we built
the GPIO driver as a module and it was unloaded/reloaded where the
starting point of the context-loss count would not be zero.
Neil, care to send a patch w/changelog?
Thanks,
Kevin
next prev parent reply other threads:[~2012-07-02 17:37 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-05-12 0:30 [GIT PULL] gpio/omap: cleanups for v3.5 Kevin Hilman
2012-05-12 0:51 ` Grant Likely
2012-06-14 0:15 ` NeilBrown
2012-06-14 17:54 ` DebBarma, Tarun Kanti
2012-06-14 21:06 ` NeilBrown
2012-06-21 3:16 ` NeilBrown
2012-06-21 6:34 ` DebBarma, Tarun Kanti
2012-06-25 6:18 ` NeilBrown
2012-06-25 8:07 ` DebBarma, Tarun Kanti
2012-07-02 17:37 ` Kevin Hilman [this message]
2012-07-02 17:48 ` Kevin Hilman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87pq8e6p4e.fsf@ti.com \
--to=khilman@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox