| Message ID | 20181202193553.29704-14-marek.vasut+renesas@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/14] gpio: pca953x: Deduplicate the bank_size | expand |
Hi Marek, On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote: > It is possible that the PCA953x is powered down during suspend. > Use regmap cache to assure the registers in the PCA953x are in > line with the driver state after resume. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Thanks for your series! Background info: the main motivation for this series is to make sure SATA keeps working after system suspend/resume on the Salvator-XS development board, where the SATA functionality is configured using a gpio hog. With your series applied, the SATA link seems to be functional after resume. Dmesg difference: ata1: link resume succeeded after 1 retries -ata1: SATA link down (SStatus 0 SControl 300) -ata1: link resume succeeded after 1 retries -ata1: SATA link down (SStatus 0 SControl 300) -ata1: link resume succeeded after 1 retries -ata1: SATA link down (SStatus 0 SControl 300) -ata1.00: disabled -sd 0:0:0:0: rejecting I/O to offline device +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) +ata1.00: configured for UDMA/133 However, when trying to read from an attached hard drive, it fails: ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen ata1.00: failed command: READ DMA ata1.00: cmd c8/00:20:00:00:00/00:00:00:00:00/e0 tag 0 dma 16384 in res 40/00:00:01:4f:c2/00:00:00:00:00/00 Emask 0x4 (timeout) ata1.00: status: { DRDY } ata1: hard resetting link ata1: link resume succeeded after 1 retries ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) ata1.00: configured for UDMA/133 sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x00 driverbyte=0x08 sd 0:0:0:0: [sda] tag#0 Sense Key : 0x5 [current] sd 0:0:0:0: [sda] tag#0 ASC=0x21 ASCQ=0x4 sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 00 00 00 00 20 00 print_req_error: I/O error, dev sda, sector 0 ata1: EH complete ... Buffer I/O error on dev sda, logical block 0, async page read Does SATA work for you after resume! This could still be an issue in the sata_rcar driver. Thanks! Gr{oetje,eeting}s, Geert
On 12/03/2018 06:55 PM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> It is possible that the PCA953x is powered down during suspend. >> Use regmap cache to assure the registers in the PCA953x are in >> line with the driver state after resume. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > Thanks for your series! > > Background info: the main motivation for this series is to make sure SATA > keeps working after system suspend/resume on the Salvator-XS development > board, where the SATA functionality is configured using a gpio hog. > > With your series applied, the SATA link seems to be functional after resume. > Dmesg difference: > > ata1: link resume succeeded after 1 retries > -ata1: SATA link down (SStatus 0 SControl 300) > -ata1: link resume succeeded after 1 retries > -ata1: SATA link down (SStatus 0 SControl 300) > -ata1: link resume succeeded after 1 retries > -ata1: SATA link down (SStatus 0 SControl 300) > -ata1.00: disabled > -sd 0:0:0:0: rejecting I/O to offline device > +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > +ata1.00: configured for UDMA/133 > > However, when trying to read from an attached hard drive, it fails: > > ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen > ata1.00: failed command: READ DMA > ata1.00: cmd c8/00:20:00:00:00/00:00:00:00:00/e0 tag 0 dma 16384 in > res 40/00:00:01:4f:c2/00:00:00:00:00/00 Emask 0x4 (timeout) > ata1.00: status: { DRDY } > ata1: hard resetting link > ata1: link resume succeeded after 1 retries > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > ata1.00: configured for UDMA/133 > sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x00 > driverbyte=0x08 > sd 0:0:0:0: [sda] tag#0 Sense Key : 0x5 [current] > sd 0:0:0:0: [sda] tag#0 ASC=0x21 ASCQ=0x4 > sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 00 00 00 00 20 00 > print_req_error: I/O error, dev sda, sector 0 > ata1: EH complete > ... > Buffer I/O error on dev sda, logical block 0, async page read > > Does SATA work for you after resume! Yes! http://paste.debian.net/1054228/ > This could still be an issue in the sata_rcar driver. I wonder if this has to do with the SATA link being cut off by the GPIO mux at a bad time OR restored at a bad time.
Hi Marek, On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote: > It is possible that the PCA953x is powered down during suspend. > Use regmap cache to assure the registers in the PCA953x are in > line with the driver state after resume. > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > +static int pca953x_suspend(struct device *dev) > +{ > + struct pca953x_chip *chip = dev_get_drvdata(dev); > + > + regcache_mark_dirty(chip->regmap); > + pca953x_regcache_sync(dev); > + regcache_cache_only(chip->regmap, true); Based on the discussion about adding suspend/resume support to the VC5 clock driver, I guess the two sync calls above are not needed here? > + > + regulator_disable(chip->regulator); > + > + return 0; > +} Gr{oetje,eeting}s, Geert
On 12/05/2018 03:39 PM, Geert Uytterhoeven wrote: > Hi Marek, Hi, > On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote: >> It is possible that the PCA953x is powered down during suspend. >> Use regmap cache to assure the registers in the PCA953x are in >> line with the driver state after resume. >> >> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > >> +static int pca953x_suspend(struct device *dev) >> +{ >> + struct pca953x_chip *chip = dev_get_drvdata(dev); >> + >> + regcache_mark_dirty(chip->regmap); >> + pca953x_regcache_sync(dev); >> + regcache_cache_only(chip->regmap, true); > > Based on the discussion about adding suspend/resume support to the VC5 > clock driver, I guess the two sync calls above are not needed here? Yes
Hi Marek, On Mon, Dec 3, 2018 at 6:55 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > It is possible that the PCA953x is powered down during suspend. > > Use regmap cache to assure the registers in the PCA953x are in > > line with the driver state after resume. > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > Thanks for your series! > > Background info: the main motivation for this series is to make sure SATA > keeps working after system suspend/resume on the Salvator-XS development > board, where the SATA functionality is configured using a gpio hog. > > With your series applied, the SATA link seems to be functional after resume. > Dmesg difference: > > ata1: link resume succeeded after 1 retries > -ata1: SATA link down (SStatus 0 SControl 300) > -ata1: link resume succeeded after 1 retries > -ata1: SATA link down (SStatus 0 SControl 300) > -ata1: link resume succeeded after 1 retries > -ata1: SATA link down (SStatus 0 SControl 300) > -ata1.00: disabled > -sd 0:0:0:0: rejecting I/O to offline device > +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > +ata1.00: configured for UDMA/133 > > However, when trying to read from an attached hard drive, it fails: > > ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen > ata1.00: failed command: READ DMA > ata1.00: cmd c8/00:20:00:00:00/00:00:00:00:00/e0 tag 0 dma 16384 in > res 40/00:00:01:4f:c2/00:00:00:00:00/00 Emask 0x4 (timeout) > ata1.00: status: { DRDY } > ata1: hard resetting link > ata1: link resume succeeded after 1 retries > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > ata1.00: configured for UDMA/133 > sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x00 > driverbyte=0x08 > sd 0:0:0:0: [sda] tag#0 Sense Key : 0x5 [current] > sd 0:0:0:0: [sda] tag#0 ASC=0x21 ASCQ=0x4 > sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 00 00 00 00 20 00 > print_req_error: I/O error, dev sda, sector 0 > ata1: EH complete > ... > Buffer I/O error on dev sda, logical block 0, async page read > > Does SATA work for you after resume! > This could still be an issue in the sata_rcar driver. FTR, it wasn't. With V3 as present in gpio/for-next, SATA works fine after resume. Major difference seems to be the use of regmap_bulk_read() vs. regmap_read(). Thanks! Gr{oetje,eeting}s, Geert
Hi Marek, On Tue, Dec 18, 2018 at 1:57 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Dec 3, 2018 at 6:55 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Sun, Dec 2, 2018 at 8:36 PM Marek Vasut <marek.vasut@gmail.com> wrote: > > > It is possible that the PCA953x is powered down during suspend. > > > Use regmap cache to assure the registers in the PCA953x are in > > > line with the driver state after resume. > > > > > > Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> > > > > Thanks for your series! > > > > Background info: the main motivation for this series is to make sure SATA > > keeps working after system suspend/resume on the Salvator-XS development > > board, where the SATA functionality is configured using a gpio hog. > > > > With your series applied, the SATA link seems to be functional after resume. > > Dmesg difference: > > > > ata1: link resume succeeded after 1 retries > > -ata1: SATA link down (SStatus 0 SControl 300) > > -ata1: link resume succeeded after 1 retries > > -ata1: SATA link down (SStatus 0 SControl 300) > > -ata1: link resume succeeded after 1 retries > > -ata1: SATA link down (SStatus 0 SControl 300) > > -ata1.00: disabled > > -sd 0:0:0:0: rejecting I/O to offline device > > +ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > > +ata1.00: configured for UDMA/133 > > > > However, when trying to read from an attached hard drive, it fails: > > > > ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen > > ata1.00: failed command: READ DMA > > ata1.00: cmd c8/00:20:00:00:00/00:00:00:00:00/e0 tag 0 dma 16384 in > > res 40/00:00:01:4f:c2/00:00:00:00:00/00 Emask 0x4 (timeout) > > ata1.00: status: { DRDY } > > ata1: hard resetting link > > ata1: link resume succeeded after 1 retries > > ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) > > ata1.00: configured for UDMA/133 > > sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x00 > > driverbyte=0x08 > > sd 0:0:0:0: [sda] tag#0 Sense Key : 0x5 [current] > > sd 0:0:0:0: [sda] tag#0 ASC=0x21 ASCQ=0x4 > > sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 00 00 00 00 00 00 20 00 > > print_req_error: I/O error, dev sda, sector 0 > > ata1: EH complete > > ... > > Buffer I/O error on dev sda, logical block 0, async page read > > > > Does SATA work for you after resume! > > This could still be an issue in the sata_rcar driver. > > FTR, it wasn't. With V3 as present in gpio/for-next, SATA works fine after > resume. > Major difference seems to be the use of regmap_bulk_read() vs. regmap_read(). I think I found the real reason: my failing config had IPMMU enabled for SATA virtualization, but the IPMMU driver doesn't handle suspend/resume yet. Gr{oetje,eeting}s, Geert
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index 7c0122fac383..e2c2e97b7321 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -972,6 +972,95 @@ static int pca953x_remove(struct i2c_client *client) return ret; } +#ifdef CONFIG_PM_SLEEP +static int pca953x_regcache_sync(struct device *dev) +{ + struct pca953x_chip *chip = dev_get_drvdata(dev); + int ret; + + /* + * The ordering between direction and output is important, + * sync these registers first and only then sync the rest. + */ + ret = regcache_sync_region(chip->regmap, chip->regs->direction, + chip->regs->direction + NBANK(chip)); + if (ret != 0) { + dev_err(dev, "Failed to sync GPIO dir registers: %d\n", ret); + return ret; + } + + ret = regcache_sync_region(chip->regmap, chip->regs->output, + chip->regs->output + NBANK(chip)); + if (ret != 0) { + dev_err(dev, "Failed to sync GPIO out registers: %d\n", ret); + return ret; + } + +#ifdef CONFIG_GPIO_PCA953X_IRQ + if (chip->driver_data & PCA_PCAL) { + pca953x_write_regs(chip, PCAL953X_IN_LATCH, chip->irq_mask); + pca953x_write_regs(chip, PCAL953X_INT_MASK, invert_irq_mask); + + ret = regcache_sync_region(chip->regmap, PCAL953X_IN_LATCH, + PCAL953X_IN_LATCH + NBANK(chip)); + if (ret != 0) { + dev_err(dev, "Failed to sync INT latch registers: %d\n", + ret); + return ret; + } + + ret = regcache_sync_region(chip->regmap, PCAL953X_INT_MASK, + PCAL953X_INT_MASK + NBANK(chip)); + if (ret != 0) { + dev_err(dev, "Failed to sync INT mask registers: %d\n", + ret); + return ret; + } + } +#endif + + return 0; +} + +static int pca953x_suspend(struct device *dev) +{ + struct pca953x_chip *chip = dev_get_drvdata(dev); + + regcache_mark_dirty(chip->regmap); + pca953x_regcache_sync(dev); + regcache_cache_only(chip->regmap, true); + + regulator_disable(chip->regulator); + + return 0; +} + +static int pca953x_resume(struct device *dev) +{ + struct pca953x_chip *chip = dev_get_drvdata(dev); + int ret; + + ret = regulator_enable(chip->regulator); + if (ret != 0) { + dev_err(dev, "Failed to enable regulator: %d\n", ret); + return 0; + } + + regcache_cache_only(chip->regmap, false); + ret = pca953x_regcache_sync(dev); + if (ret) + return ret; + + ret = regcache_sync(chip->regmap); + if (ret != 0) { + dev_err(dev, "Failed to restore register map: %d\n", ret); + return ret; + } + + return 0; +} +#endif + /* convenience to stop overlong match-table lines */ #define OF_953X(__nrgpio, __int) (void *)(__nrgpio | PCA953X_TYPE | __int) #define OF_957X(__nrgpio, __int) (void *)(__nrgpio | PCA957X_TYPE | __int) @@ -1015,9 +1104,12 @@ static const struct of_device_id pca953x_dt_ids[] = { MODULE_DEVICE_TABLE(of, pca953x_dt_ids); +static SIMPLE_DEV_PM_OPS(pca953x_pm_ops, pca953x_suspend, pca953x_resume); + static struct i2c_driver pca953x_driver = { .driver = { .name = "pca953x", + .pm = &pca953x_pm_ops, .of_match_table = pca953x_dt_ids, .acpi_match_table = ACPI_PTR(pca953x_acpi_ids), },
It is possible that the PCA953x is powered down during suspend. Use regmap cache to assure the registers in the PCA953x are in line with the driver state after resume. Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com> --- drivers/gpio/gpio-pca953x.c | 92 +++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+)