[V3,02/17] pinctrl: tegra: add suspend and resume support

Message ID	1560843991-24123-3-git-send-email-skomatineni@nvidia.com
State	Superseded
Headers	show Return-Path: <linux-tegra-owner@vger.kernel.org> TLS: TLSv1.2, DES-CBC3-SHA) id <B5d0896e00000>; Tue, 18 Jun 2019 00:46:40 -0700 From: Sowjanya Komatineni <skomatineni@nvidia.com> To: <thierry.reding@gmail.com>, <jonathanh@nvidia.com>, <tglx@linutronix.de>, <jason@lakedaemon.net>, <marc.zyngier@arm.com>, <linus.walleij@linaro.org>, <stefan@agner.ch>, <mark.rutland@arm.com> CC: <pdeschrijver@nvidia.com>, <pgaikwad@nvidia.com>, <sboyd@kernel.org>, <linux-clk@vger.kernel.org>, <linux-gpio@vger.kernel.org>, <jckuo@nvidia.com>, <josephl@nvidia.com>, <talho@nvidia.com>, <skomatineni@nvidia.com>, <linux-tegra@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <mperttunen@nvidia.com>, <spatra@nvidia.com>, <robh+dt@kernel.org>, <digetx@gmail.com>, <devicetree@vger.kernel.org> Subject: [PATCH V3 02/17] pinctrl: tegra: add suspend and resume support Date: Tue, 18 Jun 2019 00:46:16 -0700 Message-ID: <1560843991-24123-3-git-send-email-skomatineni@nvidia.com> In-Reply-To: <1560843991-24123-1-git-send-email-skomatineni@nvidia.com> References: <1560843991-24123-1-git-send-email-skomatineni@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-tegra-owner@vger.kernel.org Precedence: bulk
Series	SC7 entry and exit support for Tegra210 \| expand [V3,00/17] SC7 entry and exit support for Tegra210 [V3,01/17] irqchip: tegra: do not disable COP IRQ during suspend [V3,02/17] pinctrl: tegra: add suspend and resume support [V3,03/17] gpio: tegra: use resume_noirq for tegra gpio resume [V3,04/17] clk: tegra: save and restore divider rate [V3,05/17] clk: tegra: pllout: save and restore pllout context [V3,06/17] clk: tegra: pll: save and restore pll context [V3,07/17] clk: tegra: save and restore CPU and System clocks context [V3,08/17] clk: tegra: add support for peripheral clock suspend and resume [V3,09/17] clk: tegra: support for saving and restoring OSC clock context [V3,10/17] clk: tegra: add suspend resume support for DFLL [V3,11/17] clk: tegra210: support for Tegra210 clocks suspend and resume [V3,12/17] soc/tegra: pmc: allow support for more tegra wake [V3,13/17] soc/tegra: pmc: add pmc wake support for tegra210 [V3,14/17] arm64: tegra: enable wake from deep sleep on RTC alarm. [V3,15/17] soc/tegra: pmc: configure core power request polarity [V3,16/17] soc/tegra: pmc: configure deep sleep control settings [V3,17/17] arm64: dts: tegra210-p2180: Jetson TX1 SC7 timings

Sowjanya Komatineni June 18, 2019, 7:46 a.m. UTC

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    | 62 ++++++++++++++++++++++++++++++++
 drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
 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 | 13 +++++++
 drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
 7 files changed, 84 insertions(+)

Dmitry Osipenko June 18, 2019, 9:22 a.m. UTC | #1

18.06.2019 10:46, 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@nvidia.com>
> ---
>  drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
>  drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
>  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 | 13 +++++++
>  drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
>  7 files changed, 84 insertions(+)
> 
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 34596b246578..ceced30d8bd1 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -20,11 +20,16 @@
>  #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>  {
>  	return readl(pmx->regs[bank] + reg);
> @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>  			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>  		}
>  	}
> +
> +	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);
> +	}
> +}

Is there any reason why parked_bit can't be changed to parked_bitmask like I was
asking in a comment to v2?

I suppose that it's more preferable to keep pinctrl-tegra.c platform-agnostic for
consistency when possible, hence adding platform specifics here should be discouraged.
And then the parked_bitmask will also result in a proper hardware description in the code.

Dmitry Osipenko June 18, 2019, 9:30 a.m. UTC | #2

18.06.2019 12:22, Dmitry Osipenko пишет:
> 18.06.2019 10:46, 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@nvidia.com>
>> ---
>>  drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
>>  drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
>>  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 | 13 +++++++
>>  drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
>>  7 files changed, 84 insertions(+)
>>
>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
>> index 34596b246578..ceced30d8bd1 100644
>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>> @@ -20,11 +20,16 @@
>>  #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>>  {
>>  	return readl(pmx->regs[bank] + reg);
>> @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>>  			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>>  		}
>>  	}
>> +
>> +	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);
>> +	}
>> +}
> 
> Is there any reason why parked_bit can't be changed to parked_bitmask like I was
> asking in a comment to v2?
> 
> I suppose that it's more preferable to keep pinctrl-tegra.c platform-agnostic for
> consistency when possible, hence adding platform specifics here should be discouraged.
> And then the parked_bitmask will also result in a proper hardware description in the code.
> 

I'm now also vaguely recalling that Stephen Warren had some kind of a "code generator"
for the pinctrl drivers. So I guess all those tables were auto-generated initially.

Stephen, maybe you could adjust the generator to take into account the bitmask (of
course if that's a part of the generated code) and then re-gen it all for Sowjanya?

Thierry Reding June 18, 2019, 11:31 a.m. UTC | #3

On Tue, Jun 18, 2019 at 12:46:16AM -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.

This no longer uses syscore ops, so you need to reflect that in the
commit message.

> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
>  drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
>  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 | 13 +++++++
>  drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
>  7 files changed, 84 insertions(+)
> 
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 34596b246578..ceced30d8bd1 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -20,11 +20,16 @@
>  #include <linux/pinctrl/pinmux.h>
>  #include <linux/pinctrl/pinconf.h>
>  #include <linux/slab.h>
> +#include <linux/syscore_ops.h>

No longer needed.

>  
>  #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>  {
>  	return readl(pmx->regs[bank] + reg);
> @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>  			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>  		}
>  	}
> +
> +	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);
> +	}
> +}
> +
> +int __maybe_unused tegra_pinctrl_suspend(struct device *dev)
> +{
> +	struct tegra_pmx *pmx = dev_get_drvdata(dev);
> +	u32 *backup_regs = pmx->backup_regs;
> +	u32 *regs;
> +	int i, j;

Can be unsigned int.

> +
> +	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);
> +}
> +
> +int __maybe_unused tegra_pinctrl_resume(struct device *dev)
> +{
> +	struct tegra_pmx *pmx = dev_get_drvdata(dev);
> +	u32 *backup_regs = pmx->backup_regs;
> +	u32 *regs;
> +	int i, j;

unsigned

> +
> +	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++);
> +	}
> +
> +	return 0;
>  }
>  
>  static bool gpio_node_has_range(const char *compatible)
> @@ -645,6 +692,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
>  	int i;
>  	const char **group_pins;
>  	int fn, gn, gfn;
> +	unsigned long backup_regs_size = 0;
>  
>  	pmx = devm_kzalloc(&pdev->dev, sizeof(*pmx), GFP_KERNEL);
>  	if (!pmx)
> @@ -697,6 +745,7 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
>  		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
>  		if (!res)
>  			break;
> +		backup_regs_size += resource_size(res);
>  	}
>  	pmx->nbanks = i;
>  
> @@ -705,11 +754,24 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
>  	if (!pmx->regs)
>  		return -ENOMEM;
>  
> +	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->backup_regs = devm_kzalloc(&pdev->dev, backup_regs_size,
> +					GFP_KERNEL);
> +	if (!pmx->backup_regs)
> +		return -ENOMEM;
> +
>  	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]);
> +
> +		pmx->reg_bank_size[i] = resource_size(res);
>  	}
>  
>  	pmx->pctl = devm_pinctrl_register(&pdev->dev, &tegra_pinctrl_desc, pmx);
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
> index 287702660783..d63e472ee0e1 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
> @@ -17,6 +17,8 @@ struct tegra_pmx {
>  
>  	int nbanks;
>  	void __iomem **regs;
> +	size_t *reg_bank_size;
> +	u32 *backup_regs;
>  };
>  
>  enum tegra_pinconf_param {
> @@ -191,8 +193,11 @@ 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,
>  			const struct tegra_pinctrl_soc_data *soc_data);
> +int __maybe_unused tegra_pinctrl_suspend(struct device *dev);
> +int __maybe_unused tegra_pinctrl_resume(struct device *dev);
>  #endif
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra114.c b/drivers/pinctrl/tegra/pinctrl-tegra114.c
> index 762151f17a88..06ea8164df9d 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra114.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra114.c
> @@ -1841,6 +1841,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 930c43758c92..abc8fe92d154 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra124.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra124.c
> @@ -2053,6 +2053,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 4b7837e38fb5..993b82cbfba7 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra20.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra20.c
> @@ -2223,6 +2223,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 0b56ad5c9c1c..10e8a2ec8094 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
> @@ -1555,6 +1555,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)
> @@ -1562,6 +1563,17 @@ static int tegra210_pinctrl_probe(struct platform_device *pdev)
>  	return tegra_pinctrl_probe(pdev, &tegra210_pinctrl);
>  }
>  
> +#ifdef CONFIG_PM_SLEEP
> +static const struct dev_pm_ops tegra_pinctrl_pm = {
> +	.suspend = &tegra_pinctrl_suspend,
> +	.resume = &tegra_pinctrl_resume
> +};
> +
> +#define TEGRA_PINCTRL_PM	(&tegra_pinctrl_pm)
> +#else
> +#define TEGRA_PINCTRL_PM	NULL
> +#endif

I think we can simplify this by just dropping the #ifdef. We don't allow
!PM on Tegra anymore and suspend/resume is something that most users
will want to enable. There's very little gain in making the dev_pm_ops
conditional, and keeping them around unconditionally make it simple.

> +
>  static const struct of_device_id tegra210_pinctrl_of_match[] = {
>  	{ .compatible = "nvidia,tegra210-pinmux", },
>  	{ },
> @@ -1571,6 +1583,7 @@ static struct platform_driver tegra210_pinctrl_driver = {
>  	.driver = {
>  		.name = "tegra210-pinctrl",
>  		.of_match_table = tegra210_pinctrl_of_match,
> +		.pm    = TEGRA_PINCTRL_PM,

Please use a single space around '='. No need for arbitrary padding.

Thierry Reding

Stephen Warren June 18, 2019, 3:41 p.m. UTC | #4

On 6/18/19 3:30 AM, Dmitry Osipenko wrote:
> 18.06.2019 12:22, Dmitry Osipenko пишет:
>> 18.06.2019 10:46, 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@nvidia.com>
>>> ---
>>>   drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
>>>   drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
>>>   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 | 13 +++++++
>>>   drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
>>>   7 files changed, 84 insertions(+)
>>>
>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>> index 34596b246578..ceced30d8bd1 100644
>>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>> @@ -20,11 +20,16 @@
>>>   #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>>>   {
>>>   	return readl(pmx->regs[bank] + reg);
>>> @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>>>   			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>>>   		}
>>>   	}
>>> +
>>> +	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);
>>> +	}
>>> +}
>>
>> Is there any reason why parked_bit can't be changed to parked_bitmask like I was
>> asking in a comment to v2?
>>
>> I suppose that it's more preferable to keep pinctrl-tegra.c platform-agnostic for
>> consistency when possible, hence adding platform specifics here should be discouraged.
>> And then the parked_bitmask will also result in a proper hardware description in the code.
>>
> 
> I'm now also vaguely recalling that Stephen Warren had some kind of a "code generator"
> for the pinctrl drivers. So I guess all those tables were auto-generated initially.
> 
> Stephen, maybe you could adjust the generator to take into account the bitmask (of
> course if that's a part of the generated code) and then re-gen it all for Sowjanya?

https://github.com/NVIDIA/tegra-pinmux-scripts holds the scripts that 
generate tegra-pinctrlNNN.c. See  	soc-to-kernel-pinctrl-driver.py. 
IIRC, tegra-pinctrl.c (the core file) isn't auto-generated. Sowjanya is 
welcome to send a patch to that repo if the code needs to be updated.

Sowjanya Komatineni June 18, 2019, 4:50 p.m. UTC | #5

On 6/18/19 8:41 AM, Stephen Warren wrote:
> On 6/18/19 3:30 AM, Dmitry Osipenko wrote:
>> 18.06.2019 12:22, Dmitry Osipenko пишет:
>>> 18.06.2019 10:46, 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@nvidia.com>
>>>> ---
>>>>   drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 
>>>> ++++++++++++++++++++++++++++++++
>>>>   drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
>>>>   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 | 13 +++++++
>>>>   drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
>>>>   7 files changed, 84 insertions(+)
>>>>
>>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c 
>>>> b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>> index 34596b246578..ceced30d8bd1 100644
>>>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>> @@ -20,11 +20,16 @@
>>>>   #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 
>>>> reg)
>>>>   {
>>>>       return readl(pmx->regs[bank] + reg);
>>>> @@ -619,6 +624,48 @@ static void 
>>>> tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>>>>               pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>>>>           }
>>>>       }
>>>> +
>>>> +    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);
>>>> +    }
>>>> +}
>>>
>>> Is there any reason why parked_bit can't be changed to 
>>> parked_bitmask like I was
>>> asking in a comment to v2?
>>>
>>> I suppose that it's more preferable to keep pinctrl-tegra.c 
>>> platform-agnostic for
>>> consistency when possible, hence adding platform specifics here 
>>> should be discouraged.
>>> And then the parked_bitmask will also result in a proper hardware 
>>> description in the code.
>>>
>>
>> I'm now also vaguely recalling that Stephen Warren had some kind of a 
>> "code generator"
>> for the pinctrl drivers. So I guess all those tables were 
>> auto-generated initially.
>>
>> Stephen, maybe you could adjust the generator to take into account 
>> the bitmask (of
>> course if that's a part of the generated code) and then re-gen it all 
>> for Sowjanya?
>
> https://github.com/NVIDIA/tegra-pinmux-scripts holds the scripts that 
> generate tegra-pinctrlNNN.c. See soc-to-kernel-pinctrl-driver.py. 
> IIRC, tegra-pinctrl.c (the core file) isn't auto-generated. Sowjanya 
> is welcome to send a patch to that repo if the code needs to be updated.


Hi Dmitry,

Just want to be clear on my understanding of your request.

"change parked_bit to parked_bitmask" are you requested to change 
parked_bit of PINGROUP and DRV_PINGROUP to use bitmask value rather than 
bit position inorder to have parked bit configuration for EMMC PADs as 
well to happen by masking rather than checking for existence of parked_bit?

Trying to understand the reason/benefit for changing parked_bit to 
parked_bitmask.


thanks

Sowjanya

Sowjanya Komatineni June 18, 2019, 5:34 p.m. UTC | #6

On 6/18/19 9:50 AM, Sowjanya Komatineni wrote:
>
> On 6/18/19 8:41 AM, Stephen Warren wrote:
>> On 6/18/19 3:30 AM, Dmitry Osipenko wrote:
>>> 18.06.2019 12:22, Dmitry Osipenko пишет:
>>>> 18.06.2019 10:46, 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@nvidia.com>
>>>>> ---
>>>>>   drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 
>>>>> ++++++++++++++++++++++++++++++++
>>>>>   drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
>>>>>   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 | 13 +++++++
>>>>>   drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
>>>>>   7 files changed, 84 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c 
>>>>> b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>> index 34596b246578..ceced30d8bd1 100644
>>>>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>> @@ -20,11 +20,16 @@
>>>>>   #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 
>>>>> reg)
>>>>>   {
>>>>>       return readl(pmx->regs[bank] + reg);
>>>>> @@ -619,6 +624,48 @@ static void 
>>>>> tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>>>>>               pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>>>>>           }
>>>>>       }
>>>>> +
>>>>> +    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);
>>>>> +    }
>>>>> +}
>>>>
>>>> Is there any reason why parked_bit can't be changed to 
>>>> parked_bitmask like I was
>>>> asking in a comment to v2?
>>>>
>>>> I suppose that it's more preferable to keep pinctrl-tegra.c 
>>>> platform-agnostic for
>>>> consistency when possible, hence adding platform specifics here 
>>>> should be discouraged.
>>>> And then the parked_bitmask will also result in a proper hardware 
>>>> description in the code.
>>>>
>>>
>>> I'm now also vaguely recalling that Stephen Warren had some kind of 
>>> a "code generator"
>>> for the pinctrl drivers. So I guess all those tables were 
>>> auto-generated initially.
>>>
>>> Stephen, maybe you could adjust the generator to take into account 
>>> the bitmask (of
>>> course if that's a part of the generated code) and then re-gen it 
>>> all for Sowjanya?
>>
>> https://github.com/NVIDIA/tegra-pinmux-scripts holds the scripts that 
>> generate tegra-pinctrlNNN.c. See soc-to-kernel-pinctrl-driver.py. 
>> IIRC, tegra-pinctrl.c (the core file) isn't auto-generated. Sowjanya 
>> is welcome to send a patch to that repo if the code needs to be updated.
>
>
> Hi Dmitry,
>
> Just want to be clear on my understanding of your request.
>
> "change parked_bit to parked_bitmask" are you requested to change 
> parked_bit of PINGROUP and DRV_PINGROUP to use bitmask value rather 
> than bit position inorder to have parked bit configuration for EMMC 
> PADs as well to happen by masking rather than checking for existence 
> of parked_bit?
>
> Trying to understand the reason/benefit for changing parked_bit to 
> parked_bitmask.
Also, Park bits in CFGPAD registers are not common for all CFGPAD 
registers. Park bits are available only for EMMC and also those bits are 
used for something else on other CFGPAD registers so bitmask can't be 
common and this also need an update to DRV_PINGROUP macro args just only 
to handle EMMC parked_bitmask. So not sure of the benefit in using 
bitmask rather than parked_bit
>
> thanks
>
> Sowjanya
>

Dmitry Osipenko June 18, 2019, 8 p.m. UTC | #7

18.06.2019 20:34, Sowjanya Komatineni пишет:
> 
> On 6/18/19 9:50 AM, Sowjanya Komatineni wrote:
>>
>> On 6/18/19 8:41 AM, Stephen Warren wrote:
>>> On 6/18/19 3:30 AM, Dmitry Osipenko wrote:
>>>> 18.06.2019 12:22, Dmitry Osipenko пишет:
>>>>> 18.06.2019 10:46, 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@nvidia.com>
>>>>>> ---
>>>>>>   drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
>>>>>>   drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
>>>>>>   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 | 13 +++++++
>>>>>>   drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
>>>>>>   7 files changed, 84 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>> b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>> index 34596b246578..ceced30d8bd1 100644
>>>>>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>> @@ -20,11 +20,16 @@
>>>>>>   #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>>>>>>   {
>>>>>>       return readl(pmx->regs[bank] + reg);
>>>>>> @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>>>>>>               pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>>>>>>           }
>>>>>>       }
>>>>>> +
>>>>>> +    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);
>>>>>> +    }
>>>>>> +}
>>>>>
>>>>> Is there any reason why parked_bit can't be changed to parked_bitmask like I was
>>>>> asking in a comment to v2?
>>>>>
>>>>> I suppose that it's more preferable to keep pinctrl-tegra.c platform-agnostic for
>>>>> consistency when possible, hence adding platform specifics here should be discouraged.
>>>>> And then the parked_bitmask will also result in a proper hardware description in the code.
>>>>>
>>>>
>>>> I'm now also vaguely recalling that Stephen Warren had some kind of a "code generator"
>>>> for the pinctrl drivers. So I guess all those tables were auto-generated initially.
>>>>
>>>> Stephen, maybe you could adjust the generator to take into account the bitmask (of
>>>> course if that's a part of the generated code) and then re-gen it all for Sowjanya?
>>>
>>> https://github.com/NVIDIA/tegra-pinmux-scripts holds the scripts that generate
>>> tegra-pinctrlNNN.c. See soc-to-kernel-pinctrl-driver.py. IIRC, tegra-pinctrl.c (the core
>>> file) isn't auto-generated. Sowjanya is welcome to send a patch to that repo if the code
>>> needs to be updated.
>>
>>
>> Hi Dmitry,
>>
>> Just want to be clear on my understanding of your request.
>>
>> "change parked_bit to parked_bitmask" are you requested to change parked_bit of PINGROUP
>> and DRV_PINGROUP to use bitmask value rather than bit position inorder to have parked bit
>> configuration for EMMC PADs as well to happen by masking rather than checking for
>> existence of parked_bit?
>>
>> Trying to understand the reason/benefit for changing parked_bit to parked_bitmask.
> Also, Park bits in CFGPAD registers are not common for all CFGPAD registers. Park bits are
> available only for EMMC and also those bits are used for something else on other CFGPAD
> registers so bitmask can't be common and this also need an update to DRV_PINGROUP macro args
> just only to handle EMMC parked_bitmask. So not sure of the benefit in using bitmask rather

Hi Sowjanya,

The main motivation is to describe hardware properly in the drivers. Why to make a
hacky-looking workaround while you can make things properly? Especially if that doesn't take
much effort.

Stephen, thank you very much for the pointer to the script. Looks like it should be easy to
modify the script accordingly to the required change.

Sowjanya, below is a draft of the change that I'm suggesting. I see this as two separate
patches: first converts drivers to use parked_bitmask, second adds suspend-resume support.

Please note that in the end it's up to you and Tegra/PINCTRL maintainers to decide if this
is a worthwhile change that I'm suggesting. In my opinion it is much better to have a
generic solution rather than to have a special quirk solely for T210.

diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
index 34596b246578..4150da74bd44 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
@@ -613,9 +613,9 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)

 	for (i = 0; i < pmx->soc->ngroups; ++i) {
 		g = &pmx->soc->groups[i];
-		if (g->parked_bit >= 0) {
+		if (g->parked_bitmask != -1) {
 			val = pmx_readl(pmx, g->mux_bank, g->mux_reg);
-			val &= ~(1 << g->parked_bit);
+			val &= ~g->parked_bitmask;
 			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
 		}
 	}
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
index 287702660783..875eb7a1d838 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra.h
+++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
@@ -96,7 +96,7 @@ struct tegra_function {
  * @tri_reg:		Tri-state register offset.
  * @tri_bank:		Tri-state register bank.
  * @tri_bit:		Tri-state register bit.
- * @parked_bit:		Parked register bit. -1 if unsupported.
+ * @parked_bitmask:	Parked register bitmask. -1 if unsupported.
  * @einput_bit:		Enable-input register bit.
  * @odrain_bit:		Open-drain register bit.
  * @lock_bit:		Lock register bit.
@@ -146,7 +146,7 @@ struct tegra_pingroup {
 	s32 mux_bit:6;
 	s32 pupd_bit:6;
 	s32 tri_bit:6;
-	s32 parked_bit:6;
+	s32 parked_bitmask:26;
 	s32 einput_bit:6;
 	s32 odrain_bit:6;
 	s32 lock_bit:6;
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c b/drivers/pinctrl/tegra/pinctrl-tegra210.c
index 0b56ad5c9c1c..d2ba13466e06 100644
--- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
+++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
@@ -1302,7 +1302,7 @@ static struct tegra_function tegra210_functions[] = {
 		.lock_bit = 7,						\
 		.ioreset_bit = -1,					\
 		.rcv_sel_bit = PINGROUP_BIT_##e_io_hv(10),		\
-		.parked_bit = 5,					\
+		.parked_bitmask = BIT(5),				\
 		.hsm_bit = PINGROUP_BIT_##hsm(9),			\
 		.schmitt_bit = 12,					\
 		.drvtype_bit = PINGROUP_BIT_##drvtype(13),		\
@@ -1320,7 +1320,7 @@ static struct tegra_function tegra210_functions[] = {
 	}

 #define DRV_PINGROUP(pg_name, r, drvdn_b, drvdn_w, drvup_b, drvup_w,	\
-		     slwr_b, slwr_w, slwf_b, slwf_w)			\
+		     slwr_b, slwr_w, slwf_b, slwf_w, prk_mask)		\
 	{								\
 		.name = "drive_" #pg_name,				\
 		.pins = drive_##pg_name##_pins,				\
@@ -1335,7 +1335,7 @@ static struct tegra_function tegra210_functions[] = {
 		.rcv_sel_bit = -1,					\
 		.drv_reg = DRV_PINGROUP_REG(r),				\
 		.drv_bank = 0,						\
-		.parked_bit = -1,					\
+		.parked_bitmask = prk_mask,				\
 		.hsm_bit = -1,						\
 		.schmitt_bit = -1,					\
 		.lpmd_bit = -1,						\
@@ -1516,31 +1516,31 @@ static const struct tegra_pingroup tegra210_groups[] = {
 	PINGROUP(pz5,                  SOC,        RSVD1,  RSVD2, RSVD3, 0x3290, N,   N,       N,
     -1,    -1,      -1,      -1,      -1,      -1,     -1,     -1,     -1),

 	/* pg_name, r, drvdn_b, drvdn_w, drvup_b, drvup_w, slwr_b, slwr_w, slwf_b, slwf_w */
-	DRV_PINGROUP(pa6,    0x9c0, 12, 5,  20, 5,  -1, -1, -1, -1),
-	DRV_PINGROUP(pcc7,   0x9c4, 12, 5,  20, 5,  -1, -1, -1, -1),
-	DRV_PINGROUP(pe6,    0x9c8, 12, 5,  20, 5,  -1, -1, -1, -1),
-	DRV_PINGROUP(pe7,    0x9cc, 12, 5,  20, 5,  -1, -1, -1, -1),
-	DRV_PINGROUP(ph6,    0x9d0, 12, 5,  20, 5,  -1, -1, -1, -1),
-	DRV_PINGROUP(pk0,    0x9d4, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pk1,    0x9d8, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pk2,    0x9dc, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pk3,    0x9e0, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pk4,    0x9e4, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pk5,    0x9e8, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pk6,    0x9ec, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pk7,    0x9f0, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pl0,    0x9f4, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pl1,    0x9f8, -1, -1, -1, -1, 28, 2,  30, 2),
-	DRV_PINGROUP(pz0,    0x9fc, 12, 7,  20, 7,  -1, -1, -1, -1),
-	DRV_PINGROUP(pz1,    0xa00, 12, 7,  20, 7,  -1, -1, -1, -1),
-	DRV_PINGROUP(pz2,    0xa04, 12, 7,  20, 7,  -1, -1, -1, -1),
-	DRV_PINGROUP(pz3,    0xa08, 12, 7,  20, 7,  -1, -1, -1, -1),
-	DRV_PINGROUP(pz4,    0xa0c, 12, 7,  20, 7,  -1, -1, -1, -1),
-	DRV_PINGROUP(pz5,    0xa10, 12, 7,  20, 7,  -1, -1, -1, -1),
-	DRV_PINGROUP(sdmmc1, 0xa98, 12, 7,  20, 7,  28, 2,  30, 2),
-	DRV_PINGROUP(sdmmc2, 0xa9c, 2,  6,  8,  6,  28, 2,  30, 2),
-	DRV_PINGROUP(sdmmc3, 0xab0, 12, 7,  20, 7,  28, 2,  30, 2),
-	DRV_PINGROUP(sdmmc4, 0xab4, 2,  6,  8,  6,  28, 2,  30, 2),
+	DRV_PINGROUP(pa6,    0x9c0, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(pcc7,   0x9c4, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(pe6,    0x9c8, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(pe7,    0x9cc, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(ph6,    0x9d0, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(pk0,    0x9d4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pk1,    0x9d8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pk2,    0x9dc, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pk3,    0x9e0, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pk4,    0x9e4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pk5,    0x9e8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pk6,    0x9ec, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pk7,    0x9f0, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pl0,    0x9f4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pl1,    0x9f8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
+	DRV_PINGROUP(pz0,    0x9fc, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(pz1,    0xa00, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(pz2,    0xa04, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(pz3,    0xa08, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(pz4,    0xa0c, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(pz5,    0xa10, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
+	DRV_PINGROUP(sdmmc1, 0xa98, 12, 7,  20, 7,  28, 2,  30, 2, -1),
+	DRV_PINGROUP(sdmmc2, 0xa9c, 2,  6,  8,  6,  28, 2,  30, 2, 0x7ffc000),
+	DRV_PINGROUP(sdmmc3, 0xab0, 12, 7,  20, 7,  28, 2,  30, 2, -1),
+	DRV_PINGROUP(sdmmc4, 0xab4, 2,  6,  8,  6,  28, 2,  30, 2, 0x7ffc000),
 };

 static const struct tegra_pinctrl_soc_data tegra210_pinctrl = {

Sowjanya Komatineni June 18, 2019, 8:04 p.m. UTC | #8

On 6/18/19 1:00 PM, Dmitry Osipenko wrote:
> 18.06.2019 20:34, Sowjanya Komatineni пишет:
>> On 6/18/19 9:50 AM, Sowjanya Komatineni wrote:
>>> On 6/18/19 8:41 AM, Stephen Warren wrote:
>>>> On 6/18/19 3:30 AM, Dmitry Osipenko wrote:
>>>>> 18.06.2019 12:22, Dmitry Osipenko пишет:
>>>>>> 18.06.2019 10:46, 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@nvidia.com>
>>>>>>> ---
>>>>>>>    drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
>>>>>>>    drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
>>>>>>>    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 | 13 +++++++
>>>>>>>    drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
>>>>>>>    7 files changed, 84 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>>> b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>>> index 34596b246578..ceced30d8bd1 100644
>>>>>>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>>> @@ -20,11 +20,16 @@
>>>>>>>    #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>>>>>>>    {
>>>>>>>        return readl(pmx->regs[bank] + reg);
>>>>>>> @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>>>>>>>                pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>>>>>>>            }
>>>>>>>        }
>>>>>>> +
>>>>>>> +    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);
>>>>>>> +    }
>>>>>>> +}
>>>>>> Is there any reason why parked_bit can't be changed to parked_bitmask like I was
>>>>>> asking in a comment to v2?
>>>>>>
>>>>>> I suppose that it's more preferable to keep pinctrl-tegra.c platform-agnostic for
>>>>>> consistency when possible, hence adding platform specifics here should be discouraged.
>>>>>> And then the parked_bitmask will also result in a proper hardware description in the code.
>>>>>>
>>>>> I'm now also vaguely recalling that Stephen Warren had some kind of a "code generator"
>>>>> for the pinctrl drivers. So I guess all those tables were auto-generated initially.
>>>>>
>>>>> Stephen, maybe you could adjust the generator to take into account the bitmask (of
>>>>> course if that's a part of the generated code) and then re-gen it all for Sowjanya?
>>>> https://github.com/NVIDIA/tegra-pinmux-scripts holds the scripts that generate
>>>> tegra-pinctrlNNN.c. See soc-to-kernel-pinctrl-driver.py. IIRC, tegra-pinctrl.c (the core
>>>> file) isn't auto-generated. Sowjanya is welcome to send a patch to that repo if the code
>>>> needs to be updated.
>>>
>>> Hi Dmitry,
>>>
>>> Just want to be clear on my understanding of your request.
>>>
>>> "change parked_bit to parked_bitmask" are you requested to change parked_bit of PINGROUP
>>> and DRV_PINGROUP to use bitmask value rather than bit position inorder to have parked bit
>>> configuration for EMMC PADs as well to happen by masking rather than checking for
>>> existence of parked_bit?
>>>
>>> Trying to understand the reason/benefit for changing parked_bit to parked_bitmask.
>> Also, Park bits in CFGPAD registers are not common for all CFGPAD registers. Park bits are
>> available only for EMMC and also those bits are used for something else on other CFGPAD
>> registers so bitmask can't be common and this also need an update to DRV_PINGROUP macro args
>> just only to handle EMMC parked_bitmask. So not sure of the benefit in using bitmask rather
> Hi Sowjanya,
>
> The main motivation is to describe hardware properly in the drivers. Why to make a
> hacky-looking workaround while you can make things properly? Especially if that doesn't take
> much effort.
>
> Stephen, thank you very much for the pointer to the script. Looks like it should be easy to
> modify the script accordingly to the required change.
>
> Sowjanya, below is a draft of the change that I'm suggesting. I see this as two separate
> patches: first converts drivers to use parked_bitmask, second adds suspend-resume support.
>
> Please note that in the end it's up to you and Tegra/PINCTRL maintainers to decide if this
> is a worthwhile change that I'm suggesting. In my opinion it is much better to have a
> generic solution rather than to have a special quirk solely for T210.

OK I can change it. Just thought to find out the reason as I see other 
pinmux field also using as bits rather than bitmask.

Got it now. Will update in next version.

>
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 34596b246578..4150da74bd44 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -613,9 +613,9 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>
>   	for (i = 0; i < pmx->soc->ngroups; ++i) {
>   		g = &pmx->soc->groups[i];
> -		if (g->parked_bit >= 0) {
> +		if (g->parked_bitmask != -1) {
>   			val = pmx_readl(pmx, g->mux_bank, g->mux_reg);
> -			val &= ~(1 << g->parked_bit);
> +			val &= ~g->parked_bitmask;
>   			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>   		}
>   	}
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
> index 287702660783..875eb7a1d838 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
> @@ -96,7 +96,7 @@ struct tegra_function {
>    * @tri_reg:		Tri-state register offset.
>    * @tri_bank:		Tri-state register bank.
>    * @tri_bit:		Tri-state register bit.
> - * @parked_bit:		Parked register bit. -1 if unsupported.
> + * @parked_bitmask:	Parked register bitmask. -1 if unsupported.
>    * @einput_bit:		Enable-input register bit.
>    * @odrain_bit:		Open-drain register bit.
>    * @lock_bit:		Lock register bit.
> @@ -146,7 +146,7 @@ struct tegra_pingroup {
>   	s32 mux_bit:6;
>   	s32 pupd_bit:6;
>   	s32 tri_bit:6;
> -	s32 parked_bit:6;
> +	s32 parked_bitmask:26;
>   	s32 einput_bit:6;
>   	s32 odrain_bit:6;
>   	s32 lock_bit:6;
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c b/drivers/pinctrl/tegra/pinctrl-tegra210.c
> index 0b56ad5c9c1c..d2ba13466e06 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
> @@ -1302,7 +1302,7 @@ static struct tegra_function tegra210_functions[] = {
>   		.lock_bit = 7,						\
>   		.ioreset_bit = -1,					\
>   		.rcv_sel_bit = PINGROUP_BIT_##e_io_hv(10),		\
> -		.parked_bit = 5,					\
> +		.parked_bitmask = BIT(5),				\
>   		.hsm_bit = PINGROUP_BIT_##hsm(9),			\
>   		.schmitt_bit = 12,					\
>   		.drvtype_bit = PINGROUP_BIT_##drvtype(13),		\
> @@ -1320,7 +1320,7 @@ static struct tegra_function tegra210_functions[] = {
>   	}
>
>   #define DRV_PINGROUP(pg_name, r, drvdn_b, drvdn_w, drvup_b, drvup_w,	\
> -		     slwr_b, slwr_w, slwf_b, slwf_w)			\
> +		     slwr_b, slwr_w, slwf_b, slwf_w, prk_mask)		\
>   	{								\
>   		.name = "drive_" #pg_name,				\
>   		.pins = drive_##pg_name##_pins,				\
> @@ -1335,7 +1335,7 @@ static struct tegra_function tegra210_functions[] = {
>   		.rcv_sel_bit = -1,					\
>   		.drv_reg = DRV_PINGROUP_REG(r),				\
>   		.drv_bank = 0,						\
> -		.parked_bit = -1,					\
> +		.parked_bitmask = prk_mask,				\
>   		.hsm_bit = -1,						\
>   		.schmitt_bit = -1,					\
>   		.lpmd_bit = -1,						\
> @@ -1516,31 +1516,31 @@ static const struct tegra_pingroup tegra210_groups[] = {
>   	PINGROUP(pz5,                  SOC,        RSVD1,  RSVD2, RSVD3, 0x3290, N,   N,       N,
>       -1,    -1,      -1,      -1,      -1,      -1,     -1,     -1,     -1),
>
>   	/* pg_name, r, drvdn_b, drvdn_w, drvup_b, drvup_w, slwr_b, slwr_w, slwf_b, slwf_w */
> -	DRV_PINGROUP(pa6,    0x9c0, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pcc7,   0x9c4, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pe6,    0x9c8, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pe7,    0x9cc, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(ph6,    0x9d0, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pk0,    0x9d4, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk1,    0x9d8, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk2,    0x9dc, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk3,    0x9e0, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk4,    0x9e4, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk5,    0x9e8, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk6,    0x9ec, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk7,    0x9f0, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pl0,    0x9f4, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pl1,    0x9f8, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pz0,    0x9fc, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz1,    0xa00, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz2,    0xa04, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz3,    0xa08, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz4,    0xa0c, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz5,    0xa10, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(sdmmc1, 0xa98, 12, 7,  20, 7,  28, 2,  30, 2),
> -	DRV_PINGROUP(sdmmc2, 0xa9c, 2,  6,  8,  6,  28, 2,  30, 2),
> -	DRV_PINGROUP(sdmmc3, 0xab0, 12, 7,  20, 7,  28, 2,  30, 2),
> -	DRV_PINGROUP(sdmmc4, 0xab4, 2,  6,  8,  6,  28, 2,  30, 2),
> +	DRV_PINGROUP(pa6,    0x9c0, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pcc7,   0x9c4, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pe6,    0x9c8, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pe7,    0x9cc, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(ph6,    0x9d0, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pk0,    0x9d4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk1,    0x9d8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk2,    0x9dc, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk3,    0x9e0, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk4,    0x9e4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk5,    0x9e8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk6,    0x9ec, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk7,    0x9f0, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pl0,    0x9f4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pl1,    0x9f8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pz0,    0x9fc, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz1,    0xa00, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz2,    0xa04, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz3,    0xa08, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz4,    0xa0c, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz5,    0xa10, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(sdmmc1, 0xa98, 12, 7,  20, 7,  28, 2,  30, 2, -1),
> +	DRV_PINGROUP(sdmmc2, 0xa9c, 2,  6,  8,  6,  28, 2,  30, 2, 0x7ffc000),
> +	DRV_PINGROUP(sdmmc3, 0xab0, 12, 7,  20, 7,  28, 2,  30, 2, -1),
> +	DRV_PINGROUP(sdmmc4, 0xab4, 2,  6,  8,  6,  28, 2,  30, 2, 0x7ffc000),
>   };
>
>   static const struct tegra_pinctrl_soc_data tegra210_pinctrl = {

Thierry Reding June 19, 2019, 8:31 a.m. UTC | #9

On Tue, Jun 18, 2019 at 11:00:05PM +0300, Dmitry Osipenko wrote:
> 18.06.2019 20:34, Sowjanya Komatineni пишет:
> > 
> > On 6/18/19 9:50 AM, Sowjanya Komatineni wrote:
> >>
> >> On 6/18/19 8:41 AM, Stephen Warren wrote:
> >>> On 6/18/19 3:30 AM, Dmitry Osipenko wrote:
> >>>> 18.06.2019 12:22, Dmitry Osipenko пишет:
> >>>>> 18.06.2019 10:46, 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@nvidia.com>
> >>>>>> ---
> >>>>>>   drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
> >>>>>>   drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
> >>>>>>   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 | 13 +++++++
> >>>>>>   drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
> >>>>>>   7 files changed, 84 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
> >>>>>> b/drivers/pinctrl/tegra/pinctrl-tegra.c
> >>>>>> index 34596b246578..ceced30d8bd1 100644
> >>>>>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> >>>>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> >>>>>> @@ -20,11 +20,16 @@
> >>>>>>   #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
> >>>>>>   {
> >>>>>>       return readl(pmx->regs[bank] + reg);
> >>>>>> @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
> >>>>>>               pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
> >>>>>>           }
> >>>>>>       }
> >>>>>> +
> >>>>>> +    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);
> >>>>>> +    }
> >>>>>> +}
> >>>>>
> >>>>> Is there any reason why parked_bit can't be changed to parked_bitmask like I was
> >>>>> asking in a comment to v2?
> >>>>>
> >>>>> I suppose that it's more preferable to keep pinctrl-tegra.c platform-agnostic for
> >>>>> consistency when possible, hence adding platform specifics here should be discouraged.
> >>>>> And then the parked_bitmask will also result in a proper hardware description in the code.
> >>>>>
> >>>>
> >>>> I'm now also vaguely recalling that Stephen Warren had some kind of a "code generator"
> >>>> for the pinctrl drivers. So I guess all those tables were auto-generated initially.
> >>>>
> >>>> Stephen, maybe you could adjust the generator to take into account the bitmask (of
> >>>> course if that's a part of the generated code) and then re-gen it all for Sowjanya?
> >>>
> >>> https://github.com/NVIDIA/tegra-pinmux-scripts holds the scripts that generate
> >>> tegra-pinctrlNNN.c. See soc-to-kernel-pinctrl-driver.py. IIRC, tegra-pinctrl.c (the core
> >>> file) isn't auto-generated. Sowjanya is welcome to send a patch to that repo if the code
> >>> needs to be updated.
> >>
> >>
> >> Hi Dmitry,
> >>
> >> Just want to be clear on my understanding of your request.
> >>
> >> "change parked_bit to parked_bitmask" are you requested to change parked_bit of PINGROUP
> >> and DRV_PINGROUP to use bitmask value rather than bit position inorder to have parked bit
> >> configuration for EMMC PADs as well to happen by masking rather than checking for
> >> existence of parked_bit?
> >>
> >> Trying to understand the reason/benefit for changing parked_bit to parked_bitmask.
> > Also, Park bits in CFGPAD registers are not common for all CFGPAD registers. Park bits are
> > available only for EMMC and also those bits are used for something else on other CFGPAD
> > registers so bitmask can't be common and this also need an update to DRV_PINGROUP macro args
> > just only to handle EMMC parked_bitmask. So not sure of the benefit in using bitmask rather
> 
> Hi Sowjanya,
> 
> The main motivation is to describe hardware properly in the drivers. Why to make a
> hacky-looking workaround while you can make things properly? Especially if that doesn't take
> much effort.
> 
> Stephen, thank you very much for the pointer to the script. Looks like it should be easy to
> modify the script accordingly to the required change.
> 
> Sowjanya, below is a draft of the change that I'm suggesting. I see this as two separate
> patches: first converts drivers to use parked_bitmask, second adds suspend-resume support.
> 
> Please note that in the end it's up to you and Tegra/PINCTRL maintainers to decide if this
> is a worthwhile change that I'm suggesting. In my opinion it is much better to have a
> generic solution rather than to have a special quirk solely for T210.
> 
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> index 34596b246578..4150da74bd44 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> @@ -613,9 +613,9 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
> 
>  	for (i = 0; i < pmx->soc->ngroups; ++i) {
>  		g = &pmx->soc->groups[i];
> -		if (g->parked_bit >= 0) {
> +		if (g->parked_bitmask != -1) {
>  			val = pmx_readl(pmx, g->mux_bank, g->mux_reg);
> -			val &= ~(1 << g->parked_bit);
> +			val &= ~g->parked_bitmask;
>  			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>  		}
>  	}
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
> index 287702660783..875eb7a1d838 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
> @@ -96,7 +96,7 @@ struct tegra_function {
>   * @tri_reg:		Tri-state register offset.
>   * @tri_bank:		Tri-state register bank.
>   * @tri_bit:		Tri-state register bit.
> - * @parked_bit:		Parked register bit. -1 if unsupported.
> + * @parked_bitmask:	Parked register bitmask. -1 if unsupported.

If we're already moving to a bitmask, wouldn't it be easier to just make
0 the case where it is unsupported?

>   * @einput_bit:		Enable-input register bit.
>   * @odrain_bit:		Open-drain register bit.
>   * @lock_bit:		Lock register bit.
> @@ -146,7 +146,7 @@ struct tegra_pingroup {
>  	s32 mux_bit:6;
>  	s32 pupd_bit:6;
>  	s32 tri_bit:6;
> -	s32 parked_bit:6;
> +	s32 parked_bitmask:26;

If we make parked_bitmask == 0 the case for "unsupported" we could make
this u32 while at it.

>  	s32 einput_bit:6;
>  	s32 odrain_bit:6;
>  	s32 lock_bit:6;
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c b/drivers/pinctrl/tegra/pinctrl-tegra210.c
> index 0b56ad5c9c1c..d2ba13466e06 100644
> --- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
> @@ -1302,7 +1302,7 @@ static struct tegra_function tegra210_functions[] = {
>  		.lock_bit = 7,						\
>  		.ioreset_bit = -1,					\
>  		.rcv_sel_bit = PINGROUP_BIT_##e_io_hv(10),		\
> -		.parked_bit = 5,					\
> +		.parked_bitmask = BIT(5),				\
>  		.hsm_bit = PINGROUP_BIT_##hsm(9),			\
>  		.schmitt_bit = 12,					\
>  		.drvtype_bit = PINGROUP_BIT_##drvtype(13),		\
> @@ -1320,7 +1320,7 @@ static struct tegra_function tegra210_functions[] = {
>  	}
> 
>  #define DRV_PINGROUP(pg_name, r, drvdn_b, drvdn_w, drvup_b, drvup_w,	\
> -		     slwr_b, slwr_w, slwf_b, slwf_w)			\
> +		     slwr_b, slwr_w, slwf_b, slwf_w, prk_mask)		\
>  	{								\
>  		.name = "drive_" #pg_name,				\
>  		.pins = drive_##pg_name##_pins,				\
> @@ -1335,7 +1335,7 @@ static struct tegra_function tegra210_functions[] = {
>  		.rcv_sel_bit = -1,					\
>  		.drv_reg = DRV_PINGROUP_REG(r),				\
>  		.drv_bank = 0,						\
> -		.parked_bit = -1,					\
> +		.parked_bitmask = prk_mask,				\
>  		.hsm_bit = -1,						\
>  		.schmitt_bit = -1,					\
>  		.lpmd_bit = -1,						\
> @@ -1516,31 +1516,31 @@ static const struct tegra_pingroup tegra210_groups[] = {
>  	PINGROUP(pz5,                  SOC,        RSVD1,  RSVD2, RSVD3, 0x3290, N,   N,       N,
>      -1,    -1,      -1,      -1,      -1,      -1,     -1,     -1,     -1),
> 
>  	/* pg_name, r, drvdn_b, drvdn_w, drvup_b, drvup_w, slwr_b, slwr_w, slwf_b, slwf_w */
> -	DRV_PINGROUP(pa6,    0x9c0, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pcc7,   0x9c4, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pe6,    0x9c8, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pe7,    0x9cc, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(ph6,    0x9d0, 12, 5,  20, 5,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pk0,    0x9d4, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk1,    0x9d8, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk2,    0x9dc, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk3,    0x9e0, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk4,    0x9e4, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk5,    0x9e8, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk6,    0x9ec, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pk7,    0x9f0, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pl0,    0x9f4, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pl1,    0x9f8, -1, -1, -1, -1, 28, 2,  30, 2),
> -	DRV_PINGROUP(pz0,    0x9fc, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz1,    0xa00, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz2,    0xa04, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz3,    0xa08, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz4,    0xa0c, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(pz5,    0xa10, 12, 7,  20, 7,  -1, -1, -1, -1),
> -	DRV_PINGROUP(sdmmc1, 0xa98, 12, 7,  20, 7,  28, 2,  30, 2),
> -	DRV_PINGROUP(sdmmc2, 0xa9c, 2,  6,  8,  6,  28, 2,  30, 2),
> -	DRV_PINGROUP(sdmmc3, 0xab0, 12, 7,  20, 7,  28, 2,  30, 2),
> -	DRV_PINGROUP(sdmmc4, 0xab4, 2,  6,  8,  6,  28, 2,  30, 2),
> +	DRV_PINGROUP(pa6,    0x9c0, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pcc7,   0x9c4, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pe6,    0x9c8, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pe7,    0x9cc, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(ph6,    0x9d0, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pk0,    0x9d4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk1,    0x9d8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk2,    0x9dc, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk3,    0x9e0, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk4,    0x9e4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk5,    0x9e8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk6,    0x9ec, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pk7,    0x9f0, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pl0,    0x9f4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pl1,    0x9f8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
> +	DRV_PINGROUP(pz0,    0x9fc, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz1,    0xa00, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz2,    0xa04, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz3,    0xa08, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz4,    0xa0c, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(pz5,    0xa10, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
> +	DRV_PINGROUP(sdmmc1, 0xa98, 12, 7,  20, 7,  28, 2,  30, 2, -1),
> +	DRV_PINGROUP(sdmmc2, 0xa9c, 2,  6,  8,  6,  28, 2,  30, 2, 0x7ffc000),
> +	DRV_PINGROUP(sdmmc3, 0xab0, 12, 7,  20, 7,  28, 2,  30, 2, -1),
> +	DRV_PINGROUP(sdmmc4, 0xab4, 2,  6,  8,  6,  28, 2,  30, 2, 0x7ffc000),

Might be worth adding a new DRV_PINGROUP_PARK (or whatever) macro that
takes the additional parameter. that way we could avoid the extra churn.

Thierry

>  };
> 
>  static const struct tegra_pinctrl_soc_data tegra210_pinctrl = {

Thierry Reding June 19, 2019, 8:33 a.m. UTC | #10

On Tue, Jun 18, 2019 at 09:41:03AM -0600, Stephen Warren wrote:
> On 6/18/19 3:30 AM, Dmitry Osipenko wrote:
> > 18.06.2019 12:22, Dmitry Osipenko пишет:
> > > 18.06.2019 10:46, 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@nvidia.com>
> > > > ---
> > > >   drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
> > > >   drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
> > > >   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 | 13 +++++++
> > > >   drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
> > > >   7 files changed, 84 insertions(+)
> > > > 
> > > > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > index 34596b246578..ceced30d8bd1 100644
> > > > --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > @@ -20,11 +20,16 @@
> > > >   #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
> > > >   {
> > > >   	return readl(pmx->regs[bank] + reg);
> > > > @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
> > > >   			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
> > > >   		}
> > > >   	}
> > > > +
> > > > +	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);
> > > > +	}
> > > > +}
> > > 
> > > Is there any reason why parked_bit can't be changed to parked_bitmask like I was
> > > asking in a comment to v2?
> > > 
> > > I suppose that it's more preferable to keep pinctrl-tegra.c platform-agnostic for
> > > consistency when possible, hence adding platform specifics here should be discouraged.
> > > And then the parked_bitmask will also result in a proper hardware description in the code.
> > > 
> > 
> > I'm now also vaguely recalling that Stephen Warren had some kind of a "code generator"
> > for the pinctrl drivers. So I guess all those tables were auto-generated initially.
> > 
> > Stephen, maybe you could adjust the generator to take into account the bitmask (of
> > course if that's a part of the generated code) and then re-gen it all for Sowjanya?
> 
> https://github.com/NVIDIA/tegra-pinmux-scripts holds the scripts that
> generate tegra-pinctrlNNN.c. See  	soc-to-kernel-pinctrl-driver.py. IIRC,
> tegra-pinctrl.c (the core file) isn't auto-generated. Sowjanya is welcome to
> send a patch to that repo if the code needs to be updated.

If we want to do that, we may need to start off by bringing the pinmux
scripts up to date with the latest version of the generated files. There
have been a number of changes in the meantime that cause the scripts to
generate a bit of diff with regards to what's currently upstream. Sounds
like something fairly trivial, though.

Thierry

Dmitry Osipenko June 19, 2019, 8:40 a.m. UTC | #11

19.06.2019 11:31, Thierry Reding пишет:
> On Tue, Jun 18, 2019 at 11:00:05PM +0300, Dmitry Osipenko wrote:
>> 18.06.2019 20:34, Sowjanya Komatineni пишет:
>>>
>>> On 6/18/19 9:50 AM, Sowjanya Komatineni wrote:
>>>>
>>>> On 6/18/19 8:41 AM, Stephen Warren wrote:
>>>>> On 6/18/19 3:30 AM, Dmitry Osipenko wrote:
>>>>>> 18.06.2019 12:22, Dmitry Osipenko пишет:
>>>>>>> 18.06.2019 10:46, 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@nvidia.com>
>>>>>>>> ---
>>>>>>>>   drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
>>>>>>>>   drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
>>>>>>>>   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 | 13 +++++++
>>>>>>>>   drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
>>>>>>>>   7 files changed, 84 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>>>> b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>>>> index 34596b246578..ceced30d8bd1 100644
>>>>>>>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>>>>>>>> @@ -20,11 +20,16 @@
>>>>>>>>   #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
>>>>>>>>   {
>>>>>>>>       return readl(pmx->regs[bank] + reg);
>>>>>>>> @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>>>>>>>>               pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>>>>>>>>           }
>>>>>>>>       }
>>>>>>>> +
>>>>>>>> +    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);
>>>>>>>> +    }
>>>>>>>> +}
>>>>>>>
>>>>>>> Is there any reason why parked_bit can't be changed to parked_bitmask like I was
>>>>>>> asking in a comment to v2?
>>>>>>>
>>>>>>> I suppose that it's more preferable to keep pinctrl-tegra.c platform-agnostic for
>>>>>>> consistency when possible, hence adding platform specifics here should be discouraged.
>>>>>>> And then the parked_bitmask will also result in a proper hardware description in the code.
>>>>>>>
>>>>>>
>>>>>> I'm now also vaguely recalling that Stephen Warren had some kind of a "code generator"
>>>>>> for the pinctrl drivers. So I guess all those tables were auto-generated initially.
>>>>>>
>>>>>> Stephen, maybe you could adjust the generator to take into account the bitmask (of
>>>>>> course if that's a part of the generated code) and then re-gen it all for Sowjanya?
>>>>>
>>>>> https://github.com/NVIDIA/tegra-pinmux-scripts holds the scripts that generate
>>>>> tegra-pinctrlNNN.c. See soc-to-kernel-pinctrl-driver.py. IIRC, tegra-pinctrl.c (the core
>>>>> file) isn't auto-generated. Sowjanya is welcome to send a patch to that repo if the code
>>>>> needs to be updated.
>>>>
>>>>
>>>> Hi Dmitry,
>>>>
>>>> Just want to be clear on my understanding of your request.
>>>>
>>>> "change parked_bit to parked_bitmask" are you requested to change parked_bit of PINGROUP
>>>> and DRV_PINGROUP to use bitmask value rather than bit position inorder to have parked bit
>>>> configuration for EMMC PADs as well to happen by masking rather than checking for
>>>> existence of parked_bit?
>>>>
>>>> Trying to understand the reason/benefit for changing parked_bit to parked_bitmask.
>>> Also, Park bits in CFGPAD registers are not common for all CFGPAD registers. Park bits are
>>> available only for EMMC and also those bits are used for something else on other CFGPAD
>>> registers so bitmask can't be common and this also need an update to DRV_PINGROUP macro args
>>> just only to handle EMMC parked_bitmask. So not sure of the benefit in using bitmask rather
>>
>> Hi Sowjanya,
>>
>> The main motivation is to describe hardware properly in the drivers. Why to make a
>> hacky-looking workaround while you can make things properly? Especially if that doesn't take
>> much effort.
>>
>> Stephen, thank you very much for the pointer to the script. Looks like it should be easy to
>> modify the script accordingly to the required change.
>>
>> Sowjanya, below is a draft of the change that I'm suggesting. I see this as two separate
>> patches: first converts drivers to use parked_bitmask, second adds suspend-resume support.
>>
>> Please note that in the end it's up to you and Tegra/PINCTRL maintainers to decide if this
>> is a worthwhile change that I'm suggesting. In my opinion it is much better to have a
>> generic solution rather than to have a special quirk solely for T210.
>>
>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
>> index 34596b246578..4150da74bd44 100644
>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
>> @@ -613,9 +613,9 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
>>
>>  	for (i = 0; i < pmx->soc->ngroups; ++i) {
>>  		g = &pmx->soc->groups[i];
>> -		if (g->parked_bit >= 0) {
>> +		if (g->parked_bitmask != -1) {
>>  			val = pmx_readl(pmx, g->mux_bank, g->mux_reg);
>> -			val &= ~(1 << g->parked_bit);
>> +			val &= ~g->parked_bitmask;
>>  			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
>>  		}
>>  	}
>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.h b/drivers/pinctrl/tegra/pinctrl-tegra.h
>> index 287702660783..875eb7a1d838 100644
>> --- a/drivers/pinctrl/tegra/pinctrl-tegra.h
>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra.h
>> @@ -96,7 +96,7 @@ struct tegra_function {
>>   * @tri_reg:		Tri-state register offset.
>>   * @tri_bank:		Tri-state register bank.
>>   * @tri_bit:		Tri-state register bit.
>> - * @parked_bit:		Parked register bit. -1 if unsupported.
>> + * @parked_bitmask:	Parked register bitmask. -1 if unsupported.
> 
> If we're already moving to a bitmask, wouldn't it be easier to just make
> 0 the case where it is unsupported?
> 
>>   * @einput_bit:		Enable-input register bit.
>>   * @odrain_bit:		Open-drain register bit.
>>   * @lock_bit:		Lock register bit.
>> @@ -146,7 +146,7 @@ struct tegra_pingroup {
>>  	s32 mux_bit:6;
>>  	s32 pupd_bit:6;
>>  	s32 tri_bit:6;
>> -	s32 parked_bit:6;
>> +	s32 parked_bitmask:26;
> 
> If we make parked_bitmask == 0 the case for "unsupported" we could make
> this u32 while at it.
> 
>>  	s32 einput_bit:6;
>>  	s32 odrain_bit:6;
>>  	s32 lock_bit:6;
>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210.c b/drivers/pinctrl/tegra/pinctrl-tegra210.c
>> index 0b56ad5c9c1c..d2ba13466e06 100644
>> --- a/drivers/pinctrl/tegra/pinctrl-tegra210.c
>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra210.c
>> @@ -1302,7 +1302,7 @@ static struct tegra_function tegra210_functions[] = {
>>  		.lock_bit = 7,						\
>>  		.ioreset_bit = -1,					\
>>  		.rcv_sel_bit = PINGROUP_BIT_##e_io_hv(10),		\
>> -		.parked_bit = 5,					\
>> +		.parked_bitmask = BIT(5),				\
>>  		.hsm_bit = PINGROUP_BIT_##hsm(9),			\
>>  		.schmitt_bit = 12,					\
>>  		.drvtype_bit = PINGROUP_BIT_##drvtype(13),		\
>> @@ -1320,7 +1320,7 @@ static struct tegra_function tegra210_functions[] = {
>>  	}
>>
>>  #define DRV_PINGROUP(pg_name, r, drvdn_b, drvdn_w, drvup_b, drvup_w,	\
>> -		     slwr_b, slwr_w, slwf_b, slwf_w)			\
>> +		     slwr_b, slwr_w, slwf_b, slwf_w, prk_mask)		\
>>  	{								\
>>  		.name = "drive_" #pg_name,				\
>>  		.pins = drive_##pg_name##_pins,				\
>> @@ -1335,7 +1335,7 @@ static struct tegra_function tegra210_functions[] = {
>>  		.rcv_sel_bit = -1,					\
>>  		.drv_reg = DRV_PINGROUP_REG(r),				\
>>  		.drv_bank = 0,						\
>> -		.parked_bit = -1,					\
>> +		.parked_bitmask = prk_mask,				\
>>  		.hsm_bit = -1,						\
>>  		.schmitt_bit = -1,					\
>>  		.lpmd_bit = -1,						\
>> @@ -1516,31 +1516,31 @@ static const struct tegra_pingroup tegra210_groups[] = {
>>  	PINGROUP(pz5,                  SOC,        RSVD1,  RSVD2, RSVD3, 0x3290, N,   N,       N,
>>      -1,    -1,      -1,      -1,      -1,      -1,     -1,     -1,     -1),
>>
>>  	/* pg_name, r, drvdn_b, drvdn_w, drvup_b, drvup_w, slwr_b, slwr_w, slwf_b, slwf_w */
>> -	DRV_PINGROUP(pa6,    0x9c0, 12, 5,  20, 5,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(pcc7,   0x9c4, 12, 5,  20, 5,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(pe6,    0x9c8, 12, 5,  20, 5,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(pe7,    0x9cc, 12, 5,  20, 5,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(ph6,    0x9d0, 12, 5,  20, 5,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(pk0,    0x9d4, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pk1,    0x9d8, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pk2,    0x9dc, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pk3,    0x9e0, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pk4,    0x9e4, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pk5,    0x9e8, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pk6,    0x9ec, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pk7,    0x9f0, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pl0,    0x9f4, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pl1,    0x9f8, -1, -1, -1, -1, 28, 2,  30, 2),
>> -	DRV_PINGROUP(pz0,    0x9fc, 12, 7,  20, 7,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(pz1,    0xa00, 12, 7,  20, 7,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(pz2,    0xa04, 12, 7,  20, 7,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(pz3,    0xa08, 12, 7,  20, 7,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(pz4,    0xa0c, 12, 7,  20, 7,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(pz5,    0xa10, 12, 7,  20, 7,  -1, -1, -1, -1),
>> -	DRV_PINGROUP(sdmmc1, 0xa98, 12, 7,  20, 7,  28, 2,  30, 2),
>> -	DRV_PINGROUP(sdmmc2, 0xa9c, 2,  6,  8,  6,  28, 2,  30, 2),
>> -	DRV_PINGROUP(sdmmc3, 0xab0, 12, 7,  20, 7,  28, 2,  30, 2),
>> -	DRV_PINGROUP(sdmmc4, 0xab4, 2,  6,  8,  6,  28, 2,  30, 2),
>> +	DRV_PINGROUP(pa6,    0x9c0, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(pcc7,   0x9c4, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(pe6,    0x9c8, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(pe7,    0x9cc, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(ph6,    0x9d0, 12, 5,  20, 5,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(pk0,    0x9d4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pk1,    0x9d8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pk2,    0x9dc, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pk3,    0x9e0, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pk4,    0x9e4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pk5,    0x9e8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pk6,    0x9ec, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pk7,    0x9f0, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pl0,    0x9f4, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pl1,    0x9f8, -1, -1, -1, -1, 28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(pz0,    0x9fc, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(pz1,    0xa00, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(pz2,    0xa04, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(pz3,    0xa08, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(pz4,    0xa0c, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(pz5,    0xa10, 12, 7,  20, 7,  -1, -1, -1, -1, -1),
>> +	DRV_PINGROUP(sdmmc1, 0xa98, 12, 7,  20, 7,  28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(sdmmc2, 0xa9c, 2,  6,  8,  6,  28, 2,  30, 2, 0x7ffc000),
>> +	DRV_PINGROUP(sdmmc3, 0xab0, 12, 7,  20, 7,  28, 2,  30, 2, -1),
>> +	DRV_PINGROUP(sdmmc4, 0xab4, 2,  6,  8,  6,  28, 2,  30, 2, 0x7ffc000),
> 
> Might be worth adding a new DRV_PINGROUP_PARK (or whatever) macro that
> takes the additional parameter. that way we could avoid the extra churn.

Sounds like a very good call! +1

Thierry Reding June 19, 2019, 8:57 a.m. UTC | #12

On Wed, Jun 19, 2019 at 10:33:08AM +0200, Thierry Reding wrote:
> On Tue, Jun 18, 2019 at 09:41:03AM -0600, Stephen Warren wrote:
> > On 6/18/19 3:30 AM, Dmitry Osipenko wrote:
> > > 18.06.2019 12:22, Dmitry Osipenko пишет:
> > > > 18.06.2019 10:46, 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@nvidia.com>
> > > > > ---
> > > > >   drivers/pinctrl/tegra/pinctrl-tegra.c    | 62 ++++++++++++++++++++++++++++++++
> > > > >   drivers/pinctrl/tegra/pinctrl-tegra.h    |  5 +++
> > > > >   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 | 13 +++++++
> > > > >   drivers/pinctrl/tegra/pinctrl-tegra30.c  |  1 +
> > > > >   7 files changed, 84 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/pinctrl/tegra/pinctrl-tegra.c b/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > > index 34596b246578..ceced30d8bd1 100644
> > > > > --- a/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > > +++ b/drivers/pinctrl/tegra/pinctrl-tegra.c
> > > > > @@ -20,11 +20,16 @@
> > > > >   #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 inline u32 pmx_readl(struct tegra_pmx *pmx, u32 bank, u32 reg)
> > > > >   {
> > > > >   	return readl(pmx->regs[bank] + reg);
> > > > > @@ -619,6 +624,48 @@ static void tegra_pinctrl_clear_parked_bits(struct tegra_pmx *pmx)
> > > > >   			pmx_writel(pmx, val, g->mux_bank, g->mux_reg);
> > > > >   		}
> > > > >   	}
> > > > > +
> > > > > +	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);
> > > > > +	}
> > > > > +}
> > > > 
> > > > Is there any reason why parked_bit can't be changed to parked_bitmask like I was
> > > > asking in a comment to v2?
> > > > 
> > > > I suppose that it's more preferable to keep pinctrl-tegra.c platform-agnostic for
> > > > consistency when possible, hence adding platform specifics here should be discouraged.
> > > > And then the parked_bitmask will also result in a proper hardware description in the code.
> > > > 
> > > 
> > > I'm now also vaguely recalling that Stephen Warren had some kind of a "code generator"
> > > for the pinctrl drivers. So I guess all those tables were auto-generated initially.
> > > 
> > > Stephen, maybe you could adjust the generator to take into account the bitmask (of
> > > course if that's a part of the generated code) and then re-gen it all for Sowjanya?
> > 
> > https://github.com/NVIDIA/tegra-pinmux-scripts holds the scripts that
> > generate tegra-pinctrlNNN.c. See  	soc-to-kernel-pinctrl-driver.py. IIRC,
> > tegra-pinctrl.c (the core file) isn't auto-generated. Sowjanya is welcome to
> > send a patch to that repo if the code needs to be updated.
> 
> If we want to do that, we may need to start off by bringing the pinmux
> scripts up to date with the latest version of the generated files. There
> have been a number of changes in the meantime that cause the scripts to
> generate a bit of diff with regards to what's currently upstream. Sounds
> like something fairly trivial, though.

Something like the below should do the trick.

Thierry

--- >8 ---
From 9a684d2ad3c0e0c7b4dbda5904db1fda3757072b Mon Sep 17 00:00:00 2001
From: Thierry Reding <treding@nvidia.com>
Date: Wed, 19 Jun 2019 10:50:57 +0200
Subject: [pinmux scripts PATCH] Update kernel driver template

Some changes in recent years have modified the upstream kernel driver in
some ways that make it incompatible with the current template. Update
the template to take into account changes introduced by the following
commits:

	commit e3d2160f12d6aa7a87d9db09d8458b4a3492cd45
	Author: Paul Gortmaker <paul.gortmaker@windriver.com>
	Date:   Mon May 22 16:56:47 2017 -0400

	    pinctrl: tegra: clean up modular vs. non-modular distinctions

	    None of the Kconfigs for any of these drivers are tristate,
	    meaning that they currently are not being built as a module by anyone.

	    Lets remove the modular code that is essentially orphaned, so that
	    when reading the drivers there is no doubt they are builtin-only.  All
	    drivers get similar changes, so they are handled in batch.

	    We remove module.h from code that isn't doing anything modular at
	    all;  if they have __init sections, then replace it with init.h.

	    A couple drivers have module_exit() code that is essentially orphaned,
	    and so we remove that.

	    Quite a few bool drivers (hence non-modular) are converted over to
	    to builtin_platform_driver().

	    Since module_platform_driver() uses the same init level priority as
	    builtin_platform_driver() the init ordering remains unchanged with
	    this commit.

	    Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.

	    We also delete the MODULE_LICENSE tag etc. since all that information
	    was (or is now) contained at the top of the file in the comments.

	    Cc: Linus Walleij <linus.walleij@linaro.org>
	    Cc: Stephen Warren <swarren@wwwdotorg.org>
	    Cc: Thierry Reding <thierry.reding@gmail.com>
	    Cc: Alexandre Courbot <gnurou@gmail.com>
	    Cc: Pritesh Raithatha <praithatha@nvidia.com>
	    Cc: Ashwini Ghuge <aghuge@nvidia.com>
	    Cc: linux-gpio@vger.kernel.org
	    Cc: linux-tegra@vger.kernel.org
	    Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
	    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

	commit 3c94d2d08a032d911bbe34f2edb24cb63a63644a
	Author: Stefan Agner <stefan@agner.ch>
	Date:   Thu Jul 26 17:40:24 2018 +0200

	    pinctrl: tegra: define GPIO compatible node per SoC

	    Tegra 2 uses a different GPIO controller which uses "tegra20-gpio" as
	    compatible string.

	    Make the compatible string the GPIO node is using a SoC specific
	    property. This prevents the kernel from registering the GPIO range
	    twice in case the GPIO range is specified in the device tree.

	    Fixes: 9462510ce31e ("pinctrl: tegra: Only set the gpio range if needed")
	    Signed-off-by: Stefan Agner <stefan@agner.ch>
	    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

	commit 1e0813ee5599932c856bda64a568895ed7a33d3a
	Author: Dmitry Osipenko <digetx@gmail.com>
	Date:   Thu Aug 2 14:11:43 2018 +0300

	    pinctrl: tegra: Move drivers registration to arch_init level

	    There is a bug in regards to deferred probing within the drivers core
	    that causes GPIO-driver to suspend after its users. The bug appears if
	    GPIO-driver probe is getting deferred, which happens after introducing
	    dependency on PINCTRL-driver for the GPIO-driver by defining "gpio-ranges"
	    property in device-tree. The bug in the drivers core is old (more than 4
	    years now) and is well known, unfortunately there is no easy fix for it.
	    The good news is that we can workaround the deferred probe issue by
	    changing GPIO / PINCTRL drivers registration order and hence by moving
	    PINCTRL driver registration to the arch_init level and GPIO to the
	    subsys_init.

	    Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
	    Acked-by: Stefan Agner <stefan@agner.ch>
	    Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Note that the last one is something that we probably should fix
correctly by using device links rather than working around it by playing
init level tricks.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 soc-to-kernel-pinctrl-driver.py | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/soc-to-kernel-pinctrl-driver.py b/soc-to-kernel-pinctrl-driver.py
index 65e4c604f1c9..37f34b15db2b 100755
--- a/soc-to-kernel-pinctrl-driver.py
+++ b/soc-to-kernel-pinctrl-driver.py
@@ -41,22 +41,16 @@ if dbg: print(args)
 soc = tegra_pmx_soc_parser.load_soc(args.soc)
 
 print('''\
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Pinctrl data for the NVIDIA %s pinmux
  *
- * Copyright (c) %s, NVIDIA CORPORATION.  All rights reserved.
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
+ * Author: %s
  *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
+ * Copyright (c) %s, NVIDIA CORPORATION.  All rights reserved.
  */
 
-#include <linux/module.h>
+#include <linux/init.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -68,7 +62,7 @@ print('''\
  * Most pins affected by the pinmux can also be GPIOs. Define these first.
  * These must match how the GPIO driver names/numbers its pins.
  */
-''' % (soc.titlename, soc.kernel_copyright_years), end='')
+''' % (soc.titlename, soc.kernel_author, soc.kernel_copyright_years), end='')
 
 # Do not add any more exceptions here; new SoCs should be formatted correctly
 if soc.name == 'tegra30':
@@ -615,6 +609,7 @@ print('''\
 
 static const struct tegra_pinctrl_soc_data %(soc)s_pinctrl = {
 	.ngpios = NUM_GPIOS,
+	.gpio_compatible = "nvidia,%(soc)s-gpio",
 	.pins = %(soc)s_pins,
 	.npins = ARRAY_SIZE(%(soc)s_pins),
 	.functions = %(soc)s_functions,
@@ -635,7 +630,6 @@ static const struct of_device_id %(soc)s_pinctrl_of_match[] = {
 	{ .compatible = "nvidia,%(soc)s-pinmux", },
 	{ },
 };
-MODULE_DEVICE_TABLE(of, %(soc)s_pinctrl_of_match);
 
 static struct platform_driver %(soc)s_pinctrl_driver = {
 	.driver = {
@@ -644,9 +638,10 @@ static struct platform_driver %(soc)s_pinctrl_driver = {
 	},
 	.probe = %(soc)s_pinctrl_probe,
 };
-module_platform_driver(%(soc)s_pinctrl_driver);
 
-MODULE_AUTHOR("%(author)s");
-MODULE_DESCRIPTION("NVIDIA %(usoc)s pinctrl driver");
-MODULE_LICENSE("GPL v2");
+static int __init %(soc)s_pinctrl_init(void)
+{
+	return platform_driver_register(&%(soc)s_pinctrl_driver);
+}
+arch_initcall(%(soc)s_pinctrl_init);
 ''' % socvars, end='')

[V3,02/17] pinctrl: tegra: add suspend and resume support

Commit Message

Comments

Patch