[PATCH 0/5] firmware: arm_scmi: add SCMI v2.0 fastchannels and reset protocol support

[PATCH 0/5] firmware: arm_scmi: add SCMI v2.0 fastchannels and reset protocol support

[Sudeep Holla]

SCMI v2.0[1] released recently adds support for:

1. Performance protocol fast channels
2. Reset Management Protocol
among several other features.

This series adds support for the above 2.

The code is based on the cleanup[2] and Rx/async/delayed response series[3]
and is available @[4]

--
Regards,
Sudeep

[1] http://infocenter.arm.com/help/topic/com.arm.doc.den0056b/DEN0056B_System_Control_and_Management_Interface_v2_0.pdf
[2] https://lore.kernel.org/lkml/20190726134531.8928-1-sudeep.holla@arm.com
[3] https://lore.kernel.org/lkml/20190726135138.9858-1-sudeep.holla@arm.com/
[4] git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git scmi_updates

Sudeep Holla (5):
  firmware: arm_scmi: Add discovery of SCMI v2.0 performance fastchannels
  firmware: arm_scmi: Make use SCMI v2.0 fastchannel for performance protocol
  dt-bindings: arm: Extend SCMI to support new reset protocol
  firmware: arm_scmi: Add RESET protocol in SCMI v2.0
  reset: Add support for resets provided by SCMI

 .../devicetree/bindings/arm/arm,scmi.txt      |  17 ++
 MAINTAINERS                                   |   1 +
 drivers/firmware/arm_scmi/Makefile            |   2 +-
 drivers/firmware/arm_scmi/perf.c              | 257 +++++++++++++++++-
 drivers/firmware/arm_scmi/reset.c             | 231 ++++++++++++++++
 drivers/reset/Kconfig                         |  10 +
 drivers/reset/Makefile                        |   1 +
 drivers/reset/reset-scmi.c                    | 133 +++++++++
 include/linux/scmi_protocol.h                 |  26 ++
 9 files changed, 669 insertions(+), 9 deletions(-)
 create mode 100644 drivers/firmware/arm_scmi/reset.c
 create mode 100644 drivers/reset/reset-scmi.c

--
2.17.1

[PATCH 1/5] firmware: arm_scmi: Add discovery of SCMI v2.0 performance fastchannels

[Sudeep Holla]

SCMI v2.0 adds support for "FastChannel", a lightweight unidirectional
channel that is dedicated to a single SCMI message type for controlling
a specific platform resource. They do not use a message header as they
are specialized for a single message.

Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
commands are supported over fastchannels. As they are optional, they
need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.

Add support for discovery of these fastchannels.

Cc: Ionela Voinescu <Ionela.Voinescu@arm.com>
Cc: Chris Redpath <Chris.Redpath@arm.com>
Cc: Quentin Perret <Quentin.Perret@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/perf.c | 153 ++++++++++++++++++++++++++++++-
 1 file changed, 149 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 3c8ae7cc35de..6cce3e82e81e 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -5,7 +5,9 @@
  * Copyright (C) 2018 ARM Ltd.
  */
 
+#include <linux/bits.h>
 #include <linux/of.h>
+#include <linux/io.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
 #include <linux/sort.h>
@@ -21,6 +23,7 @@ enum scmi_performance_protocol_cmd {
 	PERF_LEVEL_GET = 0x8,
 	PERF_NOTIFY_LIMITS = 0x9,
 	PERF_NOTIFY_LEVEL = 0xa,
+	PERF_DESCRIBE_FASTCHANNEL = 0xb,
 };
 
 struct scmi_opp {
@@ -44,6 +47,7 @@ struct scmi_msg_resp_perf_domain_attributes {
 #define SUPPORTS_SET_PERF_LVL(x)	((x) & BIT(30))
 #define SUPPORTS_PERF_LIMIT_NOTIFY(x)	((x) & BIT(29))
 #define SUPPORTS_PERF_LEVEL_NOTIFY(x)	((x) & BIT(28))
+#define SUPPORTS_PERF_FASTCHANNELS(x)	((x) & BIT(27))
 	__le32 rate_limit_us;
 	__le32 sustained_freq_khz;
 	__le32 sustained_perf_level;
@@ -87,17 +91,56 @@ struct scmi_msg_resp_perf_describe_levels {
 	} opp[0];
 };
 
+struct scmi_perf_get_fc_info {
+	__le32 domain;
+	__le32 message_id;
+};
+
+struct scmi_msg_resp_perf_desc_fc {
+	__le32 attr;
+#define SUPPORTS_DOORBELL(x)		((x) & BIT(0))
+#define DOORBELL_REG_WIDTH(x)		FIELD_GET(GENMASK(2, 1), (x))
+	__le32 rate_limit;
+	__le32 chan_addr_low;
+	__le32 chan_addr_high;
+	__le32 chan_size;
+	__le32 db_addr_low;
+	__le32 db_addr_high;
+	__le32 db_set_lmask;
+	__le32 db_set_hmask;
+	__le32 db_preserve_lmask;
+	__le32 db_preserve_hmask;
+};
+
+struct scmi_fc_db_info {
+	int width;
+	u64 set;
+	u64 mask;
+	void __iomem *addr;
+};
+
+struct scmi_fc_info {
+	void __iomem *level_set_addr;
+	void __iomem *limit_set_addr;
+	void __iomem *level_get_addr;
+	void __iomem *limit_get_addr;
+	struct scmi_fc_db_info *level_set_db;
+	struct scmi_fc_db_info *limit_set_db;
+};
+
 struct perf_dom_info {
 	bool set_limits;
 	bool set_perf;
 	bool perf_limit_notify;
 	bool perf_level_notify;
+	bool perf_fastchannels;
 	u32 opp_count;
 	u32 sustained_freq_khz;
 	u32 sustained_perf_level;
 	u32 mult_factor;
 	char name[SCMI_MAX_STR_SIZE];
 	struct scmi_opp opp[MAX_OPPS];
+	struct scmi_fc_info *fc_info;
 };
 
 struct scmi_perf_info {
@@ -162,6 +205,7 @@ scmi_perf_domain_attributes_get(const struct scmi_handle *handle, u32 domain,
 		dom_info->set_perf = SUPPORTS_SET_PERF_LVL(flags);
 		dom_info->perf_limit_notify = SUPPORTS_PERF_LIMIT_NOTIFY(flags);
 		dom_info->perf_level_notify = SUPPORTS_PERF_LEVEL_NOTIFY(flags);
+		dom_info->perf_fastchannels = SUPPORTS_PERF_FASTCHANNELS(flags);
 		dom_info->sustained_freq_khz =
 					le32_to_cpu(attr->sustained_freq_khz);
 		dom_info->sustained_perf_level =
@@ -250,7 +294,7 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
 }
 
 static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
-				u32 max_perf, u32 min_perf)
+				   u32 max_perf, u32 min_perf)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -273,7 +317,7 @@ static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
 }
 
 static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
-				u32 *max_perf, u32 *min_perf)
+				   u32 *max_perf, u32 *min_perf)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -299,7 +343,7 @@ static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
 }
 
 static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
-			       u32 level, bool poll)
+				  u32 level, bool poll)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -322,7 +366,7 @@ static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
 }
 
 static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
-			       u32 *level, bool poll)
+				  u32 *level, bool poll)
 {
 	int ret;
 	struct scmi_xfer *t;
@@ -343,6 +387,104 @@ static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
 	return ret;
 }
 
+static bool scmi_perf_fc_size_is_valid(u32 msg, u32 size)
+{
+	if ((msg == PERF_LEVEL_GET || msg == PERF_LEVEL_SET) && size == 4)
+		return true;
+	if ((msg == PERF_LIMITS_GET || msg == PERF_LIMITS_SET) && size == 8)
+		return true;
+	return false;
+}
+
+static void
+scmi_perf_domain_desc_fc(const struct scmi_handle *handle, u32 domain,
+			 u32 message_id, void __iomem **p_addr,
+			 struct scmi_fc_db_info **p_db)
+{
+	int ret;
+	u32 flags;
+	u64 phys_addr;
+	u8 size;
+	void __iomem *addr;
+	struct scmi_xfer *t;
+	struct scmi_fc_db_info *db;
+	struct scmi_perf_get_fc_info *info;
+	struct scmi_msg_resp_perf_desc_fc *resp;
+
+	if (!p_addr)
+		return;
+
+	ret = scmi_xfer_get_init(handle, PERF_DESCRIBE_FASTCHANNEL,
+				 SCMI_PROTOCOL_PERF,
+				 sizeof(*info), sizeof(*resp), &t);
+	if (ret)
+		return;
+
+	info = t->tx.buf;
+	info->domain = cpu_to_le32(domain);
+	info->message_id = cpu_to_le32(message_id);
+
+	ret = scmi_do_xfer(handle, t);
+	if (ret)
+		goto err_xfer;
+
+	resp = t->rx.buf;
+	flags = le32_to_cpu(resp->attr);
+	size = le32_to_cpu(resp->chan_size);
+	if (!scmi_perf_fc_size_is_valid(message_id, size))
+		goto err_xfer;
+
+	phys_addr = le32_to_cpu(resp->chan_addr_low);
+	phys_addr |= (u64)le32_to_cpu(resp->chan_addr_high) << 32;
+	addr = devm_ioremap(handle->dev, phys_addr, size);
+	if (!addr)
+		goto err_xfer;
+	*p_addr = addr;
+
+	if (p_db && SUPPORTS_DOORBELL(flags)) {
+		db = devm_kzalloc(handle->dev, sizeof(*db), GFP_KERNEL);
+		if (!db)
+			goto err_xfer;
+
+		size = 1 << DOORBELL_REG_WIDTH(flags);
+		phys_addr = le32_to_cpu(resp->db_addr_low);
+		phys_addr |= (u64)le32_to_cpu(resp->db_addr_high) << 32;
+		addr = devm_ioremap(handle->dev, phys_addr, size);
+		if (!addr)
+			goto err_xfer;
+
+		db->addr = addr;
+		db->width = size;
+		db->set = le32_to_cpu(resp->db_set_lmask);
+		db->set |= (u64)le32_to_cpu(resp->db_set_hmask) << 32;
+		db->mask = le32_to_cpu(resp->db_preserve_lmask);
+		db->mask |= (u64)le32_to_cpu(resp->db_preserve_hmask) << 32;
+		*p_db = db;
+	}
+err_xfer:
+	scmi_xfer_put(handle, t);
+}
+
+static void scmi_perf_domain_init_fc(const struct scmi_handle *handle,
+				     u32 domain, struct scmi_fc_info **p_fc)
+{
+	struct scmi_fc_info *fc;
+
+	fc = devm_kzalloc(handle->dev, sizeof(*fc), GFP_KERNEL);
+	if (!fc)
+		return;
+
+	scmi_perf_domain_desc_fc(handle, domain, PERF_LEVEL_SET,
+				 &fc->level_set_addr, &fc->level_set_db);
+	scmi_perf_domain_desc_fc(handle, domain, PERF_LEVEL_GET,
+				 &fc->level_get_addr, NULL);
+	scmi_perf_domain_desc_fc(handle, domain, PERF_LIMITS_SET,
+				 &fc->limit_set_addr, &fc->limit_set_db);
+	scmi_perf_domain_desc_fc(handle, domain, PERF_LIMITS_GET,
+				 &fc->limit_get_addr, NULL);
+	*p_fc = fc;
+}
+
 /* Device specific ops */
 static int scmi_dev_domain_id(struct device *dev)
 {
@@ -494,6 +636,9 @@ static int scmi_perf_protocol_init(struct scmi_handle *handle)
 
 		scmi_perf_domain_attributes_get(handle, domain, dom);
 		scmi_perf_describe_levels_get(handle, domain, dom);
+
+		if (dom->perf_fastchannels)
+			scmi_perf_domain_init_fc(handle, domain, &dom->fc_info);
 	}
 
 	handle->perf_ops = &perf_ops;
-- 
2.17.1

[PATCH 2/5] firmware: arm_scmi: Make use SCMI v2.0 fastchannel for performance protocol

[Sudeep Holla]

SCMI v2.0 adds support for "FastChannel" which do not use a message
header as they are specialized for a single message.

Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
commands are supported over fastchannels. As they are optional, they
need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.

Add support for making use of these fastchannels.

Cc: Ionela Voinescu <Ionela.Voinescu@arm.com>
Cc: Chris Redpath <Chris.Redpath@arm.com>
Cc: Quentin Perret <Quentin.Perret@arm.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/perf.c | 104 +++++++++++++++++++++++++++++--
 1 file changed, 100 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
index 6cce3e82e81e..b9144efbd6fb 100644
--- a/drivers/firmware/arm_scmi/perf.c
+++ b/drivers/firmware/arm_scmi/perf.c
@@ -8,6 +8,7 @@
 #include <linux/bits.h>
 #include <linux/of.h>
 #include <linux/io.h>
+#include <linux/io-64-nonatomic-hi-lo.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
 #include <linux/sort.h>
@@ -293,7 +294,42 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
 	return ret;
 }
 
-static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
+#define SCMI_PERF_FC_RING_DB(doorbell, w)		\
+{							\
+	u##w val = 0;					\
+	struct scmi_fc_db_info *db = doorbell;		\
+							\
+	if ((db)->mask)					\
+		val = ioread##w(db->addr) & db->mask;	\
+	iowrite##w((u##w)db->set | val, db->addr);	\
+}
+
+static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
+{
+	if (!db || !db->addr)
+		return;
+
+	if (db->width == 1)
+		SCMI_PERF_FC_RING_DB(db, 8)
+	else if (db->width == 2)
+		SCMI_PERF_FC_RING_DB(db, 16)
+	else if (db->width == 4)
+		SCMI_PERF_FC_RING_DB(db, 32)
+	else /* db->width == 8 */
+#ifdef CONFIG_64BIT
+		SCMI_PERF_FC_RING_DB(db, 64)
+#else
+	{
+		u64 val = 0;
+
+		if (db->mask)
+			val = ioread64_hi_lo(db->addr) & db->mask;
+		iowrite64_hi_lo(db->set, db->addr);
+	}
+#endif
+}
+
+static int scmi_perf_mb_limits_set(const struct scmi_handle *handle, u32 domain,
 				   u32 max_perf, u32 min_perf)
 {
 	int ret;
@@ -316,7 +352,23 @@ static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
 	return ret;
 }
 
-static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
+static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
+				u32 max_perf, u32 min_perf)
+{
+	struct scmi_perf_info *pi = handle->perf_priv;
+	struct perf_dom_info *dom = pi->dom_info + domain;
+
+	if (dom->fc_info && dom->fc_info->limit_set_addr) {
+		iowrite32(max_perf, dom->fc_info->limit_set_addr);
+		iowrite32(min_perf, dom->fc_info->limit_set_addr + 4);
+		scmi_perf_fc_ring_db(dom->fc_info->limit_set_db);
+		return 0;
+	}
+
+	return scmi_perf_mb_limits_set(handle, domain, max_perf, min_perf);
+}
+
+static int scmi_perf_mb_limits_get(const struct scmi_handle *handle, u32 domain,
 				   u32 *max_perf, u32 *min_perf)
 {
 	int ret;
@@ -342,7 +394,22 @@ static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
 	return ret;
 }
 
-static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
+static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
+				u32 *max_perf, u32 *min_perf)
+{
+	struct scmi_perf_info *pi = handle->perf_priv;
+	struct perf_dom_info *dom = pi->dom_info + domain;
+
+	if (dom->fc_info && dom->fc_info->limit_get_addr) {
+		*max_perf = ioread32(dom->fc_info->limit_get_addr);
+		*min_perf = ioread32(dom->fc_info->limit_get_addr + 4);
+		return 0;
+	}
+
+	return scmi_perf_mb_limits_get(handle, domain, max_perf, min_perf);
+}
+
+static int scmi_perf_mb_level_set(const struct scmi_handle *handle, u32 domain,
 				  u32 level, bool poll)
 {
 	int ret;
@@ -365,7 +432,22 @@ static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
 	return ret;
 }
 
-static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
+static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
+			       u32 level, bool poll)
+{
+	struct scmi_perf_info *pi = handle->perf_priv;
+	struct perf_dom_info *dom = pi->dom_info + domain;
+
+	if (dom->fc_info && dom->fc_info->level_set_addr) {
+		iowrite32(level, dom->fc_info->level_set_addr);
+		scmi_perf_fc_ring_db(dom->fc_info->level_set_db);
+		return 0;
+	}
+
+	return scmi_perf_mb_level_set(handle, domain, level, poll);
+}
+
+static int scmi_perf_mb_level_get(const struct scmi_handle *handle, u32 domain,
 				  u32 *level, bool poll)
 {
 	int ret;
@@ -387,6 +469,20 @@ static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
 	return ret;
 }
 
+static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
+			       u32 *level, bool poll)
+{
+	struct scmi_perf_info *pi = handle->perf_priv;
+	struct perf_dom_info *dom = pi->dom_info + domain;
+
+	if (dom->fc_info && dom->fc_info->level_get_addr) {
+		*level = ioread32(dom->fc_info->level_get_addr);
+		return 0;
+	}
+
+	return scmi_perf_mb_level_get(handle, domain, level, poll);
+}
+
 static bool scmi_perf_fc_size_is_valid(u32 msg, u32 size)
 {
 	if ((msg == PERF_LEVEL_GET || msg == PERF_LEVEL_SET) && size == 4)
-- 
2.17.1

[PATCH 3/5] dt-bindings: arm: Extend SCMI to support new reset protocol

[Sudeep Holla]

SCMIv2.0 adds a new Reset Management Protocol to manage various reset
states a given device or domain can enter. Extend the existing SCMI
bindings to add reset protocol support by re-using the reset bindings
for bothe reset providers and consumers.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 .../devicetree/bindings/arm/arm,scmi.txt        | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt
index 317a2fc3667a..083dbf96ee00 100644
--- a/Documentation/devicetree/bindings/arm/arm,scmi.txt
+++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt
@@ -73,6 +73,16 @@ SCMI provides an API to access the various sensors on the SoC.
 			 as used by the firmware. Refer to  platform details
 			 for your implementation for the IDs to use.
 
+Reset signal bindings for the reset domains based on SCMI Message Protocol
+------------------------------------------------------------
+
+This binding for the SCMI reset domain providers uses the generic reset
+signal binding[5].
+
+Required properties:
+ - #reset-cells : Should be 1. Contains the reset domain ID value used
+		  by SCMI commands.
+
 SRAM and Shared Memory for SCMI
 -------------------------------
 
@@ -93,6 +103,7 @@ Each sub-node represents the reserved area for SCMI.
 [2] Documentation/devicetree/bindings/power/power_domain.txt
 [3] Documentation/devicetree/bindings/thermal/thermal.txt
 [4] Documentation/devicetree/bindings/sram/sram.txt
+[5] Documentation/devicetree/bindings/reset/reset.txt
 
 Example:
 
@@ -152,6 +163,11 @@ firmware {
 			reg = <0x15>;
 			#thermal-sensor-cells = <1>;
 		};
+
+		scmi_reset: protocol@16 {
+			reg = <0x16>;
+			#reset-cells = <1>;
+		};
 	};
 };
 
@@ -166,6 +182,7 @@ hdlcd@7ff60000 {
 	reg = <0 0x7ff60000 0 0x1000>;
 	clocks = <&scmi_clk 4>;
 	power-domains = <&scmi_devpd 1>;
+	resets = <&scmi_reset 10>;
 };
 
 thermal-zones {
-- 
2.17.1

[PATCH 4/5] firmware: arm_scmi: Add RESET protocol in SCMI v2.0

[Sudeep Holla]

SCMIv2.0 adds a new Reset Management Protocol to manage various reset
states a given device or domain can enter. Device(s) that can be
collectively reset through a common reset signal constitute a reset
domain for the firmware.

A reset domain can be reset autonomously or explicitly through assertion
and de-assertion of the signal. When autonomous reset is chosen, the
firmware is responsible for taking the necessary steps to reset the
domain and to subsequently bring it out of reset. When explicit reset is
chosen, the caller has to specifically assert and then de-assert the
reset signal by issuing two separate RESET commands.

Add the basic SCMI reset infrastructure that can be used by Linux
reset controller driver.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/Makefile |   2 +-
 drivers/firmware/arm_scmi/reset.c  | 231 +++++++++++++++++++++++++++++
 include/linux/scmi_protocol.h      |  26 ++++
 3 files changed, 258 insertions(+), 1 deletion(-)
 create mode 100644 drivers/firmware/arm_scmi/reset.c

diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile
index c47d28d556b6..5f298f00a82e 100644
--- a/drivers/firmware/arm_scmi/Makefile
+++ b/drivers/firmware/arm_scmi/Makefile
@@ -2,5 +2,5 @@
 obj-y	= scmi-bus.o scmi-driver.o scmi-protocols.o
 scmi-bus-y = bus.o
 scmi-driver-y = driver.o
-scmi-protocols-y = base.o clock.o perf.o power.o sensors.o
+scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
 obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o
diff --git a/drivers/firmware/arm_scmi/reset.c b/drivers/firmware/arm_scmi/reset.c
new file mode 100644
index 000000000000..11cb8b5ccf34
--- /dev/null
+++ b/drivers/firmware/arm_scmi/reset.c
@@ -0,0 +1,231 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * System Control and Management Interface (SCMI) Reset Protocol
+ *
+ * Copyright (C) 2019 ARM Ltd.
+ */
+
+#include "common.h"
+
+enum scmi_reset_protocol_cmd {
+	RESET_DOMAIN_ATTRIBUTES = 0x3,
+	RESET = 0x4,
+	RESET_NOTIFY = 0x5,
+};
+
+enum scmi_reset_protocol_notify {
+	RESET_ISSUED = 0x0,
+};
+
+#define NUM_RESET_DOMAIN_MASK	0xffff
+#define RESET_NOTIFY_ENABLE	BIT(0)
+
+struct scmi_msg_resp_reset_domain_attributes {
+	__le32 attributes;
+#define SUPPORTS_ASYNC_RESET(x)		((x) & BIT(31))
+#define SUPPORTS_NOTIFY_RESET(x)	((x) & BIT(30))
+	__le32 latency;
+	    u8 name[SCMI_MAX_STR_SIZE];
+};
+
+struct scmi_msg_reset_domain_reset {
+	__le32 domain_id;
+	__le32 flags;
+#define AUTONOMOUS_RESET	BIT(0)
+#define EXPLICIT_RESET_ASSERT	BIT(1)
+#define ASYNCHRONOUS_RESET	BIT(2)
+	__le32 reset_state;
+#define ARCH_RESET_TYPE		BIT(31)
+#define COLD_RESET_STATE	BIT(0)
+#define ARCH_COLD_RESET		(ARCH_RESET_TYPE | COLD_RESET_STATE)
+};
+
+struct reset_dom_info {
+	bool async_reset;
+	bool reset_notify;
+	u32 latency_us;
+	char name[SCMI_MAX_STR_SIZE];
+};
+
+struct scmi_reset_info {
+	int num_domains;
+	struct reset_dom_info *dom_info;
+};
+
+static int scmi_reset_attributes_get(const struct scmi_handle *handle,
+				     struct scmi_reset_info *pi)
+{
+	int ret;
+	struct scmi_xfer *t;
+	u32 *attr;
+
+	ret = scmi_xfer_get_init(handle, PROTOCOL_ATTRIBUTES,
+				 SCMI_PROTOCOL_RESET, 0, sizeof(*attr), &t);
+	if (ret)
+		return ret;
+
+	attr = t->rx.buf;
+
+	ret = scmi_do_xfer(handle, t);
+	if (!ret)
+		pi->num_domains = le32_to_cpu(*attr) & NUM_RESET_DOMAIN_MASK;
+
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
+static int
+scmi_reset_domain_attributes_get(const struct scmi_handle *handle, u32 domain,
+				 struct reset_dom_info *dom_info)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_resp_reset_domain_attributes *attr;
+
+	ret = scmi_xfer_get_init(handle, RESET_DOMAIN_ATTRIBUTES,
+				 SCMI_PROTOCOL_RESET, sizeof(domain),
+				 sizeof(*attr), &t);
+	if (ret)
+		return ret;
+
+	*(__le32 *)t->tx.buf = cpu_to_le32(domain);
+	attr = t->rx.buf;
+
+	ret = scmi_do_xfer(handle, t);
+	if (!ret) {
+		u32 attributes = le32_to_cpu(attr->attributes);
+
+		dom_info->async_reset = SUPPORTS_ASYNC_RESET(attributes);
+		dom_info->reset_notify = SUPPORTS_NOTIFY_RESET(attributes);
+		dom_info->latency_us = le32_to_cpu(attr->latency);
+		if (dom_info->latency_us == U32_MAX)
+			dom_info->latency_us = 0;
+		strlcpy(dom_info->name, attr->name, SCMI_MAX_STR_SIZE);
+	}
+
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
+static int scmi_reset_num_domains_get(const struct scmi_handle *handle)
+{
+	struct scmi_reset_info *pi = handle->reset_priv;
+
+	return pi->num_domains;
+}
+
+static char *scmi_reset_name_get(const struct scmi_handle *handle, u32 domain)
+{
+	struct scmi_reset_info *pi = handle->reset_priv;
+	struct reset_dom_info *dom = pi->dom_info + domain;
+
+	return dom->name;
+}
+
+static int scmi_reset_latency_get(const struct scmi_handle *handle, u32 domain)
+{
+	struct scmi_reset_info *pi = handle->reset_priv;
+	struct reset_dom_info *dom = pi->dom_info + domain;
+
+	return dom->latency_us;
+}
+
+static int scmi_domain_reset(const struct scmi_handle *handle, u32 domain,
+			     u32 flags, u32 state)
+{
+	int ret;
+	struct scmi_xfer *t;
+	struct scmi_msg_reset_domain_reset *dom;
+	struct scmi_reset_info *pi = handle->reset_priv;
+	struct reset_dom_info *rdom = pi->dom_info + domain;
+
+	if (rdom->async_reset)
+		flags |= ASYNCHRONOUS_RESET;
+
+	ret = scmi_xfer_get_init(handle, RESET, SCMI_PROTOCOL_RESET,
+				 sizeof(*dom), 0, &t);
+	if (ret)
+		return ret;
+
+	dom = t->tx.buf;
+	dom->domain_id = cpu_to_le32(domain);
+	dom->flags = cpu_to_le32(flags);
+	dom->domain_id = cpu_to_le32(state);
+
+	if (rdom->async_reset)
+		ret = scmi_do_xfer_with_response(handle, t);
+	else
+		ret = scmi_do_xfer(handle, t);
+
+	scmi_xfer_put(handle, t);
+	return ret;
+}
+
+static int scmi_reset_domain_reset(const struct scmi_handle *handle, u32 domain)
+{
+	return scmi_domain_reset(handle, domain, AUTONOMOUS_RESET,
+				 ARCH_COLD_RESET);
+}
+
+static int
+scmi_reset_domain_assert(const struct scmi_handle *handle, u32 domain)
+{
+	return scmi_domain_reset(handle, domain, EXPLICIT_RESET_ASSERT,
+				 ARCH_COLD_RESET);
+}
+
+static int
+scmi_reset_domain_deassert(const struct scmi_handle *handle, u32 domain)
+{
+	return scmi_domain_reset(handle, domain, 0, ARCH_COLD_RESET);
+}
+
+static struct scmi_reset_ops reset_ops = {
+	.num_domains_get = scmi_reset_num_domains_get,
+	.name_get = scmi_reset_name_get,
+	.latency_get = scmi_reset_latency_get,
+	.reset = scmi_reset_domain_reset,
+	.assert = scmi_reset_domain_assert,
+	.deassert = scmi_reset_domain_deassert,
+};
+
+static int scmi_reset_protocol_init(struct scmi_handle *handle)
+{
+	int domain;
+	u32 version;
+	struct scmi_reset_info *pinfo;
+
+	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;
+
+	scmi_reset_attributes_get(handle, pinfo);
+
+	pinfo->dom_info = devm_kcalloc(handle->dev, pinfo->num_domains,
+				       sizeof(*pinfo->dom_info), GFP_KERNEL);
+	if (!pinfo->dom_info)
+		return -ENOMEM;
+
+	for (domain = 0; domain < pinfo->num_domains; domain++) {
+		struct reset_dom_info *dom = pinfo->dom_info + domain;
+
+		scmi_reset_domain_attributes_get(handle, domain, dom);
+	}
+
+	handle->reset_ops = &reset_ops;
+	handle->reset_priv = pinfo;
+
+	return 0;
+}
+
+static int __init scmi_reset_init(void)
+{
+	return scmi_protocol_register(SCMI_PROTOCOL_RESET,
+				      &scmi_reset_protocol_init);
+}
+subsys_initcall(scmi_reset_init);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index f0f2b53a1dac..881fea47c83d 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -187,6 +187,26 @@ struct scmi_sensor_ops {
 			   u64 *value);
 };
 
+/**
+ * struct scmi_reset_ops - represents the various operations provided
+ *	by SCMI Reset Protocol
+ *
+ * @num_domains_get: get the count of reset domains provided by SCMI
+ * @name_get: gets the name of a reset domain
+ * @latency_get: gets the reset latency for the specified reset domain
+ * @reset: resets the specified reset domain
+ * @assert: explicitly assert reset signal of the specified reset domain
+ * @deassert: explicitly deassert reset signal of the specified reset domain
+ */
+struct scmi_reset_ops {
+	int (*num_domains_get)(const struct scmi_handle *handle);
+	char *(*name_get)(const struct scmi_handle *handle, u32 domain);
+	int (*latency_get)(const struct scmi_handle *handle, u32 domain);
+	int (*reset)(const struct scmi_handle *handle, u32 domain);
+	int (*assert)(const struct scmi_handle *handle, u32 domain);
+	int (*deassert)(const struct scmi_handle *handle, u32 domain);
+};
+
 /**
  * struct scmi_handle - Handle returned to ARM SCMI clients for usage.
  *
@@ -196,6 +216,7 @@ struct scmi_sensor_ops {
  * @perf_ops: pointer to set of performance protocol operations
  * @clk_ops: pointer to set of clock protocol operations
  * @sensor_ops: pointer to set of sensor protocol operations
+ * @reset_ops: pointer to set of reset protocol operations
  * @perf_priv: pointer to private data structure specific to performance
  *	protocol(for internal use only)
  * @clk_priv: pointer to private data structure specific to clock
@@ -204,6 +225,8 @@ struct scmi_sensor_ops {
  *	protocol(for internal use only)
  * @sensor_priv: pointer to private data structure specific to sensors
  *	protocol(for internal use only)
+ * @reset_priv: pointer to private data structure specific to reset
+ *	protocol(for internal use only)
  */
 struct scmi_handle {
 	struct device *dev;
@@ -212,11 +235,13 @@ struct scmi_handle {
 	struct scmi_clk_ops *clk_ops;
 	struct scmi_power_ops *power_ops;
 	struct scmi_sensor_ops *sensor_ops;
+	struct scmi_reset_ops *reset_ops;
 	/* for protocol internal use */
 	void *perf_priv;
 	void *clk_priv;
 	void *power_priv;
 	void *sensor_priv;
+	void *reset_priv;
 };
 
 enum scmi_std_protocol {
@@ -226,6 +251,7 @@ enum scmi_std_protocol {
 	SCMI_PROTOCOL_PERF = 0x13,
 	SCMI_PROTOCOL_CLOCK = 0x14,
 	SCMI_PROTOCOL_SENSOR = 0x15,
+	SCMI_PROTOCOL_RESET = 0x16,
 };
 
 struct scmi_device {
-- 
2.17.1

[PATCH 5/5] reset: Add support for resets provided by SCMI

[Sudeep Holla]

On some ARM based systems, a separate Cortex-M based System Control
Processor(SCP) provides the overall power, clock, reset and system
control. System Control and Management Interface(SCMI) Message Protocol
is defined for the communication between the Application Cores(AP)
and the SCP.

Adds support for the resets provided using SCMI protocol for performing
reset management of various devices present on the SoC. Various reset
functionalities are achieved by the means of different ARM SCMI device
operations provided by the ARM SCMI framework.

Cc: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 MAINTAINERS                |   1 +
 drivers/reset/Kconfig      |  10 +++
 drivers/reset/Makefile     |   1 +
 drivers/reset/reset-scmi.c | 133 +++++++++++++++++++++++++++++++++++++
 4 files changed, 145 insertions(+)
 create mode 100644 drivers/reset/reset-scmi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 783569e3c4b4..59df8f88b56d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15545,6 +15545,7 @@ F:	drivers/clk/clk-sc[mp]i.c
 F:	drivers/cpufreq/sc[mp]i-cpufreq.c
 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
 
 SYSTEM RESET/SHUTDOWN DRIVERS
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 21efb7d39d62..09dcc3bf3b7a 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -22,6 +22,16 @@ config RESET_A10SR
 	  This option enables support for the external reset functions for
 	  peripheral PHYs on the Altera Arria10 System Resource Chip.
 
+config RESET_ARM_SCMI
+	tristate "Reset driver controlled via ARM SCMI interface"
+	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST
+	help
+	  This driver provides support for reset signal/domains that are
+	  controlled by firmware that implements the SCMI interface.
+
+	  This driver uses SCMI Message Protocol to interact with the
+	  firmware providing all the reset signals.
+
 config RESET_ATH79
 	bool "AR71xx Reset Driver" if COMPILE_TEST
 	default ATH79
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 61456b8f659c..2f1103d31938 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -4,6 +4,7 @@ obj-y += hisilicon/
 obj-$(CONFIG_ARCH_STI) += sti/
 obj-$(CONFIG_ARCH_TEGRA) += tegra/
 obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
+obj-$(CONFIG_RESET_ARM_SCMI) += reset-scmi.o
 obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
 obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
 obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
diff --git a/drivers/reset/reset-scmi.c b/drivers/reset/reset-scmi.c
new file mode 100644
index 000000000000..9e5d07cac2ed
--- /dev/null
+++ b/drivers/reset/reset-scmi.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ARM System Control and Management Interface (ARM SCMI) reset driver
+ *
+ * Copyright (C) 2019 ARM Ltd.
+ */
+
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/scmi_protocol.h>
+
+/**
+ * struct scmi_reset_data - reset controller information structure
+ * @rcdev: reset controller entity
+ * @handle: ARM SCMI handle used for communication with system controller
+ * @dev: reset controller device pointer
+ */
+struct scmi_reset_data {
+	struct reset_controller_dev rcdev;
+	const struct scmi_handle *handle;
+	struct device *dev;
+};
+
+#define to_scmi_reset_data(p)	\
+	container_of((p), struct scmi_reset_data, rcdev)
+
+/**
+ * scmi_reset_assert() - assert device reset
+ * @rcdev: reset controller entity
+ * @id: ID of the reset to be asserted
+ *
+ * This function implements the reset driver op to assert a device's reset
+ * using the ARM SCMI protocol.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int
+scmi_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct scmi_reset_data *data = to_scmi_reset_data(rcdev);
+	const struct scmi_handle *handle = data->handle;
+	int ret;
+
+	ret = handle->reset_ops->assert(handle, id);
+
+	return ret;
+}
+
+/**
+ * scmi_reset_deassert() - deassert device reset
+ * @rcdev: reset controller entity
+ * @id: ID of the reset to be deasserted
+ *
+ * This function implements the reset driver op to deassert a device's reset
+ * using the ARM SCMI protocol.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int
+scmi_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct scmi_reset_data *data = to_scmi_reset_data(rcdev);
+	const struct scmi_handle *handle = data->handle;
+
+	return handle->reset_ops->deassert(handle, id);
+}
+
+/**
+ * scmi_reset_reset() - reset the device
+ * @rcdev: reset controller entity
+ * @id: ID of the reset signal to be reset(assert + deassert)
+ *
+ * This function implements the reset driver op to reset a device's reset
+ * signal using the ARM SCMI protocol.
+ *
+ * Return: 0 for successful request, else a corresponding error value
+ */
+static int
+scmi_reset_reset(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct scmi_reset_data *data = to_scmi_reset_data(rcdev);
+	const struct scmi_handle *handle = data->handle;
+
+	return handle->reset_ops->reset(handle, id);
+}
+
+static const struct reset_control_ops scmi_reset_ops = {
+	.assert		= scmi_reset_assert,
+	.deassert	= scmi_reset_deassert,
+	.reset		= scmi_reset_reset,
+};
+
+static int scmi_reset_probe(struct scmi_device *sdev)
+{
+	struct scmi_reset_data *data;
+	struct device *dev = &sdev->dev;
+	struct device_node *np = dev->of_node;
+	const struct scmi_handle *handle = sdev->handle;
+
+	if (!handle || !handle->reset_ops)
+		return -ENODEV;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->rcdev.ops = &scmi_reset_ops;
+	data->rcdev.owner = THIS_MODULE;
+	data->rcdev.of_node = np;
+	data->dev = dev;
+
+	return devm_reset_controller_register(dev, &data->rcdev);
+}
+
+static const struct scmi_device_id scmi_id_table[] = {
+	{ SCMI_PROTOCOL_RESET },
+	{ },
+};
+MODULE_DEVICE_TABLE(scmi, scmi_id_table);
+
+static struct scmi_driver scmi_reset_driver = {
+	.name = "scmi-reset",
+	.probe = scmi_reset_probe,
+	.id_table = scmi_id_table,
+};
+module_scmi_driver(scmi_reset_driver);
+
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_DESCRIPTION("ARM SCMI clock driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH 5/5] reset: Add support for resets provided by SCMI

[Philipp Zabel]

Hi Sudeep,

On Fri, 2019-07-26 at 14:59 +0100, Sudeep Holla wrote:
> On some ARM based systems, a separate Cortex-M based System Control
> Processor(SCP) provides the overall power, clock, reset and system
> control. System Control and Management Interface(SCMI) Message Protocol
> is defined for the communication between the Application Cores(AP)
> and the SCP.
> 
> Adds support for the resets provided using SCMI protocol for performing
> reset management of various devices present on the SoC. Various reset
> functionalities are achieved by the means of different ARM SCMI device
> operations provided by the ARM SCMI framework.
> 
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

thank you for the patch. I have a few suggestions below.

> ---
>  MAINTAINERS                |   1 +
>  drivers/reset/Kconfig      |  10 +++
>  drivers/reset/Makefile     |   1 +
>  drivers/reset/reset-scmi.c | 133 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 145 insertions(+)
>  create mode 100644 drivers/reset/reset-scmi.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 783569e3c4b4..59df8f88b56d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -15545,6 +15545,7 @@ F:	drivers/clk/clk-sc[mp]i.c
>  F:	drivers/cpufreq/sc[mp]i-cpufreq.c
>  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
>  
>  SYSTEM RESET/SHUTDOWN DRIVERS
> diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
> index 21efb7d39d62..09dcc3bf3b7a 100644
> --- a/drivers/reset/Kconfig
> +++ b/drivers/reset/Kconfig
> @@ -22,6 +22,16 @@ config RESET_A10SR
>  	  This option enables support for the external reset functions for
>  	  peripheral PHYs on the Altera Arria10 System Resource Chip.
>  
> +config RESET_ARM_SCMI
> +	tristate "Reset driver controlled via ARM SCMI interface"
> +	depends on ARM_SCMI_PROTOCOL || COMPILE_TEST

Should this have a

+	default ARM_SCMI_PROTOCOL

?	

> +	help
> +	  This driver provides support for reset signal/domains that are
> +	  controlled by firmware that implements the SCMI interface.
> +
> +	  This driver uses SCMI Message Protocol to interact with the
> +	  firmware providing all the reset signals.

s/providing/controlling/ ?

> +
>  config RESET_ATH79
>  	bool "AR71xx Reset Driver" if COMPILE_TEST
>  	default ATH79
> diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
> index 61456b8f659c..2f1103d31938 100644
> --- a/drivers/reset/Makefile
> +++ b/drivers/reset/Makefile
> @@ -4,6 +4,7 @@ obj-y += hisilicon/
>  obj-$(CONFIG_ARCH_STI) += sti/
>  obj-$(CONFIG_ARCH_TEGRA) += tegra/
>  obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
> +obj-$(CONFIG_RESET_ARM_SCMI) += reset-scmi.o

s/RESET_ARM_SCMI/RESET_SCMI/ to match the driver/module name?

>  obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
>  obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
>  obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
> diff --git a/drivers/reset/reset-scmi.c b/drivers/reset/reset-scmi.c
> new file mode 100644
> index 000000000000..9e5d07cac2ed
> --- /dev/null
> +++ b/drivers/reset/reset-scmi.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ARM System Control and Management Interface (ARM SCMI) reset driver
> + *
> + * Copyright (C) 2019 ARM Ltd.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/scmi_protocol.h>
> +
> +/**
> + * struct scmi_reset_data - reset controller information structure
> + * @rcdev: reset controller entity
> + * @handle: ARM SCMI handle used for communication with system controller
> + * @dev: reset controller device pointer
> + */
> +struct scmi_reset_data {
> +	struct reset_controller_dev rcdev;
> +	const struct scmi_handle *handle;
> +	struct device *dev;

dev is unused, you could just remove it.

> +};
> +
> +#define to_scmi_reset_data(p)	\
> +	container_of((p), struct scmi_reset_data, rcdev)
> +
> +/**
> + * scmi_reset_assert() - assert device reset
> + * @rcdev: reset controller entity
> + * @id: ID of the reset to be asserted
> + *
> + * This function implements the reset driver op to assert a device's reset
> + * using the ARM SCMI protocol.
> + *
> + * Return: 0 for successful request, else a corresponding error value
> + */
> +static int
> +scmi_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct scmi_reset_data *data = to_scmi_reset_data(rcdev);
> +	const struct scmi_handle *handle = data->handle;

This could be shortened to to_scmi_handle(rcdev), since none of the
other fields in scmi_reset_data are used by the reset_control_ops
callbacks.

> +	int ret;
> +
> +	ret = handle->reset_ops->assert(handle, id);
> +
> +	return ret;

No need for ret here, see _deassert() and _reset().

> +}
> +
> +/**
> + * scmi_reset_deassert() - deassert device reset
> + * @rcdev: reset controller entity
> + * @id: ID of the reset to be deasserted
> + *
> + * This function implements the reset driver op to deassert a device's reset
> + * using the ARM SCMI protocol.
> + *
> + * Return: 0 for successful request, else a corresponding error value
> + */
> +static int
> +scmi_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct scmi_reset_data *data = to_scmi_reset_data(rcdev);
> +	const struct scmi_handle *handle = data->handle;
> +
> +	return handle->reset_ops->deassert(handle, id);
> +}
> +
> +/**
> + * scmi_reset_reset() - reset the device
> + * @rcdev: reset controller entity
> + * @id: ID of the reset signal to be reset(assert + deassert)
> + *
> + * This function implements the reset driver op to reset a device's reset
> + * signal using the ARM SCMI protocol.

"to reset a device" or "to trigger a device's reset signal" would be a
more accurate description.

> + *
> + * Return: 0 for successful request, else a corresponding error value
> + */
> +static int
> +scmi_reset_reset(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> +	struct scmi_reset_data *data = to_scmi_reset_data(rcdev);
> +	const struct scmi_handle *handle = data->handle;
> +
> +	return handle->reset_ops->reset(handle, id);
> +}
> +
> +static const struct reset_control_ops scmi_reset_ops = {
> +	.assert		= scmi_reset_assert,
> +	.deassert	= scmi_reset_deassert,
> +	.reset		= scmi_reset_reset,
> +};
> +
> +static int scmi_reset_probe(struct scmi_device *sdev)
> +{
> +	struct scmi_reset_data *data;
> +	struct device *dev = &sdev->dev;
> +	struct device_node *np = dev->of_node;
> +	const struct scmi_handle *handle = sdev->handle;
> +
> +	if (!handle || !handle->reset_ops)
> +		return -ENODEV;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->rcdev.ops = &scmi_reset_ops;
> +	data->rcdev.owner = THIS_MODULE;
> +	data->rcdev.of_node = np;

This is missing rcdev.nr_resets. When nr_resets is kept at zero, the
check in of_reset_simple_xlate will fail for any id.

> +	data->dev = dev;
> +
> +	return devm_reset_controller_register(dev, &data->rcdev);
> +}
> +
> +static const struct scmi_device_id scmi_id_table[] = {
> +	{ SCMI_PROTOCOL_RESET },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> +
> +static struct scmi_driver scmi_reset_driver = {
> +	.name = "scmi-reset",
> +	.probe = scmi_reset_probe,
> +	.id_table = scmi_id_table,
> +};
> +module_scmi_driver(scmi_reset_driver);
> +
> +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
> +MODULE_DESCRIPTION("ARM SCMI clock driver");

s/clock/reset controller/

> +MODULE_LICENSE("GPL v2");

regards
Philipp

Re: [PATCH 5/5] reset: Add support for resets provided by SCMI

[Sudeep Holla]

On Mon, Jul 29, 2019 at 11:59:15AM +0200, Philipp Zabel wrote:
> Hi Sudeep,
>
> On Fri, 2019-07-26 at 14:59 +0100, Sudeep Holla wrote:
> > On some ARM based systems, a separate Cortex-M based System Control
> > Processor(SCP) provides the overall power, clock, reset and system
> > control. System Control and Management Interface(SCMI) Message Protocol
> > is defined for the communication between the Application Cores(AP)
> > and the SCP.
> >
> > Adds support for the resets provided using SCMI protocol for performing
> > reset management of various devices present on the SoC. Various reset
> > functionalities are achieved by the means of different ARM SCMI device
> > operations provided by the ARM SCMI framework.
> >
> > Cc: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> thank you for the patch. I have a few suggestions below.
>

Thanks for reviewing so quickly, all points taken and fixed locally.
Will wait for some more time before posting v2.

[...]

> > +static int
> > +scmi_reset_assert(struct reset_controller_dev *rcdev, unsigned long id)
> > +{
> > +	struct scmi_reset_data *data = to_scmi_reset_data(rcdev);
> > +	const struct scmi_handle *handle = data->handle;
>
> This could be shortened to to_scmi_handle(rcdev), since none of the
> other fields in scmi_reset_data are used by the reset_control_ops
> callbacks.
>

Makes sense, missed to see that.

[...]

> > +
> > +static int scmi_reset_probe(struct scmi_device *sdev)
> > +{
> > +	struct scmi_reset_data *data;
> > +	struct device *dev = &sdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	const struct scmi_handle *handle = sdev->handle;
> > +
> > +	if (!handle || !handle->reset_ops)
> > +		return -ENODEV;
> > +
> > +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	data->rcdev.ops = &scmi_reset_ops;
> > +	data->rcdev.owner = THIS_MODULE;
> > +	data->rcdev.of_node = np;
>
> This is missing rcdev.nr_resets. When nr_resets is kept at zero, the
> check in of_reset_simple_xlate will fail for any id.
>

I clearly missed to do git add for the above :(. I did find these
after testing.

> > +	data->dev = dev;
> > +
> > +	return devm_reset_controller_register(dev, &data->rcdev);
> > +}
> > +
> > +static const struct scmi_device_id scmi_id_table[] = {
> > +	{ SCMI_PROTOCOL_RESET },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> > +
> > +static struct scmi_driver scmi_reset_driver = {
> > +	.name = "scmi-reset",
> > +	.probe = scmi_reset_probe,
> > +	.id_table = scmi_id_table,
> > +};
> > +module_scmi_driver(scmi_reset_driver);
> > +
> > +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
> > +MODULE_DESCRIPTION("ARM SCMI clock driver");
>
> s/clock/reset controller/
>

Stupid copy-paste :(

Thanks again.

--
Regards,
Sudeep

Re: [PATCH 2/5] firmware: arm_scmi: Make use SCMI v2.0 fastchannel for performance protocol

[Etienne Carriere]

On Fri, 26 Jul 2019 at 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> SCMI v2.0 adds support for "FastChannel" which do not use a message
> header as they are specialized for a single message.
>
> Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
> commands are supported over fastchannels. As they are optional, they
> need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
> Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.
>
> Add support for making use of these fastchannels.
>
> Cc: Ionela Voinescu <Ionela.Voinescu@arm.com>
> Cc: Chris Redpath <Chris.Redpath@arm.com>
> Cc: Quentin Perret <Quentin.Perret@arm.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/perf.c | 104 +++++++++++++++++++++++++++++--
>  1 file changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> index 6cce3e82e81e..b9144efbd6fb 100644
> --- a/drivers/firmware/arm_scmi/perf.c
> +++ b/drivers/firmware/arm_scmi/perf.c
> @@ -8,6 +8,7 @@
>  #include <linux/bits.h>
>  #include <linux/of.h>
>  #include <linux/io.h>
> +#include <linux/io-64-nonatomic-hi-lo.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_opp.h>
>  #include <linux/sort.h>
> @@ -293,7 +294,42 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> -static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> +#define SCMI_PERF_FC_RING_DB(doorbell, w)              \

Suggest to reformat into a macro style:
    #define SCMI_PERF_FC_RING_DB(doorbell, w)              \
        do { \
                (...) \
        } while (0)

> +{                                                      \
> +       u##w val = 0;                                   \
> +       struct scmi_fc_db_info *db = doorbell;          \
> +                                                       \
> +       if ((db)->mask)                                 \

remove parentheses. I would say, could simply remove `if (db->mask)` here.

> +               val = ioread##w(db->addr) & db->mask;   \
> +       iowrite##w((u##w)db->set | val, db->addr);      \
> +}
> +
> +static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
> +{
> +       if (!db || !db->addr)
> +               return;
> +
> +       if (db->width == 1)
> +               SCMI_PERF_FC_RING_DB(db, 8)
> +       else if (db->width == 2)
> +               SCMI_PERF_FC_RING_DB(db, 16)
> +       else if (db->width == 4)
> +               SCMI_PERF_FC_RING_DB(db, 32)
> +       else /* db->width == 8 */
> +#ifdef CONFIG_64BIT
> +               SCMI_PERF_FC_RING_DB(db, 64)
> +#else
> +       {
> +               u64 val = 0;
> +
> +               if (db->mask)
> +                       val = ioread64_hi_lo(db->addr) & db->mask;
> +               iowrite64_hi_lo(db->set, db->addr);

Is `value` missing here?
Why not using SCMI_PERF_FC_RING_DB(db, 64) here?



> +       }
> +#endif
> +}
> +
> +static int scmi_perf_mb_limits_set(const struct scmi_handle *handle, u32 domain,
>                                    u32 max_perf, u32 min_perf)
>  {
>         int ret;
> @@ -316,7 +352,23 @@ static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> -static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
> +static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> +                               u32 max_perf, u32 min_perf)
> +{
> +       struct scmi_perf_info *pi = handle->perf_priv;
> +       struct perf_dom_info *dom = pi->dom_info + domain;
> +
> +       if (dom->fc_info && dom->fc_info->limit_set_addr) {
> +               iowrite32(max_perf, dom->fc_info->limit_set_addr);
> +               iowrite32(min_perf, dom->fc_info->limit_set_addr + 4);
> +               scmi_perf_fc_ring_db(dom->fc_info->limit_set_db);
> +               return 0;
> +       }
> +
> +       return scmi_perf_mb_limits_set(handle, domain, max_perf, min_perf);
> +}
> +
> +static int scmi_perf_mb_limits_get(const struct scmi_handle *handle, u32 domain,
>                                    u32 *max_perf, u32 *min_perf)
>  {
>         int ret;
> @@ -342,7 +394,22 @@ static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> -static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
> +static int scmi_perf_limits_get(const struct scmi_handle *handle, u32 domain,
> +                               u32 *max_perf, u32 *min_perf)
> +{
> +       struct scmi_perf_info *pi = handle->perf_priv;
> +       struct perf_dom_info *dom = pi->dom_info + domain;
> +
> +       if (dom->fc_info && dom->fc_info->limit_get_addr) {
> +               *max_perf = ioread32(dom->fc_info->limit_get_addr);
> +               *min_perf = ioread32(dom->fc_info->limit_get_addr + 4);
> +               return 0;
> +       }
> +
> +       return scmi_perf_mb_limits_get(handle, domain, max_perf, min_perf);
> +}
> +
> +static int scmi_perf_mb_level_set(const struct scmi_handle *handle, u32 domain,
>                                   u32 level, bool poll)
>  {
>         int ret;
> @@ -365,7 +432,22 @@ static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> -static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
> +static int scmi_perf_level_set(const struct scmi_handle *handle, u32 domain,
> +                              u32 level, bool poll)
> +{
> +       struct scmi_perf_info *pi = handle->perf_priv;
> +       struct perf_dom_info *dom = pi->dom_info + domain;
> +
> +       if (dom->fc_info && dom->fc_info->level_set_addr) {
> +               iowrite32(level, dom->fc_info->level_set_addr);
> +               scmi_perf_fc_ring_db(dom->fc_info->level_set_db);
> +               return 0;
> +       }
> +
> +       return scmi_perf_mb_level_set(handle, domain, level, poll);
> +}
> +
> +static int scmi_perf_mb_level_get(const struct scmi_handle *handle, u32 domain,
>                                   u32 *level, bool poll)
>  {
>         int ret;
> @@ -387,6 +469,20 @@ static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
>         return ret;
>  }
>
> +static int scmi_perf_level_get(const struct scmi_handle *handle, u32 domain,
> +                              u32 *level, bool poll)
> +{
> +       struct scmi_perf_info *pi = handle->perf_priv;
> +       struct perf_dom_info *dom = pi->dom_info + domain;
> +
> +       if (dom->fc_info && dom->fc_info->level_get_addr) {
> +               *level = ioread32(dom->fc_info->level_get_addr);
> +               return 0;
> +       }
> +
> +       return scmi_perf_mb_level_get(handle, domain, level, poll);
> +}
> +
>  static bool scmi_perf_fc_size_is_valid(u32 msg, u32 size)
>  {
>         if ((msg == PERF_LEVEL_GET || msg == PERF_LEVEL_SET) && size == 4)
> --
> 2.17.1
>

Re: [PATCH 2/5] firmware: arm_scmi: Make use SCMI v2.0 fastchannel for performance protocol

[Sudeep Holla]

On Mon, Aug 05, 2019 at 03:16:41PM +0200, Etienne Carriere wrote:
> On Fri, 26 Jul 2019 at 16:00, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > SCMI v2.0 adds support for "FastChannel" which do not use a message
> > header as they are specialized for a single message.
> >
> > Only PERFORMANCE_LIMITS_{SET,GET} and PERFORMANCE_LEVEL_{SET,GET}
> > commands are supported over fastchannels. As they are optional, they
> > need to be discovered by PERFORMANCE_DESCRIBE_FASTCHANNEL command.
> > Further {LIMIT,LEVEL}_SET commands can have optional doorbell support.
> >
> > Add support for making use of these fastchannels.
> >
> > Cc: Ionela Voinescu <Ionela.Voinescu@arm.com>
> > Cc: Chris Redpath <Chris.Redpath@arm.com>
> > Cc: Quentin Perret <Quentin.Perret@arm.com>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/perf.c | 104 +++++++++++++++++++++++++++++--
> >  1 file changed, 100 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c
> > index 6cce3e82e81e..b9144efbd6fb 100644
> > --- a/drivers/firmware/arm_scmi/perf.c
> > +++ b/drivers/firmware/arm_scmi/perf.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/bits.h>
> >  #include <linux/of.h>
> >  #include <linux/io.h>
> > +#include <linux/io-64-nonatomic-hi-lo.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_opp.h>
> >  #include <linux/sort.h>
> > @@ -293,7 +294,42 @@ scmi_perf_describe_levels_get(const struct scmi_handle *handle, u32 domain,
> >         return ret;
> >  }
> >
> > -static int scmi_perf_limits_set(const struct scmi_handle *handle, u32 domain,
> > +#define SCMI_PERF_FC_RING_DB(doorbell, w)              \
>
> Suggest to reformat into a macro style:
>     #define SCMI_PERF_FC_RING_DB(doorbell, w)              \
>         do { \
>                 (...) \
>         } while (0)
>

Sure I can try that.

> > +{                                                      \
> > +       u##w val = 0;                                   \
> > +       struct scmi_fc_db_info *db = doorbell;          \
> > +                                                       \
> > +       if ((db)->mask)                                 \
>
> remove parentheses. I would say, could simply remove `if (db->mask)` here.

Ah, missed to drop this one. We can avoid reading the value if mask = 0,
so I prefer to keep it.
>
> > +               val = ioread##w(db->addr) & db->mask;   \
> > +       iowrite##w((u##w)db->set | val, db->addr);      \
> > +}
> > +
> > +static void scmi_perf_fc_ring_db(struct scmi_fc_db_info *db)
> > +{
> > +       if (!db || !db->addr)
> > +               return;
> > +
> > +       if (db->width == 1)
> > +               SCMI_PERF_FC_RING_DB(db, 8)
> > +       else if (db->width == 2)
> > +               SCMI_PERF_FC_RING_DB(db, 16)
> > +       else if (db->width == 4)
> > +               SCMI_PERF_FC_RING_DB(db, 32)
> > +       else /* db->width == 8 */
> > +#ifdef CONFIG_64BIT
> > +               SCMI_PERF_FC_RING_DB(db, 64)
> > +#else
> > +       {
> > +               u64 val = 0;
> > +
> > +               if (db->mask)
> > +                       val = ioread64_hi_lo(db->addr) & db->mask;
> > +               iowrite64_hi_lo(db->set, db->addr);
>
> Is `value` missing here?
> Why not using SCMI_PERF_FC_RING_DB(db, 64) here?
>

I am still using it. Just if CONFIG_64BIT is enabled and  io{read,write}64
are defined. ARM32/MIPS and other 32-bit platform build might fail
otherwise. I don't want to restrict SCMI to 64-bit platforms only.

--
Regards,
Sudeep