imx-i2c frequency scaling issue imx6ull + edt-ft5x06
Lucas Stach
l.stach at pengutronix.de
Thu Dec 3 05:01:40 EST 2020
Am Donnerstag, den 03.12.2020, 10:40 +0100 schrieb Michael Nazzareno Trimarchi:
Hi
On Thu, Dec 3, 2020 at 10:35 AM Lucas Stach <l.stach at pengutronix.de> wrote:
>
> Am Donnerstag, den 03.12.2020, 10:18 +0100 schrieb Michael Nazzareno Trimarchi:
> Hi
>
> On Thu, Dec 3, 2020 at 10:10 AM Lucas Stach <l.stach at pengutronix.de> wrote:
> >
> > Hi Michael,
> >
> > Am Mittwoch, den 02.12.2020, 23:13 +0100 schrieb Michael Nazzareno Trimarchi:
> > Hi
> >
> > On Wed, Dec 2, 2020 at 11:24 AM Michael Nazzareno Trimarchi
> > <michael at amarulasolutions.com> wrote:
> > >
> > > Hi all
> > >
> > > I'm trying to find the best way to fix a problem connected with the
> > > touch screen controller on i2c imx6ull bus. Now
> > > according to what I understand sdma can not be connected to the i2c
> > > peripheral so it works in pio mode and that's fine.
> > > During the frequency scaling a new clock register is computed but can
> > > not be applied to the register until the next start as
> > > reported by the datasheet. The problem here is that if the i2c
> > > transaction is around PRE_CHANGE and POST_CHANGE of the
> > > rate touchscreen starts to fail and swipe can not be performed until
> > > we get a pen up event (edf- has crc error). Is there any suggestion
> > > based on your experience? In the oscilloscope I can see this effect too
> > >
> > > Michael
> >
> > I get something like this at the moment. I think that it is needed
> > only in pio mode
> >
> > Tying cpufreq into the i2c driver seems like a layering violation and
> > not something we would like to do. If at all you need to do something
> > like this in the clk notifier.
> >
>
> The pio mode and the frequency scaling give the i2c shape a
> accordion effect. The touch controller has effect on it and have crc error.
>
> > However I first want to point out the elephant in the room: why is your
> > i2c controller clock affected by CPU frequency scaling? Is the
> > controller using a suboptimal clock source (using the same PLL as the
>
> Is not the pll of the i2c, that is fine I have checked but is really the cpufreq
> of the cpu. Check my second patch. If I avoid to go to lower one is ok but
> if I just use a fixed frequency it's fine too.
>
> So the i2c controller clock is fixed, but the transition latency of the
> CPUfreq scaling is causing the issue, as the CPU doesn't read/write the
> data register fast enough?
>
> In that case the right course of action would be to look into how to
> make the CPU frequency transition faster. Maybe you can use a step
> clock with a higher rate, to keep the CPU operating faster during the
> transition period?
Frequency transition depends on:
- trnasition latency that is cpu latency (fixed in dts)
- regulator trasnition latency that are depend on those are connected
to real hardware
ondemand select the new freq to go, what does it mean make the cpu faster?
Can you give me an example?
Clock and regulator latencies should not affect handling of the i2c PIO
data register, as the the cpufreq thread just sleeps while waiting for
clocks and regulators to stabilize.
So there are two possibilities I can see right now how cpufreq could
affect i2c PIO handling:
1. The CPU is running too slow during the period where it's on the step
clock (the clock that is used while the ARM PLL and regulators are
changed). If I read the clock driver correctly this should be a ~400MHz
clock, so it's quite unlikely to be a problem, but still worth checking
in the real system.
2. Something is disabling IRQs and/or preemption in the CPUfreq
transition, so i2c doesn't get handled anymore. As CPUfreq transitions
can take quite a bit of time, that would be very bad and would also
affect overall system performance. Maybe touchscreen i2c is just the
most sensitive part of your system and falls over first. ftrace has
some tracers to find sources of long IRQ or preempt disabled sections.
>
> > CPU, while it could be using a stable system PLL), or is the clocking
> > on the i.MX6ULL just so limited that peripherals must share a PLL with
> > the CPU? In the latter case I would argue that the CPU frequency
> > scaling should not touch the PLL rate and instead only use the divider
> > after the PLL to adjust CPU frequency.
> >
>
> I have sent a fix already on this same thread
>
> This isn't a fix, but a workaround for a very specific problem at best.
> The i2c controller driver shouldn't have to concern itself with any
> effects of CPU frequency scaling.
>
Yes that is not a fix but if some peripheral is effected by the frequncy scaling
can we have some api that ask explicitly to lock it during the frequency scaling
period and avoid to send i2c message?
It's an okay workaround, but before we opt for going with something
like that I would like to make sure we are not just papering over some
more fundamental issue with CPU frequency scaling that should be fixed
at the root.
Regards,
Lucas
Michael
> Michael
>
> > Regards,
> > Lucas
> >
> > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> > index d7267dd9c7bf..07511a8a79a6 100644
> > --- a/drivers/i2c/busses/i2c-imx.c
> > +++ b/drivers/i2c/busses/i2c-imx.c
> > @@ -31,6 +31,7 @@
> >
> > #include <linux/clk.h>
> > #include <linux/completion.h>
> > +#include <linux/cpufreq.h>
> > #include <linux/delay.h>
> > #include <linux/dma-mapping.h>
> > #include <linux/dmaengine.h>
> > @@ -210,6 +211,9 @@ struct imx_i2c_struct {
> > struct pinctrl_state *pinctrl_pins_default;
> > struct pinctrl_state *pinctrl_pins_gpio;
> >
> > +#ifdef CONFIG_CPU_FREQ
> > + struct notifier_block freq_transition;
> > +#endif
> > struct imx_i2c_dma *dma;
> > };
> >
> > @@ -510,6 +514,51 @@ static void i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx,
> > #endif
> > }
> >
> > +#ifdef CONFIG_CPU_FREQ
> > +static int i2c_imx_cpufreq_transition(struct notifier_block *nb,
> > + unsigned long action, void *data)
> > +{
> > + struct imx_i2c_struct *i2c_imx;
> > + i2c_imx = container_of(nb, struct imx_i2c_struct, freq_transition);
> > +
> > + switch (action) {
> > + case CPUFREQ_PRECHANGE:
> > + i2c_lock_bus(&i2c_imx->adapter, I2C_LOCK_ROOT_ADAPTER);
> > + break;
> > + case CPUFREQ_POSTCHANGE:
> > + i2c_unlock_bus(&i2c_imx->adapter, I2C_LOCK_ROOT_ADAPTER);
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static inline int i2c_imx_cpufreq_register(struct imx_i2c_struct *dev)
> > +{
> > + dev->freq_transition.notifier_call = i2c_imx_cpufreq_transition;
> > +
> > + return cpufreq_register_notifier(&dev->freq_transition,
> > + CPUFREQ_TRANSITION_NOTIFIER);
> > +}
> > +
> > +static inline void i2c_imx_cpufreq_deregister(struct imx_i2c_struct *dev)
> > +{
> > + cpufreq_unregister_notifier(&dev->freq_transition,
> > + CPUFREQ_TRANSITION_NOTIFIER);
> > +}
> > +#else
> > +static inline int i2c_imx_cpufreq_register(struct imx_i2c_struct *dev)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline void i2c_imx_cpufreq_deregister(struct imx_i2c_struct *dev)
> > +{
> > +}
> > +#endif
> > +
> > static int i2c_imx_clk_notifier_call(struct notifier_block *nb,
> > unsigned long action, void *data)
> > {
> > @@ -1172,6 +1221,12 @@ static int i2c_imx_probe(struct platform_device *pdev)
> > i2c_imx->adapter.name);
> > dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
> >
> > + ret = i2c_imx_cpufreq_register(i2c_imx);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to register cpufreq\n");
> > + goto clk_notifier_unregister;
> > + }
> > +
> > /* Init DMA config if supported */
> > i2c_imx_dma_request(i2c_imx, phy_addr);
> >
> > @@ -1199,6 +1254,8 @@ static int i2c_imx_remove(struct platform_device *pdev)
> > if (ret < 0)
> > return ret;
> >
> > + i2c_imx_cpufreq_deregister(i2c_imx);
> > +
> > /* remove adapter */
> > dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
> > i2c_del_adapter(&i2c_imx->adapter);
> > --
> > 2.25.1
> >
> >
> >
> >
> >
>
>
>
>
More information about the linux-arm-kernel
mailing list