| Message ID | 20180110153250.13094-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10 January 2018 at 16:32, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Intel Edison the Broadcom WiFi card, which is connected to SDIO, > requires 2.0v, while the host, according to Intel Merrifield TRM, > supports 1.8v supply only. > > The card announces itself as > > mmc2: new ultra high speed DDR50 SDIO card at address 0001 > > Introduce a custom OCR mask and ->set_power() callback to override 2.0v > supply on Intel Merrifield platforms by enforcing 1.8v power choice. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > - address comments given by Adrian > drivers/mmc/host/sdhci-pci-core.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c > index 3e4f04fd5175..24c2b2504b3f 100644 > --- a/drivers/mmc/host/sdhci-pci-core.c > +++ b/drivers/mmc/host/sdhci-pci-core.c > @@ -778,6 +778,8 @@ static int intel_mrfld_mmc_probe_slot(struct sdhci_pci_slot *slot) > slot->host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > break; > case INTEL_MRFLD_SDIO: > + /* Advertise 2.0v for compatibility with the SDIO card's OCR */ > + slot->host->ocr_mask = MMC_VDD_20_21 | MMC_VDD_165_195; > slot->host->mmc->caps |= MMC_CAP_NONREMOVABLE | > MMC_CAP_POWER_OFF_CARD; > break; > @@ -789,10 +791,35 @@ static int intel_mrfld_mmc_probe_slot(struct sdhci_pci_slot *slot) > return 0; > } > > +static void intel_mrfld_sdhci_set_power(struct sdhci_host *host, > + unsigned char mode, unsigned short vdd) > +{ > + /* > + * Without a regulator, SDHCI does not support 2.0v but we get > + * here because we advertised 2.0v support for compatibility > + * with the SDIO card's OCR. Map it to 1.8v for the purpose of > + * turning on the power. > + */ > + if (IS_ERR(host->mmc->supply.vmmc) && vdd == ilog2(MMC_VDD_20_21)) > + vdd = ilog2(MMC_VDD_165_195); Why not instead extend the range in sdhci_set_power_noreg() to also check for MMC_VDD_20_21? Or is there a problem with that? > + > + sdhci_set_power(host, mode, vdd); > +} > + 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 Wed, 2018-01-10 at 18:01 +0100, Ulf Hansson wrote: > On 10 January 2018 at 16:32, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Intel Edison the Broadcom WiFi card, which is connected to SDIO, > > requires 2.0v, while the host, according to Intel Merrifield TRM, > > supports 1.8v supply only. > > + /* > > + * Without a regulator, SDHCI does not support 2.0v but we > > get > > + * here because we advertised 2.0v support for compatibility > > + * with the SDIO card's OCR. Map it to 1.8v for the purpose > > of > > + * turning on the power. > > + */ > > + if (IS_ERR(host->mmc->supply.vmmc) && vdd == > > ilog2(MMC_VDD_20_21)) > > + vdd = ilog2(MMC_VDD_165_195); > > Why not instead extend the range in sdhci_set_power_noreg() to also > check for MMC_VDD_20_21? > > Or is there a problem with that? Do we have any grounds to do this in generic code? Moreover, if we check for 2.0v what should we do in generic code? For my understanding case _20_21: pwr = _180; is absolutely wrong in generic code (see how it looked in v1 of this patch).
On 10 January 2018 at 19:04, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Wed, 2018-01-10 at 18:01 +0100, Ulf Hansson wrote: >> On 10 January 2018 at 16:32, Andy Shevchenko >> <andriy.shevchenko@linux.intel.com> wrote: >> > On Intel Edison the Broadcom WiFi card, which is connected to SDIO, >> > requires 2.0v, while the host, according to Intel Merrifield TRM, >> > supports 1.8v supply only. > >> > + /* >> > + * Without a regulator, SDHCI does not support 2.0v but we >> > get >> > + * here because we advertised 2.0v support for compatibility >> > + * with the SDIO card's OCR. Map it to 1.8v for the purpose >> > of >> > + * turning on the power. >> > + */ >> > + if (IS_ERR(host->mmc->supply.vmmc) && vdd == >> > ilog2(MMC_VDD_20_21)) >> > + vdd = ilog2(MMC_VDD_165_195); >> >> Why not instead extend the range in sdhci_set_power_noreg() to also >> check for MMC_VDD_20_21? >> >> Or is there a problem with that? > > Do we have any grounds to do this in generic code? > > Moreover, if we check for 2.0v what should we do in generic code? > For my understanding > > case _20_21: > pwr = _180; Yeah, why is that a problem? You should never reach this point, unless the host announce support via the ocr mask, for the _20_21 VDD. What does other variants do in this regards? 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 Wed, 2018-01-10 at 21:06 +0100, Ulf Hansson wrote: > On 10 January 2018 at 19:04, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, 2018-01-10 at 18:01 +0100, Ulf Hansson wrote: > > > On 10 January 2018 at 16:32, Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> wrote: > > > Why not instead extend the range in sdhci_set_power_noreg() to > > > also > > > check for MMC_VDD_20_21? > > > > > > Or is there a problem with that? > > > > Do we have any grounds to do this in generic code? > > > > Moreover, if we check for 2.0v what should we do in generic code? > > For my understanding > > > > case _20_21: > > pwr = _180; > > Yeah, why is that a problem? For me there is none. I assume that there might be hardware which we do not possess and which would be broken by this change. If you think this is okay, the resulting patch can be just two liner: 1) add custom ocr_mask in sdhci-pci-core; 2) add additional case in sdhci generic code. > You should never reach this point, unless the host announce support > via the ocr mask, for the _20_21 VDD. > > What does other variants do in this regards? I'm not sure I understand what "variants" you meant. Hardware? Host controllers? Cards?
On 10/01/18 22:06, Ulf Hansson wrote: > On 10 January 2018 at 19:04, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: >> On Wed, 2018-01-10 at 18:01 +0100, Ulf Hansson wrote: >>> On 10 January 2018 at 16:32, Andy Shevchenko >>> <andriy.shevchenko@linux.intel.com> wrote: >>>> On Intel Edison the Broadcom WiFi card, which is connected to SDIO, >>>> requires 2.0v, while the host, according to Intel Merrifield TRM, >>>> supports 1.8v supply only. >> >>>> + /* >>>> + * Without a regulator, SDHCI does not support 2.0v but we >>>> get >>>> + * here because we advertised 2.0v support for compatibility >>>> + * with the SDIO card's OCR. Map it to 1.8v for the purpose >>>> of >>>> + * turning on the power. >>>> + */ >>>> + if (IS_ERR(host->mmc->supply.vmmc) && vdd == >>>> ilog2(MMC_VDD_20_21)) >>>> + vdd = ilog2(MMC_VDD_165_195); >>> >>> Why not instead extend the range in sdhci_set_power_noreg() to also >>> check for MMC_VDD_20_21? >>> >>> Or is there a problem with that? >> >> Do we have any grounds to do this in generic code? >> >> Moreover, if we check for 2.0v what should we do in generic code? >> For my understanding >> >> case _20_21: >> pwr = _180; > > Yeah, why is that a problem? Shouldn't be a problem. Just add a comment: /* * Without a regulator, SDHCI does not support 2.0v so we only * get here if the driver deliberately added the 2.0v range to * ocr_avail. Map it to 1.8V for the purpose of turning on the * power. */ case MMC_VDD_20_21: pwr = SDHCI_POWER_180; break; > > You should never reach this point, unless the host announce support > via the ocr mask, for the _20_21 VDD. > > What does other variants do in this regards? Use regulators or 3V I expect. -- 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 Thu, 2018-01-11 at 09:57 +0200, Adrian Hunter wrote: > On 10/01/18 22:06, Ulf Hansson wrote: > > On 10 January 2018 at 19:04, Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > On Wed, 2018-01-10 at 18:01 +0100, Ulf Hansson wrote: > > > > On 10 January 2018 at 16:32, Andy Shevchenko > > > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > On Intel Edison the Broadcom WiFi card, which is connected to > > > > > SDIO, > > > > > requires 2.0v, while the host, according to Intel Merrifield > > > > > TRM, > > > > > supports 1.8v supply only. > > > > > + /* > > > > > + * Without a regulator, SDHCI does not support 2.0v > > > > > but we > > > > > get > > > > > + * here because we advertised 2.0v support for > > > > > compatibility > > > > > + * with the SDIO card's OCR. Map it to 1.8v for the > > > > > purpose > > > > > of > > > > > + * turning on the power. > > > > > + */ > > > > > + if (IS_ERR(host->mmc->supply.vmmc) && vdd == > > > > > ilog2(MMC_VDD_20_21)) > > > > > + vdd = ilog2(MMC_VDD_165_195); > > > > > > > > Why not instead extend the range in sdhci_set_power_noreg() to > > > > also > > > > check for MMC_VDD_20_21? > > > > > > > > Or is there a problem with that? > > > > > > Do we have any grounds to do this in generic code? > > > > > > Moreover, if we check for 2.0v what should we do in generic code? > > > For my understanding > > > > > > case _20_21: > > > pwr = _180; > > > > Yeah, why is that a problem? > > Shouldn't be a problem. Just add a comment: > > /* > * Without a regulator, SDHCI does not support 2.0v so > we only > * get here if the driver deliberately added the 2.0v > range to > * ocr_avail. Map it to 1.8V for the purpose of turning > on the > * power. > */ > case MMC_VDD_20_21: > pwr = SDHCI_POWER_180; > break; OK. I will send v4 soon after giving some testing. > > What does other variants do in this regards? > > Use regulators or 3V I expect. Yep, correct.
diff --git a/drivers/mmc/host/sdhci-pci-core.c b/drivers/mmc/host/sdhci-pci-core.c index 3e4f04fd5175..24c2b2504b3f 100644 --- a/drivers/mmc/host/sdhci-pci-core.c +++ b/drivers/mmc/host/sdhci-pci-core.c @@ -778,6 +778,8 @@ static int intel_mrfld_mmc_probe_slot(struct sdhci_pci_slot *slot) slot->host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; break; case INTEL_MRFLD_SDIO: + /* Advertise 2.0v for compatibility with the SDIO card's OCR */ + slot->host->ocr_mask = MMC_VDD_20_21 | MMC_VDD_165_195; slot->host->mmc->caps |= MMC_CAP_NONREMOVABLE | MMC_CAP_POWER_OFF_CARD; break; @@ -789,10 +791,35 @@ static int intel_mrfld_mmc_probe_slot(struct sdhci_pci_slot *slot) return 0; } +static void intel_mrfld_sdhci_set_power(struct sdhci_host *host, + unsigned char mode, unsigned short vdd) +{ + /* + * Without a regulator, SDHCI does not support 2.0v but we get + * here because we advertised 2.0v support for compatibility + * with the SDIO card's OCR. Map it to 1.8v for the purpose of + * turning on the power. + */ + if (IS_ERR(host->mmc->supply.vmmc) && vdd == ilog2(MMC_VDD_20_21)) + vdd = ilog2(MMC_VDD_165_195); + + sdhci_set_power(host, mode, vdd); +} + +static const struct sdhci_ops intel_mrfld_sdhci_pci_ops = { + .set_clock = sdhci_set_clock, + .set_power = intel_mrfld_sdhci_set_power, + .enable_dma = sdhci_pci_enable_dma, + .set_bus_width = sdhci_set_bus_width, + .reset = sdhci_reset, + .set_uhs_signaling = sdhci_set_uhs_signaling, +}; + static const struct sdhci_pci_fixes sdhci_intel_mrfld_mmc = { .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC, .quirks2 = SDHCI_QUIRK2_BROKEN_HS200 | SDHCI_QUIRK2_PRESET_VALUE_BROKEN, + .ops = &intel_mrfld_sdhci_pci_ops, .allow_runtime_pm = true, .probe_slot = intel_mrfld_mmc_probe_slot, };
On Intel Edison the Broadcom WiFi card, which is connected to SDIO, requires 2.0v, while the host, according to Intel Merrifield TRM, supports 1.8v supply only. The card announces itself as mmc2: new ultra high speed DDR50 SDIO card at address 0001 Introduce a custom OCR mask and ->set_power() callback to override 2.0v supply on Intel Merrifield platforms by enforcing 1.8v power choice. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- - address comments given by Adrian drivers/mmc/host/sdhci-pci-core.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)