RE: [PATCH V1 07/11] mmc: cqhci: add quirk for setting DCMD CMD_TIMING

[Hunter, Adrian]

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