Received-SPF: pass (google.com: domain of linux-kernel+bounces-196839-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Alexander Stein <alexander.stein@ew.tq-group.com>
To: Esben Haabendal <esben@geanix.com>
Cc: Shawn Guo <shawnguo@kernel.org>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>, Conor Dooley <conor+dt@kernel.org>, linux-arm-kernel@lists.infradead.org, Rasmus Villemoes <linux@rasmusvillemoes.dk>, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ARM: dts: ls1021a: add QUICC Engine node
Date: Fri, 31 May 2024 15:09:05 +0200
Message-ID: <5987259.31r3eYUQgx@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <87frtynpfx.fsf@geanix.com>
References: <20240530-arm-ls1021a-qe-dts-v1-1-2eda23bdf8c5@geanix.com> <3380831.44csPzL39Z@steina-w> <87frtynpfx.fsf@geanix.com>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"

Hi Esben,

Am Freitag, 31. Mai 2024, 14:20:02 CEST schrieb Esben Haabendal:
> Alexander Stein <alexander.stein@ew.tq-group.com> writes:
>=20
> > Would you consider current converting into YAML format?
>=20
> You mean converting Documentation/devicetree/bindings/soc/fsl/qe.txt and
> Documentation/devicetree/bindings/soc/fsl/qe/*.txt into YAML?
>=20
> I can consider that. I haven't done something like that before, but I
> assume it might include some additional work other than trivially format
> conversion. So I would prefer to do that after this patch, if that is
> ok.

Getting the constraints right is probably not that easy. But having
verifiable bindinds helps getting the .dtsi right.

> > Am Donnerstag, 30. Mai 2024, 16:22:54 CEST schrieb Esben Haabendal:
> >> The LS1021A contains a QUICC Engine Block, so add a node to device
> >> tree describing that.
> >>=20
> >> Signed-off-by: Esben Haabendal <esben@geanix.com>
> >> ---
> >>  arch/arm/boot/dts/nxp/ls/ls1021a.dtsi | 51 ++++++++++++++++++++++++++=
+++++++++
> >>  1 file changed, 51 insertions(+)
> >>=20
> >> diff --git a/arch/arm/boot/dts/nxp/ls/ls1021a.dtsi b/arch/arm/boot/dts=
/nxp/ls/ls1021a.dtsi
> >> index e86998ca77d6..ff7be69acdd5 100644
> >> --- a/arch/arm/boot/dts/nxp/ls/ls1021a.dtsi
> >> +++ b/arch/arm/boot/dts/nxp/ls/ls1021a.dtsi
> >> @@ -460,6 +460,57 @@ gpio3: gpio@2330000 {
> >>  			#interrupt-cells =3D <2>;
> >>  		};
> >> =20
> >> +		uqe: uqe@2400000 {
> >> +			#address-cells =3D <1>;
> >> +			#size-cells =3D <1>;
> >> +			device_type =3D "qe";
> >> +			compatible =3D "fsl,qe", "simple-bus";
> >> +			ranges =3D <0x0 0x0 0x2400000 0x40000>;
> >> +			reg =3D <0x0 0x2400000 0x0 0x480>;
> >
> > Properties please in this order:
> > * compatible
> > * reg
> > * #address-cells
> > * #size-cells
> > * ranges
> > * device_type
>=20
> Fixing.
>=20
> >> +			brg-frequency =3D <150000000>;
> >> +			bus-frequency =3D <300000000>;
> >
> > Mh, aren't these values depending on your actual RCW configuration?
>=20
> Yes, you are right. The QE bus-frequency comes from platform_clk which
> is controlled by various bits in RCW and sys_ref_clk.
>=20
> So I guess it should be possible to derive bus-frequency from sysclk
> clock-frequency attribute and RCW. But fsl,qe bus-frequency is a
> required property...
>=20
> Max bus-frequency for LS1021A is 300 MHz. But it should be possible to
> set it lower, although I suspect that many/most/everyone is running it
> at 300 MHz.

Thanks for confirmation. I'll let DT maintainer decide how to deal with thi=
s.

> >> +			fsl,qe-num-riscs =3D <1>;
> >> +			fsl,qe-num-snums =3D <28>;
> >
> > Current bindings defines:
> >> fsl,qe-snums: This property has to be specified as '/bits/ 8' value,
> >>   defining the array of serial number (SNUM) values for the virtual
> >>   threads.
> >
> > So '/bits/ 8' is missing.
>=20
> Ok, so you want me to add an array for fs,qe-snums attribute?
> None of the existing fsl,qe devices has a fsl,qe-snums.
> And qe_snums_init() has a fallback, so I don't think it is correct to
> specify fsl,qe-snums to be a required property in the bindings. It
> should be listed as optional.

fsl,qe-num-snums is a deprecated property, so IMHO the replacement
fsl,qe-snums should be used instead for new device tree entries.
qe_snums_init() supporting 'fsl,qe-num-snums' is just to support
"legacy bindings" as stated in the comment.

>=20
> >> +			qeic: qeic@80 {
> >> +				compatible =3D "fsl,qe-ic";
> >> +				reg =3D <0x80 0x80>;
> >> +				#address-cells =3D <0>;
> >> +				interrupt-controller;
> >> +				#interrupt-cells =3D <1>;
> >> +				interrupts =3D <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH
> >> +					      GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
> >> +			};
> >> +
> >> +			ucc@2000 {
> >> +				cell-index =3D <1>;
> >> +				reg =3D <0x2000 0x200>;
> >> +				interrupts =3D <32>;
> >> +				interrupt-parent =3D <&qeic>;
> >
> > Move cell-index to last position.
>=20
> Done.
>=20
> >> +			};
> >> +
> >> +			ucc@2200 {
> >> +				cell-index =3D <3>;
> >> +				reg =3D <0x2200 0x200>;
> >> +				interrupts =3D <34>;
> >> +				interrupt-parent =3D <&qeic>;
> >
> > Same here.
>=20
> Done.
>=20
> >> +			};
> >> +
> >> +			muram@10000 {
> >> +				#address-cells =3D <1>;
> >> +				#size-cells =3D <1>;
> >> +				compatible =3D "fsl,qe-muram", "fsl,cpm-muram";
> >> +				ranges =3D <0x0 0x10000 0x6000>;
> >
> > Node address but no 'reg' property? I have no idea if this is okay.
> > Also compatible (and possibly reg) first.
>=20
> It is done in the same way for all existing fsl,qe-muram devices. So if
> it is not okay, a tree-wide fixup would be in place.

I can't finally say if this is okay, but at least the compatible shall be
listed first.

Thanks and best regards,
Alexander
=2D-=20
TQ-Systems GmbH | M=FChlstra=DFe 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M=FCnchen, HRB 105018
Gesch=E4ftsf=FChrer: Detlef Schneider, R=FCdiger Stahl, Stefan Schneider
http://www.tq-group.com/