mbox series

[0/4] Add interconnect sync state support

Message ID 20200709110705.30359-1-georgi.djakov@linaro.org
Headers show
Series Add interconnect sync state support | expand

Message

Georgi Djakov July 9, 2020, 11:07 a.m. UTC 
Bootloaders often leave some system resources enabled such as clocks,
regulators, interconnects etc. We want to keep these resources enabled
until all their consumers are probed. These resources are often shared,
so we must wait for all the consumers to come up, before deciding
whether to turn them off or change the configuration. This patchset is
trying to solve the above problem just for the on-chip interconnects.

The problem is solved by allowing the interconnect providers to specify
an initial bandwidth value, which is enforced during boot as a floor
value, while the requests from all consumers are being collected.
Then the sync_state() callback is used to signal when all consumers have
been probed, meaning that the floor bandwidth is not needed anymore and
the framework is ready to re-aggregate and process all requests.

Georgi Djakov (4):
  interconnect: Add sync state support
  interconnect: Add get_bw() callback
  interconnect: qcom: smd845: Use icc_sync_state
  interconnect: qcom: osm-l3: Use icc_sync_state

 drivers/interconnect/core.c           | 57 +++++++++++++++++++++++++++
 drivers/interconnect/qcom/osm-l3.c    |  9 +++++
 drivers/interconnect/qcom/sdm845.c    |  9 +++++
 include/linux/interconnect-provider.h |  5 +++
 4 files changed, 80 insertions(+)

Comments

Mike Tipton July 10, 2020, 5:04 a.m. UTC | #1 
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@linaro.org>

> ---

>   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;

>