Re: [PATCH 5/9] arm64: dts: renesas: r8a77961: Add VSP device nodes

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Kieran,

On Thu, Sep 10, 2020 at 12:34 PM Kieran Bingham
<kieran.bingham+renesas@ideasonboard.com> wrote:
> On 10/09/2020 10:44, Geert Uytterhoeven wrote:
> > On Mon, Sep 7, 2020 at 5:55 PM Kieran Bingham
> > <kieran.bingham+renesas@ideasonboard.com> wrote:
> >> On 07/09/2020 03:59, Kuninori Morimoto wrote:
> >>> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>>
> >>> This patch adds VSP device nodes for R-Car M3-W+ (r8a77961) SoC.
> >>> This patch is test on R-Car M3-W+ Salvator-XS board.
> >>>
> >>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >>> ---
> >>>  arch/arm64/boot/dts/renesas/r8a77961.dtsi | 55 +++++++++++++++++++++++
> >>>  1 file changed, 55 insertions(+)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/renesas/r8a77961.dtsi b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
> >>> index fe0db11b9cb9..c2a6918ed5e6 100644
> >>> --- a/arch/arm64/boot/dts/renesas/r8a77961.dtsi
> >>> +++ b/arch/arm64/boot/dts/renesas/r8a77961.dtsi
> >>> @@ -2056,6 +2056,61 @@ fcpvd2: fcp@fea37000 {
> >>>                       iommus = <&ipmmu_vi0 10>;
> >>>               };
> >>
> >> The FCP's added are:
> >>
> >>                 fcpf0: fcp@fe950000 {
> >>                 fcpf1: fcp@fe951000 {
> >>                 fcpvb0: fcp@fe96f000 {
> >>                 fcpvb1: fcp@fe92f000 {
> >>                 fcpvi0: fcp@fe9af000 {
> >>                 fcpvi1: fcp@fe9bf000 {
> >>                 fcpvd0: fcp@fea27000 {
> >>                 fcpvd1: fcp@fea2f000 {
> >>                 fcpvd2: fcp@fea37000 {
> >>
> >> So indeed, the first fcpf0 comes before fe960000.
> >>
> >> Do we keep the items grouped by the first occurrence? or sort the nodes
> >> based on address?
> >>
> >> for some reason I thought we were ordering based on address, but I see
> >> other situations where we group too - so I'm confused (and wishing there
> >> was an automatic tool to get the sorting correct without fuss).
> >>
> >> Is there a set policy?
> >
> > For nodes with a unit-address, we usually[*] sort by unit-address, but we keep
> > similar nodes grouped.  Hence I prefer this v1 over v2.
>
> I assume then the groups are sorted by the first entry,
>
> I.e. hypothetically:
>
> fdp@0
> fcp@1
> vsp@2
> fdp@3
> fcp@4
> vsp@5
> cmm@6
> cmm@7
>
> would become
>
> fdp@0
> fdp@3
> fcp@1
> fcp@4
> vsp@2
> vsp@5
> cmm@6
> cmm@7

Exactly.  That's how we (Reneas SoCs) have been (trying to) doing it.
I am aware there are some deviations (e.g. do you keep all 4 possible
SCIF types together (they're all serial@), or do you group them per
type? And some nodes (ipmmu) may be sorted alphabetically by label).

> Has anyone already created any scripting/validation to automate the
> sorting requirements?

Not yet, AFAIK. I've been thinking about doing that several times,
though ;-)

> > [*] Seems like FCP/VSP are interleaved in r8a77990.dsi, doh.
> >
>
> Personally I prefer that - but my opinion doesn't matter here - so as
> long as the rules are defined (or even better, automatically
> enforceable) that's fine.

Indeed.

Perhaps creating rules is something to be handled at a higher level
(i.e. common for all DTS files)?

Summarizing "our" rules:
  1. Nodes without unit-address are sorted alphabetically, by node name,
  2. Nodes with unit-address are sorted numerically, by unit-address,
       a. Nodes of the same type are grouped together, i.e. the whole
          group is sorted w.r.t. to other nodes/groups based on the
          unit-address of the first member of the group.
  3. Anchors are sorted alphabetically, but anchor name.

Do they make sense?

> >>> +             vspb: vsp@fe960000 {
> >>> +                     compatible = "renesas,vsp2";
> >>> +                     reg = <0 0xfe960000 0 0x8000>;
> >>> +                     interrupts = <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>;
> >>> +                     clocks = <&cpg CPG_MOD 626>;
> >>> +                     power-domains = <&sysc R8A77961_PD_A3VC>;
> >>> +                     resets = <&cpg 626>;
> >>> +
> >>> +                     renesas,fcp = <&fcpvb0>;
> >>> +             };

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds