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

[Pierre Ossman]

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