| Message ID | 1509479663-8985-2-git-send-email-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <manivannan.sadhasivam@linaro.org> > --- > 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 <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/clock/actions,s900-clock.h> > > / { > 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 = <24000000>; > + #clock-cells = <0>; > + }; These 2 are inputs to the clock ctrlr? The clock controller should have a clocks property then. > + > + clock: clock-controller@e0160000 { > + compatible = "actions,s900-clock"; > + reg = <0 0xe0160000 0 0x1000>; > + #clock-cells = <1>; > + }; > + > soc { > compatible = "simple-bus"; > #address-cells = <2>; > -- > 2.7.4 >
Hi Rob, On 2 November 2017 at 05:36, Rob Herring <robh@kernel.org> 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 <manivannan.sadhasivam@linaro.org> >> --- >> 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 <dt-bindings/interrupt-controller/arm-gic.h> >> +#include <dt-bindings/clock/actions,s900-clock.h> >> >> / { >> 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 = <24000000>; >> + #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 = <&hosc>, <&losc>; Thanks, Mani >> + >> + clock: clock-controller@e0160000 { >> + compatible = "actions,s900-clock"; >> + reg = <0 0xe0160000 0 0x1000>; >> + #clock-cells = <1>; >> + }; >> + >> soc { >> compatible = "simple-bus"; >> #address-cells = <2>; >> -- >> 2.7.4 >>
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 <manivannan.sadhasivam@linaro.org> > --- > 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 <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/clock/actions,s900-clock.h> 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 = <24000000>; > + #clock-cells = <0>; > + }; > + > + clock: clock-controller@e0160000 { > + compatible = "actions,s900-clock"; > + reg = <0 0xe0160000 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
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 <manivannan.sadhasivam@linaro.org> > > --- > > 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 <dt-bindings/interrupt-controller/arm-gic.h> > > +#include <dt-bindings/clock/actions,s900-clock.h> > > > > / { > > 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 = <24000000>; > > + #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 = <&hosc>, <&losc>; Thanks, Mani > > + > > + clock: clock-controller@e0160000 { > > + compatible = "actions,s900-clock"; > > + reg = <0 0xe0160000 0 0x1000>; > > + #clock-cells = <1>; > > + }; > > + > > soc { > > compatible = "simple-bus"; > > #address-cells = <2>; > > -- > > 2.7.4 > >
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 <manivannan.sadhasivam@linaro.org> > > --- > > 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 <dt-bindings/interrupt-controller/arm-gic.h> > > +#include <dt-bindings/clock/actions,s900-clock.h> > > 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 = <24000000>; > > + #clock-cells = <0>; > > + }; > > + > > + clock: clock-controller@e0160000 { > > + compatible = "actions,s900-clock"; > > + reg = <0 0xe0160000 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)
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 <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/clock/actions,s900-clock.h> / { 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 = <24000000>; + #clock-cells = <0>; + }; + + clock: clock-controller@e0160000 { + compatible = "actions,s900-clock"; + reg = <0 0xe0160000 0 0x1000>; + #clock-cells = <1>; + }; + soc { compatible = "simple-bus"; #address-cells = <2>;
This patch adds clock controller nodes for Actions Semi s900 SoC Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- arch/arm64/boot/dts/actions/s900.dtsi | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)