Re: [PATCH 3/6] phy: tegra: xusb: Add Tegra194 support

[Thierry Reding]

On Wed, Oct 02, 2019 at 04:00:48PM +0800, JC Kuo wrote:
> Add support for the XUSB pad controller found on Tegra194 SoCs. It is
> mostly similar to the same IP found on Tegra186, but the number of
> pads exposed differs, as do the programming sequences. Because most of
> the Tegra194 XUSB PADCTL registers definition and programming sequence
> are the same as Tegra186, Tegra194 XUSB PADCTL can share the same
> driver, xusb-tegra186.c, with Tegra186 XUSB PADCTL.
> 
> Tegra194 XUSB PADCTL supports up to USB 3.1 Gen 2 speed, however, it
> is possible for some platforms have long signal trace that could not
> provide sufficient electrical environment for Gen 2 speed. This patch
> introduce a new device node property "nvidia,disable-gen2" that can
> be used to specifically disable Gen 2 speed for a particular USB 3.0
> port so that the port can be limited to Gen 1 speed and avoid the
> instability.
> 
> Signed-off-by: JC Kuo <jckuo@xxxxxxxxxx>
> ---
>  drivers/phy/tegra/Makefile        |  1 +
>  drivers/phy/tegra/xusb-tegra186.c | 77 +++++++++++++++++++++++++++++++
>  drivers/phy/tegra/xusb.c          | 13 ++++++
>  drivers/phy/tegra/xusb.h          |  4 ++
>  4 files changed, 95 insertions(+)
> 
> diff --git a/drivers/phy/tegra/Makefile b/drivers/phy/tegra/Makefile
> index 320dd389f34d..89b84067cb4c 100644
> --- a/drivers/phy/tegra/Makefile
> +++ b/drivers/phy/tegra/Makefile
> @@ -6,4 +6,5 @@ phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_124_SOC) += xusb-tegra124.o
>  phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_132_SOC) += xusb-tegra124.o
>  phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_210_SOC) += xusb-tegra210.o
>  phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_186_SOC) += xusb-tegra186.o
> +phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_194_SOC) += xusb-tegra186.o
>  obj-$(CONFIG_PHY_TEGRA194_P2U) += phy-tegra194-p2u.o
> diff --git a/drivers/phy/tegra/xusb-tegra186.c b/drivers/phy/tegra/xusb-tegra186.c
> index 6f3afaf9398f..4e27acf398b2 100644
> --- a/drivers/phy/tegra/xusb-tegra186.c
> +++ b/drivers/phy/tegra/xusb-tegra186.c
> @@ -64,6 +64,13 @@
>  #define  SSPX_ELPG_CLAMP_EN_EARLY(x)		BIT(1 + (x) * 3)
>  #define  SSPX_ELPG_VCORE_DOWN(x)		BIT(2 + (x) * 3)
>  
> +#if IS_ENABLED(CONFIG_ARCH_TEGRA_194_SOC)
> +#define XUSB_PADCTL_SS_PORT_CFG			0x2c
> +#define   PORTX_SPEED_SUPPORT_SHIFT(x)		((x) * 4)
> +#define   PORTX_SPEED_SUPPORT_MASK		(0x3)
> +#define     PORT_SPEED_SUPPORT_GEN1		(0x0)
> +#endif

I wouldn't bother protecting these with the #if/#endif.

> +
>  #define XUSB_PADCTL_USB2_OTG_PADX_CTL0(x)	(0x88 + (x) * 0x40)
>  #define  HS_CURR_LEVEL(x)			((x) & 0x3f)
>  #define  TERM_SEL				BIT(25)
> @@ -635,6 +642,17 @@ static int tegra186_usb3_phy_power_on(struct phy *phy)
>  
>  	padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_CAP);
>  
> +#if IS_ENABLED(CONFIG_ARCH_TEGRA_194_SOC)
> +	if (padctl->soc == &tegra194_xusb_padctl_soc && port->disable_gen2) {
> +		value = padctl_readl(padctl, XUSB_PADCTL_SS_PORT_CFG);
> +		value &= ~(PORTX_SPEED_SUPPORT_MASK <<
> +			PORTX_SPEED_SUPPORT_SHIFT(index));
> +		value |= (PORT_SPEED_SUPPORT_GEN1 <<
> +			PORTX_SPEED_SUPPORT_SHIFT(index));
> +		padctl_writel(padctl, value, XUSB_PADCTL_SS_PORT_CFG);
> +	}
> +#endif

Same here. Also, I think you can drop the extra check for padctl->soc
and only rely on port->disable_gen2. This is not a lot of code, so might
as well make our life simpler by building it unconditionally.

On another note: checking the padctl->soc pointer against a SoC-specific
structure is a neat way to check for this support. However, it's not
very flexible. Consider what happens when the next chip is released. I
think we can assume that it will also support gen 2 and may also require
some boards to disable gen 2 because of long signal traces. In order to
accomodate that, you'd have to extend this check with another comparison
to that new SoC structure.

A better alternative would be to add this as a "feature" flag to the SoC
structure:

	struct tegra_xusb_pad_soc {
		...
		bool supports_gen2;
	};

Presumably every SoC that supports gen 2 will also need support for
explicitly disabling gen 2 if the board doesn't support it, so you can
use that new feature flag to conditionalize this code.

This way, the next SoC generation can support can simply be added by
setting supports_gen2 = true, without requiring any actual code changes
(unless of course if it supports new features).

Multi-SoC support is also a good argument for dropping the #if/#endif
protection, because those would need to be extended for the next SoC
generation as well.

> +
>  	value = padctl_readl(padctl, XUSB_PADCTL_ELPG_PROGRAM_1);
>  	value &= ~SSPX_ELPG_VCORE_DOWN(index);
>  	padctl_writel(padctl, value, XUSB_PADCTL_ELPG_PROGRAM_1);
> @@ -894,6 +912,65 @@ const struct tegra_xusb_padctl_soc tegra186_xusb_padctl_soc = {
>  };
>  EXPORT_SYMBOL_GPL(tegra186_xusb_padctl_soc);
>  
> +#if IS_ENABLED(CONFIG_ARCH_TEGRA_194_SOC)

It doesn't look like we have this type of protection for Tegra186. It
might make sense to add a patch to this series (before this patch) to
slightly clean up the Tegra186 SoC data (reshuffle the data so that a
single #if/#endif block can be used, like you do for Tegra194).

But we can equally well do that in a follow-up.

> +static const char * const tegra194_xusb_padctl_supply_names[] = {
> +	"avdd-usb",
> +	"vclamp-usb",
> +};
> +
> +static const struct tegra_xusb_lane_soc tegra194_usb2_lanes[] = {
> +	TEGRA186_LANE("usb2-0", 0,  0, 0, usb2),
> +	TEGRA186_LANE("usb2-1", 0,  0, 0, usb2),
> +	TEGRA186_LANE("usb2-2", 0,  0, 0, usb2),
> +	TEGRA186_LANE("usb2-3", 0,  0, 0, usb2),
> +};
> +
> +static const struct tegra_xusb_pad_soc tegra194_usb2_pad = {
> +	.name = "usb2",
> +	.num_lanes = ARRAY_SIZE(tegra194_usb2_lanes),
> +	.lanes = tegra194_usb2_lanes,
> +	.ops = &tegra186_usb2_pad_ops,
> +};
> +
> +static const struct tegra_xusb_lane_soc tegra194_usb3_lanes[] = {
> +	TEGRA186_LANE("usb3-0", 0,  0, 0, usb3),
> +	TEGRA186_LANE("usb3-1", 0,  0, 0, usb3),
> +	TEGRA186_LANE("usb3-2", 0,  0, 0, usb3),
> +	TEGRA186_LANE("usb3-3", 0,  0, 0, usb3),
> +};
> +
> +static const struct tegra_xusb_pad_soc tegra194_usb3_pad = {
> +	.name = "usb3",
> +	.num_lanes = ARRAY_SIZE(tegra194_usb3_lanes),
> +	.lanes = tegra194_usb3_lanes,
> +	.ops = &tegra186_usb3_pad_ops,
> +};
> +
> +static const struct tegra_xusb_pad_soc * const tegra194_pads[] = {
> +	&tegra194_usb2_pad,
> +	&tegra194_usb3_pad,
> +};
> +
> +const struct tegra_xusb_padctl_soc tegra194_xusb_padctl_soc = {
> +	.num_pads = ARRAY_SIZE(tegra194_pads),
> +	.pads = tegra194_pads,
> +	.ports = {
> +		.usb2 = {
> +			.ops = &tegra186_usb2_port_ops,
> +			.count = 4,
> +		},
> +		.usb3 = {
> +			.ops = &tegra186_usb3_port_ops,
> +			.count = 4,
> +		},
> +	},
> +	.ops = &tegra186_xusb_padctl_ops,
> +	.supply_names = tegra194_xusb_padctl_supply_names,
> +	.num_supplies = ARRAY_SIZE(tegra194_xusb_padctl_supply_names),
> +};
> +EXPORT_SYMBOL_GPL(tegra194_xusb_padctl_soc);
> +#endif
> +
>  MODULE_AUTHOR("JC Kuo <jckuo@xxxxxxxxxx>");
>  MODULE_DESCRIPTION("NVIDIA Tegra186 XUSB Pad Controller driver");
>  MODULE_LICENSE("GPL v2");
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index 2ea8497af82a..266c08074b28 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -65,6 +65,12 @@ static const struct of_device_id tegra_xusb_padctl_of_match[] = {
>  		.compatible = "nvidia,tegra186-xusb-padctl",
>  		.data = &tegra186_xusb_padctl_soc,
>  	},
> +#endif
> +#if defined(CONFIG_ARCH_TEGRA_194_SOC)
> +	{
> +		.compatible = "nvidia,tegra194-xusb-padctl",
> +		.data = &tegra194_xusb_padctl_soc,
> +	},
>  #endif
>  	{ }
>  };
> @@ -739,6 +745,13 @@ static int tegra_xusb_usb3_port_parse_dt(struct tegra_xusb_usb3_port *usb3)
>  
>  	usb3->internal = of_property_read_bool(np, "nvidia,internal");
>  
> +#if IS_ENABLED(CONFIG_ARCH_TEGRA_194_SOC)
> +	if (port->padctl->soc == &tegra194_xusb_padctl_soc) {
> +		usb3->disable_gen2 = of_property_read_bool(np,
> +							"nvidia,disable-gen2");
> +	}
> +#endif

Do we really need the #if/#endif here? Or the conditional for that
matter? nvidia,disable-gen2 is only defined for Tegra194, so any earlier
SoCs are not going to have one, in which case the above code would just
set usb3->disable_gen2 to false (the default).

Removing the conditional allows you to have the above on a single line.

Thierry

> +
>  	usb3->supply = devm_regulator_get(&port->dev, "vbus");
>  	return PTR_ERR_OR_ZERO(usb3->supply);
>  }
> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
> index 093076ca27fd..6b71978ba15d 100644
> --- a/drivers/phy/tegra/xusb.h
> +++ b/drivers/phy/tegra/xusb.h
> @@ -332,6 +332,7 @@ struct tegra_xusb_usb3_port {
>  	bool context_saved;
>  	unsigned int port;
>  	bool internal;
> +	bool disable_gen2;
>  
>  	u32 tap1;
>  	u32 amp;
> @@ -444,5 +445,8 @@ extern const struct tegra_xusb_padctl_soc tegra210_xusb_padctl_soc;
>  #if defined(CONFIG_ARCH_TEGRA_186_SOC)
>  extern const struct tegra_xusb_padctl_soc tegra186_xusb_padctl_soc;
>  #endif
> +#if defined(CONFIG_ARCH_TEGRA_194_SOC)
> +extern const struct tegra_xusb_padctl_soc tegra194_xusb_padctl_soc;
> +#endif
>  
>  #endif /* __PHY_TEGRA_XUSB_H */
> -- 
> 2.17.1
>

Attachment: signature.asc
Description: PGP signature