[Patch 0/3] ARM: dts: am43x-vpfe/ov2659.patch

[Patch 0/3] ARM: dts: am43x-vpfe/ov2659.patch

[Benoit Parrot]

This patch series adds the missing camera endpoint (ov2659) as well as
the required source clocks nodes for the sensor.

On the am437x-sk-evm the camera sensor is sourced from clkout1 but that
clock nodes/tree was removed as it was unsed at the time, we are
re-adding the needed clock nodes here.

Benoit Parrot (2):
  ARM: dts: am437x-sk-evm: Add VPFE and OV2659 entries
  ARM: dts: am43x-epos-evm: Add VPFE and OV2659 entries

Tero Kristo (1):
  ARM: dts: am43xx: add support for clkout1 clock

 arch/arm/boot/dts/am437x-sk-evm.dts  | 27 +++++++++++++-
 arch/arm/boot/dts/am43x-epos-evm.dts | 23 +++++++++++-
 arch/arm/boot/dts/am43xx-clocks.dtsi | 54 ++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+), 2 deletions(-)

-- 
2.17.1

[Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

[Benoit Parrot]

From: Tero Kristo <t-kristo@ti.com>

clkout1 clock node and its generation tree was missing. Add this based
on the data on TRM and PRCM functional spec.

commit 664ae1ab2536 ("ARM: dts: am43xx: add clkctrl nodes") effectively
reverted this commit 8010f13a40d3 ("ARM: dts: am43xx: add support for
clkout1 clock") which is needed for the ov2659 camera sensor clock
definition hence it is being re-applied here.

Fixes: 664ae1ab2536 ("ARM: dts: am43xx: add clkctrl nodes")
Signed-off-by: Tero Kristo <t-kristo@ti.com>
Tested-by: Benoit Parrot <bparrot@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 arch/arm/boot/dts/am43xx-clocks.dtsi | 54 ++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/arch/arm/boot/dts/am43xx-clocks.dtsi b/arch/arm/boot/dts/am43xx-clocks.dtsi
index 091356f2a8c1..944b142dafd2 100644
--- a/arch/arm/boot/dts/am43xx-clocks.dtsi
+++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
@@ -704,6 +704,60 @@
 		ti,bit-shift = <8>;
 		reg = <0x2a48>;
 	};
+
+	clkout1_osc_div_ck: clkout1_osc_div_ck {
+		#clock-cells = <0>;
+		compatible = "ti,divider-clock";
+		clocks = <&sys_clkin_ck>;
+		ti,bit-shift = <20>;
+		ti,max-div = <4>;
+		reg = <0x4100>;
+	};
+
+	clkout1_src2_mux_ck: clkout1_src2_mux_ck {
+		#clock-cells = <0>;
+		compatible = "ti,mux-clock";
+		clocks = <&clk_rc32k_ck>, <&sysclk_div>, <&dpll_ddr_m2_ck>,
+			 <&dpll_per_m2_ck>, <&dpll_disp_m2_ck>,
+			 <&dpll_mpu_m2_ck>;
+		reg = <0x4100>;
+	};
+
+	clkout1_src2_pre_div_ck: clkout1_src2_pre_div_ck {
+		#clock-cells = <0>;
+		compatible = "ti,divider-clock";
+		clocks = <&clkout1_src2_mux_ck>;
+		ti,bit-shift = <4>;
+		ti,max-div = <8>;
+		reg = <0x4100>;
+	};
+
+	clkout1_src2_post_div_ck: clkout1_src2_post_div_ck {
+		#clock-cells = <0>;
+		compatible = "ti,divider-clock";
+		clocks = <&clkout1_src2_pre_div_ck>;
+		ti,bit-shift = <8>;
+		ti,max-div = <32>;
+		ti,index-power-of-two;
+		reg = <0x4100>;
+	};
+
+	clkout1_mux_ck: clkout1_mux_ck {
+		#clock-cells = <0>;
+		compatible = "ti,mux-clock";
+		clocks = <&clkout1_osc_div_ck>, <&clk_rc32k_ck>,
+			 <&clkout1_src2_post_div_ck>, <&dpll_extdev_m2_ck>;
+		ti,bit-shift = <16>;
+		reg = <0x4100>;
+	};
+
+	clkout1_ck: clkout1_ck {
+		#clock-cells = <0>;
+		compatible = "ti,gate-clock";
+		clocks = <&clkout1_mux_ck>;
+		ti,bit-shift = <23>;
+		reg = <0x4100>;
+	};
 };
 
 &prcm {
-- 
2.17.1

[Patch 2/3] ARM: dts: am437x-sk-evm: Add VPFE and OV2659 entries

[Benoit Parrot]

Add VPFE device nodes entries.
Add OmniVision OV2659 sensor device nodes and linkage.

The sensor clock (xvclk) is sourced from clkout1.
Add clock entries to properly select clkout1 and set its parent
clock to sys_clkin_ck.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 arch/arm/boot/dts/am437x-sk-evm.dts | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am437x-sk-evm.dts b/arch/arm/boot/dts/am437x-sk-evm.dts
index 74eaa6a3b258..25222497f828 100644
--- a/arch/arm/boot/dts/am437x-sk-evm.dts
+++ b/arch/arm/boot/dts/am437x-sk-evm.dts
@@ -272,6 +272,12 @@
 		>;
 	};
 
+	clkout1_pin: pinmux_clkout1_pin {
+		pinctrl-single,pins = <
+			0x270 (PIN_OUTPUT_PULLDOWN | MUX_MODE3)	/* XDMA_EVENT_INTR0/CLKOUT1 */
+		>;
+	};
+
 	cpsw_default: cpsw_default {
 		pinctrl-single,pins = <
 			/* Slave 1 */
@@ -593,6 +599,25 @@
 	pinctrl-0 = <&i2c1_pins>;
 	clock-frequency = <400000>;
 
+	ov2659@30 {
+		compatible = "ovti,ov2659";
+		reg = <0x30>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&clkout1_pin>;
+
+		clocks = <&clkout1_mux_ck>;
+		clock-names = "xvclk";
+		assigned-clocks = <&clkout1_mux_ck>;
+		assigned-clock-parents = <&clkout1_osc_div_ck>;
+
+		port {
+			ov2659_1: endpoint {
+				remote-endpoint = <&vpfe0_ep>;
+				link-frequencies = /bits/ 64 <70000000>;
+			};
+		};
+	};
+
 	edt-ft5306@38 {
 		status = "okay";
 		compatible = "edt,edt-ft5306", "edt,edt-ft5x06";
@@ -877,7 +902,7 @@
 	/* Camera port */
 	port {
 		vpfe0_ep: endpoint {
-			/* remote-endpoint = <&sensor>; add once we have it */
+			remote-endpoint = <&ov2659_1>;
 			ti,am437x-vpfe-interface = <0>;
 			bus-width = <8>;
 			hsync-active = <0>;
-- 
2.17.1

[Patch 3/3] ARM: dts: am43x-epos-evm: Add VPFE and OV2659 entries

[Benoit Parrot]

Add VPFE device nodes entries.
Add OmniVision OV2659 sensor device nodes and linkage.

Since Rev1.2a on this board the sensor source clock (xvclk) has a
dedicated 12Mhz oscillator instead of using clkout1.
Add 'audio_mstrclk' fixed clock object to represent it.

Signed-off-by: Benoit Parrot <bparrot@ti.com>
---
 arch/arm/boot/dts/am43x-epos-evm.dts | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/am43x-epos-evm.dts b/arch/arm/boot/dts/am43x-epos-evm.dts
index 95314121d111..1958d0985f76 100644
--- a/arch/arm/boot/dts/am43x-epos-evm.dts
+++ b/arch/arm/boot/dts/am43x-epos-evm.dts
@@ -145,6 +145,12 @@
 			system-clock-frequency = <12000000>;
 		};
 	};
+
+	audio_mstrclk: mclk_osc {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <12000000>;
+	};
 };
 
 &am43xx_pinmux {
@@ -696,6 +702,21 @@
 		IOVDD-supply = <&dcdc4>; /* V3_3D -> DCDC4 */
 		DVDD-supply = <&ldo1>; /* V1_8AUD -> V1_8D -> LDO1 */
 	};
+
+	ov2659@30 {
+		compatible = "ovti,ov2659";
+		reg = <0x30>;
+
+		clocks = <&audio_mstrclk>;
+		clock-names = "xvclk";
+
+		port {
+			ov2659_1: endpoint {
+				remote-endpoint = <&vpfe1_ep>;
+				link-frequencies = /bits/ 64 <70000000>;
+			};
+		};
+	};
 };
 
 &i2c2 {
@@ -962,7 +983,7 @@
 
 	port {
 		vpfe1_ep: endpoint {
-			/* remote-endpoint = <&sensor>; add once we have it */
+			remote-endpoint = <&ov2659_1>;
 			ti,am437x-vpfe-interface = <0>;
 			bus-width = <8>;
 			hsync-active = <0>;
-- 
2.17.1

Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

[Tony Lindgren]

* Benoit Parrot <bparrot@ti.com> [191016 18:47]:
> --- a/arch/arm/boot/dts/am43xx-clocks.dtsi
> +++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
> @@ -704,6 +704,60 @@
>  		ti,bit-shift = <8>;
>  		reg = <0x2a48>;
>  	};
> +
> +	clkout1_osc_div_ck: clkout1_osc_div_ck {
> +		#clock-cells = <0>;
> +		compatible = "ti,divider-clock";
> +		clocks = <&sys_clkin_ck>;
> +		ti,bit-shift = <20>;
> +		ti,max-div = <4>;
> +		reg = <0x4100>;
> +	};

Here too please describe why the clock names are not generic.

Regards,

Tony

Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

[Benoit Parrot]

Tony Lindgren <tony@atomide.com> wrote on Tue [2019-Oct-22 08:48:16 -0700]:
> * Benoit Parrot <bparrot@ti.com> [191016 18:47]:
> > --- a/arch/arm/boot/dts/am43xx-clocks.dtsi
> > +++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
> > @@ -704,6 +704,60 @@
> >  		ti,bit-shift = <8>;
> >  		reg = <0x2a48>;
> >  	};
> > +
> > +	clkout1_osc_div_ck: clkout1_osc_div_ck {
> > +		#clock-cells = <0>;
> > +		compatible = "ti,divider-clock";
> > +		clocks = <&sys_clkin_ck>;
> > +		ti,bit-shift = <20>;
> > +		ti,max-div = <4>;
> > +		reg = <0x4100>;
> > +	};
> 
> Here too please describe why the clock names are not generic.

Tero originally had this patch in the kernel so this is somewhat of a
revert. Since these "clock" were removed. If the name syntax is no longer
valid for some reason, then I will need a little more informations to
proceed.

Tero, can you assist here?

Benoit

> 
> Regards,
> 
> Tony

Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

[Tero Kristo]

On 22/10/2019 19:21, Benoit Parrot wrote:
> Tony Lindgren <tony@atomide.com> wrote on Tue [2019-Oct-22 08:48:16 -0700]:
>> * Benoit Parrot <bparrot@ti.com> [191016 18:47]:
>>> --- a/arch/arm/boot/dts/am43xx-clocks.dtsi
>>> +++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
>>> @@ -704,6 +704,60 @@
>>>   		ti,bit-shift = <8>;
>>>   		reg = <0x2a48>;
>>>   	};
>>> +
>>> +	clkout1_osc_div_ck: clkout1_osc_div_ck {
>>> +		#clock-cells = <0>;
>>> +		compatible = "ti,divider-clock";
>>> +		clocks = <&sys_clkin_ck>;
>>> +		ti,bit-shift = <20>;
>>> +		ti,max-div = <4>;
>>> +		reg = <0x4100>;
>>> +	};
>>
>> Here too please describe why the clock names are not generic.
> 
> Tero originally had this patch in the kernel so this is somewhat of a
> revert. Since these "clock" were removed. If the name syntax is no longer
> valid for some reason, then I will need a little more informations to
> proceed.
> 
> Tero, can you assist here?

This one is just following the naming convention of the rest of the 
clocks atm.

If we need to fix all the underscore name clocks, that requires pretty 
much complete revamp of both the dts data + clock data under the clock 
driver, and it is not backwards compatible either. How should we tackle 
that one?

We could maybe add support code in kernel to do s/-/_/g for the "new" 
clocks so that their parent-child relationships would be retained, and 
then convert the clocks in phases.

-Tero

> 
> Benoit
> 
>>
>> Regards,
>>
>> Tony

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [191022 16:48]:
> On 22/10/2019 19:21, Benoit Parrot wrote:
> > Tony Lindgren <tony@atomide.com> wrote on Tue [2019-Oct-22 08:48:16 -0700]:
> > > * Benoit Parrot <bparrot@ti.com> [191016 18:47]:
> > > > --- a/arch/arm/boot/dts/am43xx-clocks.dtsi
> > > > +++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
> > > > @@ -704,6 +704,60 @@
> > > >   		ti,bit-shift = <8>;
> > > >   		reg = <0x2a48>;
> > > >   	};
> > > > +
> > > > +	clkout1_osc_div_ck: clkout1_osc_div_ck {
> > > > +		#clock-cells = <0>;
> > > > +		compatible = "ti,divider-clock";
> > > > +		clocks = <&sys_clkin_ck>;
> > > > +		ti,bit-shift = <20>;
> > > > +		ti,max-div = <4>;
> > > > +		reg = <0x4100>;
> > > > +	};
> > > 
> > > Here too please describe why the clock names are not generic.
> > 
> > Tero originally had this patch in the kernel so this is somewhat of a
> > revert. Since these "clock" were removed. If the name syntax is no longer
> > valid for some reason, then I will need a little more informations to
> > proceed.
> > 
> > Tero, can you assist here?
> 
> This one is just following the naming convention of the rest of the clocks
> atm.
> 
> If we need to fix all the underscore name clocks, that requires pretty much
> complete revamp of both the dts data + clock data under the clock driver,
> and it is not backwards compatible either. How should we tackle that one?
> 
> We could maybe add support code in kernel to do s/-/_/g for the "new" clocks
> so that their parent-child relationships would be retained, and then convert
> the clocks in phases.

Well some of them can be fixed by configuring things based
on the compatible value and then the node name can be just
clock like it should be.

Here too one option would be to add custom compatibles like:

compatible = "ti,clkout1-osc-div", "ti,divider-clock";

And then have match data configure the rest.

The other option would be to have lookup tables in the clock
driver based on the SoC and reg address.

This is a hidden mine though.. We've hit it already several times,
and any dts clean-up effort has a chance of breaking things.

Regards,

Tony

Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

[Tony Lindgren]

* Tony Lindgren <tony@atomide.com> [191022 16:56]:
> * Tero Kristo <t-kristo@ti.com> [191022 16:48]:
> > On 22/10/2019 19:21, Benoit Parrot wrote:
> > > Tony Lindgren <tony@atomide.com> wrote on Tue [2019-Oct-22 08:48:16 -0700]:
> > > > * Benoit Parrot <bparrot@ti.com> [191016 18:47]:
> > > > > --- a/arch/arm/boot/dts/am43xx-clocks.dtsi
> > > > > +++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
> > > > > @@ -704,6 +704,60 @@
> > > > >   		ti,bit-shift = <8>;
> > > > >   		reg = <0x2a48>;
> > > > >   	};
> > > > > +
> > > > > +	clkout1_osc_div_ck: clkout1_osc_div_ck {
> > > > > +		#clock-cells = <0>;
> > > > > +		compatible = "ti,divider-clock";
> > > > > +		clocks = <&sys_clkin_ck>;
> > > > > +		ti,bit-shift = <20>;
> > > > > +		ti,max-div = <4>;
> > > > > +		reg = <0x4100>;
> > > > > +	};
> > > > 
> > > > Here too please describe why the clock names are not generic.
> > > 
> > > Tero originally had this patch in the kernel so this is somewhat of a
> > > revert. Since these "clock" were removed. If the name syntax is no longer
> > > valid for some reason, then I will need a little more informations to
> > > proceed.
> > > 
> > > Tero, can you assist here?
> > 
> > This one is just following the naming convention of the rest of the clocks
> > atm.
> > 
> > If we need to fix all the underscore name clocks, that requires pretty much
> > complete revamp of both the dts data + clock data under the clock driver,
> > and it is not backwards compatible either. How should we tackle that one?
> > 
> > We could maybe add support code in kernel to do s/-/_/g for the "new" clocks
> > so that their parent-child relationships would be retained, and then convert
> > the clocks in phases.
> 
> Well some of them can be fixed by configuring things based
> on the compatible value and then the node name can be just
> clock like it should be.
> 
> Here too one option would be to add custom compatibles like:
> 
> compatible = "ti,clkout1-osc-div", "ti,divider-clock";
> 
> And then have match data configure the rest.
> 
> The other option would be to have lookup tables in the clock
> driver based on the SoC and reg address.
> 
> This is a hidden mine though.. We've hit it already several times,
> and any dts clean-up effort has a chance of breaking things.

Hmm maybe in this case just doing this is enough:

clkout1_osc_div_ck: clock@4100 {
	... 
}

Or do all the TI clocks we have have a dependency to the
node naming?

Regards,

Tony

Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

[Benoit Parrot]

Tony Lindgren <tony@atomide.com> wrote on Wed [2019-Oct-23 08:56:57 -0700]:
> * Tony Lindgren <tony@atomide.com> [191022 16:56]:
> > * Tero Kristo <t-kristo@ti.com> [191022 16:48]:
> > > On 22/10/2019 19:21, Benoit Parrot wrote:
> > > > Tony Lindgren <tony@atomide.com> wrote on Tue [2019-Oct-22 08:48:16 -0700]:
> > > > > * Benoit Parrot <bparrot@ti.com> [191016 18:47]:
> > > > > > --- a/arch/arm/boot/dts/am43xx-clocks.dtsi
> > > > > > +++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
> > > > > > @@ -704,6 +704,60 @@
> > > > > >   		ti,bit-shift = <8>;
> > > > > >   		reg = <0x2a48>;
> > > > > >   	};
> > > > > > +
> > > > > > +	clkout1_osc_div_ck: clkout1_osc_div_ck {
> > > > > > +		#clock-cells = <0>;
> > > > > > +		compatible = "ti,divider-clock";
> > > > > > +		clocks = <&sys_clkin_ck>;
> > > > > > +		ti,bit-shift = <20>;
> > > > > > +		ti,max-div = <4>;
> > > > > > +		reg = <0x4100>;
> > > > > > +	};
> > > > > 
> > > > > Here too please describe why the clock names are not generic.
> > > > 
> > > > Tero originally had this patch in the kernel so this is somewhat of a
> > > > revert. Since these "clock" were removed. If the name syntax is no longer
> > > > valid for some reason, then I will need a little more informations to
> > > > proceed.
> > > > 
> > > > Tero, can you assist here?
> > > 
> > > This one is just following the naming convention of the rest of the clocks
> > > atm.
> > > 
> > > If we need to fix all the underscore name clocks, that requires pretty much
> > > complete revamp of both the dts data + clock data under the clock driver,
> > > and it is not backwards compatible either. How should we tackle that one?
> > > 
> > > We could maybe add support code in kernel to do s/-/_/g for the "new" clocks
> > > so that their parent-child relationships would be retained, and then convert
> > > the clocks in phases.
> > 
> > Well some of them can be fixed by configuring things based
> > on the compatible value and then the node name can be just
> > clock like it should be.
> > 
> > Here too one option would be to add custom compatibles like:
> > 
> > compatible = "ti,clkout1-osc-div", "ti,divider-clock";
> > 
> > And then have match data configure the rest.
> > 
> > The other option would be to have lookup tables in the clock
> > driver based on the SoC and reg address.
> > 
> > This is a hidden mine though.. We've hit it already several times,
> > and any dts clean-up effort has a chance of breaking things.
> 
> Hmm maybe in this case just doing this is enough:
> 
> clkout1_osc_div_ck: clock@4100 {
> 	... 
> }

But then we would end up with 6 clock node with the same name "clock@4100",
doesn't pose a problem somewhere?

Tero?

Benoit

> 
> Or do all the TI clocks we have have a dependency to the
> node naming?
> 
> Regards,
> 
> Tony
> 

Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

[Tero Kristo]

On 30/10/2019 21:59, Benoit Parrot wrote:
> Tony Lindgren <tony@atomide.com> wrote on Wed [2019-Oct-23 08:56:57 -0700]:
>> * Tony Lindgren <tony@atomide.com> [191022 16:56]:
>>> * Tero Kristo <t-kristo@ti.com> [191022 16:48]:
>>>> On 22/10/2019 19:21, Benoit Parrot wrote:
>>>>> Tony Lindgren <tony@atomide.com> wrote on Tue [2019-Oct-22 08:48:16 -0700]:
>>>>>> * Benoit Parrot <bparrot@ti.com> [191016 18:47]:
>>>>>>> --- a/arch/arm/boot/dts/am43xx-clocks.dtsi
>>>>>>> +++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
>>>>>>> @@ -704,6 +704,60 @@
>>>>>>>    		ti,bit-shift = <8>;
>>>>>>>    		reg = <0x2a48>;
>>>>>>>    	};
>>>>>>> +
>>>>>>> +	clkout1_osc_div_ck: clkout1_osc_div_ck {
>>>>>>> +		#clock-cells = <0>;
>>>>>>> +		compatible = "ti,divider-clock";
>>>>>>> +		clocks = <&sys_clkin_ck>;
>>>>>>> +		ti,bit-shift = <20>;
>>>>>>> +		ti,max-div = <4>;
>>>>>>> +		reg = <0x4100>;
>>>>>>> +	};
>>>>>>
>>>>>> Here too please describe why the clock names are not generic.
>>>>>
>>>>> Tero originally had this patch in the kernel so this is somewhat of a
>>>>> revert. Since these "clock" were removed. If the name syntax is no longer
>>>>> valid for some reason, then I will need a little more informations to
>>>>> proceed.
>>>>>
>>>>> Tero, can you assist here?
>>>>
>>>> This one is just following the naming convention of the rest of the clocks
>>>> atm.
>>>>
>>>> If we need to fix all the underscore name clocks, that requires pretty much
>>>> complete revamp of both the dts data + clock data under the clock driver,
>>>> and it is not backwards compatible either. How should we tackle that one?
>>>>
>>>> We could maybe add support code in kernel to do s/-/_/g for the "new" clocks
>>>> so that their parent-child relationships would be retained, and then convert
>>>> the clocks in phases.
>>>
>>> Well some of them can be fixed by configuring things based
>>> on the compatible value and then the node name can be just
>>> clock like it should be.
>>>
>>> Here too one option would be to add custom compatibles like:
>>>
>>> compatible = "ti,clkout1-osc-div", "ti,divider-clock";
>>>
>>> And then have match data configure the rest.
>>>
>>> The other option would be to have lookup tables in the clock
>>> driver based on the SoC and reg address.
>>>
>>> This is a hidden mine though.. We've hit it already several times,
>>> and any dts clean-up effort has a chance of breaking things.
>>
>> Hmm maybe in this case just doing this is enough:
>>
>> clkout1_osc_div_ck: clock@4100 {
>> 	...
>> }
> 
> But then we would end up with 6 clock node with the same name "clock@4100",
> doesn't pose a problem somewhere?

Yeah, clk core would not know which one to use then.

> 
> Tero?
> 
> Benoit
> 
>>
>> Or do all the TI clocks we have have a dependency to the
>> node naming?

This is a feature of clock core. Clock parents need to have distinct 
names, otherwise it won't work.

-Tero
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki

Re: [Patch 1/3] ARM: dts: am43xx: add support for clkout1 clock

[Tony Lindgren]

* Tero Kristo <t-kristo@ti.com> [191031 14:07]:
> On 30/10/2019 21:59, Benoit Parrot wrote:
> > Tony Lindgren <tony@atomide.com> wrote on Wed [2019-Oct-23 08:56:57 -0700]:
> > > Hmm maybe in this case just doing this is enough:
> > > 
> > > clkout1_osc_div_ck: clock@4100 {
> > > 	...
> > > }
> > 
> > But then we would end up with 6 clock node with the same name "clock@4100",
> > doesn't pose a problem somewhere?
> 
> Yeah, clk core would not know which one to use then.

So we have two issues with the non-standard devicetree use
for omap clocks:

1. Multiple clock nodes with the same reg property

This we sorted out for the clkctrl binding, but we still
have other clocks with this issue.

I guess some of them can be now removed if they are just
legacy clkctrl clocks.

For the rest we could maybe group them under a node with
just one reg property?

The clocks could just look up the reg property from parent
in this case.

2. Dependency to using custom clock node names

This can be sorted out by adding custom compatibles to
the parent node for both clkctrl clock manger nodes and
for the clocks grouped in step #1 above I think.

> > > Or do all the TI clocks we have have a dependency to the
> > > node naming?
> 
> This is a feature of clock core. Clock parents need to have distinct names,
> otherwise it won't work.

With steps #1 and #2 above I think this issue would
also disappear, the clock framework naming could be like
what we already use for the clkctrl clocks.

Regards,

Tony