[PATCH 1/3] scsi: ufs: Change HCI marco to actual bit position

[PATCH 1/3] scsi: ufs: Change HCI marco to actual bit position

[Alim Akhtar]

Currently UFS HCI uses UFS_BIT() macro to get various bit
position for the hardware registers status bits. Which makes
code longer instead of shorter. This macro does not improve
code readability as well.
Lets re-write these macro definition with the actual bit position.

Suggested-by: Bart Van Assche <Bart.VanAssche@wdc.com>
Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 This patch is only complied tested, appreciate testing on actual h/w.
 Please see https://lkml.org/lkml/2017/8/28/786  discussion.
 
 drivers/scsi/ufs/ufshcd.h | 14 +++++-----
 drivers/scsi/ufs/ufshci.h | 69 ++++++++++++++++++++++++-----------------------
 2 files changed, 43 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index cdc8bd0..ce2920b 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -544,13 +544,13 @@ struct ufs_hba {
 	bool is_irq_enabled;
 
 	/* Interrupt aggregation support is broken */
-	#define UFSHCD_QUIRK_BROKEN_INTR_AGGR			UFS_BIT(0)
+	#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		UFS_BIT(1)
+	#define UFSHCD_QUIRK_DELAY_BEFORE_DME_CMDS		0x2
 
 	/*
 	 * If UFS host controller is having issue in processing LCC (Line
@@ -559,21 +559,21 @@ struct ufs_hba {
 	 * the LCC transmission on UFS device (by clearing TX_LCC_ENABLE
 	 * attribute of device to 0).
 	 */
-	#define UFSHCD_QUIRK_BROKEN_LCC				UFS_BIT(2)
+	#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		UFS_BIT(3)
+	#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		UFS_BIT(4)
+	#define UFSHCD_QUIRK_DME_PEER_ACCESS_AUTO_MODE		0x10
 
 	/*
 	 * This quirk needs to be enabled if the host contoller doesn't
@@ -581,13 +581,13 @@ struct ufs_hba {
 	 * 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		UFS_BIT(5)
+	#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			UFS_BIT(7)
+	#define UFSHCD_QUIRK_PRDT_BYTE_GRAN			0x80
 
 	unsigned int quirks;	/* Deviations from standard UFSHCI spec. */
 
diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index f60145d..5a60a8f 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -119,22 +119,24 @@ enum {
 #define MANUFACTURE_ID_MASK	UFS_MASK(0xFFFF, 0)
 #define PRODUCT_ID_MASK		UFS_MASK(0xFFFF, 16)
 
-#define UFS_BIT(x)	(1L << (x))
-
-#define UTP_TRANSFER_REQ_COMPL			UFS_BIT(0)
-#define UIC_DME_END_PT_RESET			UFS_BIT(1)
-#define UIC_ERROR				UFS_BIT(2)
-#define UIC_TEST_MODE				UFS_BIT(3)
-#define UIC_POWER_MODE				UFS_BIT(4)
-#define UIC_HIBERNATE_EXIT			UFS_BIT(5)
-#define UIC_HIBERNATE_ENTER			UFS_BIT(6)
-#define UIC_LINK_LOST				UFS_BIT(7)
-#define UIC_LINK_STARTUP			UFS_BIT(8)
-#define UTP_TASK_REQ_COMPL			UFS_BIT(9)
-#define UIC_COMMAND_COMPL			UFS_BIT(10)
-#define DEVICE_FATAL_ERROR			UFS_BIT(11)
-#define CONTROLLER_FATAL_ERROR			UFS_BIT(16)
-#define SYSTEM_BUS_FATAL_ERROR			UFS_BIT(17)
+#define UFS_BIT(x)     (1L << (x))
+/*
+ * IS - Interrupt Status - 20h
+ */
+#define UTP_TRANSFER_REQ_COMPL			0x1
+#define UIC_DME_END_PT_RESET			0x2
+#define UIC_ERROR				0x4
+#define UIC_TEST_MODE				0x8
+#define UIC_POWER_MODE				0x10
+#define UIC_HIBERNATE_EXIT			0x20
+#define UIC_HIBERNATE_ENTER			0x40
+#define UIC_LINK_LOST				0x80
+#define UIC_LINK_STARTUP			0x100
+#define UTP_TASK_REQ_COMPL			0x200
+#define UIC_COMMAND_COMPL			0x400
+#define DEVICE_FATAL_ERROR			0x800
+#define CONTROLLER_FATAL_ERROR			0x10000
+#define SYSTEM_BUS_FATAL_ERROR			0x20000
 
 #define UFSHCD_UIC_PWR_MASK	(UIC_HIBERNATE_ENTER |\
 				UIC_HIBERNATE_EXIT |\
@@ -152,10 +154,10 @@ enum {
 				SYSTEM_BUS_FATAL_ERROR)
 
 /* HCS - Host Controller Status 30h */
-#define DEVICE_PRESENT				UFS_BIT(0)
-#define UTP_TRANSFER_REQ_LIST_READY		UFS_BIT(1)
-#define UTP_TASK_REQ_LIST_READY			UFS_BIT(2)
-#define UIC_COMMAND_READY			UFS_BIT(3)
+#define DEVICE_PRESENT				0x1
+#define UTP_TRANSFER_REQ_LIST_READY		0x2
+#define UTP_TASK_REQ_LIST_READY			0x4
+#define UIC_COMMAND_READY			0x8
 #define HOST_ERROR_INDICATOR			UFS_BIT(4)
 #define DEVICE_ERROR_INDICATOR			UFS_BIT(5)
 #define UIC_POWER_MODE_CHANGE_REQ_STATUS_MASK	UFS_MASK(0x7, 8)
@@ -174,46 +176,47 @@ enum {
 };
 
 /* HCE - Host Controller Enable 34h */
-#define CONTROLLER_ENABLE	UFS_BIT(0)
+#define CONTROLLER_ENABLE	0x1
 #define CONTROLLER_DISABLE	0x0
-#define CRYPTO_GENERAL_ENABLE	UFS_BIT(1)
+#define CRYPTO_GENERAL_ENABLE	0x2
 
 /* UECPA - Host UIC Error Code PHY Adapter Layer 38h */
-#define UIC_PHY_ADAPTER_LAYER_ERROR			UFS_BIT(31)
+#define UIC_PHY_ADAPTER_LAYER_ERROR			0x80000000
 #define UIC_PHY_ADAPTER_LAYER_ERROR_CODE_MASK		0x1F
 #define UIC_PHY_ADAPTER_LAYER_LANE_ERR_MASK		0xF
 
 /* UECDL - Host UIC Error Code Data Link Layer 3Ch */
-#define UIC_DATA_LINK_LAYER_ERROR		UFS_BIT(31)
+#define UIC_DATA_LINK_LAYER_ERROR		0x80000000
 #define UIC_DATA_LINK_LAYER_ERROR_CODE_MASK	0x7FFF
 #define UIC_DATA_LINK_LAYER_ERROR_PA_INIT	0x2000
 #define UIC_DATA_LINK_LAYER_ERROR_NAC_RECEIVED	0x0001
 #define UIC_DATA_LINK_LAYER_ERROR_TCx_REPLAY_TIMEOUT 0x0002
 
 /* UECN - Host UIC Error Code Network Layer 40h */
-#define UIC_NETWORK_LAYER_ERROR			UFS_BIT(31)
+#define UIC_NETWORK_LAYER_ERROR			0x80000000
 #define UIC_NETWORK_LAYER_ERROR_CODE_MASK	0x7
 
 /* UECT - Host UIC Error Code Transport Layer 44h */
-#define UIC_TRANSPORT_LAYER_ERROR		UFS_BIT(31)
+#define UIC_TRANSPORT_LAYER_ERROR		0x80000000
 #define UIC_TRANSPORT_LAYER_ERROR_CODE_MASK	0x7F
 
 /* UECDME - Host UIC Error Code DME 48h */
-#define UIC_DME_ERROR			UFS_BIT(31)
+#define UIC_DME_ERROR			0x80000000
 #define UIC_DME_ERROR_CODE_MASK		0x1
 
+/* UTRIACR - Interrupt Aggregation control register - 0x4Ch */
 #define INT_AGGR_TIMEOUT_VAL_MASK		0xFF
 #define INT_AGGR_COUNTER_THRESHOLD_MASK		UFS_MASK(0x1F, 8)
-#define INT_AGGR_COUNTER_AND_TIMER_RESET	UFS_BIT(16)
-#define INT_AGGR_STATUS_BIT			UFS_BIT(20)
-#define INT_AGGR_PARAM_WRITE			UFS_BIT(24)
-#define INT_AGGR_ENABLE				UFS_BIT(31)
+#define INT_AGGR_COUNTER_AND_TIMER_RESET	0x10000
+#define INT_AGGR_STATUS_BIT			0x100000
+#define INT_AGGR_PARAM_WRITE			0x1000000
+#define INT_AGGR_ENABLE				0x80000000
 
 /* UTRLRSR - UTP Transfer Request Run-Stop Register 60h */
-#define UTP_TRANSFER_REQ_LIST_RUN_STOP_BIT	UFS_BIT(0)
+#define UTP_TRANSFER_REQ_LIST_RUN_STOP_BIT	0x1
 
 /* UTMRLRSR - UTP Task Management Request Run-Stop Register 80h */
-#define UTP_TASK_REQ_LIST_RUN_STOP_BIT		UFS_BIT(0)
+#define UTP_TASK_REQ_LIST_RUN_STOP_BIT		0x1
 
 /* UICCMD - UIC Command */
 #define COMMAND_OPCODE_MASK		0xFF
-- 
2.7.4

[PATCH 2/3] scsi: ufs-qcom: Remove uses of UFS_BIT() macro

[Alim Akhtar]

Use actaul bit position instead of UFS_BIT() macro. This
patch also changes bit-17 to meaningful #define.

This change is as per discussion here [1]
[1] -> https://lkml.org/lkml/2017/8/28/786

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
Cc: Subhash Jadavani <subhashj@codeaurora.org>
---

 This patch is only complied tested, appreciate testing on actual h/w

 drivers/scsi/ufs/ufs-qcom.c | 4 ++--
 drivers/scsi/ufs/ufs-qcom.h | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/ufs/ufs-qcom.c b/drivers/scsi/ufs/ufs-qcom.c
index c87d770..6a548e7 100644
--- a/drivers/scsi/ufs/ufs-qcom.c
+++ b/drivers/scsi/ufs/ufs-qcom.c
@@ -1458,7 +1458,7 @@ static void ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba,
 	print_fn(hba, reg, 44, "UFS_UFS_DBG_RD_REG_OCSC ", priv);
 
 	reg = ufshcd_readl(hba, REG_UFS_CFG1);
-	reg |= UFS_BIT(17);
+	reg |= UTP_DBG_RAMS_EN;
 	ufshcd_writel(hba, reg, REG_UFS_CFG1);
 
 	reg = ufs_qcom_get_debug_reg_offset(host, UFS_UFS_DBG_RD_EDTL_RAM);
@@ -1471,7 +1471,7 @@ static void ufs_qcom_print_hw_debug_reg_all(struct ufs_hba *hba,
 	print_fn(hba, reg, 64, "UFS_UFS_DBG_RD_PRDT_RAM ", priv);
 
 	/* clear bit 17 - UTP_DBG_RAMS_EN */
-	ufshcd_rmwl(hba, UFS_BIT(17), 0, REG_UFS_CFG1);
+	ufshcd_rmwl(hba, UTP_DBG_RAMS_EN, 0, REG_UFS_CFG1);
 
 	reg = ufs_qcom_get_debug_reg_offset(host, UFS_DBG_RD_REG_UAWM);
 	print_fn(hba, reg, 4, "UFS_DBG_RD_REG_UAWM ", priv);
diff --git a/drivers/scsi/ufs/ufs-qcom.h b/drivers/scsi/ufs/ufs-qcom.h
index 076f528..295f4be 100644
--- a/drivers/scsi/ufs/ufs-qcom.h
+++ b/drivers/scsi/ufs/ufs-qcom.h
@@ -92,7 +92,8 @@ enum {
 #define UFS_CNTLR_3_x_x_VEN_REGS_OFFSET(x)	(0x400 + x)
 
 /* bit definitions for REG_UFS_CFG1 register */
-#define QUNIPRO_SEL	UFS_BIT(0)
+#define QUNIPRO_SEL		0x1
+#define UTP_DBG_RAMS_EN		0x20000
 #define TEST_BUS_EN		BIT(18)
 #define TEST_BUS_SEL		GENMASK(22, 19)
 #define UFS_REG_TEST_BUS_EN	BIT(30)
@@ -213,13 +214,13 @@ struct ufs_qcom_host {
 	 * Note: By default this capability will be kept enabled if host
 	 * controller supports the QUniPro mode.
 	 */
-	#define UFS_QCOM_CAP_QUNIPRO	UFS_BIT(0)
+	#define UFS_QCOM_CAP_QUNIPRO	0x1
 
 	/*
 	 * Set this capability if host controller can retain the secure
 	 * configuration even after UFS controller core power collapse.
 	 */
-	#define UFS_QCOM_CAP_RETAIN_SEC_CFG_AFTER_PWR_COLLAPSE	UFS_BIT(1)
+	#define UFS_QCOM_CAP_RETAIN_SEC_CFG_AFTER_PWR_COLLAPSE	0x2
 	u32 caps;
 
 	struct phy *generic_phy;
-- 
2.7.4

[PATCH 3/3] scsi: ufs: Remove unused UFS_BIT() macro

[Alim Akhtar]

Since we have coverted all the user of UFS_BIT() macro
with the actual bit position, let remove unused UFS_BIT()macro.

Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
---
 drivers/scsi/ufs/ufshci.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
index 5a60a8f..c51fa6d 100644
--- a/drivers/scsi/ufs/ufshci.h
+++ b/drivers/scsi/ufs/ufshci.h
@@ -119,7 +119,6 @@ enum {
 #define MANUFACTURE_ID_MASK	UFS_MASK(0xFFFF, 0)
 #define PRODUCT_ID_MASK		UFS_MASK(0xFFFF, 16)
 
-#define UFS_BIT(x)     (1L << (x))
 /*
  * IS - Interrupt Status - 20h
  */
-- 
2.7.4

Re: [PATCH 1/3] scsi: ufs: Change HCI marco to actual bit position

[Bart Van Assche]

On Sat, 2017-09-30 at 11:52 +0530, Alim Akhtar wrote:
> Currently UFS HCI uses UFS_BIT() macro to get various bit
> position for the hardware registers status bits. Which makes
> code longer instead of shorter. This macro does not improve
> code readability as well.
> Lets re-write these macro definition with the actual bit position.

Can the three patches in this series be combined into a single patch?

> -#define UFS_BIT(x)	(1L << (x))
> [ ... ]
> +#define UFS_BIT(x)     (1L << (x))

If you want to keep the three patches separate, please make sure that this
patch does not modify the UFS_BIT() definition. Please also fix the spelling
of the title of this patch (marco -> macro).

Thanks,

Bart.

Re: [PATCH 1/3] scsi: ufs: Change HCI marco to actual bit position

[Alim Akhtar]

Hi Bart,

On 10/02/2017 10:11 PM, Bart Van Assche wrote:
> On Sat, 2017-09-30 at 11:52 +0530, Alim Akhtar wrote:
>> Currently UFS HCI uses UFS_BIT() macro to get various bit
>> position for the hardware registers status bits. Which makes
>> code longer instead of shorter. This macro does not improve
>> code readability as well.
>> Lets re-write these macro definition with the actual bit position.
> 
> Can the three patches in this series be combined into a single patch?
> 
Since UFS_BIT() used in qcom-ufs driver as well, so I thought of keeping 
them in separate patches. This way "git bisect" will work all the time.

>> -#define UFS_BIT(x)	(1L << (x))
>> [ ... ]
>> +#define UFS_BIT(x)     (1L << (x))
> 
> If you want to keep the three patches separate, please make sure that this
> patch does not modify the UFS_BIT() definition. Please also fix the spelling
> of the title of this patch (marco -> macro).
> 
Ok, will fix these
> Thanks,
> 
> Bart.
>