| Message ID | 20240909231139.2367576-5-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improve the UFS driver link hibernation code | expand |
On Mon, 2024-09-09 at 16:11 -0700, Bart Van Assche wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > For some host controllers it is required that UIC completion > interrupts are > disabled while a power control command is submitted while for other > host > controllers it is required that UIC completion interrupts remain > enabled. > Hence introduce a quirk for preserving the current behavior and leave > UIC > completion interrupts enabled if that quirk has not been set. > Although it > has not yet been observed that the UIC completion interrupt is > reported > after the power mode change interrupt, handle this case by adding a > wait_for_completion_timeout() call on uic_cmd::done. > > Note: the code for toggling the UIC completion interrupt was > introduced > by commit d75f7fe495cf ("scsi: ufs: reduce the interrupts for power > mode > change requests"). > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/ufs/core/ufshcd.c | 15 ++++++++++++++- > drivers/ufs/host/ufs-mediatek.c | 1 + > drivers/ufs/host/ufs-qcom.c | 2 ++ > include/ufs/ufshcd.h | 6 ++++++ > 4 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index 063fb66c6719..23cd6f4a6ca2 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -4257,7 +4257,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba > *hba, struct uic_command *cmd) > goto out_unlock; > } > hba->uic_async_done = &uic_async_done; > - if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & > UIC_COMMAND_COMPL) { > + if (hba->quirks & UFSHCD_QUIRK_DISABLE_UIC_INTR_FOR_PWR_CMDS && > + ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & > UIC_COMMAND_COMPL) { > ufshcd_disable_intr(hba, UIC_COMMAND_COMPL); > /* > * Make sure UIC command completion interrupt is > disabled before > @@ -4275,6 +4276,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba > *hba, struct uic_command *cmd) > goto out; > } > > + /* Wait for power mode change interrupt. */ > if (!wait_for_completion_timeout(hba->uic_async_done, > msecs_to_jiffies(uic_cmd_timeo > ut))) { > dev_err(hba->dev, > @@ -4291,6 +4293,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba > *hba, struct uic_command *cmd) > goto out; > } > > + if (!reenable_intr) { > + /* Wait for UIC completion interrupt. */ > + ret = wait_for_completion_timeout(&cmd->done, > + msecs_to_jiffies(uic_cmd_time > out)); > + WARN_ON_ONCE(ret < 0); > + if (ret == 0) > + dev_err(hba->dev, "UIC command %#x timed > out\n", > + cmd->command); > + ret = 0; > + } > + > check_upmcrs: > status = ufshcd_get_upmcrs(hba); > if (status != PWR_LOCAL) { > diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs- > mediatek.c > index 02c9064284e1..4e18ecc54f9f 100644 > --- a/drivers/ufs/host/ufs-mediatek.c > +++ b/drivers/ufs/host/ufs-mediatek.c > @@ -1021,6 +1021,7 @@ static int ufs_mtk_init(struct ufs_hba *hba) > hba->quirks |= UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL; > hba->quirks |= UFSHCD_QUIRK_MCQ_BROKEN_INTR; > hba->quirks |= UFSHCD_QUIRK_MCQ_BROKEN_RTC; > + hba->quirks |= UFSHCD_QUIRK_DISABLE_UIC_INTR_FOR_PWR_CMDS; > > Hi Bart, I'm not sure what other SoC host think, but the meaning of a "quirk" should be to use it in situations that do not follow the spec and require special handling. However, MediaTek designs according to the spec, so using a quirk may give the impression that MediaTek does not follow the spec. Moreover, it shouldn't be the case that only MediaTek and Qualcomm need to add this quirk." Thanks. Peter > enum ufshcd_caps {
On 9/10/24 5:36 AM, Peter Wang (王信友) wrote: > I'm not sure what other SoC host think, but the meaning of a "quirk" > should be to use it in situations that do not follow the spec and > require special handling. However, MediaTek designs according to > the spec, so using a quirk may give the impression that MediaTek > does not follow the spec. > > Moreover, it shouldn't be the case that only MediaTek and Qualcomm > need to add this quirk." Got it. Let me think further about this and see whether there is perhaps a better solution. Thanks, Bart.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index 063fb66c6719..23cd6f4a6ca2 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -4257,7 +4257,8 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd) goto out_unlock; } hba->uic_async_done = &uic_async_done; - if (ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) { + if (hba->quirks & UFSHCD_QUIRK_DISABLE_UIC_INTR_FOR_PWR_CMDS && + ufshcd_readl(hba, REG_INTERRUPT_ENABLE) & UIC_COMMAND_COMPL) { ufshcd_disable_intr(hba, UIC_COMMAND_COMPL); /* * Make sure UIC command completion interrupt is disabled before @@ -4275,6 +4276,7 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd) goto out; } + /* Wait for power mode change interrupt. */ if (!wait_for_completion_timeout(hba->uic_async_done, msecs_to_jiffies(uic_cmd_timeout))) { dev_err(hba->dev, @@ -4291,6 +4293,17 @@ static int ufshcd_uic_pwr_ctrl(struct ufs_hba *hba, struct uic_command *cmd) goto out; } + if (!reenable_intr) { + /* Wait for UIC completion interrupt. */ + ret = wait_for_completion_timeout(&cmd->done, + msecs_to_jiffies(uic_cmd_timeout)); + WARN_ON_ONCE(ret < 0); + if (ret == 0) + dev_err(hba->dev, "UIC command %#x timed out\n", + cmd->command); + ret = 0; + } + check_upmcrs: status = ufshcd_get_upmcrs(hba); if (status != PWR_LOCAL) { diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-mediatek.c index 02c9064284e1..4e18ecc54f9f 100644 --- a/drivers/ufs/host/ufs-mediatek.c +++ b/drivers/ufs/host/ufs-mediatek.c @@ -1021,6 +1021,7 @@ static int ufs_mtk_init(struct ufs_hba *hba) hba->quirks |= UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL; hba->quirks |= UFSHCD_QUIRK_MCQ_BROKEN_INTR; hba->quirks |= UFSHCD_QUIRK_MCQ_BROKEN_RTC; + hba->quirks |= UFSHCD_QUIRK_DISABLE_UIC_INTR_FOR_PWR_CMDS; hba->vps->wb_flush_threshold = UFS_WB_BUF_REMAIN_PERCENT(80); if (host->caps & UFS_MTK_CAP_DISABLE_AH8) diff --git a/drivers/ufs/host/ufs-qcom.c b/drivers/ufs/host/ufs-qcom.c index 810e637047d0..07a62de80d2e 100644 --- a/drivers/ufs/host/ufs-qcom.c +++ b/drivers/ufs/host/ufs-qcom.c @@ -852,6 +852,8 @@ static void ufs_qcom_advertise_quirks(struct ufs_hba *hba) { struct ufs_qcom_host *host = ufshcd_get_variant(hba); + hba->quirks |= UFSHCD_QUIRK_DISABLE_UIC_INTR_FOR_PWR_CMDS; + if (host->hw_ver.major == 0x2) hba->quirks |= UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION; diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h index 85933775c9f3..787c44a341a7 100644 --- a/include/ufs/ufshcd.h +++ b/include/ufs/ufshcd.h @@ -676,6 +676,12 @@ enum ufshcd_quirks { * the standard best practice for managing keys). */ UFSHCD_QUIRK_KEYS_IN_PRDT = 1 << 24, + + /* + * Disable the UIC interrupt before submitting any power mode change + * commands. + */ + UFSHCD_QUIRK_DISABLE_UIC_INTR_FOR_PWR_CMDS = 1 << 25, }; enum ufshcd_caps {
For some host controllers it is required that UIC completion interrupts are disabled while a power control command is submitted while for other host controllers it is required that UIC completion interrupts remain enabled. Hence introduce a quirk for preserving the current behavior and leave UIC completion interrupts enabled if that quirk has not been set. Although it has not yet been observed that the UIC completion interrupt is reported after the power mode change interrupt, handle this case by adding a wait_for_completion_timeout() call on uic_cmd::done. Note: the code for toggling the UIC completion interrupt was introduced by commit d75f7fe495cf ("scsi: ufs: reduce the interrupts for power mode change requests"). Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/ufs/core/ufshcd.c | 15 ++++++++++++++- drivers/ufs/host/ufs-mediatek.c | 1 + drivers/ufs/host/ufs-qcom.c | 2 ++ include/ufs/ufshcd.h | 6 ++++++ 4 files changed, 23 insertions(+), 1 deletion(-)