[14/14] gpio: pca953x: Restore registers after suspend/resume cycle
diff mbox series

Message ID 20181202193553.29704-14-marek.vasut+renesas@gmail.com
State New
Headers show
Series
  • [01/14] gpio: pca953x: Deduplicate the bank_size
Related show

Commit Message

Marek Vasut Dec. 2, 2018, 7:35 p.m. UTC
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(+)

Comments

Geert Uytterhoeven Dec. 3, 2018, 5:55 p.m. UTC | #1
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
Marek Vasut Dec. 3, 2018, 9:42 p.m. UTC | #2
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.
Geert Uytterhoeven Dec. 5, 2018, 2:39 p.m. UTC | #3
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
Marek Vasut Dec. 5, 2018, 2:45 p.m. UTC | #4
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
Geert Uytterhoeven Dec. 18, 2018, 12:57 p.m. UTC | #5
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
Geert Uytterhoeven Dec. 18, 2018, 1:43 p.m. UTC | #6
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

Patch
diff mbox series

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),
 	},