Re: [PATCH v4 09/13] firmware: arm_scmi: Add Power notifications support

Cristian Marussi
2020-03-09T16:39:47Z

On 09/03/2020 12:28, Jonathan Cameron wrote:
> 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>
>

Thanks

Cristian
>> ---
>> 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...
>

ops...I'll fix

>> +}
>> +
>> +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 */
>
>