[PATCH v3 1/3] scsi: ufs: Extract devfreq registration

[Bjorn Andersson]

Failing to register with devfreq leaves hba->devfreq assigned, which
causes the error path to dereference the ERR_PTR(). Rather than bolting
on more conditionals, move the call of devm_devfreq_add_device() into
it's own function and only update hba->devfreq once it's successfully
registered.

The subsequent patch builds upon this to make UFS actually work again,
as it's been broken since f1d981eaecf8 ("PM / devfreq: Use the available
min/max frequency")

Also switch to use DEVFREQ_GOV_SIMPLE_ONDEMAND constant.

Reviewed-by: Chanwoo Choi <cw00.c...@samsung.com>
Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---

Changes since v2:
- Use DEVFREQ_GOV_SIMPLE_ONDEMAND, per Chanwoo's recommendation
- Picked up Chanwoo's R-b

Changes since v1:
- None

 drivers/scsi/ufs/ufshcd.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 00e79057f870..f04902a066cb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1287,6 +1287,26 @@ static struct devfreq_dev_profile ufs_devfreq_profile = {
.get_dev_status = ufshcd_devfreq_get_dev_status,
 };
 
+static int ufshcd_devfreq_init(struct ufs_hba *hba)
+{
+   struct devfreq *devfreq;
+   int ret;
+
+   devfreq = devm_devfreq_add_device(hba->dev,
+   _devfreq_profile,
+   DEVFREQ_GOV_SIMPLE_ONDEMAND,
+   NULL);
+   if (IS_ERR(devfreq)) {
+   ret = PTR_ERR(devfreq);
+   dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
+   return ret;
+   }
+
+   hba->devfreq = devfreq;
+
+   return 0;
+}
+
 static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 {
unsigned long flags;
@@ -6439,16 +6459,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
sizeof(struct ufs_pa_layer_attr));
hba->clk_scaling.saved_pwr_info.is_valid = true;
if (!hba->devfreq) {
-   hba->devfreq = devm_devfreq_add_device(hba->dev,
-   _devfreq_profile,
-   "simple_ondemand",
-   NULL);
-   if (IS_ERR(hba->devfreq)) {
-   ret = PTR_ERR(hba->devfreq);
-   dev_err(hba->dev, "Unable to register 
with devfreq %d\n",
-   ret);
+   ret = ufshcd_devfreq_init(hba);
+   if (ret)
goto out;
-   }
}
hba->clk_scaling.is_allowed = true;
}
-- 
2.17.0

[PATCH v3 0/3] Fix UFS and devfreq interaction

[Bjorn Andersson]

With the introduction of f1d981eaecf8 ("PM / devfreq: Use the available min/max
frequency") the UFS host controller driver (UFSHCD) stopped probing for
platforms that supports frequency scaling, e.g. all modern Qualcomm platforms.

The cause of this was UFSHCD's reliance of not registering any frequencies and
then being called by devfreq to switch between the frequencies 0 and UINT_MAX.

The devfreq code implies that the client is able to pass the frequency table,
instead of relying on opp tables, but as concluded after v1 this is not
compliant with devfreq cooling, which will enable and disable opp entries in
order to limit the valid frequencies. So instead the UFSHCD driver is modified
to read the freq-table and register the first clock's two rates as the two
available opp levels.


This follows the first patch which facilitates the implementation of this in a
clean fashion, and removes the kernel panic which previously happened when
devfreq initialization failed.


With this UFS is once again functional on the db820c, and is needed to get UFS
working on SDM845 (both tested).

Added in v3 is the dts patch for Andy to introduce UFS in msm8996 and db820c,
now that it finally works again.

Bjorn Andersson (3):
  scsi: ufs: Extract devfreq registration
  scsi: ufs: Use freq table with devfreq
  arm64: dts: qcom: msm8996: Add ufs related nodes

 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi |  8 ++
 arch/arm64/boot/dts/qcom/msm8996.dtsi| 85 
 drivers/scsi/ufs/ufshcd.c| 76 +
 3 files changed, 154 insertions(+), 15 deletions(-)

-- 
2.17.0

[PATCH v3 2/3] scsi: ufs: Use freq table with devfreq

[Bjorn Andersson]

devfreq requires that the client operates on actual frequencies, not
only 0 and UMAX_INT and as such UFS brok with the introduction of
f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency").

This patch registers the frequencies of the first clock with devfreq and
use these to determine if we're trying to step up or down.

Reviewed-by: Chanwoo Choi <cw00.c...@samsung.com> [for devfreq & OPP part]
Reviewed-by: Subhash Jadavani <subha...@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---

Changes since v2:
- Picked up R-b from Chanwoo and Subhash

Chances since v1:
- Register min_freq and max_freq as opp levels.
- Unregister opp levels on removal, to make e.g. probe defer working

 drivers/scsi/ufs/ufshcd.c | 47 +--
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f04902a066cb..3d46a70ed41d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1200,16 +1200,13 @@ static int ufshcd_devfreq_target(struct device *dev,
struct ufs_hba *hba = dev_get_drvdata(dev);
ktime_t start;
bool scale_up, sched_clk_scaling_suspend_work = false;
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
unsigned long irq_flags;
 
if (!ufshcd_is_clkscaling_supported(hba))
return -EINVAL;
 
-   if ((*freq > 0) && (*freq < UINT_MAX)) {
-   dev_err(hba->dev, "%s: invalid freq = %lu\n", __func__, *freq);
-   return -EINVAL;
-   }
-
spin_lock_irqsave(hba->host->host_lock, irq_flags);
if (ufshcd_eh_in_progress(hba)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
@@ -1219,7 +1216,13 @@ static int ufshcd_devfreq_target(struct device *dev,
if (!hba->clk_scaling.active_reqs)
sched_clk_scaling_suspend_work = true;
 
-   scale_up = (*freq == UINT_MAX) ? true : false;
+   if (list_empty(clk_list)) {
+   spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
+   goto out;
+   }
+
+   clki = list_first_entry(>clk_list_head, struct ufs_clk_info, list);
+   scale_up = (*freq == clki->max_freq) ? true : false;
if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
ret = 0;
@@ -1289,16 +1292,29 @@ static struct devfreq_dev_profile ufs_devfreq_profile = 
{
 
 static int ufshcd_devfreq_init(struct ufs_hba *hba)
 {
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
struct devfreq *devfreq;
int ret;
 
-   devfreq = devm_devfreq_add_device(hba->dev,
+   /* Skip devfreq if we don't have any clocks in the list */
+   if (list_empty(clk_list))
+   return 0;
+
+   clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+   dev_pm_opp_add(hba->dev, clki->min_freq, 0);
+   dev_pm_opp_add(hba->dev, clki->max_freq, 0);
+
+   devfreq = devfreq_add_device(hba->dev,
_devfreq_profile,
DEVFREQ_GOV_SIMPLE_ONDEMAND,
NULL);
if (IS_ERR(devfreq)) {
ret = PTR_ERR(devfreq);
dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
+
+   dev_pm_opp_remove(hba->dev, clki->min_freq);
+   dev_pm_opp_remove(hba->dev, clki->max_freq);
return ret;
}
 
@@ -1307,6 +1323,22 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
return 0;
 }
 
+static void ufshcd_devfreq_remove(struct ufs_hba *hba)
+{
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
+
+   if (!hba->devfreq)
+   return;
+
+   devfreq_remove_device(hba->devfreq);
+   hba->devfreq = NULL;
+
+   clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+   dev_pm_opp_remove(hba->dev, clki->min_freq);
+   dev_pm_opp_remove(hba->dev, clki->max_freq);
+}
+
 static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 {
unsigned long flags;
@@ -7005,6 +7037,7 @@ static void ufshcd_hba_exit(struct ufs_hba *hba)
if (hba->devfreq)
ufshcd_suspend_clkscaling(hba);
destroy_workqueue(hba->clk_scaling.workq);
+   ufshcd_devfreq_remove(hba);
}
ufshcd_setup_clocks(hba, false);
ufshcd_setup_hba_vreg(hba, false);
-- 
2.17.0

[PATCH v2 1/2] scsi: ufs: Extract devfreq registration

[Bjorn Andersson]

Failing to register with devfreq leaves hba->devfreq assigned, which
causes the error path to dereference the ERR_PTR(). Rather than bolting
on more conditionals, move the call of devm_devfreq_add_device() into
it's own function and only update hba->devfreq once it's successfully
registered.

The subsequent patch builds upon this to make UFS actually work again,
as it's been broken since f1d981eaecf8 ("PM / devfreq: Use the available
min/max frequency")

Reviewed-by: Subhash Jadavani <subha...@codeaurora.org>
Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---

Changes since v1:
- None

 drivers/scsi/ufs/ufshcd.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8f22a980b1a7..2253f24309ec 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1255,6 +1255,26 @@ static struct devfreq_dev_profile ufs_devfreq_profile = {
.get_dev_status = ufshcd_devfreq_get_dev_status,
 };
 
+static int ufshcd_devfreq_init(struct ufs_hba *hba)
+{
+   struct devfreq *devfreq;
+   int ret;
+
+   devfreq = devm_devfreq_add_device(hba->dev,
+   _devfreq_profile,
+   "simple_ondemand",
+   NULL);
+   if (IS_ERR(devfreq)) {
+   ret = PTR_ERR(devfreq);
+   dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
+   return ret;
+   }
+
+   hba->devfreq = devfreq;
+
+   return 0;
+}
+
 static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 {
unsigned long flags;
@@ -6399,16 +6419,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
sizeof(struct ufs_pa_layer_attr));
hba->clk_scaling.saved_pwr_info.is_valid = true;
if (!hba->devfreq) {
-   hba->devfreq = devm_devfreq_add_device(hba->dev,
-   _devfreq_profile,
-   "simple_ondemand",
-   NULL);
-   if (IS_ERR(hba->devfreq)) {
-   ret = PTR_ERR(hba->devfreq);
-   dev_err(hba->dev, "Unable to register 
with devfreq %d\n",
-   ret);
+   ret = ufshcd_devfreq_init(hba);
+   if (ret)
goto out;
-   }
}
hba->clk_scaling.is_allowed = true;
}
-- 
2.17.0

[PATCH v2 0/2] Fix UFS and devfreq interaction

[Bjorn Andersson]

With the introduction of f1d981eaecf8 ("PM / devfreq: Use the available min/max
frequency") the UFS host controller driver (UFSHCD) stopped probing for
platforms that supports frequency scaling, e.g. all modern Qualcomm platforms.

The cause of this was UFSHCD's reliance of not registering any frequencies and
then being called by devfreq to switch between the frequencies 0 and UINT_MAX.

The devfreq code implies that the client is able to pass the frequency table,
instead of relying on opp tables, but as concluded after v1 this is not
compliant with devfreq cooling, which will enable and disable opp entries in
order to limit the valid frequencies. So instead the UFSHCD driver is modified
to read the freq-table and register the first clock's two rates as the two
available opp levels.


This follows the first patch which facilitates the implementation of this in a
clean fashion, and removes the kernel panic which previously happened when
devfreq initialization failed.


With this UFS is once again functional on the db820c, and is needed to get UFS
working on SDM845 (both tested).

Bjorn Andersson (2):
  scsi: ufs: Extract devfreq registration
  scsi: ufs: Use freq table with devfreq

 drivers/scsi/ufs/ufshcd.c | 76 +++
 1 file changed, 61 insertions(+), 15 deletions(-)

-- 
2.17.0

[PATCH v2 2/2] scsi: ufs: Use freq table with devfreq

[Bjorn Andersson]

devfreq requires that the client operates on actual frequencies, not
only 0 and UMAX_INT and as such UFS brok with the introduction of
f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency").

This patch registers the frequencies of the first clock as opp levels
and use these to determine if we're trying to step up or down.

Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---

Chances since v1:
- Register min_freq and max_freq as opp levels.
- Unregister opp levels on removal, to make e.g. probe defer working

 drivers/scsi/ufs/ufshcd.c | 47 +--
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2253f24309ec..257614b889bd 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1168,16 +1168,13 @@ static int ufshcd_devfreq_target(struct device *dev,
struct ufs_hba *hba = dev_get_drvdata(dev);
ktime_t start;
bool scale_up, sched_clk_scaling_suspend_work = false;
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
unsigned long irq_flags;
 
if (!ufshcd_is_clkscaling_supported(hba))
return -EINVAL;
 
-   if ((*freq > 0) && (*freq < UINT_MAX)) {
-   dev_err(hba->dev, "%s: invalid freq = %lu\n", __func__, *freq);
-   return -EINVAL;
-   }
-
spin_lock_irqsave(hba->host->host_lock, irq_flags);
if (ufshcd_eh_in_progress(hba)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
@@ -1187,7 +1184,13 @@ static int ufshcd_devfreq_target(struct device *dev,
if (!hba->clk_scaling.active_reqs)
sched_clk_scaling_suspend_work = true;
 
-   scale_up = (*freq == UINT_MAX) ? true : false;
+   if (list_empty(clk_list)) {
+   spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
+   goto out;
+   }
+
+   clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+   scale_up = (*freq == clki->max_freq) ? true : false;
if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
ret = 0;
@@ -1257,16 +1260,29 @@ static struct devfreq_dev_profile ufs_devfreq_profile = 
{
 
 static int ufshcd_devfreq_init(struct ufs_hba *hba)
 {
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
struct devfreq *devfreq;
int ret;
 
-   devfreq = devm_devfreq_add_device(hba->dev,
+   /* Skip devfreq if we don't have any clocks in the list */
+   if (list_empty(clk_list))
+   return 0;
+
+   clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+   dev_pm_opp_add(hba->dev, clki->min_freq, 0);
+   dev_pm_opp_add(hba->dev, clki->max_freq, 0);
+
+   devfreq = devfreq_add_device(hba->dev,
_devfreq_profile,
"simple_ondemand",
NULL);
if (IS_ERR(devfreq)) {
ret = PTR_ERR(devfreq);
dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
+
+   dev_pm_opp_remove(hba->dev, clki->min_freq);
+   dev_pm_opp_remove(hba->dev, clki->max_freq);
return ret;
}
 
@@ -1275,6 +1291,22 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
return 0;
 }
 
+static void ufshcd_devfreq_remove(struct ufs_hba *hba)
+{
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
+
+   if (!hba->devfreq)
+   return;
+
+   devfreq_remove_device(hba->devfreq);
+   hba->devfreq = NULL;
+
+   clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+   dev_pm_opp_remove(hba->dev, clki->min_freq);
+   dev_pm_opp_remove(hba->dev, clki->max_freq);
+}
+
 static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 {
unsigned long flags;
@@ -6966,6 +6998,7 @@ static void ufshcd_hba_exit(struct ufs_hba *hba)
if (hba->devfreq)
ufshcd_suspend_clkscaling(hba);
destroy_workqueue(hba->clk_scaling.workq);
+   ufshcd_devfreq_remove(hba);
}
ufshcd_setup_clocks(hba, false);
ufshcd_setup_hba_vreg(hba, false);
-- 
2.17.0

Re: [PATCH 3/3] scsi: ufs: Use freq table with devfreq

[Bjorn Andersson]

On Tue 24 Apr 15:08 PDT 2018, Subhash Jadavani wrote:

> On 2018-04-23 17:20, Bjorn Andersson wrote:
> > devfreq requires that the client operates on actual frequencies, not
> > only 0 and UMAX_INT and as such UFS brok with the introduction of
> > f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency").
> > 
> > This patch registers the frequencies of the first clock with devfreq and
> > use these to determine if we're trying to step up or down.
> > 
[..]
> 
> Looks good to me.
> Reviewed-by: Subhash Jadavani <subha...@codeaurora.org>
> 

Thanks Subhash. Unfortunately I need to respin this to register the opp
table based on our freq table, so there will be a v2 of this soon.

Regards,
Bjorn

Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table

[Bjorn Andersson]

On Mon 23 Apr 23:09 PDT 2018, MyungJoo Ham wrote:

> >On Mon 23 Apr 19:48 PDT 2018, Chanwoo Choi wrote:
> >
> >> Hi,
> >> 
> >> On 2018??? 04??? 24??? 09:20, Bjorn Andersson wrote:
> >> > The code in devfreq_add_device() handles the case where a freq_table is
> >> > passed by the client, but then requests min and max frequences from
> >> > the, in this case absent, opp tables.
> >> > 
> >> > Read the min and max frequencies from the frequency table, which has
> >> > been built from the opp table if one exists, instead of querying the
> >> > opp table.
> >> > 
> >> > Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
> >> > ---
> >> > 
> >> > An alternative approach is to clarify in the devfreq code that it's not
> >> > possible to pass a freq_table and then in patch 3 create an opp table 
> >> > for the
> >> > device in runtime; although the error handling of this becomes 
> >> > non-trivial.
> >> > 
> >> > Transitioning the UFSHCD to use opp tables directly is hindered by the 
> >> > fact
> >> > that the Qualcomm UFS hardware has two different clocks that needs to be
> >> > running at different rates, so we would need a way to describe the two 
> >> > rates in
> >> > the opp table. (And would force us to change the DT binding)
> >> > 
> >> >  drivers/devfreq/devfreq.c | 22 --
> >> >  1 file changed, 4 insertions(+), 18 deletions(-)
> >> > 
> >> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >> > index fe2af6aa88fc..086ced50a13d 100644
> >> > --- a/drivers/devfreq/devfreq.c
> >> > +++ b/drivers/devfreq/devfreq.c
> >> > @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct 
> >> > device *dev)
> >> >  
> >> >  static unsigned long find_available_min_freq(struct devfreq *devfreq)
> >> >  {
> >> > -struct dev_pm_opp *opp;
> >> > -unsigned long min_freq = 0;
> >> > -
> >> > -opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, _freq);
> >> > -if (IS_ERR(opp))
> >> > -min_freq = 0;
> >> > -else
> >> > -dev_pm_opp_put(opp);
> >> > +struct devfreq_dev_profile *profile = devfreq->profile;
> >> >  
> >> > -return min_freq;
> >> > +return profile->freq_table[0];
> >> 
> >> It is wrong. The thermal framework support the devfreq-cooling device
> >> which uses the dev_pm_opp_enable/disable().
> >> 
> >
> >Okay, that makes sense. So rather than registering a custom freq_table I
> >should register the min and max frequency using dev_pm_opp_add().
> >
> >> In order to find the correct available min frequency,
> >> the devfreq have to use the OPP function instead of using the first entry
> >> of the freq_table array.
> >> 
> >
> >Based on this there seems to be room for cleaning out the freq_table
> >from devfreq, to reduce the confusion. I will review this further.
> 
> Could you please check if the bug suffering you gets resolved by
> replacing 0 with ULONG_MAX in the function find_available_max_freq?
> 
> -max_freq = 0;
> +max_freq = ULONG_MAX;
> 
> Even if you are not using OPP, these functions should provide somewhat
> "compatible" values.
> 

I also need to make set_freq_table() handle the case where there is no
opp table and change a min_freq of 0 from being treated as an error.

With this I think we're back at supporting using devfreq without
specifying any available frequencies. I am however uncertain if this
should be considered valid use of devfreq.

Regards,
Bjorn

Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table

[Bjorn Andersson]

On Tue 24 Apr 00:26 PDT 2018, Chanwoo Choi wrote:

> Hi,
> 
> On 2018??? 04??? 24??? 14:29, Bjorn Andersson wrote:
> > On Mon 23 Apr 19:48 PDT 2018, Chanwoo Choi wrote:
> > 
> >> Hi,
> >>
> >> On 2018??? 04??? 24??? 09:20, Bjorn Andersson wrote:
> >>> The code in devfreq_add_device() handles the case where a freq_table is
> >>> passed by the client, but then requests min and max frequences from
> >>> the, in this case absent, opp tables.
> >>>
> >>> Read the min and max frequencies from the frequency table, which has
> >>> been built from the opp table if one exists, instead of querying the
> >>> opp table.
> >>>
> >>> Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
> >>> ---
> >>>
> >>> An alternative approach is to clarify in the devfreq code that it's not
> >>> possible to pass a freq_table and then in patch 3 create an opp table for 
> >>> the
> >>> device in runtime; although the error handling of this becomes 
> >>> non-trivial.
> >>>
> >>> Transitioning the UFSHCD to use opp tables directly is hindered by the 
> >>> fact
> >>> that the Qualcomm UFS hardware has two different clocks that needs to be
> >>> running at different rates, so we would need a way to describe the two 
> >>> rates in
> >>> the opp table. (And would force us to change the DT binding)
> >>>
> >>>  drivers/devfreq/devfreq.c | 22 --
> >>>  1 file changed, 4 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >>> index fe2af6aa88fc..086ced50a13d 100644
> >>> --- a/drivers/devfreq/devfreq.c
> >>> +++ b/drivers/devfreq/devfreq.c
> >>> @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct 
> >>> device *dev)
> >>>  
> >>>  static unsigned long find_available_min_freq(struct devfreq *devfreq)
> >>>  {
> >>> - struct dev_pm_opp *opp;
> >>> - unsigned long min_freq = 0;
> >>> -
> >>> - opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, _freq);
> >>> - if (IS_ERR(opp))
> >>> - min_freq = 0;
> >>> - else
> >>> - dev_pm_opp_put(opp);
> >>> + struct devfreq_dev_profile *profile = devfreq->profile;
> >>>  
> >>> - return min_freq;
> >>> + return profile->freq_table[0];
> >>
> >> It is wrong. The thermal framework support the devfreq-cooling device
> >> which uses the dev_pm_opp_enable/disable().
> >>
> > 
> > Okay, that makes sense. So rather than registering a custom freq_table I
> > should register the min and max frequency using dev_pm_opp_add().
> 
> Thanks.
> 
> > 
> >> In order to find the correct available min frequency,
> >> the devfreq have to use the OPP function instead of using the first entry
> >> of the freq_table array.
> >>
> > 
> > Based on this there seems to be room for cleaning out the freq_table
> > from devfreq, to reduce the confusion. I will review this further.
> 
> Actually, devfreq must need to have the freq_table[] array. But, freq_table[]
> array should be handled in the devfreq core. Now, the devfreq device drivers 
> can
> touch the freq_table. I think it is not good.
> 
> There is a reason why we have to maintain the freq_table[] as the internal 
> variable.
> OPP doesn't provide the OPP API which get the all registered frequencies.
> If devfreq-cooling device disables the specific frequency by using 
> dev_pm_oppdisable(),
> the user of OPP interface can not get the disabled frequency list.
> So, I maintain the freq_table even if using the OPP interface.
> 

Thanks for the clarification, I see some possibilities for improving
this but it makes sense.

> And, devfreq-cooling device uses the freq_table directly because
> released MALi driver from ARM initializes the freq_table list
> directly.
> 

Forgive me if I misunderstand this, but isn't this exactly what I'm
trying to do in patch 3? Which stopped working back in v4.15-rc1, with
the introduction of f1d981eaecf8 ("PM / devfreq: Use the available
min/max frequency").

> I have no any objection for refactoring. Just I'm sharing the issue
> and current status.
> 

Thanks for sharing the current status and helping me understand how to
properly use devfreq.

Regards,
Bjorn

Re: [PATCH 1/3] PM / devfreq: Actually support providing freq_table

[Bjorn Andersson]

On Mon 23 Apr 19:48 PDT 2018, Chanwoo Choi wrote:

> Hi,
> 
> On 2018??? 04??? 24??? 09:20, Bjorn Andersson wrote:
> > The code in devfreq_add_device() handles the case where a freq_table is
> > passed by the client, but then requests min and max frequences from
> > the, in this case absent, opp tables.
> > 
> > Read the min and max frequencies from the frequency table, which has
> > been built from the opp table if one exists, instead of querying the
> > opp table.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
> > ---
> > 
> > An alternative approach is to clarify in the devfreq code that it's not
> > possible to pass a freq_table and then in patch 3 create an opp table for 
> > the
> > device in runtime; although the error handling of this becomes non-trivial.
> > 
> > Transitioning the UFSHCD to use opp tables directly is hindered by the fact
> > that the Qualcomm UFS hardware has two different clocks that needs to be
> > running at different rates, so we would need a way to describe the two 
> > rates in
> > the opp table. (And would force us to change the DT binding)
> > 
> >  drivers/devfreq/devfreq.c | 22 --
> >  1 file changed, 4 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> > index fe2af6aa88fc..086ced50a13d 100644
> > --- a/drivers/devfreq/devfreq.c
> > +++ b/drivers/devfreq/devfreq.c
> > @@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct 
> > device *dev)
> >  
> >  static unsigned long find_available_min_freq(struct devfreq *devfreq)
> >  {
> > -   struct dev_pm_opp *opp;
> > -   unsigned long min_freq = 0;
> > -
> > -   opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, _freq);
> > -   if (IS_ERR(opp))
> > -   min_freq = 0;
> > -   else
> > -   dev_pm_opp_put(opp);
> > +   struct devfreq_dev_profile *profile = devfreq->profile;
> >  
> > -   return min_freq;
> > +   return profile->freq_table[0];
> 
> It is wrong. The thermal framework support the devfreq-cooling device
> which uses the dev_pm_opp_enable/disable().
> 

Okay, that makes sense. So rather than registering a custom freq_table I
should register the min and max frequency using dev_pm_opp_add().

> In order to find the correct available min frequency,
> the devfreq have to use the OPP function instead of using the first entry
> of the freq_table array.
> 

Based on this there seems to be room for cleaning out the freq_table
from devfreq, to reduce the confusion. I will review this further.

Thanks,
Bjorn

[PATCH 0/3] Fix UFS and devfreq interaction

[Bjorn Andersson]

With the introduction of f1d981eaecf8 ("PM / devfreq: Use the available min/max
frequency") the UFS host controller driver (UFSHCD) stopped probing for
platforms that supports frequency scaling, e.g. all modern Qualcomm platforms.

The cause of this was UFSHCD's reliance of not registering any frequencies and
then being called by devfreq to switch between the frequencies 0 and UINT_MAX.

The devfreq code implies that the client is able to pass the frequency table,
instead of relying on opp tables, so the first patch makes this actually work.
The second patch extracts the devfreq registration in the UFSHCD driver, both
to facilitate the third patch and to remove a dereference of an ERR_PTR() in
the case that devfreq registration fails. Finally, the third patch picks the
two frequencies from the freq-table provided in UFSHCD and pass these to
devfreq, as well as map these frequencies back to the step up/down actions.


With this UFS is once again functional on the db820c, and is needed to get UFS
working on SDM845 (both tested).

Bjorn Andersson (3):
  PM / devfreq: Actually support providing freq_table
  scsi: ufs: Extract devfreq registration
  scsi: ufs: Use freq table with devfreq

 drivers/devfreq/devfreq.c | 22 +++
 drivers/scsi/ufs/ufshcd.c | 68 ---
 2 files changed, 57 insertions(+), 33 deletions(-)

-- 
2.16.2

[PATCH 2/3] scsi: ufs: Extract devfreq registration

[Bjorn Andersson]

Failing to register with devfreq leaves hba->devfreq assigned, which
causes the error path to dereference the ERR_PTR(). Rather than bolting
on more conditionals, move the call of devm_devfreq_add_device() into
it's own function and only update hba->devfreq once it's successfully
registered.

The subsequent patch builds upon this to make UFS actually work again,
as it's been broken since f1d981eaecf8 ("PM / devfreq: Use the available
min/max frequency")

Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---
 drivers/scsi/ufs/ufshcd.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 8f22a980b1a7..2253f24309ec 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1255,6 +1255,26 @@ static struct devfreq_dev_profile ufs_devfreq_profile = {
.get_dev_status = ufshcd_devfreq_get_dev_status,
 };
 
+static int ufshcd_devfreq_init(struct ufs_hba *hba)
+{
+   struct devfreq *devfreq;
+   int ret;
+
+   devfreq = devm_devfreq_add_device(hba->dev,
+   _devfreq_profile,
+   "simple_ondemand",
+   NULL);
+   if (IS_ERR(devfreq)) {
+   ret = PTR_ERR(devfreq);
+   dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
+   return ret;
+   }
+
+   hba->devfreq = devfreq;
+
+   return 0;
+}
+
 static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 {
unsigned long flags;
@@ -6399,16 +6419,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
sizeof(struct ufs_pa_layer_attr));
hba->clk_scaling.saved_pwr_info.is_valid = true;
if (!hba->devfreq) {
-   hba->devfreq = devm_devfreq_add_device(hba->dev,
-   _devfreq_profile,
-   "simple_ondemand",
-   NULL);
-   if (IS_ERR(hba->devfreq)) {
-   ret = PTR_ERR(hba->devfreq);
-   dev_err(hba->dev, "Unable to register 
with devfreq %d\n",
-   ret);
+   ret = ufshcd_devfreq_init(hba);
+   if (ret)
goto out;
-   }
}
hba->clk_scaling.is_allowed = true;
}
-- 
2.16.2

[PATCH 3/3] scsi: ufs: Use freq table with devfreq

[Bjorn Andersson]

devfreq requires that the client operates on actual frequencies, not
only 0 and UMAX_INT and as such UFS brok with the introduction of
f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency").

This patch registers the frequencies of the first clock with devfreq and
use these to determine if we're trying to step up or down.

Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---
 drivers/scsi/ufs/ufshcd.c | 39 ---
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2253f24309ec..07b1f3c7bd2d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1168,16 +1168,13 @@ static int ufshcd_devfreq_target(struct device *dev,
struct ufs_hba *hba = dev_get_drvdata(dev);
ktime_t start;
bool scale_up, sched_clk_scaling_suspend_work = false;
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
unsigned long irq_flags;
 
if (!ufshcd_is_clkscaling_supported(hba))
return -EINVAL;
 
-   if ((*freq > 0) && (*freq < UINT_MAX)) {
-   dev_err(hba->dev, "%s: invalid freq = %lu\n", __func__, *freq);
-   return -EINVAL;
-   }
-
spin_lock_irqsave(hba->host->host_lock, irq_flags);
if (ufshcd_eh_in_progress(hba)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
@@ -1187,7 +1184,13 @@ static int ufshcd_devfreq_target(struct device *dev,
if (!hba->clk_scaling.active_reqs)
sched_clk_scaling_suspend_work = true;
 
-   scale_up = (*freq == UINT_MAX) ? true : false;
+   if (list_empty(clk_list)) {
+   spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
+   goto out;
+   }
+
+   clki = list_first_entry(>clk_list_head, struct ufs_clk_info, list);
+   scale_up = (*freq == clki->max_freq) ? true : false;
if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
ret = 0;
@@ -1257,11 +1260,33 @@ static struct devfreq_dev_profile ufs_devfreq_profile = 
{
 
 static int ufshcd_devfreq_init(struct ufs_hba *hba)
 {
+   struct devfreq_dev_profile *profile;
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
struct devfreq *devfreq;
int ret;
 
+   /* Skip devfreq if we don't have any clocks in the list */
+   if (list_empty(clk_list))
+   return 0;
+
+   profile = devm_kmemdup(hba->dev, _devfreq_profile,
+  sizeof(ufs_devfreq_profile), GFP_KERNEL);
+   if (!profile)
+   return -ENOMEM;
+
+   profile->max_state = 2;
+   profile->freq_table = devm_kcalloc(hba->dev, profile->max_state,
+  sizeof(unsigned long), GFP_KERNEL);
+   if (!profile->freq_table)
+   return -ENOMEM;
+
+   clki = list_first_entry(>clk_list_head, struct ufs_clk_info, list);
+   profile->freq_table[0] = clki->min_freq;
+   profile->freq_table[1] = clki->max_freq;
+
devfreq = devm_devfreq_add_device(hba->dev,
-   _devfreq_profile,
+   profile,
"simple_ondemand",
NULL);
if (IS_ERR(devfreq)) {
-- 
2.16.2

[PATCH 1/3] PM / devfreq: Actually support providing freq_table

[Bjorn Andersson]

The code in devfreq_add_device() handles the case where a freq_table is
passed by the client, but then requests min and max frequences from
the, in this case absent, opp tables.

Read the min and max frequencies from the frequency table, which has
been built from the opp table if one exists, instead of querying the
opp table.

Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---

An alternative approach is to clarify in the devfreq code that it's not
possible to pass a freq_table and then in patch 3 create an opp table for the
device in runtime; although the error handling of this becomes non-trivial.

Transitioning the UFSHCD to use opp tables directly is hindered by the fact
that the Qualcomm UFS hardware has two different clocks that needs to be
running at different rates, so we would need a way to describe the two rates in
the opp table. (And would force us to change the DT binding)

 drivers/devfreq/devfreq.c | 22 --
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index fe2af6aa88fc..086ced50a13d 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -74,30 +74,16 @@ static struct devfreq *find_device_devfreq(struct device 
*dev)
 
 static unsigned long find_available_min_freq(struct devfreq *devfreq)
 {
-   struct dev_pm_opp *opp;
-   unsigned long min_freq = 0;
-
-   opp = dev_pm_opp_find_freq_ceil(devfreq->dev.parent, _freq);
-   if (IS_ERR(opp))
-   min_freq = 0;
-   else
-   dev_pm_opp_put(opp);
+   struct devfreq_dev_profile *profile = devfreq->profile;
 
-   return min_freq;
+   return profile->freq_table[0];
 }
 
 static unsigned long find_available_max_freq(struct devfreq *devfreq)
 {
-   struct dev_pm_opp *opp;
-   unsigned long max_freq = ULONG_MAX;
-
-   opp = dev_pm_opp_find_freq_floor(devfreq->dev.parent, _freq);
-   if (IS_ERR(opp))
-   max_freq = 0;
-   else
-   dev_pm_opp_put(opp);
+   struct devfreq_dev_profile *profile = devfreq->profile;
 
-   return max_freq;
+   return profile->freq_table[profile->max_state - 1];
 }
 
 /**
-- 
2.16.2

Re: [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default

[Bjorn Andersson]

On Mon 09 Apr 23:31 PDT 2018, Vivek Gautam wrote:

> 
> 
> On 4/10/2018 1:39 AM, Bjorn Andersson wrote:
> > On Mon 09 Apr 10:38 PDT 2018, Vivek Gautam wrote:
> > > On 4/9/2018 10:21 PM, Bjorn Andersson wrote:
> > > > On Mon 09 Apr 06:24 PDT 2018, Vivek Gautam wrote:
> > [..]
> > > > > diff --git a/include/linux/phy/phy-qcom-ufs.h 
> > > > > b/include/linux/phy/phy-qcom-ufs.h
> > > > > index 0a2c18a9771d..1388c2a2965e 100644
> > > > > --- a/include/linux/phy/phy-qcom-ufs.h
> > > > > +++ b/include/linux/phy/phy-qcom-ufs.h
> > > > > @@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy 
> > > > > *phy);
> > > > > */
> > > > >void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
> > > > > +#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
> > > > >int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
> > > > >void ufs_qcom_phy_save_controller_version(struct phy *phy,
> > > > > - u8 major, u16 minor, u16 step);
> > > > > +   u8 major, u16 minor, u16 
> > > > > step);
> > > > > +#else
> > > > > +static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, 
> > > > > u32 tx_lanes)
> > > > > +{
> > > > > + return -ENOSYS;
> > > > > +}
> > > > > +
> > > > > +static inline void ufs_qcom_phy_save_controller_version(struct phy 
> > > > > *phy,
> > > > > + u8 major, u16 
> > > > > minor,
> > > > > + u16 step)
> > > > > +{
> > > > > +}
> > > > > +#endif /* PHY_QCOM_UFS */
> > > > What's the timeline for getting rid of the references to these
> > > > functions? I presume that code depending on these being here will
> > > > compile but won't actually work?
> > > Yes, these inline definitions are just to keep ufs-qcom happy with the
> > > direct
> > > calls that it makes to these functions.
> > > As you would know these couple of functions are just used by the 20nm phy.
> > > However, we don't have any platform yet in the upstream that enables this
> > > phy.
> > > I am hoping that we will eventually get rid of these functions when we
> > > further
> > > clean up ufs-qcom driver.
> > > 
> > I see, but that means that we're calling this function with a struct phy
> > that might not be a struct ufs_qcom_phy and as such a defconfig with
> > both enabled will have undefined outcome for the migrated phys.
> 
> No, we will have to add support for separate phys as sdm845 has phy per each
> lane,
> and the older struct phy will exist alongside.
> We will call this function only with the older phy pointer.
> 
> > In particular we do expect that the same kernel will boot on db820c and
> > sdm845-mtp, so we will have to enable support for the 14nm & 20nm phy
> > driver (and we don't want random crashes because someone happened to
> > enable it).
> 
> Right, so we create new struct phy while keeping older one intact to keep
> the
> ufs-qcom work with both - ufs_qcom_phy and qmp_phy.
> Some of the controller drivers, such as usb/dwc3/ keep support for old and
> new phys,
> although there the difference is between generic phy and the usb-phy.
> So, I am assuming that if we want to keep ufs-qcom on platforms using 20nm,
> 14nm and 10nm phys happy, we will have to keep the phys separately for
> sometime.
> What do you say about it?
> 

My concern is only that the UFS HCI driver doesn't have a way to know if
it's the new or old "type" of phy, but if you can get that working then
I don't have any objections about doing so for a transitional period.

But, you may not use kernel config options to handle this, the same
Image should boot on msm8916, msm8996 and sdm845 (with appropriate dtb
for each one).

> On db820c, we can still work with the ufs_qcom_phy.
> 

I do not have an issue with that.

Regards,
Bjorn

Re: [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default

[Bjorn Andersson]

On Mon 09 Apr 10:38 PDT 2018, Vivek Gautam wrote:
> On 4/9/2018 10:21 PM, Bjorn Andersson wrote:
> > On Mon 09 Apr 06:24 PDT 2018, Vivek Gautam wrote:
[..]
> > > diff --git a/include/linux/phy/phy-qcom-ufs.h 
> > > b/include/linux/phy/phy-qcom-ufs.h
> > > index 0a2c18a9771d..1388c2a2965e 100644
> > > --- a/include/linux/phy/phy-qcom-ufs.h
> > > +++ b/include/linux/phy/phy-qcom-ufs.h
> > > @@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *phy);
> > >*/
> > >   void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
> > > +#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
> > >   int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
> > >   void ufs_qcom_phy_save_controller_version(struct phy *phy,
> > > - u8 major, u16 minor, u16 step);
> > > +   u8 major, u16 minor, u16 step);
> > > +#else
> > > +static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 
> > > tx_lanes)
> > > +{
> > > + return -ENOSYS;
> > > +}
> > > +
> > > +static inline void ufs_qcom_phy_save_controller_version(struct phy *phy,
> > > + u8 major, u16 minor,
> > > + u16 step)
> > > +{
> > > +}
> > > +#endif /* PHY_QCOM_UFS */
> > What's the timeline for getting rid of the references to these
> > functions? I presume that code depending on these being here will
> > compile but won't actually work?
> 
> Yes, these inline definitions are just to keep ufs-qcom happy with the
> direct
> calls that it makes to these functions.
> As you would know these couple of functions are just used by the 20nm phy.
> However, we don't have any platform yet in the upstream that enables this
> phy.
> I am hoping that we will eventually get rid of these functions when we
> further
> clean up ufs-qcom driver.
> 

I see, but that means that we're calling this function with a struct phy
that might not be a struct ufs_qcom_phy and as such a defconfig with
both enabled will have undefined outcome for the migrated phys.

In particular we do expect that the same kernel will boot on db820c and
sdm845-mtp, so we will have to enable support for the 14nm & 20nm phy
driver (and we don't want random crashes because someone happened to
enable it).


So either we add the 10nm support to the existing driver or I think we
should migrate, at least, the 14nm support to the QMP driver (and mark
the remaining 20nm BROKEN for now).

Regardless of the path chosen this should be cleaned up to the point
where all three phys are supported.

Regards,
Bjorn

Re: [PATCH 1/1] scsi/ufs: qcom: Don't enable PHY_QCOM_UFS by default

[Bjorn Andersson]

On Mon 09 Apr 06:24 PDT 2018, Vivek Gautam wrote:

> While we try to transition from a separate ufs phy driver to a
> more integrated qmp phy driver, don't let SCSI_UFS_QCOM to
> enable PHY_QCOM_UFS config by default.
> The users should enable this in their defconfigs.
> Also add inline definitions for couple of functions -
> a) ufs_qcom_phy_set_tx_lane_enable()
> b) void ufs_qcom_phy_save_controller_version()
> to enable clean build for SCSI_UFS_QCOM.
> 
> Signed-off-by: Vivek Gautam 
> ---
>  drivers/scsi/ufs/Kconfig |  1 -
>  include/linux/phy/phy-qcom-ufs.h | 15 ++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/Kconfig b/drivers/scsi/ufs/Kconfig
> index e27b4d4e6ae2..7e8898b6eb67 100644
> --- a/drivers/scsi/ufs/Kconfig
> +++ b/drivers/scsi/ufs/Kconfig
> @@ -91,7 +91,6 @@ config SCSI_UFS_DWC_TC_PLATFORM
>  config SCSI_UFS_QCOM
>   tristate "QCOM specific hooks to UFS controller platform driver"
>   depends on SCSI_UFSHCD_PLATFORM && ARCH_QCOM
> - select PHY_QCOM_UFS

As you're depending on function in the UFS phy, which can be compiled as
a module you might end up in a configuration where UFS_QCOM is builtin
and the PHY_QCOM_UFS is module, which will fail to link.

So you need to make this:

depends on PHY_QCOM_UFS || PHY_QCOM_UFS=n

In which case we say that if PHY_QCOM_UFS is a module UFS_QCOM must be a
module and if PHY_QCOM_UFS is not compiled in UFS_QCOM can be either
builtin or a module.

>   help
> This selects the QCOM specific additions to UFSHCD platform driver.
> UFS host on QCOM needs some vendor specific configuration before
> diff --git a/include/linux/phy/phy-qcom-ufs.h 
> b/include/linux/phy/phy-qcom-ufs.h
> index 0a2c18a9771d..1388c2a2965e 100644
> --- a/include/linux/phy/phy-qcom-ufs.h
> +++ b/include/linux/phy/phy-qcom-ufs.h
> @@ -31,8 +31,21 @@ void ufs_qcom_phy_enable_dev_ref_clk(struct phy *phy);
>   */
>  void ufs_qcom_phy_disable_dev_ref_clk(struct phy *phy);
>  
> +#if IS_ENABLED(CONFIG_PHY_QCOM_UFS)
>  int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 tx_lanes);
>  void ufs_qcom_phy_save_controller_version(struct phy *phy,
> - u8 major, u16 minor, u16 step);
> +   u8 major, u16 minor, u16 step);
> +#else
> +static inline int ufs_qcom_phy_set_tx_lane_enable(struct phy *phy, u32 
> tx_lanes)
> +{
> + return -ENOSYS;
> +}
> +
> +static inline void ufs_qcom_phy_save_controller_version(struct phy *phy,
> + u8 major, u16 minor,
> + u16 step)
> +{
> +}
> +#endif /* PHY_QCOM_UFS */

What's the timeline for getting rid of the references to these
functions? I presume that code depending on these being here will
compile but won't actually work?

Regards,
Bjorn

[PATCH] scsi: ufs: ufshcd: Enable no_write_same for scsi host

[Bjorn Andersson]

Occasionally the following error message can be seen in the logs of
Qualcomm devices using UFS:

EXT4-fs (sda9): Delayed block allocation failed for inode 685600 at logical 
offset 1086 with max blocks 3 with error 121
EXT4-fs (sda9): This should not happen!! Data will be lost

This is caused by a failing WRITE_SAME command, which per the JEDEC UFS
specification is not a supported. Set the no_write_same flag on the
ufshcd SCSI host to let the SCSI layer know this.

Fixes: 7a3e97b0dc4b ("[SCSI] ufshcd: UFS Host controller driver")
Cc: sta...@vger.kernel.org
Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---
 drivers/scsi/ufs/ufshcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 88c086f5c4e3..e5b1efd1dafd 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -6515,6 +6515,7 @@ static struct scsi_host_template ufshcd_driver_template = 
{
.can_queue  = UFSHCD_CAN_QUEUE,
.max_host_blocked   = 1,
.track_queue_depth  = 1,
+   .no_write_same  = 1,
 };
 
 static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg,
-- 
2.15.0

Re: [PATCH 11/15] remoteproc: make device_type const

[Bjorn Andersson]

On Sat 19 Aug 01:22 PDT 2017, Bhumika Goyal wrote:

> Make this const as it is only stored in the type field of a device
> structure, which is const.
> Done using Coccinelle.
> 
> Signed-off-by: Bhumika Goyal 

Applied, thanks.

Regards,
Bjorn

> ---
>  drivers/remoteproc/remoteproc_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_core.c 
> b/drivers/remoteproc/remoteproc_core.c
> index 364ef28..48b2c5d 100644
> --- a/drivers/remoteproc/remoteproc_core.c
> +++ b/drivers/remoteproc/remoteproc_core.c
> @@ -1360,7 +1360,7 @@ static void rproc_type_release(struct device *dev)
>   kfree(rproc);
>  }
>  
> -static struct device_type rproc_type = {
> +static const struct device_type rproc_type = {
>   .name   = "remoteproc",
>   .release= rproc_type_release,
>  };
> -- 
> 1.9.1
> 

Re: [PATCH 1/6] virtio: wrap find_vqs

[Bjorn Andersson]

On Wed 29 Mar 13:48 PDT 2017, Michael S. Tsirkin wrote:

> We are going to add more parameters to find_vqs, let's wrap the call so
> we don't need to tweak all drivers every time.
> 
> Signed-off-by: Michael S. Tsirkin <m...@redhat.com>

Acked-by: Bjorn Andersson <bjorn.anders...@linaro.org>

Regards,
Bjorn

[PATCH v2] ufs: qcom: Properly clear hba priv on failure

[Bjorn Andersson]

ufs_qcom_init() sets the hba priv data before attempting to acquire the
phy handle, so make sure to clear this in the case of an error. Failing
to do this will make ufs_qcom_setup_clocks() operate on the uninitalized
host object.

Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index d345434b084f..4de372d271b0 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1197,12 +1197,12 @@ static int ufs_qcom_init(struct ufs_hba *hba)
if (IS_ERR(host->generic_phy)) {
err = PTR_ERR(host->generic_phy);
dev_err(dev, "%s: PHY get failed %d\n", __func__, err);
-   goto out;
+   goto out_variant_clear;
}
 
err = ufs_qcom_bus_register(host);
if (err)
-   goto out_host_free;
+   goto out_variant_clear;
 
ufs_qcom_get_controller_revision(hba, >hw_ver.major,
>hw_ver.minor, >hw_ver.step);
@@ -1267,7 +1267,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
phy_power_off(host->generic_phy);
 out_unregister_bus:
phy_exit(host->generic_phy);
-out_host_free:
+out_variant_clear:
ufshcd_set_variant(hba, NULL);
 out:
return err;
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ufs: qcom: Properly clear hba priv on failure

[Bjorn Andersson]

On Sat 19 Nov 12:30 PST 2016, Subhash Jadavani wrote:

> On 2016-11-18 12:55, Bjorn Andersson wrote:
> >In the case where we fail to acquire the phy the hba priv will be set
> >already, so during cleanup ufs_qcom_setup_clocks() will dereference the
> >now free, but still "valid looking" pointer "host".
> 
> host (ufs_qcom_host) was allocated with devm_kzalloc() so i am not sure why
> it would be freed up before probe() returns failure.
> 

Sorry, I missed the fact that the devm_kfree() was dropped, the actual
problem still remains, although it no longer results in a panic.

As ufs_qcom_init() returns from not having found the phy it the variant
data will be a zero-initialized object.  The error path of
ufshcd_hba_init() will then take us through ufs_qcom_setup_clocks(),
which will pass the check for a NULL variant data and use the
uninitialized host object.

I can update the commit message to reflect the new state of things.

Regards,
Bjorn

> >Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
> >---
> > drivers/scsi/ufs/ufs-qcom.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
> >index d345434b084f..7bd88ffee47a 100644
> >--- a/drivers/scsi/ufs/ufs-qcom.c
> >+++ b/drivers/scsi/ufs/ufs-qcom.c
> >@@ -1197,7 +1197,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
> > if (IS_ERR(host->generic_phy)) {
> > err = PTR_ERR(host->generic_phy);
> > dev_err(dev, "%s: PHY get failed %d\n", __func__, err);
> >-goto out;
> >+goto out_host_free;
> > }
> >
> > err = ufs_qcom_bus_register(host);
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ufs: qcom: Properly clear hba priv on failure

[Bjorn Andersson]

In the case where we fail to acquire the phy the hba priv will be set
already, so during cleanup ufs_qcom_setup_clocks() will dereference the
now free, but still "valid looking" pointer "host".

Signed-off-by: Bjorn Andersson <bjorn.anders...@linaro.org>
---
 drivers/scsi/ufs/ufs-qcom.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index d345434b084f..7bd88ffee47a 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1197,7 +1197,7 @@ static int ufs_qcom_init(struct ufs_hba *hba)
if (IS_ERR(host->generic_phy)) {
err = PTR_ERR(host->generic_phy);
dev_err(dev, "%s: PHY get failed %d\n", __func__, err);
-   goto out;
+   goto out_host_free;
}
 
err = ufs_qcom_bus_register(host);
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] ufs: Rename of regulator_set_optimum_mode

[Bjorn Andersson]

On Tue 14 Apr 05:11 PDT 2015, Akinobu Mita wrote:

 2015-02-12 12:35 GMT+09:00 Bjorn Andersson bjorn.anders...@sonymobile.com:
  The function regulator_set_optimum_mode() is changing name to
  regulator_set_load(), so update the code accordingly. Also cleaned up
  ufshcd_config_vreg_load() while touching the code.
 
  Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com

[..]

   static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
   struct ufs_vreg *vreg)
   {
  -   if (!vreg)
  -   return 0;
  -
  return ufshcd_config_vreg_load(hba-dev, vreg, vreg-max_uA);
   }
 
 I tried this patch and it caused kernel null pointer dereference with
 'verg-max_uA' when vreg == NULL.  So you can't simply move !vreg check here
 into ufshcd_config_vreg_load().

Akinobu-san, you're correct, sorry for missing that (and in essence
reverting your commit).

Looks like Linus merged the fix on master, so I will send out a fixup.

Regards,
Bjorn
--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] ufs: Reinstate NULL pointer checks for regulators

[Bjorn Andersson]

The cleanup during the introduction of regulator_set_load() was a too
optimistic and re-opened an issue with vreg being dereferenced while
being NULL.

So reinstate the NULL checks and add back the BUG_ON() to follow the
general coding convention of the implementation.

Fixes: 7b16a07c3293 (ufs: Rename of regulator_set_optimum_mode)
Reported-by: Akinobu Mita akinobu.m...@gmail.com
Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com
---
 drivers/scsi/ufs/ufshcd.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 648a44675880..540e00df6de1 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4262,8 +4262,7 @@ static int ufshcd_config_vreg_load(struct device *dev, 
struct ufs_vreg *vreg,
 {
int ret;
 
-   if (!vreg)
-   return 0;
+   BUG_ON(!vreg);
 
ret = regulator_set_load(vreg-reg, ua);
if (ret  0) {
@@ -4277,12 +4276,18 @@ static int ufshcd_config_vreg_load(struct device *dev, 
struct ufs_vreg *vreg,
 static inline int ufshcd_config_vreg_lpm(struct ufs_hba *hba,
 struct ufs_vreg *vreg)
 {
+   if (!vreg)
+   return 0;
+
return ufshcd_config_vreg_load(hba-dev, vreg, UFS_VREG_LPM_LOAD_UA);
 }
 
 static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
 struct ufs_vreg *vreg)
 {
+   if (!vreg)
+   return 0;
+
return ufshcd_config_vreg_load(hba-dev, vreg, vreg-max_uA);
 }
 
-- 
1.8.2.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/5] ufs: Rename of regulator_set_optimum_mode

[Bjorn Andersson]

The function regulator_set_optimum_mode() is changing name to
regulator_set_load(), so update the code accordingly. Also cleaned up
ufshcd_config_vreg_load() while touching the code.

Signed-off-by: Bjorn Andersson bjorn.anders...@sonymobile.com
---
 drivers/scsi/ufs/ufshcd.c | 27 +++
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 2e4614b..4b73b94 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -4225,22 +4225,15 @@ static struct scsi_host_template ufshcd_driver_template 
= {
 static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg,
   int ua)
 {
-   int ret = 0;
-   struct regulator *reg = vreg-reg;
-   const char *name = vreg-name;
+   int ret;
 
-   BUG_ON(!vreg);
+   if (!vreg)
+   return 0;
 
-   ret = regulator_set_optimum_mode(reg, ua);
-   if (ret = 0) {
-   /*
-* regulator_set_optimum_mode() returns new regulator
-* mode upon success.
-*/
-   ret = 0;
-   } else {
-   dev_err(dev, %s: %s set optimum mode(ua=%d) failed, err=%d\n,
-   __func__, name, ua, ret);
+   ret = regulator_set_load(vreg-reg, ua);
+   if (ret  0) {
+   dev_err(dev, %s: %s set load (ua=%d) failed, err=%d\n,
+   __func__, vreg-name, ua, ret);
}
 
return ret;
@@ -4249,18 +4242,12 @@ static int ufshcd_config_vreg_load(struct device *dev, 
struct ufs_vreg *vreg,
 static inline int ufshcd_config_vreg_lpm(struct ufs_hba *hba,
 struct ufs_vreg *vreg)
 {
-   if (!vreg)
-   return 0;
-
return ufshcd_config_vreg_load(hba-dev, vreg, UFS_VREG_LPM_LOAD_UA);
 }
 
 static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
 struct ufs_vreg *vreg)
 {
-   if (!vreg)
-   return 0;
-
return ufshcd_config_vreg_load(hba-dev, vreg, vreg-max_uA);
 }
 
-- 
1.8.2.2

--
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html