[PATCH 0/3] Meson8/Meson8b: introduce a HHI syscon node

[PATCH 0/3] Meson8/Meson8b: introduce a HHI syscon node

[Martin Blumenstingl]

The Meson8/Meson8b/Meson8m2 SoCs are suffering from a similar problem
as the GXBB/GXL/GXM SoCs (see the GX series from Jerome: [0]):
There is a register area called "HHI" which is used for multiple IP
blocks of the SoC:
- the system clock controller
- a few reset lines (there is a separate reset controller, these reset
  lines are not part of the other reset controller). this reset
  controller is currently implemented in the clock controller driver
- a HDMI controller
- temperature sensor calibration data (by "data" I really mean data,
  the ADC driver has four bits for the TSC data in it's own register
  space, however on Meson8b and Meson8m2 there is a fifth TSC bit which
  is stored in the HHI register area)

The first three could be implemented with a single node (either in one
big driver, or using a MFD driver which would register function-
specific drivers).
However, the TSC data is a big problem, because the ADC has it's own
set of registers but needs to write one bit in the HHI register area.

NOTE: this series has multiple dependencies:
- the clock controller changes depend "meson8b: add the CPU_DIV16 clock
  for the ARM TWD" as well as "meson8b: register the clock controller
  early" [2]
- the dts changes depend on "fix clock controller register size on
  Meson8/Meson8b" [3]


[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006733.html
[1] http://lists.infradead.org/pipermail/linux-amlogic/2018-July/007890.html
[2] http://lists.infradead.org/pipermail/linux-amlogic/2018-July/007900.html
[3] http://lists.infradead.org/pipermail/linux-amlogic/2018-July/007897.html


Martin Blumenstingl (3):
  dt-bindings: clock: meson8b: use the registers from the HHI syscon
  clk: meson: meson8b: use the HHI syscon if available
  ARM: dts: meson: switch the clock controller to the HHI register area

 .../bindings/clock/amlogic,meson8b-clkc.txt   | 13 ++++------
 arch/arm/boot/dts/meson.dtsi                  |  5 ++++
 arch/arm/boot/dts/meson8.dtsi                 | 15 ++++++------
 arch/arm/boot/dts/meson8b.dtsi                | 15 ++++++------
 drivers/clk/meson/meson8b.c                   | 24 ++++++++++++-------
 5 files changed, 41 insertions(+), 31 deletions(-)

-- 
2.18.0

[PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon

[Martin Blumenstingl]

The clock controller on Meson8/Meson8m2 and Meson8b is part of a
register region called "HHI". This register area contains more
functionality than just a clock controller:
- the clock controller
- some reset controller bits
- temperature sensor calibration data (on Meson8b and Meson8m2 only)
- HDMI controller

The HHI register area may be accessed concurrently. Allow this by using
a "system controller" as parent node.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 .../bindings/clock/amlogic,meson8b-clkc.txt         | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
index b455c5aa9139..38fb979210d3 100644
--- a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
+++ b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
@@ -9,15 +9,13 @@ Required Properties:
 	- "amlogic,meson8-clkc" for Meson8 (S802) SoCs
 	- "amlogic,meson8b-clkc" for Meson8 (S805) SoCs
 	- "amlogic,meson8m2-clkc" for Meson8m2 (S812) SoCs
-- reg: it must be composed by two tuples:
-	0) physical base address of the xtal register and length of memory
-	   mapped region.
-	1) physical base address of the clock controller and length of memory
-	   mapped region.
-
 - #clock-cells: should be 1.
 - #reset-cells: should be 1.
 
+Parent node should have the following properties :
+- compatible: "syscon", "simple-mfd"
+- reg: base address and size of the HHI system control register space.
+
 Each clock is assigned an identifier and client nodes can use this identifier
 to specify the clock which they consume. All available clocks are defined as
 preprocessor macros in the dt-bindings/clock/meson8b-clkc.h header and can be
@@ -30,9 +28,8 @@ device tree sources).
 
 Example: Clock controller node:
 
-	clkc: clock-controller at c1104000 {
+	clkc: clock-controller {
 		compatible = "amlogic,meson8b-clkc";
-		reg = <0xc1108000 0x4>, <0xc1104000 0x460>;
 		#clock-cells = <1>;
 		#reset-cells = <1>;
 	};
-- 
2.18.0

[PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon

[Rob Herring]

On Sat, Jul 21, 2018 at 09:28:44PM +0200, Martin Blumenstingl wrote:
> The clock controller on Meson8/Meson8m2 and Meson8b is part of a
> register region called "HHI". This register area contains more
> functionality than just a clock controller:
> - the clock controller
> - some reset controller bits
> - temperature sensor calibration data (on Meson8b and Meson8m2 only)
> - HDMI controller
> 
> The HHI register area may be accessed concurrently. Allow this by using
> a "system controller" as parent node.

Why? A single node can be a provider of multiple things. Maybe the HDMI 
should be a child since it will involve graph nodes, but the rest can be 
one node. There should be numerous examples of blocks that are clock and 
reset controllers.

> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  .../bindings/clock/amlogic,meson8b-clkc.txt         | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
> index b455c5aa9139..38fb979210d3 100644
> --- a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
> +++ b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
> @@ -9,15 +9,13 @@ Required Properties:
>  	- "amlogic,meson8-clkc" for Meson8 (S802) SoCs
>  	- "amlogic,meson8b-clkc" for Meson8 (S805) SoCs
>  	- "amlogic,meson8m2-clkc" for Meson8m2 (S812) SoCs
> -- reg: it must be composed by two tuples:
> -	0) physical base address of the xtal register and length of memory
> -	   mapped region.
> -	1) physical base address of the clock controller and length of memory
> -	   mapped region.
> -
>  - #clock-cells: should be 1.
>  - #reset-cells: should be 1.
>  
> +Parent node should have the following properties :
> +- compatible: "syscon", "simple-mfd"

These 2 compatibles alone are not valid.

> +- reg: base address and size of the HHI system control register space.
> +
>  Each clock is assigned an identifier and client nodes can use this identifier
>  to specify the clock which they consume. All available clocks are defined as
>  preprocessor macros in the dt-bindings/clock/meson8b-clkc.h header and can be
> @@ -30,9 +28,8 @@ device tree sources).
>  
>  Example: Clock controller node:
>  
> -	clkc: clock-controller at c1104000 {
> +	clkc: clock-controller {
>  		compatible = "amlogic,meson8b-clkc";
> -		reg = <0xc1108000 0x4>, <0xc1104000 0x460>;
>  		#clock-cells = <1>;
>  		#reset-cells = <1>;
>  	};
> -- 
> 2.18.0
> 

[PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon

[Martin Blumenstingl]

Hi Rob,

On Wed, Jul 25, 2018 at 10:07 PM Rob Herring <robh@kernel.org> wrote:
>
> On Sat, Jul 21, 2018 at 09:28:44PM +0200, Martin Blumenstingl wrote:
> > The clock controller on Meson8/Meson8m2 and Meson8b is part of a
> > register region called "HHI". This register area contains more
> > functionality than just a clock controller:
> > - the clock controller
> > - some reset controller bits
> > - temperature sensor calibration data (on Meson8b and Meson8m2 only)
> > - HDMI controller
> >
> > The HHI register area may be accessed concurrently. Allow this by using
> > a "system controller" as parent node.
>
> Why? A single node can be a provider of multiple things. Maybe the HDMI
> should be a child since it will involve graph nodes, but the rest can be
> one node. There should be numerous examples of blocks that are clock and
> reset controllers.
I understand that a node can provide multiple "things" - currently
it's a clock controller and a reset controller
the HDMI controller could also be integrated in a similar way

however, I do not know how to access the temperature sensor calibration data
there is an ADC - one of it's channel has access to a temperature sensor
this ADC is located at CBUS offset 0x8680 and we already have a driver
for it (meson-saradc)
my problem is that the temperature sensor has to be calibrated - this
is done by:
- read data from efuse
- write 4 bits of calibration data to some register in the ADC's register space
- write a 5th bit of calibration data to a (seemingly random) register
in the HHI register space
(if one of the 5 bits is not written to it's correct location then the
temperature sensor reads bogus values)

I am not sure how to handle this without passing the HHI region to the
meson-saradc driver and letting that initialize all the temperature
calibration data bits (in it's own register space as well as the HHI
register space)
do you have any suggestion here?

> >
> > Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> > ---
> >  .../bindings/clock/amlogic,meson8b-clkc.txt         | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
> > index b455c5aa9139..38fb979210d3 100644
> > --- a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
> > +++ b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
> > @@ -9,15 +9,13 @@ Required Properties:
> >       - "amlogic,meson8-clkc" for Meson8 (S802) SoCs
> >       - "amlogic,meson8b-clkc" for Meson8 (S805) SoCs
> >       - "amlogic,meson8m2-clkc" for Meson8m2 (S812) SoCs
> > -- reg: it must be composed by two tuples:
> > -     0) physical base address of the xtal register and length of memory
> > -        mapped region.
> > -     1) physical base address of the clock controller and length of memory
> > -        mapped region.
> > -
> >  - #clock-cells: should be 1.
> >  - #reset-cells: should be 1.
> >
> > +Parent node should have the following properties :
> > +- compatible: "syscon", "simple-mfd"
>
> These 2 compatibles alone are not valid.
so I should add (for example) "amlogic,meson8b-hhi-sysctrl" as first compatible?
so the result would be: compatible = "amlogic,meson8b-hhi-sysctrl",
"syscon", "simple-mfd";


Regards
Martin

[PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon

[Neil Armstrong]

Hi Rob, Martin,

On 25/07/2018 23:16, Martin Blumenstingl wrote:
> Hi Rob,
> 
> On Wed, Jul 25, 2018 at 10:07 PM Rob Herring <robh@kernel.org> wrote:
>>
>> On Sat, Jul 21, 2018 at 09:28:44PM +0200, Martin Blumenstingl wrote:
>>> The clock controller on Meson8/Meson8m2 and Meson8b is part of a
>>> register region called "HHI". This register area contains more
>>> functionality than just a clock controller:
>>> - the clock controller
>>> - some reset controller bits
>>> - temperature sensor calibration data (on Meson8b and Meson8m2 only)
>>> - HDMI controller
>>>
>>> The HHI register area may be accessed concurrently. Allow this by using
>>> a "system controller" as parent node.
>>
>> Why? A single node can be a provider of multiple things. Maybe the HDMI
>> should be a child since it will involve graph nodes, but the rest can be
>> one node. There should be numerous examples of blocks that are clock and
>> reset controllers.
> I understand that a node can provide multiple "things" - currently
> it's a clock controller and a reset controller
> the HDMI controller could also be integrated in a similar way
> 
> however, I do not know how to access the temperature sensor calibration data
> there is an ADC - one of it's channel has access to a temperature sensor
> this ADC is located at CBUS offset 0x8680 and we already have a driver
> for it (meson-saradc)
> my problem is that the temperature sensor has to be calibrated - this
> is done by:
> - read data from efuse
> - write 4 bits of calibration data to some register in the ADC's register space
> - write a 5th bit of calibration data to a (seemingly random) register
> in the HHI register space
> (if one of the 5 bits is not written to it's correct location then the
> temperature sensor reads bogus values)
> 
> I am not sure how to handle this without passing the HHI region to the
> meson-saradc driver and letting that initialize all the temperature
> calibration data bits (in it's own register space as well as the HHI
> register space)
> do you have any suggestion here?

The clock controller node was badly represented from the beginning, the
HHI register zone *is* a "system control" zone with a lot of different
registers controller the system, including all the EE clocks, *some* HDMI
PHY settings, *some* Video DAC settings, *some* memory power control...

Thus, the clock controller *is* entirely in the HHI registers space, but
the HHI register space should be used by other nodes for control some
part of the SoCs. This is why we decided to model the HHI zone as a
"syscon" to permit other node to have access to these registers and
"simple-mfd" to expose the *big* feature of the HHI : the clock controller.

We are migrating the Meson8 because we already migrated the GX and because the
AXG family was pushed like this (and acked like this), so it's common sense
to align the 3 families because they share the same characteristics here.

Neil

> 
>>>
>>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>>> ---
>>>  .../bindings/clock/amlogic,meson8b-clkc.txt         | 13 +++++--------
>>>  1 file changed, 5 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
>>> index b455c5aa9139..38fb979210d3 100644
>>> --- a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
>>> +++ b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
>>> @@ -9,15 +9,13 @@ Required Properties:
>>>       - "amlogic,meson8-clkc" for Meson8 (S802) SoCs
>>>       - "amlogic,meson8b-clkc" for Meson8 (S805) SoCs
>>>       - "amlogic,meson8m2-clkc" for Meson8m2 (S812) SoCs
>>> -- reg: it must be composed by two tuples:
>>> -     0) physical base address of the xtal register and length of memory
>>> -        mapped region.
>>> -     1) physical base address of the clock controller and length of memory
>>> -        mapped region.
>>> -
>>>  - #clock-cells: should be 1.
>>>  - #reset-cells: should be 1.
>>>
>>> +Parent node should have the following properties :
>>> +- compatible: "syscon", "simple-mfd"
>>
>> These 2 compatibles alone are not valid.
> so I should add (for example) "amlogic,meson8b-hhi-sysctrl" as first compatible?
> so the result would be: compatible = "amlogic,meson8b-hhi-sysctrl",
> "syscon", "simple-mfd";

yes, we used a specific compatible for GX and AXG like this.

Question, what should be the order between "syscon" and "simple-mfd" ?

> 
> 
> Regards
> Martin
> 

[PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon

[Martin Blumenstingl]

Hi Rob,

On Thu, Jul 26, 2018 at 9:32 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Hi Rob, Martin,
>
> On 25/07/2018 23:16, Martin Blumenstingl wrote:
> > Hi Rob,
> >
> > On Wed, Jul 25, 2018 at 10:07 PM Rob Herring <robh@kernel.org> wrote:
> >>
> >> On Sat, Jul 21, 2018 at 09:28:44PM +0200, Martin Blumenstingl wrote:
> >>> The clock controller on Meson8/Meson8m2 and Meson8b is part of a
> >>> register region called "HHI". This register area contains more
> >>> functionality than just a clock controller:
> >>> - the clock controller
> >>> - some reset controller bits
> >>> - temperature sensor calibration data (on Meson8b and Meson8m2 only)
> >>> - HDMI controller
> >>>
> >>> The HHI register area may be accessed concurrently. Allow this by using
> >>> a "system controller" as parent node.
> >>
> >> Why? A single node can be a provider of multiple things. Maybe the HDMI
> >> should be a child since it will involve graph nodes, but the rest can be
> >> one node. There should be numerous examples of blocks that are clock and
> >> reset controllers.
> > I understand that a node can provide multiple "things" - currently
> > it's a clock controller and a reset controller
> > the HDMI controller could also be integrated in a similar way
> >
> > however, I do not know how to access the temperature sensor calibration data
> > there is an ADC - one of it's channel has access to a temperature sensor
> > this ADC is located at CBUS offset 0x8680 and we already have a driver
> > for it (meson-saradc)
> > my problem is that the temperature sensor has to be calibrated - this
> > is done by:
> > - read data from efuse
> > - write 4 bits of calibration data to some register in the ADC's register space
> > - write a 5th bit of calibration data to a (seemingly random) register
> > in the HHI register space
> > (if one of the 5 bits is not written to it's correct location then the
> > temperature sensor reads bogus values)
> >
> > I am not sure how to handle this without passing the HHI region to the
> > meson-saradc driver and letting that initialize all the temperature
> > calibration data bits (in it's own register space as well as the HHI
> > register space)
> > do you have any suggestion here?
>
> The clock controller node was badly represented from the beginning, the
> HHI register zone *is* a "system control" zone with a lot of different
> registers controller the system, including all the EE clocks, *some* HDMI
> PHY settings, *some* Video DAC settings, *some* memory power control...
>
> Thus, the clock controller *is* entirely in the HHI registers space, but
> the HHI register space should be used by other nodes for control some
> part of the SoCs. This is why we decided to model the HHI zone as a
> "syscon" to permit other node to have access to these registers and
> "simple-mfd" to expose the *big* feature of the HHI : the clock controller.
does our (Neil's and mine) explanation make sense to you?
I understand your point that a DT node can provide multiple "things".
this is the case on the Meson HHI region, but in addition to that the
HHI region contains additional bits for other IP blocks. so my
understanding is that using a syscon here is fine

> We are migrating the Meson8 because we already migrated the GX and because the
> AXG family was pushed like this (and acked like this), so it's common sense
> to align the 3 families because they share the same characteristics here.
>
> Neil
>
> >
> >>>
> >>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> >>> ---
> >>>  .../bindings/clock/amlogic,meson8b-clkc.txt         | 13 +++++--------
> >>>  1 file changed, 5 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
> >>> index b455c5aa9139..38fb979210d3 100644
> >>> --- a/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
> >>> +++ b/Documentation/devicetree/bindings/clock/amlogic,meson8b-clkc.txt
> >>> @@ -9,15 +9,13 @@ Required Properties:
> >>>       - "amlogic,meson8-clkc" for Meson8 (S802) SoCs
> >>>       - "amlogic,meson8b-clkc" for Meson8 (S805) SoCs
> >>>       - "amlogic,meson8m2-clkc" for Meson8m2 (S812) SoCs
> >>> -- reg: it must be composed by two tuples:
> >>> -     0) physical base address of the xtal register and length of memory
> >>> -        mapped region.
> >>> -     1) physical base address of the clock controller and length of memory
> >>> -        mapped region.
> >>> -
> >>>  - #clock-cells: should be 1.
> >>>  - #reset-cells: should be 1.
> >>>
> >>> +Parent node should have the following properties :
> >>> +- compatible: "syscon", "simple-mfd"
> >>
> >> These 2 compatibles alone are not valid.
> > so I should add (for example) "amlogic,meson8b-hhi-sysctrl" as first compatible?
> > so the result would be: compatible = "amlogic,meson8b-hhi-sysctrl",
> > "syscon", "simple-mfd";
>
> yes, we used a specific compatible for GX and AXG like this.
>
> Question, what should be the order between "syscon" and "simple-mfd" ?
for consistency I will go with whatever the GX and AXG dt-bindings use


Regards
Martin

[PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon

[Stephen Boyd]

Quoting Martin Blumenstingl (2018-08-12 11:35:17)
> Hi Rob,
> 
> On Thu, Jul 26, 2018 at 9:32 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> >
> > Hi Rob, Martin,
> >
> > On 25/07/2018 23:16, Martin Blumenstingl wrote:
> > > Hi Rob,
> > >
> > > On Wed, Jul 25, 2018 at 10:07 PM Rob Herring <robh@kernel.org> wrote:
> > >>
> > >> On Sat, Jul 21, 2018 at 09:28:44PM +0200, Martin Blumenstingl wrote:
> > >>> The clock controller on Meson8/Meson8m2 and Meson8b is part of a
> > >>> register region called "HHI". This register area contains more
> > >>> functionality than just a clock controller:
> > >>> - the clock controller
> > >>> - some reset controller bits
> > >>> - temperature sensor calibration data (on Meson8b and Meson8m2 only)
> > >>> - HDMI controller
> > >>>
> > >>> The HHI register area may be accessed concurrently. Allow this by using
> > >>> a "system controller" as parent node.
> > >>
> > >> Why? A single node can be a provider of multiple things. Maybe the HDMI
> > >> should be a child since it will involve graph nodes, but the rest can be
> > >> one node. There should be numerous examples of blocks that are clock and
> > >> reset controllers.
> > > I understand that a node can provide multiple "things" - currently
> > > it's a clock controller and a reset controller
> > > the HDMI controller could also be integrated in a similar way
> > >
> > > however, I do not know how to access the temperature sensor calibration data
> > > there is an ADC - one of it's channel has access to a temperature sensor
> > > this ADC is located at CBUS offset 0x8680 and we already have a driver
> > > for it (meson-saradc)
> > > my problem is that the temperature sensor has to be calibrated - this
> > > is done by:
> > > - read data from efuse
> > > - write 4 bits of calibration data to some register in the ADC's register space
> > > - write a 5th bit of calibration data to a (seemingly random) register
> > > in the HHI register space
> > > (if one of the 5 bits is not written to it's correct location then the
> > > temperature sensor reads bogus values)
> > >
> > > I am not sure how to handle this without passing the HHI region to the
> > > meson-saradc driver and letting that initialize all the temperature
> > > calibration data bits (in it's own register space as well as the HHI
> > > register space)
> > > do you have any suggestion here?
> >
> > The clock controller node was badly represented from the beginning, the
> > HHI register zone *is* a "system control" zone with a lot of different
> > registers controller the system, including all the EE clocks, *some* HDMI
> > PHY settings, *some* Video DAC settings, *some* memory power control...
> >
> > Thus, the clock controller *is* entirely in the HHI registers space, but
> > the HHI register space should be used by other nodes for control some
> > part of the SoCs. This is why we decided to model the HHI zone as a
> > "syscon" to permit other node to have access to these registers and
> > "simple-mfd" to expose the *big* feature of the HHI : the clock controller.
> does our (Neil's and mine) explanation make sense to you?
> I understand your point that a DT node can provide multiple "things".
> this is the case on the Meson HHI region, but in addition to that the
> HHI region contains additional bits for other IP blocks. so my
> understanding is that using a syscon here is fine
> 

I'm dropping this from my review queue because nobody has replied in
many weeks. I think Rob wants people to move away from syscon so if you
can't do that then we need good reasons. I recommend putting those
reasons in the commit text and resending this series.

[PATCH 1/3] dt-bindings: clock: meson8b: use the registers from the HHI syscon

[Jerome Brunet]

On Fri, 2018-08-31 at 10:39 -0700, Stephen Boyd wrote:
> > > The clock controller node was badly represented from the beginning, the
> > > HHI register zone *is* a "system control" zone with a lot of different
> > > registers controller the system, including all the EE clocks, *some* HDMI
> > > PHY settings, *some* Video DAC settings, *some* memory power control...
> > > 
> > > Thus, the clock controller *is* entirely in the HHI registers space, but
> > > the HHI register space should be used by other nodes for control some
> > > part of the SoCs. This is why we decided to model the HHI zone as a
> > > "syscon" to permit other node to have access to these registers and
> > > "simple-mfd" to expose the *big* feature of the HHI : the clock controller.
> > 
> > does our (Neil's and mine) explanation make sense to you?
> > I understand your point that a DT node can provide multiple "things".
> > this is the case on the Meson HHI region, but in addition to that the
> > HHI region contains additional bits for other IP blocks. so my
> > understanding is that using a syscon here is fine
> > 
> 
> I'm dropping this from my review queue because nobody has replied in
> many weeks. I think Rob wants people to move away from syscon so if you
> can't do that then we need good reasons. I recommend putting those
> reasons in the commit text and resending this series.

Doubling down on what Neil and Martin already reported:

When this clock controller what initially added, It was thought that the HHI
register space only contained clocks and resets, which we know how to handle in
a single driver and does not mandate a 'syscon'

Since then, we learn that HHI has more than just clocks and reset. This is the
case on every amlogic platform, which is why we already moved gxbb/gxl to
syscon. axg arrived later and used this model directly. Meson8 did not mandate
the change so far. We tried to avoid this mess as long as it was not absolutely
necessary.

This particular platform (meson8) has an ADC block with is own registers space.
The ADC comes with temperature sensor which has a few compensation values. 4 of
these compensation values are in the ADC register space, the 5th is apparently
in the HHI... HW is what it is ...

Bottom line is, HHI is a typical system controller which need to be access by
several devices. We are not particularly fond of syscon either but there are
still some situations were this is cleanest solution, AFAIK.

Jerome

[PATCH 2/3] clk: meson: meson8b: use the HHI syscon if available

[Martin Blumenstingl]

The clock controller is located in a register range (called "HHI") which
contains more than just registers for the clock controller. Known
consumers of the HHI register range are:
- the clock controller
- a reset controller
- temperature sensor calibration (TSC) data (Meson8b and Meson8m2 only)
- HDMI controller

Get the regmap from the parent (HHI syscon) node to support all
functionality of the HHI register range. Backwards compatibility with
old .dtbs is ensured by falling back to parsing the registers just like
before this change.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/clk/meson/meson8b.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/meson/meson8b.c b/drivers/clk/meson/meson8b.c
index e961ee9fec20..76e54f604140 100644
--- a/drivers/clk/meson/meson8b.c
+++ b/drivers/clk/meson/meson8b.c
@@ -10,6 +10,7 @@
 #include <linux/clk.h>
 #include <linux/clk-provider.h>
 #include <linux/init.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
 #include <linux/reset-controller.h>
 #include <linux/slab.h>
@@ -1109,16 +1110,21 @@ static void __init meson8b_clkc_init(struct device_node *np)
 	struct regmap *map;
 	int i, ret;
 
-	/* Generic clocks, PLLs and some of the reset-bits */
-	clk_base = of_iomap(np, 1);
-	if (!clk_base) {
-		pr_err("%s: Unable to map clk base\n", __func__);
-		return;
-	}
+	map = syscon_node_to_regmap(of_get_parent(np));
+	if (IS_ERR(map)) {
+		pr_info("failed to get HHI regmap - Trying obsolete regs\n");
 
-	map = regmap_init_mmio(NULL, clk_base, &clkc_regmap_config);
-	if (IS_ERR(map))
-		return;
+		/* Generic clocks, PLLs and some of the reset-bits */
+		clk_base = of_iomap(np, 1);
+		if (!clk_base) {
+			pr_err("%s: Unable to map clk base\n", __func__);
+			return;
+		}
+
+		map = regmap_init_mmio(NULL, clk_base, &clkc_regmap_config);
+		if (IS_ERR(map))
+			return;
+	}
 
 	rstc = kzalloc(sizeof(*rstc), GFP_KERNEL);
 	if (!rstc)
-- 
2.18.0

[PATCH 3/3] ARM: dts: meson: switch the clock controller to the HHI register area

[Martin Blumenstingl]

The clock controller on Meson8/Meson8m2 and Meson8b is part of a
register region called "HHI". This register area contains more
functionality than just a clock controller:
- the clock controller
- some reset controller bits
- temperature sensor calibration data (on Meson8b and Meson8m2 only)
- HDMI controller

Allow access to this HHI register area as "system controller". Also
migrate the Meson8 and Meson8b clock controllers to this new node.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 arch/arm/boot/dts/meson.dtsi   |  5 +++++
 arch/arm/boot/dts/meson8.dtsi  | 15 ++++++++-------
 arch/arm/boot/dts/meson8b.dtsi | 15 ++++++++-------
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/arch/arm/boot/dts/meson.dtsi b/arch/arm/boot/dts/meson.dtsi
index 0d9faf1a51ea..dc0cbdbcca54 100644
--- a/arch/arm/boot/dts/meson.dtsi
+++ b/arch/arm/boot/dts/meson.dtsi
@@ -80,6 +80,11 @@
 			#size-cells = <1>;
 			ranges = <0x0 0xc1100000 0x200000>;
 
+			hhi: system-controller at 4000 {
+				compatible = "syscon", "simple-mfd";
+				reg = <0x4000 0x400>;
+			};
+
 			assist: assist at 7c00 {
 				compatible = "amlogic,meson-mx-assist", "syscon";
 				reg = <0x7c00 0x200>;
diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index 7162e0ca05b0..3e3d9c54cddc 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -190,13 +190,6 @@
 };
 
 &cbus {
-	clkc: clock-controller at 4000 {
-		#clock-cells = <1>;
-		#reset-cells = <1>;
-		compatible = "amlogic,meson8-clkc";
-		reg = <0x8000 0x4>, <0x4000 0x400>;
-	};
-
 	reset: reset-controller at 4404 {
 		compatible = "amlogic,meson8b-reset";
 		reg = <0x4404 0x9c>;
@@ -323,6 +316,14 @@
 	status = "okay";
 };
 
+&hhi {
+	clkc: clock-controller {
+		compatible = "amlogic,meson8-clkc";
+		#clock-cells = <1>;
+		#reset-cells = <1>;
+	};
+};
+
 &hwrng {
 	compatible = "amlogic,meson8-rng", "amlogic,meson-rng";
 	clocks = <&clkc CLKID_RNG0>;
diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index f77e419c1c65..accb77ba3931 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -159,13 +159,6 @@
 };
 
 &cbus {
-	clkc: clock-controller at 4000 {
-		#clock-cells = <1>;
-		#reset-cells = <1>;
-		compatible = "amlogic,meson8b-clkc";
-		reg = <0x8000 0x4>, <0x4000 0x400>;
-	};
-
 	reset: reset-controller at 4404 {
 		compatible = "amlogic,meson8b-reset";
 		reg = <0x4404 0x9c>;
@@ -268,6 +261,14 @@
 	status = "okay";
 };
 
+&hhi {
+	clkc: clock-controller {
+		compatible = "amlogic,meson8-clkc";
+		#clock-cells = <1>;
+		#reset-cells = <1>;
+	};
+};
+
 &hwrng {
 	compatible = "amlogic,meson8b-rng", "amlogic,meson-rng";
 	clocks = <&clkc CLKID_RNG0>;
-- 
2.18.0

[PATCH 0/3] Meson8/Meson8b: introduce a HHI syscon node

[Neil Armstrong]

On 21/07/2018 21:28, Martin Blumenstingl wrote:
> The Meson8/Meson8b/Meson8m2 SoCs are suffering from a similar problem
> as the GXBB/GXL/GXM SoCs (see the GX series from Jerome: [0]):
> There is a register area called "HHI" which is used for multiple IP
> blocks of the SoC:
> - the system clock controller
> - a few reset lines (there is a separate reset controller, these reset
>   lines are not part of the other reset controller). this reset
>   controller is currently implemented in the clock controller driver
> - a HDMI controller
> - temperature sensor calibration data (by "data" I really mean data,
>   the ADC driver has four bits for the TSC data in it's own register
>   space, however on Meson8b and Meson8m2 there is a fifth TSC bit which
>   is stored in the HHI register area)
> 
> The first three could be implemented with a single node (either in one
> big driver, or using a MFD driver which would register function-
> specific drivers).
> However, the TSC data is a big problem, because the ADC has it's own
> set of registers but needs to write one bit in the HHI register area.
> 
> NOTE: this series has multiple dependencies:
> - the clock controller changes depend "meson8b: add the CPU_DIV16 clock
>   for the ARM TWD" as well as "meson8b: register the clock controller
>   early" [2]
> - the dts changes depend on "fix clock controller register size on
>   Meson8/Meson8b" [3]
> 
> 
> [0] http://lists.infradead.org/pipermail/linux-amlogic/2018-March/006733.html
> [1] http://lists.infradead.org/pipermail/linux-amlogic/2018-July/007890.html
> [2] http://lists.infradead.org/pipermail/linux-amlogic/2018-July/007900.html
> [3] http://lists.infradead.org/pipermail/linux-amlogic/2018-July/007897.html
> 
> 
> Martin Blumenstingl (3):
>   dt-bindings: clock: meson8b: use the registers from the HHI syscon
>   clk: meson: meson8b: use the HHI syscon if available
>   ARM: dts: meson: switch the clock controller to the HHI register area
> 
>  .../bindings/clock/amlogic,meson8b-clkc.txt   | 13 ++++------
>  arch/arm/boot/dts/meson.dtsi                  |  5 ++++
>  arch/arm/boot/dts/meson8.dtsi                 | 15 ++++++------
>  arch/arm/boot/dts/meson8b.dtsi                | 15 ++++++------
>  drivers/clk/meson/meson8b.c                   | 24 ++++++++++++-------
>  5 files changed, 41 insertions(+), 31 deletions(-)
> 

For the whole serie, it follow the same switch as GX so :

Acked-by: Neil Armstrong <narmstrong@baylibre.com>