Re: imx-i2c frequency scaling issue imx6ull + edt-ft5x06

[Lucas Stach <l.stach@pengutronix.de>]

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

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

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






_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel