Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190318095851.4062-1-narmstrong@baylibre.com>
 <20190318095851.4062-2-narmstrong@baylibre.com> <CAFBinCDfOEvmQcE7TgYvXmM1f=pywYbjhbAZWs4AYNXs8y6y7Q@mail.gmail.com>
 <7f391daf-db70-101a-18c7-04abf9c5a164@baylibre.com>
In-Reply-To: <7f391daf-db70-101a-18c7-04abf9c5a164@baylibre.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Tue, 19 Mar 2019 22:22:13 +0100
Message-ID: <CAFBinCBduFG5uK9ghDW98fe4t17UeZj1b3STHByVY15C-3hB2A@mail.gmail.com>
Subject: Re: [PATCH v2 1/8] arm64: dts: meson: g12a: Add AO Clock + Reset
 Controller support
To:     Neil Armstrong <narmstrong@baylibre.com>
Cc:     khilman@baylibre.com, linux-amlogic@lists.infradead.org,
        Jerome Brunet <jbrunet@baylibre.com>,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Neil,

On Tue, Mar 19, 2019 at 9:47 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> On 18/03/2019 21:02, Martin Blumenstingl wrote:
> > Hi Neil,
> >
> > On Mon, Mar 18, 2019 at 10:59 AM Neil Armstrong <narmstrong@baylibre.com> wrote:
> >>
> >> Add nodes and properties for the AO Clocks and Resets.
> >>
> >> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >>  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