* imx-i2c frequency scaling issue imx6ull + edt-ft5x06 @ 2020-12-02 10:24 Michael Nazzareno Trimarchi 2020-12-02 22:13 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 9+ messages in thread From: Michael Nazzareno Trimarchi @ 2020-12-02 10:24 UTC (permalink / raw) To: linux-arm-kernel, Wolfram Sang; +Cc: Lucas Stach 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: imx-i2c frequency scaling issue imx6ull + edt-ft5x06 2020-12-02 10:24 imx-i2c frequency scaling issue imx6ull + edt-ft5x06 Michael Nazzareno Trimarchi @ 2020-12-02 22:13 ` Michael Nazzareno Trimarchi 2020-12-03 9:10 ` Lucas Stach 0 siblings, 1 reply; 9+ messages in thread From: Michael Nazzareno Trimarchi @ 2020-12-02 22:13 UTC (permalink / raw) To: linux-arm-kernel, Wolfram Sang; +Cc: Lucas Stach 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 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: imx-i2c frequency scaling issue imx6ull + edt-ft5x06 2020-12-02 22:13 ` Michael Nazzareno Trimarchi @ 2020-12-03 9:10 ` Lucas Stach 2020-12-03 9:18 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 9+ messages in thread From: Lucas Stach @ 2020-12-03 9:10 UTC (permalink / raw) To: Michael Nazzareno Trimarchi, linux-arm-kernel, Wolfram Sang 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: imx-i2c frequency scaling issue imx6ull + edt-ft5x06 2020-12-03 9:10 ` Lucas Stach @ 2020-12-03 9:18 ` Michael Nazzareno Trimarchi 2020-12-03 9:35 ` Lucas Stach 0 siblings, 1 reply; 9+ messages in thread From: Michael Nazzareno Trimarchi @ 2020-12-03 9:18 UTC (permalink / raw) To: Lucas Stach; +Cc: linux-arm-kernel, Wolfram Sang Hi On Thu, Dec 3, 2020 at 10:10 AM Lucas Stach <l.stach@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@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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: imx-i2c frequency scaling issue imx6ull + edt-ft5x06 2020-12-03 9:18 ` Michael Nazzareno Trimarchi @ 2020-12-03 9:35 ` Lucas Stach 2020-12-03 9:40 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 9+ messages in thread From: Lucas Stach @ 2020-12-03 9:35 UTC (permalink / raw) To: Michael Nazzareno Trimarchi; +Cc: linux-arm-kernel, Wolfram Sang 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@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@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 > > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: imx-i2c frequency scaling issue imx6ull + edt-ft5x06 2020-12-03 9:35 ` Lucas Stach @ 2020-12-03 9:40 ` Michael Nazzareno Trimarchi 2020-12-03 10:01 ` Lucas Stach 0 siblings, 1 reply; 9+ messages in thread From: Michael Nazzareno Trimarchi @ 2020-12-03 9:40 UTC (permalink / raw) To: Lucas Stach; +Cc: linux-arm-kernel, Wolfram Sang Hi On Thu, Dec 3, 2020 at 10:35 AM Lucas Stach <l.stach@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@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@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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: imx-i2c frequency scaling issue imx6ull + edt-ft5x06 2020-12-03 9:40 ` Michael Nazzareno Trimarchi @ 2020-12-03 10:01 ` Lucas Stach 2020-12-03 18:36 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 9+ messages in thread From: Lucas Stach @ 2020-12-03 10:01 UTC (permalink / raw) To: Michael Nazzareno Trimarchi; +Cc: linux-arm-kernel, Wolfram Sang 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@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@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@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. 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 > > > > > > > > > > > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: imx-i2c frequency scaling issue imx6ull + edt-ft5x06 2020-12-03 10:01 ` Lucas Stach @ 2020-12-03 18:36 ` Michael Nazzareno Trimarchi 2020-12-03 19:14 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 9+ messages in thread From: Michael Nazzareno Trimarchi @ 2020-12-03 18:36 UTC (permalink / raw) To: Lucas Stach; +Cc: linux-arm-kernel, Wolfram Sang Hi On Thu, Dec 3, 2020 at 11:01 AM Lucas Stach <l.stach@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@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@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@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); + 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: imx-i2c frequency scaling issue imx6ull + edt-ft5x06 2020-12-03 18:36 ` Michael Nazzareno Trimarchi @ 2020-12-03 19:14 ` Michael Nazzareno Trimarchi 0 siblings, 0 replies; 9+ messages in thread From: Michael Nazzareno Trimarchi @ 2020-12-03 19:14 UTC (permalink / raw) To: Lucas Stach; +Cc: linux-arm-kernel, Wolfram Sang Hi Lucas On Thu, Dec 3, 2020 at 7:36 PM Michael Nazzareno Trimarchi <michael@amarulasolutions.com> wrote: > > Hi > > On Thu, Dec 3, 2020 at 11:01 AM Lucas Stach <l.stach@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@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@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@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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-12-03 19:16 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-12-02 10:24 imx-i2c frequency scaling issue imx6ull + edt-ft5x06 Michael Nazzareno Trimarchi 2020-12-02 22:13 ` Michael Nazzareno Trimarchi 2020-12-03 9:10 ` Lucas Stach 2020-12-03 9:18 ` Michael Nazzareno Trimarchi 2020-12-03 9:35 ` Lucas Stach 2020-12-03 9:40 ` Michael Nazzareno Trimarchi 2020-12-03 10:01 ` Lucas Stach 2020-12-03 18:36 ` Michael Nazzareno Trimarchi 2020-12-03 19:14 ` Michael Nazzareno Trimarchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).