Re: [PATCH V4 3/3] soc/tegra: pmc: Add support for IO pads power state and voltage

[Jon Hunter]

On 06/05/16 11:45, Laxman Dewangan wrote:
> The IO pins of Tegra SoCs are grouped for common control of IO
> interface like setting voltage signal levels and power state of
> the interface. The group is generally referred as IO pads. The
> power state and voltage control of IO pins can be done at IO pads
> level.
> 
> Tegra generation SoC supports the power down of IO pads when it
> is not used even in the active state of system. This saves power
> from that IO interface. Also it supports multiple voltage level
> in IO pins for interfacing on some of pads. The IO pad voltage is
> automatically detected till T124, hence SW need not to configure
> this. But from T210, the automatically detection logic has been
> removed, hence SW need to explicitily set the IO pad voltage into
> IO pad configuration registers.
> 
> Add support to set the power states and voltage level of the IO pads
> from client driver. The implementation for the APIs are in generic
> which is applicable for all generation os Tegra SoC.
> 
> IO pads ID and information of bit field for power state and voltage
> level controls are added for Tegra124, Tegra132 and Tegra210. The SOR
> driver is modified to use the new APIs.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@xxxxxxxxxx>
> 
> ---
> Changes from V1:
> This is reworked on earlier path to have separation between IO rails and
> io pads and add power state and voltage control APIs in single call.
> 
> Changes from V2:
> - Remove the tegra_io_rail_power_off/on() apis and change client (sor) driver
> to use the new APIs for IO pad power.
> - Remove the TEGRA_IO_RAIL_ macros.
> 
> Changes from V3:
> - Make all pad_id/io_pad_id to id.
> - tegra_io_pad_ -> tegra_io_pads
> - dpd_bit -> bit, pwr_mask/bit to mask/bit.
> - Rename function to tegra_io_pads_{set,get}_voltage_config
> - Make the io pad tables common for all SoC.
> - Make io_pads enums.
> - Add enums for voltage.
> ---
>  drivers/gpu/drm/tegra/sor.c |   8 +-
>  drivers/soc/tegra/pmc.c     | 221 ++++++++++++++++++++++++++++++++++++++------
>  include/soc/tegra/pmc.h     | 132 ++++++++++++++++++--------
>  3 files changed, 294 insertions(+), 67 deletions(-)

[...]

> +/* TEGRA_IO_PAD: The IO pins of Tegra SoCs are grouped for common control
> + * of IO interface like setting voltage signal levels, power state of the
> + * interface. The group is generally referred as io-pads. The power and
> + * voltage control of IO pins are available at io-pads level.
> + * The following macros make the super list all IO pads found on Tegra SoC
> + * generations.
> + */
> +enum tegra_io_pads {
> +	TEGRA_IO_PAD_AUDIO,
> +	TEGRA_IO_PAD_AUDIO_HV,
> +	TEGRA_IO_PAD_BB,
> +	TEGRA_IO_PAD_CAM,
> +	TEGRA_IO_PAD_COMP,
> +	TEGRA_IO_PAD_CSIA,
> +	TEGRA_IO_PAD_CSIB,
> +	TEGRA_IO_PAD_CSIC,
> +	TEGRA_IO_PAD_CSID,
> +	TEGRA_IO_PAD_CSIE,
> +	TEGRA_IO_PAD_CSIF,
> +	TEGRA_IO_PAD_DBG,
> +	TEGRA_IO_PAD_DEBUG_NONAO,
> +	TEGRA_IO_PAD_DMIC,
> +	TEGRA_IO_PAD_DP,
> +	TEGRA_IO_PAD_DSI,
> +	TEGRA_IO_PAD_DSIB,
> +	TEGRA_IO_PAD_DSIC,
> +	TEGRA_IO_PAD_DSID,
> +	TEGRA_IO_PAD_EMMC,
> +	TEGRA_IO_PAD_EMMC2,
> +	TEGRA_IO_PAD_GPIO,
> +	TEGRA_IO_PAD_HDMI,
> +	TEGRA_IO_PAD_HSIC,
> +	TEGRA_IO_PAD_HV,
> +	TEGRA_IO_PAD_LVDS,
> +	TEGRA_IO_PAD_MIPI_BIAS,
> +	TEGRA_IO_PAD_NAND,
> +	TEGRA_IO_PAD_PEX_BIAS,
> +	TEGRA_IO_PAD_PEX_CLK1,
> +	TEGRA_IO_PAD_PEX_CLK2,
> +	TEGRA_IO_PAD_PEX_CNTRL,
> +	TEGRA_IO_PAD_SDMMC1,
> +	TEGRA_IO_PAD_SDMMC3,
> +	TEGRA_IO_PAD_SDMMC4,
> +	TEGRA_IO_PAD_SPI,
> +	TEGRA_IO_PAD_SPI_HV,
> +	TEGRA_IO_PAD_SYS_DDC,
> +	TEGRA_IO_PAD_UART,
> +	TEGRA_IO_PAD_USB0,
> +	TEGRA_IO_PAD_USB1,
> +	TEGRA_IO_PAD_USB2,
> +	TEGRA_IO_PAD_USB3,
> +	TEGRA_IO_PAD_USB_BIAS,
> +
> +	/* Last entry */
> +	TEGRA_IO_PAD_MAX,

Nit should these be TEGRA_IO_PADS_xxx?

> +};
> +
> +/* tegra_io_pads_source_voltage: The voltage level of IO rails which source
> + *				 the IO pads.
> + */
> +enum tegra_io_pads_source_voltage {
> +	TEGRA_IO_PADS_SOURCE_VOLTAGE_1800000UV,
> +	TEGRA_IO_PADS_SOURCE_VOLTAGE_3300000UV,
> +};

Nit I wonder if we can make this shorter ...

enum tegra_io_pads_vconf {
	TEGRA_IO_PADS_VCONF_1V8,
	TEGRA_IO_PADS_VCONF_3V3,


> -int tegra_io_rail_power_on(unsigned int id);
> -int tegra_io_rail_power_off(unsigned int id);
> +/* Power enable/disable of the IO pads */
> +int tegra_io_pads_power_enable(enum tegra_io_pads id);
> +int tegra_io_pads_power_disable(enum tegra_io_pads id);
> +int tegra_io_pads_power_is_enabled(enum tegra_io_pads id);
> +
> +/* Set/get Tegra IO pads voltage config registers */
> +int tegra_io_pads_set_voltage_config(enum tegra_io_pads id,
> +				     enum tegra_io_pads_source_voltage rail_uv);
> +int tegra_io_pads_get_voltage_config(enum tegra_io_pads id);

Ideally, for public function we should have kernel-doc descriptions.

Otherwise looks fine. I would not rev this unless Thierry has some comments.

Cheers
Jon