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/
