RE: [PATCH v1] ufs: introduce UFSHCI_QUIRK_SKIP_INTR_AGGR quirk
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
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
> > 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
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