Hi Esben,

Am Freitag, 31. Mai 2024, 16:40:29 CEST schrieb Esben Haabendal:

Alexander Stein [off-list ref] writes:

quoted

Hi Esben,

Am Freitag, 31. Mai 2024, 14:20:02 CEST schrieb Esben Haabendal:

quoted

Alexander Stein [off-list ref] writes:

quoted

quoted

+			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.

quoted

quoted

quoted

quoted

+			fsl,qe-num-riscs = <1>;
+			fsl,qe-num-snums = <28>;

Current bindings defines:

quoted

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

quoted

quoted

quoted

quoted

+			};
+
+			muram@10000 {
+				#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/