Re: [RFC] scmi: pinctrl: support i.MX9

[AKASHI Takahiro <takahiro.akashi@xxxxxxxxxx>]

On Wed, Aug 30, 2023 at 02:37:48PM +0100, Cristian Marussi wrote:
> On Wed, Aug 30, 2023 at 12:48:37PM +0000, Peng Fan wrote:
> > Hi Cristian,
> > 
> 
> Hi,
> 
> > > 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@xxxxxxx> 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.
> > 
> 
> Yes, I know, I pointed out on his series that the protocol has still to
> be fixed to be aligned with the latest BETA2 spec (we changed the spec
> on the fly while he was already posting indeed..)
> 
> > > 
> > > 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.
> 
> Good.
> 
> > 
> > > 
> > > 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.
> >
> 
> Yes, but you are basically using some exported symbol to parse the DT in
> your way and then you do not use anything of the various
> functions/groups stuff...you just leverage some of the probing stuff and
> then issue you OEM Type configs....I mean most of the picntrl-scmi
> driver would be unused anyway in this scenario.
> 
> > > 
> > > 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.
> > 
> 
> Ok here I lost you.
> 
> The protocol ID 0x19 is bound to the protocol layer and identifies the
> standard Pinctrl protocol: usually you use a 0x99 to define and describe
> you own specific NEW vendor protocol, BUT here you are saying you are fine to
> use std Pinctrl spec AND the protocol operations as exposed in pinctrl.c, so
> I dont see why you should use a new vendor protocol_id to basically
> expose the same operations. (and I also dont see how you can do that
> without hacks in the current codebase)
> 
> You CAN have multiple SCMI drivers using the same protocol at the same
> time (even more than one protocol at the same time), even though we try
> to avoid it if there are no good reason to have more than one driver, there
> is nothing in the spec or in the current SCMI platform or agent stacks that
> inhibits such scenario (and I use iot heavily for my offline testing
> indeed.)
> 
> Look at:
> 
>  - drivers/hwmon/scmi-hwmon 
>  - drivers/iio/common/scmi_sensors/scmi_iio.c
> 
> and you'll see that these 2 drivers uses the same SENSOR protocol, just for
> different sensor types so they do not interfere one with each other.

Then, how are those two devices identified in a device tree?
That is the point in Peng's case and why he wants to have a dedicated
protocol id (I don't agree to this, though.)
If we follow Cristian's idea, we may want to have two dt nodes, say
pinctrl-scmi-generic and pinctrl-scmi-imx, as phandles for other device
nodes to refer to pins, respectively.
I think there is currently no mechanism (or binding?) to allow this
except adding a protocol id.

-Takahiro Akashi


> What happens is that the first driver using a protocol causes its
> protocol_init to be called once for all.
> 
> This should work flawlessly like this, if this is not the case for some
> reason, this will have to be fixed in the protocol implementation: you
> are supposed to be able to grab the same protocol from different
> drivers without any issue.
> 
> I agree that you have to be careful not to share the same pins across 2
> different drivers using the same Pinctrl driver, but even if both driver
> are compiled in, nothing is really happening until the related DT
> binding are parsed, and so unless you mismatch your DT and assign same
> pins to both the Generic SCMI Pinctrl and to the IMX SCMI Pinctrl I dont
> see how they can interfere. You could indeed, have a set of pins managed
> by your custom IMX driver and one distinct other set of pins handled by
> the SCMI Generic driver by Oleksii, both magically handled by the same
> SCMI Server backend :P !
> 
> BUT to be on the safe side you could anyway force a conflict in Kconfig
> to mutually exclude one driver when the other is built and vice-versa.
> 
> Am I missing something ? Why would you need a new vendor ID to define a
> new protocol without not really having any new protocol ?
> 
> Thanks,
> Cristian