Re: [RFC v1 1/2] scmi: Introduce pinctrl SCMI protocol driver

[Oleksii Moisieiev]

Hi Cristian,

On 20.04.23 20:05, Cristian Marussi wrote:
> On Wed, Apr 12, 2023 at 11:04:05PM +0100, Cristian Marussi wrote:
>> On Fri, Apr 07, 2023 at 10:18:27AM +0000, Oleksii Moisieiev wrote:
>>> Implementation of the SCMI client driver, which implements
>>> PINCTRL_PROTOCOL. This protocol has ID 19 and is described
>>> in the latest DEN0056 document.
>> Hi,
>>
> Hi Oleksii,
>
> one more thing that I missed in my previous review down below.
>
>>> This protocol is part of the feature that was designed to
>>> separate the pinctrl subsystem from the SCP firmware.
>>> The idea is to separate communication of the pin control
>>> subsystem with the hardware to SCP firmware
>>> (or a similar system, such as ATF), which provides an interface
>>> to give the OS ability to control the hardware through SCMI protocol.
>>> This is a generic driver that implements SCMI protocol,
>>> independent of the platform type.
>>>
>>> DEN0056 document:
>>> https://urldefense.com/v3/__https://developer.arm.com/documentation/den0056/latest__;!!GF_29dbcQIUBPA!0kMLQ5c3tKsMfWCqTKHp6eolY3sTZlyKmAD7B7pbiSESABUUoBzmhgrYdDgWGC_g0vgLE4zwrS4ppeTOD8KizP9fIeJkpg$ [developer[.]arm[.]com]
>>>
> [snip]
>
>>> +static int scmi_pinctrl_request_config(const struct scmi_handle *handle,
>>> +				       u32 selector,
>>> +				       enum scmi_pinctrl_selector_type type,
>>> +				       u32 *config)
>>> +{
>>> +	struct scmi_xfer *t;
>>> +	struct scmi_conf_tx *tx;
>>> +	__le32 *packed_config;
>>> +	u32 attributes = 0;
>>> +	int ret;
>>> +
>>> +	if (!handle || !config || type == FUNCTION_TYPE)
>>> +		return -EINVAL;
>>> +
>>> +	ret = scmi_pinctrl_validate_id(handle, selector, type);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	ret = scmi_xfer_get_init(handle, PINCTRL_CONFIG_GET,
>>> +				 SCMI_PROTOCOL_PINCTRL,
>>> +				 sizeof(*tx), sizeof(*packed_config), &t);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	tx = t->tx.buf;
>>> +	packed_config = t->rx.buf;
>>> +	tx->identifier = cpu_to_le32(selector);
>>> +	attributes = SET_TYPE_BITS(attributes, type);
>>> +	attributes = SET_CONFIG(attributes, *config);
>>> +
> Looking at scmi_conf_tx and these pinctrl get/set functions, you do not
> seem to consider the ConfigType field in the SCMI related messages, so
> basically using always the Default 0 Type, and as a consequence you dont
> either expose any way to choose a Different type in the related SCMI
> protocol ops; I imagine this is because the pinctrl driver currently using
> this protocol, at the end, does not need any of the other available types
> (as in Table 23 of the spec).
>
I'm not sure I've understood your point. Pinctrl subsystem pass config 
in so-called Packed format. So this means that config is both input and 
output parameter. Packed format means that u32 config has both config id 
and config value packed inside.

So I receive packed config with both id and value on config_set call and 
then just send it over SCMI, expecting error from server if config is 
invalid. On config_get call I receive config parameter with only id 
packed inside, them pass it to the server and receive packed_config with 
both id and value, which is ready to be returned to the subsystem.

> This is fine for pinctrl driver as it is now, BUT the SCMI pinctrl
> protocol implementation in the core SCMI stack and its related
> protocol_operations as exposed in scmi_protocol.h should be generic
> enough to serve any possible user of the SCMI pinctrl protocol (and there
> is already a request to extend/amend the spec somehow to send multiple pin
> setup of different types in one go as you may have seen), so I'd say it's
> better if you add also a ConfigType param to the get/set_config scmi_pinctrl_ops
> and expose the whole ConfigType enums (Table23) in scmi_protocol.h (like we do for
> sensor classes on scmi_protocol.h) to address this; the pinctrl driver
> can then anyway call such new protocol_ops with a Default type, but at
> least the SCMI protocol_ops interface will remain generic and could be
> reused iin other scenarios.
>
> This is equally true for all the other protocol messages (should I have
> miss something else for now...I'll review again you next V2 anyway).
>
> Thanks,
> Cristian
>