[PATCH v5 0/2] Clean up UFSHC driver

[PATCH v5 0/2] Clean up UFSHC driver

[Marc Gonzalez]

Casting a wide net to get as many eyeballs on the subject as possible.

I'm splitting this mini series off from the main "UFS on APQ8098/MSM8998"
as it's 1) ufs/scsi specific 2) controversial (to my surprise)

Please send all your Reviewed-by: tags if you agree with the patch,
and/or voice your concerns ASAP.

This series removes the "disable-VCCQ-power-rail-for-some-Flash-chips" quirk,
and cleans up after the dust settles.

Marc Gonzalez (2):
  scsi: ufs: Do not disable vccq in UFSHC driver
  scsi: ufs: Remove unused device quirks

 drivers/scsi/ufs/ufs.h        |  1 -
 drivers/scsi/ufs/ufs_quirks.h | 29 ----------------
 drivers/scsi/ufs/ufshcd.c     | 63 +++--------------------------------
 3 files changed, 4 insertions(+), 89 deletions(-)

-- 
2.17.1

[PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Marc Gonzalez]

Commit 60f0187031c0 ("disable vccq if it's not needed by UFS device")
introduced a small power optimization as a driver quirk: ignore the
vccq load specified in the UFSHC DT node when said host controller
is connected to specific Flash chips (Samsung and Hynix currently).

Unfortunately, this optimization breaks UFS on systems where vccq
powers not only the Flash chip, but the host controller as well,
such as APQ8098 MEDIABOX or MTP8998:

[    3.929877] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04 for idn 13 failed, index 0, err = -11
[    5.433815] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04 for idn 13 failed, index 0, err = -11
[    6.937771] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04 for idn 13 failed, index 0, err = -11
[    6.937866] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr_retry: query attribute, idn 13, failed with error -11 after 3 retires
[    6.946412] ufshcd-qcom 1da4000.ufshc: ufshcd_disable_auto_bkops: failed to enable exception event -11
[    6.957972] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1587 failed 3 retries
[    6.967181] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1586 failed 3 retries
[    6.975025] ufshcd-qcom 1da4000.ufshc: ufshcd_get_max_pwr_mode: invalid max pwm tx gear read = 0
[    6.982755] ufshcd-qcom 1da4000.ufshc: ufshcd_probe_hba: Failed getting max supported power mode
[    8.505770] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag query for idn 3 failed, err = -11
[   10.009807] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag query for idn 3 failed, err = -11
[   11.513766] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag query for idn 3 failed, err = -11
[   11.513861] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag_retry: query attribute, opcode 5, idn 3, failed with error -11 after 3 retires
[   13.049807] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor: opcode 0x01 for idn 8 failed, index 0, err = -11
[   14.553768] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor: opcode 0x01 for idn 8 failed, index 0, err = -11
[   16.057767] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor: opcode 0x01 for idn 8 failed, index 0, err = -11
[   16.057872] ufshcd-qcom 1da4000.ufshc: ufshcd_read_desc_param: Failed reading descriptor. desc_id 8, desc_index 0, param_offset 0, ret -11
[   16.067109] ufshcd-qcom 1da4000.ufshc: ufshcd_init_icc_levels: Failed reading power descriptor.len = 98 ret = -11
[   37.073787] ufshcd-qcom 1da4000.ufshc: link startup failed 1

In my opinion, the rationale for the original patch is questionable.
If neither the UFSHC, nor the Flash chip, require any load from vccq,
then that power rail should simply not be specified at all in the DT.

Working around that fact in the driver is detrimental, as evidenced
by the failure to initialize the host controller on MSM8998.

Revert the original patch, and clean up loose ends in the next patch.

Relevant patches:

60f0187031c05e04cbadffb62f557d0ff3564490
c58ab7aab71e2c783087115f0ce1623c2fdcf0b2
46c1cf706076500cdcde3445be97233793eec7f1

Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
---
 drivers/scsi/ufs/ufs.h    |  1 -
 drivers/scsi/ufs/ufshcd.c | 59 +++------------------------------------
 2 files changed, 4 insertions(+), 56 deletions(-)

diff --git a/drivers/scsi/ufs/ufs.h b/drivers/scsi/ufs/ufs.h
index 6d176815e6ce..21e4ccb5ba6e 100644
--- a/drivers/scsi/ufs/ufs.h
+++ b/drivers/scsi/ufs/ufs.h
@@ -514,7 +514,6 @@ struct ufs_vreg {
 	struct regulator *reg;
 	const char *name;
 	bool enabled;
-	bool unused;
 	int min_uV;
 	int max_uV;
 	int min_uA;
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 5ca4581e60d5..a5bfcf04fdba 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -251,7 +251,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba);
 static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
 				 bool skip_ref_clk);
 static int ufshcd_setup_clocks(struct ufs_hba *hba, bool on);
-static int ufshcd_set_vccq_rail_unused(struct ufs_hba *hba, bool unused);
 static int ufshcd_uic_hibern8_exit(struct ufs_hba *hba);
 static int ufshcd_uic_hibern8_enter(struct ufs_hba *hba);
 static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba);
@@ -6830,11 +6829,6 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
 	ufs_fixup_device_setup(hba, &card);
 	ufshcd_tune_unipro_params(hba);
 
-	ret = ufshcd_set_vccq_rail_unused(hba,
-		(hba->dev_quirks & UFS_DEVICE_NO_VCCQ) ? true : false);
-	if (ret)
-		goto out;
-
 	/* UFS device is also active now */
 	ufshcd_set_ufs_dev_active(hba);
 	ufshcd_force_reset_auto_bkops(hba);
@@ -7018,24 +7012,13 @@ static int ufshcd_config_vreg_load(struct device *dev, struct ufs_vreg *vreg,
 static inline int ufshcd_config_vreg_lpm(struct ufs_hba *hba,
 					 struct ufs_vreg *vreg)
 {
-	if (!vreg)
-		return 0;
-	else if (vreg->unused)
-		return 0;
-	else
-		return ufshcd_config_vreg_load(hba->dev, vreg,
-					       UFS_VREG_LPM_LOAD_UA);
+	return ufshcd_config_vreg_load(hba->dev, vreg, UFS_VREG_LPM_LOAD_UA);
 }
 
 static inline int ufshcd_config_vreg_hpm(struct ufs_hba *hba,
 					 struct ufs_vreg *vreg)
 {
-	if (!vreg)
-		return 0;
-	else if (vreg->unused)
-		return 0;
-	else
-		return ufshcd_config_vreg_load(hba->dev, vreg, vreg->max_uA);
+	return ufshcd_config_vreg_load(hba->dev, vreg, vreg->max_uA);
 }
 
 static int ufshcd_config_vreg(struct device *dev,
@@ -7073,9 +7056,7 @@ static int ufshcd_enable_vreg(struct device *dev, struct ufs_vreg *vreg)
 {
 	int ret = 0;
 
-	if (!vreg)
-		goto out;
-	else if (vreg->enabled || vreg->unused)
+	if (!vreg || vreg->enabled)
 		goto out;
 
 	ret = ufshcd_config_vreg(dev, vreg, true);
@@ -7095,9 +7076,7 @@ static int ufshcd_disable_vreg(struct device *dev, struct ufs_vreg *vreg)
 {
 	int ret = 0;
 
-	if (!vreg)
-		goto out;
-	else if (!vreg->enabled || vreg->unused)
+	if (!vreg || !vreg->enabled)
 		goto out;
 
 	ret = regulator_disable(vreg->reg);
@@ -7203,36 +7182,6 @@ static int ufshcd_init_hba_vreg(struct ufs_hba *hba)
 	return 0;
 }
 
-static int ufshcd_set_vccq_rail_unused(struct ufs_hba *hba, bool unused)
-{
-	int ret = 0;
-	struct ufs_vreg_info *info = &hba->vreg_info;
-
-	if (!info)
-		goto out;
-	else if (!info->vccq)
-		goto out;
-
-	if (unused) {
-		/* shut off the rail here */
-		ret = ufshcd_toggle_vreg(hba->dev, info->vccq, false);
-		/*
-		 * Mark this rail as no longer used, so it doesn't get enabled
-		 * later by mistake
-		 */
-		if (!ret)
-			info->vccq->unused = true;
-	} else {
-		/*
-		 * rail should have been already enabled hence just make sure
-		 * that unused flag is cleared.
-		 */
-		info->vccq->unused = false;
-	}
-out:
-	return ret;
-}
-
 static int __ufshcd_setup_clocks(struct ufs_hba *hba, bool on,
 					bool skip_ref_clk)
 {
-- 
2.17.1

[PATCH v5 2/2] scsi: ufs: Remove unused device quirks

[Marc Gonzalez]

The UFSHC driver defines a few quirks that are not used anywhere:

UFS_DEVICE_QUIRK_BROKEN_LCC
UFS_DEVICE_NO_VCCQ
UFS_DEVICE_QUIRK_NO_LINK_OFF
UFS_DEVICE_NO_FASTAUTO

Let's remove them.

Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
---
 drivers/scsi/ufs/ufs_quirks.h | 29 -----------------------------
 drivers/scsi/ufs/ufshcd.c     |  4 ----
 2 files changed, 33 deletions(-)

diff --git a/drivers/scsi/ufs/ufs_quirks.h b/drivers/scsi/ufs/ufs_quirks.h
index 5d2dfdb41a6f..a9bbd34d6b16 100644
--- a/drivers/scsi/ufs/ufs_quirks.h
+++ b/drivers/scsi/ufs/ufs_quirks.h
@@ -44,21 +44,6 @@ struct ufs_dev_fix {
 	.quirk = (_quirk),		   \
 }
 
-/*
- * If UFS device is having issue in processing LCC (Line Control
- * Command) coming from UFS host controller then enable this quirk.
- * When this quirk is enabled, host controller driver should disable
- * the LCC transmission on UFS host controller (by clearing
- * TX_LCC_ENABLE attribute of host to 0).
- */
-#define UFS_DEVICE_QUIRK_BROKEN_LCC (1 << 0)
-
-/*
- * Some UFS devices don't need VCCQ rail for device operations. Enabling this
- * quirk for such devices will make sure that VCCQ rail is not voted.
- */
-#define UFS_DEVICE_NO_VCCQ (1 << 1)
-
 /*
  * Some vendor's UFS device sends back to back NACs for the DL data frames
  * causing the host controller to raise the DFES error status. Sometimes
@@ -84,13 +69,6 @@ struct ufs_dev_fix {
  */
 #define UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS (1 << 2)
 
-/*
- * Some UFS devices may not work properly after resume if the link was kept
- * in off state during suspend. Enabling this quirk will not allow the
- * link to be kept in off state during suspend.
- */
-#define UFS_DEVICE_QUIRK_NO_LINK_OFF	(1 << 3)
-
 /*
  * Few Toshiba UFS device models advertise RX_MIN_ACTIVATETIME_CAPABILITY as
  * 600us which may not be enough for reliable hibern8 exit hardware sequence
@@ -100,13 +78,6 @@ struct ufs_dev_fix {
  */
 #define UFS_DEVICE_QUIRK_PA_TACTIVATE	(1 << 4)
 
-/*
- * Some UFS memory devices may have really low read/write throughput in
- * FAST AUTO mode, enable this quirk to make sure that FAST AUTO mode is
- * never enabled for such devices.
- */
-#define UFS_DEVICE_NO_FASTAUTO		(1 << 5)
-
 /*
  * It seems some UFS devices may keep drawing more than sleep current
  * (atleast for 500us) from UFS rails (especially from VCCQ rail).
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index a5bfcf04fdba..54ed1f8db1bf 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -219,11 +219,8 @@ static struct ufs_dev_fix ufs_fixups[] = {
 	/* UFS cards deviations table */
 	UFS_FIX(UFS_VENDOR_SAMSUNG, UFS_ANY_MODEL,
 		UFS_DEVICE_QUIRK_DELAY_BEFORE_LPM),
-	UFS_FIX(UFS_VENDOR_SAMSUNG, UFS_ANY_MODEL, UFS_DEVICE_NO_VCCQ),
 	UFS_FIX(UFS_VENDOR_SAMSUNG, UFS_ANY_MODEL,
 		UFS_DEVICE_QUIRK_RECOVERY_FROM_DL_NAC_ERRORS),
-	UFS_FIX(UFS_VENDOR_SAMSUNG, UFS_ANY_MODEL,
-		UFS_DEVICE_NO_FASTAUTO),
 	UFS_FIX(UFS_VENDOR_SAMSUNG, UFS_ANY_MODEL,
 		UFS_DEVICE_QUIRK_HOST_PA_TACTIVATE),
 	UFS_FIX(UFS_VENDOR_TOSHIBA, UFS_ANY_MODEL,
@@ -232,7 +229,6 @@ static struct ufs_dev_fix ufs_fixups[] = {
 		UFS_DEVICE_QUIRK_PA_TACTIVATE),
 	UFS_FIX(UFS_VENDOR_TOSHIBA, "THGLF2G9D8KBADG",
 		UFS_DEVICE_QUIRK_PA_TACTIVATE),
-	UFS_FIX(UFS_VENDOR_SKHYNIX, UFS_ANY_MODEL, UFS_DEVICE_NO_VCCQ),
 	UFS_FIX(UFS_VENDOR_SKHYNIX, UFS_ANY_MODEL,
 		UFS_DEVICE_QUIRK_HOST_PA_SAVECONFIGTIME),
 	UFS_FIX(UFS_VENDOR_SKHYNIX, "hB8aL1" /*H28U62301AMR*/,
-- 
2.17.1

Re: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Mon, Feb 11, 2019 at 02:32:15PM +0100, Marc Gonzalez wrote:

> Unfortunately, this optimization breaks UFS on systems where vccq
> powers not only the Flash chip, but the host controller as well,
> such as APQ8098 MEDIABOX or MTP8998:

...

> In my opinion, the rationale for the original patch is questionable.
> If neither the UFSHC, nor the Flash chip, require any load from vccq,
> then that power rail should simply not be specified at all in the DT.

If the supply is physically connected it should be valid to represent
this in DT regardless of how or if the supply gets used at runtime.
However it does sound like this support needs to be better thought
through to make sure we have represented the supplies to the flash chip
and the controller separately - it seems like right now there's no
tracking of the supplies needed for the controller and the assumption is
that only the flash chip needs managing which is breaking things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Marc Gonzalez]

On 11/02/2019 18:23, Mark Brown wrote:

> On Mon, Feb 11, 2019 at 02:32:15PM +0100, Marc Gonzalez wrote:
> 
>> Unfortunately, this optimization breaks UFS on systems where vccq
>> powers not only the Flash chip, but the host controller as well,
>> such as APQ8098 MEDIABOX or MTP8998.
>> 
>> In my opinion, the rationale for the original patch is questionable.
>> If neither the UFSHC, nor the Flash chip, require any load from vccq,
>> then that power rail should simply not be specified at all in the DT.
> 
> If the supply is physically connected it should be valid to represent
> this in DT regardless of how or if the supply gets used at runtime.
> However it does sound like this support needs to be better thought
> through to make sure we have represented the supplies to the flash chip
> and the controller separately - it seems like right now there's no
> tracking of the supplies needed for the controller and the assumption is
> that only the flash chip needs managing which is breaking things.

As far as I'm aware, the conflation of host controller with their respective
storage medium occurs across several techs: UFS, NAND, SDHC, EMMC.

There might be room for improvement, but I don't think these considerations
should hold up this series, which fixes something that is broken *today*.

UFS reviewers (Alim, Avri, Pedro), can I get at least one Acked-by to remove
this small power optimization that breaks UFS on my system?

Regards.

Re: [PATCH v5 0/2] Clean up UFSHC driver

[Alim Akhtar]

Hi Marc,

On 11/02/19 7:01 PM, Marc Gonzalez wrote:
> Casting a wide net to get as many eyeballs on the subject as possible.
> 
> I'm splitting this mini series off from the main "UFS on APQ8098/MSM8998"
> as it's 1) ufs/scsi specific 2) controversial (to my surprise)
> 
> Please send all your Reviewed-by: tags if you agree with the patch,
> and/or voice your concerns ASAP.
> 
> This series removes the "disable-VCCQ-power-rail-for-some-Flash-chips" quirk,
> and cleans up after the dust settles.
> 
> Marc Gonzalez (2):
>    scsi: ufs: Do not disable vccq in UFSHC driver
>    scsi: ufs: Remove unused device quirks
> 
>   drivers/scsi/ufs/ufs.h        |  1 -
>   drivers/scsi/ufs/ufs_quirks.h | 29 ----------------
>   drivers/scsi/ufs/ufshcd.c     | 63 +++--------------------------------
>   3 files changed, 4 insertions(+), 89 deletions(-)
> 
For this series
Reviewed-by: Alim Akhtar <alim.akhtar@samsung.com>
As no other platform has claimed issues with this we can land this.
Others let us know if your opinion differs here.

Acked-by: Alim Akhtar <alim.akhtar@samsung.com>

Thanks!

Re: [PATCH v5 0/2] Clean up UFSHC driver

[Marc Gonzalez]

Martin,

Could you send this series to -next so that it gets some exposure
and testing during the 5.1 RC cycle?

Regards.

On 11/02/2019 14:31, Marc Gonzalez wrote:

> Casting a wide net to get as many eyeballs on the subject as possible.
> 
> I'm splitting this mini series off from the main "UFS on APQ8098/MSM8998"
> as it's 1) ufs/scsi specific 2) controversial (to my surprise)
> 
> Please send all your Reviewed-by: tags if you agree with the patch,
> and/or voice your concerns ASAP.
> 
> This series removes the "disable-VCCQ-power-rail-for-some-Flash-chips" quirk,
> and cleans up after the dust settles.
> 
> Marc Gonzalez (2):
>   scsi: ufs: Do not disable vccq in UFSHC driver
>   scsi: ufs: Remove unused device quirks
> 
>  drivers/scsi/ufs/ufs.h        |  1 -
>  drivers/scsi/ufs/ufs_quirks.h | 29 ----------------
>  drivers/scsi/ufs/ufshcd.c     | 63 +++--------------------------------
>  3 files changed, 4 insertions(+), 89 deletions(-)

Re: [PATCH v5 0/2] Clean up UFSHC driver

[Lee Jones]

On Sat, 23 Feb 2019, Marc Gonzalez wrote:

> Martin,
> 
> Could you send this series to -next so that it gets some exposure
> and testing during the 5.1 RC cycle?

Replying for the purpose of providing some gravitas to this set.

These patches are required to support some exciting consumer hardware
which has been recently released [0][1][2][3][4].  If they do not
find their way into Mainline very soon, it would mean that supporting
these devices in the most popular Linux distos will be difficult to
impossible.

Please consider them for the v5.1 merge window which opens in a week.

[0] https://joindiaspora.com/posts/4714199011120137995e0218b7b935a5
[1] https://liliputing.com/2019/02/now-you-can-run-linux-on-some-arm-laptops-designed-for-windows-10-on-arm.html
[2] https://www.phoronix.com/scan.php?page=news_item&px=Linux-On-The-Win-Arm-Laptops
[3] https://fossbytes.com/linux-on-windows-10-arm-laptops-project/
[4] https://www.techrepublic.com/article/open-source-project-aims-to-make-ubuntu-usable-on-arm-powered-windows-laptops/

> On 11/02/2019 14:31, Marc Gonzalez wrote:
> 
> > Casting a wide net to get as many eyeballs on the subject as possible.
> > 
> > I'm splitting this mini series off from the main "UFS on APQ8098/MSM8998"
> > as it's 1) ufs/scsi specific 2) controversial (to my surprise)
> > 
> > Please send all your Reviewed-by: tags if you agree with the patch,
> > and/or voice your concerns ASAP.
> > 
> > This series removes the "disable-VCCQ-power-rail-for-some-Flash-chips" quirk,
> > and cleans up after the dust settles.
> > 
> > Marc Gonzalez (2):
> >   scsi: ufs: Do not disable vccq in UFSHC driver
> >   scsi: ufs: Remove unused device quirks
> > 
> >  drivers/scsi/ufs/ufs.h        |  1 -
> >  drivers/scsi/ufs/ufs_quirks.h | 29 ----------------
> >  drivers/scsi/ufs/ufshcd.c     | 63 +++--------------------------------
> >  3 files changed, 4 insertions(+), 89 deletions(-)

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

RE: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Avri Altman]

> 
> Commit 60f0187031c0 ("disable vccq if it's not needed by UFS device")
> introduced a small power optimization as a driver quirk: ignore the
> vccq load specified in the UFSHC DT node when said host controller
> is connected to specific Flash chips (Samsung and Hynix currently).
> 
> Unfortunately, this optimization breaks UFS on systems where vccq
> powers not only the Flash chip, but the host controller as well,
> such as APQ8098 MEDIABOX or MTP8998:
> 
> [    3.929877] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04
> for idn 13 failed, index 0, err = -11
> [    5.433815] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04
> for idn 13 failed, index 0, err = -11
> [    6.937771] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr: opcode 0x04
> for idn 13 failed, index 0, err = -11
> [    6.937866] ufshcd-qcom 1da4000.ufshc: ufshcd_query_attr_retry: query
> attribute, idn 13, failed with error -11 after 3 retires
> [    6.946412] ufshcd-qcom 1da4000.ufshc: ufshcd_disable_auto_bkops:
> failed to enable exception event -11
> [    6.957972] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1587
> failed 3 retries
> [    6.967181] ufshcd-qcom 1da4000.ufshc: dme-peer-get: attr-id 0x1586
> failed 3 retries
> [    6.975025] ufshcd-qcom 1da4000.ufshc: ufshcd_get_max_pwr_mode:
> invalid max pwm tx gear read = 0
> [    6.982755] ufshcd-qcom 1da4000.ufshc: ufshcd_probe_hba: Failed getting
> max supported power mode
> [    8.505770] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag
> query for idn 3 failed, err = -11
> [   10.009807] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag
> query for idn 3 failed, err = -11
> [   11.513766] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag: Sending flag
> query for idn 3 failed, err = -11
> [   11.513861] ufshcd-qcom 1da4000.ufshc: ufshcd_query_flag_retry: query
> attribute, opcode 5, idn 3, failed with error -11 after 3 retires
> [   13.049807] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor:
> opcode 0x01 for idn 8 failed, index 0, err = -11
> [   14.553768] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor:
> opcode 0x01 for idn 8 failed, index 0, err = -11
> [   16.057767] ufshcd-qcom 1da4000.ufshc: __ufshcd_query_descriptor:
> opcode 0x01 for idn 8 failed, index 0, err = -11
> [   16.057872] ufshcd-qcom 1da4000.ufshc: ufshcd_read_desc_param: Failed
> reading descriptor. desc_id 8, desc_index 0, param_offset 0, ret -11
> [   16.067109] ufshcd-qcom 1da4000.ufshc: ufshcd_init_icc_levels: Failed
> reading power descriptor.len = 98 ret = -11
> [   37.073787] ufshcd-qcom 1da4000.ufshc: link startup failed 1
> 
> In my opinion, the rationale for the original patch is questionable.
> If neither the UFSHC, nor the Flash chip, require any load from vccq,
> then that power rail should simply not be specified at all in the DT.
> 
> Working around that fact in the driver is detrimental, as evidenced
> by the failure to initialize the host controller on MSM8998.
> 
> Revert the original patch, and clean up loose ends in the next patch.
> 
> Relevant patches:
> 
> 60f0187031c05e04cbadffb62f557d0ff3564490
> c58ab7aab71e2c783087115f0ce1623c2fdcf0b2
> 46c1cf706076500cdcde3445be97233793eec7f1
> 
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
Acked-by: Avri Altman <avri.altman@wdc.com> 

RE: [PATCH v5 2/2] scsi: ufs: Remove unused device quirks

[Avri Altman]

> 
> The UFSHC driver defines a few quirks that are not used anywhere:
> 
> UFS_DEVICE_QUIRK_BROKEN_LCC
> UFS_DEVICE_NO_VCCQ
> UFS_DEVICE_QUIRK_NO_LINK_OFF
> UFS_DEVICE_NO_FASTAUTO
> 
> Let's remove them.
> 
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
Acked-by: Avri Altman <avri.altman@wdc.com>

Re: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Martin K. Petersen]

Hi Marc,

> Commit 60f0187031c0 ("disable vccq if it's not needed by UFS device")
> introduced a small power optimization as a driver quirk: ignore the
> vccq load specified in the UFSHC DT node when said host controller
> is connected to specific Flash chips (Samsung and Hynix currently).

[...]

> Revert the original patch, and clean up loose ends in the next patch.

This commit isn't a revert. Why not?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Marc Gonzalez]

On 26/02/2019 15:44, Martin K. Petersen wrote:

> Hi Marc,
> 
>> Commit 60f0187031c0 ("disable vccq if it's not needed by UFS device")
>> introduced a small power optimization as a driver quirk: ignore the
>> vccq load specified in the UFSHC DT node when said host controller
>> is connected to specific Flash chips (Samsung and Hynix currently).
> 
> [...]
> 
>> Revert the original patch, and clean up loose ends in the next patch.
> 
> This commit isn't a revert. Why not?

What do you mean?

Re: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Martin K. Petersen]

Marc,

>>> Revert the original patch, and clean up loose ends in the next patch.
>> 
>> This commit isn't a revert. Why not?
>
> What do you mean?

Your commit states it reverts the original patch but the submission is
not a git revert. If there are reasons why simply reverting the original
commit didn't work, I'd like to see them documented in the commit
message.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Marc Gonzalez]

[ Drop codeaurora.org devs ]

On 26/02/2019 15:52, Martin K. Petersen wrote:

>>>> Revert the original patch, and clean up loose ends in the next patch.
>>>
>>> This commit isn't a revert. Why not?
>>
>> What do you mean?
> 
> Your commit states it reverts the original patch but the submission is
> not a git revert. If there are reasons why simply reverting the original
> commit didn't work, I'd like to see them documented in the commit
> message.

Martin,

I indeed started off from 'git revert'

$ git revert 60f0187031c0
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at least 18258 and retry the command.
error: could not revert 60f0187031c0... scsi: ufs: disable vccq if it's not needed by UFS device
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

So I had to resolve the conflict in ufshcd_probe_hba()

The line:

	ufs_advertise_fixup_device(hba);

was modified by commit 93fdd5ac64bbe80dac6416f048405362d7ef0945


It's not clear to me if you want me to

1) document that there was a conflict
2) change the title of the patch
3) both
4) something else altogether

Regards.

Re: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Martin K. Petersen]

Hi Marc,

> I indeed started off from 'git revert'
>
> $ git revert 60f0187031c0
> warning: inexact rename detection was skipped due to too many files.
> warning: you may want to set your merge.renamelimit variable to at
> least 18258 and retry the command.
> error: could not revert 60f0187031c0... scsi: ufs: disable vccq if
> it's not needed by UFS device
> hint: after resolving the conflicts, mark the corrected paths
> hint: with 'git add <paths>' or 'git rm <paths>'
> hint: and commit the result with 'git commit'
>
> So I had to resolve the conflict in ufshcd_probe_hba()
>
> The line:
>
> 	ufs_advertise_fixup_device(hba);
>
> was modified by commit 93fdd5ac64bbe80dac6416f048405362d7ef0945

If it's a resolvable delta, a proper git revert is preferred. Please
document any conflicts in the commit message and list the relevant
commits that introduced them.

If you find yourself in a situation where reverting simply isn't
feasible, I'd expect the commit to state "This should have been a revert
but I'd have to boil the oceans to resolve the conflicts because XYZ..."

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Marc Gonzalez]

On 26/02/2019 17:26, Martin K. Petersen wrote:

>> I indeed started off from 'git revert'
>>
>> $ git revert 60f0187031c0
>> warning: inexact rename detection was skipped due to too many files.
>> warning: you may want to set your merge.renamelimit variable to at
>> least 18258 and retry the command.
>> error: could not revert 60f0187031c0... scsi: ufs: disable vccq if
>> it's not needed by UFS device
>> hint: after resolving the conflicts, mark the corrected paths
>> hint: with 'git add <paths>' or 'git rm <paths>'
>> hint: and commit the result with 'git commit'
>>
>> So I had to resolve the conflict in ufshcd_probe_hba()
>>
>> The line:
>>
>> 	ufs_advertise_fixup_device(hba);
>>
>> was modified by commit 93fdd5ac64bbe80dac6416f048405362d7ef0945
> 
> If it's a resolvable delta, a proper git revert is preferred. Please
> document any conflicts in the commit message and list the relevant
> commits that introduced them.
> 
> If you find yourself in a situation where reverting simply isn't
> feasible, I'd expect the commit to state "This should have been a revert
> but I'd have to boil the oceans to resolve the conflicts because XYZ..."

OK, I'll do my best, but I've never been in this situation before.

According to git blame, ufshcd_probe_hba() has been heavily modified
around the ufshcd_set_vccq_rail_unused() call to be reverted:

a4b0e8a4e92b1 (Potomski, MichalX     2017-02-23 09:05:30 +0000 6820)    /* Init check for device descriptor sizes */
a4b0e8a4e92b1 (Potomski, MichalX     2017-02-23 09:05:30 +0000 6821)    ufshcd_init_desc_sizes(hba);
a4b0e8a4e92b1 (Potomski, MichalX     2017-02-23 09:05:30 +0000 6822) 
93fdd5ac64bbe (Tomas Winkler         2017-01-05 10:45:12 +0200 6823)    ret = ufs_get_device_desc(hba, &card);
93fdd5ac64bbe (Tomas Winkler         2017-01-05 10:45:12 +0200 6824)    if (ret) {
93fdd5ac64bbe (Tomas Winkler         2017-01-05 10:45:12 +0200 6825)            dev_err(hba->dev, "%s: Failed getting device info. err = %d\n",
93fdd5ac64bbe (Tomas Winkler         2017-01-05 10:45:12 +0200 6826)                    __func__, ret);
93fdd5ac64bbe (Tomas Winkler         2017-01-05 10:45:12 +0200 6827)            goto out;
93fdd5ac64bbe (Tomas Winkler         2017-01-05 10:45:12 +0200 6828)    }
93fdd5ac64bbe (Tomas Winkler         2017-01-05 10:45:12 +0200 6829) 
93fdd5ac64bbe (Tomas Winkler         2017-01-05 10:45:12 +0200 6830)    ufs_fixup_device_setup(hba, &card);
371131065de99 (Yaniv Gardi           2016-03-10 17:37:16 +0200 6831)    ufshcd_tune_unipro_params(hba);
60f0187031c05 (Yaniv Gardi           2016-03-10 17:37:11 +0200 6832) 
60f0187031c05 (Yaniv Gardi           2016-03-10 17:37:11 +0200 6833)    ret = ufshcd_set_vccq_rail_unused(hba,
60f0187031c05 (Yaniv Gardi           2016-03-10 17:37:11 +0200 6834)            (hba->dev_quirks & UFS_DEVICE_NO_VCCQ) ? true : false);
60f0187031c05 (Yaniv Gardi           2016-03-10 17:37:11 +0200 6835)    if (ret)
60f0187031c05 (Yaniv Gardi           2016-03-10 17:37:11 +0200 6836)            goto out;
60f0187031c05 (Yaniv Gardi           2016-03-10 17:37:11 +0200 6837) 
57d104c153d3d (Subhash Jadavani      2014-09-25 15:32:30 +0300 6838)    /* UFS device is also active now */
57d104c153d3d (Subhash Jadavani      2014-09-25 15:32:30 +0300 6839)    ufshcd_set_ufs_dev_active(hba);
66ec6d59407ba (Sujit Reddy Thumma    2013-07-30 00:35:59 +0530 6840)    ufshcd_force_reset_auto_bkops(hba);
57d104c153d3d (Subhash Jadavani      2014-09-25 15:32:30 +0300 6841)    hba->wlun_dev_clr_ua = true;


So the patch to revert is 60f0187031c05.

57d104c153d3d and 66ec6d59407ba are older, so no problem there.

Basically, we just need to preserve the lines from
371131065de99, 93fdd5ac64bbe, a4b0e8a4e92b1
and we're good to go.

The conflict looks like this:

<<<<<<< HEAD
	/* Init check for device descriptor sizes */
	ufshcd_init_desc_sizes(hba);

	ret = ufs_get_device_desc(hba, &card);
	if (ret) {
		dev_err(hba->dev, "%s: Failed getting device info. err = %d\n",
			__func__, ret);
		goto out;
	}

	ufs_fixup_device_setup(hba, &card);
	ufshcd_tune_unipro_params(hba);

	ret = ufshcd_set_vccq_rail_unused(hba,
		(hba->dev_quirks & UFS_DEVICE_NO_VCCQ) ? true : false);
	if (ret)
		goto out;

=======
	ufs_advertise_fixup_device(hba);
>>>>>>> parent of 60f0187031c0... scsi: ufs: disable vccq if it's not needed by UFS device


The resolution is simple: we keep the HEAD version, and simply remove
ufshcd_set_vccq_rail_unused() and its error-handling.


Is that the information you were after?


Do you prefer the patch subject to be:
Revert "scsi: ufs: disable vccq if it's not needed by UFS device"
instead of:
scsi: ufs: Do not disable vccq in UFSHC driver

And I should leave in the automatically added
This reverts commit 60f0187031c05e04cbadffb62f557d0ff3564490.
while adding some information from the manual resolution?

Regards.

Re: [PATCH v5 2/2] scsi: ufs: Remove unused device quirks

[Evan Green]

On Mon, Feb 11, 2019 at 5:36 AM Marc Gonzalez <marc.w.gonzalez@free.fr> wrote:
>
> The UFSHC driver defines a few quirks that are not used anywhere:
>
> UFS_DEVICE_QUIRK_BROKEN_LCC
> UFS_DEVICE_NO_VCCQ
> UFS_DEVICE_QUIRK_NO_LINK_OFF
> UFS_DEVICE_NO_FASTAUTO
>
> Let's remove them.
>
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>

Whoops, I lost track of this. Thanks for cleaning this up, Marc.

Reviewed-by: Evan Green <evgreen@chromium.org>

Re: [PATCH v5 1/2] scsi: ufs: Do not disable vccq in UFSHC driver

[Martin K. Petersen]

Marc,

> The resolution is simple: we keep the HEAD version, and simply remove
> ufshcd_set_vccq_rail_unused() and its error-handling.
>
>
> Is that the information you were after?

Yes.

> Do you prefer the patch subject to be:
> Revert "scsi: ufs: disable vccq if it's not needed by UFS device"
> instead of:
> scsi: ufs: Do not disable vccq in UFSHC driver
>
> And I should leave in the automatically added
> This reverts commit 60f0187031c05e04cbadffb62f557d0ff3564490.
> while adding some information from the manual resolution?

Exactly.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering