Re: [PATCH v2 1/8] arm64: dts: meson: g12a: Add AO Clock + Reset Controller support

[Martin Blumenstingl]

Hi Neil,

On Tue, Mar 19, 2019 at 9:47 AM Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
>
> On 18/03/2019 21:02, Martin Blumenstingl wrote:
> > Hi Neil,
> >
> > On Mon, Mar 18, 2019 at 10:59 AM Neil Armstrong <narmstrong@xxxxxxxxxxxx> wrote:
> >>
> >> Add nodes and properties for the AO Clocks and Resets.
> >>
> >> Signed-off-by: Neil Armstrong <narmstrong@xxxxxxxxxxxx>
> >> Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> >> ---
> >>  arch/arm64/boot/dts/amlogic/meson-g12a.dtsi | 18 ++++++++++++++++++
> >>  1 file changed, 18 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
> >> index 31ddf9444b3e..5c0983edf837 100644
> >> --- a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
> >> +++ b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
> >> @@ -4,6 +4,7 @@
> >>   */
> >>
> >>  #include <dt-bindings/gpio/gpio.h>
> >> +#include <dt-bindings/clock/g12a-clkc.h>
> >>  #include <dt-bindings/interrupt-controller/irq.h>
> >>  #include <dt-bindings/interrupt-controller/arm-gic.h>
> >>
> >> @@ -122,6 +123,23 @@
> >>                         #size-cells = <2>;
> >>                         ranges = <0x0 0x0 0x0 0xff800000 0x0 0x100000>;
> >>
> >> +                       rti: sys-ctrl@0 {
> >> +                               compatible = "amlogic,meson-gx-ao-sysctrl",
> >> +                                            "simple-mfd", "syscon";
> >> +                               reg = <0x0 0x0 0x0 0x100>;
> >> +                               #address-cells = <2>;
> >> +                               #size-cells = <2>;
> >> +                               ranges = <0x0 0x0 0x0 0x0 0x0 0x100>;
> > sorry for noticing this only very late: I missed the #address-cells,
> > #size-cells and ranges property in my last review
> > do you have any change queued which requires this?
> > my understanding is that the drivers for all RTI children should use
> > the register offsets relative to the RTI start address. In that case
> > the child nodes neither have a unit-address nor a reg property, making
> > the last three properties unnecessary.
>
> We need the address-cells/size-cells and `ranges;` to satisfy the need
> for the gpio subnode of the pinctrl node.
>
> For GX, we didn't add the pinctrl in the sysctrl_AO subnode, but we should
> overwise we have overlapping.
ah, I see - thank you for the explanation.

setting #address-cells and #size-cells will probably yield dtc
warnings for the children without unit-address / reg property.
also the pinctrl driver and the syscon driver would still ioremap
overlapping memory regions (syscon: the full 0x100 range, the pinctrl
driver various 4 and 8 byte registers) because both are ioremap'ing
their register space.
but indeed, the overlapping dt nodes are solved with this approach.

Rob acked a binding for the Lantiq XWAY SoC's with a simple-mfd /
syscon as parent and children with a reg property. The parent node
defines a reg and ranges property, but neither #size-cells nor
#address-cells. the binding is documented here: [0]
The requirements seem similar to our pinctrl needs. We could end up
with only a single syscon and no overlapping ioremap for the pinctrl
driver and we may not even have to change the binding.

with this mail I want to start a discussion about the bindings of the
AO pin controller (similar to how we re-considered the binding of the
AO clock controller in the past). at the same time I'm not saying that
we immediately have to change it.


Regards
Martin


[0] https://www.kernel.org/doc/Documentation/devicetree/bindings/mips/lantiq/rcu.txt