[PATCH 0/4] ARM64: dts: meson: hhi syscon and clock controllers

[PATCH 0/4] ARM64: dts: meson: hhi syscon and clock controllers

[Jerome Brunet]

The HHI register region provides more than just clocks. Several drivers may
need to access this region, such as the clock controllers and the display
driver.

Meson-gx clock controllers has been developed and merged before we knew the
hhi could be needed elsewhere, which is why it requests the region directly.
Meson-axg was built following the model of meson-gx.

To fix this situation, the clock controller should be a child of the hhi
system controllers. The hhi system controller node already exists on meson-gx.
It needs to be added on the axg.

This changeset depends on [0]. This dependency has been merged in
clk-next. A topic branch is available here [1]

[0]: https://lkml.kernel.org/r/20180212145846.19380-1-jbrunet@baylibre.com
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/log/?h=clk-amlogic

Jerome Brunet (4):
  dt-bindings: clock: meson: update documentation with hhi syscon
  ARM64: dts: meson-gx: sysctrl is the parent of the clock controller
  ARM64: dts: meson-axg: use hhi syscon for the clock controller
  ARM64: dts: meson-gx: fix gxl clock controller compatible

 .../devicetree/bindings/clock/amlogic,gxbb-clkc.txt      | 16 +++++++++++-----
 arch/arm64/boot/dts/amlogic/meson-axg.dtsi               | 12 ++++++++----
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi              |  5 ++---
 arch/arm64/boot/dts/amlogic/meson-gxl.dtsi               |  7 +++----
 4 files changed, 24 insertions(+), 16 deletions(-)

-- 
2.14.3

[PATCH 1/4] dt-bindings: clock: meson: update documentation with hhi syscon

[Jerome Brunet]

The HHI register region hosts more than just clocks and needs to
accessed drivers other than the clock controller, such as the display
driver.

This register region should be managed by syscon. It is already the case
on gxbb/gxl and it soon will be on axg. The clock controllers must use
this system controller instead of directly mapping the registers.

This changes the bindings of gxbb and axg's clock controllers. This is
due to an initial 'incomplete' knowledge of these SoCs, which is why the
meson bindings are unstable ATM.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 .../devicetree/bindings/clock/amlogic,gxbb-clkc.txt      | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
index e2b377ed6f91..e950599566a9 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
+++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
@@ -10,9 +10,6 @@ Required Properties:
 		"amlogic,gxl-clkc" for GXL and GXM SoC,
 		"amlogic,axg-clkc" for AXG SoC.
 
-- reg: physical base address of the clock controller and length of memory
-       mapped region.
-
 - #clock-cells: should be 1.
 
 Each clock is assigned an identifier and client nodes can use this identifier
@@ -20,13 +17,22 @@ to specify the clock which they consume. All available clocks are defined as
 preprocessor macros in the dt-bindings/clock/gxbb-clkc.h header and can be
 used in device tree sources.
 
+Parent node should have the following properties :
+- compatible: "syscon", "simple-mfd, and "amlogic,meson-gx-hhi-sysctrl" or
+              "amlogic,meson-axg-hhi-sysctrl"
+- reg: base address and size of the HHI system control register space.
+
 Example: Clock controller node:
 
-	clkc: clock-controller@c883c000 {
+sysctrl: system-controller@0 {
+	compatible = "amlogic,meson-gx-hhi-sysctrl", "syscon", "simple-mfd";
+	reg = <0 0 0 0x400>;
+
+	clkc: clock-controller {
 		#clock-cells = <1>;
 		compatible = "amlogic,gxbb-clkc";
-		reg = <0x0 0xc883c000 0x0 0x3db>;
 	};
+};
 
 Example: UART controller node that consumes the clock generated by the clock
   controller:
-- 
2.14.3

Re: [PATCH 1/4] dt-bindings: clock: meson: update documentation with hhi syscon

[Rob Herring]

On Thu, Mar 15, 2018 at 12:55:42PM +0100, Jerome Brunet wrote:
> The HHI register region hosts more than just clocks and needs to
> accessed drivers other than the clock controller, such as the display
> driver.
> 
> This register region should be managed by syscon. It is already the case
> on gxbb/gxl and it soon will be on axg. The clock controllers must use
> this system controller instead of directly mapping the registers.

Sounds like a kernel problem, not a DT one.

> 
> This changes the bindings of gxbb and axg's clock controllers. This is
> due to an initial 'incomplete' knowledge of these SoCs, which is why the
> meson bindings are unstable ATM.
> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  .../devicetree/bindings/clock/amlogic,gxbb-clkc.txt      | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
> index e2b377ed6f91..e950599566a9 100644
> --- a/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
> +++ b/Documentation/devicetree/bindings/clock/amlogic,gxbb-clkc.txt
> @@ -10,9 +10,6 @@ Required Properties:
>  		"amlogic,gxl-clkc" for GXL and GXM SoC,
>  		"amlogic,axg-clkc" for AXG SoC.
>  
> -- reg: physical base address of the clock controller and length of memory
> -       mapped region.
> -
>  - #clock-cells: should be 1.
>  
>  Each clock is assigned an identifier and client nodes can use this identifier
> @@ -20,13 +17,22 @@ to specify the clock which they consume. All available clocks are defined as
>  preprocessor macros in the dt-bindings/clock/gxbb-clkc.h header and can be
>  used in device tree sources.
>  
> +Parent node should have the following properties :
> +- compatible: "syscon", "simple-mfd, and "amlogic,meson-gx-hhi-sysctrl" or
> +              "amlogic,meson-axg-hhi-sysctrl"
> +- reg: base address and size of the HHI system control register space.
> +
>  Example: Clock controller node:
>  
> -	clkc: clock-controller@c883c000 {
> +sysctrl: system-controller@0 {
> +	compatible = "amlogic,meson-gx-hhi-sysctrl", "syscon", "simple-mfd";
> +	reg = <0 0 0 0x400>;
> +
> +	clkc: clock-controller {

With a single child, there is really no point to this change. A single 
node can provide multiple functions. Look at nodes that are both reset 
and clock providers.

What other functions are there?

Rob

Re: [PATCH 1/4] dt-bindings: clock: meson: update documentation with hhi syscon

[Jerome Brunet]

On Sun, 2018-03-18 at 07:52 -0500, Rob Herring wrote:
> On Thu, Mar 15, 2018 at 12:55:42PM +0100, Jerome Brunet wrote:
> > The HHI register region hosts more than just clocks and needs to
> > accessed drivers other than the clock controller, such as the display
> > driver.
> > 
> > This register region should be managed by syscon. It is already the case
> > on gxbb/gxl and it soon will be on axg. The clock controllers must use
> > this system controller instead of directly mapping the registers.
> 
> Sounds like a kernel problem, not a DT one.

It's a platform problem, so it has much a kernel problem (solution already
merged) as a DT problem

DT wise, we've got two devices mapping the same region

in arch/arm64/boot/dts/amlogic/meson-gx.dtsi:
> system-controller@0 

which is used by the display device.

and in arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi (same for gxl)
> clock-controller@0

It has worked so far because the clock controller claims the region w/o
reserving it but it remains unsafe since both device may access the same region.
The fix is to have the clock controller go through the existing syscon.

> 
> > 
> 
> With a single child, there is really no point to this change. A single 
> node can provide multiple functions. Look at nodes that are both reset 
> and clock providers.

There is more than a single user, as explained above and in the cover letter of
this series.

> 
> What other functions are there?
> 
> Rob

Re: [PATCH 1/4] dt-bindings: clock: meson: update documentation with hhi syscon

[Rob Herring]

On Sun, Mar 18, 2018 at 10:09 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Sun, 2018-03-18 at 07:52 -0500, Rob Herring wrote:
>> On Thu, Mar 15, 2018 at 12:55:42PM +0100, Jerome Brunet wrote:
>> > The HHI register region hosts more than just clocks and needs to
>> > accessed drivers other than the clock controller, such as the display
>> > driver.
>> >
>> > This register region should be managed by syscon. It is already the case
>> > on gxbb/gxl and it soon will be on axg. The clock controllers must use
>> > this system controller instead of directly mapping the registers.
>>
>> Sounds like a kernel problem, not a DT one.
>
> It's a platform problem, so it has much a kernel problem (solution already
> merged) as a DT problem
>
> DT wise, we've got two devices mapping the same region
>
> in arch/arm64/boot/dts/amlogic/meson-gx.dtsi:
>> system-controller@0
>
> which is used by the display device.
>
> and in arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi (same for gxl)
>> clock-controller@0
>
> It has worked so far because the clock controller claims the region w/o
> reserving it but it remains unsafe since both device may access the same region.
> The fix is to have the clock controller go through the existing syscon.

We certainly want to avoid multiple nodes using the same mmio region.


>> With a single child, there is really no point to this change. A single
>> node can provide multiple functions. Look at nodes that are both reset
>> and clock providers.
>
> There is more than a single user, as explained above and in the cover letter of
> this series.

Right, but a single node can support more than a single user (or
provider). You don't have to have multiple nodes to have multiple
users/providers.

Rob

Re: [PATCH 1/4] dt-bindings: clock: meson: update documentation with hhi syscon

[Neil Armstrong]

On 23/03/2018 04:04, Rob Herring wrote:
> On Sun, Mar 18, 2018 at 10:09 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>> On Sun, 2018-03-18 at 07:52 -0500, Rob Herring wrote:
>>> On Thu, Mar 15, 2018 at 12:55:42PM +0100, Jerome Brunet wrote:
>>>> The HHI register region hosts more than just clocks and needs to
>>>> accessed drivers other than the clock controller, such as the display
>>>> driver.
>>>>
>>>> This register region should be managed by syscon. It is already the case
>>>> on gxbb/gxl and it soon will be on axg. The clock controllers must use
>>>> this system controller instead of directly mapping the registers.
>>>
>>> Sounds like a kernel problem, not a DT one.
>>
>> It's a platform problem, so it has much a kernel problem (solution already
>> merged) as a DT problem
>>
>> DT wise, we've got two devices mapping the same region
>>
>> in arch/arm64/boot/dts/amlogic/meson-gx.dtsi:
>>> system-controller@0
>>
>> which is used by the display device.
>>
>> and in arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi (same for gxl)
>>> clock-controller@0
>>
>> It has worked so far because the clock controller claims the region w/o
>> reserving it but it remains unsafe since both device may access the same region.
>> The fix is to have the clock controller go through the existing syscon.
> 
> We certainly want to avoid multiple nodes using the same mmio region.
> 
> 
>>> With a single child, there is really no point to this change. A single
>>> node can provide multiple functions. Look at nodes that are both reset
>>> and clock providers.
>>
>> There is more than a single user, as explained above and in the cover letter of
>> this series.
> 
> Right, but a single node can support more than a single user (or
> provider). You don't have to have multiple nodes to have multiple
> users/providers.
> 
> Rob
> 

Hi Rob,

It's a question about user/provider, the syscon node represents the entire mmio
region which has multiple sparse registers for multiple features, is used
indirectly by the display driver, ... and has multiple features, non limited to,
clock controller, multiple power domains,... and it is simpler to represent these
as subnodes instead of a big fat node.

Today, it is already represented like this within the Documentation/devicetree/bindings/power/amlogic,meson-gx-pwrc.txt
So this binding is only an extension for the clock controller which was mis-architectured
at the time due to a mis-knowledge of the HW architecture.

Neil

[PATCH 2/4] ARM64: dts: meson-gx: sysctrl is the parent of the clock controller

[Jerome Brunet]

The parent of the meson-gx clock controller should be the hhi system
controller, not the HIU bus. This way, the HHI register region can be
used safely by multiple drivers

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 5 ++---
 arch/arm64/boot/dts/amlogic/meson-gxl.dtsi  | 5 ++---
 2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index cac72acb85b1..a48d2b821a49 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -304,11 +304,10 @@
 	clock-names = "isfr", "iahb", "venci";
 };
 
-&hiubus {
-	clkc: clock-controller@0 {
+&sysctrl {
+	clkc: clock-controller {
 		compatible = "amlogic,gxbb-clkc";
 		#clock-cells = <1>;
-		reg = <0x0 0x0 0x0 0x3db>;
 	};
 };
 
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
index e1a39cbed8c9..6b7dfa2d48e4 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
@@ -206,11 +206,10 @@
 	clock-names = "isfr", "iahb", "venci";
 };
 
-&hiubus {
-	clkc: clock-controller@0 {
+&sysctrl {
+	clkc: clock-controller {
 		compatible = "amlogic,gxl-clkc", "amlogic,gxbb-clkc";
 		#clock-cells = <1>;
-		reg = <0x0 0x0 0x0 0x3db>;
 	};
 };
 
-- 
2.14.3

[PATCH 3/4] ARM64: dts: meson-axg: use hhi syscon for the clock controller

[Jerome Brunet]

Like the meson-gx, the axg clock controller should go through a syscon
to access the hhi register region, and not directly map the region.
This way, the hhi register region can be used safely by multiple drivers.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-axg.dtsi | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
index b58808eb3cc8..d4c5507fff30 100644
--- a/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-axg.dtsi
@@ -262,10 +262,14 @@
 			#size-cells = <2>;
 			ranges = <0x0 0x0 0x0 0xff63c000 0x0 0x1c00>;
 
-			clkc: clock-controller@0 {
-				compatible = "amlogic,axg-clkc";
-				#clock-cells = <1>;
-				reg = <0x0 0x0 0x0 0x320>;
+			sysctrl: system-controller@0 {
+				compatible = "amlogic,meson-axg-hhi-sysctrl", "syscon", "simple-mfd";
+				reg = <0 0 0 0x400>;
+
+				clkc: clock-controller {
+					compatible = "amlogic,axg-clkc";
+					#clock-cells = <1>;
+				};
 			};
 		};
 
-- 
2.14.3

[PATCH 4/4] ARM64: dts: meson-gx: fix gxl clock controller compatible

[Jerome Brunet]

There are a few differences between the gxbb and gxl clock controllers
which makes them incompatible. The hdmi, gp0 and fixed pll are
different. The rate of these plls reported by gxbb driver on a gxl
device would be wrong.

Remove the gxbb compatible from the gxl clock controller node so only
the correct driver is matched.

Fixes: 973fbd55b53c ("ARM64: dts: meson-gxl: Add clock nodes")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxl.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
index 6b7dfa2d48e4..86f673017a1f 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
@@ -208,7 +208,7 @@
 
 &sysctrl {
 	clkc: clock-controller {
-		compatible = "amlogic,gxl-clkc", "amlogic,gxbb-clkc";
+		compatible = "amlogic,gxl-clkc";
 		#clock-cells = <1>;
 	};
 };
-- 
2.14.3

Re: [PATCH 0/4] ARM64: dts: meson: hhi syscon and clock controllers

[Kevin Hilman]

Jerome Brunet <jbrunet@baylibre.com> writes:

> The HHI register region provides more than just clocks. Several drivers may
> need to access this region, such as the clock controllers and the display
> driver.
>
> Meson-gx clock controllers has been developed and merged before we knew the
> hhi could be needed elsewhere, which is why it requests the region directly.
> Meson-axg was built following the model of meson-gx.
>
> To fix this situation, the clock controller should be a child of the hhi
> system controllers. The hhi system controller node already exists on meson-gx.
> It needs to be added on the axg.
>
> This changeset depends on [0]. This dependency has been merged in
> clk-next. A topic branch is available here [1]

As the dependencies are now merged for v4.17, applying this to
v4.18/dt64,

Thanks,

Kevin