Re: [PATCH V2 02/12] pinctrl: tegra: add suspend and resume support

From: Sowjanya Komatineni
Date: Wed May 29 2019 - 16:14:43 EST

On 5/29/19 12:32 PM, Dmitry Osipenko wrote:
29.05.2019 21:14, Sowjanya Komatineni ÐÐÑÐÑ:
On 5/29/19 8:29 AM, Dmitry Osipenko wrote:
29.05.2019 2:08, Sowjanya Komatineni ÐÐÑÐÑ:
This patch adds suspend and resume support for Tegra pinctrl driver
and registers them to syscore so the pinmux settings are restored
before the devices resume.

Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
 drivers/pinctrl/tegra/pinctrl-tegra.c | 68
 drivers/pinctrl/tegra/pinctrl-tegra.h | 3 ++
 drivers/pinctrl/tegra/pinctrl-tegra114.c | 1 +
 drivers/pinctrl/tegra/pinctrl-tegra124.c | 1 +
 drivers/pinctrl/tegra/pinctrl-tegra20.c | 1 +
 drivers/pinctrl/tegra/pinctrl-tegra210.c | 1 +
 drivers/pinctrl/tegra/pinctrl-tegra30.c | 1 +
 7 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
index a5008c066bac..bdc47e62c457 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
@@ -28,11 +28,18 @@
 #include <linux/pinctrl/pinmux.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/slab.h>
+#include <linux/syscore_ops.h>
  #include "../core.h"
 #include "../pinctrl-utils.h"
 #include "pinctrl-tegra.h"
 +#define EMMC2_PAD_CFGPADCTRL_0 0x1c8
+#define EMMC_DPD_PARKINGÂÂÂÂÂÂÂÂÂÂÂ (0x1fff << 14)
+static struct tegra_pmx *pmx;
 static inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
ÂÂÂÂÂ return readl(pmx->regs[bank] + reg);
@@ -629,6 +636,50 @@ static void
tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
 +static int __maybe_unused tegra_pinctrl_suspend(void)
+ÂÂÂ u32 *backup_regs = pmx->backup_regs;
+ÂÂÂ u32 *regs;
+ÂÂÂ int i, j;
+ÂÂÂ for (i = 0; i < pmx->nbanks; i++) {
+ÂÂÂÂÂÂÂ regs = pmx->regs[i];
+ÂÂÂÂÂÂÂ for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
+ÂÂÂÂÂÂÂÂÂÂÂ *backup_regs++ = readl(regs++);
+ÂÂÂ }
+ÂÂÂ return pinctrl_force_sleep(pmx->pctl);
+static void __maybe_unused tegra_pinctrl_resume(void)
+ÂÂÂ u32 *backup_regs = pmx->backup_regs;
+ÂÂÂ u32 *regs;
+ÂÂÂ u32 val;
+ÂÂÂ int i, j;
+ÂÂÂ for (i = 0; i < pmx->nbanks; i++) {
+ÂÂÂÂÂÂÂ regs = pmx->regs[i];
+ÂÂÂÂÂÂÂ for (j = 0; j < pmx->reg_bank_size[i] / 4; j++)
+ÂÂÂÂÂÂÂÂÂÂÂ writel(*backup_regs++, regs++);
+ÂÂÂ }
+ÂÂÂ if (pmx->soc->has_park_padcfg) {
+ÂÂÂÂÂÂÂ val = pmx_readl(pmx, 0, EMMC2_PAD_CFGPADCTRL_0);
+ÂÂÂÂÂÂÂ pmx_writel(pmx, val, 0, EMMC2_PAD_CFGPADCTRL_0);
+ÂÂÂÂÂÂÂ val = pmx_readl(pmx, 0, EMMC4_PAD_CFGPADCTRL_0);
+ÂÂÂÂÂÂÂ pmx_writel(pmx, val, 0, EMMC4_PAD_CFGPADCTRL_0);
+ÂÂÂ }

But the CFGPADCTRL registers are already programmed by restoring the
backup_regs and hence the relevant EMMC's are already unparked. Hence
why do you need to force-unpark both of the EMMC's? What if EMMC is
unpopulated on a board, why do you need to unpark it then?
are not part of pinmux.

They are part of CFGPADCTRL register so pinctrl driver pingroup doesn't
include these registers.
I'm looking at the tegra210_groups and it clearly has these both
registers as a part of pinctrl setup because the rest of the bits
configure drive of the pads.

From pinctrl-tegra210.c:

#define DRV_PINGROUP_REG_A 0x8d4 /* bank 0 */

DRV_PINGROUP(sdmmc2, 0xa9c, 2, 6, 8, 6, 28, 2, 30, 2),
DRV_PINGROUP(sdmmc4, 0xab4, 2, 6, 8, 6, 28, 2, 30, 2),


0xa9c - 0x8d4 = 0x1c8
0xab4 - 0x8d4 = 0x1e0

Hence the PARK bits are already getting unset by restoring the
backup_regs because the CFGPADCTRL registers are a part of the "bank 0"

Am I still missing something?

DRV_PINGROUP parked_bit is -1 and will not be programmed so store and restore will not take care of it.

Also EMMC PADCFG is the only padcfg register which has parked bit and for other IO pads its part of pinmux

backup_regs doesn't take care of this and need to handled separately for

During resume we have to clear PARK bit for the pads on Tegra210 and
this is not related to presence/absence of eMMC on the board.
Okay, thank you for the clarification.

PAD is parked during LP0 entry to have it in DPD mode and it stays in
DPD till its cleared by SW on resume.
Yes, this is documented in the public TRM. My main point is that it
looks like the PARK bits are unneedlessly getting unset twice in your
code (and it still looks like that to me).