Message ID | 1460474204-5351-3-git-send-email-peter.griffin@linaro.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, Apr 12, 2016 at 04:16:41PM +0100, Peter Griffin wrote: > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index 61bfbb9..10be29b 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -108,6 +108,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o > obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o > obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o > obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o > - > +obj-$(CONFIG_REGULATOR_ST_FLASHSS) += st-flashss.o Please keep Kconfig and Makefile sorted. :/ > +static int st_set_voltage_sel(struct regulator_dev *dev, unsigned int selector) This and the get_voltage_sel() look like you can just use a MMIO regmap. > + voltage = st_type_voltages[selector]; No bounds checking on the array, though since you're working with selectors it doesn't seem like there's any need to do this. > + value |= vsense->en_psw_mask; Enable should be a separate operation. > +static void st_get_satinize_powerup_voltage(struct st_vsense *vsense) > +{ > + void __iomem *ioaddr = vsense->ioaddr; > + u32 value = readl_relaxed(ioaddr); > + > + dev_dbg(vsense->dev, "Initial start-up value: (0x%08x)\n", value); > + > + /* Sanitize voltage values forcing what is provided from start-up */ > + if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_EMMC) > + value |= TOP_VSENSE_CONFIG_REG_PSW_EMMC; > + else > + value &= ~TOP_VSENSE_CONFIG_REG_PSW_EMMC; > + > + if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_NAND) > + value |= TOP_VSENSE_CONFIG_REG_PSW_NAND; > + else > + value &= ~TOP_VSENSE_CONFIG_REG_PSW_NAND; > + > + if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_SPI) > + value |= TOP_VSENSE_CONFIG_REG_PSW_SPI; > + else > + value &= ~TOP_VSENSE_CONFIG_REG_PSW_SPI; This looks like a very complicated way of writing value &= TOP_VSENSE_CONFIG_LATCHED_PSW_SPI | TOP_VSENSE_CONFIG_LATCHED_PSW_NAND | TOP_VSENSE_CONFIG_REG_PSW_EMMC or am I missing something? Why do we need to do this anyway, it's very surprsing? > + if (device->data) { > + const struct st_vsense *data = device->data; > + > + vsense->en_psw_mask = data->en_psw_mask; > + vsense->psw_mask = data->psw_mask; > + vsense->latched_mask = data->latched_mask; > + } else > + return -ENODEV; Coding style, { } on both sides. > + > + if (of_property_read_string(np, "regulator-name", > + (const char **)&vsense->name)) > + return -EINVAL; Don't make this mandatory, that's not what it's for. Use the compatible if you really need something, or just make the framework cope with a missing name (eg, by using dev_name() or something). > + config.init_data = of_get_regulator_init_data(&pdev->dev, np, rdesc); > + if (!config.init_data) { > + dev_err(dev, "Failed to parse regulator init data\n"); > + return -ENOMEM; > + } Don't error out, carry on and let the driver work in read only mode for diagnostics. > + /* register regulator */ > + rdev = regulator_register(rdesc, &config); devm_regulator_register(). > + dev_info(dev, "%s vsense voltage regulator registered\n", rdesc->name); Remove this, it's just making the boot noisy. > +static int __init st_vsense_init(void) > +{ > + return platform_driver_register(&st_vsense_driver); > +} > + > +subsys_initcall(st_vsense_init); module_platform_driver().
On 4/13/2016 8:15 AM, Mark Brown wrote: >> >+static void st_get_satinize_powerup_voltage(struct st_vsense *vsense) >> >+{ >> >+ void __iomem *ioaddr = vsense->ioaddr; >> >+ u32 value = readl_relaxed(ioaddr); >> >+ >> >+ dev_dbg(vsense->dev, "Initial start-up value: (0x%08x)\n", value); >> >+ >> >+ /* Sanitize voltage values forcing what is provided from start-up */ >> >+ if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_EMMC) >> >+ value |= TOP_VSENSE_CONFIG_REG_PSW_EMMC; >> >+ else >> >+ value &= ~TOP_VSENSE_CONFIG_REG_PSW_EMMC; >> >+ >> >+ if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_NAND) >> >+ value |= TOP_VSENSE_CONFIG_REG_PSW_NAND; >> >+ else >> >+ value &= ~TOP_VSENSE_CONFIG_REG_PSW_NAND; >> >+ >> >+ if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_SPI) >> >+ value |= TOP_VSENSE_CONFIG_REG_PSW_SPI; >> >+ else >> >+ value &= ~TOP_VSENSE_CONFIG_REG_PSW_SPI; > This looks like a very complicated way of writing > > value &= TOP_VSENSE_CONFIG_LATCHED_PSW_SPI | > TOP_VSENSE_CONFIG_LATCHED_PSW_NAND | > TOP_VSENSE_CONFIG_REG_PSW_EMMC > > or am I missing something? Why do we need to do this anyway, it's very > surprsing? > This functions is to sanitize the vsense voltages when the regulator is probed and in some circumstances the reset value of this register does not reflect the hw status/config. For example, by default, after the reset, the bit 0 is set so the EMMC, inside the flash subsystem, is supposed to operate at 3v3. But the latched bit 24 can be 0 on a platform where it is actually set at 1v8. So the bit 0 must be reset to keep this coherent and to allow MMC framework to properly setup the Vdd when the framework starts. Hoping this can help. Regards peppe
On Wed, Apr 13, 2016 at 09:59:00AM +0200, Giuseppe CAVALLARO wrote: > On 4/13/2016 8:15 AM, Mark Brown wrote: > >>>+static void st_get_satinize_powerup_voltage(struct st_vsense *vsense) > >>>+{ > >or am I missing something? Why do we need to do this anyway, it's very > >surprsing? > This functions is to sanitize the vsense voltages when the regulator > is probed and in some circumstances the reset value of this register > does not reflect the hw status/config. For example, by default, after > the reset, the bit 0 is set so the EMMC, inside the flash subsystem, > is supposed to operate at 3v3. But the latched bit 24 can be 0 on > a platform where it is actually set at 1v8. > So the bit 0 must be reset to keep this coherent and to allow MMC > framework to properly setup the Vdd when the framework starts. I'm afraid I can't follow that explanation, perhaps because I don't know anything about the content of this register except for these three bits. I think we do need a comment in the driver explaining what's going on, and probably a simplification of the code too if my understanding of the effect of all those operations is correct.
Hello Mark On 4/13/2016 7:23 PM, Mark Brown wrote: > On Wed, Apr 13, 2016 at 09:59:00AM +0200, Giuseppe CAVALLARO wrote: >> On 4/13/2016 8:15 AM, Mark Brown wrote: >>>>> +static void st_get_satinize_powerup_voltage(struct st_vsense *vsense) >>>>> +{ > >>> or am I missing something? Why do we need to do this anyway, it's very >>> surprsing? > >> This functions is to sanitize the vsense voltages when the regulator >> is probed and in some circumstances the reset value of this register >> does not reflect the hw status/config. For example, by default, after >> the reset, the bit 0 is set so the EMMC, inside the flash subsystem, >> is supposed to operate at 3v3. But the latched bit 24 can be 0 on >> a platform where it is actually set at 1v8. >> So the bit 0 must be reset to keep this coherent and to allow MMC >> framework to properly setup the Vdd when the framework starts. > > I'm afraid I can't follow that explanation, perhaps because I don't know > anything about the content of this register except for these three bits. > I think we do need a comment in the driver explaining what's going on, yes you are right > and probably a simplification of the code too if my understanding of the > effect of all those operations is correct. Maybe we can simplify the function as: static void st_get_satinize_powerup_voltage(struct st_vsense *vsense) { void __iomem *ioaddr = vsense->ioaddr; u32 value = readl_relaxed(ioaddr); /* * After resetting, the CONFIG_REG_PSW_<xxx> are set, this * means 3v3 operating voltage. * The CONFIG_LATCHED_PSW_<xxx> must be used to fix the previous * bits so operating at 1v8 if this is the real HW configuration * at boot time. */ if (!(value & TOP_VSENSE_CONFIG_LATCHED_PSW_EMMC)) value &= ~TOP_VSENSE_CONFIG_REG_PSW_EMMC; if (!(value & TOP_VSENSE_CONFIG_LATCHED_PSW_NAND)) value &= ~TOP_VSENSE_CONFIG_REG_PSW_NAND; if (!(value & TOP_VSENSE_CONFIG_LATCHED_PSW_SPI)) value &= ~TOP_VSENSE_CONFIG_REG_PSW_SPI; writel_relaxed(value, ioaddr); } Le me know if this looks a bit more clear. Peppe
On Thu, Apr 14, 2016 at 04:15:34PM +0200, Giuseppe CAVALLARO wrote: > On 4/13/2016 7:23 PM, Mark Brown wrote: > /* > * After resetting, the CONFIG_REG_PSW_<xxx> are set, this > * means 3v3 operating voltage. > * The CONFIG_LATCHED_PSW_<xxx> must be used to fix the previous > * bits so operating at 1v8 if this is the real HW configuration > * at boot time. > */ > if (!(value & TOP_VSENSE_CONFIG_LATCHED_PSW_EMMC)) > value &= ~TOP_VSENSE_CONFIG_REG_PSW_EMMC; That's definitely better.
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index c77dc08..ddf2bcd 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -843,5 +843,12 @@ config REGULATOR_WM8994 This driver provides support for the voltage regulators on the WM8994 CODEC. +config REGULATOR_ST_FLASHSS + tristate "STMicroelectronics FlashSS regulator" + depends on ARCH_STI && OF + help + This driver provides support for the voltage regulators available + inside the FlashSS of STiH407/STiH410 SoC's. + endif diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 61bfbb9..10be29b 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -108,6 +108,6 @@ obj-$(CONFIG_REGULATOR_WM831X) += wm831x-ldo.o obj-$(CONFIG_REGULATOR_WM8350) += wm8350-regulator.o obj-$(CONFIG_REGULATOR_WM8400) += wm8400-regulator.o obj-$(CONFIG_REGULATOR_WM8994) += wm8994-regulator.o - +obj-$(CONFIG_REGULATOR_ST_FLASHSS) += st-flashss.o ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG diff --git a/drivers/regulator/st-flashss.c b/drivers/regulator/st-flashss.c new file mode 100644 index 0000000..9c0e011 --- /dev/null +++ b/drivers/regulator/st-flashss.c @@ -0,0 +1,274 @@ +/* + * ST regulator driver for flashSS vsense + * + * This is a small driver to manage the voltage regulator inside the ST flash + * sub-system that is used for configuring MMC, NAND, SPI voltages. + * + * Copyright(C) 2015 STMicroelectronics Ltd + * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. + */ +#include <linux/module.h> +#include <linux/io.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/machine.h> +#include <linux/regulator/of_regulator.h> + +/* FlashSS voltage VSENSE TOP CONFIG register defines */ +#define TOP_VSENSE_CONFIG_REG_PSW_EMMC BIT(0) +#define TOP_VSENSE_CONFIG_ENB_REG_PSW_EMMC BIT(1) +#define TOP_VSENSE_CONFIG_REG_PSW_NAND BIT(8) +#define TOP_VSENSE_CONFIG_ENB_REG_PSW_NAND BIT(9) +#define TOP_VSENSE_CONFIG_REG_PSW_SPI BIT(16) +#define TOP_VSENSE_CONFIG_ENB_REG_PSW_SPI BIT(17) +#define TOP_VSENSE_CONFIG_LATCHED_PSW_EMMC BIT(24) +#define TOP_VSENSE_CONFIG_LATCHED_PSW_NAND BIT(25) +#define TOP_VSENSE_CONFIG_LATCHED_PSW_SPI BIT(26) + +struct st_vsense { + char *name; + struct device *dev; + u8 n_voltages; /* number of supported voltages */ + void __iomem *ioaddr; /* TOP config base address */ + unsigned int en_psw_mask; /* Mask/enable vdd for each device */ + unsigned int psw_mask; /* Power sel mask for VDD */ + unsigned int latched_mask; /* Latched mask for VDD */ +}; + +static const unsigned int st_type_voltages[] = { + 1800000, + 3300000, +}; + +const struct st_vsense st_vsense_data[] = { + { + .en_psw_mask = TOP_VSENSE_CONFIG_ENB_REG_PSW_EMMC, + .psw_mask = TOP_VSENSE_CONFIG_REG_PSW_EMMC, + .latched_mask = TOP_VSENSE_CONFIG_LATCHED_PSW_EMMC, + }, { + .en_psw_mask = TOP_VSENSE_CONFIG_ENB_REG_PSW_NAND, + .psw_mask = TOP_VSENSE_CONFIG_REG_PSW_NAND, + .latched_mask = TOP_VSENSE_CONFIG_LATCHED_PSW_NAND, + }, { + .en_psw_mask = TOP_VSENSE_CONFIG_ENB_REG_PSW_SPI, + .psw_mask = TOP_VSENSE_CONFIG_REG_PSW_SPI, + .latched_mask = TOP_VSENSE_CONFIG_LATCHED_PSW_SPI, + }, +}; + +/* Get the value of Sensed-PSW of eMMC/NAND/SPI Pads */ +static int st_get_voltage_sel(struct regulator_dev *dev) +{ + struct st_vsense *vsense = rdev_get_drvdata(dev); + void __iomem *ioaddr = vsense->ioaddr; + int sel = 0; + u32 value = readl_relaxed(ioaddr); + + if (value & vsense->latched_mask) + sel = 1; + + dev_dbg(vsense->dev, "%s, selection %d (0x%08x)\n", vsense->name, sel, + readl_relaxed(ioaddr)); + + return sel; +} + +static int st_set_voltage_sel(struct regulator_dev *dev, unsigned int selector) +{ + struct st_vsense *vsense = rdev_get_drvdata(dev); + void __iomem *ioaddr = vsense->ioaddr; + unsigned int value = readl_relaxed(ioaddr); + unsigned int voltage; + + voltage = st_type_voltages[selector]; + + value |= vsense->en_psw_mask; + if (voltage == 3300000) + value |= vsense->psw_mask; + else + value &= ~vsense->psw_mask; + + writel_relaxed(value, ioaddr); + + dev_dbg(vsense->dev, "%s, required voltage %d (vsense_conf 0x%08x)\n", + vsense->name, voltage, + readl_relaxed(ioaddr)); + + return 0; +} + +static struct regulator_ops st_ops = { + .get_voltage_sel = st_get_voltage_sel, + .set_voltage_sel = st_set_voltage_sel, + .list_voltage = regulator_list_voltage_table, +}; + +static const struct of_device_id of_st_match_tbl[] = { + {.compatible = "st,vqmmc", .data = &st_vsense_data[0]}, + {.compatible = "st,vnand", .data = &st_vsense_data[1]}, + {.compatible = "st,vspi", .data = &st_vsense_data[2]}, + { /* end */ } +}; + +static void st_get_satinize_powerup_voltage(struct st_vsense *vsense) +{ + void __iomem *ioaddr = vsense->ioaddr; + u32 value = readl_relaxed(ioaddr); + + dev_dbg(vsense->dev, "Initial start-up value: (0x%08x)\n", value); + + /* Sanitize voltage values forcing what is provided from start-up */ + if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_EMMC) + value |= TOP_VSENSE_CONFIG_REG_PSW_EMMC; + else + value &= ~TOP_VSENSE_CONFIG_REG_PSW_EMMC; + + if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_NAND) + value |= TOP_VSENSE_CONFIG_REG_PSW_NAND; + else + value &= ~TOP_VSENSE_CONFIG_REG_PSW_NAND; + + if (value & TOP_VSENSE_CONFIG_LATCHED_PSW_SPI) + value |= TOP_VSENSE_CONFIG_REG_PSW_SPI; + else + value &= ~TOP_VSENSE_CONFIG_REG_PSW_SPI; + + writel_relaxed(value, ioaddr); + + dev_dbg(vsense->dev, "Sanitized value: (0x%08x)\n", value); +} + +static int st_vsense_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct st_vsense *vsense; + const struct of_device_id *device; + struct resource *res; + struct regulator_desc *rdesc; + struct regulator_dev *rdev; + struct regulator_config config = { }; + + vsense = devm_kzalloc(dev, sizeof(*vsense), GFP_KERNEL); + if (!vsense) + return -ENOMEM; + + vsense->dev = dev; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + vsense->ioaddr = devm_ioremap_resource(dev, res); + if (IS_ERR(vsense->ioaddr)) + return PTR_ERR(vsense->ioaddr); + + rdesc = devm_kzalloc(dev, sizeof(*rdesc), GFP_KERNEL); + if (!rdesc) + return -ENOMEM; + + device = of_match_device(of_st_match_tbl, &pdev->dev); + if (!device) + return -ENODEV; + + if (device->data) { + const struct st_vsense *data = device->data; + + vsense->en_psw_mask = data->en_psw_mask; + vsense->psw_mask = data->psw_mask; + vsense->latched_mask = data->latched_mask; + } else + return -ENODEV; + + if (of_property_read_string(np, "regulator-name", + (const char **)&vsense->name)) + return -EINVAL; + + memset(rdesc, 0, sizeof(*rdesc)); + rdesc->name = vsense->name; + rdesc->ops = &st_ops; + rdesc->type = REGULATOR_VOLTAGE; + rdesc->owner = THIS_MODULE; + rdesc->n_voltages = ARRAY_SIZE(st_type_voltages); + rdesc->volt_table = st_type_voltages; + config.dev = &pdev->dev; + config.driver_data = vsense; + config.of_node = np; + + config.init_data = of_get_regulator_init_data(&pdev->dev, np, rdesc); + if (!config.init_data) { + dev_err(dev, "Failed to parse regulator init data\n"); + return -ENOMEM; + } + + /* register regulator */ + rdev = regulator_register(rdesc, &config); + if (IS_ERR(rdev)) { + dev_err(dev, "failed to register %s\n", rdesc->name); + return PTR_ERR(rdev); + } + + platform_set_drvdata(pdev, rdev); + + dev_info(dev, "%s vsense voltage regulator registered\n", rdesc->name); + st_get_satinize_powerup_voltage(vsense); + + return 0; +} + +static int st_vsense_remove(struct platform_device *pdev) +{ + struct regulator_dev *rdev = platform_get_drvdata(pdev); + + regulator_unregister(rdev); + + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int st_vsense_resume(struct device *dev) +{ + struct regulator_dev *rdev = dev_get_drvdata(dev); + struct st_vsense *vsense = rdev_get_drvdata(rdev); + + st_get_satinize_powerup_voltage(vsense); + + return 0; +} +#endif + +static const struct dev_pm_ops st_vsense_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(NULL, st_vsense_resume) +}; + +static struct platform_driver st_vsense_driver = { + .driver = { + .name = "st-vsense", + .of_match_table = of_st_match_tbl, + .pm = &st_vsense_pm_ops, + }, + .probe = st_vsense_probe, + .remove = st_vsense_remove, +}; + +static int __init st_vsense_init(void) +{ + return platform_driver_register(&st_vsense_driver); +} + +subsys_initcall(st_vsense_init); + +static void __exit st_vsense_cleanup(void) +{ + platform_driver_unregister(&st_vsense_driver); +} + +module_exit(st_vsense_cleanup); + +MODULE_AUTHOR("Giuseppe Cavallaro <peppe.cavallaro@st.com>"); +MODULE_DESCRIPTION("ST voltage regulator driver for vsense flashSS"); +MODULE_LICENSE("GPL v2");