Skip to contentSkip to conversation

i2c: tegra: Remove unnecessary clk_get

PATCH In review Stephen Warren 6 participants 11 messages Feb 4, 2012 – Mar 7, 2012 8 messages not in the archive

Subsystems (3)
Tegra architecture support I2C subsystem host drivers Tegra I2C driver
lore
Share & tools
lore

Stephen Warren

author
#0 lore raw

PATCH 1/2 i2c: tegra: Remove unnecessary clk_get

From: Laxman Dewangan <[email protected]>

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 <[email protected]>, 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 <[email protected]>
---
 drivers/i2c/busses/i2c-tegra.c |   17 ++---------------
 1 files changed, 2 insertions(+), 15 deletions(-)
drivers/i2c/busses/i2c-tegra.c +2 -15
index 0ab4a95..a546ede 100644
@@ -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));
Signature · 1 line
1.7.0.4

A message that is not in the archive (sent off-list or not yet received): 1328314217-16632-1-git-send-email-swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

author
#2 lore raw

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

From: Alok Chauhan <[email protected]>

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 <[email protected]>, but S-o-b missing in our internal repo.

[swarren: Reworded commit description]
Signed-off-by Stephen Warren <[email protected]>
---
 drivers/i2c/busses/i2c-tegra.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
drivers/i2c/busses/i2c-tegra.c +0 -1
index a546ede..edac27b 100644
@@ -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;
Signature · 1 line
1.7.0.4

A message that is not in the archive (sent off-list or not yet received): 1328314217-16632-2-git-send-email-swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

#4 lore raw

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

Quoted 11 lines · Stephen Warren

From: Alok Chauhan <[email protected]>

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 <[email protected]>, but S-o-b missing in our internal repo.

[swarren: Reworded commit description]
Signed-off-by Stephen Warren <[email protected]>

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

Quoted 23 lines
---
 drivers/i2c/busses/i2c-tegra.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
drivers/i2c/busses/i2c-tegra.c +0 -1
index a546ede..edac27b 100644
@@ -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;
Signature · 6 lines
1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#6 lore raw

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

Ben Dooks wrote at Monday, February 13, 2012 4:24 PM:

Quoted 7 lines · Stephen Warren
Quoted 6 lines

From: Alok Chauhan <[email protected]>

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).

Quoted 7 lines
Quoted 5 lines

Originally by Alok Chauhan <[email protected]>, but S-o-b missing in our internal repo.

[swarren: Reworded commit description]
Signed-off-by Stephen Warren <[email protected]>

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

Signature · 1 line
nvpublic
#7 lore raw

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

In reply to a message not in the archive
Signed-off-by Alok Chauhan <[email protected]>

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

From: Alok Chauhan <[email protected]>

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 <[email protected]>, but S-o-b missing in our internal repo.

[swarren: Reworded commit description]
Signed-off-by Stephen Warren <[email protected]>
---
 drivers/i2c/busses/i2c-tegra.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)
drivers/i2c/busses/i2c-tegra.c +0 -1
@@ -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

A message that is not in the archive (sent off-list or not yet received): 7A0BFCFE3DA5CD47B0FB7984326F201A136B88647E-kdsAE/[email protected]

#9 lore raw

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

Quoted 1 line · Alok Chauhan
Signed-off-by Alok Chauhan <[email protected]>

Thanks, I updated the patch.

Signature · 2 lines
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
#10 lore raw
In reply to a message not in the archive
Quoted 11 lines · Stephen Warren

From: Laxman Dewangan<[email protected]>

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<[email protected]>, 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<[email protected]>

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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#12 lore raw

Laxman Dewangan wrote at Saturday, February 04, 2012 4:18 AM

Quoted 6 lines · Stephen Warren
Quoted 5 lines

From: Laxman Dewangan<[email protected]>

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

Quoted 6 lines
Quoted 6 lines

Originally by Laxman Dewangan<[email protected]>, 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<[email protected]>

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.

Quoted 1 line

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...

Quoted 4 lines

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.

Quoted 8 lines

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.

Signature · 1 line
nvpublic

A message that is not in the archive (sent off-list or not yet received): 74CDBE0F657A3D45AFBB94109FB122FF178E5D3162-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org

#14 lore raw

Hi Stephen,

Quoted 18 lines · Stephen Warren

Laxman Dewangan wrote at Saturday, February 04, 2012 4:18 AM

Quoted 6 lines · Stephen Warren
Quoted 5 lines

From: Laxman Dewangan<[email protected]>

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

Quoted 6 lines
Quoted 6 lines

Originally by Laxman Dewangan<[email protected]>, 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<[email protected]>

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.

Quoted 5 lines
Quoted 1 line

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.

Quoted 20 lines
Quoted 5 lines

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.

Quoted 8 lines

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.:

Quoted 7 lines

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

Quoted 8 lines

--
nvpublic

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to [email protected] More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): CAPnjgZ2r1+tSiiKg0rHH_atrLVqn6h+7ueMdtcmaRYtkdXiTRw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

author
#16 lore raw

Simon Glass wrote at Saturday, February 04, 2012 11:03 PM:

Quoted 25 lines · Stephen Warren
Quoted 18 lines

Laxman Dewangan wrote at Saturday, February 04, 2012 4:18 AM

Quoted 6 lines · Stephen Warren
Quoted 5 lines

From: Laxman Dewangan<[email protected]>

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

Quoted 6 lines
Quoted 6 lines

Originally by Laxman Dewangan<[email protected]>, 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<[email protected]>

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.

Quoted 5 lines
Quoted 1 line

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...

Quoted 5 lines

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.

Signature · 1 line
nvpublic

A message that is not in the archive (sent off-list or not yet received): 74CDBE0F657A3D45AFBB94109FB122FF178FACB8BA-C7FfzLzN0UxDw2glCA4ptUEOCMrvLtNR@public.gmane.org

#18 lore raw
Quoted 6 lines · Stephen Warren

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.

Quoted 3 lines

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..