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

[JC Kuo]

On 10/2/19 6:02 PM, Thierry Reding wrote:
> 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.
It will be removed in the next revision.
> 
>> +
>>  #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.
> 
Thanks Thierry. This implementation is better. I will take it in the next revision.

>> +
>>  	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).
Okay, I will do it in the next revision.
> 
> 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.
> 
I will remove #if/#endif and the if() in the next revision. Thanks.
> 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
>>