imx-i2c frequency scaling issue imx6ull + edt-ft5x06

[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?

>
> > 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?

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