David, > From: David Brownell [mailto:[email protected]] > Sent: Thursday, April 16, 2009 2:39 AM > > > Vipin Bhandari <[email protected]> writes: > > > > > > > > > if (host->buffer_bytes_left == 0) { > > > - host->sg_idx++; > > > - BUG_ON(host->sg_idx == host->sg_len); > > > + host->data->sg = sg_next(host->data->sg); > > Shouldn't that be using a temp, e.g. host->sg = sg_next(host->sg)? > > Otherwise this seems like it will be clobbering the mmc_data fields, > and have no way to restore them when it completes. Not a good idea, > even if *today* it may happen not to break anything.
I'll implement the same and submit the patch soon. > > > - /* Setting initialize clock */ > > > - if (cmd->opcode == 0) > > > - cmd_reg |= MMCCMD_INITCK; > > > - > > > /* Enable EDMA transfer triggers */ > > > if (host->do_dma) > > > cmd_reg |= MMCCMD_DMATRIG; > > I think the point of "don't check cmd->opcode" was either that > (a) this should be done as part of the powerup sequence, which > requires those clocks to be sent, or else (b) use the actual > opcode symbol. > > Not sending those 70-odd clocks seems, at least, unwise. Please refer to function mmc_davinci_set_ios() and inside the if statement for ios->power_mode == MMC_POWER_UP. 70 odd clocks are send here as you mentioned in point a > > > - if (!host->do_dma && (host->data_dir == DAVINCI_MMC_DATADIR_WRITE)) > > > - davinci_fifo_data_trans(host, 32); > > > + if (!host->do_dma && (host->data_dir == DAVINCI_MMC_DATADIR_WRITE) > && > > > + ((cmd->opcode == MMC_WRITE_BLOCK) || > > > + (cmd->opcode == MMC_WRITE_MULTIPLE_BLOCK))) > > > + davinci_fifo_data_trans(host, DAVINCI_MMC_FIFO_SIZE_BYTE); > > > > > > writel(cmd->arg, host->base + DAVINCI_MMCARGHL); > > > writel(cmd_reg, host->base + DAVINCI_MMCCMD); > > That confuses me, and seems like it goes against the standard "host > adapters > should not care about specific commands" policy. Won't it prevent SDIO > from > working, for example? Would a simple "host->data != NULL" address the > same > issue? Can DATADIR_WRITE even be set to that value for a non-write > command? > > There are non-MMC/non-SD write commands... Please refer to the TRM of MMCSD which states as follow xxxxxxxxxxxxxxxxxxxx When starting the write transaction, the CPU is responsible for getting the FIFO ready to start transferring data by filling up the FIFO with data prior to invoking/posting the write command to the card. Filling up the FIFO is a requirement since no interrupt/event generates at the start of the write transfer. xxxxxxxxxxxxxxxxxxxx This is required only in case of non dma based transfers and that too during write operation. > > > + if (mmc_req_freq < 400000) > > > + ns_in_one_cycle = 1000000 / ((cpu_arm_clk > > > + / (2 * (mmc_push_pull + 1)))/1000); > > > + else > > > + ns_in_one_cycle = 1000000 / ((cpu_arm_clk > > > + / (2 * (mmc_push_pull + 1)))/1000000); > > > + > > > return mmc_push_pull; > > > } > > > > > This can't be correct on systems with multiple MMC/SD controllers. > > It's updating a global to hold a card-specific value ... where it > will be used by cards that are using a different clock rate. That > value should be in the per-controller state. Point well taken. I'll implement what you suggest. > > > - /* record how much data we transferred */ > > > - temp = readl(host->base + DAVINCI_MMCNBLC); > > > - data->bytes_xfered += (data->blocks - temp) * data->blksz; > > > - > > > /* reset command and data state machines */ > > > temp = readl(host->base + DAVINCI_MMCCTL); > > > writel(temp | MMCCTL_CMDRST | MMCCTL_DATRST, > > This doesn't look right ... doesn't it introduce misbehavior > in some of the "mmctest" cases? It's at least under-reporting > the number of bytes transferred (and needlessly so). This function is an abort data call which happens during timeout or crc errors. There is no way to tell how much of the data has been transferred, as it is not acknowledged. Putting data->bytes_xfered += 0; seems to be right but as it should have been already 0 initially so not reqd. to update it again. > _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
