[PATCH v3] sdhci: Advertise 2.0v supply on SDIO host interface

[PATCH v3] sdhci: Advertise 2.0v supply on SDIO host interface

[Andy Shevchenko]

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);
+
+	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,
 };
-- 
2.15.1

Re: [PATCH v3] sdhci: Advertise 2.0v supply on SDIO host interface

[Ulf Hansson]

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

Re: [PATCH v3] sdhci: Advertise 2.0v supply on SDIO host interface

[Andy Shevchenko]

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).

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3] sdhci: Advertise 2.0v supply on SDIO host interface

[Ulf Hansson]

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

Re: [PATCH v3] sdhci: Advertise 2.0v supply on SDIO host interface

[Andy Shevchenko]

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?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v3] sdhci: Advertise 2.0v supply on SDIO host interface

[Adrian Hunter]

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.

Re: [PATCH v3] sdhci: Advertise 2.0v supply on SDIO host interface

[Andy Shevchenko]

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.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy