Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1376895 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails Laxman Dewangan <ldewangan@nvidia.com> - 2016-04-12 17:20 +0200
    Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Jon Hunter <jonathanh@nvidia.com> - 2016-04-13 10:50 +0200
      Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Laxman Dewangan <ldewangan@nvidia.com> - 2016-04-13 11:20 +0200
        Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Jon Hunter <jonathanh@nvidia.com> - 2016-04-13 11:30 +0200
          Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Laxman Dewangan <ldewangan@nvidia.com> - 2016-04-13 11:40 +0200
            Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Jon Hunter <jonathanh@nvidia.com> - 2016-04-13 12:00 +0200
    Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails Linus Walleij <linus.walleij@linaro.org> - 2016-04-15 10:00 +0200
      Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Mark Brown <broonie@kernel.org> - 2016-04-15 10:10 +0200
        Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Laxman Dewangan <ldewangan@nvidia.com> - 2016-04-15 10:40 +0200
          Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO rails Linus Walleij <linus.walleij@linaro.org> - 2016-04-15 11:30 +0200
    Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Stephen Warren <swarren@wwwdotorg.org> - 2016-04-15 18:30 +0200
      Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Laxman Dewangan <ldewangan@nvidia.com> - 2016-04-15 18:40 +0200
        Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Laxman Dewangan <ldewangan@nvidia.com> - 2016-04-15 18:50 +0200
          Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Stephen Warren <swarren@wwwdotorg.org> - 2016-04-15 19:00 +0200
        Re: [PATCH 4/7] soc/tegra: pmc: Add interface to set voltage of IO  rails Stephen Warren <swarren@wwwdotorg.org> - 2016-04-15 18:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

#1379523

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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()??

[toc] | [prev] | [next] | [standalone]

#1379627

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

[toc] | [prev] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web