linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] firmware: arm_scmi: always initialize protocols
@ 2020-10-08 14:37 Etienne Carriere
  2020-10-08 14:37 ` [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization Etienne Carriere
                   ` (5 more replies)
  0 siblings, 6 replies; 27+ messages in thread
From: Etienne Carriere @ 2020-10-08 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, Sudeep Holla, Cristian Marussi,
	Vincent Guittot, Souvik Chakravarty, Etienne Carriere

Remove the IDR replacement that prevent initializing an SCMI protocol
when it has already been initialized. This is needed when there are
several SCMI agents that do implement a given SCMI protocol unless
what only the related SCMI protocol communication is initialized only
for first probed agent.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/firmware/arm_scmi/bus.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 1377ec76a45d..8ea04b069129 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -60,11 +60,6 @@ static int scmi_protocol_init(int protocol_id, struct scmi_handle *handle)
 	return fn(handle);
 }
 
-static int scmi_protocol_dummy_init(struct scmi_handle *handle)
-{
-	return 0;
-}
-
 static int scmi_dev_probe(struct device *dev)
 {
 	struct scmi_driver *scmi_drv = to_scmi_driver(dev->driver);
@@ -83,10 +78,6 @@ static int scmi_dev_probe(struct device *dev)
 	if (ret)
 		return ret;
 
-	/* Skip protocol initialisation for additional devices */
-	idr_replace(&scmi_protocols, &scmi_protocol_dummy_init,
-		    scmi_dev->protocol_id);
-
 	return scmi_drv->probe(scmi_dev);
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization
  2020-10-08 14:37 [PATCH 1/5] firmware: arm_scmi: always initialize protocols Etienne Carriere
@ 2020-10-08 14:37 ` Etienne Carriere
  2020-10-08 21:18   ` Sudeep Holla
  2020-10-12 14:17   ` [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation Sudeep Holla
  2020-10-08 14:37 ` [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport Etienne Carriere
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 27+ messages in thread
From: Etienne Carriere @ 2020-10-08 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, Sudeep Holla, Cristian Marussi,
	Vincent Guittot, Souvik Chakravarty, Etienne Carriere

Implement helper function scmi_do_xfer_again() to process consecutive
transfers that are initialized only once with scmi_xfer_get_init()
and hence get the pool completion and responses message length not
reloaded regarding last completed transfer.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/firmware/arm_scmi/base.c    |  2 +-
 drivers/firmware/arm_scmi/clock.c   |  2 +-
 drivers/firmware/arm_scmi/common.h  |  2 ++
 drivers/firmware/arm_scmi/driver.c  | 10 ++++++++++
 drivers/firmware/arm_scmi/perf.c    |  2 +-
 drivers/firmware/arm_scmi/sensors.c |  2 +-
 6 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index 9853bd3c4d45..508f214baa1b 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -183,7 +183,7 @@ static int scmi_base_implementation_list_get(const struct scmi_handle *handle,
 		/* Set the number of protocols to be skipped/already read */
 		*num_skip = cpu_to_le32(tot_num_ret);
 
-		ret = scmi_do_xfer(handle, t);
+		ret = scmi_do_xfer_again(handle, t);
 		if (ret)
 			break;
 
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index c1cfe3ee3d55..9bb54c1a8d55 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -161,7 +161,7 @@ scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
 		/* Set the number of rates to be skipped/already read */
 		clk_desc->rate_index = cpu_to_le32(tot_rate_cnt);
 
-		ret = scmi_do_xfer(handle, t);
+		ret = scmi_do_xfer_again(handle, t);
 		if (ret)
 			goto err;
 
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 37fb583f1bf5..6d4eea7b0f3e 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -143,6 +143,8 @@ struct scmi_xfer {
 
 void scmi_xfer_put(const struct scmi_handle *h, struct scmi_xfer *xfer);
 int scmi_do_xfer(const struct scmi_handle *h, struct scmi_xfer *xfer);
+int scmi_do_xfer_again(const struct scmi_handle *handle,
+		       struct scmi_xfer *xfer);
 int scmi_do_xfer_with_response(const struct scmi_handle *h,
 			       struct scmi_xfer *xfer);
 int scmi_xfer_get_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id,
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index c5dea87edf8f..887cb8249db0 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -402,6 +402,16 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
 	return ret;
 }
 
+int scmi_do_xfer_again(const struct scmi_handle *handle, struct scmi_xfer *xfer)
+{
+	struct scmi_info *info = handle_to_scmi_info(handle);
+
+	xfer->rx.len = info->desc->max_msg_size;
+	xfer->hdr.poll_completion = false;
+
+	return scmi_do_xfer(handle, xfer);
+}
+
 #define SCMI_MAX_RESPONSE_TIMEOUT	(2 * MSEC_PER_SEC)
 
 /**
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index ed475b40bd08..66d3d8459936 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -281,7 +281,7 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
 		/* Set the number of OPPs to be skipped/already read */
 		dom_info->level_index = cpu_to_le32(tot_opp_cnt);
 
-		ret = scmi_do_xfer(handle, t);
+		ret = scmi_do_xfer_again(handle, t);
 		if (ret)
 			break;
 
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 9703cf6356a0..97addcf828a3 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -134,7 +134,7 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
 		/* Set the number of sensors to be skipped/already read */
 		put_unaligned_le32(desc_index, t->tx.buf);
 
-		ret = scmi_do_xfer(handle, t);
+		ret = scmi_do_xfer_again(handle, t);
 		if (ret)
 			break;
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport
  2020-10-08 14:37 [PATCH 1/5] firmware: arm_scmi: always initialize protocols Etienne Carriere
  2020-10-08 14:37 ` [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization Etienne Carriere
@ 2020-10-08 14:37 ` Etienne Carriere
  2020-10-08 21:08   ` Sudeep Holla
  2020-10-08 14:37 ` [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool Etienne Carriere
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 27+ messages in thread
From: Etienne Carriere @ 2020-10-08 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, Sudeep Holla, Cristian Marussi,
	Vincent Guittot, Souvik Chakravarty, Etienne Carriere

Fix dependencies for configuration switch ARM_SCMI_PROTOCOL that
is not exclusively dependent on MAILBOX since the alternate
smc transport that is depends on HAVE_ARM_SMCCC_DISCOVERY since [1].

Link: [1] d76428237784 ("firmware: arm_scmi: Use HAVE_ARM_SMCCC_DISCOVERY instead of ARM_PSCI_FW")
Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/firmware/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 3315e3c21586..5bdd411206ff 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -9,7 +9,7 @@ menu "Firmware Drivers"
 config ARM_SCMI_PROTOCOL
 	tristate "ARM System Control and Management Interface (SCMI) Message Protocol"
 	depends on ARM || ARM64 || COMPILE_TEST
-	depends on MAILBOX
+	depends on MAILBOX || HAVE_ARM_SMCCC_DISCOVERY
 	help
 	  ARM System Control and Management Interface (SCMI) protocol is a
 	  set of operating system-independent software interfaces that are
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool
  2020-10-08 14:37 [PATCH 1/5] firmware: arm_scmi: always initialize protocols Etienne Carriere
  2020-10-08 14:37 ` [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization Etienne Carriere
  2020-10-08 14:37 ` [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport Etienne Carriere
@ 2020-10-08 14:37 ` Etienne Carriere
  2020-10-08 21:11   ` Sudeep Holla
  2020-10-08 14:37 ` [PATCH 5/5] firmware: arm_scmi: fix ARCH_COLD_RESET Etienne Carriere
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 27+ messages in thread
From: Etienne Carriere @ 2020-10-08 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, Sudeep Holla, Cristian Marussi,
	Vincent Guittot, Souvik Chakravarty, Etienne Carriere, Peng Fan

There is no reason for the smc transport to restrict itself to a 1
message pool. More can be allocated, messages are copied from/to the
shared memory only on SMC exit/entry hence SCMI driver can play with
several messages.

Use value of 20 to mimic mailbox transport implementation. Any high
value could fit. This should be something configurable.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Cc: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/smc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 1a03c3ec0230..82a82a5dc86a 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -149,6 +149,6 @@ static const struct scmi_transport_ops scmi_smc_ops = {
 const struct scmi_desc scmi_smc_desc = {
 	.ops = &scmi_smc_ops,
 	.max_rx_timeout_ms = 30,
-	.max_msg = 1,
+	.max_msg = 20,
 	.max_msg_size = 128,
 };
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* [PATCH 5/5] firmware: arm_scmi: fix ARCH_COLD_RESET
  2020-10-08 14:37 [PATCH 1/5] firmware: arm_scmi: always initialize protocols Etienne Carriere
                   ` (2 preceding siblings ...)
  2020-10-08 14:37 ` [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool Etienne Carriere
@ 2020-10-08 14:37 ` Etienne Carriere
  2020-10-08 21:16   ` Sudeep Holla
  2020-10-08 19:17 ` [PATCH 1/5] firmware: arm_scmi: always initialize protocols Sudeep Holla
  2020-10-13 10:45 ` Sudeep Holla
  5 siblings, 1 reply; 27+ messages in thread
From: Etienne Carriere @ 2020-10-08 14:37 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, Sudeep Holla, Cristian Marussi,
	Vincent Guittot, Souvik Chakravarty, Etienne Carriere

Fix ARCH_COLD_RESET according to SCMI specification.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
---
 drivers/firmware/arm_scmi/reset.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index f063cfe17e02..a981a22cfe89 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -36,9 +36,7 @@ struct scmi_msg_reset_domain_reset {
 #define EXPLICIT_RESET_ASSERT	BIT(1)
 #define ASYNCHRONOUS_RESET	BIT(2)
 	__le32 reset_state;
-#define ARCH_RESET_TYPE		BIT(31)
-#define COLD_RESET_STATE	BIT(0)
-#define ARCH_COLD_RESET		(ARCH_RESET_TYPE | COLD_RESET_STATE)
+#define ARCH_COLD_RESET		0
 };
 
 struct scmi_msg_reset_notify {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols
  2020-10-08 14:37 [PATCH 1/5] firmware: arm_scmi: always initialize protocols Etienne Carriere
                   ` (3 preceding siblings ...)
  2020-10-08 14:37 ` [PATCH 5/5] firmware: arm_scmi: fix ARCH_COLD_RESET Etienne Carriere
@ 2020-10-08 19:17 ` Sudeep Holla
  2020-10-09 12:31   ` Etienne Carriere
  2020-10-13 10:45 ` Sudeep Holla
  5 siblings, 1 reply; 27+ messages in thread
From: Sudeep Holla @ 2020-10-08 19:17 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel, linux-arm-kernel, Cristian Marussi, Sudeep Holla,
	Vincent Guittot, Souvik Chakravarty

On Thu, Oct 08, 2020 at 04:37:18PM +0200, Etienne Carriere wrote:
> Remove the IDR replacement that prevent initializing an SCMI protocol
> when it has already been initialized. This is needed when there are
> several SCMI agents that do implement a given SCMI protocol unless
> what only the related SCMI protocol communication is initialized only
> for first probed agent.
> 

Can you please elaborate on your usecase please. What do you mean by several
SCMI agents here. OSPM is the only agent we are interested here. What
other agents is this driver supposed to handle here. We allocate memory
in init and calling init multiple times messes up the allocated and
initialised structures.

So NACK for this patch as it needs more work if we need this at all.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport
  2020-10-08 14:37 ` [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport Etienne Carriere
@ 2020-10-08 21:08   ` Sudeep Holla
  2020-10-09 12:33     ` Etienne Carriere
  0 siblings, 1 reply; 27+ messages in thread
From: Sudeep Holla @ 2020-10-08 21:08 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel, linux-arm-kernel, Cristian Marussi,
	Vincent Guittot, Souvik Chakravarty

On Thu, Oct 08, 2020 at 04:37:20PM +0200, Etienne Carriere wrote:
> Fix dependencies for configuration switch ARM_SCMI_PROTOCOL that
> is not exclusively dependent on MAILBOX since the alternate
> smc transport that is depends on HAVE_ARM_SMCCC_DISCOVERY since [1].
>

Do you need any build issues ? I don't see why this is needed.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool
  2020-10-08 14:37 ` [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool Etienne Carriere
@ 2020-10-08 21:11   ` Sudeep Holla
  2020-10-09 12:43     ` Etienne Carriere
  0 siblings, 1 reply; 27+ messages in thread
From: Sudeep Holla @ 2020-10-08 21:11 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel, linux-arm-kernel, Cristian Marussi,
	Vincent Guittot, Souvik Chakravarty, Peng Fan

On Thu, Oct 08, 2020 at 04:37:21PM +0200, Etienne Carriere wrote:
> There is no reason for the smc transport to restrict itself to a 1
> message pool. More can be allocated, messages are copied from/to the
> shared memory only on SMC exit/entry hence SCMI driver can play with
> several messages.
> 
> Use value of 20 to mimic mailbox transport implementation.

What is the need to mimic ?

> Any high value could fit. This should be something configurable.

Why not 10 or 100 ? I see any value other than 1 is useless as we lock
the channel in send_message and we don't maintain a queue like mailbox.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 5/5] firmware: arm_scmi: fix ARCH_COLD_RESET
  2020-10-08 14:37 ` [PATCH 5/5] firmware: arm_scmi: fix ARCH_COLD_RESET Etienne Carriere
@ 2020-10-08 21:16   ` Sudeep Holla
  0 siblings, 0 replies; 27+ messages in thread
From: Sudeep Holla @ 2020-10-08 21:16 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel, linux-arm-kernel, Cristian Marussi,
	Vincent Guittot, Souvik Chakravarty

On Thu, Oct 08, 2020 at 04:37:22PM +0200, Etienne Carriere wrote:
> Fix ARCH_COLD_RESET according to SCMI specification.
>

Thanks, nice catch.
I will send this as fix after v5.10-rc1 with fixes tag

Fixes: 95a15d80aa0d ("firmware: arm_scmi: Add RESET protocol in SCMI v2.0")

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization
  2020-10-08 14:37 ` [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization Etienne Carriere
@ 2020-10-08 21:18   ` Sudeep Holla
  2020-10-09 12:38     ` Etienne Carriere
  2020-10-12 14:17   ` [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation Sudeep Holla
  1 sibling, 1 reply; 27+ messages in thread
From: Sudeep Holla @ 2020-10-08 21:18 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel, linux-arm-kernel, Cristian Marussi,
	Vincent Guittot, Souvik Chakravarty

On Thu, Oct 08, 2020 at 04:37:19PM +0200, Etienne Carriere wrote:
> Implement helper function scmi_do_xfer_again() to process consecutive
> transfers that are initialized only once with scmi_xfer_get_init()
> and hence get the pool completion and responses message length not
> reloaded regarding last completed transfer.
> 
> Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> ---
>  drivers/firmware/arm_scmi/base.c    |  2 +-
>  drivers/firmware/arm_scmi/clock.c   |  2 +-
>  drivers/firmware/arm_scmi/common.h  |  2 ++
>  drivers/firmware/arm_scmi/driver.c  | 10 ++++++++++
>  drivers/firmware/arm_scmi/perf.c    |  2 +-
>  drivers/firmware/arm_scmi/sensors.c |  2 +-
>  6 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> index 9853bd3c4d45..508f214baa1b 100644
> --- a/drivers/firmware/arm_scmi/base.c
> +++ b/drivers/firmware/arm_scmi/base.c
> @@ -183,7 +183,7 @@ static int scmi_base_implementation_list_get(const struct scmi_handle *handle,
>  		/* Set the number of protocols to be skipped/already read */
>  		*num_skip = cpu_to_le32(tot_num_ret);
>  
> -		ret = scmi_do_xfer(handle, t);
> +		ret = scmi_do_xfer_again(handle, t);
>  		if (ret)
>  			break;
>  
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index c1cfe3ee3d55..9bb54c1a8d55 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -161,7 +161,7 @@ scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
>  		/* Set the number of rates to be skipped/already read */
>  		clk_desc->rate_index = cpu_to_le32(tot_rate_cnt);
>  
> -		ret = scmi_do_xfer(handle, t);
> +		ret = scmi_do_xfer_again(handle, t);
>  		if (ret)
>  			goto err;
>  
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 37fb583f1bf5..6d4eea7b0f3e 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -143,6 +143,8 @@ struct scmi_xfer {
>  
>  void scmi_xfer_put(const struct scmi_handle *h, struct scmi_xfer *xfer);
>  int scmi_do_xfer(const struct scmi_handle *h, struct scmi_xfer *xfer);
> +int scmi_do_xfer_again(const struct scmi_handle *handle,
> +		       struct scmi_xfer *xfer);
>  int scmi_do_xfer_with_response(const struct scmi_handle *h,
>  			       struct scmi_xfer *xfer);
>  int scmi_xfer_get_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id,
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index c5dea87edf8f..887cb8249db0 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -402,6 +402,16 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
>  	return ret;
>  }
>  
> +int scmi_do_xfer_again(const struct scmi_handle *handle, struct scmi_xfer *xfer)
> +{
> +	struct scmi_info *info = handle_to_scmi_info(handle);
> +
> +	xfer->rx.len = info->desc->max_msg_size;

I am tempted to just have helper for above and use it where needed.
Or may be I just don't like the name of the function, how about
scmi_do_xfer_rxlen_reinit or anything else. Suggestions ?

> +	xfer->hdr.poll_completion = false;

Do we really need the above ?

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols
  2020-10-08 19:17 ` [PATCH 1/5] firmware: arm_scmi: always initialize protocols Sudeep Holla
@ 2020-10-09 12:31   ` Etienne Carriere
  2020-10-09 16:31     ` Cristian Marussi
  2020-10-12  9:32     ` Sudeep Holla
  0 siblings, 2 replies; 27+ messages in thread
From: Etienne Carriere @ 2020-10-09 12:31 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty

On Thu, 8 Oct 2020 at 21:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 08, 2020 at 04:37:18PM +0200, Etienne Carriere wrote:
> > Remove the IDR replacement that prevent initializing an SCMI protocol
> > when it has already been initialized. This is needed when there are
> > several SCMI agents that do implement a given SCMI protocol unless
> > what only the related SCMI protocol communication is initialized only
> > for first probed agent.
> >
>
> Can you please elaborate on your usecase please. What do you mean by several
> SCMI agents here. OSPM is the only agent we are interested here. What
> other agents is this driver supposed to handle here. We allocate memory
> in init and calling init multiple times messes up the allocated and
> initialised structures.
>
> So NACK for this patch as it needs more work if we need this at all.
>

Hello Sudeep,

Considering a device with several SCMI servers spread over several co-processor
and possibly also in the Arm TZ secure world, each of these servers
uses a specific
SCMI channel. Without this change, each SCMI protocol gets initialized
only for the
first agent device that is probed.

My setup is also a bit specific. My device has several secure configuration
features that can individually be enabled or not. For example, configuring
domain X as secure makes some clocks reachable by Linux only through SCMI,
and configuring domain Y as secure makes other clocks reachable by Linux
only through SCMI. For flexibility, I expose domain X resources (here clocks)
to an Linux agent whereas domain Y resources (here clocks also) are
exposed to another agent, each agent with its specific transport/channel.
Enabling each agent node in the Linux FDT allows to define which SCMI clocks
get exposed and hence registered in the kernel.
Without the change proposed here, I cannot get the clocks exposed to both
agents when enabled as the SCMI clock protocol is initialized only for the 1st
probbed agent device.

Regards,
Etienne

> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport
  2020-10-08 21:08   ` Sudeep Holla
@ 2020-10-09 12:33     ` Etienne Carriere
  2020-10-09 15:58       ` Sudeep Holla
  0 siblings, 1 reply; 27+ messages in thread
From: Etienne Carriere @ 2020-10-09 12:33 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty

On Thu, 8 Oct 2020 at 23:08, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 08, 2020 at 04:37:20PM +0200, Etienne Carriere wrote:
> > Fix dependencies for configuration switch ARM_SCMI_PROTOCOL that
> > is not exclusively dependent on MAILBOX since the alternate
> > smc transport that is depends on HAVE_ARM_SMCCC_DISCOVERY since [1].
> >
>
> Do you need any build issues ? I don't see why this is needed.
>

This change is for consistency of the kernel configuration.
Without this change, a kernel configured without CONFIG_MAILBOX
cannot embed SCMI support even is using only the SMC transport
enabled thanks to HAVE_ARM_SMCCC_DISCOVERY.

Regards,
Etienne

> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization
  2020-10-08 21:18   ` Sudeep Holla
@ 2020-10-09 12:38     ` Etienne Carriere
  2020-10-09 15:20       ` Sudeep Holla
  0 siblings, 1 reply; 27+ messages in thread
From: Etienne Carriere @ 2020-10-09 12:38 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty

On Thu, 8 Oct 2020 at 23:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 08, 2020 at 04:37:19PM +0200, Etienne Carriere wrote:
> > Implement helper function scmi_do_xfer_again() to process consecutive
> > transfers that are initialized only once with scmi_xfer_get_init()
> > and hence get the pool completion and responses message length not
> > reloaded regarding last completed transfer.
> >
> > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > ---
> >  drivers/firmware/arm_scmi/base.c    |  2 +-
> >  drivers/firmware/arm_scmi/clock.c   |  2 +-
> >  drivers/firmware/arm_scmi/common.h  |  2 ++
> >  drivers/firmware/arm_scmi/driver.c  | 10 ++++++++++
> >  drivers/firmware/arm_scmi/perf.c    |  2 +-
> >  drivers/firmware/arm_scmi/sensors.c |  2 +-
> >  6 files changed, 16 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > index 9853bd3c4d45..508f214baa1b 100644
> > --- a/drivers/firmware/arm_scmi/base.c
> > +++ b/drivers/firmware/arm_scmi/base.c
> > @@ -183,7 +183,7 @@ static int scmi_base_implementation_list_get(const struct scmi_handle *handle,
> >               /* Set the number of protocols to be skipped/already read */
> >               *num_skip = cpu_to_le32(tot_num_ret);
> >
> > -             ret = scmi_do_xfer(handle, t);
> > +             ret = scmi_do_xfer_again(handle, t);
> >               if (ret)
> >                       break;
> >
> > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > index c1cfe3ee3d55..9bb54c1a8d55 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -161,7 +161,7 @@ scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
> >               /* Set the number of rates to be skipped/already read */
> >               clk_desc->rate_index = cpu_to_le32(tot_rate_cnt);
> >
> > -             ret = scmi_do_xfer(handle, t);
> > +             ret = scmi_do_xfer_again(handle, t);
> >               if (ret)
> >                       goto err;
> >
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 37fb583f1bf5..6d4eea7b0f3e 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -143,6 +143,8 @@ struct scmi_xfer {
> >
> >  void scmi_xfer_put(const struct scmi_handle *h, struct scmi_xfer *xfer);
> >  int scmi_do_xfer(const struct scmi_handle *h, struct scmi_xfer *xfer);
> > +int scmi_do_xfer_again(const struct scmi_handle *handle,
> > +                    struct scmi_xfer *xfer);
> >  int scmi_do_xfer_with_response(const struct scmi_handle *h,
> >                              struct scmi_xfer *xfer);
> >  int scmi_xfer_get_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id,
> > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > index c5dea87edf8f..887cb8249db0 100644
> > --- a/drivers/firmware/arm_scmi/driver.c
> > +++ b/drivers/firmware/arm_scmi/driver.c
> > @@ -402,6 +402,16 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
> >       return ret;
> >  }
> >
> > +int scmi_do_xfer_again(const struct scmi_handle *handle, struct scmi_xfer *xfer)
> > +{
> > +     struct scmi_info *info = handle_to_scmi_info(handle);
> > +
> > +     xfer->rx.len = info->desc->max_msg_size;
>
> I am tempted to just have helper for above and use it where needed.
> Or may be I just don't like the name of the function, how about
> scmi_do_xfer_rxlen_reinit or anything else. Suggestions ?

I think a smoother way would be that scmi_do_xfer() initializes
both
  xfer->rx.len = info->desc->max_msg_size
and
  xfer->hdr.poll_completion = false
instead of doing that from scmi_xfer_get_init().

>
> > +     xfer->hdr.poll_completion = false;
>
> Do we really need the above ?

I think so. Once a transfer is completed, poll_completion is true. But
the next transfer we expect should start assuming completion is not
yet done, hence this reset to false.

Regards,
Etienne

>
> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool
  2020-10-08 21:11   ` Sudeep Holla
@ 2020-10-09 12:43     ` Etienne Carriere
  2020-10-09 15:17       ` Sudeep Holla
  0 siblings, 1 reply; 27+ messages in thread
From: Etienne Carriere @ 2020-10-09 12:43 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty, Peng Fan

On Thu, 8 Oct 2020 at 23:11, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Thu, Oct 08, 2020 at 04:37:21PM +0200, Etienne Carriere wrote:
> > There is no reason for the smc transport to restrict itself to a 1
> > message pool. More can be allocated, messages are copied from/to the
> > shared memory only on SMC exit/entry hence SCMI driver can play with
> > several messages.
> >
> > Use value of 20 to mimic mailbox transport implementation.
>
> What is the need to mimic ?

I had to pick a value. I can't say whether 2, 5 or 20 is better.
I looks how the mailbox transport did and used the same value
as it seemed reasonable regarding its memory cost.

>
> > Any high value could fit. This should be something configurable.
>
> Why not 10 or 100 ? I see any value other than 1 is useless as we lock
> the channel in send_message and we don't maintain a queue like mailbox.

I'll check again.
Playing with SCMI voltage domain [1], it happens that I needed several
preallocated
message buffers unless what regulators fail to be probed.

[1] https://lkml.org/lkml/2020/10/5/1341

Regards,
etienne

>
> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool
  2020-10-09 12:43     ` Etienne Carriere
@ 2020-10-09 15:17       ` Sudeep Holla
  2020-10-12  8:57         ` Sudeep Holla
  0 siblings, 1 reply; 27+ messages in thread
From: Sudeep Holla @ 2020-10-09 15:17 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty,
	Sudeep Holla, Peng Fan

On Fri, Oct 09, 2020 at 02:43:31PM +0200, Etienne Carriere wrote:
> On Thu, 8 Oct 2020 at 23:11, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 08, 2020 at 04:37:21PM +0200, Etienne Carriere wrote:
> > > There is no reason for the smc transport to restrict itself to a 1
> > > message pool. More can be allocated, messages are copied from/to the
> > > shared memory only on SMC exit/entry hence SCMI driver can play with
> > > several messages.
> > >
> > > Use value of 20 to mimic mailbox transport implementation.
> >
> > What is the need to mimic ?
> 
> I had to pick a value. I can't say whether 2, 5 or 20 is better.
> I looks how the mailbox transport did and used the same value
> as it seemed reasonable regarding its memory cost.
> 
> >
> > > Any high value could fit. This should be something configurable.
> >
> > Why not 10 or 100 ? I see any value other than 1 is useless as we lock
> > the channel in send_message and we don't maintain a queue like mailbox.
> 
> I'll check again.
> Playing with SCMI voltage domain [1], it happens that I needed several
> preallocated message buffers unless what regulators fail to be probed.


I may be missing something but I can't see how, we simply block in
send_message while mailbox has a queue of 20 which is why it has 20 there.

The issue you are seeing could be different. Let me know if I am missing
something.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization
  2020-10-09 12:38     ` Etienne Carriere
@ 2020-10-09 15:20       ` Sudeep Holla
  0 siblings, 0 replies; 27+ messages in thread
From: Sudeep Holla @ 2020-10-09 15:20 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty

On Fri, Oct 09, 2020 at 02:38:16PM +0200, Etienne Carriere wrote:
> On Thu, 8 Oct 2020 at 23:18, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 08, 2020 at 04:37:19PM +0200, Etienne Carriere wrote:
> > > Implement helper function scmi_do_xfer_again() to process consecutive
> > > transfers that are initialized only once with scmi_xfer_get_init()
> > > and hence get the pool completion and responses message length not
> > > reloaded regarding last completed transfer.
> > >
> > > Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
> > > ---
> > >  drivers/firmware/arm_scmi/base.c    |  2 +-
> > >  drivers/firmware/arm_scmi/clock.c   |  2 +-
> > >  drivers/firmware/arm_scmi/common.h  |  2 ++
> > >  drivers/firmware/arm_scmi/driver.c  | 10 ++++++++++
> > >  drivers/firmware/arm_scmi/perf.c    |  2 +-
> > >  drivers/firmware/arm_scmi/sensors.c |  2 +-
> > >  6 files changed, 16 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > > index 9853bd3c4d45..508f214baa1b 100644
> > > --- a/drivers/firmware/arm_scmi/base.c
> > > +++ b/drivers/firmware/arm_scmi/base.c
> > > @@ -183,7 +183,7 @@ static int scmi_base_implementation_list_get(const struct scmi_handle *handle,
> > >               /* Set the number of protocols to be skipped/already read */
> > >               *num_skip = cpu_to_le32(tot_num_ret);
> > >
> > > -             ret = scmi_do_xfer(handle, t);
> > > +             ret = scmi_do_xfer_again(handle, t);
> > >               if (ret)
> > >                       break;
> > >
> > > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > > index c1cfe3ee3d55..9bb54c1a8d55 100644
> > > --- a/drivers/firmware/arm_scmi/clock.c
> > > +++ b/drivers/firmware/arm_scmi/clock.c
> > > @@ -161,7 +161,7 @@ scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
> > >               /* Set the number of rates to be skipped/already read */
> > >               clk_desc->rate_index = cpu_to_le32(tot_rate_cnt);
> > >
> > > -             ret = scmi_do_xfer(handle, t);
> > > +             ret = scmi_do_xfer_again(handle, t);
> > >               if (ret)
> > >                       goto err;
> > >
> > > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > > index 37fb583f1bf5..6d4eea7b0f3e 100644
> > > --- a/drivers/firmware/arm_scmi/common.h
> > > +++ b/drivers/firmware/arm_scmi/common.h
> > > @@ -143,6 +143,8 @@ struct scmi_xfer {
> > >
> > >  void scmi_xfer_put(const struct scmi_handle *h, struct scmi_xfer *xfer);
> > >  int scmi_do_xfer(const struct scmi_handle *h, struct scmi_xfer *xfer);
> > > +int scmi_do_xfer_again(const struct scmi_handle *handle,
> > > +                    struct scmi_xfer *xfer);
> > >  int scmi_do_xfer_with_response(const struct scmi_handle *h,
> > >                              struct scmi_xfer *xfer);
> > >  int scmi_xfer_get_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id,
> > > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> > > index c5dea87edf8f..887cb8249db0 100644
> > > --- a/drivers/firmware/arm_scmi/driver.c
> > > +++ b/drivers/firmware/arm_scmi/driver.c
> > > @@ -402,6 +402,16 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
> > >       return ret;
> > >  }
> > >
> > > +int scmi_do_xfer_again(const struct scmi_handle *handle, struct scmi_xfer *xfer)
> > > +{
> > > +     struct scmi_info *info = handle_to_scmi_info(handle);
> > > +
> > > +     xfer->rx.len = info->desc->max_msg_size;
> >
> > I am tempted to just have helper for above and use it where needed.
> > Or may be I just don't like the name of the function, how about
> > scmi_do_xfer_rxlen_reinit or anything else. Suggestions ?
> 
> I think a smoother way would be that scmi_do_xfer() initializes
> both
>   xfer->rx.len = info->desc->max_msg_size

Possibly

> and
>   xfer->hdr.poll_completion = false
> instead of doing that from scmi_xfer_get_init().
>
> >
> > > +     xfer->hdr.poll_completion = false;
> >
> > Do we really need the above ?
> 
> I think so. Once a transfer is completed, poll_completion is true.

Where and how ? By default it is always false and it can be set to true
only by perf set/get calls. So I still see no need for this.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport
  2020-10-09 12:33     ` Etienne Carriere
@ 2020-10-09 15:58       ` Sudeep Holla
  2020-10-12 10:11         ` Etienne Carriere
  0 siblings, 1 reply; 27+ messages in thread
From: Sudeep Holla @ 2020-10-09 15:58 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty

On Fri, Oct 09, 2020 at 02:33:41PM +0200, Etienne Carriere wrote:
> On Thu, 8 Oct 2020 at 23:08, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 08, 2020 at 04:37:20PM +0200, Etienne Carriere wrote:
> > > Fix dependencies for configuration switch ARM_SCMI_PROTOCOL that
> > > is not exclusively dependent on MAILBOX since the alternate
> > > smc transport that is depends on HAVE_ARM_SMCCC_DISCOVERY since [1].
> > >
> >
> > Do you need any build issues ? I don't see why this is needed.
> >
> 
> This change is for consistency of the kernel configuration.
> Without this change, a kernel configured without CONFIG_MAILBOX
> cannot embed SCMI support even is using only the SMC transport
> enabled thanks to HAVE_ARM_SMCCC_DISCOVERY.
> 

Fair enough, however instead of adding to the list for each added transport
we need to do better transport abstraction now that we have multiple.
I don't see this as critical, let me know if you disagree.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols
  2020-10-09 12:31   ` Etienne Carriere
@ 2020-10-09 16:31     ` Cristian Marussi
  2020-10-12  9:32     ` Sudeep Holla
  1 sibling, 0 replies; 27+ messages in thread
From: Cristian Marussi @ 2020-10-09 16:31 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: Sudeep Holla, linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Vincent Guittot, Souvik Chakravarty, cristian.marussi

Hi Etienne,

On Fri, Oct 09, 2020 at 02:31:55PM +0200, Etienne Carriere wrote:
> On Thu, 8 Oct 2020 at 21:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 08, 2020 at 04:37:18PM +0200, Etienne Carriere wrote:
> > > Remove the IDR replacement that prevent initializing an SCMI protocol
> > > when it has already been initialized. This is needed when there are
> > > several SCMI agents that do implement a given SCMI protocol unless
> > > what only the related SCMI protocol communication is initialized only
> > > for first probed agent.
> > >
> >
> > Can you please elaborate on your usecase please. What do you mean by several
> > SCMI agents here. OSPM is the only agent we are interested here. What
> > other agents is this driver supposed to handle here. We allocate memory
> > in init and calling init multiple times messes up the allocated and
> > initialised structures.
> >
> > So NACK for this patch as it needs more work if we need this at all.
> >
> 
> Hello Sudeep,
> 
> Considering a device with several SCMI servers spread over several co-processor
> and possibly also in the Arm TZ secure world, each of these servers
> uses a specific
> SCMI channel. Without this change, each SCMI protocol gets initialized
> only for the
> first agent device that is probed.
> 
> My setup is also a bit specific. My device has several secure configuration
> features that can individually be enabled or not. For example, configuring
> domain X as secure makes some clocks reachable by Linux only through SCMI,
> and configuring domain Y as secure makes other clocks reachable by Linux
> only through SCMI. For flexibility, I expose domain X resources (here clocks)
> to an Linux agent whereas domain Y resources (here clocks also) are
> exposed to another agent, each agent with its specific transport/channel.
> Enabling each agent node in the Linux FDT allows to define which SCMI clocks
> get exposed and hence registered in the kernel.
> Without the change proposed here, I cannot get the clocks exposed to both
> agents when enabled as the SCMI clock protocol is initialized only for the 1st
> probbed agent device.
> 

Just a heads up that I have a series to be posted soon(ish) that is
meant to address vendor protocols support and more in general protocols
modularization by simplifying and reorganizing 'a bit' the whole
initialization process and the handle interface itself: in that context
I've got rid as a whole of the above protocol initialization code inside
device probing and moved it at the time of first usage of the protocol,
i.e. when the first SCMI driver, inside its probe, attempts to get hold
of a protocol issuing something like:

	perf_ops = handle->get_ops(handle, SCMI_PROTOCOL_PERF);

This will effectively trigger a run of the usual protocol initialization
code if the protocol was still NOT initialized (no previous get_ops) for
that SCMI instance (server): so if your SCMI driver/device gets instantiated
multiple times against multiple different servers, the needed protocol(s)
will be initialized multiple times one for each instance (handle) upon
first usage, while any other further instance of another SCMI driver
requesting the same, already initialized, protocol will find it ready to go.

Cleanup/deinitialization is delegated to a corresponding handle->put_ops(),
while the general notifications will still be available using the current
.notify_ops() interface, but those requests will be now tracked
internally against the relevant protocol in order to avoid premature
deinitialization or removal of still in-use protocols.

Thanks

Cristian

> Regards,
> Etienne
> 
> > --
> > Regards,
> > Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool
  2020-10-09 15:17       ` Sudeep Holla
@ 2020-10-12  8:57         ` Sudeep Holla
  2020-10-12  9:12           ` Etienne Carriere
  0 siblings, 1 reply; 27+ messages in thread
From: Sudeep Holla @ 2020-10-12  8:57 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty,
	Sudeep Holla, Peng Fan

On Fri, Oct 09, 2020 at 04:17:52PM +0100, Sudeep Holla wrote:
> On Fri, Oct 09, 2020 at 02:43:31PM +0200, Etienne Carriere wrote:
> > On Thu, 8 Oct 2020 at 23:11, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Oct 08, 2020 at 04:37:21PM +0200, Etienne Carriere wrote:
> > > > There is no reason for the smc transport to restrict itself to a 1
> > > > message pool. More can be allocated, messages are copied from/to the
> > > > shared memory only on SMC exit/entry hence SCMI driver can play with
> > > > several messages.
> > > >
> > > > Use value of 20 to mimic mailbox transport implementation.
> > >
> > > What is the need to mimic ?
> > 
> > I had to pick a value. I can't say whether 2, 5 or 20 is better.
> > I looks how the mailbox transport did and used the same value
> > as it seemed reasonable regarding its memory cost.
> > 
> > >
> > > > Any high value could fit. This should be something configurable.
> > >
> > > Why not 10 or 100 ? I see any value other than 1 is useless as we lock
> > > the channel in send_message and we don't maintain a queue like mailbox.
> > 
> > I'll check again.
> > Playing with SCMI voltage domain [1], it happens that I needed several
> > preallocated message buffers unless what regulators fail to be probed.
> 
> 
> I may be missing something but I can't see how, we simply block in
> send_message while mailbox has a queue of 20 which is why it has 20 there.
> 
> The issue you are seeing could be different. Let me know if I am missing
> something.
> 

OK, I gave this some thought and realise that in-order to allow multiple
requests simultaneously, we do need this value > 1. I will take this
and make some tweaks to the commit log to indicate the same.

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool
  2020-10-12  8:57         ` Sudeep Holla
@ 2020-10-12  9:12           ` Etienne Carriere
  0 siblings, 0 replies; 27+ messages in thread
From: Etienne Carriere @ 2020-10-12  9:12 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty, Peng Fan

On Mon, 12 Oct 2020 at 10:57, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Oct 09, 2020 at 04:17:52PM +0100, Sudeep Holla wrote:
> > On Fri, Oct 09, 2020 at 02:43:31PM +0200, Etienne Carriere wrote:
> > > On Thu, 8 Oct 2020 at 23:11, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Thu, Oct 08, 2020 at 04:37:21PM +0200, Etienne Carriere wrote:
> > > > > There is no reason for the smc transport to restrict itself to a 1
> > > > > message pool. More can be allocated, messages are copied from/to the
> > > > > shared memory only on SMC exit/entry hence SCMI driver can play with
> > > > > several messages.
> > > > >
> > > > > Use value of 20 to mimic mailbox transport implementation.
> > > >
> > > > What is the need to mimic ?
> > >
> > > I had to pick a value. I can't say whether 2, 5 or 20 is better.
> > > I looks how the mailbox transport did and used the same value
> > > as it seemed reasonable regarding its memory cost.
> > >
> > > >
> > > > > Any high value could fit. This should be something configurable.
> > > >
> > > > Why not 10 or 100 ? I see any value other than 1 is useless as we lock
> > > > the channel in send_message and we don't maintain a queue like mailbox.
> > >
> > > I'll check again.
> > > Playing with SCMI voltage domain [1], it happens that I needed several
> > > preallocated message buffers unless what regulators fail to be probed.
> >
> >
> > I may be missing something but I can't see how, we simply block in
> > send_message while mailbox has a queue of 20 which is why it has 20 there.
> >
> > The issue you are seeing could be different. Let me know if I am missing
> > something.
> >
>
> OK, I gave this some thought and realise that in-order to allow multiple
> requests simultaneously, we do need this value > 1. I will take this
> and make some tweaks to the commit log to indicate the same.
>

Thanks for the feedback.
I planned to look back which value would really make sense.
Whatever, feel free to tweak or change this proposal.

Regards,
etienne

> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols
  2020-10-09 12:31   ` Etienne Carriere
  2020-10-09 16:31     ` Cristian Marussi
@ 2020-10-12  9:32     ` Sudeep Holla
  1 sibling, 0 replies; 27+ messages in thread
From: Sudeep Holla @ 2020-10-12  9:32 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Sudeep Holla,
	Souvik Chakravarty

On Fri, Oct 09, 2020 at 02:31:55PM +0200, Etienne Carriere wrote:
> On Thu, 8 Oct 2020 at 21:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Thu, Oct 08, 2020 at 04:37:18PM +0200, Etienne Carriere wrote:
> > > Remove the IDR replacement that prevent initializing an SCMI protocol
> > > when it has already been initialized. This is needed when there are
> > > several SCMI agents that do implement a given SCMI protocol unless
> > > what only the related SCMI protocol communication is initialized only
> > > for first probed agent.
> > >
> >
> > Can you please elaborate on your usecase please. What do you mean by several
> > SCMI agents here. OSPM is the only agent we are interested here. What
> > other agents is this driver supposed to handle here. We allocate memory
> > in init and calling init multiple times messes up the allocated and
> > initialised structures.
> >
> > So NACK for this patch as it needs more work if we need this at all.
> >
>
> Hello Sudeep,
>
> Considering a device with several SCMI servers spread over several co-processor
> and possibly also in the Arm TZ secure world, each of these servers
> uses a specific SCMI channel. Without this change, each SCMI protocol gets
> initialized only for the first agent device that is probed.
>

Fair enough, but it also adds complexity. Do you have a master SCMI server
implementation in that case. As some protocols like system might need to
be broadcast to all servers and master server needs to take appropriate
action.

> My setup is also a bit specific. My device has several secure configuration
> features that can individually be enabled or not. For example, configuring
> domain X as secure makes some clocks reachable by Linux only through SCMI,
> and configuring domain Y as secure makes other clocks reachable by Linux
> only through SCMI. For flexibility, I expose domain X resources (here clocks)
> to an Linux agent whereas domain Y resources (here clocks also) are
> exposed to another agent, each agent with its specific transport/channel.
> Enabling each agent node in the Linux FDT allows to define which SCMI clocks
> get exposed and hence registered in the kernel.
> Without the change proposed here, I cannot get the clocks exposed to both
> agents when enabled as the SCMI clock protocol is initialized only for the 1st
> probbed agent device.
>

OK, as Cristian has already mentioned we need to clean up a bit on these
initcalls and Cristian has some WIP patches, I would like to wait and look
at them instead of breaking other usecases with patch(multiple devices
per protocol within one scmi server)

-- 
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport
  2020-10-09 15:58       ` Sudeep Holla
@ 2020-10-12 10:11         ` Etienne Carriere
  0 siblings, 0 replies; 27+ messages in thread
From: Etienne Carriere @ 2020-10-12 10:11 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty

On Fri, 9 Oct 2020 at 17:58, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Fri, Oct 09, 2020 at 02:33:41PM +0200, Etienne Carriere wrote:
> > On Thu, 8 Oct 2020 at 23:08, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Thu, Oct 08, 2020 at 04:37:20PM +0200, Etienne Carriere wrote:
> > > > Fix dependencies for configuration switch ARM_SCMI_PROTOCOL that
> > > > is not exclusively dependent on MAILBOX since the alternate
> > > > smc transport that is depends on HAVE_ARM_SMCCC_DISCOVERY since [1].
> > > >
> > >
> > > Do you need any build issues ? I don't see why this is needed.
> > >
> >
> > This change is for consistency of the kernel configuration.
> > Without this change, a kernel configured without CONFIG_MAILBOX
> > cannot embed SCMI support even is using only the SMC transport
> > enabled thanks to HAVE_ARM_SMCCC_DISCOVERY.
> >
>
> Fair enough, however instead of adding to the list for each added transport
> we need to do better transport abstraction now that we have multiple.
> I don't see this as critical, let me know if you disagree.

Not critical, I agree :)

etienne

>
> --
> Regards,
> Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation
  2020-10-08 14:37 ` [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization Etienne Carriere
  2020-10-08 21:18   ` Sudeep Holla
@ 2020-10-12 14:17   ` Sudeep Holla
  2020-10-13  9:58     ` Etienne Carriere
  2020-10-13 10:50     ` Sudeep Holla
  1 sibling, 2 replies; 27+ messages in thread
From: Sudeep Holla @ 2020-10-12 14:17 UTC (permalink / raw)
  To: linux-kernel
  Cc: Sudeep Holla, linux-arm-kernel, Etienne Carriere,
	Cristian Marussi, vincent.guittot, Souvik.Chakravarty

Few commands provide the list of description partially and require
to be called consecutively until all the descriptors are fetched
completely. In such cases, we don't release the buffers and reuse
them for consecutive transmits.

However, currently we don't reset the Rx size which will be set as
per the response for the last transmit. This may result in incorrect
response size being interpretted as the firmware may repond with size
greater than the one set but we read only upto the size set by previous
response.

Let us reset the receive buffer size to max possible in such cases as
we don't know the exact size of the response.

Fixes: b6f20ff8bd94 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
Reported-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/base.c    | 2 ++
 drivers/firmware/arm_scmi/clock.c   | 2 ++
 drivers/firmware/arm_scmi/common.h  | 8 ++++++++
 drivers/firmware/arm_scmi/perf.c    | 2 ++
 drivers/firmware/arm_scmi/sensors.c | 2 ++
 5 files changed, 16 insertions(+)

Hi Etienne,

I reworked this in a different way and hence dropped your authorship and added
reported by. If you prefer I can attribute you as author. I want to push
2,4,5/5 as fixes and hence the rush.

Regards,
Sudeep

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index 9853bd3c4d45..017e5d8bd869 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -197,6 +197,8 @@ static int scmi_base_implementation_list_get(const struct scmi_handle *handle,
 			protocols_imp[tot_num_ret + loop] = *(list + loop);
 
 		tot_num_ret += loop_num_ret;
+
+		scmi_reset_rx_to_maxsz(handle, t);
 	} while (loop_num_ret);
 
 	scmi_xfer_put(handle, t);
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index c1cfe3ee3d55..4645677d86f1 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -192,6 +192,8 @@ scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
 		}
 
 		tot_rate_cnt += num_returned;
+
+		scmi_reset_rx_to_maxsz(handle, t);
 		/*
 		 * check for both returned and remaining to avoid infinite
 		 * loop due to buggy firmware
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 37fb583f1bf5..a3f1bc44b1de 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -245,6 +245,14 @@ extern const struct scmi_desc scmi_mailbox_desc;
 extern const struct scmi_desc scmi_smc_desc;
 #endif
 
+static inline void scmi_reset_rx_to_maxsz(const struct scmi_handle *handle,
+					  struct scmi_xfer *xfer)
+{
+	struct scmi_info *info = handle_to_scmi_info(handle);
+
+	xfer->rx.len = info->desc->max_msg_size;
+}
+
 void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr);
 void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id);
 
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index ed475b40bd08..82fb3babff72 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -304,6 +304,8 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
 		}
 
 		tot_opp_cnt += num_returned;
+
+		scmi_reset_rx_to_maxsz(handle, t);
 		/*
 		 * check for both returned and remaining to avoid infinite
 		 * loop due to buggy firmware
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 9703cf6356a0..b4232d611033 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -166,6 +166,8 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
 		}
 
 		desc_index += num_returned;
+
+		scmi_reset_rx_to_maxsz(handle, t);
 		/*
 		 * check for both returned and remaining to avoid infinite
 		 * loop due to buggy firmware
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 27+ messages in thread

* Re: [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation
  2020-10-12 14:17   ` [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation Sudeep Holla
@ 2020-10-13  9:58     ` Etienne Carriere
  2020-10-13 10:16       ` Sudeep Holla
  2020-10-13 10:50     ` Sudeep Holla
  1 sibling, 1 reply; 27+ messages in thread
From: Etienne Carriere @ 2020-10-13  9:58 UTC (permalink / raw)
  To: Sudeep Holla
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty

On Mon, 12 Oct 2020 at 16:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Few commands provide the list of description partially and require
> to be called consecutively until all the descriptors are fetched
> completely. In such cases, we don't release the buffers and reuse
> them for consecutive transmits.
>
> However, currently we don't reset the Rx size which will be set as
> per the response for the last transmit. This may result in incorrect
> response size being interpretted as the firmware may repond with size
> greater than the one set but we read only upto the size set by previous
> response.
>
> Let us reset the receive buffer size to max possible in such cases as
> we don't know the exact size of the response.
>
> Fixes: b6f20ff8bd94 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> Reported-by: Etienne Carriere <etienne.carriere@linaro.org>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/base.c    | 2 ++
>  drivers/firmware/arm_scmi/clock.c   | 2 ++
>  drivers/firmware/arm_scmi/common.h  | 8 ++++++++
>  drivers/firmware/arm_scmi/perf.c    | 2 ++
>  drivers/firmware/arm_scmi/sensors.c | 2 ++
>  5 files changed, 16 insertions(+)
>
> Hi Etienne,
>
> I reworked this in a different way and hence dropped your authorship and added
> reported by. If you prefer I can attribute you as author. I want to push
> 2,4,5/5 as fixes and hence the rush.

Hi Sudeep,

Tags are fine like that.
As for the content, it looks good to me.
When trying to apply this, I failed, but I guess I'm not testing over
the same kernel tree/branch as you.
All in one, I am really fine with this change, I think it does the job

Regards,
Etienne

>
> Regards,
> Sudeep
>
> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> index 9853bd3c4d45..017e5d8bd869 100644
> --- a/drivers/firmware/arm_scmi/base.c
> +++ b/drivers/firmware/arm_scmi/base.c
> @@ -197,6 +197,8 @@ static int scmi_base_implementation_list_get(const struct scmi_handle *handle,
>                         protocols_imp[tot_num_ret + loop] = *(list + loop);
>
>                 tot_num_ret += loop_num_ret;
> +
> +               scmi_reset_rx_to_maxsz(handle, t);
>         } while (loop_num_ret);
>
>         scmi_xfer_put(handle, t);
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index c1cfe3ee3d55..4645677d86f1 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -192,6 +192,8 @@ scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
>                 }
>
>                 tot_rate_cnt += num_returned;
> +
> +               scmi_reset_rx_to_maxsz(handle, t);
>                 /*
>                  * check for both returned and remaining to avoid infinite
>                  * loop due to buggy firmware
> diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> index 37fb583f1bf5..a3f1bc44b1de 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -245,6 +245,14 @@ extern const struct scmi_desc scmi_mailbox_desc;
>  extern const struct scmi_desc scmi_smc_desc;
>  #endif
>
> +static inline void scmi_reset_rx_to_maxsz(const struct scmi_handle *handle,
> +                                         struct scmi_xfer *xfer)
> +{
> +       struct scmi_info *info = handle_to_scmi_info(handle);
> +
> +       xfer->rx.len = info->desc->max_msg_size;
> +}
> +
>  void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr);
>  void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id);
>
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index ed475b40bd08..82fb3babff72 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -304,6 +304,8 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
>                 }
>
>                 tot_opp_cnt += num_returned;
> +
> +               scmi_reset_rx_to_maxsz(handle, t);
>                 /*
>                  * check for both returned and remaining to avoid infinite
>                  * loop due to buggy firmware
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 9703cf6356a0..b4232d611033 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -166,6 +166,8 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
>                 }
>
>                 desc_index += num_returned;
> +
> +               scmi_reset_rx_to_maxsz(handle, t);
>                 /*
>                  * check for both returned and remaining to avoid infinite
>                  * loop due to buggy firmware
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation
  2020-10-13  9:58     ` Etienne Carriere
@ 2020-10-13 10:16       ` Sudeep Holla
  0 siblings, 0 replies; 27+ messages in thread
From: Sudeep Holla @ 2020-10-13 10:16 UTC (permalink / raw)
  To: Etienne Carriere
  Cc: linux-kernel,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
	Cristian Marussi, Vincent Guittot, Souvik Chakravarty

On Tue, Oct 13, 2020 at 11:58:09AM +0200, Etienne Carriere wrote:
> On Mon, 12 Oct 2020 at 16:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > Few commands provide the list of description partially and require
> > to be called consecutively until all the descriptors are fetched
> > completely. In such cases, we don't release the buffers and reuse
> > them for consecutive transmits.
> >
> > However, currently we don't reset the Rx size which will be set as
> > per the response for the last transmit. This may result in incorrect
> > response size being interpretted as the firmware may repond with size
> > greater than the one set but we read only upto the size set by previous
> > response.
> >
> > Let us reset the receive buffer size to max possible in such cases as
> > we don't know the exact size of the response.
> >
> > Fixes: b6f20ff8bd94 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> > Reported-by: Etienne Carriere <etienne.carriere@linaro.org>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/base.c    | 2 ++
> >  drivers/firmware/arm_scmi/clock.c   | 2 ++
> >  drivers/firmware/arm_scmi/common.h  | 8 ++++++++
> >  drivers/firmware/arm_scmi/perf.c    | 2 ++
> >  drivers/firmware/arm_scmi/sensors.c | 2 ++
> >  5 files changed, 16 insertions(+)
> >
> > Hi Etienne,
> >
> > I reworked this in a different way and hence dropped your authorship and added
> > reported by. If you prefer I can attribute you as author. I want to push
> > 2,4,5/5 as fixes and hence the rush.
> 
> Hi Sudeep,
> 
> Tags are fine like that.
> As for the content, it looks good to me.

Thanks.

> When trying to apply this, I failed, but I guess I'm not testing over
> the same kernel tree/branch as you.

Ah OK. I wasn't sure if we had touch code around these recently.
Anyway, I have all these on my for-next/scmi[1][2]

--
Regards,
Sudeep

[1] https://git.kernel.org/sudeep.holla/linux/h/for-next/scmi
[2] git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git for-next/scmi

> All in one, I am really fine with this change, I think it does the job
> 
> Regards,
> Etienne
> 
> >
> > Regards,
> > Sudeep
> >
> > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > index 9853bd3c4d45..017e5d8bd869 100644
> > --- a/drivers/firmware/arm_scmi/base.c
> > +++ b/drivers/firmware/arm_scmi/base.c
> > @@ -197,6 +197,8 @@ static int scmi_base_implementation_list_get(const struct scmi_handle *handle,
> >                         protocols_imp[tot_num_ret + loop] = *(list + loop);
> >
> >                 tot_num_ret += loop_num_ret;
> > +
> > +               scmi_reset_rx_to_maxsz(handle, t);
> >         } while (loop_num_ret);
> >
> >         scmi_xfer_put(handle, t);
> > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > index c1cfe3ee3d55..4645677d86f1 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -192,6 +192,8 @@ scmi_clock_describe_rates_get(const struct scmi_handle *handle, u32 clk_id,
> >                 }
> >
> >                 tot_rate_cnt += num_returned;
> > +
> > +               scmi_reset_rx_to_maxsz(handle, t);
> >                 /*
> >                  * check for both returned and remaining to avoid infinite
> >                  * loop due to buggy firmware
> > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
> > index 37fb583f1bf5..a3f1bc44b1de 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -245,6 +245,14 @@ extern const struct scmi_desc scmi_mailbox_desc;
> >  extern const struct scmi_desc scmi_smc_desc;
> >  #endif
> >
> > +static inline void scmi_reset_rx_to_maxsz(const struct scmi_handle *handle,
> > +                                         struct scmi_xfer *xfer)
> > +{
> > +       struct scmi_info *info = handle_to_scmi_info(handle);
> > +
> > +       xfer->rx.len = info->desc->max_msg_size;
> > +}
> > +
> >  void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr);
> >  void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id);
> >
> > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> > index ed475b40bd08..82fb3babff72 100644
> > --- a/drivers/firmware/arm_scmi/perf.c
> > +++ b/drivers/firmware/arm_scmi/perf.c
> > @@ -304,6 +304,8 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
> >                 }
> >
> >                 tot_opp_cnt += num_returned;
> > +
> > +               scmi_reset_rx_to_maxsz(handle, t);
> >                 /*
> >                  * check for both returned and remaining to avoid infinite
> >                  * loop due to buggy firmware
> > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> > index 9703cf6356a0..b4232d611033 100644
> > --- a/drivers/firmware/arm_scmi/sensors.c
> > +++ b/drivers/firmware/arm_scmi/sensors.c
> > @@ -166,6 +166,8 @@ static int scmi_sensor_description_get(const struct scmi_handle *handle,
> >                 }
> >
> >                 desc_index += num_returned;
> > +
> > +               scmi_reset_rx_to_maxsz(handle, t);
> >                 /*
> >                  * check for both returned and remaining to avoid infinite
> >                  * loop due to buggy firmware
> > --
> > 2.17.1
> >

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols
  2020-10-08 14:37 [PATCH 1/5] firmware: arm_scmi: always initialize protocols Etienne Carriere
                   ` (4 preceding siblings ...)
  2020-10-08 19:17 ` [PATCH 1/5] firmware: arm_scmi: always initialize protocols Sudeep Holla
@ 2020-10-13 10:45 ` Sudeep Holla
  5 siblings, 0 replies; 27+ messages in thread
From: Sudeep Holla @ 2020-10-13 10:45 UTC (permalink / raw)
  To: linux-kernel, Etienne Carriere
  Cc: Sudeep Holla, Cristian Marussi, Vincent Guittot,
	linux-arm-kernel, Souvik Chakravarty

On Thu, 8 Oct 2020 16:37:18 +0200, Etienne Carriere wrote:
> Remove the IDR replacement that prevent initializing an SCMI protocol
> when it has already been initialized. This is needed when there are
> several SCMI agents that do implement a given SCMI protocol unless
> what only the related SCMI protocol communication is initialized only
> for first probed agent.

I am picking up 4-5.

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[4/5] firmware: arm_scmi: Expand SMC/HVC message pool to more than one
      https://git.kernel.org/sudeep.holla/c/7adb2c8aaa
[5/5] firmware: arm_scmi: Fix ARCH_COLD_RESET
      https://git.kernel.org/sudeep.holla/c/45b9e04d5b

--

Regards,
Sudeep


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation
  2020-10-12 14:17   ` [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation Sudeep Holla
  2020-10-13  9:58     ` Etienne Carriere
@ 2020-10-13 10:50     ` Sudeep Holla
  1 sibling, 0 replies; 27+ messages in thread
From: Sudeep Holla @ 2020-10-13 10:50 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-arm-kernel, Etienne Carriere, Cristian Marussi,
	Sudeep Holla, vincent.guittot, Souvik.Chakravarty

On Mon, Oct 12, 2020 at 03:17:46PM +0100, Sudeep Holla wrote:
> Few commands provide the list of description partially and require
> to be called consecutively until all the descriptors are fetched
> completely. In such cases, we don't release the buffers and reuse
> them for consecutive transmits.
> 
> However, currently we don't reset the Rx size which will be set as
> per the response for the last transmit. This may result in incorrect
> response size being interpretted as the firmware may repond with size
> greater than the one set but we read only upto the size set by previous
> response.
> 
> Let us reset the receive buffer size to max possible in such cases as
> we don't know the exact size of the response.
>

Applied to sudeep.holla/linux (for-next/scmi), thanks!

[PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation
	https://git.kernel.org/sudeep.holla/c/9724722fde

--
Regards,
Sudeep

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2020-10-13 10:50 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-08 14:37 [PATCH 1/5] firmware: arm_scmi: always initialize protocols Etienne Carriere
2020-10-08 14:37 ` [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization Etienne Carriere
2020-10-08 21:18   ` Sudeep Holla
2020-10-09 12:38     ` Etienne Carriere
2020-10-09 15:20       ` Sudeep Holla
2020-10-12 14:17   ` [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation Sudeep Holla
2020-10-13  9:58     ` Etienne Carriere
2020-10-13 10:16       ` Sudeep Holla
2020-10-13 10:50     ` Sudeep Holla
2020-10-08 14:37 ` [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport Etienne Carriere
2020-10-08 21:08   ` Sudeep Holla
2020-10-09 12:33     ` Etienne Carriere
2020-10-09 15:58       ` Sudeep Holla
2020-10-12 10:11         ` Etienne Carriere
2020-10-08 14:37 ` [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool Etienne Carriere
2020-10-08 21:11   ` Sudeep Holla
2020-10-09 12:43     ` Etienne Carriere
2020-10-09 15:17       ` Sudeep Holla
2020-10-12  8:57         ` Sudeep Holla
2020-10-12  9:12           ` Etienne Carriere
2020-10-08 14:37 ` [PATCH 5/5] firmware: arm_scmi: fix ARCH_COLD_RESET Etienne Carriere
2020-10-08 21:16   ` Sudeep Holla
2020-10-08 19:17 ` [PATCH 1/5] firmware: arm_scmi: always initialize protocols Sudeep Holla
2020-10-09 12:31   ` Etienne Carriere
2020-10-09 16:31     ` Cristian Marussi
2020-10-12  9:32     ` Sudeep Holla
2020-10-13 10:45 ` Sudeep Holla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).