[0/3] (Qualcomm) UFS device reset support

Message

Bjorn Andersson June 4, 2019, 7:19 a.m. UTC

This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
acquire and toggle this and then adds this to SDM845 MTP.

Bjorn Andersson (3):
  pinctrl: qcom: sdm845: Expose ufs_reset as gpio
  scsi: ufs: Allow resetting the UFS device
  arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

 .../bindings/pinctrl/qcom,sdm845-pinctrl.txt  |  2 +-
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts       |  2 +
 drivers/pinctrl/qcom/pinctrl-sdm845.c         | 12 ++---
 drivers/scsi/ufs/ufshcd.c                     | 44 +++++++++++++++++++
 drivers/scsi/ufs/ufshcd.h                     |  4 ++
 5 files changed, 57 insertions(+), 7 deletions(-)

Comments

John Stultz June 4, 2019, 10 p.m. UTC | #1

On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
<[e-mail]> wrote:
>
> This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> acquire and toggle this and then adds this to SDM845 MTP.
>
> Bjorn Andersson (3):
>   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
>   scsi: ufs: Allow resetting the UFS device
>   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO

Adding similar change as in sdm845-mtp to the not yet upstream
blueline dts, I validated this allows my micron UFS pixel3 to boot.

Tested-by: John Stultz <[e-mail]>

thanks
-john

Avri Altman June 5, 2019, 5:50 a.m. UTC | #2

Hi,

> 
> On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> <[e-mail]> wrote:
> >
> > This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> > acquire and toggle this and then adds this to SDM845 MTP.
> >
> > Bjorn Andersson (3):
> >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> >   scsi: ufs: Allow resetting the UFS device
> >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> 
> Adding similar change as in sdm845-mtp to the not yet upstream
> blueline dts, I validated this allows my micron UFS pixel3 to boot.
> 
> Tested-by: John Stultz <[e-mail]>
Maybe ufs_hba_variant_ops would be the proper place to add this?

Thanks,
Avri



> 
> thanks
> -john

Bjorn Andersson June 5, 2019, 6:01 a.m. UTC | #3

On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:

> Hi,
> 
> > 
> > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > <[e-mail]> wrote:
> > >
> > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> > > acquire and toggle this and then adds this to SDM845 MTP.
> > >
> > > Bjorn Andersson (3):
> > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > >   scsi: ufs: Allow resetting the UFS device
> > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > 
> > Adding similar change as in sdm845-mtp to the not yet upstream
> > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > 
> > Tested-by: John Stultz <[e-mail]>
> Maybe ufs_hba_variant_ops would be the proper place to add this?
> 

Are you saying that these memories only need a reset when they are
paired with the Qualcomm host controller?

The way it's implemented it here is that the device-reset GPIO is
optional and only if you specify it we'll toggle the reset. So if your
board design has a UFS memory that requires a reset pulse during
initialization you specify this, regardless of which vendor your SoC
comes from.

Regards,
Bjorn

Avri Altman June 5, 2019, 9:32 a.m. UTC | #4

> 
> On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:
> 
> > Hi,
> >
> > >
> > > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > > <[e-mail]> wrote:
> > > >
> > > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> > > > acquire and toggle this and then adds this to SDM845 MTP.
> > > >
> > > > Bjorn Andersson (3):
> > > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > > >   scsi: ufs: Allow resetting the UFS device
> > > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > >
> > > Adding similar change as in sdm845-mtp to the not yet upstream
> > > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > >
> > > Tested-by: John Stultz <[e-mail]>
> > Maybe ufs_hba_variant_ops would be the proper place to add this?
> >
> 
> Are you saying that these memories only need a reset when they are
> paired with the Qualcomm host controller?
ufs_hba_variant_ops is for vendors to implement their own vops,
and as you can see, many of them do.
Adding hw_reset to that template seems like the proper way
to do what you are doing.

Thanks,
Avri
> 
> The way it's implemented it here is that the device-reset GPIO is
> optional and only if you specify it we'll toggle the reset. So if your
> board design has a UFS memory that requires a reset pulse during
> initialization you specify this, regardless of which vendor your SoC
> comes from.
> 
> Regards,
> Bjorn

Bjorn Andersson June 6, 2019, 12:39 a.m. UTC | #5

On Wed 05 Jun 02:32 PDT 2019, Avri Altman wrote:

> > 
> > On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:
> > 
> > > Hi,
> > >
> > > >
> > > > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > > > <[e-mail]> wrote:
> > > > >
> > > > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd to
> > > > > acquire and toggle this and then adds this to SDM845 MTP.
> > > > >
> > > > > Bjorn Andersson (3):
> > > > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > > > >   scsi: ufs: Allow resetting the UFS device
> > > > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > > >
> > > > Adding similar change as in sdm845-mtp to the not yet upstream
> > > > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > > >
> > > > Tested-by: John Stultz <[e-mail]>
> > > Maybe ufs_hba_variant_ops would be the proper place to add this?
> > >
> > 
> > Are you saying that these memories only need a reset when they are
> > paired with the Qualcomm host controller?
> ufs_hba_variant_ops is for vendors to implement their own vops,
> and as you can see, many of them do.
> Adding hw_reset to that template seems like the proper way
> to do what you are doing.
> 

Right, but the vops is operations related to the UFS controller, this
property relates to the memory connected.

E.g I have a Hynix memory and John have a Micron memory that needs this
reset and my assumption is that these memories will need their RESET pin
toggled regardless of which controller they are connected to.

Regards,
Bjorn

Avri Altman June 6, 2019, 6:32 a.m. UTC | #6

> 
> On Wed 05 Jun 02:32 PDT 2019, Avri Altman wrote:
> 
> > >
> > > On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:
> > >
> > > > Hi,
> > > >
> > > > >
> > > > > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > > > > <[e-mail]> wrote:
> > > > > >
> > > > > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd
> to
> > > > > > acquire and toggle this and then adds this to SDM845 MTP.
> > > > > >
> > > > > > Bjorn Andersson (3):
> > > > > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > > > > >   scsi: ufs: Allow resetting the UFS device
> > > > > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > > > >
> > > > > Adding similar change as in sdm845-mtp to the not yet upstream
> > > > > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > > > >
> > > > > Tested-by: John Stultz <[e-mail]>
> > > > Maybe ufs_hba_variant_ops would be the proper place to add this?
> > > >
> > >
> > > Are you saying that these memories only need a reset when they are
> > > paired with the Qualcomm host controller?
> > ufs_hba_variant_ops is for vendors to implement their own vops,
> > and as you can see, many of them do.
> > Adding hw_reset to that template seems like the proper way
> > to do what you are doing.
> >
> 
> Right, but the vops is operations related to the UFS controller, this
> property relates to the memory connected.
This is not entirely accurate. Those are vendor/board specific,
As the original commit log indicates:
" vendor/board specific and hence determined with
 the help of compatible property in device tree."

I would rather have this new vop:
void    (*device_reset)(struct ufs_hba *), Or whatever, 
actively set in ufs_hba_variant_ops, rather than ufshcd_init_device_reset
failing as part of the default init flow.

Thanks,
Avri

> 
> E.g I have a Hynix memory and John have a Micron memory that needs this
> reset and my assumption is that these memories will need their RESET pin
> toggled regardless of which controller they are connected to.
> 
> Regards,
> Bjorn

Bjorn Andersson June 6, 2019, 7:09 a.m. UTC | #7

On Wed 05 Jun 23:32 PDT 2019, Avri Altman wrote:

> > 
> > On Wed 05 Jun 02:32 PDT 2019, Avri Altman wrote:
> > 
> > > >
> > > > On Tue 04 Jun 22:50 PDT 2019, Avri Altman wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > >
> > > > > > On Tue, Jun 4, 2019 at 12:22 AM Bjorn Andersson
> > > > > > <[e-mail]> wrote:
> > > > > > >
> > > > > > > This series exposes the ufs_reset line as a gpio, adds support for ufshcd
> > to
> > > > > > > acquire and toggle this and then adds this to SDM845 MTP.
> > > > > > >
> > > > > > > Bjorn Andersson (3):
> > > > > > >   pinctrl: qcom: sdm845: Expose ufs_reset as gpio
> > > > > > >   scsi: ufs: Allow resetting the UFS device
> > > > > > >   arm64: dts: qcom: sdm845-mtp: Specify UFS device-reset GPIO
> > > > > >
> > > > > > Adding similar change as in sdm845-mtp to the not yet upstream
> > > > > > blueline dts, I validated this allows my micron UFS pixel3 to boot.
> > > > > >
> > > > > > Tested-by: John Stultz <[e-mail]>
> > > > > Maybe ufs_hba_variant_ops would be the proper place to add this?
> > > > >
> > > >
> > > > Are you saying that these memories only need a reset when they are
> > > > paired with the Qualcomm host controller?
> > > ufs_hba_variant_ops is for vendors to implement their own vops,
> > > and as you can see, many of them do.
> > > Adding hw_reset to that template seems like the proper way
> > > to do what you are doing.
> > >
> > 
> > Right, but the vops is operations related to the UFS controller, this
> > property relates to the memory connected.
> This is not entirely accurate. Those are vendor/board specific,
> As the original commit log indicates:
> " vendor/board specific and hence determined with
>  the help of compatible property in device tree."
> 
> I would rather have this new vop:
> void    (*device_reset)(struct ufs_hba *), Or whatever, 
> actively set in ufs_hba_variant_ops, rather than ufshcd_init_device_reset
> failing as part of the default init flow.
> 

But such an vops would allow me to provide a Qualcomm-specific way of
toggling the GPIO that is connected to the UFS_RESET pin on the
Hynix/Micron memory.

But acquiring and toggling GPIOs is not a Qualcomm thing, it's a
completely generic thing, and as it's not a chip-internal line it is a
GPIO and not a reset - regardless of SoC vendor.
Further more, it's optional so boards that does not have this pin
connected will just omit the property in their hardware description
(DeviceTree).


So I think the halting part here is that we don't have a representation
of the memory device's resources, because this is really a matter of
toggling the reset pin on the memory device.

Regards,
Bjorn