Date:   Tue, 14 Dec 2021 08:51:47 +0000
Message-ID: <87ilvrk1r0.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Vladimir Oltean <vladimir.oltean@nxp.com>
Cc:     devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Rob Herring <robh+dt@kernel.org>,
        Shawn Guo <shawnguo@kernel.org>, Li Yang <leoyang.li@nxp.com>,
        Biwen Li <biwen.li@nxp.com>,
        Zhiqiang Hou <Zhiqiang.Hou@nxp.com>,
        Kurt Kanzenbach <kurt@linutronix.de>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>
Subject: Re: [RFC PATCH devicetree 00/10] Do something about ls-extirq interrupt-map breakage
In-Reply-To: <20211214013800.2703568-1-vladimir.oltean@nxp.com>
References: <20211214013800.2703568-1-vladimir.oltean@nxp.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Tue, 14 Dec 2021 01:37:50 +0000,
Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
> 
> Currently the ls-extirq driver's use of the "interrupt-map" property is
> double-broken:
> - once by Rob Herring's commit 869f0ec048dc ("arm64: dts: freescale: Fix
>   'interrupt-map' parent address cells")
> - twice by Marc Zyngier's commit 041284181226 ("of/irq: Allow matching
>   of an interrupt-map local to an interrupt controller"), later revised,
>   not very elegantly, through commit de4adddcbcc2 ("of/irq: Add a quirk

Elegance is, I'm afraid to say, bloody overrated when dealing with
this sort of crap.


>   for controllers with their own definition of interrupt-map"). So this
>   part works but we're on an offender list.

Define 'part works'. Either it does, or it doesn't. There is no middle
ground here.

> 
> Mark suggests that the problem may lie with the ls-extirq driver, and
> its interpretation of the "interrupt-map" property, to be exact.

s/Mark/Marc/, unless you are talking about someone else (who?).

> 
> This set of changes attempts to make the problem smaller by using a
> vendor-specific name for the property, and reverts Rob's patch because
> similarity with "interrupt-map" isn't actually a desirable feature after
> all, it seems.
> 
> Vladimir Oltean (10):
>   irqchip/ls-extirq: rename "interrupt-map" OF property to
>     "fsl,extirq-map"
>   Revert "arm64: dts: freescale: Fix 'interrupt-map' parent address
>     cells"
>   dt-bindings: ls-extirq: replace "interrupt-map" documentation with
>     "fsl,extirq-map"
>   arm64: dts: ls1043a: rename the "interrupt-map" of the extirq node to
>     "fsl,extirq-map"
>   arm64: dts: ls1046a: rename the "interrupt-map" of the extirq node to
>     "fsl,extirq-map"
>   arm64: dts: ls1088a: rename the "interrupt-map" of the extirq node to
>     "fsl,extirq-map"
>   arm64: dts: ls208xa: rename the "interrupt-map" of the extirq node to
>     "fsl,extirq-map"
>   arm64: dts: lx2160a: rename the "interrupt-map" of the extirq node to
>     "fsl,extirq-map"
>   ARM: dts: ls1021a: rename the "interrupt-map" of the extirq node to
>     "fsl,extirq-map"
>   dt-bindings: ls-extirq: add a YAML schema for the validator
> 
>  .../interrupt-controller/fsl,ls-extirq.txt    |  53 ---------
>  .../interrupt-controller/fsl,ls-extirq.yaml   | 110 ++++++++++++++++++
>  arch/arm/boot/dts/ls1021a.dtsi                |   3 +-
>  .../arm64/boot/dts/freescale/fsl-ls1043a.dtsi |   3 +-
>  .../arm64/boot/dts/freescale/fsl-ls1046a.dtsi |   3 +-
>  .../arm64/boot/dts/freescale/fsl-ls1088a.dtsi |  27 +++--
>  .../arm64/boot/dts/freescale/fsl-ls208xa.dtsi |  27 +++--
>  .../arm64/boot/dts/freescale/fsl-lx2160a.dtsi |  27 +++--
>  drivers/irqchip/irq-ls-extirq.c               |  12 +-
>  9 files changed, 161 insertions(+), 104 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.txt
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/fsl,ls-extirq.yaml

This is totally pointless. These machines have been in the wild for
years, and existing DTs will be there *forever*. The very notion of
'backporting' DT changes is totally ludicrous when it is some firmware
(ATF, u-boot, or something else *that isn't under your control*) that
provides the DT. It also breaks backward compatibility (old kernel
with new DT), which is just as important. Why do you think I went the
elegance-deprived route and added a quirk?

So no, I'm not taking the irqchip changes, as most of this churn
serves no purpose. The revert of 869f0ec048dc is the only thing that
makes some sense.

	M.

-- 
Without deviation from the norm, progress is not possible.