Skip to contentSkip to conversation

sdhci: Advertise 2.0v supply on SDIO host interface

PATCH Superseded Andy Shevchenko 3 participants 7 messages Jan 10, 2018 – Jan 11, 2018

Subsystems (2)
MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

v3 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Andy Shevchenko

author
#0 lore raw

PATCH v3 1/1 sdhci: Advertise 2.0v supply on SDIO host interface

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 <[email protected]>

---
- address comments given by Adrian

 drivers/mmc/host/sdhci-pci-core.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
drivers/mmc/host/sdhci-pci-core.c +27 -0
index 3e4f04fd5175..24c2b2504b3f 100644
@@ -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,
 };
Signature · 1 line
2.15.1
#1 lore raw
Quoted 35 lines · 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 <[email protected]>

---
- address comments given by Adrian

 drivers/mmc/host/sdhci-pci-core.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
drivers/mmc/host/sdhci-pci-core.c +3 -0
index 3e4f04fd5175..24c2b2504b3f 100644
@@ -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,

drivers/mmc/host/sdhci-pci-core.c static int intel_mrfld_mmc_probe_slot(struct sdhci_pci_slot *slot) · line 804

Quoted 10 lines
drivers/mmc/host/sdhci-pci-core.c +10 -0
+                                       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?

drivers/mmc/host/sdhci-pci-core.c static int intel_mrfld_mmc_probe_slot(struct sdhci_pci_slot *slot) · line 808

Quoted 4 lines
drivers/mmc/host/sdhci-pci-core.c +4 -0
+
+       sdhci_set_power(host, mode, vdd);
+}
+

Kind regards
Uffe

author
#2 lore raw
Quoted 18 lines · Ulf Hansson
Quoted 13 lines · 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.

(quoted diff) +2 -0
+       /*
+        * Without a regulator, SDHCI does not support 2.0v but we

get

(quoted diff) +2 -0
+        * here because we advertised 2.0v support for compatibility
+        * with the SDIO card's OCR. Map it to 1.8v for the purpose

of

drivers/mmc/host/sdhci-pci-core.c +3 -0
+        * turning on the power.
+        */
+       if (IS_ERR(host->mmc->supply.vmmc) && vdd ==
Quoted 2 lines

ilog2(MMC_VDD_20_21))
+ vdd = ilog2(MMC_VDD_165_195);

Quoted 4 lines

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

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
#3 lore raw
Quoted 24 lines · Andy Shevchenko
Quoted 18 lines · Ulf Hansson
Quoted 13 lines · 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.

(quoted diff) +2 -0
+       /*
+        * Without a regulator, SDHCI does not support 2.0v but we

get

(quoted diff) +2 -0
+        * here because we advertised 2.0v support for compatibility
+        * with the SDIO card's OCR. Map it to 1.8v for the purpose

of

drivers/mmc/host/sdhci-pci-core.c +3 -0
+        * turning on the power.
+        */
+       if (IS_ERR(host->mmc->supply.vmmc) && vdd ==
Quoted 2 lines

ilog2(MMC_VDD_20_21))
+ vdd = ilog2(MMC_VDD_165_195);

Quoted 4 lines

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?

Quoted 7 lines

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

author
#4 lore raw
Quoted 14 lines · Ulf Hansson
Quoted 12 lines · Andy Shevchenko
Quoted 8 lines · Ulf Hansson

On 10 January 2018 at 16:32, Andy Shevchenko
<[email protected]> 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?

Quoted 7 lines
Quoted 5 lines

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.

Quoted 4 lines

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?

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy
maintainer
#5 lore raw
In reply to Ulf Hansson
Quoted 28 lines · Ulf Hansson
Quoted 26 lines · Andy Shevchenko
Quoted 18 lines · Ulf Hansson
Quoted 13 lines · 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.

(quoted diff) +2 -0
+       /*
+        * Without a regulator, SDHCI does not support 2.0v but we

get

(quoted diff) +2 -0
+        * here because we advertised 2.0v support for compatibility
+        * with the SDIO card's OCR. Map it to 1.8v for the purpose

of

drivers/mmc/host/sdhci-pci-core.c +3 -0
+        * turning on the power.
+        */
+       if (IS_ERR(host->mmc->supply.vmmc) && vdd ==
Quoted 2 lines

ilog2(MMC_VDD_20_21))
+ vdd = ilog2(MMC_VDD_165_195);

Quoted 4 lines

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?

Quoted 7 lines
Quoted 5 lines

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;
Quoted 5 lines

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.

author
#6 lore raw
Quoted 51 lines · Adrian Hunter
Quoted 33 lines · Ulf Hansson
Quoted 31 lines · Andy Shevchenko
Quoted 22 lines · Ulf Hansson
Quoted 17 lines · 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.

(quoted diff) +2 -0
+       /*
+        * 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

drivers/mmc/host/sdhci-pci-core.c +3 -0
+        * turning on the power.
+        */
+       if (IS_ERR(host->mmc->supply.vmmc) && vdd ==
Quoted 2 lines

ilog2(MMC_VDD_20_21))
+ vdd = ilog2(MMC_VDD_165_195);

Quoted 5 lines

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?

Quoted 7 lines
Quoted 5 lines

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

Quoted 5 lines
		 * power.
		 */
		case MMC_VDD_20_21:
			pwr = SDHCI_POWER_180;
			break;

OK.
I will send v4 soon after giving some testing.

Quoted 3 lines
Quoted 1 line

What does other variants do in this regards?

Use regulators or 3V I expect.

Yep, correct.

Signature · 2 lines
Andy Shevchenko <[email protected]>
Intel Finland Oy