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

[Oleksii Moisieiev <Oleksii_Moisieiev@xxxxxxxx>]

Hi Cristian,

On 20.04.23 21:47, Cristian Marussi wrote:
> On Thu, Apr 20, 2023 at 05:23:05PM +0000, Oleksii Moisieiev wrote:
>> Hi Cristian,
>>
> Hi,
>
>> 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.
>>
> Sorry I was meant to make the above comment on the PINCTRL_SET path but
> I messed up and commented on the GET path....my bad.
>
> Anyway even considering the packed format and looking at the PINCTRL_SET
> function scmi_pinctrl_apply_config I dont understand how this works.
>
>> 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
> This is where I dont understand: you receive a packed 32bit config from
> pinctrl subsystem via ops->set_config together with the pin_id and in turn
> this calls down into this apply_config where you build the packet:
>
>
>> +	ret = scmi_pinctrl_validate_id(handle, selector, type);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = scmi_xfer_get_init(handle, PINCTRL_CONFIG_SET,
>> +				 SCMI_PROTOCOL_PINCTRL,
>> +				 sizeof(*tx), 0, &t);
>> +	if (ret)
>> +		return ret;
>> +
>> +	tx = t->tx.buf;
>> +	tx->identifier = cpu_to_le32(selector);
> you set the pin number
>
>> +	attributes = SET_TYPE_BITS(attributes, type);
> then set the Selector type as PIN_TYPE in the attributes bits 9:8
>
>
>> +	attributes = SET_CONFIG(attributes, config);
> And here you set the PackedFormat received from pinctrl into the
> bits 7:0 of attributes...(...BUT you then assign back to attributes
> which is 32 bit so I think this is another bug because this way you
> clear any bit just set above with SET_TYPE_BITS...but this is not the
> point now...lets ignore this... ad you should anyway use bitfield.h in V2)
>
> ...so this attributes now, as you explained me, include both the selector
> type (PIN vs GROUP) in bits 9:8 (bugs apart) and then, as a packed format
> the ConfigType and the Value to set, both packed into the bits 7:0
>
>> +	tx->attributes = cpu_to_le32(attributes);
>> +
> You convert to proper endianity and
>
>> +	ret = scmi_do_xfer(handle, t);
>> +
> send straight to the SCMI server as a payload for PINCTRL_SET....so you
> send basically the pin identifier in this case (u32) AND the u32 with
> the attributes which includes the Selector PIN vs GROUP and the
> ConfigType ..this last as a packed thing including also the value...
> (so this could contain....looking at Table23 as an example:
>     Bias-pull-up (4) + a value in Ohms)
>
> ....BUT from the spec v3.2-BETA regarding PINCTRL_SET (4.11.2.7), you are
> supposed to send one more u32 field "config_value" as the payload of
> PINCTRL_SET (page 172) wth such field beaing the effectively carried
> value to set....field which is instead now NOT considered at all and
> so just sent as zero all of the time ... am I missing something ?
>
> ...so my understanding is that, being the expected format by the spec as
> in 4.11.2.7, you should instead pick the PackedFormat your received as a 32bit
> config, UNPACK it and split it into the attributes and config_value field....
>
> from the v3.2 BETA spec I have I cannot see where the 32 bit attributes
> payload is supposed to carry straight away the packedformat value...
>
> ...as it is implemented now, it is out of spec (at least the latest
> v3.2-BETA public that I can see)... and if it works for you, it just means
> your backend server is equally out-of-spec....which is probably a way of being
> compliant BUT not the way we need to here :P
>
> I cannot see any other way I can interpret this to make it
> work...apologies if I am missing something, in such a case please explain...
>
> Thanks,
> Cristian

Thank you very much for the tip. This is definitely an issue that should 
be addressed. I'll fix that and provide with v2 which I'm currently prepare.