Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
Message-ID: <f30d3c21-f355-d535-480b-2fd36167bcfd@quicinc.com>
Date:   Wed, 6 Dec 2023 16:35:24 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.3.2
Subject: Re: [PATCH V1] scsi: ufs: core: store min and max clk freq from OPP
 table
Content-Language: en-US
To:     Manivannan Sadhasivam <mani@kernel.org>
CC:     "James E.J. Bottomley" <jejb@linux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@oracle.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
        <linux-scsi@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-mediatek@lists.infradead.org>, <quic_cang@quicinc.com>,
        Manish Pandey <quic_mapa@quicinc.com>
References: <20231206053628.32169-1-quic_nitirawa@quicinc.com>
 <20231206075447.GA4954@thinkpad>
From:   Nitin Rawat <quic_nitirawa@quicinc.com>
In-Reply-To: <20231206075447.GA4954@thinkpad>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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