| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <51ADD1F8.7070405@ti.com>
Date: Tue, 4 Jun 2013 14:39:36 +0300
From: Grygorii Strashko <grygorii.strashko@...com>
To: Kevin Hilman <khilman@...aro.org>
CC: Hebbar Gururaja <gururaja.hebbar@...com>,
<grant.likely@...aro.org>, <linus.walleij@...aro.org>,
<rob.herring@...xeda.com>,
<davinci-linux-open-source@...ux.davincidsp.com>,
<devicetree-discuss@...ts.ozlabs.org>,
<linux-arm-kernel@...ts.infradead.org>, <linux@....linux.org.uk>,
<linux-kernel@...r.kernel.org>, <vaibhav.bedia@...com>,
<sudhakar.raj@...com>, Tony Lindgren <tony@...mide.com>,
Wolfram Sang <wsa@...-dreams.de>, <linux-omap@...r.kernel.org>,
<linux-i2c@...r.kernel.org>
Subject: Re: [PATCH 11/11] i2c: omap: enhance pinctrl support
Hi Kevin, Gururaja
On 05/31/2013 08:34 PM, Kevin Hilman wrote:
> Hebbar Gururaja <gururaja.hebbar@...com> writes:
>
>> Amend the I2C omap pin controller to optionally take a pin control
>> handle and set the state of the pins to:
>>
>> - "default" on boot, resume and before performing an i2c transfer
>> - "idle" after initial default, after resume default, and after each
>> i2c xfer
>> - "sleep" on suspend()
>>
>> By optionally putting the pins into sleep state in the suspend callback
>> we can accomplish two things.
>> - One is to minimize current leakage from pins and thus save power,
>> - second, we can prevent the IP from driving pins output in an
>> uncontrolled manner, which may happen if the power domain drops the
>> domain regulator.
>>
>> Note:
>> A .suspend & .resume callback is added which simply puts the pins to sleep
>> state upon suspend & are moved to default & idle state upon resume.
>>
>> If any of the above pin states are missing in dt, a warning message
>> about the missing state is displayed.
>> If certain pin-states are not available, to remove this warning message
>> pass respective state name with null phandler.
>>
>> (Changes based on i2c-nomadik.c)
>>
>> Signed-off-by: Hebbar Gururaja <gururaja.hebbar@...com>
>> Cc: Tony Lindgren <tony@...mide.com>
>> Cc: Wolfram Sang <wsa@...-dreams.de>
>> Cc: linux-omap@...r.kernel.org
>> Cc: linux-i2c@...r.kernel.org
> [...]
>
>> @@ -664,7 +673,13 @@ omap_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>>
>> out:
>> pm_runtime_mark_last_busy(dev->dev);
>> +
>> pm_runtime_put_autosuspend(dev->dev);
>> + /* Optionally let pins go into idle state */
>> + if (!IS_ERR(dev->pins_idle))
>> + if (pinctrl_select_state(dev->pinctrl, dev->pins_idle))
>> + dev_err(dev->dev, "could not set pins to idle state\n");
> This is wrong. You're changing the muxing before the device actually
> goes idle. Anything you want to happen when the device actually idles
> for real has to be in runtime PM callbacks.
>
> Looking below, I see it's already in the callbacks, so why is it here also?
I have two questions regarding this patch & the whole series:
1) PM runtime suspend/resume
Current sequence:
- resume
|- omap_hwmod_enable()
|- _enable()
|- omap_hwmod_mux(oh->mux, _HWMOD_STATE_ENABLED)
|- enable module (sysc&clocks)
|- omap_i2c_runtime_resume()
- suspend
|- omap_i2c_runtime_suspend()
|- omap_hwmod_idle()
|- _idle()
|- disbale module (sysc&clocks)
|- omap_hwmod_mux(oh->mux, _HWMOD_STATE_IDLE);
The new order will change this sequence - *Is it safe?*:
- resume
|- omap_hwmod_enable()
|- _enable()
|- enable module (sysc&clocks) <---- Is it valid to enable
module before PINs?
|- omap_i2c_runtime_resume()
|- PINs state set to DEFAULT
- suspend
|- omap_i2c_runtime_suspend()
|- PINs state set to IDLE
|- omap_hwmod_idle()
|- _idle()
|- disbale module (sysc&clocks) <---- Is it valid to disable
module after PINs?
2) System suspend (OFF-mode) with assumption that "noirq" stage will be
used for PINs cfg
Current implementation: handled in the same way as PM runtime
The new implementation: -- total mess is here((:
- suspend_noirq - I2C device can be still active because of PM auto-suspend
|-_od_suspend_noirq
|- omap_i2c_suspend_noirq
|- PINs state set to SLEEP
|- pm_generic_runtime_suspend
|- omap_i2c_runtime_suspend()
|- PINs state set to IDLE <--- *oops* PINs state is IDLE and
not SLEEP
|- omap_device_idle()
|- omap_hwmod_idle()
|- _idle()
|- disbale module (sysc&clocks)
- resume_noirq - I2C was active before suspend
|-_od_resume_noirq
|- omap_hwmod_enable()
|- _enable()
|- enable module (sysc&clocks)
|- pm_generic_runtime_resume
|- omap_i2c_runtime_resume()
|- PINs state set to DEFAULT <--- !!!!
|- omap_i2c_resume_noirq
|- PINs state set to DEFAULT
|- PINs state set to IDLE <--- *big oops* we have active
module and its
PINs state is IDLE
All mentioned above, applicable for most of all patches in series.
And It seems, that this functionality can't be implemented in such way (
- OMAP device FW and Driver core might handle PINs states changing and
drivers (DT) should provide set of available states only.
Am I wrong?
Regards,
-grygorii
>
> [...]
>
>> @@ -1300,6 +1348,10 @@ static int omap_i2c_runtime_suspend(struct device *dev)
>> omap_i2c_read_reg(_dev, OMAP_I2C_STAT_REG);
>> }
>>
>> + if (!IS_ERR(_dev->pins_idle))
>> + if (pinctrl_select_state(_dev->pinctrl, _dev->pins_idle))
>> + dev_err(dev, "could not set pins to idle state\n");
>> +
>> return 0;
>> }
>>
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists