ufshcd quirk cleanup

ufshcd quirk cleanup

[Christoph Hellwig]

Hi all,

this removes lots of dead code from the ufs drivers, an cleans
up the quirk handling a little bit.

[PATCH 1/2] ufshcd: remove unused quirks

[Christoph Hellwig]

Remove various quirks that don't have users, as well as the dead code
keyed off them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/ufs/ufshcd.c | 120 +++++---------------------------------
 drivers/scsi/ufs/ufshcd.h |  22 -------
 2 files changed, 13 insertions(+), 129 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index abd0e6b05f79..2eb86851af7a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -642,11 +642,7 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
  */
 static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
 {
-	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
-		ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
-	else
-		ufshcd_writel(hba, ~(1 << pos),
-				REG_UTP_TRANSFER_REQ_LIST_CLEAR);
+	ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
 }
 
 /**
@@ -656,10 +652,7 @@ static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
  */
 static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)
 {
-	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
-		ufshcd_writel(hba, (1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
-	else
-		ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
+	ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
 }
 
 /**
@@ -2093,13 +2086,8 @@ static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 		return sg_segments;
 
 	if (sg_segments) {
-		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN)
-			lrbp->utr_descriptor_ptr->prd_table_length =
-				cpu_to_le16((u16)(sg_segments *
-					sizeof(struct ufshcd_sg_entry)));
-		else
-			lrbp->utr_descriptor_ptr->prd_table_length =
-				cpu_to_le16((u16) (sg_segments));
+		lrbp->utr_descriptor_ptr->prd_table_length =
+			cpu_to_le16((u16) (sg_segments));
 
 		prd_table = (struct ufshcd_sg_entry *)lrbp->ucd_prdt_ptr;
 
@@ -3403,21 +3391,12 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
 				cpu_to_le32(upper_32_bits(cmd_desc_element_addr));
 
 		/* Response upiu and prdt offset should be in double words */
-		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) {
-			utrdlp[i].response_upiu_offset =
-				cpu_to_le16(response_offset);
-			utrdlp[i].prd_table_offset =
-				cpu_to_le16(prdt_offset);
-			utrdlp[i].response_upiu_length =
-				cpu_to_le16(ALIGNED_UPIU_SIZE);
-		} else {
-			utrdlp[i].response_upiu_offset =
-				cpu_to_le16((response_offset >> 2));
-			utrdlp[i].prd_table_offset =
-				cpu_to_le16((prdt_offset >> 2));
-			utrdlp[i].response_upiu_length =
-				cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
-		}
+		utrdlp[i].response_upiu_offset =
+			cpu_to_le16((response_offset >> 2));
+		utrdlp[i].prd_table_offset =
+			cpu_to_le16((prdt_offset >> 2));
+		utrdlp[i].response_upiu_length =
+			cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
 
 		hba->lrb[i].utr_descriptor_ptr = (utrdlp + i);
 		hba->lrb[i].utrd_dma_addr = hba->utrdl_dma_addr +
@@ -3460,52 +3439,6 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
 			"dme-link-startup: error code %d\n", ret);
 	return ret;
 }
-/**
- * ufshcd_dme_reset - UIC command for DME_RESET
- * @hba: per adapter instance
- *
- * DME_RESET command is issued in order to reset UniPro stack.
- * This function now deal with cold reset.
- *
- * Returns 0 on success, non-zero value on failure
- */
-static int ufshcd_dme_reset(struct ufs_hba *hba)
-{
-	struct uic_command uic_cmd = {0};
-	int ret;
-
-	uic_cmd.command = UIC_CMD_DME_RESET;
-
-	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
-	if (ret)
-		dev_err(hba->dev,
-			"dme-reset: error code %d\n", ret);
-
-	return ret;
-}
-
-/**
- * ufshcd_dme_enable - UIC command for DME_ENABLE
- * @hba: per adapter instance
- *
- * DME_ENABLE command is issued in order to enable UniPro stack.
- *
- * Returns 0 on success, non-zero value on failure
- */
-static int ufshcd_dme_enable(struct ufs_hba *hba)
-{
-	struct uic_command uic_cmd = {0};
-	int ret;
-
-	uic_cmd.command = UIC_CMD_DME_ENABLE;
-
-	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
-	if (ret)
-		dev_err(hba->dev,
-			"dme-reset: error code %d\n", ret);
-
-	return ret;
-}
 
 static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
 {
@@ -4217,7 +4150,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
 }
 
 /**
- * ufshcd_hba_execute_hce - initialize the controller
+ * ufshcd_hba_enable - initialize the controller
  * @hba: per adapter instance
  *
  * The controller resets itself and controller firmware initialization
@@ -4226,7 +4159,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
  *
  * Returns 0 on success, non-zero value on failure
  */
-static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
+int ufshcd_hba_enable(struct ufs_hba *hba)
 {
 	int retry;
 
@@ -4274,32 +4207,6 @@ static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
 
 	return 0;
 }
-
-int ufshcd_hba_enable(struct ufs_hba *hba)
-{
-	int ret;
-
-	if (hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) {
-		ufshcd_set_link_off(hba);
-		ufshcd_vops_hce_enable_notify(hba, PRE_CHANGE);
-
-		/* enable UIC related interrupts */
-		ufshcd_enable_intr(hba, UFSHCD_UIC_MASK);
-		ret = ufshcd_dme_reset(hba);
-		if (!ret) {
-			ret = ufshcd_dme_enable(hba);
-			if (!ret)
-				ufshcd_vops_hce_enable_notify(hba, POST_CHANGE);
-			if (ret)
-				dev_err(hba->dev,
-					"Host controller enable failed with non-hce\n");
-		}
-	} else {
-		ret = ufshcd_hba_execute_hce(hba);
-	}
-
-	return ret;
-}
 EXPORT_SYMBOL_GPL(ufshcd_hba_enable);
 
 static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer)
@@ -4869,8 +4776,7 @@ static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 	 * false interrupt if device completes another request after resetting
 	 * aggregation and before reading the DB.
 	 */
-	if (ufshcd_is_intr_aggr_allowed(hba) &&
-	    !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
+	if (ufshcd_is_intr_aggr_allowed(hba))
 		ufshcd_reset_intr_aggr(hba);
 
 	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2ae6c7c8528c..9c2b80f87b9f 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -612,28 +612,6 @@ struct ufs_hba {
 	 */
 	#define UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION		0x20
 
-	/*
-	 * This quirk needs to be enabled if the host contoller regards
-	 * resolution of the values of PRDTO and PRDTL in UTRD as byte.
-	 */
-	#define UFSHCD_QUIRK_PRDT_BYTE_GRAN			0x80
-
-	/*
-	 * Clear handling for transfer/task request list is just opposite.
-	 */
-	#define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR		0x100
-
-	/*
-	 * This quirk needs to be enabled if host controller doesn't allow
-	 * that the interrupt aggregation timer and counter are reset by s/w.
-	 */
-	#define UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR		0x200
-
-	/*
-	 * This quirks needs to be enabled if host controller cannot be
-	 * enabled via HCE register.
-	 */
-	#define UFSHCI_QUIRK_BROKEN_HCE				0x400
 	unsigned int quirks;	/* Deviations from standard UFSHCI spec. */
 
 	/* Device deviations from standard UFS device spec. */
-- 
2.24.1

RE: [PATCH 1/2] ufshcd: remove unused quirks

[Kiwoong Kim]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org>
> On Behalf Of Christoph Hellwig
> Sent: Wednesday, February 19, 2020 8:45 AM
> To: linux-scsi@vger.kernel.org
> Cc: Alim Akhtar <alim.akhtar@samsung.com>; Avri Altman
> <avri.altman@wdc.com>
> Subject: [PATCH 1/2] ufshcd: remove unused quirks
> 
> Remove various quirks that don't have users, as well as the dead code
> keyed off them.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/ufs/ufshcd.c | 120 +++++---------------------------------
>  drivers/scsi/ufs/ufshcd.h |  22 -------
>  2 files changed, 13 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> abd0e6b05f79..2eb86851af7a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -642,11 +642,7 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb
> *lrbp)
>   */
>  static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)  {
> -	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> -		ufshcd_writel(hba, (1 << pos),
> REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> -	else
> -		ufshcd_writel(hba, ~(1 << pos),
> -				REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> +	ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
>  }
> 
>  /**
> @@ -656,10 +652,7 @@ static inline void ufshcd_utrl_clear(struct ufs_hba
> *hba, u32 pos)
>   */
>  static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)  {
> -	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> -		ufshcd_writel(hba, (1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
> -	else
> -		ufshcd_writel(hba, ~(1 << pos),
REG_UTP_TASK_REQ_LIST_CLEAR);
> +	ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
>  }
> 
>  /**
> @@ -2093,13 +2086,8 @@ static int ufshcd_map_sg(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp)
>  		return sg_segments;
> 
>  	if (sg_segments) {
> -		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN)
> -			lrbp->utr_descriptor_ptr->prd_table_length =
> -				cpu_to_le16((u16)(sg_segments *
> -					sizeof(struct ufshcd_sg_entry)));
> -		else
> -			lrbp->utr_descriptor_ptr->prd_table_length =
> -				cpu_to_le16((u16) (sg_segments));
> +		lrbp->utr_descriptor_ptr->prd_table_length =
> +			cpu_to_le16((u16) (sg_segments));
> 
>  		prd_table = (struct ufshcd_sg_entry *)lrbp->ucd_prdt_ptr;
> 
> @@ -3403,21 +3391,12 @@ static void ufshcd_host_memory_configure(struct
> ufs_hba *hba)
> 
> 	cpu_to_le32(upper_32_bits(cmd_desc_element_addr));
> 
>  		/* Response upiu and prdt offset should be in double words
> */
> -		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) {
> -			utrdlp[i].response_upiu_offset =
> -				cpu_to_le16(response_offset);
> -			utrdlp[i].prd_table_offset =
> -				cpu_to_le16(prdt_offset);
> -			utrdlp[i].response_upiu_length =
> -				cpu_to_le16(ALIGNED_UPIU_SIZE);
> -		} else {
> -			utrdlp[i].response_upiu_offset =
> -				cpu_to_le16((response_offset >> 2));
> -			utrdlp[i].prd_table_offset =
> -				cpu_to_le16((prdt_offset >> 2));
> -			utrdlp[i].response_upiu_length =
> -				cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
> -		}
> +		utrdlp[i].response_upiu_offset =
> +			cpu_to_le16((response_offset >> 2));
> +		utrdlp[i].prd_table_offset =
> +			cpu_to_le16((prdt_offset >> 2));
> +		utrdlp[i].response_upiu_length =
> +			cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
> 
>  		hba->lrb[i].utr_descriptor_ptr = (utrdlp + i);
>  		hba->lrb[i].utrd_dma_addr = hba->utrdl_dma_addr + @@ -
> 3460,52 +3439,6 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
>  			"dme-link-startup: error code %d\n", ret);
>  	return ret;
>  }
> -/**
> - * ufshcd_dme_reset - UIC command for DME_RESET
> - * @hba: per adapter instance
> - *
> - * DME_RESET command is issued in order to reset UniPro stack.
> - * This function now deal with cold reset.
> - *
> - * Returns 0 on success, non-zero value on failure
> - */
> -static int ufshcd_dme_reset(struct ufs_hba *hba) -{
> -	struct uic_command uic_cmd = {0};
> -	int ret;
> -
> -	uic_cmd.command = UIC_CMD_DME_RESET;
> -
> -	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> -	if (ret)
> -		dev_err(hba->dev,
> -			"dme-reset: error code %d\n", ret);
> -
> -	return ret;
> -}
> -
> -/**
> - * ufshcd_dme_enable - UIC command for DME_ENABLE
> - * @hba: per adapter instance
> - *
> - * DME_ENABLE command is issued in order to enable UniPro stack.
> - *
> - * Returns 0 on success, non-zero value on failure
> - */
> -static int ufshcd_dme_enable(struct ufs_hba *hba) -{
> -	struct uic_command uic_cmd = {0};
> -	int ret;
> -
> -	uic_cmd.command = UIC_CMD_DME_ENABLE;
> -
> -	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> -	if (ret)
> -		dev_err(hba->dev,
> -			"dme-reset: error code %d\n", ret);
> -
> -	return ret;
> -}
> 
>  static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
> { @@ -4217,7 +4150,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba
> *hba, bool can_sleep)  }
> 
>  /**
> - * ufshcd_hba_execute_hce - initialize the controller
> + * ufshcd_hba_enable - initialize the controller
>   * @hba: per adapter instance
>   *
>   * The controller resets itself and controller firmware initialization @@
> -4226,7 +4159,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba,
> bool can_sleep)
>   *
>   * Returns 0 on success, non-zero value on failure
>   */
> -static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
> +int ufshcd_hba_enable(struct ufs_hba *hba)
>  {
>  	int retry;
> 
> @@ -4274,32 +4207,6 @@ static int ufshcd_hba_execute_hce(struct ufs_hba
> *hba)
> 
>  	return 0;
>  }
> -
> -int ufshcd_hba_enable(struct ufs_hba *hba) -{
> -	int ret;
> -
> -	if (hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) {
> -		ufshcd_set_link_off(hba);
> -		ufshcd_vops_hce_enable_notify(hba, PRE_CHANGE);
> -
> -		/* enable UIC related interrupts */
> -		ufshcd_enable_intr(hba, UFSHCD_UIC_MASK);
> -		ret = ufshcd_dme_reset(hba);
> -		if (!ret) {
> -			ret = ufshcd_dme_enable(hba);
> -			if (!ret)
> -				ufshcd_vops_hce_enable_notify(hba,
POST_CHANGE);
> -			if (ret)
> -				dev_err(hba->dev,
> -					"Host controller enable failed with
non-
> hce\n");
> -		}
> -	} else {
> -		ret = ufshcd_hba_execute_hce(hba);
> -	}
> -
> -	return ret;
> -}
>  EXPORT_SYMBOL_GPL(ufshcd_hba_enable);
> 
>  static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer) @@ -
> 4869,8 +4776,7 @@ static irqreturn_t ufshcd_transfer_req_compl(struct
> ufs_hba *hba)
>  	 * false interrupt if device completes another request after
> resetting
>  	 * aggregation and before reading the DB.
>  	 */
> -	if (ufshcd_is_intr_aggr_allowed(hba) &&
> -	    !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
> +	if (ufshcd_is_intr_aggr_allowed(hba))
>  		ufshcd_reset_intr_aggr(hba);
> 
>  	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
> 2ae6c7c8528c..9c2b80f87b9f 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -612,28 +612,6 @@ struct ufs_hba {
>  	 */
>  	#define UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION		0x20
> 
> -	/*
> -	 * This quirk needs to be enabled if the host contoller regards
> -	 * resolution of the values of PRDTO and PRDTL in UTRD as byte.
> -	 */
> -	#define UFSHCD_QUIRK_PRDT_BYTE_GRAN			0x80
> -
> -	/*
> -	 * Clear handling for transfer/task request list is just opposite.
> -	 */
> -	#define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR		0x100
> -
> -	/*
> -	 * This quirk needs to be enabled if host controller doesn't allow
> -	 * that the interrupt aggregation timer and counter are reset by
> s/w.
> -	 */
> -	#define UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR		0x200
> -
> -	/*
> -	 * This quirks needs to be enabled if host controller cannot be
> -	 * enabled via HCE register.
> -	 */
> -	#define UFSHCI_QUIRK_BROKEN_HCE				0x400
>  	unsigned int quirks;	/* Deviations from standard UFSHCI spec.
> */
> 
>  	/* Device deviations from standard UFS device spec. */
> --
> 2.24.1

RE: [PATCH 1/2] ufshcd: remove unused quirks

[Kiwoong Kim]

> From: linux-scsi-owner@vger.kernel.org <linux-scsi-owner@vger.kernel.org>
> On Behalf Of Christoph Hellwig
> Sent: Wednesday, February 19, 2020 8:45 AM
> To: linux-scsi@vger.kernel.org
> Cc: Alim Akhtar <alim.akhtar@samsung.com>; Avri Altman
> <avri.altman@wdc.com>
> Subject: [PATCH 1/2] ufshcd: remove unused quirks
> 
> Remove various quirks that don't have users, as well as the dead code
> keyed off them.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/scsi/ufs/ufshcd.c | 120 +++++---------------------------------
>  drivers/scsi/ufs/ufshcd.h |  22 -------
>  2 files changed, 13 insertions(+), 129 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index
> abd0e6b05f79..2eb86851af7a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -642,11 +642,7 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb
> *lrbp)
>   */
>  static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)  {
> -	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> -		ufshcd_writel(hba, (1 << pos),
> REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> -	else
> -		ufshcd_writel(hba, ~(1 << pos),
> -				REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> +	ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
>  }
> 
>  /**
> @@ -656,10 +652,7 @@ static inline void ufshcd_utrl_clear(struct ufs_hba
> *hba, u32 pos)
>   */
>  static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)  {
> -	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> -		ufshcd_writel(hba, (1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
> -	else
> -		ufshcd_writel(hba, ~(1 << pos),
REG_UTP_TASK_REQ_LIST_CLEAR);
> +	ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
>  }
> 
>  /**
> @@ -2093,13 +2086,8 @@ static int ufshcd_map_sg(struct ufs_hba *hba,
> struct ufshcd_lrb *lrbp)
>  		return sg_segments;
> 
>  	if (sg_segments) {
> -		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN)
> -			lrbp->utr_descriptor_ptr->prd_table_length =
> -				cpu_to_le16((u16)(sg_segments *
> -					sizeof(struct ufshcd_sg_entry)));
> -		else
> -			lrbp->utr_descriptor_ptr->prd_table_length =
> -				cpu_to_le16((u16) (sg_segments));
> +		lrbp->utr_descriptor_ptr->prd_table_length =
> +			cpu_to_le16((u16) (sg_segments));
> 
>  		prd_table = (struct ufshcd_sg_entry *)lrbp->ucd_prdt_ptr;
> 
> @@ -3403,21 +3391,12 @@ static void ufshcd_host_memory_configure(struct
> ufs_hba *hba)
> 
> 	cpu_to_le32(upper_32_bits(cmd_desc_element_addr));
> 
>  		/* Response upiu and prdt offset should be in double words
> */
> -		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) {
> -			utrdlp[i].response_upiu_offset =
> -				cpu_to_le16(response_offset);
> -			utrdlp[i].prd_table_offset =
> -				cpu_to_le16(prdt_offset);
> -			utrdlp[i].response_upiu_length =
> -				cpu_to_le16(ALIGNED_UPIU_SIZE);
> -		} else {
> -			utrdlp[i].response_upiu_offset =
> -				cpu_to_le16((response_offset >> 2));
> -			utrdlp[i].prd_table_offset =
> -				cpu_to_le16((prdt_offset >> 2));
> -			utrdlp[i].response_upiu_length =
> -				cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
> -		}
> +		utrdlp[i].response_upiu_offset =
> +			cpu_to_le16((response_offset >> 2));
> +		utrdlp[i].prd_table_offset =
> +			cpu_to_le16((prdt_offset >> 2));
> +		utrdlp[i].response_upiu_length =
> +			cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
> 
>  		hba->lrb[i].utr_descriptor_ptr = (utrdlp + i);
>  		hba->lrb[i].utrd_dma_addr = hba->utrdl_dma_addr + @@ -
> 3460,52 +3439,6 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
>  			"dme-link-startup: error code %d\n", ret);
>  	return ret;
>  }
> -/**
> - * ufshcd_dme_reset - UIC command for DME_RESET
> - * @hba: per adapter instance
> - *
> - * DME_RESET command is issued in order to reset UniPro stack.
> - * This function now deal with cold reset.
> - *
> - * Returns 0 on success, non-zero value on failure
> - */
> -static int ufshcd_dme_reset(struct ufs_hba *hba) -{
> -	struct uic_command uic_cmd = {0};
> -	int ret;
> -
> -	uic_cmd.command = UIC_CMD_DME_RESET;
> -
> -	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> -	if (ret)
> -		dev_err(hba->dev,
> -			"dme-reset: error code %d\n", ret);
> -
> -	return ret;
> -}
> -
> -/**
> - * ufshcd_dme_enable - UIC command for DME_ENABLE
> - * @hba: per adapter instance
> - *
> - * DME_ENABLE command is issued in order to enable UniPro stack.
> - *
> - * Returns 0 on success, non-zero value on failure
> - */
> -static int ufshcd_dme_enable(struct ufs_hba *hba) -{
> -	struct uic_command uic_cmd = {0};
> -	int ret;
> -
> -	uic_cmd.command = UIC_CMD_DME_ENABLE;
> -
> -	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> -	if (ret)
> -		dev_err(hba->dev,
> -			"dme-reset: error code %d\n", ret);
> -
> -	return ret;
> -}
> 
>  static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
> { @@ -4217,7 +4150,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba
> *hba, bool can_sleep)  }
> 
>  /**
> - * ufshcd_hba_execute_hce - initialize the controller
> + * ufshcd_hba_enable - initialize the controller
>   * @hba: per adapter instance
>   *
>   * The controller resets itself and controller firmware initialization @@
> -4226,7 +4159,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba,
> bool can_sleep)
>   *
>   * Returns 0 on success, non-zero value on failure
>   */
> -static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
> +int ufshcd_hba_enable(struct ufs_hba *hba)
>  {
>  	int retry;
> 
> @@ -4274,32 +4207,6 @@ static int ufshcd_hba_execute_hce(struct ufs_hba
> *hba)
> 
>  	return 0;
>  }
> -
> -int ufshcd_hba_enable(struct ufs_hba *hba) -{
> -	int ret;
> -
> -	if (hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) {
> -		ufshcd_set_link_off(hba);
> -		ufshcd_vops_hce_enable_notify(hba, PRE_CHANGE);
> -
> -		/* enable UIC related interrupts */
> -		ufshcd_enable_intr(hba, UFSHCD_UIC_MASK);
> -		ret = ufshcd_dme_reset(hba);
> -		if (!ret) {
> -			ret = ufshcd_dme_enable(hba);
> -			if (!ret)
> -				ufshcd_vops_hce_enable_notify(hba,
POST_CHANGE);
> -			if (ret)
> -				dev_err(hba->dev,
> -					"Host controller enable failed with
non-
> hce\n");
> -		}
> -	} else {
> -		ret = ufshcd_hba_execute_hce(hba);
> -	}
> -
> -	return ret;
> -}
>  EXPORT_SYMBOL_GPL(ufshcd_hba_enable);
> 
>  static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer) @@ -
> 4869,8 +4776,7 @@ static irqreturn_t ufshcd_transfer_req_compl(struct
> ufs_hba *hba)
>  	 * false interrupt if device completes another request after
> resetting
>  	 * aggregation and before reading the DB.
>  	 */
> -	if (ufshcd_is_intr_aggr_allowed(hba) &&
> -	    !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
> +	if (ufshcd_is_intr_aggr_allowed(hba))
>  		ufshcd_reset_intr_aggr(hba);
> 
>  	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index
> 2ae6c7c8528c..9c2b80f87b9f 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -612,28 +612,6 @@ struct ufs_hba {
>  	 */
>  	#define UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION		0x20
> 
> -	/*
> -	 * This quirk needs to be enabled if the host contoller regards
> -	 * resolution of the values of PRDTO and PRDTL in UTRD as byte.
> -	 */
> -	#define UFSHCD_QUIRK_PRDT_BYTE_GRAN			0x80
> -
> -	/*
> -	 * Clear handling for transfer/task request list is just opposite.
> -	 */
> -	#define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR		0x100
> -
> -	/*
> -	 * This quirk needs to be enabled if host controller doesn't allow
> -	 * that the interrupt aggregation timer and counter are reset by
> s/w.
> -	 */
> -	#define UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR		0x200
> -
> -	/*
> -	 * This quirks needs to be enabled if host controller cannot be
> -	 * enabled via HCE register.
> -	 */
> -	#define UFSHCI_QUIRK_BROKEN_HCE				0x400
>  	unsigned int quirks;	/* Deviations from standard UFSHCI spec.
> */
> 
>  	/* Device deviations from standard UFS device spec. */
> --
> 2.24.1

Exynos specific driver sets and is using the following quirks but the driver
is not updated
yet. I'll do upstream it in the future.
- UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR
- UFSHCD_QUIRK_PRDT_BYTE_GRAN
- UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR

Re: [PATCH 1/2] ufshcd: remove unused quirks

[Eric Biggers]

On Wed, Feb 19, 2020 at 09:00:26AM +0900, Kiwoong Kim wrote:
> 
> Exynos specific driver sets and is using the following quirks but the driver
> is not updated
> yet. I'll do upstream it in the future.
> - UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR
> - UFSHCD_QUIRK_PRDT_BYTE_GRAN
> - UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR

These quirks have been there for 2-3 years without the driver that needs them
even being posted to the mailing list since 2017.  Since we don't keep unused
code in the upstream kernel, I support the removal of these quirks.  If you
don't want them to be removed, you need to get your driver upstream.

- Eric

Re: [PATCH 1/2] ufshcd: remove unused quirks

[Alim Akhtar]

Hi Eric

On Wed, Feb 19, 2020 at 6:14 AM Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Wed, Feb 19, 2020 at 09:00:26AM +0900, Kiwoong Kim wrote:
> >
> > Exynos specific driver sets and is using the following quirks but the driver
> > is not updated
> > yet. I'll do upstream it in the future.
> > - UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR
> > - UFSHCD_QUIRK_PRDT_BYTE_GRAN
> > - UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR
>
> These quirks have been there for 2-3 years without the driver that needs them
> even being posted to the mailing list since 2017.  Since we don't keep unused
> code in the upstream kernel, I support the removal of these quirks.  If you
> don't want them to be removed, you need to get your driver upstream.
>

Yes, understand it got delayed significantly, we will try to send
driver after fixing the previous review comments.
Thanks for heads-up.

> - Eric



-- 
Regards,
Alim

Re: [PATCH 1/2] ufshcd: remove unused quirks

['Christoph Hellwig']

On Tue, Feb 18, 2020 at 04:42:49PM -0800, Eric Biggers wrote:
> These quirks have been there for 2-3 years without the driver that needs them
> even being posted to the mailing list since 2017.  Since we don't keep unused
> code in the upstream kernel, I support the removal of these quirks.  If you
> don't want them to be removed, you need to get your driver upstream.

Yes.  And some of them could be improved a little bit as well if they
get added back.  But given that this didn't happen in all the years I
do not mentally expect the driver to ever make it anyway.

Re: [PATCH 1/2] ufshcd: remove unused quirks

[Eric Biggers]

On Tue, Feb 18, 2020 at 03:44:49PM -0800, Christoph Hellwig wrote:
>  static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
>  {
> -	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> -		ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> -	else
> -		ufshcd_writel(hba, ~(1 << pos),
> -				REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> +	ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
>  }

This part is keeping the quirk version.  It needs to be other way around.

- Eric

Re: [PATCH 1/2] ufshcd: remove unused quirks

[Christoph Hellwig]

On Tue, Feb 18, 2020 at 04:30:56PM -0800, Eric Biggers wrote:
> On Tue, Feb 18, 2020 at 03:44:49PM -0800, Christoph Hellwig wrote:
> >  static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
> >  {
> > -	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> > -		ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> > -	else
> > -		ufshcd_writel(hba, ~(1 << pos),
> > -				REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> > +	ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> >  }
> 
> This part is keeping the quirk version.  It needs to be other way around.

Fixed.

[PATCH 2/2] ufshcd: use an enum for quirks

[Christoph Hellwig]

Use an enum to specify the various quirks instead of #defines inside
the structure definition.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/ufs/ufshcd.h | 82 ++++++++++++++++++++-------------------
 1 file changed, 42 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 9c2b80f87b9f..f68b3cd2e465 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -469,6 +469,48 @@ struct ufs_stats {
 	struct ufs_err_reg_hist task_abort;
 };
 
+enum ufshcd_quirks {
+	/* Interrupt aggregation support is broken */
+	UFSHCD_QUIRK_BROKEN_INTR_AGGR			= 1 << 0,
+
+	/*
+	 * delay before each dme command is required as the unipro
+	 * layer has shown instabilities
+	 */
+	UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS		= 1 << 1,
+
+	/*
+	 * If UFS host controller is having issue in processing LCC (Line
+	 * Control Command) coming from device then enable this quirk.
+	 * When this quirk is enabled, host controller driver should disable
+	 * the LCC transmission on UFS device (by clearing TX_LCC_ENABLE
+	 * attribute of device to 0).
+	 */
+	UFSHCD_QUIRK_BROKEN_LCC				= 1 << 2,
+
+	/*
+	 * The attribute PA_RXHSUNTERMCAP specifies whether or not the
+	 * inbound Link supports unterminated line in HS mode. Setting this
+	 * attribute to 1 fixes moving to HS gear.
+	 */
+	UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP		= 1 << 3,
+
+	/*
+	 * This quirk needs to be enabled if the host contoller only allows
+	 * accessing the peer dme attributes in AUTO mode (FAST AUTO or
+	 * SLOW AUTO).
+	 */
+	UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE		= 1 << 4,
+
+	/*
+	 * This quirk needs to be enabled if the host contoller doesn't
+	 * advertise the correct version in UFS_VER register. If this quirk
+	 * is enabled, standard UFS host driver will call the vendor specific
+	 * ops (get_ufs_hci_version) to get the correct version.
+	 */
+	UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION		= 1 << 5,
+};
+
 /**
  * struct ufs_hba - per adapter private structure
  * @mmio_base: UFSHCI base register address
@@ -572,46 +614,6 @@ struct ufs_hba {
 	bool is_irq_enabled;
 	enum ufs_ref_clk_freq dev_ref_clk_freq;
 
-	/* Interrupt aggregation support is broken */
-	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			0x1
-
-	/*
-	 * delay before each dme command is required as the unipro
-	 * layer has shown instabilities
-	 */
-	#define UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS		0x2
-
-	/*
-	 * If UFS host controller is having issue in processing LCC (Line
-	 * Control Command) coming from device then enable this quirk.
-	 * When this quirk is enabled, host controller driver should disable
-	 * the LCC transmission on UFS device (by clearing TX_LCC_ENABLE
-	 * attribute of device to 0).
-	 */
-	#define UFSHCD_QUIRK_BROKEN_LCC				0x4
-
-	/*
-	 * The attribute PA_RXHSUNTERMCAP specifies whether or not the
-	 * inbound Link supports unterminated line in HS mode. Setting this
-	 * attribute to 1 fixes moving to HS gear.
-	 */
-	#define UFSHCD_QUIRK_BROKEN_PA_RXHSUNTERMCAP		0x8
-
-	/*
-	 * This quirk needs to be enabled if the host contoller only allows
-	 * accessing the peer dme attributes in AUTO mode (FAST AUTO or
-	 * SLOW AUTO).
-	 */
-	#define UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE		0x10
-
-	/*
-	 * This quirk needs to be enabled if the host contoller doesn't
-	 * advertise the correct version in UFS_VER register. If this quirk
-	 * is enabled, standard UFS host driver will call the vendor specific
-	 * ops (get_ufs_hci_version) to get the correct version.
-	 */
-	#define UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION		0x20
-
 	unsigned int quirks;	/* Deviations from standard UFSHCI spec. */
 
 	/* Device deviations from standard UFS device spec. */
-- 
2.24.1

Re: [PATCH 2/2] ufshcd: use an enum for quirks

[Bart Van Assche]

On 2/18/20 3:44 PM, Christoph Hellwig wrote:
> Use an enum to specify the various quirks instead of #defines inside
> the structure definition.

Hi Christoph,

Although this patch looks like a significant improvement to me: has it 
been considered to change 'quirks' from an unsigned int into a bitfield 
with one bit per quirk? I think that would allow to remove multiple 
explicit bit manipulations from the UFS driver.

Thanks,

Bart.

Re: [PATCH 2/2] ufshcd: use an enum for quirks

[Christoph Hellwig]

On Tue, Feb 18, 2020 at 04:10:57PM -0800, Bart Van Assche wrote:
> On 2/18/20 3:44 PM, Christoph Hellwig wrote:
>> Use an enum to specify the various quirks instead of #defines inside
>> the structure definition.
>
> Hi Christoph,
>
> Although this patch looks like a significant improvement to me: has it been 
> considered to change 'quirks' from an unsigned int into a bitfield with one 
> bit per quirk? I think that would allow to remove multiple explicit bit 
> manipulations from the UFS driver.

And it wouldn't make it quite as obvious what are quirks.  Never mind that
the compiler would still do the masking and potentially affect other fields
placed right next to it.  Bitfields tend to be a bad idea just about
everywhere.

ufshcd quirk cleanup v2

[Christoph Hellwig]

Hi all,

this removes lots of dead code from the ufs drivers, an cleans
up the quirk handling a little bit.

Changes since v1:
 - fix ufshcd_utrl_clear
 - minor cleanups

[PATCH 1/2] ufshcd: remove unused quirks

[Christoph Hellwig]

Remove various quirks that don't have users, as well as the dead code
keyed off them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/ufs/ufshcd.c | 119 ++++----------------------------------
 drivers/scsi/ufs/ufshcd.h |  22 -------
 2 files changed, 12 insertions(+), 129 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index abd0e6b05f79..886a8a597e6a 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -642,11 +642,7 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
  */
 static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
 {
-	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
-		ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
-	else
-		ufshcd_writel(hba, ~(1 << pos),
-				REG_UTP_TRANSFER_REQ_LIST_CLEAR);
+	ufshcd_writel(hba, ~(1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
 }
 
 /**
@@ -656,10 +652,7 @@ static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
  */
 static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)
 {
-	if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
-		ufshcd_writel(hba, (1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
-	else
-		ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
+	ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
 }
 
 /**
@@ -2093,13 +2086,8 @@ static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
 		return sg_segments;
 
 	if (sg_segments) {
-		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN)
-			lrbp->utr_descriptor_ptr->prd_table_length =
-				cpu_to_le16((u16)(sg_segments *
-					sizeof(struct ufshcd_sg_entry)));
-		else
-			lrbp->utr_descriptor_ptr->prd_table_length =
-				cpu_to_le16((u16) (sg_segments));
+		lrbp->utr_descriptor_ptr->prd_table_length =
+			cpu_to_le16((u16)sg_segments);
 
 		prd_table = (struct ufshcd_sg_entry *)lrbp->ucd_prdt_ptr;
 
@@ -3403,21 +3391,11 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
 				cpu_to_le32(upper_32_bits(cmd_desc_element_addr));
 
 		/* Response upiu and prdt offset should be in double words */
-		if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) {
-			utrdlp[i].response_upiu_offset =
-				cpu_to_le16(response_offset);
-			utrdlp[i].prd_table_offset =
-				cpu_to_le16(prdt_offset);
-			utrdlp[i].response_upiu_length =
-				cpu_to_le16(ALIGNED_UPIU_SIZE);
-		} else {
-			utrdlp[i].response_upiu_offset =
-				cpu_to_le16((response_offset >> 2));
-			utrdlp[i].prd_table_offset =
-				cpu_to_le16((prdt_offset >> 2));
-			utrdlp[i].response_upiu_length =
-				cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
-		}
+		utrdlp[i].response_upiu_offset =
+			cpu_to_le16(response_offset >> 2);
+		utrdlp[i].prd_table_offset = cpu_to_le16(prdt_offset >> 2);
+		utrdlp[i].response_upiu_length =
+			cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
 
 		hba->lrb[i].utr_descriptor_ptr = (utrdlp + i);
 		hba->lrb[i].utrd_dma_addr = hba->utrdl_dma_addr +
@@ -3460,52 +3438,6 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
 			"dme-link-startup: error code %d\n", ret);
 	return ret;
 }
-/**
- * ufshcd_dme_reset - UIC command for DME_RESET
- * @hba: per adapter instance
- *
- * DME_RESET command is issued in order to reset UniPro stack.
- * This function now deal with cold reset.
- *
- * Returns 0 on success, non-zero value on failure
- */
-static int ufshcd_dme_reset(struct ufs_hba *hba)
-{
-	struct uic_command uic_cmd = {0};
-	int ret;
-
-	uic_cmd.command = UIC_CMD_DME_RESET;
-
-	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
-	if (ret)
-		dev_err(hba->dev,
-			"dme-reset: error code %d\n", ret);
-
-	return ret;
-}
-
-/**
- * ufshcd_dme_enable - UIC command for DME_ENABLE
- * @hba: per adapter instance
- *
- * DME_ENABLE command is issued in order to enable UniPro stack.
- *
- * Returns 0 on success, non-zero value on failure
- */
-static int ufshcd_dme_enable(struct ufs_hba *hba)
-{
-	struct uic_command uic_cmd = {0};
-	int ret;
-
-	uic_cmd.command = UIC_CMD_DME_ENABLE;
-
-	ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
-	if (ret)
-		dev_err(hba->dev,
-			"dme-reset: error code %d\n", ret);
-
-	return ret;
-}
 
 static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
 {
@@ -4217,7 +4149,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
 }
 
 /**
- * ufshcd_hba_execute_hce - initialize the controller
+ * ufshcd_hba_enable - initialize the controller
  * @hba: per adapter instance
  *
  * The controller resets itself and controller firmware initialization
@@ -4226,7 +4158,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
  *
  * Returns 0 on success, non-zero value on failure
  */
-static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
+int ufshcd_hba_enable(struct ufs_hba *hba)
 {
 	int retry;
 
@@ -4274,32 +4206,6 @@ static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
 
 	return 0;
 }
-
-int ufshcd_hba_enable(struct ufs_hba *hba)
-{
-	int ret;
-
-	if (hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) {
-		ufshcd_set_link_off(hba);
-		ufshcd_vops_hce_enable_notify(hba, PRE_CHANGE);
-
-		/* enable UIC related interrupts */
-		ufshcd_enable_intr(hba, UFSHCD_UIC_MASK);
-		ret = ufshcd_dme_reset(hba);
-		if (!ret) {
-			ret = ufshcd_dme_enable(hba);
-			if (!ret)
-				ufshcd_vops_hce_enable_notify(hba, POST_CHANGE);
-			if (ret)
-				dev_err(hba->dev,
-					"Host controller enable failed with non-hce\n");
-		}
-	} else {
-		ret = ufshcd_hba_execute_hce(hba);
-	}
-
-	return ret;
-}
 EXPORT_SYMBOL_GPL(ufshcd_hba_enable);
 
 static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer)
@@ -4869,8 +4775,7 @@ static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
 	 * false interrupt if device completes another request after resetting
 	 * aggregation and before reading the DB.
 	 */
-	if (ufshcd_is_intr_aggr_allowed(hba) &&
-	    !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
+	if (ufshcd_is_intr_aggr_allowed(hba))
 		ufshcd_reset_intr_aggr(hba);
 
 	tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 2ae6c7c8528c..9c2b80f87b9f 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -612,28 +612,6 @@ struct ufs_hba {
 	 */
 	#define UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION		0x20
 
-	/*
-	 * This quirk needs to be enabled if the host contoller regards
-	 * resolution of the values of PRDTO and PRDTL in UTRD as byte.
-	 */
-	#define UFSHCD_QUIRK_PRDT_BYTE_GRAN			0x80
-
-	/*
-	 * Clear handling for transfer/task request list is just opposite.
-	 */
-	#define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR		0x100
-
-	/*
-	 * This quirk needs to be enabled if host controller doesn't allow
-	 * that the interrupt aggregation timer and counter are reset by s/w.
-	 */
-	#define UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR		0x200
-
-	/*
-	 * This quirks needs to be enabled if host controller cannot be
-	 * enabled via HCE register.
-	 */
-	#define UFSHCI_QUIRK_BROKEN_HCE				0x400
 	unsigned int quirks;	/* Deviations from standard UFSHCI spec. */
 
 	/* Device deviations from standard UFS device spec. */
-- 
2.24.1

Re: [PATCH 1/2] ufshcd: remove unused quirks

[Bart Van Assche]

On 2020-02-21 06:08, Christoph Hellwig wrote:
> Remove various quirks that don't have users, as well as the dead code
> keyed off them.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 1/2] ufshcd: remove unused quirks

[Alim Akhtar]

On Fri, Feb 21, 2020 at 7:38 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Remove various quirks that don't have users, as well as the dead code
> keyed off them.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>


This does not make sense to remove it now and revert the patch a few week later.
Martin / Avri your thought please.


> ---
>  drivers/scsi/ufs/ufshcd.c | 119 ++++----------------------------------
>  drivers/scsi/ufs/ufshcd.h |  22 -------
>  2 files changed, 12 insertions(+), 129 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index abd0e6b05f79..886a8a597e6a 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -642,11 +642,7 @@ static inline int ufshcd_get_tr_ocs(struct ufshcd_lrb *lrbp)
>   */
>  static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
>  {
> -       if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> -               ufshcd_writel(hba, (1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> -       else
> -               ufshcd_writel(hba, ~(1 << pos),
> -                               REG_UTP_TRANSFER_REQ_LIST_CLEAR);
> +       ufshcd_writel(hba, ~(1 << pos), REG_UTP_TRANSFER_REQ_LIST_CLEAR);
>  }
>
>  /**
> @@ -656,10 +652,7 @@ static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
>   */
>  static inline void ufshcd_utmrl_clear(struct ufs_hba *hba, u32 pos)
>  {
> -       if (hba->quirks & UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR)
> -               ufshcd_writel(hba, (1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
> -       else
> -               ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
> +       ufshcd_writel(hba, ~(1 << pos), REG_UTP_TASK_REQ_LIST_CLEAR);
>  }
>
>  /**
> @@ -2093,13 +2086,8 @@ static int ufshcd_map_sg(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
>                 return sg_segments;
>
>         if (sg_segments) {
> -               if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN)
> -                       lrbp->utr_descriptor_ptr->prd_table_length =
> -                               cpu_to_le16((u16)(sg_segments *
> -                                       sizeof(struct ufshcd_sg_entry)));
> -               else
> -                       lrbp->utr_descriptor_ptr->prd_table_length =
> -                               cpu_to_le16((u16) (sg_segments));
> +               lrbp->utr_descriptor_ptr->prd_table_length =
> +                       cpu_to_le16((u16)sg_segments);
>
>                 prd_table = (struct ufshcd_sg_entry *)lrbp->ucd_prdt_ptr;
>
> @@ -3403,21 +3391,11 @@ static void ufshcd_host_memory_configure(struct ufs_hba *hba)
>                                 cpu_to_le32(upper_32_bits(cmd_desc_element_addr));
>
>                 /* Response upiu and prdt offset should be in double words */
> -               if (hba->quirks & UFSHCD_QUIRK_PRDT_BYTE_GRAN) {
> -                       utrdlp[i].response_upiu_offset =
> -                               cpu_to_le16(response_offset);
> -                       utrdlp[i].prd_table_offset =
> -                               cpu_to_le16(prdt_offset);
> -                       utrdlp[i].response_upiu_length =
> -                               cpu_to_le16(ALIGNED_UPIU_SIZE);
> -               } else {
> -                       utrdlp[i].response_upiu_offset =
> -                               cpu_to_le16((response_offset >> 2));
> -                       utrdlp[i].prd_table_offset =
> -                               cpu_to_le16((prdt_offset >> 2));
> -                       utrdlp[i].response_upiu_length =
> -                               cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
> -               }
> +               utrdlp[i].response_upiu_offset =
> +                       cpu_to_le16(response_offset >> 2);
> +               utrdlp[i].prd_table_offset = cpu_to_le16(prdt_offset >> 2);
> +               utrdlp[i].response_upiu_length =
> +                       cpu_to_le16(ALIGNED_UPIU_SIZE >> 2);
>
>                 hba->lrb[i].utr_descriptor_ptr = (utrdlp + i);
>                 hba->lrb[i].utrd_dma_addr = hba->utrdl_dma_addr +
> @@ -3460,52 +3438,6 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
>                         "dme-link-startup: error code %d\n", ret);
>         return ret;
>  }
> -/**
> - * ufshcd_dme_reset - UIC command for DME_RESET
> - * @hba: per adapter instance
> - *
> - * DME_RESET command is issued in order to reset UniPro stack.
> - * This function now deal with cold reset.
> - *
> - * Returns 0 on success, non-zero value on failure
> - */
> -static int ufshcd_dme_reset(struct ufs_hba *hba)
> -{
> -       struct uic_command uic_cmd = {0};
> -       int ret;
> -
> -       uic_cmd.command = UIC_CMD_DME_RESET;
> -
> -       ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> -       if (ret)
> -               dev_err(hba->dev,
> -                       "dme-reset: error code %d\n", ret);
> -
> -       return ret;
> -}
> -
> -/**
> - * ufshcd_dme_enable - UIC command for DME_ENABLE
> - * @hba: per adapter instance
> - *
> - * DME_ENABLE command is issued in order to enable UniPro stack.
> - *
> - * Returns 0 on success, non-zero value on failure
> - */
> -static int ufshcd_dme_enable(struct ufs_hba *hba)
> -{
> -       struct uic_command uic_cmd = {0};
> -       int ret;
> -
> -       uic_cmd.command = UIC_CMD_DME_ENABLE;
> -
> -       ret = ufshcd_send_uic_cmd(hba, &uic_cmd);
> -       if (ret)
> -               dev_err(hba->dev,
> -                       "dme-reset: error code %d\n", ret);
> -
> -       return ret;
> -}
>
>  static inline void ufshcd_add_delay_before_dme_cmd(struct ufs_hba *hba)
>  {
> @@ -4217,7 +4149,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
>  }
>
>  /**
> - * ufshcd_hba_execute_hce - initialize the controller
> + * ufshcd_hba_enable - initialize the controller
>   * @hba: per adapter instance
>   *
>   * The controller resets itself and controller firmware initialization
> @@ -4226,7 +4158,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba, bool can_sleep)
>   *
>   * Returns 0 on success, non-zero value on failure
>   */
> -static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
> +int ufshcd_hba_enable(struct ufs_hba *hba)
>  {
>         int retry;
>
> @@ -4274,32 +4206,6 @@ static int ufshcd_hba_execute_hce(struct ufs_hba *hba)
>
>         return 0;
>  }
> -
> -int ufshcd_hba_enable(struct ufs_hba *hba)
> -{
> -       int ret;
> -
> -       if (hba->quirks & UFSHCI_QUIRK_BROKEN_HCE) {
> -               ufshcd_set_link_off(hba);
> -               ufshcd_vops_hce_enable_notify(hba, PRE_CHANGE);
> -
> -               /* enable UIC related interrupts */
> -               ufshcd_enable_intr(hba, UFSHCD_UIC_MASK);
> -               ret = ufshcd_dme_reset(hba);
> -               if (!ret) {
> -                       ret = ufshcd_dme_enable(hba);
> -                       if (!ret)
> -                               ufshcd_vops_hce_enable_notify(hba, POST_CHANGE);
> -                       if (ret)
> -                               dev_err(hba->dev,
> -                                       "Host controller enable failed with non-hce\n");
> -               }
> -       } else {
> -               ret = ufshcd_hba_execute_hce(hba);
> -       }
> -
> -       return ret;
> -}
>  EXPORT_SYMBOL_GPL(ufshcd_hba_enable);
>
>  static int ufshcd_disable_tx_lcc(struct ufs_hba *hba, bool peer)
> @@ -4869,8 +4775,7 @@ static irqreturn_t ufshcd_transfer_req_compl(struct ufs_hba *hba)
>          * false interrupt if device completes another request after resetting
>          * aggregation and before reading the DB.
>          */
> -       if (ufshcd_is_intr_aggr_allowed(hba) &&
> -           !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
> +       if (ufshcd_is_intr_aggr_allowed(hba))
>                 ufshcd_reset_intr_aggr(hba);
>
>         tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 2ae6c7c8528c..9c2b80f87b9f 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -612,28 +612,6 @@ struct ufs_hba {
>          */
>         #define UFSHCD_QUIRK_BROKEN_UFS_HCI_VERSION             0x20
>
> -       /*
> -        * This quirk needs to be enabled if the host contoller regards
> -        * resolution of the values of PRDTO and PRDTL in UTRD as byte.
> -        */
> -       #define UFSHCD_QUIRK_PRDT_BYTE_GRAN                     0x80
> -
> -       /*
> -        * Clear handling for transfer/task request list is just opposite.
> -        */
> -       #define UFSHCI_QUIRK_BROKEN_REQ_LIST_CLR                0x100
> -
> -       /*
> -        * This quirk needs to be enabled if host controller doesn't allow
> -        * that the interrupt aggregation timer and counter are reset by s/w.
> -        */
> -       #define UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR               0x200
> -
> -       /*
> -        * This quirks needs to be enabled if host controller cannot be
> -        * enabled via HCE register.
> -        */
> -       #define UFSHCI_QUIRK_BROKEN_HCE                         0x400
>         unsigned int quirks;    /* Deviations from standard UFSHCI spec. */
>
>         /* Device deviations from standard UFS device spec. */
> --
> 2.24.1
>


-- 
Regards,
Alim

RE: [PATCH 1/2] ufshcd: remove unused quirks

[Avri Altman]

 
> 
> On Fri, Feb 21, 2020 at 7:38 PM Christoph Hellwig <hch@lst.de> wrote:
> >
> > Remove various quirks that don't have users, as well as the dead code
> > keyed off them.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> 
> This does not make sense to remove it now and revert the patch a few week
> later.
> Martin / Avri your thought please.
Yeah - I agree with you, provided that indeed it is just few weeks.

Thanks,
Avri