imx-i2c frequency scaling issue imx6ull + edt-ft5x06
Michael Nazzareno Trimarchi
michael at amarulasolutions.com
Thu Dec 3 13:36:14 EST 2020
Hi
On Thu, Dec 3, 2020 at 11:01 AM Lucas Stach <l.stach at pengutronix.de> wrote:
>
> 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.
I vote for this one. What do you think?
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index ee805170d6d6..b9427cbbb556 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -127,7 +127,9 @@ static int imx6q_set_target(struct cpufreq_policy
*policy, unsigned int index)
* voltage of 528MHz, so lower the CPU frequency to one
* half before changing CPU frequency.
*/
- clk_set_rate(clks[ARM].clk, (old_freq >> 1) * 1000);
+ if (old_freq >= 528000)
+ clk_set_rate(clks[ARM].clk, (old_freq >> 1) * 1000);
+
clk_set_parent(clks[PLL1_SW].clk, clks[PLL1_SYS].clk);
if (freq_hz > clk_get_rate(clks[PLL2_PFD2_396M].clk))
clk_set_parent(clks[SECONDARY_SEL].clk,
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
I'm testing this one
Michael
>
> 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
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
>
>
>
>
--
Michael Nazzareno Trimarchi
Amarula Solutions BV
COO Co-Founder
Cruquiuskade 47 Amsterdam 1018 AM NL
T. +31(0)851119172
M. +39(0)3479132170
[`as] https://www.amarulasolutions.com
More information about the linux-arm-kernel
mailing list