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

[Alexander Stein]

Hi Esben,

Am Freitag, 31. Mai 2024, 14:20:02 CEST schrieb Esben Haabendal:
> Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> writes:
> 
> > Would you consider current converting into YAML format?
> 
> You mean converting Documentation/devicetree/bindings/soc/fsl/qe.txt and
> Documentation/devicetree/bindings/soc/fsl/qe/*.txt into YAML?
> 
> 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.
> >> 
> >> Signed-off-by: Esben Haabendal <esben@xxxxxxxxxx>
> >> ---
> >>  arch/arm/boot/dts/nxp/ls/ls1021a.dtsi | 51 +++++++++++++++++++++++++++++++++++
> >>  1 file changed, 51 insertions(+)
> >> 
> >> 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 = <2>;
> >>  		};
> >>  
> >> +		uqe: uqe@2400000 {
> >> +			#address-cells = <1>;
> >> +			#size-cells = <1>;
> >> +			device_type = "qe";
> >> +			compatible = "fsl,qe", "simple-bus";
> >> +			ranges = <0x0 0x0 0x2400000 0x40000>;
> >> +			reg = <0x0 0x2400000 0x0 0x480>;
> >
> > Properties please in this order:
> > * compatible
> > * reg
> > * #address-cells
> > * #size-cells
> > * ranges
> > * device_type
> 
> Fixing.
> 
> >> +			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.

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

> 
> >> +			qeic: qeic@80 {
> >> +				compatible = "fsl,qe-ic";
> >> +				reg = <0x80 0x80>;
> >> +				#address-cells = <0>;
> >> +				interrupt-controller;
> >> +				#interrupt-cells = <1>;
> >> +				interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH
> >> +					      GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
> >> +			};
> >> +
> >> +			ucc@2000 {
> >> +				cell-index = <1>;
> >> +				reg = <0x2000 0x200>;
> >> +				interrupts = <32>;
> >> +				interrupt-parent = <&qeic>;
> >
> > Move cell-index to last position.
> 
> Done.
> 
> >> +			};
> >> +
> >> +			ucc@2200 {
> >> +				cell-index = <3>;
> >> +				reg = <0x2200 0x200>;
> >> +				interrupts = <34>;
> >> +				interrupt-parent = <&qeic>;
> >
> > Same here.
> 
> Done.
> 
> >> +			};
> >> +
> >> +			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.

Thanks and best regards,
Alexander
-- 
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/