Re: [PATCH] add support PXA168/PXA910/MMP2 SD Host Controller

Eric Miao
2010-09-21T10:18:21Z

On Tue, Sep 21, 2010 at 6:11 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
>> >> >> diff --git a/drivers/mmc/host/sdhci-pxa.c b/drivers/mmc/host/sdhci-pxa.c
>> >> >> new file mode 100644
>> >> >> index 0000000..3e091c1
>> >> >> --- /dev/null
>> >> >> +++ b/drivers/mmc/host/sdhci-pxa.c
>> >> >> @@ -0,0 +1,253 @@
>> >> >> +/* linux/drivers/mmc/host/sdhci-pxa.c
>> >> >> + *
>> >> >> + * Copyright 2010 Marvell
>> >> >> + *      Zhangfei Gao <zhangfei.gao@marvell.com>
>> >> >> + *
>> >> >> + * This program is free software; you can redistribute it and/or modify
>> >> >> + * it under the terms of the GNU General Public License version 2 as
>> >> >> + * published by the Free Software Foundation.
>> >> >> + */
>> >> >> +
>> >> >> +/* Supports:
>> >> >> + * SDHCI support for MMP2/PXA910/PXA168
>> >> >> + *
>> >> >> + * Based on sdhci-platfm.c
>> >> >> + */
>> >> >
>> >> > Why is it only "based on" and not directly using it?
>> >>
>> >> Thanks for your suggestion.
>> >> This is the first stage, already implement sd card function, we will
>> >> have some patches later to enhance the driver.
>> >> We have to access many private registers to enable specific control.
>> >> Besides some sdio device need some specific control, like marvell 8787
>> >> host sleep, it is much reasonable to implement inside the driver.
>> >
>> > Hmm, which hooks are missing for you to implement this as an extension
>> > of the pltfm-driver? Maybe it makes sense to simply add this hook?
>>
>> We refered several mmc host drivers, such as sdhci-s3c.c,
>> sdhci-spear.c, and we want to align with others, which also meet pxa
>> requirement and low risk to transfer from the existing driver.
>
> I see. It is true that the pltfm-part is not widely used up to now. The
> question is if it might be a good idea to change this? There have been
> voices asking to merge the s3c-driver into the pltfm one. If you look at
> sdhci-cns3xxx.c, it looks clearer and reduces code duplication. The
> current state of the pltfm-driver might not cover all cases (yet), true,
> but those should be at least identified IMHO. This will help us for
> similar decisions in the future.
>
>> We have three or four device in one soc with the same driver, for
>> example one for sd, one for wifi, one for emmc, etc.
>> Each device have specific clock provider, which should be opened at
>> start, and closed when no operation to same power.
>> Specific quirk is needed for different device, which should be
>> transfered from platform data, for example on-chip wifi alwayes stay
>> on chip and emmc may require sd_clk free running to init.
>> The max_speed also may be different for board issue though controller
>> support max is 50M, which could get from capability.
>
> I guess to fully understand all constraints, one must really working
> with your platform, what I don't do. I agree that a nicely working
> driver is better than no driver; however, I fear once a driver hit the
> mainline being non-pltfm, it will hardly be converted later, even if it
> was considered to be worthwhile. So this is why I ask initially if it
> couldn't be done.
>

My two cents, considering the quirks and several other things that
sdhci-pltfm.c is missing, e.g. suspend/resume and clk handling. I'd
prefer a diverge at this time, until we figure out what is common to
all sdhci platform bus based driver, and get those common part back
into sdhci-pltfm.c

> Chris, do you see a rule of thumb here? Or what are your preferences?
>
> Kind regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAkyYhMQACgkQD27XaX1/VRsK1wCfaGVLkGpVEsrn+OQrpPBVHa54
> yDAAniBRPOamvs7wU8krhKMjjWqMxn9q
> =zQvu
> -----END PGP SIGNATURE-----
>
>