[PATCH 1/2] i2c: tegra: Remove unnecessary clk_get

[PATCH 1/2] i2c: tegra: Remove unnecessary clk_get

[Stephen Warren]

From: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The clock table has just one entry for a given i2c controller.
Hence, the second clk_get is not required in the driver.

Originally by Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, but S-o-b is
missing in our internal repo.

[swarren: Reworded commit description, resolved merge issue when cherry-
picking to mainline]
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-tegra.c |   17 ++---------------
 1 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 0ab4a95..a546ede 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -123,7 +123,6 @@ struct tegra_i2c_dev {
 	struct device *dev;
 	struct i2c_adapter adapter;
 	struct clk *clk;
-	struct clk *i2c_clk;
 	struct resource *iomem;
 	void __iomem *base;
 	int cont_id;
@@ -565,7 +564,6 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
 	struct resource *res;
 	struct resource *iomem;
 	struct clk *clk;
-	struct clk *i2c_clk;
 	const unsigned int *prop;
 	void __iomem *base;
 	int irq;
@@ -603,22 +601,14 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
 		goto err_release_region;
 	}
 
-	i2c_clk = clk_get(&pdev->dev, "i2c");
-	if (IS_ERR(i2c_clk)) {
-		dev_err(&pdev->dev, "missing bus clock");
-		ret = PTR_ERR(i2c_clk);
-		goto err_clk_put;
-	}
-
 	i2c_dev = kzalloc(sizeof(struct tegra_i2c_dev), GFP_KERNEL);
 	if (!i2c_dev) {
 		ret = -ENOMEM;
-		goto err_i2c_clk_put;
+		goto err_clk_put;
 	}
 
 	i2c_dev->base = base;
 	i2c_dev->clk = clk;
-	i2c_dev->i2c_clk = i2c_clk;
 	i2c_dev->iomem = iomem;
 	i2c_dev->adapter.algo = &tegra_i2c_algo;
 	i2c_dev->irq = irq;
@@ -657,7 +647,7 @@ static int __devinit tegra_i2c_probe(struct platform_device *pdev)
 		goto err_free;
 	}
 
-	clk_enable(i2c_dev->i2c_clk);
+	clk_enable(i2c_dev->clk);
 
 	i2c_set_adapdata(&i2c_dev->adapter, i2c_dev);
 	i2c_dev->adapter.owner = THIS_MODULE;
@@ -682,8 +672,6 @@ err_free_irq:
 	free_irq(i2c_dev->irq, i2c_dev);
 err_free:
 	kfree(i2c_dev);
-err_i2c_clk_put:
-	clk_put(i2c_clk);
 err_clk_put:
 	clk_put(clk);
 err_release_region:
@@ -698,7 +686,6 @@ static int __devexit tegra_i2c_remove(struct platform_device *pdev)
 	struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
 	i2c_del_adapter(&i2c_dev->adapter);
 	free_irq(i2c_dev->irq, i2c_dev);
-	clk_put(i2c_dev->i2c_clk);
 	clk_put(i2c_dev->clk);
 	release_mem_region(i2c_dev->iomem->start,
 		resource_size(i2c_dev->iomem));
-- 
1.7.0.4

[PATCH 2/2] i2c: tegra: Remove unnecessary write to INT_STATUS

[Stephen Warren]

From: Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The write is not necessary and may cause the I2C controller to misbehave.
With this fix, I2C on Tegra30 works (at least, running i2cdump repeatedly
on the WM8903 on Cardhu's I2C5/DVC bus).

Originally by Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, but S-o-b missing in our
internal repo.

[swarren: Reworded commit description]
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-tegra.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index a546ede..edac27b 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -456,7 +456,6 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	int ret;
 
 	tegra_i2c_flush_fifos(i2c_dev);
-	i2c_writel(i2c_dev, 0xFF, I2C_INT_STATUS);
 
 	if (msg->len == 0)
 		return -EINVAL;
-- 
1.7.0.4

Re: [PATCH 1/2] i2c: tegra: Remove unnecessary clk_get

[Laxman Dewangan]

On Saturday 04 February 2012 05:40 AM, Stephen Warren wrote:
> From: Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> The clock table has just one entry for a given i2c controller.
> Hence, the second clk_get is not required in the driver.
>
> Originally by Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, but S-o-b is
> missing in our internal repo.
>
> [swarren: Reworded commit description, resolved merge issue when cherry-
> picking to mainline]
> Signed-off-by: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
The tegra i2c controller have two clock inputs i2c_div and i2c_fast_clk. 
There is way to select the clock source for i2c_div clock but 
i2c_fast_clk is fixed to PLLP_OUT3 clock source.
Both clocks are needed to proper functionality. This change assume that 
fast-clk (pllp_out3) is always be ON which is wrong assumption. For 
aggressive power management, if there is no client for pllp_out3, it 
will be turned off. And so this is require.
However, the code enable fast_clk always once it is registered which is 
also not correct. The div_clk and fast_clk should be enable together and 
diable together and so driver need to call the clk_enable(div_clk) and 
clk_enable(fast_clk) for enabling clock. We have fixed this in our 
internal tree (K3.1). Let me know if I can help here.

RE: [PATCH 1/2] i2c: tegra: Remove unnecessary clk_get

[Stephen Warren]

Laxman Dewangan wrote at Saturday, February 04, 2012 4:18 AM
> On Saturday 04 February 2012 05:40 AM, Stephen Warren wrote:
> > From: Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > The clock table has just one entry for a given i2c controller.
> > Hence, the second clk_get is not required in the driver.
> >
> > Originally by Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, but S-o-b is
> > missing in our internal repo.
> >
> > [swarren: Reworded commit description, resolved merge issue when cherry-
> > picking to mainline]
> > Signed-off-by: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Ben, Wolfram. The two changes in this patch series are completely
independent. The 2nd patch is what I really care about, and can be
applied irrespective of the resolution of the discussion below.

> The tegra i2c controller have two clock inputs i2c_div and i2c_fast_clk.

Is this true on Tegra20 or just Tegra30? The Tegra20 TRM explicitly lists
the clock domains the I2C controller users, and doesn't mention anything
about this...

> There is way to select the clock source for i2c_div clock but
> i2c_fast_clk is fixed to PLLP_OUT3 clock source.
> Both clocks are needed to proper functionality. This change assume that
> fast-clk (pllp_out3) is always be ON which is wrong assumption. For

That's not something this change assumes; the assumption was already
there. The two clk_get() calls in the I2C driver today end up getting
the same clock I believe, since there is only a single clock listed in
tegra2_clock.c for the I2C controller.

> aggressive power management, if there is no client for pllp_out3, it
> will be turned off. And so this is require.
>
> However, the code enable fast_clk always once it is registered which is
> also not correct. The div_clk and fast_clk should be enable together and
> diable together and so driver need to call the clk_enable(div_clk) and
> clk_enable(fast_clk) for enabling clock. We have fixed this in our
> internal tree (K3.1). Let me know if I can help here.:

The K3.1 tree is where I got this change from, although I did notice
that there were some other changes in that tree to implement the more
aggressive clock management you described.

If I'm not making sense, We should probably continue this discussion
off-list (just between ourselves) so as to avoid spamming the mailing
list; I can summarize any results for the list archive later.

-- 
nvpublic

Re: [PATCH 1/2] i2c: tegra: Remove unnecessary clk_get

[Simon Glass]

Hi Stephen,

On Sat, Feb 4, 2012 at 9:51 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> Laxman Dewangan wrote at Saturday, February 04, 2012 4:18 AM
>> On Saturday 04 February 2012 05:40 AM, Stephen Warren wrote:
>> > From: Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> >
>> > The clock table has just one entry for a given i2c controller.
>> > Hence, the second clk_get is not required in the driver.
>> >
>> > Originally by Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, but S-o-b is
>> > missing in our internal repo.
>> >
>> > [swarren: Reworded commit description, resolved merge issue when cherry-
>> > picking to mainline]
>> > Signed-off-by: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Ben, Wolfram. The two changes in this patch series are completely
> independent. The 2nd patch is what I really care about, and can be
> applied irrespective of the resolution of the discussion below.
>
>> The tegra i2c controller have two clock inputs i2c_div and i2c_fast_clk.
>
> Is this true on Tegra20 or just Tegra30? The Tegra20 TRM explicitly lists
> the clock domains the I2C controller users, and doesn't mention anything
> about this...

As I happened to be looking at this on Friday, it seems that Tegra20
supports the fast I2C also. I couldn't quite see how to select the
fast clock, but since I wasn't planning to support it I didn't mind.
From what I could tell, the slow clock can switch between four options
and the fast clock is fixed at 72MHz (based off PLLP), as below.

>
>> There is way to select the clock source for i2c_div clock but
>> i2c_fast_clk is fixed to PLLP_OUT3 clock source.
>> Both clocks are needed to proper functionality. This change assume that
>> fast-clk (pllp_out3) is always be ON which is wrong assumption. For
>
> That's not something this change assumes; the assumption was already
> there. The two clk_get() calls in the I2C driver today end up getting
> the same clock I believe, since there is only a single clock listed in
> tegra2_clock.c for the I2C controller.
>
>> aggressive power management, if there is no client for pllp_out3, it
>> will be turned off. And so this is require.
>>
>> However, the code enable fast_clk always once it is registered which is
>> also not correct. The div_clk and fast_clk should be enable together and
>> diable together and so driver need to call the clk_enable(div_clk) and
>> clk_enable(fast_clk) for enabling clock. We have fixed this in our
>> internal tree (K3.1). Let me know if I can help here.:
>
> The K3.1 tree is where I got this change from, although I did notice
> that there were some other changes in that tree to implement the more
> aggressive clock management you described.
>
> If I'm not making sense, We should probably continue this discussion
> off-list (just between ourselves) so as to avoid spamming the mailing
> list; I can summarize any results for the list archive later.


Regards,
Simon

>
> --
> nvpublic
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] i2c: tegra: Remove unnecessary write to INT_STATUS

[Ben Dooks]

On Fri, Feb 03, 2012 at 05:10:17PM -0700, Stephen Warren wrote:
> From: Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The write is not necessary and may cause the I2C controller to misbehave.
> With this fix, I2C on Tegra30 works (at least, running i2cdump repeatedly
> on the WM8903 on Cardhu's I2C5/DVC bus).
> 
> Originally by Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, but S-o-b missing in our
> internal repo.
> 
> [swarren: Reworded commit description]
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

I'll apply this as it seems to have gone by without major comment.

> ---
>  drivers/i2c/busses/i2c-tegra.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index a546ede..edac27b 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -456,7 +456,6 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	int ret;
>  
>  	tegra_i2c_flush_fifos(i2c_dev);
> -	i2c_writel(i2c_dev, 0xFF, I2C_INT_STATUS);
>  
>  	if (msg->len == 0)
>  		return -EINVAL;
> -- 
> 1.7.0.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/2] i2c: tegra: Remove unnecessary clk_get

[Stephen Warren]

Simon Glass wrote at Saturday, February 04, 2012 11:03 PM:
> On Sat, Feb 4, 2012 at 9:51 PM, Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> > Laxman Dewangan wrote at Saturday, February 04, 2012 4:18 AM
> >> On Saturday 04 February 2012 05:40 AM, Stephen Warren wrote:
> >> > From: Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >> >
> >> > The clock table has just one entry for a given i2c controller.
> >> > Hence, the second clk_get is not required in the driver.
> >> >
> >> > Originally by Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, but S-o-b is
> >> > missing in our internal repo.
> >> >
> >> > [swarren: Reworded commit description, resolved merge issue when cherry-
> >> > picking to mainline]
> >> > Signed-off-by: Stephen Warren<swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > Ben, Wolfram. The two changes in this patch series are completely
> > independent. The 2nd patch is what I really care about, and can be
> > applied irrespective of the resolution of the discussion below.
> >
> >> The tegra i2c controller have two clock inputs i2c_div and i2c_fast_clk.
> >
> > Is this true on Tegra20 or just Tegra30? The Tegra20 TRM explicitly lists
> > the clock domains the I2C controller users, and doesn't mention anything
> > about this...
> 
> As I happened to be looking at this on Friday, it seems that Tegra20
> supports the fast I2C also. I couldn't quite see how to select the
> fast clock, but since I wasn't planning to support it I didn't mind.
> From what I could tell, the slow clock can switch between four options
> and the fast clock is fixed at 72MHz (based off PLLP), as below.

OK, I've found the location in the TRM that describes this additional
clock input to a few modules. It's in the CAR (Clock And Reset) controller
section in a couple places, e.g. Table 12 "PLL Clock Usage" and in a
section "Special PLL clock usage by certain peripheral" (in version v01p
at least).

So yes, let's not apply this one patch.

Simon, the upshot of this is that when you add the clocks property to
the I2C nodes, you'll need to add both the regular variable I2C clock
and this other clock to the list:

clocks = <&tegra_car 12> /* i2c1 */ <&tegra_car 124> /* pll_p_out3 */;

I suggest listing the regular clock first for simplicity in U-Boot's
parsing of the property (so the first clock is always the one with a
bit in the CLK_ENB registers) and these additional clocks after, as I
have above.

This appears to apply to modules lfsr, dsi, csi, i2c1/2/3, uart1/2/3/4/5,
but apparently not the DVC I2C controller according to the TRM. Laxman,
can you confirm that the DVC I2C controller doesn't take a clock from
pll_p_out3?

P.S. this patch I posted is in our downstream 2.6.39 and 3.1 kernels.
Given this thread, I assume it should be reverted there...

Thanks.

-- 
nvpublic

Re: [PATCH 1/2] i2c: tegra: Remove unnecessary clk_get

[Laxman Dewangan]

On Thursday 16 February 2012 12:13 AM, Stephen Warren wrote:
>
> This appears to apply to modules lfsr, dsi, csi, i2c1/2/3, uart1/2/3/4/5,
> but apparently not the DVC I2C controller according to the TRM. Laxman,
> can you confirm that the DVC I2C controller doesn't take a clock from
> pll_p_out3?
>

DVC i2C is also require this 2nd clock.

> P.S. this patch I posted is in our downstream 2.6.39 and 3.1 kernels.
> Given this thread, I assume it should be reverted there...
>

Yes, I have pushed the change in our downstream tree K3.1 and it is 
under testing/promotion..

RE: [PATCH 2/2] i2c: tegra: Remove unnecessary write to INT_STATUS

[Stephen Warren]

Ben Dooks wrote at Monday, February 13, 2012 4:24 PM:
> On Fri, Feb 03, 2012 at 05:10:17PM -0700, Stephen Warren wrote:
> > From: Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > The write is not necessary and may cause the I2C controller to misbehave.
> > With this fix, I2C on Tegra30 works (at least, running i2cdump repeatedly
> > on the WM8903 on Cardhu's I2C5/DVC bus).
> >
> > Originally by Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, but S-o-b missing in our
> > internal repo.
> >
> > [swarren: Reworded commit description]
> > Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> I'll apply this as it seems to have gone by without major comment.

Ben, I haven't seen this show up in linux-next yet. I'm hoping the patch
will make 3.4. Thanks

-- 
nvpublic

RE: [PATCH 2/2] i2c: tegra: Remove unnecessary write to INT_STATUS

[Alok Chauhan]

Signed-off-by: Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>


-----Original Message-----
From: Stephen Warren 
Sent: Saturday, February 04, 2012 5:40 AM
To: Ben Dooks; Wolfram Sang
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Alok Chauhan; Stephen Warren
Subject: [PATCH 2/2] i2c: tegra: Remove unnecessary write to INT_STATUS

From: Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The write is not necessary and may cause the I2C controller to misbehave.
With this fix, I2C on Tegra30 works (at least, running i2cdump repeatedly on the WM8903 on Cardhu's I2C5/DVC bus).

Originally by Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>, but S-o-b missing in our internal repo.

[swarren: Reworded commit description]
Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-tegra.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index a546ede..edac27b 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -456,7 +456,6 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	int ret;
 
 	tegra_i2c_flush_fifos(i2c_dev);
-	i2c_writel(i2c_dev, 0xFF, I2C_INT_STATUS);
 
 	if (msg->len == 0)
 		return -EINVAL;
--
1.7.0.4

Re: [PATCH 2/2] i2c: tegra: Remove unnecessary write to INT_STATUS

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 333 bytes --]

On Tue, Mar 06, 2012 at 09:37:07AM +0530, Alok Chauhan wrote:
> Signed-off-by: Alok Chauhan <alokc-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Thanks, I updated the patch.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]