imx-i2c frequency scaling issue imx6ull + edt-ft5x06

[Lucas Stach]

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?

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

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