pwm: imx27: workaround of the pwm output bug when decrease the duty cycle
Subsystems (2)
Frank Li
maintainerPATCH v5 1/1 pwm: imx27: workaround of the pwm output bug when decrease the duty cycle
From: Clark Wang <[email protected]>
Implement workaround for ERR051198
(https://www.nxp.com/docs/en/errata/IMX8MN_0N14Y.pdf)
PWM output may not function correctly if the FIFO is empty when a new SAR value is programmed
Description:
When the PWM FIFO is empty, a new value programmed to the PWM Sample register (PWM_PWMSAR) will be directly applied even if the current timer period has not expired. If the new SAMPLE value programmed in the PWM_PWMSAR register is less than the previous value, and the PWM counter register (PWM_PWMCNR) that contains the current COUNT value is greater than the new programmed SAMPLE value, the current period will not flip the level. This may result in an output pulse with a duty cycle of 100%.
Workaround:
Program the current SAMPLE value in the PWM_PWMSAR register before updating the new duty cycle to the SAMPLE value in the PWM_PWMSAR register. This will ensure that the new SAMPLE value is modified during a non-empty FIFO, and can be successfully updated after the period expires.
Write the old SAR value before updating the new duty cycle to SAR. This avoids writing the new value into an empty FIFO.
This only resolves the issue when the PWM period is longer than 2us (or <500KHz) because write register is not quick enough when PWM period is very short.
Reproduce steps:
cd /sys/class/pwm/pwmchip1/pwm0
echo 2000000000 > period # It is easy to observe by using long period
echo 1000000000 > duty_cycle
echo 1 > enable
echo 800000000 > duty_cycle # One full high plus will be seen by scope
---
Change from v4 to v5
- fix typo PMW & If
- using imx->mmio_base + MX3_PWMSAR
Change from v3 to v4
- none, wrong bump version number
Change from v2 to v3
- simple workaround implement.
- add reproduce steps.
Change from v1 to v2
- address comments in https://lore.kernel.org/linux-pwm/[email protected]/
About disable/enable pwm instead of disable/enable irq:
Some pmw periphal may sensitive to period. Disable/enable pwm will
increase period, althouhg it is okay for most case, such as LED backlight
or FAN speed. But some device such servo may require strict period.
- address comments in https://lore.kernel.org/linux-pwm/[email protected]/
Using official errata number
fix typo 'filp'
add {} for else
I supposed fixed all previous issues, let me know if I missed one.
--- drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-)
drivers/pwm/pwm-imx27.c +66 -1
@@ -27,6 +27,7 @@#define MX3_PWMSR 0x04/* PWM Status Register */ #define MX3_PWMSAR 0x0C/* PWM Sample Register */ #define MX3_PWMPR 0x10/* PWM Period Register */ +#define MX3_PWMCNR 0x14/* PWM Counter Register */ #define MX3_PWMCR_FWM GENMASK(27, 26)#define MX3_PWMCR_STOPEN BIT(25)
@@ -234,6 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,struct pwm_imx27_chip *imx =to_pwm_imx27_chip (chip);unsigned long long c;unsigned long long clkrate; +unsigned long flags; +int val;int ret;u32 cr;
@@ -274,7 +277,69 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,pwm_imx27_sw_reset (chip); } -writel (duty_cycles, imx->mmio_base +MX3_PWMSAR ); +/* +* This is a limited workaround. When the SAR FIFO is empty, the new +* write value will be directly applied to SAR even the current period +* is not over. +* +* ─────────────────────┐ +* PWM OUTPUT │ +* └───────────────────────── +* +* ┌──────────────────────────────────────────────┐ +* Counter │ XXXXXXXXXXXXXX │ +* └──────────────────────────────────────────────┘ +* ▲ ▲ +* │ │ +* New SAR Old SAR +* +* XXXX Errata happen window +* +* If the new SAR value is less than the old one, and the counter is +* greater than the new SAR value (see above diagram XXXX), the current +* period will not flip the level. This will result in a pulse with a +* duty cycle of 100%. +* +* Check new sar less than old sar and current counter is in errata +* windows, write extra old sar into FIFO and new sar will effect at +* next period. +* +* Sometime period is quite long, such as over 1 second. If add old sar +* into FIFO unconditional, new sar have to wait for next period. It +* may be too long. +* +* Turn off the interrupt to ensure that not irq and schedule happen +* during above operations. If any irq and schedule happen, counter +* in PWM will be out of data and take wrong action. +* +* Add a safety margin 1.5us because it needs some time to complete +* IO write. +* +* Use __raw_writel() to minimize the interval between two writes to +* the SAR register to increase the fastest pwm frequency supported. +* +* When the PWM period is longer than 2us(or <500KHz), this workaround +* can solve this problem. No software workaround is available if PWM +* period is shorter than IO write. +*/ + c = clkrate *1500 ; +do_div (c,NSEC_PER_SEC ); + +local_irq_save (flags); + val =FIELD_GET (MX3_PWMSR_FIFOAV ,readl_relaxed (imx->mmio_base +MX3_PWMSR )); +if (duty_cycles < imx->duty_cycle && val <MX3_PWMSR_FIFOAV_2WORDS ) { + val =readl_relaxed (imx->mmio_base +MX3_PWMCNR ); +if ((val + c >= duty_cycles && val < imx->duty_cycle) || +/* +* If counter is close to period, controller may roll over +* when next IO write. +*/ + val + c >= period_cycles) +writel_relaxed (imx->duty_cycle, imx->mmio_base +MX3_PWMSAR ); + } +writel_relaxed (duty_cycles, imx->mmio_base +MX3_PWMSAR ); +local_irq_restore (flags); +writel (period_cycles, imx->mmio_base +MX3_PWMPR );/*
Signature · 1 line
2.34.1
On 9/10/24 5:24 PM, Frank Li wrote:
Hi,
purely nitpicking further below, one quick question right below.
Quoted 3 lines
This only resolves the issue when the PWM period is longer than 2us (or <500KHz) because write register is not quick enough when PWM period is very short.
You did mention the IPS bus is slow. Do I understand it correctly that the IPS bus write takes about 1us ? Because of the PWM consumes a sample every 2us and we need to write 2 samples to avoid FIFO underrun, then to safely write those 2 samples, we need to be able to write 1 sample per 1 us into the FIFO ?
Also, would writing more samples help with such "fast" use cases ? Something like this:
if (clkrate > 500000) {
// This usleep() could use some further improvement, e.g. calculate // precise delay for the FIFO to get empty based on PWM clkrate usleep(2 * 5); // wait 2us for each of the 4 samples in FIFO and a bit // Now the FIFO is surely empty, write all four FIFO slots writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
} else { // clock rate less than 500 kHz
// Do the workaround below
if (duty_cycles < imx->duty_cycle && val < MX3_PWMSR_FIFOAV_2WORDS
... }
Quoted 79 lines
Reproduce steps:
cd/sys/class/pwm/pwmchip1/pwm0
echo 2000000000 > period # It is easy to observe by using long period
echo 1000000000 >duty_cycle
echo 1 > enable
echo 800000000 >duty_cycle# One full high plus will be seen by scopeFixes 166091b1894d ("[ARM] MXC: add pwm driver for i.MX SoCs")Reviewed-by Jun Li <[email protected]>Signed-off-by Clark Wang <[email protected]>Signed-off-by Frank Li <[email protected]>---
Change from v4 to v5
- fix typo PMW & If
- usingimx->mmio_base+MX3_PWMSARChange from v3 to v4
- none, wrong bump version number
Change from v2 to v3
- simple workaround implement.
- add reproduce steps.Change from v1 to v2
- address comments in https://lore.kernel.org/linux-pwm/[email protected]/
About disable/enable pwm instead of disable/enable irq:
Some pmw periphal may sensitive to period. Disable/enable pwm will
increase period, althouhg it is okay for most case, such as LED backlight
or FAN speed. But some device such servo may require strict period.- address comments in https://lore.kernel.org/linux-pwm/[email protected]/
Using official errata number
fix typo 'filp'
add {} for elseI supposed fixed all previous issues, let me know if I missed one.
--- drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-)drivers/pwm/pwm-imx27.c +17
-1@@ -27,6 +27,7 @@#define MX3_PWMSR 0x04/* PWM Status Register */ #define MX3_PWMSAR 0x0C/* PWM Sample Register */ #define MX3_PWMPR 0x10/* PWM Period Register */ +#define MX3_PWMCNR 0x14/* PWM Counter Register */ #define MX3_PWMCR_FWM GENMASK(27, 26)#define MX3_PWMCR_STOPEN BIT(25)@@ -234,6 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,struct pwm_imx27_chip *imx =to_pwm_imx27_chip (chip);unsigned long long c;unsigned long long clkrate; +unsigned long flags; +int val;int ret;u32 cr;@@ -274,7 +277,69 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,pwm_imx27_sw_reset (chip); } -writel (duty_cycles, imx->mmio_base +MX3_PWMSAR ); +/* +* This is a limited workaround. When the SAR FIFO is empty, the new +* write value will be directly applied to SAR even the current period +* is not over. +* +* ─────────────────────┐ +* PWM OUTPUT │ +* └───────────────────────── +* +* ┌──────────────────────────────────────────────┐ +* Counter │ XXXXXXXXXXXXXX │ +* └──────────────────────────────────────────────┘ +* ▲ ▲ +* │ │
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 303
Quoted 10 lines
drivers/pwm/pwm-imx27.c +10
-0+* New SAR Old SAR +* +* XXXX Errata happen window +* +* If the new SAR value is less than the old one, and the counter is +* greater than the new SAR value (see above diagram XXXX), the current +* period will not flip the level. This will result in a pulse with a +* duty cycle of 100%. +* +* Check new sar less than old sar and current counter is in errata
Better do 's@\<sar\>@SAR@g' so all the instances of 'sar' in the text are in uppercase. Currently, some are in lowercase and some are in uppercase.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 311
Quoted 8 lines
drivers/pwm/pwm-imx27.c +8
-0+* windows, write extra old sar into FIFO and new sar will effect at +* next period. +* +* Sometime period is quite long, such as over 1 second. If add old sar +* into FIFO unconditional, new sar have to wait for next period. It +* may be too long. +* +* Turn off the interrupt to ensure that not irq and schedule happen
IRQ, in uppercase.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 319
Quoted 8 lines
drivers/pwm/pwm-imx27.c +8
-0+* during above operations. If any irq and schedule happen, counter +* in PWM will be out of data and take wrong action. +* +* Add a safety margin 1.5us because it needs some time to complete +* IO write. +* +* Use __raw_writel() to minimize the interval between two writes to +* the SAR register to increase the fastest pwm frequency supported.
PWM, in uppercase.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 321
Quoted 1 line
drivers/pwm/pwm-imx27.c +1
-0+* When the PWM period is longer than 2us(or <500KHz), this workaround
kHz, kilo lowercase, Hz Hertz uppercase H lowercase z .
Also fix in the commit message.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 331
Quoted 10 lines
drivers/pwm/pwm-imx27.c +10
-0+* can solve this problem. No software workaround is available if PWM +* period is shorter than IO write. +*/ + c = clkrate *1500 ; +do_div (c,NSEC_PER_SEC ); + +local_irq_save (flags); + val =FIELD_GET (MX3_PWMSR_FIFOAV ,readl_relaxed (imx->mmio_base +MX3_PWMSR )); +if (duty_cycles < imx->duty_cycle && val <MX3_PWMSR_FIFOAV_2WORDS ) { + val =readl_relaxed (imx->mmio_base +MX3_PWMCNR );
I would put the comment below above this conditional statement.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 343
Quoted 12 lines
drivers/pwm/pwm-imx27.c +11
-0+if ((val + c >= duty_cycles && val < imx->duty_cycle) || +/* +* If counter is close to period, controller may roll over +* when next IO write. +*/ + val + c >= period_cycles) +writel_relaxed (imx->duty_cycle, imx->mmio_base +MX3_PWMSAR ); + } +writel_relaxed (duty_cycles, imx->mmio_base +MX3_PWMSAR ); +local_irq_restore (flags); +writel (period_cycles, imx->mmio_base +MX3_PWMPR );
The patch looks good, the above are just trivial nitpicks.
Thanks !
Frank Li
maintainerQuoted 10 lines · Marek Vasut
On 9/10/24 5:24 PM, Frank Li wrote:
Hi,
purely nitpicking further below, one quick question right below.
Quoted 3 lines
This only resolves the issue when the PWM period is longer than 2us (or <500KHz) because write register is not quick enough when PWM period is very short.
Quoted 5 lines
You did mention the IPS bus is slow. Do I understand it correctly that the IPS bus write takes about 1us ? Because of the PWM consumes a sample every 2us and we need to write 2 samples to avoid FIFO underrun, then to safely write those 2 samples, we need to be able to write 1 sample per 1 us into the FIFO ?
The above time is just estimated, which variance at difference platform and impact by other IPs. If there are pending write/read from GPIO, PWM write have to wait for GPIO's write finish. It actually depend on IPS bus's loading.
<500Khz is very less possiblity that write slower than PWM's consumes.
Quoted 4 lines
Also, would writing more samples help with such "fast" use cases ? Something like this:
Quoted 9 lines
if (clkrate > 500000) {
// Thisusleep()could use some further improvement, e.g. calculate // precise delay for the FIFO to get empty based on PWM clkrateusleep(2 * 5); // wait 2us for each of the 4 samples in FIFO and a bit // Now the FIFO is surely empty, write all four FIFO slotswritel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
It can help at some possiblity, but still have problem if > 1Mhz, write will always less than consume.
If errata happen, only 1 cycle is full high. I think it is quite less impact at such high frequency.
We found this problem by observe a screen backlight flick when change ducty_cycle. I think we try fix it after a real user visible impact happen.
Put code here can reduce some possiblity at certain freq range, but may miss-leading user the problem fixed when > 500k.
Frank
Quoted 152 lines
} else { // clock rate less than 500 kHz
// Do the workaround below
if (duty_cycles<imx->duty_cycle&& val <MX3_PWMSR_FIFOAV_2WORDS... }Quoted 76 lines
Reproduce steps:
cd/sys/class/pwm/pwmchip1/pwm0
echo 2000000000 > period # It is easy to observe by using long period
echo 1000000000 >duty_cycle
echo 1 > enable
echo 800000000 >duty_cycle# One full high plus will be seen by scopeFixes 166091b1894d ("[ARM] MXC: add pwm driver for i.MX SoCs")Reviewed-by Jun Li <[email protected]>Signed-off-by Clark Wang <[email protected]>Signed-off-by Frank Li <[email protected]>---
Change from v4 to v5
- fix typo PMW & If
- usingimx->mmio_base+MX3_PWMSARChange from v3 to v4
- none, wrong bump version number
Change from v2 to v3
- simple workaround implement.
- add reproduce steps.Change from v1 to v2
- address comments in https://lore.kernel.org/linux-pwm/[email protected]/
About disable/enable pwm instead of disable/enable irq:
Some pmw periphal may sensitive to period. Disable/enable pwm will
increase period, althouhg it is okay for most case, such as LED backlight
or FAN speed. But some device such servo may require strict period.- address comments in https://lore.kernel.org/linux-pwm/[email protected]/
Using official errata number
fix typo 'filp'
add {} for elseI supposed fixed all previous issues, let me know if I missed one.
--- drivers/pwm/pwm-imx27.c | 67 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-)drivers/pwm/pwm-imx27.c +17
-1@@ -27,6 +27,7 @@#define MX3_PWMSR 0x04/* PWM Status Register */ #define MX3_PWMSAR 0x0C/* PWM Sample Register */ #define MX3_PWMPR 0x10/* PWM Period Register */ +#define MX3_PWMCNR 0x14/* PWM Counter Register */ #define MX3_PWMCR_FWM GENMASK(27, 26)#define MX3_PWMCR_STOPEN BIT(25)@@ -234,6 +235,8 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,struct pwm_imx27_chip *imx =to_pwm_imx27_chip (chip);unsigned long long c;unsigned long long clkrate; +unsigned long flags; +int val;int ret;u32 cr;@@ -274,7 +277,69 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,pwm_imx27_sw_reset (chip); } -writel (duty_cycles, imx->mmio_base +MX3_PWMSAR ); +/* +* This is a limited workaround. When the SAR FIFO is empty, the new +* write value will be directly applied to SAR even the current period +* is not over. +* +* ─────────────────────┐ +* PWM OUTPUT │ +* └───────────────────────── +* +* ┌──────────────────────────────────────────────┐ +* Counter │ XXXXXXXXXXXXXX │ +* └──────────────────────────────────────────────┘ +* ▲ ▲ +* │ │ drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 302
Quoted 10 lines
drivers/pwm/pwm-imx27.c +10
-0+* New SAR Old SAR +* +* XXXX Errata happen window +* +* If the new SAR value is less than the old one, and the counter is +* greater than the new SAR value (see above diagram XXXX), the current +* period will not flip the level. This will result in a pulse with a +* duty cycle of 100%. +* +* Check new sar less than old sar and current counter is in errata Better do 's@\<sar\>@SAR@g' so all the instances of 'sar' in the text are in uppercase. Currently, some are in lowercase and some are in uppercase.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 311
Quoted 8 lines
drivers/pwm/pwm-imx27.c +8
-0+* windows, write extra old sar into FIFO and new sar will effect at +* next period. +* +* Sometime period is quite long, such as over 1 second. If add old sar +* into FIFO unconditional, new sar have to wait for next period. It +* may be too long. +* +* Turn off the interrupt to ensure that not irq and schedule happen IRQ, in uppercase.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 319
Quoted 8 lines
drivers/pwm/pwm-imx27.c +8
-0+* during above operations. If any irq and schedule happen, counter +* in PWM will be out of data and take wrong action. +* +* Add a safety margin 1.5us because it needs some time to complete +* IO write. +* +* Use __raw_writel() to minimize the interval between two writes to +* the SAR register to increase the fastest pwm frequency supported. PWM, in uppercase.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 321
Quoted 1 line
drivers/pwm/pwm-imx27.c +1
-0+* When the PWM period is longer than 2us(or <500KHz), this workaround kHz, kilo lowercase, Hz Hertz uppercase H lowercase z .
Also fix in the commit message.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 331
Quoted 10 lines
drivers/pwm/pwm-imx27.c +10
-0+* can solve this problem. No software workaround is available if PWM +* period is shorter than IO write. +*/ + c = clkrate *1500 ; +do_div (c,NSEC_PER_SEC ); + +local_irq_save (flags); + val =FIELD_GET (MX3_PWMSR_FIFOAV ,readl_relaxed (imx->mmio_base +MX3_PWMSR )); +if (duty_cycles < imx->duty_cycle && val <MX3_PWMSR_FIFOAV_2WORDS ) { + val =readl_relaxed (imx->mmio_base +MX3_PWMCNR );I would put the comment below above this conditional statement.
drivers/pwm/pwm-imx27.c static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm, · line 343
Quoted 12 lines
drivers/pwm/pwm-imx27.c +11
-0+if ((val + c >= duty_cycles && val < imx->duty_cycle) || +/* +* If counter is close to period, controller may roll over +* when next IO write. +*/ + val + c >= period_cycles) +writel_relaxed (imx->duty_cycle, imx->mmio_base +MX3_PWMSAR ); + } +writel_relaxed (duty_cycles, imx->mmio_base +MX3_PWMSAR ); +local_irq_restore (flags); +writel (period_cycles, imx->mmio_base +MX3_PWMPR );The patch looks good, the above are just trivial nitpicks.
Thanks !
On 9/11/24 11:55 PM, Frank Li wrote:
Hi,
Quoted 31 lines
Quoted 9 lines
Quoted 3 lines
This only resolves the issue when the PWM period is longer than 2us (or <500KHz) because write register is not quick enough when PWM period is very short.
You did mention the IPS bus is slow. Do I understand it correctly that the IPS bus write takes about 1us ? Because of the PWM consumes a sample every 2us and we need to write 2 samples to avoid FIFO underrun, then to safely write those 2 samples, we need to be able to write 1 sample per 1 us into the FIFO ?
The above time is just estimated, which variance at difference platform and impact by other IPs. If there are pending write/read from GPIO, PWM write have to wait for GPIO's write finish. It actually depend on IPS bus's loading.
<500Khz is very less possiblity that write slower than PWM's consumes.
Quoted 4 lines
Also, would writing more samples help with such "fast" use cases ? Something like this:
Quoted 9 lines
if (clkrate > 500000) { // This usleep() could use some further improvement, e.g. calculate // precise delay for the FIFO to get empty based on PWM clkrate usleep(2 * 5); // wait 2us for each of the 4 samples in FIFO and a bit // Now the FIFO is surely empty, write all four FIFO slots writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR); writel_relaxed(imx->duty_cycle, imx->mmio_base + MX3_PWMSAR);
Quoted 8 lines
It can help at some possiblity, but still have problem if > 1Mhz, write will always less than consume.
If errata happen, only 1 cycle is full high. I think it is quite less impact at such high frequency.
We found this problem by observe a screen backlight flick when change
ducty_cycle. I think we try fix it after a real user visible impact happen.
Indeed, I observed similar problem.
Quoted 2 lines
Put code here can reduce some possiblity at certain freq range, but may miss-leading user the problem fixed when > 500k.
You already have good code comments, maybe expanding one would help clarify this issue cannot really be fully fixed with current hardware.
I think the multi-write for > 500 kHz could further improve the patch, but let's wait for input from others, let's see what they think.