Re: [RFC PATCH] PM / OPP: Don't support OPP if it provides supported-hw but platform does not

[Dave Gerlach <d-gerlach@ti.com>]

Hi,
On 09/23/2016 03:07 PM, Dave Gerlach wrote:
> The OPP framework allows each OPP to set a opp-supported-hw property
> which provides values that are matched against supported_hw values
> provided by the platform to limit support for certain OPPs on specific
> hardware. Currently, if the platform does not set supported_hw values,
> all OPPs are interpreted as supported, even if they have provided their
> own opp-supported-hw values.
>
> If an OPP has provided opp-supported-hw, it is indicating that there is
> some specific hardware configuration it is supported by. These constraints
> should be honored, and if no supported_hw has been provided by the
> platform, there is no way to determine if that OPP is actually supported,
> so it should be marked as not supported.
>
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> ---

Currently only the sti-cpufreq and forthcoming ti-cpufreq [1] driver are 
making use of dev_pm_opp_set_supported_hw so maybe nobody has seen this 
yet or the framework was designed to work as it does.

I would think that if an OPP provides a set of constraints that define 
when it is supported and we can't tell if we can meet those, we should 
disable an OPP rather than enable it. Otherwise, what was the point of 
providing constraints?

Regards,
Dave

[1] http://www.spinics.net/lists/arm-kernel/msg527921.html

>   drivers/base/power/opp/of.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/opp/of.c b/drivers/base/power/opp/of.c
> index 1dfd3dd92624..ccccaf9f8968 100644
> --- a/drivers/base/power/opp/of.c
> +++ b/drivers/base/power/opp/of.c
> @@ -71,8 +71,18 @@ static bool _opp_is_supported(struct device *dev, struct opp_table *opp_table,
>   	u32 version;
>   	int ret;
>
> -	if (!opp_table->supported_hw)
> -		return true;
> +	if (!opp_table->supported_hw) {
> +		/*
> +		 * In the case that no supported_hw has been set by the
> +		 * platform but there is an opp-supported-hw value set for
> +		 * an OPP then the OPP should not be enabled as there is
> +		 * no way to see if the hardware supports it.
> +		 */
> +		if (of_find_property(np, "opp-supported-hw", NULL))
> +			return false;
> +		else
> +			return true;
> +	}
>
>   	while (count--) {
>   		ret = of_property_read_u32_index(np, "opp-supported-hw", count,
>