| Message ID | 1571804009-29787-2-git-send-email-cang@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Introduce a vops for resetting host controller | expand |
Hi, Can Guo Actually, we already have DME_RESET, this is not enough for Qualcomm host? Thanks, //Bean > > Some UFS host controllers need their specific implementations of resetting to > get them into a good state. Provide a new vops to allow the platform driver to > implement this own reset operation. > > Signed-off-by: Can Guo <cang@codeaurora.org> > --- > drivers/scsi/ufs/ufshcd.c | 16 ++++++++++++++++ drivers/scsi/ufs/ufshcd.h | 10 > ++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index > c28c144..161e3c4 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -3859,6 +3859,14 @@ static int ufshcd_link_recovery(struct ufs_hba *hba) > ufshcd_set_eh_in_progress(hba); > spin_unlock_irqrestore(hba->host->host_lock, flags); > > + ret = ufshcd_vops_full_reset(hba); > + if (ret) > + dev_warn(hba->dev, "%s: full reset returned %d\n", > + __func__, ret); > + > + /* Reset the attached device */ > + ufshcd_vops_device_reset(hba); > + > ret = ufshcd_host_reset_and_restore(hba); > > spin_lock_irqsave(hba->host->host_lock, flags); @@ -6241,6 +6249,11 > @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba) > int retries = MAX_HOST_RESET_RETRIES; > > do { > + err = ufshcd_vops_full_reset(hba); > + if (err) > + dev_warn(hba->dev, "%s: full reset returned %d\n", > + __func__, err); > + > /* Reset the attached device */ > ufshcd_vops_device_reset(hba); > > @@ -8384,6 +8397,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem > *mmio_base, unsigned int irq) > goto exit_gating; > } > > + /* Reset controller to power on reset (POR) state */ > + ufshcd_vops_full_reset(hba); > + > /* Reset the attached device */ > ufshcd_vops_device_reset(hba); > > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index > e0fe247..253b9ea 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -296,6 +296,8 @@ struct ufs_pwr_mode_info { > * @apply_dev_quirks: called to apply device specific quirks > * @suspend: called during host controller PM callback > * @resume: called during host controller PM callback > + * @full_reset: called for handling variant specific implementations of > + * resetting the hci > * @dbg_register_dump: used to dump controller debug information > * @phy_initialization: used to initialize phys > * @device_reset: called to issue a reset pulse on the UFS device @@ -325,6 > +327,7 @@ struct ufs_hba_variant_ops { > int (*apply_dev_quirks)(struct ufs_hba *); > int (*suspend)(struct ufs_hba *, enum ufs_pm_op); > int (*resume)(struct ufs_hba *, enum ufs_pm_op); > + int (*full_reset)(struct ufs_hba *hba); > void (*dbg_register_dump)(struct ufs_hba *hba); > int (*phy_initialization)(struct ufs_hba *); > void (*device_reset)(struct ufs_hba *hba); > @@ -1076,6 +1079,13 @@ static inline int ufshcd_vops_resume(struct ufs_hba > *hba, enum ufs_pm_op op) > return 0; > } > > +static inline int ufshcd_vops_full_reset(struct ufs_hba *hba) { > + if (hba->vops && hba->vops->full_reset) > + return hba->vops->full_reset(hba); > + return 0; > +} > + > static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba) { > if (hba->vops && hba->vops->dbg_register_dump) > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project
On 2019-10-23 18:39, Bean Huo (beanhuo) wrote: > Hi, Can Guo > Actually, we already have DME_RESET, this is not enough for Qualcomm > host? > Thanks, > > //Bean > Hi Bean, Yes, for Qualcomm hosts, we need this to fully reset the whole UFS controller block Can Guo >> >> Some UFS host controllers need their specific implementations of >> resetting to >> get them into a good state. Provide a new vops to allow the platform >> driver to >> implement this own reset operation. >> >> Signed-off-by: Can Guo <cang@codeaurora.org> >> --- >> drivers/scsi/ufs/ufshcd.c | 16 ++++++++++++++++ >> drivers/scsi/ufs/ufshcd.h | 10 >> ++++++++++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index >> c28c144..161e3c4 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -3859,6 +3859,14 @@ static int ufshcd_link_recovery(struct ufs_hba >> *hba) >> ufshcd_set_eh_in_progress(hba); >> spin_unlock_irqrestore(hba->host->host_lock, flags); >> >> + ret = ufshcd_vops_full_reset(hba); >> + if (ret) >> + dev_warn(hba->dev, "%s: full reset returned %d\n", >> + __func__, ret); >> + >> + /* Reset the attached device */ >> + ufshcd_vops_device_reset(hba); >> + >> ret = ufshcd_host_reset_and_restore(hba); >> >> spin_lock_irqsave(hba->host->host_lock, flags); @@ -6241,6 +6249,11 >> @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba) >> int retries = MAX_HOST_RESET_RETRIES; >> >> do { >> + err = ufshcd_vops_full_reset(hba); >> + if (err) >> + dev_warn(hba->dev, "%s: full reset returned %d\n", >> + __func__, err); >> + >> /* Reset the attached device */ >> ufshcd_vops_device_reset(hba); >> >> @@ -8384,6 +8397,9 @@ int ufshcd_init(struct ufs_hba *hba, void >> __iomem >> *mmio_base, unsigned int irq) >> goto exit_gating; >> } >> >> + /* Reset controller to power on reset (POR) state */ >> + ufshcd_vops_full_reset(hba); >> + >> /* Reset the attached device */ >> ufshcd_vops_device_reset(hba); >> >> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h >> index >> e0fe247..253b9ea 100644 >> --- a/drivers/scsi/ufs/ufshcd.h >> +++ b/drivers/scsi/ufs/ufshcd.h >> @@ -296,6 +296,8 @@ struct ufs_pwr_mode_info { >> * @apply_dev_quirks: called to apply device specific quirks >> * @suspend: called during host controller PM callback >> * @resume: called during host controller PM callback >> + * @full_reset: called for handling variant specific implementations >> of >> + * resetting the hci >> * @dbg_register_dump: used to dump controller debug information >> * @phy_initialization: used to initialize phys >> * @device_reset: called to issue a reset pulse on the UFS device @@ >> -325,6 >> +327,7 @@ struct ufs_hba_variant_ops { >> int (*apply_dev_quirks)(struct ufs_hba *); >> int (*suspend)(struct ufs_hba *, enum ufs_pm_op); >> int (*resume)(struct ufs_hba *, enum ufs_pm_op); >> + int (*full_reset)(struct ufs_hba *hba); >> void (*dbg_register_dump)(struct ufs_hba *hba); >> int (*phy_initialization)(struct ufs_hba *); >> void (*device_reset)(struct ufs_hba *hba); >> @@ -1076,6 +1079,13 @@ static inline int ufshcd_vops_resume(struct >> ufs_hba >> *hba, enum ufs_pm_op op) >> return 0; >> } >> >> +static inline int ufshcd_vops_full_reset(struct ufs_hba *hba) { >> + if (hba->vops && hba->vops->full_reset) >> + return hba->vops->full_reset(hba); >> + return 0; >> +} >> + >> static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba) >> { >> if (hba->vops && hba->vops->dbg_register_dump) >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >> Forum, >> a Linux Foundation Collaborative Project
On 10/22/19 9:13 PM, Can Guo wrote: > Some UFS host controllers need their specific implementations of resetting > to get them into a good state. Provide a new vops to allow the platform > driver to implement this own reset operation. > > Signed-off-by: Can Guo <cang@codeaurora.org> > --- > drivers/scsi/ufs/ufshcd.c | 16 ++++++++++++++++ > drivers/scsi/ufs/ufshcd.h | 10 ++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index c28c144..161e3c4 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -3859,6 +3859,14 @@ static int ufshcd_link_recovery(struct ufs_hba *hba) > ufshcd_set_eh_in_progress(hba); > spin_unlock_irqrestore(hba->host->host_lock, flags); > > + ret = ufshcd_vops_full_reset(hba); > + if (ret) > + dev_warn(hba->dev, "%s: full reset returned %d\n", > + __func__, ret); > + > + /* Reset the attached device */ > + ufshcd_vops_device_reset(hba); > + > ret = ufshcd_host_reset_and_restore(hba); > > spin_lock_irqsave(hba->host->host_lock, flags); In all your cases, especially after this adjustment, ufshcd_vops_full_reset is called blindly (+error checking message) before ufshcd_vops_device_reset. What about dropping the .full_reset (should really have been called .hw_reset or .host_reset) addition to the vops, just adding ufshcd_vops_device_reset call here before ufshcd_host_reset_and_restore, and in the driver folding the ufshcd_vops_full_reset code into the .device_reset handler? Would that be workable? It would be simpler if so. I can see a desire for the heads up (ufshcd_vops_full_reset+)ufshcd_vops_device_reset calls before ufshcd_host_reset_and_restore because that function will spin 10 seconds waiting for a response from a standardized register, that itself could be hardware locked up requiring product specific reset procedures. But if that is the case, then what about all the other calls to ufshcd_host_reset_and_restore in this file that are not provided the heads up? My guess is that the host device only demonstrated issues in the ufshcd_link_recovery handling path? Are you sure this is the only path that tickles the controller into a hardware lockup state? Sincerely -- Mark Salyzyn
On 2019-10-31 22:44, Mark Salyzyn wrote: > On 10/22/19 9:13 PM, Can Guo wrote: >> Some UFS host controllers need their specific implementations of >> resetting >> to get them into a good state. Provide a new vops to allow the >> platform >> driver to implement this own reset operation. >> >> Signed-off-by: Can Guo <cang@codeaurora.org> >> --- >> drivers/scsi/ufs/ufshcd.c | 16 ++++++++++++++++ >> drivers/scsi/ufs/ufshcd.h | 10 ++++++++++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index c28c144..161e3c4 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -3859,6 +3859,14 @@ static int ufshcd_link_recovery(struct ufs_hba >> *hba) >> ufshcd_set_eh_in_progress(hba); >> spin_unlock_irqrestore(hba->host->host_lock, flags); >> + ret = ufshcd_vops_full_reset(hba); >> + if (ret) >> + dev_warn(hba->dev, "%s: full reset returned %d\n", >> + __func__, ret); >> + >> + /* Reset the attached device */ >> + ufshcd_vops_device_reset(hba); >> + >> ret = ufshcd_host_reset_and_restore(hba); >> spin_lock_irqsave(hba->host->host_lock, flags); > > In all your cases, especially after this adjustment, > ufshcd_vops_full_reset is called blindly (+error checking message) > before ufshcd_vops_device_reset. What about dropping the .full_reset > (should really have been called .hw_reset or .host_reset) addition to > the vops, just adding ufshcd_vops_device_reset call here before > ufshcd_host_reset_and_restore, and in the driver folding the > ufshcd_vops_full_reset code into the .device_reset handler? > > Would that be workable? It would be simpler if so. > > I can see a desire for the heads up > (ufshcd_vops_full_reset+)ufshcd_vops_device_reset calls before > ufshcd_host_reset_and_restore because that function will spin 10 > seconds waiting for a response from a standardized register, that > itself could be hardware locked up requiring product specific reset > procedures. But if that is the case, then what about all the other > calls to ufshcd_host_reset_and_restore in this file that are not > provided the heads up? My guess is that the host device only > demonstrated issues in the ufshcd_link_recovery handling path? Are you > sure this is the only path that tickles the controller into a hardware > lockup state? > > Sincerely -- Mark Salyzyn Hi Mark Salyzyn, Folding the "full_reset" vops inito "device_reset" vops is one choice for now. Shall do that. Your guess is correct. the head up is needed in ufshcd_link_recovery() path because link is already in bad state when we are here, expeically after hibern8 exit fails. So we need a full reset to PHY and host controller here before host_reset_and_restore. But other calls to host_reset_and_restore are under good conditions. Regards, Can Guo.
Hi, > > Some UFS host controllers need their specific implementations of resetting to > get them into a good state. Provide a new vops to allow the platform driver to > implement this own reset operation. > > Signed-off-by: Can Guo <cang@codeaurora.org> Did you withdraw from this patches and insert them to one of your fix bundle? I couldn't tell. As this is a vop, in what way its functionality can't be included in the device reset that was recently added? Thanks, Avri
Hi, Can I agree with Avri, you can add this series patches to your bundle, and that is also easy to review for us. Otherwise, we are confused by somehow crossing different series patches. Thanks, //Bean > > Hi, > > > > Some UFS host controllers need their specific implementations of > > resetting to get them into a good state. Provide a new vops to allow > > the platform driver to implement this own reset operation. > > > > Signed-off-by: Can Guo <cang@codeaurora.org> > Did you withdraw from this patches and insert them to one of your fix bundle? > I couldn't tell. > As this is a vop, in what way its functionality can't be included in the device reset > that was recently added? > > Thanks, > Avri
On 2019-11-04 22:28, Avri Altman wrote: > Hi, >> >> Some UFS host controllers need their specific implementations of >> resetting to >> get them into a good state. Provide a new vops to allow the platform >> driver to >> implement this own reset operation. >> >> Signed-off-by: Can Guo <cang@codeaurora.org> > Did you withdraw from this patches and insert them to one of your fix > bundle? > I couldn't tell. > As this is a vop, in what way its functionality can't be included in > the device reset that was recently added? > > Thanks, > Avri Hi Avri, Sorry for making you confused. Yes, I dropped this series because it cannot fulfil its purpose anymore. I come up with a way which puts the reset in the right place in UFS QCOM platfrom driver without an extra vops, so I inserted the two changes in fix bundle 3. Thanks, Can Guo.
On 2019-11-04 22:34, Bean Huo (beanhuo) wrote: > Hi, Can > > I agree with Avri, you can add this series patches to your bundle, and > that is also easy to review for us. > Otherwise, we are confused by somehow crossing different series > patches. > Thanks, > > //Bean Hi Bean, I moved the two changes to fix bundle 3. The vops is not needed anymore. Meanwhile I find it inappropriate to put the host controller reset inside the device reset vops. So you can check the new changes in fix bundle 3. Regards, Can Guo. >> >> Hi, >> > >> > Some UFS host controllers need their specific implementations of >> > resetting to get them into a good state. Provide a new vops to allow >> > the platform driver to implement this own reset operation. >> > >> > Signed-off-by: Can Guo <cang@codeaurora.org> >> Did you withdraw from this patches and insert them to one of your fix >> bundle? >> I couldn't tell. >> As this is a vop, in what way its functionality can't be included in >> the device reset >> that was recently added? >> >> Thanks, >> Avri
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index c28c144..161e3c4 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -3859,6 +3859,14 @@ static int ufshcd_link_recovery(struct ufs_hba *hba) ufshcd_set_eh_in_progress(hba); spin_unlock_irqrestore(hba->host->host_lock, flags); + ret = ufshcd_vops_full_reset(hba); + if (ret) + dev_warn(hba->dev, "%s: full reset returned %d\n", + __func__, ret); + + /* Reset the attached device */ + ufshcd_vops_device_reset(hba); + ret = ufshcd_host_reset_and_restore(hba); spin_lock_irqsave(hba->host->host_lock, flags); @@ -6241,6 +6249,11 @@ static int ufshcd_reset_and_restore(struct ufs_hba *hba) int retries = MAX_HOST_RESET_RETRIES; do { + err = ufshcd_vops_full_reset(hba); + if (err) + dev_warn(hba->dev, "%s: full reset returned %d\n", + __func__, err); + /* Reset the attached device */ ufshcd_vops_device_reset(hba); @@ -8384,6 +8397,9 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq) goto exit_gating; } + /* Reset controller to power on reset (POR) state */ + ufshcd_vops_full_reset(hba); + /* Reset the attached device */ ufshcd_vops_device_reset(hba); diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index e0fe247..253b9ea 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -296,6 +296,8 @@ struct ufs_pwr_mode_info { * @apply_dev_quirks: called to apply device specific quirks * @suspend: called during host controller PM callback * @resume: called during host controller PM callback + * @full_reset: called for handling variant specific implementations of + * resetting the hci * @dbg_register_dump: used to dump controller debug information * @phy_initialization: used to initialize phys * @device_reset: called to issue a reset pulse on the UFS device @@ -325,6 +327,7 @@ struct ufs_hba_variant_ops { int (*apply_dev_quirks)(struct ufs_hba *); int (*suspend)(struct ufs_hba *, enum ufs_pm_op); int (*resume)(struct ufs_hba *, enum ufs_pm_op); + int (*full_reset)(struct ufs_hba *hba); void (*dbg_register_dump)(struct ufs_hba *hba); int (*phy_initialization)(struct ufs_hba *); void (*device_reset)(struct ufs_hba *hba); @@ -1076,6 +1079,13 @@ static inline int ufshcd_vops_resume(struct ufs_hba *hba, enum ufs_pm_op op) return 0; } +static inline int ufshcd_vops_full_reset(struct ufs_hba *hba) +{ + if (hba->vops && hba->vops->full_reset) + return hba->vops->full_reset(hba); + return 0; +} + static inline void ufshcd_vops_dbg_register_dump(struct ufs_hba *hba) { if (hba->vops && hba->vops->dbg_register_dump)
Some UFS host controllers need their specific implementations of resetting to get them into a good state. Provide a new vops to allow the platform driver to implement this own reset operation. Signed-off-by: Can Guo <cang@codeaurora.org> --- drivers/scsi/ufs/ufshcd.c | 16 ++++++++++++++++ drivers/scsi/ufs/ufshcd.h | 10 ++++++++++ 2 files changed, 26 insertions(+)