Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 9 Mar 2020 12:28:53 +0000
From:   Jonathan Cameron <Jonathan.Cameron@Huawei.com>
To:     Cristian Marussi <cristian.marussi@arm.com>
CC:     <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>, <sudeep.holla@arm.com>,
        <lukasz.luba@arm.com>, <james.quinlan@broadcom.com>
Subject: Re: [PATCH v4 09/13] firmware: arm_scmi: Add Power notifications
 support
Message-ID: <20200309122853.000019b0@Huawei.com>
In-Reply-To: <20200304162558.48836-10-cristian.marussi@arm.com>
References: <20200304162558.48836-1-cristian.marussi@arm.com>
        <20200304162558.48836-10-cristian.marussi@arm.com>
Organization: Huawei Technologies Research and Development (UK) Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, 4 Mar 2020 16:25:54 +0000
Cristian Marussi <cristian.marussi@arm.com> wrote:

> Make SCMI Power protocol register with the notification core.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>

One comment inline on an unusual code construct, otherwise fine.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> V3 --> V4
> - scmi_event field renamed
> V2 --> V3
> - added handle awareness
> V1 --> V2
> - simplified .set_notify_enabled() implementation moving the ALL_SRCIDs
>   logic out of protocol. ALL_SRCIDs logic is now in charge of the
>   notification core, together with proper reference counting of enables
> - switched to devres protocol-registration
> ---
>  drivers/firmware/arm_scmi/power.c | 123 ++++++++++++++++++++++++++++++
>  include/linux/scmi_protocol.h     |  15 ++++
>  2 files changed, 138 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/power.c b/drivers/firmware/arm_scmi/power.c
> index cf7f0312381b..281da7e7e33a 100644
> --- a/drivers/firmware/arm_scmi/power.c
> +++ b/drivers/firmware/arm_scmi/power.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include "common.h"
> +#include "notify.h"
>  
>  enum scmi_power_protocol_cmd {
>  	POWER_DOMAIN_ATTRIBUTES = 0x3,
> @@ -48,6 +49,12 @@ struct scmi_power_state_notify {
>  	__le32 notify_enable;
>  };
>  
> +struct scmi_power_state_notify_payld {
> +	__le32 agent_id;
> +	__le32 domain_id;
> +	__le32 power_state;
> +};
> +
>  struct power_dom_info {
>  	bool state_set_sync;
>  	bool state_set_async;
> @@ -63,6 +70,11 @@ struct scmi_power_info {
>  	struct power_dom_info *dom_info;
>  };
>  
> +static enum scmi_power_protocol_cmd evt_2_cmd[] = {
> +	POWER_STATE_NOTIFY,
> +	POWER_STATE_CHANGE_REQUESTED_NOTIFY,
> +};
> +
>  static int scmi_power_attributes_get(const struct scmi_handle *handle,
>  				     struct scmi_power_info *pi)
>  {
> @@ -186,6 +198,111 @@ static struct scmi_power_ops power_ops = {
>  	.state_get = scmi_power_state_get,
>  };
>  
> +static int scmi_power_request_notify(const struct scmi_handle *handle,
> +				     u32 domain, int message_id, bool enable)
> +{
> +	int ret;
> +	struct scmi_xfer *t;
> +	struct scmi_power_state_notify *notify;
> +
> +	ret = scmi_xfer_get_init(handle, message_id, SCMI_PROTOCOL_POWER,
> +				 sizeof(*notify), 0, &t);
> +	if (ret)
> +		return ret;
> +
> +	notify = t->tx.buf;
> +	notify->domain = cpu_to_le32(domain);
> +	notify->notify_enable = enable ? cpu_to_le32(BIT(0)) : 0;
> +
> +	ret = scmi_do_xfer(handle, t);
> +
> +	scmi_xfer_put(handle, t);
> +	return ret;
> +}
> +
> +static bool scmi_power_set_notify_enabled(const struct scmi_handle *handle,
> +					  u8 evt_id, u32 src_id, bool enable)
> +{
> +	int ret, cmd_id;
> +
> +	cmd_id = MAP_EVT_TO_ENABLE_CMD(evt_id, evt_2_cmd);
> +	if (cmd_id < 0)
> +		return false;
> +
> +	ret = scmi_power_request_notify(handle, src_id, cmd_id, enable);
> +	if (ret)
> +		pr_warn("SCMI Notifications - Proto:%X - FAIL_ENABLE - evt[%X] dom[%d] - ret:%d\n",
> +				SCMI_PROTOCOL_POWER, evt_id, src_id, ret);
> +
> +	return !ret ? true : false;

	return !ret;

	Is the same thing...

> +}
> +
> +static void *scmi_power_fill_custom_report(u8 evt_id, u64 timestamp,
> +					   const void *payld, size_t payld_sz,
> +					   void *report, u32 *src_id)
> +{
> +	void *rep = NULL;
> +
> +	switch (evt_id) {
> +	case POWER_STATE_CHANGED:
> +	{
> +		const struct scmi_power_state_notify_payld *p = payld;
> +		struct scmi_power_state_changed_report *r = report;
> +
> +		if (sizeof(*p) != payld_sz)
> +			break;
> +
> +		r->timestamp = timestamp;
> +		r->agent_id = le32_to_cpu(p->agent_id);
> +		r->domain_id = le32_to_cpu(p->domain_id);
> +		r->power_state = le32_to_cpu(p->power_state);
> +		*src_id = r->domain_id;
> +		rep = r;
> +		break;
> +	}
> +	case POWER_STATE_CHANGE_REQUESTED:
> +	{
> +		const struct scmi_power_state_notify_payld *p = payld;
> +		struct scmi_power_state_change_requested_report *r = report;
> +
> +		if (sizeof(*p) != payld_sz)
> +			break;
> +
> +		r->timestamp = timestamp;
> +		r->agent_id = le32_to_cpu(p->agent_id);
> +		r->domain_id = le32_to_cpu(p->domain_id);
> +		r->power_state = le32_to_cpu(p->power_state);
> +		*src_id = r->domain_id;
> +		rep = r;
> +		break;
> +	}
> +	default:
> +		break;
> +	}
> +
> +	return rep;
> +}
> +
> +static const struct scmi_event power_events[] = {
> +	{
> +		.id = POWER_STATE_CHANGED,
> +		.max_payld_sz = 12,
> +		.max_report_sz =
> +			sizeof(struct scmi_power_state_changed_report),
> +	},
> +	{
> +		.id = POWER_STATE_CHANGE_REQUESTED,
> +		.max_payld_sz = 12,
> +		.max_report_sz =
> +			sizeof(struct scmi_power_state_change_requested_report),
> +	},
> +};
> +
> +static const struct scmi_protocol_event_ops power_event_ops = {
> +	.set_notify_enabled = scmi_power_set_notify_enabled,
> +	.fill_custom_report = scmi_power_fill_custom_report,
> +};
> +
>  static int scmi_power_protocol_init(struct scmi_handle *handle)
>  {
>  	int domain;
> @@ -214,6 +331,12 @@ static int scmi_power_protocol_init(struct scmi_handle *handle)
>  		scmi_power_domain_attributes_get(handle, domain, dom);
>  	}
>  
> +	scmi_register_protocol_events(handle,
> +				      SCMI_PROTOCOL_POWER, PAGE_SIZE,
> +				      &power_event_ops, power_events,
> +				      ARRAY_SIZE(power_events),
> +				      pinfo->num_domains);
> +
>  	pinfo->version = version;
>  	handle->power_ops = &power_ops;
>  	handle->power_priv = pinfo;
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index 797e1e03ae52..baa117f9eda3 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -377,4 +377,19 @@ typedef int (*scmi_prot_init_fn_t)(struct scmi_handle *);
>  int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn);
>  void scmi_protocol_unregister(int protocol_id);
>  
> +/* SCMI Notification API - Custom Event Reports */
> +struct scmi_power_state_changed_report {
> +	ktime_t	timestamp;
> +	u32	agent_id;
> +	u32	domain_id;
> +	u32	power_state;
> +};
> +
> +struct scmi_power_state_change_requested_report {
> +	ktime_t	timestamp;
> +	u32	agent_id;
> +	u32	domain_id;
> +	u32	power_state;
> +};
> +
>  #endif /* _LINUX_SCMI_PROTOCOL_H */