[PATCH 00/22] SCMIv3.1 Miscellaneous changes

[PATCH 00/22] SCMIv3.1 Miscellaneous changes

[Cristian Marussi]

Hi all,

this series introduces a bunch of SCMIv3.1 miscellaneous changes to support
basically all the SCMIv3.1 specification [1] addition with the exclusion of
the Powercap protocol and driver which will be introduced later on in
another series.

Most notably the series adds:

 - supports across all protocols for long resources naming using *_NAME_GET
   dedicated new commands
 - Clock protocol Rate change pre and post notifications
 - Voltage protocol asynchronous voltage level set command
   (VOLTAGE_LEVEL_SET_COMPLETE delayed response)
 - Perf protocol power-cost in micro-watts (only internal support)
 - Perf protocol PERFORMANCE_LIMITS_SET	new checks

Beside this, the series starts with a few general fixes (01-08/22) and a
couple of refactoring:

 - one (09/22) simply to split out of common.h into a new protocols.h all
   the  structures needed only by protocol code, so that the protocol
   implementation can include such reduced header instead of the whole
   common.h which was growing insanely

 - another around the handling of multi-part commands.
   SCMI already allowed to issue some commands using a multi-message scheme
   through which a particularly big response, which could possibly not fit
   the underlying transport max payload size, could have been split into
   multiple chunks.
   Such logic, though, was scattered all across various protocols, leading
   to a lot of code duplication, so, before adding even more duplication
   with SCMIv3.1, I split out the common logic to a couple of helpers to
   handle such mechanism in a common way through some abstract iterators
   (13/22) and then ported all the protocols users of such mechanism to the
   common iterators (14,16-18/22).

SCMIv3.1 new features have been tested with an emulated backend server,
while refactoring has been chekced against a standard SCMI stack running
on a JUNO.

The series is based on sudeep/for-next/scmi/updates [2] on top of

commit 38a0e5b735d6 ("clk: scmi: Support atomic clock enable/disable API")

Any feedback very welcome,

Thanks,
Cristian


[1]: https://developer.arm.com/documentation/den0056/d/?lang=en
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git/log/?h=for-next/scmi/updates
---

Cristian Marussi (22):
  firmware: arm_scmi: Fix sorting of retrieved clock rates
  firmware: arm_scmi: Make protocols init fail on basic errors
  firmware: arm_scmi: Fix Base list protocols enumeration
  firmware: arm_scmi: Validate BASE_DISCOVER_LIST_PROTOCOLS reply
  firmware: arm_scmi: Dynamically allocate protocols array
  firmware: arm_scmi: Make name_get operations return a const
  firmware: arm_scmi: Check CLOCK_RATE_SET_COMPLETE async reply
  firmware: arm_scmi: Remove unneeded NULL termination of clk name
  firmware: arm_scmi: Split protocol specific definitions in a dedicated
    header
  firmware: arm_scmi: Introduce a common SCMIv3.1 .extended_name_get
    helper
  firmware: arm_scmi: Add SCMIv3.1 extended names protocols support
  firmware: arm_scmi: Parse clock_enable_latency conditionally
  firmware: arm_scmi: Add iterators for multi-part commands
  firmware: arm_scmi: Use common iterators in Sensor protocol
  firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support
  firmware: arm_scmi: Use common iterators in Clock protocol
  firmware: arm_scmi: Use common iterators in Voltage protocol
  firmware: arm_scmi: Use common iterators in Perf protocol
  firmware: arm_scmi: Add SCMIv3.1 Clock notifications
  firmware: arm_scmi: Add SCMIv3.1 VOLTAGE_LEVEL_SET_COMPLETE
  firmware: arm_scmi: Add SCMI v3.1 Perf power-cost in microwatts
  firmware: arm_scmi: Add SCMIv3.1 PERFORMANCE_LIMITS_SET checks

 drivers/firmware/arm_scmi/base.c      |  43 +-
 drivers/firmware/arm_scmi/clock.c     | 337 +++++++++++---
 drivers/firmware/arm_scmi/common.h    | 225 +--------
 drivers/firmware/arm_scmi/driver.c    | 165 ++++++-
 drivers/firmware/arm_scmi/perf.c      | 162 ++++---
 drivers/firmware/arm_scmi/power.c     |  44 +-
 drivers/firmware/arm_scmi/protocols.h | 318 +++++++++++++
 drivers/firmware/arm_scmi/reset.c     |  40 +-
 drivers/firmware/arm_scmi/sensors.c   | 645 +++++++++++++++-----------
 drivers/firmware/arm_scmi/system.c    |   9 +-
 drivers/firmware/arm_scmi/voltage.c   | 218 ++++++---
 include/linux/scmi_protocol.h         |  31 +-
 12 files changed, 1495 insertions(+), 742 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/protocols.h

-- 
2.32.0

[PATCH 01/22] firmware: arm_scmi: Fix sorting of retrieved clock rates

[Cristian Marussi]

During SCMI Clock protocol initialization, after having retrieved from the
SCMI platform all the available discrete rates for a specific clock, the
clock rates array is sorted, unfortunately using a pointer to its end as
a base instead of its start, so that sorting does not work.

Fix invocation of sort() passing as base a pointer to the start of the
retrieved clock rates array.

Fixes: dccec73de91 ("firmware: arm_scmi: Keep the discrete clock rates sorted")
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index cf6fed6dec77..ef6431c6eb1c 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -210,7 +210,8 @@ scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
 
 	if (rate_discrete && rate) {
 		clk->list.num_rates = tot_rate_cnt;
-		sort(rate, tot_rate_cnt, sizeof(*rate), rate_cmp_func, NULL);
+		sort(clk->list.rates, tot_rate_cnt, sizeof(*rate),
+		     rate_cmp_func, NULL);
 	}
 
 	clk->rate_discrete = rate_discrete;
-- 
2.32.0

[PATCH 02/22] firmware: arm_scmi: Make protocols init fail on basic errors

[Cristian Marussi]

Bail out of protocol initialization routine early when basic information
about protocol version and attributes could not be retrieved: failing to
act this way can lead to a successfully initialized SCMI protocol which
is in fact not fully functional.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/base.c    |  5 ++++-
 drivers/firmware/arm_scmi/clock.c   |  8 ++++++--
 drivers/firmware/arm_scmi/perf.c    | 10 +++++++---
 drivers/firmware/arm_scmi/power.c   | 10 +++++++---
 drivers/firmware/arm_scmi/reset.c   | 10 +++++++---
 drivers/firmware/arm_scmi/sensors.c |  4 +++-
 drivers/firmware/arm_scmi/system.c  |  5 ++++-
 7 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index f5219334fd3a..ebaef5d320af 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -359,7 +359,10 @@ static int scmi_base_protocol_init(const struct scmi_protocol_handle *ph)
 	rev->minor_ver = PROTOCOL_REV_MINOR(version);
 	ph->set_priv(ph, rev);
 
-	scmi_base_attributes_get(ph);
+	ret = scmi_base_attributes_get(ph);
+	if (ret)
+		return ret;
+
 	scmi_base_vendor_id_get(ph, false);
 	scmi_base_vendor_id_get(ph, true);
 	scmi_base_implementation_version_get(ph);
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index ef6431c6eb1c..0ae39ee920e9 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -361,7 +361,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
 	int clkid, ret;
 	struct clock_info *cinfo;
 
-	ph->xops->version_get(ph, &version);
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
 
 	dev_dbg(ph->dev, "Clock Version %d.%d\n",
 		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
@@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
 	if (!cinfo)
 		return -ENOMEM;
 
-	scmi_clock_protocol_attributes_get(ph, cinfo);
+	ret = scmi_clock_protocol_attributes_get(ph, cinfo);
+	if (ret)
+		return ret;
 
 	cinfo->clk = devm_kcalloc(ph->dev, cinfo->num_clocks,
 				  sizeof(*cinfo->clk), GFP_KERNEL);
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index f4cd5193b961..e9f68b91580c 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -873,11 +873,13 @@ static const struct scmi_protocol_events perf_protocol_events = {
 
 static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
 {
-	int domain;
+	int domain, ret;
 	u32 version;
 	struct scmi_perf_info *pinfo;
 
-	ph->xops->version_get(ph, &version);
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
 
 	dev_dbg(ph->dev, "Performance Version %d.%d\n",
 		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
@@ -886,7 +888,9 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
 	if (!pinfo)
 		return -ENOMEM;
 
-	scmi_perf_attributes_get(ph, pinfo);
+	ret = scmi_perf_attributes_get(ph, pinfo);
+	if (ret)
+		return ret;
 
 	pinfo->dom_info = devm_kcalloc(ph->dev, pinfo->num_domains,
 				       sizeof(*pinfo->dom_info), GFP_KERNEL);
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index ad2ab080f344..0f0b94f0b624 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -280,11 +280,13 @@ static const struct scmi_protocol_events power_protocol_events = {
 
 static int scmi_power_protocol_init(const struct scmi_protocol_handle *ph)
 {
-	int domain;
+	int domain, ret;
 	u32 version;
 	struct scmi_power_info *pinfo;
 
-	ph->xops->version_get(ph, &version);
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
 
 	dev_dbg(ph->dev, "Power Version %d.%d\n",
 		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
@@ -293,7 +295,9 @@ static int scmi_power_protocol_init(const struct scmi_protocol_handle *ph)
 	if (!pinfo)
 		return -ENOMEM;
 
-	scmi_power_attributes_get(ph, pinfo);
+	ret = scmi_power_attributes_get(ph, pinfo);
+	if (ret)
+		return ret;
 
 	pinfo->dom_info = devm_kcalloc(ph->dev, pinfo->num_domains,
 				       sizeof(*pinfo->dom_info), GFP_KERNEL);
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 9bf2478ec6d1..9cdbd133db10 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -293,11 +293,13 @@ static const struct scmi_protocol_events reset_protocol_events = {
 
 static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph)
 {
-	int domain;
+	int domain, ret;
 	u32 version;
 	struct scmi_reset_info *pinfo;
 
-	ph->xops->version_get(ph, &version);
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
 
 	dev_dbg(ph->dev, "Reset Version %d.%d\n",
 		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
@@ -306,7 +308,9 @@ static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph)
 	if (!pinfo)
 		return -ENOMEM;
 
-	scmi_reset_attributes_get(ph, pinfo);
+	ret = scmi_reset_attributes_get(ph, pinfo);
+	if (ret)
+		return ret;
 
 	pinfo->dom_info = devm_kcalloc(ph->dev, pinfo->num_domains,
 				       sizeof(*pinfo->dom_info), GFP_KERNEL);
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index cdbb287bd8bc..f37ac9824a87 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -966,7 +966,9 @@ static int scmi_sensors_protocol_init(const struct scmi_protocol_handle *ph)
 	int ret;
 	struct sensors_info *sinfo;
 
-	ph->xops->version_get(ph, &version);
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
 
 	dev_dbg(ph->dev, "Sensor Version %d.%d\n",
 		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
diff --git a/drivers/firmware/arm_scmi/system.c b/drivers/firmware/arm_scmi/system.c
index e5175ef73b40..cbfc19f7a463 100644
--- a/drivers/firmware/arm_scmi/system.c
+++ b/drivers/firmware/arm_scmi/system.c
@@ -113,10 +113,13 @@ static const struct scmi_protocol_events system_protocol_events = {
 
 static int scmi_system_protocol_init(const struct scmi_protocol_handle *ph)
 {
+	int ret;
 	u32 version;
 	struct scmi_system_info *pinfo;
 
-	ph->xops->version_get(ph, &version);
+	ret = ph->xops->version_get(ph, &version);
+	if (ret)
+		return ret;
 
 	dev_dbg(ph->dev, "System Power Version %d.%d\n",
 		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
-- 
2.32.0

Re: [PATCH 02/22] firmware: arm_scmi: Make protocols init fail on basic errors

[Sudeep Holla]

On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote:
> Bail out of protocol initialization routine early when basic information
> about protocol version and attributes could not be retrieved: failing to
> act this way can lead to a successfully initialized SCMI protocol which
> is in fact not fully functional.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/base.c    |  5 ++++-
>  drivers/firmware/arm_scmi/clock.c   |  8 ++++++--
>  drivers/firmware/arm_scmi/perf.c    | 10 +++++++---
>  drivers/firmware/arm_scmi/power.c   | 10 +++++++---
>  drivers/firmware/arm_scmi/reset.c   | 10 +++++++---
>  drivers/firmware/arm_scmi/sensors.c |  4 +++-
>  drivers/firmware/arm_scmi/system.c  |  5 ++++-
>  7 files changed, 38 insertions(+), 14 deletions(-)
>
> @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
>  	if (!cinfo)
>  		return -ENOMEM;
>
> -	scmi_clock_protocol_attributes_get(ph, cinfo);
> +	ret = scmi_clock_protocol_attributes_get(ph, cinfo);
> +	if (ret)
> +		return ret;

Does this result in removal of scmi_dev associated with devm_* calls ?
Otherwise we may need to free the allocated buffers ? I am not sure
if the dev here is individual scmi_dev or the platform scmi device.
I assume latter and it is unlikely to be removed/freed with the error in
the above path.

Similarly in couple of other instances/protocols.

--
Regards,
Sudeep

Re: [PATCH 02/22] firmware: arm_scmi: Make protocols init fail on basic errors

[Cristian Marussi]

On Tue, Apr 26, 2022 at 04:35:28PM +0100, Sudeep Holla wrote:
> On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote:
> > Bail out of protocol initialization routine early when basic information
> > about protocol version and attributes could not be retrieved: failing to
> > act this way can lead to a successfully initialized SCMI protocol which
> > is in fact not fully functional.
> >
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/base.c    |  5 ++++-
> >  drivers/firmware/arm_scmi/clock.c   |  8 ++++++--
> >  drivers/firmware/arm_scmi/perf.c    | 10 +++++++---
> >  drivers/firmware/arm_scmi/power.c   | 10 +++++++---
> >  drivers/firmware/arm_scmi/reset.c   | 10 +++++++---
> >  drivers/firmware/arm_scmi/sensors.c |  4 +++-
> >  drivers/firmware/arm_scmi/system.c  |  5 ++++-
> >  7 files changed, 38 insertions(+), 14 deletions(-)
> >

Hi Sudeep,

thanks for the review first of all...

> > @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> >  	if (!cinfo)
> >  		return -ENOMEM;
> >
> > -	scmi_clock_protocol_attributes_get(ph, cinfo);
> > +	ret = scmi_clock_protocol_attributes_get(ph, cinfo);
> > +	if (ret)
> > +		return ret;
> 
> Does this result in removal of scmi_dev associated with devm_* calls ?
> Otherwise we may need to free the allocated buffers ? I am not sure
> if the dev here is individual scmi_dev or the platform scmi device.
> I assume latter and it is unlikely to be removed/freed with the error in
> the above path.
> 
> Similarly in couple of other instances/protocols.

So, ph->dev used in the above devm_ is indeed the arm_scmi platform device
and I was *almost* gonna tell you 'Good catch', BUT then, rereading my own
code (O_o), I saw/remembered that when a protocol instance is initialized on
it first usage, there is indeed a devres_group internally managed by
the SCMI core, so that:

scmi_get_protocol_instance()

	@first_protocol_usage (refcount pi->users):

	--> scmi_get_protocol() // just in case was LKM proto
	--> scmi_alloc_init_protocol_instance()
		gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);

  		ret = pi->proto->instance_init(&pi->ph);
		  ====>>> i.e. scmi_clock_protocol_init(ph)
		if (ret)
			goto clean;
	.....

	   clean:
	   	devres_release_group(handle->dev, gid);


So basically all that happens at initialization time in scmi_clock_protocol_init,
BUT also everything that happens implicitly inside scmi_alloc_init_protocol_instance
during that protocol initialization (like the events registration) is undone on
failure transparently by the SCMI core init/free management functions
(via devres_ groups...)

All of the above is because each protocol is initialized only once on
its first usage, no matter how many SCMI driver users (and scmi_devs) are
using it...only in case (unsupported) we have multiple SCMI instances
(platforms) there will be one instance of protocol initialized per SCMI
server.

... having said that, now I'll go and double check (test) this behaviour since I
even had forgot about having implemented this kind of design :P

Thanks,
Cristian

Re: [PATCH 02/22] firmware: arm_scmi: Make protocols init fail on basic errors

[Sudeep Holla]

On Tue, Apr 26, 2022 at 05:25:28PM +0100, Cristian Marussi wrote:
> On Tue, Apr 26, 2022 at 04:35:28PM +0100, Sudeep Holla wrote:
> > On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote:
> > > Bail out of protocol initialization routine early when basic information
> > > about protocol version and attributes could not be retrieved: failing to
> > > act this way can lead to a successfully initialized SCMI protocol which
> > > is in fact not fully functional.
> > >
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > ---
> > >  drivers/firmware/arm_scmi/base.c    |  5 ++++-
> > >  drivers/firmware/arm_scmi/clock.c   |  8 ++++++--
> > >  drivers/firmware/arm_scmi/perf.c    | 10 +++++++---
> > >  drivers/firmware/arm_scmi/power.c   | 10 +++++++---
> > >  drivers/firmware/arm_scmi/reset.c   | 10 +++++++---
> > >  drivers/firmware/arm_scmi/sensors.c |  4 +++-
> > >  drivers/firmware/arm_scmi/system.c  |  5 ++++-
> > >  7 files changed, 38 insertions(+), 14 deletions(-)
> > >
> 
> Hi Sudeep,
> 
> thanks for the review first of all...
> 
> > > @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > >  	if (!cinfo)
> > >  		return -ENOMEM;
> > >
> > > -	scmi_clock_protocol_attributes_get(ph, cinfo);
> > > +	ret = scmi_clock_protocol_attributes_get(ph, cinfo);
> > > +	if (ret)
> > > +		return ret;
> > 
> > Does this result in removal of scmi_dev associated with devm_* calls ?
> > Otherwise we may need to free the allocated buffers ? I am not sure
> > if the dev here is individual scmi_dev or the platform scmi device.
> > I assume latter and it is unlikely to be removed/freed with the error in
> > the above path.
> > 
> > Similarly in couple of other instances/protocols.
> 
> So, ph->dev used in the above devm_ is indeed the arm_scmi platform device
> and I was *almost* gonna tell you 'Good catch', BUT then, rereading my own
> code (O_o), I saw/remembered that when a protocol instance is initialized on
> it first usage, there is indeed a devres_group internally managed by
> the SCMI core, so that:
> 
> scmi_get_protocol_instance()
> 
> 	@first_protocol_usage (refcount pi->users):
> 
> 	--> scmi_get_protocol() // just in case was LKM proto
> 	--> scmi_alloc_init_protocol_instance()
> 		gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
> 
>   		ret = pi->proto->instance_init(&pi->ph);
> 		  ====>>> i.e. scmi_clock_protocol_init(ph)
> 		if (ret)
> 			goto clean;
> 	.....
> 
> 	   clean:
> 	   	devres_release_group(handle->dev, gid);
> 
> 
> So basically all that happens at initialization time in scmi_clock_protocol_init,
> BUT also everything that happens implicitly inside scmi_alloc_init_protocol_instance
> during that protocol initialization (like the events registration) is undone on
> failure transparently by the SCMI core init/free management functions
> (via devres_ groups...)
> 
> All of the above is because each protocol is initialized only once on
> its first usage, no matter how many SCMI driver users (and scmi_devs) are
> using it...only in case (unsupported) we have multiple SCMI instances
> (platforms) there will be one instance of protocol initialized per SCMI
> server.
> 
> ... having said that, now I'll go and double check (test) this behaviour since I
> even had forgot about having implemented this kind of design :P
> 

Makes sense, thanks for the detailed explanation. I had totally forgotten how
devres_group works 🙁, my bad.

-- 
Regards,
Sudeep

Re: [PATCH 02/22] firmware: arm_scmi: Make protocols init fail on basic errors

[Cristian Marussi]

On Thu, Apr 28, 2022 at 11:25:24AM +0100, Sudeep Holla wrote:
> On Tue, Apr 26, 2022 at 05:25:28PM +0100, Cristian Marussi wrote:
> > On Tue, Apr 26, 2022 at 04:35:28PM +0100, Sudeep Holla wrote:
> > > On Wed, Mar 30, 2022 at 04:05:31PM +0100, Cristian Marussi wrote:
> > > > Bail out of protocol initialization routine early when basic information
> > > > about protocol version and attributes could not be retrieved: failing to
> > > > act this way can lead to a successfully initialized SCMI protocol which
> > > > is in fact not fully functional.
> > > >
> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > > ---
> > > >  drivers/firmware/arm_scmi/base.c    |  5 ++++-
> > > >  drivers/firmware/arm_scmi/clock.c   |  8 ++++++--
> > > >  drivers/firmware/arm_scmi/perf.c    | 10 +++++++---
> > > >  drivers/firmware/arm_scmi/power.c   | 10 +++++++---
> > > >  drivers/firmware/arm_scmi/reset.c   | 10 +++++++---
> > > >  drivers/firmware/arm_scmi/sensors.c |  4 +++-
> > > >  drivers/firmware/arm_scmi/system.c  |  5 ++++-
> > > >  7 files changed, 38 insertions(+), 14 deletions(-)
> > > >
> > 
> > Hi Sudeep,
> > 
> > thanks for the review first of all...
> > 
> > > > @@ -370,7 +372,9 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
> > > >  	if (!cinfo)
> > > >  		return -ENOMEM;
> > > >
> > > > -	scmi_clock_protocol_attributes_get(ph, cinfo);
> > > > +	ret = scmi_clock_protocol_attributes_get(ph, cinfo);
> > > > +	if (ret)
> > > > +		return ret;
> > > 
> > > Does this result in removal of scmi_dev associated with devm_* calls ?
> > > Otherwise we may need to free the allocated buffers ? I am not sure
> > > if the dev here is individual scmi_dev or the platform scmi device.
> > > I assume latter and it is unlikely to be removed/freed with the error in
> > > the above path.
> > > 
> > > Similarly in couple of other instances/protocols.
> > 
> > So, ph->dev used in the above devm_ is indeed the arm_scmi platform device
> > and I was *almost* gonna tell you 'Good catch', BUT then, rereading my own
> > code (O_o), I saw/remembered that when a protocol instance is initialized on
> > it first usage, there is indeed a devres_group internally managed by
> > the SCMI core, so that:
> > 
> > scmi_get_protocol_instance()
> > 
> > 	@first_protocol_usage (refcount pi->users):
> > 
> > 	--> scmi_get_protocol() // just in case was LKM proto
> > 	--> scmi_alloc_init_protocol_instance()
> > 		gid = devres_open_group(handle->dev, NULL, GFP_KERNEL);
> > 
> >   		ret = pi->proto->instance_init(&pi->ph);
> > 		  ====>>> i.e. scmi_clock_protocol_init(ph)
> > 		if (ret)
> > 			goto clean;
> > 	.....
> > 
> > 	   clean:
> > 	   	devres_release_group(handle->dev, gid);
> > 
> > 
> > So basically all that happens at initialization time in scmi_clock_protocol_init,
> > BUT also everything that happens implicitly inside scmi_alloc_init_protocol_instance
> > during that protocol initialization (like the events registration) is undone on
> > failure transparently by the SCMI core init/free management functions
> > (via devres_ groups...)
> > 
> > All of the above is because each protocol is initialized only once on
> > its first usage, no matter how many SCMI driver users (and scmi_devs) are
> > using it...only in case (unsupported) we have multiple SCMI instances
> > (platforms) there will be one instance of protocol initialized per SCMI
> > server.
> > 
> > ... having said that, now I'll go and double check (test) this behaviour since I
> > even had forgot about having implemented this kind of design :P
> > 
> 
> Makes sense, thanks for the detailed explanation. I had totally forgotten how
> devres_group works 🙁, my bad.
> 

Well I had even forgot to have used it in the SCMI core :P

Thanks,
Cristian

[PATCH 03/22] firmware: arm_scmi: Fix Base list protocols enumeration

[Cristian Marussi]

While enumerating protocols implemented by the SCMI backend server using
BASE_DISCOVER_LIST_PROTOCOLS, the number of returned protocols is currently
validated in an improper way since the check employs a sum between unsigned
integers that could overflow and cause the check itself to be silently
bypassed if the returned value 'loop_num_ret' is big enough.

Fix the validation avoiding the addition.

Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/base.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index ebaef5d320af..f279146f8110 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -197,7 +197,7 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
 			break;
 
 		loop_num_ret = le32_to_cpu(*num_ret);
-		if (tot_num_ret + loop_num_ret > MAX_PROTOCOLS_IMP) {
+		if (loop_num_ret > MAX_PROTOCOLS_IMP - tot_num_ret) {
 			dev_err(dev, "No. of Protocol > MAX_PROTOCOLS_IMP");
 			break;
 		}
-- 
2.32.0

[PATCH 04/22] firmware: arm_scmi: Validate BASE_DISCOVER_LIST_PROTOCOLS reply

[Cristian Marussi]

Do not blindly trust SCMI backend server reply about list of implemented
protocols, instead validate the reported length of the list of protocols
against the real payload size of the message reply.

Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index f279146f8110..c1165d1282ef 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
 	list = t->rx.buf + sizeof(*num_ret);
 
 	do {
+		size_t real_list_sz;
+		u32 calc_list_sz;
+
 		/* Set the number of protocols to be skipped/already read */
 		*num_skip = cpu_to_le32(tot_num_ret);
 
@@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
 			break;
 		}
 
+		if (t->rx.len < (sizeof(u32) * 2)) {
+			dev_err(dev, "Truncated reply - rx.len:%zd\n",
+				t->rx.len);
+			ret = -EPROTO;
+			break;
+		}
+
+		real_list_sz = t->rx.len - sizeof(u32);
+		calc_list_sz = ((loop_num_ret / sizeof(u32)) +
+				!!(loop_num_ret % sizeof(u32))) * sizeof(u32);
+		if (calc_list_sz != real_list_sz) {
+			dev_err(dev,
+				"Malformed reply - real_sz:%zd  calc_sz:%u\n",
+				real_list_sz, calc_list_sz);
+			ret = -EPROTO;
+			break;
+		}
+
 		for (loop = 0; loop < loop_num_ret; loop++)
 			protocols_imp[tot_num_ret + loop] = *(list + loop);
 
-- 
2.32.0

Re: [PATCH 04/22] firmware: arm_scmi: Validate BASE_DISCOVER_LIST_PROTOCOLS reply

[Sudeep Holla]

On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote:
> Do not blindly trust SCMI backend server reply about list of implemented
> protocols, instead validate the reported length of the list of protocols
> against the real payload size of the message reply.
>
> Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> index f279146f8110..c1165d1282ef 100644
> --- a/drivers/firmware/arm_scmi/base.c
> +++ b/drivers/firmware/arm_scmi/base.c
> @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
>  	list = t->rx.buf + sizeof(*num_ret);
>
>  	do {
> +		size_t real_list_sz;
> +		u32 calc_list_sz;
> +
>  		/* Set the number of protocols to be skipped/already read */
>  		*num_skip = cpu_to_le32(tot_num_ret);
>
> @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
>  			break;
>  		}
>
> +		if (t->rx.len < (sizeof(u32) * 2)) {
> +			dev_err(dev, "Truncated reply - rx.len:%zd\n",
> +				t->rx.len);
> +			ret = -EPROTO;
> +			break;
> +		}
> +
> +		real_list_sz = t->rx.len - sizeof(u32);
> +		calc_list_sz = ((loop_num_ret / sizeof(u32)) +
> +				!!(loop_num_ret % sizeof(u32))) * sizeof(u32);

Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ?

--
Regards,
Sudeep

Re: [PATCH 04/22] firmware: arm_scmi: Validate BASE_DISCOVER_LIST_PROTOCOLS reply

[Cristian Marussi]

On Thu, Apr 28, 2022 at 11:07:29AM +0100, Sudeep Holla wrote:
> On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote:
> > Do not blindly trust SCMI backend server reply about list of implemented
> > protocols, instead validate the reported length of the list of protocols
> > against the real payload size of the message reply.
> >
> > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > index f279146f8110..c1165d1282ef 100644
> > --- a/drivers/firmware/arm_scmi/base.c
> > +++ b/drivers/firmware/arm_scmi/base.c
> > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> >  	list = t->rx.buf + sizeof(*num_ret);
> >
> >  	do {
> > +		size_t real_list_sz;
> > +		u32 calc_list_sz;
> > +
> >  		/* Set the number of protocols to be skipped/already read */
> >  		*num_skip = cpu_to_le32(tot_num_ret);
> >
> > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> >  			break;
> >  		}
> >
> > +		if (t->rx.len < (sizeof(u32) * 2)) {
> > +			dev_err(dev, "Truncated reply - rx.len:%zd\n",
> > +				t->rx.len);
> > +			ret = -EPROTO;
> > +			break;
> > +		}
> > +
> > +		real_list_sz = t->rx.len - sizeof(u32);
> > +		calc_list_sz = ((loop_num_ret / sizeof(u32)) +
> > +				!!(loop_num_ret % sizeof(u32))) * sizeof(u32);
> 
> Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ?
> 

At first sight could be fine with your easier version BUT what if loop_num_ret
is returned as zero ?

real_list_sz should be ZERO length and calc_list_sz

im my version:

calc_list_sz = ((0/4) +!!(0%4)) * 4   ===>> 0

while in the simplified one gets calculated wrong:

calc_list_sz = (0-1)/4 + 1 ====> 1

...moreover being both loop_num_ret and calc_list_sz unsigned I am even
not so sure about implicit casting messing things up evenm more :D

So I sticked to the more convoluted approach :D

....Have I missed something else ?

Thanks,
Cristian

Re: [PATCH 04/22] firmware: arm_scmi: Validate BASE_DISCOVER_LIST_PROTOCOLS reply

[Sudeep Holla]

On Thu, Apr 28, 2022 at 02:45:07PM +0100, Cristian Marussi wrote:
> On Thu, Apr 28, 2022 at 11:07:29AM +0100, Sudeep Holla wrote:
> > On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote:
> > > Do not blindly trust SCMI backend server reply about list of implemented
> > > protocols, instead validate the reported length of the list of protocols
> > > against the real payload size of the message reply.
> > >
> > > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > ---
> > >  drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > > index f279146f8110..c1165d1282ef 100644
> > > --- a/drivers/firmware/arm_scmi/base.c
> > > +++ b/drivers/firmware/arm_scmi/base.c
> > > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > >  	list = t->rx.buf + sizeof(*num_ret);
> > >
> > >  	do {
> > > +		size_t real_list_sz;
> > > +		u32 calc_list_sz;
> > > +
> > >  		/* Set the number of protocols to be skipped/already read */
> > >  		*num_skip = cpu_to_le32(tot_num_ret);
> > >
> > > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > >  			break;
> > >  		}
> > >
> > > +		if (t->rx.len < (sizeof(u32) * 2)) {
> > > +			dev_err(dev, "Truncated reply - rx.len:%zd\n",
> > > +				t->rx.len);
> > > +			ret = -EPROTO;
> > > +			break;
> > > +		}
> > > +
> > > +		real_list_sz = t->rx.len - sizeof(u32);
> > > +		calc_list_sz = ((loop_num_ret / sizeof(u32)) +
> > > +				!!(loop_num_ret % sizeof(u32))) * sizeof(u32);
> > 
> > Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ?
> > 
> 
> At first sight could be fine with your easier version BUT what if loop_num_ret
> is returned as zero ?
> 
> real_list_sz should be ZERO length and calc_list_sz
> 
> im my version:
> 
> calc_list_sz = ((0/4) +!!(0%4)) * 4   ===>> 0
> 
> while in the simplified one gets calculated wrong:
> 
> calc_list_sz = (0-1)/4 + 1 ====> 1
> 
> ...moreover being both loop_num_ret and calc_list_sz unsigned I am even
> not so sure about implicit casting messing things up evenm more :D
> 
> So I sticked to the more convoluted approach :D
> 
> ....Have I missed something else ?
>

Right, but shouldn't we break if it 0 much earlier. It must not happen with
your new logic and even if it does, wouldn't it be better to break earlier ?

-- 
Regards,
Sudeep

Re: [PATCH 04/22] firmware: arm_scmi: Validate BASE_DISCOVER_LIST_PROTOCOLS reply

[Cristian Marussi]

On Thu, Apr 28, 2022 at 02:55:04PM +0100, Sudeep Holla wrote:
> On Thu, Apr 28, 2022 at 02:45:07PM +0100, Cristian Marussi wrote:
> > On Thu, Apr 28, 2022 at 11:07:29AM +0100, Sudeep Holla wrote:
> > > On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote:
> > > > Do not blindly trust SCMI backend server reply about list of implemented
> > > > protocols, instead validate the reported length of the list of protocols
> > > > against the real payload size of the message reply.
> > > >
> > > > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol")
> > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > > ---
> > > >  drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
> > > > index f279146f8110..c1165d1282ef 100644
> > > > --- a/drivers/firmware/arm_scmi/base.c
> > > > +++ b/drivers/firmware/arm_scmi/base.c
> > > > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > > >  	list = t->rx.buf + sizeof(*num_ret);
> > > >
> > > >  	do {
> > > > +		size_t real_list_sz;
> > > > +		u32 calc_list_sz;
> > > > +
> > > >  		/* Set the number of protocols to be skipped/already read */
> > > >  		*num_skip = cpu_to_le32(tot_num_ret);
> > > >
> > > > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
> > > >  			break;
> > > >  		}
> > > >
> > > > +		if (t->rx.len < (sizeof(u32) * 2)) {
> > > > +			dev_err(dev, "Truncated reply - rx.len:%zd\n",
> > > > +				t->rx.len);
> > > > +			ret = -EPROTO;
> > > > +			break;
> > > > +		}
> > > > +
> > > > +		real_list_sz = t->rx.len - sizeof(u32);
> > > > +		calc_list_sz = ((loop_num_ret / sizeof(u32)) +
> > > > +				!!(loop_num_ret % sizeof(u32))) * sizeof(u32);
> > > 
> > > Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ?
> > > 
> > 
> > At first sight could be fine with your easier version BUT what if loop_num_ret
> > is returned as zero ?
> > 
> > real_list_sz should be ZERO length and calc_list_sz
> > 
> > im my version:
> > 
> > calc_list_sz = ((0/4) +!!(0%4)) * 4   ===>> 0
> > 
> > while in the simplified one gets calculated wrong:
> > 
> > calc_list_sz = (0-1)/4 + 1 ====> 1
> > 
> > ...moreover being both loop_num_ret and calc_list_sz unsigned I am even
> > not so sure about implicit casting messing things up evenm more :D
> > 
> > So I sticked to the more convoluted approach :D
> > 
> > ....Have I missed something else ?
> >
> 
> Right, but shouldn't we break if it 0 much earlier. It must not happen with
> your new logic and even if it does, wouldn't it be better to break earlier ?
> 

Fine for me.

Thanks,
Cristian

[PATCH 05/22] firmware: arm_scmi: Dynamically allocate protocols array

[Cristian Marussi]

Move away from a statically allocated array for holding the current set of
protocols implemented by the platform in favour of allocating it
dynamically based on the number of protocols effectively advertised by the
platform via BASE protocol exchanges.

While at that, rectify the BASE_DISCOVER_LIST_PROTOCOLS loop iterations to
terminate only when a number of protocols equal to the advertised ones has
been received, instead of looping till the platform returns no more
protocols descriptors: this new behaviour is better compliant with the
specification and it has been tested to work equally well against an SCMI
stack running on top of an official SCP firmware on a JUNO board.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/base.c   | 17 ++++++++++-------
 drivers/firmware/arm_scmi/common.h |  1 -
 drivers/firmware/arm_scmi/driver.c |  6 ++++--
 3 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c
index c1165d1282ef..d6322624da9f 100644
--- a/drivers/firmware/arm_scmi/base.c
+++ b/drivers/firmware/arm_scmi/base.c
@@ -178,6 +178,7 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
 	__le32 *num_skip, *num_ret;
 	u32 tot_num_ret = 0, loop_num_ret;
 	struct device *dev = ph->dev;
+	struct scmi_revision_info *rev = ph->get_priv(ph);
 
 	ret = ph->xops->xfer_get_init(ph, BASE_DISCOVER_LIST_PROTOCOLS,
 				      sizeof(*num_skip), 0, &t);
@@ -200,8 +201,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
 			break;
 
 		loop_num_ret = le32_to_cpu(*num_ret);
-		if (loop_num_ret > MAX_PROTOCOLS_IMP - tot_num_ret) {
-			dev_err(dev, "No. of Protocol > MAX_PROTOCOLS_IMP");
+		if (loop_num_ret > rev->num_protocols - tot_num_ret) {
+			dev_err(dev,
+				"No. Returned protocols > Total protocols.\n");
 			break;
 		}
 
@@ -229,7 +231,7 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph,
 		tot_num_ret += loop_num_ret;
 
 		ph->xops->reset_rx_to_maxsz(ph, t);
-	} while (loop_num_ret);
+	} while (tot_num_ret < rev->num_protocols);
 
 	ph->xops->xfer_put(ph, t);
 
@@ -372,10 +374,6 @@ static int scmi_base_protocol_init(const struct scmi_protocol_handle *ph)
 	if (ret)
 		return ret;
 
-	prot_imp = devm_kcalloc(dev, MAX_PROTOCOLS_IMP, sizeof(u8), GFP_KERNEL);
-	if (!prot_imp)
-		return -ENOMEM;
-
 	rev->major_ver = PROTOCOL_REV_MAJOR(version),
 	rev->minor_ver = PROTOCOL_REV_MINOR(version);
 	ph->set_priv(ph, rev);
@@ -384,6 +382,11 @@ static int scmi_base_protocol_init(const struct scmi_protocol_handle *ph)
 	if (ret)
 		return ret;
 
+	prot_imp = devm_kcalloc(dev, rev->num_protocols, sizeof(u8),
+				GFP_KERNEL);
+	if (!prot_imp)
+		return -ENOMEM;
+
 	scmi_base_vendor_id_get(ph, false);
 	scmi_base_vendor_id_get(ph, true);
 	scmi_base_implementation_version_get(ph);
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 4fda84bfab42..6885348a505d 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -30,7 +30,6 @@
 #define PROTOCOL_REV_MAJOR_MASK	GENMASK(31, 16)
 #define PROTOCOL_REV_MAJOR(x)	(u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x)))
 #define PROTOCOL_REV_MINOR(x)	(u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x)))
-#define MAX_PROTOCOLS_IMP	16
 #define MAX_OPPS		16
 
 enum scmi_common_cmd {
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 7436c475e708..8e07bee16c8e 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -128,7 +128,8 @@ struct scmi_protocol_instance {
  *	       usage.
  * @protocols_mtx: A mutex to protect protocols instances initialization.
  * @protocols_imp: List of protocols implemented, currently maximum of
- *	MAX_PROTOCOLS_IMP elements allocated by the base protocol
+ *		   scmi_revision_info.num_protocols elements allocated by the
+ *		   base protocol
  * @active_protocols: IDR storing device_nodes for protocols actually defined
  *		      in the DT and confirmed as implemented by fw.
  * @atomic_threshold: Optional system wide DT-configured threshold, expressed
@@ -1309,11 +1310,12 @@ scmi_is_protocol_implemented(const struct scmi_handle *handle, u8 prot_id)
 {
 	int i;
 	struct scmi_info *info = handle_to_scmi_info(handle);
+	struct scmi_revision_info *rev = handle->version;
 
 	if (!info->protocols_imp)
 		return false;
 
-	for (i = 0; i < MAX_PROTOCOLS_IMP; i++)
+	for (i = 0; i < rev->num_protocols; i++)
 		if (info->protocols_imp[i] == prot_id)
 			return true;
 	return false;
-- 
2.32.0

Re: [PATCH 05/22] firmware: arm_scmi: Dynamically allocate protocols array

[Sudeep Holla]

On Wed, Mar 30, 2022 at 04:05:34PM +0100, Cristian Marussi wrote:
> Move away from a statically allocated array for holding the current set of
> protocols implemented by the platform in favour of allocating it
> dynamically based on the number of protocols effectively advertised by the
> platform via BASE protocol exchanges.
> 
> While at that, rectify the BASE_DISCOVER_LIST_PROTOCOLS loop iterations to
> terminate only when a number of protocols equal to the advertised ones has
> been received, instead of looping till the platform returns no more
> protocols descriptors: this new behaviour is better compliant with the
> specification and it has been tested to work equally well against an SCMI
> stack running on top of an official SCP firmware on a JUNO board.
>

Nice, no idea why I forgot that we have num_protocols from the another
command when I originally wrote this. I may be carried away with the way
OPP or even the clock rate list works I guess.

-- 
Regards,
Sudeep

[PATCH 06/22] firmware: arm_scmi: Make name_get operations return a const

[Cristian Marussi]

A few protocol operations are available that returns a pointer to an
internal char array representing resources naming: make those functions
return a const pointer to such array.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/power.c | 5 +++--
 drivers/firmware/arm_scmi/reset.c | 4 ++--
 include/linux/scmi_protocol.h     | 6 ++++--
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 0f0b94f0b624..5c04d63e7ff1 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -174,8 +174,9 @@ static int scmi_power_num_domains_get(const struct scmi_protocol_handle *ph)
 	return pi->num_domains;
 }
 
-static char *scmi_power_name_get(const struct scmi_protocol_handle *ph,
-				 u32 domain)
+static const char *
+scmi_power_name_get(const struct scmi_protocol_handle *ph,
+		    u32 domain)
 {
 	struct scmi_power_info *pi = ph->get_priv(ph);
 	struct power_dom_info *dom = pi->dom_info + domain;
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 9cdbd133db10..b823a9f64af9 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -126,8 +126,8 @@ static int scmi_reset_num_domains_get(const struct scmi_protocol_handle *ph)
 	return pi->num_domains;
 }
 
-static char *scmi_reset_name_get(const struct scmi_protocol_handle *ph,
-				 u32 domain)
+static const char *
+scmi_reset_name_get(const struct scmi_protocol_handle *ph, u32 domain)
 {
 	struct scmi_reset_info *pi = ph->get_priv(ph);
 
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index b87551f41f9f..ced37d1de1fe 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -146,7 +146,8 @@ struct scmi_perf_proto_ops {
  */
 struct scmi_power_proto_ops {
 	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
-	char *(*name_get)(const struct scmi_protocol_handle *ph, u32 domain);
+	const char *(*name_get)(const struct scmi_protocol_handle *ph,
+				u32 domain);
 #define SCMI_POWER_STATE_TYPE_SHIFT	30
 #define SCMI_POWER_STATE_ID_MASK	(BIT(28) - 1)
 #define SCMI_POWER_STATE_PARAM(type, id) \
@@ -484,7 +485,8 @@ struct scmi_sensor_proto_ops {
  */
 struct scmi_reset_proto_ops {
 	int (*num_domains_get)(const struct scmi_protocol_handle *ph);
-	char *(*name_get)(const struct scmi_protocol_handle *ph, u32 domain);
+	const char *(*name_get)(const struct scmi_protocol_handle *ph,
+				u32 domain);
 	int (*latency_get)(const struct scmi_protocol_handle *ph, u32 domain);
 	int (*reset)(const struct scmi_protocol_handle *ph, u32 domain);
 	int (*assert)(const struct scmi_protocol_handle *ph, u32 domain);
-- 
2.32.0

[PATCH 07/22] firmware: arm_scmi: Check CLOCK_RATE_SET_COMPLETE async reply

[Cristian Marussi]

When CLOCK_RATE_SET command is issued in asynchronous mode the delayed
response CLOCK_RATE_SET_COMPLETE comes back once the SCMI platform has
effectively operated the requested change: such delayed response carries
also the clock ID and the final clock rate that has been set.

As an aid to debug issues, check that the clock ID in the delayed
response matches the expected one and debug print the rate value.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 0ae39ee920e9..32ccac457d20 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -68,6 +68,12 @@ struct scmi_clock_set_rate {
 	__le32 value_high;
 };
 
+struct scmi_msg_resp_set_rate_complete {
+	__le32 id;
+	__le32 rate_low;
+	__le32 rate_high;
+};
+
 struct clock_info {
 	u32 version;
 	int num_clocks;
@@ -266,10 +272,23 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph,
 	cfg->value_low = cpu_to_le32(rate & 0xffffffff);
 	cfg->value_high = cpu_to_le32(rate >> 32);
 
-	if (flags & CLOCK_SET_ASYNC)
+	if (flags & CLOCK_SET_ASYNC) {
 		ret = ph->xops->do_xfer_with_response(ph, t);
-	else
+		if (!ret) {
+			struct scmi_msg_resp_set_rate_complete *resp;
+
+			resp = t->rx.buf;
+			if (le32_to_cpu(resp->id) == clk_id)
+				dev_dbg(ph->dev,
+					"Clock ID %d set async to %llu\n",
+					clk_id,
+					get_unaligned_le64(&resp->rate_low));
+			else
+				ret = -EPROTO;
+		}
+	} else {
 		ret = ph->xops->do_xfer(ph, t);
+	}
 
 	if (ci->max_async_req)
 		atomic_dec(&ci->cur_async_req);
-- 
2.32.0

[PATCH 08/22] firmware: arm_scmi: Remove unneeded NULL termination of clk name

[Cristian Marussi]

The string array 'name' inside struct scmi_clock_info holds the clock name
which was successfully retrieved by querying the SCMI platform, unless the
related underlying SCMI command failed.

Anyway, such scmi_clock_info structure is allocated using devm_kcalloc()
which in turn internally appends a __GFP_ZERO flag to its invocation:
as a consequence the string 'name' field does not need to be zeroed when
we fail to get the clock name via SCMI, it is already NULL terminated.

Remove unneeded explicit NULL termination.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 32ccac457d20..7adf99b92bf6 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -129,8 +129,6 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 		if (t->rx.len == sizeof(*attr))
 			clk->enable_latency =
 				le32_to_cpu(attr->clock_enable_latency);
-	} else {
-		clk->name[0] = '\0';
 	}
 
 	ph->xops->xfer_put(ph, t);
-- 
2.32.0

[PATCH 09/22] firmware: arm_scmi: Split protocol specific definitions in a dedicated header

[Cristian Marussi]

Move some SCMI protocol specific definitions from common.h into a the new
dedicated protocols.h header so that SCMI protocols core code can include
only what it needs; this is going to be useful to avoid the risk of growing
indefinitely the dimension of common.h, especially when introducing some
common protocols helper functions.

Header common.h will continue to be included by SCMI core and transport
layers.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c     |   4 +-
 drivers/firmware/arm_scmi/common.h    | 224 +----------------------
 drivers/firmware/arm_scmi/perf.c      |   6 +-
 drivers/firmware/arm_scmi/power.c     |   4 +-
 drivers/firmware/arm_scmi/protocols.h | 247 ++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/reset.c     |   4 +-
 drivers/firmware/arm_scmi/sensors.c   |   4 +-
 drivers/firmware/arm_scmi/system.c    |   4 +-
 drivers/firmware/arm_scmi/voltage.c   |   4 +-
 9 files changed, 265 insertions(+), 236 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/protocols.h

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 7adf99b92bf6..d78339dc2fdc 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -2,13 +2,13 @@
 /*
  * System Control and Management Interface (SCMI) Clock Protocol
  *
- * Copyright (C) 2018-2021 ARM Ltd.
+ * Copyright (C) 2018-2022 ARM Ltd.
  */
 
 #include <linux/module.h>
 #include <linux/sort.h>
 
-#include "common.h"
+#include "protocols.h"
 
 enum scmi_clock_protocol_cmd {
 	CLOCK_ATTRIBUTES = 0x3,
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index 6885348a505d..61aba7447c32 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -4,7 +4,7 @@
  * driver common header file containing some definitions, structures
  * and function prototypes used in all the different SCMI protocols.
  *
- * Copyright (C) 2018-2021 ARM Ltd.
+ * Copyright (C) 2018-2022 ARM Ltd.
  */
 #ifndef _SCMI_COMMON_H
 #define _SCMI_COMMON_H
@@ -24,37 +24,9 @@
 
 #include <asm/unaligned.h>
 
+#include "protocols.h"
 #include "notify.h"
 
-#define PROTOCOL_REV_MINOR_MASK	GENMASK(15, 0)
-#define PROTOCOL_REV_MAJOR_MASK	GENMASK(31, 16)
-#define PROTOCOL_REV_MAJOR(x)	(u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x)))
-#define PROTOCOL_REV_MINOR(x)	(u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x)))
-#define MAX_OPPS		16
-
-enum scmi_common_cmd {
-	PROTOCOL_VERSION = 0x0,
-	PROTOCOL_ATTRIBUTES = 0x1,
-	PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
-};
-
-/**
- * struct scmi_msg_resp_prot_version - Response for a message
- *
- * @minor_version: Minor version of the ABI that firmware supports
- * @major_version: Major version of the ABI that firmware supports
- *
- * In general, ABI version changes follow the rule that minor version increments
- * are backward compatible. Major revision changes in ABI may not be
- * backward compatible.
- *
- * Response to a generic message with message type SCMI_MSG_VERSION
- */
-struct scmi_msg_resp_prot_version {
-	__le16 minor_version;
-	__le16 major_version;
-};
-
 #define MSG_ID_MASK		GENMASK(7, 0)
 #define MSG_XTRACT_ID(hdr)	FIELD_GET(MSG_ID_MASK, (hdr))
 #define MSG_TYPE_MASK		GENMASK(9, 8)
@@ -78,28 +50,6 @@ struct scmi_msg_resp_prot_version {
  */
 #define SCMI_PENDING_XFERS_HT_ORDER_SZ		9
 
-/**
- * struct scmi_msg_hdr - Message(Tx/Rx) header
- *
- * @id: The identifier of the message being sent
- * @protocol_id: The identifier of the protocol used to send @id message
- * @type: The SCMI type for this message
- * @seq: The token to identify the message. When a message returns, the
- *	platform returns the whole message header unmodified including the
- *	token
- * @status: Status of the transfer once it's complete
- * @poll_completion: Indicate if the transfer needs to be polled for
- *	completion or interrupt mode is used
- */
-struct scmi_msg_hdr {
-	u8 id;
-	u8 protocol_id;
-	u8 type;
-	u16 seq;
-	u32 status;
-	bool poll_completion;
-};
-
 /**
  * pack_scmi_header() - packs and returns 32-bit header
  *
@@ -129,72 +79,6 @@ static inline void unpack_scmi_header(u32 msg_hdr, struct scmi_msg_hdr *hdr)
 	hdr->type = MSG_XTRACT_TYPE(msg_hdr);
 }
 
-/**
- * struct scmi_msg - Message(Tx/Rx) structure
- *
- * @buf: Buffer pointer
- * @len: Length of data in the Buffer
- */
-struct scmi_msg {
-	void *buf;
-	size_t len;
-};
-
-/**
- * struct scmi_xfer - Structure representing a message flow
- *
- * @transfer_id: Unique ID for debug & profiling purpose
- * @hdr: Transmit message header
- * @tx: Transmit message
- * @rx: Receive message, the buffer should be pre-allocated to store
- *	message. If request-ACK protocol is used, we can reuse the same
- *	buffer for the rx path as we use for the tx path.
- * @done: command message transmit completion event
- * @async_done: pointer to delayed response message received event completion
- * @pending: True for xfers added to @pending_xfers hashtable
- * @node: An hlist_node reference used to store this xfer, alternatively, on
- *	  the free list @free_xfers or in the @pending_xfers hashtable
- * @users: A refcount to track the active users for this xfer.
- *	   This is meant to protect against the possibility that, when a command
- *	   transaction times out concurrently with the reception of a valid
- *	   response message, the xfer could be finally put on the TX path, and
- *	   so vanish, while on the RX path scmi_rx_callback() is still
- *	   processing it: in such a case this refcounting will ensure that, even
- *	   though the timed-out transaction will anyway cause the command
- *	   request to be reported as failed by time-out, the underlying xfer
- *	   cannot be discarded and possibly reused until the last one user on
- *	   the RX path has released it.
- * @busy: An atomic flag to ensure exclusive write access to this xfer
- * @state: The current state of this transfer, with states transitions deemed
- *	   valid being:
- *	    - SCMI_XFER_SENT_OK -> SCMI_XFER_RESP_OK [ -> SCMI_XFER_DRESP_OK ]
- *	    - SCMI_XFER_SENT_OK -> SCMI_XFER_DRESP_OK
- *	      (Missing synchronous response is assumed OK and ignored)
- * @lock: A spinlock to protect state and busy fields.
- * @priv: A pointer for transport private usage.
- */
-struct scmi_xfer {
-	int transfer_id;
-	struct scmi_msg_hdr hdr;
-	struct scmi_msg tx;
-	struct scmi_msg rx;
-	struct completion done;
-	struct completion *async_done;
-	bool pending;
-	struct hlist_node node;
-	refcount_t users;
-#define SCMI_XFER_FREE		0
-#define SCMI_XFER_BUSY		1
-	atomic_t busy;
-#define SCMI_XFER_SENT_OK	0
-#define SCMI_XFER_RESP_OK	1
-#define SCMI_XFER_DRESP_OK	2
-	int state;
-	/* A lock to protect state and busy fields */
-	spinlock_t lock;
-	void *priv;
-};
-
 /*
  * An helper macro to lookup an xfer from the @pending_xfers hashtable
  * using the message sequence number token as a key.
@@ -210,64 +94,6 @@ struct scmi_xfer {
 	xfer_;							\
 })
 
-struct scmi_xfer_ops;
-
-/**
- * struct scmi_protocol_handle  - Reference to an initialized protocol instance
- *
- * @dev: A reference to the associated SCMI instance device (handle->dev).
- * @xops: A reference to a struct holding refs to the core xfer operations that
- *	  can be used by the protocol implementation to generate SCMI messages.
- * @set_priv: A method to set protocol private data for this instance.
- * @get_priv: A method to get protocol private data previously set.
- *
- * This structure represents a protocol initialized against specific SCMI
- * instance and it will be used as follows:
- * - as a parameter fed from the core to the protocol initialization code so
- *   that it can access the core xfer operations to build and generate SCMI
- *   messages exclusively for the specific underlying protocol instance.
- * - as an opaque handle fed by an SCMI driver user when it tries to access
- *   this protocol through its own protocol operations.
- *   In this case this handle will be returned as an opaque object together
- *   with the related protocol operations when the SCMI driver tries to access
- *   the protocol.
- */
-struct scmi_protocol_handle {
-	struct device *dev;
-	const struct scmi_xfer_ops *xops;
-	int (*set_priv)(const struct scmi_protocol_handle *ph, void *priv);
-	void *(*get_priv)(const struct scmi_protocol_handle *ph);
-};
-
-/**
- * struct scmi_xfer_ops  - References to the core SCMI xfer operations.
- * @version_get: Get this version protocol.
- * @xfer_get_init: Initialize one struct xfer if any xfer slot is free.
- * @reset_rx_to_maxsz: Reset rx size to max transport size.
- * @do_xfer: Do the SCMI transfer.
- * @do_xfer_with_response: Do the SCMI transfer waiting for a response.
- * @xfer_put: Free the xfer slot.
- *
- * Note that all this operations expect a protocol handle as first parameter;
- * they then internally use it to infer the underlying protocol number: this
- * way is not possible for a protocol implementation to forge messages for
- * another protocol.
- */
-struct scmi_xfer_ops {
-	int (*version_get)(const struct scmi_protocol_handle *ph, u32 *version);
-	int (*xfer_get_init)(const struct scmi_protocol_handle *ph, u8 msg_id,
-			     size_t tx_size, size_t rx_size,
-			     struct scmi_xfer **p);
-	void (*reset_rx_to_maxsz)(const struct scmi_protocol_handle *ph,
-				  struct scmi_xfer *xfer);
-	int (*do_xfer)(const struct scmi_protocol_handle *ph,
-		       struct scmi_xfer *xfer);
-	int (*do_xfer_with_response)(const struct scmi_protocol_handle *ph,
-				     struct scmi_xfer *xfer);
-	void (*xfer_put)(const struct scmi_protocol_handle *ph,
-			 struct scmi_xfer *xfer);
-};
-
 struct scmi_revision_info *
 scmi_revision_area_get(const struct scmi_protocol_handle *ph);
 int scmi_handle_put(const struct scmi_handle *handle);
@@ -276,55 +102,9 @@ void scmi_set_handle(struct scmi_device *scmi_dev);
 void scmi_setup_protocol_implemented(const struct scmi_protocol_handle *ph,
 				     u8 *prot_imp);
 
-typedef int (*scmi_prot_init_ph_fn_t)(const struct scmi_protocol_handle *);
-
-/**
- * struct scmi_protocol  - Protocol descriptor
- * @id: Protocol ID.
- * @owner: Module reference if any.
- * @instance_init: Mandatory protocol initialization function.
- * @instance_deinit: Optional protocol de-initialization function.
- * @ops: Optional reference to the operations provided by the protocol and
- *	 exposed in scmi_protocol.h.
- * @events: An optional reference to the events supported by this protocol.
- */
-struct scmi_protocol {
-	const u8				id;
-	struct module				*owner;
-	const scmi_prot_init_ph_fn_t		instance_init;
-	const scmi_prot_init_ph_fn_t		instance_deinit;
-	const void				*ops;
-	const struct scmi_protocol_events	*events;
-};
-
 int __init scmi_bus_init(void);
 void __exit scmi_bus_exit(void);
 
-#define DECLARE_SCMI_REGISTER_UNREGISTER(func)		\
-	int __init scmi_##func##_register(void);	\
-	void __exit scmi_##func##_unregister(void)
-DECLARE_SCMI_REGISTER_UNREGISTER(base);
-DECLARE_SCMI_REGISTER_UNREGISTER(clock);
-DECLARE_SCMI_REGISTER_UNREGISTER(perf);
-DECLARE_SCMI_REGISTER_UNREGISTER(power);
-DECLARE_SCMI_REGISTER_UNREGISTER(reset);
-DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
-DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
-DECLARE_SCMI_REGISTER_UNREGISTER(system);
-
-#define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(name, proto)	\
-static const struct scmi_protocol *__this_proto = &(proto);	\
-								\
-int __init scmi_##name##_register(void)				\
-{								\
-	return scmi_protocol_register(__this_proto);		\
-}								\
-								\
-void __exit scmi_##name##_unregister(void)			\
-{								\
-	scmi_protocol_unregister(__this_proto);			\
-}
-
 const struct scmi_protocol *scmi_protocol_get(int protocol_id);
 void scmi_protocol_put(int protocol_id);
 
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index e9f68b91580c..0e9703310758 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -2,7 +2,7 @@
 /*
  * System Control and Management Interface (SCMI) Performance Protocol
  *
- * Copyright (C) 2018-2021 ARM Ltd.
+ * Copyright (C) 2018-2022 ARM Ltd.
  */
 
 #define pr_fmt(fmt) "SCMI Notifications PERF - " fmt
@@ -17,9 +17,11 @@
 #include <linux/scmi_protocol.h>
 #include <linux/sort.h>
 
-#include "common.h"
+#include "protocols.h"
 #include "notify.h"
 
+#define MAX_OPPS		16
+
 enum scmi_performance_protocol_cmd {
 	PERF_DOMAIN_ATTRIBUTES = 0x3,
 	PERF_DESCRIBE_LEVELS = 0x4,
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 5c04d63e7ff1..e378a3eb0d07 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -2,7 +2,7 @@
 /*
  * System Control and Management Interface (SCMI) Power Protocol
  *
- * Copyright (C) 2018-2021 ARM Ltd.
+ * Copyright (C) 2018-2022 ARM Ltd.
  */
 
 #define pr_fmt(fmt) "SCMI Notifications POWER - " fmt
@@ -10,7 +10,7 @@
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
 
-#include "common.h"
+#include "protocols.h"
 #include "notify.h"
 
 enum scmi_power_protocol_cmd {
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
new file mode 100644
index 000000000000..b2a92d2b2986
--- /dev/null
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -0,0 +1,247 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * System Control and Management Interface (SCMI) Message Protocol
+ * protocols common header file containing some definitions, structures
+ * and function prototypes used in all the different SCMI protocols.
+ *
+ * Copyright (C) 2022 ARM Ltd.
+ */
+#ifndef _SCMI_PROTOCOLS_H
+#define _SCMI_PROTOCOLS_H
+
+#include <linux/bitfield.h>
+#include <linux/completion.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/hashtable.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/refcount.h>
+#include <linux/scmi_protocol.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#include <asm/unaligned.h>
+
+#define PROTOCOL_REV_MINOR_MASK	GENMASK(15, 0)
+#define PROTOCOL_REV_MAJOR_MASK	GENMASK(31, 16)
+#define PROTOCOL_REV_MAJOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))))
+#define PROTOCOL_REV_MINOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MINOR_MASK, (x))))
+
+enum scmi_common_cmd {
+	PROTOCOL_VERSION = 0x0,
+	PROTOCOL_ATTRIBUTES = 0x1,
+	PROTOCOL_MESSAGE_ATTRIBUTES = 0x2,
+};
+
+/**
+ * struct scmi_msg_resp_prot_version - Response for a message
+ *
+ * @minor_version: Minor version of the ABI that firmware supports
+ * @major_version: Major version of the ABI that firmware supports
+ *
+ * In general, ABI version changes follow the rule that minor version increments
+ * are backward compatible. Major revision changes in ABI may not be
+ * backward compatible.
+ *
+ * Response to a generic message with message type SCMI_MSG_VERSION
+ */
+struct scmi_msg_resp_prot_version {
+	__le16 minor_version;
+	__le16 major_version;
+};
+
+/**
+ * struct scmi_msg - Message(Tx/Rx) structure
+ *
+ * @buf: Buffer pointer
+ * @len: Length of data in the Buffer
+ */
+struct scmi_msg {
+	void *buf;
+	size_t len;
+};
+
+/**
+ * struct scmi_msg_hdr - Message(Tx/Rx) header
+ *
+ * @id: The identifier of the message being sent
+ * @protocol_id: The identifier of the protocol used to send @id message
+ * @type: The SCMI type for this message
+ * @seq: The token to identify the message. When a message returns, the
+ *	platform returns the whole message header unmodified including the
+ *	token
+ * @status: Status of the transfer once it's complete
+ * @poll_completion: Indicate if the transfer needs to be polled for
+ *	completion or interrupt mode is used
+ */
+struct scmi_msg_hdr {
+	u8 id;
+	u8 protocol_id;
+	u8 type;
+	u16 seq;
+	u32 status;
+	bool poll_completion;
+};
+
+/**
+ * struct scmi_xfer - Structure representing a message flow
+ *
+ * @transfer_id: Unique ID for debug & profiling purpose
+ * @hdr: Transmit message header
+ * @tx: Transmit message
+ * @rx: Receive message, the buffer should be pre-allocated to store
+ *	message. If request-ACK protocol is used, we can reuse the same
+ *	buffer for the rx path as we use for the tx path.
+ * @done: command message transmit completion event
+ * @async_done: pointer to delayed response message received event completion
+ * @pending: True for xfers added to @pending_xfers hashtable
+ * @node: An hlist_node reference used to store this xfer, alternatively, on
+ *	  the free list @free_xfers or in the @pending_xfers hashtable
+ * @users: A refcount to track the active users for this xfer.
+ *	   This is meant to protect against the possibility that, when a command
+ *	   transaction times out concurrently with the reception of a valid
+ *	   response message, the xfer could be finally put on the TX path, and
+ *	   so vanish, while on the RX path scmi_rx_callback() is still
+ *	   processing it: in such a case this refcounting will ensure that, even
+ *	   though the timed-out transaction will anyway cause the command
+ *	   request to be reported as failed by time-out, the underlying xfer
+ *	   cannot be discarded and possibly reused until the last one user on
+ *	   the RX path has released it.
+ * @busy: An atomic flag to ensure exclusive write access to this xfer
+ * @state: The current state of this transfer, with states transitions deemed
+ *	   valid being:
+ *	    - SCMI_XFER_SENT_OK -> SCMI_XFER_RESP_OK [ -> SCMI_XFER_DRESP_OK ]
+ *	    - SCMI_XFER_SENT_OK -> SCMI_XFER_DRESP_OK
+ *	      (Missing synchronous response is assumed OK and ignored)
+ * @lock: A spinlock to protect state and busy fields.
+ * @priv: A pointer for transport private usage.
+ */
+struct scmi_xfer {
+	int transfer_id;
+	struct scmi_msg_hdr hdr;
+	struct scmi_msg tx;
+	struct scmi_msg rx;
+	struct completion done;
+	struct completion *async_done;
+	bool pending;
+	struct hlist_node node;
+	refcount_t users;
+#define SCMI_XFER_FREE		0
+#define SCMI_XFER_BUSY		1
+	atomic_t busy;
+#define SCMI_XFER_SENT_OK	0
+#define SCMI_XFER_RESP_OK	1
+#define SCMI_XFER_DRESP_OK	2
+	int state;
+	/* A lock to protect state and busy fields */
+	spinlock_t lock;
+	void *priv;
+};
+
+struct scmi_xfer_ops;
+
+/**
+ * struct scmi_protocol_handle  - Reference to an initialized protocol instance
+ *
+ * @dev: A reference to the associated SCMI instance device (handle->dev).
+ * @xops: A reference to a struct holding refs to the core xfer operations that
+ *	  can be used by the protocol implementation to generate SCMI messages.
+ * @set_priv: A method to set protocol private data for this instance.
+ * @get_priv: A method to get protocol private data previously set.
+ *
+ * This structure represents a protocol initialized against specific SCMI
+ * instance and it will be used as follows:
+ * - as a parameter fed from the core to the protocol initialization code so
+ *   that it can access the core xfer operations to build and generate SCMI
+ *   messages exclusively for the specific underlying protocol instance.
+ * - as an opaque handle fed by an SCMI driver user when it tries to access
+ *   this protocol through its own protocol operations.
+ *   In this case this handle will be returned as an opaque object together
+ *   with the related protocol operations when the SCMI driver tries to access
+ *   the protocol.
+ */
+struct scmi_protocol_handle {
+	struct device *dev;
+	const struct scmi_xfer_ops *xops;
+	int (*set_priv)(const struct scmi_protocol_handle *ph, void *priv);
+	void *(*get_priv)(const struct scmi_protocol_handle *ph);
+};
+
+/**
+ * struct scmi_xfer_ops  - References to the core SCMI xfer operations.
+ * @version_get: Get this version protocol.
+ * @xfer_get_init: Initialize one struct xfer if any xfer slot is free.
+ * @reset_rx_to_maxsz: Reset rx size to max transport size.
+ * @do_xfer: Do the SCMI transfer.
+ * @do_xfer_with_response: Do the SCMI transfer waiting for a response.
+ * @xfer_put: Free the xfer slot.
+ *
+ * Note that all this operations expect a protocol handle as first parameter;
+ * they then internally use it to infer the underlying protocol number: this
+ * way is not possible for a protocol implementation to forge messages for
+ * another protocol.
+ */
+struct scmi_xfer_ops {
+	int (*version_get)(const struct scmi_protocol_handle *ph, u32 *version);
+	int (*xfer_get_init)(const struct scmi_protocol_handle *ph, u8 msg_id,
+			     size_t tx_size, size_t rx_size,
+			     struct scmi_xfer **p);
+	void (*reset_rx_to_maxsz)(const struct scmi_protocol_handle *ph,
+				  struct scmi_xfer *xfer);
+	int (*do_xfer)(const struct scmi_protocol_handle *ph,
+		       struct scmi_xfer *xfer);
+	int (*do_xfer_with_response)(const struct scmi_protocol_handle *ph,
+				     struct scmi_xfer *xfer);
+	void (*xfer_put)(const struct scmi_protocol_handle *ph,
+			 struct scmi_xfer *xfer);
+};
+
+typedef int (*scmi_prot_init_ph_fn_t)(const struct scmi_protocol_handle *);
+
+/**
+ * struct scmi_protocol  - Protocol descriptor
+ * @id: Protocol ID.
+ * @owner: Module reference if any.
+ * @instance_init: Mandatory protocol initialization function.
+ * @instance_deinit: Optional protocol de-initialization function.
+ * @ops: Optional reference to the operations provided by the protocol and
+ *	 exposed in scmi_protocol.h.
+ * @events: An optional reference to the events supported by this protocol.
+ */
+struct scmi_protocol {
+	const u8				id;
+	struct module				*owner;
+	const scmi_prot_init_ph_fn_t		instance_init;
+	const scmi_prot_init_ph_fn_t		instance_deinit;
+	const void				*ops;
+	const struct scmi_protocol_events	*events;
+};
+
+#define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(name, proto)	\
+static const struct scmi_protocol *__this_proto = &(proto);	\
+								\
+int __init scmi_##name##_register(void)				\
+{								\
+	return scmi_protocol_register(__this_proto);		\
+}								\
+								\
+void __exit scmi_##name##_unregister(void)			\
+{								\
+	scmi_protocol_unregister(__this_proto);			\
+}
+
+#define DECLARE_SCMI_REGISTER_UNREGISTER(func)		\
+	int __init scmi_##func##_register(void);	\
+	void __exit scmi_##func##_unregister(void)
+DECLARE_SCMI_REGISTER_UNREGISTER(base);
+DECLARE_SCMI_REGISTER_UNREGISTER(clock);
+DECLARE_SCMI_REGISTER_UNREGISTER(perf);
+DECLARE_SCMI_REGISTER_UNREGISTER(power);
+DECLARE_SCMI_REGISTER_UNREGISTER(reset);
+DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
+DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
+DECLARE_SCMI_REGISTER_UNREGISTER(system);
+
+#endif /* _SCMI_PROTOCOLS_H */
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index b823a9f64af9..cc465632aa1a 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -2,7 +2,7 @@
 /*
  * System Control and Management Interface (SCMI) Reset Protocol
  *
- * Copyright (C) 2019-2021 ARM Ltd.
+ * Copyright (C) 2019-2022 ARM Ltd.
  */
 
 #define pr_fmt(fmt) "SCMI Notifications RESET - " fmt
@@ -10,7 +10,7 @@
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
 
-#include "common.h"
+#include "protocols.h"
 #include "notify.h"
 
 enum scmi_reset_protocol_cmd {
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index f37ac9824a87..07c28e249f0c 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -2,7 +2,7 @@
 /*
  * System Control and Management Interface (SCMI) Sensor Protocol
  *
- * Copyright (C) 2018-2021 ARM Ltd.
+ * Copyright (C) 2018-2022 ARM Ltd.
  */
 
 #define pr_fmt(fmt) "SCMI Notifications SENSOR - " fmt
@@ -11,7 +11,7 @@
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
 
-#include "common.h"
+#include "protocols.h"
 #include "notify.h"
 
 #define SCMI_MAX_NUM_SENSOR_AXIS	63
diff --git a/drivers/firmware/arm_scmi/system.c b/drivers/firmware/arm_scmi/system.c
index cbfc19f7a463..220e399118ad 100644
--- a/drivers/firmware/arm_scmi/system.c
+++ b/drivers/firmware/arm_scmi/system.c
@@ -2,7 +2,7 @@
 /*
  * System Control and Management Interface (SCMI) System Power Protocol
  *
- * Copyright (C) 2020-2021 ARM Ltd.
+ * Copyright (C) 2020-2022 ARM Ltd.
  */
 
 #define pr_fmt(fmt) "SCMI Notifications SYSTEM - " fmt
@@ -10,7 +10,7 @@
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
 
-#include "common.h"
+#include "protocols.h"
 #include "notify.h"
 
 #define SCMI_SYSTEM_NUM_SOURCES		1
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index ac08e819088b..7aa887a7cbd2 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -2,13 +2,13 @@
 /*
  * System Control and Management Interface (SCMI) Voltage Protocol
  *
- * Copyright (C) 2020-2021 ARM Ltd.
+ * Copyright (C) 2020-2022 ARM Ltd.
  */
 
 #include <linux/module.h>
 #include <linux/scmi_protocol.h>
 
-#include "common.h"
+#include "protocols.h"
 
 #define VOLTAGE_DOMS_NUM_MASK		GENMASK(15, 0)
 #define REMAINING_LEVELS_MASK		GENMASK(31, 16)
-- 
2.32.0

[PATCH 10/22] firmware: arm_scmi: Introduce a common SCMIv3.1 .extended_name_get helper

[Cristian Marussi]

Introduce a new set of common protocol operations bound to the protocol
handle structure so that can be invoked by the protocol implementation code
even when protocols are built as distinct loadable kernel module without
the need of exporting new symbols, like already done with scmi_xfer_ops.

Add at first, as new common protocol helper, an .extended_name_get helper
which will ease implementation and will avoid code duplication when adding
new SCMIv3.1 per-protocol _NAME_GET commands.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c    | 50 +++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/protocols.h | 15 ++++++++
 2 files changed, 65 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 8e07bee16c8e..44aa13f3cb76 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1102,6 +1102,55 @@ static const struct scmi_xfer_ops xfer_ops = {
 	.xfer_put = xfer_put,
 };
 
+struct scmi_msg_resp_domain_name_get {
+	__le32 flags;
+	u8 name[SCMI_MAX_STR_SIZE];
+};
+
+/**
+ * scmi_common_extended_name_get  - Common helper to get extended resources name
+ * @ph: A protocol handle reference.
+ * @cmd_id: The specific command ID to use.
+ * @res_id: The specific resource ID to use.
+ * @name: A pointer to the preallocated area where the retrieved name will be
+ *	  stored as a NULL terminated string.
+ * @len: The len in bytes of the @name char array.
+ *
+ * Return: 0 on Succcess
+ */
+static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
+					 u8 cmd_id, u32 res_id, char *name,
+					 size_t len)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_resp_domain_name_get *resp;
+
+	ret = ph->xops->xfer_get_init(ph, cmd_id, sizeof(res_id),
+				      sizeof(*resp), &t);
+	if (ret)
+		goto out;
+
+	put_unaligned_le32(res_id, t->tx.buf);
+	resp = t->rx.buf;
+
+	ret = ph->xops->do_xfer(ph, t);
+	if (!ret)
+		strscpy(name, resp->name, len);
+
+	ph->xops->xfer_put(ph, t);
+out:
+	if (ret)
+		dev_warn(ph->dev,
+			 "Failed to get extended name - id:%u (ret:%d). Using %s\n",
+			 res_id, ret, name);
+	return ret;
+}
+
+static const struct scmi_proto_helpers_ops helpers_ops = {
+	.extended_name_get = scmi_common_extended_name_get,
+};
+
 /**
  * scmi_revision_area_get  - Retrieve version memory area.
  *
@@ -1162,6 +1211,7 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
 	pi->handle = handle;
 	pi->ph.dev = handle->dev;
 	pi->ph.xops = &xfer_ops;
+	pi->ph.hops = &helpers_ops;
 	pi->ph.set_priv = scmi_set_protocol_priv;
 	pi->ph.get_priv = scmi_get_protocol_priv;
 	refcount_set(&pi->users, 1);
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index b2a92d2b2986..5461fa333152 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -141,6 +141,7 @@ struct scmi_xfer {
 };
 
 struct scmi_xfer_ops;
+struct scmi_proto_helpers_ops;
 
 /**
  * struct scmi_protocol_handle  - Reference to an initialized protocol instance
@@ -165,10 +166,24 @@ struct scmi_xfer_ops;
 struct scmi_protocol_handle {
 	struct device *dev;
 	const struct scmi_xfer_ops *xops;
+	const struct scmi_proto_helpers_ops *hops;
 	int (*set_priv)(const struct scmi_protocol_handle *ph, void *priv);
 	void *(*get_priv)(const struct scmi_protocol_handle *ph);
 };
 
+/**
+ * struct scmi_proto_helpers_ops  - References to common protocol helpers
+ * @extended_name_get: A common helper function to retrieve extended naming
+ *		       for the specified resource using the specified command.
+ *		       Result is returned as a NULL terminated string in the
+ *		       pre-allocated area pointed to by @name with maximum
+ *		       capacity of @len bytes.
+ */
+struct scmi_proto_helpers_ops {
+	int (*extended_name_get)(const struct scmi_protocol_handle *ph,
+				 u8 cmd_id, u32 res_id, char *name, size_t len);
+};
+
 /**
  * struct scmi_xfer_ops  - References to the core SCMI xfer operations.
  * @version_get: Get this version protocol.
-- 
2.32.0

[PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

[Cristian Marussi]

Using the common protocol helper implementation add support for all new
SCMIv3.1 extended names commands related to all protocols with the
exception of SENSOR_AXIS_GET_NAME.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c     | 21 ++++++++++++++++++---
 drivers/firmware/arm_scmi/perf.c      | 22 ++++++++++++++++++----
 drivers/firmware/arm_scmi/power.c     | 25 ++++++++++++++++++++-----
 drivers/firmware/arm_scmi/protocols.h |  2 ++
 drivers/firmware/arm_scmi/reset.c     | 22 ++++++++++++++++++----
 drivers/firmware/arm_scmi/sensors.c   | 15 ++++++++++++++-
 drivers/firmware/arm_scmi/voltage.c   | 14 +++++++++++++-
 include/linux/scmi_protocol.h         |  2 +-
 8 files changed, 104 insertions(+), 19 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index d78339dc2fdc..6dd4150b761b 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -16,6 +16,7 @@ enum scmi_clock_protocol_cmd {
 	CLOCK_RATE_SET = 0x5,
 	CLOCK_RATE_GET = 0x6,
 	CLOCK_CONFIG_SET = 0x7,
+	CLOCK_NAME_GET = 0x8,
 };
 
 struct scmi_msg_resp_clock_protocol_attributes {
@@ -27,7 +28,8 @@ struct scmi_msg_resp_clock_protocol_attributes {
 struct scmi_msg_resp_clock_attributes {
 	__le32 attributes;
 #define	CLOCK_ENABLE	BIT(0)
-	u8 name[SCMI_MAX_STR_SIZE];
+#define SUPPORTS_EXTENDED_NAMES(x)	((x) & BIT(29))
+	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 	__le32 clock_enable_latency;
 };
 
@@ -108,9 +110,11 @@ scmi_clock_protocol_attributes_get(const struct scmi_protocol_handle *ph,
 }
 
 static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
-				     u32 clk_id, struct scmi_clock_info *clk)
+				     u32 clk_id, struct scmi_clock_info *clk,
+				     u32 version)
 {
 	int ret;
+	u32 attributes;
 	struct scmi_xfer *t;
 	struct scmi_msg_resp_clock_attributes *attr;
 
@@ -124,6 +128,7 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 
 	ret = ph->xops->do_xfer(ph, t);
 	if (!ret) {
+		attributes = le32_to_cpu(attr->attributes);
 		strlcpy(clk->name, attr->name, SCMI_MAX_STR_SIZE);
 		/* Is optional field clock_enable_latency provided ? */
 		if (t->rx.len == sizeof(*attr))
@@ -132,6 +137,16 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 	}
 
 	ph->xops->xfer_put(ph, t);
+
+	/*
+	 * If supported overwrite short name with the extended one;
+	 * on error just carry on and use already provided short name.
+	 */
+	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2 &&
+	    SUPPORTS_EXTENDED_NAMES(attributes))
+		ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
+					    clk->name, SCMI_MAX_STR_SIZE);
+
 	return ret;
 }
 
@@ -401,7 +416,7 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
 	for (clkid = 0; clkid < cinfo->num_clocks; clkid++) {
 		struct scmi_clock_info *clk = cinfo->clk + clkid;
 
-		ret = scmi_clock_attributes_get(ph, clkid, clk);
+		ret = scmi_clock_attributes_get(ph, clkid, clk, version);
 		if (!ret)
 			scmi_clock_describe_rates_get(ph, clkid, clk);
 	}
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 0e9703310758..9e046fd121b9 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -32,6 +32,7 @@ enum scmi_performance_protocol_cmd {
 	PERF_NOTIFY_LIMITS = 0x9,
 	PERF_NOTIFY_LEVEL = 0xa,
 	PERF_DESCRIBE_FASTCHANNEL = 0xb,
+	PERF_DOMAIN_NAME_GET = 0xc,
 };
 
 struct scmi_opp {
@@ -56,10 +57,11 @@ struct scmi_msg_resp_perf_domain_attributes {
 #define SUPPORTS_PERF_LIMIT_NOTIFY(x)	((x) & BIT(29))
 #define SUPPORTS_PERF_LEVEL_NOTIFY(x)	((x) & BIT(28))
 #define SUPPORTS_PERF_FASTCHANNELS(x)	((x) & BIT(27))
+#define SUPPORTS_EXTENDED_NAMES(x)	((x) & BIT(26))
 	__le32 rate_limit_us;
 	__le32 sustained_freq_khz;
 	__le32 sustained_perf_level;
-	    u8 name[SCMI_MAX_STR_SIZE];
+	    u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 };
 
 struct scmi_msg_perf_describe_levels {
@@ -209,9 +211,11 @@ static int scmi_perf_attributes_get(const struct scmi_protocol_handle *ph,
 
 static int
 scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
-				u32 domain, struct perf_dom_info *dom_info)
+				u32 domain, struct perf_dom_info *dom_info,
+				u32 version)
 {
 	int ret;
+	u32 flags;
 	struct scmi_xfer *t;
 	struct scmi_msg_resp_perf_domain_attributes *attr;
 
@@ -225,7 +229,7 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 
 	ret = ph->xops->do_xfer(ph, t);
 	if (!ret) {
-		u32 flags = le32_to_cpu(attr->flags);
+		flags = le32_to_cpu(attr->flags);
 
 		dom_info->set_limits = SUPPORTS_SET_LIMITS(flags);
 		dom_info->set_perf = SUPPORTS_SET_PERF_LVL(flags);
@@ -248,6 +252,16 @@ scmi_perf_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	}
 
 	ph->xops->xfer_put(ph, t);
+
+	/*
+	 * If supported overwrite short name with the extended one;
+	 * on error just carry on and use already provided short name.
+	 */
+	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
+	    SUPPORTS_EXTENDED_NAMES(flags))
+		ph->hops->extended_name_get(ph, PERF_DOMAIN_NAME_GET, domain,
+					    dom_info->name, SCMI_MAX_STR_SIZE);
+
 	return ret;
 }
 
@@ -902,7 +916,7 @@ static int scmi_perf_protocol_init(const struct scmi_protocol_handle *ph)
 	for (domain = 0; domain < pinfo->num_domains; domain++) {
 		struct perf_dom_info *dom = pinfo->dom_info + domain;
 
-		scmi_perf_domain_attributes_get(ph, domain, dom);
+		scmi_perf_domain_attributes_get(ph, domain, dom, version);
 		scmi_perf_describe_levels_get(ph, domain, dom);
 
 		if (dom->perf_fastchannels)
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index e378a3eb0d07..964882cc8747 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -18,6 +18,7 @@ enum scmi_power_protocol_cmd {
 	POWER_STATE_SET = 0x4,
 	POWER_STATE_GET = 0x5,
 	POWER_STATE_NOTIFY = 0x6,
+	POWER_DOMAIN_NAME_GET = 0x8,
 };
 
 struct scmi_msg_resp_power_attributes {
@@ -33,7 +34,8 @@ struct scmi_msg_resp_power_domain_attributes {
 #define SUPPORTS_STATE_SET_NOTIFY(x)	((x) & BIT(31))
 #define SUPPORTS_STATE_SET_ASYNC(x)	((x) & BIT(30))
 #define SUPPORTS_STATE_SET_SYNC(x)	((x) & BIT(29))
-	    u8 name[SCMI_MAX_STR_SIZE];
+#define SUPPORTS_EXTENDED_NAMES(x)	((x) & BIT(27))
+	    u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 };
 
 struct scmi_power_set_state {
@@ -97,9 +99,11 @@ static int scmi_power_attributes_get(const struct scmi_protocol_handle *ph,
 
 static int
 scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
-				 u32 domain, struct power_dom_info *dom_info)
+				 u32 domain, struct power_dom_info *dom_info,
+				 u32 version)
 {
 	int ret;
+	u32 flags;
 	struct scmi_xfer *t;
 	struct scmi_msg_resp_power_domain_attributes *attr;
 
@@ -113,15 +117,26 @@ scmi_power_domain_attributes_get(const struct scmi_protocol_handle *ph,
 
 	ret = ph->xops->do_xfer(ph, t);
 	if (!ret) {
-		u32 flags = le32_to_cpu(attr->flags);
+		flags = le32_to_cpu(attr->flags);
 
 		dom_info->state_set_notify = SUPPORTS_STATE_SET_NOTIFY(flags);
 		dom_info->state_set_async = SUPPORTS_STATE_SET_ASYNC(flags);
 		dom_info->state_set_sync = SUPPORTS_STATE_SET_SYNC(flags);
 		strlcpy(dom_info->name, attr->name, SCMI_MAX_STR_SIZE);
 	}
-
 	ph->xops->xfer_put(ph, t);
+
+	/*
+	 * If supported overwrite short name with the extended one;
+	 * on error just carry on and use already provided short name.
+	 */
+	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
+	    SUPPORTS_EXTENDED_NAMES(flags)) {
+		ph->hops->extended_name_get(ph, POWER_DOMAIN_NAME_GET,
+					    domain, dom_info->name,
+					    SCMI_MAX_STR_SIZE);
+	}
+
 	return ret;
 }
 
@@ -308,7 +323,7 @@ static int scmi_power_protocol_init(const struct scmi_protocol_handle *ph)
 	for (domain = 0; domain < pinfo->num_domains; domain++) {
 		struct power_dom_info *dom = pinfo->dom_info + domain;
 
-		scmi_power_domain_attributes_get(ph, domain, dom);
+		scmi_power_domain_attributes_get(ph, domain, dom, version);
 	}
 
 	pinfo->version = version;
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 5461fa333152..60ea880b3855 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -24,6 +24,8 @@
 
 #include <asm/unaligned.h>
 
+#define SCMI_SHORT_NAME_MAX_SIZE	16
+
 #define PROTOCOL_REV_MINOR_MASK	GENMASK(15, 0)
 #define PROTOCOL_REV_MAJOR_MASK	GENMASK(31, 16)
 #define PROTOCOL_REV_MAJOR(x)	((u16)(FIELD_GET(PROTOCOL_REV_MAJOR_MASK, (x))))
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index cc465632aa1a..a420a9102094 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -17,6 +17,7 @@ enum scmi_reset_protocol_cmd {
 	RESET_DOMAIN_ATTRIBUTES = 0x3,
 	RESET = 0x4,
 	RESET_NOTIFY = 0x5,
+	RESET_DOMAIN_NAME_GET = 0x6,
 };
 
 #define NUM_RESET_DOMAIN_MASK	0xffff
@@ -26,8 +27,9 @@ struct scmi_msg_resp_reset_domain_attributes {
 	__le32 attributes;
 #define SUPPORTS_ASYNC_RESET(x)		((x) & BIT(31))
 #define SUPPORTS_NOTIFY_RESET(x)	((x) & BIT(30))
+#define SUPPORTS_EXTENDED_NAMES(x)	((x) & BIT(29))
 	__le32 latency;
-	    u8 name[SCMI_MAX_STR_SIZE];
+	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 };
 
 struct scmi_msg_reset_domain_reset {
@@ -89,9 +91,11 @@ static int scmi_reset_attributes_get(const struct scmi_protocol_handle *ph,
 
 static int
 scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
-				 u32 domain, struct reset_dom_info *dom_info)
+				 u32 domain, struct reset_dom_info *dom_info,
+				 u32 version)
 {
 	int ret;
+	u32 attributes;
 	struct scmi_xfer *t;
 	struct scmi_msg_resp_reset_domain_attributes *attr;
 
@@ -105,7 +109,7 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
 
 	ret = ph->xops->do_xfer(ph, t);
 	if (!ret) {
-		u32 attributes = le32_to_cpu(attr->attributes);
+		attributes = le32_to_cpu(attr->attributes);
 
 		dom_info->async_reset = SUPPORTS_ASYNC_RESET(attributes);
 		dom_info->reset_notify = SUPPORTS_NOTIFY_RESET(attributes);
@@ -116,6 +120,16 @@ scmi_reset_domain_attributes_get(const struct scmi_protocol_handle *ph,
 	}
 
 	ph->xops->xfer_put(ph, t);
+
+	/*
+	 * If supported overwrite short name with the extended one;
+	 * on error just carry on and use already provided short name.
+	 */
+	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x3 &&
+	    SUPPORTS_EXTENDED_NAMES(attributes))
+		ph->hops->extended_name_get(ph, RESET_DOMAIN_NAME_GET, domain,
+					    dom_info->name, SCMI_MAX_STR_SIZE);
+
 	return ret;
 }
 
@@ -320,7 +334,7 @@ static int scmi_reset_protocol_init(const struct scmi_protocol_handle *ph)
 	for (domain = 0; domain < pinfo->num_domains; domain++) {
 		struct reset_dom_info *dom = pinfo->dom_info + domain;
 
-		scmi_reset_domain_attributes_get(ph, domain, dom);
+		scmi_reset_domain_attributes_get(ph, domain, dom, version);
 	}
 
 	pinfo->version = version;
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 07c28e249f0c..6fd8b3a874ea 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -27,6 +27,7 @@ enum scmi_sensor_protocol_cmd {
 	SENSOR_CONFIG_GET = 0x9,
 	SENSOR_CONFIG_SET = 0xA,
 	SENSOR_CONTINUOUS_UPDATE_NOTIFY = 0xB,
+	SENSOR_NAME_GET = 0xC,
 };
 
 struct scmi_msg_resp_sensor_attributes {
@@ -71,6 +72,7 @@ struct scmi_msg_resp_sensor_description {
 		__le32 attributes_low;
 /* Common attributes_low macros */
 #define SUPPORTS_ASYNC_READ(x)		FIELD_GET(BIT(31), (x))
+#define SUPPORTS_EXTENDED_NAMES(x)	FIELD_GET(BIT(29), (x))
 #define NUM_TRIP_POINTS(x)		FIELD_GET(GENMASK(7, 0), (x))
 		__le32 attributes_high;
 /* Common attributes_high macros */
@@ -78,7 +80,7 @@ struct scmi_msg_resp_sensor_description {
 #define SENSOR_SCALE_SIGN		BIT(4)
 #define SENSOR_SCALE_EXTEND		GENMASK(31, 5)
 #define SENSOR_TYPE(x)			FIELD_GET(GENMASK(7, 0), (x))
-		u8 name[SCMI_MAX_STR_SIZE];
+		u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 		/* only for version > 2.0 */
 		__le32 power;
 		__le32 resolution;
@@ -519,6 +521,17 @@ static int scmi_sensor_description_get(const struct scmi_protocol_handle *ph,
 					    SCMI_MAX_NUM_SENSOR_AXIS);
 			strlcpy(s->name, sdesc->name, SCMI_MAX_STR_SIZE);
 
+			/*
+			 * If supported overwrite short name with the extended
+			 * one; on error just carry on and use already provided
+			 * short name.
+			 */
+			if (PROTOCOL_REV_MAJOR(si->version) >= 0x3 &&
+			    SUPPORTS_EXTENDED_NAMES(attrl))
+				ph->hops->extended_name_get(ph, SENSOR_NAME_GET,
+							    s->id, s->name,
+							    SCMI_MAX_STR_SIZE);
+
 			if (s->extended_scalar_attrs) {
 				s->sensor_power = le32_to_cpu(sdesc->power);
 				dsize += sizeof(sdesc->power);
diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index 7aa887a7cbd2..5d58ba724eeb 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -21,13 +21,15 @@ enum scmi_voltage_protocol_cmd {
 	VOLTAGE_CONFIG_GET = 0x6,
 	VOLTAGE_LEVEL_SET = 0x7,
 	VOLTAGE_LEVEL_GET = 0x8,
+	VOLTAGE_DOMAIN_NAME_GET = 0x09,
 };
 
 #define NUM_VOLTAGE_DOMAINS(x)	((u16)(FIELD_GET(VOLTAGE_DOMS_NUM_MASK, (x))))
 
 struct scmi_msg_resp_domain_attributes {
 	__le32 attr;
-	u8 name[SCMI_MAX_STR_SIZE];
+#define SUPPORTS_EXTENDED_NAMES(x)	((x) & BIT(30))
+	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 };
 
 struct scmi_msg_cmd_describe_levels {
@@ -149,6 +151,16 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 		v->attributes = le32_to_cpu(resp_dom->attr);
 		strlcpy(v->name, resp_dom->name, SCMI_MAX_STR_SIZE);
 
+		/*
+		 * If supported overwrite short name with the extended one;
+		 * on error just carry on and use already provided short name.
+		 */
+		if (PROTOCOL_REV_MAJOR(vinfo->version) >= 0x2 &&
+		    SUPPORTS_EXTENDED_NAMES(v->attributes))
+			ph->hops->extended_name_get(ph, VOLTAGE_DOMAIN_NAME_GET,
+						    v->id, v->name,
+						    SCMI_MAX_STR_SIZE);
+
 		cmd = tl->tx.buf;
 		/* ...then retrieve domain levels descriptions */
 		do {
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index ced37d1de1fe..56e6f13355b8 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -13,7 +13,7 @@
 #include <linux/notifier.h>
 #include <linux/types.h>
 
-#define SCMI_MAX_STR_SIZE	16
+#define SCMI_MAX_STR_SIZE	64
 #define SCMI_MAX_NUM_RATES	16
 
 /**
-- 
2.32.0

Re: [PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

[Florian Fainelli]

On 3/30/2022 5:05 PM, Cristian Marussi wrote:
> Using the common protocol helper implementation add support for all new
> SCMIv3.1 extended names commands related to all protocols with the
> exception of SENSOR_AXIS_GET_NAME.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

This causes the following splat on a platform where regulators fail to 
initialize:

[    0.603737] ------------[ cut here ]------------
[    0.603752] WARNING: CPU: 1 PID: 1 at mm/page_alloc.c:5402 
__alloc_pages+0x6c/0x184
[    0.603797] Modules linked in:
[    0.603809] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 
5.19.0-rc1-g44dbdf3bb3f4 #42
[    0.603818] Hardware name: BCX972160SV (DT)
[    0.603825] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS 
BTYPE=--)
[    0.603834] pc : __alloc_pages+0x6c/0x184
[    0.603841] lr : kmalloc_order+0x40/0x88
[    0.603851] sp : ffffffc00a40b850
[    0.603856] x29: ffffffc00a40b850 x28: 0000000000000000 x27: 
ffffffc008d60404
[    0.603867] x26: ffffff80c1e3e1a8 x25: ffffffc00877bd78 x24: 
0000000000000058
[    0.603878] x23: ffffffc0081921a8 x22: ffffffc008cb04b0 x21: 
0000000000000000
[    0.603889] x20: 000000000000000b x19: 000000000000000b x18: 
0000000000000000
[    0.603900] x17: 0000000000000001 x16: 0000000100000000 x15: 
000000000000000a
[    0.603911] x14: 0000000000000000 x13: ffffff80c1e3c20a x12: 
ffffffffffffffff
[    0.603922] x11: 0000000000000020 x10: 0000000000000880 x9 : 
ffffffc008159dac
[    0.603932] x8 : ffffff80c02708e0 x7 : 0000000000000004 x6 : 
000000000041a880
[    0.603943] x5 : 0000000000000001 x4 : ffffff8000000000 x3 : 
0000000000000000
[    0.603954] x2 : 0000000000000000 x1 : 0000000000000001 x0 : 
ffffffc00a32d3f2
[    0.603965] Call trace:
[    0.603970]  __alloc_pages+0x6c/0x184
[    0.603977]  kmalloc_order+0x40/0x88
[    0.603984]  kmalloc_order_trace+0x30/0xd0
[    0.603992]  __kmalloc_track_caller+0x64/0x19c
[    0.603999]  devm_kmalloc+0x5c/0xe0
[    0.604009]  scmi_voltage_protocol_init+0x14c/0x2f4
[    0.604020]  scmi_get_protocol_instance+0x128/0x1f4
[    0.604030]  scmi_devm_protocol_get+0x64/0xc8
[    0.604037]  scmi_regulator_probe+0x5c/0x42c
[    0.604049]  scmi_dev_probe+0x28/0x38
[    0.604056]  really_probe+0x1b8/0x380
[    0.604065]  __driver_probe_device+0x14c/0x164
[    0.604073]  driver_probe_device+0x48/0xe0
[    0.604080]  __driver_attach+0x160/0x170
[    0.604087]  bus_for_each_dev+0x78/0xb8
[    0.604095]  driver_attach+0x28/0x30
[    0.604101]  bus_add_driver+0xf4/0x208
[    0.604108]  driver_register+0xb4/0xf0
[    0.604116]  scmi_driver_register+0x5c/0xa4
[    0.604123]  scmi_drv_init+0x28/0x30
[    0.604132]  do_one_initcall+0x80/0x1a4
[    0.604141]  kernel_init_freeable+0x220/0x23c
[    0.604149]  kernel_init+0x28/0x128
[    0.604158]  ret_from_fork+0x10/0x20
[    0.604166] ---[ end trace 0000000000000000 ]---
[    0.604194] scmi-regulator: probe of scmi_dev.2 failed with error -12
[    0.604792] arm-scmi brcm_scmi@0: Failed. SCMI protocol 22 not active.
-- 
Florian

Re: [PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

[Cristian Marussi]

On Wed, Jun 15, 2022 at 05:45:11AM +0200, Florian Fainelli wrote:
> 
> 
> On 3/30/2022 5:05 PM, Cristian Marussi wrote:
> > Using the common protocol helper implementation add support for all new
> > SCMIv3.1 extended names commands related to all protocols with the
> > exception of SENSOR_AXIS_GET_NAME.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> 
> This causes the following splat on a platform where regulators fail to
> initialize:
> 

Hi Florian,

thanks for the report.

It seems a memory error while allocating so it was not meant to be
solved by the fixes, anyway, I've never seen this splat in my testing
and at first sight I cannot see anything wrong in the devm_k* calls
inside scmi_voltage_protocol_init...is there any particular config in
your setup ?

Moreover, the WARNING line 5402 seems to match v5.19-rc1 and it has
slightly changed with -rc-1, so I'll try rebasing on that at first and
see if I can reproduce the issue locally.

Thanks,
Cristian

> [    0.603737] ------------[ cut here ]------------
> [    0.603752] WARNING: CPU: 1 PID: 1 at mm/page_alloc.c:5402
> __alloc_pages+0x6c/0x184
> [    0.603797] Modules linked in:
> [    0.603809] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
> 5.19.0-rc1-g44dbdf3bb3f4 #42
> [    0.603818] Hardware name: BCX972160SV (DT)
> [    0.603825] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS
> BTYPE=--)
> [    0.603834] pc : __alloc_pages+0x6c/0x184
> [    0.603841] lr : kmalloc_order+0x40/0x88
> [    0.603851] sp : ffffffc00a40b850
> [    0.603856] x29: ffffffc00a40b850 x28: 0000000000000000 x27:
> ffffffc008d60404
> [    0.603867] x26: ffffff80c1e3e1a8 x25: ffffffc00877bd78 x24:
> 0000000000000058
> [    0.603878] x23: ffffffc0081921a8 x22: ffffffc008cb04b0 x21:
> 0000000000000000
> [    0.603889] x20: 000000000000000b x19: 000000000000000b x18:
> 0000000000000000
> [    0.603900] x17: 0000000000000001 x16: 0000000100000000 x15:
> 000000000000000a
> [    0.603911] x14: 0000000000000000 x13: ffffff80c1e3c20a x12:
> ffffffffffffffff
> [    0.603922] x11: 0000000000000020 x10: 0000000000000880 x9 :
> ffffffc008159dac
> [    0.603932] x8 : ffffff80c02708e0 x7 : 0000000000000004 x6 :
> 000000000041a880
> [    0.603943] x5 : 0000000000000001 x4 : ffffff8000000000 x3 :
> 0000000000000000
> [    0.603954] x2 : 0000000000000000 x1 : 0000000000000001 x0 :
> ffffffc00a32d3f2
> [    0.603965] Call trace:
> [    0.603970]  __alloc_pages+0x6c/0x184
> [    0.603977]  kmalloc_order+0x40/0x88
> [    0.603984]  kmalloc_order_trace+0x30/0xd0
> [    0.603992]  __kmalloc_track_caller+0x64/0x19c
> [    0.603999]  devm_kmalloc+0x5c/0xe0
> [    0.604009]  scmi_voltage_protocol_init+0x14c/0x2f4
> [    0.604020]  scmi_get_protocol_instance+0x128/0x1f4
> [    0.604030]  scmi_devm_protocol_get+0x64/0xc8
> [    0.604037]  scmi_regulator_probe+0x5c/0x42c
> [    0.604049]  scmi_dev_probe+0x28/0x38
> [    0.604056]  really_probe+0x1b8/0x380
> [    0.604065]  __driver_probe_device+0x14c/0x164
> [    0.604073]  driver_probe_device+0x48/0xe0
> [    0.604080]  __driver_attach+0x160/0x170
> [    0.604087]  bus_for_each_dev+0x78/0xb8
> [    0.604095]  driver_attach+0x28/0x30
> [    0.604101]  bus_add_driver+0xf4/0x208
> [    0.604108]  driver_register+0xb4/0xf0
> [    0.604116]  scmi_driver_register+0x5c/0xa4
> [    0.604123]  scmi_drv_init+0x28/0x30
> [    0.604132]  do_one_initcall+0x80/0x1a4
> [    0.604141]  kernel_init_freeable+0x220/0x23c
> [    0.604149]  kernel_init+0x28/0x128
> [    0.604158]  ret_from_fork+0x10/0x20
> [    0.604166] ---[ end trace 0000000000000000 ]---
> [    0.604194] scmi-regulator: probe of scmi_dev.2 failed with error -12
> [    0.604792] arm-scmi brcm_scmi@0: Failed. SCMI protocol 22 not active.
> -- 
> Florian

Re: [PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

[Cristian Marussi]

On Wed, Jun 15, 2022 at 09:18:03AM +0100, Cristian Marussi wrote:
> On Wed, Jun 15, 2022 at 05:45:11AM +0200, Florian Fainelli wrote:
> > 
> > 
> > On 3/30/2022 5:05 PM, Cristian Marussi wrote:
> > > Using the common protocol helper implementation add support for all new
> > > SCMIv3.1 extended names commands related to all protocols with the
> > > exception of SENSOR_AXIS_GET_NAME.
> > > 
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > 
> > This causes the following splat on a platform where regulators fail to
> > initialize:
> > 
> 
> Hi Florian,
> 
> thanks for the report.
> 
> It seems a memory error while allocating so it was not meant to be
> solved by the fixes, anyway, I've never seen this splat in my testing
> and at first sight I cannot see anything wrong in the devm_k* calls
> inside scmi_voltage_protocol_init...is there any particular config in
> your setup ?
> 
> Moreover, the WARNING line 5402 seems to match v5.19-rc1 and it has
> slightly changed with -rc-1, so I'll try rebasing on that at first and
> see if I can reproduce the issue locally.
> 

I just re-tested the series rebased on v519-rc1 plus fixes and I cannot
reproduce in my setup with a few (~9) good and bad voltage domains.

How many voltage domains are advertised by the platform in your setup ?

Thanks,
Cristian

> 
> > [    0.603737] ------------[ cut here ]------------
> > [    0.603752] WARNING: CPU: 1 PID: 1 at mm/page_alloc.c:5402
> > __alloc_pages+0x6c/0x184
> > [    0.603797] Modules linked in:
> > [    0.603809] CPU: 1 PID: 1 Comm: swapper/0 Not tainted
> > 5.19.0-rc1-g44dbdf3bb3f4 #42
> > [    0.603818] Hardware name: BCX972160SV (DT)
> > [    0.603825] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS
> > BTYPE=--)
> > [    0.603834] pc : __alloc_pages+0x6c/0x184
> > [    0.603841] lr : kmalloc_order+0x40/0x88
> > [    0.603851] sp : ffffffc00a40b850
> > [    0.603856] x29: ffffffc00a40b850 x28: 0000000000000000 x27:
> > ffffffc008d60404
> > [    0.603867] x26: ffffff80c1e3e1a8 x25: ffffffc00877bd78 x24:
> > 0000000000000058
> > [    0.603878] x23: ffffffc0081921a8 x22: ffffffc008cb04b0 x21:
> > 0000000000000000
> > [    0.603889] x20: 000000000000000b x19: 000000000000000b x18:
> > 0000000000000000
> > [    0.603900] x17: 0000000000000001 x16: 0000000100000000 x15:
> > 000000000000000a
> > [    0.603911] x14: 0000000000000000 x13: ffffff80c1e3c20a x12:
> > ffffffffffffffff
> > [    0.603922] x11: 0000000000000020 x10: 0000000000000880 x9 :
> > ffffffc008159dac
> > [    0.603932] x8 : ffffff80c02708e0 x7 : 0000000000000004 x6 :
> > 000000000041a880
> > [    0.603943] x5 : 0000000000000001 x4 : ffffff8000000000 x3 :
> > 0000000000000000
> > [    0.603954] x2 : 0000000000000000 x1 : 0000000000000001 x0 :
> > ffffffc00a32d3f2
> > [    0.603965] Call trace:
> > [    0.603970]  __alloc_pages+0x6c/0x184
> > [    0.603977]  kmalloc_order+0x40/0x88
> > [    0.603984]  kmalloc_order_trace+0x30/0xd0
> > [    0.603992]  __kmalloc_track_caller+0x64/0x19c
> > [    0.603999]  devm_kmalloc+0x5c/0xe0
> > [    0.604009]  scmi_voltage_protocol_init+0x14c/0x2f4
> > [    0.604020]  scmi_get_protocol_instance+0x128/0x1f4
> > [    0.604030]  scmi_devm_protocol_get+0x64/0xc8
> > [    0.604037]  scmi_regulator_probe+0x5c/0x42c
> > [    0.604049]  scmi_dev_probe+0x28/0x38
> > [    0.604056]  really_probe+0x1b8/0x380
> > [    0.604065]  __driver_probe_device+0x14c/0x164
> > [    0.604073]  driver_probe_device+0x48/0xe0
> > [    0.604080]  __driver_attach+0x160/0x170
> > [    0.604087]  bus_for_each_dev+0x78/0xb8
> > [    0.604095]  driver_attach+0x28/0x30
> > [    0.604101]  bus_add_driver+0xf4/0x208
> > [    0.604108]  driver_register+0xb4/0xf0
> > [    0.604116]  scmi_driver_register+0x5c/0xa4
> > [    0.604123]  scmi_drv_init+0x28/0x30
> > [    0.604132]  do_one_initcall+0x80/0x1a4
> > [    0.604141]  kernel_init_freeable+0x220/0x23c
> > [    0.604149]  kernel_init+0x28/0x128
> > [    0.604158]  ret_from_fork+0x10/0x20
> > [    0.604166] ---[ end trace 0000000000000000 ]---
> > [    0.604194] scmi-regulator: probe of scmi_dev.2 failed with error -12
> > [    0.604792] arm-scmi brcm_scmi@0: Failed. SCMI protocol 22 not active.
> > -- 
> > Florian

Re: [PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

[Florian Fainelli]

On 6/15/22 02:40, Cristian Marussi wrote:
> On Wed, Jun 15, 2022 at 09:18:03AM +0100, Cristian Marussi wrote:
>> On Wed, Jun 15, 2022 at 05:45:11AM +0200, Florian Fainelli wrote:
>>>
>>>
>>> On 3/30/2022 5:05 PM, Cristian Marussi wrote:
>>>> Using the common protocol helper implementation add support for all new
>>>> SCMIv3.1 extended names commands related to all protocols with the
>>>> exception of SENSOR_AXIS_GET_NAME.
>>>>
>>>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>>>
>>> This causes the following splat on a platform where regulators fail to
>>> initialize:
>>>
>>
>> Hi Florian,
>>
>> thanks for the report.
>>
>> It seems a memory error while allocating so it was not meant to be
>> solved by the fixes, anyway, I've never seen this splat in my testing
>> and at first sight I cannot see anything wrong in the devm_k* calls
>> inside scmi_voltage_protocol_init...is there any particular config in
>> your setup ?
>>
>> Moreover, the WARNING line 5402 seems to match v5.19-rc1 and it has
>> slightly changed with -rc-1, so I'll try rebasing on that at first and
>> see if I can reproduce the issue locally.
>>
> 
> I just re-tested the series rebased on v519-rc1 plus fixes and I cannot
> reproduce in my setup with a few (~9) good and bad voltage domains.
> 
> How many voltage domains are advertised by the platform in your setup ?

There are 11 voltage regulators on this platform, and of course, now 
that I am trying to reproduce the splat I reported I just cannot 
anymore... I will let you know if there is anything that needs to be 
done. Thanks for being responsive as usual!
-- 
Florian

Re: [PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

[Cristian Marussi]

On Wed, Jun 15, 2022 at 09:10:03AM -0700, Florian Fainelli wrote:
> On 6/15/22 02:40, Cristian Marussi wrote:
> > On Wed, Jun 15, 2022 at 09:18:03AM +0100, Cristian Marussi wrote:
> > > On Wed, Jun 15, 2022 at 05:45:11AM +0200, Florian Fainelli wrote:
> > > > 
> > > > 
> > > > On 3/30/2022 5:05 PM, Cristian Marussi wrote:
> > > > > Using the common protocol helper implementation add support for all new
> > > > > SCMIv3.1 extended names commands related to all protocols with the
> > > > > exception of SENSOR_AXIS_GET_NAME.
> > > > > 
> > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > > 
> > > > This causes the following splat on a platform where regulators fail to
> > > > initialize:
> > > > 
> > > 
> > > Hi Florian,
> > > 
> > > thanks for the report.
> > > 
> > > It seems a memory error while allocating so it was not meant to be
> > > solved by the fixes, anyway, I've never seen this splat in my testing
> > > and at first sight I cannot see anything wrong in the devm_k* calls
> > > inside scmi_voltage_protocol_init...is there any particular config in
> > > your setup ?
> > > 
> > > Moreover, the WARNING line 5402 seems to match v5.19-rc1 and it has
> > > slightly changed with -rc-1, so I'll try rebasing on that at first and
> > > see if I can reproduce the issue locally.
> > > 
> > 
> > I just re-tested the series rebased on v519-rc1 plus fixes and I cannot
> > reproduce in my setup with a few (~9) good and bad voltage domains.
> > 
> > How many voltage domains are advertised by the platform in your setup ?
> 
> There are 11 voltage regulators on this platform, and of course, now that I
> am trying to reproduce the splat I reported I just cannot anymore... I will
> let you know if there is anything that needs to be done. Thanks for being
> responsive as usual!

... you're welcome...

I'm trying to figure out where an abnormal mem request could happen...

can you try adding this (for brutal debugging) when you try ?
(...just to rule out funny fw replies.... :D)

Thanks,
Cristian

--->8----

diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index 895741b66f5a..fd841292df5c 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -108,6 +108,9 @@ static int scmi_init_voltage_levels(struct device *dev,
                return -EINVAL;
        }
 
+       dev_info(dev, "num_returned:%d  num_remaining:%d\n",
+                num_returned, num_remaining);
+
        v->levels_uv = devm_kcalloc(dev, num_levels, sizeof(u32), GFP_KERNEL);
        if (!v->levels_uv)
                return -ENOMEM;


> -- 
> Florian

Re: [PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

[Florian Fainelli]

On 6/15/22 09:29, Cristian Marussi wrote:
> On Wed, Jun 15, 2022 at 09:10:03AM -0700, Florian Fainelli wrote:
>> On 6/15/22 02:40, Cristian Marussi wrote:
>>> On Wed, Jun 15, 2022 at 09:18:03AM +0100, Cristian Marussi wrote:
>>>> On Wed, Jun 15, 2022 at 05:45:11AM +0200, Florian Fainelli wrote:
>>>>>
>>>>>
>>>>> On 3/30/2022 5:05 PM, Cristian Marussi wrote:
>>>>>> Using the common protocol helper implementation add support for all new
>>>>>> SCMIv3.1 extended names commands related to all protocols with the
>>>>>> exception of SENSOR_AXIS_GET_NAME.
>>>>>>
>>>>>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>>>>>
>>>>> This causes the following splat on a platform where regulators fail to
>>>>> initialize:
>>>>>
>>>>
>>>> Hi Florian,
>>>>
>>>> thanks for the report.
>>>>
>>>> It seems a memory error while allocating so it was not meant to be
>>>> solved by the fixes, anyway, I've never seen this splat in my testing
>>>> and at first sight I cannot see anything wrong in the devm_k* calls
>>>> inside scmi_voltage_protocol_init...is there any particular config in
>>>> your setup ?
>>>>
>>>> Moreover, the WARNING line 5402 seems to match v5.19-rc1 and it has
>>>> slightly changed with -rc-1, so I'll try rebasing on that at first and
>>>> see if I can reproduce the issue locally.
>>>>
>>>
>>> I just re-tested the series rebased on v519-rc1 plus fixes and I cannot
>>> reproduce in my setup with a few (~9) good and bad voltage domains.
>>>
>>> How many voltage domains are advertised by the platform in your setup ?
>>
>> There are 11 voltage regulators on this platform, and of course, now that I
>> am trying to reproduce the splat I reported I just cannot anymore... I will
>> let you know if there is anything that needs to be done. Thanks for being
>> responsive as usual!
> 
> ... you're welcome...
> 
> I'm trying to figure out where an abnormal mem request could happen...

I think the problem is/was with the number of voltage domains being 
reported which was way too big and passed directly, without any clamping 
to devm_kcalloc() resulting the splat indicating that the allocation was 
beyond the MAX_ORDER. The specification allows for up to 2^16 domains 
which would still be way too much to allocate using kmalloc() so we 
could/should consider vmalloc() here eventually?

In all likelihood though we probably won't find a system with 65k 
voltage domains.

> 
> can you try adding this (for brutal debugging) when you try ?
> (...just to rule out funny fw replies.... :D)

Sure, nothing weird coming out and it succeeded in enumerating all of 
the regulators, I smell a transient issue with our firmware 
implementation, maybe...

[    0.560544] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.560617] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.560673] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.560730] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.560881] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.560940] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.560996] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.561054] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.561110] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.561168] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.561225] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
[    0.561652] scmi-regulator scmi_dev.2: Regulator stb_vreg_2 
registered for domain [2]
[    0.561858] scmi-regulator scmi_dev.2: Regulator stb_vreg_3 
registered for domain [3]
[    0.562030] scmi-regulator scmi_dev.2: Regulator stb_vreg_4 
registered for domain [4]
[    0.562190] scmi-regulator scmi_dev.2: Regulator stb_vreg_5 
registered for domain [5]
[    0.564427] scmi-regulator scmi_dev.2: Regulator stb_vreg_6 
registered for domain [6]
[    0.564638] scmi-regulator scmi_dev.2: Regulator stb_vreg_7 
registered for domain [7]
[    0.564817] scmi-regulator scmi_dev.2: Regulator stb_vreg_8 
registered for domain [8]
[    0.565030] scmi-regulator scmi_dev.2: Regulator stb_vreg_9 
registered for domain [9]
[    0.565191] scmi-regulator scmi_dev.2: Regulator stb_vreg_10 
registered for domain [10]

-- 
Florian

Re: [PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

[Cristian Marussi]

On Wed, Jun 15, 2022 at 10:19:53AM -0700, Florian Fainelli wrote:
> On 6/15/22 09:29, Cristian Marussi wrote:
> > On Wed, Jun 15, 2022 at 09:10:03AM -0700, Florian Fainelli wrote:
> > > On 6/15/22 02:40, Cristian Marussi wrote:
> > > > On Wed, Jun 15, 2022 at 09:18:03AM +0100, Cristian Marussi wrote:
> > > > > On Wed, Jun 15, 2022 at 05:45:11AM +0200, Florian Fainelli wrote:
> > > > > > 
> > > > > > 
> > > > > > On 3/30/2022 5:05 PM, Cristian Marussi wrote:
> > > > > > > Using the common protocol helper implementation add support for all new
> > > > > > > SCMIv3.1 extended names commands related to all protocols with the
> > > > > > > exception of SENSOR_AXIS_GET_NAME.
> > > > > > > 
> > > > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > > > > 
> > > > > > This causes the following splat on a platform where regulators fail to
> > > > > > initialize:
> > > > > > 
> > > > > 
> > > > > Hi Florian,
> > > > > 
> > > > > thanks for the report.
> > > > > 
> > > > > It seems a memory error while allocating so it was not meant to be
> > > > > solved by the fixes, anyway, I've never seen this splat in my testing
> > > > > and at first sight I cannot see anything wrong in the devm_k* calls
> > > > > inside scmi_voltage_protocol_init...is there any particular config in
> > > > > your setup ?
> > > > > 
> > > > > Moreover, the WARNING line 5402 seems to match v5.19-rc1 and it has
> > > > > slightly changed with -rc-1, so I'll try rebasing on that at first and
> > > > > see if I can reproduce the issue locally.
> > > > > 
> > > > 
> > > > I just re-tested the series rebased on v519-rc1 plus fixes and I cannot
> > > > reproduce in my setup with a few (~9) good and bad voltage domains.
> > > > 
> > > > How many voltage domains are advertised by the platform in your setup ?
> > > 
> > > There are 11 voltage regulators on this platform, and of course, now that I
> > > am trying to reproduce the splat I reported I just cannot anymore... I will
> > > let you know if there is anything that needs to be done. Thanks for being
> > > responsive as usual!
> > 
> > ... you're welcome...
> > 
> > I'm trying to figure out where an abnormal mem request could happen...
> 
> I think the problem is/was with the number of voltage domains being reported
> which was way too big and passed directly, without any clamping to
> devm_kcalloc() resulting the splat indicating that the allocation was beyond
> the MAX_ORDER. The specification allows for up to 2^16 domains which would
> still be way too much to allocate using kmalloc() so we could/should
> consider vmalloc() here eventually?
> 

Yes I was suspicious about the same and I was starting to think about vmalloc
in general across all domain enumerations even if this is may not be the case
for your splat...

> In all likelihood though we probably won't find a system with 65k voltage
> domains.
> 
> > 
> > can you try adding this (for brutal debugging) when you try ?
> > (...just to rule out funny fw replies.... :D)
> 
> Sure, nothing weird coming out and it succeeded in enumerating all of the
> regulators, I smell a transient issue with our firmware implementation,
> maybe...
> 
> [    0.560544] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.560617] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.560673] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.560730] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.560881] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.560940] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.560996] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.561054] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.561110] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.561168] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.561225] arm-scmi brcm_scmi@0: num_returned:1  num_remaining:0
> [    0.561652] scmi-regulator scmi_dev.2: Regulator stb_vreg_2 registered
> for domain [2]
> [    0.561858] scmi-regulator scmi_dev.2: Regulator stb_vreg_3 registered
> for domain [3]
> [    0.562030] scmi-regulator scmi_dev.2: Regulator stb_vreg_4 registered
> for domain [4]
> [    0.562190] scmi-regulator scmi_dev.2: Regulator stb_vreg_5 registered
> for domain [5]
> [    0.564427] scmi-regulator scmi_dev.2: Regulator stb_vreg_6 registered
> for domain [6]
> [    0.564638] scmi-regulator scmi_dev.2: Regulator stb_vreg_7 registered
> for domain [7]
> [    0.564817] scmi-regulator scmi_dev.2: Regulator stb_vreg_8 registered
> for domain [8]
> [    0.565030] scmi-regulator scmi_dev.2: Regulator stb_vreg_9 registered
> for domain [9]
> [    0.565191] scmi-regulator scmi_dev.2: Regulator stb_vreg_10 registered
> for domain [10]
> 

Ok, this was another place where a reply carrying a consistent number of
non-segmented entries could trigger an jumbo-request to kmalloc...

..maybe this is where a transient fw issue can reply with a dramatic
numbers of possible (non-segmented) levels (num_returned+num_remaining)

(I filter out replies carrying descriptors for segmented voltage-levels
that does not come in triplets (returned:3  remaining:0) since they are out
of spec...but I just hit today on another fw sending such out of spec
reply for clocks and I will relax such req probably not to break too
many out-of-spec blobs out in the wild :P)

So let me know if got new splats...

Thanks,
Cristian

Re: [PATCH 11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support

[Florian Fainelli]

On 6/15/22 10:32, Cristian Marussi wrote:
> 
> Ok, this was another place where a reply carrying a consistent number of
> non-segmented entries could trigger an jumbo-request to kmalloc...
> 
> ..maybe this is where a transient fw issue can reply with a dramatic
> numbers of possible (non-segmented) levels (num_returned+num_remaining)
> 
> (I filter out replies carrying descriptors for segmented voltage-levels
> that does not come in triplets (returned:3  remaining:0) since they are out
> of spec...but I just hit today on another fw sending such out of spec
> reply for clocks and I will relax such req probably not to break too
> many out-of-spec blobs out in the wild :P)
> 
> So let me know if got new splats...

This turned out to be a nice firmware bug, someone *cough* *cough* 
decided to add some extra "protection" using a temporary buffer and they 
completely forgot the newly added voltage domain service, so we would 
just return stale information, wonderful. This is now fixed.

Thanks a lot for your responsiveness and support.
-- 
Florian

[PATCH 12/22] firmware: arm_scmi: Parse clock_enable_latency conditionally

[Cristian Marussi]

The clock_enable_latency field in CLOCK_ATTRIBUTES response message has
been added only since SCMI v3.1: use the advertised SCMI Clock protocol
version as a proper condition check for parsing it, instead of the bare
message length lookup.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 6dd4150b761b..b46b43a99871 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -130,8 +130,8 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 	if (!ret) {
 		attributes = le32_to_cpu(attr->attributes);
 		strlcpy(clk->name, attr->name, SCMI_MAX_STR_SIZE);
-		/* Is optional field clock_enable_latency provided ? */
-		if (t->rx.len == sizeof(*attr))
+		/* clock_enable_latency field is present only since SCMI v3.1 */
+		if (PROTOCOL_REV_MAJOR(version) >= 0x2)
 			clk->enable_latency =
 				le32_to_cpu(attr->clock_enable_latency);
 	}
-- 
2.32.0

[PATCH 13/22] firmware: arm_scmi: Add iterators for multi-part commands

[Cristian Marussi]

SCMI specification defines some commands as optionally issued over multiple
messages in order to overcome possible limitations in payload size enforced
by the configured underlyinng transport.

Introduce some common protocol helpers to provide a unified solution for
issuing such SCMI multi-part commands.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/driver.c    | 109 ++++++++++++++++++++++++++
 drivers/firmware/arm_scmi/protocols.h |  54 +++++++++++++
 2 files changed, 163 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 44aa13f3cb76..454b8ff75100 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -1147,8 +1147,117 @@ static int scmi_common_extended_name_get(const struct scmi_protocol_handle *ph,
 	return ret;
 }
 
+/**
+ * struct scmi_iterator  - Iterator descriptor
+ * @msg: A reference to the message TX buffer; filled by @prepare_message with
+ *	 a proper custom command payload for each multi-part command request.
+ * @resp: A reference to the response RX buffer; used by @update_state and
+ *	  @process_response to parse the multi-part replies.
+ * @t: A reference to the underlying xfer initialized and used transparently by
+ *     the iterator internal routines.
+ * @ph: A reference to the associated protocol handle to be used.
+ * @ops: A reference to the custom provided iterator operations.
+ * @state: The current iterator state; used and updated in turn by the iterators
+ *	   internal routines and by the caller-provided @scmi_iterator_ops.
+ * @priv: A reference to optional private data as provided by the caller and
+ *	  passed back to the @@scmi_iterator_ops.
+ */
+struct scmi_iterator {
+	void *msg;
+	void *resp;
+	struct scmi_xfer *t;
+	const struct scmi_protocol_handle *ph;
+	struct scmi_iterator_ops *ops;
+	struct scmi_iterator_state state;
+	void *priv;
+};
+
+static void *scmi_iterator_init(const struct scmi_protocol_handle *ph,
+				struct scmi_iterator_ops *ops,
+				unsigned int max_resources, u8 msg_id,
+				size_t tx_size, void *priv)
+{
+	int ret;
+	struct scmi_iterator *i;
+
+	i = devm_kzalloc(ph->dev, sizeof(*i), GFP_KERNEL);
+	if (!i)
+		return ERR_PTR(-ENOMEM);
+
+	i->ph = ph;
+	i->ops = ops;
+	i->priv = priv;
+
+	ret = ph->xops->xfer_get_init(ph, msg_id, tx_size, 0, &i->t);
+	if (ret) {
+		devm_kfree(ph->dev, i);
+		return ERR_PTR(ret);
+	}
+
+	i->state.max_resources = max_resources;
+	i->msg = i->t->tx.buf;
+	i->resp = i->t->rx.buf;
+
+	return i;
+}
+
+static int scmi_iterator_run(void *iter)
+{
+	int ret = -EINVAL;
+	struct scmi_iterator *i = iter;
+	struct scmi_iterator_state *st = &i->state;
+	struct scmi_iterator_ops *iops = i->ops;
+	const struct scmi_protocol_handle *ph = i->ph;
+	const struct scmi_xfer_ops *xops = ph->xops;
+
+	if (!i)
+		return ret;
+
+	do {
+		iops->prepare_message(i->msg, st->desc_index, i->priv);
+		ret = xops->do_xfer(ph, i->t);
+		if (ret)
+			break;
+
+		ret = iops->update_state(st, i->resp, i->priv);
+		if (ret)
+			break;
+
+		if (st->num_returned > st->max_resources - st->desc_index) {
+			dev_err(ph->dev,
+				"No. of resources can't exceed %d\n",
+				st->max_resources);
+			ret = -EINVAL;
+			break;
+		}
+
+		for (st->loop_idx = 0; st->loop_idx < st->num_returned;
+		     st->loop_idx++) {
+			ret = iops->process_response(ph, i->resp, st, i->priv);
+			if (ret)
+				goto out;
+		}
+
+		st->desc_index += st->num_returned;
+		xops->reset_rx_to_maxsz(ph, i->t);
+		/*
+		 * check for both returned and remaining to avoid infinite
+		 * loop due to buggy firmware
+		 */
+	} while (st->num_returned && st->num_remaining);
+
+out:
+	/* Finalize and destroy iterator */
+	xops->xfer_put(ph, i->t);
+	devm_kfree(ph->dev, i);
+
+	return ret;
+}
+
 static const struct scmi_proto_helpers_ops helpers_ops = {
 	.extended_name_get = scmi_common_extended_name_get,
+	.iter_response_init = scmi_iterator_init,
+	.iter_response_run = scmi_iterator_run,
 };
 
 /**
diff --git a/drivers/firmware/arm_scmi/protocols.h b/drivers/firmware/arm_scmi/protocols.h
index 60ea880b3855..73304af5ec4a 100644
--- a/drivers/firmware/arm_scmi/protocols.h
+++ b/drivers/firmware/arm_scmi/protocols.h
@@ -173,6 +173,47 @@ struct scmi_protocol_handle {
 	void *(*get_priv)(const struct scmi_protocol_handle *ph);
 };
 
+/**
+ * struct scmi_iterator_state  - Iterator current state descriptor
+ * @desc_index: Starting index for the current mulit-part request.
+ * @num_returned: Number of returned items in the last multi-part reply.
+ * @num_remaining: Number of remaining items in the multi-part message.
+ * @max_resources: Maximum acceptable number of items, configured by the caller
+ *		   depending on the underlying resources that it is querying.
+ * @loop_idx: The iterator loop index in the current multi-part reply.
+ * @priv: Optional pointer to some additional state-related private data setup
+ *	  by the caller during the iterations.
+ */
+struct scmi_iterator_state {
+	unsigned int desc_index;
+	unsigned int num_returned;
+	unsigned int num_remaining;
+	unsigned int max_resources;
+	unsigned int loop_idx;
+	void *priv;
+};
+
+/**
+ * struct scmi_iterator_ops  - Custom iterator operations
+ * @prepare_message: An operation to provide the custom logic to fill in the
+ *		     SCMI command request pointed by @message. @desc_index is
+ *		     a reference to the next index to use in the multi-part
+ *		     request.
+ * @update_state: An operation to provide the custom logic to update the
+ *		  iterator state from the actual message response.
+ * @process_response: An operation to provide the custom logic needed to process
+ *		      each chunk of the multi-part message.
+ */
+struct scmi_iterator_ops {
+	void (*prepare_message)(void *message, unsigned int desc_index,
+				const void *priv);
+	int (*update_state)(struct scmi_iterator_state *st,
+			    const void *response, void *priv);
+	int (*process_response)(const struct scmi_protocol_handle *ph,
+				const void *response,
+				struct scmi_iterator_state *st, void *priv);
+};
+
 /**
  * struct scmi_proto_helpers_ops  - References to common protocol helpers
  * @extended_name_get: A common helper function to retrieve extended naming
@@ -180,10 +221,23 @@ struct scmi_protocol_handle {
  *		       Result is returned as a NULL terminated string in the
  *		       pre-allocated area pointed to by @name with maximum
  *		       capacity of @len bytes.
+ * @iter_response_init: A common helper to initialize a generic iterator to
+ *			parse multi-message responses: when run the iterator
+ *			will take care to send the initial command request as
+ *			specified by @msg_id and @tx_size and then to parse the
+ *			multi-part responses using the custom operations
+ *			provided in @ops.
+ * @iter_response_run: A common helper to trigger the run of a previously
+ *		       initialized iterator.
  */
 struct scmi_proto_helpers_ops {
 	int (*extended_name_get)(const struct scmi_protocol_handle *ph,
 				 u8 cmd_id, u32 res_id, char *name, size_t len);
+	void *(*iter_response_init)(const struct scmi_protocol_handle *ph,
+				    struct scmi_iterator_ops *ops,
+				    unsigned int max_resources, u8 msg_id,
+				    size_t tx_size, void *priv);
+	int (*iter_response_run)(void *iter);
 };
 
 /**
-- 
2.32.0

[PATCH 14/22] firmware: arm_scmi: Use common iterators in Sensor protocol

[Cristian Marussi]

Make SCMI Sensor protocol use the common iterator protocol helpers.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/sensors.c | 582 ++++++++++++++--------------
 1 file changed, 297 insertions(+), 285 deletions(-)

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 6fd8b3a874ea..e1a94463d7d8 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -64,6 +64,10 @@ struct scmi_msg_resp_attrs {
 	__le32 max_range_high;
 };
 
+struct scmi_msg_sensor_description {
+	__le32 desc_index;
+};
+
 struct scmi_msg_resp_sensor_description {
 	__le16 num_returned;
 	__le16 num_remaining;
@@ -233,346 +237,354 @@ static int scmi_sensor_attributes_get(const struct scmi_protocol_handle *ph,
 }
 
 static inline void scmi_parse_range_attrs(struct scmi_range_attrs *out,
-					  struct scmi_msg_resp_attrs *in)
+					  const struct scmi_msg_resp_attrs *in)
 {
 	out->min_range = get_unaligned_le64((void *)&in->min_range_low);
 	out->max_range = get_unaligned_le64((void *)&in->max_range_low);
 }
 
-static int scmi_sensor_update_intervals(const struct scmi_protocol_handle *ph,
-					struct scmi_sensor_info *s)
-{
-	int ret, cnt;
-	u32 desc_index = 0;
-	u16 num_returned, num_remaining;
-	struct scmi_xfer *ti;
-	struct scmi_msg_resp_sensor_list_update_intervals *buf;
-	struct scmi_msg_sensor_list_update_intervals *msg;
-
-	ret = ph->xops->xfer_get_init(ph, SENSOR_LIST_UPDATE_INTERVALS,
-				      sizeof(*msg), 0, &ti);
-	if (ret)
-		return ret;
-
-	buf = ti->rx.buf;
-	do {
-		u32 flags;
-
-		msg = ti->tx.buf;
-		/* Set the number of sensors to be skipped/already read */
-		msg->id = cpu_to_le32(s->id);
-		msg->index = cpu_to_le32(desc_index);
+struct scmi_sens_ipriv {
+	void *priv;
+	struct device *dev;
+};
 
-		ret = ph->xops->do_xfer(ph, ti);
-		if (ret)
-			break;
+static void iter_intervals_prepare_message(void *message,
+					   unsigned int desc_index,
+					   const void *p)
+{
+	struct scmi_msg_sensor_list_update_intervals *msg = message;
+	const struct scmi_sensor_info *s;
 
-		flags = le32_to_cpu(buf->num_intervals_flags);
-		num_returned = NUM_INTERVALS_RETURNED(flags);
-		num_remaining = NUM_INTERVALS_REMAINING(flags);
+	s = ((const struct scmi_sens_ipriv *)p)->priv;
+	/* Set the number of sensors to be skipped/already read */
+	msg->id = cpu_to_le32(s->id);
+	msg->index = cpu_to_le32(desc_index);
+}
 
-		/*
-		 * Max intervals is not declared previously anywhere so we
-		 * assume it's returned+remaining.
-		 */
-		if (!s->intervals.count) {
-			s->intervals.segmented = SEGMENTED_INTVL_FORMAT(flags);
-			s->intervals.count = num_returned + num_remaining;
-			/* segmented intervals are reported in one triplet */
-			if (s->intervals.segmented &&
-			    (num_remaining || num_returned != 3)) {
-				dev_err(ph->dev,
-					"Sensor ID:%d advertises an invalid segmented interval (%d)\n",
-					s->id, s->intervals.count);
+static int iter_intervals_update_state(struct scmi_iterator_state *st,
+				       const void *response, void *p)
+{
+	u32 flags;
+	struct scmi_sensor_info *s = ((struct scmi_sens_ipriv *)p)->priv;
+	struct device *dev = ((struct scmi_sens_ipriv *)p)->dev;
+	const struct scmi_msg_resp_sensor_list_update_intervals *r = response;
+
+	flags = le32_to_cpu(r->num_intervals_flags);
+	st->num_returned = NUM_INTERVALS_RETURNED(flags);
+	st->num_remaining = NUM_INTERVALS_REMAINING(flags);
+
+	/*
+	 * Max intervals is not declared previously anywhere so we
+	 * assume it's returned+remaining on first call.
+	 */
+	if (!st->max_resources) {
+		s->intervals.segmented = SEGMENTED_INTVL_FORMAT(flags);
+		s->intervals.count = st->num_returned + st->num_remaining;
+		/* segmented intervals are reported in one triplet */
+		if (s->intervals.segmented &&
+		    (st->num_remaining || st->num_returned != 3)) {
+			dev_err(dev,
+				"Sensor ID:%d advertises an invalid segmented interval (%d)\n",
+				s->id, s->intervals.count);
+			s->intervals.segmented = false;
+			s->intervals.count = 0;
+			return -EINVAL;
+		}
+		/* Direct allocation when exceeding pre-allocated */
+		if (s->intervals.count >= SCMI_MAX_PREALLOC_POOL) {
+			s->intervals.desc =
+				devm_kcalloc(dev,
+					     s->intervals.count,
+					     sizeof(*s->intervals.desc),
+					     GFP_KERNEL);
+			if (!s->intervals.desc) {
 				s->intervals.segmented = false;
 				s->intervals.count = 0;
-				ret = -EINVAL;
-				break;
-			}
-			/* Direct allocation when exceeding pre-allocated */
-			if (s->intervals.count >= SCMI_MAX_PREALLOC_POOL) {
-				s->intervals.desc =
-					devm_kcalloc(ph->dev,
-						     s->intervals.count,
-						     sizeof(*s->intervals.desc),
-						     GFP_KERNEL);
-				if (!s->intervals.desc) {
-					s->intervals.segmented = false;
-					s->intervals.count = 0;
-					ret = -ENOMEM;
-					break;
-				}
+				return -ENOMEM;
 			}
-		} else if (desc_index + num_returned > s->intervals.count) {
-			dev_err(ph->dev,
-				"No. of update intervals can't exceed %d\n",
-				s->intervals.count);
-			ret = -EINVAL;
-			break;
 		}
 
-		for (cnt = 0; cnt < num_returned; cnt++)
-			s->intervals.desc[desc_index + cnt] =
-					le32_to_cpu(buf->intervals[cnt]);
+		st->max_resources = s->intervals.count;
+	}
 
-		desc_index += num_returned;
+	return 0;
+}
 
-		ph->xops->reset_rx_to_maxsz(ph, ti);
-		/*
-		 * check for both returned and remaining to avoid infinite
-		 * loop due to buggy firmware
-		 */
-	} while (num_returned && num_remaining);
+static int
+iter_intervals_process_response(const struct scmi_protocol_handle *ph,
+				const void *response,
+				struct scmi_iterator_state *st, void *p)
+{
+	const struct scmi_msg_resp_sensor_list_update_intervals *r = response;
+	struct scmi_sensor_info *s = ((struct scmi_sens_ipriv *)p)->priv;
 
-	ph->xops->xfer_put(ph, ti);
-	return ret;
+	s->intervals.desc[st->desc_index + st->loop_idx] =
+		le32_to_cpu(r->intervals[st->loop_idx]);
+
+	return 0;
 }
 
-static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
+static int scmi_sensor_update_intervals(const struct scmi_protocol_handle *ph,
 					struct scmi_sensor_info *s)
 {
-	int ret, cnt;
-	u32 desc_index = 0;
-	u16 num_returned, num_remaining;
-	struct scmi_xfer *te;
-	struct scmi_msg_resp_sensor_axis_description *buf;
-	struct scmi_msg_sensor_axis_description_get *msg;
+	void *iter;
+	struct scmi_msg_sensor_list_update_intervals *msg;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_intervals_prepare_message,
+		.update_state = iter_intervals_update_state,
+		.process_response = iter_intervals_process_response,
+	};
+	struct scmi_sens_ipriv upriv = {
+		.priv = s,
+		.dev = ph->dev,
+	};
+
+	iter = ph->hops->iter_response_init(ph, &ops, s->intervals.count,
+					    SENSOR_LIST_UPDATE_INTERVALS,
+					    sizeof(*msg), &upriv);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	return ph->hops->iter_response_run(iter);
+}
 
-	s->axis = devm_kcalloc(ph->dev, s->num_axis,
-			       sizeof(*s->axis), GFP_KERNEL);
-	if (!s->axis)
-		return -ENOMEM;
+static void iter_axes_desc_prepare_message(void *message,
+					   const unsigned int desc_index,
+					   const void *priv)
+{
+	struct scmi_msg_sensor_axis_description_get *msg = message;
+	const struct scmi_sensor_info *s = priv;
 
-	ret = ph->xops->xfer_get_init(ph, SENSOR_AXIS_DESCRIPTION_GET,
-				      sizeof(*msg), 0, &te);
-	if (ret)
-		return ret;
+	/* Set the number of sensors to be skipped/already read */
+	msg->id = cpu_to_le32(s->id);
+	msg->axis_desc_index = cpu_to_le32(desc_index);
+}
 
-	buf = te->rx.buf;
-	do {
-		u32 flags;
-		struct scmi_axis_descriptor *adesc;
+static int
+iter_axes_desc_update_state(struct scmi_iterator_state *st,
+			    const void *response, void *priv)
+{
+	u32 flags;
+	const struct scmi_msg_resp_sensor_axis_description *r = response;
 
-		msg = te->tx.buf;
-		/* Set the number of sensors to be skipped/already read */
-		msg->id = cpu_to_le32(s->id);
-		msg->axis_desc_index = cpu_to_le32(desc_index);
+	flags = le32_to_cpu(r->num_axis_flags);
+	st->num_returned = NUM_AXIS_RETURNED(flags);
+	st->num_remaining = NUM_AXIS_REMAINING(flags);
+	st->priv = (void *)&r->desc[0];
 
-		ret = ph->xops->do_xfer(ph, te);
-		if (ret)
-			break;
+	return 0;
+}
 
-		flags = le32_to_cpu(buf->num_axis_flags);
-		num_returned = NUM_AXIS_RETURNED(flags);
-		num_remaining = NUM_AXIS_REMAINING(flags);
+static int
+iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
+				const void *response,
+				struct scmi_iterator_state *st, void *priv)
+{
+	u32 attrh, attrl;
+	struct scmi_sensor_axis_info *a;
+	size_t dsize = SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ;
+	struct scmi_sensor_info *s = priv;
+	const struct scmi_axis_descriptor *adesc = st->priv;
 
-		if (desc_index + num_returned > s->num_axis) {
-			dev_err(ph->dev, "No. of axis can't exceed %d\n",
-				s->num_axis);
-			break;
-		}
+	attrl = le32_to_cpu(adesc->attributes_low);
 
-		adesc = &buf->desc[0];
-		for (cnt = 0; cnt < num_returned; cnt++) {
-			u32 attrh, attrl;
-			struct scmi_sensor_axis_info *a;
-			size_t dsize = SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ;
+	a = &s->axis[st->desc_index + st->loop_idx];
+	a->id = le32_to_cpu(adesc->id);
+	a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
 
-			attrl = le32_to_cpu(adesc->attributes_low);
+	attrh = le32_to_cpu(adesc->attributes_high);
 
-			a = &s->axis[desc_index + cnt];
+	a->scale = S32_EXT(SENSOR_SCALE(attrh));
+	a->type = SENSOR_TYPE(attrh);
+	strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
 
-			a->id = le32_to_cpu(adesc->id);
-			a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
+	if (a->extended_attrs) {
+		unsigned int ares = le32_to_cpu(adesc->resolution);
 
-			attrh = le32_to_cpu(adesc->attributes_high);
-			a->scale = S32_EXT(SENSOR_SCALE(attrh));
-			a->type = SENSOR_TYPE(attrh);
-			strlcpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
+		a->resolution = SENSOR_RES(ares);
+		a->exponent = S32_EXT(SENSOR_RES_EXP(ares));
+		dsize += sizeof(adesc->resolution);
 
-			if (a->extended_attrs) {
-				unsigned int ares =
-					le32_to_cpu(adesc->resolution);
+		scmi_parse_range_attrs(&a->attrs, &adesc->attrs);
+		dsize += sizeof(adesc->attrs);
+	}
 
-				a->resolution = SENSOR_RES(ares);
-				a->exponent =
-					S32_EXT(SENSOR_RES_EXP(ares));
-				dsize += sizeof(adesc->resolution);
+	st->priv = ((u8 *)adesc + dsize);
 
-				scmi_parse_range_attrs(&a->attrs,
-						       &adesc->attrs);
-				dsize += sizeof(adesc->attrs);
-			}
+	return 0;
+}
 
-			adesc = (typeof(adesc))((u8 *)adesc + dsize);
-		}
+static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
+					struct scmi_sensor_info *s)
+{
+	void *iter;
+	struct scmi_msg_sensor_axis_description_get *msg;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_axes_desc_prepare_message,
+		.update_state = iter_axes_desc_update_state,
+		.process_response = iter_axes_desc_process_response,
+	};
 
-		desc_index += num_returned;
+	s->axis = devm_kcalloc(ph->dev, s->num_axis,
+			       sizeof(*s->axis), GFP_KERNEL);
+	if (!s->axis)
+		return -ENOMEM;
 
-		ph->xops->reset_rx_to_maxsz(ph, te);
-		/*
-		 * check for both returned and remaining to avoid infinite
-		 * loop due to buggy firmware
-		 */
-	} while (num_returned && num_remaining);
+	iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
+					    SENSOR_AXIS_DESCRIPTION_GET,
+					    sizeof(*msg), s);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
 
-	ph->xops->xfer_put(ph, te);
-	return ret;
+	return ph->hops->iter_response_run(iter);
 }
 
-static int scmi_sensor_description_get(const struct scmi_protocol_handle *ph,
-				       struct sensors_info *si)
+static void iter_sens_descr_prepare_message(void *message,
+					    unsigned int desc_index,
+					    const void *priv)
 {
-	int ret, cnt;
-	u32 desc_index = 0;
-	u16 num_returned, num_remaining;
-	struct scmi_xfer *t;
-	struct scmi_msg_resp_sensor_description *buf;
-
-	ret = ph->xops->xfer_get_init(ph, SENSOR_DESCRIPTION_GET,
-				      sizeof(__le32), 0, &t);
-	if (ret)
-		return ret;
-
-	buf = t->rx.buf;
-
-	do {
-		struct scmi_sensor_descriptor *sdesc;
+	struct scmi_msg_sensor_description *msg = message;
 
-		/* Set the number of sensors to be skipped/already read */
-		put_unaligned_le32(desc_index, t->tx.buf);
+	msg->desc_index = cpu_to_le32(desc_index);
+}
 
-		ret = ph->xops->do_xfer(ph, t);
-		if (ret)
-			break;
+static int iter_sens_descr_update_state(struct scmi_iterator_state *st,
+					const void *response, void *priv)
+{
+	const struct scmi_msg_resp_sensor_description *r = response;
 
-		num_returned = le16_to_cpu(buf->num_returned);
-		num_remaining = le16_to_cpu(buf->num_remaining);
+	st->num_returned = le16_to_cpu(r->num_returned);
+	st->num_remaining = le16_to_cpu(r->num_remaining);
+	st->priv = (void *)&r->desc[0];
 
-		if (desc_index + num_returned > si->num_sensors) {
-			dev_err(ph->dev, "No. of sensors can't exceed %d",
-				si->num_sensors);
-			break;
-		}
+	return 0;
+}
 
-		sdesc = &buf->desc[0];
-		for (cnt = 0; cnt < num_returned; cnt++) {
-			u32 attrh, attrl;
-			struct scmi_sensor_info *s;
-			size_t dsize = SCMI_MSG_RESP_SENS_DESCR_BASE_SZ;
-
-			s = &si->sensors[desc_index + cnt];
-			s->id = le32_to_cpu(sdesc->id);
-
-			attrl = le32_to_cpu(sdesc->attributes_low);
-			/* common bitfields parsing */
-			s->async = SUPPORTS_ASYNC_READ(attrl);
-			s->num_trip_points = NUM_TRIP_POINTS(attrl);
-			/**
-			 * only SCMIv3.0 specific bitfield below.
-			 * Such bitfields are assumed to be zeroed on non
-			 * relevant fw versions...assuming fw not buggy !
-			 */
-			s->update = SUPPORTS_UPDATE_NOTIFY(attrl);
-			s->timestamped = SUPPORTS_TIMESTAMP(attrl);
-			if (s->timestamped)
-				s->tstamp_scale =
-					S32_EXT(SENSOR_TSTAMP_EXP(attrl));
-			s->extended_scalar_attrs =
-				SUPPORTS_EXTEND_ATTRS(attrl);
-
-			attrh = le32_to_cpu(sdesc->attributes_high);
-			/* common bitfields parsing */
-			s->scale = S32_EXT(SENSOR_SCALE(attrh));
-			s->type = SENSOR_TYPE(attrh);
-			/* Use pre-allocated pool wherever possible */
-			s->intervals.desc = s->intervals.prealloc_pool;
-			if (si->version == SCMIv2_SENSOR_PROTOCOL) {
-				s->intervals.segmented = false;
-				s->intervals.count = 1;
-				/*
-				 * Convert SCMIv2.0 update interval format to
-				 * SCMIv3.0 to be used as the common exposed
-				 * descriptor, accessible via common macros.
-				 */
-				s->intervals.desc[0] =
-					(SENSOR_UPDATE_BASE(attrh) << 5) |
-					 SENSOR_UPDATE_SCALE(attrh);
-			} else {
-				/*
-				 * From SCMIv3.0 update intervals are retrieved
-				 * via a dedicated (optional) command.
-				 * Since the command is optional, on error carry
-				 * on without any update interval.
-				 */
-				if (scmi_sensor_update_intervals(ph, s))
-					dev_dbg(ph->dev,
-						"Update Intervals not available for sensor ID:%d\n",
-						s->id);
-			}
-			/**
-			 * only > SCMIv2.0 specific bitfield below.
-			 * Such bitfields are assumed to be zeroed on non
-			 * relevant fw versions...assuming fw not buggy !
-			 */
-			s->num_axis = min_t(unsigned int,
-					    SUPPORTS_AXIS(attrh) ?
-					    SENSOR_AXIS_NUMBER(attrh) : 0,
-					    SCMI_MAX_NUM_SENSOR_AXIS);
-			strlcpy(s->name, sdesc->name, SCMI_MAX_STR_SIZE);
-
-			/*
-			 * If supported overwrite short name with the extended
-			 * one; on error just carry on and use already provided
-			 * short name.
-			 */
-			if (PROTOCOL_REV_MAJOR(si->version) >= 0x3 &&
-			    SUPPORTS_EXTENDED_NAMES(attrl))
-				ph->hops->extended_name_get(ph, SENSOR_NAME_GET,
-							    s->id, s->name,
-							    SCMI_MAX_STR_SIZE);
-
-			if (s->extended_scalar_attrs) {
-				s->sensor_power = le32_to_cpu(sdesc->power);
-				dsize += sizeof(sdesc->power);
-				/* Only for sensors reporting scalar values */
-				if (s->num_axis == 0) {
-					unsigned int sres =
-						le32_to_cpu(sdesc->resolution);
-
-					s->resolution = SENSOR_RES(sres);
-					s->exponent =
-						S32_EXT(SENSOR_RES_EXP(sres));
-					dsize += sizeof(sdesc->resolution);
-
-					scmi_parse_range_attrs(&s->scalar_attrs,
-							       &sdesc->scalar_attrs);
-					dsize += sizeof(sdesc->scalar_attrs);
-				}
-			}
-			if (s->num_axis > 0) {
-				ret = scmi_sensor_axis_description(ph, s);
-				if (ret)
-					goto out;
-			}
+static int
+iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
+				 const void *response,
+				 struct scmi_iterator_state *st, void *priv)
 
-			sdesc = (typeof(sdesc))((u8 *)sdesc + dsize);
+{
+	int ret = 0;
+	u32 attrh, attrl;
+	size_t dsize = SCMI_MSG_RESP_SENS_DESCR_BASE_SZ;
+	struct scmi_sensor_info *s;
+	struct sensors_info *si = priv;
+	const struct scmi_sensor_descriptor *sdesc = st->priv;
+
+	s = &si->sensors[st->desc_index + st->loop_idx];
+	s->id = le32_to_cpu(sdesc->id);
+
+	attrl = le32_to_cpu(sdesc->attributes_low);
+	/* common bitfields parsing */
+	s->async = SUPPORTS_ASYNC_READ(attrl);
+	s->num_trip_points = NUM_TRIP_POINTS(attrl);
+	/**
+	 * only SCMIv3.0 specific bitfield below.
+	 * Such bitfields are assumed to be zeroed on non
+	 * relevant fw versions...assuming fw not buggy !
+	 */
+	s->update = SUPPORTS_UPDATE_NOTIFY(attrl);
+	s->timestamped = SUPPORTS_TIMESTAMP(attrl);
+	if (s->timestamped)
+		s->tstamp_scale = S32_EXT(SENSOR_TSTAMP_EXP(attrl));
+	s->extended_scalar_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
+
+	attrh = le32_to_cpu(sdesc->attributes_high);
+	/* common bitfields parsing */
+	s->scale = S32_EXT(SENSOR_SCALE(attrh));
+	s->type = SENSOR_TYPE(attrh);
+	/* Use pre-allocated pool wherever possible */
+	s->intervals.desc = s->intervals.prealloc_pool;
+	if (si->version == SCMIv2_SENSOR_PROTOCOL) {
+		s->intervals.segmented = false;
+		s->intervals.count = 1;
+		/*
+		 * Convert SCMIv2.0 update interval format to
+		 * SCMIv3.0 to be used as the common exposed
+		 * descriptor, accessible via common macros.
+		 */
+		s->intervals.desc[0] = (SENSOR_UPDATE_BASE(attrh) << 5) |
+					SENSOR_UPDATE_SCALE(attrh);
+	} else {
+		/*
+		 * From SCMIv3.0 update intervals are retrieved
+		 * via a dedicated (optional) command.
+		 * Since the command is optional, on error carry
+		 * on without any update interval.
+		 */
+		if (scmi_sensor_update_intervals(ph, s))
+			dev_dbg(ph->dev,
+				"Update Intervals not available for sensor ID:%d\n",
+				s->id);
+	}
+	/**
+	 * only > SCMIv2.0 specific bitfield below.
+	 * Such bitfields are assumed to be zeroed on non
+	 * relevant fw versions...assuming fw not buggy !
+	 */
+	s->num_axis = min_t(unsigned int,
+			    SUPPORTS_AXIS(attrh) ?
+			    SENSOR_AXIS_NUMBER(attrh) : 0,
+			    SCMI_MAX_NUM_SENSOR_AXIS);
+	strscpy(s->name, sdesc->name, SCMI_MAX_STR_SIZE);
+
+	/*
+	 * If supported overwrite short name with the extended
+	 * one; on error just carry on and use already provided
+	 * short name.
+	 */
+	if (PROTOCOL_REV_MAJOR(si->version) >= 0x3 &&
+	    SUPPORTS_EXTENDED_NAMES(attrl))
+		ph->hops->extended_name_get(ph, SENSOR_NAME_GET, s->id,
+					    s->name, SCMI_MAX_STR_SIZE);
+
+	if (s->extended_scalar_attrs) {
+		s->sensor_power = le32_to_cpu(sdesc->power);
+		dsize += sizeof(sdesc->power);
+
+		/* Only for sensors reporting scalar values */
+		if (s->num_axis == 0) {
+			unsigned int sres = le32_to_cpu(sdesc->resolution);
+
+			s->resolution = SENSOR_RES(sres);
+			s->exponent = S32_EXT(SENSOR_RES_EXP(sres));
+			dsize += sizeof(sdesc->resolution);
+
+			scmi_parse_range_attrs(&s->scalar_attrs,
+					       &sdesc->scalar_attrs);
+			dsize += sizeof(sdesc->scalar_attrs);
 		}
+	}
 
-		desc_index += num_returned;
+	if (s->num_axis > 0)
+		ret = scmi_sensor_axis_description(ph, s);
 
-		ph->xops->reset_rx_to_maxsz(ph, t);
-		/*
-		 * check for both returned and remaining to avoid infinite
-		 * loop due to buggy firmware
-		 */
-	} while (num_returned && num_remaining);
+	st->priv = ((u8 *)sdesc + dsize);
 
-out:
-	ph->xops->xfer_put(ph, t);
 	return ret;
 }
 
+static int scmi_sensor_description_get(const struct scmi_protocol_handle *ph,
+				       struct sensors_info *si)
+{
+	void *iter;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_sens_descr_prepare_message,
+		.update_state = iter_sens_descr_update_state,
+		.process_response = iter_sens_descr_process_response,
+	};
+
+	iter = ph->hops->iter_response_init(ph, &ops, si->num_sensors,
+					    SENSOR_DESCRIPTION_GET,
+					    sizeof(__le32), si);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	return ph->hops->iter_response_run(iter);
+}
+
 static inline int
 scmi_sensor_request_notify(const struct scmi_protocol_handle *ph, u32 sensor_id,
 			   u8 message_id, bool enable)
-- 
2.32.0

[PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support

[Cristian Marussi]

Add support for SCMIv3.1 SENSOR_AXIS_NAME_GET multi-part command using the
common iterator protocol helpers.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/sensors.c | 82 ++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index e1a94463d7d8..21e0ce89b153 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -28,6 +28,7 @@ enum scmi_sensor_protocol_cmd {
 	SENSOR_CONFIG_SET = 0xA,
 	SENSOR_CONTINUOUS_UPDATE_NOTIFY = 0xB,
 	SENSOR_NAME_GET = 0xC,
+	SENSOR_AXIS_NAME_GET = 0xD,
 };
 
 struct scmi_msg_resp_sensor_attributes {
@@ -117,13 +118,22 @@ struct scmi_msg_resp_sensor_axis_description {
 	struct scmi_axis_descriptor {
 		__le32 id;
 		__le32 attributes_low;
+#define SUPPORTS_EXTENDED_AXIS_NAMES(x)	FIELD_GET(BIT(9), (x))
 		__le32 attributes_high;
-		u8 name[SCMI_MAX_STR_SIZE];
+		u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 		__le32 resolution;
 		struct scmi_msg_resp_attrs attrs;
 	} desc[];
 };
 
+struct scmi_msg_resp_sensor_axis_names_description {
+	__le32 num_axis_flags;
+	struct scmi_sensor_axis_name_descriptor {
+		__le32 axis_id;
+		u8 name[SCMI_MAX_STR_SIZE];
+	} desc[];
+};
+
 /* Base scmi_axis_descriptor size excluding extended attrs after name */
 #define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ	28
 
@@ -393,7 +403,6 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
 	a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
 
 	attrh = le32_to_cpu(adesc->attributes_high);
-
 	a->scale = S32_EXT(SENSOR_SCALE(attrh));
 	a->type = SENSOR_TYPE(attrh);
 	strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
@@ -408,15 +417,69 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
 		scmi_parse_range_attrs(&a->attrs, &adesc->attrs);
 		dsize += sizeof(adesc->attrs);
 	}
-
 	st->priv = ((u8 *)adesc + dsize);
 
 	return 0;
 }
 
+static int
+iter_axes_extended_name_update_state(struct scmi_iterator_state *st,
+				     const void *response, void *priv)
+{
+	u32 flags;
+	const struct scmi_msg_resp_sensor_axis_names_description *r = response;
+
+	flags = le32_to_cpu(r->num_axis_flags);
+	st->num_returned = NUM_AXIS_RETURNED(flags);
+	st->num_remaining = NUM_AXIS_REMAINING(flags);
+	st->priv = (void *)&r->desc[0];
+
+	return 0;
+}
+
+static int
+iter_axes_extended_name_process_response(const struct scmi_protocol_handle *ph,
+					 const void *response,
+					 struct scmi_iterator_state *st,
+					 void *priv)
+{
+	struct scmi_sensor_axis_info *a;
+	const struct scmi_sensor_info *s = priv;
+	struct scmi_sensor_axis_name_descriptor *adesc = st->priv;
+
+	a = &s->axis[st->desc_index + st->loop_idx];
+	strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
+	st->priv = ++adesc;
+
+	return 0;
+}
+
+static int
+scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
+				    struct scmi_sensor_info *s)
+{
+	void *iter;
+	struct scmi_msg_sensor_axis_description_get *msg;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_axes_desc_prepare_message,
+		.update_state = iter_axes_extended_name_update_state,
+		.process_response = iter_axes_extended_name_process_response,
+	};
+
+	iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
+					    SENSOR_AXIS_NAME_GET,
+					    sizeof(*msg), s);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	return ph->hops->iter_response_run(iter);
+}
+
 static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
-					struct scmi_sensor_info *s)
+					struct scmi_sensor_info *s,
+					u32 version)
 {
+	int ret;
 	void *iter;
 	struct scmi_msg_sensor_axis_description_get *msg;
 	struct scmi_iterator_ops ops = {
@@ -436,7 +499,14 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
 	if (IS_ERR(iter))
 		return PTR_ERR(iter);
 
-	return ph->hops->iter_response_run(iter);
+	ret = ph->hops->iter_response_run(iter);
+	if (ret)
+		return ret;
+
+	if (PROTOCOL_REV_MAJOR(version) >= 0x3)
+		ret = scmi_sensor_axis_extended_names_get(ph, s);
+
+	return ret;
 }
 
 static void iter_sens_descr_prepare_message(void *message,
@@ -559,7 +629,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
 	}
 
 	if (s->num_axis > 0)
-		ret = scmi_sensor_axis_description(ph, s);
+		ret = scmi_sensor_axis_description(ph, s, si->version);
 
 	st->priv = ((u8 *)sdesc + dsize);
 
-- 
2.32.0

Re: [PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support

[Peter Hilber]

On 30.03.22 17:05, Cristian Marussi wrote:
> Add support for SCMIv3.1 SENSOR_AXIS_NAME_GET multi-part command using the
> common iterator protocol helpers.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/sensors.c | 82 ++++++++++++++++++++++++++---
>  1 file changed, 76 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index e1a94463d7d8..21e0ce89b153 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -28,6 +28,7 @@ enum scmi_sensor_protocol_cmd {
>  	SENSOR_CONFIG_SET = 0xA,
>  	SENSOR_CONTINUOUS_UPDATE_NOTIFY = 0xB,
>  	SENSOR_NAME_GET = 0xC,
> +	SENSOR_AXIS_NAME_GET = 0xD,
>  };
>  
>  struct scmi_msg_resp_sensor_attributes {
> @@ -117,13 +118,22 @@ struct scmi_msg_resp_sensor_axis_description {
>  	struct scmi_axis_descriptor {
>  		__le32 id;
>  		__le32 attributes_low;
> +#define SUPPORTS_EXTENDED_AXIS_NAMES(x)	FIELD_GET(BIT(9), (x))

Hi Cristian,

I saw this patch is probably going into v5.19 already, so I'm a bit late, but I
wanted to point out a compatibility issue, and a small error handling issue.

Please see below.

Best regards,

Peter

>  		__le32 attributes_high;
> -		u8 name[SCMI_MAX_STR_SIZE];
> +		u8 name[SCMI_SHORT_NAME_MAX_SIZE];
>  		__le32 resolution;
>  		struct scmi_msg_resp_attrs attrs;
>  	} desc[];
>  };
>  
> +struct scmi_msg_resp_sensor_axis_names_description {
> +	__le32 num_axis_flags;
> +	struct scmi_sensor_axis_name_descriptor {
> +		__le32 axis_id;
> +		u8 name[SCMI_MAX_STR_SIZE];
> +	} desc[];
> +};
> +
>  /* Base scmi_axis_descriptor size excluding extended attrs after name */
>  #define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ	28
>  
> @@ -393,7 +403,6 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
>  	a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
>  
>  	attrh = le32_to_cpu(adesc->attributes_high);
> -
>  	a->scale = S32_EXT(SENSOR_SCALE(attrh));
>  	a->type = SENSOR_TYPE(attrh);
>  	strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);

The strscpy() call should probably change the size parameter to
SCMI_SHORT_NAME_MAX_SIZE.

> @@ -408,15 +417,69 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
>  		scmi_parse_range_attrs(&a->attrs, &adesc->attrs);
>  		dsize += sizeof(adesc->attrs);
>  	}
> -
>  	st->priv = ((u8 *)adesc + dsize);
>  
>  	return 0;
>  }
>  
> +static int
> +iter_axes_extended_name_update_state(struct scmi_iterator_state *st,
> +				     const void *response, void *priv)
> +{
> +	u32 flags;
> +	const struct scmi_msg_resp_sensor_axis_names_description *r = response;
> +
> +	flags = le32_to_cpu(r->num_axis_flags);
> +	st->num_returned = NUM_AXIS_RETURNED(flags);
> +	st->num_remaining = NUM_AXIS_REMAINING(flags);
> +	st->priv = (void *)&r->desc[0];
> +
> +	return 0;
> +}
> +
> +static int
> +iter_axes_extended_name_process_response(const struct scmi_protocol_handle *ph,
> +					 const void *response,
> +					 struct scmi_iterator_state *st,
> +					 void *priv)
> +{
> +	struct scmi_sensor_axis_info *a;
> +	const struct scmi_sensor_info *s = priv;
> +	struct scmi_sensor_axis_name_descriptor *adesc = st->priv;
> +
> +	a = &s->axis[st->desc_index + st->loop_idx];
> +	strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
> +	st->priv = ++adesc;
> +
> +	return 0;
> +}
> +
> +static int
> +scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
> +				    struct scmi_sensor_info *s)
> +{
> +	void *iter;
> +	struct scmi_msg_sensor_axis_description_get *msg;
> +	struct scmi_iterator_ops ops = {
> +		.prepare_message = iter_axes_desc_prepare_message,
> +		.update_state = iter_axes_extended_name_update_state,
> +		.process_response = iter_axes_extended_name_process_response,
> +	};
> +
> +	iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
> +					    SENSOR_AXIS_NAME_GET,
> +					    sizeof(*msg), s);
> +	if (IS_ERR(iter))
> +		return PTR_ERR(iter);
> +
> +	return ph->hops->iter_response_run(iter);
> +}
> +
>  static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> -					struct scmi_sensor_info *s)
> +					struct scmi_sensor_info *s,
> +					u32 version)
>  {
> +	int ret;
>  	void *iter;
>  	struct scmi_msg_sensor_axis_description_get *msg;
>  	struct scmi_iterator_ops ops = {
> @@ -436,7 +499,14 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
>  	if (IS_ERR(iter))
>  		return PTR_ERR(iter);
>  
> -	return ph->hops->iter_response_run(iter);
> +	ret = ph->hops->iter_response_run(iter);
> +	if (ret)
> +		return ret;
> +
> +	if (PROTOCOL_REV_MAJOR(version) >= 0x3)
> +		ret = scmi_sensor_axis_extended_names_get(ph, s);

From the SCMI v3.1 spec, I understood that the reading of the extended axis
name should be conditional on the bit checked by SUPPORTS_EXTENDED_AXIS_NAMES()
(the `Extended axis name' bit). Yet, the implementation doesn't use the macro,
and instead decides whether to issue SENSOR_AXIS_NAME_GET depending on the
(sensor management) protocol version being at least v3.0. But, per the spec, it
would be permissible for a v3.0 protocol to not support SENSOR_AXIS_NAME_GET at
all. Is my understanding correct?

> +
> +	return ret;
>  }
>  
>  static void iter_sens_descr_prepare_message(void *message,
> @@ -559,7 +629,7 @@ iter_sens_descr_process_response(const struct scmi_protocol_handle *ph,
>  	}
>  
>  	if (s->num_axis > 0)
> -		ret = scmi_sensor_axis_description(ph, s);
> +		ret = scmi_sensor_axis_description(ph, s, si->version);
>  
>  	st->priv = ((u8 *)sdesc + dsize);
>  

Re: [PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support

[Cristian Marussi]

On Thu, Jun 02, 2022 at 04:25:45PM +0200, Peter Hilber wrote:
> On 30.03.22 17:05, Cristian Marussi wrote:
> > Add support for SCMIv3.1 SENSOR_AXIS_NAME_GET multi-part command using the
> > common iterator protocol helpers.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/sensors.c | 82 ++++++++++++++++++++++++++---
> >  1 file changed, 76 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> > index e1a94463d7d8..21e0ce89b153 100644
> > --- a/drivers/firmware/arm_scmi/sensors.c
> > +++ b/drivers/firmware/arm_scmi/sensors.c
> > @@ -28,6 +28,7 @@ enum scmi_sensor_protocol_cmd {
> >  	SENSOR_CONFIG_SET = 0xA,
> >  	SENSOR_CONTINUOUS_UPDATE_NOTIFY = 0xB,
> >  	SENSOR_NAME_GET = 0xC,
> > +	SENSOR_AXIS_NAME_GET = 0xD,
> >  };
> >  
> >  struct scmi_msg_resp_sensor_attributes {
> > @@ -117,13 +118,22 @@ struct scmi_msg_resp_sensor_axis_description {
> >  	struct scmi_axis_descriptor {
> >  		__le32 id;
> >  		__le32 attributes_low;
> > +#define SUPPORTS_EXTENDED_AXIS_NAMES(x)	FIELD_GET(BIT(9), (x))
> 
> Hi Cristian,
> 
> I saw this patch is probably going into v5.19 already, so I'm a bit late, but I
> wanted to point out a compatibility issue, and a small error handling issue.
> 
> Please see below.
> 

Hi Peter,

thanks for having a look, your feedback is always appreciated.

Plese see my answers inline.

> Best regards,
> 
> Peter
> 
> >  		__le32 attributes_high;
> > -		u8 name[SCMI_MAX_STR_SIZE];
> > +		u8 name[SCMI_SHORT_NAME_MAX_SIZE];
> >  		__le32 resolution;
> >  		struct scmi_msg_resp_attrs attrs;
> >  	} desc[];
> >  };
> >  
> > +struct scmi_msg_resp_sensor_axis_names_description {
> > +	__le32 num_axis_flags;
> > +	struct scmi_sensor_axis_name_descriptor {
> > +		__le32 axis_id;
> > +		u8 name[SCMI_MAX_STR_SIZE];
> > +	} desc[];
> > +};
> > +
> >  /* Base scmi_axis_descriptor size excluding extended attrs after name */
> >  #define SCMI_MSG_RESP_AXIS_DESCR_BASE_SZ	28
> >  
> > @@ -393,7 +403,6 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
> >  	a->extended_attrs = SUPPORTS_EXTEND_ATTRS(attrl);
> >  
> >  	attrh = le32_to_cpu(adesc->attributes_high);
> > -
> >  	a->scale = S32_EXT(SENSOR_SCALE(attrh));
> >  	a->type = SENSOR_TYPE(attrh);
> >  	strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
> 
> The strscpy() call should probably change the size parameter to
> SCMI_SHORT_NAME_MAX_SIZE.
> 

I disagree on this, the strscpy size param is meant to represent the max
size of the dest buffer and here the destination buffer 'a->name' is the
string buffer publicly exposed by include/linux/scmi_protocol.h:scmi_sensor_axis_info
whose size is SCMI_MAX_STR_SIZE.

> > @@ -408,15 +417,69 @@ iter_axes_desc_process_response(const struct scmi_protocol_handle *ph,
> >  		scmi_parse_range_attrs(&a->attrs, &adesc->attrs);
> >  		dsize += sizeof(adesc->attrs);
> >  	}
> > -
> >  	st->priv = ((u8 *)adesc + dsize);
> >  
> >  	return 0;
> >  }
> >  
> > +static int
> > +iter_axes_extended_name_update_state(struct scmi_iterator_state *st,
> > +				     const void *response, void *priv)
> > +{
> > +	u32 flags;
> > +	const struct scmi_msg_resp_sensor_axis_names_description *r = response;
> > +
> > +	flags = le32_to_cpu(r->num_axis_flags);
> > +	st->num_returned = NUM_AXIS_RETURNED(flags);
> > +	st->num_remaining = NUM_AXIS_REMAINING(flags);
> > +	st->priv = (void *)&r->desc[0];
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +iter_axes_extended_name_process_response(const struct scmi_protocol_handle *ph,
> > +					 const void *response,
> > +					 struct scmi_iterator_state *st,
> > +					 void *priv)
> > +{
> > +	struct scmi_sensor_axis_info *a;
> > +	const struct scmi_sensor_info *s = priv;
> > +	struct scmi_sensor_axis_name_descriptor *adesc = st->priv;
> > +
> > +	a = &s->axis[st->desc_index + st->loop_idx];
> > +	strscpy(a->name, adesc->name, SCMI_MAX_STR_SIZE);
> > +	st->priv = ++adesc;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
> > +				    struct scmi_sensor_info *s)
> > +{
> > +	void *iter;
> > +	struct scmi_msg_sensor_axis_description_get *msg;
> > +	struct scmi_iterator_ops ops = {
> > +		.prepare_message = iter_axes_desc_prepare_message,
> > +		.update_state = iter_axes_extended_name_update_state,
> > +		.process_response = iter_axes_extended_name_process_response,
> > +	};
> > +
> > +	iter = ph->hops->iter_response_init(ph, &ops, s->num_axis,
> > +					    SENSOR_AXIS_NAME_GET,
> > +					    sizeof(*msg), s);
> > +	if (IS_ERR(iter))
> > +		return PTR_ERR(iter);
> > +
> > +	return ph->hops->iter_response_run(iter);
> > +}
> > +
> >  static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> > -					struct scmi_sensor_info *s)
> > +					struct scmi_sensor_info *s,
> > +					u32 version)
> >  {
> > +	int ret;
> >  	void *iter;
> >  	struct scmi_msg_sensor_axis_description_get *msg;
> >  	struct scmi_iterator_ops ops = {
> > @@ -436,7 +499,14 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> >  	if (IS_ERR(iter))
> >  		return PTR_ERR(iter);
> >  
> > -	return ph->hops->iter_response_run(iter);
> > +	ret = ph->hops->iter_response_run(iter);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (PROTOCOL_REV_MAJOR(version) >= 0x3)
> > +		ret = scmi_sensor_axis_extended_names_get(ph, s);
> 
> From the SCMI v3.1 spec, I understood that the reading of the extended axis
> name should be conditional on the bit checked by SUPPORTS_EXTENDED_AXIS_NAMES()
> (the `Extended axis name' bit). Yet, the implementation doesn't use the macro,
> and instead decides whether to issue SENSOR_AXIS_NAME_GET depending on the
> (sensor management) protocol version being at least v3.0. But, per the spec, it
> would be permissible for a v3.0 protocol to not support SENSOR_AXIS_NAME_GET at
> all. Is my understanding correct?
> 

Yes, indeed this behaviour was deliberate so as to keep this code
simpler while addressing some tricky definitions in the spec.
(not so short explanation follows :P)

SENSOR_AXIS_DESCRIPTION_GET is a command that, issued against a specific
sensor, return a list of axes descriptors for that sensor and such
descriptors in turn also include the flag you're mentioning that states
if a specific ax does support an extended name or not that will have to
be fetched with SENSOR_AXIS_GET_NAME.

BUT the SENSOR_AXIS_GET_NAME command is a multi-part command issued
against a specific sensor to retrieve the list of all the axes extended
names for that sensor, NOT to retrieve a single ax extended name, so I
cannot really check each ax extended name support before issuing the
commmand and, even though weird, the axes could have different support
with some of them supporting the extended name while some other don't:
as a consequence my take about this was that the platform would reply
anyway but only with the list of axes having an extended name (possibly
a subset of all the axes).

What could be missing in this context it's the handling of the case in
which all axes does NOT support extended names where probably the platform
won't even answer my request. (unsupported even if PROTO > 3.0)

Moreover even tracking this per-ax support while iterating the replies
would have made more complex some of the logic with anyway at the same
time hitting all the limitations explained above.

In this context, it seemed to me simpler (and a good trade-off) to issue
anyway the command while checking only for the protocol version and
accepting thatSENSOR_AXIS_GET_NAME could fail because unsupported
by all the axes, with the result of leaving the ax->name string content
filled with the short name previously retrieved.

Assuming that my blabbing above is acceptable, what IS indeed wrong
(reviewig this patch) is that the any 'acceptable' failure as depicted
above is not properly ignored in fact. I'll post a fix on top like:

--->8----
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index 50502c530b2f..788b566f634b 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -472,7 +472,9 @@ scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
        if (IS_ERR(iter))
                return PTR_ERR(iter);
 
-       return ph->hops->iter_response_run(iter);
+       ph->hops->iter_response_run(iter);
+
+       return 0;
 }
 
 static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
----

Moreover even the parsing logic for the SENSOR_AXIS_GET_NAME command has to
be sligthly reviewed to address the fact that the list of returned axes
extended names is incomplete so the returned axes won't necessarily be
returned in order (i.e. I'll have to check 'axis_d' in the SENSOR_AXIS_NAME_GET
replies to look up the proper ax descriptor.).
I'll post this as a distinct fix.

Does all of this make sense/seems reasonable ?

Thanks for the review again,
Cristian

Re: [PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support

[Peter Hilber]

On 06.06.22 10:18, Cristian Marussi wrote:
> On Thu, Jun 02, 2022 at 04:25:45PM +0200, Peter Hilber wrote:
>> On 30.03.22 17:05, Cristian Marussi wrote:
>>> Add support for SCMIv3.1 SENSOR_AXIS_NAME_GET multi-part command using the
>>> common iterator protocol helpers.
>>>
>>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>>> ---
>>>  drivers/firmware/arm_scmi/sensors.c | 82 ++++++++++++++++++++++++++---
>>>  1 file changed, 76 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
>>> index e1a94463d7d8..21e0ce89b153 100644
>>> --- a/drivers/firmware/arm_scmi/sensors.c
>>> +++ b/drivers/firmware/arm_scmi/sensors.c
>>> @@ -28,6 +28,7 @@ enum scmi_sensor_protocol_cmd {
>>>  	SENSOR_CONFIG_SET = 0xA,
>>>  	SENSOR_CONTINUOUS_UPDATE_NOTIFY = 0xB,
>>>  	SENSOR_NAME_GET = 0xC,
>>> +	SENSOR_AXIS_NAME_GET = 0xD,
>>>  };
>>>  
>>>  struct scmi_msg_resp_sensor_attributes {
>>> @@ -117,13 +118,22 @@ struct scmi_msg_resp_sensor_axis_description {
>>>  	struct scmi_axis_descriptor {
>>>  		__le32 id;
>>>  		__le32 attributes_low;
>>> +#define SUPPORTS_EXTENDED_AXIS_NAMES(x)	FIELD_GET(BIT(9), (x))
>>
>> Hi Cristian,
>>
>> I saw this patch is probably going into v5.19 already, so I'm a bit late, but I
>> wanted to point out a compatibility issue, and a small error handling issue.
>>
>> Please see below.
>>
> 
> Hi Peter,
> 
> thanks for having a look, your feedback is always appreciated.
> 
> Plese see my answers inline.
> 
[snip]
>>>  static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
>>> -					struct scmi_sensor_info *s)
>>> +					struct scmi_sensor_info *s,
>>> +					u32 version)
>>>  {
>>> +	int ret;
>>>  	void *iter;
>>>  	struct scmi_msg_sensor_axis_description_get *msg;
>>>  	struct scmi_iterator_ops ops = {
>>> @@ -436,7 +499,14 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
>>>  	if (IS_ERR(iter))
>>>  		return PTR_ERR(iter);
>>>  
>>> -	return ph->hops->iter_response_run(iter);
>>> +	ret = ph->hops->iter_response_run(iter);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (PROTOCOL_REV_MAJOR(version) >= 0x3)
>>> +		ret = scmi_sensor_axis_extended_names_get(ph, s);
>>
>> From the SCMI v3.1 spec, I understood that the reading of the extended axis
>> name should be conditional on the bit checked by SUPPORTS_EXTENDED_AXIS_NAMES()
>> (the `Extended axis name' bit). Yet, the implementation doesn't use the macro,
>> and instead decides whether to issue SENSOR_AXIS_NAME_GET depending on the
>> (sensor management) protocol version being at least v3.0. But, per the spec, it
>> would be permissible for a v3.0 protocol to not support SENSOR_AXIS_NAME_GET at
>> all. Is my understanding correct?
>>
> 
> Yes, indeed this behaviour was deliberate so as to keep this code
> simpler while addressing some tricky definitions in the spec.
> (not so short explanation follows :P)
> 
> SENSOR_AXIS_DESCRIPTION_GET is a command that, issued against a specific
> sensor, return a list of axes descriptors for that sensor and such
> descriptors in turn also include the flag you're mentioning that states
> if a specific ax does support an extended name or not that will have to
> be fetched with SENSOR_AXIS_GET_NAME.
> 
> BUT the SENSOR_AXIS_GET_NAME command is a multi-part command issued
> against a specific sensor to retrieve the list of all the axes extended
> names for that sensor, NOT to retrieve a single ax extended name, so I
> cannot really check each ax extended name support before issuing the
> commmand and, even though weird, the axes could have different support
> with some of them supporting the extended name while some other don't:
> as a consequence my take about this was that the platform would reply
> anyway but only with the list of axes having an extended name (possibly
> a subset of all the axes).
> 
> What could be missing in this context it's the handling of the case in
> which all axes does NOT support extended names where probably the platform
> won't even answer my request. (unsupported even if PROTO > 3.0)
> 
> Moreover even tracking this per-ax support while iterating the replies
> would have made more complex some of the logic with anyway at the same
> time hitting all the limitations explained above.
> 
> In this context, it seemed to me simpler (and a good trade-off) to issue
> anyway the command while checking only for the protocol version and
> accepting thatSENSOR_AXIS_GET_NAME could fail because unsupported
> by all the axes, with the result of leaving the ax->name string content
> filled with the short name previously retrieved.
> 
> Assuming that my blabbing above is acceptable, what IS indeed wrong
> (reviewig this patch) is that the any 'acceptable' failure as depicted
> above is not properly ignored in fact. I'll post a fix on top like:
> 
> --->8----
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index 50502c530b2f..788b566f634b 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -472,7 +472,9 @@ scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
>         if (IS_ERR(iter))
>                 return PTR_ERR(iter);
>  
> -       return ph->hops->iter_response_run(iter);
> +       ph->hops->iter_response_run(iter);
> +
> +       return 0;
>  }
>  
>  static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> ----
> 
> Moreover even the parsing logic for the SENSOR_AXIS_GET_NAME command has to
> be sligthly reviewed to address the fact that the list of returned axes
> extended names is incomplete so the returned axes won't necessarily be
> returned in order (i.e. I'll have to check 'axis_d' in the SENSOR_AXIS_NAME_GET
> replies to look up the proper ax descriptor.).
> I'll post this as a distinct fix.
> 
> Does all of this make sense/seems reasonable ?
> 
> Thanks for the review again,
> Cristian
> 

Hi Cristian,

thanks for your quick reply, this does all make sense to me.

Best regards,

Peter

Re: [PATCH 15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support

[Cristian Marussi]

On Wed, Jun 08, 2022 at 10:40:30AM +0200, Peter Hilber wrote:
> On 06.06.22 10:18, Cristian Marussi wrote:
> > On Thu, Jun 02, 2022 at 04:25:45PM +0200, Peter Hilber wrote:
> >> On 30.03.22 17:05, Cristian Marussi wrote:
> >>> Add support for SCMIv3.1 SENSOR_AXIS_NAME_GET multi-part command using the
> >>> common iterator protocol helpers.
> >>>
> >>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >>> ---
> >>>  drivers/firmware/arm_scmi/sensors.c | 82 ++++++++++++++++++++++++++---
> >>>  1 file changed, 76 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> >>> index e1a94463d7d8..21e0ce89b153 100644
> >>> --- a/drivers/firmware/arm_scmi/sensors.c
> >>> +++ b/drivers/firmware/arm_scmi/sensors.c
> >>> @@ -28,6 +28,7 @@ enum scmi_sensor_protocol_cmd {
> >>>  	SENSOR_CONFIG_SET = 0xA,
> >>>  	SENSOR_CONTINUOUS_UPDATE_NOTIFY = 0xB,
> >>>  	SENSOR_NAME_GET = 0xC,
> >>> +	SENSOR_AXIS_NAME_GET = 0xD,
> >>>  };
> >>>  
> >>>  struct scmi_msg_resp_sensor_attributes {
> >>> @@ -117,13 +118,22 @@ struct scmi_msg_resp_sensor_axis_description {
> >>>  	struct scmi_axis_descriptor {
> >>>  		__le32 id;
> >>>  		__le32 attributes_low;
> >>> +#define SUPPORTS_EXTENDED_AXIS_NAMES(x)	FIELD_GET(BIT(9), (x))
> >>
> >> Hi Cristian,
> >>

Hi Peter,

> >> I saw this patch is probably going into v5.19 already, so I'm a bit late, but I
> >> wanted to point out a compatibility issue, and a small error handling issue.
> >>
> >> Please see below.
> >>
> > 
> > Hi Peter,
> > 
> > thanks for having a look, your feedback is always appreciated.
> > 
> > Plese see my answers inline.
> > 
> [snip]
> >>>  static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> >>> -					struct scmi_sensor_info *s)
> >>> +					struct scmi_sensor_info *s,
> >>> +					u32 version)
> >>>  {
> >>> +	int ret;
> >>>  	void *iter;
> >>>  	struct scmi_msg_sensor_axis_description_get *msg;
> >>>  	struct scmi_iterator_ops ops = {
> >>> @@ -436,7 +499,14 @@ static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> >>>  	if (IS_ERR(iter))
> >>>  		return PTR_ERR(iter);
> >>>  
> >>> -	return ph->hops->iter_response_run(iter);
> >>> +	ret = ph->hops->iter_response_run(iter);
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	if (PROTOCOL_REV_MAJOR(version) >= 0x3)
> >>> +		ret = scmi_sensor_axis_extended_names_get(ph, s);
> >>
> >> From the SCMI v3.1 spec, I understood that the reading of the extended axis
> >> name should be conditional on the bit checked by SUPPORTS_EXTENDED_AXIS_NAMES()
> >> (the `Extended axis name' bit). Yet, the implementation doesn't use the macro,
> >> and instead decides whether to issue SENSOR_AXIS_NAME_GET depending on the
> >> (sensor management) protocol version being at least v3.0. But, per the spec, it
> >> would be permissible for a v3.0 protocol to not support SENSOR_AXIS_NAME_GET at
> >> all. Is my understanding correct?
> >>
> > 
> > Yes, indeed this behaviour was deliberate so as to keep this code
> > simpler while addressing some tricky definitions in the spec.
> > (not so short explanation follows :P)
> > 
> > SENSOR_AXIS_DESCRIPTION_GET is a command that, issued against a specific
> > sensor, return a list of axes descriptors for that sensor and such
> > descriptors in turn also include the flag you're mentioning that states
> > if a specific ax does support an extended name or not that will have to
> > be fetched with SENSOR_AXIS_GET_NAME.
> > 
> > BUT the SENSOR_AXIS_GET_NAME command is a multi-part command issued
> > against a specific sensor to retrieve the list of all the axes extended
> > names for that sensor, NOT to retrieve a single ax extended name, so I
> > cannot really check each ax extended name support before issuing the
> > commmand and, even though weird, the axes could have different support
> > with some of them supporting the extended name while some other don't:
> > as a consequence my take about this was that the platform would reply
> > anyway but only with the list of axes having an extended name (possibly
> > a subset of all the axes).
> > 
> > What could be missing in this context it's the handling of the case in
> > which all axes does NOT support extended names where probably the platform
> > won't even answer my request. (unsupported even if PROTO > 3.0)
> > 
> > Moreover even tracking this per-ax support while iterating the replies
> > would have made more complex some of the logic with anyway at the same
> > time hitting all the limitations explained above.
> > 
> > In this context, it seemed to me simpler (and a good trade-off) to issue
> > anyway the command while checking only for the protocol version and
> > accepting thatSENSOR_AXIS_GET_NAME could fail because unsupported
> > by all the axes, with the result of leaving the ax->name string content
> > filled with the short name previously retrieved.
> > 
> > Assuming that my blabbing above is acceptable, what IS indeed wrong
> > (reviewig this patch) is that the any 'acceptable' failure as depicted
> > above is not properly ignored in fact. I'll post a fix on top like:
> > 
> > --->8----
> > diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> > index 50502c530b2f..788b566f634b 100644
> > --- a/drivers/firmware/arm_scmi/sensors.c
> > +++ b/drivers/firmware/arm_scmi/sensors.c
> > @@ -472,7 +472,9 @@ scmi_sensor_axis_extended_names_get(const struct scmi_protocol_handle *ph,
> >         if (IS_ERR(iter))
> >                 return PTR_ERR(iter);
> >  
> > -       return ph->hops->iter_response_run(iter);
> > +       ph->hops->iter_response_run(iter);
> > +
> > +       return 0;
> >  }
> >  
> >  static int scmi_sensor_axis_description(const struct scmi_protocol_handle *ph,
> > ----
> > 
> > Moreover even the parsing logic for the SENSOR_AXIS_GET_NAME command has to
> > be sligthly reviewed to address the fact that the list of returned axes
> > extended names is incomplete so the returned axes won't necessarily be
> > returned in order (i.e. I'll have to check 'axis_d' in the SENSOR_AXIS_NAME_GET
> > replies to look up the proper ax descriptor.).
> > I'll post this as a distinct fix.
> > 
> > Does all of this make sense/seems reasonable ?
> > 
> > Thanks for the review again,
> > Cristian
> > 
> 
> Hi Cristian,
> 
> thanks for your quick reply, this does all make sense to me.
> 

...after a quick chat with Sudeep I'll also post a fix to avoid issuing a
SENSOR_AXIS_GET_NAME if NONE of the axes has been advertised as supporting extended
names

Thanks,
Cristian

[PATCH 16/22] firmware: arm_scmi: Use common iterators in Clock protocol

[Cristian Marussi]

Make SCMI Clock protocol use the common iterator protocol helpers.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 150 ++++++++++++++++++------------
 1 file changed, 90 insertions(+), 60 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index b46b43a99871..48ce7ef9c74b 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -162,81 +162,111 @@ static int rate_cmp_func(const void *_r1, const void *_r2)
 		return 1;
 }
 
-static int
-scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
-			      struct scmi_clock_info *clk)
-{
-	u64 *rate = NULL;
-	int ret, cnt;
-	bool rate_discrete = false;
-	u32 tot_rate_cnt = 0, rates_flag;
-	u16 num_returned, num_remaining;
-	struct scmi_xfer *t;
-	struct scmi_msg_clock_describe_rates *clk_desc;
-	struct scmi_msg_resp_clock_describe_rates *rlist;
+struct scmi_clk_ipriv {
+	u32 clk_id;
+	struct scmi_clock_info *clk;
+};
 
-	ret = ph->xops->xfer_get_init(ph, CLOCK_DESCRIBE_RATES,
-				      sizeof(*clk_desc), 0, &t);
-	if (ret)
-		return ret;
+static void iter_clk_describe_prepare_message(void *message,
+					      const unsigned int desc_index,
+					      const void *priv)
+{
+	struct scmi_msg_clock_describe_rates *msg = message;
+	const struct scmi_clk_ipriv *p = priv;
 
-	clk_desc = t->tx.buf;
-	rlist = t->rx.buf;
+	msg->id = cpu_to_le32(p->clk_id);
+	/* Set the number of rates to be skipped/already read */
+	msg->rate_index = cpu_to_le32(desc_index);
+}
 
-	do {
-		clk_desc->id = cpu_to_le32(clk_id);
-		/* Set the number of rates to be skipped/already read */
-		clk_desc->rate_index = cpu_to_le32(tot_rate_cnt);
+static int
+iter_clk_describe_update_state(struct scmi_iterator_state *st,
+			       const void *response, void *priv)
+{
+	u32 flags;
+	struct scmi_clk_ipriv *p = priv;
+	const struct scmi_msg_resp_clock_describe_rates *r = response;
 
-		ret = ph->xops->do_xfer(ph, t);
-		if (ret)
-			goto err;
+	flags = le32_to_cpu(r->num_rates_flags);
+	st->num_remaining = NUM_REMAINING(flags);
+	st->num_returned = NUM_RETURNED(flags);
+	p->clk->rate_discrete = RATE_DISCRETE(flags);
 
-		rates_flag = le32_to_cpu(rlist->num_rates_flags);
-		num_remaining = NUM_REMAINING(rates_flag);
-		rate_discrete = RATE_DISCRETE(rates_flag);
-		num_returned = NUM_RETURNED(rates_flag);
+	return 0;
+}
 
-		if (tot_rate_cnt + num_returned > SCMI_MAX_NUM_RATES) {
-			dev_err(ph->dev, "No. of rates > MAX_NUM_RATES");
+static int
+iter_clk_describe_process_response(const struct scmi_protocol_handle *ph,
+				   const void *response,
+				   struct scmi_iterator_state *st, void *priv)
+{
+	int ret = 0;
+	struct scmi_clk_ipriv *p = priv;
+	const struct scmi_msg_resp_clock_describe_rates *r = response;
+
+	if (!p->clk->rate_discrete) {
+		switch (st->desc_index + st->loop_idx) {
+		case 0:
+			p->clk->range.min_rate = RATE_TO_U64(r->rate[0]);
 			break;
-		}
-
-		if (!rate_discrete) {
-			clk->range.min_rate = RATE_TO_U64(rlist->rate[0]);
-			clk->range.max_rate = RATE_TO_U64(rlist->rate[1]);
-			clk->range.step_size = RATE_TO_U64(rlist->rate[2]);
-			dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
-				clk->range.min_rate, clk->range.max_rate,
-				clk->range.step_size);
+		case 1:
+			p->clk->range.max_rate = RATE_TO_U64(r->rate[1]);
+			break;
+		case 2:
+			p->clk->range.step_size = RATE_TO_U64(r->rate[2]);
+			break;
+		default:
+			ret = -EINVAL;
 			break;
 		}
+	} else {
+		u64 *rate = &p->clk->list.rates[st->desc_index + st->loop_idx];
 
-		rate = &clk->list.rates[tot_rate_cnt];
-		for (cnt = 0; cnt < num_returned; cnt++, rate++) {
-			*rate = RATE_TO_U64(rlist->rate[cnt]);
-			dev_dbg(ph->dev, "Rate %llu Hz\n", *rate);
-		}
+		*rate = RATE_TO_U64(r->rate[st->loop_idx]);
+		p->clk->list.num_rates++;
+		//XXX dev_dbg(ph->dev, "Rate %llu Hz\n", *rate);
+	}
+
+	return ret;
+}
 
-		tot_rate_cnt += num_returned;
+static int
+scmi_clock_describe_rates_get(const struct scmi_protocol_handle *ph, u32 clk_id,
+			      struct scmi_clock_info *clk)
+{
+	int ret;
 
-		ph->xops->reset_rx_to_maxsz(ph, t);
-		/*
-		 * check for both returned and remaining to avoid infinite
-		 * loop due to buggy firmware
-		 */
-	} while (num_returned && num_remaining);
+	void *iter;
+	struct scmi_msg_clock_describe_rates *msg;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_clk_describe_prepare_message,
+		.update_state = iter_clk_describe_update_state,
+		.process_response = iter_clk_describe_process_response,
+	};
+	struct scmi_clk_ipriv cpriv = {
+		.clk_id = clk_id,
+		.clk = clk,
+	};
+
+	iter = ph->hops->iter_response_init(ph, &ops, SCMI_MAX_NUM_RATES,
+					    CLOCK_DESCRIBE_RATES,
+					    sizeof(*msg), &cpriv);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	ret = ph->hops->iter_response_run(iter);
+	if (ret)
+		return ret;
 
-	if (rate_discrete && rate) {
-		clk->list.num_rates = tot_rate_cnt;
-		sort(clk->list.rates, tot_rate_cnt, sizeof(*rate),
-		     rate_cmp_func, NULL);
+	if (!clk->rate_discrete) {
+		dev_dbg(ph->dev, "Min %llu Max %llu Step %llu Hz\n",
+			clk->range.min_rate, clk->range.max_rate,
+			clk->range.step_size);
+	} else if (clk->list.num_rates) {
+		sort(clk->list.rates, clk->list.num_rates,
+		     sizeof(clk->list.rates[0]), rate_cmp_func, NULL);
 	}
 
-	clk->rate_discrete = rate_discrete;
-
-err:
-	ph->xops->xfer_put(ph, t);
 	return ret;
 }
 
-- 
2.32.0

[PATCH 17/22] firmware: arm_scmi: Use common iterators in Voltage protocol

[Cristian Marussi]

Make SCMI Voltage protocol use the common iterator protocol helpers.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/voltage.c | 161 ++++++++++++++++------------
 1 file changed, 93 insertions(+), 68 deletions(-)

diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index 5d58ba724eeb..e1bdce573c4f 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -112,14 +112,100 @@ static int scmi_init_voltage_levels(struct device *dev,
 	return 0;
 }
 
+struct scmi_volt_ipriv {
+	struct device *dev;
+	struct scmi_voltage_info *v;
+};
+
+static void iter_volt_levels_prepare_message(void *message,
+					     unsigned int desc_index,
+					     const void *priv)
+{
+	struct scmi_msg_cmd_describe_levels *msg = message;
+	const struct scmi_volt_ipriv *p = priv;
+
+	msg->domain_id = cpu_to_le32(p->v->id);
+	msg->level_index = cpu_to_le32(desc_index);
+}
+
+static int iter_volt_levels_update_state(struct scmi_iterator_state *st,
+					 const void *response, void *priv)
+{
+	int ret = 0;
+	u32 flags;
+	const struct scmi_msg_resp_describe_levels *r = response;
+	struct scmi_volt_ipriv *p = priv;
+
+	flags = le32_to_cpu(r->flags);
+	st->num_returned = NUM_RETURNED_LEVELS(flags);
+	st->num_remaining = NUM_REMAINING_LEVELS(flags);
+
+	/* Allocate space for num_levels if not already done */
+	if (!p->v->num_levels) {
+		ret = scmi_init_voltage_levels(p->dev, p->v, st->num_returned,
+					       st->num_remaining,
+					      SUPPORTS_SEGMENTED_LEVELS(flags));
+		if (!ret)
+			st->max_resources = p->v->num_levels;
+	}
+
+	return ret;
+}
+
+static int
+iter_volt_levels_process_response(const struct scmi_protocol_handle *ph,
+				  const void *response,
+				  struct scmi_iterator_state *st, void *priv)
+{
+	s32 val;
+	const struct scmi_msg_resp_describe_levels *r = response;
+	struct scmi_volt_ipriv *p = priv;
+
+	val = (s32)le32_to_cpu(r->voltage[st->loop_idx]);
+	p->v->levels_uv[st->desc_index + st->loop_idx] = val;
+	if (val < 0)
+		p->v->negative_volts_allowed = true;
+
+	return 0;
+}
+
+static int scmi_voltage_levels_get(const struct scmi_protocol_handle *ph,
+				   struct scmi_voltage_info *v)
+{
+	int ret;
+	void *iter;
+	struct scmi_msg_cmd_describe_levels *msg;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_volt_levels_prepare_message,
+		.update_state = iter_volt_levels_update_state,
+		.process_response = iter_volt_levels_process_response,
+	};
+	struct scmi_volt_ipriv vpriv = {
+		.dev = ph->dev,
+		.v = v,
+	};
+
+	iter = ph->hops->iter_response_init(ph, &ops, v->num_levels,
+					    VOLTAGE_DESCRIBE_LEVELS,
+					    sizeof(*msg), &vpriv);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	ret = ph->hops->iter_response_run(iter);
+	if (ret) {
+		v->num_levels = 0;
+		devm_kfree(ph->dev, v->levels_uv);
+	}
+
+	return ret;
+}
+
 static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 					struct voltage_info *vinfo)
 {
 	int ret, dom;
-	struct scmi_xfer *td, *tl;
-	struct device *dev = ph->dev;
+	struct scmi_xfer *td;
 	struct scmi_msg_resp_domain_attributes *resp_dom;
-	struct scmi_msg_resp_describe_levels *resp_levels;
 
 	ret = ph->xops->xfer_get_init(ph, VOLTAGE_DOMAIN_ATTRIBUTES,
 				      sizeof(__le32), sizeof(*resp_dom), &td);
@@ -127,16 +213,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 		return ret;
 	resp_dom = td->rx.buf;
 
-	ret = ph->xops->xfer_get_init(ph, VOLTAGE_DESCRIBE_LEVELS,
-				      sizeof(__le64), 0, &tl);
-	if (ret)
-		goto outd;
-	resp_levels = tl->rx.buf;
-
 	for (dom = 0; dom < vinfo->num_domains; dom++) {
-		u32 desc_index = 0;
-		u16 num_returned = 0, num_remaining = 0;
-		struct scmi_msg_cmd_describe_levels *cmd;
 		struct scmi_voltage_info *v;
 
 		/* Retrieve domain attributes at first ... */
@@ -161,66 +238,14 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 						    v->id, v->name,
 						    SCMI_MAX_STR_SIZE);
 
-		cmd = tl->tx.buf;
-		/* ...then retrieve domain levels descriptions */
-		do {
-			u32 flags;
-			int cnt;
-
-			cmd->domain_id = cpu_to_le32(v->id);
-			cmd->level_index = cpu_to_le32(desc_index);
-			ret = ph->xops->do_xfer(ph, tl);
-			if (ret)
-				break;
-
-			flags = le32_to_cpu(resp_levels->flags);
-			num_returned = NUM_RETURNED_LEVELS(flags);
-			num_remaining = NUM_REMAINING_LEVELS(flags);
-
-			/* Allocate space for num_levels if not already done */
-			if (!v->num_levels) {
-				ret = scmi_init_voltage_levels(dev, v,
-							       num_returned,
-							       num_remaining,
-					      SUPPORTS_SEGMENTED_LEVELS(flags));
-				if (ret)
-					break;
-			}
-
-			if (desc_index + num_returned > v->num_levels) {
-				dev_err(ph->dev,
-					"No. of voltage levels can't exceed %d\n",
-					v->num_levels);
-				ret = -EINVAL;
-				break;
-			}
-
-			for (cnt = 0; cnt < num_returned; cnt++) {
-				s32 val;
-
-				val =
-				    (s32)le32_to_cpu(resp_levels->voltage[cnt]);
-				v->levels_uv[desc_index + cnt] = val;
-				if (val < 0)
-					v->negative_volts_allowed = true;
-			}
-
-			desc_index += num_returned;
-
-			ph->xops->reset_rx_to_maxsz(ph, tl);
-			/* check both to avoid infinite loop due to buggy fw */
-		} while (num_returned && num_remaining);
-
-		if (ret) {
-			v->num_levels = 0;
-			devm_kfree(dev, v->levels_uv);
-		}
+		ret = scmi_voltage_levels_get(ph, v);
+		/* Skip invalid voltage descriptors */
+		if (ret)
+			continue;
 
 		ph->xops->reset_rx_to_maxsz(ph, td);
 	}
 
-	ph->xops->xfer_put(ph, tl);
-outd:
 	ph->xops->xfer_put(ph, td);
 
 	return ret;
-- 
2.32.0

[PATCH 18/22] firmware: arm_scmi: Use common iterators in Perf protocol

[Cristian Marussi]

Make SCMI Perf protocol use the common iterator protocol helpers.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/perf.c | 117 ++++++++++++++++++-------------
 1 file changed, 69 insertions(+), 48 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 9e046fd121b9..e1aa0ed67971 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -272,66 +272,87 @@ static int opp_cmp_func(const void *opp1, const void *opp2)
 	return t1->perf - t2->perf;
 }
 
-static int
-scmi_perf_describe_levels_get(const struct scmi_protocol_handle *ph, u32 domain,
-			      struct perf_dom_info *perf_dom)
+struct scmi_perf_ipriv {
+	u32 domain;
+	struct perf_dom_info *perf_dom;
+};
+
+static void iter_perf_levels_prepare_message(void *message,
+					     unsigned int desc_index,
+					     const void *priv)
 {
-	int ret, cnt;
-	u32 tot_opp_cnt = 0;
-	u16 num_returned, num_remaining;
-	struct scmi_xfer *t;
-	struct scmi_opp *opp;
-	struct scmi_msg_perf_describe_levels *dom_info;
-	struct scmi_msg_resp_perf_describe_levels *level_info;
+	struct scmi_msg_perf_describe_levels *msg = message;
+	const struct scmi_perf_ipriv *p = priv;
 
-	ret = ph->xops->xfer_get_init(ph, PERF_DESCRIBE_LEVELS,
-				      sizeof(*dom_info), 0, &t);
-	if (ret)
-		return ret;
+	msg->domain = cpu_to_le32(p->domain);
+	/* Set the number of OPPs to be skipped/already read */
+	msg->level_index = cpu_to_le32(desc_index);
+}
 
-	dom_info = t->tx.buf;
-	level_info = t->rx.buf;
+static int iter_perf_levels_update_state(struct scmi_iterator_state *st,
+					 const void *response, void *priv)
+{
+	const struct scmi_msg_resp_perf_describe_levels *r = response;
 
-	do {
-		dom_info->domain = cpu_to_le32(domain);
-		/* Set the number of OPPs to be skipped/already read */
-		dom_info->level_index = cpu_to_le32(tot_opp_cnt);
+	st->num_returned = le16_to_cpu(r->num_returned);
+	st->num_remaining = le16_to_cpu(r->num_remaining);
 
-		ret = ph->xops->do_xfer(ph, t);
-		if (ret)
-			break;
+	return 0;
+}
 
-		num_returned = le16_to_cpu(level_info->num_returned);
-		num_remaining = le16_to_cpu(level_info->num_remaining);
-		if (tot_opp_cnt + num_returned > MAX_OPPS) {
-			dev_err(ph->dev, "No. of OPPs exceeded MAX_OPPS");
-			break;
-		}
+static int
+iter_perf_levels_process_response(const struct scmi_protocol_handle *ph,
+				  const void *response,
+				  struct scmi_iterator_state *st, void *priv)
+{
+	struct scmi_opp *opp;
+	const struct scmi_msg_resp_perf_describe_levels *r = response;
+	struct scmi_perf_ipriv *p = priv;
 
-		opp = &perf_dom->opp[tot_opp_cnt];
-		for (cnt = 0; cnt < num_returned; cnt++, opp++) {
-			opp->perf = le32_to_cpu(level_info->opp[cnt].perf_val);
-			opp->power = le32_to_cpu(level_info->opp[cnt].power);
-			opp->trans_latency_us = le16_to_cpu
-				(level_info->opp[cnt].transition_latency_us);
+	opp = &p->perf_dom->opp[st->desc_index + st->loop_idx];
+	opp->perf = le32_to_cpu(r->opp[st->loop_idx].perf_val);
+	opp->power = le32_to_cpu(r->opp[st->loop_idx].power);
+	opp->trans_latency_us =
+		le16_to_cpu(r->opp[st->loop_idx].transition_latency_us);
+	p->perf_dom->opp_count++;
 
-			dev_dbg(ph->dev, "Level %d Power %d Latency %dus\n",
-				opp->perf, opp->power, opp->trans_latency_us);
-		}
+	dev_dbg(ph->dev, "Level %d Power %d Latency %dus\n",
+		opp->perf, opp->power, opp->trans_latency_us);
 
-		tot_opp_cnt += num_returned;
+	return 0;
+}
 
-		ph->xops->reset_rx_to_maxsz(ph, t);
-		/*
-		 * check for both returned and remaining to avoid infinite
-		 * loop due to buggy firmware
-		 */
-	} while (num_returned && num_remaining);
+static int
+scmi_perf_describe_levels_get(const struct scmi_protocol_handle *ph, u32 domain,
+			      struct perf_dom_info *perf_dom)
+{
+	int ret;
+	void *iter;
+	struct scmi_msg_perf_describe_levels *msg;
+	struct scmi_iterator_ops ops = {
+		.prepare_message = iter_perf_levels_prepare_message,
+		.update_state = iter_perf_levels_update_state,
+		.process_response = iter_perf_levels_process_response,
+	};
+	struct scmi_perf_ipriv ppriv = {
+		.domain = domain,
+		.perf_dom = perf_dom,
+	};
+
+	iter = ph->hops->iter_response_init(ph, &ops, MAX_OPPS,
+					    PERF_DESCRIBE_LEVELS,
+					    sizeof(*msg), &ppriv);
+	if (IS_ERR(iter))
+		return PTR_ERR(iter);
+
+	ret = ph->hops->iter_response_run(iter);
+	if (ret)
+		return ret;
 
-	perf_dom->opp_count = tot_opp_cnt;
-	ph->xops->xfer_put(ph, t);
+	if (perf_dom->opp_count)
+		sort(perf_dom->opp, perf_dom->opp_count,
+		     sizeof(struct scmi_opp), opp_cmp_func, NULL);
 
-	sort(perf_dom->opp, tot_opp_cnt, sizeof(*opp), opp_cmp_func, NULL);
 	return ret;
 }
 
-- 
2.32.0

[PATCH 19/22] firmware: arm_scmi: Add SCMIv3.1 Clock notifications

[Cristian Marussi]

Add SCMIv3.1 Clock pre and post notifications.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/clock.c | 136 ++++++++++++++++++++++++++++--
 include/linux/scmi_protocol.h     |  11 +++
 2 files changed, 142 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 48ce7ef9c74b..f72f2bb171cd 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -9,6 +9,7 @@
 #include <linux/sort.h>
 
 #include "protocols.h"
+#include "notify.h"
 
 enum scmi_clock_protocol_cmd {
 	CLOCK_ATTRIBUTES = 0x3,
@@ -17,6 +18,8 @@ enum scmi_clock_protocol_cmd {
 	CLOCK_RATE_GET = 0x6,
 	CLOCK_CONFIG_SET = 0x7,
 	CLOCK_NAME_GET = 0x8,
+	CLOCK_RATE_NOTIFY = 0x9,
+	CLOCK_RATE_CHANGE_REQUESTED_NOTIFY = 0xA,
 };
 
 struct scmi_msg_resp_clock_protocol_attributes {
@@ -28,7 +31,9 @@ struct scmi_msg_resp_clock_protocol_attributes {
 struct scmi_msg_resp_clock_attributes {
 	__le32 attributes;
 #define	CLOCK_ENABLE	BIT(0)
-#define SUPPORTS_EXTENDED_NAMES(x)	((x) & BIT(29))
+#define SUPPORTS_RATE_CHANGED_NOTIF(x)		((x) & BIT(31))
+#define SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(x)	((x) & BIT(30))
+#define SUPPORTS_EXTENDED_NAMES(x)		((x) & BIT(29))
 	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 	__le32 clock_enable_latency;
 };
@@ -76,6 +81,18 @@ struct scmi_msg_resp_set_rate_complete {
 	__le32 rate_high;
 };
 
+struct scmi_msg_clock_rate_notify {
+	__le32 clk_id;
+	__le32 notify_enable;
+};
+
+struct scmi_clock_rate_notify_payld {
+	__le32 agent_id;
+	__le32 clock_id;
+	__le32 rate_low;
+	__le32 rate_high;
+};
+
 struct clock_info {
 	u32 version;
 	int num_clocks;
@@ -84,6 +101,11 @@ struct clock_info {
 	struct scmi_clock_info *clk;
 };
 
+static enum scmi_clock_protocol_cmd evt_2_cmd[] = {
+	CLOCK_RATE_NOTIFY,
+	CLOCK_RATE_CHANGE_REQUESTED_NOTIFY,
+};
+
 static int
 scmi_clock_protocol_attributes_get(const struct scmi_protocol_handle *ph,
 				   struct clock_info *ci)
@@ -142,10 +164,17 @@ static int scmi_clock_attributes_get(const struct scmi_protocol_handle *ph,
 	 * If supported overwrite short name with the extended one;
 	 * on error just carry on and use already provided short name.
 	 */
-	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2 &&
-	    SUPPORTS_EXTENDED_NAMES(attributes))
-		ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
-					    clk->name, SCMI_MAX_STR_SIZE);
+	if (!ret && PROTOCOL_REV_MAJOR(version) >= 0x2) {
+		if (SUPPORTS_EXTENDED_NAMES(attributes))
+			ph->hops->extended_name_get(ph, CLOCK_NAME_GET, clk_id,
+						    clk->name,
+						    SCMI_MAX_STR_SIZE);
+
+		if (SUPPORTS_RATE_CHANGED_NOTIF(attributes))
+			clk->rate_changed_notifications = true;
+		if (SUPPORTS_RATE_CHANGE_REQUESTED_NOTIF(attributes))
+			clk->rate_change_requested_notifications = true;
+	}
 
 	return ret;
 }
@@ -417,6 +446,102 @@ static const struct scmi_clk_proto_ops clk_proto_ops = {
 	.disable_atomic = scmi_clock_disable_atomic,
 };
 
+static int scmi_clk_rate_notify(const struct scmi_protocol_handle *ph,
+				u32 clk_id, int message_id, bool enable)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_clock_rate_notify *notify;
+
+	ret = ph->xops->xfer_get_init(ph, message_id, sizeof(*notify), 0, &t);
+	if (ret)
+		return ret;
+
+	notify = t->tx.buf;
+	notify->clk_id = cpu_to_le32(clk_id);
+	notify->notify_enable = enable ? cpu_to_le32(BIT(0)) : 0;
+
+	ret = ph->xops->do_xfer(ph, t);
+
+	ph->xops->xfer_put(ph, t);
+	return ret;
+}
+
+static int scmi_clk_set_notify_enabled(const struct scmi_protocol_handle *ph,
+				       u8 evt_id, u32 src_id, bool enable)
+{
+	int ret, cmd_id;
+
+	if (evt_id >= ARRAY_SIZE(evt_2_cmd))
+		return -EINVAL;
+
+	cmd_id = evt_2_cmd[evt_id];
+	ret = scmi_clk_rate_notify(ph, src_id, cmd_id, enable);
+	if (ret)
+		pr_debug("FAIL_ENABLED - evt[%X] dom[%d] - ret:%d\n",
+			 evt_id, src_id, ret);
+
+	return ret;
+}
+
+static void *scmi_clk_fill_custom_report(const struct scmi_protocol_handle *ph,
+					 u8 evt_id, ktime_t timestamp,
+					 const void *payld, size_t payld_sz,
+					 void *report, u32 *src_id)
+{
+	const struct scmi_clock_rate_notify_payld *p = payld;
+	struct scmi_clock_rate_notif_report *r = report;
+
+	if (sizeof(*p) != payld_sz ||
+	    (evt_id != SCMI_EVENT_CLOCK_RATE_CHANGED &&
+	     evt_id != SCMI_EVENT_CLOCK_RATE_CHANGE_REQUESTED))
+		return NULL;
+
+	r->timestamp = timestamp;
+	r->agent_id = le32_to_cpu(p->agent_id);
+	r->clock_id = le32_to_cpu(p->clock_id);
+	r->rate = get_unaligned_le64(&p->rate_low);
+	*src_id = r->clock_id;
+
+	return r;
+}
+
+static int scmi_clk_get_num_sources(const struct scmi_protocol_handle *ph)
+{
+	struct clock_info *ci = ph->get_priv(ph);
+
+	if (!ci)
+		return -EINVAL;
+
+	return ci->num_clocks;
+}
+
+static const struct scmi_event clk_events[] = {
+	{
+		.id = SCMI_EVENT_CLOCK_RATE_CHANGED,
+		.max_payld_sz = sizeof(struct scmi_clock_rate_notify_payld),
+		.max_report_sz = sizeof(struct scmi_clock_rate_notif_report),
+	},
+	{
+		.id = SCMI_EVENT_CLOCK_RATE_CHANGE_REQUESTED,
+		.max_payld_sz = sizeof(struct scmi_clock_rate_notify_payld),
+		.max_report_sz = sizeof(struct scmi_clock_rate_notif_report),
+	},
+};
+
+static const struct scmi_event_ops clk_event_ops = {
+	.get_num_sources = scmi_clk_get_num_sources,
+	.set_notify_enabled = scmi_clk_set_notify_enabled,
+	.fill_custom_report = scmi_clk_fill_custom_report,
+};
+
+static const struct scmi_protocol_events clk_protocol_events = {
+	.queue_sz = SCMI_PROTO_QUEUE_SZ,
+	.ops = &clk_event_ops,
+	.evts = clk_events,
+	.num_events = ARRAY_SIZE(clk_events),
+};
+
 static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph)
 {
 	u32 version;
@@ -460,6 +585,7 @@ static const struct scmi_protocol scmi_clock = {
 	.owner = THIS_MODULE,
 	.instance_init = &scmi_clock_protocol_init,
 	.ops = &clk_proto_ops,
+	.events = &clk_protocol_events,
 };
 
 DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(clock, scmi_clock)
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 56e6f13355b8..0e20acc80d50 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -44,6 +44,8 @@ struct scmi_clock_info {
 	char name[SCMI_MAX_STR_SIZE];
 	unsigned int enable_latency;
 	bool rate_discrete;
+	bool rate_changed_notifications;
+	bool rate_change_requested_notifications;
 	union {
 		struct {
 			int num_rates;
@@ -744,6 +746,8 @@ void scmi_protocol_unregister(const struct scmi_protocol *proto);
 /* SCMI Notification API - Custom Event Reports */
 enum scmi_notification_events {
 	SCMI_EVENT_POWER_STATE_CHANGED = 0x0,
+	SCMI_EVENT_CLOCK_RATE_CHANGED = 0x0,
+	SCMI_EVENT_CLOCK_RATE_CHANGE_REQUESTED = 0x1,
 	SCMI_EVENT_PERFORMANCE_LIMITS_CHANGED = 0x0,
 	SCMI_EVENT_PERFORMANCE_LEVEL_CHANGED = 0x1,
 	SCMI_EVENT_SENSOR_TRIP_POINT_EVENT = 0x0,
@@ -760,6 +764,13 @@ struct scmi_power_state_changed_report {
 	unsigned int	power_state;
 };
 
+struct scmi_clock_rate_notif_report {
+	ktime_t			timestamp;
+	unsigned int		agent_id;
+	unsigned int		clock_id;
+	unsigned long long	rate;
+};
+
 struct scmi_system_power_state_notifier_report {
 	ktime_t		timestamp;
 	unsigned int	agent_id;
-- 
2.32.0

[PATCH 20/22] firmware: arm_scmi: Add SCMIv3.1 VOLTAGE_LEVEL_SET_COMPLETE

[Cristian Marussi]

Add SCMIv3.1 Voltage protocol support for asynchronous VOLTAGE_LEVEL_SET
command.

Note that, if a Voltage domain is advertised to support the asynchronous
version of VOLTAGE_LEVEL_SET, the command will be issued asynchronously
unless explicitly requested to use the synchronous version by setting the
mode to SCMI_VOLTAGE_LEVEL_SET_SYNC when calling voltage_ops->level_set.

The SCMI Regulator driver level_set invocation has been left unchanged
so that it will transparently use the asynchronous version if available.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/voltage.c | 51 ++++++++++++++++++++++++-----
 include/linux/scmi_protocol.h       | 12 +++++--
 2 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/arm_scmi/voltage.c b/drivers/firmware/arm_scmi/voltage.c
index e1bdce573c4f..9d195d8719ab 100644
--- a/drivers/firmware/arm_scmi/voltage.c
+++ b/drivers/firmware/arm_scmi/voltage.c
@@ -28,6 +28,7 @@ enum scmi_voltage_protocol_cmd {
 
 struct scmi_msg_resp_domain_attributes {
 	__le32 attr;
+#define SUPPORTS_ASYNC_LEVEL_SET(x)	((x) & BIT(31))
 #define SUPPORTS_EXTENDED_NAMES(x)	((x) & BIT(30))
 	u8 name[SCMI_SHORT_NAME_MAX_SIZE];
 };
@@ -56,6 +57,11 @@ struct scmi_msg_cmd_level_set {
 	__le32 voltage_level;
 };
 
+struct scmi_resp_voltage_level_set_complete {
+	__le32 domain_id;
+	__le32 voltage_level;
+};
+
 struct voltage_info {
 	unsigned int version;
 	unsigned int num_domains;
@@ -214,6 +220,7 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 	resp_dom = td->rx.buf;
 
 	for (dom = 0; dom < vinfo->num_domains; dom++) {
+		u32 attributes;
 		struct scmi_voltage_info *v;
 
 		/* Retrieve domain attributes at first ... */
@@ -225,18 +232,22 @@ static int scmi_voltage_descriptors_get(const struct scmi_protocol_handle *ph,
 
 		v = vinfo->domains + dom;
 		v->id = dom;
-		v->attributes = le32_to_cpu(resp_dom->attr);
+		attributes = le32_to_cpu(resp_dom->attr);
 		strlcpy(v->name, resp_dom->name, SCMI_MAX_STR_SIZE);
 
 		/*
 		 * If supported overwrite short name with the extended one;
 		 * on error just carry on and use already provided short name.
 		 */
-		if (PROTOCOL_REV_MAJOR(vinfo->version) >= 0x2 &&
-		    SUPPORTS_EXTENDED_NAMES(v->attributes))
-			ph->hops->extended_name_get(ph, VOLTAGE_DOMAIN_NAME_GET,
-						    v->id, v->name,
-						    SCMI_MAX_STR_SIZE);
+		if (PROTOCOL_REV_MAJOR(vinfo->version) >= 0x2) {
+			if (SUPPORTS_EXTENDED_NAMES(attributes))
+				ph->hops->extended_name_get(ph,
+							VOLTAGE_DOMAIN_NAME_GET,
+							v->id, v->name,
+							SCMI_MAX_STR_SIZE);
+			if (SUPPORTS_ASYNC_LEVEL_SET(attributes))
+				v->async_level_set = true;
+		}
 
 		ret = scmi_voltage_levels_get(ph, v);
 		/* Skip invalid voltage descriptors */
@@ -308,12 +319,15 @@ static int scmi_voltage_config_get(const struct scmi_protocol_handle *ph,
 }
 
 static int scmi_voltage_level_set(const struct scmi_protocol_handle *ph,
-				  u32 domain_id, u32 flags, s32 volt_uV)
+				  u32 domain_id,
+				  enum scmi_voltage_level_mode mode,
+				  s32 volt_uV)
 {
 	int ret;
 	struct scmi_xfer *t;
 	struct voltage_info *vinfo = ph->get_priv(ph);
 	struct scmi_msg_cmd_level_set *cmd;
+	struct scmi_voltage_info *v;
 
 	if (domain_id >= vinfo->num_domains)
 		return -EINVAL;
@@ -323,12 +337,31 @@ static int scmi_voltage_level_set(const struct scmi_protocol_handle *ph,
 	if (ret)
 		return ret;
 
+	v = vinfo->domains + domain_id;
+
 	cmd = t->tx.buf;
 	cmd->domain_id = cpu_to_le32(domain_id);
-	cmd->flags = cpu_to_le32(flags);
 	cmd->voltage_level = cpu_to_le32(volt_uV);
 
-	ret = ph->xops->do_xfer(ph, t);
+	if (!v->async_level_set || mode != SCMI_VOLTAGE_LEVEL_SET_AUTO) {
+		cmd->flags = cpu_to_le32(0x0);
+		ret = ph->xops->do_xfer(ph, t);
+	} else {
+		cmd->flags = cpu_to_le32(0x1);
+		ret = ph->xops->do_xfer_with_response(ph, t);
+		if (!ret) {
+			struct scmi_resp_voltage_level_set_complete *resp;
+
+			resp = t->rx.buf;
+			if (le32_to_cpu(resp->domain_id) == domain_id)
+				dev_dbg(ph->dev,
+					"Voltage domain %d set async to %d\n",
+					v->id,
+					le32_to_cpu(resp->voltage_level));
+			else
+				ret = -EPROTO;
+		}
+	}
 
 	ph->xops->xfer_put(ph, t);
 	return ret;
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 0e20acc80d50..1c58646ba381 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -495,6 +495,11 @@ struct scmi_reset_proto_ops {
 	int (*deassert)(const struct scmi_protocol_handle *ph, u32 domain);
 };
 
+enum scmi_voltage_level_mode {
+	SCMI_VOLTAGE_LEVEL_SET_AUTO,
+	SCMI_VOLTAGE_LEVEL_SET_SYNC,
+};
+
 /**
  * struct scmi_voltage_info - describe one available SCMI Voltage Domain
  *
@@ -507,7 +512,8 @@ struct scmi_reset_proto_ops {
  *	         supported voltage level
  * @negative_volts_allowed: True if any of the entries of @levels_uv represent
  *			    a negative voltage.
- * @attributes: represents Voltage Domain advertised attributes
+ * @async_level_set: True when the voltage domain supports asynchronous level
+ *		     set commands.
  * @name: name assigned to the Voltage Domain by platform
  * @num_levels: number of total entries in @levels_uv.
  * @levels_uv: array of entries describing the available voltage levels for
@@ -517,7 +523,7 @@ struct scmi_voltage_info {
 	unsigned int id;
 	bool segmented;
 	bool negative_volts_allowed;
-	unsigned int attributes;
+	bool async_level_set;
 	char name[SCMI_MAX_STR_SIZE];
 	unsigned int num_levels;
 #define SCMI_VOLTAGE_SEGMENT_LOW	0
@@ -548,7 +554,7 @@ struct scmi_voltage_proto_ops {
 	int (*config_get)(const struct scmi_protocol_handle *ph, u32 domain_id,
 			  u32 *config);
 	int (*level_set)(const struct scmi_protocol_handle *ph, u32 domain_id,
-			 u32 flags, s32 volt_uV);
+			 enum scmi_voltage_level_mode mode, s32 volt_uV);
 	int (*level_get)(const struct scmi_protocol_handle *ph, u32 domain_id,
 			 s32 *volt_uV);
 };
-- 
2.32.0

[PATCH 21/22] firmware: arm_scmi: Add SCMI v3.1 Perf power-cost in microwatts

[Cristian Marussi]

Add SCMIv3.1 internal support for parsing message attributes reporting
the capability of a performance domain to report power-cost in microwatts.

Cc: Lukasz Luba <lukasz.luba@arm.com>
Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/perf.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index e1aa0ed67971..65ffda5495d6 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -45,6 +45,7 @@ struct scmi_msg_resp_perf_attributes {
 	__le16 num_domains;
 	__le16 flags;
 #define POWER_SCALE_IN_MILLIWATT(x)	((x) & BIT(0))
+#define POWER_SCALE_IN_MICROWATT(x)	((x) & BIT(1))
 	__le32 stats_addr_low;
 	__le32 stats_addr_high;
 	__le32 stats_size;
@@ -170,6 +171,7 @@ struct scmi_perf_info {
 	u32 version;
 	int num_domains;
 	bool power_scale_mw;
+	bool power_scale_uw;
 	u64 stats_addr;
 	u32 stats_size;
 	struct perf_dom_info *dom_info;
@@ -200,6 +202,8 @@ static int scmi_perf_attributes_get(const struct scmi_protocol_handle *ph,
 
 		pi->num_domains = le16_to_cpu(attr->num_domains);
 		pi->power_scale_mw = POWER_SCALE_IN_MILLIWATT(flags);
+		if (PROTOCOL_REV_MAJOR(pi->version) >= 0x3)
+			pi->power_scale_uw = POWER_SCALE_IN_MICROWATT(flags);
 		pi->stats_addr = le32_to_cpu(attr->stats_addr_low) |
 				(u64)le32_to_cpu(attr->stats_addr_high) << 32;
 		pi->stats_size = le32_to_cpu(attr->stats_size);
-- 
2.32.0

Re: [PATCH 21/22] firmware: arm_scmi: Add SCMI v3.1 Perf power-cost in microwatts

[Lukasz Luba]

On 3/30/22 16:05, Cristian Marussi wrote:
> Add SCMIv3.1 internal support for parsing message attributes reporting
> the capability of a performance domain to report power-cost in microwatts.
> 
> Cc: Lukasz Luba <lukasz.luba@arm.com>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>   drivers/firmware/arm_scmi/perf.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index e1aa0ed67971..65ffda5495d6 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -45,6 +45,7 @@ struct scmi_msg_resp_perf_attributes {
>   	__le16 num_domains;
>   	__le16 flags;
>   #define POWER_SCALE_IN_MILLIWATT(x)	((x) & BIT(0))
> +#define POWER_SCALE_IN_MICROWATT(x)	((x) & BIT(1))
>   	__le32 stats_addr_low;
>   	__le32 stats_addr_high;
>   	__le32 stats_size;
> @@ -170,6 +171,7 @@ struct scmi_perf_info {
>   	u32 version;
>   	int num_domains;
>   	bool power_scale_mw;
> +	bool power_scale_uw;
>   	u64 stats_addr;
>   	u32 stats_size;
>   	struct perf_dom_info *dom_info;
> @@ -200,6 +202,8 @@ static int scmi_perf_attributes_get(const struct scmi_protocol_handle *ph,
>   
>   		pi->num_domains = le16_to_cpu(attr->num_domains);
>   		pi->power_scale_mw = POWER_SCALE_IN_MILLIWATT(flags);
> +		if (PROTOCOL_REV_MAJOR(pi->version) >= 0x3)
> +			pi->power_scale_uw = POWER_SCALE_IN_MICROWATT(flags);
>   		pi->stats_addr = le32_to_cpu(attr->stats_addr_low) |
>   				(u64)le32_to_cpu(attr->stats_addr_high) << 32;
>   		pi->stats_size = le32_to_cpu(attr->stats_size);

LGTM,

Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>

I'll send a follow-up patch which is using this information when this
patch gets into linux-next.

[PATCH 22/22] firmware: arm_scmi: Add SCMIv3.1 PERFORMANCE_LIMITS_SET checks

[Cristian Marussi]

Starting with SCMIv3.1, the PERFORMANCE_LIMITS_SET command allows a user
to request only one between max and min ranges to be changed, while leaving
the other untouched if set to zero in the request; anyway SCMIv3.1 states
also explicitly that you cannot leave both of those unchanged (zeroed) when
issuing such command: add a proper check for this condition.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 drivers/firmware/arm_scmi/perf.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 65ffda5495d6..8f4051aca220 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -423,6 +423,9 @@ static int scmi_perf_limits_set(const struct scmi_protocol_handle *ph,
 	struct scmi_perf_info *pi = ph->get_priv(ph);
 	struct perf_dom_info *dom = pi->dom_info + domain;
 
+	if (PROTOCOL_REV_MAJOR(pi->version) >= 0x3 && !max_perf && !min_perf)
+		return -EINVAL;
+
 	if (dom->fc_info && dom->fc_info->limit_set_addr) {
 		iowrite32(max_perf, dom->fc_info->limit_set_addr);
 		iowrite32(min_perf, dom->fc_info->limit_set_addr + 4);
-- 
2.32.0

Re: [PATCH 22/22] firmware: arm_scmi: Add SCMIv3.1 PERFORMANCE_LIMITS_SET checks

[Sudeep Holla]

On Wed, Mar 30, 2022 at 04:05:51PM +0100, Cristian Marussi wrote:
> Starting with SCMIv3.1, the PERFORMANCE_LIMITS_SET command allows a user
> to request only one between max and min ranges to be changed, while leaving
> the other untouched if set to zero in the request; anyway SCMIv3.1 states
> also explicitly that you cannot leave both of those unchanged (zeroed) when
> issuing such command: add a proper check for this condition.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  drivers/firmware/arm_scmi/perf.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 65ffda5495d6..8f4051aca220 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -423,6 +423,9 @@ static int scmi_perf_limits_set(const struct scmi_protocol_handle *ph,
>  	struct scmi_perf_info *pi = ph->get_priv(ph);
>  	struct perf_dom_info *dom = pi->dom_info + domain;
>  
> +	if (PROTOCOL_REV_MAJOR(pi->version) >= 0x3 && !max_perf && !min_perf)
> +		return -EINVAL;
> +

Do we really need the version check here ? I agree it was explicitly added
in v3.1, but it makes sense on any version really. No ?

-- 
Regards,
Sudeep

Re: [PATCH 22/22] firmware: arm_scmi: Add SCMIv3.1 PERFORMANCE_LIMITS_SET checks

[Cristian Marussi]

On Thu, Apr 28, 2022 at 02:13:57PM +0100, Sudeep Holla wrote:
> On Wed, Mar 30, 2022 at 04:05:51PM +0100, Cristian Marussi wrote:
> > Starting with SCMIv3.1, the PERFORMANCE_LIMITS_SET command allows a user
> > to request only one between max and min ranges to be changed, while leaving
> > the other untouched if set to zero in the request; anyway SCMIv3.1 states
> > also explicitly that you cannot leave both of those unchanged (zeroed) when
> > issuing such command: add a proper check for this condition.
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/perf.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> > index 65ffda5495d6..8f4051aca220 100644
> > --- a/drivers/firmware/arm_scmi/perf.c
> > +++ b/drivers/firmware/arm_scmi/perf.c
> > @@ -423,6 +423,9 @@ static int scmi_perf_limits_set(const struct scmi_protocol_handle *ph,
> >  	struct scmi_perf_info *pi = ph->get_priv(ph);
> >  	struct perf_dom_info *dom = pi->dom_info + domain;
> >  
> > +	if (PROTOCOL_REV_MAJOR(pi->version) >= 0x3 && !max_perf && !min_perf)
> > +		return -EINVAL;
> > +
> 
> Do we really need the version check here ? I agree it was explicitly added
> in v3.1, but it makes sense on any version really. No ?

Indeed seemed a silly patch also to me but given that only in v3.1 it is
explicitly stated that you cannot issue this command with both min and
max ZEROED I though this could have broken older fw that allowed
setting PERF_LIMITS_SET max=0 min=0

....maybe overthought ...

Thanks,
Cristian

Re: [PATCH 22/22] firmware: arm_scmi: Add SCMIv3.1 PERFORMANCE_LIMITS_SET checks

[Sudeep Holla]

On Thu, Apr 28, 2022 at 02:49:48PM +0100, Cristian Marussi wrote:
> On Thu, Apr 28, 2022 at 02:13:57PM +0100, Sudeep Holla wrote:
> > On Wed, Mar 30, 2022 at 04:05:51PM +0100, Cristian Marussi wrote:
> > > Starting with SCMIv3.1, the PERFORMANCE_LIMITS_SET command allows a user
> > > to request only one between max and min ranges to be changed, while leaving
> > > the other untouched if set to zero in the request; anyway SCMIv3.1 states
> > > also explicitly that you cannot leave both of those unchanged (zeroed) when
> > > issuing such command: add a proper check for this condition.
> > > 
> > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > > ---
> > >  drivers/firmware/arm_scmi/perf.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> > > index 65ffda5495d6..8f4051aca220 100644
> > > --- a/drivers/firmware/arm_scmi/perf.c
> > > +++ b/drivers/firmware/arm_scmi/perf.c
> > > @@ -423,6 +423,9 @@ static int scmi_perf_limits_set(const struct scmi_protocol_handle *ph,
> > >  	struct scmi_perf_info *pi = ph->get_priv(ph);
> > >  	struct perf_dom_info *dom = pi->dom_info + domain;
> > >  
> > > +	if (PROTOCOL_REV_MAJOR(pi->version) >= 0x3 && !max_perf && !min_perf)
> > > +		return -EINVAL;
> > > +
> > 
> > Do we really need the version check here ? I agree it was explicitly added
> > in v3.1, but it makes sense on any version really. No ?
> 
> Indeed seemed a silly patch also to me but given that only in v3.1 it is
> explicitly stated that you cannot issue this command with both min and
> max ZEROED I though this could have broken older fw that allowed
> setting PERF_LIMITS_SET max=0 min=0
> 
> ....maybe overthought ...

Hmm, let's keep it unconditional for now. We can add if someone reports
broken firmware. BTW there are no users in the kernel 😄.

-- 
Regards,
Sudeep

Re: [PATCH 00/22] SCMIv3.1 Miscellaneous changes

[Sudeep Holla]

On Wed, Mar 30, 2022 at 04:05:29PM +0100, Cristian Marussi wrote:
> Hi all,
> 
> this series introduces a bunch of SCMIv3.1 miscellaneous changes to support
> basically all the SCMIv3.1 specification [1] addition with the exclusion of
> the Powercap protocol and driver which will be introduced later on in
> another series.
> 
> Most notably the series adds:
> 
>  - supports across all protocols for long resources naming using *_NAME_GET
>    dedicated new commands
>  - Clock protocol Rate change pre and post notifications
>  - Voltage protocol asynchronous voltage level set command
>    (VOLTAGE_LEVEL_SET_COMPLETE delayed response)
>  - Perf protocol power-cost in micro-watts (only internal support)
>  - Perf protocol PERFORMANCE_LIMITS_SET	new checks
> 

Apart from minor comments I have, this looks good and I have queued it
provisionally with the changes I have mentioned in the thread.

-- 
Regards,
Sudeep

Re: [PATCH 00/22] SCMIv3.1 Miscellaneous changes

[Sudeep Holla]

On Wed, 30 Mar 2022 16:05:29 +0100, Cristian Marussi wrote:
> this series introduces a bunch of SCMIv3.1 miscellaneous changes to support
> basically all the SCMIv3.1 specification [1] addition with the exclusion of
> the Powercap protocol and driver which will be introduced later on in
> another series.
> 
> Most notably the series adds:
> 
> [...]

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

[02/22] firmware: arm_scmi: Make protocols init fail on basic errors
        https://git.kernel.org/sudeep.holla/c/4de1b36fae
[03/22] firmware: arm_scmi: Fix Base list protocols enumeration
        https://git.kernel.org/sudeep.holla/c/8009120e03
[04/22] firmware: arm_scmi: Validate BASE_DISCOVER_LIST_PROTOCOLS reply
        https://git.kernel.org/sudeep.holla/c/3b0041f6e1
[05/22] firmware: arm_scmi: Dynamically allocate protocols array
        https://git.kernel.org/sudeep.holla/c/776b6c8a25
[06/22] firmware: arm_scmi: Make name_get operations return a const
        https://git.kernel.org/sudeep.holla/c/992be5d3c8
[07/22] firmware: arm_scmi: Check CLOCK_RATE_SET_COMPLETE async reply
        https://git.kernel.org/sudeep.holla/c/c7e223f5c7
[08/22] firmware: arm_scmi: Remove unneeded NULL termination of clk name
        https://git.kernel.org/sudeep.holla/c/91ebc56cbc
[09/22] firmware: arm_scmi: Split protocol specific definitions in a dedicated header
        https://git.kernel.org/sudeep.holla/c/23136bff80
[10/22] firmware: arm_scmi: Introduce a common SCMIv3.1 .extended_name_get helper
        https://git.kernel.org/sudeep.holla/c/5c873d120d
[11/22] firmware: arm_scmi: Add SCMIv3.1 extended names protocols support
        https://git.kernel.org/sudeep.holla/c/b260fccaeb
[12/22] firmware: arm_scmi: Parse clock_enable_latency conditionally
        https://git.kernel.org/sudeep.holla/c/df3576d14a
[13/22] firmware: arm_scmi: Add iterators for multi-part commands
        https://git.kernel.org/sudeep.holla/c/36b6ea0fc6
[14/22] firmware: arm_scmi: Use common iterators in Sensor protocol
        https://git.kernel.org/sudeep.holla/c/7cab537704
[15/22] firmware: arm_scmi: Add SCMIv3.1 SENSOR_AXIS_NAME_GET support
        https://git.kernel.org/sudeep.holla/c/802b0bed01
[16/22] firmware: arm_scmi: Use common iterators in Clock protocol
        https://git.kernel.org/sudeep.holla/c/7bc7caafe6
[17/22] firmware: arm_scmi: Use common iterators in Voltage protocol
        https://git.kernel.org/sudeep.holla/c/d8d7e91316
[18/22] firmware: arm_scmi: Use common iterators in Perf protocol
        https://git.kernel.org/sudeep.holla/c/79d2ea9244
[19/22] firmware: arm_scmi: Add SCMIv3.1 Clock notifications
        https://git.kernel.org/sudeep.holla/c/7aa75496ea
[20/22] firmware: arm_scmi: Add SCMIv3.1 VOLTAGE_LEVEL_SET_COMPLETE
        https://git.kernel.org/sudeep.holla/c/4c74701b1e
[21/22] firmware: arm_scmi: Add SCMI v3.1 Perf power-cost in microwatts
        https://git.kernel.org/sudeep.holla/c/3630cd8130
[22/22] firmware: arm_scmi: Add SCMIv3.1 PERFORMANCE_LIMITS_SET checks
        https://git.kernel.org/sudeep.holla/c/71bea05797

--
Regards,
Sudeep