Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
Hi Andreas, On Sat, Nov 04, 2017 at 04:43:31PM +0800, Andreas Färber wrote: > Am 01.11.2017 um 03:54 schrieb Manivannan Sadhasivam: > > This patch adds clock controller nodes for Actions Semi s900 SoC > > Please avoid "This patch". Once applied, it's a commit. :) > Ack. > > > > Signed-off-by: Manivannan Sadhasivam> > --- > > arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/actions/s900.dtsi > > b/arch/arm64/boot/dts/actions/s900.dtsi > > index 11406f6..c099ef1 100644 > > --- a/arch/arm64/boot/dts/actions/s900.dtsi > > +++ b/arch/arm64/boot/dts/actions/s900.dtsi > > @@ -5,6 +5,7 @@ > > */ > > > > #include > > +#include > > Alphabetical order please. > Ack. > > > > / { > > compatible = "actions,s900"; > > @@ -88,6 +89,24 @@ > > #clock-cells = <0>; > > }; > > > > + losc: losc { > > + compatible = "fixed-clock"; > > + clock-frequency = <32768>; > > + #clock-cells = <0>; > > + }; > > + > > + diff_24M: diff_24M { > > No underscore in node names please - maybe diff-24M or simpler diff24M? > Please double-check that name - is it from manual or from schematics? > Actually, this name comes from vendor driver and they have used "24M_diff". As per DT conventions I renamed it to "diff_24M". But I'm not clear about that clock. As I said in my reply to Rob's review there isn't much documentation also. So, I guess we can name it as diff24M for now. > > + compatible = "fixed-clock"; > > + clock-frequency = <2400>; > > + #clock-cells = <0>; > > + }; > > + > > + clock: clock-controller@e016 { > > + compatible = "actions,s900-clock"; > > + reg = <0 0xe016 0 0x1000>; > > + #clock-cells = <1>; > > + }; > > This node belongs into the soc node below, ordered by unit address. > And maybe there's a better label than "clock"? "clkc" or some official > name from the manual? > Will use "cmu". Thanks, Mani > > + > > soc { > > compatible = "simple-bus"; > > #address-cells = <2>; > > Regards, > Andreas > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg)
Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
Hi Andreas, On Sat, Nov 04, 2017 at 04:43:31PM +0800, Andreas Färber wrote: > Am 01.11.2017 um 03:54 schrieb Manivannan Sadhasivam: > > This patch adds clock controller nodes for Actions Semi s900 SoC > > Please avoid "This patch". Once applied, it's a commit. :) > Ack. > > > > Signed-off-by: Manivannan Sadhasivam > > --- > > arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/actions/s900.dtsi > > b/arch/arm64/boot/dts/actions/s900.dtsi > > index 11406f6..c099ef1 100644 > > --- a/arch/arm64/boot/dts/actions/s900.dtsi > > +++ b/arch/arm64/boot/dts/actions/s900.dtsi > > @@ -5,6 +5,7 @@ > > */ > > > > #include > > +#include > > Alphabetical order please. > Ack. > > > > / { > > compatible = "actions,s900"; > > @@ -88,6 +89,24 @@ > > #clock-cells = <0>; > > }; > > > > + losc: losc { > > + compatible = "fixed-clock"; > > + clock-frequency = <32768>; > > + #clock-cells = <0>; > > + }; > > + > > + diff_24M: diff_24M { > > No underscore in node names please - maybe diff-24M or simpler diff24M? > Please double-check that name - is it from manual or from schematics? > Actually, this name comes from vendor driver and they have used "24M_diff". As per DT conventions I renamed it to "diff_24M". But I'm not clear about that clock. As I said in my reply to Rob's review there isn't much documentation also. So, I guess we can name it as diff24M for now. > > + compatible = "fixed-clock"; > > + clock-frequency = <2400>; > > + #clock-cells = <0>; > > + }; > > + > > + clock: clock-controller@e016 { > > + compatible = "actions,s900-clock"; > > + reg = <0 0xe016 0 0x1000>; > > + #clock-cells = <1>; > > + }; > > This node belongs into the soc node below, ordered by unit address. > And maybe there's a better label than "clock"? "clkc" or some official > name from the manual? > Will use "cmu". Thanks, Mani > > + > > soc { > > compatible = "simple-bus"; > > #address-cells = <2>; > > Regards, > Andreas > > -- > SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Felix Imendörffer, Jane Smithard, Graham Norton > HRB 21284 (AG Nürnberg)
Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
Hi Rob, On Wed, Nov 01, 2017 at 07:06:16PM -0500, Rob Herring wrote: > On Wed, Nov 01, 2017 at 01:24:21AM +0530, Manivannan Sadhasivam wrote: > > This patch adds clock controller nodes for Actions Semi s900 SoC > > > > Signed-off-by: Manivannan Sadhasivam> > --- > > arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/actions/s900.dtsi > > b/arch/arm64/boot/dts/actions/s900.dtsi > > index 11406f6..c099ef1 100644 > > --- a/arch/arm64/boot/dts/actions/s900.dtsi > > +++ b/arch/arm64/boot/dts/actions/s900.dtsi > > @@ -5,6 +5,7 @@ > > */ > > > > #include > > +#include > > > > / { > > compatible = "actions,s900"; > > @@ -88,6 +89,24 @@ > > #clock-cells = <0>; > > }; > > > > + losc: losc { > > + compatible = "fixed-clock"; > > + clock-frequency = <32768>; > > + #clock-cells = <0>; > > + }; > > + > > + diff_24M: diff_24M { > > + compatible = "fixed-clock"; > > + clock-frequency = <2400>; > > + #clock-cells = <0>; > > + }; > > These 2 are inputs to the clock ctrlr? The clock controller should have > a clocks property then. > Actually losc and hosc (already defined) are the sources to the clock controller. There isn't much documentation about diff_24M clock which seems to be only sourcing eDP. Since, the vendor driver defined these 3 clocks (losc, hosc, diff_24M) as fixed rate clocks I have added them in the device tree as "fixed-clock" nodes. So, as per your suggestion I will add hosc and losc as clock providers for the clock controller like below in next revision: clocks = <>, <>; Thanks, Mani > > + > > + clock: clock-controller@e016 { > > + compatible = "actions,s900-clock"; > > + reg = <0 0xe016 0 0x1000>; > > + #clock-cells = <1>; > > + }; > > + > > soc { > > compatible = "simple-bus"; > > #address-cells = <2>; > > -- > > 2.7.4 > >
Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
Hi Rob, On Wed, Nov 01, 2017 at 07:06:16PM -0500, Rob Herring wrote: > On Wed, Nov 01, 2017 at 01:24:21AM +0530, Manivannan Sadhasivam wrote: > > This patch adds clock controller nodes for Actions Semi s900 SoC > > > > Signed-off-by: Manivannan Sadhasivam > > --- > > arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/actions/s900.dtsi > > b/arch/arm64/boot/dts/actions/s900.dtsi > > index 11406f6..c099ef1 100644 > > --- a/arch/arm64/boot/dts/actions/s900.dtsi > > +++ b/arch/arm64/boot/dts/actions/s900.dtsi > > @@ -5,6 +5,7 @@ > > */ > > > > #include > > +#include > > > > / { > > compatible = "actions,s900"; > > @@ -88,6 +89,24 @@ > > #clock-cells = <0>; > > }; > > > > + losc: losc { > > + compatible = "fixed-clock"; > > + clock-frequency = <32768>; > > + #clock-cells = <0>; > > + }; > > + > > + diff_24M: diff_24M { > > + compatible = "fixed-clock"; > > + clock-frequency = <2400>; > > + #clock-cells = <0>; > > + }; > > These 2 are inputs to the clock ctrlr? The clock controller should have > a clocks property then. > Actually losc and hosc (already defined) are the sources to the clock controller. There isn't much documentation about diff_24M clock which seems to be only sourcing eDP. Since, the vendor driver defined these 3 clocks (losc, hosc, diff_24M) as fixed rate clocks I have added them in the device tree as "fixed-clock" nodes. So, as per your suggestion I will add hosc and losc as clock providers for the clock controller like below in next revision: clocks = <>, <>; Thanks, Mani > > + > > + clock: clock-controller@e016 { > > + compatible = "actions,s900-clock"; > > + reg = <0 0xe016 0 0x1000>; > > + #clock-cells = <1>; > > + }; > > + > > soc { > > compatible = "simple-bus"; > > #address-cells = <2>; > > -- > > 2.7.4 > >
Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
Am 01.11.2017 um 03:54 schrieb Manivannan Sadhasivam: > This patch adds clock controller nodes for Actions Semi s900 SoC Please avoid "This patch". Once applied, it's a commit. :) > > Signed-off-by: Manivannan Sadhasivam> --- > arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ > 1 file changed, 19 insertions(+) > > diff --git a/arch/arm64/boot/dts/actions/s900.dtsi > b/arch/arm64/boot/dts/actions/s900.dtsi > index 11406f6..c099ef1 100644 > --- a/arch/arm64/boot/dts/actions/s900.dtsi > +++ b/arch/arm64/boot/dts/actions/s900.dtsi > @@ -5,6 +5,7 @@ > */ > > #include > +#include Alphabetical order please. > > / { > compatible = "actions,s900"; > @@ -88,6 +89,24 @@ > #clock-cells = <0>; > }; > > + losc: losc { > + compatible = "fixed-clock"; > + clock-frequency = <32768>; > + #clock-cells = <0>; > + }; > + > + diff_24M: diff_24M { No underscore in node names please - maybe diff-24M or simpler diff24M? Please double-check that name - is it from manual or from schematics? > + compatible = "fixed-clock"; > + clock-frequency = <2400>; > + #clock-cells = <0>; > + }; > + > + clock: clock-controller@e016 { > + compatible = "actions,s900-clock"; > + reg = <0 0xe016 0 0x1000>; > + #clock-cells = <1>; > + }; This node belongs into the soc node below, ordered by unit address. And maybe there's a better label than "clock"? "clkc" or some official name from the manual? > + > soc { > compatible = "simple-bus"; > #address-cells = <2>; Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
Am 01.11.2017 um 03:54 schrieb Manivannan Sadhasivam: > This patch adds clock controller nodes for Actions Semi s900 SoC Please avoid "This patch". Once applied, it's a commit. :) > > Signed-off-by: Manivannan Sadhasivam > --- > arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ > 1 file changed, 19 insertions(+) > > diff --git a/arch/arm64/boot/dts/actions/s900.dtsi > b/arch/arm64/boot/dts/actions/s900.dtsi > index 11406f6..c099ef1 100644 > --- a/arch/arm64/boot/dts/actions/s900.dtsi > +++ b/arch/arm64/boot/dts/actions/s900.dtsi > @@ -5,6 +5,7 @@ > */ > > #include > +#include Alphabetical order please. > > / { > compatible = "actions,s900"; > @@ -88,6 +89,24 @@ > #clock-cells = <0>; > }; > > + losc: losc { > + compatible = "fixed-clock"; > + clock-frequency = <32768>; > + #clock-cells = <0>; > + }; > + > + diff_24M: diff_24M { No underscore in node names please - maybe diff-24M or simpler diff24M? Please double-check that name - is it from manual or from schematics? > + compatible = "fixed-clock"; > + clock-frequency = <2400>; > + #clock-cells = <0>; > + }; > + > + clock: clock-controller@e016 { > + compatible = "actions,s900-clock"; > + reg = <0 0xe016 0 0x1000>; > + #clock-cells = <1>; > + }; This node belongs into the soc node below, ordered by unit address. And maybe there's a better label than "clock"? "clkc" or some official name from the manual? > + > soc { > compatible = "simple-bus"; > #address-cells = <2>; Regards, Andreas -- SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
Hi Rob, On 2 November 2017 at 05:36, Rob Herringwrote: > On Wed, Nov 01, 2017 at 01:24:21AM +0530, Manivannan Sadhasivam wrote: >> This patch adds clock controller nodes for Actions Semi s900 SoC >> >> Signed-off-by: Manivannan Sadhasivam >> --- >> arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/actions/s900.dtsi >> b/arch/arm64/boot/dts/actions/s900.dtsi >> index 11406f6..c099ef1 100644 >> --- a/arch/arm64/boot/dts/actions/s900.dtsi >> +++ b/arch/arm64/boot/dts/actions/s900.dtsi >> @@ -5,6 +5,7 @@ >> */ >> >> #include >> +#include >> >> / { >> compatible = "actions,s900"; >> @@ -88,6 +89,24 @@ >> #clock-cells = <0>; >> }; >> >> + losc: losc { >> + compatible = "fixed-clock"; >> + clock-frequency = <32768>; >> + #clock-cells = <0>; >> + }; >> + >> + diff_24M: diff_24M { >> + compatible = "fixed-clock"; >> + clock-frequency = <2400>; >> + #clock-cells = <0>; >> + }; > > These 2 are inputs to the clock ctrlr? The clock controller should have > a clocks property then. > Actually losc and hosc (already defined) are the sources to the clock controller. There isn't much documentation about diff_24M clock which seems to be only sourcing eDP. Since, the vendor driver defined these 3 clocks (losc, hosc, diff_24M) as fixed rate clocks I have added them in the device tree as "fixed-clock" nodes. So, as per your suggestion I will add hosc and losc as clock providers for the clock controller like below in next revision: clocks = <>, <>; Thanks, Mani >> + >> + clock: clock-controller@e016 { >> + compatible = "actions,s900-clock"; >> + reg = <0 0xe016 0 0x1000>; >> + #clock-cells = <1>; >> + }; >> + >> soc { >> compatible = "simple-bus"; >> #address-cells = <2>; >> -- >> 2.7.4 >>
Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
Hi Rob, On 2 November 2017 at 05:36, Rob Herring wrote: > On Wed, Nov 01, 2017 at 01:24:21AM +0530, Manivannan Sadhasivam wrote: >> This patch adds clock controller nodes for Actions Semi s900 SoC >> >> Signed-off-by: Manivannan Sadhasivam >> --- >> arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ >> 1 file changed, 19 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/actions/s900.dtsi >> b/arch/arm64/boot/dts/actions/s900.dtsi >> index 11406f6..c099ef1 100644 >> --- a/arch/arm64/boot/dts/actions/s900.dtsi >> +++ b/arch/arm64/boot/dts/actions/s900.dtsi >> @@ -5,6 +5,7 @@ >> */ >> >> #include >> +#include >> >> / { >> compatible = "actions,s900"; >> @@ -88,6 +89,24 @@ >> #clock-cells = <0>; >> }; >> >> + losc: losc { >> + compatible = "fixed-clock"; >> + clock-frequency = <32768>; >> + #clock-cells = <0>; >> + }; >> + >> + diff_24M: diff_24M { >> + compatible = "fixed-clock"; >> + clock-frequency = <2400>; >> + #clock-cells = <0>; >> + }; > > These 2 are inputs to the clock ctrlr? The clock controller should have > a clocks property then. > Actually losc and hosc (already defined) are the sources to the clock controller. There isn't much documentation about diff_24M clock which seems to be only sourcing eDP. Since, the vendor driver defined these 3 clocks (losc, hosc, diff_24M) as fixed rate clocks I have added them in the device tree as "fixed-clock" nodes. So, as per your suggestion I will add hosc and losc as clock providers for the clock controller like below in next revision: clocks = <>, <>; Thanks, Mani >> + >> + clock: clock-controller@e016 { >> + compatible = "actions,s900-clock"; >> + reg = <0 0xe016 0 0x1000>; >> + #clock-cells = <1>; >> + }; >> + >> soc { >> compatible = "simple-bus"; >> #address-cells = <2>; >> -- >> 2.7.4 >>
Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
On Wed, Nov 01, 2017 at 01:24:21AM +0530, Manivannan Sadhasivam wrote: > This patch adds clock controller nodes for Actions Semi s900 SoC > > Signed-off-by: Manivannan Sadhasivam> --- > arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ > 1 file changed, 19 insertions(+) > > diff --git a/arch/arm64/boot/dts/actions/s900.dtsi > b/arch/arm64/boot/dts/actions/s900.dtsi > index 11406f6..c099ef1 100644 > --- a/arch/arm64/boot/dts/actions/s900.dtsi > +++ b/arch/arm64/boot/dts/actions/s900.dtsi > @@ -5,6 +5,7 @@ > */ > > #include > +#include > > / { > compatible = "actions,s900"; > @@ -88,6 +89,24 @@ > #clock-cells = <0>; > }; > > + losc: losc { > + compatible = "fixed-clock"; > + clock-frequency = <32768>; > + #clock-cells = <0>; > + }; > + > + diff_24M: diff_24M { > + compatible = "fixed-clock"; > + clock-frequency = <2400>; > + #clock-cells = <0>; > + }; These 2 are inputs to the clock ctrlr? The clock controller should have a clocks property then. > + > + clock: clock-controller@e016 { > + compatible = "actions,s900-clock"; > + reg = <0 0xe016 0 0x1000>; > + #clock-cells = <1>; > + }; > + > soc { > compatible = "simple-bus"; > #address-cells = <2>; > -- > 2.7.4 >
Re: [PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
On Wed, Nov 01, 2017 at 01:24:21AM +0530, Manivannan Sadhasivam wrote: > This patch adds clock controller nodes for Actions Semi s900 SoC > > Signed-off-by: Manivannan Sadhasivam > --- > arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ > 1 file changed, 19 insertions(+) > > diff --git a/arch/arm64/boot/dts/actions/s900.dtsi > b/arch/arm64/boot/dts/actions/s900.dtsi > index 11406f6..c099ef1 100644 > --- a/arch/arm64/boot/dts/actions/s900.dtsi > +++ b/arch/arm64/boot/dts/actions/s900.dtsi > @@ -5,6 +5,7 @@ > */ > > #include > +#include > > / { > compatible = "actions,s900"; > @@ -88,6 +89,24 @@ > #clock-cells = <0>; > }; > > + losc: losc { > + compatible = "fixed-clock"; > + clock-frequency = <32768>; > + #clock-cells = <0>; > + }; > + > + diff_24M: diff_24M { > + compatible = "fixed-clock"; > + clock-frequency = <2400>; > + #clock-cells = <0>; > + }; These 2 are inputs to the clock ctrlr? The clock controller should have a clocks property then. > + > + clock: clock-controller@e016 { > + compatible = "actions,s900-clock"; > + reg = <0 0xe016 0 0x1000>; > + #clock-cells = <1>; > + }; > + > soc { > compatible = "simple-bus"; > #address-cells = <2>; > -- > 2.7.4 >
[PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
This patch adds clock controller nodes for Actions Semi s900 SoC Signed-off-by: Manivannan Sadhasivam--- arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ 1 file changed, 19 insertions(+) diff --git a/arch/arm64/boot/dts/actions/s900.dtsi b/arch/arm64/boot/dts/actions/s900.dtsi index 11406f6..c099ef1 100644 --- a/arch/arm64/boot/dts/actions/s900.dtsi +++ b/arch/arm64/boot/dts/actions/s900.dtsi @@ -5,6 +5,7 @@ */ #include +#include / { compatible = "actions,s900"; @@ -88,6 +89,24 @@ #clock-cells = <0>; }; + losc: losc { + compatible = "fixed-clock"; + clock-frequency = <32768>; + #clock-cells = <0>; + }; + + diff_24M: diff_24M { + compatible = "fixed-clock"; + clock-frequency = <2400>; + #clock-cells = <0>; + }; + + clock: clock-controller@e016 { + compatible = "actions,s900-clock"; + reg = <0 0xe016 0 0x1000>; + #clock-cells = <1>; + }; + soc { compatible = "simple-bus"; #address-cells = <2>; -- 2.7.4
[PATCH 1/3] arm64: dts: actions: add s900 clock controller nodes
This patch adds clock controller nodes for Actions Semi s900 SoC Signed-off-by: Manivannan Sadhasivam --- arch/arm64/boot/dts/actions/s900.dtsi | 19 +++ 1 file changed, 19 insertions(+) diff --git a/arch/arm64/boot/dts/actions/s900.dtsi b/arch/arm64/boot/dts/actions/s900.dtsi index 11406f6..c099ef1 100644 --- a/arch/arm64/boot/dts/actions/s900.dtsi +++ b/arch/arm64/boot/dts/actions/s900.dtsi @@ -5,6 +5,7 @@ */ #include +#include / { compatible = "actions,s900"; @@ -88,6 +89,24 @@ #clock-cells = <0>; }; + losc: losc { + compatible = "fixed-clock"; + clock-frequency = <32768>; + #clock-cells = <0>; + }; + + diff_24M: diff_24M { + compatible = "fixed-clock"; + clock-frequency = <2400>; + #clock-cells = <0>; + }; + + clock: clock-controller@e016 { + compatible = "actions,s900-clock"; + reg = <0 0xe016 0 0x1000>; + #clock-cells = <1>; + }; + soc { compatible = "simple-bus"; #address-cells = <2>; -- 2.7.4