[PATCH 0/7] pinctrl: soc/tegra: Add support to configure IO rail voltage and pad power states

[PATCH 0/7] pinctrl: soc/tegra: Add support to configure IO rail voltage and pad power states

[Laxman Dewangan]

NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
or 3.3V I/O voltage levels. Also the IO pads can be configured
for power down state if it is not used. SW needs to configure the
voltage level of IO pads based on IO rail voltage and its power
state based on platform usage.
    
The voltage rail configuration and pad power states are done in
Tegra PMC registers.

This series add the required interface in soc/tegra/pmc for required
configruations.  This series also add the pincontrol driver for tegra210
IO pads conifguration. This will helps to provide framework for IO pad
configurations via pincontrol DT support and pincontrol dynamic
configruations.

Laxman Dewangan (7):
  soc/tegra: pmc: Use BIT macro for register field definition
  soc/tegra: pmc: Add new Tegra210 IO rails
  soc/tegra: pmc: Add interface to get IO rail power status
  soc/tegra: pmc: Add interface to set voltage of IO rails
  soc/tegra: pmc: Register sub-devices of PMC
  pinctrl: tegra: Add DT binding for io pads control
  pinctrl: tegra: Add driver to configure voltage and power state of io
    pads

 .../bindings/pinctrl/nvidia,tegra210-io-pad.txt    | 102 +++++++
 drivers/pinctrl/tegra/Kconfig                      |  11 +
 drivers/pinctrl/tegra/Makefile                     |   1 +
 drivers/pinctrl/tegra/pinctrl-tegra210-io-pad.c    | 302 +++++++++++++++++++++
 drivers/soc/tegra/pmc.c                            | 159 +++++++++--
 .../dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h  |  24 ++
 include/soc/tegra/pmc.h                            |  52 ++++
 7 files changed, 630 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
 create mode 100644 drivers/pinctrl/tegra/pinctrl-tegra210-io-pad.c
 create mode 100644 include/dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h

-- 
2.1.4

[PATCH 1/7] soc/tegra: pmc: Use BIT macro for register field definition

[Laxman Dewangan]

Use BIT macro for register field definition and make constant as
unsigned when using in shift operator like instead of (3 << 30),
make it to (3U << 30).

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 42 +++++++++++++++++++++---------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 08966c2..762f4fa 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -42,29 +42,29 @@
 #include <soc/tegra/pmc.h>
 
 #define PMC_CNTRL			0x0
-#define  PMC_CNTRL_SYSCLK_POLARITY	(1 << 10)  /* sys clk polarity */
-#define  PMC_CNTRL_SYSCLK_OE		(1 << 11)  /* system clock enable */
-#define  PMC_CNTRL_SIDE_EFFECT_LP0	(1 << 14)  /* LP0 when CPU pwr gated */
-#define  PMC_CNTRL_CPU_PWRREQ_POLARITY	(1 << 15)  /* CPU pwr req polarity */
-#define  PMC_CNTRL_CPU_PWRREQ_OE	(1 << 16)  /* CPU pwr req enable */
-#define  PMC_CNTRL_INTR_POLARITY	(1 << 17)  /* inverts INTR polarity */
+#define PMC_CNTRL_SYSCLK_POLARITY	BIT(10) /* sys clk polarity */
+#define PMC_CNTRL_SYSCLK_OE		BIT(11) /* system clock enable */
+#define PMC_CNTRL_SIDE_EFFECT_LP0	BIT(14) /* LP0 when CPU pwr gated */
+#define PMC_CNTRL_CPU_PWRREQ_POLARITY	BIT(15) /* CPU pwr req polarity */
+#define PMC_CNTRL_CPU_PWRREQ_OE		BIT(16) /* CPU pwr req enable */
+#define PMC_CNTRL_INTR_POLARITY		BIT(17)/* inverts INTR polarity */
 
 #define DPD_SAMPLE			0x020
-#define  DPD_SAMPLE_ENABLE		(1 << 0)
-#define  DPD_SAMPLE_DISABLE		(0 << 0)
+#define DPD_SAMPLE_ENABLE		BIT(0)
+#define DPD_SAMPLE_DISABLE		(0 << 0)
 
 #define PWRGATE_TOGGLE			0x30
-#define  PWRGATE_TOGGLE_START		(1 << 8)
+#define PWRGATE_TOGGLE_START		BIT(8)
 
 #define REMOVE_CLAMPING			0x34
 
 #define PWRGATE_STATUS			0x38
 
 #define PMC_SCRATCH0			0x50
-#define  PMC_SCRATCH0_MODE_RECOVERY	(1 << 31)
-#define  PMC_SCRATCH0_MODE_BOOTLOADER	(1 << 30)
-#define  PMC_SCRATCH0_MODE_RCM		(1 << 1)
-#define  PMC_SCRATCH0_MODE_MASK		(PMC_SCRATCH0_MODE_RECOVERY | \
+#define PMC_SCRATCH0_MODE_RECOVERY	BIT(31)
+#define PMC_SCRATCH0_MODE_BOOTLOADER	BIT(30)
+#define PMC_SCRATCH0_MODE_RCM		BIT(1)
+#define PMC_SCRATCH0_MODE_MASK		(PMC_SCRATCH0_MODE_RECOVERY | \
 					 PMC_SCRATCH0_MODE_BOOTLOADER | \
 					 PMC_SCRATCH0_MODE_RCM)
 
@@ -74,14 +74,14 @@
 #define PMC_SCRATCH41			0x140
 
 #define PMC_SENSOR_CTRL			0x1b0
-#define PMC_SENSOR_CTRL_SCRATCH_WRITE	(1 << 2)
-#define PMC_SENSOR_CTRL_ENABLE_RST	(1 << 1)
+#define PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
+#define PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
 
 #define IO_DPD_REQ			0x1b8
-#define  IO_DPD_REQ_CODE_IDLE		(0 << 30)
-#define  IO_DPD_REQ_CODE_OFF		(1 << 30)
-#define  IO_DPD_REQ_CODE_ON		(2 << 30)
-#define  IO_DPD_REQ_CODE_MASK		(3 << 30)
+#define IO_DPD_REQ_CODE_IDLE		(0 << 30)
+#define IO_DPD_REQ_CODE_OFF		(1U << 30)
+#define IO_DPD_REQ_CODE_ON		(2U << 30)
+#define IO_DPD_REQ_CODE_MASK		(3U << 30)
 
 #define IO_DPD_STATUS			0x1bc
 #define IO_DPD2_REQ			0x1c0
@@ -93,10 +93,10 @@
 #define PMC_SCRATCH54_ADDR_SHIFT	0
 
 #define PMC_SCRATCH55			0x25c
-#define PMC_SCRATCH55_RESET_TEGRA	(1 << 31)
+#define PMC_SCRATCH55_RESET_TEGRA	BIT(31)
 #define PMC_SCRATCH55_CNTRL_ID_SHIFT	27
 #define PMC_SCRATCH55_PINMUX_SHIFT	24
-#define PMC_SCRATCH55_16BITOP		(1 << 15)
+#define PMC_SCRATCH55_16BITOP		BIT(15)
 #define PMC_SCRATCH55_CHECKSUM_SHIFT	16
 #define PMC_SCRATCH55_I2CSLV1_SHIFT	0
 
-- 
2.1.4

Re: [PATCH 1/7] soc/tegra: pmc: Use BIT macro for register field definition

[Thierry Reding]

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

On Tue, Apr 12, 2016 at 08:26:41PM +0530, Laxman Dewangan wrote:
> Use BIT macro for register field definition and make constant as
> unsigned when using in shift operator like instead of (3 << 30),
> make it to (3U << 30).
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 42 +++++++++++++++++++++---------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)

Does this matter at all? We use the explicit notation in quite a number
of places and it works great. I'd like to avoid needless churn unless
there is a very good reason to switch.

Also this contains whitespace changes that remove the extra level of
indentation that is used to separate register field definitions from the
register definitions.

Thierry

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

Re: [PATCH 1/7] soc/tegra: pmc: Use BIT macro for register field definition

[Laxman Dewangan]

On Tuesday 12 April 2016 08:56 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Apr 12, 2016 at 08:26:41PM +0530, Laxman Dewangan wrote:
>> Use BIT macro for register field definition and make constant as
>> unsigned when using in shift operator like instead of (3 << 30),
>> make it to (3U << 30).
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> ---
>>   drivers/soc/tegra/pmc.c | 42 +++++++++++++++++++++---------------------
>>   1 file changed, 21 insertions(+), 21 deletions(-)
> Does this matter at all? We use the explicit notation in quite a number
> of places and it works great. I'd like to avoid needless churn unless
> there is a very good reason to switch.

When I run the checkpatch, I got the error and thought that better to 
fix and cleanup some warning/error from checkpatch.
This is just part of cleanups and properly defining constant like 3U 
instead of 3 to avoid any issue.

Re: [PATCH 1/7] soc/tegra: pmc: Use BIT macro for register field definition

[Linus Walleij]

On Tue, Apr 12, 2016 at 5:26 PM, Thierry Reding
<thierry.reding@gmail.com> wrote:

> Does this matter at all? We use the explicit notation in quite a number
> of places and it works great. I'd like to avoid needless churn unless
> there is a very good reason to switch.

I am softly enforcing <linux/bitops.h> in the GPIO subsystem
because I think it's a great perceptual aid, I can skim code
faster if this is used, easier for the brain to handle and thus
simplifies maintenance.

Others may disagree on its virtues.

It is not a subjective opinion, but proving the point needs
perceptual psychology research.

Yours,
Linus Walleij

[PATCH 2/7] soc/tegra: pmc: Add new Tegra210 IO rails

[Laxman Dewangan]

NVIDIA Tegra210 has extended the IO rails for new IO pads
and added some new IO rails on top of its previous SoC.

Add all supported IO rails from Tegra210 to the Tegra PMC header.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 include/soc/tegra/pmc.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
index 07e332d..58fadc5 100644
--- a/include/soc/tegra/pmc.h
+++ b/include/soc/tegra/pmc.h
@@ -90,22 +90,36 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid);
 #define TEGRA_IO_RAIL_UART	14
 #define TEGRA_IO_RAIL_BB	15
 #define TEGRA_IO_RAIL_AUDIO	17
+#define TEGRA_IO_RAIL_USB3	18
 #define TEGRA_IO_RAIL_HSIC	19
 #define TEGRA_IO_RAIL_COMP	22
+#define TEGRA_IO_RAIL_DBG	25
+#define TEGRA_IO_RAIL_DBG_NONAO	26
+#define TEGRA_IO_RAIL_GPIO	27
 #define TEGRA_IO_RAIL_HDMI	28
 #define TEGRA_IO_RAIL_PEX_CNTRL	32
 #define TEGRA_IO_RAIL_SDMMC1	33
 #define TEGRA_IO_RAIL_SDMMC3	34
 #define TEGRA_IO_RAIL_SDMMC4	35
+#define TEGRA_IO_RAIL_EMMC	35
 #define TEGRA_IO_RAIL_CAM	36
 #define TEGRA_IO_RAIL_RES	37
+#define TEGRA_IO_RAIL_EMMC2	37
 #define TEGRA_IO_RAIL_HV	38
 #define TEGRA_IO_RAIL_DSIB	39
 #define TEGRA_IO_RAIL_DSIC	40
 #define TEGRA_IO_RAIL_DSID	41
+#define TEGRA_IO_RAIL_CSIC	42
+#define TEGRA_IO_RAIL_CSID	43
 #define TEGRA_IO_RAIL_CSIE	44
+#define TEGRA_IO_RAIL_CSIF	45
+#define TEGRA_IO_RAIL_SPI	46
+#define TEGRA_IO_RAIL_SPI_HV	47
+#define TEGRA_IO_RAIL_DMIC	50
+#define TEGRA_IO_RAIL_DP	51
 #define TEGRA_IO_RAIL_LVDS	57
 #define TEGRA_IO_RAIL_SYS_DDC	58
+#define TEGRA_IO_RAIL_AUDIO_HV	61
 
 #ifdef CONFIG_ARCH_TEGRA
 int tegra_powergate_is_powered(unsigned int id);
-- 
2.1.4

Re: [PATCH 2/7] soc/tegra: pmc: Add new Tegra210 IO rails

[Thierry Reding]

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

On Tue, Apr 12, 2016 at 08:26:42PM +0530, Laxman Dewangan wrote:
> NVIDIA Tegra210 has extended the IO rails for new IO pads
> and added some new IO rails on top of its previous SoC.
> 
> Add all supported IO rails from Tegra210 to the Tegra PMC header.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  include/soc/tegra/pmc.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
> index 07e332d..58fadc5 100644
> --- a/include/soc/tegra/pmc.h
> +++ b/include/soc/tegra/pmc.h
> @@ -90,22 +90,36 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid);
>  #define TEGRA_IO_RAIL_UART	14
>  #define TEGRA_IO_RAIL_BB	15
>  #define TEGRA_IO_RAIL_AUDIO	17
> +#define TEGRA_IO_RAIL_USB3	18
>  #define TEGRA_IO_RAIL_HSIC	19
>  #define TEGRA_IO_RAIL_COMP	22
> +#define TEGRA_IO_RAIL_DBG	25
> +#define TEGRA_IO_RAIL_DBG_NONAO	26
> +#define TEGRA_IO_RAIL_GPIO	27
>  #define TEGRA_IO_RAIL_HDMI	28
>  #define TEGRA_IO_RAIL_PEX_CNTRL	32
>  #define TEGRA_IO_RAIL_SDMMC1	33
>  #define TEGRA_IO_RAIL_SDMMC3	34
>  #define TEGRA_IO_RAIL_SDMMC4	35
> +#define TEGRA_IO_RAIL_EMMC	35
>  #define TEGRA_IO_RAIL_CAM	36
>  #define TEGRA_IO_RAIL_RES	37
> +#define TEGRA_IO_RAIL_EMMC2	37

We have a duplicate entry for 37 now. The _RES might have meant
"reserved", in which case maybe just replace it with the new symbolic
name?

Thierry

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

Re: [PATCH 2/7] soc/tegra: pmc: Add new Tegra210 IO rails

[Laxman Dewangan]

On Tuesday 12 April 2016 08:58 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Tue, Apr 12, 2016 at 08:26:42PM +0530, Laxman Dewangan wrote:
>> NVIDIA Tegra210 has extended the IO rails for new IO pads
>> and added some new IO rails on top of its previous SoC.
>>
>> Add all supported IO rails from Tegra210 to the Tegra PMC header.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> ---
>>   include/soc/tegra/pmc.h | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
>> index 07e332d..58fadc5 100644
>> --- a/include/soc/tegra/pmc.h
>> +++ b/include/soc/tegra/pmc.h
>> @@ -90,22 +90,36 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid);
>>   #define TEGRA_IO_RAIL_UART	14
>>   #define TEGRA_IO_RAIL_BB	15
>>   #define TEGRA_IO_RAIL_AUDIO	17
>> +#define TEGRA_IO_RAIL_USB3	18
>>   #define TEGRA_IO_RAIL_HSIC	19
>>   #define TEGRA_IO_RAIL_COMP	22
>> +#define TEGRA_IO_RAIL_DBG	25
>> +#define TEGRA_IO_RAIL_DBG_NONAO	26
>> +#define TEGRA_IO_RAIL_GPIO	27
>>   #define TEGRA_IO_RAIL_HDMI	28
>>   #define TEGRA_IO_RAIL_PEX_CNTRL	32
>>   #define TEGRA_IO_RAIL_SDMMC1	33
>>   #define TEGRA_IO_RAIL_SDMMC3	34
>>   #define TEGRA_IO_RAIL_SDMMC4	35
>> +#define TEGRA_IO_RAIL_EMMC	35
>>   #define TEGRA_IO_RAIL_CAM	36
>>   #define TEGRA_IO_RAIL_RES	37
>> +#define TEGRA_IO_RAIL_EMMC2	37
> We have a duplicate entry for 37 now. The _RES might have meant
> "reserved", in which case maybe just replace it with the new symbolic
> name?

OK, then make sense to replace RES with EMMC2.

Re: [PATCH 2/7] soc/tegra: pmc: Add new Tegra210 IO rails

[Jon Hunter]

On 12/04/16 17:59, Laxman Dewangan wrote:
> 
> On Tuesday 12 April 2016 08:58 PM, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Tue, Apr 12, 2016 at 08:26:42PM +0530, Laxman Dewangan wrote:
>>> NVIDIA Tegra210 has extended the IO rails for new IO pads
>>> and added some new IO rails on top of its previous SoC.
>>>
>>> Add all supported IO rails from Tegra210 to the Tegra PMC header.
>>>
>>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>>> ---
>>>   include/soc/tegra/pmc.h | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
>>> index 07e332d..58fadc5 100644
>>> --- a/include/soc/tegra/pmc.h
>>> +++ b/include/soc/tegra/pmc.h
>>> @@ -90,22 +90,36 @@ int tegra_pmc_cpu_remove_clamping(unsigned int
>>> cpuid);
>>>   #define TEGRA_IO_RAIL_UART    14
>>>   #define TEGRA_IO_RAIL_BB    15
>>>   #define TEGRA_IO_RAIL_AUDIO    17
>>> +#define TEGRA_IO_RAIL_USB3    18
>>>   #define TEGRA_IO_RAIL_HSIC    19
>>>   #define TEGRA_IO_RAIL_COMP    22
>>> +#define TEGRA_IO_RAIL_DBG    25
>>> +#define TEGRA_IO_RAIL_DBG_NONAO    26
>>> +#define TEGRA_IO_RAIL_GPIO    27
>>>   #define TEGRA_IO_RAIL_HDMI    28
>>>   #define TEGRA_IO_RAIL_PEX_CNTRL    32
>>>   #define TEGRA_IO_RAIL_SDMMC1    33
>>>   #define TEGRA_IO_RAIL_SDMMC3    34
>>>   #define TEGRA_IO_RAIL_SDMMC4    35
>>> +#define TEGRA_IO_RAIL_EMMC    35
>>>   #define TEGRA_IO_RAIL_CAM    36
>>>   #define TEGRA_IO_RAIL_RES    37
>>> +#define TEGRA_IO_RAIL_EMMC2    37
>> We have a duplicate entry for 37 now. The _RES might have meant
>> "reserved", in which case maybe just replace it with the new symbolic
>> name?
> 
> OK, then make sense to replace RES with EMMC2.

Looking at the Tegra124 TRM it was reserved and so renaming makes sense
here. However, that also prompts the question how do we check to ensure
that the IO rail is valid for a given SoC?

Should we define a 'valid' mask for IO_DPD_STATUS and IO_DPD2_STATUS
registers in the SoC data so we can check if the rail is valid?

Cheers
Jon

Re: [PATCH 2/7] soc/tegra: pmc: Add new Tegra210 IO rails

[Laxman Dewangan]

On Tuesday 12 April 2016 11:33 PM, Jon Hunter wrote:
> On 12/04/16 17:59, Laxman Dewangan wrote:
>> On Tuesday 12 April 2016 08:58 PM, Thierry Reding wrote:
>>> * PGP Signed by an unknown key
>>>
>>> On Tue, Apr 12, 2016 at 08:26:42PM +0530, Laxman Dewangan wrote:
>>>> +#define TEGRA_IO_RAIL_EMMC    35
>>>>    #define TEGRA_IO_RAIL_CAM    36
>>>>    #define TEGRA_IO_RAIL_RES    37
>>>> +#define TEGRA_IO_RAIL_EMMC2    37
>>> We have a duplicate entry for 37 now. The _RES might have meant
>>> "reserved", in which case maybe just replace it with the new symbolic
>>> name?
>> OK, then make sense to replace RES with EMMC2.
> Looking at the Tegra124 TRM it was reserved and so renaming makes sense
> here. However, that also prompts the question how do we check to ensure
> that the IO rail is valid for a given SoC?
>
> Should we define a 'valid' mask for IO_DPD_STATUS and IO_DPD2_STATUS
> registers in the SoC data so we can check if the rail is valid?
>

Yes, that is good idea.
Infact, we should decouple  RAIL_ID with the bit location of register.
This will help on mapping any rail ID to SoC specific bit location and 
need not to worry if bit location of rail get changed on any generation. 
Local lookup table from ID to bit location can make validation as well 
as the decoupling.

[PATCH 3/7] soc/tegra: pmc: Add interface to get IO rail power status

[Laxman Dewangan]

Add API to get the IO rail power status of the Tegra IO pads.
This will help client driver to get the current power status
of IO pads for handling IO pad power.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 16 ++++++++++++++++
 include/soc/tegra/pmc.h |  6 ++++++
 2 files changed, 22 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 762f4fa..0bc8219 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -613,6 +613,22 @@ error:
 }
 EXPORT_SYMBOL(tegra_io_rail_power_off);
 
+int tegra_io_rail_power_get_status(unsigned int id)
+{
+	unsigned long status, value;
+	unsigned int mask;
+
+	if ((id > 63) || (id == 30) || (id == 31))
+		return -EINVAL;
+
+	status = (id < 32) ? IO_DPD_STATUS : IO_DPD2_STATUS;
+	mask = BIT(id % 32);
+	value = tegra_pmc_readl(status);
+
+	return !!(value & mask);
+}
+EXPORT_SYMBOL(tegra_io_rail_power_get_status);
+
 #ifdef CONFIG_PM_SLEEP
 enum tegra_suspend_mode tegra_pmc_get_suspend_mode(void)
 {
diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
index 58fadc5..4f3db41 100644
--- a/include/soc/tegra/pmc.h
+++ b/include/soc/tegra/pmc.h
@@ -133,6 +133,7 @@ int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
 
 int tegra_io_rail_power_on(unsigned int id);
 int tegra_io_rail_power_off(unsigned int id);
+int tegra_io_rail_power_get_status(unsigned int id);
 #else
 static inline int tegra_powergate_is_powered(unsigned int id)
 {
@@ -170,6 +171,11 @@ static inline int tegra_io_rail_power_off(unsigned int id)
 {
 	return -ENOSYS;
 }
+
+static inline int tegra_io_rail_power_get_status(unsigned int id)
+{
+	return -ENOTSUP;
+}
 #endif /* CONFIG_ARCH_TEGRA */
 
 #endif /* __SOC_TEGRA_PMC_H__ */
-- 
2.1.4

Re: [PATCH 3/7] soc/tegra: pmc: Add interface to get IO rail power status

[kbuild test robot]

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

Hi Laxman,

[auto build test ERROR on tegra/for-next]
[also build test ERROR on next-20160412]
[cannot apply to v4.6-rc3]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Laxman-Dewangan/pinctrl-soc-tegra-Add-support-to-configure-IO-rail-voltage-and-pad-power-states/20160412-232024
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux for-next
config: i386-randconfig-s1-201615 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from drivers/gpu/drm/nouveau/include/nvif/os.h:35:0,
                    from drivers/gpu/drm/nouveau/include/nvif/object.h:4,
                    from drivers/gpu/drm/nouveau/nvif/object.c:25:
   include/soc/tegra/pmc.h: In function 'tegra_io_rail_power_get_status':
>> include/soc/tegra/pmc.h:177:10: error: 'ENOTSUP' undeclared (first use in this function)
     return -ENOTSUP;
             ^
   include/soc/tegra/pmc.h:177:10: note: each undeclared identifier is reported only once for each function it appears in

vim +/ENOTSUP +177 include/soc/tegra/pmc.h

   171	{
   172		return -ENOSYS;
   173	}
   174	
   175	static inline int tegra_io_rail_power_get_status(unsigned int id)
   176	{
 > 177		return -ENOTSUP;
   178	}
   179	#endif /* CONFIG_ARCH_TEGRA */
   180	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 26205 bytes --]

Re: [PATCH 3/7] soc/tegra: pmc: Add interface to get IO rail power status

[Jon Hunter]

On 12/04/16 15:56, Laxman Dewangan wrote:
> Add API to get the IO rail power status of the Tegra IO pads.
> This will help client driver to get the current power status
> of IO pads for handling IO pad power.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 16 ++++++++++++++++
>  include/soc/tegra/pmc.h |  6 ++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 762f4fa..0bc8219 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -613,6 +613,22 @@ error:
>  }
>  EXPORT_SYMBOL(tegra_io_rail_power_off);
>  
> +int tegra_io_rail_power_get_status(unsigned int id)
> +{
> +	unsigned long status, value;
> +	unsigned int mask;

These should all be u32 as tegra_pmc_readl() returns a u32.

Hmmm ... I see some of the other tegra_io_rail_xxx() APIs use a unsigned
long here too. We should fix those as well.

> +
> +	if ((id > 63) || (id == 30) || (id == 31))
> +		return -EINVAL;

Seems that this could vary per SoC and so I am wondering if we need a
bitmap for this?

> +
> +	status = (id < 32) ? IO_DPD_STATUS : IO_DPD2_STATUS;
> +	mask = BIT(id % 32);
> +	value = tegra_pmc_readl(status);
> +
> +	return !!(value & mask);

Nit-pick ... do you need this mask variable?

Cheers
Jon

[PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Laxman Dewangan]

NVIDIA Tegra210 supports some of the IO interface which can operate
at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
Tegra PMC register to set different voltage level of IO interface based
on IO rail voltage from power supply i.e. power regulators.

Add APIs to set and get IO rail voltage from the client driver.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/tegra/pmc.h | 32 +++++++++++++++++
 2 files changed, 127 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 0bc8219..968f7cb 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -73,6 +73,10 @@
 
 #define PMC_SCRATCH41			0x140
 
+/* Power detect for IO rail voltage */
+#define PMC_PWR_DET			0x48
+#define PMC_PWR_DET_VAL			0xe4
+
 #define PMC_SENSOR_CTRL			0x1b0
 #define PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
 #define PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
@@ -102,6 +106,8 @@
 
 #define GPU_RG_CNTRL			0x2d4
 
+static DEFINE_SPINLOCK(tegra_pmc_access_lock);
+
 struct tegra_pmc_soc {
 	unsigned int num_powergates;
 	const char *const *powergates;
@@ -110,6 +116,7 @@ struct tegra_pmc_soc {
 
 	bool has_tsense_reset;
 	bool has_gpu_clamps;
+	bool has_io_rail_voltage_config;
 };
 
 /**
@@ -160,11 +167,31 @@ struct tegra_pmc {
 	struct mutex powergates_lock;
 };
 
+struct tegra_io_rail_voltage_bit_info {
+	int io_rail_id;
+	int bit_position;
+};
+
 static struct tegra_pmc *pmc = &(struct tegra_pmc) {
 	.base = NULL,
 	.suspend_mode = TEGRA_SUSPEND_NONE,
 };
 
+#define TEGRA_IO_RAIL_VOLTAGE(_io_rail, _pos)		\
+{							\
+	.io_rail_id = TEGRA_IO_RAIL_##_io_rail,		\
+	.bit_position = _pos,				\
+}
+
+static struct tegra_io_rail_voltage_bit_info tegra210_io_rail_voltage_info[] = {
+	TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
+	TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
+	TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
+	TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
+	TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
+	TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
+};
+
 static u32 tegra_pmc_readl(unsigned long offset)
 {
 	return readl(pmc->base + offset);
@@ -175,6 +202,16 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
 	writel(value, pmc->base + offset);
 }
 
+static void _tegra_pmc_register_update(unsigned long addr, unsigned long mask,
+				       unsigned long val)
+{
+	u32 pmc_reg;
+
+	pmc_reg = tegra_pmc_readl(addr);
+	pmc_reg = (pmc_reg & ~mask) | (val & mask);
+	tegra_pmc_writel(pmc_reg, addr);
+}
+
 static inline bool tegra_powergate_state(int id)
 {
 	if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
@@ -410,6 +447,63 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid)
 }
 #endif /* CONFIG_SMP */
 
+static int tegra_io_rail_voltage_get_bit_pos(int io_rail_id)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tegra210_io_rail_voltage_info); ++i) {
+		if (tegra210_io_rail_voltage_info[i].io_rail_id == io_rail_id)
+			return tegra210_io_rail_voltage_info[i].bit_position;
+	}
+
+	return -EINVAL;
+}
+
+int tegra_io_rail_voltage_set(int io_rail, int val)
+{
+	unsigned long flags;
+	unsigned long bval, mask;
+	int bpos;
+
+	if (!pmc->soc->has_io_rail_voltage_config)
+		return -ENODEV;
+
+	bpos = tegra_io_rail_voltage_get_bit_pos(io_rail);
+	if (bpos < 0)
+		return bpos;
+
+	mask = BIT(bpos);
+	bval = (val) ? mask : 0;
+
+	spin_lock_irqsave(&tegra_pmc_access_lock, flags);
+	_tegra_pmc_register_update(PMC_PWR_DET, mask, mask);
+	_tegra_pmc_register_update(PMC_PWR_DET_VAL, mask, bval);
+	spin_unlock_irqrestore(&tegra_pmc_access_lock, flags);
+
+	usleep_range(5, 10);
+
+	return 0;
+}
+EXPORT_SYMBOL(tegra_io_rail_voltage_set);
+
+int tegra_io_rail_voltage_get(int io_rail)
+{
+	u32 rval;
+	int bpos;
+
+	if (!pmc->soc->has_io_rail_voltage_config)
+		return -ENODEV;
+
+	bpos = tegra_io_rail_voltage_get_bit_pos(io_rail);
+	if (bpos < 0)
+		return bpos;
+
+	rval = tegra_pmc_readl(PMC_PWR_DET_VAL);
+
+	return !!(rval & BIT(bpos));
+}
+EXPORT_SYMBOL(tegra_io_rail_voltage_get);
+
 static int tegra_pmc_restart_notify(struct notifier_block *this,
 				    unsigned long action, void *data)
 {
@@ -1102,6 +1196,7 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
 	.cpu_powergates = tegra210_cpu_powergates,
 	.has_tsense_reset = true,
 	.has_gpu_clamps = true,
+	.has_io_rail_voltage_config = true,
 };
 
 static const struct of_device_id tegra_pmc_match[] = {
diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
index 4f3db41..98ebf35 100644
--- a/include/soc/tegra/pmc.h
+++ b/include/soc/tegra/pmc.h
@@ -134,6 +134,27 @@ int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
 int tegra_io_rail_power_on(unsigned int id);
 int tegra_io_rail_power_off(unsigned int id);
 int tegra_io_rail_power_get_status(unsigned int id);
+
+/*
+ * tegra_io_rail_voltage_set: Set IO rail voltage.
+ * @io_rail: Tegra IO rail ID as defined in macro TEGRA_IO_RAIL_*
+ * @val: Voltage need to be set. The values are:
+ *		0 for 1.8V,
+ *		1 for 3.3V.
+ *
+ * Returns 0 if success otherwise error number.
+ */
+int tegra_io_rail_voltage_set(int io_rail, int val);
+
+/*
+ * tegra_io_rail_voltage_get: Get IO rail voltage.
+ * @io_rail: Tegra IO rail ID as defined in macro TEGRA_IO_RAIL_*
+ *
+ * Returns negative error number if it fails due to invalid io pad id.
+ * Otherwise 0 for 1.8V, 1 for 3.3V.
+ */
+int tegra_io_rail_voltage_get(int io_rail);
+
 #else
 static inline int tegra_powergate_is_powered(unsigned int id)
 {
@@ -176,6 +197,17 @@ static inline int tegra_io_rail_power_get_status(unsigned int id)
 {
 	return -ENOTSUP;
 }
+
+static inline int tegra_io_rail_voltage_set(int io_rail, int val)
+{
+	return -ENOTSUP;
+}
+
+static inline int tegra_io_rail_voltage_get(int io_rail)
+{
+	return -ENOTSUP;
+}
+
 #endif /* CONFIG_ARCH_TEGRA */
 
 #endif /* __SOC_TEGRA_PMC_H__ */
-- 
2.1.4

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Jon Hunter]

On 12/04/16 15:56, Laxman Dewangan wrote:
> NVIDIA Tegra210 supports some of the IO interface which can operate
> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
> Tegra PMC register to set different voltage level of IO interface based
> on IO rail voltage from power supply i.e. power regulators.
> 
> Add APIs to set and get IO rail voltage from the client driver.

I think that we need some further explanation about the scenario when
this is used. In other words, why this configuration needs to be done in
the kernel versus the bootloader. Is this something that can change at
runtime? I could see that for SD cards it may.

> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  drivers/soc/tegra/pmc.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/soc/tegra/pmc.h | 32 +++++++++++++++++
>  2 files changed, 127 insertions(+)
> 
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
> index 0bc8219..968f7cb 100644
> --- a/drivers/soc/tegra/pmc.c
> +++ b/drivers/soc/tegra/pmc.c
> @@ -73,6 +73,10 @@
>  
>  #define PMC_SCRATCH41			0x140
>  
> +/* Power detect for IO rail voltage */
> +#define PMC_PWR_DET			0x48
> +#define PMC_PWR_DET_VAL			0xe4
> +
>  #define PMC_SENSOR_CTRL			0x1b0
>  #define PMC_SENSOR_CTRL_SCRATCH_WRITE	BIT(2)
>  #define PMC_SENSOR_CTRL_ENABLE_RST	BIT(1)
> @@ -102,6 +106,8 @@
>  
>  #define GPU_RG_CNTRL			0x2d4
>  
> +static DEFINE_SPINLOCK(tegra_pmc_access_lock);
> +

We already have a mutex for managing concurrent accesses, do we need this?

>  struct tegra_pmc_soc {
>  	unsigned int num_powergates;
>  	const char *const *powergates;
> @@ -110,6 +116,7 @@ struct tegra_pmc_soc {
>  
>  	bool has_tsense_reset;
>  	bool has_gpu_clamps;
> +	bool has_io_rail_voltage_config;
>  };
>  
>  /**
> @@ -160,11 +167,31 @@ struct tegra_pmc {
>  	struct mutex powergates_lock;
>  };
>  
> +struct tegra_io_rail_voltage_bit_info {
> +	int io_rail_id;
> +	int bit_position;
> +};
> +
>  static struct tegra_pmc *pmc = &(struct tegra_pmc) {
>  	.base = NULL,
>  	.suspend_mode = TEGRA_SUSPEND_NONE,
>  };
>  
> +#define TEGRA_IO_RAIL_VOLTAGE(_io_rail, _pos)		\
> +{							\
> +	.io_rail_id = TEGRA_IO_RAIL_##_io_rail,		\
> +	.bit_position = _pos,				\
> +}
> +
> +static struct tegra_io_rail_voltage_bit_info tegra210_io_rail_voltage_info[] = {
> +	TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
> +	TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
> +	TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
> +	TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
> +	TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
> +	TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
> +};
> +

You could simply this by having a look-up table similar to what we do
for the powergates.

>  static u32 tegra_pmc_readl(unsigned long offset)
>  {
>  	return readl(pmc->base + offset);
> @@ -175,6 +202,16 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>  	writel(value, pmc->base + offset);
>  }
>  
> +static void _tegra_pmc_register_update(unsigned long addr, unsigned long mask,
> +				       unsigned long val)

u32 for mask and val. May be consider renaming to tegra_pmc_rmw().

> +{
> +	u32 pmc_reg;
> +
> +	pmc_reg = tegra_pmc_readl(addr);
> +	pmc_reg = (pmc_reg & ~mask) | (val & mask);
> +	tegra_pmc_writel(pmc_reg, addr);
> +}
> +
>  static inline bool tegra_powergate_state(int id)
>  {
>  	if (id == TEGRA_POWERGATE_3D && pmc->soc->has_gpu_clamps)
> @@ -410,6 +447,63 @@ int tegra_pmc_cpu_remove_clamping(unsigned int cpuid)
>  }
>  #endif /* CONFIG_SMP */
>  
> +static int tegra_io_rail_voltage_get_bit_pos(int io_rail_id)

All the tegra_io_rail_xxx() APIs just use "id" and it is an unsigned
int. Any reason this needs to be an int? We should keep the naming and
type consistent.

> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tegra210_io_rail_voltage_info); ++i) {
> +		if (tegra210_io_rail_voltage_info[i].io_rail_id == io_rail_id)
> +			return tegra210_io_rail_voltage_info[i].bit_position;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +int tegra_io_rail_voltage_set(int io_rail, int val)

Same here w.r.t "io_rail".

Also it appears that "val" should only be 0 or 1 but we don't check for
this. I see that you treat all non-zero values as '1' but that seems a
bit funny. You may consider having the user pass uV and then you could
check for either 3300000 or 1800000.

> +{
> +	unsigned long flags;
> +	unsigned long bval, mask;
> +	int bpos;
> +
> +	if (!pmc->soc->has_io_rail_voltage_config)
> +		return -ENODEV;
> +
> +	bpos = tegra_io_rail_voltage_get_bit_pos(io_rail);
> +	if (bpos < 0)
> +		return bpos;
> +
> +	mask = BIT(bpos);
> +	bval = (val) ? mask : 0;
> +
> +	spin_lock_irqsave(&tegra_pmc_access_lock, flags);
> +	_tegra_pmc_register_update(PMC_PWR_DET, mask, mask);
> +	_tegra_pmc_register_update(PMC_PWR_DET_VAL, mask, bval);

Hmmm ... this does not appear to be consistent with the TRM. It says to
write to the PMC_PWR_DET register and then the PMC_PWR_LATCH register. I
see in the Tegra210 TRM it says to only write the to ONLY the
PMC_PWR_DET_VAL register in other places. The TRM appears to be quite
confusing here, can you explain this?

> +	spin_unlock_irqrestore(&tegra_pmc_access_lock, flags);
> +
> +	usleep_range(5, 10);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(tegra_io_rail_voltage_set);
> +
> +int tegra_io_rail_voltage_get(int io_rail)
> +{
> +	u32 rval;
> +	int bpos;
> +
> +	if (!pmc->soc->has_io_rail_voltage_config)
> +		return -ENODEV;
> +
> +	bpos = tegra_io_rail_voltage_get_bit_pos(io_rail);
> +	if (bpos < 0)
> +		return bpos;
> +
> +	rval = tegra_pmc_readl(PMC_PWR_DET_VAL);
> +
> +	return !!(rval & BIT(bpos));
> +}
> +EXPORT_SYMBOL(tegra_io_rail_voltage_get);
> +
>  static int tegra_pmc_restart_notify(struct notifier_block *this,
>  				    unsigned long action, void *data)
>  {
> @@ -1102,6 +1196,7 @@ static const struct tegra_pmc_soc tegra210_pmc_soc = {
>  	.cpu_powergates = tegra210_cpu_powergates,
>  	.has_tsense_reset = true,
>  	.has_gpu_clamps = true,
> +	.has_io_rail_voltage_config = true,
>  };
>  
>  static const struct of_device_id tegra_pmc_match[] = {
> diff --git a/include/soc/tegra/pmc.h b/include/soc/tegra/pmc.h
> index 4f3db41..98ebf35 100644
> --- a/include/soc/tegra/pmc.h
> +++ b/include/soc/tegra/pmc.h
> @@ -134,6 +134,27 @@ int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
>  int tegra_io_rail_power_on(unsigned int id);
>  int tegra_io_rail_power_off(unsigned int id);
>  int tegra_io_rail_power_get_status(unsigned int id);
> +
> +/*
> + * tegra_io_rail_voltage_set: Set IO rail voltage.
> + * @io_rail: Tegra IO rail ID as defined in macro TEGRA_IO_RAIL_*
> + * @val: Voltage need to be set. The values are:
> + *		0 for 1.8V,
> + *		1 for 3.3V.
> + *
> + * Returns 0 if success otherwise error number.
> + */
> +int tegra_io_rail_voltage_set(int io_rail, int val);
> +
> +/*
> + * tegra_io_rail_voltage_get: Get IO rail voltage.
> + * @io_rail: Tegra IO rail ID as defined in macro TEGRA_IO_RAIL_*
> + *
> + * Returns negative error number if it fails due to invalid io pad id.
> + * Otherwise 0 for 1.8V, 1 for 3.3V.
> + */
> +int tegra_io_rail_voltage_get(int io_rail);
> +
>  #else
>  static inline int tegra_powergate_is_powered(unsigned int id)
>  {
> @@ -176,6 +197,17 @@ static inline int tegra_io_rail_power_get_status(unsigned int id)
>  {
>  	return -ENOTSUP;
>  }
> +
> +static inline int tegra_io_rail_voltage_set(int io_rail, int val)
> +{
> +	return -ENOTSUP;
> +}
> +
> +static inline int tegra_io_rail_voltage_get(int io_rail)
> +{
> +	return -ENOTSUP;
> +}

I think that you are missing a 'P' in -ENOTSUPP.

Cheers
Jon

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Laxman Dewangan]

On Wednesday 13 April 2016 02:17 PM, Jon Hunter wrote:
> On 12/04/16 15:56, Laxman Dewangan wrote:
>> NVIDIA Tegra210 supports some of the IO interface which can operate
>> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
>> Tegra PMC register to set different voltage level of IO interface based
>> on IO rail voltage from power supply i.e. power regulators.
>>
>> Add APIs to set and get IO rail voltage from the client driver.
> I think that we need some further explanation about the scenario when
> this is used. In other words, why this configuration needs to be done in
> the kernel versus the bootloader. Is this something that can change at
> runtime? I could see that for SD cards it may.

Yes, SDIO3.0 support needs dynamic IO rail voltage change and so pad 
voltage change.

>>   
>>   #define GPU_RG_CNTRL			0x2d4
>>   
>> +static DEFINE_SPINLOCK(tegra_pmc_access_lock);
>> +
> We already have a mutex for managing concurrent accesses, do we need this?
Mutex is sleeping calls and we really dont need this. This is sleep for 
small duration and we should do this in spinlock.


>>   
>> +
>> +static struct tegra_io_rail_voltage_bit_info tegra210_io_rail_voltage_info[] = {
>> +	TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
>> +	TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
>> +	TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
>> +	TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
>> +	TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
>> +	TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
>> +};
>> +
> You could simply this by having a look-up table similar to what we do
> for the powergates.
Revising the power gate code, it needs ID matches with bit location but 
it is not the case here. We need to have lookup from ID to  bit position.


>
>>   static u32 tegra_pmc_readl(unsigned long offset)
>>   {
>>   	return readl(pmc->base + offset);
>> @@ -175,6 +202,16 @@ static void tegra_pmc_writel(u32 value, unsigned long offset)
>>   	writel(value, pmc->base + offset);
>>   }
>>   
>> +static void _tegra_pmc_register_update(unsigned long addr, unsigned long mask,
>> +				       unsigned long val)
> u32 for mask and val. May be consider renaming to tegra_pmc_rmw().

update is common from the other framework like regmap so it is here.


>
>>   
>> +static int tegra_io_rail_voltage_get_bit_pos(int io_rail_id)
> All the tegra_io_rail_xxx() APIs just use "id" and it is an unsigned
> int. Any reason this needs to be an int? We should keep the naming and
> type consistent.

OK, will do.

>> +
>> +int tegra_io_rail_voltage_set(int io_rail, int val)
> Same here w.r.t "io_rail".
>
> Also it appears that "val" should only be 0 or 1 but we don't check for
> this. I see that you treat all non-zero values as '1' but that seems a
> bit funny. You may consider having the user pass uV and then you could
> check for either 3300000 or 1800000.

What about enums then?
May be we can use the DT binding header here.


>
>> +
>> +	spin_lock_irqsave(&tegra_pmc_access_lock, flags);
>> +	_tegra_pmc_register_update(PMC_PWR_DET, mask, mask);
>> +	_tegra_pmc_register_update(PMC_PWR_DET_VAL, mask, bval);
> Hmmm ... this does not appear to be consistent with the TRM. It says to
> write to the PMC_PWR_DET register and then the PMC_PWR_LATCH register. I
> see in the Tegra210 TRM it says to only write the to ONLY the
> PMC_PWR_DET_VAL register in other places. The TRM appears to be quite
> confusing here, can you explain this?

The TRM needs to be update. There is no LATCH register in the T210. 
PMC_PWR_DET and PMC_PWR_DET_VAL are registers for this. I have internal 
tracking bug for correcting this.



>
>> +static inline int tegra_io_rail_voltage_get(int io_rail)
>> +{
>> +	return -ENOTSUP;
>> +}
> I think that you are missing a 'P' in -ENOTSUPP.
>
Yes and kbuildtest reported the failure. :-)

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Jon Hunter]

On 13/04/16 10:00, Laxman Dewangan wrote:
> 
> On Wednesday 13 April 2016 02:17 PM, Jon Hunter wrote:
>> On 12/04/16 15:56, Laxman Dewangan wrote:
>>> NVIDIA Tegra210 supports some of the IO interface which can operate
>>> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
>>> Tegra PMC register to set different voltage level of IO interface based
>>> on IO rail voltage from power supply i.e. power regulators.
>>>
>>> Add APIs to set and get IO rail voltage from the client driver.
>> I think that we need some further explanation about the scenario when
>> this is used. In other words, why this configuration needs to be done in
>> the kernel versus the bootloader. Is this something that can change at
>> runtime? I could see that for SD cards it may.
> 
> Yes, SDIO3.0 support needs dynamic IO rail voltage change and so pad
> voltage change.
> 
>>>     #define GPU_RG_CNTRL            0x2d4
>>>   +static DEFINE_SPINLOCK(tegra_pmc_access_lock);
>>> +
>> We already have a mutex for managing concurrent accesses, do we need
>> this?
> Mutex is sleeping calls and we really dont need this. This is sleep for
> small duration and we should do this in spinlock.

Yes but do you need to call it from a interrupt context? It seems that
these are not called very often, may be on boot, or when swapping an SD
card, and so although a spinlock would be faster, the overhead of the
mutex would be negligible in this case. I think that you need to justify
why this needs to be a spinlock with a use-case that requires it.

>>>   +
>>> +static struct tegra_io_rail_voltage_bit_info
>>> tegra210_io_rail_voltage_info[] = {
>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
>>> +    TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
>>> +    TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
>>> +    TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
>>> +    TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
>>> +};
>>> +
>> You could simply this by having a look-up table similar to what we do
>> for the powergates.
> Revising the power gate code, it needs ID matches with bit location but
> it is not the case here. We need to have lookup from ID to  bit position.

I still don't see why you could not have ...

static unsigned int tegra210_io_rail_voltage_bit[] = {
	[TEGRA_IO_RAIL_SDMMC1] = 12,
	...
}

You could avoid the for-loop in the lookup as well as all the extra
definitions. Seems a lot simpler.

> 
>>
>>>   static u32 tegra_pmc_readl(unsigned long offset)
>>>   {
>>>       return readl(pmc->base + offset);
>>> @@ -175,6 +202,16 @@ static void tegra_pmc_writel(u32 value, unsigned
>>> long offset)
>>>       writel(value, pmc->base + offset);
>>>   }
>>>   +static void _tegra_pmc_register_update(unsigned long addr,
>>> unsigned long mask,
>>> +                       unsigned long val)
>> u32 for mask and val. May be consider renaming to tegra_pmc_rmw().
> 
> update is common from the other framework like regmap so it is here.
> 
> 
>>
>>>   +static int tegra_io_rail_voltage_get_bit_pos(int io_rail_id)
>> All the tegra_io_rail_xxx() APIs just use "id" and it is an unsigned
>> int. Any reason this needs to be an int? We should keep the naming and
>> type consistent.
> 
> OK, will do.
> 
>>> +
>>> +int tegra_io_rail_voltage_set(int io_rail, int val)
>> Same here w.r.t "io_rail".
>>
>> Also it appears that "val" should only be 0 or 1 but we don't check for
>> this. I see that you treat all non-zero values as '1' but that seems a
>> bit funny. You may consider having the user pass uV and then you could
>> check for either 3300000 or 1800000.
> 
> What about enums then?
> May be we can use the DT binding header here.

Seems better just to specify the voltage in uV in the DT and pass it to
this function.

>>
>>> +
>>> +    spin_lock_irqsave(&tegra_pmc_access_lock, flags);
>>> +    _tegra_pmc_register_update(PMC_PWR_DET, mask, mask);
>>> +    _tegra_pmc_register_update(PMC_PWR_DET_VAL, mask, bval);
>> Hmmm ... this does not appear to be consistent with the TRM. It says to
>> write to the PMC_PWR_DET register and then the PMC_PWR_LATCH register. I
>> see in the Tegra210 TRM it says to only write the to ONLY the
>> PMC_PWR_DET_VAL register in other places. The TRM appears to be quite
>> confusing here, can you explain this?
> 
> The TRM needs to be update. There is no LATCH register in the T210.
> PMC_PWR_DET and PMC_PWR_DET_VAL are registers for this. I have internal
> tracking bug for correcting this.

Why do you need to program both? I think that we should be clear here
about the procedure. If the TRM is wrong, then there should be at least
a comment here describing the correct sequence.

Cheers
Jon

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Laxman Dewangan]

On Wednesday 13 April 2016 02:55 PM, Jon Hunter wrote:
> On 13/04/16 10:00, Laxman Dewangan wrote:
>> On Wednesday 13 April 2016 02:17 PM, Jon Hunter wrote:
>>> On 12/04/16 15:56, Laxman Dewangan wrote:
>>>> NVIDIA Tegra210 supports some of the IO interface which can operate
>>>> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
>>>> Tegra PMC register to set different voltage level of IO interface based
>>>> on IO rail voltage from power supply i.e. power regulators.
>>>>
>>>> Add APIs to set and get IO rail voltage from the client driver.
>>> I think that we need some further explanation about the scenario when
>>> this is used. In other words, why this configuration needs to be done in
>>> the kernel versus the bootloader. Is this something that can change at
>>> runtime? I could see that for SD cards it may.
>> Yes, SDIO3.0 support needs dynamic IO rail voltage change and so pad
>> voltage change.
>>
>>>>      #define GPU_RG_CNTRL            0x2d4
>>>>    +static DEFINE_SPINLOCK(tegra_pmc_access_lock);
>>>> +
>>> We already have a mutex for managing concurrent accesses, do we need
>>> this?
>> Mutex is sleeping calls and we really dont need this. This is sleep for
>> small duration and we should do this in spinlock.
> Yes but do you need to call it from a interrupt context? It seems that
> these are not called very often, may be on boot, or when swapping an SD
> card, and so although a spinlock would be faster, the overhead of the
> mutex would be negligible in this case. I think that you need to justify
> why this needs to be a spinlock with a use-case that requires it.
>

This is just based on my OS theory that if critical region is taking 
less time, in order of us instead of ms then better to use spin lock 
instead of mutex lock.


>>>>    +
>>>> +static struct tegra_io_rail_voltage_bit_info
>>>> tegra210_io_rail_voltage_info[] = {
>>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
>>>> +};
>>>> +
>>> You could simply this by having a look-up table similar to what we do
>>> for the powergates.
>> Revising the power gate code, it needs ID matches with bit location but
>> it is not the case here. We need to have lookup from ID to  bit position.
> I still don't see why you could not have ...
>
> static unsigned int tegra210_io_rail_voltage_bit[] = {
> 	[TEGRA_IO_RAIL_SDMMC1] = 12,
> 	...
> }
>
> You could avoid the for-loop in the lookup as well as all the extra
> definitions. Seems a lot simpler.

This makes the table in larger size, max index is maximum of all the  
macros used in LHS.
Also if we have 0 as valid (which is not there now) then it can be trouble.
>> The TRM needs to be update. There is no LATCH register in the T210.
>> PMC_PWR_DET and PMC_PWR_DET_VAL are registers for this. I have internal
>> tracking bug for correcting this.
> Why do you need to program both? I think that we should be clear here
> about the procedure. If the TRM is wrong, then there should be at least
> a comment here describing the correct sequence.
OK, will mention the details.

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Jon Hunter]

On 13/04/16 10:20, Laxman Dewangan wrote:
> 
> On Wednesday 13 April 2016 02:55 PM, Jon Hunter wrote:
>> On 13/04/16 10:00, Laxman Dewangan wrote:
>>> On Wednesday 13 April 2016 02:17 PM, Jon Hunter wrote:
>>>> On 12/04/16 15:56, Laxman Dewangan wrote:
>>>>> NVIDIA Tegra210 supports some of the IO interface which can operate
>>>>> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
>>>>> Tegra PMC register to set different voltage level of IO interface
>>>>> based
>>>>> on IO rail voltage from power supply i.e. power regulators.
>>>>>
>>>>> Add APIs to set and get IO rail voltage from the client driver.
>>>> I think that we need some further explanation about the scenario when
>>>> this is used. In other words, why this configuration needs to be
>>>> done in
>>>> the kernel versus the bootloader. Is this something that can change at
>>>> runtime? I could see that for SD cards it may.
>>> Yes, SDIO3.0 support needs dynamic IO rail voltage change and so pad
>>> voltage change.
>>>
>>>>>      #define GPU_RG_CNTRL            0x2d4
>>>>>    +static DEFINE_SPINLOCK(tegra_pmc_access_lock);
>>>>> +
>>>> We already have a mutex for managing concurrent accesses, do we need
>>>> this?
>>> Mutex is sleeping calls and we really dont need this. This is sleep for
>>> small duration and we should do this in spinlock.
>> Yes but do you need to call it from a interrupt context? It seems that
>> these are not called very often, may be on boot, or when swapping an SD
>> card, and so although a spinlock would be faster, the overhead of the
>> mutex would be negligible in this case. I think that you need to justify
>> why this needs to be a spinlock with a use-case that requires it.
>>
> 
> This is just based on my OS theory that if critical region is taking
> less time, in order of us instead of ms then better to use spin lock
> instead of mutex lock.

Yes, but you also need to be practical. Furthermore, someone could be
powering up/down a rail at the same time someone is setting the voltage.
May be this is not a big deal ...

>>>>>    +
>>>>> +static struct tegra_io_rail_voltage_bit_info
>>>>> tegra210_io_rail_voltage_info[] = {
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
>>>>> +};
>>>>> +
>>>> You could simply this by having a look-up table similar to what we do
>>>> for the powergates.
>>> Revising the power gate code, it needs ID matches with bit location but
>>> it is not the case here. We need to have lookup from ID to  bit
>>> position.
>> I still don't see why you could not have ...
>>
>> static unsigned int tegra210_io_rail_voltage_bit[] = {
>>     [TEGRA_IO_RAIL_SDMMC1] = 12,
>>     ...
>> }
>>
>> You could avoid the for-loop in the lookup as well as all the extra
>> definitions. Seems a lot simpler.
> 
> This makes the table in larger size, max index is maximum of all the 
> macros used in LHS.

True. May be there is not a better way to do this ...

> Also if we have 0 as valid (which is not there now) then it can be trouble.
>>> The TRM needs to be update. There is no LATCH register in the T210.
>>> PMC_PWR_DET and PMC_PWR_DET_VAL are registers for this. I have internal
>>> tracking bug for correcting this.
>> Why do you need to program both? I think that we should be clear here
>> about the procedure. If the TRM is wrong, then there should be at least
>> a comment here describing the correct sequence.
> OK, will mention the details.

Thanks
Jon

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Linus Walleij]

On Tue, Apr 12, 2016 at 4:56 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> NVIDIA Tegra210 supports some of the IO interface which can operate
> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
> Tegra PMC register to set different voltage level of IO interface based
> on IO rail voltage from power supply i.e. power regulators.
>
> Add APIs to set and get IO rail voltage from the client driver.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

Nobody seems to mention the elephant in the room: why is this
not using the regulator subsystem and instead using custom
code under drivers/soc? We have worried before about drivers/soc
becoming a dumping ground akin to drivers/misc

IIRC in the past we tried to use regulators for this kind of fast
switches at ST-Ericsson, and the problem we ran into was that
regulators were kind of heavyweight and would need some
locking and required to run in slowpath.

But as complexity increases the question has to be asked
again, because what we don't want is for every SOC to start
reimplementing stuff from the regulator framework under
drivers/soc.

I'm asking you to make a case for this necessarily
different, "we are special" custom code.

Yours,
Linus Walleij

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Mark Brown]

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

On Fri, Apr 15, 2016 at 09:54:34AM +0200, Linus Walleij wrote:
> On Tue, Apr 12, 2016 at 4:56 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> > NVIDIA Tegra210 supports some of the IO interface which can operate
> > at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
> > Tegra PMC register to set different voltage level of IO interface based
> > on IO rail voltage from power supply i.e. power regulators.

> Nobody seems to mention the elephant in the room: why is this
> not using the regulator subsystem and instead using custom
> code under drivers/soc? We have worried before about drivers/soc
> becoming a dumping ground akin to drivers/misc

The above changelog sounds like a regulator consumer not a regulator -
based on what I'm reading there it's a driver that looks at the voltage
being supplied to the device and sets some configuration in the device
based on that voltage.  This isn't that unusual for analogue circuits
but it's definitely not something that's actually doing voltage
regulation.

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

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Laxman Dewangan]

On Friday 15 April 2016 01:30 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Fri, Apr 15, 2016 at 09:54:34AM +0200, Linus Walleij wrote:
>> On Tue, Apr 12, 2016 at 4:56 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>>> NVIDIA Tegra210 supports some of the IO interface which can operate
>>> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
>>> Tegra PMC register to set different voltage level of IO interface based
>>> on IO rail voltage from power supply i.e. power regulators.
>> Nobody seems to mention the elephant in the room: why is this
>> not using the regulator subsystem and instead using custom
>> code under drivers/soc? We have worried before about drivers/soc
>> becoming a dumping ground akin to drivers/misc
> The above changelog sounds like a regulator consumer not a regulator -
> based on what I'm reading there it's a driver that looks at the voltage
> being supplied to the device and sets some configuration in the device
> based on that voltage.  This isn't that unusual for analogue circuits
> but it's definitely not something that's actually doing voltage
> regulation.

Yes, this is not the voltage regulation or supply the voltage and hence 
can not be in regulator.


The IO pads voltage need to be configure by SW based on voltage level on 
this it is connected.
Some of tegra IO pads design like that they do not have auto detect for 
voltage level and SW needs to explicitly  set.

Because this is for making IO interface to be proper functioning, I put 
this in the pin controller rather than some other folder.
Other place my be soc/tegra but as the interfaces are from pin control 
framework, it is here.

Hope I have not confused  with APIs, tegra_io_rail_voltage_set(). 
Otherwise, tegra_io_rail_voltage_configure()??

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Linus Walleij]

On Fri, Apr 15, 2016 at 10:25 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Friday 15 April 2016 01:30 PM, Mark Brown wrote:

>> The above changelog sounds like a regulator consumer not a regulator -
>> based on what I'm reading there it's a driver that looks at the voltage
>> being supplied to the device and sets some configuration in the device
>> based on that voltage.  This isn't that unusual for analogue circuits
>> but it's definitely not something that's actually doing voltage
>> regulation.
>
> Yes, this is not the voltage regulation or supply the voltage and hence can
> not be in regulator.

OK I buy that. (And stand corrected.)
Sorry for the fuzz.

Yours,
Linus Walleij

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Stephen Warren]

On 04/12/2016 08:56 AM, Laxman Dewangan wrote:
> NVIDIA Tegra210 supports some of the IO interface which can operate
> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
> Tegra PMC register to set different voltage level of IO interface based
> on IO rail voltage from power supply i.e. power regulators.
>
> Add APIs to set and get IO rail voltage from the client driver.

> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c

> +static struct tegra_io_rail_voltage_bit_info tegra210_io_rail_voltage_info[] = {
> +	TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
> +	TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
> +	TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
> +	TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
> +	TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
> +	TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
> +};

That table is likely specific to Tegra210, yet ...

> +static int tegra_io_rail_voltage_get_bit_pos(int io_rail_id)
> +int tegra_io_rail_voltage_set(int io_rail, int val)
> +int tegra_io_rail_voltage_get(int io_rail)

... these functions are all named as if they are generic. Presumably 
they will indeed be needed for the next chip too? How will you prevent 
their use, or turn these functions into no-ops, or return errors, on 
other SoCs?

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Laxman Dewangan]

On Friday 15 April 2016 09:54 PM, Stephen Warren wrote:
> On 04/12/2016 08:56 AM, Laxman Dewangan wrote:
>> NVIDIA Tegra210 supports some of the IO interface which can operate
>> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
>> Tegra PMC register to set different voltage level of IO interface based
>> on IO rail voltage from power supply i.e. power regulators.
>>
>> Add APIs to set and get IO rail voltage from the client driver.
>
>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>
>> +static struct tegra_io_rail_voltage_bit_info 
>> tegra210_io_rail_voltage_info[] = {
>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
>> +    TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
>> +    TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
>> +    TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
>> +    TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
>> +};
>
> That table is likely specific to Tegra210, yet ...
>
>> +static int tegra_io_rail_voltage_get_bit_pos(int io_rail_id)
>> +int tegra_io_rail_voltage_set(int io_rail, int val)
>> +int tegra_io_rail_voltage_get(int io_rail)
>
> ... these functions are all named as if they are generic. Presumably 
> they will indeed be needed for the next chip too? How will you prevent 
> their use, or turn these functions into no-ops, or return errors, on 
> other SoCs?

It will return error for the Soc which does to support or the parameter 
to the apis which are not applicable.

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Stephen Warren]

On 04/15/2016 10:21 AM, Laxman Dewangan wrote:
>
> On Friday 15 April 2016 09:54 PM, Stephen Warren wrote:
>> On 04/12/2016 08:56 AM, Laxman Dewangan wrote:
>>> NVIDIA Tegra210 supports some of the IO interface which can operate
>>> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
>>> Tegra PMC register to set different voltage level of IO interface based
>>> on IO rail voltage from power supply i.e. power regulators.
>>>
>>> Add APIs to set and get IO rail voltage from the client driver.
>>
>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>
>>> +static struct tegra_io_rail_voltage_bit_info
>>> tegra210_io_rail_voltage_info[] = {
>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
>>> +    TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
>>> +    TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
>>> +    TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
>>> +    TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
>>> +};
>>
>> That table is likely specific to Tegra210, yet ...
>>
>>> +static int tegra_io_rail_voltage_get_bit_pos(int io_rail_id)
>>> +int tegra_io_rail_voltage_set(int io_rail, int val)
>>> +int tegra_io_rail_voltage_get(int io_rail)
>>
>> ... these functions are all named as if they are generic. Presumably
>> they will indeed be needed for the next chip too? How will you prevent
>> their use, or turn these functions into no-ops, or return errors, on
>> other SoCs?
>
> It will return error for the Soc which does to support or the parameter
> to the apis which are not applicable.

Are you saying that will happen in the current code? I don't see where 
there's anything that validates that.

Or does "will" mean "I will do that in the next patch revision"?

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Laxman Dewangan]

On Friday 15 April 2016 10:11 PM, Stephen Warren wrote:
> On 04/15/2016 10:21 AM, Laxman Dewangan wrote:
>>
>> On Friday 15 April 2016 09:54 PM, Stephen Warren wrote:
>>> On 04/12/2016 08:56 AM, Laxman Dewangan wrote:
>>>> NVIDIA Tegra210 supports some of the IO interface which can operate
>>>> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
>>>> Tegra PMC register to set different voltage level of IO interface 
>>>> based
>>>> on IO rail voltage from power supply i.e. power regulators.
>>>>
>>>> Add APIs to set and get IO rail voltage from the client driver.
>>>
>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>
>>>> +static struct tegra_io_rail_voltage_bit_info
>>>> tegra210_io_rail_voltage_info[] = {
>>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
>>>> +    TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
>>>> +};
>>>
>>> That table is likely specific to Tegra210, yet ...
>>>
>>>> +static int tegra_io_rail_voltage_get_bit_pos(int io_rail_id)
>>>> +int tegra_io_rail_voltage_set(int io_rail, int val)
>>>> +int tegra_io_rail_voltage_get(int io_rail)
>>>
>>> ... these functions are all named as if they are generic. Presumably
>>> they will indeed be needed for the next chip too? How will you prevent
>>> their use, or turn these functions into no-ops, or return errors, on
>>> other SoCs?
>>
>> It will return error for the Soc which does to support or the parameter
>> to the apis which are not applicable.
>
> Are you saying that will happen in the current code? I don't see where 
> there's anything that validates that.
>
> Or does "will" mean "I will do that in the next patch revision"?

I have code like this in this patch


+int tegra_pmc_pad_voltage_update(unsigned long offset, unsigned long mask,
+                                unsigned long val)
+{
+       unsigned long flags;
+
+       if (!pmc->soc->has_pad_voltage_config)
+               return -ENODEV;
+


So this flag is try only for T210 and all previous chip has false setting.

Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails

[Stephen Warren]

On 04/15/2016 10:33 AM, Laxman Dewangan wrote:
>
> On Friday 15 April 2016 10:11 PM, Stephen Warren wrote:
>> On 04/15/2016 10:21 AM, Laxman Dewangan wrote:
>>>
>>> On Friday 15 April 2016 09:54 PM, Stephen Warren wrote:
>>>> On 04/12/2016 08:56 AM, Laxman Dewangan wrote:
>>>>> NVIDIA Tegra210 supports some of the IO interface which can operate
>>>>> at 1.8V or 3.3V I/O rail voltage levels. SW needs to configure
>>>>> Tegra PMC register to set different voltage level of IO interface
>>>>> based
>>>>> on IO rail voltage from power supply i.e. power regulators.
>>>>>
>>>>> Add APIs to set and get IO rail voltage from the client driver.
>>>>
>>>>> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
>>>>
>>>>> +static struct tegra_io_rail_voltage_bit_info
>>>>> tegra210_io_rail_voltage_info[] = {
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC1, 12),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(SDMMC3, 13),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(AUDIO_HV, 18),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(DMIC, 20),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(GPIO, 21),
>>>>> +    TEGRA_IO_RAIL_VOLTAGE(SPI_HV, 23),
>>>>> +};
>>>>
>>>> That table is likely specific to Tegra210, yet ...
>>>>
>>>>> +static int tegra_io_rail_voltage_get_bit_pos(int io_rail_id)
>>>>> +int tegra_io_rail_voltage_set(int io_rail, int val)
>>>>> +int tegra_io_rail_voltage_get(int io_rail)
>>>>
>>>> ... these functions are all named as if they are generic. Presumably
>>>> they will indeed be needed for the next chip too? How will you prevent
>>>> their use, or turn these functions into no-ops, or return errors, on
>>>> other SoCs?
>>>
>>> It will return error for the Soc which does to support or the parameter
>>> to the apis which are not applicable.
>>
>> Are you saying that will happen in the current code? I don't see where
>> there's anything that validates that.
>>
>> Or does "will" mean "I will do that in the next patch revision"?
>
> I have code like this in this patch
>
>
> +int tegra_pmc_pad_voltage_update(unsigned long offset, unsigned long mask,
> +                                unsigned long val)
> +{
> +       unsigned long flags;
> +
> +       if (!pmc->soc->has_pad_voltage_config)
> +               return -ENODEV;
> +
>
>
> So this flag is try only for T210 and all previous chip has false setting.

I assume that's in the next patch revision? I don't see it in the 
patches you posted.

[PATCH 5/7] soc/tegra: pmc: Register sub-devices of PMC

[Laxman Dewangan]

Register sub devices of the PMC to support multiple functionalities
of PMC.
The sub devices are the subnode of PMC DT node with containing the
compatible string of sub devices as follows:

        pmc@0,7000e400 {
		pmc-pad-control {
			compatible = "nvidia,tegra210-io-pad";
			::
		};
	};

In this pmc-pad-control is the sub device of PMC and the device
compatibility is nvidia,tegra210-io-pad.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/soc/tegra/pmc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 968f7cb..c044f3b 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -32,6 +32,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
 #include <linux/reboot.h>
 #include <linux/reset.h>
 #include <linux/seq_file.h>
@@ -1002,6 +1003,11 @@ static int tegra_pmc_probe(struct platform_device *pdev)
 	pmc->base = base;
 	mutex_unlock(&pmc->powergates_lock);
 
+	err = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
+	if (err < 0)
+		dev_err(&pdev->dev,
+			"Failed to register PMC Sub module driver: %d\n", err);
+
 	return 0;
 }
 
-- 
2.1.4

Re: [PATCH 5/7] soc/tegra: pmc: Register sub-devices of PMC

[Stephen Warren]

On 04/12/2016 08:56 AM, Laxman Dewangan wrote:
> Register sub devices of the PMC to support multiple functionalities
> of PMC.
> The sub devices are the subnode of PMC DT node with containing the
> compatible string of sub devices as follows:
>
>          pmc@0,7000e400 {
> 		pmc-pad-control {
> 			compatible = "nvidia,tegra210-io-pad";
> 			::
> 		};
> 	};
>
> In this pmc-pad-control is the sub device of PMC and the device
> compatibility is nvidia,tegra210-io-pad.

Since I don't think anyone has mentioned this point on the public lists, 
I'll echo the earlier internal discussion on this patch. I think the PMC 
driver should handle this all entirely internally. The PMC HW module 
doesn't have well-defined sub-modules, and hence we shouldn't represent 
sub-modules in DT. DT should represent HW structure, not Linux SW structure.

[PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Laxman Dewangan]

NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
or 3.3V I/O voltage levels. Also IO pads can be configured for
power down state if it is not in used. SW needs to configure the
voltage level of IO pads based on IO rail voltage and its power
state based on platform usage.

Add DT binding document for detailing the DT properties for
configuring IO pads voltage levels and its power state.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 .../bindings/pinctrl/nvidia,tegra210-io-pad.txt    | 102 +++++++++++++++++++++
 .../dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h  |  24 +++++
 2 files changed, 126 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
 create mode 100644 include/dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h

diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
new file mode 100644
index 0000000..97cdd4f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
@@ -0,0 +1,102 @@
+NVIDIA Tegra210 PMC IO pad controller
+
+NVIDIA Tegra210 supports IO pads which can operate at 1.8V or 3.3V I/O
+power rail voltages. SW needs to configure the voltage level of IO pads
+based on platform specific power tree.
+
+The voltage configurations of IO pads should be done in boot if it is not
+going to change other wise dynamically based on IO rail voltage on that
+IO pads.
+
+The node for the Tegra210 io-pad driver must be sub node of pmc@0,7000e400.
+
+Required properties:
+- compatible: "nvidia,tegra210-io-pad"
+
+Please refer to <pinctrl-bindings.txt> in this directory for details of the
+common pinctrl bindings used by client devices, including the meaning of the
+phrase "pin configuration node".
+
+Tegra's pin configuration nodes act as a container for an arbitrary number of
+subnodes. Each of these subnodes represents some desired configuration for an
+IO pads, or a list of IO pads. This configuration can include the voltage and
+power enable/disable control
+
+The name of each subnode is not important; all subnodes should be enumerated
+and processed purely based on their content. Each subnode only affects those
+parameters that are explicitly listed. Unspecified is represented as an absent
+property,
+
+See the TRM to determine which properties and values apply to each IO pads.
+Macro values for property values are defined in
+<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
+
+The voltage supported on the pads are 1.8V and 3.3V. The enums are defined as:
+	For 1.8V, use TEGRA210_IO_RAIL_1800000UV
+	For 3.3V, use TEGRA210_IO_RAIL_3300000UV
+
+Required subnode-properties:
+==========================
+- pins : An array of strings. Each string contains the name of an IO pads. Valid
+	 values for these names are listed below.
+
+Optional subnode-properties:
+==========================
+-nvidia,io-rail-voltage:	Integer. The voltage level of IO pads. The
+				valid values are 1.8V and 3.3V. Macros are
+				defined for these voltage levels in
+				<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
+					Use TEGRA210_IO_RAIL_1800000UV for 1.8V
+					Use TEGRA210_IO_RAIL_3300000UV for 3.3V
+
+-nvidia,io-pad-deep-power-down: Integer, representing the deep power down state
+				of the IO pads. If this is enable then IO pads
+				will be in power down state and interface is not
+				enabled for any transaction. This is power
+				saving mode of the IO pads. The macros are
+				defined for enable/disable in
+				<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
+				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE for
+					disable.
+				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_ENABLE for
+					enable.
+Valid values for pin are:
+	audio, audio-hv, cam, csia, csib, csic, csid, csie, csif,
+	dbg, debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2,
+	gpio, hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2,
+	pex-ctrl, sdmmc1, sdmmc3, spi, spi-hv, uart, usb-bias, usb0,
+	usb1, usb2, usb3.
+
+All IO pads do not support the 1.8V/3.3V configurations. Valid values for
+nvidia,io-rail-voltage are:
+	audio-hv, dmic, gpio, sdmmc1, sdmmc3, spi-hv.
+
+All above IO pads supports the deep power down state.
+
+Example:
+	#include <dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
+	pmc@0,7000e400 {
+		pmc-pad-control {
+			compatible = "nvidia,tegra210-io-pad";
+			pinctrl-names = "default";
+			pinctrl-0 = <&tegra_io_pad_volt_default>;
+			tegra_io_pad_volt_default: common {
+				audio {
+					pins = "audio";
+					nvidia,io-pad-deep-power-down = <TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE>;
+				};
+				audio-hv {
+					pins = "audio-hv";
+					nvidia,io-rail-voltage = <TEGRA210_IO_RAIL_1800000UV>;
+				};
+				gpio {
+					pins = "gpio";
+					nvidia,io-rail-voltage = <TEGRA210_IO_RAIL_1800000UV>;
+				};
+				rest {
+					pins = "dmic", "sdmmc1", "sdmmc3";
+					nvidia,io-rail-voltage = <TEGRA210_IO_RAIL_1800000UV>;
+				};
+			};
+		};
+	};
diff --git a/include/dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h b/include/dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h
new file mode 100644
index 0000000..e32166b
--- /dev/null
+++ b/include/dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h
@@ -0,0 +1,24 @@
+/*
+ * This header provides constants for Tegra210 IO pads pinctrl bindings.
+ *
+ * Copyright (c) 2016, NVIDIA CORPORATION.  All rights reserved.
+ *
+ * Author: Laxman Dewangan <ldewangan@nvidia.com>
+ *
+ * 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.
+ */
+
+#ifndef _DT_BINDINGS_PINCTRL_TEGRA210_IO_PAD_H
+#define _DT_BINDINGS_PINCTRL_TEGRA210_IO_PAD_H
+
+/* Voltage levels of Tegra210 IO rails. */
+#define TEGRA210_IO_RAIL_1800000UV		0
+#define TEGRA210_IO_RAIL_3300000UV		1
+
+/* Deep power down state enable/disable for Tegra210 IO pads */
+#define TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE	0
+#define TEGRA210_IO_PAD_DEEP_POWER_DOWN_ENABLE	1
+
+#endif
-- 
2.1.4

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Jon Hunter]

On 12/04/16 15:56, Laxman Dewangan wrote:
> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
> or 3.3V I/O voltage levels. Also IO pads can be configured for
> power down state if it is not in used. SW needs to configure the
> voltage level of IO pads based on IO rail voltage and its power
> state based on platform usage.
> 
> Add DT binding document for detailing the DT properties for
> configuring IO pads voltage levels and its power state.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
>  .../bindings/pinctrl/nvidia,tegra210-io-pad.txt    | 102 +++++++++++++++++++++
>  .../dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h  |  24 +++++
>  2 files changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
>  create mode 100644 include/dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
> new file mode 100644
> index 0000000..97cdd4f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
> @@ -0,0 +1,102 @@
> +NVIDIA Tegra210 PMC IO pad controller
> +
> +NVIDIA Tegra210 supports IO pads which can operate at 1.8V or 3.3V I/O
> +power rail voltages. SW needs to configure the voltage level of IO pads
> +based on platform specific power tree.
> +
> +The voltage configurations of IO pads should be done in boot if it is not
> +going to change other wise dynamically based on IO rail voltage on that
> +IO pads.
> +
> +The node for the Tegra210 io-pad driver must be sub node of pmc@0,7000e400.

This should be 'pmc@7000e400'. We were incorrectly adding the '0,'
previously.

> +
> +Required properties:
> +- compatible: "nvidia,tegra210-io-pad"

I think you have have "Must be ..." here. I am also wondering if the
pinctrl device should be registered by the pmc driver and so not a
separate driver to the PMC driver. In other words, the PMC driver calls
pinctrl_register() directly.

> +Please refer to <pinctrl-bindings.txt> in this directory for details of the
> +common pinctrl bindings used by client devices, including the meaning of the
> +phrase "pin configuration node".
> +
> +Tegra's pin configuration nodes act as a container for an arbitrary number of
> +subnodes. Each of these subnodes represents some desired configuration for an
> +IO pads, or a list of IO pads. This configuration can include the voltage and
> +power enable/disable control
> +
> +The name of each subnode is not important; all subnodes should be enumerated
> +and processed purely based on their content. Each subnode only affects those
> +parameters that are explicitly listed. Unspecified is represented as an absent
> +property,
> +
> +See the TRM to determine which properties and values apply to each IO pads.
> +Macro values for property values are defined in
> +<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
> +
> +The voltage supported on the pads are 1.8V and 3.3V. The enums are defined as:
> +	For 1.8V, use TEGRA210_IO_RAIL_1800000UV
> +	For 3.3V, use TEGRA210_IO_RAIL_3300000UV

You may consider just using integer values here like we do for regulators.

> +
> +Required subnode-properties:
> +==========================
> +- pins : An array of strings. Each string contains the name of an IO pads. Valid
> +	 values for these names are listed below.

Why are they not listed here? Array of strings sounds odd. Array/list of
pin names seems more appropriate.

> +Optional subnode-properties:
> +==========================
> +-nvidia,io-rail-voltage:	Integer. The voltage level of IO pads. The
> +				valid values are 1.8V and 3.3V. Macros are
> +				defined for these voltage levels in
> +				<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
> +					Use TEGRA210_IO_RAIL_1800000UV for 1.8V
> +					Use TEGRA210_IO_RAIL_3300000UV for 3.3V
> +
> +-nvidia,io-pad-deep-power-down: Integer, representing the deep power down state
> +				of the IO pads. If this is enable then IO pads
> +				will be in power down state and interface is not
> +				enabled for any transaction. This is power
> +				saving mode of the IO pads. The macros are
> +				defined for enable/disable in
> +				<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
> +				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE for
> +					disable.
> +				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_ENABLE for
> +					enable.

Sounds like a boolean parameter. So may consider that if the property
'nvidia,io-pad-deep-power-down' is present then it means enable
deep-power-down and if not present then don't. Then you do not need to
assign a value to it.

> +Valid values for pin are:
> +	audio, audio-hv, cam, csia, csib, csic, csid, csie, csif,
> +	dbg, debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2,
> +	gpio, hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2,
> +	pex-ctrl, sdmmc1, sdmmc3, spi, spi-hv, uart, usb-bias, usb0,
> +	usb1, usb2, usb3.
> +
> +All IO pads do not support the 1.8V/3.3V configurations. Valid values for
> +nvidia,io-rail-voltage are:
> +	audio-hv, dmic, gpio, sdmmc1, sdmmc3, spi-hv.

May be this should be moved under the nvidia,io-rail-voltage description?

> +All above IO pads supports the deep power down state.

May be this should be moved under the nvidia,io-pad-deep-power-down
description?

> +Example:
> +	#include <dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
> +	pmc@0,7000e400 {
> +		pmc-pad-control {
> +			compatible = "nvidia,tegra210-io-pad";
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&tegra_io_pad_volt_default>;
> +			tegra_io_pad_volt_default: common {
> +				audio {
> +					pins = "audio";
> +					nvidia,io-pad-deep-power-down = <TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE>;
> +				};
> +				audio-hv {
> +					pins = "audio-hv";
> +					nvidia,io-rail-voltage = <TEGRA210_IO_RAIL_1800000UV>;
> +				};
> +				gpio {
> +					pins = "gpio";
> +					nvidia,io-rail-voltage = <TEGRA210_IO_RAIL_1800000UV>;
> +				};
> +				rest {
> +					pins = "dmic", "sdmmc1", "sdmmc3";
> +					nvidia,io-rail-voltage = <TEGRA210_IO_RAIL_1800000UV>;
> +				};

I know this is an example, but it does not make sense to me why audio-hv
and gpio and separated from 'rest' when they have the same configuration.

Cheers
Jon

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Laxman Dewangan]

On Wednesday 13 April 2016 02:34 PM, Jon Hunter wrote:
> On 12/04/16 15:56, Laxman Dewangan wrote:
>> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
>> or 3.3V I/O voltage levels. Also IO pads can be configured for
>> power down state if it is not in used. SW needs to configure the
>> voltage level of IO pads based on IO rail voltage and its power
>> state based on platform usage.
>>
>> Add DT binding document for detailing the DT properties for
>> configuring IO pads voltage levels and its power state.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> ---
>>   .../bindings/pinctrl/nvidia,tegra210-io-pad.txt    | 102 +++++++++++++++++++++
>>   .../dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h  |  24 +++++
>>   2 files changed, 126 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
>>   create mode 100644 include/dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h
>>
>> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
>> new file mode 100644
>> index 0000000..97cdd4f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra210-io-pad.txt
>> @@ -0,0 +1,102 @@
>> +NVIDIA Tegra210 PMC IO pad controller
>> +
>> +NVIDIA Tegra210 supports IO pads which can operate at 1.8V or 3.3V I/O
>> +power rail voltages. SW needs to configure the voltage level of IO pads
>> +based on platform specific power tree.
>> +
>> +The voltage configurations of IO pads should be done in boot if it is not
>> +going to change other wise dynamically based on IO rail voltage on that
>> +IO pads.
>> +
>> +The node for the Tegra210 io-pad driver must be sub node of pmc@0,7000e400.
> This should be 'pmc@7000e400'. We were incorrectly adding the '0,'
> previously.

For T210, 64 bit, it is
  tegra210.dtsi:    pmc: pmc@0,7000e400 {

For T124, it is

pmc@7000e400



>
>> +
>> +Required properties:
>> +- compatible: "nvidia,tegra210-io-pad"
> I think you have have "Must be ..." here. I am also wondering if the
> pinctrl device should be registered by the pmc driver and so not a
> separate driver to the PMC driver. In other words, the PMC driver calls
> pinctrl_register() directly.

I like to keep the pmc driver as main interface and other sub 
functionalities like pad control for voltage and power states, no 
iopower control etc as sub drivers. This will help in modular approach 
of the driver.


>
>> +The voltage supported on the pads are 1.8V and 3.3V. The enums are defined as:
>> +	For 1.8V, use TEGRA210_IO_RAIL_1800000UV
>> +	For 3.3V, use TEGRA210_IO_RAIL_3300000UV
> You may consider just using integer values here like we do for regulators.

We just support two values 1.8V and 3.3V only. I am fine with either way 
also.

>
>> +				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE for
>> +					disable.
>> +				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_ENABLE for
>> +					enable.
> Sounds like a boolean parameter. So may consider that if the property
> 'nvidia,io-pad-deep-power-down' is present then it means enable
> deep-power-down and if not present then don't. Then you do not need to
> assign a value to it.

Three states, enable, disable and left default. So absent will be left 
default.

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Jon Hunter]

On 13/04/16 10:08, Laxman Dewangan wrote:

[snip]

> For T210, 64 bit, it is
>  tegra210.dtsi:    pmc: pmc@0,7000e400 {

Yes and this is wrong [0].

>>> +Required properties:
>>> +- compatible: "nvidia,tegra210-io-pad"
>> I think you have have "Must be ..." here. I am also wondering if the
>> pinctrl device should be registered by the pmc driver and so not a
>> separate driver to the PMC driver. In other words, the PMC driver calls
>> pinctrl_register() directly.
> 
> I like to keep the pmc driver as main interface and other sub
> functionalities like pad control for voltage and power states, no
> iopower control etc as sub drivers. This will help in modular approach
> of the driver.

OK, let's see what Thierry thinks about this.

>>> +The voltage supported on the pads are 1.8V and 3.3V. The enums are
>>> defined as:
>>> +    For 1.8V, use TEGRA210_IO_RAIL_1800000UV
>>> +    For 3.3V, use TEGRA210_IO_RAIL_3300000UV
>> You may consider just using integer values here like we do for
>> regulators.
> 
> We just support two values 1.8V and 3.3V only. I am fine with either way
> also.
> 
>>
>>> +                  TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE for
>>> +                    disable.
>>> +                  TEGRA210_IO_PAD_DEEP_POWER_DOWN_ENABLE for
>>> +                    enable.
>> Sounds like a boolean parameter. So may consider that if the property
>> 'nvidia,io-pad-deep-power-down' is present then it means enable
>> deep-power-down and if not present then don't. Then you do not need to
>> assign a value to it.
> 
> Three states, enable, disable and left default. So absent will be left
> default.

Fine with me.

Cheers
Jon

[0] http://marc.info/?l=linux-tegra&m=146038329109871&w=2

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Jon Hunter]

On 12/04/16 15:56, Laxman Dewangan wrote:
> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
> or 3.3V I/O voltage levels. Also IO pads can be configured for
> power down state if it is not in used. SW needs to configure the
> voltage level of IO pads based on IO rail voltage and its power
> state based on platform usage.
> 
> Add DT binding document for detailing the DT properties for
> configuring IO pads voltage levels and its power state.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

[snip]

> +Required subnode-properties:
> +==========================
> +- pins : An array of strings. Each string contains the name of an IO pads. Valid
> +	 values for these names are listed below.
> +
> +Optional subnode-properties:
> +==========================
> +-nvidia,io-rail-voltage:	Integer. The voltage level of IO pads. The
> +				valid values are 1.8V and 3.3V. Macros are
> +				defined for these voltage levels in
> +				<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
> +					Use TEGRA210_IO_RAIL_1800000UV for 1.8V
> +					Use TEGRA210_IO_RAIL_3300000UV for 3.3V
> +
> +-nvidia,io-pad-deep-power-down: Integer, representing the deep power down state
> +				of the IO pads. If this is enable then IO pads
> +				will be in power down state and interface is not
> +				enabled for any transaction. This is power
> +				saving mode of the IO pads. The macros are
> +				defined for enable/disable in
> +				<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
> +				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE for
> +					disable.
> +				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_ENABLE for
> +					enable.
> +Valid values for pin are:
> +	audio, audio-hv, cam, csia, csib, csic, csid, csie, csif,
> +	dbg, debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2,
> +	gpio, hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2,
> +	pex-ctrl, sdmmc1, sdmmc3, spi, spi-hv, uart, usb-bias, usb0,
> +	usb1, usb2, usb3.

Thinking about this some more, the above are not IO pads but supply
pads, AFAICT. And these supply pads, are supplying the voltage to
various IO pads. I am not sure if these should be named vddio_xxx. The
'pins' properties says these are IO pads, but this does not seem correct.

We also need to think about how these supply pads are linked to the
actual IO pads. Or at least it seems they should be some how ...

Cheers
Jon

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Laxman Dewangan]

On Friday 15 April 2016 07:46 PM, Jon Hunter wrote:
> On 12/04/16 15:56, Laxman Dewangan wrote:
>> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
>> or 3.3V I/O voltage levels. Also IO pads can be configured for
>> power down state if it is not in used. SW needs to configure the
>> voltage level of IO pads based on IO rail voltage and its power
>> state based on platform usage.
>>
>> Add DT binding document for detailing the DT properties for
>> configuring IO pads voltage levels and its power state.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> [snip]
>
>> +Required subnode-properties:
>> +==========================
>> +- pins : An array of strings. Each string contains the name of an IO pads. Valid
>> +	 values for these names are listed below.
>> +
>> +Optional subnode-properties:
>> +==========================
>> +-nvidia,io-rail-voltage:	Integer. The voltage level of IO pads. The
>> +				valid values are 1.8V and 3.3V. Macros are
>> +				defined for these voltage levels in
>> +				<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
>> +					Use TEGRA210_IO_RAIL_1800000UV for 1.8V
>> +					Use TEGRA210_IO_RAIL_3300000UV for 3.3V
>> +
>> +-nvidia,io-pad-deep-power-down: Integer, representing the deep power down state
>> +				of the IO pads. If this is enable then IO pads
>> +				will be in power down state and interface is not
>> +				enabled for any transaction. This is power
>> +				saving mode of the IO pads. The macros are
>> +				defined for enable/disable in
>> +				<dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
>> +				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE for
>> +					disable.
>> +				  TEGRA210_IO_PAD_DEEP_POWER_DOWN_ENABLE for
>> +					enable.
>> +Valid values for pin are:
>> +	audio, audio-hv, cam, csia, csib, csic, csid, csie, csif,
>> +	dbg, debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2,
>> +	gpio, hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2,
>> +	pex-ctrl, sdmmc1, sdmmc3, spi, spi-hv, uart, usb-bias, usb0,
>> +	usb1, usb2, usb3.
> Thinking about this some more, the above are not IO pads but supply
> pads, AFAICT. And these supply pads, are supplying the voltage to
> various IO pads. I am not sure if these should be named vddio_xxx. The
> 'pins' properties says these are IO pads, but this does not seem correct.

These are IO pads. One IO rail  have multiple sub pads to power down 
some of interface when not used. Like if CSIA is active, we can power 
down CSIB, CSIC etc.
All CSI pads are lined to single IO rail.

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Jon Hunter]

On 15/04/16 15:12, Laxman Dewangan wrote:
> 
> On Friday 15 April 2016 07:46 PM, Jon Hunter wrote:
>> On 12/04/16 15:56, Laxman Dewangan wrote:
>>> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
>>> or 3.3V I/O voltage levels. Also IO pads can be configured for
>>> power down state if it is not in used. SW needs to configure the
>>> voltage level of IO pads based on IO rail voltage and its power
>>> state based on platform usage.
>>>
>>> Add DT binding document for detailing the DT properties for
>>> configuring IO pads voltage levels and its power state.
>>>
>>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
>> [snip]
>>
>>> +Required subnode-properties:
>>> +==========================
>>> +- pins : An array of strings. Each string contains the name of an IO
>>> pads. Valid
>>> +     values for these names are listed below.
>>> +
>>> +Optional subnode-properties:
>>> +==========================
>>> +-nvidia,io-rail-voltage:    Integer. The voltage level of IO pads. The
>>> +                valid values are 1.8V and 3.3V. Macros are
>>> +                defined for these voltage levels in
>>> +                <dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
>>> +                    Use TEGRA210_IO_RAIL_1800000UV for 1.8V
>>> +                    Use TEGRA210_IO_RAIL_3300000UV for 3.3V
>>> +
>>> +-nvidia,io-pad-deep-power-down: Integer, representing the deep power
>>> down state
>>> +                of the IO pads. If this is enable then IO pads
>>> +                will be in power down state and interface is not
>>> +                enabled for any transaction. This is power
>>> +                saving mode of the IO pads. The macros are
>>> +                defined for enable/disable in
>>> +                <dt-bindings/pinctrl/pinctrl-tegra210-io-pad.h>
>>> +                  TEGRA210_IO_PAD_DEEP_POWER_DOWN_DISABLE for
>>> +                    disable.
>>> +                  TEGRA210_IO_PAD_DEEP_POWER_DOWN_ENABLE for
>>> +                    enable.
>>> +Valid values for pin are:
>>> +    audio, audio-hv, cam, csia, csib, csic, csid, csie, csif,
>>> +    dbg, debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2,
>>> +    gpio, hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2,
>>> +    pex-ctrl, sdmmc1, sdmmc3, spi, spi-hv, uart, usb-bias, usb0,
>>> +    usb1, usb2, usb3.
>> Thinking about this some more, the above are not IO pads but supply
>> pads, AFAICT. And these supply pads, are supplying the voltage to
>> various IO pads. I am not sure if these should be named vddio_xxx. The
>> 'pins' properties says these are IO pads, but this does not seem correct.
> 
> These are IO pads. One IO rail  have multiple sub pads to power down
> some of interface when not used. Like if CSIA is active, we can power
> down CSIB, CSIC etc.

To me, 'IO rail' implies a supply rail, but this is not the same as an
IO pad (or pin/ball). And hence, I think the terminology here is confusing.

For example, audio_hv powers the following IO pads ...

DAP1_DIN
DAP1_DOUT
DAP1_FS
DAP1_SCLK
SPI2_MOSI
SPI2_MISO
SPI2_SCK
SPI2_CS0
SPI2_CS1

And sdmmc1 powers the following IO pads ...

SDMMC1_CLK
SDMMC1_CMD
SDMMC1_DAT0
SDMMC1_DAT1
SDMMC1_DAT2
SDMMC1_DAT3
SDMMC1_COMP

As for CSIA, I don't think this is a pin/pad at all, but a software
means to control the power down for the CSI_A_xxx pads. If CSIA is an IO
pad then what is the ball number for Tegra210? In the datasheet I only
see ...

CSI_A_CLK_N Y6
CSI_A_CLK_P Y7
CSI_A_D0_N Y4
CSI_A_D0_P Y5
CSI_A_D1_N Y1
CSI_A_D1_P AA1

> All CSI pads are lined to single IO rail.

I agree with this and from the data-sheet I see the rail that powers the
CSI (and DSI) interfaces is called AVDD_DSI_CSI. But again, in the DT
document you are referring to csia, csib, csic, csid, csie, csif as
pins, but these don't appear to be physical pins, and this appears to be
more of a software means to control power to the various csi_x pins.

It seems to me that each of the existing CSI_A_xxx pins/pads should be
mapped to or register with the appropriate power-down control and when
all pads are set to inactive this then triggers the power-down of all
the CSI_A_xxx pads.

Cheers
Jon

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Laxman Dewangan]

On Friday 15 April 2016 08:44 PM, Jon Hunter wrote:
> On 15/04/16 15:12, Laxman Dewangan wrote:
>>
>>
>> All CSI pads are lined to single IO rail.
> I agree with this and from the data-sheet I see the rail that powers the
> CSI (and DSI) interfaces is called AVDD_DSI_CSI. But again, in the DT
> document you are referring to csia, csib, csic, csid, csie, csif as
> pins, but these don't appear to be physical pins, and this appears to be
> more of a software means to control power to the various csi_x pins.
>
> It seems to me that each of the existing CSI_A_xxx pins/pads should be
> mapped to or register with the appropriate power-down control and when
> all pads are set to inactive this then triggers the power-down of all
> the CSI_A_xxx pads.

I used pins as this is the property from pincon generic so that I can 
use the generic implementation.

Here, I will not go to the pin level control as HW does not support pin 
level control.

I will say the unit should be interface level. Should we say 
IO_GROUP_CSIA, IO_GROUP_CSIB etc?


>

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Jon Hunter]

On 15/04/16 16:14, Laxman Dewangan wrote:
> On Friday 15 April 2016 08:44 PM, Jon Hunter wrote:
>> On 15/04/16 15:12, Laxman Dewangan wrote:
>>>
>>>
>>> All CSI pads are lined to single IO rail.
>> I agree with this and from the data-sheet I see the rail that powers the
>> CSI (and DSI) interfaces is called AVDD_DSI_CSI. But again, in the DT
>> document you are referring to csia, csib, csic, csid, csie, csif as
>> pins, but these don't appear to be physical pins, and this appears to be
>> more of a software means to control power to the various csi_x pins.
>>
>> It seems to me that each of the existing CSI_A_xxx pins/pads should be
>> mapped to or register with the appropriate power-down control and when
>> all pads are set to inactive this then triggers the power-down of all
>> the CSI_A_xxx pads.
>
> I used pins as this is the property from pincon generic so that I can
> use the generic implementation.
> 
> Here, I will not go to the pin level control as HW does not support pin
> level control.
> 
> I will say the unit should be interface level. Should we say
> IO_GROUP_CSIA, IO_GROUP_CSIB etc?

So we need to reflect the hardware in device-tree and although yes the
power-down for the CSI_x_xxx pads are all controlled together as a
single group, it does not feel right that we add a pseudo pin called
csix to represent these.

The CSI_x_xxx pads are already in device-tree and so why not add a
property to each of these pads which has the IO rail information for
power-down and voltage-select?

Cheers
Jon

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Laxman Dewangan]

On Friday 15 April 2016 09:15 PM, Jon Hunter wrote:
> On 15/04/16 16:14, Laxman Dewangan wrote:
>>
>> I used pins as this is the property from pincon generic so that I can
>> use the generic implementation.
>>
>> Here, I will not go to the pin level control as HW does not support pin
>> level control.
>>
>> I will say the unit should be interface level. Should we say
>> IO_GROUP_CSIA, IO_GROUP_CSIB etc?
> So we need to reflect the hardware in device-tree and although yes the
> power-down for the CSI_x_xxx pads are all controlled together as a
> single group, it does not feel right that we add a pseudo pin called
> csix to represent these.
>
> The CSI_x_xxx pads are already in device-tree and so why not add a
> property to each of these pads which has the IO rail information for
> power-down and voltage-select?

Which dt binding docs have these?
I looked for nvidia,tegra210-pinmux.txt and not able to find csi_xxx.

Here I dont want to refer the individual pins as control should be as group.

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Jon Hunter]

On 15/04/16 17:41, Laxman Dewangan wrote:
> 
> On Friday 15 April 2016 09:15 PM, Jon Hunter wrote:
>> On 15/04/16 16:14, Laxman Dewangan wrote:
>>>
>>> I used pins as this is the property from pincon generic so that I can
>>> use the generic implementation.
>>>
>>> Here, I will not go to the pin level control as HW does not support pin
>>> level control.
>>>
>>> I will say the unit should be interface level. Should we say
>>> IO_GROUP_CSIA, IO_GROUP_CSIB etc?
>> So we need to reflect the hardware in device-tree and although yes the
>> power-down for the CSI_x_xxx pads are all controlled together as a
>> single group, it does not feel right that we add a pseudo pin called
>> csix to represent these.
>>
>> The CSI_x_xxx pads are already in device-tree and so why not add a
>> property to each of these pads which has the IO rail information for
>> power-down and voltage-select?
> 
> Which dt binding docs have these?
> I looked for nvidia,tegra210-pinmux.txt and not able to find csi_xxx.

For CSI you are right they are not included by the current DT binding
docs, however, the sdmmc1/3 pads are. So that makes things a bit more
messy as some are and some are not.

> Here I dont want to refer the individual pins as control should be as
> group.

I understand, however, at least for power-down control I don't see why
we cannot refer to the individual pins and once all are inactive then
the rail can be powered down.

For switching the voltage it is a bit more complex, but may be we could
still look-up the IO rail based upon the pads the device uses.

Cheers
Jon

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Laxman Dewangan]

On Friday 15 April 2016 11:14 PM, Jon Hunter wrote:
> On 15/04/16 17:41, Laxman Dewangan wrote:
>> On Friday 15 April 2016 09:15 PM, Jon Hunter wrote:
>>> On 15/04/16 16:14, Laxman Dewangan wrote:
>>>> I used pins as this is the property from pincon generic so that I can
>>>> use the generic implementation.
>>>>
>>>> Here, I will not go to the pin level control as HW does not support pin
>>>> level control.
>>>>
>>>> I will say the unit should be interface level. Should we say
>>>> IO_GROUP_CSIA, IO_GROUP_CSIB etc?
>>> So we need to reflect the hardware in device-tree and although yes the
>>> power-down for the CSI_x_xxx pads are all controlled together as a
>>> single group, it does not feel right that we add a pseudo pin called
>>> csix to represent these.
>>>
>>> The CSI_x_xxx pads are already in device-tree and so why not add a
>>> property to each of these pads which has the IO rail information for
>>> power-down and voltage-select?
>> Which dt binding docs have these?
>> I looked for nvidia,tegra210-pinmux.txt and not able to find csi_xxx.
> For CSI you are right they are not included by the current DT binding
> docs, however, the sdmmc1/3 pads are. So that makes things a bit more
> messy as some are and some are not.

Yaah and so lets have the names in new dt files. Names may be same but 
define all possible names f groups in dt binding and need not to refer 
from other file which does not have all.

>> Here I dont want to refer the individual pins as control should be as
>> group.
> I understand, however, at least for power-down control I don't see why
> we cannot refer to the individual pins and once all are inactive then
> the rail can be powered down.
>
> For switching the voltage it is a bit more complex, but may be we could
> still look-up the IO rail based upon the pads the device uses.
>

Yes, it can be done with ref count also for power down.
But interfaces are complex. As a client, it is easy to say power down 
SDMMC1 IO interface rather than saying power down 10 pins (names) of 
that group.
We need to provide all these from DT for dynamic and static 
configuration and listing all pins of groups are complicate then the pad 
names.

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Jon Hunter]

On 15/04/16 18:49, Laxman Dewangan wrote:
> 
> On Friday 15 April 2016 11:14 PM, Jon Hunter wrote:
>> On 15/04/16 17:41, Laxman Dewangan wrote:
>>> On Friday 15 April 2016 09:15 PM, Jon Hunter wrote:
>>>> On 15/04/16 16:14, Laxman Dewangan wrote:
>>>>> I used pins as this is the property from pincon generic so that I can
>>>>> use the generic implementation.
>>>>>
>>>>> Here, I will not go to the pin level control as HW does not support
>>>>> pin
>>>>> level control.
>>>>>
>>>>> I will say the unit should be interface level. Should we say
>>>>> IO_GROUP_CSIA, IO_GROUP_CSIB etc?
>>>> So we need to reflect the hardware in device-tree and although yes the
>>>> power-down for the CSI_x_xxx pads are all controlled together as a
>>>> single group, it does not feel right that we add a pseudo pin called
>>>> csix to represent these.
>>>>
>>>> The CSI_x_xxx pads are already in device-tree and so why not add a
>>>> property to each of these pads which has the IO rail information for
>>>> power-down and voltage-select?
>>> Which dt binding docs have these?
>>> I looked for nvidia,tegra210-pinmux.txt and not able to find csi_xxx.
>> For CSI you are right they are not included by the current DT binding
>> docs, however, the sdmmc1/3 pads are. So that makes things a bit more
>> messy as some are and some are not.
> 
> Yaah and so lets have the names in new dt files. Names may be same but
> define all possible names f groups in dt binding and need not to refer
> from other file which does not have all.

I still do not like that. In the case of sdmmc we now have two pinctrl
drivers to deal with for a single set of pins. That does not seem
correct IMO.

>>> Here I dont want to refer the individual pins as control should be as
>>> group.
>> I understand, however, at least for power-down control I don't see why
>> we cannot refer to the individual pins and once all are inactive then
>> the rail can be powered down.
>>
>> For switching the voltage it is a bit more complex, but may be we could
>> still look-up the IO rail based upon the pads the device uses.
>>
> 
> Yes, it can be done with ref count also for power down.

Exactly.

> But interfaces are complex. As a client, it is easy to say power down
> SDMMC1 IO interface rather than saying power down 10 pins (names) of
> that group.

Right and like I said, we could always look up the IO rail from the pins
associated once at probe time and then control it from there.

Cheers
Jon

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Laxman Dewangan]

On Saturday 16 April 2016 12:00 AM, Jon Hunter wrote:
> On 15/04/16 18:49, Laxman Dewangan wrote:
>> On Friday 15 April 2016 11:14 PM, Jon Hunter wrote:
>>> On 15/04/16 17:41, Laxman Dewangan wrote:
>>>> On Friday 15 April 2016 09:15 PM, Jon Hunter wrote:
>>>>> On 15/04/16 16:14, Laxman Dewangan wrote:
>>>>>> I used pins as this is the property from pincon generic so that I can
>>>>>> use the generic implementation.
>>>>>>
>>>>>> Here, I will not go to the pin level control as HW does not support
>>>>>> pin
>>>>>> level control.
>>>>>>
>>>>>> I will say the unit should be interface level. Should we say
>>>>>> IO_GROUP_CSIA, IO_GROUP_CSIB etc?
>>>>> So we need to reflect the hardware in device-tree and although yes the
>>>>> power-down for the CSI_x_xxx pads are all controlled together as a
>>>>> single group, it does not feel right that we add a pseudo pin called
>>>>> csix to represent these.
>>>>>
>>>>> The CSI_x_xxx pads are already in device-tree and so why not add a
>>>>> property to each of these pads which has the IO rail information for
>>>>> power-down and voltage-select?
>>>> Which dt binding docs have these?
>>>> I looked for nvidia,tegra210-pinmux.txt and not able to find csi_xxx.
>>> For CSI you are right they are not included by the current DT binding
>>> docs, however, the sdmmc1/3 pads are. So that makes things a bit more
>>> messy as some are and some are not.
>> Yaah and so lets have the names in new dt files. Names may be same but
>> define all possible names f groups in dt binding and need not to refer
>> from other file which does not have all.
> I still do not like that. In the case of sdmmc we now have two pinctrl
> drivers to deal with for a single set of pins. That does not seem
> correct IMO.

We are ending two drivers because of the HW blocks. Pins interface and 
pad control are seen two different blocks.

Do you want to add the IO group names also in existing pin control 
driver and the new property, power-enable/disable and 
power-source-voltage belongs to these new io group names.

In this way we will have single driver. We need to see how we can 
support group/pins together.


>
>
>> But interfaces are complex. As a client, it is easy to say power down
>> SDMMC1 IO interface rather than saying power down 10 pins (names) of
>> that group.
> Right and like I said, we could always look up the IO rail from the pins
> associated once at probe time and then control it from there.

I did not get it fully. Can you please help on this using some psuedo 
code and dt property.

For  init, we can pass the regulator handle of the supply to this driver 
and during probe, it can get the voltage from regulator call and then 
set 1.8V or 3.3V. So we need to provide regulator handle from DT instead 
of voltage for probe configuration. Is this what you mean?

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Stephen Warren]

On 04/12/2016 08:56 AM, Laxman Dewangan wrote:
> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
> or 3.3V I/O voltage levels. Also IO pads can be configured for
> power down state if it is not in used. SW needs to configure the
> voltage level of IO pads based on IO rail voltage and its power
> state based on platform usage.
>
> Add DT binding document for detailing the DT properties for
> configuring IO pads voltage levels and its power state.

I hope that we only intend to use this in the case where Linux must make 
dynamic changes to the IO voltage (e.g. SD cards switching between speeds).

All static settings, and good boot defaults, should be set up by system 
FW. Perhaps not all FW does this on Tegra210 platforms:-( I hope that on 
future chips, the same FW that sets up the static pinmux sets up the 
static IO voltage configuration, in exactly the same way.

Re: [PATCH 6/7] pinctrl: tegra: Add DT binding for io pads control

[Laxman Dewangan]

On Friday 15 April 2016 10:05 PM, Stephen Warren wrote:
> On 04/12/2016 08:56 AM, Laxman Dewangan wrote:
>> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
>> or 3.3V I/O voltage levels. Also IO pads can be configured for
>> power down state if it is not in used. SW needs to configure the
>> voltage level of IO pads based on IO rail voltage and its power
>> state based on platform usage.
>>
>> Add DT binding document for detailing the DT properties for
>> configuring IO pads voltage levels and its power state.
>
> I hope that we only intend to use this in the case where Linux must 
> make dynamic changes to the IO voltage (e.g. SD cards switching 
> between speeds).
>
> All static settings, and good boot defaults, should be set up by 
> system FW. Perhaps not all FW does this on Tegra210 platforms:-( I 
> hope that on future chips, the same FW that sets up the static pinmux 
> sets up the static IO voltage configuration, in exactly the same way.

Exactly yes.

[PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Laxman Dewangan]

NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
or 3.3V I/O voltage levels. Also the IO pads can be configured
for power down state if it is not used. SW needs to configure the
voltage level of IO pads based on IO rail voltage and its power
state based on platform usage.

The voltage and power state configurations of pads are provided
through pin control frameworks. Add pin control driver for Tegra's
IO pads' voltage and power state configurations.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/pinctrl/tegra/Kconfig                   |  11 +
 drivers/pinctrl/tegra/Makefile                  |   1 +
 drivers/pinctrl/tegra/pinctrl-tegra210-io-pad.c | 302 ++++++++++++++++++++++++
 3 files changed, 314 insertions(+)
 create mode 100644 drivers/pinctrl/tegra/pinctrl-tegra210-io-pad.c

diff --git a/drivers/pinctrl/tegra/Kconfig b/drivers/pinctrl/tegra/Kconfig
index 24e20cc..37b85d7 100644
--- a/drivers/pinctrl/tegra/Kconfig
+++ b/drivers/pinctrl/tegra/Kconfig
@@ -23,6 +23,17 @@ config PINCTRL_TEGRA210
 	bool
 	select PINCTRL_TEGRA
 
+config PINCTRL_TEGRA210_IO_PAD
+	bool "Tegra210 IO pad Control Driver"
+	depends on ARCH_TEGRA
+	select PINCONF
+	select PINMUX
+	help
+	  NVIDIA Tegra210 SoC has IO pads which supports mult-voltage level
+	  of interfacing. The voltage of IO pads are SW configurable based
+	  on IO rail of that pads. This driver provides the interface to
+	  change IO pad voltage and power state via pincontrol interface.
+
 config PINCTRL_TEGRA_XUSB
 	def_bool y if ARCH_TEGRA
 	select GENERIC_PHY
diff --git a/drivers/pinctrl/tegra/Makefile b/drivers/pinctrl/tegra/Makefile
index a927379..90f4000 100644
--- a/drivers/pinctrl/tegra/Makefile
+++ b/drivers/pinctrl/tegra/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_PINCTRL_TEGRA30)		+= pinctrl-tegra30.o
 obj-$(CONFIG_PINCTRL_TEGRA114)		+= pinctrl-tegra114.o
 obj-$(CONFIG_PINCTRL_TEGRA124)		+= pinctrl-tegra124.o
 obj-$(CONFIG_PINCTRL_TEGRA210)		+= pinctrl-tegra210.o
+obj-$(CONFIG_PINCTRL_TEGRA210_IO_PAD)	+= pinctrl-tegra210-io-pad.o
 obj-$(CONFIG_PINCTRL_TEGRA_XUSB)	+= pinctrl-tegra-xusb.o
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra210-io-pad.c b/drivers/pinctrl/tegra/pinctrl-tegra210-io-pad.c
new file mode 100644
index 0000000..4959c19
--- /dev/null
+++ b/drivers/pinctrl/tegra/pinctrl-tegra210-io-pad.c
@@ -0,0 +1,302 @@
+/*
+ * Generic ADC thermal driver
+ *
+ * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved.
+ *
+ * Author: Laxman Dewangan <ldewangan@nvidia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <soc/tegra/pmc.h>
+
+#include "../core.h"
+#include "../pinconf.h"
+#include "../pinctrl-utils.h"
+
+enum tegra_io_rail_pads_params {
+	TEGRA_IO_RAIL_VOLTAGE = PIN_CONFIG_END + 1,
+	TEGRA_IO_PAD_DEEP_POWER_DOWN,
+};
+
+static const struct pinconf_generic_params tegra_io_pads_cfg_params[] = {
+	{
+		.property = "nvidia,io-rail-voltage",
+		.param = TEGRA_IO_RAIL_VOLTAGE,
+	}, {
+		.property = "nvidia,io-pad-deep-power-down",
+		.param = TEGRA_IO_PAD_DEEP_POWER_DOWN,
+	},
+};
+
+struct tegra_io_pads_cfg_info {
+	const char *name;
+	const unsigned int pins[1];
+	int io_rail_id;
+};
+
+#define TEGRA210_PAD_INFO_TABLE(_entry_)		\
+	_entry_(0, "audio", AUDIO),			\
+	_entry_(1, "audio-hv", AUDIO_HV),	\
+	_entry_(2, "cam", CAM),			\
+	_entry_(3, "csia", CSIA),			\
+	_entry_(4, "csib", CSIB),			\
+	_entry_(5, "csic", CSIC),			\
+	_entry_(6, "csid", CSID),			\
+	_entry_(7, "csie", CSIE),			\
+	_entry_(8, "csif", CSIF),			\
+	_entry_(9, "dbg", DBG),			\
+	_entry_(10, "debug-nonao", DBG_NONAO), \
+	_entry_(11, "dmic", DMIC),			\
+	_entry_(12, "dp", DP),				\
+	_entry_(13, "dsi", DSI),			\
+	_entry_(14, "dsib", DSIB),			\
+	_entry_(15, "dsic", DSIC),			\
+	_entry_(16, "dsid", DSID),			\
+	_entry_(17, "emmc", SDMMC4),			\
+	_entry_(18, "emmc2", EMMC2),			\
+	_entry_(19, "gpio", GPIO),			\
+	_entry_(20, "hdmi", HDMI),			\
+	_entry_(21, "hsic", HSIC),			\
+	_entry_(22, "lvds", LVDS),			\
+	_entry_(23, "mipi-bias", MIPI_BIAS),	\
+	_entry_(24, "pex-bias", PEX_BIAS),	\
+	_entry_(25, "pex-clk1", PEX_CLK1),	\
+	_entry_(26, "pex-clk2", PEX_CLK2),	\
+	_entry_(27, "pex-ctrl", PEX_CNTRL),	\
+	_entry_(28, "sdmmc1", SDMMC1),		\
+	_entry_(29, "sdmmc3", SDMMC3),		\
+	_entry_(30, "spi", SPI),			\
+	_entry_(31, "spi-hv", SPI_HV),		\
+	_entry_(32, "uart", UART),			\
+	_entry_(33, "usb-bias", USB_BIAS),	\
+	_entry_(34, "usb0", USB0),			\
+	_entry_(35, "usb1", USB1),			\
+	_entry_(36, "usb2", USB2),			\
+	_entry_(37, "usb3", USB3)
+
+#define TEGRA_IO_PAD_INFO(_id, _name, _io_rail_id)			\
+	{								\
+		.name = _name,						\
+		.pins = {(_id)},					\
+		.io_rail_id = TEGRA_IO_RAIL_##_io_rail_id,		\
+	}
+
+static struct tegra_io_pads_cfg_info tegra210_io_pads_cfg_info[] = {
+	TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
+};
+
+#define TEGRA_IO_PAD_DESC(_id, _name, _io_rail_id)			\
+	PINCTRL_PIN(_id, _name)
+
+static const struct pinctrl_pin_desc tegra210_io_pads_pinctrl_desc[] = {
+	TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_DESC),
+};
+
+struct tegra_io_pads_info {
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	struct tegra_io_pads_cfg_info *pads_cfg;
+	unsigned int num_pads_cfg;
+};
+
+static int tegra_iop_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+
+	return tiopi->num_pads_cfg;
+}
+
+static const char *tegra_iop_pinctrl_get_group_name(struct pinctrl_dev *pctldev,
+						    unsigned int group)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+
+	return tiopi->pads_cfg[group].name;
+}
+
+static int tegra_iop_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+					    unsigned int group,
+					    const unsigned int **pins,
+					    unsigned int *num_pins)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = tiopi->pads_cfg[group].pins;
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const struct pinctrl_ops tegra_iop_pinctrl_ops = {
+	.get_groups_count = tegra_iop_pinctrl_get_groups_count,
+	.get_group_name = tegra_iop_pinctrl_get_group_name,
+	.get_group_pins = tegra_iop_pinctrl_get_group_pins,
+	.dt_node_to_map = pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map = pinctrl_utils_dt_free_map,
+};
+
+static int tegra_io_pad_pinconf_get(struct pinctrl_dev *pctldev,
+				    unsigned int pin, unsigned long *config)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+	int param = pinconf_to_config_param(*config);
+	struct tegra_io_pads_cfg_info *pad_cfg = &tiopi->pads_cfg[pin];
+	int io_rail_id = pad_cfg->io_rail_id;
+	int arg = 0;
+	int ret;
+
+	switch (param) {
+	case TEGRA_IO_RAIL_VOLTAGE:
+		ret = tegra_io_rail_voltage_get(io_rail_id);
+		if (ret < 0)
+			return ret;
+		arg = ret;
+		break;
+
+	case TEGRA_IO_PAD_DEEP_POWER_DOWN:
+		ret = tegra_io_rail_power_get_status(io_rail_id);
+		if (ret < 0)
+			return ret;
+		arg = !ret;
+		break;
+
+	default:
+		dev_err(tiopi->dev, "The parameter %d not supported\n", param);
+		return -EINVAL;
+	}
+
+	*config = pinconf_to_config_packed(param, (u16)arg);
+	return 0;
+}
+
+static int tegra_io_pad_pinconf_set(struct pinctrl_dev *pctldev,
+				    unsigned int pin, unsigned long *configs,
+				    unsigned int num_configs)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+	struct tegra_io_pads_cfg_info *pad_cfg = &tiopi->pads_cfg[pin];
+	int io_rail_id = pad_cfg->io_rail_id;
+	int param;
+	u16 param_val;
+	int ret;
+	int i;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		param_val = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case TEGRA_IO_RAIL_VOLTAGE:
+			ret = tegra_io_rail_voltage_set(io_rail_id, param_val);
+			if (ret < 0) {
+				dev_err(tiopi->dev,
+					"Failed to set voltage %d of pin %u: %d\n",
+					param_val, pin, ret);
+				return ret;
+			}
+			break;
+
+		case TEGRA_IO_PAD_DEEP_POWER_DOWN:
+			if (param_val)
+				ret = tegra_io_rail_power_off(io_rail_id);
+			else
+				ret = tegra_io_rail_power_on(io_rail_id);
+			if (ret < 0) {
+				dev_err(tiopi->dev,
+					"Failed to set DPD %d of pin %u: %d\n",
+					param_val, pin, ret);
+				return ret;
+			}
+			break;
+
+		default:
+			dev_err(tiopi->dev, "The parameter %d not supported\n",
+				param);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops tegra_io_pad_pinconf_ops = {
+	.pin_config_get = tegra_io_pad_pinconf_get,
+	.pin_config_set = tegra_io_pad_pinconf_set,
+};
+
+static struct pinctrl_desc tegra_iop_pinctrl_desc = {
+	.name = "pinctrl-tegra-io-pads",
+	.pctlops = &tegra_iop_pinctrl_ops,
+	.confops = &tegra_io_pad_pinconf_ops,
+	.pins = tegra210_io_pads_pinctrl_desc,
+	.npins = ARRAY_SIZE(tegra210_io_pads_pinctrl_desc),
+	.custom_params = tegra_io_pads_cfg_params,
+	.num_custom_params = ARRAY_SIZE(tegra_io_pads_cfg_params),
+};
+
+static int tegra_iop_pinctrl_probe(struct platform_device *pdev)
+{
+	struct tegra_io_pads_info *tiopi;
+	struct device *dev = &pdev->dev;
+
+	tiopi = devm_kzalloc(&pdev->dev, sizeof(*tiopi), GFP_KERNEL);
+	if (!tiopi)
+		return -ENOMEM;
+
+	tiopi->dev = &pdev->dev;
+	tiopi->dev->of_node = pdev->dev.of_node;
+	tiopi->pads_cfg = tegra210_io_pads_cfg_info;
+	tiopi->num_pads_cfg = ARRAY_SIZE(tegra210_io_pads_cfg_info);
+
+	platform_set_drvdata(pdev, tiopi);
+
+	tiopi->pctl = pinctrl_register(&tegra_iop_pinctrl_desc, dev, tiopi);
+	if (IS_ERR(tiopi->pctl)) {
+		dev_err(&pdev->dev, "Couldn't register pinctrl driver\n");
+		return PTR_ERR(tiopi->pctl);
+	}
+
+	return 0;
+}
+
+static int tegra_iop_pinctrl_remove(struct platform_device *pdev)
+{
+	struct tegra_io_pads_info *tiopi = platform_get_drvdata(pdev);
+
+	pinctrl_unregister(tiopi->pctl);
+
+	return 0;
+}
+
+static const struct of_device_id tegra_io_pads_of_match[] = {
+	{ .compatible = "nvidia,tegra210-io-pad", },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, tegra_iop_pinctrl_devtype);
+
+static struct platform_driver tegra_iop_pinctrl_driver = {
+	.driver = {
+		.name = "pinctrl-tegra-io-pad",
+		.of_match_table = tegra_io_pads_of_match,
+	},
+	.probe = tegra_iop_pinctrl_probe,
+	.remove = tegra_iop_pinctrl_remove,
+};
+
+module_platform_driver(tegra_iop_pinctrl_driver);
+
+MODULE_DESCRIPTION("NVIDIA TEGRA IO pad Control Driver");
+MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
+MODULE_ALIAS("platform:pinctrl-tegra-io-pads");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Linus Walleij]

On Tue, Apr 12, 2016 at 4:56 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
> or 3.3V I/O voltage levels. Also the IO pads can be configured
> for power down state if it is not used. SW needs to configure the
> voltage level of IO pads based on IO rail voltage and its power
> state based on platform usage.
>
> The voltage and power state configurations of pads are provided
> through pin control frameworks. Add pin control driver for Tegra's
> IO pads' voltage and power state configurations.
>
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

(...)
> +config PINCTRL_TEGRA210_IO_PAD

Why does this need its own Kconfig option?
Can't you just unconditionally compile it in if
PINCTRL_TEGRA210 is selected, you seem to say
it is there on all these platforms anyway.

> +static const struct pinconf_generic_params tegra_io_pads_cfg_params[] = {
> +       {
> +               .property = "nvidia,io-rail-voltage",
> +               .param = TEGRA_IO_RAIL_VOLTAGE,
> +       }, {

What's so nvidia-specific about this?
We have power-source in
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
which takes a custom argument. This is obviously what you
are doing (selecting one of two rails), so use that binding.

> +               .property = "nvidia,io-pad-deep-power-down",
> +               .param = TEGRA_IO_PAD_DEEP_POWER_DOWN,
> +       },

Likewise the generic bindings have low-power-enable and
low-power-disable, this seems like a copy of low-power-enable;
Even if it needs a new binding, it doesn't seem very nVidia-specific
so then propose something to the generic bindings.

Even if Tegra is not using the generic code for handling the
standard bindings (GENERIC_PINCONF) it doesn't stop
you from using the generic bindings and contributing to them.

Historically you have a few custom bindings like these:

nvidia,pins
nvidia,function
nvidia,pull
nvidia,tristate

etc etc, but that is just unfortunate and due to preceding the
generic bindings. I would appreciate if you started to support
the generic bindings in parallel, but I'm not gonna push that issue.
However for *new* stuff, I don't want the custom bindings
to proliferate. Use the generic stuff, I'm trying to keep the weirdness
in one place, and the generic stuff is needed for standardization
across platforms going forward.

Yours,
Linus Walleij

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Laxman Dewangan]

On Friday 15 April 2016 01:38 PM, Linus Walleij wrote:
> On Tue, Apr 12, 2016 at 4:56 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>
>> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
>> or 3.3V I/O voltage levels. Also the IO pads can be configured
>> for power down state if it is not used. SW needs to configure the
>> voltage level of IO pads based on IO rail voltage and its power
>> state based on platform usage.
>>
>> The voltage and power state configurations of pads are provided
>> through pin control frameworks. Add pin control driver for Tegra's
>> IO pads' voltage and power state configurations.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> (...)
>> +config PINCTRL_TEGRA210_IO_PAD
> Why does this need its own Kconfig option?
> Can't you just unconditionally compile it in if
> PINCTRL_TEGRA210 is selected, you seem to say
> it is there on all these platforms anyway.

Yes, it can be done. The reason I kept is that this driver needed T210 
onwards and not for older generation of SoC.

May be we can select from T210 pincontrol.


>
>> +static const struct pinconf_generic_params tegra_io_pads_cfg_params[] = {
>> +       {
>> +               .property = "nvidia,io-rail-voltage",
>> +               .param = TEGRA_IO_RAIL_VOLTAGE,
>> +       }, {
> What's so nvidia-specific about this?
> We have power-source in
> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> which takes a custom argument. This is obviously what you
> are doing (selecting one of two rails), so use that binding.

Yes, I looked for the common property but did not found anything near to 
this.
My understating for power-source is that selecting the source of supply, 
not the voltages.
I am looking something  power-source-voltage-level.
Should we add this?




>
>> +               .property = "nvidia,io-pad-deep-power-down",
>> +               .param = TEGRA_IO_PAD_DEEP_POWER_DOWN,
>> +       },
> Likewise the generic bindings have low-power-enable and
> low-power-disable, this seems like a copy of low-power-enable;
When writing, I considered this property but was not able to fully 
convinced myself to use this but I think now I am fine to use this as 
you suggested.



>
> Even if Tegra is not using the generic code for handling the
> standard bindings (GENERIC_PINCONF) it doesn't stop
> you from using the generic bindings and contributing to them.
>
> Historically you have a few custom bindings like these:
>
> nvidia,pins
> nvidia,function
> nvidia,pull
> nvidia,tristate
>
> etc etc, but that is just unfortunate and due to preceding the
> generic bindings. I would appreciate if you started to support
> the generic bindings in parallel, but I'm not gonna push that issue.

Yaah, these are in my plate to cleanup. Let me work with Stephen, what 
he think here.

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Linus Walleij]

On Fri, Apr 15, 2016 at 10:39 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Friday 15 April 2016 01:38 PM, Linus Walleij wrote:
>> On Tue, Apr 12, 2016 at 4:56 PM, Laxman Dewangan <ldewangan@nvidia.com>
>> wrote:

>>> +static const struct pinconf_generic_params tegra_io_pads_cfg_params[] =
>>> {
>>> +       {
>>> +               .property = "nvidia,io-rail-voltage",
>>> +               .param = TEGRA_IO_RAIL_VOLTAGE,
>>> +       }, {
>>
>> What's so nvidia-specific about this?
>> We have power-source in
>> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>> which takes a custom argument. This is obviously what you
>> are doing (selecting one of two rails), so use that binding.
>
> Yes, I looked for the common property but did not found anything near to
> this.
> My understating for power-source is that selecting the source of supply, not
> the voltages.

Well in a comment to the previous patch you just said that the
hardware actually does not regulate voltages. Isn't the actual case
that there are two rails with two different voltages, and you select one
of the rails for the pin?

That is not really selecting a voltage, that is selecting a power
rail.

> I am looking something  power-source-voltage-level.
> Should we add this?

If the pin could actually set a voltage level it would have a regulator.
I don't believe that. I think it is selecting one of two rails which
could theoretically hold two totally different voltages.

And that is what power-source is about.

>>> +               .property = "nvidia,io-pad-deep-power-down",
>>> +               .param = TEGRA_IO_PAD_DEEP_POWER_DOWN,
>>> +       },
>>
>> Likewise the generic bindings have low-power-enable and
>> low-power-disable, this seems like a copy of low-power-enable;
>
> When writing, I considered this property but was not able to fully convinced
> myself to use this but I think now I am fine to use this as you suggested.

Thanks.

>> Even if Tegra is not using the generic code for handling the
>> standard bindings (GENERIC_PINCONF) it doesn't stop
>> you from using the generic bindings and contributing to them.
>>
>> Historically you have a few custom bindings like these:
>>
>> nvidia,pins
>> nvidia,function
>> nvidia,pull
>> nvidia,tristate
>>
>> etc etc, but that is just unfortunate and due to preceding the
>> generic bindings. I would appreciate if you started to support
>> the generic bindings in parallel, but I'm not gonna push that issue.
>
> Yaah, these are in my plate to cleanup. Let me work with Stephen, what he
> think here.

Much appreciated, thanks!

Yours,
Linus Walleij

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Laxman Dewangan]

On Friday 15 April 2016 02:55 PM, Linus Walleij wrote:
> On Fri, Apr 15, 2016 at 10:39 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> On Friday 15 April 2016 01:38 PM, Linus Walleij wrote:
>>> On Tue, Apr 12, 2016 at 4:56 PM, Laxman Dewangan <ldewangan@nvidia.com>
>>> wrote:
>>>> +static const struct pinconf_generic_params tegra_io_pads_cfg_params[] =
>>>> {
>>>> +       {
>>>> +               .property = "nvidia,io-rail-voltage",
>>>> +               .param = TEGRA_IO_RAIL_VOLTAGE,
>>>> +       }, {
>>> What's so nvidia-specific about this?
>>> We have power-source in
>>> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
>>> which takes a custom argument. This is obviously what you
>>> are doing (selecting one of two rails), so use that binding.
>> Yes, I looked for the common property but did not found anything near to
>> this.
>> My understating for power-source is that selecting the source of supply, not
>> the voltages.
> Well in a comment to the previous patch you just said that the
> hardware actually does not regulate voltages. Isn't the actual case
> that there are two rails with two different voltages, and you select one
> of the rails for the pin?
>
> That is not really selecting a voltage, that is selecting a power
> rail.
>
>> I am looking something  power-source-voltage-level.
>> Should we add this?
> If the pin could actually set a voltage level it would have a regulator.
> I don't believe that. I think it is selecting one of two rails which
> could theoretically hold two totally different voltages.
>
> And that is what power-source is about.
>
>


The IO rails connected to PMIC rail and connection does not get change.
We change the voltage of PMIC rails via regulator calls. And then 
configure pads for the new voltage.
We really do not witch the supply connection here. power-source is about 
the power supply connection, not about the voltage change on same supply.

Like in MMC driver for SDIO3.0:

regulator_set_voltage(sdmmc_rail, 3300000, 3300000);
tegra_io_pad_voltage_configure(SDMMC, 3300000)


For 1.8V

regulator_set_voltage(sdmmc_rail, 1800000, 1800000);
tegra_io_pad_voltage_configure(SDMMC, 1800000)

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Linus Walleij]

On Fri, Apr 15, 2016 at 11:55 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Friday 15 April 2016 02:55 PM, Linus Walleij wrote:

>> If the pin could actually set a voltage level it would have a regulator.
>> I don't believe that. I think it is selecting one of two rails which
>> could theoretically hold two totally different voltages.
>>
>> And that is what power-source is about.
>
> The IO rails connected to PMIC rail and connection does not get change.
> We change the voltage of PMIC rails via regulator calls. And then configure
> pads for the new voltage.

Aha I get it! So you adjust something in the I/O-cell so that it is adapted
for the new voltage.

OK that seems to be something new. I suspect
power-voltage-select = <n>; where N i in uV would solve this?
(We should use uV since regulators use this.)

But to be sure we would like to know what is actually happening,
electronically speaking, when you set this up. Do you have any
idea?

Yours,
Linus Walleij

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Laxman Dewangan]

On Friday 15 April 2016 04:45 PM, Linus Walleij wrote:
> On Fri, Apr 15, 2016 at 11:55 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> On Friday 15 April 2016 02:55 PM, Linus Walleij wrote:
>>> If the pin could actually set a voltage level it would have a regulator.
>>> I don't believe that. I think it is selecting one of two rails which
>>> could theoretically hold two totally different voltages.
>>>
>>> And that is what power-source is about.
>> The IO rails connected to PMIC rail and connection does not get change.
>> We change the voltage of PMIC rails via regulator calls. And then configure
>> pads for the new voltage.
> Aha I get it! So you adjust something in the I/O-cell so that it is adapted
> for the new voltage.
>
> OK that seems to be something new. I suspect
> power-voltage-select = <n>; where N i in uV would solve this?
> (We should use uV since regulators use this.)

Thanks for new property. I will make the unit and type same as the 
regulator framework.



>
> But to be sure we would like to know what is actually happening,
> electronically speaking, when you set this up. Do you have any
> idea?
>


 From electronic point of view, the value of VIL, VIH, VOL, VOH 
(Input/output voltage level for low and high state) are different when 
talking for 0 t 1.8V and 0 to 3.3V.

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Linus Walleij]

On Fri, Apr 15, 2016 at 1:47 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
> On Friday 15 April 2016 04:45 PM, Linus Walleij wrote:
>> On Fri, Apr 15, 2016 at 11:55 AM, Laxman Dewangan <ldewangan@nvidia.com>
>> wrote:

>> But to be sure we would like to know what is actually happening,
>> electronically speaking, when you set this up. Do you have any
>> idea?
>
> From electronic point of view, the value of VIL, VIH, VOL, VOH (Input/output
> voltage level for low and high state) are different when talking for 0 t
> 1.8V and 0 to 3.3V.

Yeah that I get. But since it is switched on a per-pin basis, and
this is not about what voltage is actually supplied to the I/O cell,
because that comes from the outside, it is a mystery why it is
even needed.

I understand that there is a bit selecting driving voltage level in
the register range, what I don't understand is what that is
doing in the I/O cell.

The bit in the register must be routed to somehing in the I/O cell
and I would like to know what. I take it that an ordinary CMOS
totem-pole push-pull output is going to work the same with 1.8
and 3.3V alike so it's obviously not enabling any extra transistors
or anything.

Yours,
Linus Walleij

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Laxman Dewangan]

On Friday 15 April 2016 07:33 PM, Linus Walleij wrote:
> On Fri, Apr 15, 2016 at 1:47 PM, Laxman Dewangan <ldewangan@nvidia.com> wrote:
>> On Friday 15 April 2016 04:45 PM, Linus Walleij wrote:
>>> On Fri, Apr 15, 2016 at 11:55 AM, Laxman Dewangan <ldewangan@nvidia.com>
>>> wrote:
>>> But to be sure we would like to know what is actually happening,
>>> electronically speaking, when you set this up. Do you have any
>>> idea?
>>  From electronic point of view, the value of VIL, VIH, VOL, VOH (Input/output
>> voltage level for low and high state) are different when talking for 0 t
>> 1.8V and 0 to 3.3V.
> Yeah that I get. But since it is switched on a per-pin basis, and
> this is not about what voltage is actually supplied to the I/O cell,
> because that comes from the outside, it is a mystery why it is
> even needed.
>
> I understand that there is a bit selecting driving voltage level in
> the register range, what I don't understand is what that is
> doing in the I/O cell.
>
> The bit in the register must be routed to somehing in the I/O cell
> and I would like to know what. I take it that an ordinary CMOS
> totem-pole push-pull output is going to work the same with 1.8
> and 3.3V alike so it's obviously not enabling any extra transistors
> or anything.
>
>
I dont have answer for this now and I need to discuss with HW team to 
get this info.

I will be back here after discussion with HW team.

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Laxman Dewangan]

Hi Linus,

On Friday 15 April 2016 07:29 PM, Laxman Dewangan wrote:
>
> On Friday 15 April 2016 07:33 PM, Linus Walleij wrote:
>> On Fri, Apr 15, 2016 at 1:47 PM, Laxman Dewangan 
>> <ldewangan@nvidia.com> wrote:
>>> On Friday 15 April 2016 04:45 PM, Linus Walleij wrote:
>>>> On Fri, Apr 15, 2016 at 11:55 AM, Laxman Dewangan 
>>>> <ldewangan@nvidia.com>
>>>> wrote:
>>>> But to be sure we would like to know what is actually happening,
>>>> electronically speaking, when you set this up. Do you have any
>>>> idea?
>>>  From electronic point of view, the value of VIL, VIH, VOL, VOH 
>>> (Input/output
>>> voltage level for low and high state) are different when talking for 
>>> 0 t
>>> 1.8V and 0 to 3.3V.
>> Yeah that I get. But since it is switched on a per-pin basis, and
>> this is not about what voltage is actually supplied to the I/O cell,
>> because that comes from the outside, it is a mystery why it is
>> even needed.
>>
>> I understand that there is a bit selecting driving voltage level in
>> the register range, what I don't understand is what that is
>> doing in the I/O cell.
>>
>> The bit in the register must be routed to somehing in the I/O cell
>> and I would like to know what. I take it that an ordinary CMOS
>> totem-pole push-pull output is going to work the same with 1.8
>> and 3.3V alike so it's obviously not enabling any extra transistors
>> or anything.
>>
>>
> I dont have answer for this now and I need to discuss with HW team to 
> get this info.
>
> I will be back here after discussion with HW team.
>
I had discussion with HW team to get this info from analog point of view.

The IO circuitry has to be configured correctly to engage the right 
level shifting circuits between the IO rail and the core voltage rail in 
each direction; and that is the main purpose of this configuration. You 
are correct, the output drivers will naturally drive towards the rails, 
whatever their voltage may be; and the input receiver will likewise 
reference itself naturally to the rail, although the switching threshold 
of the receiver transistors may sometimes need configuration too.

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Laxman Dewangan]

On Friday 15 April 2016 05:17 PM, Laxman Dewangan wrote:
>
> On Friday 15 April 2016 04:45 PM, Linus Walleij wrote:
>> On Fri, Apr 15, 2016 at 11:55 AM, Laxman Dewangan 
>> <ldewangan@nvidia.com> wrote:
>>> On Friday 15 April 2016 02:55 PM, Linus Walleij wrote:
>>>> If the pin could actually set a voltage level it would have a 
>>>> regulator.
>>>> I don't believe that. I think it is selecting one of two rails which
>>>> could theoretically hold two totally different voltages.
>>>>
>>>> And that is what power-source is about.
>>> The IO rails connected to PMIC rail and connection does not get change.
>>> We change the voltage of PMIC rails via regulator calls. And then 
>>> configure
>>> pads for the new voltage.
>> Aha I get it! So you adjust something in the I/O-cell so that it is 
>> adapted
>> for the new voltage.
>>
>> OK that seems to be something new. I suspect
>> power-voltage-select = <n>; where N i in uV would solve this?
>> (We should use uV since regulators use this.)
>
> Thanks for new property. I will make the unit and type same as the 
> regulator framework.

We have the ops for configuring the pin config as

         int (*pin_config_group_set) (struct pinctrl_dev *pctldev,
                                      unsigned selector,
                                      unsigned long *configs,
                                      unsigned num_configs);


The config is 32 bit, upper 16 for config argument and  and lower 16 for 
the config param.

So we can not accommodate 3300000uV until we change it to mV i.e. 3300mV.

So on interface, we can read uV from DT but when making config, we can 
translate it to mV before passing to pin_config_group_set.

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Stephen Warren]

On 04/26/2016 07:32 AM, Laxman Dewangan wrote:
>
> On Friday 15 April 2016 05:17 PM, Laxman Dewangan wrote:
>>
>> On Friday 15 April 2016 04:45 PM, Linus Walleij wrote:
>>> On Fri, Apr 15, 2016 at 11:55 AM, Laxman Dewangan
>>> <ldewangan@nvidia.com> wrote:
>>>> On Friday 15 April 2016 02:55 PM, Linus Walleij wrote:
>>>>> If the pin could actually set a voltage level it would have a
>>>>> regulator.
>>>>> I don't believe that. I think it is selecting one of two rails which
>>>>> could theoretically hold two totally different voltages.
>>>>>
>>>>> And that is what power-source is about.
>>>> The IO rails connected to PMIC rail and connection does not get change.
>>>> We change the voltage of PMIC rails via regulator calls. And then
>>>> configure
>>>> pads for the new voltage.
>>> Aha I get it! So you adjust something in the I/O-cell so that it is
>>> adapted
>>> for the new voltage.
>>>
>>> OK that seems to be something new. I suspect
>>> power-voltage-select = <n>; where N i in uV would solve this?
>>> (We should use uV since regulators use this.)
>>
>> Thanks for new property. I will make the unit and type same as the
>> regulator framework.
>
> We have the ops for configuring the pin config as
>
>          int (*pin_config_group_set) (struct pinctrl_dev *pctldev,
>                                       unsigned selector,
>                                       unsigned long *configs,
>                                       unsigned num_configs);
>
>
> The config is 32 bit, upper 16 for config argument and  and lower 16 for
> the config param.
>
> So we can not accommodate 3300000uV until we change it to mV i.e. 3300mV.
>
> So on interface, we can read uV from DT but when making config, we can
> translate it to mV before passing to pin_config_group_set.

A SoC-specific enum would make more sense. It would avoid any 
translation. There's no need for a generic value since the available set 
of options is SoC-specific, the data source is SoC-specific, and there's 
no need for any code besides the SoC-specific driver to interpret the 
value in any way at all.

Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Stephen Warren]

On 04/15/2016 02:39 AM, Laxman Dewangan wrote:
>
> On Friday 15 April 2016 01:38 PM, Linus Walleij wrote:
>> On Tue, Apr 12, 2016 at 4:56 PM, Laxman Dewangan
>> <ldewangan@nvidia.com> wrote:
>>
>>> NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
>>> or 3.3V I/O voltage levels. Also the IO pads can be configured
>>> for power down state if it is not used. SW needs to configure the
>>> voltage level of IO pads based on IO rail voltage and its power
>>> state based on platform usage.
>>>
>>> The voltage and power state configurations of pads are provided
>>> through pin control frameworks. Add pin control driver for Tegra's
>>> IO pads' voltage and power state configurations.

>> Even if Tegra is not using the generic code for handling the
>> standard bindings (GENERIC_PINCONF) it doesn't stop
>> you from using the generic bindings and contributing to them.
>>
>> Historically you have a few custom bindings like these:
>>
>> nvidia,pins
>> nvidia,function
>> nvidia,pull
>> nvidia,tristate
>>
>> etc etc, but that is just unfortunate and due to preceding the
>> generic bindings. I would appreciate if you started to support
>> the generic bindings in parallel, but I'm not gonna push that issue.
>
> Yaah, these are in my plate to cleanup. Let me work with Stephen, what
> he think here.

For existing chips, we must always support the existing bindings. 
There's no point moving to the new bindings since it'll just add more 
code that just does the same thing.

For new chips perhaps it makes sense to move to the new standardized 
properties. That said, I don't expect we'll need pinmux on those chips 
since it's guaranteed that FW will set up the static pinmux, and HW 
explicitly doesn't support dynamic pinmuxing?