[PATCH 0/3] soc/tegra: bpmp: Generic PM domains for Tegra186

[PATCH 0/3] soc/tegra: bpmp: Generic PM domains for Tegra186

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Hi Rafael, Kevin, Ulf,

This series implement generic power domains for Tegra186. Since the
power partitions on Tegra186 have a non-contiguous ID range, the PM
domain core is enhanced with a way to allow PM domain providers to
override the default ->xlate() implementation. This is used to look
up the correct PM domain given the ID read from device tree.

The second patch updates the BPMP ABI header to a version that has
support for the MRQ_PG request which is used to enumerate available
power partitions and control them.

An implementation of the generic PM domains using the MRQ_PG request
is provided in patch 3.

Given the dependency of patch 3 on patch 1 I think it'd be easiest
to get an Acked-by on patch 1 and take all of these through the Tegra
tree. If that's okay with you, I can provide a stable branch with the
first patch that could be pulled into the PM tree to resolve potential
conflicts.

Thanks,
Thierry

Thierry Reding (3):
  PM / Domains: Allow overriding the ->xlate() callback
  soc/tegra: bpmp: Update ABI header
  soc/tegra: bpmp: Implement generic PM domains

 drivers/base/power/domain.c        |   8 +-
 drivers/firmware/tegra/bpmp.c      |   4 +
 drivers/soc/tegra/Kconfig          |   5 +
 drivers/soc/tegra/Makefile         |   1 +
 drivers/soc/tegra/powergate-bpmp.c | 358 +++++++++++++++++++++++++++++++
 include/linux/pm_domain.h          |   4 +
 include/soc/tegra/bpmp-abi.h       | 418 ++++++++++++++++++++++++++++++++++++-
 include/soc/tegra/bpmp.h           |  12 ++
 8 files changed, 796 insertions(+), 14 deletions(-)
 create mode 100644 drivers/soc/tegra/powergate-bpmp.c

-- 
2.12.0

[PATCH 1/3] PM / Domains: Allow overriding the ->xlate() callback

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Allow generic power domain providers to override the ->xlate() callback
in case the default genpd_xlate_onecell() translation callback is not
good enough.

One potential use-case for this is to allow generic power domains to be
specified by an ID rather than an index.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/base/power/domain.c | 8 ++++----
 include/linux/pm_domain.h   | 4 ++++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e697dec9d25b..c102557f6c7e 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1570,9 +1570,6 @@ EXPORT_SYMBOL_GPL(pm_genpd_remove);
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
 
-typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
-						   void *data);
-
 /*
  * Device Tree based PM domain providers.
  *
@@ -1730,6 +1727,9 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 
 	mutex_lock(&gpd_list_lock);
 
+	if (!data->xlate)
+		data->xlate = genpd_xlate_onecell;
+
 	for (i = 0; i < data->num_domains; i++) {
 		if (!data->domains[i])
 			continue;
@@ -1740,7 +1740,7 @@ int of_genpd_add_provider_onecell(struct device_node *np,
 		data->domains[i]->has_provider = true;
 	}
 
-	ret = genpd_add_provider(np, genpd_xlate_onecell, data);
+	ret = genpd_add_provider(np, data->xlate, data);
 	if (ret < 0)
 		goto error;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 5339ed5bd6f9..5163f371a308 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -204,9 +204,13 @@ static inline void pm_genpd_syscore_poweron(struct device *dev) {}
 /* OF PM domain providers */
 struct of_device_id;
 
+typedef struct generic_pm_domain *(*genpd_xlate_t)(struct of_phandle_args *args,
+						   void *data);
+
 struct genpd_onecell_data {
 	struct generic_pm_domain **domains;
 	unsigned int num_domains;
+	genpd_xlate_t xlate;
 };
 
 #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
-- 
2.12.0

Re: [PATCH 1/3] PM / Domains: Allow overriding the ->xlate() callback

[Jon Hunter]

On 14/03/17 19:15, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> Allow generic power domain providers to override the ->xlate() callback
> in case the default genpd_xlate_onecell() translation callback is not
> good enough.
> 
> One potential use-case for this is to allow generic power domains to be
> specified by an ID rather than an index.

Are you sure this is necessary, because I recall that the genpd core
does allow for holes in the domain array (which is what we have). See
commit 609bed67bd8f ('PM / Domains: Allow holes in genpd_data.domains
array').

Cheers
Jon

-- 
nvpublic

Re: [PATCH 1/3] PM / Domains: Allow overriding the ->xlate() callback

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]

On Tue, Mar 14, 2017 at 08:34:01PM +0000, Jon Hunter wrote:
> 
> On 14/03/17 19:15, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > Allow generic power domain providers to override the ->xlate() callback
> > in case the default genpd_xlate_onecell() translation callback is not
> > good enough.
> > 
> > One potential use-case for this is to allow generic power domains to be
> > specified by an ID rather than an index.
> 
> Are you sure this is necessary, because I recall that the genpd core
> does allow for holes in the domain array (which is what we have). See
> commit 609bed67bd8f ('PM / Domains: Allow holes in genpd_data.domains
> array').

I suppose we could use that, but it feels like the wrong solution to me.
Why would we want to deal with holes in the array when we can simply do
all the required work at probe time and add only the existing domains in
any particular SoC.

For example on Tegra186 we've got a maximum ID of 43, so we'd need to
allocate 44 pointers, but we only use 17 of those IDs. People usually
reply to that that newer SoCs come with enough memory to make this
waste irrelevant. I don't buy that. Why should we waste memory if it
can be easily avoided?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] PM / Domains: Allow overriding the ->xlate() callback

[Jon Hunter]

On 15/03/17 13:36, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Tue, Mar 14, 2017 at 08:34:01PM +0000, Jon Hunter wrote:
>>
>> On 14/03/17 19:15, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Allow generic power domain providers to override the ->xlate() callback
>>> in case the default genpd_xlate_onecell() translation callback is not
>>> good enough.
>>>
>>> One potential use-case for this is to allow generic power domains to be
>>> specified by an ID rather than an index.
>>
>> Are you sure this is necessary, because I recall that the genpd core
>> does allow for holes in the domain array (which is what we have). See
>> commit 609bed67bd8f ('PM / Domains: Allow holes in genpd_data.domains
>> array').
> 
> I suppose we could use that, but it feels like the wrong solution to me.
> Why would we want to deal with holes in the array when we can simply do
> all the required work at probe time and add only the existing domains in
> any particular SoC.
> 
> For example on Tegra186 we've got a maximum ID of 43, so we'd need to
> allocate 44 pointers, but we only use 17 of those IDs. People usually
> reply to that that newer SoCs come with enough memory to make this
> waste irrelevant. I don't buy that. Why should we waste memory if it
> can be easily avoided?

I was wondering how many holes we have. That seems a bit excessive and
so I would agree it is a waste.

Cheers
Jon

-- 
nvpublic

[PATCH 2/3] soc/tegra: bpmp: Update ABI header

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Update the BPMP ABI header to a more recent version. The new version
adds support for a new powergating ABI as well as access to the ring
buffer console, which allows debug messages to be output to the BPMP
debug console.

Some of the previously undocumented fields have been documented and
missing bitmasks have been added. Furthermore the MRQ_RESET request
now has a sub-command that allows to determine the maximum ID which
in turn allows the resets to be enumerated, thereby allowing drivers
to become agnostic of the Tegra generation.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 include/soc/tegra/bpmp-abi.h | 418 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 408 insertions(+), 10 deletions(-)

diff --git a/include/soc/tegra/bpmp-abi.h b/include/soc/tegra/bpmp-abi.h
index 0aaef5960e29..98d8d38b99a1 100644
--- a/include/soc/tegra/bpmp-abi.h
+++ b/include/soc/tegra/bpmp-abi.h
@@ -81,13 +81,18 @@
  * Provides the MRQ number for the MRQ message: #mrq. The remainder of
  * the MRQ message is a payload (immediately following the
  * mrq_request) whose format depends on mrq.
- *
- * @todo document the flags
  */
 struct mrq_request {
 	/** @brief MRQ number of the request */
 	uint32_t mrq;
-	/** @brief flags for the request */
+	/**
+	 * @brief flags providing follow up directions to the receiver
+	 *
+	 * | Bit | Description                                |
+	 * |-----|--------------------------------------------|
+	 * | 1   | ring the sender's doorbell when responding |
+	 * | 0   | should be 1                                |
+	 */
 	uint32_t flags;
 } __ABI_PACKED;
 
@@ -99,13 +104,11 @@ struct mrq_request {
  *  remainder of the MRQ response is a payload (immediately following
  *  the mrq_response) whose format depends on the associated
  *  mrq_request::mrq
- *
- * @todo document the flags
  */
 struct mrq_response {
 	/** @brief error code for the MRQ request itself */
 	int32_t err;
-	/** @brief flags for the response */
+	/** @brief reserved for future use */
 	uint32_t flags;
 } __ABI_PACKED;
 
@@ -147,6 +150,8 @@ struct mrq_response {
 #define MRQ_ABI_RATCHET		29
 #define MRQ_EMC_DVFS_LATENCY	31
 #define MRQ_TRACE_ITER		64
+#define MRQ_RINGBUF_CONSOLE	65
+#define MRQ_PG			66
 
 /** @} */
 
@@ -155,7 +160,7 @@ struct mrq_response {
  * @brief Maximum MRQ code to be sent by CPU software to
  * BPMP. Subject to change in future
  */
-#define MAX_CPU_MRQ_ID		64
+#define MAX_CPU_MRQ_ID		66
 
 /**
  * @addtogroup MRQ_Payloads Message Payloads
@@ -175,6 +180,7 @@ struct mrq_response {
  *   @defgroup Vhint CPU Voltage hint
  *   @defgroup MRQ_Deprecated Deprecated MRQ messages
  *   @defgroup EMC
+ *   @defgroup RingbufConsole
  * @}
  */
 
@@ -637,7 +643,7 @@ struct mrq_debugfs_response {
  * * Initiators: Any
  * * Targets: BPMP
  * * Request Payload: @ref mrq_reset_request
- * * Response Payload: N/A
+ * * Response Payload: @ref mrq_reset_response
  */
 
 /**
@@ -647,6 +653,7 @@ enum mrq_reset_commands {
 	CMD_RESET_ASSERT = 1,
 	CMD_RESET_DEASSERT = 2,
 	CMD_RESET_MODULE = 3,
+	CMD_RESET_GET_MAX_ID = 4,
 	CMD_RESET_MAX, /* not part of ABI and subject to change */
 };
 
@@ -665,6 +672,38 @@ struct mrq_reset_request {
 } __ABI_PACKED;
 
 /**
+ * @ingroup Reset
+ * @brief Response for MRQ_RESET sub-command CMD_RESET_GET_MAX_ID. When
+ * this sub-command is not supported, firmware will return -BPMP_EBADCMD
+ * in mrq_response::err.
+ */
+struct cmd_reset_get_max_id_response {
+	/** @brief max reset id */
+	uint32_t max_id;
+} __ABI_PACKED;
+
+/**
+ * @ingroup Reset
+ * @brief Response with MRQ_RESET
+ *
+ * Each sub-command supported by @ref mrq_reset_request may return
+ * sub-command-specific data. Some do and some do not as indicated
+ * in the following table
+ *
+ * | sub-command          | payload          |
+ * |----------------------|------------------|
+ * | CMD_RESET_ASSERT     | -                |
+ * | CMD_RESET_DEASSERT   | -                |
+ * | CMD_RESET_MODULE     | -                |
+ * | CMD_RESET_GET_MAX_ID | reset_get_max_id |
+ */
+struct mrq_reset_response {
+	union {
+		struct cmd_reset_get_max_id_response reset_get_max_id;
+	} __UNION_ANON;
+} __ABI_PACKED;
+
+/**
  * @ingroup MRQ_Codes
  * @def MRQ_I2C
  * @brief issue an i2c transaction
@@ -812,6 +851,17 @@ enum {
 };
 /** @} */
 
+/**
+ * @name MRQ_CLK properties
+ * Flag bits for cmd_clk_properties_response::flags and
+ * cmd_clk_get_all_info_response::flags
+ * @{
+ */
+#define BPMP_CLK_HAS_MUX	(1 << 0)
+#define BPMP_CLK_HAS_SET_RATE	(1 << 1)
+#define BPMP_CLK_IS_ROOT	(1 << 2)
+/** @} */
+
 #define MRQ_CLK_NAME_MAXLEN	40
 #define MRQ_CLK_MAX_PARENTS	16
 
@@ -1010,7 +1060,7 @@ struct mrq_clk_response {
  *
  * * Platforms: All
  * * Initiators: Any
- * * Targets: Any
+ * * Targets: Any except DMCE
  * * Request Payload: @ref mrq_query_abi_request
  * * Response Payload: @ref mrq_query_abi_response
  */
@@ -1030,6 +1080,9 @@ struct mrq_query_abi_request {
 /**
  * @ingroup ABI_info
  * @brief response to MRQ_QUERY_ABI
+ *
+ * @note mrq_response::err of 0 indicates that the query was
+ * successful, not that the MRQ itself is supported!
  */
 struct mrq_query_abi_response {
 	/** @brief 0 if queried MRQ is supported. Else, -#BPMP_ENODEV */
@@ -1080,7 +1133,9 @@ struct mrq_pg_read_state_response {
 /**
  * @ingroup MRQ_Codes
  * @def MRQ_PG_UPDATE_STATE
- * @brief modify the power-gating state of a partition
+ * @brief modify the power-gating state of a partition. In contrast to
+ * MRQ_PG calls, the operations that change state (on/off) of power
+ * partition are reference counted.
  *
  * * Platforms: T186
  * * Initiators: Any
@@ -1126,6 +1181,171 @@ struct mrq_pg_update_state_request {
 
 /**
  * @ingroup MRQ_Codes
+ * @def MRQ_PG
+ * @brief Control power-gating state of a partition. In contrast to
+ * MRQ_PG_UPDATE_STATE, operations that change the power partition
+ * state are NOT reference counted
+ *
+ * * Platforms: T186
+ * * Initiators: Any
+ * * Targets: BPMP
+ * * Request Payload: @ref mrq_pg_request
+ * * Response Payload: @ref mrq_pg_response
+ * @addtogroup Powergating
+ * @{
+ */
+
+/**
+ * @name MRQ_PG sub-commands
+ * @{
+ */
+enum mrq_pg_cmd {
+	/**
+	 * @brief Check whether the BPMP driver supports the specified
+	 * request type
+	 *
+	 * mrq_response::err is 0 if the specified request is
+	 * supported and -#BPMP_ENODEV otherwise.
+	 */
+	CMD_PG_QUERY_ABI = 0,
+
+	/**
+	 * @brief Set the current state of specified power domain. The
+	 * possible values for power domains are defined in enum
+	 * pg_states
+	 *
+	 * mrq_response:err is
+	 * 0: Success
+	 * -#BPMP_EINVAL: Invalid request parameters
+	 */
+	CMD_PG_SET_STATE = 1,
+
+	/**
+	 * @brief Get the current state of specified power domain. The
+	 * possible values for power domains are defined in enum
+	 * pg_states
+	 *
+	 * mrq_response:err is
+	 * 0: Success
+	 * -#BPMP_EINVAL: Invalid request parameters
+	 */
+	CMD_PG_GET_STATE = 2,
+
+	/**
+	 * @brief get the name string of specified power domain id.
+	 *
+	 * mrq_response:err is
+	 * 0: Success
+	 * -#BPMP_EINVAL: Invalid request parameters
+	 */
+	CMD_PG_GET_NAME = 3,
+
+
+	/**
+	 * @brief get the highest power domain id in the system. Not
+	 * all IDs between 0 and max_id are valid IDs.
+	 *
+	 * mrq_response:err is
+	 * 0: Success
+	 * -#BPMP_EINVAL: Invalid request parameters
+	 */
+	CMD_PG_GET_MAX_ID = 4,
+};
+/** @} */
+
+#define MRQ_PG_NAME_MAXLEN	40
+
+/**
+ * @brief possible power domain states in
+ * cmd_pg_set_state_request:state and cmd_pg_get_state_response:state.
+ *  PG_STATE_OFF: power domain is OFF
+ *  PG_STATE_ON: power domain is ON
+ *  PG_STATE_RUNNING: power domain is ON and made into directly usable
+ *                    state by turning on the clocks associated with
+ *                    the domain
+ */
+enum pg_states {
+	PG_STATE_OFF = 0,
+	PG_STATE_ON = 1,
+	PG_STATE_RUNNING = 2,
+};
+
+struct cmd_pg_query_abi_request {
+	uint32_t type; /* enum mrq_pg_cmd */
+} __ABI_PACKED;
+
+struct cmd_pg_set_state_request {
+	uint32_t state; /* enum pg_states */
+} __ABI_PACKED;
+
+struct cmd_pg_get_state_response {
+	uint32_t state; /* enum pg_states */
+} __ABI_PACKED;
+
+struct cmd_pg_get_name_response {
+	uint8_t name[MRQ_PG_NAME_MAXLEN];
+} __ABI_PACKED;
+
+struct cmd_pg_get_max_id_response {
+	uint32_t max_id;
+} __ABI_PACKED;
+
+/**
+ * @ingroup Powergating
+ * @brief request with #MRQ_PG
+ *
+ * Used by the sender of an #MRQ_PG message to control power
+ * partitions. The pg_request is split into several sub-commands. Some
+ * sub-commands require no additional data. Others have a sub-command
+ * specific payload
+ *
+ * |sub-command                 |payload                |
+ * |----------------------------|-----------------------|
+ * |CMD_PG_QUERY_ABI            | query_abi             |
+ * |CMD_PG_SET_STATE            | set_state             |
+ * |CMD_PG_GET_STATE            | -                     |
+ * |CMD_PG_GET_NAME             | -                     |
+ * |CMD_PG_GET_MAX_ID           | -                     |
+ *
+ */
+
+struct mrq_pg_request {
+	uint32_t cmd;
+	uint32_t id;
+	union {
+		struct cmd_pg_query_abi_request query_abi;
+		struct cmd_pg_set_state_request set_state;
+	} __UNION_ANON;
+} __ABI_PACKED;
+
+/**
+ * @ingroup Powergating
+ * @brief response to MRQ_PG
+ *
+ * Each sub-command supported by @ref mrq_pg_request may return
+ * sub-command-specific data. Some do and some do not as indicated in
+ * the following table
+ *
+ * |sub-command                 |payload                |
+ * |----------------------------|-----------------------|
+ * |CMD_PG_QUERY_ABI            | -                     |
+ * |CMD_PG_SET_STATE            | -                     |
+ * |CMD_PG_GET_STATE            | get_state             |
+ * |CMD_PG_GET_NAME             | get_name              |
+ * |CMD_PG_GET_MAX_ID           | get_max_id            |
+ *
+ */
+
+struct mrq_pg_response {
+	union {
+		struct cmd_pg_get_state_response get_state;
+		struct cmd_pg_get_name_response get_name;
+		struct cmd_pg_get_max_id_response get_max_id;
+	} __UNION_ANON;
+} __ABI_PACKED;
+
+/**
+ * @ingroup MRQ_Codes
  * @def MRQ_THERMAL
  * @brief interact with BPMP thermal framework
  *
@@ -1529,6 +1749,184 @@ struct mrq_trace_iter_request {
 
 /** @} */
 
+/**
+ * @ingroup MRQ_Codes
+ * @def MRQ_RINGBUF_CONSOLE
+ * @brief A ring buffer debug console for BPMP
+ * @addtogroup RingbufConsole
+ *
+ * The ring buffer debug console aims to be a substitute for the UART debug
+ * console. The debug console is implemented with two ring buffers in the
+ * BPMP-FW, the RX (receive) and TX (transmit) buffers. Characters can be read
+ * and written to the buffers by the host via the MRQ interface.
+ *
+ * @{
+ */
+
+/**
+ * @brief Maximum number of bytes transferred in a single write command to the
+ * BPMP
+ *
+ * This is determined by the number of free bytes in the message struct,
+ * rounded down to a multiple of four.
+ */
+#define MRQ_RINGBUF_CONSOLE_MAX_WRITE_LEN 112
+
+/**
+ * @brief Maximum number of bytes transferred in a single read command to the
+ * BPMP
+ *
+ * This is determined by the number of free bytes in the message struct,
+ * rounded down to a multiple of four.
+ */
+#define MRQ_RINGBUF_CONSOLE_MAX_READ_LEN 116
+
+enum mrq_ringbuf_console_host_to_bpmp_cmd {
+	/**
+	 * @brief Check whether the BPMP driver supports the specified request
+	 * type
+	 *
+	 * mrq_response::err is 0 if the specified request is supported and
+	 * -#BPMP_ENODEV otherwise
+	 */
+	CMD_RINGBUF_CONSOLE_QUERY_ABI = 0,
+	/**
+	 * @brief Perform a read operation on the BPMP TX buffer
+	 *
+	 * mrq_response::err is 0
+	 */
+	CMD_RINGBUF_CONSOLE_READ = 1,
+	/**
+	 * @brief Perform a write operation on the BPMP RX buffer
+	 *
+	 * mrq_response::err is 0 if the operation was successful and
+	 * -#BPMP_ENODEV otherwise
+	 */
+	CMD_RINGBUF_CONSOLE_WRITE = 2,
+	/**
+	 * @brief Get the length of the buffer and the physical addresses of
+	 * the buffer data and the head and tail counters
+	 *
+	 * mrq_response::err is 0 if the operation was successful and
+	 * -#BPMP_ENODEV otherwise
+	 */
+	CMD_RINGBUF_CONSOLE_GET_FIFO = 3,
+};
+
+/**
+ * @ingroup RingbufConsole
+ * @brief Host->BPMP request data for request type
+ * #CMD_RINGBUF_CONSOLE_QUERY_ABI
+ */
+struct cmd_ringbuf_console_query_abi_req {
+	/** @brief Command identifier to be queried */
+	uint32_t cmd;
+} __ABI_PACKED;
+
+/** @private */
+struct cmd_ringbuf_console_query_abi_resp {
+	EMPTY
+} __ABI_PACKED;
+
+/**
+ * @ingroup RingbufConsole
+ * @brief Host->BPMP request data for request type #CMD_RINGBUF_CONSOLE_READ
+ */
+struct cmd_ringbuf_console_read_req {
+	/**
+	 * @brief Number of bytes requested to be read from the BPMP TX buffer
+	 */
+	uint8_t len;
+} __ABI_PACKED;
+
+/**
+ * @ingroup RingbufConsole
+ * @brief BPMP->Host response data for request type #CMD_RINGBUF_CONSOLE_READ
+ */
+struct cmd_ringbuf_console_read_resp {
+	/** @brief The actual data read from the BPMP TX buffer */
+	uint8_t data[MRQ_RINGBUF_CONSOLE_MAX_READ_LEN];
+	/** @brief Number of bytes in cmd_ringbuf_console_read_resp::data */
+	uint8_t len;
+} __ABI_PACKED;
+
+/**
+ * @ingroup RingbufConsole
+ * @brief Host->BPMP request data for request type #CMD_RINGBUF_CONSOLE_WRITE
+ */
+struct cmd_ringbuf_console_write_req {
+	/** @brief The actual data to be written to the BPMP RX buffer */
+	uint8_t data[MRQ_RINGBUF_CONSOLE_MAX_WRITE_LEN];
+	/** @brief Number of bytes in cmd_ringbuf_console_write_req::data */
+	uint8_t len;
+} __ABI_PACKED;
+
+/**
+ * @ingroup RingbufConsole
+ * @brief BPMP->Host response data for request type #CMD_RINGBUF_CONSOLE_WRITE
+ */
+struct cmd_ringbuf_console_write_resp {
+	/** @brief Number of bytes of available space in the BPMP RX buffer */
+	uint32_t space_avail;
+	/** @brief Number of bytes that were written to the BPMP RX buffer */
+	uint8_t len;
+} __ABI_PACKED;
+
+/** @private */
+struct cmd_ringbuf_console_get_fifo_req {
+	EMPTY
+} __ABI_PACKED;
+
+/**
+ * @ingroup RingbufConsole
+ * @brief BPMP->Host reply data for request type #CMD_RINGBUF_CONSOLE_GET_FIFO
+ */
+struct cmd_ringbuf_console_get_fifo_resp {
+	/** @brief Physical address of the BPMP TX buffer */
+	uint64_t bpmp_tx_buf_addr;
+	/** @brief Physical address of the BPMP TX buffer head counter */
+	uint64_t bpmp_tx_head_addr;
+	/** @brief Physical address of the BPMP TX buffer tail counter */
+	uint64_t bpmp_tx_tail_addr;
+	/** @brief Length of the BPMP TX buffer */
+	uint32_t bpmp_tx_buf_len;
+} __ABI_PACKED;
+
+/**
+ * @ingroup RingbufConsole
+ * @brief Host->BPMP request data.
+ *
+ * Reply type is union #mrq_ringbuf_console_bpmp_to_host_response .
+ */
+struct mrq_ringbuf_console_host_to_bpmp_request {
+	/**
+	 * @brief type of request. Values listed in enum
+	 * #mrq_ringbuf_console_host_to_bpmp_cmd.
+	 */
+	uint32_t type;
+	/** @brief  request type specific parameters. */
+	union {
+		struct cmd_ringbuf_console_query_abi_req query_abi;
+		struct cmd_ringbuf_console_read_req read;
+		struct cmd_ringbuf_console_write_req write;
+		struct cmd_ringbuf_console_get_fifo_req get_fifo;
+	} __UNION_ANON;
+} __ABI_PACKED;
+
+/**
+ * @ingroup RingbufConsole
+ * @brief Host->BPMP reply data
+ *
+ * In response to struct #mrq_ringbuf_console_host_to_bpmp_request.
+ */
+union mrq_ringbuf_console_bpmp_to_host_response {
+	struct cmd_ringbuf_console_query_abi_resp query_abi;
+	struct cmd_ringbuf_console_read_resp read;
+	struct cmd_ringbuf_console_write_resp write;
+	struct cmd_ringbuf_console_get_fifo_resp get_fifo;
+} __ABI_PACKED;
+/** @} */
+
 /*
  *  4. Enumerations
  */
-- 
2.12.0

[PATCH 3/3] soc/tegra: bpmp: Implement generic PM domains

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The BPMP firmware, found on Tegra186 and later, provides an ABI that can
be used to enable and disable power to several power partitions in Tegra
SoCs. The ABI allows for enumeration of the available power partitions,
so the driver can be reused on future generations, provided the BPMP ABI
remains stable.

Based on work by Stefan Kristiansson <stefank@nvidia.com> and Mikko
Perttunen <mperttunen@nvidia.com>.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/firmware/tegra/bpmp.c      |   4 +
 drivers/soc/tegra/Kconfig          |   5 +
 drivers/soc/tegra/Makefile         |   1 +
 drivers/soc/tegra/powergate-bpmp.c | 358 +++++++++++++++++++++++++++++++++++++
 include/soc/tegra/bpmp.h           |  12 ++
 5 files changed, 380 insertions(+)
 create mode 100644 drivers/soc/tegra/powergate-bpmp.c

diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
index 84e4c9a58a0c..f11c7025b4a1 100644
--- a/drivers/firmware/tegra/bpmp.c
+++ b/drivers/firmware/tegra/bpmp.c
@@ -810,6 +810,10 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
 	if (err < 0)
 		goto free_mrq;
 
+	err = tegra_bpmp_init_powergates(bpmp);
+	if (err < 0)
+		goto free_mrq;
+
 	platform_set_drvdata(pdev, bpmp);
 
 	return 0;
diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
index 208d6edb3fdb..e052664439a1 100644
--- a/drivers/soc/tegra/Kconfig
+++ b/drivers/soc/tegra/Kconfig
@@ -106,3 +106,8 @@ config SOC_TEGRA_PMC
 
 config SOC_TEGRA_PMC_TEGRA186
 	bool
+
+config SOC_TEGRA_POWERGATE_BPMP
+	def_bool y
+	depends on PM_GENERIC_DOMAINS
+	depends on TEGRA_BPMP
diff --git a/drivers/soc/tegra/Makefile b/drivers/soc/tegra/Makefile
index b4425e4319ff..17f2e25e7954 100644
--- a/drivers/soc/tegra/Makefile
+++ b/drivers/soc/tegra/Makefile
@@ -3,3 +3,4 @@ obj-y += fuse/
 obj-y += common.o
 obj-$(CONFIG_SOC_TEGRA_PMC) += pmc.o
 obj-$(CONFIG_SOC_TEGRA_PMC_TEGRA186) += pmc-tegra186.o
+obj-$(CONFIG_SOC_TEGRA_POWERGATE_BPMP) += powergate-bpmp.o
diff --git a/drivers/soc/tegra/powergate-bpmp.c b/drivers/soc/tegra/powergate-bpmp.c
new file mode 100644
index 000000000000..d75144baa3ef
--- /dev/null
+++ b/drivers/soc/tegra/powergate-bpmp.c
@@ -0,0 +1,358 @@
+/*
+ * Copyright (c) 2016-2017, NVIDIA CORPORATION. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/slab.h>
+#include <linux/version.h>
+
+#include <soc/tegra/bpmp.h>
+#include <soc/tegra/bpmp-abi.h>
+
+struct tegra_powergate_info {
+	unsigned int id;
+	char *name;
+};
+
+struct tegra_powergate {
+	struct generic_pm_domain genpd;
+	struct tegra_bpmp *bpmp;
+	unsigned int id;
+};
+
+static inline struct tegra_powergate *
+to_tegra_powergate(struct generic_pm_domain *genpd)
+{
+	return container_of(genpd, struct tegra_powergate, genpd);
+}
+
+static int tegra_bpmp_powergate_set_state(struct tegra_bpmp *bpmp,
+					  unsigned int id, u32 state)
+{
+	struct mrq_pg_request request;
+	struct tegra_bpmp_message msg;
+
+	memset(&request, 0, sizeof(request));
+	request.cmd = CMD_PG_SET_STATE;
+	request.id = id;
+	request.set_state.state = state;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.mrq = MRQ_PG;
+	msg.tx.data = &request;
+	msg.tx.size = sizeof(request);
+
+	return tegra_bpmp_transfer(bpmp, &msg);
+}
+
+static int tegra_bpmp_powergate_get_state(struct tegra_bpmp *bpmp,
+					  unsigned int id)
+{
+	struct mrq_pg_response response;
+	struct mrq_pg_request request;
+	struct tegra_bpmp_message msg;
+	int err;
+
+	memset(&request, 0, sizeof(request));
+	request.cmd = CMD_PG_GET_STATE;
+	request.id = id;
+
+	memset(&response, 0, sizeof(response));
+
+	memset(&msg, 0, sizeof(msg));
+	msg.mrq = MRQ_PG;
+	msg.tx.data = &request;
+	msg.tx.size = sizeof(request);
+	msg.rx.data = &response;
+	msg.rx.size = sizeof(response);
+
+	err = tegra_bpmp_transfer(bpmp, &msg);
+	if (err < 0)
+		return PG_STATE_OFF;
+
+	return response.get_state.state;
+}
+
+static int tegra_bpmp_powergate_get_max_id(struct tegra_bpmp *bpmp)
+{
+	struct mrq_pg_response response;
+	struct mrq_pg_request request;
+	struct tegra_bpmp_message msg;
+	int err;
+
+	memset(&request, 0, sizeof(request));
+	request.cmd = CMD_PG_GET_MAX_ID;
+
+	memset(&response, 0, sizeof(response));
+
+	memset(&msg, 0, sizeof(msg));
+	msg.mrq = MRQ_PG;
+	msg.tx.data = &request;
+	msg.tx.size = sizeof(request);
+	msg.rx.data = &response;
+	msg.rx.size = sizeof(response);
+
+	err = tegra_bpmp_transfer(bpmp, &msg);
+	if (err < 0)
+		return err;
+
+	return response.get_max_id.max_id;
+}
+
+static char *tegra_bpmp_powergate_get_name(struct tegra_bpmp *bpmp,
+					   unsigned int id)
+{
+	struct mrq_pg_response response;
+	struct mrq_pg_request request;
+	struct tegra_bpmp_message msg;
+	int err;
+
+	memset(&request, 0, sizeof(request));
+	request.cmd = CMD_PG_GET_NAME;
+	request.id = id;
+
+	memset(&response, 0, sizeof(response));
+
+	memset(&msg, 0, sizeof(msg));
+	msg.mrq = MRQ_PG;
+	msg.tx.data = &request;
+	msg.tx.size = sizeof(request);
+	msg.rx.data = &response;
+	msg.rx.size = sizeof(response);
+
+	err = tegra_bpmp_transfer(bpmp, &msg);
+	if (err < 0)
+		return NULL;
+
+	return kstrdup(response.get_name.name, GFP_KERNEL);
+}
+
+static inline bool tegra_bpmp_powergate_is_powered(struct tegra_bpmp *bpmp,
+						   unsigned int id)
+{
+	return tegra_bpmp_powergate_get_state(bpmp, id) == PG_STATE_ON;
+}
+
+static int tegra_powergate_power_on(struct generic_pm_domain *domain)
+{
+	struct tegra_powergate *powergate = to_tegra_powergate(domain);
+	struct tegra_bpmp *bpmp = powergate->bpmp;
+
+	return tegra_bpmp_powergate_set_state(bpmp, powergate->id,
+					      PG_STATE_ON);
+}
+
+static int tegra_powergate_power_off(struct generic_pm_domain *domain)
+{
+	struct tegra_powergate *powergate = to_tegra_powergate(domain);
+	struct tegra_bpmp *bpmp = powergate->bpmp;
+
+	return tegra_bpmp_powergate_set_state(bpmp, powergate->id,
+					      PG_STATE_OFF);
+}
+
+static struct tegra_powergate *
+tegra_powergate_add(struct tegra_bpmp *bpmp,
+		    const struct tegra_powergate_info *info)
+{
+	struct tegra_powergate *powergate;
+	bool off;
+
+	off = !tegra_bpmp_powergate_is_powered(bpmp, info->id);
+
+	powergate = devm_kzalloc(bpmp->dev, sizeof(*powergate), GFP_KERNEL);
+	if (!powergate)
+		return ERR_PTR(-ENOMEM);
+
+	powergate->id = info->id;
+	powergate->bpmp = bpmp;
+
+	powergate->genpd.name = kstrdup(info->name, GFP_KERNEL);
+	powergate->genpd.power_on = tegra_powergate_power_on;
+	powergate->genpd.power_off = tegra_powergate_power_off;
+
+	pm_genpd_init(&powergate->genpd, NULL, off);
+
+	return powergate;
+}
+
+static void tegra_powergate_remove(struct tegra_powergate *powergate)
+{
+	struct generic_pm_domain *genpd = &powergate->genpd;
+	struct tegra_bpmp *bpmp = powergate->bpmp;
+	int err;
+
+	err = pm_genpd_remove(genpd);
+	if (err < 0)
+		dev_err(bpmp->dev, "failed to remove power domain %s: %d\n",
+			genpd->name, err);
+
+	kfree(genpd->name);
+}
+
+static int
+tegra_bpmp_probe_powergates(struct tegra_bpmp *bpmp,
+			    struct tegra_powergate_info **powergatesp)
+{
+	struct tegra_powergate_info *powergates;
+	unsigned int max_id, id, count = 0;
+	unsigned int num_holes = 0;
+	int err;
+
+	err = tegra_bpmp_powergate_get_max_id(bpmp);
+	if (err < 0)
+		return err;
+
+	max_id = err;
+
+	dev_dbg(bpmp->dev, "maximum powergate ID: %u\n", max_id);
+
+	powergates = kcalloc(max_id + 1, sizeof(*powergates), GFP_KERNEL);
+	if (!powergates)
+		return -ENOMEM;
+
+	for (id = 0; id <= max_id; id++) {
+		struct tegra_powergate_info *info = &powergates[count];
+
+		info->name = tegra_bpmp_powergate_get_name(bpmp, id);
+		if (!info->name || info->name[0] == '\0') {
+			num_holes++;
+			continue;
+		}
+
+		info->id = id;
+		count++;
+	}
+
+	dev_dbg(bpmp->dev, "holes: %u\n", num_holes);
+
+	*powergatesp = powergates;
+
+	return count;
+}
+
+static int tegra_bpmp_add_powergates(struct tegra_bpmp *bpmp,
+				     struct tegra_powergate_info *powergates,
+				     unsigned int count)
+{
+	struct genpd_onecell_data *genpd = &bpmp->genpd;
+	struct generic_pm_domain **domains;
+	struct tegra_powergate *powergate;
+	unsigned int i;
+	int err;
+
+	domains = kcalloc(count, sizeof(*domains), GFP_KERNEL);
+	if (!domains)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		powergate = tegra_powergate_add(bpmp, &powergates[i]);
+		if (IS_ERR(powergate)) {
+			err = PTR_ERR(powergate);
+			goto remove;
+		}
+
+		dev_dbg(bpmp->dev, "added power domain %s\n",
+			powergate->genpd.name);
+		domains[i] = &powergate->genpd;
+	}
+
+	genpd->num_domains = count;
+	genpd->domains = domains;
+
+	return 0;
+
+remove:
+	while (i--) {
+		powergate = to_tegra_powergate(domains[i]);
+		tegra_powergate_remove(powergate);
+	}
+
+	kfree(genpd->domains);
+	return err;
+}
+
+static void tegra_bpmp_remove_powergates(struct tegra_bpmp *bpmp)
+{
+	struct genpd_onecell_data *genpd = &bpmp->genpd;
+	unsigned int i = genpd->num_domains;
+	struct tegra_powergate *powergate;
+
+	while (i--) {
+		dev_dbg(bpmp->dev, "removing power domain %s\n",
+			genpd->domains[i]->name);
+		powergate = to_tegra_powergate(genpd->domains[i]);
+		tegra_powergate_remove(powergate);
+	}
+}
+
+static struct generic_pm_domain *
+tegra_powergate_xlate(struct of_phandle_args *spec, void *data)
+{
+	struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
+	struct genpd_onecell_data *genpd = data;
+	unsigned int i;
+
+	for (i = 0; i < genpd->num_domains; i++) {
+		struct tegra_powergate *powergate;
+
+		powergate = to_tegra_powergate(genpd->domains[i]);
+		if (powergate->id == spec->args[0]) {
+			domain = &powergate->genpd;
+			break;
+		}
+	}
+
+	return domain;
+}
+
+int tegra_bpmp_init_powergates(struct tegra_bpmp *bpmp)
+{
+	struct device_node *np = bpmp->dev->of_node;
+	struct tegra_powergate_info *powergates;
+	struct device *dev = bpmp->dev;
+	unsigned int count, i;
+	int err;
+
+	err = tegra_bpmp_probe_powergates(bpmp, &powergates);
+	if (err < 0)
+		return err;
+
+	count = err;
+
+	dev_dbg(dev, "%u power domains probed\n", count);
+
+	err = tegra_bpmp_add_powergates(bpmp, powergates, count);
+	if (err < 0)
+		goto free;
+
+	bpmp->genpd.xlate = tegra_powergate_xlate;
+
+	err = of_genpd_add_provider_onecell(np, &bpmp->genpd);
+	if (err < 0) {
+		dev_err(dev, "failed to add power domain provider: %d\n", err);
+		goto remove;
+	}
+
+free:
+	for (i = 0; i < count; i++)
+		kfree(powergates[i].name);
+
+	kfree(powergates);
+	return err;
+
+remove:
+	tegra_bpmp_remove_powergates(bpmp);
+	goto free;
+}
diff --git a/include/soc/tegra/bpmp.h b/include/soc/tegra/bpmp.h
index 13dcd44e91bb..9ba65222bd3f 100644
--- a/include/soc/tegra/bpmp.h
+++ b/include/soc/tegra/bpmp.h
@@ -15,6 +15,7 @@
 #define __SOC_TEGRA_BPMP_H
 
 #include <linux/mailbox_client.h>
+#include <linux/pm_domain.h>
 #include <linux/reset-controller.h>
 #include <linux/semaphore.h>
 #include <linux/types.h>
@@ -91,6 +92,8 @@ struct tegra_bpmp {
 	unsigned int num_clocks;
 
 	struct reset_controller_dev rstc;
+
+	struct genpd_onecell_data genpd;
 };
 
 struct tegra_bpmp *tegra_bpmp_get(struct device *dev);
@@ -138,4 +141,13 @@ static inline int tegra_bpmp_init_resets(struct tegra_bpmp *bpmp)
 }
 #endif
 
+#if IS_ENABLED(CONFIG_SOC_TEGRA_POWERGATE_BPMP)
+int tegra_bpmp_init_powergates(struct tegra_bpmp *bpmp);
+#else
+static inline int tegra_bpmp_init_powergates(struct tegra_bpmp *bpmp)
+{
+	return 0;
+}
+#endif
+
 #endif /* __SOC_TEGRA_BPMP_H */
-- 
2.12.0

Re: [PATCH 3/3] soc/tegra: bpmp: Implement generic PM domains

[Jon Hunter]

On 14/03/17 19:15, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The BPMP firmware, found on Tegra186 and later, provides an ABI that can
> be used to enable and disable power to several power partitions in Tegra
> SoCs. The ABI allows for enumeration of the available power partitions,
> so the driver can be reused on future generations, provided the BPMP ABI
> remains stable.
> 
> Based on work by Stefan Kristiansson <stefank@nvidia.com> and Mikko
> Perttunen <mperttunen@nvidia.com>.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/firmware/tegra/bpmp.c      |   4 +
>  drivers/soc/tegra/Kconfig          |   5 +
>  drivers/soc/tegra/Makefile         |   1 +
>  drivers/soc/tegra/powergate-bpmp.c | 358 +++++++++++++++++++++++++++++++++++++
>  include/soc/tegra/bpmp.h           |  12 ++
>  5 files changed, 380 insertions(+)
>  create mode 100644 drivers/soc/tegra/powergate-bpmp.c
> 
> diff --git a/drivers/firmware/tegra/bpmp.c b/drivers/firmware/tegra/bpmp.c
> index 84e4c9a58a0c..f11c7025b4a1 100644
> --- a/drivers/firmware/tegra/bpmp.c
> +++ b/drivers/firmware/tegra/bpmp.c
> @@ -810,6 +810,10 @@ static int tegra_bpmp_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		goto free_mrq;
>  
> +	err = tegra_bpmp_init_powergates(bpmp);
> +	if (err < 0)
> +		goto free_mrq;
> +
>  	platform_set_drvdata(pdev, bpmp);
>  
>  	return 0;
> diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
> index 208d6edb3fdb..e052664439a1 100644
> --- a/drivers/soc/tegra/Kconfig
> +++ b/drivers/soc/tegra/Kconfig
> @@ -106,3 +106,8 @@ config SOC_TEGRA_PMC
>  
>  config SOC_TEGRA_PMC_TEGRA186
>  	bool
> +
> +config SOC_TEGRA_POWERGATE_BPMP
> +	def_bool y
> +	depends on PM_GENERIC_DOMAINS
> +	depends on TEGRA_BPMP
> diff --git a/drivers/soc/tegra/Makefile b/drivers/soc/tegra/Makefile
> index b4425e4319ff..17f2e25e7954 100644
> --- a/drivers/soc/tegra/Makefile
> +++ b/drivers/soc/tegra/Makefile
> @@ -3,3 +3,4 @@ obj-y += fuse/
>  obj-y += common.o
>  obj-$(CONFIG_SOC_TEGRA_PMC) += pmc.o
>  obj-$(CONFIG_SOC_TEGRA_PMC_TEGRA186) += pmc-tegra186.o
> +obj-$(CONFIG_SOC_TEGRA_POWERGATE_BPMP) += powergate-bpmp.o
> diff --git a/drivers/soc/tegra/powergate-bpmp.c b/drivers/soc/tegra/powergate-bpmp.c
> new file mode 100644
> index 000000000000..d75144baa3ef
> --- /dev/null
> +++ b/drivers/soc/tegra/powergate-bpmp.c
> @@ -0,0 +1,358 @@
> +/*
> + * Copyright (c) 2016-2017, NVIDIA CORPORATION. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/slab.h>
> +#include <linux/version.h>
> +
> +#include <soc/tegra/bpmp.h>
> +#include <soc/tegra/bpmp-abi.h>
> +
> +struct tegra_powergate_info {
> +	unsigned int id;
> +	char *name;
> +};
> +
> +struct tegra_powergate {
> +	struct generic_pm_domain genpd;
> +	struct tegra_bpmp *bpmp;
> +	unsigned int id;
> +};
> +
> +static inline struct tegra_powergate *
> +to_tegra_powergate(struct generic_pm_domain *genpd)
> +{
> +	return container_of(genpd, struct tegra_powergate, genpd);
> +}
> +
> +static int tegra_bpmp_powergate_set_state(struct tegra_bpmp *bpmp,
> +					  unsigned int id, u32 state)
> +{
> +	struct mrq_pg_request request;
> +	struct tegra_bpmp_message msg;
> +
> +	memset(&request, 0, sizeof(request));
> +	request.cmd = CMD_PG_SET_STATE;
> +	request.id = id;
> +	request.set_state.state = state;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.mrq = MRQ_PG;
> +	msg.tx.data = &request;
> +	msg.tx.size = sizeof(request);
> +
> +	return tegra_bpmp_transfer(bpmp, &msg);
> +}
> +
> +static int tegra_bpmp_powergate_get_state(struct tegra_bpmp *bpmp,
> +					  unsigned int id)
> +{
> +	struct mrq_pg_response response;
> +	struct mrq_pg_request request;
> +	struct tegra_bpmp_message msg;
> +	int err;
> +
> +	memset(&request, 0, sizeof(request));
> +	request.cmd = CMD_PG_GET_STATE;
> +	request.id = id;
> +
> +	memset(&response, 0, sizeof(response));
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.mrq = MRQ_PG;
> +	msg.tx.data = &request;
> +	msg.tx.size = sizeof(request);
> +	msg.rx.data = &response;
> +	msg.rx.size = sizeof(response);
> +
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return PG_STATE_OFF;
> +
> +	return response.get_state.state;
> +}
> +
> +static int tegra_bpmp_powergate_get_max_id(struct tegra_bpmp *bpmp)
> +{
> +	struct mrq_pg_response response;
> +	struct mrq_pg_request request;
> +	struct tegra_bpmp_message msg;
> +	int err;
> +
> +	memset(&request, 0, sizeof(request));
> +	request.cmd = CMD_PG_GET_MAX_ID;
> +
> +	memset(&response, 0, sizeof(response));
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.mrq = MRQ_PG;
> +	msg.tx.data = &request;
> +	msg.tx.size = sizeof(request);
> +	msg.rx.data = &response;
> +	msg.rx.size = sizeof(response);
> +
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return err;
> +
> +	return response.get_max_id.max_id;
> +}
> +
> +static char *tegra_bpmp_powergate_get_name(struct tegra_bpmp *bpmp,
> +					   unsigned int id)
> +{
> +	struct mrq_pg_response response;
> +	struct mrq_pg_request request;
> +	struct tegra_bpmp_message msg;
> +	int err;
> +
> +	memset(&request, 0, sizeof(request));
> +	request.cmd = CMD_PG_GET_NAME;
> +	request.id = id;
> +
> +	memset(&response, 0, sizeof(response));
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.mrq = MRQ_PG;
> +	msg.tx.data = &request;
> +	msg.tx.size = sizeof(request);
> +	msg.rx.data = &response;
> +	msg.rx.size = sizeof(response);
> +
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err < 0)
> +		return NULL;
> +
> +	return kstrdup(response.get_name.name, GFP_KERNEL);
> +}
> +
> +static inline bool tegra_bpmp_powergate_is_powered(struct tegra_bpmp *bpmp,
> +						   unsigned int id)
> +{
> +	return tegra_bpmp_powergate_get_state(bpmp, id) == PG_STATE_ON;
> +}
> +
> +static int tegra_powergate_power_on(struct generic_pm_domain *domain)
> +{
> +	struct tegra_powergate *powergate = to_tegra_powergate(domain);
> +	struct tegra_bpmp *bpmp = powergate->bpmp;
> +
> +	return tegra_bpmp_powergate_set_state(bpmp, powergate->id,
> +					      PG_STATE_ON);
> +}
> +
> +static int tegra_powergate_power_off(struct generic_pm_domain *domain)
> +{
> +	struct tegra_powergate *powergate = to_tegra_powergate(domain);
> +	struct tegra_bpmp *bpmp = powergate->bpmp;
> +
> +	return tegra_bpmp_powergate_set_state(bpmp, powergate->id,
> +					      PG_STATE_OFF);
> +}
> +
> +static struct tegra_powergate *
> +tegra_powergate_add(struct tegra_bpmp *bpmp,
> +		    const struct tegra_powergate_info *info)
> +{
> +	struct tegra_powergate *powergate;
> +	bool off;
> +
> +	off = !tegra_bpmp_powergate_is_powered(bpmp, info->id);
> +
> +	powergate = devm_kzalloc(bpmp->dev, sizeof(*powergate), GFP_KERNEL);
> +	if (!powergate)
> +		return ERR_PTR(-ENOMEM);
> +
> +	powergate->id = info->id;
> +	powergate->bpmp = bpmp;
> +
> +	powergate->genpd.name = kstrdup(info->name, GFP_KERNEL);
> +	powergate->genpd.power_on = tegra_powergate_power_on;
> +	powergate->genpd.power_off = tegra_powergate_power_off;
> +
> +	pm_genpd_init(&powergate->genpd, NULL, off);

There was a recent change made so that pm_genpd_init() now returns
success/failure so we should check it.

> +
> +	return powergate;
> +}
> +
> +static void tegra_powergate_remove(struct tegra_powergate *powergate)
> +{
> +	struct generic_pm_domain *genpd = &powergate->genpd;
> +	struct tegra_bpmp *bpmp = powergate->bpmp;
> +	int err;
> +
> +	err = pm_genpd_remove(genpd);
> +	if (err < 0)
> +		dev_err(bpmp->dev, "failed to remove power domain %s: %d\n",
> +			genpd->name, err);
> +
> +	kfree(genpd->name);

Should we still free the memory here on failure?

> +}
> +
> +static int
> +tegra_bpmp_probe_powergates(struct tegra_bpmp *bpmp,
> +			    struct tegra_powergate_info **powergatesp)
> +{
> +	struct tegra_powergate_info *powergates;
> +	unsigned int max_id, id, count = 0;
> +	unsigned int num_holes = 0;
> +	int err;
> +
> +	err = tegra_bpmp_powergate_get_max_id(bpmp);
> +	if (err < 0)
> +		return err;
> +
> +	max_id = err;
> +
> +	dev_dbg(bpmp->dev, "maximum powergate ID: %u\n", max_id);
> +
> +	powergates = kcalloc(max_id + 1, sizeof(*powergates), GFP_KERNEL);
> +	if (!powergates)
> +		return -ENOMEM;
> +
> +	for (id = 0; id <= max_id; id++) {
> +		struct tegra_powergate_info *info = &powergates[count];
> +
> +		info->name = tegra_bpmp_powergate_get_name(bpmp, id);
> +		if (!info->name || info->name[0] == '\0') {
> +			num_holes++;
> +			continue;
> +		}
> +
> +		info->id = id;
> +		count++;
> +	}
> +
> +	dev_dbg(bpmp->dev, "holes: %u\n", num_holes);
> +
> +	*powergatesp = powergates;
> +
> +	return count;
> +}
> +
> +static int tegra_bpmp_add_powergates(struct tegra_bpmp *bpmp,
> +				     struct tegra_powergate_info *powergates,
> +				     unsigned int count)
> +{
> +	struct genpd_onecell_data *genpd = &bpmp->genpd;
> +	struct generic_pm_domain **domains;
> +	struct tegra_powergate *powergate;
> +	unsigned int i;
> +	int err;
> +
> +	domains = kcalloc(count, sizeof(*domains), GFP_KERNEL);
> +	if (!domains)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < count; i++) {
> +		powergate = tegra_powergate_add(bpmp, &powergates[i]);
> +		if (IS_ERR(powergate)) {
> +			err = PTR_ERR(powergate);
> +			goto remove;
> +		}
> +
> +		dev_dbg(bpmp->dev, "added power domain %s\n",
> +			powergate->genpd.name);
> +		domains[i] = &powergate->genpd;
> +	}
> +
> +	genpd->num_domains = count;
> +	genpd->domains = domains;
> +
> +	return 0;
> +
> +remove:
> +	while (i--) {
> +		powergate = to_tegra_powergate(domains[i]);
> +		tegra_powergate_remove(powergate);
> +	}
> +
> +	kfree(genpd->domains);
> +	return err;
> +}
> +
> +static void tegra_bpmp_remove_powergates(struct tegra_bpmp *bpmp)
> +{
> +	struct genpd_onecell_data *genpd = &bpmp->genpd;
> +	unsigned int i = genpd->num_domains;
> +	struct tegra_powergate *powergate;
> +
> +	while (i--) {
> +		dev_dbg(bpmp->dev, "removing power domain %s\n",
> +			genpd->domains[i]->name);
> +		powergate = to_tegra_powergate(genpd->domains[i]);
> +		tegra_powergate_remove(powergate);
> +	}
> +}
> +
> +static struct generic_pm_domain *
> +tegra_powergate_xlate(struct of_phandle_args *spec, void *data)
> +{
> +	struct generic_pm_domain *domain = ERR_PTR(-ENOENT);
> +	struct genpd_onecell_data *genpd = data;
> +	unsigned int i;
> +
> +	for (i = 0; i < genpd->num_domains; i++) {
> +		struct tegra_powergate *powergate;
> +
> +		powergate = to_tegra_powergate(genpd->domains[i]);
> +		if (powergate->id == spec->args[0]) {
> +			domain = &powergate->genpd;
> +			break;
> +		}
> +	}
> +
> +	return domain;
> +}
> +
> +int tegra_bpmp_init_powergates(struct tegra_bpmp *bpmp)
> +{
> +	struct device_node *np = bpmp->dev->of_node;
> +	struct tegra_powergate_info *powergates;
> +	struct device *dev = bpmp->dev;
> +	unsigned int count, i;
> +	int err;
> +
> +	err = tegra_bpmp_probe_powergates(bpmp, &powergates);
> +	if (err < 0)
> +		return err;
> +
> +	count = err;
> +
> +	dev_dbg(dev, "%u power domains probed\n", count);
> +
> +	err = tegra_bpmp_add_powergates(bpmp, powergates, count);
> +	if (err < 0)
> +		goto free;
> +
> +	bpmp->genpd.xlate = tegra_powergate_xlate;
> +
> +	err = of_genpd_add_provider_onecell(np, &bpmp->genpd);
> +	if (err < 0) {
> +		dev_err(dev, "failed to add power domain provider: %d\n", err);
> +		goto remove;
> +	}
> +
> +free:
> +	for (i = 0; i < count; i++)
> +		kfree(powergates[i].name);
> +
> +	kfree(powergates);
> +	return err;
> +
> +remove:
> +	tegra_bpmp_remove_powergates(bpmp);
> +	goto free;

Nit ... why not move the call to tegra_bpmp_remove_powergates() to where
we have the 'goto remove' and avoid this extra jump?

Otherwise looks good.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 3/3] soc/tegra: bpmp: Implement generic PM domains

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 3660 bytes --]

On Tue, Mar 14, 2017 at 08:46:52PM +0000, Jon Hunter wrote:
> On 14/03/17 19:15, Thierry Reding wrote:
[...]
> > diff --git a/drivers/soc/tegra/powergate-bpmp.c b/drivers/soc/tegra/powergate-bpmp.c
[...]
> > +static struct tegra_powergate *
> > +tegra_powergate_add(struct tegra_bpmp *bpmp,
> > +		    const struct tegra_powergate_info *info)
> > +{
> > +	struct tegra_powergate *powergate;
> > +	bool off;
> > +
> > +	off = !tegra_bpmp_powergate_is_powered(bpmp, info->id);
> > +
> > +	powergate = devm_kzalloc(bpmp->dev, sizeof(*powergate), GFP_KERNEL);
> > +	if (!powergate)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	powergate->id = info->id;
> > +	powergate->bpmp = bpmp;
> > +
> > +	powergate->genpd.name = kstrdup(info->name, GFP_KERNEL);
> > +	powergate->genpd.power_on = tegra_powergate_power_on;
> > +	powergate->genpd.power_off = tegra_powergate_power_off;
> > +
> > +	pm_genpd_init(&powergate->genpd, NULL, off);
> 
> There was a recent change made so that pm_genpd_init() now returns
> success/failure so we should check it.

Okay, will do.

> > +static void tegra_powergate_remove(struct tegra_powergate *powergate)
> > +{
> > +	struct generic_pm_domain *genpd = &powergate->genpd;
> > +	struct tegra_bpmp *bpmp = powergate->bpmp;
> > +	int err;
> > +
> > +	err = pm_genpd_remove(genpd);
> > +	if (err < 0)
> > +		dev_err(bpmp->dev, "failed to remove power domain %s: %d\n",
> > +			genpd->name, err);
> > +
> > +	kfree(genpd->name);
> 
> Should we still free the memory here on failure?

I think at this point we loose either way. In all current cases this
should not fail, since we only use this to remove domains on probe
failure, so before the provider is registered and before any devices
actually start using the domains.

In the case where we want to remove upon driver unload, it's fairly
unlikely that freeing the memory or not would be relevant. Not freeing
it would mean leaking the memory because the driver will go away and
hence there won't be a chance to clean up afterwards. And given that
the driver will disappear anyway, further access to the genpd is very
likely to cause a crash, due to some other resources being cleaned up
as part of the driver removal.

[...]
> > +int tegra_bpmp_init_powergates(struct tegra_bpmp *bpmp)
> > +{
> > +	struct device_node *np = bpmp->dev->of_node;
> > +	struct tegra_powergate_info *powergates;
> > +	struct device *dev = bpmp->dev;
> > +	unsigned int count, i;
> > +	int err;
> > +
> > +	err = tegra_bpmp_probe_powergates(bpmp, &powergates);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	count = err;
> > +
> > +	dev_dbg(dev, "%u power domains probed\n", count);
> > +
> > +	err = tegra_bpmp_add_powergates(bpmp, powergates, count);
> > +	if (err < 0)
> > +		goto free;
> > +
> > +	bpmp->genpd.xlate = tegra_powergate_xlate;
> > +
> > +	err = of_genpd_add_provider_onecell(np, &bpmp->genpd);
> > +	if (err < 0) {
> > +		dev_err(dev, "failed to add power domain provider: %d\n", err);
> > +		goto remove;
> > +	}
> > +
> > +free:
> > +	for (i = 0; i < count; i++)
> > +		kfree(powergates[i].name);
> > +
> > +	kfree(powergates);
> > +	return err;
> > +
> > +remove:
> > +	tegra_bpmp_remove_powergates(bpmp);
> > +	goto free;
> 
> Nit ... why not move the call to tegra_bpmp_remove_powergates() to where
> we have the 'goto remove' and avoid this extra jump?

Yeah, I can do that. The original motivation for doing it this way was
to make it less error prone to leak the domains if new code was added,
but that seems fairly unlikely and we can catch that during review.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] soc/tegra: bpmp: Implement generic PM domains

[Mikko Perttunen]

On 14.03.2017 21:15, Thierry Reding wrote:
> ...
> +
> +static inline bool tegra_bpmp_powergate_is_powered(struct tegra_bpmp *bpmp,
> +						   unsigned int id)
> +{
> +	return tegra_bpmp_powergate_get_state(bpmp, id) == PG_STATE_ON;
> +}

This should also check for PG_STATE_RUNNING and consider that to be on. 
There's this tristate thing for legacy reasons..

> ...

Cheers,
Mikko.

Re: [PATCH 3/3] soc/tegra: bpmp: Implement generic PM domains

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 546 bytes --]

On Wed, Mar 15, 2017 at 12:49:47PM +0200, Mikko Perttunen wrote:
> On 14.03.2017 21:15, Thierry Reding wrote:
> > ...
> > +
> > +static inline bool tegra_bpmp_powergate_is_powered(struct tegra_bpmp *bpmp,
> > +						   unsigned int id)
> > +{
> > +	return tegra_bpmp_powergate_get_state(bpmp, id) == PG_STATE_ON;
> > +}
> 
> This should also check for PG_STATE_RUNNING and consider that to be on.
> There's this tristate thing for legacy reasons..

Okay, I've made this check for != PG_STATE_OFF to avoid multiple cases.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]