imx-i2c frequency scaling issue imx6ull + edt-ft5x06

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

> 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

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