[PATCH 1/3] clk: tegra114: Initialize clocks needed for HDMI

[PATCH 1/3] clk: tegra114: Initialize clocks needed for HDMI

[Thierry Reding]

From: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Add disp1 and disp2 clocks to the clock initialization table. These
clocks are required for display and HDMI support.

Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/clk/tegra/clk-tegra114.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/clk/tegra/clk-tegra114.c b/drivers/clk/tegra/clk-tegra114.c
index e365b35..624077d 100644
--- a/drivers/clk/tegra/clk-tegra114.c
+++ b/drivers/clk/tegra/clk-tegra114.c
@@ -1300,6 +1300,8 @@ static struct tegra_clk_init_table init_table[] __initdata = {
 	{TEGRA114_CLK_HOST1X, TEGRA114_CLK_PLL_P, 136000000, 0},
 	{TEGRA114_CLK_DFLL_SOC, TEGRA114_CLK_PLL_P, 51000000, 1},
 	{TEGRA114_CLK_DFLL_REF, TEGRA114_CLK_PLL_P, 51000000, 1},
+	{TEGRA114_CLK_DISP1, TEGRA114_CLK_PLL_P, 600000000, 0},
+	{TEGRA114_CLK_DISP2, TEGRA114_CLK_PLL_P, 600000000, 0},
 	{TEGRA114_CLK_GR2D, TEGRA114_CLK_PLL_C2, 300000000, 0},
 	{TEGRA114_CLK_GR3D, TEGRA114_CLK_PLL_C2, 300000000, 0},
 
-- 
1.8.4

[PATCH 2/3] clk: tegra: Initialize secondary gr3d clock on Tegra30

[Thierry Reding]

There are two GPUs on Tegra30 and each of them uses a separate clock, so
the secondary clock needs to be initialized in order for the gr3d module
to work properly.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/clk/tegra/clk-tegra30.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index 880290f..24ce357 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -1377,6 +1377,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
 	{TEGRA30_CLK_TWD, TEGRA30_CLK_CLK_MAX, 0, 1},
 	{TEGRA30_CLK_GR2D, TEGRA30_CLK_PLL_C, 300000000, 0},
 	{TEGRA30_CLK_GR3D, TEGRA30_CLK_PLL_C, 300000000, 0},
+	{TEGRA30_CLK_GR3D2, TEGRA30_CLK_PLL_C, 300000000, 0},
 	{TEGRA30_CLK_CLK_MAX, TEGRA30_CLK_CLK_MAX, 0, 0}, /* This MUST be the last entry. */
 };
 
-- 
1.8.4

[PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Thierry Reding]

The clock for the PWM controller is slightly different from other
peripheral clocks on Tegra30. The clock source mux field start at
bit position 28 rather than 30.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/clk/tegra/clk-tegra30.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
index 24ce357..51c093c 100644
--- a/drivers/clk/tegra/clk-tegra30.c
+++ b/drivers/clk/tegra/clk-tegra30.c
@@ -104,6 +104,7 @@
 #define PMC_CLK_OUT_CNTRL 0x1a8
 
 #define CLK_SOURCE_SPDIF_OUT 0x108
+#define CLK_SOURCE_PWM 0x110
 #define CLK_SOURCE_D_AUDIO 0x3d0
 #define CLK_SOURCE_DAM0 0x3d8
 #define CLK_SOURCE_DAM1 0x3dc
@@ -836,7 +837,6 @@ static struct tegra_clk tegra30_clks[tegra_clk_max] __initdata = {
 	[tegra_clk_extern1] = { .dt_id = TEGRA30_CLK_EXTERN1, .present = true },
 	[tegra_clk_extern2] = { .dt_id = TEGRA30_CLK_EXTERN2, .present = true },
 	[tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3, .present = true },
-	[tegra_clk_pwm] = { .dt_id = TEGRA30_CLK_PWM, .present = true },
 	[tegra_clk_disp1] = { .dt_id = TEGRA30_CLK_DISP1, .present = true },
 	[tegra_clk_disp2] = { .dt_id = TEGRA30_CLK_DISP2, .present = true },
 	[tegra_clk_apbdma] = { .dt_id = TEGRA30_CLK_APBDMA, .present = true },
@@ -1120,6 +1120,7 @@ static const char *mux_pllpmdacd2_clkm[] = { "pll_p", "pll_m", "pll_d_out0",
 					     "pll_d2_out0", "clk_m" };
 static const char *mux_plld_out0_plld2_out0[] = { "pll_d_out0",
 						  "pll_d2_out0" };
+static const char *pwm_parents[] = { "pll_p", "pll_c", "clk_32k", "clk_m" };
 
 static struct tegra_periph_init_data tegra_periph_clk_list[] = {
 	TEGRA_INIT_DATA_MUX("spdif_out", spdif_out_parents, CLK_SOURCE_SPDIF_OUT, 10, TEGRA_PERIPH_ON_APB, TEGRA30_CLK_SPDIF_OUT),
@@ -1130,6 +1131,7 @@ static struct tegra_periph_init_data tegra_periph_clk_list[] = {
 	TEGRA_INIT_DATA_INT("3d2", mux_pllmcpa, CLK_SOURCE_3D2, 98, TEGRA_PERIPH_MANUAL_RESET, TEGRA30_CLK_GR3D2),
 	TEGRA_INIT_DATA_INT("se", mux_pllpcm_clkm, CLK_SOURCE_SE, 127, 0, TEGRA30_CLK_SE),
 	TEGRA_INIT_DATA_MUX8("hdmi", mux_pllpmdacd2_clkm, CLK_SOURCE_HDMI, 51, 0, TEGRA30_CLK_HDMI),
+	TEGRA_INIT_DATA("pwm", NULL, NULL, pwm_parents, CLK_SOURCE_PWM, 28, 2, 0, 0, 8, 1, 0, 17, TEGRA_PERIPH_ON_APB, TEGRA30_CLK_PWM),
 };
 
 static struct tegra_periph_init_data tegra_periph_nodiv_clk_list[] = {
-- 
1.8.4

Re: [PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Stephen Warren]

On 10/29/2013 09:51 AM, Thierry Reding wrote:
> The clock for the PWM controller is slightly different from other
> peripheral clocks on Tegra30. The clock source mux field start at
> bit position 28 rather than 30.

I think you need to CC this series to Mike Turquette and relevant lists
for review. While Peter is sending pull requests to Mike for the Tegra
clock driver, Mike still needs to see the patches before that happens.

> diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c

> @@ -836,7 +837,6 @@ static struct tegra_clk tegra30_clks[tegra_clk_max] __initdata = {
>  	[tegra_clk_extern1] = { .dt_id = TEGRA30_CLK_EXTERN1, .present = true },
>  	[tegra_clk_extern2] = { .dt_id = TEGRA30_CLK_EXTERN2, .present = true },
>  	[tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3, .present = true },
> -	[tegra_clk_pwm] = { .dt_id = TEGRA30_CLK_PWM, .present = true },

I think you still need an entry in this table; isn't it used by the
DT->internal clock ID translation function?

Either way, it seems like this patch might want to add a
tegra_clk_pwm_tegra30 so that the common C files can still implement
this clock, just with different parameters?

Re: [PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Thierry Reding]

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

On Tue, Oct 29, 2013 at 01:45:38PM -0600, Stephen Warren wrote:
> On 10/29/2013 09:51 AM, Thierry Reding wrote:
> > The clock for the PWM controller is slightly different from other
> > peripheral clocks on Tegra30. The clock source mux field start at
> > bit position 28 rather than 30.
> 
> I think you need to CC this series to Mike Turquette and relevant lists
> for review. While Peter is sending pull requests to Mike for the Tegra
> clock driver, Mike still needs to see the patches before that happens.

I meant for this to be reviewed primarily by Peter and/or Prashant
before sending it to Mike "officially" because I wasn't at all sure if
it was correct, even though it fixed the issue I was seeing on Cardhu.

But Cc'ing Mike probably wouldn't have hurt either, so I'll just add him
from the start next time.

> > diff --git a/drivers/clk/tegra/clk-tegra30.c b/drivers/clk/tegra/clk-tegra30.c
> 
> > @@ -836,7 +837,6 @@ static struct tegra_clk tegra30_clks[tegra_clk_max] __initdata = {
> >  	[tegra_clk_extern1] = { .dt_id = TEGRA30_CLK_EXTERN1, .present = true },
> >  	[tegra_clk_extern2] = { .dt_id = TEGRA30_CLK_EXTERN2, .present = true },
> >  	[tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3, .present = true },
> > -	[tegra_clk_pwm] = { .dt_id = TEGRA30_CLK_PWM, .present = true },
> 
> I think you still need an entry in this table; isn't it used by the
> DT->internal clock ID translation function?

As far as I can tell, this is used by the generic code to determine
which of the generic clocks to register. If TEGRA30_CLK_PWM is kept
within this list, tegra_periph_clk_init() will register the clock a
second time.

> Either way, it seems like this patch might want to add a
> tegra_clk_pwm_tegra30 so that the common C files can still implement
> this clock, just with different parameters?

That's pretty much what this patch does, isn't it? It adds a custom
entry for the PWM clock to the Tegra30-specific tegra_periph_clk_list
and keeps the common one from being registered by dropping the entry
from tegra30_clks.

The same is already done on Tegra20, where the PWM clock is similarly
weird. On Tegra114 and Tegra124 the clock is still weird, but it can be
tweaked into behaving more commonly by lying about the actual position
and size of the mux field.

Thierry

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

Re: [PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Stephen Warren]

On 10/30/2013 03:00 PM, Thierry Reding wrote:
> On Tue, Oct 29, 2013 at 01:45:38PM -0600, Stephen Warren wrote:
>> On 10/29/2013 09:51 AM, Thierry Reding wrote:
>>> The clock for the PWM controller is slightly different from
>>> other peripheral clocks on Tegra30. The clock source mux field
>>> start at bit position 28 rather than 30.

>>> diff --git a/drivers/clk/tegra/clk-tegra30.c
>>> b/drivers/clk/tegra/clk-tegra30.c
>> 
>>> @@ -836,7 +837,6 @@ static struct tegra_clk
>>> tegra30_clks[tegra_clk_max] __initdata = { [tegra_clk_extern1]
>>> = { .dt_id = TEGRA30_CLK_EXTERN1, .present = true }, 
>>> [tegra_clk_extern2] = { .dt_id = TEGRA30_CLK_EXTERN2, .present
>>> = true }, [tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3,
>>> .present = true }, -	[tegra_clk_pwm] = { .dt_id =
>>> TEGRA30_CLK_PWM, .present = true },
>> 
>> I think you still need an entry in this table; isn't it used by
>> the DT->internal clock ID translation function?
> 
> As far as I can tell, this is used by the generic code to
> determine which of the generic clocks to register. If
> TEGRA30_CLK_PWM is kept within this list, tegra_periph_clk_init()
> will register the clock a second time.
> 
>> Either way, it seems like this patch might want to add a 
>> tegra_clk_pwm_tegra30 so that the common C files can still
>> implement this clock, just with different parameters?
> 
> That's pretty much what this patch does, isn't it? It adds a
> custom entry for the PWM clock to the Tegra30-specific
> tegra_periph_clk_list and keeps the common one from being
> registered by dropping the entry from tegra30_clks.

I was hoping for the new clock that gets added to be added to the
common file that defines the common clocks rather than to be added to
the Tegra30-specific files. However, I suppose if it really is a
Tegra30-specific clock, then keeping it isolated to the
Tegra30-specific file might make sense.

That said, I wonder if we can't keep the PWM clock definition in the
common file, and just make it a bit parameterized? Perhaps that
defeats the purpose of a common definition though.

> The same is already done on Tegra20, where the PWM clock is
> similarly weird.

OK, perhaps that's reasonable precedent for this then.

> On Tegra114 and Tegra124 the clock is still weird, but it can be 
> tweaked into behaving more commonly by lying about the actual
> position and size of the mux field.

That doesn't sound good... What if someone actually wants to use the
correct position/size (e.g. use mux options that presumably can't be
selected now we've lied)? We really shouldn't lie about things.

Re: [PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Thierry Reding]

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

On Wed, Oct 30, 2013 at 03:38:23PM -0600, Stephen Warren wrote:
> On 10/30/2013 03:00 PM, Thierry Reding wrote:
> > On Tue, Oct 29, 2013 at 01:45:38PM -0600, Stephen Warren wrote:
> >> On 10/29/2013 09:51 AM, Thierry Reding wrote:
> >>> The clock for the PWM controller is slightly different from
> >>> other peripheral clocks on Tegra30. The clock source mux field
> >>> start at bit position 28 rather than 30.
> 
> >>> diff --git a/drivers/clk/tegra/clk-tegra30.c
> >>> b/drivers/clk/tegra/clk-tegra30.c
> >> 
> >>> @@ -836,7 +837,6 @@ static struct tegra_clk
> >>> tegra30_clks[tegra_clk_max] __initdata = { [tegra_clk_extern1]
> >>> = { .dt_id = TEGRA30_CLK_EXTERN1, .present = true }, 
> >>> [tegra_clk_extern2] = { .dt_id = TEGRA30_CLK_EXTERN2, .present
> >>> = true }, [tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3,
> >>> .present = true }, -	[tegra_clk_pwm] = { .dt_id =
> >>> TEGRA30_CLK_PWM, .present = true },
> >> 
> >> I think you still need an entry in this table; isn't it used by
> >> the DT->internal clock ID translation function?
> > 
> > As far as I can tell, this is used by the generic code to
> > determine which of the generic clocks to register. If
> > TEGRA30_CLK_PWM is kept within this list, tegra_periph_clk_init()
> > will register the clock a second time.
> > 
> >> Either way, it seems like this patch might want to add a 
> >> tegra_clk_pwm_tegra30 so that the common C files can still
> >> implement this clock, just with different parameters?
> > 
> > That's pretty much what this patch does, isn't it? It adds a
> > custom entry for the PWM clock to the Tegra30-specific
> > tegra_periph_clk_list and keeps the common one from being
> > registered by dropping the entry from tegra30_clks.
> 
> I was hoping for the new clock that gets added to be added to the
> common file that defines the common clocks rather than to be added to
> the Tegra30-specific files. However, I suppose if it really is a
> Tegra30-specific clock, then keeping it isolated to the
> Tegra30-specific file might make sense.
> 
> That said, I wonder if we can't keep the PWM clock definition in the
> common file, and just make it a bit parameterized? Perhaps that
> defeats the purpose of a common definition though.
> 
> > The same is already done on Tegra20, where the PWM clock is
> > similarly weird.
> 
> OK, perhaps that's reasonable precedent for this then.
> 
> > On Tegra114 and Tegra124 the clock is still weird, but it can be 
> > tweaked into behaving more commonly by lying about the actual
> > position and size of the mux field.
> 
> That doesn't sound good... What if someone actually wants to use the
> correct position/size (e.g. use mux options that presumably can't be
> selected now we've lied)? We really shouldn't lie about things.

Perhaps I've misinterpreted the code. At least comparing to the register
definitions the field should still be 3 bits wide, but the code clamps
it to the upper 2, which makes it compatible with the regular peripheral
clocks. The mux options that are left out are apparently very uncommonly
used (PLLC2 and PLLC3).

I suppose it's always possible that the register definition is wrong or
there was perhaps another good reason to do it this way? Maybe Peter
knows.

Thierry

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

Re: [PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Peter De Schrijver]

On Wed, Oct 30, 2013 at 11:54:44PM +0100, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Oct 30, 2013 at 03:38:23PM -0600, Stephen Warren wrote:
> > On 10/30/2013 03:00 PM, Thierry Reding wrote:
> > > On Tue, Oct 29, 2013 at 01:45:38PM -0600, Stephen Warren wrote:
> > >> On 10/29/2013 09:51 AM, Thierry Reding wrote:
> > >>> The clock for the PWM controller is slightly different from
> > >>> other peripheral clocks on Tegra30. The clock source mux field
> > >>> start at bit position 28 rather than 30.
> > 
> > >>> diff --git a/drivers/clk/tegra/clk-tegra30.c
> > >>> b/drivers/clk/tegra/clk-tegra30.c
> > >> 
> > >>> @@ -836,7 +837,6 @@ static struct tegra_clk
> > >>> tegra30_clks[tegra_clk_max] __initdata = { [tegra_clk_extern1]
> > >>> = { .dt_id = TEGRA30_CLK_EXTERN1, .present = true }, 
> > >>> [tegra_clk_extern2] = { .dt_id = TEGRA30_CLK_EXTERN2, .present
> > >>> = true }, [tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3,
> > >>> .present = true }, -	[tegra_clk_pwm] = { .dt_id =
> > >>> TEGRA30_CLK_PWM, .present = true },
> > >> 
> > >> I think you still need an entry in this table; isn't it used by
> > >> the DT->internal clock ID translation function?
> > > 
> > > As far as I can tell, this is used by the generic code to
> > > determine which of the generic clocks to register. If
> > > TEGRA30_CLK_PWM is kept within this list, tegra_periph_clk_init()
> > > will register the clock a second time.
> > > 
> > >> Either way, it seems like this patch might want to add a 
> > >> tegra_clk_pwm_tegra30 so that the common C files can still
> > >> implement this clock, just with different parameters?
> > > 
> > > That's pretty much what this patch does, isn't it? It adds a
> > > custom entry for the PWM clock to the Tegra30-specific
> > > tegra_periph_clk_list and keeps the common one from being
> > > registered by dropping the entry from tegra30_clks.
> > 
> > I was hoping for the new clock that gets added to be added to the
> > common file that defines the common clocks rather than to be added to
> > the Tegra30-specific files. However, I suppose if it really is a
> > Tegra30-specific clock, then keeping it isolated to the
> > Tegra30-specific file might make sense.
> > 
> > That said, I wonder if we can't keep the PWM clock definition in the
> > common file, and just make it a bit parameterized? Perhaps that
> > defeats the purpose of a common definition though.
> > 
> > > The same is already done on Tegra20, where the PWM clock is
> > > similarly weird.
> > 
> > OK, perhaps that's reasonable precedent for this then.
> > 
> > > On Tegra114 and Tegra124 the clock is still weird, but it can be 
> > > tweaked into behaving more commonly by lying about the actual
> > > position and size of the mux field.
> > 
> > That doesn't sound good... What if someone actually wants to use the
> > correct position/size (e.g. use mux options that presumably can't be
> > selected now we've lied)? We really shouldn't lie about things.
> 
> Perhaps I've misinterpreted the code. At least comparing to the register
> definitions the field should still be 3 bits wide, but the code clamps
> it to the upper 2, which makes it compatible with the regular peripheral
> clocks. The mux options that are left out are apparently very uncommonly
> used (PLLC2 and PLLC3).
> 

And also not validated. Also by policy PLLC2 and PLLC3 are used for scaling
IP blocks. So I don't think it makes sense to use them for PWM?

Cheers,

Peter.

Re: [PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Thierry Reding]

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

On Thu, Oct 31, 2013 at 05:39:23PM +0200, Peter De Schrijver wrote:
> On Wed, Oct 30, 2013 at 11:54:44PM +0100, Thierry Reding wrote:
> > * PGP Signed by an unknown key
> > 
> > On Wed, Oct 30, 2013 at 03:38:23PM -0600, Stephen Warren wrote:
> > > On 10/30/2013 03:00 PM, Thierry Reding wrote:
> > > > On Tue, Oct 29, 2013 at 01:45:38PM -0600, Stephen Warren wrote:
> > > >> On 10/29/2013 09:51 AM, Thierry Reding wrote:
> > > >>> The clock for the PWM controller is slightly different from
> > > >>> other peripheral clocks on Tegra30. The clock source mux field
> > > >>> start at bit position 28 rather than 30.
> > > 
> > > >>> diff --git a/drivers/clk/tegra/clk-tegra30.c
> > > >>> b/drivers/clk/tegra/clk-tegra30.c
> > > >> 
> > > >>> @@ -836,7 +837,6 @@ static struct tegra_clk
> > > >>> tegra30_clks[tegra_clk_max] __initdata = { [tegra_clk_extern1]
> > > >>> = { .dt_id = TEGRA30_CLK_EXTERN1, .present = true }, 
> > > >>> [tegra_clk_extern2] = { .dt_id = TEGRA30_CLK_EXTERN2, .present
> > > >>> = true }, [tegra_clk_extern3] = { .dt_id = TEGRA30_CLK_EXTERN3,
> > > >>> .present = true }, -	[tegra_clk_pwm] = { .dt_id =
> > > >>> TEGRA30_CLK_PWM, .present = true },
> > > >> 
> > > >> I think you still need an entry in this table; isn't it used by
> > > >> the DT->internal clock ID translation function?
> > > > 
> > > > As far as I can tell, this is used by the generic code to
> > > > determine which of the generic clocks to register. If
> > > > TEGRA30_CLK_PWM is kept within this list, tegra_periph_clk_init()
> > > > will register the clock a second time.
> > > > 
> > > >> Either way, it seems like this patch might want to add a 
> > > >> tegra_clk_pwm_tegra30 so that the common C files can still
> > > >> implement this clock, just with different parameters?
> > > > 
> > > > That's pretty much what this patch does, isn't it? It adds a
> > > > custom entry for the PWM clock to the Tegra30-specific
> > > > tegra_periph_clk_list and keeps the common one from being
> > > > registered by dropping the entry from tegra30_clks.
> > > 
> > > I was hoping for the new clock that gets added to be added to the
> > > common file that defines the common clocks rather than to be added to
> > > the Tegra30-specific files. However, I suppose if it really is a
> > > Tegra30-specific clock, then keeping it isolated to the
> > > Tegra30-specific file might make sense.
> > > 
> > > That said, I wonder if we can't keep the PWM clock definition in the
> > > common file, and just make it a bit parameterized? Perhaps that
> > > defeats the purpose of a common definition though.
> > > 
> > > > The same is already done on Tegra20, where the PWM clock is
> > > > similarly weird.
> > > 
> > > OK, perhaps that's reasonable precedent for this then.
> > > 
> > > > On Tegra114 and Tegra124 the clock is still weird, but it can be 
> > > > tweaked into behaving more commonly by lying about the actual
> > > > position and size of the mux field.
> > > 
> > > That doesn't sound good... What if someone actually wants to use the
> > > correct position/size (e.g. use mux options that presumably can't be
> > > selected now we've lied)? We really shouldn't lie about things.
> > 
> > Perhaps I've misinterpreted the code. At least comparing to the register
> > definitions the field should still be 3 bits wide, but the code clamps
> > it to the upper 2, which makes it compatible with the regular peripheral
> > clocks. The mux options that are left out are apparently very uncommonly
> > used (PLLC2 and PLLC3).
> > 
> 
> And also not validated. Also by policy PLLC2 and PLLC3 are used for scaling
> IP blocks. So I don't think it makes sense to use them for PWM?

But the policy is already defined in the clock initialization tables, so
we could still setup the clock to exhibit all the possible HW choices
and simply not use those excluded "by policy".

Thierry

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

Re: [PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Peter De Schrijver]

> > 
> > And also not validated. Also by policy PLLC2 and PLLC3 are used for scaling
> > IP blocks. So I don't think it makes sense to use them for PWM?
> 
> But the policy is already defined in the clock initialization tables, so
> we could still setup the clock to exhibit all the possible HW choices
> and simply not use those excluded "by policy".
> 

That's only the 'default'. Nothing prevents a driver from doing a clk_set_parent().

Cheers,

Peter.

Re: [PATCH 3/3] clk: tegra: Properly setup PWM clock on Tegra30

[Thierry Reding]

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

On Fri, Nov 01, 2013 at 06:47:09PM +0200, Peter De Schrijver wrote:
> > > 
> > > And also not validated. Also by policy PLLC2 and PLLC3 are used for scaling
> > > IP blocks. So I don't think it makes sense to use them for PWM?
> > 
> > But the policy is already defined in the clock initialization tables, so
> > we could still setup the clock to exhibit all the possible HW choices
> > and simply not use those excluded "by policy".
> > 
> 
> That's only the 'default'. Nothing prevents a driver from doing a
> clk_set_parent().

Well, I think this is one of the rare cases where it would even make
sense for the driver to impose policy. I would still rather see the
driver expose all possible hardware options and leave up any policy to
other code.

If you really feel strongly about not exposing PLLC2 and PLLC3, can we
at least have a comment in the driver describing why they aren't exposed
so that if people stumble over this again they know why?

Thierry

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