Re: [PATCH v7 01/20] pinctrl: tegra: Add suspend and resume support

From: Sowjanya Komatineni
Date: Tue Aug 06 2019 - 17:54:26 EST



On 8/6/19 10:59 AM, Dmitry Osipenko wrote:
05.08.2019 21:06, Sowjanya Komatineni ÐÐÑÐÑ:
On 8/5/19 3:50 AM, Dmitry Osipenko wrote:
01.08.2019 0:10, Sowjanya Komatineni ÐÐÑÐÑ:
This patch adds support for Tegra pinctrl driver suspend and resume.

During suspend, context of all pinctrl registers are stored and
on resume they are all restored to have all the pinmux and pad
configuration for normal operation.

Acked-by: Thierry Reding <treding@xxxxxxxxxx>
Reviewed-by: Dmitry Osipenko <digetx@xxxxxxxxx>
Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
---
 drivers/pinctrl/tegra/pinctrl-tegra.c | 59
+++++++++++++++++++++++++++++++++++
 drivers/pinctrl/tegra/pinctrl-tegra.h | 3 ++
 2 files changed, 62 insertions(+)

diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
b/drivers/pinctrl/tegra/pinctrl-tegra.c
index 186ef98e7b2b..e3a237534281 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
@@ -631,6 +631,58 @@ static void
tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
ÂÂÂÂÂ }
 }
 +static size_t tegra_pinctrl_get_bank_size(struct device *dev,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ unsigned int bank_id)
+{
+ÂÂÂ struct platform_device *pdev = to_platform_device(dev);
+ÂÂÂ struct resource *res;
+
+ÂÂÂ res = platform_get_resource(pdev, IORESOURCE_MEM, bank_id);
+
+ÂÂÂ return resource_size(res) / 4;
+}
+
+static int tegra_pinctrl_suspend(struct device *dev)
+{
+ÂÂÂ struct tegra_pmx *pmx = dev_get_drvdata(dev);
+ÂÂÂ u32 *backup_regs = pmx->backup_regs;
+ÂÂÂ u32 *regs;
+ÂÂÂ size_t bank_size;
+ÂÂÂ unsigned int i, k;
+
+ÂÂÂ for (i = 0; i < pmx->nbanks; i++) {
+ÂÂÂÂÂÂÂ bank_size = tegra_pinctrl_get_bank_size(dev, i);
+ÂÂÂÂÂÂÂ regs = pmx->regs[i];
+ÂÂÂÂÂÂÂ for (k = 0; k < bank_size; k++)
+ÂÂÂÂÂÂÂÂÂÂÂ *backup_regs++ = readl_relaxed(regs++);
+ÂÂÂ }
+
+ÂÂÂ return pinctrl_force_sleep(pmx->pctl);
+}
+
+static int tegra_pinctrl_resume(struct device *dev)
+{
+ÂÂÂ struct tegra_pmx *pmx = dev_get_drvdata(dev);
+ÂÂÂ u32 *backup_regs = pmx->backup_regs;
+ÂÂÂ u32 *regs;
+ÂÂÂ size_t bank_size;
+ÂÂÂ unsigned int i, k;
+
+ÂÂÂ for (i = 0; i < pmx->nbanks; i++) {
+ÂÂÂÂÂÂÂ bank_size = tegra_pinctrl_get_bank_size(dev, i);
+ÂÂÂÂÂÂÂ regs = pmx->regs[i];
+ÂÂÂÂÂÂÂ for (k = 0; k < bank_size; k++)
+ÂÂÂÂÂÂÂÂÂÂÂ writel_relaxed(*backup_regs++, regs++);
+ÂÂÂ }
I'm now curious whether any kind of barrier is needed after the
writings. The pmx_writel() doesn't insert a barrier after the write and
seems it just misuses writel, which actually should be writel_relaxed()
+ barrier, IIUC.
pmx_writel uses writel and it has wmb before raw_write which complete
all writes initiated prior to this.

By misusing writel, you mean to have barrier after register write?
Yes, at least to me it doesn't make much sense for this driver to stall
before the write. It's the pinctrl user which should be taking care
about everything to be ready before making a change to the pinctrl's
configuration.

It's also not obvious whether PINCTRL HW has any kind of write-FIFO and
thus maybe read-back + rmb() is needed in order ensure that writes are
actually completed.
I believe adding write barrier wmb after writel_relaxed should be good
rather than doing readback + rmb
The last thing which is not obvious is when the new configuration
actually takes into effect, does it happen immediately or maybe some
delay is needed?

[snip]
Based on internal design there is no internal delay and it all depends
on APB rate that it takes to write to register.

Pinmux value change to reflect internally might take couple of clock
cycles which is much faster than SW can read.
Still not quite obvious if it's possible to have a case where some
hardware is touched before necessary pinctrl change is fully completed
and then to get into trouble because of it.

To be safer, will add write barrier after all writes in resume and also will have separate patch for pmx_writel fix to use writel_relaxed followed by write barrier.

Thanks

Sowjanya