| Message ID | 1441188795-4600-14-git-send-email-ygardi@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>: > Performing several writes to UFS host controller registers has > no gurrantee of ordering, so we must make sure register writes > to setup request list base address etc. are performed before the > run/stop register is enabled. > In addition, when setting up a task request, we must make sure > the updating of descriptors takes places before ringing the > doorbell, similarly to setting up a transfer request. > > Signed-off-by: Gilad Broner <gbroner@codeaurora.org> > Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org> > > --- > drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index fc2a52d..298511a 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg) > * 1 UTRLRDY > * 2 UTMRLRDY > * 3 UCRDY > - * 4 HEI > - * 5 DEI > - * 6-7 reserved > + * 4-7 reserved > */ > - return (((reg) & (0xFF)) >> 1) ^ (0x07); > + return ((reg & 0xFF) >> 1) ^ 0x07; > } > > /** > @@ -2726,7 +2724,7 @@ out: > * To bring UFS host controller to operational state, > * 1. Enable required interrupts > * 2. Configure interrupt aggregation > - * 3. Program UTRL and UTMRL base addres > + * 3. Program UTRL and UTMRL base address > * 4. Configure run-stop-registers > * > * Returns 0 on success, non-zero value on failure > @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba) > REG_UTP_TASK_REQ_LIST_BASE_H); > > /* > + * Make sure base address and interrupt setup are updated before > + * enabling the run/stop registers below. > + */ > + wmb(); > + > + /* > * UCRDY, UTMRLDY and UTRLRDY bits must be 1 > - * DEI, HEI bits must be 0 > */ > reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS); > if (!(ufshcd_get_lists_status(reg))) { > @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id, > > /* send command to the controller */ > __set_bit(free_slot, &hba->outstanding_tasks); > + > + /* Make sure descriptors are ready before ringing the task doorbell */ > + wmb(); > + > ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); > + /* Make sure that doorbell is committed immediately */ > + wmb(); Is this wmb() after ringing tm doorbell is needed? > > spin_unlock_irqrestore(host->host_lock, flags); > > -- > 1.8.5.2 > > -- > 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>: >> Performing several writes to UFS host controller registers has >> no gurrantee of ordering, so we must make sure register writes >> to setup request list base address etc. are performed before the >> run/stop register is enabled. >> In addition, when setting up a task request, we must make sure >> the updating of descriptors takes places before ringing the >> doorbell, similarly to setting up a transfer request. >> >> Signed-off-by: Gilad Broner <gbroner@codeaurora.org> >> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org> >> >> --- >> drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------ >> 1 file changed, 15 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >> index fc2a52d..298511a 100644 >> --- a/drivers/scsi/ufs/ufshcd.c >> +++ b/drivers/scsi/ufs/ufshcd.c >> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg) >> * 1 UTRLRDY >> * 2 UTMRLRDY >> * 3 UCRDY >> - * 4 HEI >> - * 5 DEI >> - * 6-7 reserved >> + * 4-7 reserved >> */ >> - return (((reg) & (0xFF)) >> 1) ^ (0x07); >> + return ((reg & 0xFF) >> 1) ^ 0x07; >> } >> >> /** >> @@ -2726,7 +2724,7 @@ out: >> * To bring UFS host controller to operational state, >> * 1. Enable required interrupts >> * 2. Configure interrupt aggregation >> - * 3. Program UTRL and UTMRL base addres >> + * 3. Program UTRL and UTMRL base address >> * 4. Configure run-stop-registers >> * >> * Returns 0 on success, non-zero value on failure >> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct >> ufs_hba *hba) >> REG_UTP_TASK_REQ_LIST_BASE_H); >> >> /* >> + * Make sure base address and interrupt setup are updated before >> + * enabling the run/stop registers below. >> + */ >> + wmb(); >> + >> + /* >> * UCRDY, UTMRLDY and UTRLRDY bits must be 1 >> - * DEI, HEI bits must be 0 >> */ >> reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS); >> if (!(ufshcd_get_lists_status(reg))) { >> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba >> *hba, int lun_id, int task_id, >> >> /* send command to the controller */ >> __set_bit(free_slot, &hba->outstanding_tasks); >> + >> + /* Make sure descriptors are ready before ringing the task >> doorbell */ >> + wmb(); >> + >> ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); >> + /* Make sure that doorbell is committed immediately */ >> + wmb(); > > Is this wmb() after ringing tm doorbell is needed? Well, Mita, in the case of DB register (Request DB and TASK DB), every write operation to the DB should be barrier, as if not, out of order writing to this register might cause inconsistency in its value, and thus, un-handled requests/tasks. > >> >> spin_unlock_irqrestore(host->host_lock, flags); >> >> -- >> 1.8.5.2 >> >> -- >> 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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2015-10-25 23:40 GMT+09:00 <ygardi@codeaurora.org>: >> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>: >>> Performing several writes to UFS host controller registers has >>> no gurrantee of ordering, so we must make sure register writes >>> to setup request list base address etc. are performed before the >>> run/stop register is enabled. >>> In addition, when setting up a task request, we must make sure >>> the updating of descriptors takes places before ringing the >>> doorbell, similarly to setting up a transfer request. >>> >>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org> >>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org> >>> >>> --- >>> drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------ >>> 1 file changed, 15 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>> index fc2a52d..298511a 100644 >>> --- a/drivers/scsi/ufs/ufshcd.c >>> +++ b/drivers/scsi/ufs/ufshcd.c >>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg) >>> * 1 UTRLRDY >>> * 2 UTMRLRDY >>> * 3 UCRDY >>> - * 4 HEI >>> - * 5 DEI >>> - * 6-7 reserved >>> + * 4-7 reserved >>> */ >>> - return (((reg) & (0xFF)) >> 1) ^ (0x07); >>> + return ((reg & 0xFF) >> 1) ^ 0x07; >>> } >>> >>> /** >>> @@ -2726,7 +2724,7 @@ out: >>> * To bring UFS host controller to operational state, >>> * 1. Enable required interrupts >>> * 2. Configure interrupt aggregation >>> - * 3. Program UTRL and UTMRL base addres >>> + * 3. Program UTRL and UTMRL base address >>> * 4. Configure run-stop-registers >>> * >>> * Returns 0 on success, non-zero value on failure >>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct >>> ufs_hba *hba) >>> REG_UTP_TASK_REQ_LIST_BASE_H); >>> >>> /* >>> + * Make sure base address and interrupt setup are updated before >>> + * enabling the run/stop registers below. >>> + */ >>> + wmb(); >>> + >>> + /* >>> * UCRDY, UTMRLDY and UTRLRDY bits must be 1 >>> - * DEI, HEI bits must be 0 >>> */ >>> reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS); >>> if (!(ufshcd_get_lists_status(reg))) { >>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba >>> *hba, int lun_id, int task_id, >>> >>> /* send command to the controller */ >>> __set_bit(free_slot, &hba->outstanding_tasks); >>> + >>> + /* Make sure descriptors are ready before ringing the task >>> doorbell */ >>> + wmb(); >>> + >>> ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); >>> + /* Make sure that doorbell is committed immediately */ >>> + wmb(); >> >> Is this wmb() after ringing tm doorbell is needed? > > Well, Mita, in the case of DB register (Request DB and TASK DB), every > write operation to the DB should be barrier, as if not, out of order > writing to this register might cause inconsistency in its value, and thus, > un-handled requests/tasks. I couldn't fully understand why out of order writing to TASK DB register causes inconsistency. In the TASK request completion (ufshcd_tmc_handler), TASK DB register is read before handling finished requests, so it ensures that all write operations for TASK DB have been performed. >> >>> >>> spin_unlock_irqrestore(host->host_lock, flags); >>> >>> -- >>> 1.8.5.2 >>> >>> -- >>> 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 majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 2015-10-25 23:40 GMT+09:00 <ygardi@codeaurora.org>: >>> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>: >>>> Performing several writes to UFS host controller registers has >>>> no gurrantee of ordering, so we must make sure register writes >>>> to setup request list base address etc. are performed before the >>>> run/stop register is enabled. >>>> In addition, when setting up a task request, we must make sure >>>> the updating of descriptors takes places before ringing the >>>> doorbell, similarly to setting up a transfer request. >>>> >>>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org> >>>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org> >>>> >>>> --- >>>> drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------ >>>> 1 file changed, 15 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>>> index fc2a52d..298511a 100644 >>>> --- a/drivers/scsi/ufs/ufshcd.c >>>> +++ b/drivers/scsi/ufs/ufshcd.c >>>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 >>>> reg) >>>> * 1 UTRLRDY >>>> * 2 UTMRLRDY >>>> * 3 UCRDY >>>> - * 4 HEI >>>> - * 5 DEI >>>> - * 6-7 reserved >>>> + * 4-7 reserved >>>> */ >>>> - return (((reg) & (0xFF)) >> 1) ^ (0x07); >>>> + return ((reg & 0xFF) >> 1) ^ 0x07; >>>> } >>>> >>>> /** >>>> @@ -2726,7 +2724,7 @@ out: >>>> * To bring UFS host controller to operational state, >>>> * 1. Enable required interrupts >>>> * 2. Configure interrupt aggregation >>>> - * 3. Program UTRL and UTMRL base addres >>>> + * 3. Program UTRL and UTMRL base address >>>> * 4. Configure run-stop-registers >>>> * >>>> * Returns 0 on success, non-zero value on failure >>>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct >>>> ufs_hba *hba) >>>> REG_UTP_TASK_REQ_LIST_BASE_H); >>>> >>>> /* >>>> + * Make sure base address and interrupt setup are updated >>>> before >>>> + * enabling the run/stop registers below. >>>> + */ >>>> + wmb(); >>>> + >>>> + /* >>>> * UCRDY, UTMRLDY and UTRLRDY bits must be 1 >>>> - * DEI, HEI bits must be 0 >>>> */ >>>> reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS); >>>> if (!(ufshcd_get_lists_status(reg))) { >>>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba >>>> *hba, int lun_id, int task_id, >>>> >>>> /* send command to the controller */ >>>> __set_bit(free_slot, &hba->outstanding_tasks); >>>> + >>>> + /* Make sure descriptors are ready before ringing the task >>>> doorbell */ >>>> + wmb(); >>>> + >>>> ufshcd_writel(hba, 1 << free_slot, >>>> REG_UTP_TASK_REQ_DOOR_BELL); >>>> + /* Make sure that doorbell is committed immediately */ >>>> + wmb(); >>> >>> Is this wmb() after ringing tm doorbell is needed? >> >> Well, Mita, in the case of DB register (Request DB and TASK DB), every >> write operation to the DB should be barrier, as if not, out of order >> writing to this register might cause inconsistency in its value, and >> thus, >> un-handled requests/tasks. > > I couldn't fully understand why out of order writing to TASK DB > register causes inconsistency. > In the TASK request completion (ufshcd_tmc_handler), TASK DB register > is read before handling finished requests, so it ensures that all > write operations for TASK DB have been performed. > thanks, Mita your explanation makes sense. It will be fixed in v4. >>> >>>> >>>> spin_unlock_irqrestore(host->host_lock, flags); >>>> >>>> -- >>>> 1.8.5.2 >>>> >>>> -- >>>> 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 majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> >> > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>> 2015-10-25 23:40 GMT+09:00 <ygardi@codeaurora.org>: >>>> 2015-09-02 19:13 GMT+09:00 Yaniv Gardi <ygardi@codeaurora.org>: >>>>> Performing several writes to UFS host controller registers has >>>>> no gurrantee of ordering, so we must make sure register writes >>>>> to setup request list base address etc. are performed before the >>>>> run/stop register is enabled. >>>>> In addition, when setting up a task request, we must make sure >>>>> the updating of descriptors takes places before ringing the >>>>> doorbell, similarly to setting up a transfer request. >>>>> >>>>> Signed-off-by: Gilad Broner <gbroner@codeaurora.org> >>>>> Signed-off-by: Yaniv Gardi <ygardi@codeaurora.org> >>>>> >>>>> --- >>>>> drivers/scsi/ufs/ufshcd.c | 21 +++++++++++++++------ >>>>> 1 file changed, 15 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c >>>>> index fc2a52d..298511a 100644 >>>>> --- a/drivers/scsi/ufs/ufshcd.c >>>>> +++ b/drivers/scsi/ufs/ufshcd.c >>>>> @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 >>>>> reg) >>>>> * 1 UTRLRDY >>>>> * 2 UTMRLRDY >>>>> * 3 UCRDY >>>>> - * 4 HEI >>>>> - * 5 DEI >>>>> - * 6-7 reserved >>>>> + * 4-7 reserved >>>>> */ >>>>> - return (((reg) & (0xFF)) >> 1) ^ (0x07); >>>>> + return ((reg & 0xFF) >> 1) ^ 0x07; >>>>> } >>>>> >>>>> /** >>>>> @@ -2726,7 +2724,7 @@ out: >>>>> * To bring UFS host controller to operational state, >>>>> * 1. Enable required interrupts >>>>> * 2. Configure interrupt aggregation >>>>> - * 3. Program UTRL and UTMRL base addres >>>>> + * 3. Program UTRL and UTMRL base address >>>>> * 4. Configure run-stop-registers >>>>> * >>>>> * Returns 0 on success, non-zero value on failure >>>>> @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct >>>>> ufs_hba *hba) >>>>> REG_UTP_TASK_REQ_LIST_BASE_H); >>>>> >>>>> /* >>>>> + * Make sure base address and interrupt setup are updated >>>>> before >>>>> + * enabling the run/stop registers below. >>>>> + */ >>>>> + wmb(); >>>>> + >>>>> + /* >>>>> * UCRDY, UTMRLDY and UTRLRDY bits must be 1 >>>>> - * DEI, HEI bits must be 0 >>>>> */ >>>>> reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS); >>>>> if (!(ufshcd_get_lists_status(reg))) { >>>>> @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba >>>>> *hba, int lun_id, int task_id, >>>>> >>>>> /* send command to the controller */ >>>>> __set_bit(free_slot, &hba->outstanding_tasks); >>>>> + >>>>> + /* Make sure descriptors are ready before ringing the task >>>>> doorbell */ >>>>> + wmb(); >>>>> + >>>>> ufshcd_writel(hba, 1 << free_slot, >>>>> REG_UTP_TASK_REQ_DOOR_BELL); >>>>> + /* Make sure that doorbell is committed immediately */ >>>>> + wmb(); >>>> >>>> Is this wmb() after ringing tm doorbell is needed? >>> >>> Well, Mita, in the case of DB register (Request DB and TASK DB), every >>> write operation to the DB should be barrier, as if not, out of order >>> writing to this register might cause inconsistency in its value, and >>> thus, >>> un-handled requests/tasks. >> >> I couldn't fully understand why out of order writing to TASK DB >> register causes inconsistency. >> In the TASK request completion (ufshcd_tmc_handler), TASK DB register >> is read before handling finished requests, so it ensures that all >> write operations for TASK DB have been performed. >> > > thanks, Mita > your explanation makes sense. > It will be fixed in v4. > sorry, it will fixed in v5. Yaniv > >>>> >>>>> >>>>> spin_unlock_irqrestore(host->host_lock, flags); >>>>> >>>>> -- >>>>> 1.8.5.2 >>>>> >>>>> -- >>>>> 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 majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>> >>> >> > > > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index fc2a52d..298511a 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -401,11 +401,9 @@ static inline int ufshcd_get_lists_status(u32 reg) * 1 UTRLRDY * 2 UTMRLRDY * 3 UCRDY - * 4 HEI - * 5 DEI - * 6-7 reserved + * 4-7 reserved */ - return (((reg) & (0xFF)) >> 1) ^ (0x07); + return ((reg & 0xFF) >> 1) ^ 0x07; } /** @@ -2726,7 +2724,7 @@ out: * To bring UFS host controller to operational state, * 1. Enable required interrupts * 2. Configure interrupt aggregation - * 3. Program UTRL and UTMRL base addres + * 3. Program UTRL and UTMRL base address * 4. Configure run-stop-registers * * Returns 0 on success, non-zero value on failure @@ -2756,8 +2754,13 @@ static int ufshcd_make_hba_operational(struct ufs_hba *hba) REG_UTP_TASK_REQ_LIST_BASE_H); /* + * Make sure base address and interrupt setup are updated before + * enabling the run/stop registers below. + */ + wmb(); + + /* * UCRDY, UTMRLDY and UTRLRDY bits must be 1 - * DEI, HEI bits must be 0 */ reg = ufshcd_readl(hba, REG_CONTROLLER_STATUS); if (!(ufshcd_get_lists_status(reg))) { @@ -3920,7 +3923,13 @@ static int ufshcd_issue_tm_cmd(struct ufs_hba *hba, int lun_id, int task_id, /* send command to the controller */ __set_bit(free_slot, &hba->outstanding_tasks); + + /* Make sure descriptors are ready before ringing the task doorbell */ + wmb(); + ufshcd_writel(hba, 1 << free_slot, REG_UTP_TASK_REQ_DOOR_BELL); + /* Make sure that doorbell is committed immediately */ + wmb(); spin_unlock_irqrestore(host->host_lock, flags);