Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 22 Feb 2022 10:16:21 -0800
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Cc:     Alim Akhtar <alim.akhtar@samsung.com>,
        Avri Altman <avri.altman@wdc.com>,
        Rob Herring <robh+dt@kernel.org>,
        Andy Gross <agross@kernel.org>, Wei Xu <xuwei5@hisilicon.com>,
        Nishanth Menon <nm@ti.com>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Tero Kristo <kristo@kernel.org>,
        "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Jan Kotas <jank@cadence.com>, Li Wei <liwei213@huawei.com>,
        Stanley Chu <stanley.chu@mediatek.com>,
        Yaniv Gardi <ygardi@codeaurora.org>,
        linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-samsung-soc@vger.kernel.org,
        linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v2 09/15] scsi: ufs: deprecate 'freq-table-hz' property
Message-ID: <YhUodbzxx4wbr+gy@ripper>
References: <20220222145854.358646-1-krzysztof.kozlowski@canonical.com>
 <20220222145854.358646-10-krzysztof.kozlowski@canonical.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220222145854.358646-10-krzysztof.kozlowski@canonical.com>
Precedence: bulk

On Tue 22 Feb 06:58 PST 2022, Krzysztof Kozlowski wrote:

> The 'freq-table-hz' is not correct in DT schema, because '-hz' suffix
> defines uint32 type, not an array.  Therefore deprecate 'freq-table-hz'
> and use 'freq-table' instead.
> 

Patch looks good in itself, but why don't we use opp-table to describe
the performance states?

In particular looking at the two columns of frequencies for various
Qualcomm boards they require different performance-states.

A concrete example is sm8350.dtsi, which specifies 75MHz and 300MHz as
the first frequency pair. The lower level requires the VDD_CX power rail
to be at least &rpmhpd_opp_low_svs, the higher frequency has a
required-opps of &rpmhpd_opp_nom.


As this isn't possible to express in the current binding we've just been
forced to always run at a higher voltage level and kept this in the todo
list.

But rather than migrating freq-table-hz to freq-table and then having to
introduce an opp table to express the power constraints, could we
perhaps skip the intermediate step?

Or would you have any other suggestion about how we can represent the
required-opps level together with the freq-table (if that's what we want
to stick with).

Regards,
Bjorn

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  drivers/scsi/ufs/ufshcd-pltfrm.c | 21 +++++++++++++--------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd-pltfrm.c b/drivers/scsi/ufs/ufshcd-pltfrm.c
> index 87975d1a21c8..2b192477d158 100644
> --- a/drivers/scsi/ufs/ufshcd-pltfrm.c
> +++ b/drivers/scsi/ufs/ufshcd-pltfrm.c
> @@ -47,8 +47,9 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  	if (cnt <= 0)
>  		goto out;
>  
> -	if (!of_get_property(np, "freq-table-hz", &len)) {
> -		dev_info(dev, "freq-table-hz property not specified\n");
> +	if (!of_get_property(np, "freq-table", &len) ||
> +	    !of_get_property(np, "freq-table-hz", &len)) {
> +		dev_info(dev, "freq-table property not specified\n");
>  		goto out;
>  	}
>  
> @@ -57,7 +58,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  
>  	sz = len / sizeof(*clkfreq);
>  	if (sz != 2 * cnt) {
> -		dev_err(dev, "%s len mismatch\n", "freq-table-hz");
> +		dev_err(dev, "%s len mismatch\n", "freq-table");
>  		ret = -EINVAL;
>  		goto out;
>  	}
> @@ -69,12 +70,16 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  		goto out;
>  	}
>  
> -	ret = of_property_read_u32_array(np, "freq-table-hz",
> +	ret = of_property_read_u32_array(np, "freq-table",
>  			clkfreq, sz);
>  	if (ret && (ret != -EINVAL)) {
> -		dev_err(dev, "%s: error reading array %d\n",
> -				"freq-table-hz", ret);
> -		return ret;
> +		ret = of_property_read_u32_array(np, "freq-table-hz",
> +						 clkfreq, sz);
> +		if (ret && (ret != -EINVAL)) {
> +			dev_err(dev, "%s: error reading array %d\n",
> +				"freq-table", ret);
> +			return ret;
> +		}
>  	}
>  
>  	for (i = 0; i < sz; i += 2) {
> @@ -99,7 +104,7 @@ static int ufshcd_parse_clock_info(struct ufs_hba *hba)
>  
>  		if (!strcmp(name, "ref_clk"))
>  			clki->keep_link_active = true;
> -		dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table-hz",
> +		dev_dbg(dev, "%s: min %u max %u name %s\n", "freq-table",
>  				clki->min_freq, clki->max_freq, clki->name);
>  		list_add_tail(&clki->list, &hba->clk_list_head);
>  	}
> -- 
> 2.32.0
>