Re: [PATCH 0/5] consolidate sdhci pltfm & OF drivers and get them self registered

[Kyungmin Park]

On Wed, Apr 20, 2011 at 2:47 AM, Wolfram Sang  wrote:
>
>> BTW, Are there reason to omit the sdhci-s3c.c? Maybe Mr. Jung will handle it.
>
> The difference is that sdhci-s3c.c was more like a fork of sdhci-pltfm while
> the others were users of it. With the new interface, s3c can be converted 
> using
> pltfm more easily. If somebody is willing to do that, this is very much
> welcome. I'd suggest waiting for Shawn's interface to stabilize, though.
Okay Thanks, we will prepare it.
>
> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: USB SDIO/SD/MMC Host Controller (VUB300) driver Re-Re-Resubmission

[Wolfram Sang]

Hi Tony,

> Add a driver for Elan Digital System's VUB300 chip
> which is a USB connected SDIO/SDmem/MMC host controller.
> A VUB300 chip enables a USB 2.0 or USB 1.1 connected host
> computer to use SDIO/SD/MMC cards without the need for
> a directly connected, for example via PCI, SDIO host
> controller.
> 
> Signed-off-by: Anthony F Olech 
> ---
> This is the forth submission attempt.

Looks to me that the concerns have not been addressed since last submission.
I had a few a while ago, but Arnd added to it and summed it up nicely:

http://ns3.spinics.net/lists/linux-mmc/msg06693.html

> There are no errors reported by scripts/checkpatch.pl

Just to make sure:  This only means that there are no obvious (= detectable by
a script) flaws. That makes it ready for submission, not necessarily for
inclusion. So, just resending will not be enough.

> This driver has been tested on
> a) 32bit x86
> b) 64bit x86
> c) dual processor
> d) PowerPC

The test coverage is great, yet the driver needs to comply to the kernel coding
rules. I'd suggest to look at Arnd's review and fix the issues.

Regards,

  Wolfram

-- 
Pengutronix e.K.   | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/  |


signature.asc
Description: Digital signature

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Guennadi Liakhovetski]

Thanks to all for clarifications. Since everyone is convinced, that that 
idle function in mmc bus.c is appropriate, I restored it and managed to 
achieve my goals also without it by adjusting the platform runtime pm and 
power domain prototype support.

But I still need those "cheating" calls to

pm_runtime_put_noidle(&pdev->dev);
and
pm_runtime_get_noresume(&pdev->dev);

in the sh_mmcif.c driver. If I use the patch as posted at

http://article.gmane.org/gmane.linux.ports.sh.devel/10724

but without those two calls, and load and unload the driver, while a card 
is plugged in, unloading the driver doesn't power down the interface, 
because the usage_count == 1 also after the kernel has soft-ejected the 
card

mmc0: card 0001 removed

With my put_noidle() / get_noresume() hack all cases of modprobe / rmmod, 
card insert / eject work correctly.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: sdhci: Fix SDHCI_QUIRK_TIMEOUT_USES_SDCLK

[Chris Ball]

Hi Mark,

On Tue, Apr 19 2011, Mark Brown wrote:
> Commit 373e6a (mmc: sdhci: R1B command handling + MMC_CAP_ERASE) moved the
> handling of SDHCI_QUIRK_TIMEOUT_USES_SDCLK from sdhci_calc_timeout() to
> sdhci_add_host(). This causes division by zero errors on at least the S3C
> SDHCI controller as the quirk implementation needs host->clock set to work
> but host->clock has not been set when sdhci_add_host() is called.
>
> Fix this by backing out that portion of the change, the clock may vary at
> runtime anyway. It does occur to me that we may want to move the quirk to
> where we set the clock but this seems more invasive and I'm concerned
> about undesirable side effects.
>
> Signed-off-by: Mark Brown 

Thanks for catching this, pushed to mmc-next for .40.

- Chris.
-- 
Chris Ball  
One Laptop Per Child
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: sdhci: Fix SDHCI_QUIRK_TIMEOUT_USES_SDCLK

[Andrei Warkentin]

On Tue, Apr 19, 2011 at 12:44 PM, Mark Brown
 wrote:
> Commit 373e6a (mmc: sdhci: R1B command handling + MMC_CAP_ERASE) moved the
> handling of SDHCI_QUIRK_TIMEOUT_USES_SDCLK from sdhci_calc_timeout() to
> sdhci_add_host(). This causes division by zero errors on at least the S3C
> SDHCI controller as the quirk implementation needs host->clock set to work
> but host->clock has not been set when sdhci_add_host() is called.
>
> Fix this by backing out that portion of the change, the clock may vary at
> runtime anyway. It does occur to me that we may want to move the quirk to
> where we set the clock but this seems more invasive and I'm concerned
> about undesirable side effects.
>
> Signed-off-by: Mark Brown 
> ---
>  drivers/mmc/host/sdhci.c |    7 ---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index f79dead..da57bdb 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -616,6 +616,9 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
> struct mmc_command *cmd)
>                target_timeout = data->timeout_ns / 1000 +
>                        data->timeout_clks / host->clock;
>
> +       if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
> +               host->timeout_clk = host->clock / 1000;
> +
>        /*
>         * Figure out needed cycles.
>         * We do this in steps in order to fit inside a 32 bit int.
> @@ -626,6 +629,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
> struct mmc_command *cmd)
>         *     =>
>         *     (1) / (2) > 2^6
>         */
> +       BUG_ON(!host->timeout_clk);
>        count = 0;
>        current_timeout = (1 << 13) * 1000 / host->timeout_clk;
>        while (current_timeout < target_timeout) {
> @@ -1894,9 +1898,6 @@ int sdhci_add_host(struct sdhci_host *host)
>        if (caps & SDHCI_TIMEOUT_CLK_UNIT)
>                host->timeout_clk *= 1000;
>
> -       if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
> -               host->timeout_clk = host->clock / 1000;
> -
>        /*
>         * Set host parameters.
>         */
> --
> 1.7.4.1
>
>

Sorry about that. I thought I had the right idea.

I'll try to be more careful.

A
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] consolidate sdhci pltfm & OF drivers and get them self registered

[Wolfram Sang]

> BTW, Are there reason to omit the sdhci-s3c.c? Maybe Mr. Jung will handle it.

The difference is that sdhci-s3c.c was more like a fork of sdhci-pltfm while
the others were users of it. With the new interface, s3c can be converted using
pltfm more easily. If somebody is willing to do that, this is very much
welcome. I'd suggest waiting for Shawn's interface to stabilize, though.

Regards,

   Wolfram

-- 
Pengutronix e.K.   | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/  |


signature.asc
Description: Digital signature

[PATCH] mmc: sdhci: Fix SDHCI_QUIRK_TIMEOUT_USES_SDCLK

[Mark Brown]

Commit 373e6a (mmc: sdhci: R1B command handling + MMC_CAP_ERASE) moved the
handling of SDHCI_QUIRK_TIMEOUT_USES_SDCLK from sdhci_calc_timeout() to
sdhci_add_host(). This causes division by zero errors on at least the S3C
SDHCI controller as the quirk implementation needs host->clock set to work
but host->clock has not been set when sdhci_add_host() is called.

Fix this by backing out that portion of the change, the clock may vary at
runtime anyway. It does occur to me that we may want to move the quirk to
where we set the clock but this seems more invasive and I'm concerned
about undesirable side effects.

Signed-off-by: Mark Brown 
---
 drivers/mmc/host/sdhci.c |7 ---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index f79dead..da57bdb 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -616,6 +616,9 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
struct mmc_command *cmd)
target_timeout = data->timeout_ns / 1000 +
data->timeout_clks / host->clock;
 
+   if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
+   host->timeout_clk = host->clock / 1000;
+
/*
 * Figure out needed cycles.
 * We do this in steps in order to fit inside a 32 bit int.
@@ -626,6 +629,7 @@ static u8 sdhci_calc_timeout(struct sdhci_host *host, 
struct mmc_command *cmd)
 * =>
 * (1) / (2) > 2^6
 */
+   BUG_ON(!host->timeout_clk);
count = 0;
current_timeout = (1 << 13) * 1000 / host->timeout_clk;
while (current_timeout < target_timeout) {
@@ -1894,9 +1898,6 @@ int sdhci_add_host(struct sdhci_host *host)
if (caps & SDHCI_TIMEOUT_CLK_UNIT)
host->timeout_clk *= 1000;
 
-   if (host->quirks & SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK)
-   host->timeout_clk = host->clock / 1000;
-
/*
 * Set host parameters.
 */
-- 
1.7.4.1

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patchv3 2/5] MMC: Use CMD23 for multiblock transfers when we can.

[Andrei Warkentin]

On Tue, Apr 19, 2011 at 2:39 AM, Arnd Bergmann  wrote:
> On Tuesday 19 April 2011, Andrei Warkentin wrote:
>
>> So maybe this should be a blacklist for known bad cards. And the
>> entire support should be a "default-N" compile option for MMCs (not
>> SDs). That way someone who just does an "make oldconfig" will see
>> "CONFIG_MMC_BLK_CMD23 - I/O performance improvement for newer eMMC
>> cards, may cause degradation on older cards". What do you think?
>
> I'm not sure if I understand the distinction between MMC and SD
> here. Do you suggest we always enable it for SD but make it compile-time
> selected for MMC?

I did, because CMD23 support on SDs is a new feature mandatory on
UHS104 cards. However, I guess no matter
what you do, it's going to break something.  Right now I'm only aware
of Toshiba perf regressions.

>
> I generally argue against compile time options. A distribution
> integrator needs to choose a reasonable default, and giving them
> an option makes it possible to get it wrong.
>
> I believe the best way would be trying to warn people against
> regressions while going forward with this enabled unconditionally,
> unless we hear back from people that actually got regressions.

Alright. Then I'll just blacklist the known-affected Toshiba cards for
now, and we'll keep it enabled by default.

>
> We could perhaps key enabling the feature by the production date
> on the card, so it only gets turned on for new cards.
>

Unfortunately, date by itself is meaningless too (current Toshiba eMMCs)

A
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/12] mmc: use nonblock mmc requests to minimize latency

[Jae hoon Chung]

Hi Per..

2011/4/11 Per Forlin :
> On 9 April 2011 13:55, Jae hoon Chung  wrote:
>> Hi Per..
>>
>> I'm applied your patch..and sent the patch about dw_mmc.c.
>> I think good this approach..
>>
> Do you have any test results from the mmc_tests I added?
> I am interested in the results.

I didn't test with mmc_tests..but i tested with IOzone..
I'll test with mmc_test..then i'll share the results..

Regards,
Jaehoon Chung
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Alan Stern]

On Tue, 19 Apr 2011, Guennadi Liakhovetski wrote:

> On Tue, 19 Apr 2011, Ohad Ben-Cohen wrote:
> 
> > On Tue, Apr 19, 2011 at 1:46 PM, Guennadi Liakhovetski
> >  wrote:
> > > MMC bus PM operations implement a .runtime_idle() method, which calls
> > > pm_runtime_suspend(), but this call is not balanced by a resume
> > > counterpart,
> > 
> > What's the exact flow you refer to ?
> 
> Seeing a "struct dev_pm_ops" instance with .runtime_suspend(), 
> .runtime_resume(), and .runtime_idle() methods I understand, that 
> "suspend" and "resume" are two counterparts, that balance each other.

No, they do not balance each other.  There is no "suspend counter" or
"resume counter".  Whenever a suspend or resume call is made, it
overrides all previous calls.

This is different from dev_pm_info's usage_count value, which _is_ a
counter.  However it does not count suspend or resume calls; instead it
counts the number of routines that need to use the device.  When
usage_count drops to 0, the PM core assumes the device is no longer
being used and may therefore be suspended safely (at which point the
core invokes the pm_idle callback).  If usage_count > 0 then attempts
to call pm_runtime_suspend will fail.

>  Now 
> with "idle" I am not sure which method should balance it. With platform 
> devices in the generic case idle ends up calling 
> pm_generic_runtime_idle(), which then calls pm_runtime_suspend(). So, 
> there should be a balancing pm_runtime_resume() somewhere?

No.  If the device is idle, suspending it is appropriate.  The device 
will be resumed later when a driver needs to use it.

> > > which causes problems with repeated card-plug and driver-load
> > > cycles.
> > 
> > Can you please be more specific ? What are you trying to achieve, what
> > are the problems you encounter ?
> 
> See this patch:
> 
> http://article.gmane.org/gmane.linux.ports.sh.devel/10724
> 
> The purpose of that patch is to (1) implement runtime PM in a way, that 
> whenever the driver is unloaded or the card is ejected the interface is 
> powered down, and (2) implement system-wide PM. For (1) doing the usual
> 
> probe()
> {
>   ...
>   pm_runtime_enable();
>   pm_runtime_resume();
>   ...
> }
> 
> remove()
> {
>   ...
>   pm_runtime_suspend();
>   pm_runtime_dieabls();
>   ...
> }
> 
> set_ios()
> {
>   ...
>   if (power_down)
>   pm_runtime_put();
>   if (power_up)
>   pm_runtime_get_sync();
>   ...
> }
> 
> Doesn't work: some internal MMC counters become disbalanced and after one 
> card replug or driver reloading the interface gets stuck with power either 
> permanently on or off.

I'm not familiar enough with the inner workings of the MMC subsystem to 
help much with this.  You'd have to explain things in sufficient detail 
first.

> > >  Removing this method fixes the disbalance.
> > 
> > I'm not sure which disbalance you're referring to, but if you'll
> > remove this method you will break MMC/SDIO runtime PM.
> > 
> > More specifically, without having this ->runtime_idle() handler, the
> > last user giving up its power usage_count (e.g. by calling
> > pm_runtime_put{_sync}) will not end up powering down the MMC card.
> 
> How do they work then? Who does the pm_runtime_resume() to undo the 
> effects of the pm_runtime_suspend(), or is it the platform runtime-pm, 
> that is implementing the "idle" method wrongly?

Put it this way: When do you want the interface to be powered up and 
powered down?  That is, what events should cause the power level to 
change?  The code that handles those events is responsible for calling 
the appropriate PM runtime routines.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Ohad Ben-Cohen]

On Tue, Apr 19, 2011 at 4:23 PM, Guennadi Liakhovetski
 wrote:
> Seeing a "struct dev_pm_ops" instance with .runtime_suspend(),
> .runtime_resume(), and .runtime_idle() methods I understand, that
> "suspend" and "resume" are two counterparts, that balance each other. Now
> with "idle" I am not sure which method should balance it. With platform
> devices in the generic case idle ends up calling
> pm_generic_runtime_idle(), which then calls pm_runtime_suspend(). So,
> there should be a balancing pm_runtime_resume() somewhere?

There are many ways with which the dev_pm_ops handlers get called, but
none of them include imbalance.

E.g. take a look how pm_runtime_{get,put}_sync balance each other,
while involving all three runtime pm handlers that you've specified
(suspend/resume/idle).

Can you point out an existing device/flow that demonstrates a runtime
pm imbalance ?

>> More specifically, without having this ->runtime_idle() handler, the
>> last user giving up its power usage_count (e.g. by calling
>> pm_runtime_put{_sync}) will not end up powering down the MMC card.
>
> How do they work then? Who does the pm_runtime_resume() to undo the
> effects of the pm_runtime_suspend()

Let's take SDIO for example; note how sdio_bus_probe and
sdio_bus_remove balance each other with respect to runtime PM api
invocations.

> or is it the platform runtime-pm that is implementing the "idle" method 
> wrongly?

I'm not following what's wrong exactly. If you point out specific code
and specify exactly the issues you witness, it might be easier to
help.

Thanks,
Ohad.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Guennadi Liakhovetski]

On Tue, 19 Apr 2011, Ohad Ben-Cohen wrote:

> On Tue, Apr 19, 2011 at 1:46 PM, Guennadi Liakhovetski
>  wrote:
> > MMC bus PM operations implement a .runtime_idle() method, which calls
> > pm_runtime_suspend(), but this call is not balanced by a resume
> > counterpart,
> 
> What's the exact flow you refer to ?

Seeing a "struct dev_pm_ops" instance with .runtime_suspend(), 
.runtime_resume(), and .runtime_idle() methods I understand, that 
"suspend" and "resume" are two counterparts, that balance each other. Now 
with "idle" I am not sure which method should balance it. With platform 
devices in the generic case idle ends up calling 
pm_generic_runtime_idle(), which then calls pm_runtime_suspend(). So, 
there should be a balancing pm_runtime_resume() somewhere?

> > which causes problems with repeated card-plug and driver-load
> > cycles.
> 
> Can you please be more specific ? What are you trying to achieve, what
> are the problems you encounter ?

See this patch:

http://article.gmane.org/gmane.linux.ports.sh.devel/10724

The purpose of that patch is to (1) implement runtime PM in a way, that 
whenever the driver is unloaded or the card is ejected the interface is 
powered down, and (2) implement system-wide PM. For (1) doing the usual

probe()
{
...
pm_runtime_enable();
pm_runtime_resume();
...
}

remove()
{
...
pm_runtime_suspend();
pm_runtime_dieabls();
...
}

set_ios()
{
...
if (power_down)
pm_runtime_put();
if (power_up)
pm_runtime_get_sync();
...
}

Doesn't work: some internal MMC counters become disbalanced and after one 
card replug or driver reloading the interface gets stuck with power either 
permanently on or off.

> >  Removing this method fixes the disbalance.
> 
> I'm not sure which disbalance you're referring to, but if you'll
> remove this method you will break MMC/SDIO runtime PM.
> 
> More specifically, without having this ->runtime_idle() handler, the
> last user giving up its power usage_count (e.g. by calling
> pm_runtime_put{_sync}) will not end up powering down the MMC card.

How do they work then? Who does the pm_runtime_resume() to undo the 
effects of the pm_runtime_suspend(), or is it the platform runtime-pm, 
that is implementing the "idle" method wrongly?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: USB SDIO/SD/MMC Host Controller (VUB300) driver Re-Resubmission

[Arnd Bergmann]

On Tuesday 19 April 2011, Tony Olech wrote:
> What is wrong in wanting to support non-technical users?
> Especially since there are no backward compatible issues
> involved at all.

It's good to support non-technical users. Things that are
bad about your patch are:

* Have interfaces that work only for your customers. If a
  specific interface is a good thing to have, it certainly
  is good to have it for everyone.

* Fragmentation between drivers. In addition, if someone
  else needs a similar interface and introduces it in another
  driver, we can end up with incompatible ways of getting
  the same information, which is an absolute support nightmare
  for people that want to support all users.

* Not following the interface standards. You don't get to
  make the decision which interfaces are helpful and which
  ones are not, they have to be agreed upon. The files you were
  suggesting violate the rules about how they should look
  like, in particular having one value per file. Doing something
  else is very confusing to users that have reasonable expectations
  about the contents.

So even though you had the intentions to make support easier
for some people, you end up with a situation that makes it harder
to support for everyone.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/5] consolidate sdhci pltfm & OF drivers and get them self registered

[Kyungmin Park]

Hi,

BTW, Are there reason to omit the sdhci-s3c.c? Maybe Mr. Jung will handle it.

Thank you,
Kyungmin Park

On Tue, Apr 19, 2011 at 7:20 PM, Wolfram Sang  wrote:
> Hi Shawn,
>
> On Fri, Mar 25, 2011 at 04:48:46PM +0800, Shawn Guo wrote:
>> Here are what the patch set does.
>>
>> * Remove .probe and .remove hooks from sdhci-pltfm.c and make it be
>>   a pure common helper function providers.
>> * Add .probe and .remove hooks for sdhci pltfm drivers sdhci-cns3xxx,
>>   sdhci-dove, sdhci-tegra, and sdhci-esdhc-imx to make them self
>>   registered with calling helper functions created above.
>> * Migrate the use of sdhci_of_host and sdhci_of_data to
>>   sdhci_pltfm_host and sdhci_pltfm_data, so that OF version host and
>>   data structure works can be saved, and pltfm version works for both
>>   cases.
>> * Add OF common helper stuff into sdhci-pltfm.c, and make OF version
>>   sdhci drivers sdhci-of-esdhc and sdhci-of-hlwd become self
>>   registered as well, so that sdhci-of-core.c and sdhci-of.h can be
>>   removed.
>> * Consolidate the OF and pltfm esdhc drivers into one with sharing
>>   the same pair of .probe and .remove hooks.  As a result,
>>   sdhci-esdhc-imx.c and sdhci-of-esdhc.c go away, while
>>   sdhci-esdhc.c comes in and works for both MPCxxx and i.MX.
>> * Eliminate include/linux/mmc/sdhci-pltfm.h with moving stuff into
>>   drivers/mmc/host/sdhci-pltfm.h.
>>
>> And the benefits we gain from the changes are:
>>
>> * Get the sdhci device driver follow the Linux trend that driver
>>   makes the registration by its own.
>> * sdhci-pltfm.c becomes simple and clean as it only has common helper
>>   stuff there now.
>> * All sdhci device specific things are going back its own driver.
>> * The dt and non-dt drivers are consolidated to use the same pair of
>>   .probe and .remove hooks.
>> * SDHCI driver for Freescale eSDHC controller found on both MPCxxx
>>   and i.MX platforms is consolidated to use the same one .probe
>>   function.
>>
>> The patch set works against the tree below, and was only tested on
>> i.mx51 babbage board, all other targets were build tested.
>>
>>   git://git.secretlab.ca/git/linux-2.6.git devicetree/test
>>
>> Comments are welcomed and appreciated.
>
> First of all, thanks _a lot_ for doing this! Many people have promised
> to do such an approach, but you finally made it!
>
> Sorry for the long delay in reviewing it, I didn't want to rush a review
> so I needed some time for it which I didn't have much in the last weeks.
> Let's hope my battery will have enough power on my flight back to
> Germany ;) So, this is for now a purely "visual" review. I hope I can
> check V2 on real hardware then.
>
> The approach seems sensible, so have a look at my (mostly minor)
> comments inside the patches. However, there is one bigger piece missing.
> You converted all the drivers which had a seperate source-file and
> hooked into sdhci-pltfm.c. However, those are only those users which
> need additional code to work around the quirks. There are also users
> which can take the plain pltfm-driver with a properly set
> platform_data (check the thread "[PATCH] mmc: add SDHCI driver for STM
> platforms (V2)" for an example). Those have to be converted, too.
> Now the discussion could be if every of those users gets its own
> pltfm-.c or if we create something similat to
> sdhci-pltfm-generic, which can also be setup with platform_data like the
> old driver (/me likes the latter a bit more. If we don't change the name
> of the driver (not talking about the sourcefile) and keep it
> "sdhci-pltfm", then you wouldn't need to change all those users if you
> ensured it behaves the same.
>
> Also, I think the next version of this series should have all makers of
> a sdhci-pltfm user CCed so we give them a chance to report breakage. Or
> donate acks or tested-by.
>
> Regards,
>
>   Wolfram
>
> --
> Pengutronix e.K.                           | Wolfram Sang                |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> -BEGIN PGP SIGNATURE-
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk2tYe8ACgkQD27XaX1/VRsCRgCgkp+BmJJsLsKllKL0OI/BnO9F
> PRkAn1BpnXBv1exnkJFlqFAPe5O2Yt8w
> =GSqA
> -END PGP SIGNATURE-
>
> ___
> linaro-dev mailing list
> linaro-...@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Ohad Ben-Cohen]

On Tue, Apr 19, 2011 at 1:46 PM, Guennadi Liakhovetski
 wrote:
> MMC bus PM operations implement a .runtime_idle() method, which calls
> pm_runtime_suspend(), but this call is not balanced by a resume
> counterpart,

What's the exact flow you refer to ?

> which causes problems with repeated card-plug and driver-load
> cycles.

Can you please be more specific ? What are you trying to achieve, what
are the problems you encounter ?

>  Removing this method fixes the disbalance.

I'm not sure which disbalance you're referring to, but if you'll
remove this method you will break MMC/SDIO runtime PM.

More specifically, without having this ->runtime_idle() handler, the
last user giving up its power usage_count (e.g. by calling
pm_runtime_put{_sync}) will not end up powering down the MMC card.
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: USB SDIO/SD/MMC Host Controller (VUB300) driver Re-Resubmission

[Tony Olech]

On Tue, 2011-04-19 at 14:10 +0200, Arnd Bergmann wrote:
> On Tuesday 19 April 2011, Tony Olech wrote:
> > The purpose of this read-only interface is for VUB300 support
> > staff to obtain information from our customers. Our customers
> > are not like the people on this list. If you have ever tried
> > to do telephone support you would appreciate the difficulty
> > of getting a non-technical person to first of all find the
> > log files and then to extract the correct lines. It therefore
> > follows that if the 1 or 4 bit mode is useful info for the 
> > VUB300 support staff, then this read-only interface is the
> > appropriate single place for it to be provided. The overhead
> > of providing such an interface is negligible and the existance
> > of such an interface demonstrates that linux is no longer
> > designed only for geeks.
> 
> Wrong answer.
> 
> You are missing the point. Sysfs is not the place where you can
> put random interfaces you want for you own needs. By adding
> files there, you create a support burden for everyone who now
> has to maintain backwards-compatibility.
> 
> You cannot seriously ask for inclusion of a kernel interface
> just to fit the use of your support team at the expense of
> everyone else.
> 
> Please remove the sysfs attributes from the driver before resubmitting.
> We can have a separate discussion about core interfaces that will
> help everyone and that are consistent with how Linux kernels are
> supported.
> 
>   Arnd

You have not answered my points. If linux is to move from
the preserve of geeks, then support is an issue.

What is wrong in wanting to support non-technical users?
Especially since there are no backward compatible issues
involved at all.

Tony


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mmc: USB SDIO/SD/MMC Host Controller (VUB300) driver Re-Resubmission

[Arnd Bergmann]

On Tuesday 19 April 2011, Tony Olech wrote:
> The purpose of this read-only interface is for VUB300 support
> staff to obtain information from our customers. Our customers
> are not like the people on this list. If you have ever tried
> to do telephone support you would appreciate the difficulty
> of getting a non-technical person to first of all find the
> log files and then to extract the correct lines. It therefore
> follows that if the 1 or 4 bit mode is useful info for the 
> VUB300 support staff, then this read-only interface is the
> appropriate single place for it to be provided. The overhead
> of providing such an interface is negligible and the existance
> of such an interface demonstrates that linux is no longer
> designed only for geeks.

Wrong answer.

You are missing the point. Sysfs is not the place where you can
put random interfaces you want for you own needs. By adding
files there, you create a support burden for everyone who now
has to maintain backwards-compatibility.

You cannot seriously ask for inclusion of a kernel interface
just to fit the use of your support team at the expense of
everyone else.

Please remove the sysfs attributes from the driver before resubmitting.
We can have a separate discussion about core interfaces that will
help everyone and that are consistent with how Linux kernels are
supported.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3 v2] MMC: add runtime and system power-management support to the MMCIF driver

[Guennadi Liakhovetski]

Adding support for runtime power-management to the MMCIF driver allows
it to save power as long as no card is present. System-wide power
management has been verified with experimental PM patches on AP4-
based systems.

Signed-off-by: Guennadi Liakhovetski 
---

With this patch and with the patch to mmc/core/bus.c, that I've sent a 
couple of minutes ago

http://article.gmane.org/gmane.linux.kernel.mmc/7433

PM works correctly with all possible modprobe / rmmod, card-insert / 
eject, and STR scenarios, that I could come up with. The part in 
sh_mmcif_remove() is a bit rough though...

diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c
index d3871b6..23eb0ec 100644
--- a/drivers/mmc/host/sh_mmcif.c
+++ b/drivers/mmc/host/sh_mmcif.c
@@ -29,6 +29,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 #define DRIVER_NAME"sh_mmcif"
@@ -173,6 +174,7 @@ struct sh_mmcif_host {
struct completion intr_wait;
enum mmcif_state state;
spinlock_t lock;
+   bool power;
 
/* DMA support */
struct dma_chan *chan_rx;
@@ -877,11 +879,21 @@ static void sh_mmcif_set_ios(struct mmc_host *mmc, struct 
mmc_ios *ios)
if (ios->power_mode == MMC_POWER_UP) {
if (p->set_pwr)
p->set_pwr(host->pd, ios->power_mode);
+   if (!host->power) {
+   pm_runtime_get_sync(&host->pd->dev);
+   host->power = true;
+   }
} else if (ios->power_mode == MMC_POWER_OFF || !ios->clock) {
/* clock stop */
sh_mmcif_clock_control(host, 0);
-   if (ios->power_mode == MMC_POWER_OFF && p->down_pwr)
-   p->down_pwr(host->pd);
+   if (ios->power_mode == MMC_POWER_OFF) {
+   if (host->power) {
+   pm_runtime_put(&host->pd->dev);
+   host->power = false;
+   }
+   if (p->down_pwr)
+   p->down_pwr(host->pd);
+   }
host->state = STATE_IDLE;
return;
}
@@ -1053,6 +1065,12 @@ static int __devinit sh_mmcif_probe(struct 
platform_device *pdev)
sh_mmcif_sync_reset(host);
platform_set_drvdata(pdev, host);
 
+   pm_runtime_enable(&pdev->dev);
+   host->power = false;
+   ret = pm_runtime_resume(&pdev->dev);
+   if (ret < 0)
+   goto clean_up2;
+
/* See if we also get DMA */
sh_mmcif_request_dma(host, pd);
 
@@ -1063,13 +1081,13 @@ static int __devinit sh_mmcif_probe(struct 
platform_device *pdev)
ret = request_irq(irq[0], sh_mmcif_intr, 0, "sh_mmc:error", host);
if (ret) {
dev_err(&pdev->dev, "request_irq error (sh_mmc:error)\n");
-   goto clean_up2;
+   goto clean_up3;
}
ret = request_irq(irq[1], sh_mmcif_intr, 0, "sh_mmc:int", host);
if (ret) {
free_irq(irq[0], host);
dev_err(&pdev->dev, "request_irq error (sh_mmc:int)\n");
-   goto clean_up2;
+   goto clean_up3;
}
 
sh_mmcif_detect(host->mmc);
@@ -1079,7 +1097,10 @@ static int __devinit sh_mmcif_probe(struct 
platform_device *pdev)
sh_mmcif_readl(host->addr, MMCIF_CE_VERSION) & 0x);
return ret;
 
+clean_up3:
+   pm_runtime_suspend(&pdev->dev);
 clean_up2:
+   pm_runtime_disable(&pdev->dev);
clk_disable(host->hclk);
 clean_up1:
mmc_free_host(mmc);
@@ -1112,15 +1133,54 @@ static int __devexit sh_mmcif_remove(struct 
platform_device *pdev)
 
clk_disable(host->hclk);
mmc_free_host(host->mmc);
+   pm_runtime_put_noidle(&pdev->dev);
+   pm_runtime_suspend(&pdev->dev);
+   pm_runtime_disable(&pdev->dev);
+   pm_runtime_get_noresume(&pdev->dev);
 
return 0;
 }
 
+#ifdef CONFIG_PM
+static int sh_mmcif_suspend(struct device *dev)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+   struct sh_mmcif_host *host = platform_get_drvdata(pdev);
+   int ret = mmc_suspend_host(host->mmc);
+
+   if (!ret) {
+   sh_mmcif_writel(host->addr, MMCIF_CE_INT_MASK, MASK_ALL);
+   clk_disable(host->hclk);
+   }
+
+   return ret;
+}
+
+static int sh_mmcif_resume(struct device *dev)
+{
+   struct platform_device *pdev = to_platform_device(dev);
+   struct sh_mmcif_host *host = platform_get_drvdata(pdev);
+
+   clk_enable(host->hclk);
+
+   return mmc_resume_host(host->mmc);
+}
+#else
+#define sh_mmcif_suspend   NULL
+#define sh_mmcif_resumeNULL
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops sh_mmcif_dev_pm_ops = {
+   .suspend = sh_mmcif_suspend,
+   .resume = sh_mmcif_resume,
+};
+
 static struct platform_driver sh_mmcif_driver = {
.probe  = sh_mmcif_pro

[PATCH/RFC] MMC: remove unbalanced pm_runtime_suspend()

[Guennadi Liakhovetski]

MMC bus PM operations implement a .runtime_idle() method, which calls 
pm_runtime_suspend(), but this call is not balanced by a resume 
counterpart, which causes problems with repeated card-plug and driver-load 
cycles. Removing this method fixes the disbalance.

Signed-off-by: Guennadi Liakhovetski 
---

With this patch and with v2 of the MMCIF PM patch, that I'll be posting 
shortly, I can load / unload the driver, insert and remove the card and 
suspend and wake up the system multiple times and each time the full PM 
cycle is performed, going down to the platform callbacks. However, it 
might well be, that there's a more correct way to achieve the same.

diff --git a/drivers/mmc/core/bus.c b/drivers/mmc/core/bus.c
index 63667a8..44866a6 100644
--- a/drivers/mmc/core/bus.c
+++ b/drivers/mmc/core/bus.c
@@ -158,15 +158,9 @@ static int mmc_runtime_resume(struct device *dev)
return mmc_power_restore_host(card->host);
 }
 
-static int mmc_runtime_idle(struct device *dev)
-{
-   return pm_runtime_suspend(dev);
-}
-
 static const struct dev_pm_ops mmc_bus_pm_ops = {
.runtime_suspend= mmc_runtime_suspend,
.runtime_resume = mmc_runtime_resume,
-   .runtime_idle   = mmc_runtime_idle,
 };
 
 #define MMC_PM_OPS_PTR (&mmc_bus_pm_ops)
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] mmc: sdhci: merge two sdhci-pltfm.h into one

[Wolfram Sang]

On Fri, Mar 25, 2011 at 04:48:51PM +0800, Shawn Guo wrote:
> The structure sdhci_pltfm_data is not necessarily to be in a public
> header like include/linux/mmc/sdhci-pltfm.h, so the patch moves it
> into drivers/mmc/host/sdhci-pltfm.h and eliminates the former one.
> 
> Signed-off-by: Shawn Guo 

Reviewed-by: Wolfram Sang 

-- 
Pengutronix e.K.   | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/  |


signature.asc
Description: Digital signature

Re: [PATCH 4/5] mmc: sdhci: consolidate sdhci-of-esdhc and sdhci-esdhc-imx

[Wolfram Sang]

>  config MMC_SDHCI_ESDHC_IMX
> - bool "SDHCI platform support for the Freescale eSDHC i.MX controller"
> + bool "SDHCI support for the Freescale eSDHC i.MX controller"
>   depends on ARCH_MX25 || ARCH_MX35 || ARCH_MX5
>   depends on MMC_SDHCI
> - select MMC_SDHCI_PLTFM
> + select MMC_SDHCI_ESDHC
>   select MMC_SDHCI_IO_ACCESSORS
>   help
> -   This selects the Freescale eSDHC controller support on the platform
> -   bus, found on platforms like mx35/51.
> +   This selects the Freescale eSDHC controller support on platforms
> +   like mx35/51.

While we are at it, mx25 could be added and mx53 (and you know better
what else will be coming ;))

> diff --git a/drivers/mmc/host/sdhci-esdhc.c b/drivers/mmc/host/sdhci-esdhc.c
> new file mode 100644
> index 000..b3d1bc1
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-esdhc.c
> @@ -0,0 +1,413 @@
> +/*
> + * Freescale eSDHC controller driver for MPCxxx and i.MX.
> + *
> + * Copyright (c) 2007 Freescale Semiconductor, Inc.
> + *   Author: Xiaobo Xie 
> + *
> + * Copyright (c) 2009 MontaVista Software, Inc.
> + *   Author: Anton Vorontsov 
> + *
> + * Copyright (c) 2010 Pengutronix e.K.
> + *   Author: Wolfram Sang 
> + *
> + * 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.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#ifdef CONFIG_MMC_SDHCI_ESDHC_IMX
> +#include 
> +#endif
> +#include "sdhci.h"
> +#include "sdhci-pltfm.h"
> +
> +/*
> + * Ops and quirks for the Freescale eSDHC controller.
> + */
> +
> +#define ESDHC_DEFAULT_QUIRKS (SDHCI_QUIRK_FORCE_BLK_SZ_2048 | \
> + SDHCI_QUIRK_BROKEN_CARD_DETECTION | \
> + SDHCI_QUIRK_NO_BUSY_IRQ | \
> + SDHCI_QUIRK_NONSTANDARD_CLOCK | \
> + SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK | \
> + SDHCI_QUIRK_PIO_NEEDS_DELAY | \
> + SDHCI_QUIRK_RESTORE_IRQS_AFTER_RESET | \
> + SDHCI_QUIRK_NO_CARD_NO_RESET)

These are not the current quirks. Meanwhile BROKEN_CARD_DETECTION is
gone as well as NO_CARD_NO_RESET (at least for imx). My additions to use
GPIOs for card detect and write protect are also not in this version.

[...]

> +static struct sdhci_pltfm_data sdhci_esdhc_mpc_pdata = {
> + .quirks = ESDHC_DEFAULT_QUIRKS,
> + .ops = &sdhci_esdhc_mpc_ops,
> +};
> +#endif

Please mark the #endif with comments of the expression they are
depending on, e.g.

#endif /* CONFIG_MMC_SDHCI_ESDHC_MPC */

if it is not immediately visible. That helps readability.

[...]

Phew, due to all these hardware bugs, the driver will get messy, even if
we try hard. Any chances that future revisions of the core will be
updated? :)

-- 
Pengutronix e.K.   | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/  |


signature.asc
Description: Digital signature

Re: [PATCH 3/5] mmc: sdhci: make sdhci-of device drivers self registered

[Wolfram Sang]

> +static int __devinit sdhci_esdhc_probe(struct platform_device *pdev)
> +{
> + struct sdhci_host *host;
> + int ret;
> +
> + host = sdhci_pltfm_init(pdev, &sdhci_esdhc_pdata);
> + if (!host)
> + return -ENOMEM;

Just noticed: Since pltfm_init may fail due to various reasons, maybe
ERRPTR might be a good idea?

[...]

> +static int __init sdhci_hlwd_init(void)
> +{
> + return platform_driver_register(&sdhci_hlwd_driver);
> +}
> +module_init(sdhci_hlwd_init);
> +
> +static void __exit sdhci_hlwd_exit(void)
> +{
> + platform_driver_unregister(&sdhci_hlwd_driver);
> +}
> +module_exit(sdhci_hlwd_exit);
> +
> +MODULE_DESCRIPTION("Secure Digital Host Controller Interface OF driver");
> +MODULE_AUTHOR("Xiaobo Xie , "
> +   "Anton Vorontsov ");
> +MODULE_LICENSE("GPL v2");

Please double check the authors. It is based on the fsl driver, but the
copyright should go to

 * Copyright (C) 2009 The GameCube Linux Team
 * Copyright (C) 2009 Albert Herranz

I think.

-- 
Pengutronix e.K.   | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/  |


signature.asc
Description: Digital signature

Re: [PATCH 2/5] mmc: sdhci: eliminate sdhci_of_host and sdhci_of_data

[Wolfram Sang]

On Fri, Mar 25, 2011 at 04:48:48PM +0800, Shawn Guo wrote:
> The patch is to migrate the use of sdhci_of_host and sdhci_of_data
> to sdhci_pltfm_host and sdhci_pltfm_data, so that the former pair can
> be eliminated.
> 
> Signed-off-by: Shawn Guo 
> ---

[...]

> diff --git a/drivers/mmc/host/sdhci-pltfm.h b/drivers/mmc/host/sdhci-pltfm.h
> index a3e4be0..12afe86 100644
> --- a/drivers/mmc/host/sdhci-pltfm.h
> +++ b/drivers/mmc/host/sdhci-pltfm.h
> @@ -19,6 +19,10 @@
>  struct sdhci_pltfm_host {
>   struct clk *clk;
>   u32 scratchpad; /* to handle quirks across io-accessor calls */
> +
> + /* migrate from sdhci_of_host */
> + unsigned int clock;
> + u16 xfer_mode_shadow;

xfer_mode_shadow can be merged into scratchpad. They both fix the same
issue.

-- 
Pengutronix e.K.   | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/  |


signature.asc
Description: Digital signature

Re: [PATCH 1/5] mmc: sdhci: make sdhci-pltfm device drivers self registered

[Wolfram Sang]

On Fri, Mar 25, 2011 at 04:48:47PM +0800, Shawn Guo wrote:
> The patch turns the common stuff in sdhci-pltfm.c into functions, and
> add device drivers their own .probe and .remove which in turn call
> into the common functions, so that those sdhci-pltfm device drivers
> register itself and keep all device specific things away from common
> sdhci-pltfm file.
> 
> Signed-off-by: Shawn Guo 
> ---

I'll second the comments from Grant (with one slight exception which is
noted below)

> +static int __devexit sdhci_dove_remove(struct platform_device *pdev)
> +{
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + int dead = 0;
> + u32 scratch;
> +
> + scratch = readl(host->ioaddr + SDHCI_INT_STATUS);
> + if (scratch == (u32)-1)
> + dead = 1;

I'd prefer

dead = (readl() == 0x);

(or (u32)-1 if you prefer). But keeping a variable 'dead' is more
descriptive than keeping 'scratch'.

> +MODULE_LICENSE("GPL v2");

Just to be sure: Did you double-check if the original licenses were v2
or v2+?

> --- a/drivers/mmc/host/sdhci-pltfm.c
> +++ b/drivers/mmc/host/sdhci-pltfm.c

[...]

> -err_add_host:
> - if (pdata && pdata->exit)
> - pdata->exit(host);
> -err_plat_init:
> - iounmap(host->ioaddr);
>  err_remap:
>   release_mem_region(iomem->start, resource_size(iomem));
>  err_request:
>   sdhci_free_host(host);
>  err:
> - printk(KERN_ERR"Probing of sdhci-pltfm failed: %d\n", ret);
> - return ret;
> + pr_err("%s failed %d\n", __func__, ret);

dev_err?

> + return NULL;
>  }
>  

I didn't pay much attention to the OF version of the tegra driver, since
it still is not upstream yet, right?

Regards,

   Wolfram

-- 
Pengutronix e.K.   | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/  |


signature.asc
Description: Digital signature

Re: [PATCH 0/5] consolidate sdhci pltfm & OF drivers and get them self registered

[Wolfram Sang]

Hi Shawn,

On Fri, Mar 25, 2011 at 04:48:46PM +0800, Shawn Guo wrote:
> Here are what the patch set does.
> 
> * Remove .probe and .remove hooks from sdhci-pltfm.c and make it be
>   a pure common helper function providers.
> * Add .probe and .remove hooks for sdhci pltfm drivers sdhci-cns3xxx,
>   sdhci-dove, sdhci-tegra, and sdhci-esdhc-imx to make them self
>   registered with calling helper functions created above.
> * Migrate the use of sdhci_of_host and sdhci_of_data to
>   sdhci_pltfm_host and sdhci_pltfm_data, so that OF version host and
>   data structure works can be saved, and pltfm version works for both
>   cases.
> * Add OF common helper stuff into sdhci-pltfm.c, and make OF version
>   sdhci drivers sdhci-of-esdhc and sdhci-of-hlwd become self
>   registered as well, so that sdhci-of-core.c and sdhci-of.h can be
>   removed.
> * Consolidate the OF and pltfm esdhc drivers into one with sharing
>   the same pair of .probe and .remove hooks.  As a result,
>   sdhci-esdhc-imx.c and sdhci-of-esdhc.c go away, while
>   sdhci-esdhc.c comes in and works for both MPCxxx and i.MX.
> * Eliminate include/linux/mmc/sdhci-pltfm.h with moving stuff into
>   drivers/mmc/host/sdhci-pltfm.h.
> 
> And the benefits we gain from the changes are:
> 
> * Get the sdhci device driver follow the Linux trend that driver
>   makes the registration by its own.
> * sdhci-pltfm.c becomes simple and clean as it only has common helper
>   stuff there now.
> * All sdhci device specific things are going back its own driver.
> * The dt and non-dt drivers are consolidated to use the same pair of
>   .probe and .remove hooks.
> * SDHCI driver for Freescale eSDHC controller found on both MPCxxx
>   and i.MX platforms is consolidated to use the same one .probe
>   function.
> 
> The patch set works against the tree below, and was only tested on
> i.mx51 babbage board, all other targets were build tested.
> 
>   git://git.secretlab.ca/git/linux-2.6.git devicetree/test
> 
> Comments are welcomed and appreciated.

First of all, thanks _a lot_ for doing this! Many people have promised
to do such an approach, but you finally made it!

Sorry for the long delay in reviewing it, I didn't want to rush a review
so I needed some time for it which I didn't have much in the last weeks.
Let's hope my battery will have enough power on my flight back to
Germany ;) So, this is for now a purely "visual" review. I hope I can
check V2 on real hardware then.

The approach seems sensible, so have a look at my (mostly minor)
comments inside the patches. However, there is one bigger piece missing.
You converted all the drivers which had a seperate source-file and
hooked into sdhci-pltfm.c. However, those are only those users which
need additional code to work around the quirks. There are also users
which can take the plain pltfm-driver with a properly set
platform_data (check the thread "[PATCH] mmc: add SDHCI driver for STM
platforms (V2)" for an example). Those have to be converted, too.
Now the discussion could be if every of those users gets its own
pltfm-.c or if we create something similat to
sdhci-pltfm-generic, which can also be setup with platform_data like the
old driver (/me likes the latter a bit more. If we don't change the name
of the driver (not talking about the sourcefile) and keep it
"sdhci-pltfm", then you wouldn't need to change all those users if you
ensured it behaves the same.

Also, I think the next version of this series should have all makers of
a sdhci-pltfm user CCed so we give them a chance to report breakage. Or
donate acks or tested-by.

Regards,

   Wolfram

-- 
Pengutronix e.K.   | Wolfram Sang|
Industrial Linux Solutions | http://www.pengutronix.de/  |


signature.asc
Description: Digital signature

Re: [PATCH] mmc: USB SDIO/SD/MMC Host Controller (VUB300) driver Re-Resubmission

[Tony Olech]

On Tue, 2011-03-15 at 17:23 +0100, Arnd Bergmann wrote:
> On Thursday 10 March 2011, Tony Olech wrote:
...
> > +static ssize_t __show_operating_mode(struct vub300_mmc_host *vub300,
> > +   struct mmc_host *mmc, char *buf)
> > +{
> > +   int usb_packet_size = vub300->large_usb_packets ? 512 : 64;
> > +   if (vub300->vub_name[0])
> > +   return sprintf(buf, "VUB %s %s %d MHz %s %d byte USB 
> > packets"
> > +   " using %s\n",
> > +  (mmc->caps & MMC_CAP_SDIO_IRQ) ? "IRQs" : "POLL",
> > +  (mmc->caps & MMC_CAP_4_BIT_DATA) ? "4-bit" : "1-bit",
> > +  mmc->f_max / 100,
> > +  pad_input_to_usb_pkt ? "padding input data to" : 
> > "with",
> > +  usb_packet_size, vub300->vub_name);
> > +   else
> > +   return sprintf(buf, "VUB %s %s %d MHz %s %d byte USB 
> > packets"
> > +   " and no offload processing\n",
> > +  (mmc->caps & MMC_CAP_SDIO_IRQ) ? "IRQs" : "POLL",
> > +  (mmc->caps & MMC_CAP_4_BIT_DATA) ? "4-bit" : "1-bit",
> > +  mmc->f_max / 100,
> > +  pad_input_to_usb_pkt ? "padding input data to" : 
> > "with",
> > +  usb_packet_size);
> > +}
> > +
> > +static ssize_t show_operating_mode(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > +   struct mmc_host *mmc = container_of(dev, struct mmc_host, 
> > class_dev);
> > +   if (mmc) {
> > +   struct vub300_mmc_host *vub300 = mmc_priv(mmc);
> > +   return __show_operating_mode(vub300, mmc, buf);
> > +   } else {
> > +   return sprintf(buf, "VUB driver has no attached device");
> > +   }
> > +}
> 
> This sysfs attribute is rather hard to parse from user space, it looks
> like it's designed only to be read by humans. I think it would be better
> to use multiple attributes, each of which has only a single piece
> of information in it.
> 
> Some of these attributes however don't really belong into this driver
> but into the core, like the 1-bit / 4-bit mode. Please leave this out
> of your driver, and submit a separate patch to the mmc core if you
> think it's reasonable.

The purpose of this read-only interface is for VUB300 support
staff to obtain information from our customers. Our customers
are not like the people on this list. If you have ever tried
to do telephone support you would appreciate the difficulty
of getting a non-technical person to first of all find the
log files and then to extract the correct lines. It therefore
follows that if the 1 or 4 bit mode is useful info for the 
VUB300 support staff, then this read-only interface is the
appropriate single place for it to be provided. The overhead
of providing such an interface is negligible and the existance
of such an interface demonstrates that linux is no longer
designed only for geeks.

Tony Olech




--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patchv3 2/5] MMC: Use CMD23 for multiblock transfers when we can.

[Arnd Bergmann]

On Tuesday 19 April 2011, Andrei Warkentin wrote:

> So maybe this should be a blacklist for known bad cards. And the
> entire support should be a "default-N" compile option for MMCs (not
> SDs). That way someone who just does an "make oldconfig" will see
> "CONFIG_MMC_BLK_CMD23 - I/O performance improvement for newer eMMC
> cards, may cause degradation on older cards". What do you think?

I'm not sure if I understand the distinction between MMC and SD
here. Do you suggest we always enable it for SD but make it compile-time
selected for MMC?

I generally argue against compile time options. A distribution
integrator needs to choose a reasonable default, and giving them
an option makes it possible to get it wrong.

I believe the best way would be trying to warn people against
regressions while going forward with this enabled unconditionally,
unless we hear back from people that actually got regressions.

We could perhaps key enabling the feature by the production date
on the card, so it only gets turned on for new cards.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html