Received-SPF: pass (google.com: domain of linux-kernel+bounces-196763-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: Esben Haabendal <esben@geanix.com>
To: Alexander Stein <alexander.stein@ew.tq-group.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
In-Reply-To: <3380831.44csPzL39Z@steina-w> (Alexander Stein's message of "Fri,
	31 May 2024 08:32:44 +0200")
References: <20240530-arm-ls1021a-qe-dts-v1-1-2eda23bdf8c5@geanix.com>
	<3380831.44csPzL39Z@steina-w>
Date: Fri, 31 May 2024 14:20:02 +0200
Message-ID: <87frtynpfx.fsf@geanix.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain

Alexander Stein <alexander.stein@ew.tq-group.com> 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@geanix.com>
>> ---
>>  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