Message-ID: <20161125091354.GA11512@ulmo.ba.sec>
Date:   Fri, 25 Nov 2016 10:13:54 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Laxman Dewangan <ldewangan@...dia.com>
Cc:     linus.walleij@...aro.org, robh+dt@...nel.org, mark.rutland@....com,
        swarren@...dotorg.org, gnurou@...il.com, jonathanh@...dia.com,
        joe@...ches.com, yamada.masahiro@...ionext.com,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4 1/2] pinctrl: tegra: Add DT binding for io pads control

On Thu, Nov 24, 2016 at 02:08:53PM +0530, Laxman Dewangan wrote:
> NVIDIA Tegra124 and later SoCs support the multi-voltage level and
> low power state of some of its IO pads. The IO pads can work in
> the voltage of the 1.8V and 3.3V of IO voltage from IO power rail
> sources. When IO interfaces are not used then IO pads can be
> configure in low power state to reduce the power consumption from
> that IO pads.
> 
> On Tegra124, the voltage level of IO power rail source is auto
> detected by hardware(SoC) and hence it is only require to configure
> in low power mode if IO pads are not used.
> 
> On T210 onwards, the auto-detection of voltage level from IO power

You say Tegra124 above but T210 here. Please use consistent spelling to
make it more obvious that we're talking about chip generations here.
Tegra<gen> is preferred over T<gen>.

> rail is removed from SoC and hence SW need to configure the PMC
> register explicitly to set proper voltage in IO pads based on
> IO rail power source voltage.
> 
> Add DT binding document for detailing the DT properties for
> configuring IO pads voltage levels and its power state.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@...dia.com>
> 
> ---
> Changes from V1:
>  New in series based on pinctrl driver requirement.
> 
> Changes from V2:
>  Updated the statement to say 1.8V and 3.3V as nominal voltage.
>  Corrected DT example by adding -supply and taken care of V1 review
>  from Rob.
> 
>  .../bindings/pinctrl/nvidia,tegra-io-pad.txt       | 126 +++++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> new file mode 100644
> index 0000000..a88c484
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> @@ -0,0 +1,126 @@
> +NVIDIA Tegra PMC IO pad controller
> +
> +NVIDIA Tegra124 and later SoCs support the multi-voltage level and low power
> +state of some of its IO pads. When IO interface are not used then IO pads can
> +be configure in low power state to reduce the power from that IO pads. The IO
> +pads can work in the nominal IO voltage of 1.8V and 3.3V from power rail
> +sources.
> +
> +On Tegra124, the voltage of IO power rail source is auto detected by SoC and
> +hence it is only require to configure in low power mode if IO pads are not
> +used.
> +
> +On T210 onwards, the HW based auto-detection for IO voltage is removed and
> +hence SW need to configure the PMC register explicitly, to set proper voltage
> +in IO pads, based on IO rail power source voltage.
> +
> +The voltage configurations and low power state of IO pads should be done in
> +boot if it is not going to change otherwise dynamically based on IO rail
> +voltage on that IO pads and usage of IO pads

Missing fullstop at the end.

> +
> +The DT property of the IO pads must be under the node of pmc i.e.
> +pmc@...0e400 for Tegra124 onwards.

The PMC is at a different address on Tegra186, so I think we should just
drop this to avoid having to update it whenever a new chip relocates it
to a different address.

Come to think of it, if these I/O pads are represented as subnodes in
the PMC device tree node, perhaps this should be merged into the PMC
documentation?

> +Please refer to <pinctrl-bindings.txt> in this directory for details of the
> +common pinctrl bindings used by client devices, including the meaning of the
> +phrase "pin configuration node".
> +
> +Tegra's pin configuration nodes act as a container for an arbitrary number of
> +subnodes. Each of these subnodes represents some desired configuration for an
> +IO pads, or a list of IO pads. This configuration can include the voltage and
> +power enable/disable control

Missing fullstop.

> +
> +The name of each subnode is not important; all subnodes should be enumerated
> +and processed purely based on their content. Each subnode only affects those
> +parameters that are explicitly listed. Unspecified is represented as an absent
> +property,

Should be fullstop instead of comma at the end of the sentence. Also I'm
not sure the last sentence is even necessary. The previous one already
says that only explicitly listed parameters take effect.

> +
> +See the TRM to determine which properties and values apply to each IO pads.

Perhaps give a reference to where in the TRM this can be found?

> +
> +Required subnode-properties:
> +==========================
> +- pins : An array of strings. Each string contains the name of an IO pads. Valid
> +	 values for these names are listed below.
> +
> +Optional subnode-properties:
> +==========================
> +Following properties are supported from generic pin configuration explained
> +in <dt-bindings/pinctrl/pinctrl-binding.txt>.

I don't think such a directory exists. Maybe make these relative
references, though that might become slightly more difficult if this
whole document is merged with the PMC documentation.

> +low-power-enable:		enable low power mode.
> +low-power-disable:		disable low power mode.

Why the extra padding with tabs? I find that difficult to read. Also, no
need for a fullstop since it's not a proper sentence.

> +Valid values for pin for T124 are:

Tegra124

> +	audio, bb, cam, comp, csia, csib, csie, dsi, dsib, dsic, dsid, hdmi,
> +	hsic, hv, lvds, mipi-bias, nand, pex-bias, pex-clk1, pex-clk2,
> +	pex-ctrl, sdmmc1, sdmmc3, sdmmc4, sys-ddc, uart, usb0, usb1, usb2,
> +	usb-bias
> +
> +Valid values for pin for T210 are:

Tegra210

> +	audio, audio-hv, cam, csia, csib, csic, csid, csie, csif,
> +	dbg, debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2,
> +	gpio, hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2,
> +	pex-ctrl, sdmmc1, sdmmc3, spi, spi-hv, uart, usb-bias, usb0,
> +	usb1, usb2, usb3.
> +
> +To find out the IO rail voltage for setting the voltage of IO pad by SW,
> +the regulator supply handle must provided from the DT and it is explained
> +in the regulator DT binding document
> +	<devicetree/bindings/regulator/regulator.txt>.

Again, maybe make this relative because as it is it isn't obvious as to
what the base of the above path is.

> +For example, for GPIO rail the supply name is vddio-gpio and regulator
> +handle is supplied from DT as
> +	vddio-gpio-supply = <&regulator_xyz>;
> +
> +For T210, following IO pads support the 1.8V/3.3V and the corresponding

Tegra210

> +IO voltage pin names are as follows:
> +	audio -> vddio-audio
> +	audio-hv -> vddio-audio-hv
> +	cam ->vddio-cam
> +	dbg -> vddio-dbg
> +	dmic -> vddio-dmic
> +	gpio -> vddio-gpio
> +	pex-ctrl -> vddio-pex-ctrl
> +	sdmmc1 -> vddio-sdmmc1
> +	sdmmc3 -> vddio-sdmmc3
> +	spi -> vddio-spi
> +	spi-hv -> vddio-spi-hv
> +	uart -> vddio-uart

It's slightly confusing to only have this list for Tegra210. I assume
that is because on Tegra124 there is no way to control the voltage of
the pins, but I think that could be made clearer. Also, it might be
worth explicitly mentioning that this is a subset of the list of pins
given above and that the other pins (those not in this list) don't
support 1.8/3.3 V control, but only the low power state.

> +
> +Example:
> +	i2c@...0d000 {
> +		pmic@3c {
> +			regulators {
> +				vddio_sdmmc1: ldo2 {
> +					/* Regulator entries for LDO2 */
> +				};
> +
> +				vdd_cam: ldo3 {
> +					/* Regulator entries for LDO3 */

These comments are somewhat stating the obvious. Perhaps simply use a
... as a placeholder?

> +				};
> +			};
> +		};
> +	};
> +
> +	pmc@...0e400 {
> +		vddio-cam-supply = <&vdd_cam>;
> +		vddio-sdmmc1-supply = <&vddio_sdmmc1>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&tegra_io_pad_volt_default>;
> +		tegra_io_pad_volt_default: common {
> +			audio-hv {
> +				pins = "audio-hv";
> +				low-power-disable;
> +			};

I wonder if this is at all useful. Shouldn't we rather put all pads into
a low-power state by default and only take them out of the low-power
state when the driver decides to do so?

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)

Powered by blists - more mailing lists