2015-12-23 14:39 GMT+08:00 Peter Guo <[email protected]>:
> Hi Ulf,
>
> Thanks for your suggestion, Please check my comment.
>
> BR
> Peter.guo
>
> -----Original Message-----
> From: Ulf Hansson [mailto:[email protected]]
> Sent: Tuesday, December 22, 2015 4:30 PM
> To: Peter Guo
> Cc: Alex Ballas; [email protected]; [email protected]; 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 <[email protected]> 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.
I noticed your discussing since it is me to send this
SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD patch long time ago. :)
As I know, It is not necessary to clear this transfer mode register
for a normal workable host. But I must do this quirk for my abnormal
host behavior.
>
>>>
>>> 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.
>
Here it is reason why I did not met same issue with yours in tuning
mode when I am using this quirk, since I just use mmc_send_tuning to
do software tuning, which is precondition of using this quirk.
>>>
>>> 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
>
>
--
---
Vincent Wan(Zongshun)
www.mcuos.com
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
