diff mbox series

[v3,2/3] opp/of: Allow empty opp-table with opp-shared

Message ID 20201102120115.29993-3-nicola.mazzucato@arm.com (mailing list archive)
State New
Delegated to: viresh kumar
Headers show
Series CPUFreq: Add support for cpu performance dependencies | expand

Commit Message

Nicola Mazzucato Nov. 2, 2020, 12:01 p.m. UTC
The opp binding now allows to have an empty opp table and shared-opp to
merely describe a hw connection among devices (f/v lines).

When initialising an opp table, allow such case by:
- treating some errors as warnings
- do not mark empty tables as shared
- don't fail on empty table

Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
---
 drivers/opp/of.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

Comments

Viresh Kumar Nov. 3, 2020, 5:01 a.m. UTC | #1
On 02-11-20, 12:01, Nicola Mazzucato wrote:
> The opp binding now allows to have an empty opp table and shared-opp to
> merely describe a hw connection among devices (f/v lines).
> 
> When initialising an opp table, allow such case by:
> - treating some errors as warnings
> - do not mark empty tables as shared
> - don't fail on empty table
> 
> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
> ---
>  drivers/opp/of.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 874b58756220..b0230490bb31 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -157,6 +157,11 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
>  /*
>   * Populate all devices and opp tables which are part of "required-opps" list.
>   * Checking only the first OPP node should be enough.
> + *
> + * Corner case: empty opp table and opp-shared found. In this case we set
> + * unconditionally the opp table access to exclusive, as the opp-shared property
> + * is used purely to describe hw connections. Such information will be retrieved
> + * via dev_pm_opp_of_get_sharing_cpus().
>   */
>  static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>  					     struct device *dev,
> @@ -169,7 +174,9 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>  	/* Traversing the first OPP node is all we need */
>  	np = of_get_next_available_child(opp_np, NULL);
>  	if (!np) {
> -		dev_err(dev, "Empty OPP table\n");
> +		dev_warn(dev, "Empty OPP table\n");
> +
> +		opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE;

I am not sure I understand the reasoning behind this.

>  		return;
>  	}
>  
> @@ -377,7 +384,9 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev,
>  	struct icc_path **paths;
>  
>  	ret = _bandwidth_supported(dev, opp_table);
> -	if (ret <= 0)
> +	if (ret == -EINVAL)
> +		return 0; /* Empty OPP table is a valid corner-case, let's not fail */
> +	else if (ret <= 0)
>  		return ret;
>  
>  	ret = 0;
> -- 
> 2.27.0
Nicola Mazzucato Nov. 4, 2020, 5:54 p.m. UTC | #2
Hi Viresh, thanks for looking into this.

On 11/3/20 5:01 AM, Viresh Kumar wrote:
> On 02-11-20, 12:01, Nicola Mazzucato wrote:
>> The opp binding now allows to have an empty opp table and shared-opp to
>> merely describe a hw connection among devices (f/v lines).
>>
>> When initialising an opp table, allow such case by:
>> - treating some errors as warnings
>> - do not mark empty tables as shared
>> - don't fail on empty table
>>
>> Signed-off-by: Nicola Mazzucato <nicola.mazzucato@arm.com>
>> ---
>>  drivers/opp/of.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
>> index 874b58756220..b0230490bb31 100644
>> --- a/drivers/opp/of.c
>> +++ b/drivers/opp/of.c
>> @@ -157,6 +157,11 @@ static void _opp_table_free_required_tables(struct opp_table *opp_table)
>>  /*
>>   * Populate all devices and opp tables which are part of "required-opps" list.
>>   * Checking only the first OPP node should be enough.
>> + *
>> + * Corner case: empty opp table and opp-shared found. In this case we set
>> + * unconditionally the opp table access to exclusive, as the opp-shared property
>> + * is used purely to describe hw connections. Such information will be retrieved
>> + * via dev_pm_opp_of_get_sharing_cpus().
>>   */
>>  static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>>  					     struct device *dev,
>> @@ -169,7 +174,9 @@ static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
>>  	/* Traversing the first OPP node is all we need */
>>  	np = of_get_next_available_child(opp_np, NULL);
>>  	if (!np) {
>> -		dev_err(dev, "Empty OPP table\n");
>> +		dev_warn(dev, "Empty OPP table\n");
>> +
>> +		opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE;
> 
> I am not sure I understand the reasoning behind this.

Initially I thought to place a comment right there but I ended up with an
explanation of this case at the top of this function (the corner-case). It
probably also needs more details..
Basically, on this case - empty opp table & opp-shared - we limit the scope of
opp-shared to *only* tell us about hw description, and not marking the opp
points as shared, since they are not present in DT. It would be the equivalent
of describing that devices share clock/voltage lines, but we can't tell anything
about opp points cause they are not there (in DT).
OTOH If we don't set shared_opp to OPP_TABLE_ACCESS_EXCLUSIVE for that specific
case, we won't be able to add opps for the remaining cpus as the opp core
will find the opps as duplicated. This is a corner case, really.

Please let me know if it's not clear.

Many thanks
Nicola

> 
>>  		return;
>>  	}
>>  
>> @@ -377,7 +384,9 @@ int dev_pm_opp_of_find_icc_paths(struct device *dev,
>>  	struct icc_path **paths;
>>  
>>  	ret = _bandwidth_supported(dev, opp_table);
>> -	if (ret <= 0)
>> +	if (ret == -EINVAL)
>> +		return 0; /* Empty OPP table is a valid corner-case, let's not fail */
>> +	else if (ret <= 0)
>>  		return ret;
>>  
>>  	ret = 0;
>> -- 
>> 2.27.0
>
Viresh Kumar Nov. 5, 2020, 4:41 a.m. UTC | #3
On 04-11-20, 17:54, Nicola Mazzucato wrote:
> Initially I thought to place a comment right there but I ended up with an
> explanation of this case at the top of this function (the corner-case). It
> probably also needs more details..

I read it earlier as well but yeah, that wasn't good enough for me to
understand what you are doing.

> Basically, on this case - empty opp table & opp-shared - we limit the scope of
> opp-shared to *only* tell us about hw description, and not marking the opp
> points as shared, since they are not present in DT.

It doesn't matter where the OPP table entries are coming from. The OPP
table should be marked shared if it is found to be shared.

> It would be the equivalent
> of describing that devices share clock/voltage lines, but we can't tell anything
> about opp points cause they are not there (in DT).

Its okay, even then we should set the right flags here. It is really
confusing that we blindly set it as exclusive, even though it may be
shared.

> OTOH If we don't set shared_opp to OPP_TABLE_ACCESS_EXCLUSIVE for that specific
> case, we won't be able to add opps for the remaining cpus as the opp core
> will find the opps as duplicated. This is a corner case, really.

Hmm, I am not sure where you fail and how but this should be set to
OPP_TABLE_ACCESS_EXCLUSIVE only if the OPP table isn't shared. else it
should be OPP_TABLE_ACCESS_SHARED.
Nicola Mazzucato Nov. 16, 2020, 11:45 a.m. UTC | #4
Hi Viresh,

sorry for being late in replying.


On 11/5/20 4:41 AM, Viresh Kumar wrote:
> On 04-11-20, 17:54, Nicola Mazzucato wrote:
>> Initially I thought to place a comment right there but I ended up with an
>> explanation of this case at the top of this function (the corner-case). It
>> probably also needs more details..
> 
> I read it earlier as well but yeah, that wasn't good enough for me to
> understand what you are doing.
> 
>> Basically, on this case - empty opp table & opp-shared - we limit the scope of
>> opp-shared to *only* tell us about hw description, and not marking the opp
>> points as shared, since they are not present in DT.
> 
> It doesn't matter where the OPP table entries are coming from. The OPP
> table should be marked shared if it is found to be shared.
> 
>> It would be the equivalent
>> of describing that devices share clock/voltage lines, but we can't tell anything
>> about opp points cause they are not there (in DT).
> 
> Its okay, even then we should set the right flags here. It is really
> confusing that we blindly set it as exclusive, even though it may be
> shared.
> 
>> OTOH If we don't set shared_opp to OPP_TABLE_ACCESS_EXCLUSIVE for that specific
>> case, we won't be able to add opps for the remaining cpus as the opp core
>> will find the opps as duplicated. This is a corner case, really.
> 
> Hmm, I am not sure where you fail and how but this should be set to
> OPP_TABLE_ACCESS_EXCLUSIVE only if the OPP table isn't shared. else it
> should be OPP_TABLE_ACCESS_SHARED.
> 
Thanks for providing more details around the meaning of opp-shared, much
appreciated. I had some time to play a bit more, and yes, there is no need to
set shared_opp to OPP_TABLE_ACCESS_EXCLUSIVE. A minimal change in the driver
sequence would suffice.

I will remove that in the V4.

Many thanks,
Nicola
diff mbox series

Patch

diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 874b58756220..b0230490bb31 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -157,6 +157,11 @@  static void _opp_table_free_required_tables(struct opp_table *opp_table)
 /*
  * Populate all devices and opp tables which are part of "required-opps" list.
  * Checking only the first OPP node should be enough.
+ *
+ * Corner case: empty opp table and opp-shared found. In this case we set
+ * unconditionally the opp table access to exclusive, as the opp-shared property
+ * is used purely to describe hw connections. Such information will be retrieved
+ * via dev_pm_opp_of_get_sharing_cpus().
  */
 static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 					     struct device *dev,
@@ -169,7 +174,9 @@  static void _opp_table_alloc_required_tables(struct opp_table *opp_table,
 	/* Traversing the first OPP node is all we need */
 	np = of_get_next_available_child(opp_np, NULL);
 	if (!np) {
-		dev_err(dev, "Empty OPP table\n");
+		dev_warn(dev, "Empty OPP table\n");
+
+		opp_table->shared_opp = OPP_TABLE_ACCESS_EXCLUSIVE;
 		return;
 	}
 
@@ -377,7 +384,9 @@  int dev_pm_opp_of_find_icc_paths(struct device *dev,
 	struct icc_path **paths;
 
 	ret = _bandwidth_supported(dev, opp_table);
-	if (ret <= 0)
+	if (ret == -EINVAL)
+		return 0; /* Empty OPP table is a valid corner-case, let's not fail */
+	else if (ret <= 0)
 		return ret;
 
 	ret = 0;