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@xxxxxxx>
> ---
> 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 {
