Re: [MFD] Patchset UPDATED

Pierre Ossman <drzeus-list@xxxxxxxxx> · Thu, 24 Jul 2008 01:09:18 +0200

On Mon, 21 Jul 2008 13:28:48 +0100
ian <spyro@xxxxxxx> wrote:

> On Mon, 2008-07-21 at 14:02 +0200, Pierre Ossman wrote:
> > > How would I do that? the hardware isnt going to just turn round and say
> > > 'ow!' when I prod it.
> 
> > It should. That's how I figured out all the bits for the sdhci driver.
> > There aren't that many attributes defining a response type, so you
> > should be able to figure out the different bits.
> 
> I have a problem with this approach.
> 
> First, reverse engineering working drivers from winCE, whilst admittedly
> not the definitive reference, these bits ARE used.
> 

It doesn't mean they do anything of value though. :)

> Secondly, whilst things might *appear* to work when they arent set this
> way, its impossible to tell wether the bits may affect the hardwares
> state machine or error handling in other ways that dont show up
> immediately.

Not likely. There has been one instance where the value (for two
otherwise identical response types) actually meant something to the
controller, and that was for the OMAP controller. That hw checks
certain bits of the responses that contain a status reply and set an
error register if any of those bits are set. This is of course not
in line with the layering of Linux' MMC layer, so these days the driver
just ignores that error bit.

> 
> I'll go over the bits, but unless I see good reasons not to set them as
> I do already, I am reluctant to change them.
> 

I still think using the types is a bad idea, but I won't NAK the patch
because of it.

> > > Who knows? I was hoping to leave that there until I can get round to
> > > implementing SDIO on this chipset, just in case. 
> > 
> > I don't see how SDIO changes anything. It's just interrupt handling.
> 
> Not on this chip. It involves a complete extra set of 'nearly the same'
> registers that duplicate the main register set. I have yet to work out
> why or how it works. It kinda looks like theres two HCs - an SD one and
> an SDIO one, that can be switched between at will.
> 

Funky. I wonder if it's to work around bugs/limitations/"features" in
some Wintendo driver. Almost all sdhci hardware is incredibly broken to
work around the fact that Microsoft's driver doesn't support MMC.

> I think the constant is clearer. I can look at it and go 'oh, thats the
> lowest clock speed' and not have to calculate it. If you like, I could
> put
> 
> mmc->f_min = 46875; /* 24000000 / 512 */
> 

That would make me happier, yes. It should be clear that this is not a
fundamental, but a derived value.

As for your latest version of the code, there's just a few things:

> +       if (cmd->opcode == 12 && !cmd.arg) {
> +               iowrite16(0x001, ctl + CTL_STOP_INTERNAL_ACTION);
> +               return 0;
> +       }

A comment would be nice. This isn't obvious to the MMC noob.

> +	buf = (unsigned short *)(tmio_mmc_kmap_atomic(host, &flags) +
> +	      host->sg_off);

I assume this platform has no highmem to deal with? You might want to
have a look at the new sg iterator stuff that just hit Linus' tree. It
is used in sdhci to handle mapping the sg list in a safe manner.

> +	/*FIXME - other drivers allow an optional stop command of any given type
> +	 *        which we dont do, as the chip can auto generate them.
> +	 *        Perhaps we can be smarter about when to use auto CMD12 and
> +	 *        only issue the auto request when we know this is the desired
> +	 *        stop command, allowing fallback to the stop command the
> +	 *        upper layers expect. For now, we do what works.
> +	 */
> +

I couldn't see a check for this.

> +	if (mrq->data) {
> +		ret = tmio_mmc_start_data(host, mrq->data);
> +		if (ret)
> +			goto fail;
> +	}

To be completely correct, you should probably set mrq->data->error, not
mrq->cmd->error here.

> +	ret = tmio_mmc_start_command(host, mrq->cmd);
> +
> +	if (!ret)
> +		return;

Shouldn't you have goto fail; here?

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.

Re: [MFD] Patchset *UPDATED*

Re: [MFD] Patchset UPDATED