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

[Nicola Mazzucato]

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@xxxxxxx>
>> ---
>>  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
>