| Message ID | 1551504025-3541-7-git-send-email-skomatineni@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V1,01/11] mmc: tegra: fix ddr signaling for non-ddr modes | expand |
On 2/03/19 7:20 AM, Sowjanya Komatineni wrote: > This patch adds a quirk for setting CMD_TIMING to 1 in descriptor > for DCMD with R1B response type to allow the command to be sent to > device during data activity or busy time. > > Tegra186 CQHCI host has bug where it selects DATA_PRESENT_SELECT > to 1 by CQHCI controller for DCMDs with R1B response type and > since DCMD does not trigger any data transfer, DCMD task complete > happens leaving the DATA FSM of host controller in wait state for > data. > > This effects the data transfer task issued after R1B DCMD task > and no interrupt is generated for the data transfer task. > > SW WAR for this issue is to set CMD_TIMING bit to 1 in DCMD task > descriptor and as DCMD task descriptor preparation is done by > cqhci driver, this patch adds cqequirk to handle this. > > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > --- > drivers/mmc/host/cqhci.c | 5 ++++- > drivers/mmc/host/cqhci.h | 1 + > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c > index a8af682a9182..b34c07125f32 100644 > --- a/drivers/mmc/host/cqhci.c > +++ b/drivers/mmc/host/cqhci.c > @@ -521,7 +521,10 @@ static void cqhci_prep_dcmd_desc(struct mmc_host *mmc, > } else { > if (mrq->cmd->flags & MMC_RSP_R1B) { > resp_type = 0x3; > - timing = 0x0; > + if (cq_host->quirks & CQHCI_QUIRK_CMD_TIMING_R1B_DCMD) > + timing = 0x1; > + else > + timing = 0x0; I was thinking it would be nice if there was a generic way for drivers to make changes to descriptors before a task is started. Currently there is host->ops->write_l() which would make it possible by checking for CQHCI_TDBR register and, in this case, the DCMD tag. We would need to export get_desc(), perhaps rename it cqhci_get_desc() and put it in cqhci.h since it is an inline function. Alternatively we could add host->ops for descriptor preparation. What do people think? > } else { > resp_type = 0x2; > timing = 0x1; > diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h > index 9e68286a07b4..f96d8565cc07 100644 > --- a/drivers/mmc/host/cqhci.h > +++ b/drivers/mmc/host/cqhci.h > @@ -170,6 +170,7 @@ struct cqhci_host { > > u32 quirks; > #define CQHCI_QUIRK_SHORT_TXFR_DESC_SZ 0x1 > +#define CQHCI_QUIRK_CMD_TIMING_R1B_DCMD 0x2 > > bool enabled; > bool halted; >
On 3/6/2019 6:30 PM, Adrian Hunter wrote: > On 2/03/19 7:20 AM, Sowjanya Komatineni wrote: >> This patch adds a quirk for setting CMD_TIMING to 1 in descriptor >> for DCMD with R1B response type to allow the command to be sent to >> device during data activity or busy time. >> >> Tegra186 CQHCI host has bug where it selects DATA_PRESENT_SELECT >> to 1 by CQHCI controller for DCMDs with R1B response type and >> since DCMD does not trigger any data transfer, DCMD task complete >> happens leaving the DATA FSM of host controller in wait state for >> data. >> >> This effects the data transfer task issued after R1B DCMD task >> and no interrupt is generated for the data transfer task. >> >> SW WAR for this issue is to set CMD_TIMING bit to 1 in DCMD task >> descriptor and as DCMD task descriptor preparation is done by >> cqhci driver, this patch adds cqequirk to handle this. >> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >> --- >> drivers/mmc/host/cqhci.c | 5 ++++- >> drivers/mmc/host/cqhci.h | 1 + >> 2 files changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c >> index a8af682a9182..b34c07125f32 100644 >> --- a/drivers/mmc/host/cqhci.c >> +++ b/drivers/mmc/host/cqhci.c >> @@ -521,7 +521,10 @@ static void cqhci_prep_dcmd_desc(struct mmc_host *mmc, >> } else { >> if (mrq->cmd->flags & MMC_RSP_R1B) { >> resp_type = 0x3; >> - timing = 0x0; >> + if (cq_host->quirks & CQHCI_QUIRK_CMD_TIMING_R1B_DCMD) >> + timing = 0x1; >> + else >> + timing = 0x0; > I was thinking it would be nice if there was a generic way for drivers to > make changes to descriptors before a task is started. Currently there is > host->ops->write_l() which would make it possible by checking for CQHCI_TDBR > register and, in this case, the DCMD tag. We would need to export > get_desc(), perhaps rename it cqhci_get_desc() and put it in cqhci.h since > it is an inline function. We take spin_lock_irqsave after the descriptor is prepared and before writing to TDBR. Not sure but tomorrow this may become a limitation for drivers to make changes to descriptors if they edit descriptors in host->ops->write_l() call. Though in this case it is not required here. > > Alternatively we could add host->ops for descriptor preparation. Both ways sounds good to me. But maybe adding a host->ops for descriptor preparation is better way to go, since that will be the right interface exposed to make changes to descriptors. > > What do people think? > >> } else { >> resp_type = 0x2; >> timing = 0x1; >> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h >> index 9e68286a07b4..f96d8565cc07 100644 >> --- a/drivers/mmc/host/cqhci.h >> +++ b/drivers/mmc/host/cqhci.h >> @@ -170,6 +170,7 @@ struct cqhci_host { >> >> u32 quirks; >> #define CQHCI_QUIRK_SHORT_TXFR_DESC_SZ 0x1 >> +#define CQHCI_QUIRK_CMD_TIMING_R1B_DCMD 0x2 >> >> bool enabled; >> bool halted; >>
> On 3/6/2019 6:30 PM, Adrian Hunter wrote: >> On 2/03/19 7:20 AM, Sowjanya Komatineni wrote: >>> This patch adds a quirk for setting CMD_TIMING to 1 in descriptor for >>> DCMD with R1B response type to allow the command to be sent to device >>> during data activity or busy time. >>> >>> Tegra186 CQHCI host has bug where it selects DATA_PRESENT_SELECT to 1 >>> by CQHCI controller for DCMDs with R1B response type and since DCMD >>> does not trigger any data transfer, DCMD task complete happens >>> leaving the DATA FSM of host controller in wait state for data. >>> >>> This effects the data transfer task issued after R1B DCMD task and no >>> interrupt is generated for the data transfer task. >>> >>> SW WAR for this issue is to set CMD_TIMING bit to 1 in DCMD task >>> descriptor and as DCMD task descriptor preparation is done by cqhci >>> driver, this patch adds cqequirk to handle this. >>> >>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>> --- >>> drivers/mmc/host/cqhci.c | 5 ++++- >>> drivers/mmc/host/cqhci.h | 1 + >>> 2 files changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c >>> index a8af682a9182..b34c07125f32 100644 >>> --- a/drivers/mmc/host/cqhci.c >>> +++ b/drivers/mmc/host/cqhci.c >>> @@ -521,7 +521,10 @@ static void cqhci_prep_dcmd_desc(struct mmc_host *mmc, >>> } else { >>> if (mrq->cmd->flags & MMC_RSP_R1B) { >>> resp_type = 0x3; >>> - timing = 0x0; >>> + if (cq_host->quirks & CQHCI_QUIRK_CMD_TIMING_R1B_DCMD) >>> + timing = 0x1; >>> + else >>> + timing = 0x0; >> I was thinking it would be nice if there was a generic way for drivers >> to make changes to descriptors before a task is started. Currently >> there is >> host->ops->write_l() which would make it possible by checking for >> host->ops->CQHCI_TDBR >> register and, in this case, the DCMD tag. We would need to export >> get_desc(), perhaps rename it cqhci_get_desc() and put it in cqhci.h >> since it is an inline function. > >We take spin_lock_irqsave after the descriptor is prepared and before writing to TDBR. >Not sure but tomorrow this may become a limitation for drivers to make changes to descriptors if they edit descriptors in host->ops->write_l() call. >Though in this case it is not required here. > >> >> Alternatively we could add host->ops for descriptor preparation. > >Both ways sounds good to me. >But maybe adding a host->ops for descriptor preparation is better way to go, >since that will be the right interface exposed to make changes to >descriptors. > DCMD descriptor attributes remain same for any host and also task parameters as QBR need to be enabled with DCMD. So I believe it should be ok if we just add callback to allow hosts to update command parameters of DCMD descriptor only thru cqhci_host_ops. Also, don’t see any requirement for host specific Task parameter updates in Task descriptor so not sure if there is any need to provide callback for task descriptor data preparation to hosts. Please confirm. >> >> What do people think? >> >>> } else { >>> resp_type = 0x2; >>> timing = 0x1; >>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h >>> index 9e68286a07b4..f96d8565cc07 100644 >>> --- a/drivers/mmc/host/cqhci.h >>> +++ b/drivers/mmc/host/cqhci.h >>> @@ -170,6 +170,7 @@ struct cqhci_host { >>> >>> u32 quirks; >>> #define CQHCI_QUIRK_SHORT_TXFR_DESC_SZ 0x1 >>> +#define CQHCI_QUIRK_CMD_TIMING_R1B_DCMD 0x2 >>> >>> bool enabled; >>> bool halted; >>>
On 7/03/19 8:16 PM, Sowjanya Komatineni wrote: > >> On 3/6/2019 6:30 PM, Adrian Hunter wrote: >>> On 2/03/19 7:20 AM, Sowjanya Komatineni wrote: >>>> This patch adds a quirk for setting CMD_TIMING to 1 in descriptor for >>>> DCMD with R1B response type to allow the command to be sent to device >>>> during data activity or busy time. >>>> >>>> Tegra186 CQHCI host has bug where it selects DATA_PRESENT_SELECT to 1 >>>> by CQHCI controller for DCMDs with R1B response type and since DCMD >>>> does not trigger any data transfer, DCMD task complete happens >>>> leaving the DATA FSM of host controller in wait state for data. >>>> >>>> This effects the data transfer task issued after R1B DCMD task and no >>>> interrupt is generated for the data transfer task. >>>> >>>> SW WAR for this issue is to set CMD_TIMING bit to 1 in DCMD task >>>> descriptor and as DCMD task descriptor preparation is done by cqhci >>>> driver, this patch adds cqequirk to handle this. >>>> >>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>>> --- >>>> drivers/mmc/host/cqhci.c | 5 ++++- >>>> drivers/mmc/host/cqhci.h | 1 + >>>> 2 files changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c >>>> index a8af682a9182..b34c07125f32 100644 >>>> --- a/drivers/mmc/host/cqhci.c >>>> +++ b/drivers/mmc/host/cqhci.c >>>> @@ -521,7 +521,10 @@ static void cqhci_prep_dcmd_desc(struct mmc_host *mmc, >>>> } else { >>>> if (mrq->cmd->flags & MMC_RSP_R1B) { >>>> resp_type = 0x3; >>>> - timing = 0x0; >>>> + if (cq_host->quirks & CQHCI_QUIRK_CMD_TIMING_R1B_DCMD) >>>> + timing = 0x1; >>>> + else >>>> + timing = 0x0; >>> I was thinking it would be nice if there was a generic way for drivers >>> to make changes to descriptors before a task is started. Currently >>> there is >>> host->ops->write_l() which would make it possible by checking for >>> host->ops->CQHCI_TDBR >>> register and, in this case, the DCMD tag. We would need to export >>> get_desc(), perhaps rename it cqhci_get_desc() and put it in cqhci.h >>> since it is an inline function. >> >> We take spin_lock_irqsave after the descriptor is prepared and before writing to TDBR. >> Not sure but tomorrow this may become a limitation for drivers to make changes to descriptors if they edit descriptors in host->ops->write_l() call. >> Though in this case it is not required here. >> >>> >>> Alternatively we could add host->ops for descriptor preparation. >> >> Both ways sounds good to me. >> But maybe adding a host->ops for descriptor preparation is better way to go, >> since that will be the right interface exposed to make changes to >> descriptors. >> > > DCMD descriptor attributes remain same for any host and also task parameters as QBR need to be enabled with DCMD. > So I believe it should be ok if we just add callback to allow hosts to update command parameters of DCMD descriptor only thru cqhci_host_ops. That sounds fine to me. Maybe do that for the next version of this patchset. > > Also, don’t see any requirement for host specific Task parameter updates in Task descriptor so not sure if there is any need to provide callback for task descriptor data preparation to hosts. > Please confirm. > >>> >>> What do people think? >>> >>>> } else { >>>> resp_type = 0x2; >>>> timing = 0x1; >>>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h >>>> index 9e68286a07b4..f96d8565cc07 100644 >>>> --- a/drivers/mmc/host/cqhci.h >>>> +++ b/drivers/mmc/host/cqhci.h >>>> @@ -170,6 +170,7 @@ struct cqhci_host { >>>> >>>> u32 quirks; >>>> #define CQHCI_QUIRK_SHORT_TXFR_DESC_SZ 0x1 >>>> +#define CQHCI_QUIRK_CMD_TIMING_R1B_DCMD 0x2 >>>> >>>> bool enabled; >>>> bool halted; >>>>
> On 7/03/19 8:16 PM, Sowjanya Komatineni wrote: >> >>> On 3/6/2019 6:30 PM, Adrian Hunter wrote: >>>> On 2/03/19 7:20 AM, Sowjanya Komatineni wrote: >>>>> This patch adds a quirk for setting CMD_TIMING to 1 in descriptor >>>>> for DCMD with R1B response type to allow the command to be sent to >>>>> device during data activity or busy time. >>>>> >>>>> Tegra186 CQHCI host has bug where it selects DATA_PRESENT_SELECT to >>>>> 1 by CQHCI controller for DCMDs with R1B response type and since >>>>> DCMD does not trigger any data transfer, DCMD task complete happens >>>>> leaving the DATA FSM of host controller in wait state for data. >>>>> >>>>> This effects the data transfer task issued after R1B DCMD task and >>>>> no interrupt is generated for the data transfer task. >>>>> >>>>> SW WAR for this issue is to set CMD_TIMING bit to 1 in DCMD task >>>>> descriptor and as DCMD task descriptor preparation is done by cqhci >>>>> driver, this patch adds cqequirk to handle this. >>>>> >>>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>>>> --- >>>>> drivers/mmc/host/cqhci.c | 5 ++++- >>>>> drivers/mmc/host/cqhci.h | 1 + >>>>> 2 files changed, 5 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c >>>>> index a8af682a9182..b34c07125f32 100644 >>>>> --- a/drivers/mmc/host/cqhci.c >>>>> +++ b/drivers/mmc/host/cqhci.c >>>>> @@ -521,7 +521,10 @@ static void cqhci_prep_dcmd_desc(struct mmc_host *mmc, >>>>> } else { >>>>> if (mrq->cmd->flags & MMC_RSP_R1B) { >>>>> resp_type = 0x3; >>>>> - timing = 0x0; >>>>> + if (cq_host->quirks & CQHCI_QUIRK_CMD_TIMING_R1B_DCMD) >>>>> + timing = 0x1; >>>>> + else >>>>> + timing = 0x0; >>>> I was thinking it would be nice if there was a generic way for >>>> drivers to make changes to descriptors before a task is started. >>>> Currently there is >>>> host->ops->write_l() which would make it possible by checking for >>>> host->ops->CQHCI_TDBR >>>> register and, in this case, the DCMD tag. We would need to export >>>> get_desc(), perhaps rename it cqhci_get_desc() and put it in cqhci.h >>>> since it is an inline function. >>> >>> We take spin_lock_irqsave after the descriptor is prepared and before writing to TDBR. >>> Not sure but tomorrow this may become a limitation for drivers to make changes to descriptors if they edit descriptors in host->ops->write_l() call. >>> Though in this case it is not required here. >>> >>>> >>>> Alternatively we could add host->ops for descriptor preparation. >>> >>> Both ways sounds good to me. >>> But maybe adding a host->ops for descriptor preparation is better way >>> to go, since that will be the right interface exposed to make changes >>> to descriptors. >>> >> >> DCMD descriptor attributes remain same for any host and also task parameters as QBR need to be enabled with DCMD. >> So I believe it should be ok if we just add callback to allow hosts to update command parameters of DCMD descriptor only thru cqhci_host_ops. > >That sounds fine to me. Maybe do that for the next version of this patchset. Thanks Adrian. Actually, DCMD command parameters include opcode, timing, and response type where opcode and response type stays same. So I only see need for hosts to choose cmd timing to control if hardware should wait for busy time or not basically to send command during busy or idle time. Will add cqhci_host_ops interface to allow host driver to choose cmd timing and will send along with the other tuning related feedback fixes of this series. >> >> Also, don’t see any requirement for host specific Task parameter updates in Task descriptor so not sure if there is any need to provide callback for task descriptor data preparation to hosts. >> Please confirm. >> >>>> >>>> What do people think? >>>> >>>>> } else { >>>>> resp_type = 0x2; >>>>> timing = 0x1; >>>>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h >>>>> index 9e68286a07b4..f96d8565cc07 100644 >>>>> --- a/drivers/mmc/host/cqhci.h >>>>> +++ b/drivers/mmc/host/cqhci.h >>>>> @@ -170,6 +170,7 @@ struct cqhci_host { >>>>> >>>>> u32 quirks; >>>>> #define CQHCI_QUIRK_SHORT_TXFR_DESC_SZ 0x1 >>>>> +#define CQHCI_QUIRK_CMD_TIMING_R1B_DCMD 0x2 >>>>> >>>>> bool enabled; >>>>> bool halted; >>>>>
On 3/7/2019 11:46 PM, Sowjanya Komatineni wrote: >> On 3/6/2019 6:30 PM, Adrian Hunter wrote: >>> On 2/03/19 7:20 AM, Sowjanya Komatineni wrote: >>>> This patch adds a quirk for setting CMD_TIMING to 1 in descriptor for >>>> DCMD with R1B response type to allow the command to be sent to device >>>> during data activity or busy time. >>>> >>>> Tegra186 CQHCI host has bug where it selects DATA_PRESENT_SELECT to 1 >>>> by CQHCI controller for DCMDs with R1B response type and since DCMD >>>> does not trigger any data transfer, DCMD task complete happens >>>> leaving the DATA FSM of host controller in wait state for data. >>>> >>>> This effects the data transfer task issued after R1B DCMD task and no >>>> interrupt is generated for the data transfer task. >>>> >>>> SW WAR for this issue is to set CMD_TIMING bit to 1 in DCMD task >>>> descriptor and as DCMD task descriptor preparation is done by cqhci >>>> driver, this patch adds cqequirk to handle this. >>>> >>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> >>>> --- >>>> drivers/mmc/host/cqhci.c | 5 ++++- >>>> drivers/mmc/host/cqhci.h | 1 + >>>> 2 files changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c >>>> index a8af682a9182..b34c07125f32 100644 >>>> --- a/drivers/mmc/host/cqhci.c >>>> +++ b/drivers/mmc/host/cqhci.c >>>> @@ -521,7 +521,10 @@ static void cqhci_prep_dcmd_desc(struct mmc_host *mmc, >>>> } else { >>>> if (mrq->cmd->flags & MMC_RSP_R1B) { >>>> resp_type = 0x3; >>>> - timing = 0x0; >>>> + if (cq_host->quirks & CQHCI_QUIRK_CMD_TIMING_R1B_DCMD) >>>> + timing = 0x1; >>>> + else >>>> + timing = 0x0; >>> I was thinking it would be nice if there was a generic way for drivers >>> to make changes to descriptors before a task is started. Currently >>> there is >>> host->ops->write_l() which would make it possible by checking for >>> host->ops->CQHCI_TDBR >>> register and, in this case, the DCMD tag. We would need to export >>> get_desc(), perhaps rename it cqhci_get_desc() and put it in cqhci.h >>> since it is an inline function. >> We take spin_lock_irqsave after the descriptor is prepared and before writing to TDBR. >> Not sure but tomorrow this may become a limitation for drivers to make changes to descriptors if they edit descriptors in host->ops->write_l() call. >> Though in this case it is not required here. >> >>> Alternatively we could add host->ops for descriptor preparation. >> Both ways sounds good to me. >> But maybe adding a host->ops for descriptor preparation is better way to go, >> since that will be the right interface exposed to make changes to >> descriptors. >> > DCMD descriptor attributes remain same for any host and also task parameters as QBR need to be enabled with DCMD. > So I believe it should be ok if we just add callback to allow hosts to update command parameters of DCMD descriptor only thru cqhci_host_ops. For now we can add host->ops as update_dcmd_desc and pass the task_desc of DCMD for updating any params which host may want to update. > > Also, don’t see any requirement for host specific Task parameter updates in Task descriptor so not sure if there is any need to provide callback for task descriptor data preparation to hosts. > Please confirm. Sure, for now the requirement has come up only for DCMD desc update. Sure we can add task descriptor ops in the similar way later when required. Adrian, please confirm if you are fine with both of above? > >>> What do people think? >>> >>>> } else { >>>> resp_type = 0x2; >>>> timing = 0x1; >>>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h >>>> index 9e68286a07b4..f96d8565cc07 100644 >>>> --- a/drivers/mmc/host/cqhci.h >>>> +++ b/drivers/mmc/host/cqhci.h >>>> @@ -170,6 +170,7 @@ struct cqhci_host { >>>> >>>> u32 quirks; >>>> #define CQHCI_QUIRK_SHORT_TXFR_DESC_SZ 0x1 >>>> +#define CQHCI_QUIRK_CMD_TIMING_R1B_DCMD 0x2 >>>> >>>> bool enabled; >>>> bool halted; >>>>
> -----Original Message----- > From: Ritesh Harjani [mailto:riteshh@codeaurora.org] > Sent: Wednesday, March 13, 2019 4:31 AM > To: Sowjanya Komatineni <skomatineni@nvidia.com>; Hunter, Adrian > <adrian.hunter@intel.com>; ulf.hansson@linaro.org; robh+dt@kernel.org; > mark.rutland@arm.com; Asutosh Das <asutoshd@codeaurora.org> > Cc: thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>; > Aniruddha Tvs Rao <anrao@nvidia.com>; linux-tegra@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-mmc@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [PATCH V1 07/11] mmc: cqhci: add quirk for setting DCMD > CMD_TIMING > > > On 3/7/2019 11:46 PM, Sowjanya Komatineni wrote: > >> On 3/6/2019 6:30 PM, Adrian Hunter wrote: > >>> On 2/03/19 7:20 AM, Sowjanya Komatineni wrote: > >>>> This patch adds a quirk for setting CMD_TIMING to 1 in descriptor > >>>> for DCMD with R1B response type to allow the command to be sent to > >>>> device during data activity or busy time. > >>>> > >>>> Tegra186 CQHCI host has bug where it selects DATA_PRESENT_SELECT > to > >>>> 1 by CQHCI controller for DCMDs with R1B response type and since > >>>> DCMD does not trigger any data transfer, DCMD task complete > happens > >>>> leaving the DATA FSM of host controller in wait state for data. > >>>> > >>>> This effects the data transfer task issued after R1B DCMD task and > >>>> no interrupt is generated for the data transfer task. > >>>> > >>>> SW WAR for this issue is to set CMD_TIMING bit to 1 in DCMD task > >>>> descriptor and as DCMD task descriptor preparation is done by cqhci > >>>> driver, this patch adds cqequirk to handle this. > >>>> > >>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > >>>> --- > >>>> drivers/mmc/host/cqhci.c | 5 ++++- > >>>> drivers/mmc/host/cqhci.h | 1 + > >>>> 2 files changed, 5 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c > >>>> index a8af682a9182..b34c07125f32 100644 > >>>> --- a/drivers/mmc/host/cqhci.c > >>>> +++ b/drivers/mmc/host/cqhci.c > >>>> @@ -521,7 +521,10 @@ static void cqhci_prep_dcmd_desc(struct > mmc_host *mmc, > >>>> } else { > >>>> if (mrq->cmd->flags & MMC_RSP_R1B) { > >>>> resp_type = 0x3; > >>>> - timing = 0x0; > >>>> + if (cq_host->quirks & > CQHCI_QUIRK_CMD_TIMING_R1B_DCMD) > >>>> + timing = 0x1; > >>>> + else > >>>> + timing = 0x0; > >>> I was thinking it would be nice if there was a generic way for > >>> drivers to make changes to descriptors before a task is started. > >>> Currently there is > >>> host->ops->write_l() which would make it possible by checking for > >>> host->ops->CQHCI_TDBR > >>> register and, in this case, the DCMD tag. We would need to export > >>> get_desc(), perhaps rename it cqhci_get_desc() and put it in cqhci.h > >>> since it is an inline function. > >> We take spin_lock_irqsave after the descriptor is prepared and before > writing to TDBR. > >> Not sure but tomorrow this may become a limitation for drivers to make > changes to descriptors if they edit descriptors in host->ops->write_l() call. > >> Though in this case it is not required here. > >> > >>> Alternatively we could add host->ops for descriptor preparation. > >> Both ways sounds good to me. > >> But maybe adding a host->ops for descriptor preparation is better way > >> to go, since that will be the right interface exposed to make changes > >> to descriptors. > >> > > DCMD descriptor attributes remain same for any host and also task > parameters as QBR need to be enabled with DCMD. > > So I believe it should be ok if we just add callback to allow hosts to update > command parameters of DCMD descriptor only thru cqhci_host_ops. > > For now we can add host->ops as update_dcmd_desc and pass the > task_desc of DCMD for updating any params which host may want to update. > > > > > Also, don’t see any requirement for host specific Task parameter updates > in Task descriptor so not sure if there is any need to provide callback for task > descriptor data preparation to hosts. > > Please confirm. > > Sure, for now the requirement has come up only for DCMD desc update. > Sure we can add task descriptor ops in the similar way later when required. > > Adrian, please confirm if you are fine with both of above? Yes, I agree. Sowjanya's V2 06/10 patch is quite narrowly focused, whereas update_dcmd_desc as you described seems just as easy to implement, but Is more flexible. > > > > >>> What do people think? > >>> > >>>> } else { > >>>> resp_type = 0x2; > >>>> timing = 0x1; > >>>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h > >>>> index 9e68286a07b4..f96d8565cc07 100644 > >>>> --- a/drivers/mmc/host/cqhci.h > >>>> +++ b/drivers/mmc/host/cqhci.h > >>>> @@ -170,6 +170,7 @@ struct cqhci_host { > >>>> > >>>> u32 quirks; > >>>> #define CQHCI_QUIRK_SHORT_TXFR_DESC_SZ 0x1 > >>>> +#define CQHCI_QUIRK_CMD_TIMING_R1B_DCMD 0x2 > >>>> > >>>> bool enabled; > >>>> bool halted; > >>>>
> -----Original Message----- > From: Hunter, Adrian <adrian.hunter@intel.com> > Sent: Wednesday, March 13, 2019 2:57 AM > To: Ritesh Harjani <riteshh@codeaurora.org>; Sowjanya Komatineni <skomatineni@nvidia.com>; ulf.hansson@linaro.org; robh+dt@kernel.org; mark.rutland@arm.com; Asutosh Das <asutoshd@codeaurora.org> > Cc: thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>; Aniruddha Tvs Rao <anrao@nvidia.com>; linux-tegra@vger.kernel.org; linux-kernel@vger.kernel.org; linux-mmc@vger.kernel.org; devicetree@vger.kernel.org > Subject: RE: [PATCH V1 07/11] mmc: cqhci: add quirk for setting DCMD CMD_TIMING > > > -----Original Message----- > > From: Ritesh Harjani [mailto:riteshh@codeaurora.org] > > Sent: Wednesday, March 13, 2019 4:31 AM > > To: Sowjanya Komatineni <skomatineni@nvidia.com>; Hunter, Adrian > > <adrian.hunter@intel.com>; ulf.hansson@linaro.org; robh+dt@kernel.org; > > mark.rutland@arm.com; Asutosh Das <asutoshd@codeaurora.org> > > Cc: thierry.reding@gmail.com; Jonathan Hunter <jonathanh@nvidia.com>; > > Aniruddha Tvs Rao <anrao@nvidia.com>; linux-tegra@vger.kernel.org; > > linux- kernel@vger.kernel.org; linux-mmc@vger.kernel.org; > > devicetree@vger.kernel.org > > Subject: Re: [PATCH V1 07/11] mmc: cqhci: add quirk for setting DCMD > > CMD_TIMING > > > > > > On 3/7/2019 11:46 PM, Sowjanya Komatineni wrote: > > >> On 3/6/2019 6:30 PM, Adrian Hunter wrote: > > >>> On 2/03/19 7:20 AM, Sowjanya Komatineni wrote: > > >>>> This patch adds a quirk for setting CMD_TIMING to 1 in descriptor > > >>>> for DCMD with R1B response type to allow the command to be sent > > >>>> to device during data activity or busy time. > > >>>> > > >>>> Tegra186 CQHCI host has bug where it selects DATA_PRESENT_SELECT > > to > > >>>> 1 by CQHCI controller for DCMDs with R1B response type and since > > >>>> DCMD does not trigger any data transfer, DCMD task complete > > happens > > >>>> leaving the DATA FSM of host controller in wait state for data. > > >>>> > > >>>> This effects the data transfer task issued after R1B DCMD task > > >>>> and no interrupt is generated for the data transfer task. > > >>>> > > >>>> SW WAR for this issue is to set CMD_TIMING bit to 1 in DCMD task > > >>>> descriptor and as DCMD task descriptor preparation is done by > > >>>> cqhci driver, this patch adds cqequirk to handle this. > > >>>> > > >>>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> > > >>>> --- > > >>>> drivers/mmc/host/cqhci.c | 5 ++++- > > >>>> drivers/mmc/host/cqhci.h | 1 + > > >>>> 2 files changed, 5 insertions(+), 1 deletion(-) > > >>>> > > >>>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c > > >>>> index a8af682a9182..b34c07125f32 100644 > > >>>> --- a/drivers/mmc/host/cqhci.c > > >>>> +++ b/drivers/mmc/host/cqhci.c > > >>>> @@ -521,7 +521,10 @@ static void cqhci_prep_dcmd_desc(struct > > mmc_host *mmc, > > >>>> } else { > > >>>> if (mrq->cmd->flags & MMC_RSP_R1B) { > > >>>> resp_type = 0x3; > > >>>> - timing = 0x0; > > >>>> + if (cq_host->quirks & > > CQHCI_QUIRK_CMD_TIMING_R1B_DCMD) > > >>>> + timing = 0x1; > > >>>> + else > > >>>> + timing = 0x0; > > >>> I was thinking it would be nice if there was a generic way for > > >>> drivers to make changes to descriptors before a task is started. > > >>> Currently there is > > >>> host->ops->write_l() which would make it possible by checking for > > >>> host->ops->CQHCI_TDBR > > >>> register and, in this case, the DCMD tag. We would need to export > > >>> get_desc(), perhaps rename it cqhci_get_desc() and put it in > > >>> cqhci.h since it is an inline function. > > >> We take spin_lock_irqsave after the descriptor is prepared and > > >> before > > writing to TDBR. > > >> Not sure but tomorrow this may become a limitation for drivers to > > >> make > > changes to descriptors if they edit descriptors in host->ops->write_l() call. > > >> Though in this case it is not required here. > > >> > > >>> Alternatively we could add host->ops for descriptor preparation. > > >> Both ways sounds good to me. > > >> But maybe adding a host->ops for descriptor preparation is better > > >> way to go, since that will be the right interface exposed to make > > >> changes to descriptors. > > >> > > > DCMD descriptor attributes remain same for any host and also task > > parameters as QBR need to be enabled with DCMD. > > > So I believe it should be ok if we just add callback to allow hosts > > > to update > > command parameters of DCMD descriptor only thru cqhci_host_ops. > > > > For now we can add host->ops as update_dcmd_desc and pass the > > task_desc of DCMD for updating any params which host may want to update. > > > > > > > > Also, don’t see any requirement for host specific Task parameter > > > updates > > in Task descriptor so not sure if there is any need to provide > > callback for task descriptor data preparation to hosts. > > > Please confirm. > > > > Sure, for now the requirement has come up only for DCMD desc update. > > Sure we can add task descriptor ops in the similar way later when required. > > > > Adrian, please confirm if you are fine with both of above? > > Yes, I agree. Sowjanya's V2 06/10 patch is quite narrowly focused, whereas update_dcmd_desc as you described seems just as easy to implement, but Is more flexible. > In case if my response to feedback got missed, DCMD descriptor attributes and task parameters remain same for any host including QBR as its must to enable with DCMD. DCMD command parameters include opcode, timing, and response type where opcode and response type stays same. So added interface only to allow command timing parameter which host can set either 1 or 0 in V2. Irrespective of DCMD descriptor update possibilities needed for hosts, we can expose complete DCMD descriptor data to hosts. Based on your both feedbacks, Will go ahead and update to allow complete DCMD descriptor data update for hosts for more flexibility. > > > > > > > >>> What do people think? > > >>> > > >>>> } else { > > >>>> resp_type = 0x2; > > >>>> timing = 0x1; > > >>>> diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h > > >>>> index 9e68286a07b4..f96d8565cc07 100644 > > >>>> --- a/drivers/mmc/host/cqhci.h > > >>>> +++ b/drivers/mmc/host/cqhci.h > > >>>> @@ -170,6 +170,7 @@ struct cqhci_host { > > >>>> > > >>>> u32 quirks; > > >>>> #define CQHCI_QUIRK_SHORT_TXFR_DESC_SZ 0x1 > > >>>> +#define CQHCI_QUIRK_CMD_TIMING_R1B_DCMD 0x2 > > >>>> > > >>>> bool enabled; > > >>>> bool halted; > > > >>>>
diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c index a8af682a9182..b34c07125f32 100644 --- a/drivers/mmc/host/cqhci.c +++ b/drivers/mmc/host/cqhci.c @@ -521,7 +521,10 @@ static void cqhci_prep_dcmd_desc(struct mmc_host *mmc, } else { if (mrq->cmd->flags & MMC_RSP_R1B) { resp_type = 0x3; - timing = 0x0; + if (cq_host->quirks & CQHCI_QUIRK_CMD_TIMING_R1B_DCMD) + timing = 0x1; + else + timing = 0x0; } else { resp_type = 0x2; timing = 0x1; diff --git a/drivers/mmc/host/cqhci.h b/drivers/mmc/host/cqhci.h index 9e68286a07b4..f96d8565cc07 100644 --- a/drivers/mmc/host/cqhci.h +++ b/drivers/mmc/host/cqhci.h @@ -170,6 +170,7 @@ struct cqhci_host { u32 quirks; #define CQHCI_QUIRK_SHORT_TXFR_DESC_SZ 0x1 +#define CQHCI_QUIRK_CMD_TIMING_R1B_DCMD 0x2 bool enabled; bool halted;
This patch adds a quirk for setting CMD_TIMING to 1 in descriptor for DCMD with R1B response type to allow the command to be sent to device during data activity or busy time. Tegra186 CQHCI host has bug where it selects DATA_PRESENT_SELECT to 1 by CQHCI controller for DCMDs with R1B response type and since DCMD does not trigger any data transfer, DCMD task complete happens leaving the DATA FSM of host controller in wait state for data. This effects the data transfer task issued after R1B DCMD task and no interrupt is generated for the data transfer task. SW WAR for this issue is to set CMD_TIMING bit to 1 in DCMD task descriptor and as DCMD task descriptor preparation is done by cqhci driver, this patch adds cqequirk to handle this. Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com> --- drivers/mmc/host/cqhci.c | 5 ++++- drivers/mmc/host/cqhci.h | 1 + 2 files changed, 5 insertions(+), 1 deletion(-)