[v2,4/6] clk: tegra20: init NDFLASH clock to sensible rate

Message ID 20180527215442.14760-5-stefan@agner.ch
State Superseded
Headers show
Series
  • Untitled series #46913
Related show

Commit Message

Stefan Agner May 27, 2018, 9:54 p.m.
From: Lucas Stach <dev@lynxeye.de>

Set up the NAND Flash controller clock to run at 150MHz
instead of the rate set by the bootloader. This is a
conservative rate which also yields good performance.

Signed-off-by: Lucas Stach <dev@lynxeye.de>
Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/clk/tegra/clk-tegra20.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Peter De Schrijver May 28, 2018, 7:55 a.m. | #1
On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> From: Lucas Stach <dev@lynxeye.de>
> 
> Set up the NAND Flash controller clock to run at 150MHz
> instead of the rate set by the bootloader. This is a
> conservative rate which also yields good performance.
> 
> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/clk/tegra/clk-tegra20.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> index 0ee56dd04cec..dff8c425cd28 100644
> --- a/drivers/clk/tegra/clk-tegra20.c
> +++ b/drivers/clk/tegra/clk-tegra20.c
> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
>  	{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 300000000, 0 },
>  	{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 300000000, 0 },
>  	{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 300000000, 0 },
> +	{ TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 150000000, 0 },
>  	/* must be the last entry */
>  	{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
>  };
> -- 
> 2.17.0
> 

Maybe better to specify this in the Tegra20 dtsi? See 
"Assigned clock parents and rates" in Documentation/devicetree/bindings/clock/clock-bindings.txt

Peter.
--
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
Stefan Agner May 28, 2018, 3:53 p.m. | #2
On 28.05.2018 09:55, Peter De Schrijver wrote:
> On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
>> From: Lucas Stach <dev@lynxeye.de>
>>
>> Set up the NAND Flash controller clock to run at 150MHz
>> instead of the rate set by the bootloader. This is a
>> conservative rate which also yields good performance.
>>
>> Signed-off-by: Lucas Stach <dev@lynxeye.de>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/clk/tegra/clk-tegra20.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
>> index 0ee56dd04cec..dff8c425cd28 100644
>> --- a/drivers/clk/tegra/clk-tegra20.c
>> +++ b/drivers/clk/tegra/clk-tegra20.c
>> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
>>  	{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 300000000, 0 },
>>  	{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 300000000, 0 },
>>  	{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 300000000, 0 },
>> +	{ TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 150000000, 0 },
>>  	/* must be the last entry */
>>  	{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
>>  };
>> --
>> 2.17.0
>>
> 
> Maybe better to specify this in the Tegra20 dtsi? See 
> "Assigned clock parents and rates" in
> Documentation/devicetree/bindings/clock/clock-bindings.txt

assigned-clocks indeed works just fine for this case. Thanks for
bringing this up, will drop this patch and add the device tree
properties in v3.

Hm, interesting that none of the Tegra device tree make use of the
feature so far. I guess there would be other cases where this would be
useful as well (the one just above, VDE?).

--
Stefan

--
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
Lucas Stach May 28, 2018, 3:58 p.m. | #3
Am Montag, den 28.05.2018, 17:53 +0200 schrieb Stefan Agner:
> On 28.05.2018 09:55, Peter De Schrijver wrote:
> > On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> > > From: Lucas Stach <dev@lynxeye.de>
> > > 
> > > Set up the NAND Flash controller clock to run at 150MHz
> > > instead of the rate set by the bootloader. This is a
> > > conservative rate which also yields good performance.
> > > 
> > > Signed-off-by: Lucas Stach <dev@lynxeye.de>
> > > Signed-off-by: Stefan Agner <stefan@agner.ch>
> > > ---
> > >  drivers/clk/tegra/clk-tegra20.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> > > index 0ee56dd04cec..dff8c425cd28 100644
> > > --- a/drivers/clk/tegra/clk-tegra20.c
> > > +++ b/drivers/clk/tegra/clk-tegra20.c
> > > @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
> > >  	{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 300000000, 0 },
> > >  	{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 300000000, 0 },
> > >  	{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 300000000, 0 },
> > > +	{ TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 150000000, 0 },
> > >  	/* must be the last entry */
> > >  	{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
> > >  };
> > > --
> > > 2.17.0
> > > 
> > 
> > Maybe better to specify this in the Tegra20 dtsi? See 
> > "Assigned clock parents and rates" in
> > Documentation/devicetree/bindings/clock/clock-bindings.txt
> 
> assigned-clocks indeed works just fine for this case. Thanks for
> bringing this up, will drop this patch and add the device tree
> properties in v3.
> 
> Hm, interesting that none of the Tegra device tree make use of the
> feature so far. I guess there would be other cases where this would be
> useful as well (the one just above, VDE?).

Most of the Tegra clock init stuff (including this patch) was written
before we had assigned-clocks as some common DT facility. But fully
agree on dropping this patch.

Regards,
Lucas
--
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
Peter De Schrijver May 29, 2018, 7:48 a.m. | #4
On Mon, May 28, 2018 at 05:53:08PM +0200, Stefan Agner wrote:
> On 28.05.2018 09:55, Peter De Schrijver wrote:
> > On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> >> From: Lucas Stach <dev@lynxeye.de>
> >>
> >> Set up the NAND Flash controller clock to run at 150MHz
> >> instead of the rate set by the bootloader. This is a
> >> conservative rate which also yields good performance.
> >>
> >> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >> ---
> >>  drivers/clk/tegra/clk-tegra20.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> >> index 0ee56dd04cec..dff8c425cd28 100644
> >> --- a/drivers/clk/tegra/clk-tegra20.c
> >> +++ b/drivers/clk/tegra/clk-tegra20.c
> >> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
> >>  	{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 300000000, 0 },
> >>  	{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 300000000, 0 },
> >>  	{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 300000000, 0 },
> >> +	{ TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 150000000, 0 },
> >>  	/* must be the last entry */
> >>  	{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
> >>  };
> >> --
> >> 2.17.0
> >>
> > 
> > Maybe better to specify this in the Tegra20 dtsi? See 
> > "Assigned clock parents and rates" in
> > Documentation/devicetree/bindings/clock/clock-bindings.txt
> 
> assigned-clocks indeed works just fine for this case. Thanks for
> bringing this up, will drop this patch and add the device tree
> properties in v3.
> 
> Hm, interesting that none of the Tegra device tree make use of the
> feature so far. I guess there would be other cases where this would be
> useful as well (the one just above, VDE?).
> 

Yes, historically this feature wasn't available, so we used these init tables.
Unfortunately it's not easy to get rid of them for parent and rate
configuration, because new kernels should also work with existing DTBs, so we
can't just add assigned-clock properties and remove the existing table
entries. What we could do is use the CLK_IS_CRITICAL flag for all clocks which
are only enabled by the init table. For not yet merged blocks, this is
ofcourse not a concern.

Cheers,

Peter.
--
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
Stefan Agner May 29, 2018, 12:12 p.m. | #5
On 29.05.2018 09:48, Peter De Schrijver wrote:
> On Mon, May 28, 2018 at 05:53:08PM +0200, Stefan Agner wrote:
>> On 28.05.2018 09:55, Peter De Schrijver wrote:
>> > On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
>> >> From: Lucas Stach <dev@lynxeye.de>
>> >>
>> >> Set up the NAND Flash controller clock to run at 150MHz
>> >> instead of the rate set by the bootloader. This is a
>> >> conservative rate which also yields good performance.
>> >>
>> >> Signed-off-by: Lucas Stach <dev@lynxeye.de>
>> >> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> >> ---
>> >>  drivers/clk/tegra/clk-tegra20.c | 1 +
>> >>  1 file changed, 1 insertion(+)
>> >>
>> >> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
>> >> index 0ee56dd04cec..dff8c425cd28 100644
>> >> --- a/drivers/clk/tegra/clk-tegra20.c
>> >> +++ b/drivers/clk/tegra/clk-tegra20.c
>> >> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
>> >>  	{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 300000000, 0 },
>> >>  	{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 300000000, 0 },
>> >>  	{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 300000000, 0 },
>> >> +	{ TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 150000000, 0 },
>> >>  	/* must be the last entry */
>> >>  	{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
>> >>  };
>> >> --
>> >> 2.17.0
>> >>
>> >
>> > Maybe better to specify this in the Tegra20 dtsi? See
>> > "Assigned clock parents and rates" in
>> > Documentation/devicetree/bindings/clock/clock-bindings.txt
>>
>> assigned-clocks indeed works just fine for this case. Thanks for
>> bringing this up, will drop this patch and add the device tree
>> properties in v3.
>>
>> Hm, interesting that none of the Tegra device tree make use of the
>> feature so far. I guess there would be other cases where this would be
>> useful as well (the one just above, VDE?).
>>
> 
> Yes, historically this feature wasn't available, so we used these init tables.
> Unfortunately it's not easy to get rid of them for parent and rate
> configuration, because new kernels should also work with existing DTBs, so we
> can't just add assigned-clock properties and remove the existing table
> entries. What we could do is use the CLK_IS_CRITICAL flag for all clocks which
> are only enabled by the init table. For not yet merged blocks, this is
> ofcourse not a concern.

Sure I understand.

Was just somewhat surprised that it isn't used at all yet (grep -r -e
assigned-clock arch/arm/boot/dts/tegra* returns nothing). After all,
assigned clocks bindings have been merged in 2014 :-)

At least "clk: tegra: Specify VDE clock rate" merged earlier this year
would have been a candidate already.

--
Stefan
--
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
Dmitry Osipenko May 29, 2018, 12:19 p.m. | #6
On 29.05.2018 15:12, Stefan Agner wrote:
> On 29.05.2018 09:48, Peter De Schrijver wrote:
>> On Mon, May 28, 2018 at 05:53:08PM +0200, Stefan Agner wrote:
>>> On 28.05.2018 09:55, Peter De Schrijver wrote:
>>>> On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
>>>>> From: Lucas Stach <dev@lynxeye.de>
>>>>>
>>>>> Set up the NAND Flash controller clock to run at 150MHz
>>>>> instead of the rate set by the bootloader. This is a
>>>>> conservative rate which also yields good performance.
>>>>>
>>>>> Signed-off-by: Lucas Stach <dev@lynxeye.de>
>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>> ---
>>>>>  drivers/clk/tegra/clk-tegra20.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
>>>>> index 0ee56dd04cec..dff8c425cd28 100644
>>>>> --- a/drivers/clk/tegra/clk-tegra20.c
>>>>> +++ b/drivers/clk/tegra/clk-tegra20.c
>>>>> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
>>>>>  	{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 300000000, 0 },
>>>>>  	{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 300000000, 0 },
>>>>>  	{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 300000000, 0 },
>>>>> +	{ TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 150000000, 0 },
>>>>>  	/* must be the last entry */
>>>>>  	{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
>>>>>  };
>>>>> --
>>>>> 2.17.0
>>>>>
>>>>
>>>> Maybe better to specify this in the Tegra20 dtsi? See
>>>> "Assigned clock parents and rates" in
>>>> Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>
>>> assigned-clocks indeed works just fine for this case. Thanks for
>>> bringing this up, will drop this patch and add the device tree
>>> properties in v3.
>>>
>>> Hm, interesting that none of the Tegra device tree make use of the
>>> feature so far. I guess there would be other cases where this would be
>>> useful as well (the one just above, VDE?).
>>>
>>
>> Yes, historically this feature wasn't available, so we used these init tables.
>> Unfortunately it's not easy to get rid of them for parent and rate
>> configuration, because new kernels should also work with existing DTBs, so we
>> can't just add assigned-clock properties and remove the existing table
>> entries. What we could do is use the CLK_IS_CRITICAL flag for all clocks which
>> are only enabled by the init table. For not yet merged blocks, this is
>> ofcourse not a concern.
> 
> Sure I understand.
> 
> Was just somewhat surprised that it isn't used at all yet (grep -r -e
> assigned-clock arch/arm/boot/dts/tegra* returns nothing). After all,
> assigned clocks bindings have been merged in 2014 :-)
> 
> At least "clk: tegra: Specify VDE clock rate" merged earlier this year
> would have been a candidate already.

I wasn't even aware of existence of the assigned-clock properties, probably just
like others.
--
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
Peter De Schrijver May 30, 2018, 7:42 a.m. | #7
On Tue, May 29, 2018 at 03:19:47PM +0300, Dmitry Osipenko wrote:
> On 29.05.2018 15:12, Stefan Agner wrote:
> > On 29.05.2018 09:48, Peter De Schrijver wrote:
> >> On Mon, May 28, 2018 at 05:53:08PM +0200, Stefan Agner wrote:
> >>> On 28.05.2018 09:55, Peter De Schrijver wrote:
> >>>> On Sun, May 27, 2018 at 11:54:40PM +0200, Stefan Agner wrote:
> >>>>> From: Lucas Stach <dev@lynxeye.de>
> >>>>>
> >>>>> Set up the NAND Flash controller clock to run at 150MHz
> >>>>> instead of the rate set by the bootloader. This is a
> >>>>> conservative rate which also yields good performance.
> >>>>>
> >>>>> Signed-off-by: Lucas Stach <dev@lynxeye.de>
> >>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
> >>>>> ---
> >>>>>  drivers/clk/tegra/clk-tegra20.c | 1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
> >>>>> index 0ee56dd04cec..dff8c425cd28 100644
> >>>>> --- a/drivers/clk/tegra/clk-tegra20.c
> >>>>> +++ b/drivers/clk/tegra/clk-tegra20.c
> >>>>> @@ -1049,6 +1049,7 @@ static struct tegra_clk_init_table init_table[] __initdata = {
> >>>>>  	{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 300000000, 0 },
> >>>>>  	{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 300000000, 0 },
> >>>>>  	{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 300000000, 0 },
> >>>>> +	{ TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 150000000, 0 },
> >>>>>  	/* must be the last entry */
> >>>>>  	{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
> >>>>>  };
> >>>>> --
> >>>>> 2.17.0
> >>>>>
> >>>>
> >>>> Maybe better to specify this in the Tegra20 dtsi? See
> >>>> "Assigned clock parents and rates" in
> >>>> Documentation/devicetree/bindings/clock/clock-bindings.txt
> >>>
> >>> assigned-clocks indeed works just fine for this case. Thanks for
> >>> bringing this up, will drop this patch and add the device tree
> >>> properties in v3.
> >>>
> >>> Hm, interesting that none of the Tegra device tree make use of the
> >>> feature so far. I guess there would be other cases where this would be
> >>> useful as well (the one just above, VDE?).
> >>>
> >>
> >> Yes, historically this feature wasn't available, so we used these init tables.
> >> Unfortunately it's not easy to get rid of them for parent and rate
> >> configuration, because new kernels should also work with existing DTBs, so we
> >> can't just add assigned-clock properties and remove the existing table
> >> entries. What we could do is use the CLK_IS_CRITICAL flag for all clocks which
> >> are only enabled by the init table. For not yet merged blocks, this is
> >> ofcourse not a concern.
> > 
> > Sure I understand.
> > 
> > Was just somewhat surprised that it isn't used at all yet (grep -r -e
> > assigned-clock arch/arm/boot/dts/tegra* returns nothing). After all,
> > assigned clocks bindings have been merged in 2014 :-)
> > 
> > At least "clk: tegra: Specify VDE clock rate" merged earlier this year
> > would have been a candidate already.
> 
> I wasn't even aware of existence of the assigned-clock properties, probably just
> like others.

This feature seems to be little used indeed. Not sure why.

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

Patch

diff --git a/drivers/clk/tegra/clk-tegra20.c b/drivers/clk/tegra/clk-tegra20.c
index 0ee56dd04cec..dff8c425cd28 100644
--- a/drivers/clk/tegra/clk-tegra20.c
+++ b/drivers/clk/tegra/clk-tegra20.c
@@ -1049,6 +1049,7 @@  static struct tegra_clk_init_table init_table[] __initdata = {
 	{ TEGRA20_CLK_GR2D, TEGRA20_CLK_PLL_C, 300000000, 0 },
 	{ TEGRA20_CLK_GR3D, TEGRA20_CLK_PLL_C, 300000000, 0 },
 	{ TEGRA20_CLK_VDE, TEGRA20_CLK_CLK_MAX, 300000000, 0 },
+	{ TEGRA20_CLK_NDFLASH, TEGRA20_CLK_PLL_P, 150000000, 0 },
 	/* must be the last entry */
 	{ TEGRA20_CLK_CLK_MAX, TEGRA20_CLK_CLK_MAX, 0, 0 },
 };