RE: 1217:8520 [Dell Latitude E7450] O2 Micro, SD/MMC Card Reader doesn't work

[Peter Guo <peter.guo@bayhubtech.com>]

Hi Ulf,

Thanks for your suggestion, Please check my comment.

BR
Peter.guo

-----Original Message-----
From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
Sent: Tuesday, December 22, 2015 4:30 PM
To: Peter Guo
Cc: Alex Ballas; adam8157@gmail.com; linux-mmc@VGER.KERNEL.ORG; Adrian Hunter; Russell King - ARM Linux
Subject: Re: 1217:8520 [Dell Latitude E7450] O2 Micro, SD/MMC Card Reader doesn't work

+ Adrian, Russell

On 22 December 2015 at 04:24, Peter Guo <peter.guo@bayhubtech.com> wrote:
>> Hi Ulf & Alex,
>>
>> Below is the complete analysis of this issue.
>>
>> The basic flow of send command is as below:
>> (1) Upper layer  prepare the command parameter;
>> (2) Call sdhci_send_command, and sdhci_send_command will call 
>> sdhci_set_transfer_mode  to set Host Register 0xC;
>> (3) Call sdhci_writew(host, SDHCI_MAKE_CMD(cmd->opcode, flags),
>> SDHCI_COMMAND) to send command;
>>
>> For Linux kernel mainstream (without any quirk), MMC case:
>> (1) It will call  mmc_get_ext_csd and this is an DMA transfer, then DMA enable bit will be set to 1 (0xC[0]=1'b1).
>> (2) Then next Command is mmc_switch(cmd6) which is none-data command, the driver will keep DMA enable bit to 1; this will let our Host failed to execute the CMD06.
>>
>> According to SD Host Spec(SD Host Controller Specification verson 4.1 page 41):
>> Bit0 of  Transfer Mode Register(0x0C) is DMA Enable bit, this bit set to 1 means a DMA operation shall begin when  Host Driver writes to the upper byte of Command Register(0x0F).
>>
>> The DMA enable bit should be set to zero for this none-data command(CMD6).
> Our host follow the spec. so I think it should be the sdhci driver’s issue (sdhci_set_transfer_mode).
>>
>>
>> So We tried  SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD to fix above issue.

>This quirk seems a bit strange. It looks like a generic problem being solved by the wrong approach. Although, my knowledge of sdhci HW is limited so I might be wrong.

>Why doesn't sdhci *always* reset the related registers when the command or data transfer gets *completed*? Instead as currently, delaying that until the *next* request is started and via using SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD?

Yes, this is one solution. We can clear TRANSMER_MODE register(0xC) after SD cmd done to solve current issue, below is the change method:
static void sdhci_tasklet_finish(unsigned long param) {
	.....
	host->mrq = NULL;
	host->cmd = NULL;
	host->data = NULL;
	sdhci_writew(host, 0x0, SDHCI_TRANSFER_MODE);//Add Clear Transfer Mode here
	.....
}

But the Disadvantage of this method is: Driver need do additional one register write operation after each command. Please check whether you can accept this change or not. Also it is looks weird to touch setting registers here.

>>
>> But it encounter new problem as Alex reported for tuning command as tuning command flow in sdhci_execute_tuning function is:
>> (1). cmd.data = NULL;
>> (2). sdhci_writew(host, SDHCI_TRNS_READ, SDHCI_TRANSFER_MODE); (3). 
>> sdhci_send_command(host, &cmd); As data is null, this quirk will  set 
>> 0x0C[0:15] to ALL zero before send tuning command including SDHCI_TRNS_READ, and the tuning command will failed.

>I see.

>Sdhci's sdhci_execute_tuning() function must be converted to use mmc_send_tuning().

>This means the regular request path will be maintained and thus no special treatment should be needed.

>Nevertheless, if my suggestions around changing sdhci's default behaviour instead of using SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD, >perhaps you don't need to fix sdhci_execute_tuning() to solve the error here!?

I have checked the mmc_send_tuning flow, and it can't replace sdhci_execute_tuning because the tuning command for sdhci.c is special command compare with normal command for 2 points: 
(1) It don't generate Command Complete Interrupt(register 0x30 bit0)
(2) It has no data for host driver to read but SDHCI_TRNS_READ flag need to be set.

>>
>> So, can we commit one patch as below to fix this DMA enable bit for non-data command bug if you cannot accept add new QUIRK?

>Nope, no new quirks.

>> Code should like below(sdhci_set_transfer_mode):
>>                 if (data == NULL) {
>>                                 if (host->quirks2 &
>>                                                 SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD) {
>>                                                 sdhci_writew(host, 0x0, SDHCI_TRANSFER_MODE);
>>                                 } else {
>>                                 /* clear Auto CMD settings for no data CMDs */
>>                                                 mode = sdhci_readw(host, SDHCI_TRANSFER_MODE);
>>                                                 sdhci_writew(host, mode & ~(SDHCI_TRNS_AUTO_CMD12 | SDHCI_TRNS_DMA    /*Clear DMA enable bit also for no data CMDs*/
>>                                                                 SDHCI_TRNS_AUTO_CMD23), SDHCI_TRANSFER_MODE);
>>                                 }
>>                                 return;
>>                 }

>Thanks for the suggested solution, although I think this looks like yet another hacky workaround for a generic problem.

>Before I further consider that change, can you please look into my suggestions above and see if any of those can simplify the solution?

>[...]


So my suggestion is:
(1) mmc_send_tuning can't replace sdhci_execute_tuning as tuning command is special one;
(2) modify the sdhci_set_transfer_mode to clear SDHCI_TRNS_DMA without quirk is better than clear transfer mode after command done. From my point of view, SDHCI_TRNS_DMA should be cleared for no-data command before issue command. It follows the spec.


BR
Peter.Guo