[<RFC,RESEND,v2>,0/3] WriteBooster Feature Support

Message

Asutosh Das (asd) March 21, 2020, 12:59 a.m. UTC

Still a RFC patch, because I'm still expecting some comments
on the design.

v1 -> v2:
- Addressed comments on v1

- Supports shared buffer mode only

- Didn't use exception event as suggested.
  The reason being while testing I saw that the WriteBooster
  available buffer remains at 0x1 for a longer time if flush is
  enabled all the time as compared to an event-based enablement.
  This essentially means that writes go to the WriteBooster buffer
  more. Spec says that the if flush is enabled, the device would
  flush when it sees the command queue empty. So I guess that'd trigger
  flush more than an event based approach.
  Anyway the Vcc would be turned-off during system suspend, so flush
  would stop anyway.
  In this patchset, I never turn-off flush.
  Hence the RFC.

Asutosh Das (3):
  scsi: ufs: add write booster feature support
  ufs-qcom: scsi: configure write booster type
  ufs: sysfs: add sysfs entries for write booster

 drivers/scsi/ufs/ufs-qcom.c  |   7 ++
 drivers/scsi/ufs/ufs-sysfs.c |  39 ++++++-
 drivers/scsi/ufs/ufs.h       |  37 ++++++-
 drivers/scsi/ufs/ufshcd.c    | 238 ++++++++++++++++++++++++++++++++++++++++++-
 drivers/scsi/ufs/ufshcd.h    |   9 ++
 5 files changed, 324 insertions(+), 6 deletions(-)

Comments

Avri Altman March 26, 2020, 9:53 p.m. UTC | #1

Hi,
The code looks good to me and I encourage you to submit your patches.
I just want to make some general remarks:
 - The way you use the clock scaling to set WB on/off, is IMO, an elegant design choice.
 - You should add a platform capability for WB
 - As for setting user space configuration options - I think you've got it wrong.
   You should read it from  bSupportedWriteBoosterBufferUserSpaceReductionTypes
   (offset 0x55 in the device descriptor).
   This is a configurable parameter that the chipset vendor control and can set during provisioning.
 - I agree with your approach toward flush during runtime suspend (keep-vcc-on):
    it is the host's interest to assist the device with some extra idle time if needed for WB flush.
    This way you will assure consistent write performance.

Thanks,
Avri

> 
> 
> Still a RFC patch, because I'm still expecting some comments
> on the design.
> 
> v1 -> v2:
> - Addressed comments on v1
> 
> - Supports shared buffer mode only
> 
> - Didn't use exception event as suggested.
>   The reason being while testing I saw that the WriteBooster
>   available buffer remains at 0x1 for a longer time if flush is
>   enabled all the time as compared to an event-based enablement.
>   This essentially means that writes go to the WriteBooster buffer
>   more. Spec says that the if flush is enabled, the device would
>   flush when it sees the command queue empty. So I guess that'd trigger
>   flush more than an event based approach.
>   Anyway the Vcc would be turned-off during system suspend, so flush
>   would stop anyway.
>   In this patchset, I never turn-off flush.
>   Hence the RFC.
> 
> Asutosh Das (3):
>   scsi: ufs: add write booster feature support
>   ufs-qcom: scsi: configure write booster type
>   ufs: sysfs: add sysfs entries for write booster
> 
>  drivers/scsi/ufs/ufs-qcom.c  |   7 ++
>  drivers/scsi/ufs/ufs-sysfs.c |  39 ++++++-
>  drivers/scsi/ufs/ufs.h       |  37 ++++++-
>  drivers/scsi/ufs/ufshcd.c    | 238
> ++++++++++++++++++++++++++++++++++++++++++-
>  drivers/scsi/ufs/ufshcd.h    |   9 ++
>  5 files changed, 324 insertions(+), 6 deletions(-)
> 
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project.