Re: [RFC v1 0/2] Introducing generic SCMI pinctrl driver implementation

[Sudeep Holla]

On Tue, Apr 11, 2023 at 02:27:53PM +0200, Linus Walleij wrote:
> Hi Oleksii,
> 
> thanks for your patches!
> 
> On Fri, Apr 7, 2023 at 12:18 PM Oleksii Moisieiev
> <Oleksii_Moisieiev@xxxxxxxx> wrote:
> 
> > This RFC patch series is intended to introduce the potential generic driver for
> > pin controls over SCMI protocol, provided in the latest beta version of DEN0056 [0].
> >
> > On ARM-based systems, a separate Cortex-M based System Control Processor (SCP)
> > provides control on pins, as well as with power, clocks, reset controllers. In this case,
> > kernel should use one of the possible transports, described in [0] to access SCP and
> > control clocks/power-domains etc. This driver is using SMC transport to communicate with SCP via
> > SCMI protocol and access to the Pin Control Subsystem.
> >
> > The provided driver consists of 2 parts:
> >  - firmware/arm_scmi/pinctrl.c - the SCMI pinctrl protocol inmplementation
> >    responsible for the communication with SCP firmware.
> >
> >  - drivers/pinctrl/pinctrl-scmi.c - pinctrl driver, which is using pinctrl
> >   protocol implementation to access all necessary data.
> 
> TBH this looks so good that I am happy to merge it once you send a non-RFC
> version.
> 
> My main concern would have been the protocol itself, but that was very
> carefully tailored to match what the pin control subsystem needs and
> I am quite happy with it the way it came out: using strings for groups and
> functions.
> 
> The scmi code in patch 1 adds an extra layer of abstraction and a vtable
> that would not have been necessary if all of the code was confined in
> one file in drivers/pinctrl but it is not up to me how the SCMI people
> want to abstract their stuff and there seems to be precedents to do things
> this way.
>

I haven't looked at the code to comment on the details, but in general the
SCMI abstraction was created and used in other kernel subsystems for couple
of reasons:

1. Leave all the protocol specific details like the version and other in
   the abstraction so that the driver remains simple and doesn't have to deal
   with those details.

2. Similar to the version and other details(generic or vendor specific), since
   there is a firmware involved, there might be need for quirks and again
   dealing with those in these SCMI layer is better as we will not have any
   specific compatible say just for pinctrl or dvfs ..etc.

3. Other reason was to allow testing of features that are in the spec and
   firmware but not used by any framework. But I think the way we introduced
   raw scmi interface nullifies it as it allows to bypass any framework and do
   raw SCMI transfers now. But originally the abstraction considered that
   possibility as well.

> I heard that someone wanted to also implement GPIO over SCMI, but
> it is not part of this driver so I guess that will be a future addition.
> It's a good starting point to add GPIO later.
>

Yes, Xilinx people want GPIO for moving away from their custom but similar to
SCMI like interface. There are yet to start exploring details.

--
Regards,
Sudeep