| Message ID | 1446763200-10234-1-git-send-email-alcooperx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote: > Add quirk to disable SDR50 mode for controllers/boards that have > problems with this mode. No thanks! No more quirks please! Kind regards Uffe > > Signed-off-by: Al Cooper <alcooperx@gmail.com> > --- > drivers/mmc/host/sdhci.c | 3 +++ > drivers/mmc/host/sdhci.h | 2 ++ > 2 files changed, 5 insertions(+) > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c > index b48565e..71067c7 100644 > --- a/drivers/mmc/host/sdhci.c > +++ b/drivers/mmc/host/sdhci.c > @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host) > } else if (caps[1] & SDHCI_SUPPORT_SDR50) > mmc->caps |= MMC_CAP_UHS_SDR50; > > + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50) > + mmc->caps &= ~MMC_CAP_UHS_SDR50; > + > if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 && > (caps[1] & SDHCI_SUPPORT_HS400)) > mmc->caps2 |= MMC_CAP2_HS400; > diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h > index 9d4aa31..0941c94 100644 > --- a/drivers/mmc/host/sdhci.h > +++ b/drivers/mmc/host/sdhci.h > @@ -412,6 +412,8 @@ struct sdhci_host { > #define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14) > /* Broken Clock divider zero in controller */ > #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15) > +/* Controller does not support SDR50 */ > +#define SDHCI_QUIRK2_BROKEN_SDR50 (1<<16) > /* > * When internal clock is disabled, a delay is needed before modifying the > * SD clock frequency or enabling back the internal clock. > -- > 1.9.0.138.g2de3478 > -- 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 Fri, Nov 6, 2015 at 3:14 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> No thanks! No more quirks please!
OK, I'll move this functionality into the sdhci-brcmstb driver and
re-submit the patch set.
Thanks
Al
--
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
Hi Ulf, On 15-11-06 12:14 AM, Ulf Hansson wrote: > On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote: >> Add quirk to disable SDR50 mode for controllers/boards that have >> problems with this mode. > > No thanks! No more quirks please! > I'm fine with not having this quirk added (I don't need this one but use multiple of the other quirks in the driver) But, what if I also needed it in my driver? When do we determine when a quirk should be added to sdhci.c or not. What about existing quirks - should the current ones be moved to multiple existing drivers? > Kind regards > Uffe > >> >> Signed-off-by: Al Cooper <alcooperx@gmail.com> >> --- >> drivers/mmc/host/sdhci.c | 3 +++ >> drivers/mmc/host/sdhci.h | 2 ++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >> index b48565e..71067c7 100644 >> --- a/drivers/mmc/host/sdhci.c >> +++ b/drivers/mmc/host/sdhci.c >> @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host) >> } else if (caps[1] & SDHCI_SUPPORT_SDR50) >> mmc->caps |= MMC_CAP_UHS_SDR50; >> >> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50) >> + mmc->caps &= ~MMC_CAP_UHS_SDR50; >> + Perhaps a lot of these quirks can be solved by having a generic mechanism to override any of the values in the caps registers rather than adding all these quirks? >> if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 && >> (caps[1] & SDHCI_SUPPORT_HS400)) >> mmc->caps2 |= MMC_CAP2_HS400; >> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h >> index 9d4aa31..0941c94 100644 >> --- a/drivers/mmc/host/sdhci.h >> +++ b/drivers/mmc/host/sdhci.h >> @@ -412,6 +412,8 @@ struct sdhci_host { >> #define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14) >> /* Broken Clock divider zero in controller */ >> #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15) >> +/* Controller does not support SDR50 */ >> +#define SDHCI_QUIRK2_BROKEN_SDR50 (1<<16) >> /* >> * When internal clock is disabled, a delay is needed before modifying the >> * SD clock frequency or enabling back the internal clock. >> -- >> 1.9.0.138.g2de3478 >> Regards, Scott -- 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 06/11/15 15:55, Scott Branden wrote: > Hi Ulf, > > On 15-11-06 12:14 AM, Ulf Hansson wrote: >> On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote: >>> Add quirk to disable SDR50 mode for controllers/boards that have >>> problems with this mode. >> >> No thanks! No more quirks please! >> > > I'm fine with not having this quirk added (I don't need this one but use > multiple of the other quirks in the driver) But, what if I also needed > it in my driver? When do we determine when a quirk should be added to > sdhci.c or not. What about existing quirks - should the current ones be > moved to multiple existing drivers? >> Kind regards >> Uffe >> >>> >>> Signed-off-by: Al Cooper <alcooperx@gmail.com> >>> --- >>> drivers/mmc/host/sdhci.c | 3 +++ >>> drivers/mmc/host/sdhci.h | 2 ++ >>> 2 files changed, 5 insertions(+) >>> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> index b48565e..71067c7 100644 >>> --- a/drivers/mmc/host/sdhci.c >>> +++ b/drivers/mmc/host/sdhci.c >>> @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host) >>> } else if (caps[1] & SDHCI_SUPPORT_SDR50) >>> mmc->caps |= MMC_CAP_UHS_SDR50; >>> >>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50) >>> + mmc->caps &= ~MMC_CAP_UHS_SDR50; >>> + > Perhaps a lot of these quirks can be solved by having a generic > mechanism to override any of the values in the caps registers rather > than adding all these quirks? Are the capabilities register override specific to the Arasan controller or is there a generic and well define SDIO configuration register for these registers? The register information I am looking at seems to suggest this is part of how you glue your SDIO controller to your SoC. The entire purpose of Al's changes were precisely so we do not have to fiddle with these capabilities register like we are currently doing in some versions of our downstream kernel.
On 7 November 2015 at 00:55, Scott Branden <sbranden@broadcom.com> wrote: > Hi Ulf, > > On 15-11-06 12:14 AM, Ulf Hansson wrote: >> >> On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote: >>> >>> Add quirk to disable SDR50 mode for controllers/boards that have >>> problems with this mode. >> >> >> No thanks! No more quirks please! >> > > I'm fine with not having this quirk added (I don't need this one but use > multiple of the other quirks in the driver) But, what if I also needed it > in my driver? When do we determine when a quirk should be added to sdhci.c > or not. What about existing quirks - should the current ones be moved to > multiple existing drivers? The sdhci driver should turn into a library providing generic helper functions. Each host can then pick which functions to use and also deal with its own "quirks", instead of managing those in generic code. I guess we might end up getting a bit more code in total, but on the other hand the code would be better optimized and also maintainable. >> >> Kind regards >> Uffe >> >>> >>> Signed-off-by: Al Cooper <alcooperx@gmail.com> >>> --- >>> drivers/mmc/host/sdhci.c | 3 +++ >>> drivers/mmc/host/sdhci.h | 2 ++ >>> 2 files changed, 5 insertions(+) >>> >>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>> index b48565e..71067c7 100644 >>> --- a/drivers/mmc/host/sdhci.c >>> +++ b/drivers/mmc/host/sdhci.c >>> @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host) >>> } else if (caps[1] & SDHCI_SUPPORT_SDR50) >>> mmc->caps |= MMC_CAP_UHS_SDR50; >>> >>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50) >>> + mmc->caps &= ~MMC_CAP_UHS_SDR50; >>> + > > Perhaps a lot of these quirks can be solved by having a generic mechanism to > override any of the values in the caps registers rather than adding all > these quirks? Sure, it makes sense if it can decreases the number of quirks! > >>> if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 && >>> (caps[1] & SDHCI_SUPPORT_HS400)) >>> mmc->caps2 |= MMC_CAP2_HS400; >>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h >>> index 9d4aa31..0941c94 100644 >>> --- a/drivers/mmc/host/sdhci.h >>> +++ b/drivers/mmc/host/sdhci.h >>> @@ -412,6 +412,8 @@ struct sdhci_host { >>> #define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14) >>> /* Broken Clock divider zero in controller */ >>> #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15) >>> +/* Controller does not support SDR50 */ >>> +#define SDHCI_QUIRK2_BROKEN_SDR50 (1<<16) >>> /* >>> * When internal clock is disabled, a delay is needed before modifying >>> the >>> * SD clock frequency or enabling back the internal clock. >>> -- >>> 1.9.0.138.g2de3478 >>> > > Regards, > Scott 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 15-11-09 02:45 AM, Ulf Hansson wrote: > On 7 November 2015 at 00:55, Scott Branden <sbranden@broadcom.com> wrote: >> Hi Ulf, >> >> On 15-11-06 12:14 AM, Ulf Hansson wrote: >>> >>> On 5 November 2015 at 23:39, Al Cooper <alcooperx@gmail.com> wrote: >>>> >>>> Add quirk to disable SDR50 mode for controllers/boards that have >>>> problems with this mode. >>> >>> >>> No thanks! No more quirks please! >>> >> >> I'm fine with not having this quirk added (I don't need this one but use >> multiple of the other quirks in the driver) But, what if I also needed it >> in my driver? When do we determine when a quirk should be added to sdhci.c >> or not. What about existing quirks - should the current ones be moved to >> multiple existing drivers? > > The sdhci driver should turn into a library providing generic helper > functions. Each host can then pick which functions to use and also > deal with its own "quirks", instead of managing those in generic code. > > I guess we might end up getting a bit more code in total, but on the > other hand the code would be better optimized and also maintainable. > OK, if I need to add any new quirks I will look into this a bit more and see if it can be done. I don't have a need to do this right now so if anyone else wants to have a look feel free to. >>> >>> Kind regards >>> Uffe >>> >>>> >>>> Signed-off-by: Al Cooper <alcooperx@gmail.com> >>>> --- >>>> drivers/mmc/host/sdhci.c | 3 +++ >>>> drivers/mmc/host/sdhci.h | 2 ++ >>>> 2 files changed, 5 insertions(+) >>>> >>>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c >>>> index b48565e..71067c7 100644 >>>> --- a/drivers/mmc/host/sdhci.c >>>> +++ b/drivers/mmc/host/sdhci.c >>>> @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host) >>>> } else if (caps[1] & SDHCI_SUPPORT_SDR50) >>>> mmc->caps |= MMC_CAP_UHS_SDR50; >>>> >>>> + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50) >>>> + mmc->caps &= ~MMC_CAP_UHS_SDR50; >>>> + >> >> Perhaps a lot of these quirks can be solved by having a generic mechanism to >> override any of the values in the caps registers rather than adding all >> these quirks? > > Sure, it makes sense if it can decreases the number of quirks! > I think it can in some cases. In fact - the hardware designers do not even set the correct settings in the caps register on some of the SoCs. The caps register does not appear to be used in the hardware other than for "info" purposes to read by the driver - and when the info is wrong this may lead to many of the quirks that have been added by others over the years. >> >>>> if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 && >>>> (caps[1] & SDHCI_SUPPORT_HS400)) >>>> mmc->caps2 |= MMC_CAP2_HS400; >>>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h >>>> index 9d4aa31..0941c94 100644 >>>> --- a/drivers/mmc/host/sdhci.h >>>> +++ b/drivers/mmc/host/sdhci.h >>>> @@ -412,6 +412,8 @@ struct sdhci_host { >>>> #define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14) >>>> /* Broken Clock divider zero in controller */ >>>> #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15) >>>> +/* Controller does not support SDR50 */ >>>> +#define SDHCI_QUIRK2_BROKEN_SDR50 (1<<16) >>>> /* >>>> * When internal clock is disabled, a delay is needed before modifying >>>> the >>>> * SD clock frequency or enabling back the internal clock. >>>> -- >>>> 1.9.0.138.g2de3478 >>>> >> >> Regards, >> Scott > > Kind regards > Uffe > Thanks, Scott -- 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 b48565e..71067c7 100644 --- a/drivers/mmc/host/sdhci.c +++ b/drivers/mmc/host/sdhci.c @@ -3176,6 +3176,9 @@ int sdhci_add_host(struct sdhci_host *host) } else if (caps[1] & SDHCI_SUPPORT_SDR50) mmc->caps |= MMC_CAP_UHS_SDR50; + if (host->quirks2 & SDHCI_QUIRK2_BROKEN_SDR50) + mmc->caps &= ~MMC_CAP_UHS_SDR50; + if (host->quirks2 & SDHCI_QUIRK2_CAPS_BIT63_FOR_HS400 && (caps[1] & SDHCI_SUPPORT_HS400)) mmc->caps2 |= MMC_CAP2_HS400; diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h index 9d4aa31..0941c94 100644 --- a/drivers/mmc/host/sdhci.h +++ b/drivers/mmc/host/sdhci.h @@ -412,6 +412,8 @@ struct sdhci_host { #define SDHCI_QUIRK2_ACMD23_BROKEN (1<<14) /* Broken Clock divider zero in controller */ #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15) +/* Controller does not support SDR50 */ +#define SDHCI_QUIRK2_BROKEN_SDR50 (1<<16) /* * When internal clock is disabled, a delay is needed before modifying the * SD clock frequency or enabling back the internal clock.
Add quirk to disable SDR50 mode for controllers/boards that have problems with this mode. Signed-off-by: Al Cooper <alcooperx@gmail.com> --- drivers/mmc/host/sdhci.c | 3 +++ drivers/mmc/host/sdhci.h | 2 ++ 2 files changed, 5 insertions(+)