[PATCH] mmc: Reduce fOD to 200 kHz if possible

[PATCH] mmc: Reduce fOD to 200 kHz if possible

[Haavard Skinnemoen]

Since the identification frequency was increased to 400 kHz, my
ATSTK1000 board has not been able to initialize any MMC or SD cards.
Reducing the identification mode frequency to 200 kHz fixes the problem.

This is definitely a board-specific issue, probably due to weak pull-up
resistor values, but this is the simplest fix I could come up with.
Alternatively, we could try to pass some "maximum fOD" parameter from
the board code somehow.

Different boards using the same controller (AVR32 AP7000) and the same
cards have no problems at 400 kHz, but I believe reducing fOD a bit
allows a bit more tolerance for poor board designs, which can only be a
good thing. Also, using too strong pull-ups might cause problems at high
speeds, which is IMO worse.

Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
Sorry about bringing this up again, but it's been a pretty longstanding
issue on at least one board, and selecting correct pull-ups requires
careful reading of several specification documents, so selecting a
minimum frequency which is the maximum that the spec allows seems like
asking for trouble.

Both the MMC and SD specs allow frequencies from 100 kHz to 400 kHz. 200
kHz seems like a good compromise to me.

 drivers/mmc/core/core.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5db49b1..b4d9e8b 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -911,8 +911,11 @@ static void mmc_power_up(struct mmc_host *host)
 		pr_warning("%s: Minimum clock frequency too high for "
 				"identification mode\n", mmc_hostname(host));
 		host->ios.clock = host->f_min;
-	} else
-		host->ios.clock = 400000;
+	} else if (host->f_min > 200000) {
+		host->ios.clock = host->f_min;
+	} else {
+		host->ios.clock = 200000;
+	}
 
 	host->ios.power_mode = MMC_POWER_ON;
 	mmc_set_ios(host);
-- 
1.7.0.4

Re: [PATCH] mmc: Reduce fOD to 200 kHz if possible

[Ben Nizette]

On 15/09/2010, at 6:59 PM, Haavard Skinnemoen wrote:

> Since the identification frequency was increased to 400 kHz, my
> ATSTK1000 board has not been able to initialize any MMC or SD cards.
> Reducing the identification mode frequency to 200 kHz fixes the problem.
> 
> This is definitely a board-specific issue, probably due to weak pull-up
> resistor values, but this is the simplest fix I could come up with.

I wonder whether there's an Atmel reference schematic around with an incorrect resistor value on it, the only three people I know affected by this are myself, Hein Tibosch and you, all on AVR32.  We've now submitted one fix each, mine was similar to yours [1] but there's been movement on Hein's more comprehensive patch recently [2].  I don't know who's supposed to be merging MMC patches atm, Chris Ball added on CC on a hunch.

	--Ben.

[1] http://lkml.org/lkml/2010/1/1/85
[2] http://lkml.org/lkml/2010/9/2/494

Re: [PATCH] mmc: Reduce fOD to 200 kHz if possible

[Haavard Skinnemoen]

Ben Nizette <bn@niasdigital.com> wrote:
> On 15/09/2010, at 6:59 PM, Haavard Skinnemoen wrote:
> 
> > Since the identification frequency was increased to 400 kHz, my
> > ATSTK1000 board has not been able to initialize any MMC or SD cards.
> > Reducing the identification mode frequency to 200 kHz fixes the problem.
> > 
> > This is definitely a board-specific issue, probably due to weak pull-up
> > resistor values, but this is the simplest fix I could come up with.  
> 
> I wonder whether there's an Atmel reference schematic around with an incorrect resistor value on it, the only three people I know affected by this are myself, Hein Tibosch and you, all on AVR32.

Yes, if you've used the STK1000 schematics as a reference, the resistor
values are probably a bit on the high side. I can't really suggest any
better values, as I don't remember what they should be, and the various
specs are quite inconsistent so it's not straightforward to find the
best values.

On the other hand, you could argue that the resistor values are fine as
they are, and that the Linux MMC subsystem is broken because it runs
the bus faster than what the hardware allows. Replacing the resistors
with stronger ones might reduce the maximum speed in push-pull mode, so
I wouldn't necessarily recommend it, especially when it's trivial to fix
the issue in software.

> We've now submitted one fix each, mine was similar to yours [1] but there's been movement on Hein's more comprehensive patch recently [2].  I don't know who's supposed to be merging MMC patches atm, Chris Ball added on CC on a hunch.

Thanks for the references. IMO Hein's patch is overkill. There is
absolutely no reason why 200 kHz should be a problem on any setup, and
I haven't found any indication in any discussions that it is.

The reason why fOD was set to 400 kHz in the first place is that some
controllers have a very low f_min so running the initialization at that
frequency causes problems. Which makes sense because the SD standard
clearly says that the clock can't be slower than 100 kHz.

But I have never seen any reasons why we absolutely _have_ to run the
clock at the maximum frequency allowed by the spec. In fact, Sascha
Hauer, who was the one who changed the minimum clock frequency to 400
kHz, said he would be fine with any frequency between 50 kHz and 400
kHz [1].

Haavard

[1] http://lkml.org/lkml/2010/1/5/120

Re: [PATCH] mmc: Reduce fOD to 200 kHz if possible

[Chris Ball]

Hi Haavard,

On Wed, Sep 15, 2010 at 10:59:00AM +0200, Haavard Skinnemoen wrote:
> Since the identification frequency was increased to 400 kHz, my
> ATSTK1000 board has not been able to initialize any MMC or SD cards.
> Reducing the identification mode frequency to 200 kHz fixes the problem.

Good news -- there's already a patch to fix this in -mm.

Please test http://article.gmane.org/gmane.linux.kernel.commits.mm/62969
and let us know whether it works for you.

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: Reduce fOD to 200 kHz if possible

[Chris Ball]

Hi Haavard,

On Wed, Sep 15, 2010 at 12:51:38PM +0200, Haavard Skinnemoen wrote:
> Thanks for the references. IMO Hein's patch is overkill. There is
> absolutely no reason why 200 kHz should be a problem on any setup, and
> I haven't found any indication in any discussions that it is.

Pierre was worried that we'd ping-pong between having an f_min too high
for some cards and too low for others, and be breaking a new set of
cards each time we changed the value.  He may have been being overly
pessimistic, but I don't think being cautious comes with any significant
downsides here.

There's also a (small) performance concern when f_min gets low, though
I agree that it wouldn't be a problem at 200 kHz.

> The reason why fOD was set to 400 kHz in the first place is that some
> controllers have a very low f_min so running the initialization at that
> frequency causes problems. Which makes sense because the SD standard
> clearly says that the clock can't be slower than 100 kHz.
> 
> But I have never seen any reasons why we absolutely _have_ to run the
> clock at the maximum frequency allowed by the spec. In fact, Sascha
> Hauer, who was the one who changed the minimum clock frequency to 400
> kHz, said he would be fine with any frequency between 50 kHz and 400
> kHz [1].

I agree that we aren't trying to run at the maximum possible frequency,
just trying to avoid having to choose a single perfect value without
enough information on what it is.

Thanks,

-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] mmc: Reduce fOD to 200 kHz if possible

[Hein_Tibosch]

 On 15-9-2010 18:51, Haavard Skinnemoen wrote:
> Ben Nizette <bn@niasdigital.com> wrote:
>> On 15/09/2010, at 6:59 PM, Haavard Skinnemoen wrote:
>>
>>> Since the identification frequency was increased to 400 kHz, my
>>> ATSTK1000 board has not been able to initialize any MMC or SD cards.
>>> Reducing the identification mode frequency to 200 kHz fixes the problem.
>>>
>>> This is definitely a board-specific issue, probably due to weak pull-up
>>> resistor values, but this is the simplest fix I could come up with.  
>> I wonder whether there's an Atmel reference schematic around with an
>> incorrect resistor value on it, the only three people I know affected
>> by this are myself, Hein Tibosch and you, all on AVR32.

It was reported by a few more, who wrote me "off-list", but all of them were
using AVR32

> Yes, if you've used the STK1000 schematics as a reference, the resistor
> values are probably a bit on the high side. I can't really suggest any
> better values, as I don't remember what they should be, and the various
> specs are quite inconsistent so it's not straightforward to find the
> best values.
>
> On the other hand, you could argue that the resistor values are fine as
> they are, and that the Linux MMC subsystem is broken because it runs
> the bus faster than what the hardware allows. Replacing the resistors
> with stronger ones might reduce the maximum speed in push-pull mode, so
> I wouldn't necessarily recommend it, especially when it's trivial to fix
> the issue in software.
>

I did find it strange to put the freq at the memory's maximum of 400 Khz

>> We've now submitted one fix each, mine was similar to yours [1] but there's been movement on Hein's more comprehensive patch recently [2].  I don't know who's supposed to be merging MMC patches atm, Chris Ball added on CC on a hunch.
> Thanks for the references. IMO Hein's patch is overkill. There is
> absolutely no reason why 200 kHz should be a problem on any setup, and
> I haven't found any indication in any discussions that it is.
>

I have also seen situations in which the SD will only start up at 180 Khz
or lower.
IMO my patch is indeed a bit of an overkill (the amount of code changed),
I'd rather see a patch like yours or Ben's, but settings fOD at 100 or 50 Khz.

It was Pierre who suggested to try several frequencies[1]: most systems will start
using 400 Khz (so they won't suffer any loss of performance) and in some rare
cases, the SD will only be identified at 100 Khz (which does happen, sometimes)

[1] http://article.gmane.org/gmane.linux.kernel.mmc/994

> The reason why fOD was set to 400 kHz in the first place is that some
> controllers have a very low f_min so running the initialization at that
> frequency causes problems. Which makes sense because the SD standard
> clearly says that the clock can't be slower than 100 kHz.
>
> But I have never seen any reasons why we absolutely _have_ to run the
> clock at the maximum frequency allowed by the spec. In fact, Sascha
> Hauer, who was the one who changed the minimum clock frequency to 400
> kHz, said he would be fine with any frequency between 50 kHz and 400
> kHz

That's right

Hein

Re: [PATCH] mmc: Reduce fOD to 200 kHz if possible

[Haavard Skinnemoen]

Hein_Tibosch <hein_tibosch@yahoo.es> wrote:
> >> We've now submitted one fix each, mine was similar to yours [1] but there's been movement on Hein's more comprehensive patch recently [2].  I don't know who's supposed to be merging MMC patches atm, Chris Ball added on CC on a hunch.
> > Thanks for the references. IMO Hein's patch is overkill. There is
> > absolutely no reason why 200 kHz should be a problem on any setup, and
> > I haven't found any indication in any discussions that it is.
> >
> 
> I have also seen situations in which the SD will only start up at 180 Khz
> or lower.

Ok, so my patch would obviously not fix those. Could you tell me more
about those situations? What kind of board are you running at, etc?

> IMO my patch is indeed a bit of an overkill (the amount of code changed),
> I'd rather see a patch like yours or Ben's, but settings fOD at 100 or 50 Khz.

I guess 100 kHz would make sense. 50 kHz is not allowed according to
the SD card spec.

Another solution might be to introduce a new field, f_max_od, which the
host controller driver could use to limit the bus frequency in open
drain mode.

> It was Pierre who suggested to try several frequencies[1]: most systems will start
> using 400 Khz (so they won't suffer any loss of performance) and in some rare
> cases, the SD will only be identified at 100 Khz (which does happen, sometimes)

I think any performance hit in open drain mode is not likely to be
noticeable, as it is only used for a couple of commands. The only thing
that really matters for performance is push-pull mode, where too strong
pull-ups might cause problems at high speed.

But if there really are cards that have problems at 100 kHz, I guess
your patch might be the best bet.

Haavard

Re: [PATCH] mmc: Reduce fOD to 200 kHz if possible

[Haavard Skinnemoen]

Chris Ball <cjb@laptop.org> wrote:
> Hi Haavard,
> 
> On Wed, Sep 15, 2010 at 10:59:00AM +0200, Haavard Skinnemoen wrote:
> > Since the identification frequency was increased to 400 kHz, my
> > ATSTK1000 board has not been able to initialize any MMC or SD cards.
> > Reducing the identification mode frequency to 200 kHz fixes the problem.
> 
> Good news -- there's already a patch to fix this in -mm.
> 
> Please test http://article.gmane.org/gmane.linux.kernel.commits.mm/62969
> and let us know whether it works for you.

It does, sort of. I get this:

atmel_mci atmel_mci.0: Atmel MCI controller at 0xfff02400 irq 28, 1 slots
mmc0: mmc_rescan: trying to init card at 400000 Hz
Freeing init memory: 916K (90000000 - 900e5000)
mmc0: mmc_rescan: trying to init card at 300000 Hz
mmc0: mmc_rescan: trying to init card at 200000 Hz
mmc0: mmc_rescan: trying to init card at 136719 Hz
mmc0: host does not support reading read-only switch. assuming write-enable.
mmc0: new SD card at address 1234
mmcblk0: mmc0:1234 SD1GB 972 MiB 
 mmcblk0: p1

which is mostly ok, since it does succeed at initializing the card
eventually, but it is somewhat surprising that it needs to go all the
way down to 136 kHz. Usually, it works fine at 200 kHz.

On the other hand, I did try something else before testing the 200 kHz
patch: I enabled the internal pull-ups on the MMC lines. Turning them
off again makes the card initialization fail at 200 kHz.

So please ignore my patch, and let's go for Hein's patch instead.

Haavard