Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320611 > unrolled thread

Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION  quirk Adrian Hunter <adrian.hunter@intel.com> - 2016-01-28 13:10 +0100
    Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk Ulf Hansson <ulf.hansson@linaro.org> - 2016-01-28 16:20 +0100
      Re: [RFC PATCH 0/21] Totally remove  SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-01-28 17:30 +0100
      Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION  quirk Adrian Hunter <adrian.hunter@intel.com> - 2016-01-29 13:20 +0100
        Re: [RFC PATCH 0/21] Totally remove  SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk Russell King - ARM Linux <linux@arm.linux.org.uk> - 2016-01-29 18:30 +0100
          Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION  quirk Adrian Hunter <adrian.hunter@intel.com> - 2016-02-01 13:40 +0100

#1320611 — Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk

On 27/01/16 17:07, Ulf Hansson wrote:
> On 27 January 2016 at 14:23, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Wed, Jan 27, 2016 at 02:59:14PM +0200, Adrian Hunter wrote:
>>> In my view Ulf needs to explain how the SDHCI library is going to work,
>>> particularly in the absence of new callbacks.
>>
>> We need to add new callbacks as part of the conversion to a library,
>> otherwise we're very much into a total rewrite from scratch (which
>> I think is far too much work, and prone to errors) or a big flag day
>> to switch everything over (which will require a moritorium on sdhci
>> patches while the effort to switch everything is ongoing.)
>>
>> Both of those approaches suffer from one huge drawback: there is no
>> way to bisect between them to locate the cause of a regression.
>>
>> A piece-meal approach, where the driver is gradually converted is a
>> far saner approach, because it means that each conversion in the step
>> can be done as a series of transformations, which not only can be
>> properly reviewed, but also bisected - and that is _hugely_ important
>> for the existing state of SDHCI.
>>
>> The chances of some hardware behavioural quirk being missed while
>> trying to convert SDHCI to a library are _extremely_ high, and the
>> only sane approach to this is one which allows a progressive
>> transformation of the driver.
>>
>> Also, the last thing we want is for drivers to end up duplicating
>> entire functions from sdhci.c just because they have one thing
>> different (eg, because they need to do something in the middle of
>> a set_ios() call which no other SDHCI driver needs.)  Having such
>> code duplication will just make maintanence even more of a
>> nightmare.
>>
>> set_ios() is probably one of the worst functions in sdhci right now,
>> and there's no obvious way to split it up into several stand-alone
>> functions which drivers could chain together.
>>
>> I think what needs to happen here is that Ulf needs to leave such
>> decisions about what is acceptable or unacceptable to those who are
>> trying to convert sdhci to a library, otherwise the conversion will
>> probably never happen... unless Ulf wants to get directly involved
>> in the conversion effort, producing patches to make it happen.
>>
> 
> I don't intend to contribute much with actual patches. I am willing to
> help review and also help with expertise around the PM related parts.
> 
> I do realize that some callbacks may still be needed, even in the end
> when sdhci has become a pure library. Although, those should be far
> less then those we have today.
> 
> Currently I am more or less unable to properly maintain sdhci because
> of it's bad code structure. Therefore I have taken a quite simple
> approach by rejecting new callbacks and quirks, in a way to prevent it
> from being worse. To me, the best way forward would be if some of you
> experienced sdhci developers stepped in as a maintainer for it. In
> that way, I can trust the development moving in the "library
> direction" so I can pull back from nacking potential interim sdhci
> callbacks/quirks.
> 
> Does it make sense?

I am happy to help and even be the SDHCI maintainer if Russell King and
others agree.  I have an interest in sdhci-acpi and sdhci-pci and also there
is UHS-II and ADMA3 on the horizon.

I agree with Russell that a re-write would introduce more bugs and more work
than it would be worth.  Making many small steps in the general direction is
preferable.

Initially it would nice to see it made easy for drivers to replace specific
mmc ops and sdhci ops and then call the standard version before/after doing
some custom code.  For example, P L Sai Krishna's auto-tuning problem might
be solved by something to the effect of:

int arasan_execute_tuning(struct mmc_host *mmc, u32 opcode)
{
	struct sdhci_host *host = mmc_priv(mmc);
	int err;

	err = sdhci_execute_tuning(mmc, opcode);
	if (!err)
		arasan_tune_sdclk(host);
	return err;
}

And Wan Zongshun also wanted to be able directly to replace
sdhci_execute_tuning() from sdhci-pci.

As suggested, my get_cd problem could also be solved by replacing the mmc
get_cd op.

[toc] | [next] | [standalone]

#1320778 — Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk

[...]

>> I don't intend to contribute much with actual patches. I am willing to
>> help review and also help with expertise around the PM related parts.
>>
>> I do realize that some callbacks may still be needed, even in the end
>> when sdhci has become a pure library. Although, those should be far
>> less then those we have today.
>>
>> Currently I am more or less unable to properly maintain sdhci because
>> of it's bad code structure. Therefore I have taken a quite simple
>> approach by rejecting new callbacks and quirks, in a way to prevent it
>> from being worse. To me, the best way forward would be if some of you
>> experienced sdhci developers stepped in as a maintainer for it. In
>> that way, I can trust the development moving in the "library
>> direction" so I can pull back from nacking potential interim sdhci
>> callbacks/quirks.
>>
>> Does it make sense?
>
> I am happy to help and even be the SDHCI maintainer if Russell King and
> others agree.  I have an interest in sdhci-acpi and sdhci-pci and also there
> is UHS-II and ADMA3 on the horizon.

That's really great news. Thank you very much Adrian!

Perhaps Russell is willing to help co-maintain it?

>
> I agree with Russell that a re-write would introduce more bugs and more work
> than it would be worth.  Making many small steps in the general direction is
> preferable.
>
> Initially it would nice to see it made easy for drivers to replace specific
> mmc ops and sdhci ops and then call the standard version before/after doing
> some custom code.  For example, P L Sai Krishna's auto-tuning problem might
> be solved by something to the effect of:
>
> int arasan_execute_tuning(struct mmc_host *mmc, u32 opcode)
> {
>         struct sdhci_host *host = mmc_priv(mmc);
>         int err;
>
>         err = sdhci_execute_tuning(mmc, opcode);
>         if (!err)
>                 arasan_tune_sdclk(host);
>         return err;
> }
>
> And Wan Zongshun also wanted to be able directly to replace
> sdhci_execute_tuning() from sdhci-pci.
>
> As suggested, my get_cd problem could also be solved by replacing the mmc
> get_cd op.
>

Sounds like a perfect plan!

Do you want to send a patch to the MAINTAINERS file?

From my side I can also continue doing the administrative part of the
work, so there's need for you to set up a separate git tree or send
pull request. At least initially.
Instead I will just pick patches that's been acked by you (and
possibly Russell).

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1320846 — Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk

On Thu, Jan 28, 2016 at 04:16:27PM +0100, Ulf Hansson wrote:
> That's really great news. Thank you very much Adrian!
> 
> Perhaps Russell is willing to help co-maintain it?

Unfortunately, I'm not really in a position to co-maintain it, as
I'd be doing it in my spare time, and my spare time is already
spread thinly over much of the kernel (I've crypto, phy/sfp,
etnaviv drm, xf86-video-armada, etnaviv mesa, HDMI CEC, and DSA
stuff all wanting attention, and I'm having to tell people "sorry,
I won't be able to do anything on XYZ for a while".)  I'm quite
sure the Marvell Dove maintainers are going to want some more
patches from me for the next merge window soon...

Of course, that could change if Linaro wishes to fund work in this
area, because it would then take priority... ;)

I'm more than willing to put whatever time I can into helping with
SDHCI, which is exactly why you get the occasional large series of
patches from me - the whole reason behind these patch series are to
improve SDHCI in an incremental fashion.  However, I'd be lying if
I didn't say that there's an alterior motive behind it, which is to
get SDHCI on iMX6 stable.  What I realise is that the current
situation is quite dire, and SDHCI needs improving if we're not
going to decend into a totally unmaintainable mess.

Just before Christmas, I was working on a way to change the way we
kick off commands in SDHCI in order to clean up those paths - the
patches which follow on from the 25 I've already posted, and are:

mmc: sdhci: move interrupt enable settings to task
mmc: sdhci: move command and argument to sdhci_cmd_task
mmc: sdhci: move block size and block count into sdhci task structure
mmc: sdhci: compute transfer mode separately from programming it
mmc: sdhci: replace 'set_timeout' method with 'calculate_timeout'
mmc: sdhci: validate command response type
mmc: sdhci: clean up
mmc: sdhci: rearrange sdhci_set_transfer_mode() a little

It's a work-in-progress, which is why I haven't posted these yet.
The outline of it is that we (currently) end up with:

+struct sdhci_cmd_task {
+       bool has_argument2;
+       bool has_data;
+       u32 ier;
+       u32 argument2;
+       u16 block_size;
+       u16 block_count;
+       u32 argument;
+       u16 transfer_mode;
+       u16 command;
+};
+

which allows separation of the preparation step for a MMC command from
touching the hardware - this means that quirkly SDHCI drivers can do
this instead of litering the core code with quirk tests:

	struct sdhci_cmd_task task;

	sdhci_prepare_command(sdhci, &task, mmc_command);

	/* do whatever quirks */

	sdhci_execute_command(sdhci, &task);

This structure means that (eg) some of the quirks such as
SDHCI_QUIRK2_SUPPORT_SINGLE,
SDHCI_QUIRK2_CLEAR_TRANSFERMODE_REG_BEFORE_CMD (which I'm sure is
actually a bug in the SDHCI driver) and eventually
SDHCI_QUIRK_BROKEN_TIMEOUT_VAL can all be eliminated from the core
code and moved out into their respective drivers - because we have
the core code preparing "the standard" set of register settings for
the command to be sent, and then the driver gets the opportunity to
tweak them according to the bugs it has.

... and even after all these changes, I still haven't solved the
problems which bugs me on iMX6, which were my motivation for coming
back to putting some more effort into SDHCI! :p

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1321721

On 28/01/16 17:16, Ulf Hansson wrote:
> [...]
> 
>>> I don't intend to contribute much with actual patches. I am willing to
>>> help review and also help with expertise around the PM related parts.
>>>
>>> I do realize that some callbacks may still be needed, even in the end
>>> when sdhci has become a pure library. Although, those should be far
>>> less then those we have today.
>>>
>>> Currently I am more or less unable to properly maintain sdhci because
>>> of it's bad code structure. Therefore I have taken a quite simple
>>> approach by rejecting new callbacks and quirks, in a way to prevent it
>>> from being worse. To me, the best way forward would be if some of you
>>> experienced sdhci developers stepped in as a maintainer for it. In
>>> that way, I can trust the development moving in the "library
>>> direction" so I can pull back from nacking potential interim sdhci
>>> callbacks/quirks.
>>>
>>> Does it make sense?
>>
>> I am happy to help and even be the SDHCI maintainer if Russell King and
>> others agree.  I have an interest in sdhci-acpi and sdhci-pci and also there
>> is UHS-II and ADMA3 on the horizon.
> 
> That's really great news. Thank you very much Adrian!
> 
> Perhaps Russell is willing to help co-maintain it?
> 
>>
>> I agree with Russell that a re-write would introduce more bugs and more work
>> than it would be worth.  Making many small steps in the general direction is
>> preferable.
>>
>> Initially it would nice to see it made easy for drivers to replace specific
>> mmc ops and sdhci ops and then call the standard version before/after doing
>> some custom code.  For example, P L Sai Krishna's auto-tuning problem might
>> be solved by something to the effect of:
>>
>> int arasan_execute_tuning(struct mmc_host *mmc, u32 opcode)
>> {
>>         struct sdhci_host *host = mmc_priv(mmc);
>>         int err;
>>
>>         err = sdhci_execute_tuning(mmc, opcode);
>>         if (!err)
>>                 arasan_tune_sdclk(host);
>>         return err;
>> }
>>
>> And Wan Zongshun also wanted to be able directly to replace
>> sdhci_execute_tuning() from sdhci-pci.
>>
>> As suggested, my get_cd problem could also be solved by replacing the mmc
>> get_cd op.
>>
> 
> Sounds like a perfect plan!
> 
> Do you want to send a patch to the MAINTAINERS file?

Yes, I'll do that.

> 
>>From my side I can also continue doing the administrative part of the
> work, so there's need for you to set up a separate git tree or send
> pull request. At least initially.
> Instead I will just pick patches that's been acked by you (and
> possibly Russell).

I might make a tree because I want to try to separate Russell's bug fixes
from the clean-ups, and then cc stable on the bug fixes.

[toc] | [prev] | [next] | [standalone]

#1321902 — Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk

On Fri, Jan 29, 2016 at 02:08:21PM +0200, Adrian Hunter wrote:
> I might make a tree because I want to try to separate Russell's bug fixes
> from the clean-ups, and then cc stable on the bug fixes.

It would be good if you could ask for that, I'll look at rearranging
(and re-testing) the patches to achieve that.  Had I known that, I
could've done it before posting the latest set of patches.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1323055

On 29/01/16 19:28, Russell King - ARM Linux wrote:
> On Fri, Jan 29, 2016 at 02:08:21PM +0200, Adrian Hunter wrote:
>> I might make a tree because I want to try to separate Russell's bug fixes
>> from the clean-ups, and then cc stable on the bug fixes.
> 
> It would be good if you could ask for that, I'll look at rearranging
> (and re-testing) the patches to achieve that.  Had I known that, I
> could've done it before posting the latest set of patches.

If you don't mind doing that, that would be great!

So far the fixes, I have identified:

1. mmc: sdhci: command response CRC error handling

Could have "fix" in the subject i.e. "fix command response CRC error handling".

Could change to be dependent only on "mmc: sdhci: move initialisation of
command error member".  Also I would treat end-bit and index errors the same
as CRC errors i.e. probably end up with:

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index d622435d1bcc..6cae93a89eba 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2325,8 +2325,23 @@ static void sdhci_cmd_irq(struct sdhci_host *host,
u32 intmask, u32 *mask)
 	if (intmask & SDHCI_INT_TIMEOUT)
 		host->cmd->error = -ETIMEDOUT;
 	else if (intmask & (SDHCI_INT_CRC | SDHCI_INT_END_BIT |
-			SDHCI_INT_INDEX))
+			SDHCI_INT_INDEX)) {
 		host->cmd->error = -EILSEQ;
+		/*
+		 * If this command initiates a data phase and a response
+		 * CRC error is signalled, the card can start transferring
+		 * data - the card may have received the command without
+		 * error.  We must not terminate the mmc_request early.
+		 *
+		 * If the card did not receive the command or returned an
+		 * error which prevented it sending data, the data phase
+		 * will time out.
+		 */
+		if (host->cmd->data) {
+			host->cmd = NULL;
+			return;
+		}
+	}

 	if (host->cmd->error) {
 		tasklet_schedule(&host->finish_tasklet);

2. mmc: sdhci: avoid unnecessary mapping/unmapping of align buffer

This looks like a bug fix because the mapping can be leaked on the error
path i.e. similar problem to the one fixed by "mmc: sdhci: plug DMA mapping
leak on error"

3. mmc: sdhci: plug DMA mapping leak on error

It looks like the 2nd chunk could be taken as a separate fix without
dependence on other patches.

4. mmc: sdhci-pxav3: fix higher speed mode capabilities

I can't test this so please indicate if you want it for stable.

5. mmc: sdhci: further fix for DMA unmapping in sdhci_post_req()

Could be made independent of other patches.

6. mmc: sdhci: fix data timeout (part 1)
mmc: sdhci: fix data timeout (part 2)

Could be just 1 patch.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web