LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thierry Reding <thierry.reding@gmail.com>
To: Sowjanya Komatineni <skomatineni@nvidia.com>
Cc: jonathanh@nvidia.com, jckuo@nvidia.com, talho@nvidia.com,
	josephl@nvidia.com, linux-tegra@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH V1 02/12] pinctrl: tegra: add suspend and resume support
Date: Wed, 22 May 2019 14:37:19 +0200	[thread overview]
Message-ID: <20190522123719.GH30938@ulmo> (raw)
In-Reply-To: <1558481483-22254-3-git-send-email-skomatineni@nvidia.com>

[-- Attachment #1: Type: text/plain, Size: 9673 bytes --]

On Tue, May 21, 2019 at 04:31:13PM -0700, Sowjanya Komatineni wrote:
> 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@nvidia.com>
> ---
>  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..585debbc4291 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>

I'd like to avoid moving this to syscore_ops if possible. The reason is
that there are other mechanisms that allow proper sequencing of suspend
and resume. syscore_ops might give you the correct sequence with respect
to device drivers, but there's no way to control the sequence with
respect to other syscore_ops. It also sets a bad precedent because this
requires a global variable and is therefore no longer properly
encapsulated (i.e. you can't have more than one instance, otherwise you
would end up overwriting the global variable).

>  
>  #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)

Use lower-case hexadecimal digits consistently.

> +
> +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 *pg_data = pmx->pg_data;

This is a very generic name. Perhaps use something more contextual like
pmx->suspend_backup, pmx->saved_regs or something along those lines.

> +	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++)
> +			*pg_data++ = readl(regs++);

Perhaps avoid the regs temporary variable and just do regs[i][j]?

> +	}
> +
> +	return pinctrl_force_sleep(pmx->pctl);
> +}
> +
> +static void __maybe_unused tegra_pinctrl_resume(void)
> +{
> +	u32 *pg_data = pmx->pg_data;
> +	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(*pg_data++, 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);
> +	}
> +}
> +
> +static struct syscore_ops pinctrl_syscore_ops = {
> +	.suspend = tegra_pinctrl_suspend,
> +	.resume = tegra_pinctrl_resume,
> +};
> +
>  static bool gpio_node_has_range(const char *compatible)
>  {
>  	struct device_node *np;
> @@ -648,11 +699,11 @@ static bool gpio_node_has_range(const char *compatible)
>  int tegra_pinctrl_probe(struct platform_device *pdev,
>  			const struct tegra_pinctrl_soc_data *soc_data)
>  {
> -	struct tegra_pmx *pmx;
>  	struct resource *res;
>  	int i;
>  	const char **group_pins;
>  	int fn, gn, gfn;
> +	int pg_data_size = 0;
>  
>  	pmx = devm_kzalloc(&pdev->dev, sizeof(*pmx), GFP_KERNEL);
>  	if (!pmx)
> @@ -705,6 +756,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
>  		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>  		if (!res)
>  			break;
> +		pg_data_size += resource_size(res);
>  	}
>  	pmx->nbanks = i;
>  
> @@ -712,12 +764,25 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
>  				 GFP_KERNEL);
>  	if (!pmx->regs)
>  		return -ENOMEM;
> +#ifdef CONFIG_PM_SLEEP

Do we really need the #ifdef here? I don't think it buys us much and
PM_SLEEP is likely always going to be enabled.

> +	pmx->reg_bank_size = devm_kcalloc(&pdev->dev, pmx->nbanks,
> +					  sizeof(*pmx->reg_bank_size),
> +					  GFP_KERNEL);
> +	if (!pmx->reg_bank_size)
> +		return -ENOMEM;
>  
> +	pmx->pg_data = devm_kzalloc(&pdev->dev, pg_data_size, GFP_KERNEL);
> +	if (!pmx->pg_data)
> +		return -ENOMEM;
> +#endif
>  	for (i = 0; i < pmx->nbanks; i++) {
>  		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>  		pmx->regs[i] = devm_ioremap_resource(&pdev->dev, res);
>  		if (IS_ERR(pmx->regs[i]))
>  			return PTR_ERR(pmx->regs[i]);
> +#ifdef CONFIG_PM_SLEEP
> +		pmx->reg_bank_size[i] = resource_size(res);
> +#endif
>  	}
>  
>  	pmx->pctl = devm_pinctrl_register(&pdev->dev, &tegra_pinctrl_desc, pmx);
> @@ -732,6 +797,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
>  		pinctrl_add_gpio_range(pmx->pctl, &tegra_pinctrl_gpio_range);
>  
>  	platform_set_drvdata(pdev, pmx);
> +	register_syscore_ops(&pinctrl_syscore_ops);
>  
>  	dev_dbg(&pdev->dev, "Probed Tegra pinctrl driver\n");
>  
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
> index 44c71941b5f8..8c7cd94124ea 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
> @@ -25,6 +25,8 @@ struct tegra_pmx {
>  
>  	int nbanks;
>  	void __iomem **regs;
> +	int *reg_bank_size;

size_t

However, I wonder if there's not a better way to do this. You already do
pinctrl_force_sleep(). I wonder if it'd make sense to add functionality
to the pinctrl framework to restore the current state.

Adding Linus Walleij. Linus, do you know if there's some way of tracking
the current state for all of the pins? I can't seem to find anything
like that anywhere, but it seems to me like a bit of a waste for every
driver to have to allocate extra system memory to store register values
that could equally well just be reconstructed from state kept in the
pinctrl framework.

Any ideas if this is possible, or whether that'd be worth doing?

Thierry

> +	u32 *pg_data;
>  };
>  
>  enum tegra_pinconf_param {
> @@ -199,6 +201,7 @@ struct tegra_pinctrl_soc_data {
>  	bool hsm_in_mux;
>  	bool schmitt_in_mux;
>  	bool drvtype_in_mux;
> +	bool has_park_padcfg;
>  };
>  
>  int tegra_pinctrl_probe(struct platform_device *pdev,
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra114.c b/drivers/pinctrl/tegra/pinctrl-tegra114.c
> index d43c209e9c30..4ac44f34dccf 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra114.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra114.c
> @@ -1849,6 +1849,7 @@ static const struct tegra_pinctrl_soc_data tegra114_pinctrl = {
>  	.hsm_in_mux = false,
>  	.schmitt_in_mux = false,
>  	.drvtype_in_mux = false,
> +	.has_park_padcfg = false,
>  };
>  
>  static int tegra114_pinctrl_probe(struct platform_device *pdev)
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra124.c b/drivers/pinctrl/tegra/pinctrl-tegra124.c
> index 5b07a5834d15..1dac7648b41f 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra124.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra124.c
> @@ -2061,6 +2061,7 @@ static const struct tegra_pinctrl_soc_data tegra124_pinctrl = {
>  	.hsm_in_mux = false,
>  	.schmitt_in_mux = false,
>  	.drvtype_in_mux = false,
> +	.has_park_padcfg = false,
>  };
>  
>  static int tegra124_pinctrl_probe(struct platform_device *pdev)
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra20.c b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> index 1fc82a9576e0..9d2b25200f32 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra20.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> @@ -2231,6 +2231,7 @@ static const struct tegra_pinctrl_soc_data tegra20_pinctrl = {
>  	.hsm_in_mux = false,
>  	.schmitt_in_mux = false,
>  	.drvtype_in_mux = false,
> +	.has_park_padcfg = false,
>  };
>  
>  static const char *cdev1_parents[] = {
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c b/drivers/pinctrl/tegra/pinctrl-tegra210.c
> index 3e77f5474dd8..dc06c36e698a 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
> @@ -1563,6 +1563,7 @@ static const struct tegra_pinctrl_soc_data tegra210_pinctrl = {
>  	.hsm_in_mux = true,
>  	.schmitt_in_mux = true,
>  	.drvtype_in_mux = true,
> +	.has_park_padcfg = true,
>  };
>  
>  static int tegra210_pinctrl_probe(struct platform_device *pdev)
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra30.c b/drivers/pinctrl/tegra/pinctrl-tegra30.c
> index 10e617003e9c..42182d714950 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra30.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra30.c
> @@ -2484,6 +2484,7 @@ static const struct tegra_pinctrl_soc_data tegra30_pinctrl = {
>  	.hsm_in_mux = false,
>  	.schmitt_in_mux = false,
>  	.drvtype_in_mux = false,
> +	.has_park_padcfg = false,
>  };
>  
>  static int tegra30_pinctrl_probe(struct platform_device *pdev)
> -- 
> 2.7.4
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2019-05-22 12:37 UTC|newest]

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-21 23:31 [PATCH V1 00/12] LP0 entry and exit support for Tegra210 Sowjanya Komatineni
2019-05-21 23:31 ` [PATCH V1 01/12] irqchip: tegra: do not disable COP IRQ during suspend Sowjanya Komatineni
2019-05-22 12:12   ` Thierry Reding
2019-05-21 23:31 ` [PATCH V1 02/12] pinctrl: tegra: add suspend and resume support Sowjanya Komatineni
2019-05-22 12:37   ` Thierry Reding [this message]
2019-05-21 23:31 ` [PATCH V1 03/12] clk: tegra: save and restore PLLs state for system Sowjanya Komatineni
2019-05-22 13:31   ` Thierry Reding
2019-05-21 23:31 ` [PATCH V1 04/12] clk: tegra: add support for peripheral clock suspend and resume Sowjanya Komatineni
2019-05-21 23:31 ` [PATCH V1 05/12] clk: tegra: add support for OSC clock resume Sowjanya Komatineni
2019-05-21 23:31 ` [PATCH V1 06/12] clk: tegra: add suspend resume support for DFLL clock Sowjanya Komatineni
2019-05-21 23:31 ` [PATCH V1 07/12] clk: tegra: support for Tegra210 clocks suspend-resume Sowjanya Komatineni
2019-05-21 23:31 ` [PATCH V1 08/12] soc/tegra: pmc: allow support for more tegra wake models Sowjanya Komatineni
2019-05-22 12:49   ` Thierry Reding
2019-05-22 13:02   ` Thierry Reding
2019-05-21 23:31 ` [PATCH V1 09/12] soc/tegra: pmc: add pmc wake support for tegra210 Sowjanya Komatineni
2019-05-22 13:01   ` Thierry Reding
2019-05-21 23:31 ` [PATCH V1 10/12] gpio: tegra: implement wake event support for Tegra210 and prior GPIO Sowjanya Komatineni
2019-05-22 13:24   ` Thierry Reding
2019-05-25 20:39     ` Sowjanya Komatineni
2019-05-21 23:31 ` [PATCH V1 11/12] soc/tegra: pmc: configure tegra deep sleep control settings Sowjanya Komatineni
2019-05-22 13:28   ` Thierry Reding
2019-05-21 23:31 ` [PATCH V1 12/12] arm64: tegra: enable wake from deep sleep on RTC alarm Sowjanya Komatineni
2019-05-22 13:33 ` [PATCH V1 00/12] LP0 entry and exit support for Tegra210 Thierry Reding

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190522123719.GH30938@ulmo \
    --to=thierry.reding@gmail.com \
    --cc=jckuo@nvidia.com \
    --cc=jonathanh@nvidia.com \
    --cc=josephl@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=skomatineni@nvidia.com \
    --cc=talho@nvidia.com \
    --subject='Re: [PATCH V1 02/12] pinctrl: tegra: add suspend and resume support' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).