Re: [PATCH v2 4/7] atmel-mci: support multiple mmc slots

From: Pierre Ossman
Date: Sun Oct 12 2008 - 04:40:50 EST


On Sun, 5 Oct 2008 18:21:27 +0200
Haavard Skinnemoen <haavard.skinnemoen@xxxxxxxxx> wrote:

>
> if (ios->clock) {
> + unsigned int clock_min = ~0U;
> u32 clkdiv;
>
> - if (!host->mode_reg)
> + spin_lock_bh(&host->lock);
> + if (!host->mode_reg) {
> clk_enable(host->mck);
> + mci_writel(host, CR, MCI_CR_SWRST);
> + mci_writel(host, CR, MCI_CR_MCIEN);
> + }
>
> - /* Set clock rate */
> - clkdiv = DIV_ROUND_UP(host->bus_hz, 2 * ios->clock) - 1;
> + /*
> + * Use mirror of ios->clock to prevent race with mmc
> + * core ios update when finding the minimum.
> + */
> + slot->clock = ios->clock;
> + for (i = 0; i < ATMEL_MCI_MAX_NR_SLOTS; i++) {
> + if (host->slot[i] && host->slot[i]->clock
> + && host->slot[i]->clock < clock_min)
> + clock_min = host->slot[i]->clock;
> + }
> +

Hmm... This does not cover the power up case. You can only ignore the
other slot's clock setting if it is unpowered.

> + /* Calculate clock divider */
> + clkdiv = DIV_ROUND_UP(host->bus_hz, 2 * clock_min) - 1;
> if (clkdiv > 255) {
> dev_warn(&mmc->class_dev,
> "clock %u too slow; using %lu\n",
> - ios->clock, host->bus_hz / (2 * 256));
> + clock_min, host->bus_hz / (2 * 256));
> clkdiv = 255;
> }
>

Has this ever happened, or is it just a bit defensive? :)

(not that there is something wrong with that)

> default:
> /*
> * TODO: None of the currently available AVR32-based
> * boards allow MMC power to be turned off. Implement
> * power control when this can be tested properly.
> + *
> + * We also need to hook this into the clock management
> + * somehow so that newly inserted cards aren't
> + * subjected to a fast clock before we have a chance
> + * to figure out what the maximum rate is. Currently,
> + * there's no way to avoid this, and there never will
> + * be for boards that don't support power control.
> */
> break;
> }

Hmm again... As you've pointed out, this could be a problem. The card
should survive getting jittery power during the insertion, but I
wouldn't chance having the clock running at that point (which will
happen if the other slot is populated).

I don't see a decent way of solving this, so I guess we'll have to
stick our heads in the sand for now...

(did I mention that I think this slot multiplexing thing is a bad idea?)

> @@ -922,9 +1191,11 @@ static void atmci_write_data_pio(struct atmel_mci *host)
> mci_writel(host, IDR, (MCI_NOTBUSY | MCI_TXRDY
> | ATMCI_DATA_ERROR_FLAGS));
> host->data_status = status;
> + data->bytes_xfered += nbytes;
> + smp_wmb();
> atmci_set_pending(host, EVENT_DATA_ERROR);
> tasklet_schedule(&host->tasklet);
> - break;
> + return;
> }
> } while (status & MCI_TXRDY);
>
> @@ -937,29 +1208,26 @@ done:
> mci_writel(host, IDR, MCI_TXRDY);
> mci_writel(host, IER, MCI_NOTBUSY);
> data->bytes_xfered += nbytes;
> + smp_wmb();
> atmci_set_pending(host, EVENT_XFER_COMPLETE);
> }
>

I'm not sure I commented on this during the last run, but bytes_xfered
should only be updated upon confirmation from the card, not as it goes
out over the bus (there might be CRC error for instance).

Rgds
--
-- Pierre Ossman

Linux kernel, MMC maintainer http://www.kernel.org
rdesktop, core developer http://www.rdesktop.org

WARNING: This correspondence is being monitored by the
Swedish government. Make sure your server uses encryption
for SMTP traffic and consider using PGP for end-to-end
encryption.

Attachment: signature.asc
Description: PGP signature