Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210825205041.927788-1-vladimir.oltean@nxp.com>
 <20210825205041.927788-3-vladimir.oltean@nxp.com> <CAK8P3a1oDeU-S5dLqKTT3YFvGREvNt_a=PTkVoDhUJYquJGePQ@mail.gmail.com>
 <20210825220023.rqskspy2usvleoqr@skbuf>
In-Reply-To: <20210825220023.rqskspy2usvleoqr@skbuf>
From:   Arnd Bergmann <arnd@arndb.de>
Date:   Thu, 26 Aug 2021 11:24:24 +0200
Message-ID: <CAK8P3a3FnsWbGRU7BNc8uwt0nFAHa7K4c+qpoZixwdW9kihC5w@mail.gmail.com>
Subject: Re: [PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for
 some devices
To:     Vladimir Oltean <olteanv@gmail.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Vladimir Oltean <vladimir.oltean@nxp.com>,
        Mark Brown <broonie@kernel.org>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        Lee Jones <lee.jones@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Hou Zhiqiang <Zhiqiang.Hou@nxp.com>,
        Biwen Li <biwen.li@nxp.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Aug 26, 2021 at 12:01 AM Vladimir Oltean <olteanv@gmail.com> wrote:
> On Wed, Aug 25, 2021 at 11:24:52PM +0200, Arnd Bergmann wrote:
>
> > Are there any other users of the syscon?
>
> Not that I can see, but that doesn't make the fact that it uses a syscon a bad decision.
>
> For context, Layerscape devices have a "Misc" / "And Others" memory region
> called "Supplemental Configuration Unit" (SCFG) which "provides the
> chip-specific configuration and status registers for the device. It is the
> chip-defined module for extending the device configuration unit (DCFG) module."
> to quote the documentation.
>
> The ls-extirq file is a driver around _a_single_register_ of SCFG. SCFG
> provides an option of reversing the interrupt polarity of the external IRQ
> pins: make them active-low instead of active-high, or rising instead of
> falling.
>
> The reason for the existence of the driver is that we got some pushback during
> device tree submission: while we could describe in the device tree an interrupt
> as "active-high" and going straight to the GIC, in reality that interrupt is
> "active-low" but inverted by the SCFG (the inverted is enabled by default).
> Additionally, the GIC cannot process active-low interrupts in the first place
> AFAIR, which is why an inverter exists in front of it.
>
> Some other SCFG registers are (at least on LS1021A):
>
> Deep Sleep Control Register
> eTSEC Clock Deep Sleep Control Register (eTSEC is our Ethernet controller)
> Pixel Clock Control Register
> PCIe PM Write Control Register
> PCIe PM Read Control Register
> USB3 parameter 1 control register
> ETSEC MAC1 ICID
> SATA ICID
> QuadSPI configuration
> Endianness Control Register
> Snoop configuration
> Interrupt Polarity <- this is the register controlled by ls-extirq
> etc etc.
>
> Also, even if you were to convince me that we shouldn't use a syscon, I feel
> that the implication (change the device trees for 7 SoCs) just to solve a
> kernel splat would be like hitting a nail with an atomic bomb. I'm not going to
> do it.

I was not suggesting changing the DT files. The way we describe syscon
devices is generally meant to allow replacing them with a custom driver
as an implementation detail of the OS, you just have a driver that binds
against the more specific compatible string as opposed to the generic
compatible="syscon" check, and you replace all
syscon_regmap_lookup_by_phandle() calls with direct function calls
into exported symbols from that driver that perform high-level functions.

In this particular case, I think a high-level interface from a drviers/soc/
driver works just as well as the syscon method if there was raw_spinlock
requirement, but with the irqchip driver needing the regmap, the custom
driver would a better interface.

        Arnd