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

Groups > linux.kernel > #1318660 > unrolled thread

[RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk Shawn Lin <shawn.lin@rock-chips.com> - 2016-01-27 06:20 +0100
    Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION  quirk Adrian Hunter <adrian.hunter@intel.com> - 2016-01-27 14:10 +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-27 14:30 +0100
        Re: [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk Ulf Hansson <ulf.hansson@linaro.org> - 2016-01-27 16:10 +0100

#1318660 — [RFC PATCH 0/21] Totally remove SDHCI_QUIRK_BROKEN_CARD_DETECTION quirk

Ulf wants to make sdhci into a library, but it's a huge task
since any improvemts may touch too much platforms. But at least
we should make some effort to push things torwards to this target.

This patchset remove SDHCI_QUIRK_BROKEN_CARD_DETECTION from sdhci
to gradually reduce quirk of sdhci.

Firstly, SDHCI_QUIRK_BROKEN_CARD_DETECTION aims at claiming the slot is
a "broken-cd" one, but "broken-cd" is not a quirk from my view.
In addition, mmc core stack had already obtain "broken-cd" from dts via
mmc_of_parse and pass MMC_CAP_NEEDS_POLL to mmc->caps. So we can reuse it
instead of SDHCI_QUIRK_BROKEN_CARD_DETECTION.

However, before doing the cleanup work, I find that geting _of_ property
for sdhci* is not so pretty good and consistent. Some variant drives use
mmc_of_parse, while another ones use sdhci_get_of_property. I also find some
variant drivers combine these two paths. So in order to make my "broken-cd"
cleanup running, I decide to add mmc_of_parse into sdhci_get_of_property and
replace mmc_of_parse with sdhci_get_of_property for all variant drivers if
needed.

Unfortunately, I don't have all these platforms touched to test my patchset.
I might make some mistakes for these changes, so any comments are welcomed.



Shawn Lin (21):
  mmc: sdhci-pltfm: consolidate parsing path
  mmc: sdhci-iproc: consolidate parsing path
  mmc: sdhci-msm: consolidate parsing path
  mmc: sdhci-of-arasan: consolidate parsing path
  mmc: sdhci-of-at91: consolidate parsing path
  mmc: sdhci-of-esdhc: consolidate parsing path
  mmc: sdhci-pxav3: consolidate parsing path
  mmc: sdhci-sirf: check sdhci_get_of_property return value
  mmc: sdhci_f_sdh30: check sdhci_get_of_property return value
  mmc: sdhci: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-acpi: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-bcm-kona: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-bcm2835: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-esdhc-imx: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-msm: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-of-esdhc: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-pci-core: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-pltfm: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-pxav2: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci-s3c: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION
  mmc: sdhci.h: remove SDHCI_QUIRK_BROKEN_CARD_DETECTION

 drivers/mmc/host/sdhci-acpi.c      |  3 +--
 drivers/mmc/host/sdhci-bcm-kona.c  |  3 ---
 drivers/mmc/host/sdhci-bcm2835.c   |  5 +++--
 drivers/mmc/host/sdhci-esdhc-imx.c |  9 +++++----
 drivers/mmc/host/sdhci-iproc.c     |  5 +++--
 drivers/mmc/host/sdhci-msm.c       |  6 ++----
 drivers/mmc/host/sdhci-of-arasan.c | 11 ++++-------
 drivers/mmc/host/sdhci-of-at91.c   |  4 +---
 drivers/mmc/host/sdhci-of-esdhc.c  | 25 ++++++++++++-------------
 drivers/mmc/host/sdhci-pci-core.c  |  5 ++++-
 drivers/mmc/host/sdhci-pltfm.c     | 26 +++++++++++++++++++-------
 drivers/mmc/host/sdhci-pltfm.h     |  2 +-
 drivers/mmc/host/sdhci-pxav2.c     |  1 -
 drivers/mmc/host/sdhci-pxav3.c     |  4 +---
 drivers/mmc/host/sdhci-s3c.c       |  2 +-
 drivers/mmc/host/sdhci-sirf.c      |  4 +++-
 drivers/mmc/host/sdhci.c           | 14 +++++---------
 drivers/mmc/host/sdhci.h           | 30 ++++++++++++++----------------
 drivers/mmc/host/sdhci_f_sdh30.c   |  5 ++++-
 19 files changed, 83 insertions(+), 81 deletions(-)

-- 
2.3.7

[toc] | [next] | [standalone]

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

On 27/01/16 07:05, Shawn Lin wrote:
> Ulf wants to make sdhci into a library, but it's a huge task
> since any improvemts may touch too much platforms. But at least
> we should make some effort to push things torwards to this target.
> 
> This patchset remove SDHCI_QUIRK_BROKEN_CARD_DETECTION from sdhci
> to gradually reduce quirk of sdhci.
> 
> Firstly, SDHCI_QUIRK_BROKEN_CARD_DETECTION aims at claiming the slot is
> a "broken-cd" one, but "broken-cd" is not a quirk from my view.
> In addition, mmc core stack had already obtain "broken-cd" from dts via
> mmc_of_parse and pass MMC_CAP_NEEDS_POLL to mmc->caps. So we can reuse it
> instead of SDHCI_QUIRK_BROKEN_CARD_DETECTION.

That assumes there is no driver that wants to disable the card detect
interrupts and disable the use of the Present State register, but still use
a Card Detect GPIO and therefore not have MMC_CAP_NEEDS_POLL.

A way forward should provide for drivers do to things like that.

One way is to make selected existing functions into library functions and
provide callbacks for them.  In this case do with sdhci_set_card_detection()
what Russell King did with sdhci_reset().  However Ulf is against new callbacks.

I would much prefer the structure for a SDHCI library be put in place, or at
least agreed to, before individual quirks are tackled.

In my view Ulf needs to explain how the SDHCI library is going to work,
particularly in the absence of new callbacks.

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

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

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.

-- 
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]

#1319050

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?

Kind regards
Uffe

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web