[PATCH 1/5] firmware: arm_scmi: always initialize protocols

[PATCH 1/5] firmware: arm_scmi: always initialize protocols

[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

[PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization

[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

Re: [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization

[Sudeep Holla]

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

Re: [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization

[Etienne Carriere]

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

Re: [PATCH 2/5] firmware: arm_scmi: fix transfer missing re-initialization

[Sudeep Holla]

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

[PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation

[Sudeep Holla]

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

Re: [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation

[Etienne Carriere]

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
>

Re: [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation

[Sudeep Holla]

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

Re: [PATCH] firmware: arm_scmi: Add missing Rx size re-initialisation

[Sudeep Holla]

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

[PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport

[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

Re: [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport

[Sudeep Holla]

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

Re: [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport

[Etienne Carriere]

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

Re: [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport

[Sudeep Holla]

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

Re: [PATCH 3/5] firmware: arm_scmi: add config dependency for smc transport

[Etienne Carriere]

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

[PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool

[Etienne Carriere]

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

Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool

[Sudeep Holla]

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

Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool

[Etienne Carriere]

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

Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool

[Sudeep Holla]

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

Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool

[Sudeep Holla]

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

Re: [PATCH 4/5] firmware: arm_scmi: smc transport supports multi-message pool

[Etienne Carriere]

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

[PATCH 5/5] firmware: arm_scmi: fix ARCH_COLD_RESET

[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

Re: [PATCH 5/5] firmware: arm_scmi: fix ARCH_COLD_RESET

[Sudeep Holla]

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

Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols

[Sudeep Holla]

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

Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols

[Etienne Carriere]

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

Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols

[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

Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols

[Sudeep Holla]

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

Re: [PATCH 1/5] firmware: arm_scmi: always initialize protocols

[Sudeep Holla]

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