[PATCH 0/4] scsi: ufs-mediatek: Fixes for kernel v5.10

[PATCH 0/4] scsi: ufs-mediatek: Fixes for kernel v5.10

[Stanley Chu]

Hi Martin, Avri,

This series fix some defects and introduce host reset mechanism in MediaTek UFS platforms.
Please consider this patch series for kernel v5.10.

Thanks,

Stanley Chu

Stanley Chu (4):
  scsi: ufs-mediatek: Eliminate error message for unbound mphy
  scsi: ufs-mediatek: Fix HOST_PA_TACTIVATE quirk
  scsi: ufs-mediatek: Fix flag of unipro low-power mode
  scsi: ufs-mediatek: Add host reset mechanism

 drivers/scsi/ufs/ufs-mediatek.c | 79 ++++++++++++++++++++++++++-------
 drivers/scsi/ufs/ufs-mediatek.h |  3 ++
 2 files changed, 67 insertions(+), 15 deletions(-)

-- 
2.18.0

[PATCH 1/4] scsi: ufs-mediatek: Eliminate error message for unbound mphy

[Stanley Chu]

Some MediaTek platforms does not have to bind MPHY so users
shall not see any unnecessary logs. Simply remove logs for this
case.

Fixes: fc4983018fea ("scsi: ufs-mediatek: Allow unbound mphy")
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs-mediatek.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index 29cd017c1aa0..7487b25fa651 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -129,7 +129,10 @@ static int ufs_mtk_bind_mphy(struct ufs_hba *hba)
 			__func__, err);
 	} else if (IS_ERR(host->mphy)) {
 		err = PTR_ERR(host->mphy);
-		dev_info(dev, "%s: PHY get failed %d\n", __func__, err);
+		if (err != -ENODEV) {
+			dev_info(dev, "%s: PHY get failed %d\n", __func__,
+				 err);
+		}
 	}
 
 	if (err)
-- 
2.18.0

RE: [PATCH 1/4] scsi: ufs-mediatek: Eliminate error message for unbound mphy

[Avri Altman]

> 
> Some MediaTek platforms does not have to bind MPHY so users
> shall not see any unnecessary logs. Simply remove logs for this
> case.
> 
> Fixes: fc4983018fea ("scsi: ufs-mediatek: Allow unbound mphy")
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

[PATCH 2/4] scsi: ufs-mediatek: Fix HOST_PA_TACTIVATE quirk

[Stanley Chu]

Simply add HOST_PA_TACTIVATE quirk back since it was incorrectly
removed before.

Fixes: 47d054580a75 ("scsi: ufs-mediatek: fix HOST_PA_TACTIVATE quirk for Samsung UFS Devices")
Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs-mediatek.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index 7487b25fa651..887c03e8bcc0 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -672,13 +672,7 @@ static int ufs_mtk_apply_dev_quirks(struct ufs_hba *hba)
 
 static void ufs_mtk_fixup_dev_quirks(struct ufs_hba *hba)
 {
-	struct ufs_dev_info *dev_info = &hba->dev_info;
-	u16 mid = dev_info->wmanufacturerid;
-
 	ufshcd_fixup_dev_quirks(hba, ufs_mtk_dev_fixups);
-
-	if (mid == UFS_VENDOR_SAMSUNG)
-		hba->dev_quirks &= ~UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE;
 }
 
 /**
-- 
2.18.0

RE: [PATCH 2/4] scsi: ufs-mediatek: Fix HOST_PA_TACTIVATE quirk

[Avri Altman]

> 
> Simply add HOST_PA_TACTIVATE quirk back since it was incorrectly
> removed before.
> 
> Fixes: 47d054580a75 ("scsi: ufs-mediatek: fix HOST_PA_TACTIVATE quirk for
> Samsung UFS Devices")
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>

[PATCH 3/4] scsi: ufs-mediatek: Fix flag of unipro low-power mode

[Stanley Chu]

Forcibly leave UniPro low-power mode if UIC commands is failed.
This makes hba_enable_delay_us as correct (default) value for
re-enabling the host.

At the same time, change type of parameter "lpm" in function
ufs_mtk_unipro_set_pm() to "bool".

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs-mediatek.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index 887c03e8bcc0..feba74a72309 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -419,7 +419,7 @@ static int ufs_mtk_pwr_change_notify(struct ufs_hba *hba,
 	return ret;
 }
 
-static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, u32 lpm)
+static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, bool lpm)
 {
 	int ret;
 	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
@@ -427,8 +427,14 @@ static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, u32 lpm)
 	ret = ufshcd_dme_set(hba,
 			     UIC_ARG_MIB_SEL(VS_UNIPROPOWERDOWNCONTROL, 0),
 			     lpm);
-	if (!ret)
+	if (!ret || !lpm) {
+		/*
+		 * Forcibly set as non-LPM mode if UIC commands is failed
+		 * to use default hba_enable_delay_us value for re-enabling
+		 * the host.
+		 */
 		host->unipro_lpm = lpm;
+	}
 
 	return ret;
 }
@@ -438,7 +444,9 @@ static int ufs_mtk_pre_link(struct ufs_hba *hba)
 	int ret;
 	u32 tmp;
 
-	ufs_mtk_unipro_set_pm(hba, 0);
+	ret = ufs_mtk_unipro_set_pm(hba, false);
+	if (ret)
+		return ret;
 
 	/*
 	 * Setting PA_Local_TX_LCC_Enable to 0 before link startup
@@ -546,7 +554,7 @@ static int ufs_mtk_link_set_hpm(struct ufs_hba *hba)
 	if (err)
 		return err;
 
-	err = ufs_mtk_unipro_set_pm(hba, 0);
+	err = ufs_mtk_unipro_set_pm(hba, false);
 	if (err)
 		return err;
 
@@ -567,10 +575,10 @@ static int ufs_mtk_link_set_lpm(struct ufs_hba *hba)
 {
 	int err;
 
-	err = ufs_mtk_unipro_set_pm(hba, 1);
+	err = ufs_mtk_unipro_set_pm(hba, true);
 	if (err) {
 		/* Resume UniPro state for following error recovery */
-		ufs_mtk_unipro_set_pm(hba, 0);
+		ufs_mtk_unipro_set_pm(hba, false);
 		return err;
 	}
 
-- 
2.18.0

RE: [PATCH 3/4] scsi: ufs-mediatek: Fix flag of unipro low-power mode

[Avri Altman]

> 
> Forcibly leave UniPro low-power mode if UIC commands is failed.
> This makes hba_enable_delay_us as correct (default) value for
> re-enabling the host.
> 
> At the same time, change type of parameter "lpm" in function
> ufs_mtk_unipro_set_pm() to "bool".
Semantically, better leave it u32 as its eventually assigned to the arg3 of the uic command 

> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> ---
>  drivers/scsi/ufs/ufs-mediatek.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> index 887c03e8bcc0..feba74a72309 100644
> --- a/drivers/scsi/ufs/ufs-mediatek.c
> +++ b/drivers/scsi/ufs/ufs-mediatek.c
> @@ -419,7 +419,7 @@ static int ufs_mtk_pwr_change_notify(struct ufs_hba
> *hba,
>         return ret;
>  }
> 
> -static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, u32 lpm)
> +static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, bool lpm)
>  {
>         int ret;
>         struct ufs_mtk_host *host = ufshcd_get_variant(hba);
> @@ -427,8 +427,14 @@ static int ufs_mtk_unipro_set_pm(struct ufs_hba
> *hba, u32 lpm)
>         ret = ufshcd_dme_set(hba,
>                              UIC_ARG_MIB_SEL(VS_UNIPROPOWERDOWNCONTROL, 0),
>                              lpm);
> -       if (!ret)
> +       if (!ret || !lpm) {
> +               /*
> +                * Forcibly set as non-LPM mode if UIC commands is failed
> +                * to use default hba_enable_delay_us value for re-enabling
> +                * the host.
> +                */
>                 host->unipro_lpm = lpm;
Maybe just host->unipro_lpm = false; instead

RE: [PATCH 3/4] scsi: ufs-mediatek: Fix flag of unipro low-power mode

[Stanley Chu]

Hi Avri,

On Sat, 2020-09-19 at 08:08 +0000, Avri Altman wrote:
> > 
> > Forcibly leave UniPro low-power mode if UIC commands is failed.
> > This makes hba_enable_delay_us as correct (default) value for
> > re-enabling the host.
> > 
> > At the same time, change type of parameter "lpm" in function
> > ufs_mtk_unipro_set_pm() to "bool".
> Semantically, better leave it u32 as its eventually assigned to the arg3 of the uic command 
> 

Thanks for reminding.
I will use specific u32 values while sending arg3 to fix this.

> > 
> > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
> > ---
> >  drivers/scsi/ufs/ufs-mediatek.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
> > index 887c03e8bcc0..feba74a72309 100644
> > --- a/drivers/scsi/ufs/ufs-mediatek.c
> > +++ b/drivers/scsi/ufs/ufs-mediatek.c
> > @@ -419,7 +419,7 @@ static int ufs_mtk_pwr_change_notify(struct ufs_hba
> > *hba,
> >         return ret;
> >  }
> > 
> > -static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, u32 lpm)
> > +static int ufs_mtk_unipro_set_pm(struct ufs_hba *hba, bool lpm)
> >  {
> >         int ret;
> >         struct ufs_mtk_host *host = ufshcd_get_variant(hba);
> > @@ -427,8 +427,14 @@ static int ufs_mtk_unipro_set_pm(struct ufs_hba
> > *hba, u32 lpm)
> >         ret = ufshcd_dme_set(hba,
> >                              UIC_ARG_MIB_SEL(VS_UNIPROPOWERDOWNCONTROL, 0),
> >                              lpm);
> > -       if (!ret)
> > +       if (!ret || !lpm) {
> > +               /*
> > +                * Forcibly set as non-LPM mode if UIC commands is failed
> > +                * to use default hba_enable_delay_us value for re-enabling
> > +                * the host.
> > +                */
> >                 host->unipro_lpm = lpm;
> Maybe just host->unipro_lpm = false; instead

This statement shall stay like this for case: "lpm = true"

Thanks,
Stanley Chu

[PATCH 4/4] scsi: ufs-mediatek: Add host reset mechanism

[Stanley Chu]

Add host reset mechanism to try to recover host-side errors
during recovery flow.

Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
---
 drivers/scsi/ufs/ufs-mediatek.c | 48 +++++++++++++++++++++++++++++++--
 drivers/scsi/ufs/ufs-mediatek.h |  3 +++
 2 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-mediatek.c b/drivers/scsi/ufs/ufs-mediatek.c
index feba74a72309..1a9133ac6efb 100644
--- a/drivers/scsi/ufs/ufs-mediatek.c
+++ b/drivers/scsi/ufs/ufs-mediatek.c
@@ -13,6 +13,7 @@
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/regulator/consumer.h>
+#include <linux/reset.h>
 #include <linux/soc/mediatek/mtk_sip_svc.h>
 
 #include "ufshcd.h"
@@ -91,16 +92,57 @@ static void ufs_mtk_crypto_enable(struct ufs_hba *hba)
 	}
 }
 
+static void ufs_mtk_host_reset(struct ufs_hba *hba)
+{
+	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
+
+	reset_control_assert(host->hci_reset);
+	reset_control_assert(host->crypto_reset);
+	reset_control_assert(host->unipro_reset);
+
+	usleep_range(100, 110);
+
+	reset_control_deassert(host->unipro_reset);
+	reset_control_deassert(host->crypto_reset);
+	reset_control_deassert(host->hci_reset);
+}
+
+static void ufs_mtk_init_reset_control(struct ufs_hba *hba,
+				       struct reset_control **rc,
+				       char *str)
+{
+	*rc = devm_reset_control_get(hba->dev, str);
+	if (IS_ERR(*rc)) {
+		dev_info(hba->dev, "Failed to get reset control %s: %d\n",
+			 str, PTR_ERR(*rc));
+		*rc = NULL;
+	}
+}
+
+static void ufs_mtk_init_reset(struct ufs_hba *hba)
+{
+	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
+
+	ufs_mtk_init_reset_control(hba, &host->hci_reset,
+				   "hci_rst");
+	ufs_mtk_init_reset_control(hba, &host->unipro_reset,
+				   "unipro_rst");
+	ufs_mtk_init_reset_control(hba, &host->crypto_reset,
+				   "crypto_rst");
+}
+
 static int ufs_mtk_hce_enable_notify(struct ufs_hba *hba,
 				     enum ufs_notify_change_status status)
 {
 	struct ufs_mtk_host *host = ufshcd_get_variant(hba);
 
 	if (status == PRE_CHANGE) {
-		if (host->unipro_lpm)
+		if (host->unipro_lpm) {
 			hba->vps->hba_enable_delay_us = 0;
-		else
+		} else {
 			hba->vps->hba_enable_delay_us = 600;
+			ufs_mtk_host_reset(hba);
+		}
 
 		if (hba->caps & UFSHCD_CAP_CRYPTO)
 			ufs_mtk_crypto_enable(hba);
@@ -335,6 +377,8 @@ static int ufs_mtk_init(struct ufs_hba *hba)
 	if (err)
 		goto out_variant_clear;
 
+	ufs_mtk_init_reset(hba);
+
 	/* Enable runtime autosuspend */
 	hba->caps |= UFSHCD_CAP_RPM_AUTOSUSPEND;
 
diff --git a/drivers/scsi/ufs/ufs-mediatek.h b/drivers/scsi/ufs/ufs-mediatek.h
index 87657376d27a..5c32d5f52759 100644
--- a/drivers/scsi/ufs/ufs-mediatek.h
+++ b/drivers/scsi/ufs/ufs-mediatek.h
@@ -92,6 +92,9 @@ enum {
 struct ufs_mtk_host {
 	struct ufs_hba *hba;
 	struct phy *mphy;
+	struct reset_control *hci_reset;
+	struct reset_control *unipro_reset;
+	struct reset_control *crypto_reset;
 	bool mphy_powered_on;
 	bool unipro_lpm;
 	bool ref_clk_enabled;
-- 
2.18.0

RE: [PATCH 4/4] scsi: ufs-mediatek: Add host reset mechanism

[Avri Altman]

> 
> 
> Add host reset mechanism to try to recover host-side errors
> during recovery flow.
> 
> Signed-off-by: Stanley Chu <stanley.chu@mediatek.com>
Reviewed-by Avri Altman <avri.altman@wdc.com>

See some nit below as well.
Thanks,
Avri

> +static void ufs_mtk_init_reset_control(struct ufs_hba *hba,
> +                                      struct reset_control **rc,
> +                                      char *str)
> +{
> +       *rc = devm_reset_control_get(hba->dev, str);
> +       if (IS_ERR(*rc)) {
How about verifying that the line is not shared as well?
Although this might not be an issue on your current platforms,
it might save you aggravation in the future..

> +               dev_info(hba->dev, "Failed to get reset control %s: %d\n",
> +                        str, PTR_ERR(*rc));
> +               *rc = NULL;
> +       }
> +}

Re: [PATCH 0/4] scsi: ufs-mediatek: Fixes for kernel v5.10

[Martin K. Petersen]

Stanley,

> This series fix some defects and introduce host reset mechanism in
> MediaTek UFS platforms.  Please consider this patch series for kernel
> v5.10.

Applied to the 5.10 SCSI staging tree. Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering