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

[Subhash Jadavani]

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.

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.

> Rob

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