On Fri, 23 Feb 2018 16:23:36 +0000
Sudeep Holla <[email protected]> wrote:
> The performance protocol is intended for the performance management of
> group(s) of device(s) that run in the same performance domain. It
> includes even the CPUs. A performance domain is defined by a set of
> devices that always have to run at the same performance level.
> For example, a set of CPUs that share a voltage domain, and have a
> common frequency control, is said to be in the same performance domain.
>
> The commands in this protocol provide functionality to describe the
> protocol version, describe various attribute flags, set and get the
> performance level of a domain. It also supports discovery of the list
> of performance levels supported by a performance domain, and the
> properties of each performance level.
>
<snip>
> +
> +static int scmi_perf_attributes_get(const struct scmi_handle *handle,
> + struct scmi_perf_info *pi)
> +{
> + int ret;
> + struct scmi_xfer *t;
> + struct scmi_msg_resp_perf_attributes *attr;
> +
> + ret = scmi_one_xfer_init(handle, PROTOCOL_ATTRIBUTES,
> + SCMI_PROTOCOL_PERF, 0, sizeof(*attr), &t);
> + if (ret)
> + return ret;
> +
> + attr = t->rx.buf;
> +
> + ret = scmi_do_xfer(handle, t);
> + if (!ret) {
Use a goto for the error path rather than indenting all this good path stuff.
The same would help readability in various other places.
> + u16 flags = le16_to_cpu(attr->flags);
> +
> + pi->num_domains = le16_to_cpu(attr->num_domains);
> + pi->power_scale_mw = POWER_SCALE_IN_MILLIWATT(flags);
> + pi->stats_addr = le32_to_cpu(attr->stats_addr_low) |
> + (u64)le32_to_cpu(attr->stats_addr_high) << 32;
> + pi->stats_size = le32_to_cpu(attr->stats_size);
> + }
> +
> + scmi_one_xfer_put(handle, t);
> + return ret;
> +}
> +
...
> +
> +static struct scmi_perf_ops perf_ops = {
> + .limits_set = scmi_perf_limits_set,
> + .limits_get = scmi_perf_limits_get,
> + .level_set = scmi_perf_level_set,
> + .level_get = scmi_perf_level_get,
> + .device_domain_id = scmi_dev_domain_id,
> + .get_transition_latency = scmi_dvfs_get_transition_latency,
> + .add_opps_to_device = scmi_dvfs_add_opps_to_device,
> + .freq_set = scmi_dvfs_freq_set,
> + .freq_get = scmi_dvfs_freq_get,
> +};
> +
> +static int scmi_perf_protocol_init(struct scmi_handle *handle)
> +{
> + int domain;
> + u32 version;
> + struct scmi_perf_info *pinfo;
> +
> + scmi_version_get(handle, SCMI_PROTOCOL_PERF, &version);
> +
> + dev_dbg(handle->dev, "Performance 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_perf_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 perf_dom_info *dom = pinfo->dom_info + domain;
> +
> + scmi_perf_domain_attributes_get(handle, domain, dom);
> + scmi_perf_describe_levels_get(handle, domain, dom);
> + }
> +
> + handle->perf_ops = &perf_ops;
> + handle->perf_priv = pinfo;
> +
> + return 0;
> +}
> +
> +static int __init scmi_perf_init(void)
> +{
> + return scmi_protocol_register(SCMI_PROTOCOL_PERF,
> + &scmi_perf_protocol_init);
> +}
> +subsys_initcall(scmi_perf_init);
> diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> index db995126134d..d80f4c9a0fad 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -44,15 +44,57 @@ struct scmi_revision_info {
> char sub_vendor_id[SCMI_MAX_STR_SIZE];
> };
>
> +struct scmi_handle;
> +
> +/**
> + * struct scmi_perf_ops - represents the various operations provided
> + * by SCMI Performance Protocol
> + *
> + * @limits_set: sets limits on the performance level of a domain
> + * @limits_get: gets limits on the performance level of a domain
> + * @level_set: sets the performance level of a domain
> + * @level_get: gets the performance level of a domain
> + * @device_domain_id: gets the scmi domain id for a given device
> + * @get_transition_latency: gets the DVFS transition latency for a given
> device
> + * @add_opps_to_device: adds all the OPPs for a given device
> + * @freq_set: sets the frequency for a given device using sustained frequency
> + * to sustained performance level mapping
> + * @freq_get: gets the frequency for a given device using sustained frequency
> + * to sustained performance level mapping
> + */
> +struct scmi_perf_ops {
> + int (*limits_set)(const struct scmi_handle *handle, u32 domain,
> + u32 max_perf, u32 min_perf);
> + int (*limits_get)(const struct scmi_handle *handle, u32 domain,
> + u32 *max_perf, u32 *min_perf);
> + int (*level_set)(const struct scmi_handle *handle, u32 domain,
> + u32 level);
> + int (*level_get)(const struct scmi_handle *handle, u32 domain,
> + u32 *level);
> + int (*device_domain_id)(struct device *dev);
> + int (*get_transition_latency)(const struct scmi_handle *handle,
> + struct device *dev);
Naming consistency would improve this.
transition_latency_get for example.
> + int (*add_opps_to_device)(const struct scmi_handle *handle,
> + struct device *dev);
> + int (*freq_set)(const struct scmi_handle *handle, u32 domain,
> + unsigned long rate);
> + int (*freq_get)(const struct scmi_handle *handle, u32 domain,
> + unsigned long *rate);
> +};
> +
> /**
> * struct scmi_handle - Handle returned to ARM SCMI clients for usage.
> *
> * @dev: pointer to the SCMI device
> * @version: pointer to the structure containing SCMI version information
> + * @perf_ops: pointer to set of performance protocol operations
> */
> struct scmi_handle {
> struct device *dev;
> struct scmi_revision_info *version;
> + struct scmi_perf_ops *perf_ops;
> + /* for protocol internal use */
> + void *perf_priv;
> };
>
> enum scmi_std_protocol {
