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.