diff mbox series

[V1,3/3] scsi: ufs: qcom: Add support to disable UFS LPM Feature

Message ID 20250417124645.24456-4-quic_nitirawa@quicinc.com
State New
Headers show
Series Add support to disable UFS LPM | expand

Commit Message

Nitin Rawat April 17, 2025, 12:46 p.m. UTC 
There are emulation FPGA platforms or other platforms where UFS low
power mode is either unsupported or power efficiency is not a critical
requirement.

Disable all low power mode UFS feature based on the "disable-lpm" device
tree property parsed in platform driver.

Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
---
 drivers/ufs/host/ufs-qcom.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

--
2.48.1

Comments

Rob Herring (Arm) April 22, 2025, 12:45 p.m. UTC | #1 
On Thu, Apr 17, 2025 at 06:16:45PM +0530, Nitin Rawat wrote:
> There are emulation FPGA platforms or other platforms where UFS low
> power mode is either unsupported or power efficiency is not a critical
> requirement.
> 
> Disable all low power mode UFS feature based on the "disable-lpm" device
> tree property parsed in platform driver.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> index 1b37449fbffc..1024edf36b68 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1014,13 +1014,14 @@ static void ufs_qcom_set_host_caps(struct ufs_hba *hba)
> 
>  static void ufs_qcom_set_caps(struct ufs_hba *hba)
>  {
> -	hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> -	hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
> -	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
> -	hba->caps |= UFSHCD_CAP_WB_EN;
> -	hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
> -	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
> -
> +	if (!hba->disable_lpm) {
> +		hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> +		hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
> +		hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
> +		hba->caps |= UFSHCD_CAP_WB_EN;
> +		hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
> +		hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
> +	}

Doesn't RuntimePM already have userspace controls? And that's a Linux 
feature that shouldn't really be controlled by DT. I think this property 
should still to things defined by the UFS spec.

Rob
Nitin Rawat April 22, 2025, 7:44 p.m. UTC | #2 
On 4/22/2025 6:15 PM, Rob Herring wrote:
> On Thu, Apr 17, 2025 at 06:16:45PM +0530, Nitin Rawat wrote:
>> There are emulation FPGA platforms or other platforms where UFS low
>> power mode is either unsupported or power efficiency is not a critical
>> requirement.
>>
>> Disable all low power mode UFS feature based on the "disable-lpm" device
>> tree property parsed in platform driver.
>>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>> index 1b37449fbffc..1024edf36b68 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -1014,13 +1014,14 @@ static void ufs_qcom_set_host_caps(struct ufs_hba *hba)
>>
>>   static void ufs_qcom_set_caps(struct ufs_hba *hba)
>>   {
>> -	hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
>> -	hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
>> -	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
>> -	hba->caps |= UFSHCD_CAP_WB_EN;
>> -	hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
>> -	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
>> -
>> +	if (!hba->disable_lpm) {
>> +		hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
>> +		hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
>> +		hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
>> +		hba->caps |= UFSHCD_CAP_WB_EN;
>> +		hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
>> +		hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
>> +	}
> 
> Doesn't RuntimePM already have userspace controls? And that's a Linux
> feature that shouldn't really be controlled by DT. I think this property
> should still to things defined by the UFS spec.

Hi Rob,
Yes userspace has runtime PM control but by the time UFS driver probes 
completes and userspace is up, there are chances runtime PM may get 
kicked in.

Regards,
Nitin

> 
> Rob
Peter Wang (王信友) April 23, 2025, 5 a.m. UTC | #3 
On Thu, 2025-04-17 at 18:16 +0530, Nitin Rawat wrote:
> 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> There are emulation FPGA platforms or other platforms where UFS low
> power mode is either unsupported or power efficiency is not a
> critical
> requirement.
> 
> Disable all low power mode UFS feature based on the "disable-lpm"
> device
> tree property parsed in platform driver.
> 
> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> ---
>  drivers/ufs/host/ufs-qcom.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-
> qcom.c
> index 1b37449fbffc..1024edf36b68 100644
> --- a/drivers/ufs/host/ufs-qcom.c
> +++ b/drivers/ufs/host/ufs-qcom.c
> @@ -1014,13 +1014,14 @@ static void ufs_qcom_set_host_caps(struct
> ufs_hba *hba)
> 
>  static void ufs_qcom_set_caps(struct ufs_hba *hba)
>  {
> -       hba->caps |= UFSHCD_CAP_CLK_GATING |
> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> -       hba->caps |= UFSHCD_CAP_CLK_SCALING |
> UFSHCD_CAP_WB_WITH_CLK_SCALING;
> -       hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
> -       hba->caps |= UFSHCD_CAP_WB_EN;
> -       hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
> -       hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
> -
> +       if (!hba->disable_lpm) {
> +               hba->caps |= UFSHCD_CAP_CLK_GATING |
> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> +               hba->caps |= UFSHCD_CAP_CLK_SCALING |
> UFSHCD_CAP_WB_WITH_CLK_SCALING;
> +               hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
> +               hba->caps |= UFSHCD_CAP_WB_EN;
> 

Hi, Nitin,

If hba->disable_lpm is true, WB should enable?
Normally, you don't care about low power, so why wouldn't you enable
WB?

Thanks.
Peter



> +               hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
> +               hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
> +       }
>         ufs_qcom_set_host_caps(hba);
>  }
> 
> --
> 2.48.1
>
Nitin Rawat April 23, 2025, 6:24 a.m. UTC | #4 
On 4/23/2025 10:30 AM, Peter Wang (王信友) wrote:
> On Thu, 2025-04-17 at 18:16 +0530, Nitin Rawat wrote:
>>
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> There are emulation FPGA platforms or other platforms where UFS low
>> power mode is either unsupported or power efficiency is not a
>> critical
>> requirement.
>>
>> Disable all low power mode UFS feature based on the "disable-lpm"
>> device
>> tree property parsed in platform driver.
>>
>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>> ---
>>   drivers/ufs/host/ufs-qcom.c | 15 ++++++++-------
>>   1 file changed, 8 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-
>> qcom.c
>> index 1b37449fbffc..1024edf36b68 100644
>> --- a/drivers/ufs/host/ufs-qcom.c
>> +++ b/drivers/ufs/host/ufs-qcom.c
>> @@ -1014,13 +1014,14 @@ static void ufs_qcom_set_host_caps(struct
>> ufs_hba *hba)
>>
>>   static void ufs_qcom_set_caps(struct ufs_hba *hba)
>>   {
>> -       hba->caps |= UFSHCD_CAP_CLK_GATING |
>> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
>> -       hba->caps |= UFSHCD_CAP_CLK_SCALING |
>> UFSHCD_CAP_WB_WITH_CLK_SCALING;
>> -       hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
>> -       hba->caps |= UFSHCD_CAP_WB_EN;
>> -       hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
>> -       hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
>> -
>> +       if (!hba->disable_lpm) {
>> +               hba->caps |= UFSHCD_CAP_CLK_GATING |
>> UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
>> +               hba->caps |= UFSHCD_CAP_CLK_SCALING |
>> UFSHCD_CAP_WB_WITH_CLK_SCALING;
>> +               hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
>> +               hba->caps |= UFSHCD_CAP_WB_EN;
>>
> 
> Hi, Nitin,
> 
> If hba->disable_lpm is true, WB should enable?
> Normally, you don't care about low power, so why wouldn't you enable
> WB?
> 
Hi Peter,

Thanks for review. Agree with you.
I will update this in next patchset.

Regards,
Nitin


> Thanks.
> Peter
> 
> 
> 
>> +               hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
>> +               hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
>> +       }
>>          ufs_qcom_set_host_caps(hba);
>>   }
>>
>> --
>> 2.48.1
>>
>
Rob Herring (Arm) April 23, 2025, 1:56 p.m. UTC | #5 
On Wed, Apr 23, 2025 at 01:14:27AM +0530, Nitin Rawat wrote:
> 
> 
> On 4/22/2025 6:15 PM, Rob Herring wrote:
> > On Thu, Apr 17, 2025 at 06:16:45PM +0530, Nitin Rawat wrote:
> > > There are emulation FPGA platforms or other platforms where UFS low
> > > power mode is either unsupported or power efficiency is not a critical
> > > requirement.
> > > 
> > > Disable all low power mode UFS feature based on the "disable-lpm" device
> > > tree property parsed in platform driver.
> > > 
> > > Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
> > > ---
> > >   drivers/ufs/host/ufs-qcom.c | 15 ++++++++-------
> > >   1 file changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
> > > index 1b37449fbffc..1024edf36b68 100644
> > > --- a/drivers/ufs/host/ufs-qcom.c
> > > +++ b/drivers/ufs/host/ufs-qcom.c
> > > @@ -1014,13 +1014,14 @@ static void ufs_qcom_set_host_caps(struct ufs_hba *hba)
> > > 
> > >   static void ufs_qcom_set_caps(struct ufs_hba *hba)
> > >   {
> > > -	hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> > > -	hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
> > > -	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
> > > -	hba->caps |= UFSHCD_CAP_WB_EN;
> > > -	hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
> > > -	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
> > > -
> > > +	if (!hba->disable_lpm) {
> > > +		hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
> > > +		hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
> > > +		hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
> > > +		hba->caps |= UFSHCD_CAP_WB_EN;
> > > +		hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
> > > +		hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
> > > +	}
> > 
> > Doesn't RuntimePM already have userspace controls? And that's a Linux
> > feature that shouldn't really be controlled by DT. I think this property
> > should still to things defined by the UFS spec.
> 
> Hi Rob,
> Yes userspace has runtime PM control but by the time UFS driver probes
> completes and userspace is up, there are chances runtime PM may get kicked
> in.

That sounds like a problem more than 1 device would have...

Rob
Nitin Rawat April 25, 2025, 8:22 p.m. UTC | #6 
On 4/23/2025 7:26 PM, Rob Herring wrote:
> On Wed, Apr 23, 2025 at 01:14:27AM +0530, Nitin Rawat wrote:
>>
>>
>> On 4/22/2025 6:15 PM, Rob Herring wrote:
>>> On Thu, Apr 17, 2025 at 06:16:45PM +0530, Nitin Rawat wrote:
>>>> There are emulation FPGA platforms or other platforms where UFS low
>>>> power mode is either unsupported or power efficiency is not a critical
>>>> requirement.
>>>>
>>>> Disable all low power mode UFS feature based on the "disable-lpm" device
>>>> tree property parsed in platform driver.
>>>>
>>>> Signed-off-by: Nitin Rawat <quic_nitirawa@quicinc.com>
>>>> ---
>>>>    drivers/ufs/host/ufs-qcom.c | 15 ++++++++-------
>>>>    1 file changed, 8 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
>>>> index 1b37449fbffc..1024edf36b68 100644
>>>> --- a/drivers/ufs/host/ufs-qcom.c
>>>> +++ b/drivers/ufs/host/ufs-qcom.c
>>>> @@ -1014,13 +1014,14 @@ static void ufs_qcom_set_host_caps(struct ufs_hba *hba)
>>>>
>>>>    static void ufs_qcom_set_caps(struct ufs_hba *hba)
>>>>    {
>>>> -	hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
>>>> -	hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
>>>> -	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
>>>> -	hba->caps |= UFSHCD_CAP_WB_EN;
>>>> -	hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
>>>> -	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
>>>> -
>>>> +	if (!hba->disable_lpm) {
>>>> +		hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
>>>> +		hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
>>>> +		hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
>>>> +		hba->caps |= UFSHCD_CAP_WB_EN;
>>>> +		hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
>>>> +		hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
>>>> +	}
>>>
>>> Doesn't RuntimePM already have userspace controls? And that's a Linux
>>> feature that shouldn't really be controlled by DT. I think this property
>>> should still to things defined by the UFS spec.
>>
>> Hi Rob,
>> Yes userspace has runtime PM control but by the time UFS driver probes
>> completes and userspace is up, there are chances runtime PM may get kicked
>> in.
> 
> That sounds like a problem more than 1 device would have...

Yes, Rob, since there's a possibility that runtime suspend might be 
triggered for some devices before probe completes and userspace is up, 
IMO it's better to disable the runtime PM capability during 
initialization based on the device tree for specific FPGA platforms that 
don't support LPM.

What's your opinion on this?"

Regards,
Nitin



> 
> Rob
diff mbox series

Patch

diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c
index 1b37449fbffc..1024edf36b68 100644
--- a/drivers/ufs/host/ufs-qcom.c
+++ b/drivers/ufs/host/ufs-qcom.c
@@ -1014,13 +1014,14 @@  static void ufs_qcom_set_host_caps(struct ufs_hba *hba)

 static void ufs_qcom_set_caps(struct ufs_hba *hba)
 {
-	hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
-	hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
-	hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
-	hba->caps |= UFSHCD_CAP_WB_EN;
-	hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
-	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
-
+	if (!hba->disable_lpm) {
+		hba->caps |= UFSHCD_CAP_CLK_GATING | UFSHCD_CAP_HIBERN8_WITH_CLK_GATING;
+		hba->caps |= UFSHCD_CAP_CLK_SCALING | UFSHCD_CAP_WB_WITH_CLK_SCALING;
+		hba->caps |= UFSHCD_CAP_AUTO_BKOPS_SUSPEND;
+		hba->caps |= UFSHCD_CAP_WB_EN;
+		hba->caps |= UFSHCD_CAP_AGGR_POWER_COLLAPSE;
+		hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
+	}
 	ufs_qcom_set_host_caps(hba);
 }