| Message ID | 20211005003216.2670632-4-minyard@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c:imx: Deliver a timely stop on slave side, fix recv | expand |
On Mon, Oct 04, 2021 at 07:32:16PM -0500, minyard@acm.org wrote: > From: Corey Minyard <cminyard@mvista.com> > > The timer is too slow and significantly reduces performance. Use an > hrtimer to get things working faster. > > Signed-off-by: Corey Minyard <minyard@acm.org> > Tested-by: Andrew Manley <andrew.manley@sealingtech.com> > Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com> > --- > drivers/i2c/busses/i2c-imx.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 26a04dc0590b..4b0e9d1784dd 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -38,7 +38,7 @@ > #include <linux/iopoll.h> > #include <linux/kernel.h> > #include <linux/spinlock.h> > -#include <linux/timer.h> > +#include <linux/hrtimer.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > @@ -53,6 +53,8 @@ > /* This will be the driver name the kernel reports */ > #define DRIVER_NAME "imx-i2c" > > +#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */ > + > /* > * Enable DMA if transfer byte size is bigger than this threshold. > * As the hardware request, it must bigger than 4 bytes.\ > @@ -214,8 +216,8 @@ struct imx_i2c_struct { > enum i2c_slave_event last_slave_event; > > /* For checking slave events. */ > - spinlock_t slave_lock; > - struct timer_list slave_timer; > + spinlock_t slave_lock; > + struct hrtimer slave_timer; This is unrelated to this patch, moreover it was introduced only in patch 1. > }; > > static const struct imx_i2c_hwdata imx1_i2c_hwdata = { > @@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx, > } > > out: > - mod_timer(&i2c_imx->slave_timer, jiffies + 1); > + hrtimer_try_to_cancel(&i2c_imx->slave_timer); Don't you need to check the return value here? > + hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY); > + hrtimer_restart(&i2c_imx->slave_timer); > return IRQ_HANDLED; > } > Best regards Uwe
On Tue, Nov 02, 2021 at 09:58:06AM +0100, Uwe Kleine-König wrote: > On Mon, Oct 04, 2021 at 07:32:16PM -0500, minyard@acm.org wrote: > > From: Corey Minyard <cminyard@mvista.com> > > > > The timer is too slow and significantly reduces performance. Use an > > hrtimer to get things working faster. > > > > Signed-off-by: Corey Minyard <minyard@acm.org> > > Tested-by: Andrew Manley <andrew.manley@sealingtech.com> > > Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com> > > --- > > drivers/i2c/busses/i2c-imx.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > > index 26a04dc0590b..4b0e9d1784dd 100644 > > --- a/drivers/i2c/busses/i2c-imx.c > > +++ b/drivers/i2c/busses/i2c-imx.c > > @@ -38,7 +38,7 @@ > > #include <linux/iopoll.h> > > #include <linux/kernel.h> > > #include <linux/spinlock.h> > > -#include <linux/timer.h> > > +#include <linux/hrtimer.h> > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/of_device.h> > > @@ -53,6 +53,8 @@ > > /* This will be the driver name the kernel reports */ > > #define DRIVER_NAME "imx-i2c" > > > > +#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */ > > + > > /* > > * Enable DMA if transfer byte size is bigger than this threshold. > > * As the hardware request, it must bigger than 4 bytes.\ > > @@ -214,8 +216,8 @@ struct imx_i2c_struct { > > enum i2c_slave_event last_slave_event; > > > > /* For checking slave events. */ > > - spinlock_t slave_lock; > > - struct timer_list slave_timer; > > + spinlock_t slave_lock; > > + struct hrtimer slave_timer; > > This is unrelated to this patch, moreover it was introduced only in > patch 1. The second line is important for this patch, of course. I assume you mean the indention of the first line, which is just keeping things lined up. > > > }; > > > > static const struct imx_i2c_hwdata imx1_i2c_hwdata = { > > @@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx, > > } > > > > out: > > - mod_timer(&i2c_imx->slave_timer, jiffies + 1); > > + hrtimer_try_to_cancel(&i2c_imx->slave_timer); > > Don't you need to check the return value here? Not really. The possible return values are: * * 0 when the timer was not active * * 1 when the timer was active * * -1 when the timer is currently executing the callback function and * cannot be stopped and if it returns 0 or 1, then everything is fine. If it returns -1, then the code will still work, though it may be redone (or already have been done) by the timer function. So it doesn't matter. Maybe I should add a comment about this? Thanks for reviewing. -corey > > > + hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY); > > + hrtimer_restart(&i2c_imx->slave_timer); > > return IRQ_HANDLED; > > } > > > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Oct 04, 2021 at 07:32:16PM -0500, minyard@acm.org wrote: > From: Corey Minyard <cminyard@mvista.com> > > The timer is too slow and significantly reduces performance. Use an > hrtimer to get things working faster. > > Signed-off-by: Corey Minyard <minyard@acm.org> > Tested-by: Andrew Manley <andrew.manley@sealingtech.com> > Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com> Do we need to keep this change history? If no, please merge it to the first patch. > --- > drivers/i2c/busses/i2c-imx.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > index 26a04dc0590b..4b0e9d1784dd 100644 > --- a/drivers/i2c/busses/i2c-imx.c > +++ b/drivers/i2c/busses/i2c-imx.c > @@ -38,7 +38,7 @@ > #include <linux/iopoll.h> > #include <linux/kernel.h> > #include <linux/spinlock.h> > -#include <linux/timer.h> > +#include <linux/hrtimer.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > @@ -53,6 +53,8 @@ > /* This will be the driver name the kernel reports */ > #define DRIVER_NAME "imx-i2c" > > +#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */ > + > /* > * Enable DMA if transfer byte size is bigger than this threshold. > * As the hardware request, it must bigger than 4 bytes.\ > @@ -214,8 +216,8 @@ struct imx_i2c_struct { > enum i2c_slave_event last_slave_event; > > /* For checking slave events. */ > - spinlock_t slave_lock; > - struct timer_list slave_timer; > + spinlock_t slave_lock; > + struct hrtimer slave_timer; > }; > > static const struct imx_i2c_hwdata imx1_i2c_hwdata = { > @@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx, > } > > out: > - mod_timer(&i2c_imx->slave_timer, jiffies + 1); > + hrtimer_try_to_cancel(&i2c_imx->slave_timer); > + hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY); > + hrtimer_restart(&i2c_imx->slave_timer); > return IRQ_HANDLED; > } > > -static void i2c_imx_slave_timeout(struct timer_list *t) > +static enum hrtimer_restart i2c_imx_slave_timeout(struct hrtimer *t) > { > - struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer); > + struct imx_i2c_struct *i2c_imx = container_of(t, struct imx_i2c_struct, > + slave_timer); > unsigned int ctl, status; > unsigned long flags; > > @@ -798,6 +803,7 @@ static void i2c_imx_slave_timeout(struct timer_list *t) > ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > i2c_imx_slave_handle(i2c_imx, status, ctl); > spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); > + return HRTIMER_NORESTART; > } > > static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx) > @@ -1423,7 +1429,8 @@ static int i2c_imx_probe(struct platform_device *pdev) > return -ENOMEM; > > spin_lock_init(&i2c_imx->slave_lock); > - timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0); > + hrtimer_init(&i2c_imx->slave_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > + i2c_imx->slave_timer.function = i2c_imx_slave_timeout; > > match = device_get_match_data(&pdev->dev); > if (match) > @@ -1538,7 +1545,7 @@ static int i2c_imx_remove(struct platform_device *pdev) > if (ret < 0) > return ret; > > - del_timer_sync(&i2c_imx->slave_timer); > + hrtimer_cancel(&i2c_imx->slave_timer); > > /* remove adapter */ > dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); > -- > 2.25.1 > >
On Wed, Nov 10, 2021 at 10:03:38AM +0100, Oleksij Rempel wrote: > On Mon, Oct 04, 2021 at 07:32:16PM -0500, minyard@acm.org wrote: > > From: Corey Minyard <cminyard@mvista.com> > > > > The timer is too slow and significantly reduces performance. Use an > > hrtimer to get things working faster. > > > > Signed-off-by: Corey Minyard <minyard@acm.org> > > Tested-by: Andrew Manley <andrew.manley@sealingtech.com> > > Reviewed-by: Andrew Manley <andrew.manley@sealingtech.com> > > Do we need to keep this change history? If no, please merge it to the > first patch. Yeah, I can do that. It make sense. Thanks, -corey > > > --- > > drivers/i2c/busses/i2c-imx.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c > > index 26a04dc0590b..4b0e9d1784dd 100644 > > --- a/drivers/i2c/busses/i2c-imx.c > > +++ b/drivers/i2c/busses/i2c-imx.c > > @@ -38,7 +38,7 @@ > > #include <linux/iopoll.h> > > #include <linux/kernel.h> > > #include <linux/spinlock.h> > > -#include <linux/timer.h> > > +#include <linux/hrtimer.h> > > #include <linux/module.h> > > #include <linux/of.h> > > #include <linux/of_device.h> > > @@ -53,6 +53,8 @@ > > /* This will be the driver name the kernel reports */ > > #define DRIVER_NAME "imx-i2c" > > > > +#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */ > > + > > /* > > * Enable DMA if transfer byte size is bigger than this threshold. > > * As the hardware request, it must bigger than 4 bytes.\ > > @@ -214,8 +216,8 @@ struct imx_i2c_struct { > > enum i2c_slave_event last_slave_event; > > > > /* For checking slave events. */ > > - spinlock_t slave_lock; > > - struct timer_list slave_timer; > > + spinlock_t slave_lock; > > + struct hrtimer slave_timer; > > }; > > > > static const struct imx_i2c_hwdata imx1_i2c_hwdata = { > > @@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx, > > } > > > > out: > > - mod_timer(&i2c_imx->slave_timer, jiffies + 1); > > + hrtimer_try_to_cancel(&i2c_imx->slave_timer); > > + hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY); > > + hrtimer_restart(&i2c_imx->slave_timer); > > return IRQ_HANDLED; > > } > > > > -static void i2c_imx_slave_timeout(struct timer_list *t) > > +static enum hrtimer_restart i2c_imx_slave_timeout(struct hrtimer *t) > > { > > - struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer); > > + struct imx_i2c_struct *i2c_imx = container_of(t, struct imx_i2c_struct, > > + slave_timer); > > unsigned int ctl, status; > > unsigned long flags; > > > > @@ -798,6 +803,7 @@ static void i2c_imx_slave_timeout(struct timer_list *t) > > ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); > > i2c_imx_slave_handle(i2c_imx, status, ctl); > > spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); > > + return HRTIMER_NORESTART; > > } > > > > static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx) > > @@ -1423,7 +1429,8 @@ static int i2c_imx_probe(struct platform_device *pdev) > > return -ENOMEM; > > > > spin_lock_init(&i2c_imx->slave_lock); > > - timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0); > > + hrtimer_init(&i2c_imx->slave_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); > > + i2c_imx->slave_timer.function = i2c_imx_slave_timeout; > > > > match = device_get_match_data(&pdev->dev); > > if (match) > > @@ -1538,7 +1545,7 @@ static int i2c_imx_remove(struct platform_device *pdev) > > if (ret < 0) > > return ret; > > > > - del_timer_sync(&i2c_imx->slave_timer); > > + hrtimer_cancel(&i2c_imx->slave_timer); > > > > /* remove adapter */ > > dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n"); > > -- > > 2.25.1 > > > > > > -- > Pengutronix e.K. | | > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 26a04dc0590b..4b0e9d1784dd 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -38,7 +38,7 @@ #include <linux/iopoll.h> #include <linux/kernel.h> #include <linux/spinlock.h> -#include <linux/timer.h> +#include <linux/hrtimer.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> @@ -53,6 +53,8 @@ /* This will be the driver name the kernel reports */ #define DRIVER_NAME "imx-i2c" +#define I2C_IMX_CHECK_DELAY 30000 /* Time to check for bus idle, in NS */ + /* * Enable DMA if transfer byte size is bigger than this threshold. * As the hardware request, it must bigger than 4 bytes.\ @@ -214,8 +216,8 @@ struct imx_i2c_struct { enum i2c_slave_event last_slave_event; /* For checking slave events. */ - spinlock_t slave_lock; - struct timer_list slave_timer; + spinlock_t slave_lock; + struct hrtimer slave_timer; }; static const struct imx_i2c_hwdata imx1_i2c_hwdata = { @@ -783,13 +785,16 @@ static irqreturn_t i2c_imx_slave_handle(struct imx_i2c_struct *i2c_imx, } out: - mod_timer(&i2c_imx->slave_timer, jiffies + 1); + hrtimer_try_to_cancel(&i2c_imx->slave_timer); + hrtimer_forward_now(&i2c_imx->slave_timer, I2C_IMX_CHECK_DELAY); + hrtimer_restart(&i2c_imx->slave_timer); return IRQ_HANDLED; } -static void i2c_imx_slave_timeout(struct timer_list *t) +static enum hrtimer_restart i2c_imx_slave_timeout(struct hrtimer *t) { - struct imx_i2c_struct *i2c_imx = from_timer(i2c_imx, t, slave_timer); + struct imx_i2c_struct *i2c_imx = container_of(t, struct imx_i2c_struct, + slave_timer); unsigned int ctl, status; unsigned long flags; @@ -798,6 +803,7 @@ static void i2c_imx_slave_timeout(struct timer_list *t) ctl = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR); i2c_imx_slave_handle(i2c_imx, status, ctl); spin_unlock_irqrestore(&i2c_imx->slave_lock, flags); + return HRTIMER_NORESTART; } static void i2c_imx_slave_init(struct imx_i2c_struct *i2c_imx) @@ -1423,7 +1429,8 @@ static int i2c_imx_probe(struct platform_device *pdev) return -ENOMEM; spin_lock_init(&i2c_imx->slave_lock); - timer_setup(&i2c_imx->slave_timer, i2c_imx_slave_timeout, 0); + hrtimer_init(&i2c_imx->slave_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS); + i2c_imx->slave_timer.function = i2c_imx_slave_timeout; match = device_get_match_data(&pdev->dev); if (match) @@ -1538,7 +1545,7 @@ static int i2c_imx_remove(struct platform_device *pdev) if (ret < 0) return ret; - del_timer_sync(&i2c_imx->slave_timer); + hrtimer_cancel(&i2c_imx->slave_timer); /* remove adapter */ dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");