| Message ID | 20180628081331.13051-3-stefan@agner.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote: > Some hosts are capable of running higher speed modes but do not > have the board support for it. Introduce a quirk which prevents > the stack from using modes running at 100MHz or faster. To cap the freq, use the DT property "max-frequency". To enable certain speed modes, use the corresponding speed mode binding. For example "sd-uhs-sdr*" and "mmc-hs200*". Documented in Documentation/devicetree/bindings/mmc/mmc.txt In case the sdhci cap register, doesn't reflect the board support properly, such that you may want to disable some speed modes, then you may benefit from using the DT properties "sdhci-caps*. Documented in Documentation/devicetree/bindings/mmc/sdhci.txt Kind regards Uffe > > Signed-off-by: Stefan Agner <stefan@agner.ch> > --- > drivers/mmc/host/sdhci.c | 8 ++++++++ > drivers/mmc/host/sdhci.h | 2 ++ > 2 files changed, 10 insertions(+) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index 1c828e0e9905..8ac257dfaab3 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -3749,6 +3749,14 @@ int sdhci_setup_host(struct sdhci_host *host) > } > } > > + if (host->quirks2 & SDHCI_QUIRK2_NO_UHS_HS200_HS400) { > + host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | > + SDHCI_SUPPORT_DDR50); > + > + mmc->caps2 &= ~(MMC_CAP2_HSX00_1_8V | MMC_CAP2_HSX00_1_2V | > + MMC_CAP2_HS400_ES); > + } > + > if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) { > host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | > SDHCI_SUPPORT_DDR50); > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index 23966f887da6..cb2433d6d61f 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -450,6 +450,8 @@ struct sdhci_host { > * obtainable timeout. > */ > #define SDHCI_QUIRK2_DISABLE_HW_TIMEOUT (1<<17) > +/* Do not support any higher speeds (>50MHz) */ > +#define SDHCI_QUIRK2_NO_UHS_HS200_HS400 (1<<18) > > int irq; /* Device IRQ */ > void __iomem *ioaddr; /* Mapped address */ > -- > 2.18.0 > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02.07.2018 16:36, Ulf Hansson wrote: > On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote: >> Some hosts are capable of running higher speed modes but do not >> have the board support for it. Introduce a quirk which prevents >> the stack from using modes running at 100MHz or faster. > > To cap the freq, use the DT property "max-frequency". To enable > certain speed modes, use the corresponding speed mode binding. For > example "sd-uhs-sdr*" and "mmc-hs200*". > Documented in Documentation/devicetree/bindings/mmc/mmc.txt I had bad experience with max-frequency: Some higher speed modes seem not to work reliably if constraint to low frequencies. E.g. we had lots of devices fail in practise with HS400@100MHz... So it is doing what it should, but it just seems that higher speed modes do not necessarily run well with lower frequencies... So I'd rather prefer to limit speed modes as it is done right now. > > In case the sdhci cap register, doesn't reflect the board support > properly, such that you may want to disable some speed modes, then you > may benefit from using the DT properties "sdhci-caps*. > Documented in Documentation/devicetree/bindings/mmc/sdhci.txt Hm, yeah I guess something like sdhci-caps-mask = /bits/ 64 <((SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)> would come close. But it does not restrict MMC modes such as HS200/HS400. There seem to be no mmc-caps... My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not restrict modes correctly. Currently the driver checks whether >=100MHz pinctrl settings are available, and if not uses the quirk to restrict higher speed modes. Removing that would break device tree backward compatibility... We probably could do something like this: if (!100mhzpinctrl) { if (!sdhci-caps) { /* * If no 100MHz/200MHz pinctrl are available, SDHC caps should be used to restrict * modes. Falling back to old behavior... */ pr_warn(...) host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; } } -- Stefan > > Kind regards > Uffe > >> >> Signed-off-by: Stefan Agner <stefan@agner.ch> >> --- >> drivers/mmc/host/sdhci.c | 8 ++++++++ >> drivers/mmc/host/sdhci.h | 2 ++ >> 2 files changed, 10 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index 1c828e0e9905..8ac257dfaab3 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -3749,6 +3749,14 @@ int sdhci_setup_host(struct sdhci_host *host) >> } >> } >> >> + if (host->quirks2 & SDHCI_QUIRK2_NO_UHS_HS200_HS400) { >> + host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | >> + SDHCI_SUPPORT_DDR50); >> + >> + mmc->caps2 &= ~(MMC_CAP2_HSX00_1_8V | MMC_CAP2_HSX00_1_2V | >> + MMC_CAP2_HS400_ES); >> + } >> + >> if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) { >> host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | >> SDHCI_SUPPORT_DDR50); >> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h >> index 23966f887da6..cb2433d6d61f 100644 >> --- a/drivers/mmc/host/sdhci.h >> +++ b/drivers/mmc/host/sdhci.h >> @@ -450,6 +450,8 @@ struct sdhci_host { >> * obtainable timeout. >> */ >> #define SDHCI_QUIRK2_DISABLE_HW_TIMEOUT (1<<17) >> +/* Do not support any higher speeds (>50MHz) */ >> +#define SDHCI_QUIRK2_NO_UHS_HS200_HS400 (1<<18) >> >> int irq; /* Device IRQ */ >> void __iomem *ioaddr; /* Mapped address */ >> -- >> 2.18.0 >> -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 July 2018 at 10:48, Stefan Agner <stefan@agner.ch> wrote: > On 02.07.2018 16:36, Ulf Hansson wrote: >> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote: >>> Some hosts are capable of running higher speed modes but do not >>> have the board support for it. Introduce a quirk which prevents >>> the stack from using modes running at 100MHz or faster. >> >> To cap the freq, use the DT property "max-frequency". To enable >> certain speed modes, use the corresponding speed mode binding. For >> example "sd-uhs-sdr*" and "mmc-hs200*". >> Documented in Documentation/devicetree/bindings/mmc/mmc.txt > > I had bad experience with max-frequency: Some higher speed modes seem > not to work reliably if constraint to low frequencies. E.g. we had lots > of devices fail in practise with HS400@100MHz... So it is doing what it > should, but it just seems that higher speed modes do not necessarily run > well with lower frequencies... > > So I'd rather prefer to limit speed modes as it is done right now. > >> >> In case the sdhci cap register, doesn't reflect the board support >> properly, such that you may want to disable some speed modes, then you >> may benefit from using the DT properties "sdhci-caps*. >> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt > > Hm, yeah I guess something like > > sdhci-caps-mask = /bits/ 64 <((SDHCI_SUPPORT_SDR104 | > SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)> > > would come close. > > But it does not restrict MMC modes such as HS200/HS400. There seem to be > no mmc-caps... Right. The solution to fix this, should be to *not* set those DT properties, like "mmc-hs*" for example. That should work, no? > > > My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not > restrict modes correctly. Currently the driver checks whether >=100MHz > pinctrl settings are available, and if not uses the quirk to restrict > higher speed modes. Removing that would break device tree backward > compatibility... Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather how the pinctrl setting becomes interpreted when setting the quirk. > > We probably could do something like this: > if (!100mhzpinctrl) { > if (!sdhci-caps) { > /* > * If no 100MHz/200MHz pinctrl are available, SDHC caps should > be used to restrict > * modes. Falling back to old behavior... > */ > pr_warn(...) > host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > } > } > I am not sure what makes best sense here. Let me have a look at patch 3 as well. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04.07.2018 12:07, Ulf Hansson wrote: > On 3 July 2018 at 10:48, Stefan Agner <stefan@agner.ch> wrote: >> On 02.07.2018 16:36, Ulf Hansson wrote: >>> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote: >>>> Some hosts are capable of running higher speed modes but do not >>>> have the board support for it. Introduce a quirk which prevents >>>> the stack from using modes running at 100MHz or faster. >>> >>> To cap the freq, use the DT property "max-frequency". To enable >>> certain speed modes, use the corresponding speed mode binding. For >>> example "sd-uhs-sdr*" and "mmc-hs200*". >>> Documented in Documentation/devicetree/bindings/mmc/mmc.txt >> >> I had bad experience with max-frequency: Some higher speed modes seem >> not to work reliably if constraint to low frequencies. E.g. we had lots >> of devices fail in practise with HS400@100MHz... So it is doing what it >> should, but it just seems that higher speed modes do not necessarily run >> well with lower frequencies... >> >> So I'd rather prefer to limit speed modes as it is done right now. >> >>> >>> In case the sdhci cap register, doesn't reflect the board support >>> properly, such that you may want to disable some speed modes, then you >>> may benefit from using the DT properties "sdhci-caps*. >>> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt >> >> Hm, yeah I guess something like >> >> sdhci-caps-mask = /bits/ 64 <((SDHCI_SUPPORT_SDR104 | >> SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)> >> >> would come close. >> >> But it does not restrict MMC modes such as HS200/HS400. There seem to be >> no mmc-caps... > > Right. > > The solution to fix this, should be to *not* set those DT properties, > like "mmc-hs*" for example. That should work, no? > The controller does not make use of the dt modes so far, so I can't not set those properties... >> >> >> My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not >> restrict modes correctly. Currently the driver checks whether >=100MHz >> pinctrl settings are available, and if not uses the quirk to restrict >> higher speed modes. Removing that would break device tree backward >> compatibility... > > Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather > how the pinctrl setting becomes interpreted when setting the quirk. > Yes, sorry for the confusion. SDHCI_QUIRK2_NO_1_8_V is fine, it is just not the quirk this driver needs. I argue that commit ad93220de7da ("mmc: sdhci-esdhc-imx: change pinctrl state according to uhs mode") chose the wrong quirk from the beginning... Afaict, the quirk needed here does not exist. -- Stefan >> >> We probably could do something like this: >> if (!100mhzpinctrl) { >> if (!sdhci-caps) { >> /* >> * If no 100MHz/200MHz pinctrl are available, SDHC caps should >> be used to restrict >> * modes. Falling back to old behavior... >> */ >> pr_warn(...) >> host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; >> } >> } >> > > I am not sure what makes best sense here. Let me have a look at patch 3 as well. > > [...] > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4 July 2018 at 12:55, Stefan Agner <stefan@agner.ch> wrote: > On 04.07.2018 12:07, Ulf Hansson wrote: >> On 3 July 2018 at 10:48, Stefan Agner <stefan@agner.ch> wrote: >>> On 02.07.2018 16:36, Ulf Hansson wrote: >>>> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote: >>>>> Some hosts are capable of running higher speed modes but do not >>>>> have the board support for it. Introduce a quirk which prevents >>>>> the stack from using modes running at 100MHz or faster. >>>> >>>> To cap the freq, use the DT property "max-frequency". To enable >>>> certain speed modes, use the corresponding speed mode binding. For >>>> example "sd-uhs-sdr*" and "mmc-hs200*". >>>> Documented in Documentation/devicetree/bindings/mmc/mmc.txt >>> >>> I had bad experience with max-frequency: Some higher speed modes seem >>> not to work reliably if constraint to low frequencies. E.g. we had lots >>> of devices fail in practise with HS400@100MHz... So it is doing what it >>> should, but it just seems that higher speed modes do not necessarily run >>> well with lower frequencies... >>> >>> So I'd rather prefer to limit speed modes as it is done right now. >>> >>>> >>>> In case the sdhci cap register, doesn't reflect the board support >>>> properly, such that you may want to disable some speed modes, then you >>>> may benefit from using the DT properties "sdhci-caps*. >>>> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt >>> >>> Hm, yeah I guess something like >>> >>> sdhci-caps-mask = /bits/ 64 <((SDHCI_SUPPORT_SDR104 | >>> SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)> >>> >>> would come close. >>> >>> But it does not restrict MMC modes such as HS200/HS400. There seem to be >>> no mmc-caps... >> >> Right. >> >> The solution to fix this, should be to *not* set those DT properties, >> like "mmc-hs*" for example. That should work, no? >> > > The controller does not make use of the dt modes so far, so I can't not > set those properties... Then where are the corresponding caps for the eMMC speed modes being set? Can't you just avoid setting them? > >>> >>> >>> My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not >>> restrict modes correctly. Currently the driver checks whether >=100MHz >>> pinctrl settings are available, and if not uses the quirk to restrict >>> higher speed modes. Removing that would break device tree backward >>> compatibility... >> >> Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather >> how the pinctrl setting becomes interpreted when setting the quirk. >> > > Yes, sorry for the confusion. SDHCI_QUIRK2_NO_1_8_V is fine, it is just > not the quirk this driver needs. > > I argue that commit ad93220de7da ("mmc: sdhci-esdhc-imx: change pinctrl > state according to uhs mode") chose the wrong quirk from the > beginning... That's seems reasonable! But it's been there since 3.13, so I guess we have to think about backwards compatibility issues, as you stated. > > Afaict, the quirk needed here does not exist. [...] Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04.07.2018 13:16, Ulf Hansson wrote: > On 4 July 2018 at 12:55, Stefan Agner <stefan@agner.ch> wrote: >> On 04.07.2018 12:07, Ulf Hansson wrote: >>> On 3 July 2018 at 10:48, Stefan Agner <stefan@agner.ch> wrote: >>>> On 02.07.2018 16:36, Ulf Hansson wrote: >>>>> On 28 June 2018 at 10:13, Stefan Agner <stefan@agner.ch> wrote: >>>>>> Some hosts are capable of running higher speed modes but do not >>>>>> have the board support for it. Introduce a quirk which prevents >>>>>> the stack from using modes running at 100MHz or faster. >>>>> >>>>> To cap the freq, use the DT property "max-frequency". To enable >>>>> certain speed modes, use the corresponding speed mode binding. For >>>>> example "sd-uhs-sdr*" and "mmc-hs200*". >>>>> Documented in Documentation/devicetree/bindings/mmc/mmc.txt >>>> >>>> I had bad experience with max-frequency: Some higher speed modes seem >>>> not to work reliably if constraint to low frequencies. E.g. we had lots >>>> of devices fail in practise with HS400@100MHz... So it is doing what it >>>> should, but it just seems that higher speed modes do not necessarily run >>>> well with lower frequencies... >>>> >>>> So I'd rather prefer to limit speed modes as it is done right now. >>>> >>>>> >>>>> In case the sdhci cap register, doesn't reflect the board support >>>>> properly, such that you may want to disable some speed modes, then you >>>>> may benefit from using the DT properties "sdhci-caps*. >>>>> Documented in Documentation/devicetree/bindings/mmc/sdhci.txt >>>> >>>> Hm, yeah I guess something like >>>> >>>> sdhci-caps-mask = /bits/ 64 <((SDHCI_SUPPORT_SDR104 | >>>> SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50) << 32)> >>>> >>>> would come close. >>>> >>>> But it does not restrict MMC modes such as HS200/HS400. There seem to be >>>> no mmc-caps... >>> >>> Right. >>> >>> The solution to fix this, should be to *not* set those DT properties, >>> like "mmc-hs*" for example. That should work, no? >>> >> >> The controller does not make use of the dt modes so far, so I can't not >> set those properties... > > Then where are the corresponding caps for the eMMC speed modes being set? > > Can't you just avoid setting them? > That was the right question: On closer look, the higher speed MMC modes are actually not set at all! So just using sdhci-caps-mask = <0x7 0x0>; (which masks SDR104/SDR50/DDR50) is doing the job as far as I can tell. >> >>>> >>>> >>>> My aim is to replace the SDHCI_QUIRK2_NO_1_8_V fix, which does not >>>> restrict modes correctly. Currently the driver checks whether >=100MHz >>>> pinctrl settings are available, and if not uses the quirk to restrict >>>> higher speed modes. Removing that would break device tree backward >>>> compatibility... >>> >>> Looks like the problem is not really SDHCI_QUIRK2_NO_1_8_V, but rather >>> how the pinctrl setting becomes interpreted when setting the quirk. >>> >> >> Yes, sorry for the confusion. SDHCI_QUIRK2_NO_1_8_V is fine, it is just >> not the quirk this driver needs. >> >> I argue that commit ad93220de7da ("mmc: sdhci-esdhc-imx: change pinctrl >> state according to uhs mode") chose the wrong quirk from the >> beginning... > > That's seems reasonable! But it's been there since 3.13, so I guess we > have to think about backwards compatibility issues, as you stated. > It seems that the driver already fakes SDHCI_CAPABILITIES/SDHCI_CAPABILITIES_1 in esdhc_readl_le, so instead using the sdhci-caps-mask we could implement the work around there. Not very clean, but backward compatible and everything self contained in the driver. -- Stefan >> >> Afaict, the quirk needed here does not exist. > > [...] > > Kind regards > Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c index 1c828e0e9905..8ac257dfaab3 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -3749,6 +3749,14 @@ int sdhci_setup_host(struct sdhci_host *host) } } + if (host->quirks2 & SDHCI_QUIRK2_NO_UHS_HS200_HS400) { + host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | + SDHCI_SUPPORT_DDR50); + + mmc->caps2 &= ~(MMC_CAP2_HSX00_1_8V | MMC_CAP2_HSX00_1_2V | + MMC_CAP2_HS400_ES); + } + if (host->quirks2 & SDHCI_QUIRK2_NO_1_8_V) { host->caps1 &= ~(SDHCI_SUPPORT_SDR104 | SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_DDR50); diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index 23966f887da6..cb2433d6d61f 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -450,6 +450,8 @@ struct sdhci_host { * obtainable timeout. */ #define SDHCI_QUIRK2_DISABLE_HW_TIMEOUT (1<<17) +/* Do not support any higher speeds (>50MHz) */ +#define SDHCI_QUIRK2_NO_UHS_HS200_HS400 (1<<18) int irq; /* Device IRQ */ void __iomem *ioaddr; /* Mapped address */
Some hosts are capable of running higher speed modes but do not have the board support for it. Introduce a quirk which prevents the stack from using modes running at 100MHz or faster. Signed-off-by: Stefan Agner <stefan@agner.ch> --- drivers/mmc/host/sdhci.c | 8 ++++++++ drivers/mmc/host/sdhci.h | 2 ++ 2 files changed, 10 insertions(+)