Re: [PATCH v2, 2/2] mmc: sdhci-pltfm: enable interrupt mode to detect card

[Scott Wood <scottwood@freescale.com>]

On Mon, 2015-05-18 at 21:16 -0500, Lu Yangbo-B47093 wrote:
> 
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, May 19, 2015 4:43 AM
> > To: Lu Yangbo-B47093
> > Cc: Ulf Hansson; linux-mmc; Chris Ball
> > Subject: Re: [PATCH v2, 2/2] mmc: sdhci-pltfm: enable interrupt mode to
> > detect card
> > 
> > On Mon, 2015-05-18 at 04:55 -0500, Lu Yangbo-B47093 wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > > > Sent: Monday, May 18, 2015 5:50 PM
> > > > To: Lu Yangbo-B47093
> > > > Cc: linux-mmc; Chris Ball; Wood Scott-B07421
> > > > Subject: Re: [PATCH v2, 2/2] mmc: sdhci-pltfm: enable interrupt mode
> > > > to detect card
> > > >
> > > > On 18 May 2015 at 11:30, Lu Y.B. <yangbo.lu@freescale.com> wrote:
> > > > >
> > > > >
> > > > >> -----Original Message-----
> > > > >> From: Ulf Hansson [mailto:ulf.hansson@linaro.org]
> > > > >> Sent: Monday, May 18, 2015 5:04 PM
> > > > >> To: Lu Yangbo-B47093
> > > > >> Cc: linux-mmc; Chris Ball; Wood Scott-B07421
> > > > >> Subject: Re: [PATCH v2, 2/2] mmc: sdhci-pltfm: enable interrupt
> > > > >> mode to detect card
> > > > >>
> > > > >> On 15 May 2015 at 04:46, Yangbo Lu <yangbo.lu@freescale.com> wrote:
> > > > >> > Enable interrupt mode to detect card instead of polling mode
> > > > >> > for
> > > > >> > P1020/P4080/P5020/P5040/T1040 by removing the quirk
> > > > >> > SDHCI_QUIRK_BROKEN_CARD_DETECTION. This could improve data
> > > > >> > transferring performance and avoid the call trace caused by
> > > > >> > polling card status sometime.
> > > > >> >
> > > > >> > Signed-off-by: Yangbo Lu <yangbo.lu@freescale.com>
> > > > >> > ----
> > > > >> > Changes for v2:
> > > > >> >         - Aligned all "of_device_is_compatibles" in same column
> > > > >> > ---
> > > > >> >  drivers/mmc/host/sdhci-pltfm.c | 7 +++++++
> > > > >> >  1 file changed, 7 insertions(+)
> > > > >> >
> > > > >> > diff --git a/drivers/mmc/host/sdhci-pltfm.c
> > > > >> > b/drivers/mmc/host/sdhci-pltfm.c index c5b01d6..97128f3 100644
> > > > >> > --- a/drivers/mmc/host/sdhci-pltfm.c
> > > > >> > +++ b/drivers/mmc/host/sdhci-pltfm.c
> > > > >> > @@ -102,6 +102,13 @@ void sdhci_get_of_property(struct
> > > > >> > platform_device
> > > > >> *pdev)
> > > > >> >                     of_device_is_compatible(np, "fsl,mpc8536-
> > esdhc"))
> > > > >> >                         host->quirks |=
> > > > >> > SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
> > > > >> >
> > > > >> > +               if (of_device_is_compatible(np,
> > > > >> > + "fsl,p5040-esdhc")
> > > > ||
> > > > >> > +                   of_device_is_compatible(np,
> > > > >> > + "fsl,p5020-esdhc")
> > > > ||
> > > > >> > +                   of_device_is_compatible(np,
> > > > >> > + "fsl,p4080-esdhc")
> > > > ||
> > > > >> > +                   of_device_is_compatible(np,
> > > > >> > + "fsl,p1020-esdhc")
> > > > ||
> > > > >> > +                   of_device_is_compatible(np, "fsl,t1040-
> > esdhc"))
> > > > >> > +                       host->quirks &=
> > > > >> > + ~SDHCI_QUIRK_BROKEN_CARD_DETECTION;
> > > > >>
> > > > >> This looks strange to me.
> > > > >>
> > > > >> Why not just change the DT files for the relevant platforms to
> > > > >> not enable "broken-cd"?
> > > > >>
> > > > > Thanks Uffe.
> > > > > Because most platforms using sdhci-of-esdhc have broken CD, we
> > > > > hope
> > > > this quirk is selected in default.
> > > > > Only several platforms could use CD to detect and we clear this
> > > > > bit for
> > > > them.
> > > >
> > > > Hmm.
> > > >
> > > > Those platforms could still update their DT files and add
> > > > "broken-cd", since that would be the proper description of the HW.
> > > > Once that's done, it would enable you to remove the
> > > > SDHCI_QUIRK_BROKEN_CARD_DETECTION as default, right?
> > > >
> > > Yes, and if remove SDHCI_QUIRK_BROKEN_CARD_DETECTION as default,
> > 'borken-cd' would be needed to be added for most platforms using esdhc.
> > 
> > I was OK with changing the device tree if it just meant that things that
> > previously didn't work now work.  I'm not OK with requiring the device
> > trees to change in order for things that already work to stay working.
> > 
> > In general it is not reasonable to expect device trees to enumerate
> > hardware bugs since the bugs (or the need to work aronud them) are often
> > discovered after the device tree has been created.
> > 
> > Yangbo, when I asked why you couldn't use SVR you said that it was a
> > common SDHC file.  But, the file that sets
> > SDHCI_QUIRK_BROKEN_CARD_DETECTION in the first place is sdhci-of-esdhc.c
> > which is Freescale-specific.  Why not just test SVR in there to determine
> > whether the quirk should be set?
> Thanks Scott.
> The SVR testing could be only used for PPC. It doesn’t support ARM that we will use.
> I think using the dts should be better way.

None of the chips that this patch looks for are ARM chips.  Are you're
saying our ARM chips don't have any form of identification?  Even if
that's the case, there's nothing wrong with looking at the device tree;
it's changing the device tree that I'm objecting to if there's an
alternative.

In any case, I don't know why these checks are being added to common
code rather than sdhci-of-esdhc.c.

-Scott