| Message ID | 1460473007-11535-5-git-send-email-ldewangan@nvidia.com |
|---|---|
| State | New |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. :-) -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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()?? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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"? -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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__ */
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(+)