ufshcd quirk cleanup v2

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

[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

[Asutosh Das (asd)]

Hi Christoph

On 2/21/2020 6:08 AM, Christoph Hellwig wrote:
> 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,
> +

How about using BIT() here?
> +	/*
> +	 * 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. */
> 

-asd

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

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

[Bart Van Assche]

On 2020-02-21 10:18, Asutosh Das (asd) wrote:
> On 2/21/2020 6:08 AM, Christoph Hellwig wrote:
>> +    /* Interrupt aggregation support is broken */
>> +    UFSHCD_QUIRK_BROKEN_INTR_AGGR            = 1 << 0,
>> +
> 
> How about using BIT() here?

Not everyone is convinced that using BIT() improves code readability.

Bart.

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 2/2] ufshcd: use an enum for quirks

[Bart Van Assche]

On 2020-02-21 06:08, Christoph Hellwig wrote:
> Use an enum to specify the various quirks instead of #defines inside
> the structure definition.

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

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

[Christoph Hellwig]

On Fri, Feb 21, 2020 at 06:45:37PM -0800, Bart Van Assche wrote:
> On 2020-02-21 10:18, Asutosh Das (asd) wrote:
> > On 2/21/2020 6:08 AM, Christoph Hellwig wrote:
> >> +    /* Interrupt aggregation support is broken */
> >> +    UFSHCD_QUIRK_BROKEN_INTR_AGGR            = 1 << 0,
> >> +
> > 
> > How about using BIT() here?
> 
> Not everyone is convinced that using BIT() improves code readability.

I for one am not. 1 << N shoud be obvious to anyone with a basic
understanding of C code, BIT() needs to be looked up.  And it isn't
actually any shorter.

Re: ufshcd quirk cleanup v2

[Martin K. Petersen]

Christoph,

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

Applied to 5.7/scsi-queue, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

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.

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

[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

[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

[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

[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

[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

[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