| Message ID | 20180922181709.13007-7-gregory.clement@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add CPU clock support for Armada 7K/8K | expand |
+Rob Quoting Gregory CLEMENT (2018-09-22 11:17:09) > diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi > index 4a65e4e830aa..27c840e91abe 100644 > --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi > +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi > @@ -280,6 +280,12 @@ > #address-cells = <1>; > #size-cells = <1>; > > + cpu_clk: clock-cpu { > + compatible = "marvell,ap806-cpu-clock"; > + clocks = <&ap_clk 0>, <&ap_clk 1>; > + #clock-cells = <1>; > + }; This looks like the wrong place because there isn't a reg property. It should go to the root of the tree. And then it looks like we're adding something to DT to get a driver to probe, which is improper DT design.
Hi Stephen, On ven., oct. 12 2018, Stephen Boyd <sboyd@kernel.org> wrote: > +Rob > > Quoting Gregory CLEMENT (2018-09-22 11:17:09) >> diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi >> index 4a65e4e830aa..27c840e91abe 100644 >> --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi >> +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi >> @@ -280,6 +280,12 @@ >> #address-cells = <1>; >> #size-cells = <1>; >> >> + cpu_clk: clock-cpu { >> + compatible = "marvell,ap806-cpu-clock"; >> + clocks = <&ap_clk 0>, <&ap_clk 1>; >> + #clock-cells = <1>; >> + }; > > This looks like the wrong place because there isn't a reg property. It There is no reg property because we are inside a syscon node where the registers are shared between multiple IPs. > should go to the root of the tree. And then it looks like we're adding > something to DT to get a driver to probe, which is improper DT design. There is nothing related to the driver, this subnode describes the way the hardware is designed. Under the system controller node there are several IPs , like the CPU clocks, but also the GPIO or the pinctrl. Gregory >
Quoting Gregory CLEMENT (2018-11-23 07:02:56) > Hi Stephen, > > On ven., oct. 12 2018, Stephen Boyd <sboyd@kernel.org> wrote: > > > +Rob > > > > Quoting Gregory CLEMENT (2018-09-22 11:17:09) > >> diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi > >> index 4a65e4e830aa..27c840e91abe 100644 > >> --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi > >> +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi > >> @@ -280,6 +280,12 @@ > >> #address-cells = <1>; > >> #size-cells = <1>; > >> > >> + cpu_clk: clock-cpu { > >> + compatible = "marvell,ap806-cpu-clock"; > >> + clocks = <&ap_clk 0>, <&ap_clk 1>; > >> + #clock-cells = <1>; > >> + }; > > > > This looks like the wrong place because there isn't a reg property. It > > There is no reg property because we are inside a syscon node where the > registers are shared between multiple IPs. The node right after this node has a reg property. What's going on? > > > should go to the root of the tree. And then it looks like we're adding > > something to DT to get a driver to probe, which is improper DT design. > > There is nothing related to the driver, this subnode describes the way > the hardware is designed. Under the system controller node there are > several IPs , like the CPU clocks, but also the GPIO or the pinctrl. > Ok. And some of them have reg properties and others don't? We should somehow deprecate the idea of a syscon with child nodes. It leads to this weird twisting of DT. The only use for syscon from my perspective is when we have a register region that other random devices with their own 'reg' property need to peek/poke random things into.
diff --git a/arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi index 64632c873888..f2fd777d1944 100644 --- a/arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi +++ b/arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi @@ -21,6 +21,7 @@ reg = <0x000>; enable-method = "psci"; cpu-idle-states = <&CPU_SLEEP_0>; + clocks = <&cpu_clk 0>; }; cpu1: cpu@1 { device_type = "cpu"; @@ -28,6 +29,7 @@ reg = <0x001>; enable-method = "psci"; cpu-idle-states = <&CPU_SLEEP_0>; + clocks = <&cpu_clk 0>; }; cpu2: cpu@100 { device_type = "cpu"; @@ -35,6 +37,7 @@ reg = <0x100>; enable-method = "psci"; cpu-idle-states = <&CPU_SLEEP_0>; + clocks = <&cpu_clk 1>; }; cpu3: cpu@101 { device_type = "cpu"; @@ -42,6 +45,7 @@ reg = <0x101>; enable-method = "psci"; cpu-idle-states = <&CPU_SLEEP_0>; + clocks = <&cpu_clk 1>; }; }; }; diff --git a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi index 4a65e4e830aa..27c840e91abe 100644 --- a/arch/arm64/boot/dts/marvell/armada-ap806.dtsi +++ b/arch/arm64/boot/dts/marvell/armada-ap806.dtsi @@ -280,6 +280,12 @@ #address-cells = <1>; #size-cells = <1>; + cpu_clk: clock-cpu { + compatible = "marvell,ap806-cpu-clock"; + clocks = <&ap_clk 0>, <&ap_clk 1>; + #clock-cells = <1>; + }; + ap_thermal: thermal-sensor@80 { compatible = "marvell,armada-ap806-thermal"; reg = <0x80 0x10>;
Add cpu clock node on AP Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com> --- arch/arm64/boot/dts/marvell/armada-ap806-quad.dtsi | 4 ++++ arch/arm64/boot/dts/marvell/armada-ap806.dtsi | 6 ++++++ 2 files changed, 10 insertions(+)