Functionality add and bug-fixes related to MMCI regulators

The normal course of action would be to defer the MMCI driver too, but these IOS level-shifter regulators aren't present on all platforms, so deferring until one is found would be incorrect.

So the best solution is to demote the GPIO Regulator driver, so it starts later than the TC35892 GPIO controller, which is also configured to start at subsys_initcall() time, but before deferred probing time, which starts at late_initcall(), after many of the drivers requiring these regulators would have already started.

Quoted 9 lines · Lee Jones

The normal course of action would be to defer the MMCI driver too, but these IOS level-shifter regulators aren't present on all platforms, so deferring until one is found would be incorrect.

So the best solution is to demote the GPIO Regulator driver, so it starts later than the TC35892 GPIO controller, which is also configured to start at subsys_initcall() time, but before deferred probing time, which starts at late_initcall(), after many of the drivers requiring these regulators would have already started.

This really isn't a good solution, especially not for a system that's DT based - on a DT system we can tell if there should be a GPIO present so we should be able to defer only when there's something that might provide the GPIO later on.

Quoted 4 lines · Mark Brown

This really isn't a good solution, especially not for a system that's DT based - on a DT system we can tell if there should be a GPIO present so we should be able to defer only when there's something that might provide the GPIO later on.

Understood, but what's the solution for non-DT systems?

Quoted 8 lines · Lee Jones

Quoted 4 lines · Mark Brown

This really isn't a good solution, especially not for a system that's DT based - on a DT system we can tell if there should be a GPIO present so we should be able to defer only when there's something that might provide the GPIO later on.

Understood, but what's the solution for non-DT systems?

Provide a fixed regulator or something, perhaps we need a "definitely does not exist" regulator to help with this. For every board you help with a sequencing bodge you're probably going to break another that needs different sequencing; for that matter it's not like GPIO controlled regulators are exclusively used for MMC, or that MMC exclusively uses GPIO - doing this for only one regulator is a bit of a red flag.

I understand your logic, hence why I wrote such a lengthy commit message. However, I'm not sure I see a logical way around it. Asking all users of MMCI to provide a not-regulator to declare that a secondary regulator isn't available seems a little unreasonable to me.

Is there anything else we can do?

If a regulator is specified as always-on, then it can't have an enable/disable pin, as it can't be turned off.

Quoted 2 lines · Lee Jones

If a regulator is specified as always-on, then it can't have an enable/disable pin, as it can't be turned off.

Sometimes always on gets set for regulators which do have a physical control wired up - the control might exist for use in suspend mode for example. Is the ability to specify an enable pin causing a practical problem for systems? If it is we should fix that.

Quoted 8 lines · Mark Brown

Quoted 2 lines · Lee Jones

If a regulator is specified as always-on, then it can't have an enable/disable pin, as it can't be turned off.

Sometimes always on gets set for regulators which do have a physical control wired up - the control might exist for use in suspend mode for example. Is the ability to specify an enable pin causing a practical problem for systems? If it is we should fix that.

My logic is that there is no point in requesting a pin which can disable a regulator that can't be disabled. Then we can follow on from that logic and say that if a regulator is _not_ always on this we _require_ a way to disable it, thus we insist on an enable GPIO pin.

With me?

The cond-statement of this particular for() loop will always be true as long as at least one voltage-shifting GPIO is present. If it wasn't for the break below, we'd be stuck in a forever loop. This patch inserts the correct cond-statement into the statement.

If it's possible for gpio_set_value to sleep, we should be using the *_cansleep call instead. This patch fixes multiple warnings from gpiolib.

The voltages for the MMCI GPIO controlled voltage regulation differ depending on who you believe and where you pick the information up from. More recent internal code suggests that the true upper voltage from the shifter is actually v3.3, instead of the previously indicated v2.9. Let's mirror that change in the Device Tree file.

Signed-off-by Lee Jones <[email protected]>

Quoted 5 lines · Lee Jones

The voltages for the MMCI GPIO controlled voltage regulation differ depending on who you believe and where you pick the information up from. More recent internal code suggests that the true upper voltage from the shifter is actually v3.3, instead of the previously indicated v2.9. Let's mirror that change in the Device Tree file.

Quoted 2 lines

Signed-off-by Lee Jones <[email protected]>

Acked-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Quoted 7 lines · Linus Walleij

Quoted 5 lines · Lee Jones

The voltages for the MMCI GPIO controlled voltage regulation differ depending on who you believe and where you pick the information up from. More recent internal code suggests that the true upper voltage from the shifter is actually v3.3, instead of the previously indicated v2.9. Let's mirror that change in the Device Tree file.

This is wrong. The value should be 2.9V nothing else!

Quoted 6 lines

Quoted 1 line

Signed-off-by Lee Jones <[email protected]>

Acked-by Linus Walleij <[email protected]>

Yours,
Linus Walleij

Kind regards
Ulf Hansson

If not specified, the GPIO control bit is inverted by default i.e. low-enable and high-disable. This is not the case with the MMCI regulator, hence it will turn on during a disable and off when regulator_enable() is invoked.

Signed-off-by Lee Jones <[email protected]>

In an effort to move platform specific GPIO controlled regulators out from platform code we've created a new mechanism to specify them from within the MMCI driver using the supply name 'vmmc-ios'. For that to happen when booting device tree, we need to supply it in the MMCI (SDI) node.

Signed-off-by Lee Jones <[email protected]>

Correct a typo in the Device Tree source file, where instead of specifying property 'enable-gpio', which the driver is expecting we specified 'gpio-enable' instead.

Signed-off-by Lee Jones <[email protected]>

The GPIO controlled MMCI regulator used on the ux5x0 boards takes 100us to settle. There's already a binding to provide such information. Let's make use of it.

Signed-off-by Lee Jones <[email protected]>

To prevent lots of unnecessary call-backs into platform code, we're now using the GPIO regulator framework to control the 'enable' (en) and 'voltage select' (vsel) GPIO pins which in turn control the MMCI's secondary regulator settings. This already works with Device Tree, but when booting with ATAGs we need to register it as a platform device.

Signed-off-by Lee Jones <[email protected]>

---
 arch/arm/mach-ux500/board-mop500.c |   61 ++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

arch/arm/mach-ux500/board-mop500.c +2 -0

index daa4237..9f9fe7f 100644
@@ -24,6 +24,8 @@
 #include <linux/mfd/abx500/ab8500.h>
 #include <linux/regulator/ab8500.h>
 #include <linux/regulator/fixed.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/gpio-regulator.h>
 #include <linux/mfd/tc3589x.h>
 #include <linux/mfd/tps6105x.h>
 #include <linux/mfd/abx500/ab8500-gpio.h>

arch/arm/mach-ux500/board-mop500.c +4 -0

+                .max_uV         = 3300000,
+                .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|REGULATOR_CHANGE_STATUS,
+        },
+        .num_consumer_supplies  = ARRAY_SIZE(sdi0_reg_consumers),

arch/arm/mach-ux500/board-mop500.c +38 -1

+        { .value = 1800000, .gpios = (1 << 0) },
+};
+
+static struct gpio_regulator_config sdi0_reg_info = {
+        .supply_name = "mmci-reg",
+
+        .enable_high = 1,
+        .enabled_at_boot = 0,
+
+        .gpios = sdi0_reg_gpios,
+        .nr_gpios = ARRAY_SIZE(sdi0_reg_gpios),
+
+        .states = sdi0_reg_states,
+        .nr_states = ARRAY_SIZE(sdi0_reg_states),
+
+        .type = REGULATOR_VOLTAGE,
+        .init_data = &sdi0_reg_init_data,
+};
+
+static struct platform_device sdi0_regulator = {
+        .name = "gpio-regulator",
+        .id   = -1,
+        .dev  = {
+                .platform_data = &sdi0_reg_info,
+        },
+};
+
 static struct ab8500_gpio_platform_data ab8500_gpio_pdata = {
        .gpio_base              = MOP500_AB8500_PIN_GPIO(1),
        .irq_base               = MOP500_AB8500_VIR_GPIO_IRQ_BASE,

@@ -440,6 +490,7 @@ static struct hash_platform_data u8500_hash1_platform_data = {
 /* add any platform devices here - TODO */
 static struct platform_device *mop500_platform_devs[] __initdata = {
        &mop500_gpio_keys_device,
+       &sdi0_regulator,
 };
 
 #ifdef CONFIG_STE_DMA40

@@ -581,6 +632,7 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
        &snowball_key_dev,
        &snowball_sbnet_dev,
        &snowball_gpio_en_3v3_regulator_dev,
+       &sdi0_regulator,
 };
 
 static void __init mop500_init_machine(void)

@@ -591,6 +643,9 @@ static void __init mop500_init_machine(void)
 
        mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
 
+       sdi0_reg_info.enable_gpio = GPIO_SDMMC_EN;
+       sdi0_reg_info.gpios[0].gpio = GPIO_SDMMC_1V8_3V_SEL;
+
        mop500_pinmaps_init();
        parent = u8500_init_devices(&ab8500_platdata);
 

@@ -623,6 +678,9 @@ static void __init snowball_init_machine(void)
        struct device *parent = NULL;
        int i;
 
+       sdi0_reg_info.enable_gpio = SNOWBALL_SDMMC_EN_GPIO;
+       sdi0_reg_info.gpios[0].gpio = SNOWBALL_SDMMC_1V8_3V_GPIO;
+
        snowball_pinmaps_init();
        parent = u8500_init_devices(&ab8500_platdata);
 

@@ -655,6 +713,9 @@ static void __init hrefv60_init_machine(void)
         */
        mop500_gpio_keys[0].gpio = HREFV60_PROX_SENSE_GPIO;
 
+       sdi0_reg_info.enable_gpio = HREFV60_SDMMC_EN_GPIO;
+       sdi0_reg_info.gpios[0].gpio = HREFV60_SDMMC_1V8_3V_GPIO;
+
        hrefv60_pinmaps_init();
        parent = u8500_init_devices(&ab8500_platdata);
 
--

1.7.9.5

Quoted 26 lines · Lee Jones

To prevent lots of unnecessary call-backs into platform code, we're now using the GPIO regulator framework to control the 'enable' (en) and 'voltage select' (vsel) GPIO pins which in turn control the MMCI's secondary regulator settings. This already works with Device Tree, but when booting with ATAGs we need to register it as a platform device.

Signed-off-by Lee Jones <[email protected]>

---
 arch/arm/mach-ux500/board-mop500.c |   61 ++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

arch/arm/mach-ux500/board-mop500.c +2 -0

index daa4237..9f9fe7f 100644
@@ -24,6 +24,8 @@
 #include <linux/mfd/abx500/ab8500.h>
 #include <linux/regulator/ab8500.h>
 #include <linux/regulator/fixed.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/gpio-regulator.h>
 #include <linux/mfd/tc3589x.h>
 #include <linux/mfd/tps6105x.h>
 #include <linux/mfd/abx500/ab8500-gpio.h>

arch/arm/mach-ux500/board-mop500.c static struct platform_device snowball_gpio_en_3v3_regulator_dev = { · line 97

Quoted 5 lines

arch/arm/mach-ux500/board-mop500.c +2 -0

@@ -91,6 +93,54 @@ static struct platform_device snowball_gpio_en_3v3_regulator_dev = {
        },
 };
 
+static struct regulator_consumer_supply sdi0_reg_consumers[] = {
+        REGULATOR_SUPPLY("vqmmc", NULL),

REGULATOR_SUPPLY("vqmmc", "sdi0"),

arch/arm/mach-ux500/board-mop500.c static struct platform_device snowball_gpio_en_3v3_regulator_dev = { · line 100

Quoted 3 lines

arch/arm/mach-ux500/board-mop500.c +3 -0

+};
+
+static struct regulator_init_data sdi0_reg_init_data = {

The levelshifter uses the ab8500-ext-supply3. Reflect that here.

.supply_regulator = "ab8500-ext-supply3"

arch/arm/mach-ux500/board-mop500.c static struct platform_device snowball_gpio_en_3v3_regulator_dev = { · line 102

Quoted 2 lines

arch/arm/mach-ux500/board-mop500.c +2 -0

+        .constraints = {
+                .min_uV         = 1800000,

2.9V, not 3.3V

Quoted 4 lines

arch/arm/mach-ux500/board-mop500.c +4 -0

+                .max_uV         = 3300000,
+                .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|REGULATOR_CHANGE_STATUS,
+        },
+        .num_consumer_supplies  = ARRAY_SIZE(sdi0_reg_consumers),

arch/arm/mach-ux500/board-mop500.c static struct platform_device snowball_gpio_en_3v3_regulator_dev = { · line 116

Quoted 10 lines

arch/arm/mach-ux500/board-mop500.c +10 -0

+        .consumer_supplies      = sdi0_reg_consumers,
+};
+
+/* Dynamically populated. */
+static struct gpio sdi0_reg_gpios[] = {
+        { 0, GPIOF_OUT_INIT_LOW, "mmci_vsel" },
+};
+
+static struct gpio_regulator_state sdi0_reg_states[] = {
+        { .value = 3300000, .gpios = (0 << 0) },

2.9V, not 3.3V

arch/arm/mach-ux500/board-mop500.c static struct platform_device snowball_gpio_en_3v3_regulator_dev = { · line 128

Quoted 12 lines

arch/arm/mach-ux500/board-mop500.c +12 -0

+        { .value = 1800000, .gpios = (1 << 0) },
+};
+
+static struct gpio_regulator_config sdi0_reg_info = {
+        .supply_name = "mmci-reg",
+
+        .enable_high = 1,
+        .enabled_at_boot = 0,
+
+        .gpios = sdi0_reg_gpios,
+        .nr_gpios = ARRAY_SIZE(sdi0_reg_gpios),
+

Quoted 65 lines

arch/arm/mach-ux500/board-mop500.c +26 -1

+        .states = sdi0_reg_states,
+        .nr_states = ARRAY_SIZE(sdi0_reg_states),
+
+        .type = REGULATOR_VOLTAGE,
+        .init_data = &sdi0_reg_init_data,
+};
+
+static struct platform_device sdi0_regulator = {
+        .name = "gpio-regulator",
+        .id   = -1,
+        .dev  = {
+                .platform_data = &sdi0_reg_info,
+        },
+};
+
 static struct ab8500_gpio_platform_data ab8500_gpio_pdata = {
        .gpio_base              = MOP500_AB8500_PIN_GPIO(1),
        .irq_base               = MOP500_AB8500_VIR_GPIO_IRQ_BASE,

@@ -440,6 +490,7 @@ static struct hash_platform_data u8500_hash1_platform_data = {
 /* add any platform devices here - TODO */
 static struct platform_device *mop500_platform_devs[] __initdata = {
        &mop500_gpio_keys_device,
+       &sdi0_regulator,
 };
 
 #ifdef CONFIG_STE_DMA40

@@ -581,6 +632,7 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
        &snowball_key_dev,
        &snowball_sbnet_dev,
        &snowball_gpio_en_3v3_regulator_dev,
+       &sdi0_regulator,
 };
 
 static void __init mop500_init_machine(void)

@@ -591,6 +643,9 @@ static void __init mop500_init_machine(void)
 
        mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
 
+       sdi0_reg_info.enable_gpio = GPIO_SDMMC_EN;
+       sdi0_reg_info.gpios[0].gpio = GPIO_SDMMC_1V8_3V_SEL;
+
        mop500_pinmaps_init();
        parent = u8500_init_devices(&ab8500_platdata);
 

@@ -623,6 +678,9 @@ static void __init snowball_init_machine(void)
        struct device *parent = NULL;
        int i;
 
+       sdi0_reg_info.enable_gpio = SNOWBALL_SDMMC_EN_GPIO;
+       sdi0_reg_info.gpios[0].gpio = SNOWBALL_SDMMC_1V8_3V_GPIO;
+
        snowball_pinmaps_init();
        parent = u8500_init_devices(&ab8500_platdata);
 

@@ -655,6 +713,9 @@ static void __init hrefv60_init_machine(void)
         */
        mop500_gpio_keys[0].gpio = HREFV60_PROX_SENSE_GPIO;
 
+       sdi0_reg_info.enable_gpio = HREFV60_SDMMC_EN_GPIO;
+       sdi0_reg_info.gpios[0].gpio = HREFV60_SDMMC_1V8_3V_GPIO;
+
        hrefv60_pinmaps_init();
        parent = u8500_init_devices(&ab8500_platdata);
 
--

Quoted 2 lines

1.7.9.5

Finally, I suppose regulator inits i located in board-mop500-regulators.c, so I would suggest to move most of this code in there instead.

Kind regards
Ulf Hansson

arch/arm/mach-ux500/board-mop500.c static struct platform_device snowball_gpio_en_3v3_regulator_dev = { · line 102

Quoted 2 lines

arch/arm/mach-ux500/board-mop500.c +2 -0

+        .constraints = {
+                .min_uV         = 1800000,

2.9V, not 3.3V

arch/arm/mach-ux500/board-mop500.c static struct platform_device snowball_gpio_en_3v3_regulator_dev = { · line 116

Quoted 2 lines

arch/arm/mach-ux500/board-mop500.c +2 -0

+static struct gpio_regulator_state sdi0_reg_states[] = {
+        { .value = 3300000, .gpios = (0 << 0) },

2.9V, not 3.3V

Quoted 8 lines

arch/arm/mach-ux500/board-mop500.c static struct platform_device snowball_gpio_en_3v3_regulator_dev = { · line 102

Quoted 2 lines

arch/arm/mach-ux500/board-mop500.c +2 -0

+        .constraints = {
+                .min_uV         = 1800000,

2.9V, not 3.3V

Quoted 2 lines

arch/arm/mach-ux500/board-mop500.c +2 -0

+static struct gpio_regulator_state sdi0_reg_states[] = {
+        { .value = 3300000, .gpios = (0 << 0) },

Quoted 1 line

2.9V, not 3.3V

I'm still a little unsure about this. I know the actual voltage is v2.9, but the supported/requested MMC voltage from the driver is v3.3.

Will it still work if I set it all up as v2.9?

--
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog

To prevent lots of unnecessary call-backs into platform code, we're now using the GPIO regulator framework to control the 'enable' (en) and 'voltage select' (vsel) GPIO pins which in turn control the MMCI's secondary regulator settings. This already works with Device Tree, but when booting with ATAGs we need to register it as a platform device.

Signed-off-by Lee Jones <[email protected]>

---
 arch/arm/mach-ux500/board-mop500-regulators.c |   14 ++++++++
 arch/arm/mach-ux500/board-mop500-regulators.h |    1 +
 arch/arm/mach-ux500/board-mop500.c            |   44 +++++++++++++++++++++++++
 3 files changed, 59 insertions(+)

arch/arm/mach-ux500/board-mop500-regulators.c +0 -0

index 2a17bc5..cb75405 100644

arch/arm/mach-ux500/board-mop500-regulators.c +9 -0

+        .constraints = {
+                .min_uV         = 1800000,
+                .max_uV         = 2900000,
+                .valid_ops_mask = REGULATOR_CHANGE_VOLTAGE|REGULATOR_CHANGE_STATUS,
+        },
+        .num_consumer_supplies  = ARRAY_SIZE(sdi0_reg_consumers),
+        .consumer_supplies      = sdi0_reg_consumers,
+};
+
 /*
  * TPS61052 regulator
  */

arch/arm/mach-ux500/board-mop500-regulators.h +1 -0

index 78a0642..0c79d90 100644
@@ -19,5 +19,6 @@ ab8500_regulator_reg_init[AB8500_NUM_REGULATOR_REGISTERS];
 extern struct regulator_init_data ab8500_regulators[AB8500_NUM_REGULATORS];
 extern struct regulator_init_data tps61052_regulator;
 extern struct regulator_init_data gpio_en_3v3_regulator;
+extern struct regulator_init_data sdi0_reg_init_data;
 
 #endif

arch/arm/mach-ux500/board-mop500.c +4 -0

index daa4237..1d721ea 100644
@@ -24,6 +24,8 @@
 #include <linux/mfd/abx500/ab8500.h>
 #include <linux/regulator/ab8500.h>
 #include <linux/regulator/fixed.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/gpio-regulator.h>
 #include <linux/mfd/tc3589x.h>
 #include <linux/mfd/tps6105x.h>
 #include <linux/mfd/abx500/ab8500-gpio.h>

@@ -91,6 +93,37 @@ static struct platform_device snowball_gpio_en_3v3_regulator_dev = {
        },
 };
 
+/* Dynamically populated. */
+static struct gpio sdi0_reg_gpios[] = {t

arch/arm/mach-ux500/board-mop500.c +30 -1

+       .gpios                 = sdi0_reg_gpios,
+       .nr_gpios              = ARRAY_SIZE(sdi0_reg_gpios),
+       .states                = sdi0_reg_states,
+       .nr_states             = ARRAY_SIZE(sdi0_reg_states),
+       .type                  = REGULATOR_VOLTAGE,
+       .enable_high           = 1,
+       .enabled_at_boot       = 0,
+       .init_data             = &sdi0_reg_init_data,
+       .startup_delay         = 100,
+};
+
+static struct platform_device sdi0_regulator = {
+        .name = "gpio-regulator",
+        .id   = -1,
+        .dev  = {
+                .platform_data = &sdi0_reg_info,
+        },
+};
+
 static struct ab8500_gpio_platform_data ab8500_gpio_pdata = {
        .gpio_base              = MOP500_AB8500_PIN_GPIO(1),
        .irq_base               = MOP500_AB8500_VIR_GPIO_IRQ_BASE,

@@ -440,6 +473,7 @@ static struct hash_platform_data u8500_hash1_platform_data = {
 /* add any platform devices here - TODO */
 static struct platform_device *mop500_platform_devs[] __initdata = {
        &mop500_gpio_keys_device,
+       &sdi0_regulator,
 };
 
 #ifdef CONFIG_STE_DMA40

@@ -581,6 +615,7 @@ static struct platform_device *snowball_platform_devs[] __initdata = {
        &snowball_key_dev,
        &snowball_sbnet_dev,
        &snowball_gpio_en_3v3_regulator_dev,
+       &sdi0_regulator,
 };
 
 static void __init mop500_init_machine(void)

@@ -591,6 +626,9 @@ static void __init mop500_init_machine(void)
 
        mop500_gpio_keys[0].gpio = GPIO_PROX_SENSOR;
 
+       sdi0_reg_info.enable_gpio = GPIO_SDMMC_EN;
+       sdi0_reg_info.gpios[0].gpio = GPIO_SDMMC_1V8_3V_SEL;
+
        mop500_pinmaps_init();
        parent = u8500_init_devices(&ab8500_platdata);
 

@@ -623,6 +661,9 @@ static void __init snowball_init_machine(void)
        struct device *parent = NULL;
        int i;
 
+       sdi0_reg_info.enable_gpio = SNOWBALL_SDMMC_EN_GPIO;
+       sdi0_reg_info.gpios[0].gpio = SNOWBALL_SDMMC_1V8_3V_GPIO;
+
        snowball_pinmaps_init();
        parent = u8500_init_devices(&ab8500_platdata);
 

@@ -655,6 +696,9 @@ static void __init hrefv60_init_machine(void)
         */
        mop500_gpio_keys[0].gpio = HREFV60_PROX_SENSE_GPIO;
 
+       sdi0_reg_info.enable_gpio = HREFV60_SDMMC_EN_GPIO;
+       sdi0_reg_info.gpios[0].gpio = HREFV60_SDMMC_1V8_3V_GPIO;
+
        hrefv60_pinmaps_init();
        parent = u8500_init_devices(&ab8500_platdata);
 
--

1.7.9.5

Now MMCI on/off functionality is using the regulator framework from the MMCI driver, there is no need to keep the ios_handler laying around, duplicating functionality. So we're removing it.

Signed-off-by Lee Jones <[email protected]>