Re: [PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for some devices

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Vladimir Oltean <olteanv@gmail.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>, "Z.Q. Hou" <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>
Subject: Re: [PATCH 2/2] mfd: syscon: request a regmap with raw spinlocks for some devices
Date: Thu, 26 Aug 2021 13:57:26 +0000	[thread overview]
Message-ID: <20210826135725.lrn2afoekxzzvtbf@skbuf> (raw)
In-Reply-To: <CAK8P3a3FnsWbGRU7BNc8uwt0nFAHa7K4c+qpoZixwdW9kihC5w@mail.gmail.com>

On Thu, Aug 26, 2021 at 11:24:24AM +0200, Arnd Bergmann wrote:
> 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.

So basically you want me to create a platform driver under drivers/soc
which probes on:
	"fsl,lx2160a-isc"
	"fsl,ls2080a-isc"
	"fsl,ls2080a-isc"
	"fsl,ls1088a-isc"
	"fsl,ls1043a-scfg"
	"fsl,ls1046a-scfg"
	"fsl,ls1021a-scfg"
and does the same thing as syscon, but sets "syscon_config.use_raw_spinlock = true;"
and that is the only difference?

By the way, how does syscon probe its children exactly? And how would
this driver probe its children?