[PATCH 0/6] ufs-exynos fixes for gs101

[PATCH 0/6] ufs-exynos fixes for gs101

[Peter Griffin]

Hi folks,

Whilst investigating some stability issues with the upstream ufs-exynos
driver on gs101/Pixel 6 the following fixes have been authored.

Whilst some of the stability issues remain these patches do improve
certain aspects and make things more deterministic especially module
load/unload.

regards,

Peter

Peter Griffin (6):
  scsi: ufs: exynos: ensure pre_link() executes before
    exynos_ufs_phy_init()
  scsi: ufs: exynos: move ufs shareability value to drvdata
  scsi: ufs: exynos: ensure consistent phy reference counts
  scsi: ufs: exynos: Enable PRDT pre-fetching with UFSHCD_CAP_CRYPTO
  scsi: ufs: exynos: Move phy calls to .exit() callback
  scsi: ufs: exynos: put ufs device in reset on .exit() and .suspend()

 drivers/ufs/host/ufs-exynos.c | 62 +++++++++++++++++++++++++----------
 drivers/ufs/host/ufs-exynos.h |  2 ++
 2 files changed, 46 insertions(+), 18 deletions(-)

-- 
2.48.1.658.g4767266eb4-goog

[PATCH 1/6] scsi: ufs: exynos: ensure pre_link() executes before exynos_ufs_phy_init()

[Peter Griffin]

Ensure clocks are enabled before configuring unipro. Additionally move the
pre_link() hook before the exynos_ufs_phy_init() calls. This means the
register write sequence  more closely resembles the ordering of the
downstream driver.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/ufs/host/ufs-exynos.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 13dd5dfc03eb..cd750786187c 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1049,9 +1049,14 @@ static int exynos_ufs_pre_link(struct ufs_hba *hba)
 	exynos_ufs_config_intr(ufs, DFES_DEF_L4_ERRS, UNIPRO_L4);
 	exynos_ufs_set_unipro_pclk_div(ufs);
 
+	exynos_ufs_setup_clocks(hba, true, PRE_CHANGE);
+
 	/* unipro */
 	exynos_ufs_config_unipro(ufs);
 
+	if (ufs->drv_data->pre_link)
+		ufs->drv_data->pre_link(ufs);
+
 	/* m-phy */
 	exynos_ufs_phy_init(ufs);
 	if (!(ufs->opts & EXYNOS_UFS_OPT_SKIP_CONFIG_PHY_ATTR)) {
@@ -1059,11 +1064,6 @@ static int exynos_ufs_pre_link(struct ufs_hba *hba)
 		exynos_ufs_config_phy_cap_attr(ufs);
 	}
 
-	exynos_ufs_setup_clocks(hba, true, PRE_CHANGE);
-
-	if (ufs->drv_data->pre_link)
-		ufs->drv_data->pre_link(ufs);
-
 	return 0;
 }
 
-- 
2.48.1.658.g4767266eb4-goog

[PATCH 2/6] scsi: ufs: exynos: move ufs shareability value to drvdata

[Peter Griffin]

gs101 IO coherency shareability bits differ from exynosauto SoC. To
support both SoCs move this info the SoC drvdata.

Currently both the value and mask are the same for both gs101 and
exynosauto, thus we use the same value.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Fixes: d11e0a318df8 ("scsi: ufs: exynos: Add support for Tensor gs101 SoC")
Cc: stable@vger.kernel.org
---
 drivers/ufs/host/ufs-exynos.c | 20 ++++++++++++++------
 drivers/ufs/host/ufs-exynos.h |  2 ++
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index cd750786187c..a00256ede659 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -92,11 +92,16 @@
 				 UIC_TRANSPORT_NO_CONNECTION_RX |\
 				 UIC_TRANSPORT_BAD_TC)
 
-/* FSYS UFS Shareability */
-#define UFS_WR_SHARABLE		BIT(2)
-#define UFS_RD_SHARABLE		BIT(1)
-#define UFS_SHARABLE		(UFS_WR_SHARABLE | UFS_RD_SHARABLE)
-#define UFS_SHAREABILITY_OFFSET	0x710
+/* UFS Shareability */
+#define UFS_EXYNOSAUTO_WR_SHARABLE	BIT(2)
+#define UFS_EXYNOSAUTO_RD_SHARABLE	BIT(1)
+#define UFS_EXYNOSAUTO_SHARABLE		(UFS_EXYNOSAUTO_WR_SHARABLE | \
+					 UFS_EXYNOSAUTO_RD_SHARABLE)
+#define UFS_GS101_WR_SHARABLE		BIT(1)
+#define UFS_GS101_RD_SHARABLE		BIT(0)
+#define UFS_GS101_SHARABLE		(UFS_GS101_WR_SHARABLE | \
+					 UFS_GS101_RD_SHARABLE)
+#define UFS_SHAREABILITY_OFFSET		0x710
 
 /* Multi-host registers */
 #define MHCTRL			0xC4
@@ -210,7 +215,7 @@ static int exynos_ufs_shareability(struct exynos_ufs *ufs)
 	if (ufs->sysreg) {
 		return regmap_update_bits(ufs->sysreg,
 					  ufs->shareability_reg_offset,
-					  UFS_SHARABLE, UFS_SHARABLE);
+					  ufs->shareability, ufs->shareability);
 	}
 
 	return 0;
@@ -1193,6 +1198,7 @@ static inline void exynos_ufs_priv_init(struct ufs_hba *hba,
 {
 	ufs->hba = hba;
 	ufs->opts = ufs->drv_data->opts;
+	ufs->shareability = ufs->drv_data->shareability;
 	ufs->rx_sel_idx = PA_MAXDATALANES;
 	if (ufs->opts & EXYNOS_UFS_OPT_BROKEN_RX_SEL_IDX)
 		ufs->rx_sel_idx = 0;
@@ -2034,6 +2040,7 @@ static const struct exynos_ufs_drv_data exynosauto_ufs_drvs = {
 	.opts			= EXYNOS_UFS_OPT_BROKEN_AUTO_CLK_CTRL |
 				  EXYNOS_UFS_OPT_SKIP_CONFIG_PHY_ATTR |
 				  EXYNOS_UFS_OPT_BROKEN_RX_SEL_IDX,
+	.shareability		= UFS_EXYNOSAUTO_SHARABLE,
 	.drv_init		= exynosauto_ufs_drv_init,
 	.post_hce_enable	= exynosauto_ufs_post_hce_enable,
 	.pre_link		= exynosauto_ufs_pre_link,
@@ -2135,6 +2142,7 @@ static const struct exynos_ufs_drv_data gs101_ufs_drvs = {
 	.opts			= EXYNOS_UFS_OPT_SKIP_CONFIG_PHY_ATTR |
 				  EXYNOS_UFS_OPT_UFSPR_SECURE |
 				  EXYNOS_UFS_OPT_TIMER_TICK_SELECT,
+	.shareability		= UFS_GS101_SHARABLE,
 	.drv_init		= gs101_ufs_drv_init,
 	.pre_link		= gs101_ufs_pre_link,
 	.post_link		= gs101_ufs_post_link,
diff --git a/drivers/ufs/host/ufs-exynos.h b/drivers/ufs/host/ufs-exynos.h
index 9670dc138d1e..78bd13dc2d70 100644
--- a/drivers/ufs/host/ufs-exynos.h
+++ b/drivers/ufs/host/ufs-exynos.h
@@ -181,6 +181,7 @@ struct exynos_ufs_drv_data {
 	struct exynos_ufs_uic_attr *uic_attr;
 	unsigned int quirks;
 	unsigned int opts;
+	u32 shareability;
 	/* SoC's specific operations */
 	int (*drv_init)(struct exynos_ufs *ufs);
 	int (*pre_link)(struct exynos_ufs *ufs);
@@ -231,6 +232,7 @@ struct exynos_ufs {
 	const struct exynos_ufs_drv_data *drv_data;
 	struct regmap *sysreg;
 	u32 shareability_reg_offset;
+	u32 shareability;
 
 	u32 opts;
 #define EXYNOS_UFS_OPT_HAS_APB_CLK_CTRL		BIT(0)
-- 
2.48.1.658.g4767266eb4-goog

[PATCH 3/6] scsi: ufs: exynos: ensure consistent phy reference counts

[Peter Griffin]

ufshcd_link_startup() can call ufshcd_vops_link_startup_notify()
multiple times when retrying. This causes the phy reference count
to keep increasing and the phy to not properly re-initialize.

If the phy has already been previously powered on, first issue a
phy_power_off() and phy_exit(), before re-initializing and powering
on again.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/ufs/host/ufs-exynos.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index a00256ede659..943cea569f66 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -962,6 +962,12 @@ static int exynos_ufs_phy_init(struct exynos_ufs *ufs)
 	}
 
 	phy_set_bus_width(generic_phy, ufs->avail_ln_rx);
+
+	if (generic_phy->power_count) {
+		phy_power_off(generic_phy);
+		phy_exit(generic_phy);
+	}
+
 	ret = phy_init(generic_phy);
 	if (ret) {
 		dev_err(hba->dev, "%s: phy init failed, ret = %d\n",
-- 
2.48.1.658.g4767266eb4-goog

[PATCH 4/6] scsi: ufs: exynos: Enable PRDT pre-fetching with UFSHCD_CAP_CRYPTO

[Peter Griffin]

PRDT_PREFETCH_ENABLE[31] bit should be set when desctype field of
fmpsecurity0 register is type2 (double file encryption) or type3
(file and disk excryption). Setting this bit enables PRDT
pre-fetching on both TXPRDT and RXPRDT.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/ufs/host/ufs-exynos.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 943cea569f66..27eb360458a7 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1098,12 +1098,17 @@ static int exynos_ufs_post_link(struct ufs_hba *hba)
 	struct exynos_ufs *ufs = ufshcd_get_variant(hba);
 	struct phy *generic_phy = ufs->phy;
 	struct exynos_ufs_uic_attr *attr = ufs->drv_data->uic_attr;
+	u32 val = ilog2(DATA_UNIT_SIZE);
 
 	exynos_ufs_establish_connt(ufs);
 	exynos_ufs_fit_aggr_timeout(ufs);
 
 	hci_writel(ufs, 0xa, HCI_DATA_REORDER);
-	hci_writel(ufs, ilog2(DATA_UNIT_SIZE), HCI_TXPRDT_ENTRY_SIZE);
+
+	if (hba->caps & UFSHCD_CAP_CRYPTO)
+		val |= PRDT_PREFECT_EN;
+	hci_writel(ufs, val, HCI_TXPRDT_ENTRY_SIZE);
+
 	hci_writel(ufs, ilog2(DATA_UNIT_SIZE), HCI_RXPRDT_ENTRY_SIZE);
 	hci_writel(ufs, (1 << hba->nutrs) - 1, HCI_UTRL_NEXUS_TYPE);
 	hci_writel(ufs, (1 << hba->nutmrs) - 1, HCI_UTMRL_NEXUS_TYPE);
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH 4/6] scsi: ufs: exynos: Enable PRDT pre-fetching with UFSHCD_CAP_CRYPTO

[Bart Van Assche]

On 2/26/25 2:04 PM, Peter Griffin wrote:
> -	hci_writel(ufs, ilog2(DATA_UNIT_SIZE), HCI_TXPRDT_ENTRY_SIZE);
> +
> +	if (hba->caps & UFSHCD_CAP_CRYPTO)
> +		val |= PRDT_PREFECT_EN;

In a future patch series, please consider renaming PRDT_PREFECT_EN into
PRDT_PREFECTH_EN.

Thanks,

Bart.

Re: [PATCH 4/6] scsi: ufs: exynos: Enable PRDT pre-fetching with UFSHCD_CAP_CRYPTO

[Peter Griffin]

Hi Bart,

Thanks for the review feedback.

On Fri, 28 Feb 2025 at 19:18, Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2/26/25 2:04 PM, Peter Griffin wrote:
> > -     hci_writel(ufs, ilog2(DATA_UNIT_SIZE), HCI_TXPRDT_ENTRY_SIZE);
> > +
> > +     if (hba->caps & UFSHCD_CAP_CRYPTO)
> > +             val |= PRDT_PREFECT_EN;
>
> In a future patch series, please consider renaming PRDT_PREFECT_EN into
> PRDT_PREFECTH_EN.

I was just checking the datasheet naming (it is listed as
PRDT_PREFETCH_ENABLE). As well as the typo in my patch I think your
reply also has a typo :) I'm assuming you would like it renamed to
PRDT_PREFETCH_EN?

Thanks,

Peter

Re: [PATCH 4/6] scsi: ufs: exynos: Enable PRDT pre-fetching with UFSHCD_CAP_CRYPTO

[Eric Biggers]

On Wed, Feb 26, 2025 at 10:04:12PM +0000, Peter Griffin wrote:
> PRDT_PREFETCH_ENABLE[31] bit should be set when desctype field of
> fmpsecurity0 register is type2 (double file encryption) or type3
> (file and disk excryption). Setting this bit enables PRDT
> pre-fetching on both TXPRDT and RXPRDT.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>

I assume you mean that desctype 3 provides "support for file and disk
encryption"?  The driver does use desctype 3, but it only uses the "file
encryption".  So this confused me a bit.  (BTW, in FMP terminology, "file
encryption" seems to mean "use the key provided in the I/O request", and "disk
encryption" seems to mean "use some key the firmware provided somehow".  They
can be cascaded, and the intended use cases are clearly file and disk encryption
respectively, but they don't necessarily have to be used that way.)

- Eric

Re: [PATCH 4/6] scsi: ufs: exynos: Enable PRDT pre-fetching with UFSHCD_CAP_CRYPTO

[Peter Griffin]

Hi Eric,

Thanks for your review feedback.

On Wed, 5 Mar 2025 at 02:40, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Feb 26, 2025 at 10:04:12PM +0000, Peter Griffin wrote:
> > PRDT_PREFETCH_ENABLE[31] bit should be set when desctype field of
> > fmpsecurity0 register is type2 (double file encryption) or type3
> > (file and disk excryption). Setting this bit enables PRDT
> > pre-fetching on both TXPRDT and RXPRDT.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
>
> I assume you mean that desctype 3 provides "support for file and disk
> encryption"?

Yes, the PRDT_PREFETCH_ENABLE description in the commit message I
copied from the datasheet. But I can re-word it like you suggest if
you think that it's clearer? I notice now there is also a typo with
the word 'encryption' which I can also fix.

This patch came about whilst comparing UFS SFR register dumps of
upstream and downstream drivers. I noticed that PRDT_PREFETCH_ENABLE
is enabled downstream but not upstream, and after checking the
datasheet description it seemed like we should set this if
exynos_ufs_fmp_init() completed and set CFG_DESCTYPE_3.

> The driver does use desctype 3, but it only uses the "file
> encryption".  So this confused me a bit.  (BTW, in FMP terminology, "file
> encryption" seems to mean "use the key provided in the I/O request", and "disk
> encryption" seems to mean "use some key the firmware provided somehow".  They
> can be cascaded, and the intended use cases are clearly file and disk encryption
> respectively, but they don't necessarily have to be used that way.)

Thanks for the additional context :)

Peter

[PATCH 5/6] scsi: ufs: exynos: Move phy calls to .exit() callback

[Peter Griffin]

ufshcd_pltfrm_remove() calls ufshcd_remove(hba) which in turn calls
ufshcd_hba_exit().

By moving the phy_power_off() and phy_exit() calls to the newly created
.exit callback they get called by ufshcd_variant_hba_exit() before
ufshcd_hba_exit() turns off the regulators. This is also similar flow
to the ufs-qcom driver.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/ufs/host/ufs-exynos.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 27eb360458a7..4c3e03a3b8d9 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1513,6 +1513,15 @@ static int exynos_ufs_init(struct ufs_hba *hba)
 	return ret;
 }
 
+static void exynos_ufs_exit(struct ufs_hba *hba)
+{
+	struct exynos_ufs *ufs = ufshcd_get_variant(hba);
+
+	phy_power_off(ufs->phy);
+	phy_exit(ufs->phy);
+}
+
+
 static int exynos_ufs_host_reset(struct ufs_hba *hba)
 {
 	struct exynos_ufs *ufs = ufshcd_get_variant(hba);
@@ -1968,6 +1977,7 @@ static int gs101_ufs_pre_pwr_change(struct exynos_ufs *ufs,
 static const struct ufs_hba_variant_ops ufs_hba_exynos_ops = {
 	.name				= "exynos_ufs",
 	.init				= exynos_ufs_init,
+	.exit				= exynos_ufs_exit,
 	.hce_enable_notify		= exynos_ufs_hce_enable_notify,
 	.link_startup_notify		= exynos_ufs_link_startup_notify,
 	.pwr_change_notify		= exynos_ufs_pwr_change_notify,
@@ -2006,13 +2016,7 @@ static int exynos_ufs_probe(struct platform_device *pdev)
 
 static void exynos_ufs_remove(struct platform_device *pdev)
 {
-	struct ufs_hba *hba =  platform_get_drvdata(pdev);
-	struct exynos_ufs *ufs = ufshcd_get_variant(hba);
-
 	ufshcd_pltfrm_remove(pdev);
-
-	phy_power_off(ufs->phy);
-	phy_exit(ufs->phy);
 }
 
 static struct exynos_ufs_uic_attr exynos7_uic_attr = {
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH 5/6] scsi: ufs: exynos: Move phy calls to .exit() callback

[Bart Van Assche]

On 2/26/25 2:04 PM, Peter Griffin wrote:
> +static void exynos_ufs_exit(struct ufs_hba *hba)
> +{
> +	struct exynos_ufs *ufs = ufshcd_get_variant(hba);
> +
> +	phy_power_off(ufs->phy);
> +	phy_exit(ufs->phy);
> +}
> +
> +

For future patches, please follow the convention that is used elsewhere
in the kernel and separate functions with a single blank line instead of
two.


Thanks,

Bart.

Re: [PATCH 5/6] scsi: ufs: exynos: Move phy calls to .exit() callback

[Peter Griffin]

Hi Bart,

Thanks for your review feedback!

On Fri, 28 Feb 2025 at 19:20, Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2/26/25 2:04 PM, Peter Griffin wrote:
> > +static void exynos_ufs_exit(struct ufs_hba *hba)
> > +{
> > +     struct exynos_ufs *ufs = ufshcd_get_variant(hba);
> > +
> > +     phy_power_off(ufs->phy);
> > +     phy_exit(ufs->phy);
> > +}
> > +
> > +
>
> For future patches, please follow the convention that is used elsewhere
> in the kernel and separate functions with a single blank line instead of
> two.

That was an oversight on my part, will fix.

Thanks,

Peter

[PATCH 6/6] scsi: ufs: exynos: put ufs device in reset on .exit() and .suspend()

[Peter Griffin]

GPIO_OUT[0] is connected to the reset pin of embedded UFS device.
Before powering off the phy assert the reset signal.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 drivers/ufs/host/ufs-exynos.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ufs/host/ufs-exynos.c b/drivers/ufs/host/ufs-exynos.c
index 4c3e03a3b8d9..64e2bf924213 100644
--- a/drivers/ufs/host/ufs-exynos.c
+++ b/drivers/ufs/host/ufs-exynos.c
@@ -1517,6 +1517,7 @@ static void exynos_ufs_exit(struct ufs_hba *hba)
 {
 	struct exynos_ufs *ufs = ufshcd_get_variant(hba);
 
+	hci_writel(ufs, 0 << 0, HCI_GPIO_OUT);
 	phy_power_off(ufs->phy);
 	phy_exit(ufs->phy);
 }
@@ -1700,6 +1701,8 @@ static int exynos_ufs_suspend(struct ufs_hba *hba, enum ufs_pm_op pm_op,
 	if (status == PRE_CHANGE)
 		return 0;
 
+	hci_writel(ufs, 0 << 0, HCI_GPIO_OUT);
+
 	if (!ufshcd_is_link_active(hba))
 		phy_power_off(ufs->phy);
 
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH 6/6] scsi: ufs: exynos: put ufs device in reset on .exit() and .suspend()

[Bart Van Assche]

On 2/26/25 2:04 PM, Peter Griffin wrote:
> GPIO_OUT[0] is connected to the reset pin of embedded UFS device.
> Before powering off the phy assert the reset signal.
Does the above apply to the GS implementation only or does it apply to
all SoC's with an Exynos UFS host controller?

Thanks,

Bart.

Re: [PATCH 6/6] scsi: ufs: exynos: put ufs device in reset on .exit() and .suspend()

[Peter Griffin]

Hi Bart,

Thanks for the review feedback.

On Fri, 28 Feb 2025 at 19:21, Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2/26/25 2:04 PM, Peter Griffin wrote:
> > GPIO_OUT[0] is connected to the reset pin of embedded UFS device.
> > Before powering off the phy assert the reset signal.
> Does the above apply to the GS implementation only or does it apply to
> all SoC's with an Exynos UFS host controller?

The reason I went with a generic approach (rather than adding another
SoC specific hook) was that exynos_ufs_dev_hw_reset() is already
called by all users of this driver. From that I concluded it is a
common register shared by all exynos implementations.

It is hard to be 100% sure though as I don't personally have any of
the other Exynos platforms supported by this driver to test on.
Another approach would be to add some more gs101 SoC specific hooks
for suspend() and exit() to exynos_ufs_drv_data() or another
EXYNOS_UFS_OPT_.

Thanks,

Peter