Re: [PATCH v1 07/12] scsi: ufs: add option to change default UFS power management level

[Subhash Jadavani <subhashj@codeaurora.org>]

On 2016-12-19 10:38, Rob Herring wrote:
> On Tue, Dec 13, 2016 at 2:16 PM, Subhash Jadavani
> <subhashj@codeaurora.org> wrote:
>> On 2016-12-13 12:04, Rob Herring wrote:
>>> 
>>> On Mon, Dec 12, 2016 at 04:54:20PM -0800, Subhash Jadavani wrote:
>>>> 
>>>> UFS device and link can be put in multiple different low power modes
>>>> hence
>>>> UFS driver supports multiple different low power modes. By default 
>>>> UFS
>>>> driver selects the default (optimal) low power mode (which gives 
>>>> moderate
>>>> power savings and have relatively less enter and exit latencies) but
>>>> we might have to tune this default power mode for different chipset
>>>> platforms to meet the low power requirements/goals. Hence this patch
>>>> adds option to change default UFS low power mode (level).
>>>> 
>>>> Reviewed-by: Yaniv Gardi <ygardi@codeaurora.org>
>>>> Signed-off-by: Subhash Jadavani <subhashj@codeaurora.org>
>>>> ---
>>>>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt      | 10 ++++++
>>>>  drivers/scsi/ufs/ufshcd-pltfrm.c                   | 14 ++++++++
>>>>  drivers/scsi/ufs/ufshcd.c                          | 39
>>>> ++++++++++++++++++++++
>>>>  drivers/scsi/ufs/ufshcd.h                          |  4 +--
>>>>  4 files changed, 65 insertions(+), 2 deletions(-)
>>>> 
>>>> diff --git a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>>> b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>>> index a99ed55..c3836c5 100644
>>>> --- a/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>>> +++ b/Documentation/devicetree/bindings/ufs/ufshcd-pltfrm.txt
>>>> @@ -41,6 +41,14 @@ Optional properties:
>>>>  -lanes-per-direction   : number of lanes available per direction -
>>>> either 1 or 2.
>>>>                           Note that it is assume same number of 
>>>> lanes is
>>>> used both
>>>>                           directions at once. If not specified, 
>>>> default
>>>> is 2 lanes per direction.
>>>> +- rpm-level            : UFS Runtime power management level. 
>>>> Following
>>>> PM levels are supported:
>>>> +                         0 - Both UFS device and Link in active 
>>>> state
>>>> (Highest power consumption)
>>>> +                         1 - UFS device in active state but Link in
>>>> Hibern8 state
>>>> +                         2 - UFS device in Sleep state but Link in
>>>> active state
>>>> +                         3 - UFS device in Sleep state and Link in
>>>> hibern8 state (default PM level)
>>>> +                         4 - UFS device in Power-down state and 
>>>> Link in
>>>> Hibern8 state
>>>> +                         5 - UFS device in Power-down state and 
>>>> Link in
>>>> OFF state (Lowest power consumption)
>>>> +- spm-level            : UFS System power management level. Allowed 
>>>> PM
>>>> levels are same as rpm-level.
>>> 
>>> 
>>> This looks like you are putting policy for Linux into DT.
>>> 
>>> What I would expect to see here is disabling of states that don't 
>>> work
>>> due to some h/w limitation. Otherwise, it is a user decision for what
>>> modes to go into. Also, I think link and device states should be
>>> separate.
>> 
>> 
>> Yes, generally default level (3) is good enough (and recommended) for 
>> all
>> platforms and most likely user is only expected to change this if they 
>> see
>> issues (most H/W) on their platform or they want even more aggressive 
>> power
>> state (level-4 or level-5) and ready to take the performance hit 
>> associated
>> with resume latencies.
> 
> What latencies can be tolerated is going to depend on the application
> and could vary while running, so putting in DT doesn't make sense. I
> would break down settings like this:
> 
> broken h/w -> DT
> user tuning/config -> sysfs
> sensible defaults -> driver

Make sense.
we already have #2 and #3 in place, will rework this patch so we have a 
way to specify what is broken in h/w.

> 
>> Also, I think it is better to keep Link and device states tied, one 
>> reason
>> is that we can't keep device in sleep/active state when Link is in OFF
>> state.
> 
> The driver can tie the states to together if needed. Just document
> what's broken in DT and let the driver make decisions.

Yes, agreed.  will rework this patch so we have a way to specify what is 
broken in h/w and separate the device and link states (something like 
broken-hibern8, broken-sleep etc.)
Thanks for the suggestions.

> 
> Rob

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project