Re: [PATCH 2/4] interconnect: Add get_bw() callback

[Mike Tipton]

On 7/9/2020 4:07 AM, Georgi Djakov wrote:
> The interconnect controller hardware may support querying the current
> bandwidth settings, so add a callback for providers to implement this
> functionality if supported.
>
> Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
> ---
>   drivers/interconnect/core.c           | 3 ++-
>   include/linux/interconnect-provider.h | 2 ++
>   2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index e53adfee1ee3..edbfe8380e83 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -926,7 +926,8 @@ void icc_node_add(struct icc_node *node, struct icc_provider *provider)
>   	list_add_tail(&node->node_list, &provider->nodes);
>   
>   	/* get the bandwidth value and sync it with the hardware */
> -	if (node->init_bw && provider->set) {
> +	if (provider->get_bw && provider->set) {
> +		provider->get_bw(node, &node->init_bw);

I'm not sure what benefit this callback provides over the provider 
directly setting init_bw in the structure. Additionally, "get_bw" is a 
more generic callback than just for getting the *initial* BW 
requirement. Currently it's only used that way, but staying true to the 
spirit of the callback would require us to return the current BW at any 
point in time.

We can only detect the current HW vote at BCM-level granularity and a 
single BCM can have many nodes. So since this callback is being used to 
determine init_bw, we'd end up voting way more nodes than necessary. In 
practice we'll only need to enforce minimum initial BW on a small subset 
of them, but I wouldn't want to hardcode that init-specific logic in a 
generic "get_bw" callback.

>   		node->peak_bw = node->init_bw;
>   		provider->set(node, node);
>   	}
> diff --git a/include/linux/interconnect-provider.h b/include/linux/interconnect-provider.h
> index 153fb7616f96..329eccb19f58 100644
> --- a/include/linux/interconnect-provider.h
> +++ b/include/linux/interconnect-provider.h
> @@ -38,6 +38,7 @@ struct icc_node *of_icc_xlate_onecell(struct of_phandle_args *spec,
>    * @aggregate: pointer to device specific aggregate operation function
>    * @pre_aggregate: pointer to device specific function that is called
>    *		   before the aggregation begins (optional)
> + * @get_bw: pointer to device specific function to get current bandwidth
>    * @xlate: provider-specific callback for mapping nodes from phandle arguments
>    * @dev: the device this interconnect provider belongs to
>    * @users: count of active users
> @@ -50,6 +51,7 @@ struct icc_provider {
>   	int (*aggregate)(struct icc_node *node, u32 tag, u32 avg_bw,
>   			 u32 peak_bw, u32 *agg_avg, u32 *agg_peak);
>   	void (*pre_aggregate)(struct icc_node *node);
> +	int (*get_bw)(struct icc_node *node, u32 *bw);
>   	struct icc_node* (*xlate)(struct of_phandle_args *spec, void *data);
>   	struct device		*dev;
>   	int			users;