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

[Subhash Jadavani]

On 2016-12-19 10:38, Rob Herring wrote:
> On Tue, Dec 13, 2016 at 2:16 PM, Subhash Jadavani
> <subhashj@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > > > Signed-off-by: Subhash Jadavani <subhashj@xxxxxxxxxxxxxx>
> > > > ---
> > > >  .../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