[PATCH 00/15] firmware: arm_scmi: Add support for multiple device per protocol

[PATCH 00/15] firmware: arm_scmi: Add support for multiple device per protocol

[Sudeep Holla]

Currently only one scmi device is created for each protocol enumerated.
However, there is requirement to make use of some procotols by multiple
kernel subsystems/frameworks. One such example is SCMI PERFORMANCE
protocol which can be used by both cpufreq and devfreq drivers.
Similarly, SENSOR protocol may be used by hwmon and iio subsystems,
and POWER protocol may be used by genpd and regulator drivers.

This series adds support for multiple device per protocol using scmi device
name if one is available. It also updates existing drivers to add
scmi device names to driver id tables.

Regards,
Sudeep

Sudeep Holla (15):
  firmware: arm_scmi: Add support for multiple device per protocol
  firmware: arm_scmi: Skip scmi mbox channel setup for addtional devices
  firmware: arm_scmi: Skip protocol initialisation for additional devices
  firmware: arm_scmi: Add names to scmi devices created
  firmware: arm_scmi: Add versions and identifier attributes using dev_groups
  firmware: arm_scmi: Update scmi_prot_init_fn_t to use device instead of handle
  firmware: arm_scmi: Stash version in protocol init functions
  firmware: arm_scmi: Add and initialise protocol version to scmi_device structure
  firmware: arm_scmi: Add scmi protocol version and id device attributes
  firmware: arm_scmi: Drop logging individual scmi protocol version
  firmware: arm_scmi: Match scmi device by both name and protocol id
  clk: scmi: Match scmi device by both name and protocol id
  cpufreq: scmi: Match scmi device by both name and protocol id
  hwmon: (scmi-hwmon) Match scmi device by both name and protocol id
  reset: reset-scmi: Match scmi device by both name and protocol id

 drivers/clk/clk-scmi.c                     |  2 +-
 drivers/cpufreq/scmi-cpufreq.c             |  2 +-
 drivers/firmware/arm_scmi/bus.c            | 53 +++++++++++--
 drivers/firmware/arm_scmi/clock.c          | 15 +++-
 drivers/firmware/arm_scmi/driver.c         | 92 +++++++++++++++++++++-
 drivers/firmware/arm_scmi/perf.c           | 15 +++-
 drivers/firmware/arm_scmi/power.c          | 15 +++-
 drivers/firmware/arm_scmi/reset.c          | 15 +++-
 drivers/firmware/arm_scmi/scmi_pm_domain.c |  2 +-
 drivers/firmware/arm_scmi/sensors.c        | 15 +++-
 drivers/hwmon/scmi-hwmon.c                 |  2 +-
 drivers/reset/reset-scmi.c                 |  2 +-
 include/linux/scmi_protocol.h              |  8 +-
 13 files changed, 202 insertions(+), 36 deletions(-)

--
2.17.1

[PATCH 01/15] firmware: arm_scmi: Add support for multiple device per protocol

[Sudeep Holla]

Currently only one scmi device is created for each protocol enumerated.
However, there is requirement to make use of some procotols by multiple
kernel subsystems/frameworks. One such example is SCMI PERFORMANCE
protocol which can be used by both cpufreq and devfreq drivers.
Similarly, SENSOR protocol may be used by hwmon and iio subsystems,
and POWER protocol may be used by genpd and regulator drivers.

In order to achieve that, let us extend the scmi bus to match based
not only protocol id but also the scmi device name if one is available.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/bus.c    | 20 +++++++++++++++++---
 drivers/firmware/arm_scmi/driver.c |  6 +++---
 include/linux/scmi_protocol.h      |  5 ++++-
 3 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 7a30952b463d..3714e6307b05 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -28,8 +28,12 @@ scmi_dev_match_id(struct scmi_device *scmi_dev, struct scmi_driver *scmi_drv)
 		return NULL;

 	for (; id->protocol_id; id++)
-		if (id->protocol_id == scmi_dev->protocol_id)
-			return id;
+		if (id->protocol_id == scmi_dev->protocol_id) {
+			if (!id->name)
+				return id;
+			else if (!strcmp(id->name, scmi_dev->name))
+				return id;
+		}

 	return NULL;
 }
@@ -125,7 +129,8 @@ static void scmi_device_release(struct device *dev)
 }

 struct scmi_device *
-scmi_device_create(struct device_node *np, struct device *parent, int protocol)
+scmi_device_create(struct device_node *np, struct device *parent, int protocol,
+		   const char *name)
 {
 	int id, retval;
 	struct scmi_device *scmi_dev;
@@ -134,8 +139,15 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol)
 	if (!scmi_dev)
 		return NULL;

+	scmi_dev->name = kstrdup_const(name ?: "unknown", GFP_KERNEL);
+	if (!scmi_dev->name) {
+		kfree(scmi_dev);
+		return NULL;
+	}
+
 	id = ida_simple_get(&scmi_bus_id, 1, 0, GFP_KERNEL);
 	if (id < 0) {
+		kfree_const(scmi_dev->name);
 		kfree(scmi_dev);
 		return NULL;
 	}
@@ -154,6 +166,7 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol)

 	return scmi_dev;
 put_dev:
+	kfree_const(scmi_dev->name);
 	put_device(&scmi_dev->dev);
 	ida_simple_remove(&scmi_bus_id, id);
 	return NULL;
@@ -161,6 +174,7 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol)

 void scmi_device_destroy(struct scmi_device *scmi_dev)
 {
+	kfree_const(scmi_dev->name);
 	scmi_handle_put(scmi_dev->handle);
 	ida_simple_remove(&scmi_bus_id, scmi_dev->id);
 	device_unregister(&scmi_dev->dev);
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 3eb0382491ce..dee7ce3bd815 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -803,11 +803,11 @@ scmi_mbox_txrx_setup(struct scmi_info *info, struct device *dev, int prot_id)

 static inline void
 scmi_create_protocol_device(struct device_node *np, struct scmi_info *info,
-			    int prot_id)
+			    int prot_id, const char *name)
 {
 	struct scmi_device *sdev;

-	sdev = scmi_device_create(np, info->dev, prot_id);
+	sdev = scmi_device_create(np, info->dev, prot_id, name);
 	if (!sdev) {
 		dev_err(info->dev, "failed to create %d protocol device\n",
 			prot_id);
@@ -892,7 +892,7 @@ static int scmi_probe(struct platform_device *pdev)
 			continue;
 		}

-		scmi_create_protocol_device(child, info, prot_id);
+		scmi_create_protocol_device(child, info, prot_id, NULL);
 	}

 	return 0;
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 881fea47c83d..5c873a59b387 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -257,6 +257,7 @@ enum scmi_std_protocol {
 struct scmi_device {
 	u32 id;
 	u8 protocol_id;
+	const char *name;
 	struct device dev;
 	struct scmi_handle *handle;
 };
@@ -264,11 +265,13 @@ struct scmi_device {
 #define to_scmi_dev(d) container_of(d, struct scmi_device, dev)

 struct scmi_device *
-scmi_device_create(struct device_node *np, struct device *parent, int protocol);
+scmi_device_create(struct device_node *np, struct device *parent, int protocol,
+		   const char *name);
 void scmi_device_destroy(struct scmi_device *scmi_dev);

 struct scmi_device_id {
 	u8 protocol_id;
+	const char *name;
 };

 struct scmi_driver {
--
2.17.1

Re: [PATCH 01/15] firmware: arm_scmi: Add support for multiple device per protocol

[Cristian Marussi]

Hi Sudeep,

On 10/12/2019 14:53, Sudeep Holla wrote:
> Currently only one scmi device is created for each protocol enumerated.
> However, there is requirement to make use of some procotols by multiple
> kernel subsystems/frameworks. One such example is SCMI PERFORMANCE
> protocol which can be used by both cpufreq and devfreq drivers.
> Similarly, SENSOR protocol may be used by hwmon and iio subsystems,
> and POWER protocol may be used by genpd and regulator drivers.
> 
> In order to achieve that, let us extend the scmi bus to match based
> not only protocol id but also the scmi device name if one is available.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/bus.c    | 20 +++++++++++++++++---
>  drivers/firmware/arm_scmi/driver.c |  6 +++---
>  include/linux/scmi_protocol.h      |  5 ++++-
>  3 files changed, 24 insertions(+), 7 deletions(-)
> 

Looks good to me.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Cheers

Cristian

> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 7a30952b463d..3714e6307b05 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -28,8 +28,12 @@ scmi_dev_match_id(struct scmi_device *scmi_dev, struct scmi_driver *scmi_drv)
>  		return NULL;
> 
>  	for (; id->protocol_id; id++)
> -		if (id->protocol_id == scmi_dev->protocol_id)
> -			return id;
> +		if (id->protocol_id == scmi_dev->protocol_id) {
> +			if (!id->name)
> +				return id;
> +			else if (!strcmp(id->name, scmi_dev->name))
> +				return id;
> +		}
> 
>  	return NULL;
>  }
> @@ -125,7 +129,8 @@ static void scmi_device_release(struct device *dev)
>  }
> 
>  struct scmi_device *
> -scmi_device_create(struct device_node *np, struct device *parent, int protocol)
> +scmi_device_create(struct device_node *np, struct device *parent, int protocol,
> +		   const char *name)
>  {
>  	int id, retval;
>  	struct scmi_device *scmi_dev;
> @@ -134,8 +139,15 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol)
>  	if (!scmi_dev)
>  		return NULL;
> 
> +	scmi_dev->name = kstrdup_const(name ?: "unknown", GFP_KERNEL);
> +	if (!scmi_dev->name) {
> +		kfree(scmi_dev);
> +		return NULL;
> +	}
> +
>  	id = ida_simple_get(&scmi_bus_id, 1, 0, GFP_KERNEL);
>  	if (id < 0) {
> +		kfree_const(scmi_dev->name);
>  		kfree(scmi_dev);
>  		return NULL;
>  	}
> @@ -154,6 +166,7 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol)
> 
>  	return scmi_dev;
>  put_dev:
> +	kfree_const(scmi_dev->name);
>  	put_device(&scmi_dev->dev);
>  	ida_simple_remove(&scmi_bus_id, id);
>  	return NULL;
> @@ -161,6 +174,7 @@ scmi_device_create(struct device_node *np, struct device *parent, int protocol)
> 
>  void scmi_device_destroy(struct scmi_device *scmi_dev)
>  {
> +	kfree_const(scmi_dev->name);
>  	scmi_handle_put(scmi_dev->handle);
>  	ida_simple_remove(&scmi_bus_id, scmi_dev->id);
>  	device_unregister(&scmi_dev->dev);
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 3eb0382491ce..dee7ce3bd815 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -803,11 +803,11 @@ scmi_mbox_txrx_setup(struct scmi_info *info, struct device *dev, int prot_id)
> 
>  static inline void
>  scmi_create_protocol_device(struct device_node *np, struct scmi_info *info,
> -			    int prot_id)
> +			    int prot_id, const char *name)
>  {
>  	struct scmi_device *sdev;
> 
> -	sdev = scmi_device_create(np, info->dev, prot_id);
> +	sdev = scmi_device_create(np, info->dev, prot_id, name);
>  	if (!sdev) {
>  		dev_err(info->dev, "failed to create %d protocol device\n",
>  			prot_id);
> @@ -892,7 +892,7 @@ static int scmi_probe(struct platform_device *pdev)
>  			continue;
>  		}
> 
> -		scmi_create_protocol_device(child, info, prot_id);
> +		scmi_create_protocol_device(child, info, prot_id, NULL);
>  	}
> 
>  	return 0;
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 881fea47c83d..5c873a59b387 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -257,6 +257,7 @@ enum scmi_std_protocol {
>  struct scmi_device {
>  	u32 id;
>  	u8 protocol_id;
> +	const char *name;
>  	struct device dev;
>  	struct scmi_handle *handle;
>  };
> @@ -264,11 +265,13 @@ struct scmi_device {
>  #define to_scmi_dev(d) container_of(d, struct scmi_device, dev)
> 
>  struct scmi_device *
> -scmi_device_create(struct device_node *np, struct device *parent, int protocol);
> +scmi_device_create(struct device_node *np, struct device *parent, int protocol,
> +		   const char *name);
>  void scmi_device_destroy(struct scmi_device *scmi_dev);
> 
>  struct scmi_device_id {
>  	u8 protocol_id;
> +	const char *name;
>  };
> 
>  struct scmi_driver {
> --
> 2.17.1
> 

[PATCH 02/15] firmware: arm_scmi: Skip scmi mbox channel setup for addtional devices

[Sudeep Holla]

Now that the scmi bus supports adding multiple devices per protocol,
and since scmi_create_protocol_device calls scmi_mbox_chan_setup,
we must avoid allocating and initialising the mbox channel if it is
already initialised.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index dee7ce3bd815..2952fcd8dd8a 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -735,6 +735,11 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
 	idx = tx ? 0 : 1;
 	idr = tx ? &info->tx_idr : &info->rx_idr;

+	/* check if already allocated, used for multiple device per protocol */
+	cinfo = idr_find(idr, prot_id);
+	if (cinfo)
+		return 0;
+
 	if (scmi_mailbox_check(np, idx)) {
 		cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
 		if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
--
2.17.1

Re: [PATCH 02/15] firmware: arm_scmi: Skip scmi mbox channel setup for addtional devices

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> Now that the scmi bus supports adding multiple devices per protocol,
> and since scmi_create_protocol_device calls scmi_mbox_chan_setup,
> we must avoid allocating and initialising the mbox channel if it is
> already initialised.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/driver.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index dee7ce3bd815..2952fcd8dd8a 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -735,6 +735,11 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
>  	idx = tx ? 0 : 1;
>  	idr = tx ? &info->tx_idr : &info->rx_idr;
> 
> +	/* check if already allocated, used for multiple device per protocol */
> +	cinfo = idr_find(idr, prot_id);
> +	if (cinfo)
> +		return 0;
> +
>  	if (scmi_mailbox_check(np, idx)) {
>  		cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
>  		if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
> --
> 2.17.1
> 

Fine for me.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>


Cristian

[PATCH 03/15] firmware: arm_scmi: Skip protocol initialisation for additional devices

[Sudeep Holla]

The scmi bus now supports adding multiple devices per protocol,
and since scmi_protocol_init is called for each scmi device created,
we must avoid allocating protocol private data and initialising the
protocol itself if it is already initialised.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/clock.c   | 3 +++
 drivers/firmware/arm_scmi/perf.c    | 3 +++
 drivers/firmware/arm_scmi/power.c   | 3 +++
 drivers/firmware/arm_scmi/reset.c   | 3 +++
 drivers/firmware/arm_scmi/sensors.c | 3 +++
 5 files changed, 15 insertions(+)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 32526a793f3a..922b22aaaf84 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -316,6 +316,9 @@ static int scmi_clock_protocol_init(struct scmi_handle *handle)
 	int clkid, ret;
 	struct clock_info *cinfo;

+	if (handle->clk_ops && handle->clk_priv)
+		return 0; /* initialised already for the first device */
+
 	scmi_version_get(handle, SCMI_PROTOCOL_CLOCK, &version);

 	dev_dbg(handle->dev, "Clock Version %d.%d\n",
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 601af4edad5e..55c2a4c21ccb 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -710,6 +710,9 @@ static int scmi_perf_protocol_init(struct scmi_handle *handle)
 	u32 version;
 	struct scmi_perf_info *pinfo;

+	if (handle->perf_ops && handle->perf_priv)
+		return 0; /* initialised already for the first device */
+
 	scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version);

 	dev_dbg(handle->dev, "Performance Version %d.%d\n",
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 5abef7079c0a..9a7593238b8f 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -185,6 +185,9 @@ static int scmi_power_protocol_init(struct scmi_handle *handle)
 	u32 version;
 	struct scmi_power_info *pinfo;

+	if (handle->power_ops && handle->power_priv)
+		return 0; /* initialised already for the first device */
+
 	scmi_version_get(handle, SCMI_PROTOCOL_POWER, &version);

 	dev_dbg(handle->dev, "Power Version %d.%d\n",
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index ab42c21c5517..809dc8faee1e 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -195,6 +195,9 @@ static int scmi_reset_protocol_init(struct scmi_handle *handle)
 	u32 version;
 	struct scmi_reset_info *pinfo;

+	if (handle->reset_ops && handle->reset_priv)
+		return 0; /* initialised already for the first device */
+
 	scmi_version_get(handle, SCMI_PROTOCOL_RESET, &version);

 	dev_dbg(handle->dev, "Reset Version %d.%d\n",
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index a400ea805fc2..b7f92c37c8a4 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -276,6 +276,9 @@ static int scmi_sensors_protocol_init(struct scmi_handle *handle)
 	u32 version;
 	struct sensors_info *sinfo;

+	if (handle->sensor_ops && handle->sensor_priv)
+		return 0; /* initialised already for the first device */
+
 	scmi_version_get(handle, SCMI_PROTOCOL_SENSOR, &version);

 	dev_dbg(handle->dev, "Sensor Version %d.%d\n",
--
2.17.1

Re: [PATCH 03/15] firmware: arm_scmi: Skip protocol initialisation for additional devices

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> The scmi bus now supports adding multiple devices per protocol,
> and since scmi_protocol_init is called for each scmi device created,
> we must avoid allocating protocol private data and initialising the
> protocol itself if it is already initialised.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Wouldn't be better to add some kind of per-protocol 'initialized' flag somewhere
in the bus abstraction so that the protocol_id itself could be marked as initialized
once bus::scmi_protocol_init() completes successfully so that we could just skip the
invocation itself of bus::scmi_protocol_init() for all the protocols already detected
as initialized ?

Or, if not a flag, maybe deactivating the registered protocol init function itself
once it has been successfully called once .....something along the lines of:

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 7a30952b463d..a551a00586c6 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -73,6 +73,8 @@ static int scmi_dev_probe(struct device *dev)
        ret = scmi_protocol_init(scmi_dev->protocol_id, scmi_dev->handle);
        if (ret)
                return ret;
+       idr_replace(&scmi_protocols, dummy_return_0_callback,
+                       scmi_dev->protocol_id);
 
        return scmi_drv->probe(scmi_dev);

[not really tested eh ... :D]

This way we can drop this patch as a whole and avoid any future needs to remember
to add this same sort of logic in the next XYZ protocol implementation.

Cheers

Cristian

>  drivers/firmware/arm_scmi/clock.c   | 3 +++
>  drivers/firmware/arm_scmi/perf.c    | 3 +++
>  drivers/firmware/arm_scmi/power.c   | 3 +++
>  drivers/firmware/arm_scmi/reset.c   | 3 +++
>  drivers/firmware/arm_scmi/sensors.c | 3 +++
>  5 files changed, 15 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 32526a793f3a..922b22aaaf84 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -316,6 +316,9 @@ static int scmi_clock_protocol_init(struct scmi_handle *handle)
>  	int clkid, ret;
>  	struct clock_info *cinfo;
> 
> +	if (handle->clk_ops && handle->clk_priv)
> +		return 0; /* initialised already for the first device */
> +
>  	scmi_version_get(handle, SCMI_PROTOCOL_CLOCK, &version);
> 
>  	dev_dbg(handle->dev, "Clock Version %d.%d\n",
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 601af4edad5e..55c2a4c21ccb 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -710,6 +710,9 @@ static int scmi_perf_protocol_init(struct scmi_handle *handle)
>  	u32 version;
>  	struct scmi_perf_info *pinfo;
> 
> +	if (handle->perf_ops && handle->perf_priv)
> +		return 0; /* initialised already for the first device */
> +
>  	scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version);
> 
>  	dev_dbg(handle->dev, "Performance Version %d.%d\n",
> diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
> index 5abef7079c0a..9a7593238b8f 100644
> --- a/drivers/firmware/arm_scmi/power.c
> +++ b/drivers/firmware/arm_scmi/power.c
> @@ -185,6 +185,9 @@ static int scmi_power_protocol_init(struct scmi_handle *handle)
>  	u32 version;
>  	struct scmi_power_info *pinfo;
> 
> +	if (handle->power_ops && handle->power_priv)
> +		return 0; /* initialised already for the first device */
> +
>  	scmi_version_get(handle, SCMI_PROTOCOL_POWER, &version);
> 
>  	dev_dbg(handle->dev, "Power Version %d.%d\n",
> diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> index ab42c21c5517..809dc8faee1e 100644
> --- a/drivers/firmware/arm_scmi/reset.c
> +++ b/drivers/firmware/arm_scmi/reset.c
> @@ -195,6 +195,9 @@ static int scmi_reset_protocol_init(struct scmi_handle *handle)
>  	u32 version;
>  	struct scmi_reset_info *pinfo;
> 
> +	if (handle->reset_ops && handle->reset_priv)
> +		return 0; /* initialised already for the first device */
> +
>  	scmi_version_get(handle, SCMI_PROTOCOL_RESET, &version);
> 
>  	dev_dbg(handle->dev, "Reset Version %d.%d\n",
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index a400ea805fc2..b7f92c37c8a4 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -276,6 +276,9 @@ static int scmi_sensors_protocol_init(struct scmi_handle *handle)
>  	u32 version;
>  	struct sensors_info *sinfo;
> 
> +	if (handle->sensor_ops && handle->sensor_priv)
> +		return 0; /* initialised already for the first device */
> +
>  	scmi_version_get(handle, SCMI_PROTOCOL_SENSOR, &version);
> 
>  	dev_dbg(handle->dev, "Sensor Version %d.%d\n",
> --
> 2.17.1
> 

[PATCH 04/15] firmware: arm_scmi: Add names to scmi devices created

[Sudeep Holla]

Now that scmi bus provides option to create named scmi device, let us
create the default devices with names. This will help to add names for
matching to respective drivers and eventually to add multiple devices
and drivers per protocol.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 36 +++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 2952fcd8dd8a..0bbdc7c9eb0f 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -829,6 +829,40 @@ scmi_create_protocol_device(struct device_node *np, struct scmi_info *info,
 	scmi_set_handle(sdev);
 }

+#define MAX_SCMI_DEV_PER_PROTOCOL	2
+struct scmi_prot_devnames {
+	int protocol_id;
+	char *names[MAX_SCMI_DEV_PER_PROTOCOL];
+};
+
+static struct scmi_prot_devnames devnames[] = {
+	{ SCMI_PROTOCOL_POWER,  { "genpd" },},
+	{ SCMI_PROTOCOL_PERF,   { "cpufreq" },},
+	{ SCMI_PROTOCOL_CLOCK,  { "clocks" },},
+	{ SCMI_PROTOCOL_SENSOR, { "hwmon" },},
+	{ SCMI_PROTOCOL_RESET,  { "reset" },},
+};
+
+static inline void
+scmi_create_protocol_devices(struct device_node *np, struct scmi_info *info,
+			     int prot_id)
+{
+	int loop, cnt;
+
+	for (loop = 0; loop < ARRAY_SIZE(devnames); loop++) {
+		if (devnames[loop].protocol_id != prot_id)
+			continue;
+
+		for (cnt = 0; cnt < ARRAY_SIZE(devnames[loop].names); cnt++) {
+			const char *name = devnames[loop].names[cnt];
+
+			if (name)
+				scmi_create_protocol_device(np, info, prot_id,
+							    name);
+		}
+	}
+}
+
 static int scmi_probe(struct platform_device *pdev)
 {
 	int ret;
@@ -897,7 +931,7 @@ static int scmi_probe(struct platform_device *pdev)
 			continue;
 		}

-		scmi_create_protocol_device(child, info, prot_id, NULL);
+		scmi_create_protocol_devices(child, info, prot_id);
 	}

 	return 0;
--
2.17.1

Re: [PATCH 04/15] firmware: arm_scmi: Add names to scmi devices created

[Cristian Marussi]

Hi

On 10/12/2019 14:53, Sudeep Holla wrote:
> Now that scmi bus provides option to create named scmi device, let us
> create the default devices with names. This will help to add names for
> matching to respective drivers and eventually to add multiple devices
> and drivers per protocol.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/driver.c | 36 +++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 2952fcd8dd8a..0bbdc7c9eb0f 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -829,6 +829,40 @@ scmi_create_protocol_device(struct device_node *np, struct scmi_info *info,
>  	scmi_set_handle(sdev);
>  }
> 
> +#define MAX_SCMI_DEV_PER_PROTOCOL	2
> +struct scmi_prot_devnames {
> +	int protocol_id;
> +	char *names[MAX_SCMI_DEV_PER_PROTOCOL];
> +};
> +
> +static struct scmi_prot_devnames devnames[] = {
> +	{ SCMI_PROTOCOL_POWER,  { "genpd" },},
> +	{ SCMI_PROTOCOL_PERF,   { "cpufreq" },},
> +	{ SCMI_PROTOCOL_CLOCK,  { "clocks" },},
> +	{ SCMI_PROTOCOL_SENSOR, { "hwmon" },},
> +	{ SCMI_PROTOCOL_RESET,  { "reset" },},
> +};
> +
> +static inline void
> +scmi_create_protocol_devices(struct device_node *np, struct scmi_info *info,
> +			     int prot_id)
> +{
> +	int loop, cnt;
> +
> +	for (loop = 0; loop < ARRAY_SIZE(devnames); loop++) {
> +		if (devnames[loop].protocol_id != prot_id)
> +			continue;
> +
> +		for (cnt = 0; cnt < ARRAY_SIZE(devnames[loop].names); cnt++) {
> +			const char *name = devnames[loop].names[cnt];
> +
> +			if (name)
> +				scmi_create_protocol_device(np, info, prot_id,
> +							    name);
> +		}
> +	}
> +}
> +
>  static int scmi_probe(struct platform_device *pdev)
>  {
>  	int ret;
> @@ -897,7 +931,7 @@ static int scmi_probe(struct platform_device *pdev)
>  			continue;
>  		}
> 
> -		scmi_create_protocol_device(child, info, prot_id, NULL);
> +		scmi_create_protocol_devices(child, info, prot_id);
>  	}
> 
>  	return 0;
> --
> 2.17.1
> 

I'm a little bit puzzled by the builtin fixed define MAX_SCMI_DEV_PER_PROTOCOL, but
I have not really an alternative solution as of now, so looks good to me.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Cheers

Cristian

[PATCH 05/15] firmware: arm_scmi: Add versions and identifier attributes using dev_groups

[Sudeep Holla]

Platform drivers now have the option to have the platform core create
and remove any needed sysfs attribute files. Using the same, let's add
the scmi firmware and protocol version attributes as well as vendor and
sub-vendor identifiers to sysfs.

It helps to identify the firmware details from the sysfs entries similar
to ARM SCPI implementation.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/driver.c | 47 ++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 0bbdc7c9eb0f..26b2c438bd59 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -979,6 +979,52 @@ static int scmi_remove(struct platform_device *pdev)
 	return ret;
 }

+static ssize_t protocol_version_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct scmi_info *info = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u.%u\n", info->version.major_ver,
+		       info->version.minor_ver);
+}
+static DEVICE_ATTR_RO(protocol_version);
+
+static ssize_t firmware_version_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct scmi_info *info = dev_get_drvdata(dev);
+
+	return sprintf(buf, "0x%x\n", info->version.impl_ver);
+}
+static DEVICE_ATTR_RO(firmware_version);
+
+static ssize_t vendor_id_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct scmi_info *info = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", info->version.vendor_id);
+}
+static DEVICE_ATTR_RO(vendor_id);
+
+static ssize_t sub_vendor_id_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct scmi_info *info = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", info->version.sub_vendor_id);
+}
+static DEVICE_ATTR_RO(sub_vendor_id);
+
+static struct attribute *versions_attrs[] = {
+	&dev_attr_firmware_version.attr,
+	&dev_attr_protocol_version.attr,
+	&dev_attr_vendor_id.attr,
+	&dev_attr_sub_vendor_id.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(versions);
+
 static const struct scmi_desc scmi_generic_desc = {
 	.max_rx_timeout_ms = 30,	/* We may increase this if required */
 	.max_msg = 20,		/* Limited by MBOX_TX_QUEUE_LEN */
@@ -997,6 +1043,7 @@ static struct platform_driver scmi_driver = {
 	.driver = {
 		   .name = "arm-scmi",
 		   .of_match_table = scmi_of_match,
+		   .dev_groups = versions_groups,
 		   },
 	.probe = scmi_probe,
 	.remove = scmi_remove,
--
2.17.1

Re: [PATCH 05/15] firmware: arm_scmi: Add versions and identifier attributes using dev_groups

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> Platform drivers now have the option to have the platform core create
> and remove any needed sysfs attribute files. Using the same, let's add
> the scmi firmware and protocol version attributes as well as vendor and
> sub-vendor identifiers to sysfs.
> 
> It helps to identify the firmware details from the sysfs entries similar
> to ARM SCPI implementation.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/driver.c | 47 ++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index 0bbdc7c9eb0f..26b2c438bd59 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -979,6 +979,52 @@ static int scmi_remove(struct platform_device *pdev)
>  	return ret;
>  }
> 
> +static ssize_t protocol_version_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct scmi_info *info = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u.%u\n", info->version.major_ver,
> +		       info->version.minor_ver);
> +}
> +static DEVICE_ATTR_RO(protocol_version);
> +
> +static ssize_t firmware_version_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct scmi_info *info = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "0x%x\n", info->version.impl_ver);
> +}
> +static DEVICE_ATTR_RO(firmware_version);
> +
> +static ssize_t vendor_id_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct scmi_info *info = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", info->version.vendor_id);
> +}
> +static DEVICE_ATTR_RO(vendor_id);
> +
> +static ssize_t sub_vendor_id_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct scmi_info *info = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", info->version.sub_vendor_id);
> +}
> +static DEVICE_ATTR_RO(sub_vendor_id);
> +
> +static struct attribute *versions_attrs[] = {
> +	&dev_attr_firmware_version.attr,
> +	&dev_attr_protocol_version.attr,
> +	&dev_attr_vendor_id.attr,
> +	&dev_attr_sub_vendor_id.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(versions);
> +
>  static const struct scmi_desc scmi_generic_desc = {
>  	.max_rx_timeout_ms = 30,	/* We may increase this if required */
>  	.max_msg = 20,		/* Limited by MBOX_TX_QUEUE_LEN */
> @@ -997,6 +1043,7 @@ static struct platform_driver scmi_driver = {
>  	.driver = {
>  		   .name = "arm-scmi",
>  		   .of_match_table = scmi_of_match,
> +		   .dev_groups = versions_groups,
>  		   },
>  	.probe = scmi_probe,
>  	.remove = scmi_remove,
> --
> 2.17.1
> 

LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>


Cristian

[PATCH 06/15] firmware: arm_scmi: Update scmi_prot_init_fn_t to use device instead of handle

[Sudeep Holla]

The scmi bus now supports multiple device per protocol. So, in order to
initialise each device and it's attributes, it's better to pass scmi_device
pointer to the protocol initialise function rather than scmi_handle.
scmi_handle can be still fetched from the scmi_device pointer.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/bus.c     | 6 +++---
 drivers/firmware/arm_scmi/clock.c   | 3 ++-
 drivers/firmware/arm_scmi/perf.c    | 3 ++-
 drivers/firmware/arm_scmi/power.c   | 3 ++-
 drivers/firmware/arm_scmi/reset.c   | 3 ++-
 drivers/firmware/arm_scmi/sensors.c | 3 ++-
 include/linux/scmi_protocol.h       | 2 +-
 7 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 3714e6307b05..f619da2634a6 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -51,13 +51,13 @@ static int scmi_dev_match(struct device *dev, struct device_driver *drv)
 	return 0;
 }

-static int scmi_protocol_init(int protocol_id, struct scmi_handle *handle)
+static int scmi_protocol_init(int protocol_id, struct scmi_device *dev)
 {
 	scmi_prot_init_fn_t fn = idr_find(&scmi_protocols, protocol_id);

 	if (unlikely(!fn))
 		return -EINVAL;
-	return fn(handle);
+	return fn(dev);
 }

 static int scmi_dev_probe(struct device *dev)
@@ -74,7 +74,7 @@ static int scmi_dev_probe(struct device *dev)
 	if (!scmi_dev->handle)
 		return -EPROBE_DEFER;

-	ret = scmi_protocol_init(scmi_dev->protocol_id, scmi_dev->handle);
+	ret = scmi_protocol_init(scmi_dev->protocol_id, scmi_dev);
 	if (ret)
 		return ret;

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 922b22aaaf84..6c24eb8a4e68 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -310,11 +310,12 @@ static struct scmi_clk_ops clk_ops = {
 	.disable = scmi_clock_disable,
 };

-static int scmi_clock_protocol_init(struct scmi_handle *handle)
+static int scmi_clock_protocol_init(struct scmi_device *dev)
 {
 	u32 version;
 	int clkid, ret;
 	struct clock_info *cinfo;
+	struct scmi_handle *handle = dev->handle;

 	if (handle->clk_ops && handle->clk_priv)
 		return 0; /* initialised already for the first device */
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 55c2a4c21ccb..4f02bfba98ba 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -704,11 +704,12 @@ static struct scmi_perf_ops perf_ops = {
 	.est_power_get = scmi_dvfs_est_power_get,
 };

-static int scmi_perf_protocol_init(struct scmi_handle *handle)
+static int scmi_perf_protocol_init(struct scmi_device *dev)
 {
 	int domain;
 	u32 version;
 	struct scmi_perf_info *pinfo;
+	struct scmi_handle *handle = dev->handle;

 	if (handle->perf_ops && handle->perf_priv)
 		return 0; /* initialised already for the first device */
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 9a7593238b8f..5a8faa369d82 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -179,11 +179,12 @@ static struct scmi_power_ops power_ops = {
 	.state_get = scmi_power_state_get,
 };

-static int scmi_power_protocol_init(struct scmi_handle *handle)
+static int scmi_power_protocol_init(struct scmi_device *dev)
 {
 	int domain;
 	u32 version;
 	struct scmi_power_info *pinfo;
+	struct scmi_handle *handle = dev->handle;

 	if (handle->power_ops && handle->power_priv)
 		return 0; /* initialised already for the first device */
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 809dc8faee1e..438d74a2c80a 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -189,11 +189,12 @@ static struct scmi_reset_ops reset_ops = {
 	.deassert = scmi_reset_domain_deassert,
 };

-static int scmi_reset_protocol_init(struct scmi_handle *handle)
+static int scmi_reset_protocol_init(struct scmi_device *dev)
 {
 	int domain;
 	u32 version;
 	struct scmi_reset_info *pinfo;
+	struct scmi_handle *handle = dev->handle;

 	if (handle->reset_ops && handle->reset_priv)
 		return 0; /* initialised already for the first device */
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index b7f92c37c8a4..afa51bedfa5d 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -271,10 +271,11 @@ static struct scmi_sensor_ops sensor_ops = {
 	.reading_get = scmi_sensor_reading_get,
 };

-static int scmi_sensors_protocol_init(struct scmi_handle *handle)
+static int scmi_sensors_protocol_init(struct scmi_device *dev)
 {
 	u32 version;
 	struct sensors_info *sinfo;
+	struct scmi_handle *handle = dev->handle;

 	if (handle->sensor_ops && handle->sensor_priv)
 		return 0; /* initialised already for the first device */
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 5c873a59b387..b676825e6eb0 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -316,6 +316,6 @@ static inline void scmi_driver_unregister(struct scmi_driver *driver) {}
 #define module_scmi_driver(__scmi_driver)	\
 	module_driver(__scmi_driver, scmi_register, scmi_unregister)

-typedef int (*scmi_prot_init_fn_t)(struct scmi_handle *);
+typedef int (*scmi_prot_init_fn_t)(struct scmi_device *);
 int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn);
 void scmi_protocol_unregister(int protocol_id);
--
2.17.1

Re: [PATCH 06/15] firmware: arm_scmi: Update scmi_prot_init_fn_t to use device instead of handle

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> The scmi bus now supports multiple device per protocol. So, in order to
> initialise each device and it's attributes, it's better to pass scmi_device
> pointer to the protocol initialise function rather than scmi_handle.
> scmi_handle can be still fetched from the scmi_device pointer.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/bus.c     | 6 +++---
>  drivers/firmware/arm_scmi/clock.c   | 3 ++-
>  drivers/firmware/arm_scmi/perf.c    | 3 ++-
>  drivers/firmware/arm_scmi/power.c   | 3 ++-
>  drivers/firmware/arm_scmi/reset.c   | 3 ++-
>  drivers/firmware/arm_scmi/sensors.c | 3 ++-
>  include/linux/scmi_protocol.h       | 2 +-
>  7 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 3714e6307b05..f619da2634a6 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -51,13 +51,13 @@ static int scmi_dev_match(struct device *dev, struct device_driver *drv)
>  	return 0;
>  }
> 
> -static int scmi_protocol_init(int protocol_id, struct scmi_handle *handle)
> +static int scmi_protocol_init(int protocol_id, struct scmi_device *dev)
>  {
>  	scmi_prot_init_fn_t fn = idr_find(&scmi_protocols, protocol_id);
> 
>  	if (unlikely(!fn))
>  		return -EINVAL;
> -	return fn(handle);
> +	return fn(dev);
>  }
> 

As talked offline, for the same reasons of comments in 03/15, it seems to me
that here we are slipping device specific initialization stuff into the
protocol_init function which should be called once for all for every protocol.

BTW, scmi_dev is used just to extract the handle here, beside version caching
in following patches.

Cheers

Cristian
>  static int scmi_dev_probe(struct device *dev)
> @@ -74,7 +74,7 @@ static int scmi_dev_probe(struct device *dev)
>  	if (!scmi_dev->handle)
>  		return -EPROBE_DEFER;
> 
> -	ret = scmi_protocol_init(scmi_dev->protocol_id, scmi_dev->handle);
> +	ret = scmi_protocol_init(scmi_dev->protocol_id, scmi_dev);
>  	if (ret)
>  		return ret;
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 922b22aaaf84..6c24eb8a4e68 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -310,11 +310,12 @@ static struct scmi_clk_ops clk_ops = {
>  	.disable = scmi_clock_disable,
>  };
> 
> -static int scmi_clock_protocol_init(struct scmi_handle *handle)
> +static int scmi_clock_protocol_init(struct scmi_device *dev)
>  {
>  	u32 version;
>  	int clkid, ret;
>  	struct clock_info *cinfo;
> +	struct scmi_handle *handle = dev->handle;
> 
>  	if (handle->clk_ops && handle->clk_priv)
>  		return 0; /* initialised already for the first device */
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 55c2a4c21ccb..4f02bfba98ba 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -704,11 +704,12 @@ static struct scmi_perf_ops perf_ops = {
>  	.est_power_get = scmi_dvfs_est_power_get,
>  };
> 
> -static int scmi_perf_protocol_init(struct scmi_handle *handle)
> +static int scmi_perf_protocol_init(struct scmi_device *dev)
>  {
>  	int domain;
>  	u32 version;
>  	struct scmi_perf_info *pinfo;
> +	struct scmi_handle *handle = dev->handle;
> 
>  	if (handle->perf_ops && handle->perf_priv)
>  		return 0; /* initialised already for the first device */
> diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
> index 9a7593238b8f..5a8faa369d82 100644
> --- a/drivers/firmware/arm_scmi/power.c
> +++ b/drivers/firmware/arm_scmi/power.c
> @@ -179,11 +179,12 @@ static struct scmi_power_ops power_ops = {
>  	.state_get = scmi_power_state_get,
>  };
> 
> -static int scmi_power_protocol_init(struct scmi_handle *handle)
> +static int scmi_power_protocol_init(struct scmi_device *dev)
>  {
>  	int domain;
>  	u32 version;
>  	struct scmi_power_info *pinfo;
> +	struct scmi_handle *handle = dev->handle;
> 
>  	if (handle->power_ops && handle->power_priv)
>  		return 0; /* initialised already for the first device */
> diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> index 809dc8faee1e..438d74a2c80a 100644
> --- a/drivers/firmware/arm_scmi/reset.c
> +++ b/drivers/firmware/arm_scmi/reset.c
> @@ -189,11 +189,12 @@ static struct scmi_reset_ops reset_ops = {
>  	.deassert = scmi_reset_domain_deassert,
>  };
> 
> -static int scmi_reset_protocol_init(struct scmi_handle *handle)
> +static int scmi_reset_protocol_init(struct scmi_device *dev)
>  {
>  	int domain;
>  	u32 version;
>  	struct scmi_reset_info *pinfo;
> +	struct scmi_handle *handle = dev->handle;
> 
>  	if (handle->reset_ops && handle->reset_priv)
>  		return 0; /* initialised already for the first device */
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index b7f92c37c8a4..afa51bedfa5d 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -271,10 +271,11 @@ static struct scmi_sensor_ops sensor_ops = {
>  	.reading_get = scmi_sensor_reading_get,
>  };
> 
> -static int scmi_sensors_protocol_init(struct scmi_handle *handle)
> +static int scmi_sensors_protocol_init(struct scmi_device *dev)
>  {
>  	u32 version;
>  	struct sensors_info *sinfo;
> +	struct scmi_handle *handle = dev->handle;
> 
>  	if (handle->sensor_ops && handle->sensor_priv)
>  		return 0; /* initialised already for the first device */
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 5c873a59b387..b676825e6eb0 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -316,6 +316,6 @@ static inline void scmi_driver_unregister(struct scmi_driver *driver) {}
>  #define module_scmi_driver(__scmi_driver)	\
>  	module_driver(__scmi_driver, scmi_register, scmi_unregister)
> 
> -typedef int (*scmi_prot_init_fn_t)(struct scmi_handle *);
> +typedef int (*scmi_prot_init_fn_t)(struct scmi_device *);
>  int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn);
>  void scmi_protocol_unregister(int protocol_id);
> --
> 2.17.1
> 

[PATCH 07/15] firmware: arm_scmi: Stash version in protocol init functions

[Sudeep Holla]

In order to avoid querying the individual protocol versions multiple
time with more that one device created for each protocol, we can simple
store the copy in the protocol specific private data and use them whenever
required.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/clock.c   | 2 ++
 drivers/firmware/arm_scmi/perf.c    | 2 ++
 drivers/firmware/arm_scmi/power.c   | 2 ++
 drivers/firmware/arm_scmi/reset.c   | 2 ++
 drivers/firmware/arm_scmi/sensors.c | 2 ++
 5 files changed, 10 insertions(+)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index 6c24eb8a4e68..b567ec03f711 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -65,6 +65,7 @@ struct scmi_clock_set_rate {
 };

 struct clock_info {
+	u32 version;
 	int num_clocks;
 	int max_async_req;
 	atomic_t cur_async_req;
@@ -344,6 +345,7 @@ static int scmi_clock_protocol_init(struct scmi_device *dev)
 			scmi_clock_describe_rates_get(handle, clkid, clk);
 	}

+	cinfo->version = version;
 	handle->clk_ops = &clk_ops;
 	handle->clk_priv = cinfo;

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 4f02bfba98ba..b1de6197f61c 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -145,6 +145,7 @@ struct perf_dom_info {
 };

 struct scmi_perf_info {
+	u32 version;
 	int num_domains;
 	bool power_scale_mw;
 	u64 stats_addr;
@@ -740,6 +741,7 @@ static int scmi_perf_protocol_init(struct scmi_device *dev)
 			scmi_perf_domain_init_fc(handle, domain, &dom->fc_info);
 	}

+	pinfo->version = version;
 	handle->perf_ops = &perf_ops;
 	handle->perf_priv = pinfo;

diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 5a8faa369d82..d11c6cd6bbab 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -50,6 +50,7 @@ struct power_dom_info {
 };

 struct scmi_power_info {
+	u32 version;
 	int num_domains;
 	u64 stats_addr;
 	u32 stats_size;
@@ -211,6 +212,7 @@ static int scmi_power_protocol_init(struct scmi_device *dev)
 		scmi_power_domain_attributes_get(handle, domain, dom);
 	}

+	pinfo->version = version;
 	handle->power_ops = &power_ops;
 	handle->power_priv = pinfo;

diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 438d74a2c80a..dce103781b3f 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -48,6 +48,7 @@ struct reset_dom_info {
 };

 struct scmi_reset_info {
+	u32 version;
 	int num_domains;
 	struct reset_dom_info *dom_info;
 };
@@ -221,6 +222,7 @@ static int scmi_reset_protocol_init(struct scmi_device *dev)
 		scmi_reset_domain_attributes_get(handle, domain, dom);
 	}

+	pinfo->version = version;
 	handle->reset_ops = &reset_ops;
 	handle->reset_priv = pinfo;

diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index afa51bedfa5d..aac0243e2880 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -68,6 +68,7 @@ struct scmi_msg_sensor_reading_get {
 };

 struct sensors_info {
+	u32 version;
 	int num_sensors;
 	int max_requests;
 	u64 reg_addr;
@@ -298,6 +299,7 @@ static int scmi_sensors_protocol_init(struct scmi_device *dev)

 	scmi_sensor_description_get(handle, sinfo);

+	sinfo->version = version;
 	handle->sensor_ops = &sensor_ops;
 	handle->sensor_priv = sinfo;

--
2.17.1

Re: [PATCH 07/15] firmware: arm_scmi: Stash version in protocol init functions

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> In order to avoid querying the individual protocol versions multiple
> time with more that one device created for each protocol, we can simple
> store the copy in the protocol specific private data and use them whenever
> required.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/clock.c   | 2 ++
>  drivers/firmware/arm_scmi/perf.c    | 2 ++
>  drivers/firmware/arm_scmi/power.c   | 2 ++
>  drivers/firmware/arm_scmi/reset.c   | 2 ++
>  drivers/firmware/arm_scmi/sensors.c | 2 ++
>  5 files changed, 10 insertions(+)
> 

LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>


Cristian



> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index 6c24eb8a4e68..b567ec03f711 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -65,6 +65,7 @@ struct scmi_clock_set_rate {
>  };
> 
>  struct clock_info {
> +	u32 version;
>  	int num_clocks;
>  	int max_async_req;
>  	atomic_t cur_async_req;
> @@ -344,6 +345,7 @@ static int scmi_clock_protocol_init(struct scmi_device *dev)
>  			scmi_clock_describe_rates_get(handle, clkid, clk);
>  	}
> 
> +	cinfo->version = version;
>  	handle->clk_ops = &clk_ops;
>  	handle->clk_priv = cinfo;
> 
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 4f02bfba98ba..b1de6197f61c 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -145,6 +145,7 @@ struct perf_dom_info {
>  };
> 
>  struct scmi_perf_info {
> +	u32 version;
>  	int num_domains;
>  	bool power_scale_mw;
>  	u64 stats_addr;
> @@ -740,6 +741,7 @@ static int scmi_perf_protocol_init(struct scmi_device *dev)
>  			scmi_perf_domain_init_fc(handle, domain, &dom->fc_info);
>  	}
> 
> +	pinfo->version = version;
>  	handle->perf_ops = &perf_ops;
>  	handle->perf_priv = pinfo;
> 
> diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
> index 5a8faa369d82..d11c6cd6bbab 100644
> --- a/drivers/firmware/arm_scmi/power.c
> +++ b/drivers/firmware/arm_scmi/power.c
> @@ -50,6 +50,7 @@ struct power_dom_info {
>  };
> 
>  struct scmi_power_info {
> +	u32 version;
>  	int num_domains;
>  	u64 stats_addr;
>  	u32 stats_size;
> @@ -211,6 +212,7 @@ static int scmi_power_protocol_init(struct scmi_device *dev)
>  		scmi_power_domain_attributes_get(handle, domain, dom);
>  	}
> 
> +	pinfo->version = version;
>  	handle->power_ops = &power_ops;
>  	handle->power_priv = pinfo;
> 
> diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> index 438d74a2c80a..dce103781b3f 100644
> --- a/drivers/firmware/arm_scmi/reset.c
> +++ b/drivers/firmware/arm_scmi/reset.c
> @@ -48,6 +48,7 @@ struct reset_dom_info {
>  };
> 
>  struct scmi_reset_info {
> +	u32 version;
>  	int num_domains;
>  	struct reset_dom_info *dom_info;
>  };
> @@ -221,6 +222,7 @@ static int scmi_reset_protocol_init(struct scmi_device *dev)
>  		scmi_reset_domain_attributes_get(handle, domain, dom);
>  	}
> 
> +	pinfo->version = version;
>  	handle->reset_ops = &reset_ops;
>  	handle->reset_priv = pinfo;
> 
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index afa51bedfa5d..aac0243e2880 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -68,6 +68,7 @@ struct scmi_msg_sensor_reading_get {
>  };
> 
>  struct sensors_info {
> +	u32 version;
>  	int num_sensors;
>  	int max_requests;
>  	u64 reg_addr;
> @@ -298,6 +299,7 @@ static int scmi_sensors_protocol_init(struct scmi_device *dev)
> 
>  	scmi_sensor_description_get(handle, sinfo);
> 
> +	sinfo->version = version;
>  	handle->sensor_ops = &sensor_ops;
>  	handle->sensor_priv = sinfo;
> 
> --
> 2.17.1
> 

[PATCH 08/15] firmware: arm_scmi: Add and initialise protocol version to scmi_device structure

[Sudeep Holla]

It's useful to keep track of scmi protocol version in the scmi device
structure along with the protocol id. These can be used to expose the
information to the userspace via bus dev_groups attributes as well.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/clock.c   | 6 +++++-
 drivers/firmware/arm_scmi/perf.c    | 6 +++++-
 drivers/firmware/arm_scmi/power.c   | 6 +++++-
 drivers/firmware/arm_scmi/reset.c   | 6 +++++-
 drivers/firmware/arm_scmi/sensors.c | 6 +++++-
 include/linux/scmi_protocol.h       | 1 +
 6 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index b567ec03f711..b68736ae7f88 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -318,8 +318,11 @@ static int scmi_clock_protocol_init(struct scmi_device *dev)
 	struct clock_info *cinfo;
 	struct scmi_handle *handle = dev->handle;

-	if (handle->clk_ops && handle->clk_priv)
+	if (handle->clk_ops && handle->clk_priv) {
+		cinfo = handle->clk_priv;
+		dev->version = cinfo->version;
 		return 0; /* initialised already for the first device */
+	}

 	scmi_version_get(handle, SCMI_PROTOCOL_CLOCK, &version);

@@ -345,6 +348,7 @@ static int scmi_clock_protocol_init(struct scmi_device *dev)
 			scmi_clock_describe_rates_get(handle, clkid, clk);
 	}

+	dev->version = version;
 	cinfo->version = version;
 	handle->clk_ops = &clk_ops;
 	handle->clk_priv = cinfo;
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index b1de6197f61c..8a02dc453894 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -712,8 +712,11 @@ static int scmi_perf_protocol_init(struct scmi_device *dev)
 	struct scmi_perf_info *pinfo;
 	struct scmi_handle *handle = dev->handle;

-	if (handle->perf_ops && handle->perf_priv)
+	if (handle->perf_ops && handle->perf_priv) {
+		pinfo = handle->perf_priv;
+		dev->version = pinfo->version;
 		return 0; /* initialised already for the first device */
+	}

 	scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version);

@@ -741,6 +744,7 @@ static int scmi_perf_protocol_init(struct scmi_device *dev)
 			scmi_perf_domain_init_fc(handle, domain, &dom->fc_info);
 	}

+	dev->version = version;
 	pinfo->version = version;
 	handle->perf_ops = &perf_ops;
 	handle->perf_priv = pinfo;
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index d11c6cd6bbab..6267111e38e6 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -187,8 +187,11 @@ static int scmi_power_protocol_init(struct scmi_device *dev)
 	struct scmi_power_info *pinfo;
 	struct scmi_handle *handle = dev->handle;

-	if (handle->power_ops && handle->power_priv)
+	if (handle->power_ops && handle->power_priv) {
+		pinfo = handle->power_priv;
+		dev->version = pinfo->version;
 		return 0; /* initialised already for the first device */
+	}

 	scmi_version_get(handle, SCMI_PROTOCOL_POWER, &version);

@@ -212,6 +215,7 @@ static int scmi_power_protocol_init(struct scmi_device *dev)
 		scmi_power_domain_attributes_get(handle, domain, dom);
 	}

+	dev->version = version;
 	pinfo->version = version;
 	handle->power_ops = &power_ops;
 	handle->power_priv = pinfo;
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index dce103781b3f..76f1cee85a06 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -197,8 +197,11 @@ static int scmi_reset_protocol_init(struct scmi_device *dev)
 	struct scmi_reset_info *pinfo;
 	struct scmi_handle *handle = dev->handle;

-	if (handle->reset_ops && handle->reset_priv)
+	if (handle->reset_ops && handle->reset_priv) {
+		pinfo = handle->reset_priv;
+		dev->version = pinfo->version;
 		return 0; /* initialised already for the first device */
+	}

 	scmi_version_get(handle, SCMI_PROTOCOL_RESET, &version);

@@ -222,6 +225,7 @@ static int scmi_reset_protocol_init(struct scmi_device *dev)
 		scmi_reset_domain_attributes_get(handle, domain, dom);
 	}

+	dev->version = version;
 	pinfo->version = version;
 	handle->reset_ops = &reset_ops;
 	handle->reset_priv = pinfo;
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index aac0243e2880..fb3bed4cb171 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -278,8 +278,11 @@ static int scmi_sensors_protocol_init(struct scmi_device *dev)
 	struct sensors_info *sinfo;
 	struct scmi_handle *handle = dev->handle;

-	if (handle->sensor_ops && handle->sensor_priv)
+	if (handle->sensor_ops && handle->sensor_priv) {
+		sinfo = handle->sensor_priv;
+		dev->version = sinfo->version;
 		return 0; /* initialised already for the first device */
+	}

 	scmi_version_get(handle, SCMI_PROTOCOL_SENSOR, &version);

@@ -299,6 +302,7 @@ static int scmi_sensors_protocol_init(struct scmi_device *dev)

 	scmi_sensor_description_get(handle, sinfo);

+	dev->version = version;
 	sinfo->version = version;
 	handle->sensor_ops = &sensor_ops;
 	handle->sensor_priv = sinfo;
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index b676825e6eb0..a863bc0cdf28 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -256,6 +256,7 @@ enum scmi_std_protocol {

 struct scmi_device {
 	u32 id;
+	u32 version;
 	u8 protocol_id;
 	const char *name;
 	struct device dev;
--
2.17.1

Re: [PATCH 08/15] firmware: arm_scmi: Add and initialise protocol version to scmi_device structure

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> It's useful to keep track of scmi protocol version in the scmi device
> structure along with the protocol id. These can be used to expose the
> information to the userspace via bus dev_groups attributes as well.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/clock.c   | 6 +++++-
>  drivers/firmware/arm_scmi/perf.c    | 6 +++++-
>  drivers/firmware/arm_scmi/power.c   | 6 +++++-
>  drivers/firmware/arm_scmi/reset.c   | 6 +++++-
>  drivers/firmware/arm_scmi/sensors.c | 6 +++++-
>  include/linux/scmi_protocol.h       | 1 +
>  6 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index b567ec03f711..b68736ae7f88 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -318,8 +318,11 @@ static int scmi_clock_protocol_init(struct scmi_device *dev)
>  	struct clock_info *cinfo;
>  	struct scmi_handle *handle = dev->handle;
> 
> -	if (handle->clk_ops && handle->clk_priv)
> +	if (handle->clk_ops && handle->clk_priv) {
> +		cinfo = handle->clk_priv;
> +		dev->version = cinfo->version;
>  		return 0; /* initialised already for the first device */
> +	}
> 

This is the device specific init stuff which I would remove from this proto initialization,
which is the reason for this proto_init to be invoked for all devices defined for such proto.

I'd say to move dev->version initialization into the specific scmi_drv->probe
which is called after scmi_protocol_init inside bus:scmi_dev_probe, after having
disabled the proto_init after the first invocation, once the protocol is initialized,
BUT this would result anyway in duplication since you'll have to fill dev->version
from the custom protocol info in each of the related scmi drivers, and that would also mean
delegating to a possible user scmi driver .probe an initialization which is then needed by
the sysfs attribute exposed by the SCMI framework code.

So not a solution.

Cristian


>  	scmi_version_get(handle, SCMI_PROTOCOL_CLOCK, &version);
> 
> @@ -345,6 +348,7 @@ static int scmi_clock_protocol_init(struct scmi_device *dev)
>  			scmi_clock_describe_rates_get(handle, clkid, clk);
>  	}
> 
> +	dev->version = version;
>  	cinfo->version = version;
>  	handle->clk_ops = &clk_ops;
>  	handle->clk_priv = cinfo;
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index b1de6197f61c..8a02dc453894 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -712,8 +712,11 @@ static int scmi_perf_protocol_init(struct scmi_device *dev)
>  	struct scmi_perf_info *pinfo;
>  	struct scmi_handle *handle = dev->handle;
> 
> -	if (handle->perf_ops && handle->perf_priv)
> +	if (handle->perf_ops && handle->perf_priv) {
> +		pinfo = handle->perf_priv;
> +		dev->version = pinfo->version;
>  		return 0; /* initialised already for the first device */
> +	}
> 
>  	scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version);
> 
> @@ -741,6 +744,7 @@ static int scmi_perf_protocol_init(struct scmi_device *dev)
>  			scmi_perf_domain_init_fc(handle, domain, &dom->fc_info);
>  	}
> 
> +	dev->version = version;
>  	pinfo->version = version;
>  	handle->perf_ops = &perf_ops;
>  	handle->perf_priv = pinfo;
> diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
> index d11c6cd6bbab..6267111e38e6 100644
> --- a/drivers/firmware/arm_scmi/power.c
> +++ b/drivers/firmware/arm_scmi/power.c
> @@ -187,8 +187,11 @@ static int scmi_power_protocol_init(struct scmi_device *dev)
>  	struct scmi_power_info *pinfo;
>  	struct scmi_handle *handle = dev->handle;
> 
> -	if (handle->power_ops && handle->power_priv)
> +	if (handle->power_ops && handle->power_priv) {
> +		pinfo = handle->power_priv;
> +		dev->version = pinfo->version;
>  		return 0; /* initialised already for the first device */
> +	}
> 
>  	scmi_version_get(handle, SCMI_PROTOCOL_POWER, &version);
> 
> @@ -212,6 +215,7 @@ static int scmi_power_protocol_init(struct scmi_device *dev)
>  		scmi_power_domain_attributes_get(handle, domain, dom);
>  	}
> 
> +	dev->version = version;
>  	pinfo->version = version;
>  	handle->power_ops = &power_ops;
>  	handle->power_priv = pinfo;
> diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> index dce103781b3f..76f1cee85a06 100644
> --- a/drivers/firmware/arm_scmi/reset.c
> +++ b/drivers/firmware/arm_scmi/reset.c
> @@ -197,8 +197,11 @@ static int scmi_reset_protocol_init(struct scmi_device *dev)
>  	struct scmi_reset_info *pinfo;
>  	struct scmi_handle *handle = dev->handle;
> 
> -	if (handle->reset_ops && handle->reset_priv)
> +	if (handle->reset_ops && handle->reset_priv) {
> +		pinfo = handle->reset_priv;
> +		dev->version = pinfo->version;
>  		return 0; /* initialised already for the first device */
> +	}
> 
>  	scmi_version_get(handle, SCMI_PROTOCOL_RESET, &version);
> 
> @@ -222,6 +225,7 @@ static int scmi_reset_protocol_init(struct scmi_device *dev)
>  		scmi_reset_domain_attributes_get(handle, domain, dom);
>  	}
> 
> +	dev->version = version;
>  	pinfo->version = version;
>  	handle->reset_ops = &reset_ops;
>  	handle->reset_priv = pinfo;
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index aac0243e2880..fb3bed4cb171 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -278,8 +278,11 @@ static int scmi_sensors_protocol_init(struct scmi_device *dev)
>  	struct sensors_info *sinfo;
>  	struct scmi_handle *handle = dev->handle;
> 
> -	if (handle->sensor_ops && handle->sensor_priv)
> +	if (handle->sensor_ops && handle->sensor_priv) {
> +		sinfo = handle->sensor_priv;
> +		dev->version = sinfo->version;
>  		return 0; /* initialised already for the first device */
> +	}
> 
>  	scmi_version_get(handle, SCMI_PROTOCOL_SENSOR, &version);
> 
> @@ -299,6 +302,7 @@ static int scmi_sensors_protocol_init(struct scmi_device *dev)
> 
>  	scmi_sensor_description_get(handle, sinfo);
> 
> +	dev->version = version;
>  	sinfo->version = version;
>  	handle->sensor_ops = &sensor_ops;
>  	handle->sensor_priv = sinfo;
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index b676825e6eb0..a863bc0cdf28 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -256,6 +256,7 @@ enum scmi_std_protocol {
> 
>  struct scmi_device {
>  	u32 id;
> +	u32 version;
>  	u8 protocol_id;
>  	const char *name;
>  	struct device dev;
> --
> 2.17.1
> 

Re: [PATCH 08/15] firmware: arm_scmi: Add and initialise protocol version to scmi_device structure

[Sudeep Holla]

On Wed, Dec 11, 2019 at 06:06:50PM +0000, Cristian Marussi wrote:
> On 10/12/2019 14:53, Sudeep Holla wrote:
> > It's useful to keep track of scmi protocol version in the scmi device
> > structure along with the protocol id. These can be used to expose the
> > information to the userspace via bus dev_groups attributes as well.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/clock.c   | 6 +++++-
> >  drivers/firmware/arm_scmi/perf.c    | 6 +++++-
> >  drivers/firmware/arm_scmi/power.c   | 6 +++++-
> >  drivers/firmware/arm_scmi/reset.c   | 6 +++++-
> >  drivers/firmware/arm_scmi/sensors.c | 6 +++++-
> >  include/linux/scmi_protocol.h       | 1 +
> >  6 files changed, 26 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> > index b567ec03f711..b68736ae7f88 100644
> > --- a/drivers/firmware/arm_scmi/clock.c
> > +++ b/drivers/firmware/arm_scmi/clock.c
> > @@ -318,8 +318,11 @@ static int scmi_clock_protocol_init(struct scmi_device *dev)
> >  	struct clock_info *cinfo;
> >  	struct scmi_handle *handle = dev->handle;
> >
> > -	if (handle->clk_ops && handle->clk_priv)
> > +	if (handle->clk_ops && handle->clk_priv) {
> > +		cinfo = handle->clk_priv;
> > +		dev->version = cinfo->version;
> >  		return 0; /* initialised already for the first device */
> > +	}
> >
>
> This is the device specific init stuff which I would remove from this proto
> initialization, which is the reason for this proto_init to be invoked for
> all devices defined for such proto.
>

Agreed, this is something I could come up with quickly, I have to think about
this more for sure.

> I'd say to move dev->version initialization into the specific
> scmi_drv->probe which is called after scmi_protocol_init inside
> bus:scmi_dev_probe, after having disabled the proto_init after the first
> invocation, once the protocol is initialized, BUT this would result anyway
> in duplication since you'll have to fill dev->version from the custom
> protocol info in each of the related scmi drivers, and that would also mean
> delegating to a possible user scmi driver .probe an initialization which is
> then needed by the sysfs attribute exposed by the SCMI framework code.
>
I am trying to avoid that as it's just version and we should be able to
manage this in the scmi_bus layer. I agree what we have in these patches
are not so pretty.

Anyways, thanks a lot for all the review.

--
Regards,
Sudeep

[PATCH 09/15] firmware: arm_scmi: Add scmi protocol version and id device attributes

[Sudeep Holla]

Linux kernel bus driver management layer provides way to add set of
default attributes of the devices on the bus. Using the same, let's add
the scmi per protocol version and id attributes to the sysfs.

It helps to identify the individual protocol details from the sysfs
entries similar to the SCMI protocol and firmware version.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/bus.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index f619da2634a6..ed0ed02f7158 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -92,11 +92,38 @@ static int scmi_dev_remove(struct device *dev)
 	return 0;
 }

+static ssize_t protocol_version_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	struct scmi_device *scmi_dev = to_scmi_dev(dev);
+
+	return sprintf(buf, "%u.%u\n", PROTOCOL_REV_MAJOR(scmi_dev->version),
+		       PROTOCOL_REV_MINOR(scmi_dev->version));
+}
+static DEVICE_ATTR_RO(protocol_version);
+
+static ssize_t protocol_id_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	struct scmi_device *scmi_dev = to_scmi_dev(dev);
+
+	return sprintf(buf, "%u\n", scmi_dev->protocol_id);
+}
+static DEVICE_ATTR_RO(protocol_id);
+
+static struct attribute *versions_attrs[] = {
+	&dev_attr_protocol_version.attr,
+	&dev_attr_protocol_id.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(versions);
+
 static struct bus_type scmi_bus_type = {
 	.name =	"scmi_protocol",
 	.match = scmi_dev_match,
 	.probe = scmi_dev_probe,
 	.remove = scmi_dev_remove,
+	.dev_groups = versions_groups,
 };

 int scmi_driver_register(struct scmi_driver *driver, struct module *owner,
--
2.17.1

Re: [PATCH 09/15] firmware: arm_scmi: Add scmi protocol version and id device attributes

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> Linux kernel bus driver management layer provides way to add set of
> default attributes of the devices on the bus. Using the same, let's add
> the scmi per protocol version and id attributes to the sysfs.
> 
> It helps to identify the individual protocol details from the sysfs
> entries similar to the SCMI protocol and firmware version.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/bus.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index f619da2634a6..ed0ed02f7158 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -92,11 +92,38 @@ static int scmi_dev_remove(struct device *dev)
>  	return 0;
>  }
> 
> +static ssize_t protocol_version_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	struct scmi_device *scmi_dev = to_scmi_dev(dev);
> +
> +	return sprintf(buf, "%u.%u\n", PROTOCOL_REV_MAJOR(scmi_dev->version),
> +		       PROTOCOL_REV_MINOR(scmi_dev->version));
> +}
> +static DEVICE_ATTR_RO(protocol_version);
> +

Similar issue related to proto/device mixup as said.
Here bus exposes sysfs attributes depending on an scmi_dev

Cristian


> +static ssize_t protocol_id_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	struct scmi_device *scmi_dev = to_scmi_dev(dev);
> +
> +	return sprintf(buf, "%u\n", scmi_dev->protocol_id);
> +}
> +static DEVICE_ATTR_RO(protocol_id);
> +
> +static struct attribute *versions_attrs[] = {
> +	&dev_attr_protocol_version.attr,
> +	&dev_attr_protocol_id.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(versions);
> +
>  static struct bus_type scmi_bus_type = {
>  	.name =	"scmi_protocol",
>  	.match = scmi_dev_match,
>  	.probe = scmi_dev_probe,
>  	.remove = scmi_dev_remove,
> +	.dev_groups = versions_groups,
>  };
> 
>  int scmi_driver_register(struct scmi_driver *driver, struct module *owner,
> --
> 2.17.1
> 

[PATCH 10/15] firmware: arm_scmi: Drop logging individual scmi protocol version

[Sudeep Holla]

SCMI firmware and individual protocol versions and other attributes are
now exposed as device attributes through sysfs entries. These debug logs
can be dropped as the same information is available through sysfs.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/clock.c   | 3 ---
 drivers/firmware/arm_scmi/perf.c    | 3 ---
 drivers/firmware/arm_scmi/power.c   | 3 ---
 drivers/firmware/arm_scmi/reset.c   | 3 ---
 drivers/firmware/arm_scmi/sensors.c | 3 ---
 5 files changed, 15 deletions(-)

diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
index b68736ae7f88..ce8cbefb0aa6 100644
--- a/drivers/firmware/arm_scmi/clock.c
+++ b/drivers/firmware/arm_scmi/clock.c
@@ -326,9 +326,6 @@ static int scmi_clock_protocol_init(struct scmi_device *dev)

 	scmi_version_get(handle, SCMI_PROTOCOL_CLOCK, &version);

-	dev_dbg(handle->dev, "Clock Version %d.%d\n",
-		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
-
 	cinfo = devm_kzalloc(handle->dev, sizeof(*cinfo), GFP_KERNEL);
 	if (!cinfo)
 		return -ENOMEM;
diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 8a02dc453894..2ad3bc792692 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -720,9 +720,6 @@ static int scmi_perf_protocol_init(struct scmi_device *dev)

 	scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version);

-	dev_dbg(handle->dev, "Performance Version %d.%d\n",
-		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
-
 	pinfo = devm_kzalloc(handle->dev, sizeof(*pinfo), GFP_KERNEL);
 	if (!pinfo)
 		return -ENOMEM;
diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
index 6267111e38e6..29d72fa7d085 100644
--- a/drivers/firmware/arm_scmi/power.c
+++ b/drivers/firmware/arm_scmi/power.c
@@ -195,9 +195,6 @@ static int scmi_power_protocol_init(struct scmi_device *dev)

 	scmi_version_get(handle, SCMI_PROTOCOL_POWER, &version);

-	dev_dbg(handle->dev, "Power Version %d.%d\n",
-		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
-
 	pinfo = devm_kzalloc(handle->dev, sizeof(*pinfo), GFP_KERNEL);
 	if (!pinfo)
 		return -ENOMEM;
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
index 76f1cee85a06..a49155628ccf 100644
--- a/drivers/firmware/arm_scmi/reset.c
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -205,9 +205,6 @@ static int scmi_reset_protocol_init(struct scmi_device *dev)

 	scmi_version_get(handle, SCMI_PROTOCOL_RESET, &version);

-	dev_dbg(handle->dev, "Reset Version %d.%d\n",
-		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
-
 	pinfo = devm_kzalloc(handle->dev, sizeof(*pinfo), GFP_KERNEL);
 	if (!pinfo)
 		return -ENOMEM;
diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
index fb3bed4cb171..61e12f2fb587 100644
--- a/drivers/firmware/arm_scmi/sensors.c
+++ b/drivers/firmware/arm_scmi/sensors.c
@@ -286,9 +286,6 @@ static int scmi_sensors_protocol_init(struct scmi_device *dev)

 	scmi_version_get(handle, SCMI_PROTOCOL_SENSOR, &version);

-	dev_dbg(handle->dev, "Sensor Version %d.%d\n",
-		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
-
 	sinfo = devm_kzalloc(handle->dev, sizeof(*sinfo), GFP_KERNEL);
 	if (!sinfo)
 		return -ENOMEM;
--
2.17.1

Re: [PATCH 10/15] firmware: arm_scmi: Drop logging individual scmi protocol version

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> SCMI firmware and individual protocol versions and other attributes are
> now exposed as device attributes through sysfs entries. These debug logs
> can be dropped as the same information is available through sysfs.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/clock.c   | 3 ---
>  drivers/firmware/arm_scmi/perf.c    | 3 ---
>  drivers/firmware/arm_scmi/power.c   | 3 ---
>  drivers/firmware/arm_scmi/reset.c   | 3 ---
>  drivers/firmware/arm_scmi/sensors.c | 3 ---
>  5 files changed, 15 deletions(-)
> 

LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>


Cristian


> diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c
> index b68736ae7f88..ce8cbefb0aa6 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -326,9 +326,6 @@ static int scmi_clock_protocol_init(struct scmi_device *dev)
> 
>  	scmi_version_get(handle, SCMI_PROTOCOL_CLOCK, &version);
> 
> -	dev_dbg(handle->dev, "Clock Version %d.%d\n",
> -		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> -
>  	cinfo = devm_kzalloc(handle->dev, sizeof(*cinfo), GFP_KERNEL);
>  	if (!cinfo)
>  		return -ENOMEM;
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 8a02dc453894..2ad3bc792692 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -720,9 +720,6 @@ static int scmi_perf_protocol_init(struct scmi_device *dev)
> 
>  	scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version);
> 
> -	dev_dbg(handle->dev, "Performance Version %d.%d\n",
> -		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> -
>  	pinfo = devm_kzalloc(handle->dev, sizeof(*pinfo), GFP_KERNEL);
>  	if (!pinfo)
>  		return -ENOMEM;
> diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
> index 6267111e38e6..29d72fa7d085 100644
> --- a/drivers/firmware/arm_scmi/power.c
> +++ b/drivers/firmware/arm_scmi/power.c
> @@ -195,9 +195,6 @@ static int scmi_power_protocol_init(struct scmi_device *dev)
> 
>  	scmi_version_get(handle, SCMI_PROTOCOL_POWER, &version);
> 
> -	dev_dbg(handle->dev, "Power Version %d.%d\n",
> -		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> -
>  	pinfo = devm_kzalloc(handle->dev, sizeof(*pinfo), GFP_KERNEL);
>  	if (!pinfo)
>  		return -ENOMEM;
> diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
> index 76f1cee85a06..a49155628ccf 100644
> --- a/drivers/firmware/arm_scmi/reset.c
> +++ b/drivers/firmware/arm_scmi/reset.c
> @@ -205,9 +205,6 @@ static int scmi_reset_protocol_init(struct scmi_device *dev)
> 
>  	scmi_version_get(handle, SCMI_PROTOCOL_RESET, &version);
> 
> -	dev_dbg(handle->dev, "Reset Version %d.%d\n",
> -		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> -
>  	pinfo = devm_kzalloc(handle->dev, sizeof(*pinfo), GFP_KERNEL);
>  	if (!pinfo)
>  		return -ENOMEM;
> diff --git a/drivers/firmware/arm_scmi/sensors.c b/drivers/firmware/arm_scmi/sensors.c
> index fb3bed4cb171..61e12f2fb587 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -286,9 +286,6 @@ static int scmi_sensors_protocol_init(struct scmi_device *dev)
> 
>  	scmi_version_get(handle, SCMI_PROTOCOL_SENSOR, &version);
> 
> -	dev_dbg(handle->dev, "Sensor Version %d.%d\n",
> -		PROTOCOL_REV_MAJOR(version), PROTOCOL_REV_MINOR(version));
> -
>  	sinfo = devm_kzalloc(handle->dev, sizeof(*sinfo), GFP_KERNEL);
>  	if (!sinfo)
>  		return -ENOMEM;
> --
> 2.17.1
> 

[PATCH 11/15] firmware: arm_scmi: Match scmi device by both name and protocol id

[Sudeep Holla]

The scmi bus now has support to match the driver with devices not only
based on their protocol id but also based on their device name if one is
available. This was added to cater the need to support multiple devices
and drivers for the same protocol.

Let us add the name "genpd" to scmi_device_id table in the driver so
that in matches only with device with the same name and protocol id
SCMI_PROTOCOL_POWER.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/scmi_pm_domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/scmi_pm_domain.c b/drivers/firmware/arm_scmi/scmi_pm_domain.c
index 87f737e01473..bafbfe358f97 100644
--- a/drivers/firmware/arm_scmi/scmi_pm_domain.c
+++ b/drivers/firmware/arm_scmi/scmi_pm_domain.c
@@ -112,7 +112,7 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
 }

 static const struct scmi_device_id scmi_id_table[] = {
-	{ SCMI_PROTOCOL_POWER },
+	{ SCMI_PROTOCOL_POWER, "genpd" },
 	{ },
 };
 MODULE_DEVICE_TABLE(scmi, scmi_id_table);
--
2.17.1

Re: [PATCH 11/15] firmware: arm_scmi: Match scmi device by both name and protocol id

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> The scmi bus now has support to match the driver with devices not only
> based on their protocol id but also based on their device name if one is
> available. This was added to cater the need to support multiple devices
> and drivers for the same protocol.
> 
> Let us add the name "genpd" to scmi_device_id table in the driver so
> that in matches only with device with the same name and protocol id
> SCMI_PROTOCOL_POWER.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

>  drivers/firmware/arm_scmi/scmi_pm_domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/scmi_pm_domain.c b/drivers/firmware/arm_scmi/scmi_pm_domain.c
> index 87f737e01473..bafbfe358f97 100644
> --- a/drivers/firmware/arm_scmi/scmi_pm_domain.c
> +++ b/drivers/firmware/arm_scmi/scmi_pm_domain.c
> @@ -112,7 +112,7 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
>  }
> 
>  static const struct scmi_device_id scmi_id_table[] = {
> -	{ SCMI_PROTOCOL_POWER },
> +	{ SCMI_PROTOCOL_POWER, "genpd" },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> --
> 2.17.1
> 

Re: [PATCH 11/15] firmware: arm_scmi: Match scmi device by both name and protocol id

[Cristian Marussi]

On 10/12/2019 14:53, Sudeep Holla wrote:
> The scmi bus now has support to match the driver with devices not only
> based on their protocol id but also based on their device name if one is
> available. This was added to cater the need to support multiple devices
> and drivers for the same protocol.
> 
> Let us add the name "genpd" to scmi_device_id table in the driver so
> that in matches only with device with the same name and protocol id
> SCMI_PROTOCOL_POWER.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/scmi_pm_domain.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/arm_scmi/scmi_pm_domain.c b/drivers/firmware/arm_scmi/scmi_pm_domain.c
> index 87f737e01473..bafbfe358f97 100644
> --- a/drivers/firmware/arm_scmi/scmi_pm_domain.c
> +++ b/drivers/firmware/arm_scmi/scmi_pm_domain.c
> @@ -112,7 +112,7 @@ static int scmi_pm_domain_probe(struct scmi_device *sdev)
>  }
> 
>  static const struct scmi_device_id scmi_id_table[] = {
> -	{ SCMI_PROTOCOL_POWER },
> +	{ SCMI_PROTOCOL_POWER, "genpd" },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> --
> 2.17.1
> 
LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>


Cristian

[PATCH 12/15] clk: scmi: Match scmi device by both name and protocol id

[Sudeep Holla]

The scmi bus now has support to match the driver with devices not only
based on their protocol id but also based on their device name if one is
available. This was added to cater the need to support multiple devices
and drivers for the same protocol.

Let us add the name "clocks" to scmi_device_id table in the driver so
that in matches only with device with the same name and protocol id
SCMI_PROTOCOL_CLOCK.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/clk/clk-scmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c
index 886f7c5df51a..c491f5de0f3f 100644
--- a/drivers/clk/clk-scmi.c
+++ b/drivers/clk/clk-scmi.c
@@ -176,7 +176,7 @@ static int scmi_clocks_probe(struct scmi_device *sdev)
 }

 static const struct scmi_device_id scmi_id_table[] = {
-	{ SCMI_PROTOCOL_CLOCK },
+	{ SCMI_PROTOCOL_CLOCK, "clocks" },
 	{ },
 };
 MODULE_DEVICE_TABLE(scmi, scmi_id_table);
--
2.17.1

Re: [PATCH 12/15] clk: scmi: Match scmi device by both name and protocol id

[Stephen Boyd]

Quoting Sudeep Holla (2019-12-10 06:53:42)
> The scmi bus now has support to match the driver with devices not only
> based on their protocol id but also based on their device name if one is
> available. This was added to cater the need to support multiple devices
> and drivers for the same protocol.
> 
> Let us add the name "clocks" to scmi_device_id table in the driver so
> that in matches only with device with the same name and protocol id
> SCMI_PROTOCOL_CLOCK.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Acked-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 13/15] cpufreq: scmi: Match scmi device by both name and protocol id

[Sudeep Holla]

The scmi bus now has support to match the driver with devices not only
based on their protocol id but also based on their device name if one is
available. This was added to cater the need to support multiple devices
and drivers for the same protocol.

Let us add the name "cpufreq" to scmi_device_id table in the driver so
that in matches only with device with the same name and protocol id
SCMI_PROTOCOL_PERF. This will help to add "devfreq" device/driver.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/cpufreq/scmi-cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
index e6182c89df79..61623e2ff149 100644
--- a/drivers/cpufreq/scmi-cpufreq.c
+++ b/drivers/cpufreq/scmi-cpufreq.c
@@ -261,7 +261,7 @@ static void scmi_cpufreq_remove(struct scmi_device *sdev)
 }

 static const struct scmi_device_id scmi_id_table[] = {
-	{ SCMI_PROTOCOL_PERF },
+	{ SCMI_PROTOCOL_PERF, "cpufreq" },
 	{ },
 };
 MODULE_DEVICE_TABLE(scmi, scmi_id_table);
--
2.17.1

Re: [PATCH 13/15] cpufreq: scmi: Match scmi device by both name and protocol id

[Viresh Kumar]

On 10-12-19, 14:53, Sudeep Holla wrote:
> The scmi bus now has support to match the driver with devices not only
> based on their protocol id but also based on their device name if one is
> available. This was added to cater the need to support multiple devices
> and drivers for the same protocol.
> 
> Let us add the name "cpufreq" to scmi_device_id table in the driver so
> that in matches only with device with the same name and protocol id
> SCMI_PROTOCOL_PERF. This will help to add "devfreq" device/driver.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/cpufreq/scmi-cpufreq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> index e6182c89df79..61623e2ff149 100644
> --- a/drivers/cpufreq/scmi-cpufreq.c
> +++ b/drivers/cpufreq/scmi-cpufreq.c
> @@ -261,7 +261,7 @@ static void scmi_cpufreq_remove(struct scmi_device *sdev)
>  }
> 
>  static const struct scmi_device_id scmi_id_table[] = {
> -	{ SCMI_PROTOCOL_PERF },
> +	{ SCMI_PROTOCOL_PERF, "cpufreq" },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(scmi, scmi_id_table);

Applied. Thanks.

-- 
viresh

Re: [PATCH 13/15] cpufreq: scmi: Match scmi device by both name and protocol id

[Sudeep Holla]

On Wed, Dec 11, 2019 at 08:09:09AM +0530, Viresh Kumar wrote:
> On 10-12-19, 14:53, Sudeep Holla wrote:
> > The scmi bus now has support to match the driver with devices not only
> > based on their protocol id but also based on their device name if one is
> > available. This was added to cater the need to support multiple devices
> > and drivers for the same protocol.
> >
> > Let us add the name "cpufreq" to scmi_device_id table in the driver so
> > that in matches only with device with the same name and protocol id
> > SCMI_PROTOCOL_PERF. This will help to add "devfreq" device/driver.
> >
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Cc: linux-pm@vger.kernel.org
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/cpufreq/scmi-cpufreq.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> > index e6182c89df79..61623e2ff149 100644
> > --- a/drivers/cpufreq/scmi-cpufreq.c
> > +++ b/drivers/cpufreq/scmi-cpufreq.c
> > @@ -261,7 +261,7 @@ static void scmi_cpufreq_remove(struct scmi_device *sdev)
> >  }
> >
> >  static const struct scmi_device_id scmi_id_table[] = {
> > -	{ SCMI_PROTOCOL_PERF },
> > +	{ SCMI_PROTOCOL_PERF, "cpufreq" },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(scmi, scmi_id_table);
>
> Applied. Thanks.

This will break the build without patch 1/15, so preferably must go as
part of the series. Sorry for not missing to specify that detail.

--
Regards,
Sudeep

Re: [PATCH 13/15] cpufreq: scmi: Match scmi device by both name and protocol id

[Viresh Kumar]

On 11-12-19, 10:13, Sudeep Holla wrote:
> On Wed, Dec 11, 2019 at 08:09:09AM +0530, Viresh Kumar wrote:
> > On 10-12-19, 14:53, Sudeep Holla wrote:
> > > The scmi bus now has support to match the driver with devices not only
> > > based on their protocol id but also based on their device name if one is
> > > available. This was added to cater the need to support multiple devices
> > > and drivers for the same protocol.
> > >
> > > Let us add the name "cpufreq" to scmi_device_id table in the driver so
> > > that in matches only with device with the same name and protocol id
> > > SCMI_PROTOCOL_PERF. This will help to add "devfreq" device/driver.
> > >
> > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > > Cc: linux-pm@vger.kernel.org
> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > > ---
> > >  drivers/cpufreq/scmi-cpufreq.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c
> > > index e6182c89df79..61623e2ff149 100644
> > > --- a/drivers/cpufreq/scmi-cpufreq.c
> > > +++ b/drivers/cpufreq/scmi-cpufreq.c
> > > @@ -261,7 +261,7 @@ static void scmi_cpufreq_remove(struct scmi_device *sdev)
> > >  }
> > >
> > >  static const struct scmi_device_id scmi_id_table[] = {
> > > -	{ SCMI_PROTOCOL_PERF },
> > > +	{ SCMI_PROTOCOL_PERF, "cpufreq" },
> > >  	{ },
> > >  };
> > >  MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> >
> > Applied. Thanks.
> 
> This will break the build without patch 1/15, so preferably must go as
> part of the series. Sorry for not missing to specify that detail.

Dropped now.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[PATCH 14/15] hwmon: (scmi-hwmon) Match scmi device by both name and protocol id

[Sudeep Holla]

The scmi bus now has support to match the driver with devices not only
based on their protocol id but also based on their device name if one is
available. This was added to cater the need to support multiple devices
and drivers for the same protocol.

Let us add the name "hwmon" to scmi_device_id table in the driver so
that in matches only with device with the same name and protocol id
SCMI_PROTOCOL_SENSOR. This will help to add IIO support in parallel if
needed.

Cc: Jean Delvare <jdelvare@suse.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/hwmon/scmi-hwmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
index 8a7732c0bef3..286d3cfda7de 100644
--- a/drivers/hwmon/scmi-hwmon.c
+++ b/drivers/hwmon/scmi-hwmon.c
@@ -259,7 +259,7 @@ static int scmi_hwmon_probe(struct scmi_device *sdev)
 }

 static const struct scmi_device_id scmi_id_table[] = {
-	{ SCMI_PROTOCOL_SENSOR },
+	{ SCMI_PROTOCOL_SENSOR, "hwmon" },
 	{ },
 };
 MODULE_DEVICE_TABLE(scmi, scmi_id_table);
--
2.17.1

Re: [PATCH 14/15] hwmon: (scmi-hwmon) Match scmi device by both name and protocol id

[Guenter Roeck]

On Tue, Dec 10, 2019 at 02:53:44PM +0000, Sudeep Holla wrote:
> The scmi bus now has support to match the driver with devices not only
> based on their protocol id but also based on their device name if one is
> available. This was added to cater the need to support multiple devices
> and drivers for the same protocol.
> 
> Let us add the name "hwmon" to scmi_device_id table in the driver so
> that in matches only with device with the same name and protocol id
> SCMI_PROTOCOL_SENSOR. This will help to add IIO support in parallel if
> needed.

If you are planning to re-implement the driver as iio driver, it would
make more sense to drop the hwmon driver entirely and use the iio->hwmon
bridge to access the sensors as hwmon devices if needed.

Guenter

> 
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: linux-hwmon@vger.kernel.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/hwmon/scmi-hwmon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> index 8a7732c0bef3..286d3cfda7de 100644
> --- a/drivers/hwmon/scmi-hwmon.c
> +++ b/drivers/hwmon/scmi-hwmon.c
> @@ -259,7 +259,7 @@ static int scmi_hwmon_probe(struct scmi_device *sdev)
>  }
> 
>  static const struct scmi_device_id scmi_id_table[] = {
> -	{ SCMI_PROTOCOL_SENSOR },
> +	{ SCMI_PROTOCOL_SENSOR, "hwmon" },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> --
> 2.17.1
> 

Re: [PATCH 14/15] hwmon: (scmi-hwmon) Match scmi device by both name and protocol id

[Sudeep Holla]

On Tue, Dec 10, 2019 at 10:06:43AM -0800, Guenter Roeck wrote:
> On Tue, Dec 10, 2019 at 02:53:44PM +0000, Sudeep Holla wrote:
> > The scmi bus now has support to match the driver with devices not only
> > based on their protocol id but also based on their device name if one is
> > available. This was added to cater the need to support multiple devices
> > and drivers for the same protocol.
> >
> > Let us add the name "hwmon" to scmi_device_id table in the driver so
> > that in matches only with device with the same name and protocol id
> > SCMI_PROTOCOL_SENSOR. This will help to add IIO support in parallel if
> > needed.
>
> If you are planning to re-implement the driver as iio driver, it would
> make more sense to drop the hwmon driver entirely and use the iio->hwmon
> bridge to access the sensors as hwmon devices if needed.
>

Ah, does it provides the same interface as hwmon to userspace ? Sorry but
I haven't spent much time looking at IIO yet, but since there are similar
needs to share protocol between subsystems in the kernel, this was just
an example that I listed as recently some requirement to add IIO SCMI
support had come up. If we can achieve hwmon kind of interface with iio->hwmon,
we should do that. We have other examples like devfreq and cpufreq, genpd
and regulators.

This patch is optional at least as of now (but good to have for completeness),
if the driver provides no name, we just match on protocol id only.

--
Regards,
Sudeep

[PATCH 15/15] reset: reset-scmi: Match scmi device by both name and protocol id

[Sudeep Holla]

The scmi bus now has support to match the driver with devices not only
based on their protocol id but also based on their device name if one is
available. This was added to cater the need to support multiple devices
and drivers for the same protocol.

Let us add the name "reset" to scmi_device_id table in the driver so
that in matches only with device with the same name and protocol id
SCMI_PROTOCOL_RESET.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/reset/reset-scmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/reset/reset-scmi.c b/drivers/reset/reset-scmi.c
index b46df80ec6c3..8d3a858e3b19 100644
--- a/drivers/reset/reset-scmi.c
+++ b/drivers/reset/reset-scmi.c
@@ -108,7 +108,7 @@ static int scmi_reset_probe(struct scmi_device *sdev)
 }

 static const struct scmi_device_id scmi_id_table[] = {
-	{ SCMI_PROTOCOL_RESET },
+	{ SCMI_PROTOCOL_RESET, "reset" },
 	{ },
 };
 MODULE_DEVICE_TABLE(scmi, scmi_id_table);
--
2.17.1

Re: [PATCH 15/15] reset: reset-scmi: Match scmi device by both name and protocol id

[Philipp Zabel]

Hi Sudeep,

On Tue, 2019-12-10 at 14:53 +0000, Sudeep Holla wrote:
> The scmi bus now has support to match the driver with devices not only
> based on their protocol id but also based on their device name if one is
> available. This was added to cater the need to support multiple devices
> and drivers for the same protocol.
> 
> Let us add the name "reset" to scmi_device_id table in the driver so
> that in matches only with device with the same name and protocol id
> SCMI_PROTOCOL_RESET.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/reset/reset-scmi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/reset/reset-scmi.c b/drivers/reset/reset-scmi.c
> index b46df80ec6c3..8d3a858e3b19 100644
> --- a/drivers/reset/reset-scmi.c
> +++ b/drivers/reset/reset-scmi.c
> @@ -108,7 +108,7 @@ static int scmi_reset_probe(struct scmi_device *sdev)
>  }
> 
>  static const struct scmi_device_id scmi_id_table[] = {
> -	{ SCMI_PROTOCOL_RESET },
> +	{ SCMI_PROTOCOL_RESET, "reset" },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> --
> 2.17.1

I can't speak to the correctness of this approach, but in case the rest
of the series passes review, this patch is

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

to be merged together with the other patches.

regards
Philipp

Re: [PATCH 15/15] reset: reset-scmi: Match scmi device by both name and protocol id

[Sudeep Holla]

On Wed, Dec 11, 2019 at 10:51:41AM +0100, Philipp Zabel wrote:
> Hi Sudeep,
>
> On Tue, 2019-12-10 at 14:53 +0000, Sudeep Holla wrote:
> > The scmi bus now has support to match the driver with devices not only
> > based on their protocol id but also based on their device name if one is
> > available. This was added to cater the need to support multiple devices
> > and drivers for the same protocol.
> >
> > Let us add the name "reset" to scmi_device_id table in the driver so
> > that in matches only with device with the same name and protocol id
> > SCMI_PROTOCOL_RESET.
> >
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/reset/reset-scmi.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/reset/reset-scmi.c b/drivers/reset/reset-scmi.c
> > index b46df80ec6c3..8d3a858e3b19 100644
> > --- a/drivers/reset/reset-scmi.c
> > +++ b/drivers/reset/reset-scmi.c
> > @@ -108,7 +108,7 @@ static int scmi_reset_probe(struct scmi_device *sdev)
> >  }
> >
> >  static const struct scmi_device_id scmi_id_table[] = {
> > -	{ SCMI_PROTOCOL_RESET },
> > +	{ SCMI_PROTOCOL_RESET, "reset" },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> > --
> > 2.17.1
>
> I can't speak to the correctness of this approach, but in case the rest
> of the series passes review, this patch is
>

I understand that and completely depends on the review of the patches
implementing it.

> Acked-by: Philipp Zabel <p.zabel@pengutronix.de>
>
> to be merged together with the other patches.
>

Thanks.

--
Regards,
Sudeep