Re: [PATCH v6 06/20] firmware: arm_scmi: add initial support for performance protocol

[Jonathan Cameron]

On Fri, 23 Feb 2018 16:23:36 +0000
Sudeep Holla <sudeep.holla@xxxxxxx> 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 {