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

  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.

Configuration:
The scmi-pinctrl driver can be configured using DT bindings.
For example:
/ {
	cpu_scp_shm: scp-shmem@0x53FF0000 {
		compatible = "arm,scmi-shmem";
		reg = <0x0 0x53FF0000 0x0 0x1000>;
	};

	firmware {
		scmi {
			compatible = "arm,scmi-smc";
			arm,smc-id = <0x82000002>;
			shmem = <&cpu_scp_shm>;
			#address-cells = <1>;
			#size-cells = <0>;

			scmi_pinctrl: protocol@19 {
				reg = <0x18>;
				#pinctrl-cells = <0>;

				i2c2_pins: i2c2 {
					groups = "i2c2_a";
					function = "i2c2";
				};
			};
		};
	};
};

&pfc {
	/delete-node/i2c2;
};

So basically, it's enough to move pfc subnode, which configures pin group that should work through
SCMI protocol to scmi_pinctrl node. The current driver implementation is using generic pinctrl dt_node
format.

I've tested this driver on the Renesas H3ULCB Kingfisher board with pinctrl driver ported to the
Arm-trusted-firmware. Unfortunately, not all hardware was possible to test because the Renesas
pinctrl driver has gaps in pins and groups numeration, when Spec [0] requires pins, groups and
functions numerations to be 0..n without gaps.

This implementation still reqires some features, such as gpio support, but I've posted it as RFC to
start the discussion regarding the driver format.

[0] https://developer.arm.com/documentation/den0056/latest

Oleksii Moisieiev (2):
  scmi: Introduce pinctrl SCMI protocol driver
  pinctrl: Implementation of the generic scmi-pinctrl driver

 MAINTAINERS                         |   7 +
 drivers/firmware/arm_scmi/Makefile  |   2 +-
 drivers/firmware/arm_scmi/common.h  |   1 +
 drivers/firmware/arm_scmi/driver.c  |   3 +
 drivers/firmware/arm_scmi/pinctrl.c | 905 ++++++++++++++++++++++++++++
 drivers/pinctrl/Kconfig             |   9 +
 drivers/pinctrl/Makefile            |   1 +
 drivers/pinctrl/pinctrl-scmi.c      | 555 +++++++++++++++++
 include/linux/scmi_protocol.h       |  58 +-
 9 files changed, 1539 insertions(+), 2 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/pinctrl.c
 create mode 100644 drivers/pinctrl/pinctrl-scmi.c
  

Hi Oleksii,

thanks for your patches!

On Fri, Apr 7, 2023 at 12:18 PM Oleksii Moisieiev
<Oleksii_Moisieiev@epam.com> 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 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.

Yours,
Linus Walleij
  

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@epam.com> 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
  

On Fri, Apr 07, 2023 at 10:18:26AM +0000, Oleksii Moisieiev 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].
> 

Hi Oleksii,

a few general remarks here down below and more specific comments will
come inline in the remaining patches of the series.

CC'ing also a few more people possibly interested in your series.

> 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.
> 

As discussed offline, the patches you posted to add support for the new
SCMI pinctrl protocol and the related SCMI pinctrl driver are using the
old SCMI API (include/linux/scmi_protocol.h) that changed significantly
since v5.13, so at first they need to be ported to current mainline API.

You can look on the latest v6.3-rc  at:

	drivers/firmware/arm_scmi/power.c
 	drivers/firmware/arm_scmi/scmi_pm_domain.c

for a simple example of a core protocol and related driver using the new API.

On the protocol side you can ignore really the part related to "scmi_protocol_events"
that you find there since it is related to notifications and there are no notifs in
SCMI Pinctrl as of now.

In a nutshell the protocol layer now receives a protocol handle (instead
of the instance handle) during protocol_init and uses that to build/send
messages; it has to be used also to store any protocol private data
with ph->set/get_priv() (no more direct access to handle->privs)

That same protocol handle is then used by the SCMI driver users during
tehir probes, so, in your pinctrl SCMI driver probe, you should do something like
this early on:

	pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph);

store the 'ph' somewhere and and then use it all over:

	pinctrl_ops->set_mux(ph, selector, group);

Beside the API a few helpers has been added too (ph->hops) that can be
re-used by protocol code to implement straight away some common SCMI machinery
like the extended_name_get and the handling of multi-part replies
(like in PINCTRL_LIST_ASSOCIATIONS)...I think you can ignore these ph->hops and
just keep your original code, I'll take care to port these functionalities to
the common helpers later on top of your series if it is fine for you...
(also because I think at least a small modification in the core helpers will be
needed to support PINCTRL usage since it deviates a bit from existent protos...:P)

> Configuration:
> The scmi-pinctrl driver can be configured using DT bindings.
> For example:
> / {
> 	cpu_scp_shm: scp-shmem@0x53FF0000 {
> 		compatible = "arm,scmi-shmem";
> 		reg = <0x0 0x53FF0000 0x0 0x1000>;
> 	};
> 
> 	firmware {
> 		scmi {
> 			compatible = "arm,scmi-smc";
> 			arm,smc-id = <0x82000002>;
> 			shmem = <&cpu_scp_shm>;
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			scmi_pinctrl: protocol@19 {
> 				reg = <0x18>;
> 				#pinctrl-cells = <0>;
> 
> 				i2c2_pins: i2c2 {
> 					groups = "i2c2_a";
> 					function = "i2c2";
> 				};
> 			};
> 		};
> 	};
> };
> 

These will need a proper formal explanation in DT bindings at:

	Documentation/devicetree/bindings/firmware/arm,scmi.yaml

to highlight the usage of "#pinctrl-cells" and whatever else is needed
to be documented with related links to existing reused bindings is any.
(and CC that patch to the proper DT maintainers and MLs... IOW look at
what get_maintanel.pl says)

> &pfc {
> 	/delete-node/i2c2;
> };
> 
> So basically, it's enough to move pfc subnode, which configures pin group that should work through
> SCMI protocol to scmi_pinctrl node. The current driver implementation is using generic pinctrl dt_node
> format.
> 
> I've tested this driver on the Renesas H3ULCB Kingfisher board with pinctrl driver ported to the
> Arm-trusted-firmware. Unfortunately, not all hardware was possible to test because the Renesas
> pinctrl driver has gaps in pins and groups numeration, when Spec [0] requires pins, groups and
> functions numerations to be 0..n without gaps.
> 
> This implementation still reqires some features, such as gpio support, but I've posted it as RFC to
> start the discussion regarding the driver format.
> 
> [0] https://developer.arm.com/documentation/den0056/latest
> 

Thanks for this,
Cristian
  

Hi,

On 4/7/23 12:18, Oleksii Moisieiev 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.
> 
> Configuration:
> The scmi-pinctrl driver can be configured using DT bindings.
> For example:
> / {
> 	cpu_scp_shm: scp-shmem@0x53FF0000 {
> 		compatible = "arm,scmi-shmem";
> 		reg = <0x0 0x53FF0000 0x0 0x1000>;
> 	};
> 
> 	firmware {
> 		scmi {
> 			compatible = "arm,scmi-smc";
> 			arm,smc-id = <0x82000002>;
> 			shmem = <&cpu_scp_shm>;
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			scmi_pinctrl: protocol@19 {
> 				reg = <0x18>;
> 				#pinctrl-cells = <0>;
> 
> 				i2c2_pins: i2c2 {
> 					groups = "i2c2_a";
> 					function = "i2c2";
> 				};
> 			};
> 		};
> 	};
> };

Spec itself has as the part of pinctrl also gpio support.
Here the example there is no gpio-cells property that's why I am curious if you 
plan to also add it here.

Thanks,
Michal