Add I2C support to 66AK2G. Primary requirement is to add PM
Runtime support to the driver.
This has been tested on following platforms by performing simple i2c test
such as i2c detect and reading on board i2c devices:
K2G GP evm
OMAPL138
K2L GP EVM
and boot tested on:
K2E GP EVM
K2HK GP EVM
Version 2 changes:
Moved ordering of pm runtime calls
Version 3 changes:
Dropped first patch since it was pulled in.
Remove clk = NULL statements
Fix error path
Version 4 changes:
Fix typo in commit message
Fix error path
Franklin S Cooper Jr (2):
i2c: davinci: Add PM Runtime Support
dt-bindings: i2c: i2c-davinci: Update binding for 66AK2Gx pwr dm
property
.../devicetree/bindings/i2c/i2c-davinci.txt | 12 ++++
drivers/i2c/busses/i2c-davinci.c | 67 ++++++++++++++++++----
2 files changed, 67 insertions(+), 12 deletions(-)
--
2.9.4.dirty
Add pm-domains property which is required for 66AK2Gx. Also document 66AK2G
unique clocks property usage.
Signed-off-by: Franklin S Cooper Jr <[email protected]>
Acked-by: Rob Herring <[email protected]>
Acked-by: Sekhar Nori <[email protected]>
---
Documentation/devicetree/bindings/i2c/i2c-davinci.txt | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
index 5b123e0..64e6e65 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-davinci.txt
@@ -6,6 +6,18 @@ davinci/keystone i2c interface contains.
Required properties:
- compatible: "ti,davinci-i2c" or "ti,keystone-i2c";
- reg : Offset and length of the register set for the device
+- clocks: I2C functional clock phandle.
+ For 66AK2G this property should be set per binding,
+ Documentation/devicetree/bindings/clock/ti,sci-clk.txt
+
+SoC-specific Required Properties:
+
+The following are mandatory properties for Keystone 2 66AK2G SoCs only:
+
+- power-domains: Should contain a phandle to a PM domain provider node
+ and an args specifier containing the I2C device id
+ value. This property is as per the binding,
+ Documentation/devicetree/bindings/soc/ti/sci-pm-domain.txt
Recommended properties :
- interrupts : standard interrupt property.
--
2.9.4.dirty
66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
unlike other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
is required to insure the power domain used by the specific I2C instance is
properly turned on along with its functional clock.
Signed-off-by: Franklin S Cooper Jr <[email protected]>
---
Version 4 changes:
Fix wording on error message
Fix additional error path
Version 3 changes:
Remove several statements that set clk to NULL
Fix error path
drivers/i2c/busses/i2c-davinci.c | 67 +++++++++++++++++++++++++++++++++-------
1 file changed, 55 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index b8c4353..a2d9f7c 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
#include <linux/gpio.h>
#include <linux/of_device.h>
#include <linux/platform_data/i2c-davinci.h>
+#include <linux/pm_runtime.h>
/* ----- global defines ----------------------------------------------- */
@@ -122,6 +123,9 @@
/* set the SDA GPIO low */
#define DAVINCI_I2C_DCLR_PDCLR1 BIT(1)
+/* timeout for pm runtime autosuspend */
+#define DAVINCI_I2C_PM_TIMEOUT 1000 /* ms */
+
struct davinci_i2c_dev {
struct device *dev;
void __iomem *base;
@@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
+ ret = pm_runtime_get_sync(dev->dev);
+ if (ret < 0) {
+ dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
+ pm_runtime_put_noidle(dev->dev);
+ return ret;
+ }
+
ret = i2c_davinci_wait_bus_not_busy(dev);
if (ret < 0) {
dev_warn(dev->dev, "timeout waiting for bus ready\n");
- return ret;
+ goto out;
}
for (i = 0; i < num; i++) {
@@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
ret);
if (ret < 0)
- return ret;
+ goto out;
}
+ ret = num;
#ifdef CONFIG_CPU_FREQ
complete(&dev->xfr_complete);
#endif
- return num;
+out:
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
+ return ret;
}
static u32 i2c_davinci_func(struct i2c_adapter *adap)
@@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
int count = 0;
u16 w;
+ if (pm_runtime_suspended(dev->dev))
+ return IRQ_NONE;
+
while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
if (count++ == 100) {
@@ -802,13 +821,24 @@ static int davinci_i2c_probe(struct platform_device *pdev)
dev->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(dev->clk))
return PTR_ERR(dev->clk);
- clk_prepare_enable(dev->clk);
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
dev->base = devm_ioremap_resource(&pdev->dev, mem);
if (IS_ERR(dev->base)) {
- r = PTR_ERR(dev->base);
- goto err_unuse_clocks;
+ return PTR_ERR(dev->base);
+ }
+
+ pm_runtime_set_autosuspend_delay(dev->dev,
+ DAVINCI_I2C_PM_TIMEOUT);
+ pm_runtime_use_autosuspend(dev->dev);
+
+ pm_runtime_enable(dev->dev);
+
+ r = pm_runtime_get_sync(dev->dev);
+ if (r < 0) {
+ dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
+ pm_runtime_put_noidle(dev->dev);
+ return r;
}
i2c_davinci_init(dev);
@@ -849,27 +879,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
if (r)
goto err_unuse_clocks;
+ pm_runtime_mark_last_busy(dev->dev);
+ pm_runtime_put_autosuspend(dev->dev);
+
return 0;
err_unuse_clocks:
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
return r;
}
static int davinci_i2c_remove(struct platform_device *pdev)
{
struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
+ int ret;
i2c_davinci_cpufreq_deregister(dev);
i2c_del_adapter(&dev->adapter);
- clk_disable_unprepare(dev->clk);
- dev->clk = NULL;
+ ret = pm_runtime_get_sync(&pdev->dev);
+ if (ret < 0) {
+ pm_runtime_put_noidle(&pdev->dev);
+ return ret;
+ }
davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
+ pm_runtime_dont_use_autosuspend(dev->dev);
+ pm_runtime_put_sync(dev->dev);
+ pm_runtime_disable(dev->dev);
+
return 0;
}
@@ -880,7 +923,6 @@ static int davinci_i2c_suspend(struct device *dev)
/* put I2C into reset */
davinci_i2c_reset_ctrl(i2c_dev, 0);
- clk_disable_unprepare(i2c_dev->clk);
return 0;
}
@@ -889,7 +931,6 @@ static int davinci_i2c_resume(struct device *dev)
{
struct davinci_i2c_dev *i2c_dev = dev_get_drvdata(dev);
- clk_prepare_enable(i2c_dev->clk);
/* take I2C out of reset */
davinci_i2c_reset_ctrl(i2c_dev, 1);
@@ -899,6 +940,8 @@ static int davinci_i2c_resume(struct device *dev)
static const struct dev_pm_ops davinci_i2c_pm = {
.suspend = davinci_i2c_suspend,
.resume = davinci_i2c_resume,
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+ pm_runtime_force_resume)
};
#define davinci_i2c_pm_ops (&davinci_i2c_pm)
--
2.9.4.dirty
Hi,
On 12 September 2017 at 04:11, Franklin S Cooper Jr <[email protected]> wrote:
> 66AK2G has I2C instances that are not apart of the ALWAYS_ON power domain
> unlike other Keystone 2 SoCs and OMAPL138. Therefore, pm_runtime
> is required to insure the power domain used by the specific I2C instance is
> properly turned on along with its functional clock.
>
> Signed-off-by: Franklin S Cooper Jr <[email protected]>
> ---
> Version 4 changes:
> Fix wording on error message
> Fix additional error path
>
> Version 3 changes:
> Remove several statements that set clk to NULL
> Fix error path
>
> drivers/i2c/busses/i2c-davinci.c | 67 +++++++++++++++++++++++++++++++++-------
> 1 file changed, 55 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index b8c4353..a2d9f7c 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -36,6 +36,7 @@
> #include <linux/gpio.h>
> #include <linux/of_device.h>
> #include <linux/platform_data/i2c-davinci.h>
> +#include <linux/pm_runtime.h>
>
> /* ----- global defines ----------------------------------------------- */
>
> @@ -122,6 +123,9 @@
> /* set the SDA GPIO low */
> #define DAVINCI_I2C_DCLR_PDCLR1 BIT(1)
>
> +/* timeout for pm runtime autosuspend */
> +#define DAVINCI_I2C_PM_TIMEOUT 1000 /* ms */
> +
> struct davinci_i2c_dev {
> struct device *dev;
> void __iomem *base;
> @@ -541,10 +545,17 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>
> dev_dbg(dev->dev, "%s: msgs: %d\n", __func__, num);
>
> + ret = pm_runtime_get_sync(dev->dev);
> + if (ret < 0) {
> + dev_err(dev->dev, "Failed to runtime_get device: %d\n", ret);
> + pm_runtime_put_noidle(dev->dev);
> + return ret;
> + }
> +
> ret = i2c_davinci_wait_bus_not_busy(dev);
> if (ret < 0) {
> dev_warn(dev->dev, "timeout waiting for bus ready\n");
> - return ret;
> + goto out;
> }
>
> for (i = 0; i < num; i++) {
> @@ -552,14 +563,19 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
> dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num,
> ret);
> if (ret < 0)
> - return ret;
> + goto out;
> }
>
> + ret = num;
> #ifdef CONFIG_CPU_FREQ
> complete(&dev->xfr_complete);
> #endif
>
> - return num;
> +out:
> + pm_runtime_mark_last_busy(dev->dev);
> + pm_runtime_put_autosuspend(dev->dev);
> +
> + return ret;
> }
>
> static u32 i2c_davinci_func(struct i2c_adapter *adap)
> @@ -599,6 +615,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
> int count = 0;
> u16 w;
>
> + if (pm_runtime_suspended(dev->dev))
> + return IRQ_NONE;
> +
> while ((stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG))) {
> dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
> if (count++ == 100) {
> @@ -802,13 +821,24 @@ static int davinci_i2c_probe(struct platform_device *pdev)
> dev->clk = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(dev->clk))
> return PTR_ERR(dev->clk);
> - clk_prepare_enable(dev->clk);
You removed clk enable here, I think it can not work if we did not
open CONFIG_PM macro. I think you should keep clk enable here, and set
rpm active by pm_runtime_set_active() before issuing
pm_runtime_enable().
>
> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> dev->base = devm_ioremap_resource(&pdev->dev, mem);
> if (IS_ERR(dev->base)) {
> - r = PTR_ERR(dev->base);
> - goto err_unuse_clocks;
> + return PTR_ERR(dev->base);
> + }
> +
> + pm_runtime_set_autosuspend_delay(dev->dev,
> + DAVINCI_I2C_PM_TIMEOUT);
> + pm_runtime_use_autosuspend(dev->dev);
> +
> + pm_runtime_enable(dev->dev);
> +
> + r = pm_runtime_get_sync(dev->dev);
> + if (r < 0) {
> + dev_err(dev->dev, "failed to runtime_get device: %d\n", r);
> + pm_runtime_put_noidle(dev->dev);
> + return r;
> }
>
> i2c_davinci_init(dev);
> @@ -849,27 +879,40 @@ static int davinci_i2c_probe(struct platform_device *pdev)
> if (r)
> goto err_unuse_clocks;
>
> + pm_runtime_mark_last_busy(dev->dev);
> + pm_runtime_put_autosuspend(dev->dev);
> +
> return 0;
>
> err_unuse_clocks:
> - clk_disable_unprepare(dev->clk);
> - dev->clk = NULL;
> + pm_runtime_dont_use_autosuspend(dev->dev);
> + pm_runtime_put_sync(dev->dev);
> + pm_runtime_disable(dev->dev);
> +
> return r;
> }
>
> static int davinci_i2c_remove(struct platform_device *pdev)
> {
> struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
> + int ret;
>
> i2c_davinci_cpufreq_deregister(dev);
>
> i2c_del_adapter(&dev->adapter);
>
> - clk_disable_unprepare(dev->clk);
> - dev->clk = NULL;
> + ret = pm_runtime_get_sync(&pdev->dev);
> + if (ret < 0) {
> + pm_runtime_put_noidle(&pdev->dev);
> + return ret;
> + }
>
> davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
>
> + pm_runtime_dont_use_autosuspend(dev->dev);
> + pm_runtime_put_sync(dev->dev);
> + pm_runtime_disable(dev->dev);
> +
> return 0;
> }
>
> @@ -880,7 +923,6 @@ static int davinci_i2c_suspend(struct device *dev)
>
> /* put I2C into reset */
> davinci_i2c_reset_ctrl(i2c_dev, 0);
> - clk_disable_unprepare(i2c_dev->clk);
>
> return 0;
> }
> @@ -889,7 +931,6 @@ static int davinci_i2c_resume(struct device *dev)
> {
> struct davinci_i2c_dev *i2c_dev = dev_get_drvdata(dev);
>
> - clk_prepare_enable(i2c_dev->clk);
> /* take I2C out of reset */
> davinci_i2c_reset_ctrl(i2c_dev, 1);
>
> @@ -899,6 +940,8 @@ static int davinci_i2c_resume(struct device *dev)
> static const struct dev_pm_ops davinci_i2c_pm = {
> .suspend = davinci_i2c_suspend,
> .resume = davinci_i2c_resume,
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> + pm_runtime_force_resume)
> };
>
> #define davinci_i2c_pm_ops (&davinci_i2c_pm)
> --
> 2.9.4.dirty
>
--
Baolin.wang
Best Regards
On Tuesday 12 September 2017 07:28 AM, Baolin Wang wrote:
>> @@ -802,13 +821,24 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>> dev->clk = devm_clk_get(&pdev->dev, NULL);
>> if (IS_ERR(dev->clk))
>> return PTR_ERR(dev->clk);
>> - clk_prepare_enable(dev->clk);
> You removed clk enable here, I think it can not work if we did not
> open CONFIG_PM macro. I think you should keep clk enable here, and set
What do you mean by "open CONFIG_PM macro" ?
> rpm active by pm_runtime_set_active() before issuing
> pm_runtime_enable().
Can you explain why you want to do this instead of relying on
pm_runtime_get_sync() to enable clock?
Thanks,
Sekhar
Hi,
On 12 September 2017 at 16:48, Sekhar Nori <[email protected]> wrote:
> On Tuesday 12 September 2017 07:28 AM, Baolin Wang wrote:
>>> @@ -802,13 +821,24 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>> dev->clk = devm_clk_get(&pdev->dev, NULL);
>>> if (IS_ERR(dev->clk))
>>> return PTR_ERR(dev->clk);
>>> - clk_prepare_enable(dev->clk);
>
>> You removed clk enable here, I think it can not work if we did not
>> open CONFIG_PM macro. I think you should keep clk enable here, and set
>
> What do you mean by "open CONFIG_PM macro" ?
If you did not open CONFIG_PM macro, then the pm_runtime_xxx() will be
dummy functions, but now the i2c driver can not work since you did not
enable clock, right?
>
>> rpm active by pm_runtime_set_active() before issuing
>> pm_runtime_enable().
>
> Can you explain why you want to do this instead of relying on
> pm_runtime_get_sync() to enable clock?
What I mean is you should compatible whether CONFIG_PM is enabled or not.
--
Baolin.wang
Best Regards
On Tuesday 12 September 2017 02:44 PM, Baolin Wang wrote:
> Hi,
>
> On 12 September 2017 at 16:48, Sekhar Nori <[email protected]> wrote:
>> On Tuesday 12 September 2017 07:28 AM, Baolin Wang wrote:
>>>> @@ -802,13 +821,24 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>>> dev->clk = devm_clk_get(&pdev->dev, NULL);
>>>> if (IS_ERR(dev->clk))
>>>> return PTR_ERR(dev->clk);
>>>> - clk_prepare_enable(dev->clk);
>>
>>> You removed clk enable here, I think it can not work if we did not
>>> open CONFIG_PM macro. I think you should keep clk enable here, and set
>>
>> What do you mean by "open CONFIG_PM macro" ?
>
> If you did not open CONFIG_PM macro, then the pm_runtime_xxx() will be
> dummy functions, but now the i2c driver can not work since you did not
> enable clock, right?
Ah, okay. I am not sure thats a concern on platforms on which this
driver is used. Without PM runtime support, most likely the platforms
will not boot and multiple drivers will fail.
Thanks,
Sekhar
On Tuesday 12 September 2017 01:41 AM, Franklin S Cooper Jr wrote:
> Add I2C support to 66AK2G. Primary requirement is to add PM
> Runtime support to the driver.
>
> This has been tested on following platforms by performing simple i2c test
> such as i2c detect and reading on board i2c devices:
> K2G GP evm
> OMAPL138
> K2L GP EVM
>
> and boot tested on:
> K2E GP EVM
> K2HK GP EVM
>
> Version 2 changes:
> Moved ordering of pm runtime calls
>
> Version 3 changes:
> Dropped first patch since it was pulled in.
> Remove clk = NULL statements
> Fix error path
>
> Version 4 changes:
> Fix typo in commit message
> Fix error path
Acked-by: Sekhar Nori <[email protected]>
Tested on DA850 LCDK board with some basic i2cdetect/i2cdump and
suspend/resume tests.
Thanks,
Sekhar