From: Subhash Jadavani <subhashj@codeaurora.org>
To: Seungwon Jeon <tgih.jun@samsung.com>
Cc: linux-scsi@vger.kernel.org,
'Vinayak Holikatti' <vinholikatti@gmail.com>,
'Santosh Y' <santoshsy@gmail.com>,
"'James E.J. Bottomley'" <JBottomley@parallels.com>
Subject: Re: [PATCH 3/5] scsi: ufs: amend interrupt configuration
Date: Tue, 30 Apr 2013 17:20:58 +0530 [thread overview]
Message-ID: <517FB022.1060400@codeaurora.org> (raw)
In-Reply-To: <001f01ce4105$af6a3c20$0e3eb460$%jun@samsung.com>
Patch looks good but one minor comment below.
On 4/24/2013 9:36 PM, Seungwon Jeon wrote:
> It makes interrupt setting more flexible especially
> for disabling. And wrong bit mask is fixed for ver 1.0.
> [17:16] is added for mask.
>
> Signed-off-by: Seungwon Jeon <tgih.jun@samsung.com>
> ---
> drivers/scsi/ufs/ufshcd.c | 86 ++++++++++++++++++++++++++++++++-------------
> drivers/scsi/ufs/ufshcd.h | 4 +-
> drivers/scsi/ufs/ufshci.h | 5 ++-
> 3 files changed, 66 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index b6c19b0..efe2256 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -35,6 +35,10 @@
>
> #include "ufshcd.h"
>
> +#define UFSHCD_ENABLE_INTRS (UTP_TRANSFER_REQ_COMPL |\
> + UTP_TASK_REQ_COMPL |\
> + UFSHCD_ERROR_MASK)
> +
I don't see any use of this macro in this patch. could you please remove
it or move it to the patch-set where its really being used.
> enum {
> UFSHCD_MAX_CHANNEL = 0,
> UFSHCD_MAX_ID = 1,
> @@ -64,6 +68,20 @@ enum {
> };
>
> /**
> + * ufshcd_get_intr_mask - Get the interrupt bit mask
> + * @hba - Pointer to adapter instance
> + *
> + * Returns interrupt bit mask per version
> + */
> +static inline u32 ufshcd_get_intr_mask(struct ufs_hba *hba)
> +{
> + if (hba->ufs_version == UFSHCI_VERSION_10)
> + return INTERRUPT_MASK_ALL_VER_10;
> + else
> + return INTERRUPT_MASK_ALL_VER_11;
> +}
> +
> +/**
> * ufshcd_get_ufs_version - Get the UFS version supported by the HBA
> * @hba - Pointer to adapter instance
> *
> @@ -397,25 +415,45 @@ static int ufshcd_map_sg(struct ufshcd_lrb *lrbp)
> }
>
> /**
> - * ufshcd_int_config - enable/disable interrupts
> + * ufshcd_enable_intr - enable interrupts
> * @hba: per adapter instance
> - * @option: interrupt option
> + * @intrs: interrupt bits
> */
> -static void ufshcd_int_config(struct ufs_hba *hba, u32 option)
> +static void ufshcd_enable_intr(struct ufs_hba *hba, u32 intrs)
> {
> - switch (option) {
> - case UFSHCD_INT_ENABLE:
> - ufshcd_writel(hba, REG_INTERRUPT_ENABLE, hba->int_enable_mask);
> - break;
> - case UFSHCD_INT_DISABLE:
> - if (hba->ufs_version == UFSHCI_VERSION_10)
> - ufshcd_writel(hba, REG_INTERRUPT_ENABLE,
> - INTERRUPT_DISABLE_MASK_10);
> - else
> - ufshcd_writel(hba, REG_INTERRUPT_ENABLE,
> - INTERRUPT_DISABLE_MASK_11);
> - break;
> + u32 set = ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> +
> + if (hba->ufs_version == UFSHCI_VERSION_10) {
> + u32 rw;
> + rw = set & INTERRUPT_MASK_RW_VER_10;
> + set = rw | ((set ^ intrs) & intrs);
> + } else {
> + set |= intrs;
> + }
> +
> + ufshcd_writel(hba, REG_INTERRUPT_ENABLE, set);
> +}
> +
> +/**
> + * ufshcd_disable_intr - disable interrupts
> + * @hba: per adapter instance
> + * @intrs: interrupt bits
> + */
> +static void ufshcd_disable_intr(struct ufs_hba *hba, u32 intrs)
> +{
> + u32 set = ufshcd_readl(hba, REG_INTERRUPT_ENABLE);
> +
> + if (hba->ufs_version == UFSHCI_VERSION_10) {
> + u32 rw;
> + rw = (set & INTERRUPT_MASK_RW_VER_10) &
> + ~(intrs & INTERRUPT_MASK_RW_VER_10);
> + set = rw | ((set & intrs) & ~INTERRUPT_MASK_RW_VER_10);
> +
> + } else {
> + set &= ~intrs;
> }
> +
> + ufshcd_writel(hba, REG_INTERRUPT_ENABLE, set);
> }
>
> /**
> @@ -717,8 +755,7 @@ static int ufshcd_dme_link_startup(struct ufs_hba *hba)
> uic_cmd->argument3 = 0;
>
> /* enable UIC related interrupts */
> - hba->int_enable_mask |= UIC_COMMAND_COMPL;
> - ufshcd_int_config(hba, UFSHCD_INT_ENABLE);
> + ufshcd_enable_intr(hba, UIC_COMMAND_COMPL);
>
> /* sending UIC commands to controller */
> ufshcd_send_uic_command(hba, uic_cmd);
> @@ -765,13 +802,9 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba)
> }
>
> /* Enable required interrupts */
> - hba->int_enable_mask |= (UTP_TRANSFER_REQ_COMPL |
> - UIC_ERROR |
> - UTP_TASK_REQ_COMPL |
> - DEVICE_FATAL_ERROR |
> - CONTROLLER_FATAL_ERROR |
> - SYSTEM_BUS_FATAL_ERROR);
> - ufshcd_int_config(hba, UFSHCD_INT_ENABLE);
> + ufshcd_enable_intr(hba, UTP_TRANSFER_REQ_COMPL | UIC_ERROR |
> + UTP_TASK_REQ_COMPL | DEVICE_FATAL_ERROR |
> + CONTROLLER_FATAL_ERROR | SYSTEM_BUS_FATAL_ERROR);
>
> /* Configure interrupt aggregation */
> ufshcd_config_int_aggr(hba, INT_AGGR_CONFIG);
> @@ -1578,7 +1611,7 @@ static void ufshcd_hba_free(struct ufs_hba *hba)
> void ufshcd_remove(struct ufs_hba *hba)
> {
> /* disable interrupts */
> - ufshcd_int_config(hba, UFSHCD_INT_DISABLE);
> + ufshcd_disable_intr(hba, hba->intr_mask);
>
> ufshcd_hba_stop(hba);
> ufshcd_hba_free(hba);
> @@ -1636,6 +1669,9 @@ int ufshcd_init(struct device *dev, struct ufs_hba **hba_handle,
> /* Get UFS version supported by the controller */
> hba->ufs_version = ufshcd_get_ufs_version(hba);
>
> + /* Get Interrupt bit mask per version */
> + hba->intr_mask = ufshcd_get_intr_mask(hba);
> +
> /* Allocate memory for host memory space */
> err = ufshcd_memory_alloc(hba);
> if (err) {
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 6728450..87d5a94 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -139,7 +139,7 @@ struct ufshcd_lrb {
> * @ufshcd_tm_wait_queue: wait queue for task management
> * @tm_condition: condition variable for task management
> * @ufshcd_state: UFSHCD states
> - * @int_enable_mask: Interrupt Mask Bits
> + * @intr_mask: Interrupt Mask Bits
> * @uic_workq: Work queue for UIC completion handling
> * @feh_workq: Work queue for fatal controller error handling
> * @errors: HBA errors
> @@ -176,7 +176,7 @@ struct ufs_hba {
> unsigned long tm_condition;
>
> u32 ufshcd_state;
> - u32 int_enable_mask;
> + u32 intr_mask;
>
> /* Work Queues */
> struct work_struct uic_workq;
> diff --git a/drivers/scsi/ufs/ufshci.h b/drivers/scsi/ufs/ufshci.h
> index 0c16484..d5c5f14 100644
> --- a/drivers/scsi/ufs/ufshci.h
> +++ b/drivers/scsi/ufs/ufshci.h
> @@ -232,10 +232,11 @@ enum {
> /* Interrupt disable masks */
> enum {
> /* Interrupt disable mask for UFSHCI v1.0 */
> - INTERRUPT_DISABLE_MASK_10 = 0xFFFF,
> + INTERRUPT_MASK_ALL_VER_10 = 0x30FFF,
> + INTERRUPT_MASK_RW_VER_10 = 0x30000,
>
> /* Interrupt disable mask for UFSHCI v1.1 */
> - INTERRUPT_DISABLE_MASK_11 = 0x0,
> + INTERRUPT_MASK_ALL_VER_11 = 0x31FFF,
> };
>
> /*
next prev parent reply other threads:[~2013-04-30 11:51 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-24 16:06 [PATCH 3/5] scsi: ufs: amend interrupt configuration Seungwon Jeon
2013-04-30 11:50 ` Subhash Jadavani [this message]
2013-05-01 7:50 ` merez
2013-05-02 7:00 ` Seungwon Jeon
2013-05-04 8:45 ` [PATCH v2 3/7] " Seungwon Jeon
2013-05-06 10:40 ` merez
2013-05-06 14:22 ` James Bottomley
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=517FB022.1060400@codeaurora.org \
--to=subhashj@codeaurora.org \
--cc=JBottomley@parallels.com \
--cc=linux-scsi@vger.kernel.org \
--cc=santoshsy@gmail.com \
--cc=tgih.jun@samsung.com \
--cc=vinholikatti@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox