Re: [PATCH 1/5] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 6784 bytes --]

On Sun, Dec 15, 2013 at 03:20:19PM +0100, Hans de Goede wrote:
> On 12/15/2013 02:44 PM, Maxime Ripard wrote:
> >Hi Hans,
> >
> >I won't comment on the MMC driver itself, and leave Chris comment on
> >that, but still, I have a few things.
> >
> >On Sat, Dec 14, 2013 at 10:58:11PM +0100, Hans de Goede wrote:
> >>From: David Lanzendörfer <david.lanzendoerfer-Z7Kmv9EsliU@public.gmane.org>
> >>
> >>This is based on the driver Allwinner ships in there Android kernel sources.
> >>
> >>Initial porting to upstream kernels done by David Lanzendörfer, additional
> >>fixes and cleanups by Hans de Goede.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> >Your commit log is a bit sparse. What capabilities this controller
> >has? Is it using DMA? If so, how? What SoCs are supported/it has been
> >tested on? etc.
> 
> David, since you'll be doing v2 I guess you'll be filling in v2. FYI
> I've tested this on sun4i-a10, sun5i-a10s sun5i-a13 and sun7i-a20.
> 
> It uses dma in bus-master mode using a built-in designware idmac controller,
> which is identical to the one found in the mmc-dw hosts. Note the rest of
> the host is not identical, I've looked into reusing the mmc-dw driver but
> that does not seem like a good idea (manual sending stop commands
> versus auto stop on sunxi, completely different registers, etc.).

Thanks :)

This is exactly what I expect from a commit log of such driver ;)

> >>+static const struct of_device_id sunxi_mmc_of_match[] = {
> >>+	{ .compatible = "allwinner,sun4i-mmc", },
> >>+	{ .compatible = "allwinner,sun5i-mmc", },
> >
> >Please use sun5i-a13-mmc as your compatible.
> 
> Can you explain a bit why? In essence currently we have
> 2 versions of the mmc controller, those found on sun4i
> and those found on sun5i and sun7i. I thought that the
> norm was to use the oldest soc version in which a revision
> of an ip-block first appears as the compatible string ?

Indeed.

> Note I've tested this with both a13 and a10s SOCs, if we
> add a sun5i-a13-mmc we should also add a sun5i-a10s-mmc
> and a sun5i-a20-mmc, or would that then be sun7i-a20-mmc?

And the A13 has been the first SoC in the sun5i family, hence why we
should use sun5i-a13 as the prefix here. If the A10s and A20 would not
have been compatible with the A13 MMC controller, we would have used
sun5i-a10s-mmc and sun7i-a20-mmc, respectively.

> To me just having sun5i-mmc for sun5i+ socs seems simpler.

And if we ever find out that A10s or A13 differs in some way, we will
end up introducing a compatible that will be sun5i-a10s-mmc, along
with the sun5i-mmc we already have, which is not really the more
consistent thing we would have done.

> <snip>
> 
> >>+	mmc->ops		= &sunxi_mmc_ops;
> >>+	mmc->max_blk_count	= 8192;
> >>+	mmc->max_blk_size	= 4096;
> >>+	mmc->max_segs		= PAGE_SIZE / sizeof(struct sunxi_idma_des);
> >>+	mmc->max_seg_size	= (1 << host->idma_des_size_bits);
> >>+	mmc->max_req_size	= mmc->max_seg_size * mmc->max_segs;
> >>+	/* 400kHz ~ 50MHz */
> >>+	mmc->f_min		=   400000;
> >>+	mmc->f_max		= 50000000;
> >
> >Hmmm, the tables earlier seem to suggest it can do much more than that.
> 
> I know, but this is what the allwinner android kernels are using, actually
> in case of sdc3 they are putting 200000000 in f_max (as that is often
> used for sdio cards) but then later in set_ios they clamp the passed
> in clock to 47000000 Mhz, so I seriously doubt that 200Mhz has actually
> worked. Hence I've simply gone for a safe range for now. If someone has
> cards capable of doing 200 MHz we could certainly run various tests and
> try to improve this, but for now this seems a sane range to start with.

That's probably something that you should mention in your comment then :)

> <snip>
> 
> >> +	/* indicator pins */
> >> +	int wp_pin;
> >> +	int cd_pin;
> >> +	int cd_mode;
> >> +#define CARD_DETECT_BY_GPIO_POLL (1)	/* mmc detected by gpio check */
> >> +#define CARD_ALWAYS_PRESENT      (2)	/* mmc always present */
> >
> > Just a question here, have you tested to use the external interrupts?
> > (Is that even possible on the pins that are used as card detect?)
> >
> 
> No I've not tried that, there was code for this in the original allwinner
> driver, but no boards were actually using it.
> 
> As for it being possible on the pins being used, it is possible on (most)
> port H pins and the cubie* and a20-olinuxino-micro boards are using PH pins
> for cd. Others are not.  One thing which worries me about this is debouncing,
> using polling is automatically debounced, using an external interrupt won't
> be. Ideally there would be some common code somewhere to deal with gpio
> connected buttons (which this in essence is) which does debouncing ...
> 
> Again I think this is something best left as a future enhancement.

Yep, definitely, I was just being curious :)

> >>diff --git a/include/linux/clk/sunxi.h b/include/linux/clk/sunxi.h
> >>new file mode 100644
> >>index 0000000..1ef5c89
> >>--- /dev/null
> >>+++ b/include/linux/clk/sunxi.h
> >>@@ -0,0 +1,22 @@
> >>+/*
> >>+ * Copyright 2013 - Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >>+ *
> >>+ * This program is free software; you can redistribute it and/or modify
> >>+ * it under the terms of the GNU General Public License as published by
> >>+ * the Free Software Foundation; either version 2 of the License, or
> >>+ * (at your option) any later version.
> >>+ *
> >>+ * This program is distributed in the hope that it will be useful,
> >>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
> >>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >>+ * GNU General Public License for more details.
> >>+ */
> >>+
> >>+#ifndef __LINUX_CLK_SUNXI_H_
> >>+#define __LINUX_CLK_SUNXI_H_
> >>+
> >>+#include <linux/clk.h>
> >>+
> >>+void clk_sunxi_mmc_phase_control(struct clk_hw *hw, u8 sample, u8 output);
> >>+
> >>+#endif
> >
> >Hmmm, I see no implementation for this function. Didn't you forget
> >a file here? (and it should probably be a separate patch anyway).
> 
> The implementation is part of Emilio's clk branch, but he forgot
> to add a header for it, so I'm fixing that up here, I guess this
> should go through Emlio's tree as a separate patch.

As far as I know, this work has never been posted, let alone
merged. Anyway, the dependencies you have is something that you should
mention in your cover letter, so that we know what to merge, in which
order, and when to merge it.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]