[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Peter De Schrijver]

Add references to tegra_car clocks for the basic device nodes.

Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 arch/arm/boot/dts/tegra114.dtsi |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index 96a8235..6b0a01e 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -24,10 +24,11 @@
 			      0 42 0x04
 			      0 121 0x04
 			      0 122 0x04>;
+		clocks = <&tegra_car 5>;
 	};
 
 	tegra_car: clock {
-		compatible = "nvidia,tegra114-car, nvidia,tegra30-car";
+		compatible = "nvidia,tegra114-car";
 		reg = <0x60006000 0x1000>;
 		#clock-cells = <1>;
 	};
@@ -43,6 +44,7 @@
 		reg-shift = <2>;
 		interrupts = <0 36 0x04>;
 		status = "disabled";
+		clocks = <&tegra_car 6>;
 	};
 
 	serial at 70006040 {
@@ -51,6 +53,7 @@
 		reg-shift = <2>;
 		interrupts = <0 37 0x04>;
 		status = "disabled";
+		clocks = <&tegra_car 192>;
 	};
 
 	serial at 70006200 {
@@ -59,6 +62,7 @@
 		reg-shift = <2>;
 		interrupts = <0 46 0x04>;
 		status = "disabled";
+		clocks = <&tegra_car 55>;
 	};
 
 	serial at 70006300 {
@@ -67,12 +71,14 @@
 		reg-shift = <2>;
 		interrupts = <0 90 0x04>;
 		status = "disabled";
+		clocks = <&tegra_car 65>;
 	};
 
 	rtc {
 		compatible = "nvidia,tegra114-rtc", "nvidia,tegra20-rtc";
 		reg = <0x7000e000 0x100>;
 		interrupts = <0 2 0x04>;
+		clocks = <&tegra_car 4>;
 	};
 
 	pmc {
-- 
1.7.1

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Felipe Balbi]

Hi,

On Fri, Feb 08, 2013 at 03:36:40PM +0200, Peter De Schrijver wrote:
>  	tegra_car: clock {
> -		compatible = "nvidia,tegra114-car, nvidia,tegra30-car";
> +		compatible = "nvidia,tegra114-car";

trailing change or intentional ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130208/202974d2/attachment.sig>

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Stephen Warren]

On 02/08/2013 07:03 AM, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Feb 08, 2013 at 03:36:40PM +0200, Peter De Schrijver
> wrote:
>> tegra_car: clock { -		compatible = "nvidia,tegra114-car,
>> nvidia,tegra30-car"; +		compatible = "nvidia,tegra114-car";
> 
> trailing change or intentional ?

I believe it's intentional since the HW isn't fully
backwards-compatible. However, the commit description should mention this.

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Peter De Schrijver]

On Fri, Feb 08, 2013 at 03:03:02PM +0100, Felipe Balbi wrote:
> * PGP Signed by an unknown key
> 
> Hi,
> 
> On Fri, Feb 08, 2013 at 03:36:40PM +0200, Peter De Schrijver wrote:
> >  	tegra_car: clock {
> > -		compatible = "nvidia,tegra114-car, nvidia,tegra30-car";
> > +		compatible = "nvidia,tegra114-car";
> 
> trailing change or intentional ?

No, the Tegra30 car driver shouldn't be used for Tegra114.

Cheers,

Peter.

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Peter De Schrijver]

Add references to tegra_car clocks for the basic device nodes.

Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
---
 arch/arm/boot/dts/tegra114.dtsi |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index 96a8235..6b0a01e 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -24,10 +24,11 @@
 			      0 42 0x04
 			      0 121 0x04
 			      0 122 0x04>;
+		clocks = <&tegra_car 5>;
 	};
 
 	tegra_car: clock {
-		compatible = "nvidia,tegra114-car, nvidia,tegra30-car";
+		compatible = "nvidia,tegra114-car";
 		reg = <0x60006000 0x1000>;
 		#clock-cells = <1>;
 	};
@@ -43,6 +44,7 @@
 		reg-shift = <2>;
 		interrupts = <0 36 0x04>;
 		status = "disabled";
+		clocks = <&tegra_car 6>;
 	};
 
 	serial at 70006040 {
@@ -51,6 +53,7 @@
 		reg-shift = <2>;
 		interrupts = <0 37 0x04>;
 		status = "disabled";
+		clocks = <&tegra_car 192>;
 	};
 
 	serial at 70006200 {
@@ -59,6 +62,7 @@
 		reg-shift = <2>;
 		interrupts = <0 46 0x04>;
 		status = "disabled";
+		clocks = <&tegra_car 55>;
 	};
 
 	serial at 70006300 {
@@ -67,12 +71,14 @@
 		reg-shift = <2>;
 		interrupts = <0 90 0x04>;
 		status = "disabled";
+		clocks = <&tegra_car 65>;
 	};
 
 	rtc {
 		compatible = "nvidia,tegra114-rtc", "nvidia,tegra20-rtc";
 		reg = <0x7000e000 0x100>;
 		interrupts = <0 2 0x04>;
+		clocks = <&tegra_car 4>;
 	};
 
 	pmc {
-- 
1.7.1

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Stephen Warren]

On 02/11/2013 04:04 AM, Peter De Schrijver wrote:
> Add references to tegra_car clocks for the basic device nodes.

In this patch, you also need to remove the "clock-frequency" properties
from tegra114-*.dts board files.

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Peter De Schrijver]

On Tue, Feb 12, 2013 at 06:24:05PM +0100, Stephen Warren wrote:
> On 02/11/2013 04:04 AM, Peter De Schrijver wrote:
> > Add references to tegra_car clocks for the basic device nodes.
> 
> In this patch, you also need to remove the "clock-frequency" properties
> from tegra114-*.dts board files.

That would probably break bisectability no? I think this needs to be done
in a separate patch. Also, the UARTD enabling in init_table[] can then be
removed.

Cheers,

Peter.

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Stephen Warren]

On 02/13/2013 04:38 AM, Peter De Schrijver wrote:
> On Tue, Feb 12, 2013 at 06:24:05PM +0100, Stephen Warren wrote:
>> On 02/11/2013 04:04 AM, Peter De Schrijver wrote:
>>> Add references to tegra_car clocks for the basic device nodes.
>>
>> In this patch, you also need to remove the "clock-frequency" properties
>> from tegra114-*.dts board files.
> 
> That would probably break bisectability no?

Oh right, I read the patch order backwards. You can fix this by moving
this patch to the end of the series.

> I think this needs to be done in a separate patch.

To avoid bisect issues, the clock-frequency property should be removed
in the same patch that adds the clocks property, or after it.

> Also, the UARTD enabling in init_table[] can then be removed.

You still need to initialize all the UART clocks in init_table[]. This
is required to make sure the clocks have the correct parent. Without
this explicit initialization, on Tegra30 at least, it was found that
UART C didn't work correctly for Bluetooth since the default parent was
something that wasn't generating the expected frequency.

Before removing the clock-frequency property, you also need to make sure
that init_table[] forces the UART clock on, otherwise since nothing will
clk_get()/enable() the UART clock, clk_disable_unused() will disable it.
After removing clock-frequency property, of_serial.c will
clk_get()/enable() the UART clock based on the clocks property, and
hence init_table[] doesn't need to force the UART clock on.

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Peter De Schrijver]

On Wed, Feb 13, 2013 at 05:48:03PM +0100, Stephen Warren wrote:
> On 02/13/2013 04:38 AM, Peter De Schrijver wrote:
> > On Tue, Feb 12, 2013 at 06:24:05PM +0100, Stephen Warren wrote:
> >> On 02/11/2013 04:04 AM, Peter De Schrijver wrote:
> >>> Add references to tegra_car clocks for the basic device nodes.
> >>
> >> In this patch, you also need to remove the "clock-frequency" properties
> >> from tegra114-*.dts board files.
> > 
> > That would probably break bisectability no?
> 
> Oh right, I read the patch order backwards. You can fix this by moving
> this patch to the end of the series.
> 

Probably. Need to check if this doesn't cause other problems.

> > I think this needs to be done in a separate patch.
> 
> To avoid bisect issues, the clock-frequency property should be removed
> in the same patch that adds the clocks property, or after it.
> 
> > Also, the UARTD enabling in init_table[] can then be removed.
> 
> You still need to initialize all the UART clocks in init_table[]. This
> is required to make sure the clocks have the correct parent. Without
> this explicit initialization, on Tegra30 at least, it was found that
> UART C didn't work correctly for Bluetooth since the default parent was
> something that wasn't generating the expected frequency.
> 

Yes. The parent relationships still need to be defined. But I think that's
the only thing we actually need to define still? Also the parent relationships
can be board specific I think in some cases, so maybe we want to move those to
DT as well at some point?

Cheers,

Peter.

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Stephen Warren]

On 02/14/2013 03:01 AM, Peter De Schrijver wrote:
> On Wed, Feb 13, 2013 at 05:48:03PM +0100, Stephen Warren wrote:
...
>> You still need to initialize all the UART clocks in init_table[]. This
...
> Yes. The parent relationships still need to be defined. But I think that's
> the only thing we actually need to define still?

You might want to explicitly set the rate too, if there is a divider in
the clk module that affects it. If not, then parenting is indeed all you
need.

> Also the parent relationships
> can be board specific I think in some cases, so maybe we want to move those to
> DT as well at some point?

Yes, that's the plan. I think/thought Prashant was planning to work on that.

[PATCH v6 08/10] ARM: dt: Add references to tegra_car clocks

[Peter De Schrijver]

On Thu, Feb 14, 2013 at 06:30:39PM +0100, Stephen Warren wrote:
> On 02/14/2013 03:01 AM, Peter De Schrijver wrote:
> > On Wed, Feb 13, 2013 at 05:48:03PM +0100, Stephen Warren wrote:
> ...
> >> You still need to initialize all the UART clocks in init_table[]. This
> ...
> > Yes. The parent relationships still need to be defined. But I think that's
> > the only thing we actually need to define still?
> 
> You might want to explicitly set the rate too, if there is a divider in
> the clk module that affects it. If not, then parenting is indeed all you
> need.

Yes, for PLLs that might be useful. Device clock rates can also be set by the
driver and probably should be set by the driver.

Cheers,

Peter.