* Re: [PATCH] i2c-eg20t: fix race between i2c init and interrupt enable
From: Yadi @ 2016-09-02 10:05 UTC (permalink / raw)
To: jdelvare, wsa; +Cc: linux-i2c
In-Reply-To: <1472439415-4024-1-git-send-email-yadi.hu@windriver.com>
Hi,guys
it may more make sense to add a check point on interrupt handler in case
field 'pch_base_address' has been assigned a effective value when an
interrupt arrives.
So something like:
diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
index 137125b..4ac8a49 100644
--- a/drivers/i2c/busses/i2c-eg20t.c
+++ b/drivers/i2c/busses/i2c-eg20t.c
@@ -152,6 +152,7 @@ struct i2c_algo_pch_data {
int pch_buff_mode_en;
u32 pch_event_flag;
bool pch_i2c_xfer_in_progress;
+ bool initflag;
};
/**
@@ -635,6 +636,8 @@ static irqreturn_t pch_i2c_handler(int irq, void *pData)
u32 mode;
for (i = 0, flag = 0; i < adap_info->ch_num; i++) {
+ if (!adap_info->pch_data[i].initflag)
+ continue;
p = adap_info->pch_data[i].pch_base_address;
mode = ioread32(p + PCH_I2CMOD);
mode &= BUFFER_MODE | EEPROM_SR_MODE;
@@ -783,6 +786,7 @@ static int pch_i2c_probe(struct pci_dev *pdev,
for (i = 0; i < adap_info->ch_num; i++) {
pch_adap = &adap_info->pch_data[i].pch_adapter;
adap_info->pch_i2c_suspended = false;
+ adap_info->pch_data[i].initflag = false;
adap_info->pch_data[i].p_adapter_info = adap_info;
@@ -806,6 +810,7 @@ static int pch_i2c_probe(struct pci_dev *pdev,
pch_pci_err(pdev, "i2c_add_adapter[ch:%d]
FAILED\n", i);
goto err_add_adapter;
}
+ adap_info->pch_data[i].initflag = ture;
}
Compared with last one, which one is better?
Yadi
On 2016年08月29日 10:56, Yadi Hu wrote:
> From: "Yadi.hu" <yadi.hu@windriver.com>
>
> the driver executes request_irq() function before the base address
> of i2c registers is remapped in kernel space. If i2c controller
> shares an interrupt line with other devices,it is possible that
> an interrupt arrives immediately after request_irq() is called.
> Since the interrupt handler pch_i2c_handler() is active, it will
> read its own register to determine if this interrupt was from
> its own device.
> At this moment, base address of i2c registers is not remapped
> in kernel space yet,so the INT handler access a unknown address
> and a error occurs.
>
> Bad IO access at port 0x18 (return inl(port))
> Call Trace:
> [<c102c733>] warn_slowpath_common+0x73/0xa0
> [<c13109f5>] ? bad_io_access+0x45/0x50
> [<c13109f5>] ? bad_io_access+0x45/0x50
> [<c102c804>] warn_slowpath_fmt+0x34/0x40
> [<c13109f5>] bad_io_access+0x45/0x50
> [<c1310b62>] ioread32+0x22/0x40
> [<c14c60db>] pch_i2c_handler+0x3b/0x120
> [<c1092f34>] handle_irq_event_percpu+0x64/0x330
> [<c109323b>] handle_irq_event+0x3b/0x60
> [<c1095b50>] ? unmask_irq+0x30/0x30
> [<c1095ba0>] handle_fasteoi_irq+0x50/0xe0
> <IRQ> [<c16e7e78>] ? do_IRQ+0x48/0xc0
> [<c16e7f6f>] ? smp_apic_timer_interrupt+0x7f/0x17a
> [<c16e7da9>] ? common_interrupt+0x29/0x30
> [<c1008ebb>] ? mwait_idle+0x8b/0x2c0
> [<c1009e8e>] ? cpu_idle+0x9e/0xc0
> [<c16be408>] ? rest_init+0x6c/0x74
> [<c19f770a>] ? start_kernel+0x311/0x318
> [<c19f7231>] ? repair_env_string+0x51/0x51
> [<c19f7078>] ? i386_start_kernel+0x78/0x7d
> ---[ end trace eb3a1028f468a140 ]---
> i2c_eg20t 0000:05:0c.2: pch_i2c_handler :I2C-3 mode(0) is not supported
>
> Signed-off-by: Yadi.hu <yadi.hu@windriver.com>
> ---
> drivers/i2c/busses/i2c-eg20t.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
> index 137125b..48483a5 100644
> --- a/drivers/i2c/busses/i2c-eg20t.c
> +++ b/drivers/i2c/busses/i2c-eg20t.c
> @@ -773,13 +773,6 @@ static int pch_i2c_probe(struct pci_dev *pdev,
> /* Set the number of I2C channel instance */
> adap_info->ch_num = id->driver_data;
>
> - ret = request_irq(pdev->irq, pch_i2c_handler, IRQF_SHARED,
> - KBUILD_MODNAME, adap_info);
> - if (ret) {
> - pch_pci_err(pdev, "request_irq FAILED\n");
> - goto err_request_irq;
> - }
> -
> for (i = 0; i < adap_info->ch_num; i++) {
> pch_adap = &adap_info->pch_data[i].pch_adapter;
> adap_info->pch_i2c_suspended = false;
> @@ -808,16 +801,23 @@ static int pch_i2c_probe(struct pci_dev *pdev,
> }
> }
>
> + ret = request_irq(pdev->irq, pch_i2c_handler, IRQF_SHARED,
> + KBUILD_MODNAME, adap_info);
> + if (ret) {
> + pch_pci_err(pdev, "request_irq FAILED\n");
> + goto err_request_irq;
> + }
> +
> pci_set_drvdata(pdev, adap_info);
> pch_pci_dbg(pdev, "returns %d.\n", ret);
> return 0;
>
> +err_request_irq:
> + pci_iounmap(pdev, base_addr);
> err_add_adapter:
> for (j = 0; j < i; j++)
> i2c_del_adapter(&adap_info->pch_data[j].pch_adapter);
> free_irq(pdev->irq, adap_info);
> -err_request_irq:
> - pci_iounmap(pdev, base_addr);
> err_pci_iomap:
> pci_release_regions(pdev);
> err_pci_req:
^ permalink raw reply related
* Re: [PATCH] i2c-eg20t: use dynamically registered adapter number
From: Yadi @ 2016-09-02 9:44 UTC (permalink / raw)
To: Jean Delvare; +Cc: Wolfram Sang, linux-i2c
In-Reply-To: <20160826173019.1f939ce5@endymion>
On 2016年08月26日 23:30, Jean Delvare wrote:
> Hi Yadi,
>
> On Tue, 23 Aug 2016 17:05:58 +0800, Yadi Hu wrote:
>> From: Hu Yadi <Yadi.hu@windriver.com>
>>
>> The eg20t driver uses i2c_add_numbered_adapter() to register adapter:
>>
>> pch_adap->nr = i;
>> ret = i2c_add_numbered_adapter(pch_adap);
>>
>> Variable i is assigned to 0, it means that i2c_eg20t is the first adapter
>> by default. if another adapter registers before eg20t, above code return
>> error for index conflict:
>>
>> i2c_eg20t 0000:05:0c.2: pch_i2c_probe :i2c_add_adapter[ch:0] FAILED
>> i2c_eg20t: probe of 0000:05:0c.2 failed with error -16
>>
>> So, we can replace i2c_add_numbered_adapter() with i2c_add_adapter()
>> interface.since it dynamically allocates the index number.
> This does the exact opposite of:
>
> commit 07e8a51ff68353e01d795cceafbac9f54c49132b
> Author: Feng Tang <feng.tang@intel.com>
> Date: Thu Jan 12 15:38:02 2012 +0800
>
> i2c-eg20t: use i2c_add_numbered_adapter to get a fixed bus number
>
> So my understanding is that you are not supposed to register another
> i2c bus before the eg20t buses. What is the conflicting driver? Is it
> creating the buses with static number or not?
I am using one Kontron M2M, on which both i2c-eg20t and i2c-kempld are
used simultaneously.
since kempld always is firstly initialized and configured to register
dynamically, it always occupied the first bus-id. i.e. 0.
consequently, eg20t will complain to fail to register for no space for
"0" i2c-bus.
>
> Looking at
>
> commit 03bde7c31a360f814ca42101d60563b1b803dca1
> Author: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Date: Thu Mar 12 17:17:59 2015 +0100
>
> i2c: busses with dynamic ids should start after fixed ids for DT
>
> it could be that you need to set some OF attribute to reserve i2c bus
> numbers <= 1 for static usage. Assuming you use OF. Or is it automatic,
> Wolfram?
>
> If not, it may make sense to add a helper function exposing
> __i2c_first_dynamic_bus_num to drivers (something like
> i2c_is_dynamic_bus_num().) After all, i2c_add_numbered_adapter() mostly
> makes sense if static i2c device definitions exist. If not,
> i2c_add_adapter() is just as good. So something like:
>
> if (i2c_is_dynamic_bus_num(i))
> ret = i2c_add_adapter(pch_adap);
> else {
> pch_adap->nr = i;
> ret = i2c_add_numbered_adapter(pch_adap);
> }
>
> may make sense. Unless someone has a better idea.
I totally agree with you and send off V2 patch soon per your suggestion.
Yadi
>
>> Signed-off-by: Hu Yadi <Yadi.hu@windriver.com>
>> ---
>> drivers/i2c/busses/i2c-eg20t.c | 3 +--
>> 1 files changed, 1 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-eg20t.c b/drivers/i2c/busses/i2c-eg20t.c
>> index 7a51ddc..2f4c2af 100644
>> --- a/drivers/i2c/busses/i2c-eg20t.c
>> +++ b/drivers/i2c/busses/i2c-eg20t.c
>> @@ -913,8 +913,7 @@ static int __devinit pch_i2c_probe(struct pci_dev *pdev,
>>
>> pch_i2c_init(&adap_info->pch_data[i]);
>>
>> - pch_adap->nr = i;
>> - ret = i2c_add_numbered_adapter(pch_adap);
>> + ret = i2c_add_adapter(pch_adap);
> Coding style is wrong here. Please use tabs, as ./scripts/checkpatch.pl
> tells you. Always check your patches with this script before you post
> them, thanks.
>
>> if (ret) {
>> pch_pci_err(pdev, "i2c_add_adapter[ch:%d] FAILED\n", i);
>> goto err_add_adapter;
>
^ permalink raw reply
* Re: [PATCH] i2c: shmobile: Use ARCH_SHMOBILE instead of SUPERH
From: Simon Horman @ 2016-09-01 13:44 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Wolfram Sang, linux-i2c, linux-sh, linux-renesas-soc
In-Reply-To: <1472636113-26988-1-git-send-email-geert+renesas@glider.be>
On Wed, Aug 31, 2016 at 11:35:13AM +0200, Geert Uytterhoeven wrote:
> "i2c-sh_mobile" is used on sh7343, sh7366, sh7722, sh7723, and sh7724
> only. As all of the above select ARCH_SHMOBILE, restrict its driver
> dependencies from SUPERH to ARCH_SHMOBILE.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> drivers/i2c/busses/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 1f3239ead0377426..6d94e2ec5b4f7183 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -836,7 +836,7 @@ config I2C_SH7760
> config I2C_SH_MOBILE
> tristate "SuperH Mobile I2C Controller"
> depends on HAS_DMA
> - depends on SUPERH || ARCH_RENESAS || COMPILE_TEST
> + depends on ARCH_SHMOBILE || ARCH_RENESAS || COMPILE_TEST
> help
> If you say yes to this option, support will be included for the
> built-in I2C interface on the Renesas SH-Mobile processor.
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sh" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* [PATCH 3/3] i2c: uniphier-f: set the adapter to master mode when probing
From: Masahiro Yamada @ 2016-09-01 11:46 UTC (permalink / raw)
To: linux-i2c; +Cc: Masahiro Yamada, linux-arm-kernel, linux-kernel, Wolfram Sang
In-Reply-To: <1472730390-8907-1-git-send-email-yamada.masahiro@socionext.com>
Currently, the adapter is set to the master mode at the first use.
Since then, it is kept in the slave mode, so unexpected glitch
signals on the I2C lines may cause the adapter into insane state.
Setting it to the master mode along with initialization solves the
problem.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Reported-by: Akio Noda <noda.akio@socionext.com>
---
drivers/i2c/busses/i2c-uniphier-f.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-uniphier-f.c b/drivers/i2c/busses/i2c-uniphier-f.c
index 2886685..829df91 100644
--- a/drivers/i2c/busses/i2c-uniphier-f.c
+++ b/drivers/i2c/busses/i2c-uniphier-f.c
@@ -458,16 +458,20 @@ static struct i2c_bus_recovery_info uniphier_fi2c_bus_recovery_info = {
static void uniphier_fi2c_hw_init(struct uniphier_fi2c_priv *priv,
u32 bus_speed, unsigned long clk_rate)
{
- u32 clk_count;
+ u32 tmp;
+
+ tmp = readl(priv->membase + UNIPHIER_FI2C_CR);
+ tmp |= UNIPHIER_FI2C_CR_MST;
+ writel(tmp, priv->membase + UNIPHIER_FI2C_CR);
uniphier_fi2c_reset(priv);
- clk_count = clk_rate / bus_speed;
+ tmp = clk_rate / bus_speed;
- writel(clk_count, priv->membase + UNIPHIER_FI2C_CYC);
- writel(clk_count / 2, priv->membase + UNIPHIER_FI2C_LCTL);
- writel(clk_count / 2, priv->membase + UNIPHIER_FI2C_SSUT);
- writel(clk_count / 16, priv->membase + UNIPHIER_FI2C_DSUT);
+ writel(tmp, priv->membase + UNIPHIER_FI2C_CYC);
+ writel(tmp / 2, priv->membase + UNIPHIER_FI2C_LCTL);
+ writel(tmp / 2, priv->membase + UNIPHIER_FI2C_SSUT);
+ writel(tmp / 16, priv->membase + UNIPHIER_FI2C_DSUT);
uniphier_fi2c_prepare_operation(priv);
}
--
1.9.1
^ permalink raw reply related
* [PATCH 2/3] i2c: uniphier-f: avoid WARN_ON() of clk_disable() in failure path
From: Masahiro Yamada @ 2016-09-01 11:46 UTC (permalink / raw)
To: linux-i2c; +Cc: Masahiro Yamada, linux-arm-kernel, linux-kernel, Wolfram Sang
In-Reply-To: <1472730390-8907-1-git-send-email-yamada.masahiro@socionext.com>
If clk_prepare_enable() fails, clk_disable_unprepare() is called in
the failure path, where the enable_count is still zero, so it hits
WARN_ON(core->enable_count == 0) in the clk_core_disable().
To fix this, make the clock setting more linear in the probe function
so that it can exploit "goto err" in case of error.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
drivers/i2c/busses/i2c-uniphier-f.c | 73 ++++++++++++++++---------------------
1 file changed, 32 insertions(+), 41 deletions(-)
diff --git a/drivers/i2c/busses/i2c-uniphier-f.c b/drivers/i2c/busses/i2c-uniphier-f.c
index 3560853..2886685 100644
--- a/drivers/i2c/busses/i2c-uniphier-f.c
+++ b/drivers/i2c/busses/i2c-uniphier-f.c
@@ -455,41 +455,10 @@ static struct i2c_bus_recovery_info uniphier_fi2c_bus_recovery_info = {
.unprepare_recovery = uniphier_fi2c_unprepare_recovery,
};
-static int uniphier_fi2c_clk_init(struct device *dev,
- struct uniphier_fi2c_priv *priv)
+static void uniphier_fi2c_hw_init(struct uniphier_fi2c_priv *priv,
+ u32 bus_speed, unsigned long clk_rate)
{
- struct device_node *np = dev->of_node;
- unsigned long clk_rate;
- u32 bus_speed, clk_count;
- int ret;
-
- if (of_property_read_u32(np, "clock-frequency", &bus_speed))
- bus_speed = UNIPHIER_FI2C_DEFAULT_SPEED;
-
- if (!bus_speed) {
- dev_err(dev, "clock-frequency should not be zero\n");
- return -EINVAL;
- }
-
- if (bus_speed > UNIPHIER_FI2C_MAX_SPEED)
- bus_speed = UNIPHIER_FI2C_MAX_SPEED;
-
- /* Get input clk rate through clk driver */
- priv->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(priv->clk)) {
- dev_err(dev, "failed to get clock\n");
- return PTR_ERR(priv->clk);
- }
-
- ret = clk_prepare_enable(priv->clk);
- if (ret)
- return ret;
-
- clk_rate = clk_get_rate(priv->clk);
- if (!clk_rate) {
- dev_err(dev, "input clock rate should not be zero\n");
- return -EINVAL;
- }
+ u32 clk_count;
uniphier_fi2c_reset(priv);
@@ -501,8 +470,6 @@ static int uniphier_fi2c_clk_init(struct device *dev,
writel(clk_count / 16, priv->membase + UNIPHIER_FI2C_DSUT);
uniphier_fi2c_prepare_operation(priv);
-
- return 0;
}
static int uniphier_fi2c_probe(struct platform_device *pdev)
@@ -510,8 +477,9 @@ static int uniphier_fi2c_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct uniphier_fi2c_priv *priv;
struct resource *regs;
- int irq;
- int ret;
+ u32 bus_speed;
+ unsigned long clk_rate;
+ int irq, ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
@@ -528,6 +496,31 @@ static int uniphier_fi2c_probe(struct platform_device *pdev)
return irq;
}
+ if (of_property_read_u32(dev->of_node, "clock-frequency", &bus_speed))
+ bus_speed = UNIPHIER_FI2C_DEFAULT_SPEED;
+
+ if (!bus_speed || bus_speed > UNIPHIER_FI2C_MAX_SPEED) {
+ dev_err(dev, "invalid clock-frequency %d\n", bus_speed);
+ return -EINVAL;
+ }
+
+ priv->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev, "failed to get clock\n");
+ return PTR_ERR(priv->clk);
+ }
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ clk_rate = clk_get_rate(priv->clk);
+ if (!clk_rate) {
+ dev_err(dev, "input clock rate should not be zero\n");
+ ret = -EINVAL;
+ goto err;
+ }
+
init_completion(&priv->comp);
priv->adap.owner = THIS_MODULE;
priv->adap.algo = &uniphier_fi2c_algo;
@@ -538,9 +531,7 @@ static int uniphier_fi2c_probe(struct platform_device *pdev)
i2c_set_adapdata(&priv->adap, priv);
platform_set_drvdata(pdev, priv);
- ret = uniphier_fi2c_clk_init(dev, priv);
- if (ret)
- goto err;
+ uniphier_fi2c_hw_init(priv, bus_speed, clk_rate);
ret = devm_request_irq(dev, irq, uniphier_fi2c_interrupt, 0,
pdev->name, priv);
--
1.9.1
^ permalink raw reply related
* [PATCH 1/3] i2c: uniphier: avoid WARN_ON() of clk_disable() in failure path
From: Masahiro Yamada @ 2016-09-01 11:46 UTC (permalink / raw)
To: linux-i2c; +Cc: Masahiro Yamada, linux-kernel, linux-arm-kernel, Wolfram Sang
In-Reply-To: <1472730390-8907-1-git-send-email-yamada.masahiro@socionext.com>
If clk_prepare_enable() fails, clk_disable_unprepare() is called in
the failure path, where the enable_count is still zero, so it hits
WARN_ON(core->enable_count == 0) in the clk_core_disable().
To fix this, make the clock setting more linear in the probe function
so that it can exploit "goto err" in case of error.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
drivers/i2c/busses/i2c-uniphier.c | 73 +++++++++++++++++----------------------
1 file changed, 31 insertions(+), 42 deletions(-)
diff --git a/drivers/i2c/busses/i2c-uniphier.c b/drivers/i2c/busses/i2c-uniphier.c
index d6e612a..56e92af 100644
--- a/drivers/i2c/busses/i2c-uniphier.c
+++ b/drivers/i2c/busses/i2c-uniphier.c
@@ -316,50 +316,15 @@ static struct i2c_bus_recovery_info uniphier_i2c_bus_recovery_info = {
.unprepare_recovery = uniphier_i2c_unprepare_recovery,
};
-static int uniphier_i2c_clk_init(struct device *dev,
- struct uniphier_i2c_priv *priv)
+static void uniphier_i2c_hw_init(struct uniphier_i2c_priv *priv,
+ u32 bus_speed, unsigned long clk_rate)
{
- struct device_node *np = dev->of_node;
- unsigned long clk_rate;
- u32 bus_speed;
- int ret;
-
- if (of_property_read_u32(np, "clock-frequency", &bus_speed))
- bus_speed = UNIPHIER_I2C_DEFAULT_SPEED;
-
- if (!bus_speed) {
- dev_err(dev, "clock-frequency should not be zero\n");
- return -EINVAL;
- }
-
- if (bus_speed > UNIPHIER_I2C_MAX_SPEED)
- bus_speed = UNIPHIER_I2C_MAX_SPEED;
-
- /* Get input clk rate through clk driver */
- priv->clk = devm_clk_get(dev, NULL);
- if (IS_ERR(priv->clk)) {
- dev_err(dev, "failed to get clock\n");
- return PTR_ERR(priv->clk);
- }
-
- ret = clk_prepare_enable(priv->clk);
- if (ret)
- return ret;
-
- clk_rate = clk_get_rate(priv->clk);
- if (!clk_rate) {
- dev_err(dev, "input clock rate should not be zero\n");
- return -EINVAL;
- }
-
uniphier_i2c_reset(priv, true);
writel((clk_rate / bus_speed / 2 << 16) | (clk_rate / bus_speed),
priv->membase + UNIPHIER_I2C_CLK);
uniphier_i2c_reset(priv, false);
-
- return 0;
}
static int uniphier_i2c_probe(struct platform_device *pdev)
@@ -367,8 +332,9 @@ static int uniphier_i2c_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct uniphier_i2c_priv *priv;
struct resource *regs;
- int irq;
- int ret;
+ u32 bus_speed;
+ unsigned long clk_rate;
+ int irq, ret;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
@@ -385,6 +351,31 @@ static int uniphier_i2c_probe(struct platform_device *pdev)
return irq;
}
+ if (of_property_read_u32(dev->of_node, "clock-frequency", &bus_speed))
+ bus_speed = UNIPHIER_I2C_DEFAULT_SPEED;
+
+ if (!bus_speed || bus_speed > UNIPHIER_I2C_MAX_SPEED) {
+ dev_err(dev, "invalid clock-frequency %d\n", bus_speed);
+ return -EINVAL;
+ }
+
+ priv->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(priv->clk)) {
+ dev_err(dev, "failed to get clock\n");
+ return PTR_ERR(priv->clk);
+ }
+
+ ret = clk_prepare_enable(priv->clk);
+ if (ret)
+ return ret;
+
+ clk_rate = clk_get_rate(priv->clk);
+ if (!clk_rate) {
+ dev_err(dev, "input clock rate should not be zero\n");
+ ret = -EINVAL;
+ goto err;
+ }
+
init_completion(&priv->comp);
priv->adap.owner = THIS_MODULE;
priv->adap.algo = &uniphier_i2c_algo;
@@ -395,9 +386,7 @@ static int uniphier_i2c_probe(struct platform_device *pdev)
i2c_set_adapdata(&priv->adap, priv);
platform_set_drvdata(pdev, priv);
- ret = uniphier_i2c_clk_init(dev, priv);
- if (ret)
- goto err;
+ uniphier_i2c_hw_init(priv, bus_speed, clk_rate);
ret = devm_request_irq(dev, irq, uniphier_i2c_interrupt, 0, pdev->name,
priv);
--
1.9.1
^ permalink raw reply related
* [PATCH 0/3] i2c: uniphier: updates for v4.9
From: Masahiro Yamada @ 2016-09-01 11:46 UTC (permalink / raw)
To: linux-i2c; +Cc: Masahiro Yamada, linux-arm-kernel, linux-kernel, Wolfram Sang
Masahiro Yamada (3):
i2c: uniphier: avoid WARN_ON() of clk_disable() in failure path
i2c: uniphier-f: avoid WARN_ON() of clk_disable() in failure path
i2c: uniphier-f: set the adapter to master mode when probing
drivers/i2c/busses/i2c-uniphier-f.c | 85 +++++++++++++++++--------------------
drivers/i2c/busses/i2c-uniphier.c | 73 ++++++++++++++-----------------
2 files changed, 71 insertions(+), 87 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH v4 1/2] Documentation: Add sbs-manager device tree node documentation
From: Phil Reid @ 2016-09-01 9:37 UTC (permalink / raw)
To: karl-heinz, Rob Herring
Cc: devicetree, linux-pm, linux-acpi, linux-i2c, Mark Rutland,
Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Rafael J . Wysocki, Peter Rosin
In-Reply-To: <4d255da80e88b400bc6f481b7fa48403@schneider-inet.de>
Sorry for the resend, I noticed my reply also got horrible mangled.
On 1/09/2016 06:34, Karl-Heinz Schneider wrote:
> Hi Rob,
>
> Sorry for resending this mail, evolution mixed it up...
>
> On Wed, 31 Aug 2016 09:43:49 -0500, Rob Herring wrote:
>> On Thu, Aug 25, 2016 at 10:21:00PM +0200, Karl-Heinz Schneider wrote:
>>> This patch adds device tree documentation for the sbs-manager
>>>
>>> Reviewed-by: Phil Reid <preid@electromag.com.au>
>>> Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
>>> ---
>>> .../devicetree/bindings/power/sbs,sbs-manager.txt | 53 ++++++++++++++++++++++
>>> 1 file changed, 53 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>>> new file mode 100644
>>> index 0000000..6b1a87ce
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>>> @@ -0,0 +1,53 @@
>>> +Binding for sbs-manager
>>> +
>>> +Required properties:
>>> +- compatible: should be "lltc,ltc1760" or use "sbs,sbs-manager" as fallback.
>>> +- reg: integer, i2c address of the device. Should be <0xa>.
>>
>> What happened to adding Phil's interrupt support into this? You don't
>> have to add the whole patch, just the binding part. The driver support
>> can come later.
>
> Phil revoked his patch. Right now it's not clear how the interrupt/gpio
> solution will look like and if they will need an device tree binding at
> all...
>
I'm getting back to this now.
I think we can use the smbalert driver to handle the source interrupt.
Which I think doesn't need anything in the dt binding. Driver just needs to
implement the alert callback.
I haven't figured out if the smb_alert could be used for the sbs-battery
driver interrupt or if this would make sense. However I think it would still need
to be a gpio controller in either case so that it can report the presence of the
batteries in a manner suitable for the sbs-battery driver to use.
I think the gpio controller approach is the simpler method.
It looks like the smbalert_driver driver needs some updates to work with device trees.
I need a bit more time to investigate this option.
--
Regards
Phil Reid
^ permalink raw reply
* Re: [PATCH v4 1/2] Documentation: Add sbs-manager device tree node documentation
From: Phil Reid @ 2016-09-01 9:00 UTC (permalink / raw)
To: Karl-Heinz Schneider, Rob Herring
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-pm-u79uwXL29TY76Z2rM5mHXA,
linux-acpi-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA, Mark Rutland, Sebastian Reichel,
Dmitry Eremin-Solenikov, David Woodhouse, Rafael J . Wysocki,
Peter Rosin
In-Reply-To: <1472673151.5102.12.camel-X5L7DgJ4l23oE99TX8zNy7NAH6kLmebB@public.gmane.org>
On 1/09/2016 03:52, Karl-Heinz Schneider wrote:
> Hi Rob,
>
> Am Mittwoch, den 31.08.2016, 09:43 -0500 schrieb Rob Herring:
>> On Thu, Aug 25, 2016 at 10:21:00PM +0200, Karl-Heinz Schneider wrote:
>>>
>>> This patch adds device tree documentation for the sbs-manager
>>>
>>> Reviewed-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
>>> Signed-off-by: Karl-Heinz Schneider <karl-heinz-X5L7DgJ4l23oE99TX8zNy7NAH6kLmebB@public.gmane.org>
>>> ---
>>> .../devicetree/bindings/power/sbs,sbs-manager.txt | 53
>>> ++++++++++++++++++++++
>>> 1 file changed, 53 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-
>>> manager.txt b/Documentation/devicetree/bindings/power/sbs,sbs-
>>> manager.txt
>>> new file mode 100644
>>> index 0000000..6b1a87ce
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>>> @@ -0,0 +1,53 @@
>>> +Binding for sbs-manager
>>> +
>>> +Required properties:
>>> +- compatible: should be "lltc,ltc1760" or use "sbs,sbs-manager" as
>>> fallback.
>>> +- reg: integer, i2c address of the device. Should be <0xa>.
>> What happened to adding Phil's interrupt support into this? You
>> don't
>> have to add the whole patch, just the binding part. The driver
>> support
>> can come later.
>
> Phil revoked his patch. Right now it's not clear how the interrupt/gpio
> solution will look like and if they will need an device tree binding at
> all...
I'm getting back to this now.
I think we can use the smbalert driver to handle the source interrupt.
Which I think doesn't need anything in the dt binding. Driver just needs to
implement the alert callback.
I haven't figured out if the smb_alert could be used for the sbs-battery
driver interrupt or if this would make sense. However I think it would still need
to be a gpio controller in either case so that it can report the presence of the
batteries in a manner suitable for the sbs-battery driver to use.
I think the gpio controller approach is the simpler method.
It looks like the smbalert_driver driver needs some updates to work with device trees.
I need a bit more time to investigate this option.
>
>>
>>>
>>> +
>>> +From OS view the device is basically an i2c-mux used to
>>> communicate with up to
>>> +four smart battery devices at address 0xb. The driver actually
>>> implements this
>>> +behaviour. So standard i2c-mux nodes can be used to register up to
>>> four slave
>>> +batteries. See Documentation/devicetree/bindings/i2c/i2c-mux.txt
>>> for more
>>> +information on i2c-mux nodes. Channels will be numerated starting
>>> from 1 to 4.
>>> +
>>> +Example:
>>> +
>>> +batman@0a {
>> drop leading 0.
>
> OK.
>
>>>
>>> + compatible = "lltc,ltc1760";
>>> + reg = <0x0a>;
>
> Will remove leading 0 at this places too.
>
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> +
>>> + i2c@1 {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + reg = <1>;
>>> +
>>> + battery@0b {
>> and here...
>
> OK.
>
>>>
>>> + compatible = "ti,bq2060", "sbs,sbs-battery";
>>> + reg = <0x0b>;
>>> + };
>>> + };
>>> +
>>> + i2c@2 {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + reg = <2>;
>>> +
>>> + battery@0b {
>>> + compatible = "ti,bq2060", "sbs,sbs-battery";
>>> + reg = <0x0b>;
>>> + };
>>> + };
>>> +
>>> + i2c@3 {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + reg = <3>;
>>> +
>>> + battery@0b {
>>> + compatible = "ti,bq2060", "sbs,sbs-battery";
>>> + reg = <0x0b>;
>>> + };
>>> + };
>>> +};
>
>
> Thanks for review Rob.
>
--
Regards
Phil Reid
ElectroMagnetic Imaging Technology Pty Ltd
Development of Geophysical Instrumentation & Software
www.electromag.com.au
3 The Avenue, Midland WA 6056, AUSTRALIA
Ph: +61 8 9250 8100
Fax: +61 8 9250 7100
Email: preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Greetings
From: Mrs Julie Leach @ 2016-09-01 2:29 UTC (permalink / raw)
To: Recipients
You are a recipient to Mrs Julie Leach Donation of $3 million USD. Contact ( julieleach811@outlook.com ) for claims.
^ permalink raw reply
* Re: [PATCH] i2c: rk3x: Restore clock settings at resume time
From: David.Wu @ 2016-09-01 2:10 UTC (permalink / raw)
To: Douglas Anderson, wsa, heiko
Cc: linux-rockchip, wxt, linux-arm-kernel, linux-i2c, linux-kernel
In-Reply-To: <1472505756-8302-1-git-send-email-dianders@chromium.org>
Hi Doug,
Last replied email HTML HEAD error, resend it.
在 2016/8/30 5:22, Douglas Anderson 写道:
> Depending on a number of factors including:
> - Which exact Rockchip SoC we're working with
> - How deep we suspend
> - Which i2c port we're on
>
> We might lose the state of the i2c registers at suspend time.
> Specifically we've found that on rk3399 the i2c ports that are not in
> the PMU power domain lose their state with the current suspend depth
> configured by ARM Tursted Firmware.
>
> Note that there are very few actual i2c registers that aren't configured
> per transfer anyway so all we actually need to re-configure are the
> clock config registers. We'll just add a call to rk3x_i2c_adapt_div()
> at resume time and be done with it.
>
> NOTE: On rk3399 on ports whose power was lost, I put printouts in at
> resume time. I saw things like:
> before: con=0x00010300, div=0x00060006
> after: con=0x00010200, div=0x00180025
>
I do the suspend/resume stress test on my rk3399 & rk3288 board more
than 24 hours,
the patch works well, it looks nice to me.
Reviewed-by: David Wu <david.wu@rock-chips.com>
Tested-by: David Wu <david.wu@rock-chips.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Tested on chromeos-4.4 kernel with backports.
>
> drivers/i2c/busses/i2c-rk3x.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-rk3x.c b/drivers/i2c/busses/i2c-rk3x.c
> index 3b87afe82f63..0df5ad6bfa31 100644
> --- a/drivers/i2c/busses/i2c-rk3x.c
> +++ b/drivers/i2c/busses/i2c-rk3x.c
> @@ -1111,6 +1111,15 @@ static int rk3x_i2c_xfer(struct i2c_adapter *adap,
> return ret < 0 ? ret : num;
> }
>
> +static __maybe_unused int rk3x_i2c_resume(struct device *dev)
> +{
> + struct rk3x_i2c *i2c = dev_get_drvdata(dev);
> +
> + rk3x_i2c_adapt_div(i2c, clk_get_rate(i2c->clk));
> +
> + return 0;
> +}
> +
> static u32 rk3x_i2c_func(struct i2c_adapter *adap)
> {
> return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_PROTOCOL_MANGLING;
> @@ -1332,12 +1341,15 @@ static int rk3x_i2c_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static const SIMPLE_DEV_PM_OPS(rk3x_i2c_pm_ops, NULL, rk3x_i2c_resume);
> +
> static struct platform_driver rk3x_i2c_driver = {
> .probe = rk3x_i2c_probe,
> .remove = rk3x_i2c_remove,
> .driver = {
> .name = "rk3x-i2c",
> .of_match_table = rk3x_i2c_match,
> + .pm = &rk3x_i2c_pm_ops,
> },
> };
>
>
^ permalink raw reply
* Re: [PATCH v4 1/2] Documentation: Add sbs-manager device tree node documentation
From: Karl-Heinz Schneider @ 2016-08-31 22:34 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, linux-pm, linux-acpi, linux-i2c, Mark Rutland,
Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Rafael J . Wysocki, Peter Rosin, Phil Reid
In-Reply-To: <20160831144349.GA18510@rob-hp-laptop>
Hi Rob,
Sorry for resending this mail, evolution mixed it up...
On Wed, 31 Aug 2016 09:43:49 -0500, Rob Herring wrote:
> On Thu, Aug 25, 2016 at 10:21:00PM +0200, Karl-Heinz Schneider wrote:
>> This patch adds device tree documentation for the sbs-manager
>>
>> Reviewed-by: Phil Reid <preid@electromag.com.au>
>> Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
>> ---
>> .../devicetree/bindings/power/sbs,sbs-manager.txt | 53
>> ++++++++++++++++++++++
>> 1 file changed, 53 insertions(+)
>> create mode 100644
>> Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>> b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>> new file mode 100644
>> index 0000000..6b1a87ce
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>> @@ -0,0 +1,53 @@
>> +Binding for sbs-manager
>> +
>> +Required properties:
>> +- compatible: should be "lltc,ltc1760" or use "sbs,sbs-manager" as
>> fallback.
>> +- reg: integer, i2c address of the device. Should be <0xa>.
>
> What happened to adding Phil's interrupt support into this? You don't
> have to add the whole patch, just the binding part. The driver
> support
> can come later.
Phil revoked his patch. Right now it's not clear how the interrupt/gpio
solution will look like and if they will need an device tree binding at
all...
>
>> +
>> +From OS view the device is basically an i2c-mux used to communicate
>> with up to
>> +four smart battery devices at address 0xb. The driver actually
>> implements this
>> +behaviour. So standard i2c-mux nodes can be used to register up to
>> four slave
>> +batteries. See Documentation/devicetree/bindings/i2c/i2c-mux.txt
>> for more
>> +information on i2c-mux nodes. Channels will be numerated starting
>> from 1 to 4.
>> +
>> +Example:
>> +
>> +batman@0a {
>
> drop leading 0.
OK.
>> + compatible = "lltc,ltc1760";
>> + reg = <0x0a>;
Will remove leading 0 at this places too.
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + i2c@1 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <1>;
>> +
>> + battery@0b {
>
> and here...
OK.
>
>> + compatible = "ti,bq2060", "sbs,sbs-battery";
>> + reg = <0x0b>;
>> + };
>> + };
>> +
>> + i2c@2 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <2>;
>> +
>> + battery@0b {
>> + compatible = "ti,bq2060", "sbs,sbs-battery";
>> + reg = <0x0b>;
>> + };
>> + };
>> +
>> + i2c@3 {
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + reg = <3>;
>> +
>> + battery@0b {
>> + compatible = "ti,bq2060", "sbs,sbs-battery";
>> + reg = <0x0b>;
>> + };
>> + };
>> +};
>> --
>> 2.7.4
>>
Thanks for review Rob.
--
Karl-Heinz
^ permalink raw reply
* Re: [PATCH v4 0/2] Add support for Smart Battery System Manager
From: Karl-Heinz Schneider @ 2016-08-31 19:54 UTC (permalink / raw)
To: Sebastian Reichel
Cc: devicetree, linux-pm, linux-acpi, linux-i2c, Rob Herring,
Mark Rutland, Dmitry Eremin-Solenikov, David Woodhouse,
Rafael J . Wysocki, Peter Rosin, Phil Reid
In-Reply-To: <20160831151044.xftg6k5f66n53jem@earth>
Hi Sebastian,
Am Mittwoch, den 31.08.2016, 17:10 +0200 schrieb Sebastian Reichel:
> Hi,
>
> On Thu, Aug 25, 2016 at 10:20:59PM +0200, Karl-Heinz Schneider wrote:
> >
> > This patch series adds support for Smart Battery System Manager.
> > A SBSM is a device listening at I2C/SMBus address 0x0a and is
> > capable of
> > communicating up to four I2C smart battery devices. All smart
> > battery
> > devices are listening at address 0x0b, so the SBSM muliplexes
> > between
> > them. The driver makes use of the I2C-Mux framework to allow smart
> > batteries to be bound via device tree, i.e. the sbs-battery driver.
> >
> > Via sysfs interface the online state and charge type are presented.
> > If
> > the driver is bound as ltc1760 (a Dual Smart Battery System
> > Manager)
> > the charge type can also be changed from trickle to fast.
> Driver looks fine to me. Waiting for Rob's ACK on the DT bindings.
>
> -- Sebastian
Thanks
--
Karl-Heinz
^ permalink raw reply
* Re: [PATCH v4 0/2] Add support for Smart Battery System Manager
From: Sebastian Reichel @ 2016-08-31 15:10 UTC (permalink / raw)
To: Karl-Heinz Schneider
Cc: devicetree, linux-pm, linux-acpi, linux-i2c, Rob Herring,
Mark Rutland, Dmitry Eremin-Solenikov, David Woodhouse,
Rafael J . Wysocki, Peter Rosin, Phil Reid
In-Reply-To: <1472156461-25180-1-git-send-email-karl-heinz@schneider-inet.de>
[-- Attachment #1: Type: text/plain, Size: 806 bytes --]
Hi,
On Thu, Aug 25, 2016 at 10:20:59PM +0200, Karl-Heinz Schneider wrote:
> This patch series adds support for Smart Battery System Manager.
> A SBSM is a device listening at I2C/SMBus address 0x0a and is capable of
> communicating up to four I2C smart battery devices. All smart battery
> devices are listening at address 0x0b, so the SBSM muliplexes between
> them. The driver makes use of the I2C-Mux framework to allow smart
> batteries to be bound via device tree, i.e. the sbs-battery driver.
>
> Via sysfs interface the online state and charge type are presented. If
> the driver is bound as ltc1760 (a Dual Smart Battery System Manager)
> the charge type can also be changed from trickle to fast.
Driver looks fine to me. Waiting for Rob's ACK on the DT bindings.
-- Sebastian
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/2] Documentation: Add sbs-manager device tree node documentation
From: Rob Herring @ 2016-08-31 14:43 UTC (permalink / raw)
To: Karl-Heinz Schneider
Cc: devicetree, linux-pm, linux-acpi, linux-i2c, Mark Rutland,
Sebastian Reichel, Dmitry Eremin-Solenikov, David Woodhouse,
Rafael J . Wysocki, Peter Rosin, Phil Reid
In-Reply-To: <1472156461-25180-2-git-send-email-karl-heinz@schneider-inet.de>
On Thu, Aug 25, 2016 at 10:21:00PM +0200, Karl-Heinz Schneider wrote:
> This patch adds device tree documentation for the sbs-manager
>
> Reviewed-by: Phil Reid <preid@electromag.com.au>
> Signed-off-by: Karl-Heinz Schneider <karl-heinz@schneider-inet.de>
> ---
> .../devicetree/bindings/power/sbs,sbs-manager.txt | 53 ++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
>
> diff --git a/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
> new file mode 100644
> index 0000000..6b1a87ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/sbs,sbs-manager.txt
> @@ -0,0 +1,53 @@
> +Binding for sbs-manager
> +
> +Required properties:
> +- compatible: should be "lltc,ltc1760" or use "sbs,sbs-manager" as fallback.
> +- reg: integer, i2c address of the device. Should be <0xa>.
What happened to adding Phil's interrupt support into this? You don't
have to add the whole patch, just the binding part. The driver support
can come later.
> +
> +From OS view the device is basically an i2c-mux used to communicate with up to
> +four smart battery devices at address 0xb. The driver actually implements this
> +behaviour. So standard i2c-mux nodes can be used to register up to four slave
> +batteries. See Documentation/devicetree/bindings/i2c/i2c-mux.txt for more
> +information on i2c-mux nodes. Channels will be numerated starting from 1 to 4.
> +
> +Example:
> +
> +batman@0a {
drop leading 0.
> + compatible = "lltc,ltc1760";
> + reg = <0x0a>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + i2c@1 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <1>;
> +
> + battery@0b {
and here...
> + compatible = "ti,bq2060", "sbs,sbs-battery";
> + reg = <0x0b>;
> + };
> + };
> +
> + i2c@2 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <2>;
> +
> + battery@0b {
> + compatible = "ti,bq2060", "sbs,sbs-battery";
> + reg = <0x0b>;
> + };
> + };
> +
> + i2c@3 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <3>;
> +
> + battery@0b {
> + compatible = "ti,bq2060", "sbs,sbs-battery";
> + reg = <0x0b>;
> + };
> + };
> +};
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH 1/5] i2c: tegra: use readl_poll_timeout after config_load reg programmed
From: Shardar Mohammed @ 2016-08-31 13:30 UTC (permalink / raw)
To: wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org,
swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Jonathan Hunter
In-Reply-To: <1472649758-5608-1-git-send-email-smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Sorry ignore this series, forgot to mention patch series version.
Pushed v11 series, please review v11 series.
Thanks,
Shardar
________________________________________
From: Shardar Mohammed
Sent: Wednesday, August 31, 2016 6:52 PM
To: Shardar Mohammed; wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org; swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org; thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; gnurou-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jonathan Hunter
Subject: [PATCH 1/5] i2c: tegra: use readl_poll_timeout after config_load reg programmed
After CONFIG_LOAD register is programmed instead of explicitly waiting
for timeout, use readl_poll_timeout() to check for register value to get
updated or wait till timeout.
Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v4:
- Split timeout calculation to separate patch
Changes in v5:
- Move disabling of clock to separate patch
Changes in v8:
- 1st change of [PATCH v7] series is merged, v8 is rebased on top of
merged change, here patch series is changed accordingly.
- Updated the commit message as per review to properly reflect change.
- calculate the register offset seperately to make code more readable.
Changes in v9:
- Use readl_poll_timeout() instead of readx_poll_timeout()
Changes in V10:
- Rebase on top of [PATCH V2 0/9] Some Tegra I2C Updates
Changes in V11:
- Fix sparse error
---
---
drivers/i2c/busses/i2c-tegra.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index d86a993..e93c72a 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -30,6 +30,7 @@
#include <linux/reset.h>
#include <linux/pinctrl/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/iopoll.h>
#include <asm/unaligned.h>
@@ -112,6 +113,8 @@
#define I2C_CLKEN_OVERRIDE 0x090
#define I2C_MST_CORE_CLKEN_OVR BIT(0)
+#define I2C_CONFIG_LOAD_TIMEOUT 1000000
+
/*
* msg_end_type: The bus control which need to be send at end of transfer.
* @MSG_END_STOP: Send stop pulse at end of transfer.
@@ -448,7 +451,6 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
u32 val;
int err;
u32 clk_divisor;
- unsigned long timeout = jiffies + HZ;
err = pm_runtime_get_sync(i2c_dev->dev);
if (err < 0) {
@@ -497,15 +499,18 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
if (i2c_dev->hw->has_config_load_reg) {
+ unsigned long reg_offset;
+ void __iomem *addr;
+
+ reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
+ addr = i2c_dev->base + reg_offset;
i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
- while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
- if (time_after(jiffies, timeout)) {
- dev_warn(i2c_dev->dev,
- "timeout waiting for config load\n");
- err = -ETIMEDOUT;
- goto err;
- }
- msleep(1);
+ err = readl_poll_timeout(addr, val, val == 0, 1000,
+ I2C_CONFIG_LOAD_TIMEOUT);
+ if (err) {
+ dev_warn(i2c_dev->dev,
+ "timeout waiting for config load\n");
+ goto err;
}
}
--
1.8.1.5
^ permalink raw reply related
* [PATCH v11 5/5] i2c: tegra: proper handling of error cases
From: Shardar Shariff Md @ 2016-08-31 13:28 UTC (permalink / raw)
To: smohammed, wsa, swarren, thierry.reding, gnurou, linux-i2c,
linux-tegra, linux-kernel, jonathanh, ldewangan
In-Reply-To: <1472650124-5702-1-git-send-email-smohammed@nvidia.com>
To summarize the issue observed in error cases:
SW Flow: For i2c message transfer, packet header and data payload is
posted and then required error/packet completion interrupts are enabled
later.
HW flow: HW process the packet just after packet header is posted, if
ARB lost/NACK error occurs (SW will not handle immediately when error
happens as error interrupts are not enabled at this point). HW assumes
error is acknowledged and clears current data in FIFO, But SW here posts
the remaining data payload which still stays in FIFO as stale data
(data without packet header).
Now once the interrupts are enabled, SW handles ARB lost/NACK error by
clearing the ARB lost/NACK interrupt. Now HW assumes that SW attended
the error and will parse/process stale data (data without packet header)
present in FIFO which causes invalid NACK errors.
Fix: Enable the error interrupts before posting the packet into FIFO
which make sure HW to not clear the fifo. Also disable the packet mode
before acknowledging errors (ARB lost/NACK error) to not process any
stale data. As error interrupts are enabled before posting the packet
header use spinlock to avoid preempting.
Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
---
Changes in v2:
- Align the commit message to 72 characters per line.
- Removing unnecessary paranthesis.
- Handle error in isr
Changes in v3:
- Printing error if tegra_i2c_disable_packet_mode() fails
is already present and handling error is not taken cared
in ISR which was done in v2 but keeping return error in
*wait_for_config_load() as its used in tegra_i2c_init()
Changes in V10:
- Rebase on top of [PATCH V2 0/9] Some Tegra I2C Updates
---
---
drivers/i2c/busses/i2c-tegra.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 2437535..c15d575 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -196,6 +196,7 @@ struct tegra_i2c_dev {
u16 clk_divisor_non_hs_mode;
bool is_suspended;
bool is_multimaster_mode;
+ spinlock_t xfer_lock;
};
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
@@ -542,14 +543,27 @@ err:
return err;
}
+static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev)
+{
+ u32 cnfg;
+
+ cnfg = i2c_readl(i2c_dev, I2C_CNFG);
+ if (cnfg & I2C_CNFG_PACKET_MODE_EN)
+ i2c_writel(i2c_dev, cnfg & ~I2C_CNFG_PACKET_MODE_EN, I2C_CNFG);
+
+ return tegra_i2c_wait_for_config_load(i2c_dev);
+}
+
static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
{
u32 status;
const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
struct tegra_i2c_dev *i2c_dev = dev_id;
+ unsigned long flags;
status = i2c_readl(i2c_dev, I2C_INT_STATUS);
+ spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
if (status == 0) {
dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS),
@@ -565,6 +579,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
}
if (unlikely(status & status_err)) {
+ tegra_i2c_disable_packet_mode(i2c_dev);
if (status & I2C_INT_NO_ACK)
i2c_dev->msg_err |= I2C_ERR_NO_ACK;
if (status & I2C_INT_ARBITRATION_LOST)
@@ -594,7 +609,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
BUG_ON(i2c_dev->msg_buf_remaining);
complete(&i2c_dev->msg_complete);
}
- return IRQ_HANDLED;
+ goto done;
err:
/* An error occurred, mask all interrupts */
tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
@@ -605,6 +620,8 @@ err:
dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
complete(&i2c_dev->msg_complete);
+done:
+ spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
return IRQ_HANDLED;
}
@@ -614,6 +631,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
u32 packet_header;
u32 int_mask;
unsigned long time_left;
+ unsigned long flags;
tegra_i2c_flush_fifos(i2c_dev);
@@ -626,6 +644,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
i2c_dev->msg_read = (msg->flags & I2C_M_RD);
reinit_completion(&i2c_dev->msg_complete);
+ spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
+
+ int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
+ tegra_i2c_unmask_irq(i2c_dev, int_mask);
+
packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
PACKET_HEADER0_PROTOCOL_I2C |
(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
@@ -655,14 +678,15 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
if (!(msg->flags & I2C_M_RD))
tegra_i2c_fill_tx_fifo(i2c_dev);
- int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
if (msg->flags & I2C_M_RD)
int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
else if (i2c_dev->msg_buf_remaining)
int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
+
tegra_i2c_unmask_irq(i2c_dev, int_mask);
+ spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
i2c_readl(i2c_dev, I2C_INT_MASK));
@@ -899,6 +923,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
"nvidia,tegra20-i2c-dvc");
init_completion(&i2c_dev->msg_complete);
+ spin_lock_init(&i2c_dev->xfer_lock);
if (!i2c_dev->hw->has_single_clk_source) {
fast_clk = devm_clk_get(&pdev->dev, "fast-clk");
--
1.8.1.5
^ permalink raw reply related
* [PATCH v11 4/5] i2c: tegra: use atomic poll function during configuration
From: Shardar Shariff Md @ 2016-08-31 13:28 UTC (permalink / raw)
To: smohammed, wsa, swarren, thierry.reding, gnurou, linux-i2c,
linux-tegra, linux-kernel, jonathanh, ldewangan
In-Reply-To: <1472650124-5702-1-git-send-email-smohammed@nvidia.com>
Use readl_poll_timeout_atomic() function as *wait_for_config_load()
function can be called from interrupt context.
Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
---
Changes in v10:
- Rebase on top of [PATCH V2 0/9] Some Tegra I2C Updates
Changes in v11:
- Fix checkpatch error of using in_atomic() API
---
---
drivers/i2c/busses/i2c-tegra.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index daab0ce..2437535 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -457,8 +457,13 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
addr = i2c_dev->base + reg_offset;
i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
- err = readl_poll_timeout(addr, val, val == 0, 1000,
- I2C_CONFIG_LOAD_TIMEOUT);
+ if (in_interrupt())
+ err = readl_poll_timeout_atomic(addr, val, val == 0,
+ 1000, I2C_CONFIG_LOAD_TIMEOUT);
+ else
+ err = readl_poll_timeout(addr, val, val == 0,
+ 1000, I2C_CONFIG_LOAD_TIMEOUT);
+
if (err) {
dev_warn(i2c_dev->dev,
"timeout waiting for config load\n");
--
1.8.1.5
^ permalink raw reply related
* [PATCH v11 3/5] i2c: tegra: add separate function for config_load programing
From: Shardar Shariff Md @ 2016-08-31 13:28 UTC (permalink / raw)
To: smohammed, wsa, swarren, thierry.reding, gnurou, linux-i2c,
linux-tegra, linux-kernel, jonathanh, ldewangan
In-Reply-To: <1472650124-5702-1-git-send-email-smohammed@nvidia.com>
Define separate function for configuration load register handling
to make it use by different functions later.
Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
---
Changes in v2:
- Remove unnecessary paranthesis and align to 80 characters per line
Changes in v3:
- Add separate function for config load handling
Changes in v4:
- Move timeout calculation to separate patch
Changes in v9:
- Rebase with the changes to earlier patch1
Changes in v10:
- Rebase on top of [PATCH V2 0/9] Some Tegra I2C Updates
---
---
drivers/i2c/busses/i2c-tegra.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 07a39b7..daab0ce 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -446,6 +446,29 @@ static int tegra_i2c_runtime_suspend(struct device *dev)
return pinctrl_pm_select_idle_state(i2c_dev->dev);
}
+static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
+{
+ unsigned long reg_offset;
+ void __iomem *addr;
+ u32 val;
+ int err;
+
+ if (i2c_dev->hw->has_config_load_reg) {
+ reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
+ addr = i2c_dev->base + reg_offset;
+ i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
+ err = readl_poll_timeout(addr, val, val == 0, 1000,
+ I2C_CONFIG_LOAD_TIMEOUT);
+ if (err) {
+ dev_warn(i2c_dev->dev,
+ "timeout waiting for config load\n");
+ return err;
+ }
+ }
+
+ return 0;
+}
+
static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
{
u32 val;
@@ -500,21 +523,9 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
if (i2c_dev->is_multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
- if (i2c_dev->hw->has_config_load_reg) {
- unsigned long reg_offset;
- void __iomem *addr;
-
- reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
- addr = i2c_dev->base + reg_offset;
- i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
- err = readl_poll_timeout(addr, val, val == 0, 1000,
- I2C_CONFIG_LOAD_TIMEOUT);
- if (err) {
- dev_warn(i2c_dev->dev,
- "timeout waiting for config load\n");
- goto err;
- }
- }
+ err = tegra_i2c_wait_for_config_load(i2c_dev);
+ if (err)
+ goto err;
if (i2c_dev->irq_disabled) {
i2c_dev->irq_disabled = 0;
--
1.8.1.5
^ permalink raw reply related
* [PATCH v11 2/5] i2c: tegra: If fifo flush fails return error
From: Shardar Shariff Md @ 2016-08-31 13:28 UTC (permalink / raw)
To: smohammed, wsa, swarren, thierry.reding, gnurou, linux-i2c,
linux-tegra, linux-kernel, jonathanh, ldewangan
In-Reply-To: <1472650124-5702-1-git-send-email-smohammed@nvidia.com>
During i2c controller initialization, when fifo flush fails return error
instead of returning the error during exit.
Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
---
Changes in v11:
- Fix smatch error for below warning
tegra_i2c_init warn: unused return: err = tegra_i2c_flush_fifos()
---
---
drivers/i2c/busses/i2c-tegra.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index e93c72a..07a39b7 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -494,6 +494,8 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, val, I2C_FIFO_CONTROL);
err = tegra_i2c_flush_fifos(i2c_dev);
+ if (err)
+ goto err;
if (i2c_dev->is_multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
--
1.8.1.5
^ permalink raw reply related
* [PATCH v11 1/5] i2c: tegra: use readl_poll_timeout after config_load reg programmed
From: Shardar Shariff Md @ 2016-08-31 13:28 UTC (permalink / raw)
To: smohammed-DDmLM1+adcrQT0dZR+AlfA, wsa-z923LK4zBo2bacvFa/9K2g,
swarren-3lzwWm7+Weoh9ZMKESR00Q,
thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
gnurou-Re5JQEeQqe8AvxtiuMwx3w, linux-i2c-u79uwXL29TY76Z2rM5mHXA,
linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
jonathanh-DDmLM1+adcrQT0dZR+AlfA,
ldewangan-DDmLM1+adcrQT0dZR+AlfA
After CONFIG_LOAD register is programmed instead of explicitly waiting
for timeout, use readl_poll_timeout() to check for register value to get
updated or wait till timeout.
Signed-off-by: Shardar Shariff Md <smohammed-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
Changes in v4:
- Split timeout calculation to separate patch
Changes in v5:
- Move disabling of clock to separate patch
Changes in v8:
- 1st change of [PATCH v7] series is merged, v8 is rebased on top of
merged change, here patch series is changed accordingly.
- Updated the commit message as per review to properly reflect change.
- calculate the register offset seperately to make code more readable.
Changes in v9:
- Use readl_poll_timeout() instead of readx_poll_timeout()
Changes in V10:
- Rebase on top of [PATCH V2 0/9] Some Tegra I2C Updates
Changes in V11:
- Fix sparse error
---
---
drivers/i2c/busses/i2c-tegra.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index d86a993..e93c72a 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -30,6 +30,7 @@
#include <linux/reset.h>
#include <linux/pinctrl/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/iopoll.h>
#include <asm/unaligned.h>
@@ -112,6 +113,8 @@
#define I2C_CLKEN_OVERRIDE 0x090
#define I2C_MST_CORE_CLKEN_OVR BIT(0)
+#define I2C_CONFIG_LOAD_TIMEOUT 1000000
+
/*
* msg_end_type: The bus control which need to be send at end of transfer.
* @MSG_END_STOP: Send stop pulse at end of transfer.
@@ -448,7 +451,6 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
u32 val;
int err;
u32 clk_divisor;
- unsigned long timeout = jiffies + HZ;
err = pm_runtime_get_sync(i2c_dev->dev);
if (err < 0) {
@@ -497,15 +499,18 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
if (i2c_dev->hw->has_config_load_reg) {
+ unsigned long reg_offset;
+ void __iomem *addr;
+
+ reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
+ addr = i2c_dev->base + reg_offset;
i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
- while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
- if (time_after(jiffies, timeout)) {
- dev_warn(i2c_dev->dev,
- "timeout waiting for config load\n");
- err = -ETIMEDOUT;
- goto err;
- }
- msleep(1);
+ err = readl_poll_timeout(addr, val, val == 0, 1000,
+ I2C_CONFIG_LOAD_TIMEOUT);
+ if (err) {
+ dev_warn(i2c_dev->dev,
+ "timeout waiting for config load\n");
+ goto err;
}
}
--
1.8.1.5
^ permalink raw reply related
* [PATCH 1/5] i2c: tegra: use readl_poll_timeout after config_load reg programmed
From: Shardar Shariff Md @ 2016-08-31 13:22 UTC (permalink / raw)
To: smohammed, wsa, swarren, thierry.reding, gnurou, linux-i2c,
linux-tegra, linux-kernel, jonathanh
After CONFIG_LOAD register is programmed instead of explicitly waiting
for timeout, use readl_poll_timeout() to check for register value to get
updated or wait till timeout.
Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
---
Changes in v4:
- Split timeout calculation to separate patch
Changes in v5:
- Move disabling of clock to separate patch
Changes in v8:
- 1st change of [PATCH v7] series is merged, v8 is rebased on top of
merged change, here patch series is changed accordingly.
- Updated the commit message as per review to properly reflect change.
- calculate the register offset seperately to make code more readable.
Changes in v9:
- Use readl_poll_timeout() instead of readx_poll_timeout()
Changes in V10:
- Rebase on top of [PATCH V2 0/9] Some Tegra I2C Updates
Changes in V11:
- Fix sparse error
---
---
drivers/i2c/busses/i2c-tegra.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index d86a993..e93c72a 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -30,6 +30,7 @@
#include <linux/reset.h>
#include <linux/pinctrl/consumer.h>
#include <linux/pm_runtime.h>
+#include <linux/iopoll.h>
#include <asm/unaligned.h>
@@ -112,6 +113,8 @@
#define I2C_CLKEN_OVERRIDE 0x090
#define I2C_MST_CORE_CLKEN_OVR BIT(0)
+#define I2C_CONFIG_LOAD_TIMEOUT 1000000
+
/*
* msg_end_type: The bus control which need to be send at end of transfer.
* @MSG_END_STOP: Send stop pulse at end of transfer.
@@ -448,7 +451,6 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
u32 val;
int err;
u32 clk_divisor;
- unsigned long timeout = jiffies + HZ;
err = pm_runtime_get_sync(i2c_dev->dev);
if (err < 0) {
@@ -497,15 +499,18 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
if (i2c_dev->hw->has_config_load_reg) {
+ unsigned long reg_offset;
+ void __iomem *addr;
+
+ reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
+ addr = i2c_dev->base + reg_offset;
i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
- while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
- if (time_after(jiffies, timeout)) {
- dev_warn(i2c_dev->dev,
- "timeout waiting for config load\n");
- err = -ETIMEDOUT;
- goto err;
- }
- msleep(1);
+ err = readl_poll_timeout(addr, val, val == 0, 1000,
+ I2C_CONFIG_LOAD_TIMEOUT);
+ if (err) {
+ dev_warn(i2c_dev->dev,
+ "timeout waiting for config load\n");
+ goto err;
}
}
--
1.8.1.5
^ permalink raw reply related
* [PATCH 5/5] i2c: tegra: proper handling of error cases
From: Shardar Shariff Md @ 2016-08-31 13:22 UTC (permalink / raw)
To: smohammed, wsa, swarren, thierry.reding, gnurou, linux-i2c,
linux-tegra, linux-kernel, jonathanh
In-Reply-To: <1472649758-5608-1-git-send-email-smohammed@nvidia.com>
To summarize the issue observed in error cases:
SW Flow: For i2c message transfer, packet header and data payload is
posted and then required error/packet completion interrupts are enabled
later.
HW flow: HW process the packet just after packet header is posted, if
ARB lost/NACK error occurs (SW will not handle immediately when error
happens as error interrupts are not enabled at this point). HW assumes
error is acknowledged and clears current data in FIFO, But SW here posts
the remaining data payload which still stays in FIFO as stale data
(data without packet header).
Now once the interrupts are enabled, SW handles ARB lost/NACK error by
clearing the ARB lost/NACK interrupt. Now HW assumes that SW attended
the error and will parse/process stale data (data without packet header)
present in FIFO which causes invalid NACK errors.
Fix: Enable the error interrupts before posting the packet into FIFO
which make sure HW to not clear the fifo. Also disable the packet mode
before acknowledging errors (ARB lost/NACK error) to not process any
stale data. As error interrupts are enabled before posting the packet
header use spinlock to avoid preempting.
Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
---
Changes in v2:
- Align the commit message to 72 characters per line.
- Removing unnecessary paranthesis.
- Handle error in isr
Changes in v3:
- Printing error if tegra_i2c_disable_packet_mode() fails
is already present and handling error is not taken cared
in ISR which was done in v2 but keeping return error in
*wait_for_config_load() as its used in tegra_i2c_init()
Changes in V10:
- Rebase on top of [PATCH V2 0/9] Some Tegra I2C Updates
---
---
drivers/i2c/busses/i2c-tegra.c | 29 +++++++++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 2437535..c15d575 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -196,6 +196,7 @@ struct tegra_i2c_dev {
u16 clk_divisor_non_hs_mode;
bool is_suspended;
bool is_multimaster_mode;
+ spinlock_t xfer_lock;
};
static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
@@ -542,14 +543,27 @@ err:
return err;
}
+static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev)
+{
+ u32 cnfg;
+
+ cnfg = i2c_readl(i2c_dev, I2C_CNFG);
+ if (cnfg & I2C_CNFG_PACKET_MODE_EN)
+ i2c_writel(i2c_dev, cnfg & ~I2C_CNFG_PACKET_MODE_EN, I2C_CNFG);
+
+ return tegra_i2c_wait_for_config_load(i2c_dev);
+}
+
static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
{
u32 status;
const u32 status_err = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
struct tegra_i2c_dev *i2c_dev = dev_id;
+ unsigned long flags;
status = i2c_readl(i2c_dev, I2C_INT_STATUS);
+ spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
if (status == 0) {
dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
i2c_readl(i2c_dev, I2C_PACKET_TRANSFER_STATUS),
@@ -565,6 +579,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
}
if (unlikely(status & status_err)) {
+ tegra_i2c_disable_packet_mode(i2c_dev);
if (status & I2C_INT_NO_ACK)
i2c_dev->msg_err |= I2C_ERR_NO_ACK;
if (status & I2C_INT_ARBITRATION_LOST)
@@ -594,7 +609,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
BUG_ON(i2c_dev->msg_buf_remaining);
complete(&i2c_dev->msg_complete);
}
- return IRQ_HANDLED;
+ goto done;
err:
/* An error occurred, mask all interrupts */
tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
@@ -605,6 +620,8 @@ err:
dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
complete(&i2c_dev->msg_complete);
+done:
+ spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
return IRQ_HANDLED;
}
@@ -614,6 +631,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
u32 packet_header;
u32 int_mask;
unsigned long time_left;
+ unsigned long flags;
tegra_i2c_flush_fifos(i2c_dev);
@@ -626,6 +644,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
i2c_dev->msg_read = (msg->flags & I2C_M_RD);
reinit_completion(&i2c_dev->msg_complete);
+ spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
+
+ int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
+ tegra_i2c_unmask_irq(i2c_dev, int_mask);
+
packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
PACKET_HEADER0_PROTOCOL_I2C |
(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
@@ -655,14 +678,15 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
if (!(msg->flags & I2C_M_RD))
tegra_i2c_fill_tx_fifo(i2c_dev);
- int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
if (msg->flags & I2C_M_RD)
int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
else if (i2c_dev->msg_buf_remaining)
int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
+
tegra_i2c_unmask_irq(i2c_dev, int_mask);
+ spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
i2c_readl(i2c_dev, I2C_INT_MASK));
@@ -899,6 +923,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
"nvidia,tegra20-i2c-dvc");
init_completion(&i2c_dev->msg_complete);
+ spin_lock_init(&i2c_dev->xfer_lock);
if (!i2c_dev->hw->has_single_clk_source) {
fast_clk = devm_clk_get(&pdev->dev, "fast-clk");
--
1.8.1.5
^ permalink raw reply related
* [PATCH 4/5] i2c: tegra: use atomic poll function during configuration
From: Shardar Shariff Md @ 2016-08-31 13:22 UTC (permalink / raw)
To: smohammed, wsa, swarren, thierry.reding, gnurou, linux-i2c,
linux-tegra, linux-kernel, jonathanh
In-Reply-To: <1472649758-5608-1-git-send-email-smohammed@nvidia.com>
Use readl_poll_timeout_atomic() function as *wait_for_config_load()
function can be called from interrupt context.
Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
---
Changes in v10:
- Rebase on top of [PATCH V2 0/9] Some Tegra I2C Updates
Changes in v11:
- Fix checkpatch error of using in_atomic() API
---
---
drivers/i2c/busses/i2c-tegra.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index daab0ce..2437535 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -457,8 +457,13 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
addr = i2c_dev->base + reg_offset;
i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
- err = readl_poll_timeout(addr, val, val == 0, 1000,
- I2C_CONFIG_LOAD_TIMEOUT);
+ if (in_interrupt())
+ err = readl_poll_timeout_atomic(addr, val, val == 0,
+ 1000, I2C_CONFIG_LOAD_TIMEOUT);
+ else
+ err = readl_poll_timeout(addr, val, val == 0,
+ 1000, I2C_CONFIG_LOAD_TIMEOUT);
+
if (err) {
dev_warn(i2c_dev->dev,
"timeout waiting for config load\n");
--
1.8.1.5
^ permalink raw reply related
* [PATCH 3/5] i2c: tegra: add separate function for config_load programing
From: Shardar Shariff Md @ 2016-08-31 13:22 UTC (permalink / raw)
To: smohammed, wsa, swarren, thierry.reding, gnurou, linux-i2c,
linux-tegra, linux-kernel, jonathanh
In-Reply-To: <1472649758-5608-1-git-send-email-smohammed@nvidia.com>
Define separate function for configuration load register handling
to make it use by different functions later.
Signed-off-by: Shardar Shariff Md <smohammed@nvidia.com>
---
Changes in v2:
- Remove unnecessary paranthesis and align to 80 characters per line
Changes in v3:
- Add separate function for config load handling
Changes in v4:
- Move timeout calculation to separate patch
Changes in v9:
- Rebase with the changes to earlier patch1
Changes in v10:
- Rebase on top of [PATCH V2 0/9] Some Tegra I2C Updates
---
---
drivers/i2c/busses/i2c-tegra.c | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 07a39b7..daab0ce 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -446,6 +446,29 @@ static int tegra_i2c_runtime_suspend(struct device *dev)
return pinctrl_pm_select_idle_state(i2c_dev->dev);
}
+static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
+{
+ unsigned long reg_offset;
+ void __iomem *addr;
+ u32 val;
+ int err;
+
+ if (i2c_dev->hw->has_config_load_reg) {
+ reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
+ addr = i2c_dev->base + reg_offset;
+ i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
+ err = readl_poll_timeout(addr, val, val == 0, 1000,
+ I2C_CONFIG_LOAD_TIMEOUT);
+ if (err) {
+ dev_warn(i2c_dev->dev,
+ "timeout waiting for config load\n");
+ return err;
+ }
+ }
+
+ return 0;
+}
+
static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
{
u32 val;
@@ -500,21 +523,9 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
if (i2c_dev->is_multimaster_mode && i2c_dev->hw->has_slcg_override_reg)
i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
- if (i2c_dev->hw->has_config_load_reg) {
- unsigned long reg_offset;
- void __iomem *addr;
-
- reg_offset = tegra_i2c_reg_addr(i2c_dev, I2C_CONFIG_LOAD);
- addr = i2c_dev->base + reg_offset;
- i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
- err = readl_poll_timeout(addr, val, val == 0, 1000,
- I2C_CONFIG_LOAD_TIMEOUT);
- if (err) {
- dev_warn(i2c_dev->dev,
- "timeout waiting for config load\n");
- goto err;
- }
- }
+ err = tegra_i2c_wait_for_config_load(i2c_dev);
+ if (err)
+ goto err;
if (i2c_dev->irq_disabled) {
i2c_dev->irq_disabled = 0;
--
1.8.1.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox