Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 22 Aug 2022 20:30:39 +0200
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali@kernel.org>
To:     Rob Herring <robh@kernel.org>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        devicetree@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] powerpc/85xx: DTS: Add CPLD definitions for P1021RDB
 Combo Board CPL Design
Message-ID: <20220822183039.sfpupym236ubkeio@pali>
References: <20220819084433.26011-1-pali@kernel.org>
 <20220822170056.GA4135542-robh@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220822170056.GA4135542-robh@kernel.org>
User-Agent: NeoMutt/20180716
Precedence: bulk

On Monday 22 August 2022 12:00:56 Rob Herring wrote:
> On Fri, Aug 19, 2022 at 10:44:33AM +0200, Pali Rohár wrote:
> > P1021RDB Combo Board CPLD Design is used on following Freescale boards:
> > P1021RDB-PC, P1020RDB-PD, P1020MBG-PC, P1020UTM-PC and P2020RDB-PCA.
> > 
> > Add CPLD definitions for all these boards for which already exist DTS file.
> > 
> > CPLD has bank size 128kB, it is connected via CS3 on LBC and mapped to
> > memory range 0xFFA00000~0xFFA1FFFF.
> > 
> > As CPLD firmware is common on all these boards, use just one compatible
> > string "fsl,p1021rdb-pc-cpld".
> > 
> > In some DTS files is CPLD already defined, but definition is either
> > incomplete or wrong. So fix it.
> > 
> > All these boards have via CPLD connected max6370 watchdog at offset 0x2
> > with GPIO 11, status led at offset 0x8 and reset controller at offset 0xd.
> > Additionally P1020MBG-PC and P1020RDB-PD boards have FXO led at offset 0x9
> > and FXS leds at offset 0xa.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > ---
> >  arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi    | 92 +++++++++++++++++++
> >  arch/powerpc/boot/dts/fsl/p1020mbg-pc_32b.dts |  6 +-
> >  arch/powerpc/boot/dts/fsl/p1020mbg-pc_36b.dts |  6 +-
> >  arch/powerpc/boot/dts/fsl/p1020rdb-pd.dts     | 44 +++++++--
> >  arch/powerpc/boot/dts/fsl/p1020utm-pc.dtsi    | 37 ++++++++
> >  arch/powerpc/boot/dts/fsl/p1020utm-pc_32b.dts |  4 +-
> >  arch/powerpc/boot/dts/fsl/p1020utm-pc_36b.dts |  4 +-
> >  arch/powerpc/boot/dts/fsl/p1021rdb-pc.dtsi    | 37 ++++++++
> >  arch/powerpc/boot/dts/fsl/p1021rdb-pc_32b.dts |  5 +-
> >  arch/powerpc/boot/dts/fsl/p1021rdb-pc_36b.dts |  5 +-
> >  arch/powerpc/boot/dts/fsl/p2020rdb-pc.dtsi    | 33 ++++++-
> >  11 files changed, 251 insertions(+), 22 deletions(-)
> > 
> > diff --git a/arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi b/arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi
> > index a24699cfea9c..c73996dcd809 100644
> > --- a/arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi
> > +++ b/arch/powerpc/boot/dts/fsl/p1020mbg-pc.dtsi
> > @@ -83,6 +83,95 @@
> >  		compatible = "vitesse-7385";
> >  		reg = <0x2 0x0 0x20000>;
> >  	};
> > +
> > +	cpld@3,0 {
> > +		#address-cells = <1>;
> > +		#size-cells = <1>;
> > +		compatible = "fsl,p1021rdb-pc-cpld", "simple-bus", "syscon";
> > +		reg = <0x3 0x0 0x20000>;
> > +		ranges = <0x0 0x3 0x0 0x20000>;
> > +
> > +		watchdog@2 {
> > +			compatible = "maxim,max6370";
> > +			reg = <0x2 0x1>;
> > +			gpios = <&gpio 11 1>;
> > +		};
> > +
> > +		led@8 {
> 
> The register-bit-led schema says this should be 'led@8,0'. Did you 
> run 'dtbs_check'? 

No, it does not work and I lost interest to trying setup machine for it again.

> But that's going to conflict with what simple-bus schema says.

Another reason why not try it again. Lot of schemas says that are in
conflict, nobody understand it and when I ask how to do it, I just get
silence or answer which does not work on the real hw. And if there is
some schema error message then it looks like it was generated by random
word generator.

> I don't 
> know that 'simple-bus' is really appropriate here. The CPLD isn't really 
> just a bus. 'simple-mfd' is what's more commonly used with 'syscon'.

Sorry, I do not understand those schemas anymore. And based on previous
failures, I'm not going to try it again.

It is a _bus_ and it was tested that it works as a bus with more
existing drivers.

> > +			compatible = "register-bit-led";
> > +			reg = <0x8 0x1>;
> > +			offset = <0x8>;
> > +			mask = <0x1>;
> > +			active-low;
> > +			default-state = "keep";
> > +			label = "status";
> > +			function = "status";
> > +			color = <6>; /* LED_COLOR_ID_YELLOW */
> > +		};
>