firmware: arm_scmi: add timeout in do_xfer_with_response()

Add timeout argument to do_xfer_with_response() with subsequent changes in corresponding drivers. To maintain backward compatibility use previous hardcoded timeout value.

According to SCMI specification [1] there is no defined timeout for delayed messages in the interface. While hardcoded 2 seconds timeout might be good enough for existing protocol drivers, moving it to the function argument may be useful for vendor-specific protocols with different timing needs.

Quoted 4 lines · Matthew Bystrin

Add timeout argument to do_xfer_with_response() with subsequent changes in corresponding drivers. To maintain backward compatibility use previous hardcoded timeout value.

Quoted 6 lines

According to SCMI specification [1] there is no defined timeout for delayed messages in the interface. While hardcoded 2 seconds timeout might be good enough for existing protocol drivers, moving it to the function argument may be useful for vendor-specific protocols with different timing needs.

Please post this patch along with the vendor specific protocols mentioned above and with the reasoning as why 2s is not sufficient.

Also instead of churning up existing users/usage, we can explore to had one with this timeout as alternative if you present and convince the validity of your use-case and the associated timing requirement.

Quoted 5 lines · Sudeep Holla

Quoted 4 lines · Matthew Bystrin

Add timeout argument to do_xfer_with_response() with subsequent changes in corresponding drivers. To maintain backward compatibility use previous hardcoded timeout value.

Hi Matthew, Sudeep,

this is something I had my eyes on since a while and never get back to it....so thanks for looking at this first of all...

Quoted 9 lines

Quoted 6 lines

According to SCMI specification [1] there is no defined timeout for delayed messages in the interface. While hardcoded 2 seconds timeout might be good enough for existing protocol drivers, moving it to the function argument may be useful for vendor-specific protocols with different timing needs.

Please post this patch along with the vendor specific protocols mentioned above and with the reasoning as why 2s is not sufficient.

Ack on this, it would be good to understand why a huge 2 secs is not enough...and also...

Quoted 5 lines

Also instead of churning up existing users/usage, we can explore to had one with this timeout as alternative if you present and convince the validity of your use-case and the associated timing requirement.

...with the proposed patch (and any kind of alternative API proposed by Sudeep) the delayed response timeout becomes a parameter of the method do_xfer_with_response() and so, as a consequence, this timoeut becomes effectively configurable per-transaction, while usually a timeout is commonly configurable per-channel, so valid as a whole for any protocol on that channel across the whole platform, AND optionally describable as different from the default standard value via DT props (like max-rx-timeout).

Is this what we want ? (a per-transaction configurable timeout ?)

If not, it could be an option to make instead this a per-channel optional new DT described property so that you can configure globally a different delayed timeout.

If yes, how this new parameter is meant to be used/configured/chosen ? on a per-protocol/command basis, unrelated to the specific platform we run on ?

Quoted 2 lines

Please post this patch along with the vendor specific protocols mentioned above and with the reasoning as why 2s is not sufficient.

Ack on this, it would be good to understand why a huge 2 secs is not enough...and also...

Quoted 4 lines

Also instead of churning up existing users/usage, we can explore to had one with this timeout as alternative if you present and convince the validity of your use-case and the associated timing requirement.

...with the proposed patch (and any kind of alternative API proposed by Sudeep) the delayed response timeout becomes a parameter of the method do_xfer_with_response() and so, as a consequence, this timoeut becomes effectively configurable per-transaction, while usually a timeout is commonly configurable per-channel,

so valid as a whole for any protocol
on that channel across the whole platform, AND optionally describable as different from the default standard value via DT props (like max-rx-timeout).

Is this what we want ? (a per-transaction configurable timeout ?)

If not, it could be an option to make instead this a per-channel optional new DT described property so that you can configure globally a different delayed timeout.

If yes, how this new parameter is meant to be used/configured/chosen ? on a per-protocol/command basis, unrelated to the specific platform we run on ?

Thanks,
Cristian

Sudeep, Cristian,

Gentle ping.

Quoted 2 lines · Matthew Bystrin

Sudeep, Cristian,

Hi,

Quoted 2 lines

Gentle ping.

we replied already...both of us :P

https://lore.kernel.org/arm-scmi/[email protected]/

Maybe in your spam folder ?

Subject: Re: [PATCH] firmware: arm_scmi: add timeout in do_xfer_with_response()

[You don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

Hi Sudeep, Cristian,

Thanks for having a look on the patch.

Cristian Marussi, Apr 02, 2025 at 19:05:

Quoted 5 lines

Quoted 2 lines

Please post this patch along with the vendor specific protocols mentioned above and with the reasoning as why 2s is not sufficient.

Ack on this, it would be good to understand why a huge 2 secs is not enough...and also...

I've been working on firmware update using SCMI vendor/platform- specific extension on FPGA prototype, so not posted it initially. I'm open to share the details if needed, but need some extra time for preparations. For now I'm posting a brief description of the extension. It has 2 commands:

- Obtain firmware version number.
- Update firmware. Firmware image is placed into shared physically contiguous
memory, Agent sends to platform micro controller (PuC) physical
address and
size of the update image to start update procedure. After update is
completed
(successfully or not) PuC sends delayed response.

Agent ---- start update ---> Platform uC
Agent <--- update procedure started ---- Platform uC
...
Agent <--- (async) update completed ---- Platform uC

I've faced timeout problem with the async completion response. And update can't be done faster than 10s due to SPI flash write speed limit.

Why not to use notifications?

First of all, semantics. IIUC notifications can be sent by PuC in any time. This is not suitable for updates, because procedure is initiated by an agent, not by a platform.

Secondly, code implementing notification waiting duplicates delayed response code. I had implemented it as a proof-of-concept before I prepared this patch.

Quoted 2 lines

Quoted 2 lines

Also instead of churning up existing users/usage, we can explore to had one with this timeout as alternative if you present and

convince

Quoted 5 lines

Quoted 2 lines

the validity of your use-case and the associated timing requirement.

...with the proposed patch (and any kind of alternative API proposed by Sudeep) the delayed response timeout becomes a parameter of

the

Quoted 2 lines

method
do_xfer_with_response() and so, as a consequence, this timoeut

becomes

Quoted 2 lines

effectively configurable per-transaction, while usually a timeout is commonly configurable per-channel,

Totally agree, usually it is. And that's why I didn't change do_xfer() call. Here is the thing I want to pay attention to.

Let's focus on delayed responses. I think delayed response timeout should not be defined by transport but rather should be defined by _function_ PuC providing.
And of course platform and transport could influence on the timeout value.

Quoted 2 lines

so valid as a whole for any protocol
on that channel across the whole platform, AND optionally

describable

Quoted 1 line

as different from the default standard value via DT props (like max-rx-

timeout).

Quoted 5 lines

Is this what we want ? (a per-transaction configurable timeout ?)

If not, it could be an option to make instead this a per-channel optional new DT described property so that you can configure

globally

Quoted 1 line

a different delayed timeout.

Taking into account my previous comment, I don't think that having a per-channel timeout for delayed response would solve the problem in the right way. What about having a per-protocol timeout at least?

Quoted 1 line

If yes, how this new parameter is meant to be

used/configured/chosen ?

Quoted 1 line

on a per-protocol/command basis, unrelated to the specific platform

we run on ?

As delayed timeout is IMO should be defined by PuC services (in other words by command), new parameter can be set directly in the driver. If we talking about per-protocol solution, using DT is also a good approach.

Quoted 3 lines

Thanks,
Cristian

Sudeep, hope I also answered your comments from the last email as well.

Thanks,
Matthew

Hi Sudeep, Cristian,

Thanks for having a look on the patch.

Cristian Marussi, Apr 02, 2025 at 19:05:

Quoted 5 lines

Quoted 2 lines

Please post this patch along with the vendor specific protocols mentioned above and with the reasoning as why 2s is not sufficient.

Ack on this, it would be good to understand why a huge 2 secs is not enough...and also...

I've been working on firmware update using SCMI vendor/platform-specific extension on FPGA prototype, so not posted it initially. I'm open to share the details if needed, but need some extra time for preparations. For now I'm posting a brief description of the extension. It has 2 commands:

- Obtain firmware version number.
- Update firmware. Firmware image is placed into shared physically contiguous memory, Agent sends to platform micro controller (PuC) physical address and size of the update image to start update procedure. After update is completed (successfully or not) PuC sends delayed response.

	Agent ----     start update         ---> Platform uC
	Agent <--- update procedure started ---- Platform uC
	...
	Agent <--- (async) update completed ---- Platform uC

I've faced timeout problem with the async completion response. And update can't be done faster than 10s due to SPI flash write speed limit.

Why not to use notifications?

First of all, semantics. IIUC notifications can be sent by PuC in any time. This is not suitable for updates, because procedure is initiated by an agent, not by a platform.

Secondly, code implementing notification waiting duplicates delayed response code. I had implemented it as a proof-of-concept before I prepared this patch.

Quoted 10 lines

Quoted 4 lines

Also instead of churning up existing users/usage, we can explore to had one with this timeout as alternative if you present and convince the validity of your use-case and the associated timing requirement.

...with the proposed patch (and any kind of alternative API proposed by Sudeep) the delayed response timeout becomes a parameter of the method do_xfer_with_response() and so, as a consequence, this timoeut becomes effectively configurable per-transaction, while usually a timeout is commonly configurable per-channel,

Totally agree, usually it is. And that's why I didn't change do_xfer() call. Here is the thing I want to pay attention to.

Let's focus on delayed responses. I think delayed response timeout should not be defined by transport but rather should be defined by _function_ PuC providing. And of course platform and transport could influence on the timeout value.

Quoted 6 lines

so valid as a whole for any protocol
on that channel across the whole platform, AND optionally describable as different from the default standard value via DT props (like max-rx-timeout).

Is this what we want ? (a per-transaction configurable timeout ?)

Quoted 3 lines

If not, it could be an option to make instead this a per-channel optional new DT described property so that you can configure globally a different delayed timeout.

Taking into account my previous comment, I don't think that having a per-channel timeout for delayed response would solve the problem in the right way. What about having a per-protocol timeout at least?

The start update should retain as soon as Platform uC acks the request. And 2 notifications can be sent out for update procedure started and completed. I don't see any issue there. What is the semantics you are talking about ?

Even delayed response as some timeout so I would rather prefer to use notifications

in your usecase as it is completely async.

Yes neither per-transport nor per-protocol timeout will suffice in your case. This 10s timeout is specific to the update operation and hence use notification. All other solution is just workarounds not generic solution.

Signature · 2 lines

Regards,
Sudeep

Sudeep,

Thanks for taking your time.

Sudeep Holla, Apr 09, 2025 at 14:12:

Quoted 4 lines

The start update should retain as soon as Platform uC acks the request. And 2 notifications can be sent out for update procedure started and completed. I don't see any issue there. What is the semantics you are talking about ?

I'm going to refer to section 4.1.1 from the spec, where stated following about delayed responses,

"Messages sent to indicate completion of the work that is associated with an asynchronous command"

Compared to notifications,

"These messages provide notifications of events taking place in the platform. Events might include changes in power state, performance state, or other platform status"

So before I implemented mentioned driver I had red this two and had chosen delayed responses, because it had seemed more appropriate. Details below.

Quoted 2 lines

Even delayed response as some timeout so I would rather prefer to use notifications

Hmm, I see.

Quoted 1 line

in your usecase as it is completely async.

Just to emphasize, according to the spec I don't think that delayed responses and events have different degree of asynchrony. The difference is in the initiator of 'messaging'. Events are sent by platform to indicate its' state and delayed responses are sent to indicate status of previously requested operation.