| Message ID | 1363629655-28917-1-git-send-email-pawel.moll@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Mar 18, 2013 at 06:00:55PM +0000, Pawel Moll wrote: > The MMCI driver, when used with a Device Tree described device, relies > on the "vmmc" voltage regulator supply to set the OCR register voltage bits, > using MMC core's mmc_regulator_get_supply() function. > > Without the regulator framework present there are no valid operating > voltages reported and the device initialisation fails: > > mmci-pl18x 1c050000.mmci: mmc0: PL180 manf 41 rev0 at 0x1c050000 irq 41,42 (pio) > mmc0: host doesn't support card's voltages > mmc0: error -22 whilst initialising MMC card Okay, this isn't going to work, because this means it's broken on a whole load of platforms. What it means is that this commit: commit 599c1d5c750ddf528c7c6d3cdc466708f0502e66 Author: Ulf Hansson <ulf.hansson@linaro.org> Date: Mon Jan 7 16:22:50 2013 +0100 ARM: 7620/1: mmc: mmci: Convert to use mmc_regulator_get_supply By using the mmc_regulator_get_supply API we are able to do some cleanups of the regulator code. Additionally let the regulator API handle the error printing. Cc: Chris Ball <cjb@laptop.org> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> is actually wrong and probably needs reverting.
On Mon, 2013-03-18 at 18:03 +0000, Russell King - ARM Linux wrote: > > Without the regulator framework present there are no valid operating > > voltages reported and the device initialisation fails: > > > > mmci-pl18x 1c050000.mmci: mmc0: PL180 manf 41 rev0 at 0x1c050000 irq 41,42 (pio) > > mmc0: host doesn't support card's voltages > > mmc0: error -22 whilst initialising MMC card > > Okay, this isn't going to work, because this means it's broken on a whole > load of platforms. What it means is that this commit: > > commit 599c1d5c750ddf528c7c6d3cdc466708f0502e66 > Author: Ulf Hansson <ulf.hansson@linaro.org> > Date: Mon Jan 7 16:22:50 2013 +0100 > > ARM: 7620/1: mmc: mmci: Convert to use mmc_regulator_get_supply > > By using the mmc_regulator_get_supply API we are able to do some > cleanups of the regulator code. Additionally let the regulator > API handle the error printing. > > Cc: Chris Ball <cjb@laptop.org> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > is actually wrong and probably needs reverting. Reverting it will not change anything as the driver had the same dependency before, just using : @@ -1391,29 +1379,13 @@ static int mmci_probe(struct amba_device *dev, } else dev_warn(&dev->dev, "could not get default pinstate\n"); -#ifdef CONFIG_REGULATOR - /* If we're using the regulator framework, try to fetch a regulator */ - host->vcc = regulator_get(&dev->dev, "vmmc"); - if (IS_ERR(host->vcc)) - host->vcc = NULL; - else { - int mask = mmc_regulator_get_ocrmask(host->vcc); - - if (mask < 0) - dev_err(&dev->dev, "error getting OCR mask (%d)\n", - mask); - else { - host->mmc->ocr_avail = (u32) mask; - if (plat->ocr_mask) - dev_warn(&dev->dev, - "Provided ocr_mask/setpower will not be used " - "(using regulator instead)\n"); - } - } -#endif - /* Fall back to platform data if no regulator is found */ - if (host->vcc == NULL) + /* Get regulators and the supported OCR mask */ + mmc_regulator_get_supply(mmc); + if (!mmc->ocr_avail) mmc->ocr_avail = plat->ocr_mask; + else if (plat->ocr_mask) + dev_warn(mmc_dev(mmc), "Platform OCR mask is ignored\n"); + mmc->caps = plat->capabilities; mmc->caps2 = plat->capabilities2; It was using the other MMC/regulator core helper, introduced by: commit 5c13941acc513669c7d07b28789c3f9ba66ddddf Author: David Brownell <dbrownell@users.sourceforge.net> Date: Wed Mar 11 03:30:43 2009 -0800 MMC: regulator utilities Glue between MMC and regulator stacks ... verified with some OMAP3 boards using adjustable and configured-as-fixed regulators on several MMC controllers. These calls are intended to be used by MMC host adapters using at least one regulator per host. Examples include slots with regulators supporting multiple voltages and ones using multiple voltage rails (e.g. DAT4..DAT7 using a separate supply, or a split rail chip like certain SDIO WLAN or eMMC solutions). Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> Acked-by: Pierre Ossman <drzeus@drzeus.cx> Signed-off-by: Liam Girdwood <lrg@slimlogic.co.uk> Pawel
On Mon, Mar 18, 2013 at 06:16:57PM +0000, Pawel Moll wrote: > On Mon, 2013-03-18 at 18:03 +0000, Russell King - ARM Linux wrote: > > > Without the regulator framework present there are no valid operating > > > voltages reported and the device initialisation fails: > > > > > > mmci-pl18x 1c050000.mmci: mmc0: PL180 manf 41 rev0 at 0x1c050000 irq 41,42 (pio) > > > mmc0: host doesn't support card's voltages > > > mmc0: error -22 whilst initialising MMC card > > > > Okay, this isn't going to work, because this means it's broken on a whole > > load of platforms. What it means is that this commit: > > > > commit 599c1d5c750ddf528c7c6d3cdc466708f0502e66 > > Author: Ulf Hansson <ulf.hansson@linaro.org> > > Date: Mon Jan 7 16:22:50 2013 +0100 > > > > ARM: 7620/1: mmc: mmci: Convert to use mmc_regulator_get_supply > > > > By using the mmc_regulator_get_supply API we are able to do some > > cleanups of the regulator code. Additionally let the regulator > > API handle the error printing. > > > > Cc: Chris Ball <cjb@laptop.org> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org> > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > > > is actually wrong and probably needs reverting. > > Reverting it will not change anything as the driver had the same > dependency before, just using : Umm. If CONFIG_REGULATOR is not set with the patch reverted, the regulator support is not used at all, and the ocrmask comes from the platform data.
On Monday 18 March 2013, Russell King - ARM Linux wrote: > On Mon, Mar 18, 2013 at 06:16:57PM +0000, Pawel Moll wrote: > > > > Reverting it will not change anything as the driver had the same > > dependency before, just using : > > Umm. If CONFIG_REGULATOR is not set with the patch reverted, the > regulator support is not used at all, and the ocrmask comes from > the platform data. I think the point is that case of DT booting, the platform data has a zero ocrmask, because Versatile Express has no auxdata to set a platform_data pointer for the mmci device, and the default allocated mmci device driver does not initialize the ocrmask unless the regulator API is used. This took me a while to figure out, the point being that this is not actually a regression, it never worked as far as I can tell. I don't think that selecting the regulator API is a good idea, for the reasons that you mentioned, and I still wouldn't take the patch. In theory we could do a workaround by adding a DT property to the mmci node that describes the fixed voltage supplied to the card if there are no regulators, but then again we already describe the voltage using the regulator binding and I would not want to duplicate that information. What I think we should have though is a better diagnostic here: If neither the platform data nor the DT contain a valid description of the voltage supplied to the card, the probe should fail saying that the information is missing, not that the card is incompatible. Arnd
On 18 March 2013 22:40, Arnd Bergmann <arnd@arndb.de> wrote: > On Monday 18 March 2013, Russell King - ARM Linux wrote: >> On Mon, Mar 18, 2013 at 06:16:57PM +0000, Pawel Moll wrote: >> > >> > Reverting it will not change anything as the driver had the same >> > dependency before, just using : >> >> Umm. If CONFIG_REGULATOR is not set with the patch reverted, the >> regulator support is not used at all, and the ocrmask comes from >> the platform data. > > I think the point is that case of DT booting, the platform data has > a zero ocrmask, because Versatile Express has no auxdata to set > a platform_data pointer for the mmci device, and the default allocated > mmci device driver does not initialize the ocrmask unless the > regulator API is used. > > This took me a while to figure out, the point being that this is not > actually a regression, it never worked as far as I can tell. > > I don't think that selecting the regulator API is a good idea, for > the reasons that you mentioned, and I still wouldn't take the > patch. In theory we could do a workaround by adding a DT property > to the mmci node that describes the fixed voltage supplied to the > card if there are no regulators, but then again we already describe > the voltage using the regulator binding and I would not want to > duplicate that information. > > What I think we should have though is a better diagnostic here: > If neither the platform data nor the DT contain a valid description > of the voltage supplied to the card, the probe should fail saying > that the information is missing, not that the card is incompatible. > > Arnd I agree Arnd. If mmci during probe, don't get a regulator and don't have an OCR mask from dt/platform data, probe should fail. Kind regards Ulf Hansson
diff --git a/arch/arm/mach-vexpress/Kconfig b/arch/arm/mach-vexpress/Kconfig index 52d315b..46ab88d 100644 --- a/arch/arm/mach-vexpress/Kconfig +++ b/arch/arm/mach-vexpress/Kconfig @@ -17,6 +17,7 @@ config ARCH_VEXPRESS select NO_IOPORT select PLAT_VERSATILE select PLAT_VERSATILE_CLCD + select REGULATOR if MMC_ARMMMCI select REGULATOR_FIXED_VOLTAGE if REGULATOR select VEXPRESS_CONFIG help
The MMCI driver, when used with a Device Tree described device, relies on the "vmmc" voltage regulator supply to set the OCR register voltage bits, using MMC core's mmc_regulator_get_supply() function. Without the regulator framework present there are no valid operating voltages reported and the device initialisation fails: mmci-pl18x 1c050000.mmci: mmc0: PL180 manf 41 rev0 at 0x1c050000 irq 41,42 (pio) mmc0: host doesn't support card's voltages mmc0: error -22 whilst initialising MMC card Signed-off-by: Pawel Moll <pawel.moll@arm.com> --- arch/arm/mach-vexpress/Kconfig | 1 + 1 file changed, 1 insertion(+)