linux-kernel - Re: [PATCH] ARM: dts: ls1021a: add QUICC Engine node

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Tue, 04 Jun 2024 13:38:44 +0200
From: Alexander Stein <alexander.stein@...tq-group.com>
To: Esben Haabendal <esben@...nix.com>
Cc: Shawn Guo <shawnguo@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-arm-kernel@...ts.infradead.org, Rasmus Villemoes <linux@...musvillemoes.dk>, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ARM: dts: ls1021a: add QUICC Engine node

Hi Esben,

Am Freitag, 31. Mai 2024, 16:40:29 CEST schrieb Esben Haabendal:
> Alexander Stein <alexander.stein@...tq-group.com> writes:
> 
> > Hi Esben,
> >
> > Am Freitag, 31. Mai 2024, 14:20:02 CEST schrieb Esben Haabendal:
> >> Alexander Stein <alexander.stein@...tq-group.com> writes:
> >> >> +			brg-frequency = <150000000>;
> >> >> +			bus-frequency = <300000000>;
> >> >
> >> > Mh, aren't these values depending on your actual RCW configuration?
> >> 
> >> Yes, you are right. The QE bus-frequency comes from platform_clk which
> >> is controlled by various bits in RCW and sys_ref_clk.
> >> 
> >> 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...
> >> 
> >> 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.
> 
> For reference.
> 
> The existing DTS with fsl,qe have the following bus-frequency property values:
> 
> arch/arm64/boot/dts/freescale/fsl-ls1043a.dtsi:  bus-frequency = <200000000>
> arch/powerpc/boot/dts/fsl/mpc8568si-post.dtsi:   bus-frequency = <396000000>
> arch/powerpc/boot/dts/fsl/mpc8569si-post.dtsi:   bus-frequency = <0>
> arch/powerpc/boot/dts/fsl/p1021si-post.dtsi:     missing!
> arch/powerpc/boot/dts/fsl/t1024si-post.dtsi:     bus-frequency = <0>
> arch/powerpc/boot/dts/fsl/t1040si-post.dtsi:     missing!
> arch/powerpc/boot/dts/kmeter1.dts:               bus-frequency = <0>
> arch/powerpc/boot/dts/mpc836x_rdk.dts:           bus-frequency = <0>
> arch/powerpc/boot/dts/mpc832x_rdb.dts:           bus-frequency = <198000000>
> 
> 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 = <1>;
> >> >> +			fsl,qe-num-snums = <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.
> >> 
> >> 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.
> 
> 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 = <28> does
> not look right in any way, but seems to work.
> 
> 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

> >> 
> >> >> +			};
> >> >> +
> >> >> +			muram@...00 {
> >> >> +				#address-cells = <1>;
> >> >> +				#size-cells = <1>;
> >> >> +				compatible = "fsl,qe-muram", "fsl,cpm-muram";
> >> >> +				ranges = <0x0 0x10000 0x6000>;
> >> >
> >> > Node address but no 'reg' property? I have no idea if this is okay.
> >> > Also compatible (and possibly reg) first.
> >> 
> >> 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.
> 
> Done.
> 
> /Esben
> 


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/