> Vipin Bhandari <[email protected]> writes:
>
> > @@ -220,8 +221,7 @@ static void davinci_fifo_data_trans(struct
> > mmc_davinci_host *host,
> > unsigned int i;
> >
> > 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.
> > mmc_davinci_sg_to_buf(host);
> > }
> >
> > @@ -312,10 +312,6 @@ static void mmc_davinci_start_command(struct
> > mmc_davinci_host *host,
> > /* Set command index */
> > cmd_reg |= cmd->opcode;
> >
> > - /* 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.
> > @@ -358,10 +354,12 @@ static void mmc_davinci_start_command(struct
> > mmc_davinci_host *host,
> >
> > /*
> > * Before non-DMA WRITE commands the controller needs priming:
> > - * FIFO should be populated with 32 bytes
> > + * FIFO should be populated with 32 bytes i.e. whatever is the FIFO size
> > */
> > - 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...
> > @@ -736,6 +730,14 @@ static unsigned int calculate_freq_for_card(struct
> > mmc_davinci_host *host,
> > if (mmc_freq > mmc_req_freq)
> > mmc_push_pull = mmc_push_pull + 1;
> >
> > + /* Convert ns to clock cycles */
> > + 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.
> > @@ -863,10 +876,6 @@ davinci_abort_data(struct mmc_davinci_host *host,
> > struct mmc_data *data)
> > {
> > u32 temp;
> >
> > - /* 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).
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
