| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 27 Sep 2021 18:36:27 +0200
From: Fabrice Gasnier <fabrice.gasnier@...s.st.com>
To: Olivier Moysan <olivier.moysan@...s.st.com>,
Jonathan Cameron <jic23@...nel.org>
CC: Alexandre Torgue <alexandre.torgue@...s.st.com>,
Fabrice Gasnier <fabrice.gasnier@...s.st.com>,
Lars-Peter Clausen <lars@...afoo.de>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Rob Herring <robh+dt@...nel.org>, <linux-iio@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
<linux-stm32@...md-mailman.stormreply.com>
Subject: Re: [Linux-stm32] [PATCH v3 5/7] iio: adc: stm32-adc: add support of
internal channels
On 9/24/21 10:34 AM, Olivier Moysan wrote:
> Add support of ADC2 internal channels VDDCORE, VREFINT and VBAT.
> The reserved label name "vddcore", "vrefint" and "vbat" must
> be used in Device Tree channel node, to enable the corresponding
> internal channel.
>
> Note: This patch does not provide support of internal channels
> for F4 and H7.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@...s.st.com>
> ---
> drivers/iio/adc/stm32-adc-core.h | 8 ++
> drivers/iio/adc/stm32-adc.c | 142 ++++++++++++++++++++++++++++++-
> 2 files changed, 147 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-adc-core.h b/drivers/iio/adc/stm32-adc-core.h
> index 2322809bfd2f..7c924f463f67 100644
> --- a/drivers/iio/adc/stm32-adc-core.h
> +++ b/drivers/iio/adc/stm32-adc-core.h
> @@ -102,6 +102,9 @@
> #define STM32H7_ADC_CALFACT 0xC4
> #define STM32H7_ADC_CALFACT2 0xC8
>
> +/* STM32MP1 - ADC2 instance option register */
> +#define STM32MP1_ADC2_OR 0xD0
> +
> /* STM32H7 - common registers for all ADC instances */
> #define STM32H7_ADC_CSR (STM32_ADCX_COMN_OFFSET + 0x00)
> #define STM32H7_ADC_CCR (STM32_ADCX_COMN_OFFSET + 0x08)
> @@ -168,11 +171,16 @@ enum stm32h7_adc_dmngt {
> #define STM32H7_EOC_MST BIT(2)
>
> /* STM32H7_ADC_CCR - bit fields */
> +#define STM32H7_VBATEN BIT(24)
> +#define STM32H7_VREFEN BIT(22)
> #define STM32H7_PRESC_SHIFT 18
> #define STM32H7_PRESC_MASK GENMASK(21, 18)
> #define STM32H7_CKMODE_SHIFT 16
> #define STM32H7_CKMODE_MASK GENMASK(17, 16)
>
> +/* STM32MP1_ADC2_OR - bit fields */
> +#define STM32MP1_VDDCOREEN BIT(0)
> +
> /**
> * struct stm32_adc_common - stm32 ADC driver common data (for all instances)
> * @base: control registers base cpu addr
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 85d09cbd41ae..5a7ac45a0725 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -77,6 +77,30 @@ enum stm32_adc_extsel {
> STM32_EXT20,
> };
>
> +enum stm32_adc_int_ch {
> + STM32_ADC_INT_CH_NONE = -1,
> + STM32_ADC_INT_CH_VDDCORE,
> + STM32_ADC_INT_CH_VREFINT,
> + STM32_ADC_INT_CH_VBAT,
> + STM32_ADC_INT_CH_NB,
> +};
> +
> +/**
> + * struct stm32_adc_ic - ADC internal channels
> + * @name: name of the internal channel
> + * @idx: internal channel enum index
> + */
> +struct stm32_adc_ic {
> + const char *name;
> + u32 idx;
> +};
> +
> +static const struct stm32_adc_ic stm32_adc_ic[STM32_ADC_INT_CH_NB] = {
> + { "vddcore", STM32_ADC_INT_CH_VDDCORE },
> + { "vrefint", STM32_ADC_INT_CH_VREFINT },
> + { "vbat", STM32_ADC_INT_CH_VBAT },
> +};
> +
> /**
> * struct stm32_adc_trig_info - ADC trigger info
> * @name: name of the trigger, corresponding to its source
> @@ -126,6 +150,9 @@ struct stm32_adc_regs {
> * @res: resolution selection register & bitfield
> * @smpr: smpr1 & smpr2 registers offset array
> * @smp_bits: smpr1 & smpr2 index and bitfields
> + * @or_vdd: option register & vddcore bitfield
> + * @ccr_vbat: common register & vbat bitfield
> + * @ccr_vref: common register & vrefint bitfield
> */
> struct stm32_adc_regspec {
> const u32 dr;
> @@ -139,6 +166,9 @@ struct stm32_adc_regspec {
> const struct stm32_adc_regs res;
> const u32 smpr[2];
> const struct stm32_adc_regs *smp_bits;
> + const struct stm32_adc_regs or_vdd;
> + const struct stm32_adc_regs ccr_vbat;
> + const struct stm32_adc_regs ccr_vref;
> };
>
> struct stm32_adc;
> @@ -195,6 +225,7 @@ struct stm32_adc_cfg {
> * @cal: optional calibration data on some devices
> * @chan_name: channel name array
> * @num_diff: number of differential channels
> + * @int_ch: internal channel indexes array
> */
> struct stm32_adc {
> struct stm32_adc_common *common;
> @@ -219,6 +250,7 @@ struct stm32_adc {
> struct stm32_adc_calib cal;
> char chan_name[STM32_ADC_CH_MAX][STM32_ADC_CH_SZ];
> u32 num_diff;
> + int int_ch[STM32_ADC_INT_CH_NB];
> };
>
> struct stm32_adc_diff_channel {
> @@ -451,6 +483,24 @@ static const struct stm32_adc_regspec stm32h7_adc_regspec = {
> .smp_bits = stm32h7_smp_bits,
> };
>
> +static const struct stm32_adc_regspec stm32mp1_adc_regspec = {
> + .dr = STM32H7_ADC_DR,
> + .ier_eoc = { STM32H7_ADC_IER, STM32H7_EOCIE },
> + .ier_ovr = { STM32H7_ADC_IER, STM32H7_OVRIE },
> + .isr_eoc = { STM32H7_ADC_ISR, STM32H7_EOC },
> + .isr_ovr = { STM32H7_ADC_ISR, STM32H7_OVR },
> + .sqr = stm32h7_sq,
> + .exten = { STM32H7_ADC_CFGR, STM32H7_EXTEN_MASK, STM32H7_EXTEN_SHIFT },
> + .extsel = { STM32H7_ADC_CFGR, STM32H7_EXTSEL_MASK,
> + STM32H7_EXTSEL_SHIFT },
> + .res = { STM32H7_ADC_CFGR, STM32H7_RES_MASK, STM32H7_RES_SHIFT },
> + .smpr = { STM32H7_ADC_SMPR1, STM32H7_ADC_SMPR2 },
> + .smp_bits = stm32h7_smp_bits,
> + .or_vdd = { STM32MP1_ADC2_OR, STM32MP1_VDDCOREEN },
> + .ccr_vbat = { STM32H7_ADC_CCR, STM32H7_VBATEN },
> + .ccr_vref = { STM32H7_ADC_CCR, STM32H7_VREFEN },
> +};
> +
> /*
> * STM32 ADC registers access routines
> * @adc: stm32 adc instance
> @@ -489,6 +539,16 @@ static void stm32_adc_set_bits(struct stm32_adc *adc, u32 reg, u32 bits)
> spin_unlock_irqrestore(&adc->lock, flags);
> }
>
> +static void stm32_adc_set_bits_common(struct stm32_adc *adc, u32 reg, u32 bits)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&adc->lock, flags);
Hi Olivier,
Sorry, I missed it in earlier version. I think there's no need for
protection against IRQs on "common" registers here.
But, this "common" register is touched in stm32-adc-core driver, in the
probe and pm routines. So maybe a lock in the adc->common structure
could be added here, instead of private (child only) adc lock?
> + writel_relaxed(readl_relaxed(adc->common->base + reg) | bits,
> + adc->common->base + reg);
> + spin_unlock_irqrestore(&adc->lock, flags);
> +}
> +
> static void stm32_adc_clr_bits(struct stm32_adc *adc, u32 reg, u32 bits)
> {
> unsigned long flags;
> @@ -498,6 +558,16 @@ static void stm32_adc_clr_bits(struct stm32_adc *adc, u32 reg, u32 bits)
> spin_unlock_irqrestore(&adc->lock, flags);
> }
>
> +static void stm32_adc_clr_bits_common(struct stm32_adc *adc, u32 reg, u32 bits)
> +{
> + unsigned long flags;
> +
> + spin_lock_irqsave(&adc->lock, flags);
same here
Thanks,
Fabrice
> + writel_relaxed(readl_relaxed(adc->common->base + reg) & ~bits,
> + adc->common->base + reg);
> + spin_unlock_irqrestore(&adc->lock, flags);
> +}
> +
> /**
> * stm32_adc_conv_irq_enable() - Enable end of conversion interrupt
> * @adc: stm32 adc instance
> @@ -579,6 +649,60 @@ static int stm32_adc_hw_start(struct device *dev)
> return ret;
> }
>
> +static void stm32_adc_int_ch_enable(struct iio_dev *indio_dev)
> +{
> + struct stm32_adc *adc = iio_priv(indio_dev);
> + u32 i;
> +
> + for (i = 0; i < STM32_ADC_INT_CH_NB; i++) {
> + if (adc->int_ch[i] == STM32_ADC_INT_CH_NONE)
> + continue;
> +
> + switch (i) {
> + case STM32_ADC_INT_CH_VDDCORE:
> + dev_dbg(&indio_dev->dev, "Enable VDDCore\n");
> + stm32_adc_set_bits(adc, adc->cfg->regs->or_vdd.reg,
> + adc->cfg->regs->or_vdd.mask);
> + break;
> + case STM32_ADC_INT_CH_VREFINT:
> + dev_dbg(&indio_dev->dev, "Enable VREFInt\n");
> + stm32_adc_set_bits_common(adc, adc->cfg->regs->ccr_vref.reg,
> + adc->cfg->regs->ccr_vref.mask);
> + break;
> + case STM32_ADC_INT_CH_VBAT:
> + dev_dbg(&indio_dev->dev, "Enable VBAT\n");
> + stm32_adc_set_bits_common(adc, adc->cfg->regs->ccr_vbat.reg,
> + adc->cfg->regs->ccr_vbat.mask);
> + break;
> + }
> + }
> +}
> +
> +static void stm32_adc_int_ch_disable(struct stm32_adc *adc)
> +{
> + u32 i;
> +
> + for (i = 0; i < STM32_ADC_INT_CH_NB; i++) {
> + if (adc->int_ch[i] == STM32_ADC_INT_CH_NONE)
> + continue;
> +
> + switch (i) {
> + case STM32_ADC_INT_CH_VDDCORE:
> + stm32_adc_clr_bits(adc, adc->cfg->regs->or_vdd.reg,
> + adc->cfg->regs->or_vdd.mask);
> + break;
> + case STM32_ADC_INT_CH_VREFINT:
> + stm32_adc_clr_bits_common(adc, adc->cfg->regs->ccr_vref.reg,
> + adc->cfg->regs->ccr_vref.mask);
> + break;
> + case STM32_ADC_INT_CH_VBAT:
> + stm32_adc_clr_bits_common(adc, adc->cfg->regs->ccr_vbat.reg,
> + adc->cfg->regs->ccr_vbat.mask);
> + break;
> + }
> + }
> +}
> +
> /**
> * stm32f4_adc_start_conv() - Start conversions for regular channels.
> * @indio_dev: IIO device instance
> @@ -947,11 +1071,13 @@ static int stm32h7_adc_prepare(struct iio_dev *indio_dev)
> goto pwr_dwn;
> calib = ret;
>
> + stm32_adc_int_ch_enable(indio_dev);
> +
> stm32_adc_writel(adc, STM32H7_ADC_DIFSEL, adc->difsel);
>
> ret = stm32h7_adc_enable(indio_dev);
> if (ret)
> - goto pwr_dwn;
> + goto ch_disable;
>
> /* Either restore or read calibration result for future reference */
> if (calib)
> @@ -967,6 +1093,8 @@ static int stm32h7_adc_prepare(struct iio_dev *indio_dev)
>
> disable:
> stm32h7_adc_disable(indio_dev);
> +ch_disable:
> + stm32_adc_int_ch_disable(adc);
> pwr_dwn:
> stm32h7_adc_enter_pwr_down(adc);
>
> @@ -978,6 +1106,7 @@ static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
> struct stm32_adc *adc = iio_priv(indio_dev);
>
> stm32h7_adc_disable(indio_dev);
> + stm32_adc_int_ch_disable(adc);
> stm32h7_adc_enter_pwr_down(adc);
> }
>
> @@ -1800,7 +1929,7 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev,
> const struct stm32_adc_info *adc_info = adc->cfg->adc_info;
> struct device_node *child;
> const char *name;
> - int val, scan_index = 0, ret;
> + int val, scan_index = 0, ret, i;
> bool differential;
> u32 vin[2];
>
> @@ -1820,6 +1949,10 @@ static int stm32_adc_generic_chan_init(struct iio_dev *indio_dev,
> return -EINVAL;
> }
> strncpy(adc->chan_name[val], name, STM32_ADC_CH_SZ);
> + for (i = 0; i < STM32_ADC_INT_CH_NB; i++) {
> + if (!strncmp(stm32_adc_ic[i].name, name, STM32_ADC_CH_SZ))
> + adc->int_ch[i] = val;
> + }
> } else if (ret != -EINVAL) {
> dev_err(&indio_dev->dev, "Invalid label %d\n", ret);
> goto err;
> @@ -1869,6 +2002,9 @@ static int stm32_adc_chan_of_init(struct iio_dev *indio_dev, bool timestamping)
> u32 smp = 0;
> bool legacy = false;
>
> + for (i = 0; i < STM32_ADC_INT_CH_NB; i++)
> + adc->int_ch[i] = STM32_ADC_INT_CH_NONE;
> +
> num_channels = of_get_available_child_count(node);
> /* If no channels have been found, fallback to channels legacy properties. */
> if (!num_channels) {
> @@ -2219,7 +2355,7 @@ static const struct stm32_adc_cfg stm32h7_adc_cfg = {
> };
>
> static const struct stm32_adc_cfg stm32mp1_adc_cfg = {
> - .regs = &stm32h7_adc_regspec,
> + .regs = &stm32mp1_adc_regspec,
> .adc_info = &stm32h7_adc_info,
> .trigs = stm32h7_adc_trigs,
> .has_vregready = true,
>
Powered by blists - more mailing lists