RE: [PATCH 2/5] scsi: ufs: wrap the i/o access operations
On Wednesday, May 01, 2013, merez wrote: Tested-by: Maya Erez me...@codeaurora.org I also tend to agree with Sujit on the order of the wrappers parameters. Okay, I'll take the views of all(Sujit, Subhash and you) Thanks, Seungwon Jeon Thanks, Maya On 4/26/2013 10:36 AM, Seungwon Jeon wrote: Hi, On Thursday, April 25, 2013, Sujit Reddy Thumma wrote: On 4/24/2013 9:36 PM, Seungwon Jeon wrote: Simplify operations with hiding mmio_base. Signed-off-by: Seungwon Jeon tgih@samsung.com --- drivers/scsi/ufs/ufshcd.c | 106 +++-- drivers/scsi/ufs/ufshcd.h |5 ++ 2 files changed, 49 insertions(+), 62 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 1680394..6728450 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -190,4 +190,9 @@ int ufshcd_init(struct device *, struct ufs_hba ** , void __iomem * , unsigned int); void ufshcd_remove(struct ufs_hba *); +#define ufshcd_writel(hba, reg, val)\ Let this be consistent with writel() arguments - val as second arg and reg as third? You got a point there. When considering an array of arguments in two functions and value part can be some long expression, I think it seems more coherent. ufshcd_readl(hba, reg); ufshcd_writel(hba, reg, val); How about keeping these? I somehow tend to agree with what Sujit suggested. Its good to be consitent with writel() for better code readability. Thanks, Seungwon Jeon +writel((val), (hba)-mmio_base + (reg)) +#define ufshcd_readl(hba, reg) \ +readl((hba)-mmio_base + (reg)) + #endif /* End of Header */ -- Regards, Sujit -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Maya Erez QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] scsi: ufs: wrap the i/o access operations
Tested-by: Maya Erez me...@codeaurora.org I also tend to agree with Sujit on the order of the wrappers parameters. Thanks, Maya On 4/26/2013 10:36 AM, Seungwon Jeon wrote: Hi, On Thursday, April 25, 2013, Sujit Reddy Thumma wrote: On 4/24/2013 9:36 PM, Seungwon Jeon wrote: Simplify operations with hiding mmio_base. Signed-off-by: Seungwon Jeon tgih@samsung.com --- drivers/scsi/ufs/ufshcd.c | 106 +++-- drivers/scsi/ufs/ufshcd.h |5 ++ 2 files changed, 49 insertions(+), 62 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 1680394..6728450 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -190,4 +190,9 @@ int ufshcd_init(struct device *, struct ufs_hba ** , void __iomem * , unsigned int); void ufshcd_remove(struct ufs_hba *); +#define ufshcd_writel(hba, reg, val) \ Let this be consistent with writel() arguments - val as second arg and reg as third? You got a point there. When considering an array of arguments in two functions and value part can be some long expression, I think it seems more coherent. ufshcd_readl(hba, reg); ufshcd_writel(hba, reg, val); How about keeping these? I somehow tend to agree with what Sujit suggested. Its good to be consitent with writel() for better code readability. Thanks, Seungwon Jeon + writel((val), (hba)-mmio_base + (reg)) +#define ufshcd_readl(hba, reg)\ + readl((hba)-mmio_base + (reg)) + #endif /* End of Header */ -- Regards, Sujit -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Maya Erez QUALCOMM ISRAEL, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] scsi: ufs: wrap the i/o access operations
On 4/26/2013 10:36 AM, Seungwon Jeon wrote: Hi, On Thursday, April 25, 2013, Sujit Reddy Thumma wrote: On 4/24/2013 9:36 PM, Seungwon Jeon wrote: Simplify operations with hiding mmio_base. Signed-off-by: Seungwon Jeon tgih@samsung.com --- drivers/scsi/ufs/ufshcd.c | 106 +++-- drivers/scsi/ufs/ufshcd.h |5 ++ 2 files changed, 49 insertions(+), 62 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 1680394..6728450 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -190,4 +190,9 @@ int ufshcd_init(struct device *, struct ufs_hba ** , void __iomem * , unsigned int); void ufshcd_remove(struct ufs_hba *); +#define ufshcd_writel(hba, reg, val) \ Let this be consistent with writel() arguments - val as second arg and reg as third? You got a point there. When considering an array of arguments in two functions and value part can be some long expression, I think it seems more coherent. ufshcd_readl(hba, reg); ufshcd_writel(hba, reg, val); How about keeping these? I somehow tend to agree with what Sujit suggested. Its good to be consitent with writel() for better code readability. Thanks, Seungwon Jeon + writel((val), (hba)-mmio_base + (reg)) +#define ufshcd_readl(hba, reg) \ + readl((hba)-mmio_base + (reg)) + #endif /* End of Header */ -- Regards, Sujit -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/5] scsi: ufs: wrap the i/o access operations
Hi, On Thursday, April 25, 2013, Sujit Reddy Thumma wrote: On 4/24/2013 9:36 PM, Seungwon Jeon wrote: Simplify operations with hiding mmio_base. Signed-off-by: Seungwon Jeon tgih@samsung.com --- drivers/scsi/ufs/ufshcd.c | 106 +++-- drivers/scsi/ufs/ufshcd.h |5 ++ 2 files changed, 49 insertions(+), 62 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index 1680394..6728450 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -190,4 +190,9 @@ int ufshcd_init(struct device *, struct ufs_hba ** , void __iomem * , unsigned int); void ufshcd_remove(struct ufs_hba *); +#define ufshcd_writel(hba, reg, val) \ Let this be consistent with writel() arguments - val as second arg and reg as third? You got a point there. When considering an array of arguments in two functions and value part can be some long expression, I think it seems more coherent. ufshcd_readl(hba, reg); ufshcd_writel(hba, reg, val); How about keeping these? Thanks, Seungwon Jeon + writel((val), (hba)-mmio_base + (reg)) +#define ufshcd_readl(hba, reg) \ + readl((hba)-mmio_base + (reg)) + #endif /* End of Header */ -- Regards, Sujit -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe linux-scsi in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/5] scsi: ufs: wrap the i/o access operations
On 4/24/2013 9:36 PM, Seungwon Jeon wrote:
Simplify operations with hiding mmio_base.
Signed-off-by: Seungwon Jeon tgih@samsung.com
---
drivers/scsi/ufs/ufshcd.c | 106 +++--
drivers/scsi/ufs/ufshcd.h |5 ++
2 files changed, 49 insertions(+), 62 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 41b9639..b6c19b0 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -71,7 +71,7 @@ enum {
*/
static inline u32 ufshcd_get_ufs_version(struct ufs_hba *hba)
{
- return readl(hba-mmio_base + REG_UFS_VERSION);
+ return ufshcd_readl(hba, REG_UFS_VERSION);
}
/**
@@ -130,8 +130,7 @@ static inline int ufshcd_get_tm_free_slot(struct ufs_hba
*hba)
*/
static inline void ufshcd_utrl_clear(struct ufs_hba *hba, u32 pos)
{
- writel(~(1 pos),
- (hba-mmio_base + REG_UTP_TRANSFER_REQ_LIST_CLEAR));
+ ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_CLEAR, ~(1 pos));
}
/**
@@ -165,7 +164,7 @@ static inline int ufshcd_get_lists_status(u32 reg)
*/
static inline int ufshcd_get_uic_cmd_result(struct ufs_hba *hba)
{
- return readl(hba-mmio_base + REG_UIC_COMMAND_ARG_2)
+ return ufshcd_readl(hba, REG_UIC_COMMAND_ARG_2)
MASK_UIC_COMMAND_RESULT;
}
@@ -243,18 +242,14 @@ ufshcd_config_int_aggr(struct ufs_hba *hba, int option)
{
switch (option) {
case INT_AGGR_RESET:
- writel((INT_AGGR_ENABLE |
- INT_AGGR_COUNTER_AND_TIMER_RESET),
- (hba-mmio_base +
-REG_UTP_TRANSFER_REQ_INT_AGG_CONTROL));
+ ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_INT_AGG_CONTROL,
+ INT_AGGR_ENABLE |
INT_AGGR_COUNTER_AND_TIMER_RESET);
break;
case INT_AGGR_CONFIG:
- writel((INT_AGGR_ENABLE |
- INT_AGGR_PARAM_WRITE |
- INT_AGGR_COUNTER_THRESHOLD_VALUE |
- INT_AGGR_TIMEOUT_VALUE),
- (hba-mmio_base +
-REG_UTP_TRANSFER_REQ_INT_AGG_CONTROL));
+ ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_INT_AGG_CONTROL,
+ INT_AGGR_ENABLE | INT_AGGR_PARAM_WRITE |
+ INT_AGGR_COUNTER_THRESHOLD_VALUE |
+ INT_AGGR_TIMEOUT_VALUE);
break;
}
}
@@ -267,12 +262,10 @@ ufshcd_config_int_aggr(struct ufs_hba *hba, int option)
*/
static void ufshcd_enable_run_stop_reg(struct ufs_hba *hba)
{
- writel(UTP_TASK_REQ_LIST_RUN_STOP_BIT,
- (hba-mmio_base +
- REG_UTP_TASK_REQ_LIST_RUN_STOP));
- writel(UTP_TRANSFER_REQ_LIST_RUN_STOP_BIT,
- (hba-mmio_base +
- REG_UTP_TRANSFER_REQ_LIST_RUN_STOP));
+ ufshcd_writel(hba, REG_UTP_TASK_REQ_LIST_RUN_STOP,
+ UTP_TASK_REQ_LIST_RUN_STOP_BIT);
+ ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_LIST_RUN_STOP,
+ UTP_TRANSFER_REQ_LIST_RUN_STOP_BIT);
}
/**
@@ -281,7 +274,7 @@ static void ufshcd_enable_run_stop_reg(struct ufs_hba *hba)
*/
static inline void ufshcd_hba_start(struct ufs_hba *hba)
{
- writel(CONTROLLER_ENABLE , (hba-mmio_base + REG_CONTROLLER_ENABLE));
+ ufshcd_writel(hba, REG_CONTROLLER_ENABLE, CONTROLLER_ENABLE);
}
/**
@@ -290,7 +283,7 @@ static inline void ufshcd_hba_start(struct ufs_hba *hba)
*/
static inline void ufshcd_hba_stop(struct ufs_hba *hba)
{
- writel(CONTROLLER_DISABLE, (hba-mmio_base + REG_CONTROLLER_ENABLE));
+ ufshcd_writel(hba, REG_CONTROLLER_ENABLE, CONTROLLER_DISABLE);
}
/**
@@ -301,7 +294,7 @@ static inline void ufshcd_hba_stop(struct ufs_hba *hba)
*/
static inline int ufshcd_is_hba_active(struct ufs_hba *hba)
{
- return (readl(hba-mmio_base + REG_CONTROLLER_ENABLE) 0x1) ? 0 : 1;
+ return (ufshcd_readl(hba, REG_CONTROLLER_ENABLE) 0x1) ? 0 : 1;
}
/**
@@ -313,8 +306,7 @@ static inline
void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
{
__set_bit(task_tag, hba-outstanding_reqs);
- writel((1 task_tag),
- (hba-mmio_base + REG_UTP_TRANSFER_REQ_DOOR_BELL));
+ ufshcd_writel(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL, 1 task_tag);
}
/**
@@ -338,8 +330,7 @@ static inline void ufshcd_copy_sense_data(struct ufshcd_lrb
*lrbp)
*/
static inline void ufshcd_hba_capabilities(struct ufs_hba *hba)
{
- hba-capabilities =
- readl(hba-mmio_base + REG_CONTROLLER_CAPABILITIES);
+ hba-capabilities = ufshcd_readl(hba, REG_CONTROLLER_CAPABILITIES);
/* nutrs and nutmrs are 0 based values */
hba-nutrs = (hba-capabilities MASK_TRANSFER_REQUESTS_SLOTS) + 1;
@@ -356,16 +347,13 @@ static inline void
ufshcd_send_uic_command(struct ufs_hba *hba, struct uic_command
