From ee7a9c9f67c59008b330deff2762bd8cf1407eec Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Fri, 21 Dec 2018 18:08:08 +0000 Subject: [PATCH 01/13] firmware: arm_scmi: Add support for multiple device per protocol 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. Reviewed-by: Cristian Marussi Signed-off-by: Sudeep Holla --- 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 @@ put_dev: 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 { From 11040889afe3c0b084de0de1782534d529493e9b Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Wed, 6 Nov 2019 11:32:05 +0000 Subject: [PATCH 02/13] firmware: arm_scmi: Skip scmi mbox channel setup for addtional devices 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. Reviewed-by: Cristian Marussi Signed-off-by: Sudeep Holla --- 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 */ From 9c5c463f2adf140a126c7ab52bd10012c70fdd53 Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Wed, 6 Nov 2019 15:17:26 +0000 Subject: [PATCH 03/13] firmware: arm_scmi: Add names to scmi devices created 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. Reviewed-by: Cristian Marussi Signed-off-by: Sudeep Holla --- 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; From 4605e224db2ec4ce8f0eb4e91feba7eaf5677ba9 Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Tue, 22 Oct 2019 10:09:01 +0100 Subject: [PATCH 04/13] firmware: arm_scmi: Add versions and identifier attributes using dev_groups 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. Reviewed-by: Cristian Marussi Signed-off-by: Sudeep Holla --- 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, From 50872a94637b1e7c92b43280adb71dd8e30fd246 Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Wed, 6 Nov 2019 17:55:03 +0000 Subject: [PATCH 05/13] firmware: arm_scmi: Match scmi device by both name and protocol id 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. Reviewed-by: Cristian Marussi Signed-off-by: Sudeep Holla --- 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); From b55b06b79445574fa031158fe2ae2946cde0d1b7 Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Fri, 22 Nov 2019 14:48:40 +0000 Subject: [PATCH 06/13] firmware: arm_scmi: Stash version in protocol init functions 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. Reviewed-by: Cristian Marussi Signed-off-by: Sudeep Holla --- 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 32526a793f3a..4c2227662b26 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; @@ -340,6 +341,7 @@ static int scmi_clock_protocol_init(struct scmi_handle *handle) 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 601af4edad5e..ec81e6f7e7a4 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; @@ -736,6 +737,7 @@ static int scmi_perf_protocol_init(struct scmi_handle *handle) 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 5abef7079c0a..214886ce84f1 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; @@ -207,6 +208,7 @@ static int scmi_power_protocol_init(struct scmi_handle *handle) 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 ab42c21c5517..de73054554f3 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; }; @@ -217,6 +218,7 @@ static int scmi_reset_protocol_init(struct scmi_handle *handle) 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 a400ea805fc2..eba61b9c1f53 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; @@ -294,6 +295,7 @@ static int scmi_sensors_protocol_init(struct scmi_handle *handle) scmi_sensor_description_get(handle, sinfo); + sinfo->version = version; handle->sensor_ops = &sensor_ops; handle->sensor_priv = sinfo; From 2deb267b26b5441e0e77f999ea084bf02c5c0ef1 Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Thu, 7 Nov 2019 11:39:45 +0000 Subject: [PATCH 07/13] firmware: arm_scmi: Skip protocol initialisation for additional devices 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. In order to achieve the same, we can simple replace the idr pointer from protocol initialisation function to a dummy function. Suggested-by: Cristian Marussi Reviewed-by: Cristian Marussi Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/bus.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index 3714e6307b05..db55c43a2cbd 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c @@ -60,6 +60,11 @@ static int scmi_protocol_init(int protocol_id, struct scmi_handle *handle) return fn(handle); } +static int scmi_protocol_dummy_init(struct scmi_handle *handle) +{ + return 0; +} + static int scmi_dev_probe(struct device *dev) { struct scmi_driver *scmi_drv = to_scmi_driver(dev->driver); @@ -78,6 +83,10 @@ static int scmi_dev_probe(struct device *dev) if (ret) return ret; + /* Skip protocol initialisation for additional devices */ + idr_replace(&scmi_protocols, &scmi_protocol_dummy_init, + scmi_dev->protocol_id); + return scmi_drv->probe(scmi_dev); } From 43998dfe24c19c2b29db64ae2b99e51e6c018aba Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Wed, 6 Nov 2019 17:55:47 +0000 Subject: [PATCH 08/13] clk: scmi: Match scmi device by both name and protocol id 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: linux-clk@vger.kernel.org Cc: Michael Turquette Acked-by: Stephen Boyd Signed-off-by: Sudeep Holla --- 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); From 12b7662618efd08486c00fbfd98222e16c248c04 Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Wed, 6 Nov 2019 17:56:25 +0000 Subject: [PATCH 09/13] cpufreq: scmi: Match scmi device by both name and protocol id 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" Acked-by: Viresh Kumar Signed-off-by: Sudeep Holla --- 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); From b017b2791c9cd6b2e6ec28265e7da3069f25c34f Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Wed, 6 Nov 2019 17:57:30 +0000 Subject: [PATCH 10/13] hwmon: (scmi-hwmon) Match scmi device by both name and protocol id 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 is just for sake of completion and must not be used to add IIO support in parallel. Instead, if IIO support is added ever in future, we need to drop this hwmon driver entirely and use the iio->hwmon bridge to access the sensors as hwmon devices if needed. Cc: linux-hwmon@vger.kernel.org Acked-by: Guenter Roeck Signed-off-by: Sudeep Holla --- 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); From 34ce3c5e691206d388fcc61f3da2ce8fe8a1907a Mon Sep 17 00:00:00 2001 From: Sudeep Holla Date: Wed, 6 Nov 2019 17:58:03 +0000 Subject: [PATCH 11/13] reset: reset-scmi: Match scmi device by both name and protocol id 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. Acked-by: Philipp Zabel Signed-off-by: Sudeep Holla --- 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); From 257d0e20ec4f94e33ac463b918fb82bb1b2dcbe0 Mon Sep 17 00:00:00 2001 From: Lukasz Luba Date: Tue, 17 Dec 2019 13:43:44 +0000 Subject: [PATCH 12/13] include: trace: Add SCMI header with trace events Adding trace events would help to measure the speed of the communication channel. It can be also potentially used helpful during investigation of some issues platforms which use different transport layer. Update also MAINTAINERS file with information that the new trace events are maintained. Suggested-by: Jim Quinlan Signed-off-by: Lukasz Luba Signed-off-by: Sudeep Holla --- MAINTAINERS | 1 + include/trace/events/scmi.h | 90 +++++++++++++++++++++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 include/trace/events/scmi.h diff --git a/MAINTAINERS b/MAINTAINERS index bd5847e802de..5bea0797a086 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -15965,6 +15965,7 @@ F: drivers/firmware/arm_scpi.c F: drivers/firmware/arm_scmi/ F: drivers/reset/reset-scmi.c F: include/linux/sc[mp]i_protocol.h +F: include/trace/events/scmi.h SYSTEM RESET/SHUTDOWN DRIVERS M: Sebastian Reichel diff --git a/include/trace/events/scmi.h b/include/trace/events/scmi.h new file mode 100644 index 000000000000..f076c430d243 --- /dev/null +++ b/include/trace/events/scmi.h @@ -0,0 +1,90 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM scmi + +#if !defined(_TRACE_SCMI_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_SCMI_H + +#include + +TRACE_EVENT(scmi_xfer_begin, + TP_PROTO(int transfer_id, u8 msg_id, u8 protocol_id, u16 seq, + bool poll), + TP_ARGS(transfer_id, msg_id, protocol_id, seq, poll), + + TP_STRUCT__entry( + __field(int, transfer_id) + __field(u8, msg_id) + __field(u8, protocol_id) + __field(u16, seq) + __field(bool, poll) + ), + + TP_fast_assign( + __entry->transfer_id = transfer_id; + __entry->msg_id = msg_id; + __entry->protocol_id = protocol_id; + __entry->seq = seq; + __entry->poll = poll; + ), + + TP_printk("transfer_id=%d msg_id=%u protocol_id=%u seq=%u poll=%u", + __entry->transfer_id, __entry->msg_id, __entry->protocol_id, + __entry->seq, __entry->poll) +); + +TRACE_EVENT(scmi_xfer_end, + TP_PROTO(int transfer_id, u8 msg_id, u8 protocol_id, u16 seq, + u32 status), + TP_ARGS(transfer_id, msg_id, protocol_id, seq, status), + + TP_STRUCT__entry( + __field(int, transfer_id) + __field(u8, msg_id) + __field(u8, protocol_id) + __field(u16, seq) + __field(u32, status) + ), + + TP_fast_assign( + __entry->transfer_id = transfer_id; + __entry->msg_id = msg_id; + __entry->protocol_id = protocol_id; + __entry->seq = seq; + __entry->status = status; + ), + + TP_printk("transfer_id=%d msg_id=%u protocol_id=%u seq=%u status=%u", + __entry->transfer_id, __entry->msg_id, __entry->protocol_id, + __entry->seq, __entry->status) +); + +TRACE_EVENT(scmi_rx_done, + TP_PROTO(int transfer_id, u8 msg_id, u8 protocol_id, u16 seq, + u8 msg_type), + TP_ARGS(transfer_id, msg_id, protocol_id, seq, msg_type), + + TP_STRUCT__entry( + __field(int, transfer_id) + __field(u8, msg_id) + __field(u8, protocol_id) + __field(u16, seq) + __field(u8, msg_type) + ), + + TP_fast_assign( + __entry->transfer_id = transfer_id; + __entry->msg_id = msg_id; + __entry->protocol_id = protocol_id; + __entry->seq = seq; + __entry->msg_type = msg_type; + ), + + TP_printk("transfer_id=%d msg_id=%u protocol_id=%u seq=%u msg_type=%u", + __entry->transfer_id, __entry->msg_id, __entry->protocol_id, + __entry->seq, __entry->msg_type) +); +#endif /* _TRACE_SCMI_H */ + +/* This part must be outside protection */ +#include From 729d3530a50417a88f3f485ba2dc88ff8adfeacb Mon Sep 17 00:00:00 2001 From: Lukasz Luba Date: Tue, 17 Dec 2019 13:43:45 +0000 Subject: [PATCH 13/13] drivers: firmware: scmi: Extend SCMI transport layer by trace events The SCMI transport layer communicates via mailboxes and shared memory with firmware running on a microcontroller. It is platform specific how long it takes to pass a SCMI message. The most sensitive requests are coming from CPUFreq subsystem, which might be used by the scheduler. Thus, there is a need to measure these delays and capture anomalies. This change introduces trace events wrapped around transfer code. According to Jim's suggestion a unique transfer_id is to distinguish similar entries which might have the same message id, protocol id and sequence. This is a case then there are some timeouts in transfers. Suggested-by: Jim Quinlan Signed-off-by: Lukasz Luba Signed-off-by: Sudeep Holla --- drivers/firmware/arm_scmi/common.h | 2 ++ drivers/firmware/arm_scmi/driver.c | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h index 5237c2ff79fe..df35358ff324 100644 --- a/drivers/firmware/arm_scmi/common.h +++ b/drivers/firmware/arm_scmi/common.h @@ -81,6 +81,7 @@ struct scmi_msg { /** * struct scmi_xfer - Structure representing a message flow * + * @transfer_id: Unique ID for debug & profiling purpose * @hdr: Transmit message header * @tx: Transmit message * @rx: Receive message, the buffer should be pre-allocated to store @@ -90,6 +91,7 @@ struct scmi_msg { * @async: pointer to delayed response message received event completion */ struct scmi_xfer { + int transfer_id; struct scmi_msg_hdr hdr; struct scmi_msg tx; struct scmi_msg rx; diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index 26b2c438bd59..2c96f6b5a7d8 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -29,6 +29,9 @@ #include "common.h" +#define CREATE_TRACE_POINTS +#include + #define MSG_ID_MASK GENMASK(7, 0) #define MSG_XTRACT_ID(hdr) FIELD_GET(MSG_ID_MASK, (hdr)) #define MSG_TYPE_MASK GENMASK(9, 8) @@ -61,6 +64,8 @@ enum scmi_error_codes { static LIST_HEAD(scmi_list); /* Protection for the entire list */ static DEFINE_MUTEX(scmi_list_mutex); +/* Track the unique id for the transfers for debug & profiling purpose */ +static atomic_t transfer_last_id; /** * struct scmi_xfers_info - Structure to manage transfer information @@ -304,6 +309,7 @@ static struct scmi_xfer *scmi_xfer_get(const struct scmi_handle *handle, xfer = &minfo->xfer_block[xfer_id]; xfer->hdr.seq = xfer_id; reinit_completion(&xfer->done); + xfer->transfer_id = atomic_inc_return(&transfer_last_id); return xfer; } @@ -374,6 +380,10 @@ static void scmi_rx_callback(struct mbox_client *cl, void *m) scmi_fetch_response(xfer, mem); + trace_scmi_rx_done(xfer->transfer_id, xfer->hdr.id, + xfer->hdr.protocol_id, xfer->hdr.seq, + msg_type); + if (msg_type == MSG_TYPE_DELAYED_RESP) complete(xfer->async_done); else @@ -439,6 +449,10 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer) if (unlikely(!cinfo)) return -EINVAL; + trace_scmi_xfer_begin(xfer->transfer_id, xfer->hdr.id, + xfer->hdr.protocol_id, xfer->hdr.seq, + xfer->hdr.poll_completion); + ret = mbox_send_message(cinfo->chan, xfer); if (ret < 0) { dev_dbg(dev, "mbox send fail %d\n", ret); @@ -478,6 +492,10 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer) */ mbox_client_txdone(cinfo->chan, ret); + trace_scmi_xfer_end(xfer->transfer_id, xfer->hdr.id, + xfer->hdr.protocol_id, xfer->hdr.seq, + xfer->hdr.status); + return ret; }