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

  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.

Add code to store the min and max ufs clk frequency from OPP table.

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(+)

--
2.17.1
  

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.
> 
> 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.

> 
>  #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.

> +
> +	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 (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()

> +			/* 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.

- Mani

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

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
  

On Wed, Dec 06, 2023 at 04:35:24PM +0530, Nitin Rawat wrote:
> 
> 
> 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.

Ah, right. Please increment it in dev_pm_opp_find_freq_ceil_indexed() instead.

- Mani
  

On Wed, Dec 06, 2023 at 01:24:47PM +0530, Manivannan Sadhasivam wrote:
> 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

There is no such thing as a "vendor" driver.