| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <3220d9f4-0a01-65c8-84d8-f6e0e465c7d5@nvidia.com>
Date: Thu, 3 Oct 2019 10:00:53 +0800
From: JC Kuo <jckuo@...dia.com>
To: Thierry Reding <thierry.reding@...il.com>
CC: <gregkh@...uxfoundation.org>, <jonathanh@...dia.com>,
<linux-tegra@...r.kernel.org>, <linux-usb@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
<nkristam@...dia.com>, <skomatineni@...dia.com>
Subject: Re: [PATCH 3/6] phy: tegra: xusb: Add Tegra194 support
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@...dia.com>
>> ---
>> 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@...dia.com>");
>> 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
>>
Powered by blists - more mailing lists