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

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

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

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.

Link: https://developer.arm.com/Architectures/System%20Control%20and%20Management%20Interface
Signed-off-by: Matthew Bystrin <dev.mbstr@gmail.com>
---
 drivers/firmware/arm_scmi/clock.c     | 2 +-
 drivers/firmware/arm_scmi/common.h    | 1 -
 drivers/firmware/arm_scmi/driver.c    | 6 ++++--
 drivers/firmware/arm_scmi/powercap.c  | 2 +-
 drivers/firmware/arm_scmi/protocols.h | 3 ++-
 drivers/firmware/arm_scmi/reset.c     | 2 +-
 drivers/firmware/arm_scmi/sensors.c   | 4 ++--
 drivers/firmware/arm_scmi/voltage.c   | 2 +-
 include/linux/scmi_protocol.h         | 1 +
 9 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 2ed2279388f0..4b5cd73384c3 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -596,7 +596,7 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
 	cfg->value_high = cpu_to_le32(rate >> 32);
 
 	if (flags & CLOCK_SET_ASYNC) {
-		ret = ph->xops->do_xfer_with_response(ph, t);
+		ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT);
 		if (!ret) {
 			struct scmi_msg_resp_set_rate_complete *resp;
 
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 10ea7962323e..34527366c909 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -29,7 +29,6 @@
 
 #define SCMI_MAX_CHANNELS		256
 
-#define SCMI_MAX_RESPONSE_TIMEOUT	(2 * MSEC_PER_SEC)
 
 #define SCMI_SHMEM_MAX_PAYLOAD_SIZE	104
 
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 1c75a4c9c371..51c6634d5505 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1490,6 +1490,7 @@ static void reset_rx_to_maxsz(const struct scmi_protocol_handle *ph,
  *
  * @ph: Pointer to SCMI protocol handle
  * @xfer: Transfer to initiate and wait for response
+ * @timeout_ms: Delayed response wait timeout, if unsure use SCMI_DEFAULT_TIMEOUT
  *
  * Using asynchronous commands in atomic/polling mode should be avoided since
  * it could cause long busy-waiting here, so ignore polling for the delayed
@@ -1509,9 +1510,10 @@ static void reset_rx_to_maxsz(const struct scmi_protocol_handle *ph,
  *	return corresponding error, else if all goes well, return 0.
  */
 static int do_xfer_with_response(const struct scmi_protocol_handle *ph,
-				 struct scmi_xfer *xfer)
+				 struct scmi_xfer *xfer, unsigned long timeout_ms)
 {
-	int ret, timeout = msecs_to_jiffies(SCMI_MAX_RESPONSE_TIMEOUT);
+	int ret;
+	unsigned long timeout = msecs_to_jiffies(timeout_ms);
 	DECLARE_COMPLETION_ONSTACK(async_response);
 
 	xfer->async_done = &async_response;
diff --git a/drivers/firmware/arm_scmi/powercap.c b/drivers/firmware/arm_scmi/powercap.c
index 1fa79bba492e..82565dacd301 100644
--- a/drivers/firmware/arm_scmi/powercap.c
+++ b/drivers/firmware/arm_scmi/powercap.c
@@ -386,7 +386,7 @@ static int scmi_powercap_xfer_cap_set(const struct scmi_protocol_handle *ph,
 	if (!pc->async_powercap_cap_set || ignore_dresp) {
 		ret = ph->xops->do_xfer(ph, t);
 	} else {
-		ret = ph->xops->do_xfer_with_response(ph, t);
+		ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT);
 		if (!ret) {
 			struct scmi_msg_resp_powercap_cap_set_complete *resp;
 
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index aaee57cdcd55..b1e3cf3601fe 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -307,7 +307,8 @@ struct scmi_xfer_ops {
 	int (*do_xfer)(const struct scmi_protocol_handle *ph,
 		       struct scmi_xfer *xfer);
 	int (*do_xfer_with_response)(const struct scmi_protocol_handle *ph,
-				     struct scmi_xfer *xfer);
+				     struct scmi_xfer *xfer,
+				     unsigned long timeout_ms);
 	void (*xfer_put)(const struct scmi_protocol_handle *ph,
 			 struct scmi_xfer *xfer);
 };
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 0aa82b96f41b..a458b1c16c51 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -198,7 +198,7 @@ static int scmi_domain_reset(const struct scmi_protocol_handle *ph, u32 domain,
 	dom->reset_state = cpu_to_le32(state);
 
 	if (flags & ASYNCHRONOUS_RESET)
-		ret = ph->xops->do_xfer_with_response(ph, t);
+		ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT);
 	else
 		ret = ph->xops->do_xfer(ph, t);
 
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 791efd0f82d7..9a07399237f5 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -871,7 +871,7 @@ static int scmi_sensor_reading_get(const struct scmi_protocol_handle *ph,
 	s = si->sensors + sensor_id;
 	if (s->async) {
 		sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
-		ret = ph->xops->do_xfer_with_response(ph, t);
+		ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT);
 		if (!ret) {
 			struct scmi_resp_sensor_reading_complete *resp;
 
@@ -943,7 +943,7 @@ scmi_sensor_reading_get_timestamped(const struct scmi_protocol_handle *ph,
 	sensor->id = cpu_to_le32(sensor_id);
 	if (s->async) {
 		sensor->flags = cpu_to_le32(SENSOR_READ_ASYNC);
-		ret = ph->xops->do_xfer_with_response(ph, t);
+		ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT);
 		if (!ret) {
 			int i;
 			struct scmi_resp_sensor_reading_complete_v3 *resp;
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index fda6a1573609..26b1f034256b 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -348,7 +348,7 @@ static int scmi_voltage_level_set(const struct scmi_protocol_handle *ph,
 		ret = ph->xops->do_xfer(ph, t);
 	} else {
 		cmd->flags = cpu_to_le32(0x1);
-		ret = ph->xops->do_xfer_with_response(ph, t);
+		ret = ph->xops->do_xfer_with_response(ph, t, SCMI_DEFAULT_TIMEOUT);
 		if (!ret) {
 			struct scmi_resp_voltage_level_set_complete *resp;
 
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 688466a0e816..2426daae8a87 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -16,6 +16,7 @@
 #define SCMI_MAX_STR_SIZE		64
 #define SCMI_SHORT_NAME_MAX_SIZE	16
 #define SCMI_MAX_NUM_RATES		16
+#define SCMI_DEFAULT_TIMEOUT		(2 * MSEC_PER_SEC)
 
 /**
  * struct scmi_revision_info - version information structure
-- 
2.47.2

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

[Sudeep Holla]

On Wed, Apr 02, 2025 at 01:42:54PM +0300, Matthew Bystrin wrote:
> 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.
> 

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.

-- 
Regards,
Sudeep

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

[Cristian Marussi]

On Wed, Apr 02, 2025 at 11:59:47AM +0100, Sudeep Holla wrote:
> On Wed, Apr 02, 2025 at 01:42:54PM +0300, Matthew Bystrin wrote:
> > 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...

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

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

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

[Sudeep Holla]

On Wed, Apr 02, 2025 at 05:05:55PM +0100, Cristian Marussi wrote:
> On Wed, Apr 02, 2025 at 11:59:47AM +0100, Sudeep Holla wrote:
> > On Wed, Apr 02, 2025 at 01:42:54PM +0300, Matthew Bystrin wrote:
> > > 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...
> 
> > > 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...
> 
> > 
> > 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 ?
>  

+1 on all the points above. I agree this must be per transport. If it is
per message then you need to think why it needs to sync command. Why can't
it be changed to async or use notification. I am waiting to see the usage
in your vendor protocols to understand it in more detail.

-- 
Regards,
Sudeep

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

[Matthew Bystrin]

Hi Sudeep, Cristian,

Thanks for having a look on the patch.

Cristian Marussi, Apr 02, 2025 at 19:05:
> > 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.

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

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

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?

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

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.

> Thanks,
> Cristian

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

Thanks,
Matthew

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

[Matthew Bystrin]

Sudeep, Cristian,

Gentle ping.

Best regards,
Matthew

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

[Cristian Marussi]

On Tue, Apr 08, 2025 at 11:22:38PM +0300, Matthew Bystrin wrote:
> Sudeep, Cristian,
> 

Hi,

> Gentle ping.
>

we replied already...both of us :P

https://lore.kernel.org/arm-scmi/20250402104254.149998-1-dev.mbstr@gmail.com/

Maybe in your spam folder ?

Thanks,
Cristian

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

[Cristian Marussi]

On Wed, Apr 09, 2025 at 11:52:12AM +0100, Cristian Marussi wrote:
> On Tue, Apr 08, 2025 at 11:22:38PM +0300, Matthew Bystrin wrote:
> > Sudeep, Cristian,
> > 
> 
> Hi,
> 
> > Gentle ping.
> >
> 
> we replied already...both of us :P
> 
> https://lore.kernel.org/arm-scmi/20250402104254.149998-1-dev.mbstr@gmail.com/
> 
> Maybe in your spam folder ?

Wait...I have just seen Peng's reply mentioning some reply of yours,
which I never saw ..  maybe MY spam folder :P ....jeezzz...

Thanks,
Cristian

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

[Peng Fan]

> Subject: Re: [PATCH] firmware: arm_scmi: add timeout in
> do_xfer_with_response()
> 
> [You don't often get email from dev.mbstr@gmail.com. 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:
> > > 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.
> 
> > > 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.
> 
> > 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.
> 
> 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?
> 
> > 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 ?
> 
> 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.

i.MX SCMI does not use delayed response as of now, so just ignore me
if I am wrong.

delayed response time may vary based on SCMI platform loading
or the time the command requires, this should be platform/transport
stuff. Define a max waiting time for delayed response saying
"arm,max-delayed-response-timeout-us"
for the transport, similar as "arm,max-rx-timeout-ms".
Then no need to be per protocol.

Regards,
Peng.
> 
> > Thanks,
> > Cristian
> 
> Sudeep, hope I also answered your comments from the last email as
> well.
> 
> Thanks,
> Matthew

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

[Sudeep Holla]

On Thu, Apr 03, 2025 at 10:50:17PM +0300, Matthew Bystrin wrote:
> Hi Sudeep, Cristian,
> 
> Thanks for having a look on the patch.
> 
> Cristian Marussi, Apr 02, 2025 at 19:05:
> > > 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.
> 

Understood.

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

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 ?

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

Even delayed response as some timeout so I would rather prefer to use
notifications in your usecase as it is completely async.

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

I think in your case, it is not even transport specific. It is more operation
specific and hence I prefer notifications.

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

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.

-- 
Regards,
Sudeep

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

[Matthew Bystrin]

Sudeep,

Thanks for taking your time.

Sudeep Holla, Apr 09, 2025 at 14:12:
> 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.

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

Hmm, I see.

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

I used the latter, because firmware update can't happen spontaneously. That what
I meant when used term 'semantics'.

> 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.
>
> -- 
> Regards,
> Sudeep

I see your point of view. However, taking into account given arguments, did I
convince you that delayed responses handling should be implemented in slightly
different way?

-- 
Best regards,
Matthew

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

[Cristian Marussi]

On Sat, Apr 12, 2025 at 01:39:45PM +0300, Matthew Bystrin wrote:
> Sudeep,
> 

Hi Matthew,

> Thanks for taking your time.
> 
> Sudeep Holla, Apr 09, 2025 at 14:12:
> > 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.
> 
> > Even delayed response as some timeout so I would rather prefer to use
> > notifications
> 
> Hmm, I see.
> 
> > 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.
> 

Delayed reponses are certainly better than notification for completion
of agent initiated actions BUT this does not exclude the usage instead
of a sync-command to start the operation and a notification to signal
its completion...depends really on the case.

The classic example of a needed async-cmd is reading a sensor that takes
a long time due to its own physical nature...

AFAIU, in this case you have an async operation whose completion time is
considerably longer (so you aim to configure a specific timeout for that
specific command) BUT it is also bound to the payload itself that you
are trying to load AND/OR to other platform specific HW charactristics
(like how slow are your flashes in this HW releases...): this means that
while the sensor slowness is stable and predictable, and the timeout can
be fixed a-priori, in this case you risk to have in the future anyway to
have to refine and tune this ad-hoc custom timeout....while you'd have
none of this issue by simply waiting for a notification (ofc you could
have to set a large timeout on your side anyway while waiting for
notifs...)

...unless you plan to dynamically tune the async-cmd timeout at runtime
based on the known payload size (that means more commands to query the
soon-to-be-flahsed payload) but anyway this does NOT solve the fact that
the platform characteristics can influence the length of the operation.

Thanks,
Cristian

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

[Matthew Bystrin]

Cristian, Sudeep,

thanks for taking your time!

I'll figure something out :)

-- 
Best regards,
Matthew