Hi Ulf, Thanks for your suggestion, Please check my comment.
BR Peter.guo -----Original Message----- From: Ulf Hansson [mailto:ulf.hans...@linaro.org] Sent: Tuesday, December 22, 2015 4:30 PM To: Peter Guo Cc: Alex Ballas; adam8...@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....@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
