Received-SPF: pass (google.com: domain of linux-kernel+bounces-200524-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
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: Tue, 04 Jun 2024 13:38:44 +0200
Message-ID: <8425529.NyiUUSuA9g@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <87ttiem4de.fsf@geanix.com>
References: <20240530-arm-ls1021a-qe-dts-v1-1-2eda23bdf8c5@geanix.com> <5987259.31r3eYUQgx@steina-w> <87ttiem4de.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, 16:40:29 CEST schrieb Esben Haabendal:
> Alexander Stein <alexander.stein@ew.tq-group.com> writes:
>=20
> > Hi Esben,
> >
> > Am Freitag, 31. Mai 2024, 14:20:02 CEST schrieb Esben Haabendal:
> >> Alexander Stein <alexander.stein@ew.tq-group.com> writes:
> >> >> +			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=
 this.
>=20
> For reference.
>=20
> The existing DTS with fsl,qe have the following bus-frequency property va=
lues:
>=20
> arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi:  bus-frequency =3D <20000=
0000>
> arch/powerpc/boot/dts/fsl/mpc8568si-post.dtsi:   bus-frequency =3D <39600=
0000>
> arch/powerpc/boot/dts/fsl/mpc8569si-post.dtsi:   bus-frequency =3D <0>
> arch/powerpc/boot/dts/fsl/p1021si-post.dtsi:     missing!
> arch/powerpc/boot/dts/fsl/t1024si-post.dtsi:     bus-frequency =3D <0>
> arch/powerpc/boot/dts/fsl/t1040si-post.dtsi:     missing!
> arch/powerpc/boot/dts/kmeter1.dts:               bus-frequency =3D <0>
> arch/powerpc/boot/dts/mpc836x_rdk.dts:           bus-frequency =3D <0>
> arch/powerpc/boot/dts/mpc832x_rdb.dts:           bus-frequency =3D <19800=
0000>
>=20
> The 3 non-zero values are most likely also not guaranteed by SoC design
> to always be the right values. But I haven't checked.

PowerPC might be completely different. Apparently that's the way it is
done until now.

> >> >> +			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
> Figuring out the correct array values for fsl,qe-snums for ls1021a is
> not so easy. It is not so clear from the reference manual, what it
> should be. And the default array used for fsl,qe-num-snums =3D <28> does
> not look right in any way, but seems to work.
>=20
> It would not feel right to just copy those values and put into DTS, as
> it would imply that the values are truly a correct description for the
> LS1021A hardware.

Maybe copy the currently hardcoded values and add a REVISIT comment or
similar describing these are copied, apparently working, but unverified.

Best regards,
Alexander

> >>=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.
>=20
> Done.
>=20
> /Esben
>=20


=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/