On 6/22/19 1:05 PM, Jonathan Cameron wrote: > On Wed, 19 Jun 2019 14:38:42 +0200 > Fabrice Gasnier <[email protected]> wrote: > >> On 6/17/19 2:43 PM, Fabrice Gasnier wrote: >>> On 6/16/19 5:07 PM, Jonathan Cameron wrote: >>>> On Wed, 12 Jun 2019 09:24:35 +0200 >>>> Fabrice Gasnier <[email protected]> wrote: >>>> >>>>> On stm32h7 and stm32mp1, the ADC inputs are multiplexed with analog >>>>> switches which have reduced performances when their supply is below 2.7V >>>>> (vdda by default): >>>>> - vdd supply can be selected if above 2.7V by setting ANASWVDD syscfg bit >>>>> (STM32MP1 only). >>>>> - Voltage booster can be used, to get full ADC performances by setting >>>>> BOOSTE/EN_BOOSTER syscfg bit (increases power consumption). >>>>> >>>>> Make this optional, since this is a trade-off between analog performance >>>>> and power consumption. >>>>> >>>>> Note: STM32H7 syscfg has a set and clear register for "BOOSTE" control. >>>>> STM32MP1 has separate set and clear registers pair to control EN_BOOSTER >>>>> and ANASWVDD bits. >>>>> >>>>> Signed-off-by: Fabrice Gasnier <[email protected]> >>>> >>>> A few minor bits inline, but mostly seems fine to me. >>>> >>>> Jonathan >>>> >>>>> --- >>>>> drivers/iio/adc/stm32-adc-core.c | 232 >>>>> ++++++++++++++++++++++++++++++++++++++- >>>>> 1 file changed, 230 insertions(+), 2 deletions(-) >>>>> >>>>> diff --git a/drivers/iio/adc/stm32-adc-core.c >>>>> b/drivers/iio/adc/stm32-adc-core.c >>>>> index 2327ec1..9d41b16 100644 >>>>> --- a/drivers/iio/adc/stm32-adc-core.c >>>>> +++ b/drivers/iio/adc/stm32-adc-core.c >>>>> @@ -14,9 +14,11 @@ >>>>> #include <linux/irqchip/chained_irq.h> >>>>> #include <linux/irqdesc.h> >>>>> #include <linux/irqdomain.h> >>>>> +#include <linux/mfd/syscon.h> >>>>> #include <linux/module.h> >>>>> #include <linux/of_device.h> >>>>> #include <linux/pm_runtime.h> >>>>> +#include <linux/regmap.h> >>>>> #include <linux/regulator/consumer.h> >>>>> #include <linux/slab.h> >>>>> >>>>> @@ -51,6 +53,20 @@ >>>>> >>>>> #define STM32_ADC_CORE_SLEEP_DELAY_MS 2000 >>>>> >>>>> +/* SYSCFG registers */ >>>>> +#define STM32H7_SYSCFG_PMCR 0x04 >>>>> +#define STM32MP1_SYSCFG_PMCSETR 0x04 >>>>> +#define STM32MP1_SYSCFG_PMCCLRR 0x44 >>>>> + >>>>> +/* SYSCFG bit fields */ >>>>> +#define STM32H7_SYSCFG_BOOSTE_MASK BIT(8) >>>>> +#define STM32MP1_SYSCFG_ANASWVDD_MASK BIT(9) >>>>> + >>>>> +/* SYSCFG capability flags */ >>>>> +#define HAS_VBOOSTER BIT(0) >>>>> +#define HAS_ANASWVDD BIT(1) >>>>> +#define HAS_CLEAR_REG BIT(2) >>>>> + >>>>> /** >>>>> * stm32_adc_common_regs - stm32 common registers, compatible dependent >>>>> data >>>>> * @csr: common status register offset >>>>> @@ -58,6 +74,11 @@ >>>>> * @eoc1: adc1 end of conversion flag in @csr >>>>> * @eoc2: adc2 end of conversion flag in @csr >>>>> * @eoc3: adc3 end of conversion flag in @csr >>>>> + * @has_syscfg: SYSCFG capability flags >>>>> + * @pmcr: SYSCFG_PMCSETR/SYSCFG_PMCR register offset >>>>> + * @pmcc: SYSCFG_PMCCLRR clear register offset >>>>> + * @booste_msk: SYSCFG BOOSTE / EN_BOOSTER bitmask in PMCR & PMCCLRR >>>>> + * @anaswvdd_msk: SYSCFG ANASWVDD bitmask in PMCR & PMCCLRR >>>>> */ >>>>> struct stm32_adc_common_regs { >>>>> u32 csr; >>>>> @@ -65,6 +86,11 @@ struct stm32_adc_common_regs { >>>>> u32 eoc1_msk; >>>>> u32 eoc2_msk; >>>>> u32 eoc3_msk; >>>>> + unsigned int has_syscfg; >>>>> + u32 pmcr; >>>>> + u32 pmcc; >>>>> + u32 booste_msk; >>>>> + u32 anaswvdd_msk; >>>>> }; >>>>> >>>>> struct stm32_adc_priv; >>>>> @@ -87,20 +113,26 @@ struct stm32_adc_priv_cfg { >>>>> * @domain: irq domain reference >>>>> * @aclk: clock reference for the analog circuitry >>>>> * @bclk: bus clock common for all ADCs, depends on part >>>>> used >>>>> + * @vdd: vdd supply reference >>>>> + * @vdda: vdda supply reference >>>>> * @vref: regulator reference >>>>> * @cfg: compatible configuration data >>>>> * @common: common data for all ADC instances >>>>> * @ccr_bak: backup CCR in low power mode >>>>> + * @syscfg: reference to syscon, system control registers >>>>> */ >>>>> struct stm32_adc_priv { >>>>> int irq[STM32_ADC_MAX_ADCS]; >>>>> struct irq_domain *domain; >>>>> struct clk *aclk; >>>>> struct clk *bclk; >>>>> + struct regulator *vdd; >>>>> + struct regulator *vdda; >>>>> struct regulator *vref; >>>>> const struct stm32_adc_priv_cfg *cfg; >>>>> struct stm32_adc_common common; >>>>> u32 ccr_bak; >>>>> + struct regmap *syscfg; >>>>> }; >>>>> >>>>> static struct stm32_adc_priv *to_stm32_adc_priv(struct stm32_adc_common >>>>> *com) >>>>> @@ -284,6 +316,22 @@ static const struct stm32_adc_common_regs >>>>> stm32h7_adc_common_regs = { >>>>> .ccr = STM32H7_ADC_CCR, >>>>> .eoc1_msk = STM32H7_EOC_MST, >>>>> .eoc2_msk = STM32H7_EOC_SLV, >>>>> + .has_syscfg = HAS_VBOOSTER, >>>>> + .pmcr = STM32H7_SYSCFG_PMCR, >>>>> + .booste_msk = STM32H7_SYSCFG_BOOSTE_MASK, >>>>> +}; >>>>> + >>>>> +/* STM32MP1 common registers definitions */ >>>>> +static const struct stm32_adc_common_regs stm32mp1_adc_common_regs = { >>>>> + .csr = STM32H7_ADC_CSR, >>>>> + .ccr = STM32H7_ADC_CCR, >>>>> + .eoc1_msk = STM32H7_EOC_MST, >>>>> + .eoc2_msk = STM32H7_EOC_SLV, >>>>> + .has_syscfg = HAS_VBOOSTER | HAS_ANASWVDD | HAS_CLEAR_REG, >>>> >>>> Extra space after = >>> >>> Hi Jonathan, >>> >>> Oops, I'll fix it in v2. >>> >>>> >>>> >>>>> + .pmcr = STM32MP1_SYSCFG_PMCSETR, >>>>> + .pmcc = STM32MP1_SYSCFG_PMCCLRR, >>>>> + .booste_msk = STM32H7_SYSCFG_BOOSTE_MASK, >>>>> + .anaswvdd_msk = STM32MP1_SYSCFG_ANASWVDD_MASK, >>>>> }; >>>>> >>>>> /* ADC common interrupt for all instances */ >>>>> @@ -388,16 +436,145 @@ static void stm32_adc_irq_remove(struct >>>>> platform_device *pdev, >>>>> } >>>>> } >>>>> >>>>> +static int stm32_adc_core_switches_supply_en(struct device *dev) >>>>> +{ >>>>> + struct stm32_adc_common *common = dev_get_drvdata(dev); >>>>> + struct stm32_adc_priv *priv = to_stm32_adc_priv(common); >>>>> + const struct stm32_adc_common_regs *regs = priv->cfg->regs; >>>>> + int ret, vdda, vdd = 0; >>>>> + u32 mask, clrmask, setmask = 0; >>>>> + >>>>> + /* >>>>> + * On STM32H7 and STM32MP1, the ADC inputs are multiplexed with analog >>>>> + * switches (via PCSEL) which have reduced performances when their >>>>> + * supply is below 2.7V (vdda by default): >>>>> + * - Voltage booster can be used, to get full ADC performances >>>>> + * (increases power consumption). >>>>> + * - Vdd can be used to supply them, if above 2.7V (STM32MP1 only). >>>>> + * >>>>> + * This is optional, as this is a trade-off between analog performance >>>>> + * and power consumption. >>>>> + */ >>>>> + if (!regs->has_syscfg || !priv->vdda || !priv->syscfg) { >>>>> + dev_dbg(dev, "Not configuring analog switches\n"); >>>>> + return 0; >>>>> + } >>>>> + >>>>> + ret = regulator_enable(priv->vdda); >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "vdda enable failed %d\n", ret); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + ret = regulator_get_voltage(priv->vdda); >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "vdda get voltage failed %d\n", ret); >>>>> + goto vdda_dis; >>>>> + } >>>>> + vdda = ret; >>>>> + >>>> We only need to do the following block if vdaa is too low. Should probably >>>> not turn on vdd if there is not chance we are going to use it? >>> >>> You're right, then I probably need to move the regulator_get_voltage() >>> call at probe time, to avoid enabling it for nothing at runtime. (e.g. >>> to figure out it's not going to be used). >>> In fact, vdd is used also for other things on the platform (I/Os, other >>> supplies...), and is marked "always-on" in the device tree. But I >>> understand your point. >>> >>> I'll rework this and send a v2. >> >> Hi Jonathan, >> >> When reworking this part, I figured out the vdda should be described as >> required supply for the STM32 ADC. So I pushed out a fix series to >> address this "Add missing vdda-supply to STM32 ADC". I'll resume v2 of >> this series that has some dependencies on the fix series . > > Cool. Given timing I'm taking fixes and new stuff through the togreg > branch anyway at the moment so dependencies on fixes are easier than > normal for the next week or so ;)
Hi Jonathan I did some additional changes at my end. Just for reference, you can see "regulator: add support for the STM32 ADC booster" series here: https://lkml.org/lkml/2019/6/28/188 I think the proper way to handle the booster is to have it fully described as a regulator (that's one). This will also simplify handling it here. I'll resume this series once the regulator series has been discussed. Many thanks, Best regards, Fabrice > > Jonathan > >> >> Thanks >> Best Regards, >> Fabrice >> >>> >>> Thanks, >>> Fabrice >>> >>>> >>>>> + if (priv->vdd && regs->has_syscfg & HAS_ANASWVDD) { >>>>> + ret = regulator_enable(priv->vdd); >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "vdd enable failed %d\n", ret); >>>>> + goto vdda_dis; >>>>> + } >>>>> + >>>>> + ret = regulator_get_voltage(priv->vdd); >>>>> + if (ret < 0) { >>>>> + dev_err(dev, "vdd get voltage failed %d\n", ret); >>>>> + goto vdd_dis; >>>>> + } >>>>> + vdd = ret; >>>>> + } >>>>> + >>>>> + /* >>>>> + * Recommended settings for ANASWVDD and EN_BOOSTER: >>>>> + * - vdda < 2.7V but vdd > 2.7V: ANASWVDD = 1, EN_BOOSTER = 0 (stm32mp1) >>>>> + * - vdda < 2.7V and vdd < 2.7V: ANASWVDD = 0, EN_BOOSTER = 1 >>>>> + * - vdda >= 2.7V: ANASWVDD = 0, EN_BOOSTER = 0 (default) >>>>> + */ >>>>> + if (vdda < 2700000) { >>>>> + if (vdd > 2700000) { >>>>> + dev_dbg(dev, "analog switches supplied by vdd\n"); >>>>> + setmask = regs->anaswvdd_msk; >>>>> + clrmask = regs->booste_msk; >>>>> + } else { >>>>> + dev_dbg(dev, "Enabling voltage booster\n"); >>>>> + setmask = regs->booste_msk; >>>>> + clrmask = regs->anaswvdd_msk; >>>>> + } >>>>> + } else { >>>>> + dev_dbg(dev, "analog switches supplied by vdda\n"); >>>>> + clrmask = regs->booste_msk | regs->anaswvdd_msk; >>>>> + } >>>>> + >>>>> + mask = regs->booste_msk | regs->anaswvdd_msk; >>>>> + if (regs->has_syscfg & HAS_CLEAR_REG) { >>>>> + ret = regmap_write(priv->syscfg, regs->pmcc, clrmask); >>>>> + if (ret) { >>>>> + dev_err(dev, "syscfg clear failed, %d\n", ret); >>>>> + goto vdd_dis; >>>>> + } >>>>> + mask = setmask; >>>>> + } >>>>> + >>>>> + ret = regmap_update_bits(priv->syscfg, regs->pmcr, mask, setmask); >>>>> + if (ret) { >>>>> + dev_err(dev, "syscfg update failed, %d\n", ret); >>>>> + goto vdd_dis; >>>>> + } >>>>> + >>>>> + /* Booster voltage can take up to 50 us to stabilize */ >>>>> + if (setmask & regs->booste_msk) >>>>> + usleep_range(50, 100); >>>>> + >>>>> + return ret; >>>>> + >>>>> +vdd_dis: >>>>> + if (priv->vdd && (regs->has_syscfg & HAS_ANASWVDD)) >>>>> + regulator_disable(priv->vdd); >>>>> +vdda_dis: >>>>> + regulator_disable(priv->vdda); >>>>> + >>>>> + return ret; >>>>> +} >>>>> + >>>>> +static void stm32_adc_core_switches_supply_dis(struct device *dev) >>>>> +{ >>>>> + struct stm32_adc_common *common = dev_get_drvdata(dev); >>>>> + struct stm32_adc_priv *priv = to_stm32_adc_priv(common); >>>>> + const struct stm32_adc_common_regs *regs = priv->cfg->regs; >>>>> + u32 mask = regs->booste_msk | regs->anaswvdd_msk; >>>>> + >>>>> + if (!regs->has_syscfg || !priv->vdda || !priv->syscfg) >>>>> + return; >>>>> + >>>>> + if (regs->has_syscfg & HAS_CLEAR_REG) >>>>> + regmap_write(priv->syscfg, regs->pmcc, mask); >>>>> + else >>>>> + regmap_update_bits(priv->syscfg, regs->pmcr, mask, 0); >>>>> + >>>>> + if (priv->vdd && (regs->has_syscfg & HAS_ANASWVDD)) >>>>> + regulator_disable(priv->vdd); >>>>> + >>>>> + regulator_disable(priv->vdda); >>>>> +} >>>>> + >>>>> static int stm32_adc_core_hw_start(struct device *dev) >>>>> { >>>>> struct stm32_adc_common *common = dev_get_drvdata(dev); >>>>> struct stm32_adc_priv *priv = to_stm32_adc_priv(common); >>>>> int ret; >>>>> >>>>> + ret = stm32_adc_core_switches_supply_en(dev); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> ret = regulator_enable(priv->vref); >>>>> if (ret < 0) { >>>>> dev_err(dev, "vref enable failed\n"); >>>>> - return ret; >>>>> + goto err_switches_disable; >>>>> } >>>>> >>>>> if (priv->bclk) { >>>>> @@ -425,6 +602,8 @@ static int stm32_adc_core_hw_start(struct device *dev) >>>>> clk_disable_unprepare(priv->bclk); >>>>> err_regulator_disable: >>>>> regulator_disable(priv->vref); >>>>> +err_switches_disable: >>>>> + stm32_adc_core_switches_supply_dis(dev); >>>>> >>>>> return ret; >>>>> } >>>>> @@ -441,6 +620,24 @@ static void stm32_adc_core_hw_stop(struct device >>>>> *dev) >>>>> if (priv->bclk) >>>>> clk_disable_unprepare(priv->bclk); >>>>> regulator_disable(priv->vref); >>>>> + stm32_adc_core_switches_supply_dis(dev); >>>>> +} >>>>> + >>>>> +static int stm32_adc_core_syscfg_probe(struct device_node *np, >>>>> + struct stm32_adc_priv *priv) >>>>> +{ >>>>> + if (!priv->cfg->regs->has_syscfg) >>>>> + return 0; >>>>> + >>>>> + priv->syscfg = syscon_regmap_lookup_by_phandle(np, "st,syscfg"); >>>>> + if (IS_ERR(priv->syscfg)) { >>>>> + /* Optional */ >>>>> + if (PTR_ERR(priv->syscfg) != -ENODEV) >>>>> + return PTR_ERR(priv->syscfg); >>>>> + priv->syscfg = NULL; >>>>> + } >>>>> + >>>>> + return 0; >>>>> } >>>>> >>>>> static int stm32_adc_probe(struct platform_device *pdev) >>>>> @@ -475,6 +672,30 @@ static int stm32_adc_probe(struct platform_device >>>>> *pdev) >>>>> return ret; >>>>> } >>>>> >>>>> + priv->vdda = devm_regulator_get_optional(&pdev->dev, "vdda"); >>>>> + if (IS_ERR(priv->vdda)) { >>>>> + ret = PTR_ERR(priv->vdda); >>>>> + if (ret != -ENODEV) { >>>>> + if (ret != -EPROBE_DEFER) >>>>> + dev_err(&pdev->dev, "vdda get failed, %d\n", >>>>> + ret); >>>>> + return ret; >>>>> + } >>>>> + priv->vdda = NULL; >>>>> + } >>>>> + >>>>> + priv->vdd = devm_regulator_get_optional(&pdev->dev, "vdd"); >>>>> + if (IS_ERR(priv->vdd)) { >>>>> + ret = PTR_ERR(priv->vdd); >>>>> + if (ret != -ENODEV) { >>>>> + if (ret != -EPROBE_DEFER) >>>>> + dev_err(&pdev->dev, "vdd get failed, %d\n", >>>>> + ret); >>>>> + return ret; >>>>> + } >>>>> + priv->vdd = NULL; >>>>> + } >>>>> + >>>>> priv->aclk = devm_clk_get(&pdev->dev, "adc"); >>>>> if (IS_ERR(priv->aclk)) { >>>>> ret = PTR_ERR(priv->aclk); >>>>> @@ -495,6 +716,13 @@ static int stm32_adc_probe(struct platform_device >>>>> *pdev) >>>>> priv->bclk = NULL; >>>>> } >>>>> >>>>> + ret = stm32_adc_core_syscfg_probe(np, priv); >>>>> + if (ret) { >>>>> + if (ret != -EPROBE_DEFER) >>>>> + dev_err(&pdev->dev, "Can't probe syscfg: %d\n", ret); >>>>> + return ret; >>>>> + } >>>>> + >>>>> pm_runtime_get_noresume(dev); >>>>> pm_runtime_set_active(dev); >>>>> pm_runtime_set_autosuspend_delay(dev, STM32_ADC_CORE_SLEEP_DELAY_MS); >>>>> @@ -595,7 +823,7 @@ static const struct stm32_adc_priv_cfg >>>>> stm32h7_adc_priv_cfg = { >>>>> }; >>>>> >>>>> static const struct stm32_adc_priv_cfg stm32mp1_adc_priv_cfg = { >>>>> - .regs = &stm32h7_adc_common_regs, >>>>> + .regs = &stm32mp1_adc_common_regs, >>>>> .clk_sel = stm32h7_adc_clk_sel, >>>>> .max_clk_rate_hz = 40000000, >>>>> }; >>>> >
