Re: [PATCH V1] scsi: ufs: core: store min and max clk freq from OPP table

Nitin Rawat
2023-12-06T11:08:42Z

On 12/6/2023 1:24 PM, Manivannan Sadhasivam wrote:
> On Wed, Dec 06, 2023 at 11:06:28AM +0530, Nitin Rawat wrote:
>> OPP support will make use of OPP table in device tree and removes
>> freq-table-hz property from device tree.
>>
>> With OPP enabled in devicetree, clki->min_freq and clki->maxfreq
>> currently is not getting updated and the value is set to 0.
>>
>> Soc vendors like qcom, mediatek uses clki->minfreq and clki->maxfreq
>> in vendor specific file. These frequencies values are used to update
>> vendor specific configurations. Since the value is 0, it is causing
>> functional issue.
>
> How about,
>
> "OPP support added by commit 72208ebe181e ("scsi: ufs: core: Add support
> for parsing OPP") doesn't update the min_freq and max_freq of each clocks
> in 'struct ufs_clk_info'.
>
> But these values are used by the vendor host drivers internally for controller
> configuration. When the OPP support is enabled in devicetree, these values will
> be 0, causing boot issues on the respective platforms.
>
> So let's parse the min_freq and max_freq of all clocks while parsing the OPP
> table."
>
>>
>> Add code to store the min and max ufs clk frequency from OPP table.

Sure. Will update in next patchset.

>>
>> Fixes: 72208ebe181e ("scsi: ufs: core: Add support for parsing OPP")
>> Co-developed-by: Manish Pandey <quic_mapa@quicinc.com>
>> Signed-off-by: Manish Pandey <quic_mapa@quicinc.com>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>> drivers/ufs/host/ufshcd-pltfrm.c | 56 ++++++++++++++++++++++++++++++++
>> 1 file changed, 56 insertions(+)
>>
>> diff --git a/drivers/ufs/host/ufshcd-pltfrm.c b/drivers/ufs/host/ufshcd-pltfrm.c
>> index da2558e274b4..12fa6f7d6a97 100644
>> --- a/drivers/ufs/host/ufshcd-pltfrm.c
>> +++ b/drivers/ufs/host/ufshcd-pltfrm.c
>> @@ -13,6 +13,7 @@
>> #include <linux/pm_opp.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/of.h>
>> +#include <linux/clk.h>
>
> Sort includes alphabetically.

Sure. Will update in next patchset

>
>>
>> #include <ufs/ufshcd.h>
>> #include "ufshcd-pltfrm.h"
>> @@ -213,6 +214,55 @@ static void ufshcd_init_lanes_per_dir(struct ufs_hba *hba)
>> }
>> }
>>
>> +/**
>> + * ufshcd_config_min_max_clk_freq - update min and max freq
>
> "ufshcd_parse_clock_min_max_freq - Parse MIN and MAX frequencies of clocks"
>
>> + * @hba: per adapter instance
>> + *
>> + * This function store min and max freq for all the clocks.
>> + *
>
> "This function parses MIN and MAX frequencies of all clocks required by the
> vendor host drivers."
>
>> + * Returns 0 for success and non-zero for failure
>> + */
>> +static int ufshcd_config_min_max_clk_freq(struct ufs_hba *hba)
>> +{
>> + struct list_head *head = &hba->clk_list_head;
>> + struct dev_pm_opp *opp;
>> + struct ufs_clk_info *clki;
>
> Please maintain reverse Xmas tree order. It's not a rule for this driver, but my
> own preference.
>
>> + unsigned long freq;
>> + u8 idx = 0;
>> + int ret;
>
> This won't be needed if all the return values are directly returned as I shared
> below.

Will Addressed all ret comments in next patchset.
>
>> +
>> + list_for_each_entry(clki, head, list) {
>> + if (!clki->name)
>> + continue;
>> +
>> + clki->clk = devm_clk_get(hba->dev, clki->name);
>> + if (!IS_ERR_OR_NULL(clki->clk)) {
>
> This function won't return NULL, so IS_ERR() is sufficient.
>
>> + /* Find Max Freq */
>> + freq = ULONG_MAX;
>> + opp = dev_pm_opp_find_freq_floor_indexed(hba->dev, &freq, idx);
>
> Use idx++ and get rid of the increment at the end of the 'if' condition.

If we increment idx++ here, dev_pm_opp_find_freq_ceil_indexed will use
incremented idx which is not correct. Hence i added at end after both
the call.

>
>> + if (IS_ERR(opp)) {
>> + dev_err(hba->dev, "failed to find dev_pm_opp\n");
>
> "Failed to find OPP for MAX frequency"
>
>> + ret = PTR_ERR(opp);
>
> return PTR_ERR(opp);
>
>> + return ret;
>> + }
>> + clki->max_freq = dev_pm_opp_get_freq_indexed(opp, idx);
>> +
>
> Missing dev_pm_opp_put()

Thanks. Will update in next patchset.

>
>> + /* Find Min Freq */
>> + freq = 0;
>> + opp = dev_pm_opp_find_freq_ceil_indexed(hba->dev, &freq, idx);
>> + if (IS_ERR(opp)) {
>> + dev_err(hba->dev, "failed to find dev_pm_opp\n");
>
> "Failed to find OPP for MIN frequency"
>
>> + ret = PTR_ERR(opp);
>
> return PTR_ERR(opp);
>
>> + return ret;
>> + }
>> + clki->min_freq = dev_pm_opp_get_freq_indexed(opp, idx);
>
> Missing dev_pm_opp_put()
>
>> + idx++;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int ufshcd_parse_operating_points(struct ufs_hba *hba)
>> {
>> struct device *dev = hba->dev;
>> @@ -279,6 +329,12 @@ static int ufshcd_parse_operating_points(struct ufs_hba *hba)
>> return ret;
>> }
>>
>> + ret = ufshcd_config_min_max_clk_freq(hba);
>> + if (ret) {
>> + dev_err(dev, "Failed to get min max freq: %d\n", ret);
>
> Since we already print error message inside the function, no need to do the same
> here.

Sure. Will update in next patchset

>
> - Mani
>
>> + return ret;
>> + }
>> +
>> hba->use_pm_opp = true;
>>
>> return 0;
>> --
>> 2.17.1
>>
>

Thanks,
Nitin