Message-ID: <DU0PR04MB941726AF86EEB0BBB55B9A2A88E6A@DU0PR04MB9417.eurprd04.prod.outlook.com>
Date:   Wed, 30 Aug 2023 12:48:37 +0000
From:   Peng Fan <peng.fan@....com>
To:     Cristian Marussi <cristian.marussi@....com>
CC:     Linus Walleij <linus.walleij@...aro.org>,
        "Peng Fan (OSS)" <peng.fan@....nxp.com>,
        "oleksii_moisieiev@...m.com" <oleksii_moisieiev@...m.com>,
        "sudeep.holla@....com" <sudeep.holla@....com>,
        Aisheng Dong <aisheng.dong@....com>,
        "festevam@...il.com" <festevam@...il.com>,
        Jacky Bai <ping.bai@....com>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "kernel@...gutronix.de" <kernel@...gutronix.de>,
        dl-linux-imx <linux-imx@....com>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>
Subject: RE: [RFC] scmi: pinctrl: support i.MX9

Hi Cristian,

> Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> 
> On Fri, Aug 25, 2023 at 08:43:38AM +0000, Peng Fan wrote:
> > > Subject: Re: [RFC] scmi: pinctrl: support i.MX9
> > >
> > > On Thu, Aug 24, 2023 at 2:47 PM Peng Fan <peng.fan@....com> wrote:
> > > > Me:
> 
> Hi Peng,
> 
> > >
> > > >> it is merely making things more complex and also slower
> > > > > bymaking the registers only accessible from this SCMI link.
> > > >
> > > > This is for safety reason, the pinctrl hardware must be handled by
> > > > a system manager entity. So mmio direct access not allowed from
> > > > Cortex-A side.
> > >
> > > Yeah I understood as much. But I don't think that the firmware is
> > > really filtering any of the access, it will just poke into any
> > > pinctrl register as instructed anyway so what's the point. Just looks like a
> layer of indirection.
> >
> > No, the firmware has a check on whether a pin is allowed to be
> > configured by the agent that wanna to configure the pin.
> >
> > > But I'm not your system manager, so it's not my decision.
> > >
> > > > The SCMI firmware is very straightforward, there is no group or
> > > > function.
> > > >
> > > > It just accepts the format as this:
> > > > MUX_TYPE, MUX VALUE, CONF_TYPE, CONF_VAL, DAISY_TYPE, DAISY
> ID,
> > > > DAISY_CFG, DAISY_VALUE.
> > > >
> > > > Similar as linux MMIO format.
> > > >
> > > > Our i.MX95 platform will support two settings, one with SCMI
> > > > firmware, one without SCMI. These two settings will share the same
> > > > pinctrl header file.
> > > >
> > > > And to simplify the scmi firmware design(anyway I am not owner of
> > > > the firmware), to make pinctrl header shared w/o scmi, we take the
> > > > current in-upstream freescale imx binding format.
> > >
> > > The SCMI people will have to state their position on this.
> > > Like what they consider conformance and what extensions are allowed.
> > > This is more a standardization question than an implementation
> > > question so it's not really my turf.
> >
> > The i.MX95 SCMI firmware uses OEM extension type. So I just follow
> > what the firmware did and support it in linux. Anyway let's wait
> > Sudeep's reply.
> >
> 
> So my unsderstanding on this matter as of now is that:
> 
> 1. the current SCMI Pinctrl specification can support your usecase by using
>    OEM Types and multiple pins/values CONFIG_GET/SET commands

Yes, based on the Oleksii patchset with my local multiple configs support.

> 
> 2. the Kernel SCMI protocol layer (driver/firmware/arm_scmi/pinctrl.c)
>    is equally fine and can support your usecase, AFTER Oleksii fixes it to
>    align it to the latest v3.2-BETA2 specification changes.
>    IOW, this means that, using the SCMI Pinctrl protocol operations
>    exposed in scmi_protocol.h, from somewhere, you are able to properly
>    configure multiple pins/values with your specific OEM types.

Yes.

> 
> 3. The SCMI Pinctrl driver (by Oleksii) built on top of the pinctrl protocol
>    operations is instead NOT suitable for your usecase since it uses the Linux
>    Generic Pinconf and IMX does not make use of it, and instead IMX has
>    its own bindings and related parsing logic.

Yes.

> 
> Am I right ?

You are right.

> 
> If this is the case, I would NOT try to abuse the current SCMI Pinctrl Generic
> driver (by Oleksii) by throwing into it a bunch of IMX specific DT parsing,
> also because you'll end-up NOT using most of the generic SCMI Pinctrl driver
> but just reusing a bit of the probe (customized with your own DT maps
> parsing)

Only DT map to parse the dts and map to config array. Others are same,
so need to export some symbols for pinctrl-scmi-imx.c driver if build imx
scmi driver.

> 
> Instead, given that the spec[1.] and the protocol layer[2.] are fine for your
> use case and you indeed have already a custom way to parse your DT
> mappings, I would say that you could just write your own custom SCMI
> driver ( ? pinctrl-imx-scmi), distinct and much more simple than the generic
> one, that does its own IMX DT parsing and calls just the SCMI protocol
> operations that it needs in the way that your platform expects: so basically
> another Pinctrl SCMI driver that does not use the generic pinconf DT
> configuration BUT DO USE the underlying SCMI Pinctrl protocol (via its
> exposed protocol operations...)

I am ok with this approach, but I need use the other ID, saying 0x99, not 0x19,
because 0x19 will bind with the pinctrl-scmi.c driver, I could not reuse
this ID for i.MX pinctrl-scmi-imx driver. Otherwise there will be issue if both
driver are built in kernel image.

> 
> Not sure what Sudeep thinks about supporting multiple SCMI driver for the
> same protocol (we did it already for Sensors hwmon && iio), and if this
> approach won't need some sort of mutual exclusion mechanism in Kconfig
> to avoid loading both the generic and the custom IMX (even though they
> should be able to co-exist from the SCMI kernel/fw stack pint of view, as
> long as you dont mess-up the DTs and mixup generic pins with custom IMX
> pins...)
> 
> Instead, adding an IMX-custom extension to what it was supposed to be a
> generic driver (as you propose) seems to me like a stretch of the generic
> Pinctrl driver that is not really worth, since you'll end up polluting the
> generic driver with some highly custom and specific IMX bits. while really
> NOT reusing so much of the generic driver at all.

If Sudeep agree, I will follow your suggestion and post a patch for review, later.

Thanks,
Peng.
> 
> Thanks,
> Cristian

Powered by blists - more mailing lists