imx-i2c frequency scaling issue imx6ull + edt-ft5x06
Michael Nazzareno Trimarchi
michael at amarulasolutions.com
Thu Dec 3 14:14:59 EST 2020
Hi Lucas
On Thu, Dec 3, 2020 at 7:36 PM Michael Nazzareno Trimarchi
<michael at amarulasolutions.com> wrote:
>
> 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);
> +
I have tested but the correct threshold is the min freq for the cpu.
So we don't need
to split the core half of the min, but stay on min. Seems that it works
Michael
> 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
--
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