| Message ID | 1328314217-16632-1-git-send-email-swarren@nvidia.com |
|---|---|
| State | Rejected, archived |
| Headers | show |
On Saturday 04 February 2012 05:40 AM, Stephen Warren wrote: > From: Laxman Dewangan<ldewangan@nvidia.com> > > 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@nvidia.com>, 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@nvidia.com> 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. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@nvidia.com> > > > > 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@nvidia.com>, 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@nvidia.com> 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.
Hi Stephen, On Sat, Feb 4, 2012 at 9:51 PM, Stephen Warren <swarren@nvidia.com> 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@nvidia.com> >> > >> > 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@nvidia.com>, 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@nvidia.com> > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Simon Glass wrote at Saturday, February 04, 2012 11:03 PM: > On Sat, Feb 4, 2012 at 9:51 PM, Stephen Warren <swarren@nvidia.com> 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@nvidia.com> > >> > > >> > 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@nvidia.com>, 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@nvidia.com> > > > > 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.
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.. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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));