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

[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?

> 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.