> -----Original Message----- > From: Ritesh Harjani [mailto:riteshh@xxxxxxxxxxxxxx] > Sent: Wednesday, March 13, 2019 4:31 AM > To: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>; Hunter, Adrian > <adrian.hunter@xxxxxxxxx>; ulf.hansson@xxxxxxxxxx; robh+dt@xxxxxxxxxx; > mark.rutland@xxxxxxx; Asutosh Das <asutoshd@xxxxxxxxxxxxxx> > Cc: thierry.reding@xxxxxxxxx; Jonathan Hunter <jonathanh@xxxxxxxxxx>; > Aniruddha Tvs Rao <anrao@xxxxxxxxxx>; linux-tegra@xxxxxxxxxxxxxxx; linux- > kernel@xxxxxxxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx > 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@xxxxxxxxxx> > >>>> --- > >>>> 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; > >>>>
