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

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

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

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

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

/Esben