Re: [PATCH V2 02/12] pinctrl: tegra: add suspend and resume support
From: Sowjanya Komatineni
Date: Wed May 29 2019 - 17:31:19 EST
On 5/29/19 2:25 PM, Dmitry Osipenko wrote:
29.05.2019 23:56, Sowjanya Komatineni ÐÐÑÐÑ:
On 5/29/19 1:47 PM, Dmitry Osipenko wrote:
29.05.2019 23:11, Sowjanya Komatineni ÐÐÑÐÑ:
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
b/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 EMMC4_PAD_CFGPADCTRL_0ÂÂÂÂÂÂÂÂÂÂÂ 0x1e0
+#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);
+ÂÂÂÂÂÂÂ val &= ~EMMC_DPD_PARKING;
+ÂÂÂÂÂÂÂ pmx_writel(pmx, val, 0, EMMC2_PAD_CFGPADCTRL_0);
+
+ÂÂÂÂÂÂÂ val = pmx_readl(pmx, 0, EMMC4_PAD_CFGPADCTRL_0);
+ÂÂÂÂÂÂÂ val &= ~EMMC_DPD_PARKING;
+ÂÂÂÂÂÂÂ 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?
PARK bit for EMMC2/EMMC4 (EMMC2_PAD_CFGPADCTRL and
EMMC4_PAD_CFGPADCTRL)
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"
registers.
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
You're storing raw values of all of the PINCTRL registers and then
restoring the raw values (if I'm not misreading that part on the patch),
it's absolutely meaningless that DRV_PINGROUP doesn't define the PARK
bits.
In a result, the backup_regs array contains raw CFGPADCTRL value with
the PARK bits being unset on store, that value is written out on the
restore as-is and hence the PARK bits are getting unset as well.
And why DRV_PINGROUP misses PARK bits for the EMMC's? Looks like a
driver's drawback that need to be addressed.
Parked bits from padcfg are available only for couple of EMMC registers.
default PARK bits are set so stored value contains park bit set. on
resume, after restoring park bit is cleared.
TRM says that the PARK bits are set on HW reset (and on DPD IIUC) and
then SW should unset the bits. I assume that the PARK bits of the EMMC
pads are unset by bootloader and that's why it doesn't cause problems
for kernel, in oppose to PARK bits of the rest of pinmux that are unset
by the driver in tegra_pinctrl_clear_parked_bits() on driver's probe.
on subsequence DPD entry, stored value contains park bit 0 and HW clamps
park bit to logic 1 during DPD entry and cleared again on resume.
I assume that EMMC won't work properly if pads are "parked" and the PARK
bits should be in unset state on kernel boot-up as I wrote above, hence
stored value should always contain 0 for the EMMC PARK bits. No?
On bootup, pads still works. PARK bit is to keep pads in DPD once they
enter into DPD (LP0) and on resume they need to be cleared to be out of DPD