Re: [PATCH 6/6] arm64: tegra: Enable XUSB host in P2972-0000 board

[Thierry Reding]

On Wed, Oct 02, 2019 at 04:00:51PM +0800, JC Kuo wrote:
> This commit enables XUSB host and pad controller in Tegra194
> P2972-0000 board.
> 
> Signed-off-by: JC Kuo <jckuo@xxxxxxxxxx>
> ---
>  .../arm64/boot/dts/nvidia/tegra194-p2888.dtsi | 31 +++++++++-
>  .../boot/dts/nvidia/tegra194-p2972-0000.dts   | 59 +++++++++++++++++++
>  2 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi b/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
> index 4c38426a6969..cb236edc6a0d 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
> +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2888.dtsi
> @@ -229,7 +229,7 @@
>  						regulator-max-microvolt = <3300000>;
>  					};
>  
> -					ldo5 {
> +					vdd_usb_3v3: ldo5 {
>  						regulator-name = "VDD_USB_3V3";
>  						regulator-min-microvolt = <3300000>;
>  						regulator-max-microvolt = <3300000>;
> @@ -313,5 +313,34 @@
>  			regulator-boot-on;
>  			enable-active-low;
>  		};
> +
> +		vdd_5v_sata: regulator@4 {
> +			compatible = "regulator-fixed";
> +			reg = <4>;
> +
> +			regulator-name = "vdd-5v-sata";

Please keep capitalization of regulator names consistent. We use
all-caps and underscores for the others (which mirrors the names in the
schematics), so please stick with that for this one as well.

Also, I'm wondering if perhaps you can clarify something here. My
understanding is that SATA functionality is provided via a controller on
the PCI bus. Why is it that we route the 5 V SATA power to the USB port?
Oh wait... this is one of those eSATAp (hybrid) ports that can take
either eSATA or USB, right? Do we need any additional setup to switch
between eSATA and USB modes? Or is this all done in hardware? That is,
if I plug in an eSATA, does it automatically hotplug detect the device
as SATA and if I plug in a USB device, does it automatically detect it
as USB?

> +			regulator-min-microvolt = <5000000>;
> +			regulator-max-microvolt = <5000000>;
> +			gpio = <&gpio TEGRA194_MAIN_GPIO(Z, 1) GPIO_ACTIVE_LOW>;

This will actually cause a warning on boot. For fixed regulators the
polarity of the enable GPIO is not specified in the GPIO specifier.
Instead you're supposed to use the boolean enable-active-high property
to specify if the enable GPIO is active-high. By default the enable GPIO
is considered to be active-low. The GPIO specifier needs to have the
GPIO_ACTIVE_HIGH flag set regardless for backwards-compatibilitiy
reasons.

Note that regulator@3 above your new entry does this wrongly, but
next-20191002 should have a fix for that.

> +		};
> +	};
> +
> +	padctl@3520000 {

Don't forget to move this into /cbb as well to match the changes to
patch 5/6.

> +		avdd-usb-supply = <&vdd_usb_3v3>;
> +		vclamp-usb-supply = <&vdd_1v8ao>;
> +		ports {

Blank line between the above two for better readability.

> +			usb2-1 {
> +				vbus-supply = <&vdd_5v0_sys>;
> +			};
> +			usb2-3 {

Same here and below.

> +				vbus-supply = <&vdd_5v_sata>;
> +			};
> +			usb3-0 {
> +				vbus-supply = <&vdd_5v0_sys>;
> +			};
> +			usb3-3 {
> +				vbus-supply = <&vdd_5v0_sys>;
> +			};
> +		};
>  	};
>  };
> diff --git a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> index d47cd8c4dd24..410221927dfa 100644
> --- a/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> +++ b/arch/arm64/boot/dts/nvidia/tegra194-p2972-0000.dts
> @@ -222,4 +222,63 @@
>  			};
>  		};
>  	};
> +
> +	padctl@3520000 {

Same comment as above. Move this into /cbb.

> +		status = "okay";
> +
> +		pads {
> +			usb2 {
> +				lanes {
> +					usb2-1 {
> +						status = "okay";
> +					};
> +					usb2-2 {

And blank lines for readability here and below.

> +						status = "okay";
> +					};
> +					usb2-3 {
> +						status = "okay";
> +					};
> +				};
> +			};
> +			usb3 {
> +				lanes {
> +					usb3-0 {
> +						status = "okay";
> +					};
> +					usb3-3 {
> +						status = "okay";
> +					};
> +				};
> +			};
> +		};
> +
> +		ports {
> +			usb2-1 {
> +				mode = "host";
> +				status = "okay";
> +			};
> +			usb2-3 {
> +				mode = "host";
> +				status = "okay";
> +			};
> +			usb3-0 {
> +				nvidia,usb2-companion = <1>;
> +				status = "okay";
> +			};
> +			usb3-3 {
> +				nvidia,usb2-companion = <3>;
> +				nvidia,disable-gen2;
> +				status = "okay";
> +			};
> +		};
> +	};
> +
> +	tegra_xhci: xhci@3610000 {

Also needs to move into /cbb. Also, you can drop the tegra_xhci label
here since we never reference the controller from elsewhere.

Also make sure to update the name here to match the changes in 5/6.

Thierry

> +		status = "okay";
> +		phys =	<&{/padctl@3520000/pads/usb2/lanes/usb2-1}>,
> +			<&{/padctl@3520000/pads/usb2/lanes/usb2-3}>,
> +			<&{/padctl@3520000/pads/usb3/lanes/usb3-0}>,
> +			<&{/padctl@3520000/pads/usb3/lanes/usb3-3}>;
> +		phy-names = "usb2-1", "usb2-3", "usb3-0", "usb3-3";
> +	};
>  };
> -- 
> 2.17.1
>

Attachment: signature.asc
Description: PGP signature