| Message ID | 20180420172408.26164-1-sebastian.reichel@collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Sebastian, On Fri, 2018-04-20 at 19:24 +0200, Sebastian Reichel wrote: > The current reset-gpio support triggers an interrupt storm on platforms > using the maxtouch with level based interrupt. The Motorola Droid 4, > which I used for some of the tests is not affected, since it uses a level > based interrupt. > I found this confusing. Interrupt storm happen with level-based interrupts, but the Droid4 is not affected? > This change avoids the interrupt storm by enabling the device while > its interrupt is disabled. The following mxt_initialize() requires, > that the device is responsive (at least mxt224E is unresponsive for > ~22ms), so we wait some time. We don't wait for leaving bootloader > mode anymore, since mxt_initialize() checks for it anyways. > IMHO, having some more or less arbritrary sleeps is almost always a problem. This value might be enough for some platform, might be too short for some other, and then it might get too large for someone else. > This fixes a boot problem on GE PPD (watchdog kills device) and also > has been tested on the Droid 4. > > Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > --- > drivers/input/touchscreen/atmel_mxt_ts.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > index 5d9699fe1b55..f8a9f2a47e78 100644 > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > @@ -194,6 +194,7 @@ enum t100_type { > > /* Delay times */ > #define MXT_BACKUP_TIME 50 /* msec */ > +#define MXT_RESET_GPIO_TIME 20 /* msec */ > #define MXT_RESET_TIME 200 /* msec */ > #define MXT_RESET_TIMEOUT 3000 /* msec */ > #define MXT_CRC_TIMEOUT 1000 /* msec */ > @@ -3167,20 +3168,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) > return error; > } > > + disable_irq(client->irq); > + > if (data->reset_gpio) { > - data->in_bootloader = true; > - msleep(MXT_RESET_TIME); > - reinit_completion(&data->bl_completion); > + msleep(MXT_RESET_GPIO_TIME); > gpiod_set_value(data->reset_gpio, 1); Can't we enable the IRQ here, just before the wait... > - error = mxt_wait_for_completion(data, &data->bl_completion, > - MXT_RESET_TIMEOUT); ... and then disable it back afterwards? > - if (error) > - return error; > - data->in_bootloader = false; > + msleep(MXT_RESET_TIME); > } > > - disable_irq(client->irq); > - > error = mxt_initialize(data); > if (error) > return error; > -- > 2.17.0 > > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Apr 20, 2018 at 02:44:02PM -0300, Ezequiel Garcia wrote: > Hi Sebastian, > > On Fri, 2018-04-20 at 19:24 +0200, Sebastian Reichel wrote: > > The current reset-gpio support triggers an interrupt storm on platforms > > using the maxtouch with level based interrupt. The Motorola Droid 4, > > which I used for some of the tests is not affected, since it uses a level > > based interrupt. > > > > I found this confusing. Interrupt storm happen with level-based interrupts, > but the Droid4 is not affected? This is a typo. Droid 4 has an edge based interrupt defined, PPD has an level based interrupt defined. PPD got an interrupt storm, Droid 4 did not. > > This change avoids the interrupt storm by enabling the device while > > its interrupt is disabled. The following mxt_initialize() requires, > > that the device is responsive (at least mxt224E is unresponsive for > > ~22ms), so we wait some time. We don't wait for leaving bootloader > > mode anymore, since mxt_initialize() checks for it anyways. > > > > IMHO, having some more or less arbritrary sleeps is almost > always a problem. This value might be enough for some platform, > might be too short for some other, and then it might get too large > for someone else. The 22ms chip-being-unresponsive are not newly introduced. The same 22ms are also required for soft-reset. I did introduce a new time (MXT_RESET_GPIO_TIME) for the "chip being reset" state, since my randomly chosen 200ms from before were exaggerated considering all mxt datasheets I checked stated only a few nano seconds. > > This fixes a boot problem on GE PPD (watchdog kills device) and also > > has been tested on the Droid 4. > > > > Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > --- > > drivers/input/touchscreen/atmel_mxt_ts.c | 15 +++++---------- > > 1 file changed, 5 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c > > index 5d9699fe1b55..f8a9f2a47e78 100644 > > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > > @@ -194,6 +194,7 @@ enum t100_type { > > > > /* Delay times */ > > #define MXT_BACKUP_TIME 50 /* msec */ > > +#define MXT_RESET_GPIO_TIME 20 /* msec */ > > #define MXT_RESET_TIME 200 /* msec */ > > #define MXT_RESET_TIMEOUT 3000 /* msec */ > > #define MXT_CRC_TIMEOUT 1000 /* msec */ > > @@ -3167,20 +3168,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) > > return error; > > } > > > > + disable_irq(client->irq); > > + > > if (data->reset_gpio) { > > - data->in_bootloader = true; > > - msleep(MXT_RESET_TIME); > > - reinit_completion(&data->bl_completion); > > + msleep(MXT_RESET_GPIO_TIME); > > gpiod_set_value(data->reset_gpio, 1); > > Can't we enable the IRQ here, just before the wait... > > > - error = mxt_wait_for_completion(data, &data->bl_completion, > > - MXT_RESET_TIMEOUT); > > ... and then disable it back afterwards? Yes and no. We need the wait, since the interrupt pin does random stuff for 100ms after reset. We can do the wait-for-completion after that time (see mxt_soft_reset function). I kept it simple, since the following init does another soft reset anyways. > > - if (error) > > - return error; > > - data->in_bootloader = false; > > + msleep(MXT_RESET_TIME); > > > } > > > > - disable_irq(client->irq); > > - > > error = mxt_initialize(data); > > if (error) > > return error; -- Sebastian
On Fri, Apr 20, 2018 at 09:42:07PM +0200, Sebastian Reichel wrote: > On Fri, Apr 20, 2018 at 02:44:02PM -0300, Ezequiel Garcia wrote: > > Hi Sebastian, > > > > On Fri, 2018-04-20 at 19:24 +0200, Sebastian Reichel wrote: > > > The current reset-gpio support triggers an interrupt storm on platforms > > > using the maxtouch with level based interrupt. The Motorola Droid 4, > > > which I used for some of the tests is not affected, since it uses a level > > > based interrupt. > > > > > > > I found this confusing. Interrupt storm happen with level-based interrupts, > > but the Droid4 is not affected? Can I ask what happens during the interrupt storm. Are you getting lots of the "failed to read T44 and T5" message, or something else? > > > This change avoids the interrupt storm by enabling the device while > > > its interrupt is disabled. The following mxt_initialize() requires, > > > that the device is responsive (at least mxt224E is unresponsive for > > > ~22ms), so we wait some time. We don't wait for leaving bootloader > > > mode anymore, since mxt_initialize() checks for it anyways. > > > > > > > IMHO, having some more or less arbritrary sleeps is almost > > always a problem. This value might be enough for some platform, > > might be too short for some other, and then it might get too large > > for someone else. > > The 22ms chip-being-unresponsive are not newly introduced. The > same 22ms are also required for soft-reset. I did introduce a > new time (MXT_RESET_GPIO_TIME) for the "chip being reset" state, > since my randomly chosen 200ms from before were exaggerated > considering all mxt datasheets I checked stated only a few nano > seconds. According to the data sheets there is a period after a reset where the CHG line is temporarily set as an input, during which the host should ignore it. If you don't, you might get a stray interrupt and try and communicate with the device, which might leave it in a bad state. I think you mentioned that later in your email. The reset time varies per chip, but the 100ms in mxt_soft_reset() was based on discussions with app support at Atmel, so should be correct in most cases. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Sat, Apr 21, 2018 at 09:11:37PM +0100, Nick Dyer wrote: > On Fri, Apr 20, 2018 at 09:42:07PM +0200, Sebastian Reichel wrote: > > On Fri, Apr 20, 2018 at 02:44:02PM -0300, Ezequiel Garcia wrote: > > > Hi Sebastian, > > > > > > On Fri, 2018-04-20 at 19:24 +0200, Sebastian Reichel wrote: > > > > The current reset-gpio support triggers an interrupt storm on platforms > > > > using the maxtouch with level based interrupt. The Motorola Droid 4, > > > > which I used for some of the tests is not affected, since it uses a level > > > > based interrupt. > > > > > > > > > > I found this confusing. Interrupt storm happen with level-based interrupts, > > > but the Droid4 is not affected? > > Can I ask what happens during the interrupt storm. Are you getting lots > of the "failed to read T44 and T5" message, or something else? I don't get any message (with DEBUG enabled for the driver). The boot process hangs and after a few seconds the watchdog kicks in. > > > > This change avoids the interrupt storm by enabling the device while > > > > its interrupt is disabled. The following mxt_initialize() requires, > > > > that the device is responsive (at least mxt224E is unresponsive for > > > > ~22ms), so we wait some time. We don't wait for leaving bootloader > > > > mode anymore, since mxt_initialize() checks for it anyways. > > > > > > > > > > IMHO, having some more or less arbritrary sleeps is almost > > > always a problem. This value might be enough for some platform, > > > might be too short for some other, and then it might get too large > > > for someone else. > > > > The 22ms chip-being-unresponsive are not newly introduced. The > > same 22ms are also required for soft-reset. I did introduce a > > new time (MXT_RESET_GPIO_TIME) for the "chip being reset" state, > > since my randomly chosen 200ms from before were exaggerated > > considering all mxt datasheets I checked stated only a few nano > > seconds. > > According to the data sheets there is a period after a reset where the > CHG line is temporarily set as an input, during which the host should > ignore it. If you don't, you might get a stray interrupt and try and > communicate with the device, which might leave it in a bad state. I > think you mentioned that later in your email. > > The reset time varies per chip, but the 100ms in mxt_soft_reset() was > based on discussions with app support at Atmel, so should be correct in > most cases. Right. This sleep time is missing for the gpio based reset. The other checks are not required, since the chip initialization routine is called directly afterwards. I will send a PATCHv2, that fixes the patch description and uses the same timeout value for both resets. -- Sebastian
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 5d9699fe1b55..f8a9f2a47e78 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -194,6 +194,7 @@ enum t100_type { /* Delay times */ #define MXT_BACKUP_TIME 50 /* msec */ +#define MXT_RESET_GPIO_TIME 20 /* msec */ #define MXT_RESET_TIME 200 /* msec */ #define MXT_RESET_TIMEOUT 3000 /* msec */ #define MXT_CRC_TIMEOUT 1000 /* msec */ @@ -3167,20 +3168,14 @@ static int mxt_probe(struct i2c_client *client, const struct i2c_device_id *id) return error; } + disable_irq(client->irq); + if (data->reset_gpio) { - data->in_bootloader = true; - msleep(MXT_RESET_TIME); - reinit_completion(&data->bl_completion); + msleep(MXT_RESET_GPIO_TIME); gpiod_set_value(data->reset_gpio, 1); - error = mxt_wait_for_completion(data, &data->bl_completion, - MXT_RESET_TIMEOUT); - if (error) - return error; - data->in_bootloader = false; + msleep(MXT_RESET_TIME); } - disable_irq(client->irq); - error = mxt_initialize(data); if (error) return error;
The current reset-gpio support triggers an interrupt storm on platforms using the maxtouch with level based interrupt. The Motorola Droid 4, which I used for some of the tests is not affected, since it uses a level based interrupt. This change avoids the interrupt storm by enabling the device while its interrupt is disabled. The following mxt_initialize() requires, that the device is responsive (at least mxt224E is unresponsive for ~22ms), so we wait some time. We don't wait for leaving bootloader mode anymore, since mxt_initialize() checks for it anyways. This fixes a boot problem on GE PPD (watchdog kills device) and also has been tested on the Droid 4. Fixes: f657b00df22e ("Input: atmel_mxt_ts - add support for reset line") Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> --- drivers/input/touchscreen/atmel_mxt_ts.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)