| Message ID | 1434227067-9600-1-git-send-email-festevam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jun 13, 2015 at 5:24 PM, Fabio Estevam <festevam@gmail.com> wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Since commit 312449efd16bb06 ("mmc: core: Fix sequence for I/O voltage > in DDR mode for eMMC") the mmc voltage is set to 1.8V inside > mmc_select_hs_ddr(), even if 'no-1-8-v' property is present. > > This causes the following error on a mx6sl board with the 'no-1-8-v' > property passed in the device tree: > > mmc0: power class selection to bus width 8 ddr 4 failed > mmc0: error -110 whilst initialising MMC card > > Fix this problem by only setting the mmc voltage to 1.8V if the > 'no-1-8-v' property is absent. > > Reported-by: Kevin Lemoi <kevin.lemoi@savant.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> Acked-by: Otavio Salvador <otavio@ossystems.com.br>
On 13 June 2015 at 22:24, Fabio Estevam <festevam@gmail.com> wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > Since commit 312449efd16bb06 ("mmc: core: Fix sequence for I/O voltage > in DDR mode for eMMC") the mmc voltage is set to 1.8V inside > mmc_select_hs_ddr(), even if 'no-1-8-v' property is present. > > This causes the following error on a mx6sl board with the 'no-1-8-v' > property passed in the device tree: > > mmc0: power class selection to bus width 8 ddr 4 failed > mmc0: error -110 whilst initialising MMC card > > Fix this problem by only setting the mmc voltage to 1.8V if the > 'no-1-8-v' property is absent. What card (eMMC, SD, MMC) and which host driver is being used here? > > Reported-by: Kevin Lemoi <kevin.lemoi@savant.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > Changes since v1: > - Fix it in the core code instead of doing it inside the sdhci imx code > > drivers/mmc/core/host.c | 2 ++ > drivers/mmc/core/mmc.c | 8 +++++++- > include/linux/mmc/host.h | 1 + > include/linux/mmc/mmc.h | 1 + > 4 files changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c > index 01fa1ed..736c985 100644 > --- a/drivers/mmc/core/host.c > +++ b/drivers/mmc/core/host.c > @@ -527,6 +527,8 @@ int mmc_of_parse(struct mmc_host *host) > host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR; > if (of_find_property(np, "mmc-hs400-1_2v", &len)) > host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR; > + if (of_find_property(np, "no-1-8-v", &len)) > + host->caps2 |= MMC_CAP_3_3V_ONLY_DDR; Do you intend to use 3.3V as the I/O voltage level for an eMMCs running in DDR mode? Isn't that violation of the eMMC spec? > > host->dsr_req = !of_property_read_u32(np, "dsr", &host->dsr); > if (host->dsr_req && (host->dsr & ~0xffff)) { > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c > index e726903..e76b9af 100644 > --- a/drivers/mmc/core/mmc.c > +++ b/drivers/mmc/core/mmc.c > @@ -205,6 +205,10 @@ static void mmc_select_card_type(struct mmc_card *card) > avail_type |= EXT_CSD_CARD_TYPE_DDR_1_8V; > } > > + if (caps2 & MMC_CAP_3_3V_ONLY_DDR && > + card_type & EXT_CSD_CARD_TYPE_DDR_1_8V) > + avail_type |= EXT_CSD_CARD_TYPE_DDR_3_3V_ONLY; > + > if (caps & MMC_CAP_1_2V_DDR && > card_type & EXT_CSD_CARD_TYPE_DDR_1_2V) { > hs_max_dtr = MMC_HIGH_DDR_MAX_DTR; > @@ -1028,7 +1032,9 @@ static int mmc_select_hs_ddr(struct mmc_card *card) > err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120); > > if (err && (card->mmc_avail_type & EXT_CSD_CARD_TYPE_DDR_1_8V)) > - err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180); > + if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_DDR_3_3V_ONLY)) > + err = __mmc_set_signal_voltage(host, > + MMC_SIGNAL_VOLTAGE_180); > > /* make sure vccq is 3.3v after switching disaster */ > if (err) > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h > index 1369e54..5aa999f 100644 > --- a/include/linux/mmc/host.h > +++ b/include/linux/mmc/host.h > @@ -289,6 +289,7 @@ struct mmc_host { > #define MMC_CAP2_HSX00_1_2V (MMC_CAP2_HS200_1_2V_SDR | MMC_CAP2_HS400_1_2V) > #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17) > #define MMC_CAP2_NO_WRITE_PROTECT (1 << 18) /* No physical write protect pin, assume that card is always read-write */ > +#define MMC_CAP_3_3V_ONLY_DDR (1 << 19) /* Only supports 3.3V DDR */ > > mmc_pm_flag_t pm_caps; /* supported pm features */ > > diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h > index 15f2c4a..b3cbd3c 100644 > --- a/include/linux/mmc/mmc.h > +++ b/include/linux/mmc/mmc.h > @@ -380,6 +380,7 @@ struct _mmc_csd { > #define EXT_CSD_CARD_TYPE_HS400_1_2V (1<<7) /* Card can run at 200MHz DDR, 1.2V */ > #define EXT_CSD_CARD_TYPE_HS400 (EXT_CSD_CARD_TYPE_HS400_1_8V | \ > EXT_CSD_CARD_TYPE_HS400_1_2V) > +#define EXT_CSD_CARD_TYPE_DDR_3_3V_ONLY (1<<8) > > #define EXT_CSD_BUS_WIDTH_1 0 /* Card is in 1 bit mode */ > #define EXT_CSD_BUS_WIDTH_4 1 /* Card is in 4 bit mode */ > -- > 1.9.1 > 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
Hi Ulf, On Mon, Jun 15, 2015 at 9:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > What card (eMMC, SD, MMC) and which host driver is being used here? It is an eMMC and the host driver is sdhci-esdhc-imx.c. >> --- a/drivers/mmc/core/host.c >> +++ b/drivers/mmc/core/host.c >> @@ -527,6 +527,8 @@ int mmc_of_parse(struct mmc_host *host) >> host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR; >> if (of_find_property(np, "mmc-hs400-1_2v", &len)) >> host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR; >> + if (of_find_property(np, "no-1-8-v", &len)) >> + host->caps2 |= MMC_CAP_3_3V_ONLY_DDR; > > Do you intend to use 3.3V as the I/O voltage level for an eMMCs > running in DDR mode? Isn't that violation of the eMMC spec? Yes, I would like to use 3.3V as the I/O voltage level for an eMMC in DDR mode. This was the behaviour prior to 312449efd16bb0, which I would like to keep so that our custom board based on mx6sl could work. It works fine with 3.14 kernel, but not on 4.1-rc due to the fact that the board cannot provide 1.8V level, hence 'no-1-8-v' property is passed in the device tree. With this patch applied 3.3V is applied and the eMMC can successfully operate in 3.3V DDR mode. Regards, Fabio Estevam -- 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 Mon, Jun 15, 2015 at 9:23 AM, Fabio Estevam <festevam@gmail.com> wrote: > Hi Ulf, > > On Mon, Jun 15, 2015 at 9:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> What card (eMMC, SD, MMC) and which host driver is being used here? > > It is an eMMC and the host driver is sdhci-esdhc-imx.c. > >>> --- a/drivers/mmc/core/host.c >>> +++ b/drivers/mmc/core/host.c >>> @@ -527,6 +527,8 @@ int mmc_of_parse(struct mmc_host *host) >>> host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR; >>> if (of_find_property(np, "mmc-hs400-1_2v", &len)) >>> host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR; >>> + if (of_find_property(np, "no-1-8-v", &len)) >>> + host->caps2 |= MMC_CAP_3_3V_ONLY_DDR; >> >> Do you intend to use 3.3V as the I/O voltage level for an eMMCs >> running in DDR mode? Isn't that violation of the eMMC spec? > > Yes, I would like to use 3.3V as the I/O voltage level for an eMMC in DDR mode. > > This was the behaviour prior to 312449efd16bb0, which I would like to > keep so that our custom board based on mx6sl could work. It works fine > with 3.14 kernel, but not on 4.1-rc due to the fact that the board > cannot provide 1.8V level, hence 'no-1-8-v' property is passed in the > device tree. With this patch applied 3.3V is applied and the eMMC can > successfully operate in 3.3V DDR mode. Also, looked at the eMMC spec and the only restriction I saw about using 3.3V voltage is with HS200 or HS400 devices: "VCCQ (I/O) 3.3 V range is not supported in either HS200 or HS400 devices" Regards, Fabio Estevam -- 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 June 2015 at 14:49, Fabio Estevam <festevam@gmail.com> wrote: > On Mon, Jun 15, 2015 at 9:23 AM, Fabio Estevam <festevam@gmail.com> wrote: >> Hi Ulf, >> >> On Mon, Jun 15, 2015 at 9:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: >> >>> What card (eMMC, SD, MMC) and which host driver is being used here? >> >> It is an eMMC and the host driver is sdhci-esdhc-imx.c. >> >>>> --- a/drivers/mmc/core/host.c >>>> +++ b/drivers/mmc/core/host.c >>>> @@ -527,6 +527,8 @@ int mmc_of_parse(struct mmc_host *host) >>>> host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR; >>>> if (of_find_property(np, "mmc-hs400-1_2v", &len)) >>>> host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR; >>>> + if (of_find_property(np, "no-1-8-v", &len)) >>>> + host->caps2 |= MMC_CAP_3_3V_ONLY_DDR; >>> >>> Do you intend to use 3.3V as the I/O voltage level for an eMMCs >>> running in DDR mode? Isn't that violation of the eMMC spec? >> >> Yes, I would like to use 3.3V as the I/O voltage level for an eMMC in DDR mode. >> >> This was the behaviour prior to 312449efd16bb0, which I would like to >> keep so that our custom board based on mx6sl could work. It works fine >> with 3.14 kernel, but not on 4.1-rc due to the fact that the board >> cannot provide 1.8V level, hence 'no-1-8-v' property is passed in the >> device tree. With this patch applied 3.3V is applied and the eMMC can >> successfully operate in 3.3V DDR mode. I had a closer look, should have done that before. I think we need to decide what "no-1-8-v" should be used for. Currently the DT documentation is unclear and depending on what SDHCI variant, the binding has different purpose. It's a mess! sdhci-pltfm + sdhci-esdhc-imx use the "no-1-8-v" DT binding to enable the SDHCI_QUIRK2_NO_1_8_V. The sdhci driver uses this quirk to mask the bus speed modes for SD UHS cards. In this regards, consider the two below options. 1) We have DT bindings for SD UHS speed modes, which somehow makes the "no-1-8-v" binding redundant (or deprecated). 2) If you can model the VCCQ power supply as as a regulator, you could use regulator_is_supported_voltage() API to find out similar information used to set SDHCI_QUIRK2_NO_1_8_V. In fact, sdhci already does that to mask MMC_CAP2_HSX00_1_2V, unless 1.2V is supported. In a third case, sdhci-pxav3 use the "no-1-8-v" DT binding to mask SDHCI_CAN_VDD_180/SDHCI_CAN_VDD_330, which seems wrong, as it has nothing to do with the power to the card (VMMC). It's also used it to mask MMC_CAP_1_8V_DDR. The summary, is that I would rather see us to move away from using the "no-1-8-v" DT binding and use the proper SD UHS bus speed modes binding instead. > > Also, looked at the eMMC spec and the only restriction I saw about > using 3.3V voltage is with HS200 or HS400 devices: > > "VCCQ (I/O) 3.3 V range is not supported in either HS200 or HS400 devices" You are correct! We should add a MMC_CAP_3V_DDR for this and a corresponding DT binding. Then we need to teach mmc_select_hs_ddr() and mmc_select_card_type() about the above new possible configuration. 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
Hi Ulf, On Tue, Jun 16, 2015 at 6:31 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > I had a closer look, should have done that before. > > I think we need to decide what "no-1-8-v" should be used for. > Currently the DT documentation is unclear and depending on what SDHCI > variant, the binding has different purpose. It's a mess! Thanks for your feedback. > > sdhci-pltfm + sdhci-esdhc-imx use the "no-1-8-v" DT binding to enable > the SDHCI_QUIRK2_NO_1_8_V. The sdhci driver uses this quirk to mask > the bus speed modes for SD UHS cards. > > In this regards, consider the two below options. > > 1) We have DT bindings for SD UHS speed modes, which somehow makes the > "no-1-8-v" binding redundant (or deprecated). > > 2) If you can model the VCCQ power supply as as a regulator, you could > use regulator_is_supported_voltage() API to find out similar > information used to set SDHCI_QUIRK2_NO_1_8_V. In fact, sdhci already > does that to mask MMC_CAP2_HSX00_1_2V, unless 1.2V is supported. > > In a third case, sdhci-pxav3 use the "no-1-8-v" DT binding to mask > SDHCI_CAN_VDD_180/SDHCI_CAN_VDD_330, which seems wrong, as it has > nothing to do with the power to the card (VMMC). It's also used it to > mask MMC_CAP_1_8V_DDR. > > The summary, is that I would rather see us to move away from using the > "no-1-8-v" DT binding and use the proper SD UHS bus speed modes > binding instead. Makes sense indeed. The challenge here is that we still need to keep the old dtb's working. Motivation for this patch was a custom mx6sl board that works fine with 3.14, but after upgrading the kernel to 4.0 the eMMC no longer can mount the rootfs. This is a regression caused by commit 312449efd16bb06, which applies 1.8V to the eMMC card that can only operate at 3.3V. Same thing happens on imx6q-sabresd: we have 3.3V voltage being supplied to the eMMC, but now we are operating it at 1.8V, even though if we have 'no-1-8-v' property in the device tree. While I understand the need for improvement that you clearly described, I would also like that the old dtb's could still work 'as-is' with a modern kernel. The path I propose here was the minimal invasive method I could come up to allow the old dtb compatibility. Thanks, Fabio Estevam -- 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 17 June 2015 at 18:25, Fabio Estevam <festevam@gmail.com> wrote: > Hi Ulf, > > On Tue, Jun 16, 2015 at 6:31 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> I had a closer look, should have done that before. >> >> I think we need to decide what "no-1-8-v" should be used for. >> Currently the DT documentation is unclear and depending on what SDHCI >> variant, the binding has different purpose. It's a mess! > > Thanks for your feedback. > >> >> sdhci-pltfm + sdhci-esdhc-imx use the "no-1-8-v" DT binding to enable >> the SDHCI_QUIRK2_NO_1_8_V. The sdhci driver uses this quirk to mask >> the bus speed modes for SD UHS cards. >> >> In this regards, consider the two below options. >> >> 1) We have DT bindings for SD UHS speed modes, which somehow makes the >> "no-1-8-v" binding redundant (or deprecated). >> >> 2) If you can model the VCCQ power supply as as a regulator, you could >> use regulator_is_supported_voltage() API to find out similar >> information used to set SDHCI_QUIRK2_NO_1_8_V. In fact, sdhci already >> does that to mask MMC_CAP2_HSX00_1_2V, unless 1.2V is supported. >> >> In a third case, sdhci-pxav3 use the "no-1-8-v" DT binding to mask >> SDHCI_CAN_VDD_180/SDHCI_CAN_VDD_330, which seems wrong, as it has >> nothing to do with the power to the card (VMMC). It's also used it to >> mask MMC_CAP_1_8V_DDR. >> >> The summary, is that I would rather see us to move away from using the >> "no-1-8-v" DT binding and use the proper SD UHS bus speed modes >> binding instead. > > Makes sense indeed. > > The challenge here is that we still need to keep the old dtb's working. We need to keep something that's been "broken" for ever to keep working. Huh, it prevents us from doing the correct solution. :-( How hard is it to update the DTBs in these cases? > > Motivation for this patch was a custom mx6sl board that works fine > with 3.14, but after upgrading the kernel to 4.0 the eMMC no longer > can mount the rootfs. This is a regression caused by commit > 312449efd16bb06, which applies 1.8V to the eMMC card that can only > operate at 3.3V. > > Same thing happens on imx6q-sabresd: we have 3.3V voltage being > supplied to the eMMC, but now we are operating it at 1.8V, even though > if we have 'no-1-8-v' property in the device tree. > > While I understand the need for improvement that you clearly > described, I would also like that the old dtb's could still work > 'as-is' with a modern kernel. > > The path I propose here was the minimal invasive method I could come > up to allow the old dtb compatibility. No, I don't want to sprinkle the core with hacks to make host driver works. Those hacks will have to reside in host drivers instead. Unless updating the DTB is out of the questions.... I suggest the following instead. 1) Let's add MMC_CAP_3V_DDR and a corresponding DT binding for it. Adopt mmc_select_hs_ddr() and mmc_select_card_type() to know about this configuration. 2) If we can assume that "no-1-8-v" for sdhci has the meaning of enabling MMC_CAP_3V_DDR, let's do that parsing in drivers/mmc/host/sdhci-pltfm.c. 3) Update the DT documentation to better describe the "no-1-8-v" binding. Let's also move it to be a sdhci specific binding and not a generic mmc binding, to limit its use. 4) Update the DTB files in the kernel for imx to use new binding for MMC_CAP_3V_DDR where applicable. Moreover convert the DTB for imx to use the proper bindings for UHS bus speed modes. Thus imx should have moved away from the "no-1-8-v" for future DTB updates. 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 Tue, Jun 16, 2015 at 11:31:50AM +0200, Ulf Hansson wrote: > On 15 June 2015 at 14:49, Fabio Estevam <festevam@gmail.com> wrote: > > On Mon, Jun 15, 2015 at 9:23 AM, Fabio Estevam <festevam@gmail.com> wrote: > >> Hi Ulf, > >> > >> On Mon, Jun 15, 2015 at 9:08 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > >> > >>> What card (eMMC, SD, MMC) and which host driver is being used here? > >> > >> It is an eMMC and the host driver is sdhci-esdhc-imx.c. > >> > >>>> --- a/drivers/mmc/core/host.c > >>>> +++ b/drivers/mmc/core/host.c > >>>> @@ -527,6 +527,8 @@ int mmc_of_parse(struct mmc_host *host) > >>>> host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR; > >>>> if (of_find_property(np, "mmc-hs400-1_2v", &len)) > >>>> host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR; > >>>> + if (of_find_property(np, "no-1-8-v", &len)) > >>>> + host->caps2 |= MMC_CAP_3_3V_ONLY_DDR; > >>> > >>> Do you intend to use 3.3V as the I/O voltage level for an eMMCs > >>> running in DDR mode? Isn't that violation of the eMMC spec? > >> > >> Yes, I would like to use 3.3V as the I/O voltage level for an eMMC in DDR mode. > >> > >> This was the behaviour prior to 312449efd16bb0, which I would like to > >> keep so that our custom board based on mx6sl could work. It works fine > >> with 3.14 kernel, but not on 4.1-rc due to the fact that the board > >> cannot provide 1.8V level, hence 'no-1-8-v' property is passed in the > >> device tree. With this patch applied 3.3V is applied and the eMMC can > >> successfully operate in 3.3V DDR mode. > > I had a closer look, should have done that before. > > I think we need to decide what "no-1-8-v" should be used for. > Currently the DT documentation is unclear and depending on what SDHCI > variant, the binding has different purpose. It's a mess! > That's true! > sdhci-pltfm + sdhci-esdhc-imx use the "no-1-8-v" DT binding to enable > the SDHCI_QUIRK2_NO_1_8_V. The sdhci driver uses this quirk to mask > the bus speed modes for SD UHS cards. > > In this regards, consider the two below options. > > 1) We have DT bindings for SD UHS speed modes, which somehow makes the > "no-1-8-v" binding redundant (or deprecated). > > 2) If you can model the VCCQ power supply as as a regulator, you could > use regulator_is_supported_voltage() API to find out similar > information used to set SDHCI_QUIRK2_NO_1_8_V. In fact, sdhci already > does that to mask MMC_CAP2_HSX00_1_2V, unless 1.2V is supported. > One problem of this way for IMX is that the voltage switch is controlled by register bit. It may be not suitable to model VCCQ as regualtor. > In a third case, sdhci-pxav3 use the "no-1-8-v" DT binding to mask > SDHCI_CAN_VDD_180/SDHCI_CAN_VDD_330, which seems wrong, as it has > nothing to do with the power to the card (VMMC). It's also used it to > mask MMC_CAP_1_8V_DDR. > > The summary, is that I would rather see us to move away from using the > "no-1-8-v" DT binding and use the proper SD UHS bus speed modes > binding instead. > One concern of using SD UHS bus speed binding is that we may have to define all speed mode under the node to support all different kinds of card. e.g. Originally it's: &usdhc3 { pinctrl-names = "default", "state_100mhz", "state_200mhz"; pinctrl-0 = <&pinctrl_usdhc3>; pinctrl-1 = <&pinctrl_usdhc3_100mhz>; pinctrl-2 = <&pinctrl_usdhc3_200mhz>; bus-width = <8>; cd-gpios = <&gpio2 10 GPIO_ACTIVE_HIGH>; wp-gpios = <&gpio2 15 GPIO_ACTIVE_HIGH>; keep-power-in-suspend; enable-sdio-wakeup; vmmc-supply = <&vcc_sd3>; status = "okay"; }; Now: &usdhc3 { pinctrl-names = "default", "state_100mhz", "state_200mhz"; pinctrl-0 = <&pinctrl_usdhc3>; pinctrl-1 = <&pinctrl_usdhc3_100mhz>; pinctrl-2 = <&pinctrl_usdhc3_200mhz>; bus-width = <8>; cd-gpios = <&gpio2 10 GPIO_ACTIVE_HIGH>; wp-gpios = <&gpio2 15 GPIO_ACTIVE_HIGH>; keep-power-in-suspend; enable-sdio-wakeup; vmmc-supply = <&vcc_sd3>; cap-sd-highspeed; cap-mmc-highspeed; sd-uhs-sdr12; sd-uhs-sdr25; sd-uhs-sdr50; sd-uhs-sdr104; sd-uhs-ddr50; status = "okay"; }; So the question comes: 1) it adds a lot duplicated things for different host node in device tree and this increase dts size. 2) do we really need to claim this uhs capability from device tree? Since it looks to me most is host controller capablity and not board dependant. 3) those uhs mode defined in device tree may be also conflict with common sdhci probe/detect function and maybe overwritten later. If do that, we may need re-structure the common sdhci driver. > > > > Also, looked at the eMMC spec and the only restriction I saw about > > using 3.3V voltage is with HS200 or HS400 devices: > > > > "VCCQ (I/O) 3.3 V range is not supported in either HS200 or HS400 devices" > > You are correct! We should add a MMC_CAP_3V_DDR for this and a > corresponding DT binding. > How about MMC_CAP_VIO_3V and MMC_CAP_VIO_1_8_V/1.2V to directly reflect the VIO capability? It can be reused for UHS/HSxx. > Then we need to teach mmc_select_hs_ddr() and mmc_select_card_type() > about the above new possible configuration. > > Kind regards > Uffe Regards Dong Aisheng -- 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, Jun 18, 2015 at 09:58:43AM +0200, Ulf Hansson wrote: > On 17 June 2015 at 18:25, Fabio Estevam <festevam@gmail.com> wrote: > > Hi Ulf, > > > > On Tue, Jun 16, 2015 at 6:31 AM, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > > >> I had a closer look, should have done that before. > >> > >> I think we need to decide what "no-1-8-v" should be used for. > >> Currently the DT documentation is unclear and depending on what SDHCI > >> variant, the binding has different purpose. It's a mess! > > > > Thanks for your feedback. > > > >> > >> sdhci-pltfm + sdhci-esdhc-imx use the "no-1-8-v" DT binding to enable > >> the SDHCI_QUIRK2_NO_1_8_V. The sdhci driver uses this quirk to mask > >> the bus speed modes for SD UHS cards. > >> > >> In this regards, consider the two below options. > >> > >> 1) We have DT bindings for SD UHS speed modes, which somehow makes the > >> "no-1-8-v" binding redundant (or deprecated). > >> > >> 2) If you can model the VCCQ power supply as as a regulator, you could > >> use regulator_is_supported_voltage() API to find out similar > >> information used to set SDHCI_QUIRK2_NO_1_8_V. In fact, sdhci already > >> does that to mask MMC_CAP2_HSX00_1_2V, unless 1.2V is supported. > >> > >> In a third case, sdhci-pxav3 use the "no-1-8-v" DT binding to mask > >> SDHCI_CAN_VDD_180/SDHCI_CAN_VDD_330, which seems wrong, as it has > >> nothing to do with the power to the card (VMMC). It's also used it to > >> mask MMC_CAP_1_8V_DDR. > >> > >> The summary, is that I would rather see us to move away from using the > >> "no-1-8-v" DT binding and use the proper SD UHS bus speed modes > >> binding instead. > > > > Makes sense indeed. > > > > The challenge here is that we still need to keep the old dtb's working. > > We need to keep something that's been "broken" for ever to keep > working. Huh, it prevents us from doing the correct solution. :-( > > How hard is it to update the DTBs in these cases? > > > > > Motivation for this patch was a custom mx6sl board that works fine > > with 3.14, but after upgrading the kernel to 4.0 the eMMC no longer > > can mount the rootfs. This is a regression caused by commit > > 312449efd16bb06, which applies 1.8V to the eMMC card that can only > > operate at 3.3V. > > > > Same thing happens on imx6q-sabresd: we have 3.3V voltage being > > supplied to the eMMC, but now we are operating it at 1.8V, even though > > if we have 'no-1-8-v' property in the device tree. > > > > While I understand the need for improvement that you clearly > > described, I would also like that the old dtb's could still work > > 'as-is' with a modern kernel. > > > > The path I propose here was the minimal invasive method I could come > > up to allow the old dtb compatibility. > > No, I don't want to sprinkle the core with hacks to make host driver > works. Those hacks will have to reside in host drivers instead. > > Unless updating the DTB is out of the questions.... I suggest the > following instead. > > 1) > Let's add MMC_CAP_3V_DDR and a corresponding DT binding for it. Adopt > mmc_select_hs_ddr() and mmc_select_card_type() to know about this > configuration. > Yes, that's probably a quirk way to fix the issue. Host needs to consider the DDR mode support for both 3V and 1.8v way. If host does not support 1.8V VIO but it supports DDR, then we only claim 3V DDR support. It may be something like the treating way of MMC_CAP2_HSX00_1_2V in sdhci.c. > 2) > If we can assume that "no-1-8-v" for sdhci has the meaning of enabling > MMC_CAP_3V_DDR, let's do that parsing in > drivers/mmc/host/sdhci-pltfm.c. > For me, no-1-8-v may be better only reflect the host VIO capability, no DDR functionality. > 3) > Update the DT documentation to better describe the "no-1-8-v" binding. > Let's also move it to be a sdhci specific binding and not a generic > mmc binding, to limit its use. > > 4) > Update the DTB files in the kernel for imx to use new binding for > MMC_CAP_3V_DDR where applicable. Moreover convert the DTB for imx to > use the proper bindings for UHS bus speed modes. Thus imx should have > moved away from the "no-1-8-v" for future DTB updates. > Yes, i agree from a long term, we should move away from no-1-8-v since no-1-8-v is not well defined. Another issue of this way is that how about host also not support 1.2v? no-1-2-v? One different option for a long term fix may be adding VIO capability in device tree. Then each host could claim it's VIO capability in device tree. Then Host driver could select the correct DDR/UHS/HSxx mode based on VIO capability. Regards Dong Aisheng > 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/core/host.c b/drivers/mmc/core/host.c index 01fa1ed..736c985 100644 --- a/drivers/mmc/core/host.c +++ b/drivers/mmc/core/host.c @@ -527,6 +527,8 @@ int mmc_of_parse(struct mmc_host *host) host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR; if (of_find_property(np, "mmc-hs400-1_2v", &len)) host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR; + if (of_find_property(np, "no-1-8-v", &len)) + host->caps2 |= MMC_CAP_3_3V_ONLY_DDR; host->dsr_req = !of_property_read_u32(np, "dsr", &host->dsr); if (host->dsr_req && (host->dsr & ~0xffff)) { diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c index e726903..e76b9af 100644 --- a/drivers/mmc/core/mmc.c +++ b/drivers/mmc/core/mmc.c @@ -205,6 +205,10 @@ static void mmc_select_card_type(struct mmc_card *card) avail_type |= EXT_CSD_CARD_TYPE_DDR_1_8V; } + if (caps2 & MMC_CAP_3_3V_ONLY_DDR && + card_type & EXT_CSD_CARD_TYPE_DDR_1_8V) + avail_type |= EXT_CSD_CARD_TYPE_DDR_3_3V_ONLY; + if (caps & MMC_CAP_1_2V_DDR && card_type & EXT_CSD_CARD_TYPE_DDR_1_2V) { hs_max_dtr = MMC_HIGH_DDR_MAX_DTR; @@ -1028,7 +1032,9 @@ static int mmc_select_hs_ddr(struct mmc_card *card) err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_120); if (err && (card->mmc_avail_type & EXT_CSD_CARD_TYPE_DDR_1_8V)) - err = __mmc_set_signal_voltage(host, MMC_SIGNAL_VOLTAGE_180); + if (!(card->mmc_avail_type & EXT_CSD_CARD_TYPE_DDR_3_3V_ONLY)) + err = __mmc_set_signal_voltage(host, + MMC_SIGNAL_VOLTAGE_180); /* make sure vccq is 3.3v after switching disaster */ if (err) diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h index 1369e54..5aa999f 100644 --- a/include/linux/mmc/host.h +++ b/include/linux/mmc/host.h @@ -289,6 +289,7 @@ struct mmc_host { #define MMC_CAP2_HSX00_1_2V (MMC_CAP2_HS200_1_2V_SDR | MMC_CAP2_HS400_1_2V) #define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17) #define MMC_CAP2_NO_WRITE_PROTECT (1 << 18) /* No physical write protect pin, assume that card is always read-write */ +#define MMC_CAP_3_3V_ONLY_DDR (1 << 19) /* Only supports 3.3V DDR */ mmc_pm_flag_t pm_caps; /* supported pm features */ diff --git a/include/linux/mmc/mmc.h b/include/linux/mmc/mmc.h index 15f2c4a..b3cbd3c 100644 --- a/include/linux/mmc/mmc.h +++ b/include/linux/mmc/mmc.h @@ -380,6 +380,7 @@ struct _mmc_csd { #define EXT_CSD_CARD_TYPE_HS400_1_2V (1<<7) /* Card can run at 200MHz DDR, 1.2V */ #define EXT_CSD_CARD_TYPE_HS400 (EXT_CSD_CARD_TYPE_HS400_1_8V | \ EXT_CSD_CARD_TYPE_HS400_1_2V) +#define EXT_CSD_CARD_TYPE_DDR_3_3V_ONLY (1<<8) #define EXT_CSD_BUS_WIDTH_1 0 /* Card is in 1 bit mode */ #define EXT_CSD_BUS_WIDTH_4 1 /* Card is in 4 bit mode */