RE: [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk

2016-11-09 Thread Kiwoong Kim 
Hi, Subhash

I'll update this patch referring what you said (including adding comments of
other quirks)

> > If UFS driver resets interrupt aggregation timer and counter when
> > there is a pending doorbell, an interrupt of IO completion of a
> > corresponding task may be missed.
> > That would cause a command timeout.
> >
> > If UFS driver resets interrupt aggregation timer and counter when
> > there is a pending doorbell, a competion interrupt of a corresponing
> > task may be issued.
> > That would casue a command timeout.
> >
> > Signed-off-by: Kiwoong Kim 
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 4 +++-
> >  drivers/scsi/ufs/ufshcd.h | 1 +
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index c904854..65bbf59 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -3730,7 +3730,9 @@ static void 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))
> > +   if ((ufshcd_is_intr_aggr_allowed(hba))
> > +   && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))
> 
> Why do we need this new quirk? If controller has the issue with interrupt
> aggregation, you can remove this UFSHCD_CAP_INTR_AGGR to disable the
> aggregation altogether.
> 
> > +   ufshcd_reset_intr_aggr(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 6a96f24..6a9c6e9 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -497,6 +497,7 @@ struct ufs_hba {
> > #define UFSHCD_QUIRK_BROKEN_DWORD_UTRD  UFS_BIT(7)
> > #define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLRUFS_BIT(8)
> > #define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
> > +   #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)
> >
> >
> > unsigned int quirks;/* Deviations from standard UFSHCI spec.
> */
> 
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux Foundation Collaborative Project
> --
> 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 v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk

2016-11-09 Thread Subhash Jadavani 

On 2016-11-08 21:43, Kiwoong Kim wrote:

> If UFS driver resets interrupt aggregation timer and counter
> when there is a pending doorbell, an interrupt of IO completion
> of a corresponding task may be missed.
> That would cause a command timeout.
>
> If UFS driver resets interrupt aggregation timer and counter
> when there is a pending doorbell, a competion interrupt
> of a corresponing task may be issued.
> That would casue a command timeout.


Please add how are you fixing the issue in commit text.


>
> Signed-off-by: Kiwoong Kim 
> ---
>  drivers/scsi/ufs/ufshcd.c | 4 +++-
>  drivers/scsi/ufs/ufshcd.h | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index c904854..65bbf59 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -3730,7 +3730,9 @@ static void 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))
> +  if ((ufshcd_is_intr_aggr_allowed(hba))
> +  && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))

Why do we need this new quirk? If controller has the issue with
interrupt aggregation, you can remove this UFSHCD_CAP_INTR_AGGR to
disable the aggregation altogether.


This fragment doesn't mean disabling interrupt aggregation.
It just means that interrupt aggregation logic would not be reset in 
ISR.


ok.



But I think that the name of the quirk doesn't represent the function 
of it

expectedly.
I'll change the name with another one, such as
UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR.


May be UFSHCD_QUIRK_BROKEN_INTR_AGGR_RESET ?






> +  ufshcd_reset_intr_aggr(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 6a96f24..6a9c6e9 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -497,6 +497,7 @@ struct ufs_hba {
>#define UFSHCD_QUIRK_BROKEN_DWORD_UTRD  UFS_BIT(7)
>#define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLRUFS_BIT(8)
>#define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
> +  #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)


Please also add code comments explaining what this quirk means and when 
should we use it? Please add same for other quirks you introduced in 
different patches.



>
>
>unsigned int quirks;/* Deviations from standard UFSHCI spec.
*/

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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


--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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 v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk

2016-11-08 Thread Kiwoong Kim 
> > If UFS driver resets interrupt aggregation timer and counter
> > when there is a pending doorbell, an interrupt of IO completion
> > of a corresponding task may be missed.
> > That would cause a command timeout.
> >
> > If UFS driver resets interrupt aggregation timer and counter
> > when there is a pending doorbell, a competion interrupt
> > of a corresponing task may be issued.
> > That would casue a command timeout.
> >
> > Signed-off-by: Kiwoong Kim 
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 4 +++-
> >  drivers/scsi/ufs/ufshcd.h | 1 +
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index c904854..65bbf59 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -3730,7 +3730,9 @@ static void 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))
> > +   if ((ufshcd_is_intr_aggr_allowed(hba))
> > +   && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))
> 
> Why do we need this new quirk? If controller has the issue with
> interrupt aggregation, you can remove this UFSHCD_CAP_INTR_AGGR to
> disable the aggregation altogether.

This fragment doesn't mean disabling interrupt aggregation.
It just means that interrupt aggregation logic would not be reset in ISR.

But I think that the name of the quirk doesn't represent the function of it
expectedly.
I'll change the name with another one, such as
UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR.


> 
> > +   ufshcd_reset_intr_aggr(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 6a96f24..6a9c6e9 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -497,6 +497,7 @@ struct ufs_hba {
> > #define UFSHCD_QUIRK_BROKEN_DWORD_UTRD  UFS_BIT(7)
> > #define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLRUFS_BIT(8)
> > #define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
> > +   #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)
> >
> >
> > unsigned int quirks;/* Deviations from standard UFSHCI spec.
> */
> 
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project
> --
> 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 v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk

2016-11-08 Thread Subhash Jadavani 

On 2016-11-07 23:59, Kiwoong Kim wrote:

If UFS driver resets interrupt aggregation timer and counter
when there is a pending doorbell, an interrupt of IO completion
of a corresponding task may be missed.
That would cause a command timeout.

If UFS driver resets interrupt aggregation timer and counter
when there is a pending doorbell, a competion interrupt
of a corresponing task may be issued.
That would casue a command timeout.

Signed-off-by: Kiwoong Kim 
---
 drivers/scsi/ufs/ufshcd.c | 4 +++-
 drivers/scsi/ufs/ufshcd.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c904854..65bbf59 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3730,7 +3730,9 @@ static void 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))
+   if ((ufshcd_is_intr_aggr_allowed(hba))
+   && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))


Why do we need this new quirk? If controller has the issue with 
interrupt aggregation, you can remove this UFSHCD_CAP_INTR_AGGR to 
disable the aggregation altogether.



+   ufshcd_reset_intr_aggr(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 6a96f24..6a9c6e9 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -497,6 +497,7 @@ struct ufs_hba {
#define UFSHCD_QUIRK_BROKEN_DWORD_UTRD  UFS_BIT(7)
#define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLRUFS_BIT(8)
#define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
+   #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)


unsigned int quirks;/* Deviations from standard UFSHCI spec. */


--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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

[PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk

2016-11-08 Thread Kiwoong Kim 
If UFS driver resets interrupt aggregation timer and counter
when there is a pending doorbell, an interrupt of IO completion
of a corresponding task may be missed.
That would cause a command timeout.

If UFS driver resets interrupt aggregation timer and counter
when there is a pending doorbell, a competion interrupt
of a corresponing task may be issued.
That would casue a command timeout.

Signed-off-by: Kiwoong Kim 
---
 drivers/scsi/ufs/ufshcd.c | 4 +++-
 drivers/scsi/ufs/ufshcd.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c904854..65bbf59 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -3730,7 +3730,9 @@ static void 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))
+   if ((ufshcd_is_intr_aggr_allowed(hba))
+   && !(hba->quirks & UFSHCI_QUIRK_SKIP_INTR_AGGR))
+   ufshcd_reset_intr_aggr(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 6a96f24..6a9c6e9 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -497,6 +497,7 @@ struct ufs_hba {
#define UFSHCD_QUIRK_BROKEN_DWORD_UTRD  UFS_BIT(7)
#define UFSHCD_QUIRK_BROKEN_REQ_LIST_CLRUFS_BIT(8)
#define UFSHCD_QUIRK_USE_OF_HCE UFS_BIT(9)
+   #define UFSHCI_QUIRK_SKIP_INTR_AGGR UFS_BIT(10)
 
 
unsigned int quirks;/* Deviations from standard UFSHCI spec. */
-- 
2.1.4

--
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