| Message ID | 20191009024343.30218-4-jckuo@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | add Tegra194 XUSB host and pad controller support | expand |
On Wed, Oct 09, 2019 at 10:43:41AM +0800, JC Kuo wrote: > Extend the bindings to cover the set of features found in Tegra194. > Note that, technically, there are four more supplies connected to the > XUSB pad controller (DVDD_PEX, DVDD_PEX_PLL, HVDD_PEX and HVDD_PEX_PLL) > , but the power sequencing requirements of Tegra194 require these to be > under the control of the PMIC. > > 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. To deal with > this, a new device node property "nvidia,disable-gen2" was added to > Tegra194 that 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. I suspect this may be a common issue and we should have a common property. Typically, this kind of property is in the controller though and supports multiple speed limits. See PCI bindings for inspiration. Rob
On Wed, Oct 09, 2019 at 06:39:00PM -0500, Rob Herring wrote: > On Wed, Oct 09, 2019 at 10:43:41AM +0800, JC Kuo wrote: > > Extend the bindings to cover the set of features found in Tegra194. > > Note that, technically, there are four more supplies connected to the > > XUSB pad controller (DVDD_PEX, DVDD_PEX_PLL, HVDD_PEX and HVDD_PEX_PLL) > > , but the power sequencing requirements of Tegra194 require these to be > > under the control of the PMIC. > > > > 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. To deal with > > this, a new device node property "nvidia,disable-gen2" was added to > > Tegra194 that 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. > > I suspect this may be a common issue and we should have a common > property. Typically, this kind of property is in the controller though > and supports multiple speed limits. See PCI bindings for inspiration. Given that support for gen 2 speeds is dependent on signal trace length, it doesn't really make sense to restrict the whole controller to a given speed if only the signal trace for a single port exceeds the limit for which gen 2 would work. Also, the USB PHYs are in a different hardware block than the USB controller, so this really is a property of the PHY block, not the USB controller. Thierry
On Mon, Oct 14, 2019 at 8:17 AM Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Oct 09, 2019 at 06:39:00PM -0500, Rob Herring wrote: > > On Wed, Oct 09, 2019 at 10:43:41AM +0800, JC Kuo wrote: > > > Extend the bindings to cover the set of features found in Tegra194. > > > Note that, technically, there are four more supplies connected to the > > > XUSB pad controller (DVDD_PEX, DVDD_PEX_PLL, HVDD_PEX and HVDD_PEX_PLL) > > > , but the power sequencing requirements of Tegra194 require these to be > > > under the control of the PMIC. > > > > > > 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. To deal with > > > this, a new device node property "nvidia,disable-gen2" was added to > > > Tegra194 that 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. > > > > I suspect this may be a common issue and we should have a common > > property. Typically, this kind of property is in the controller though > > and supports multiple speed limits. See PCI bindings for inspiration. > > Given that support for gen 2 speeds is dependent on signal trace length, > it doesn't really make sense to restrict the whole controller to a given > speed if only the signal trace for a single port exceeds the limit for > which gen 2 would work. > > Also, the USB PHYs are in a different hardware block than the USB > controller, so this really is a property of the PHY block, not the USB > controller. Okay, but still should be common for USB PHYs IMO. Rob
Hi Thierry, Hi Rob, Hi Kishon, Please let me know your thoughts of the below implementation. 1. Add a "bool disable_gen2" to "phy->attrs" structure. 2. In _of_phy_get() of phy-core.c to add the follow to parse a generic property. phy->attrs.disable_gen2 = of_property_read_bool(args.np, "usb-disable-gen2"); 3. In individual phy driver, to add SOC/PHY specific programming accordingly. Thanks, JC On 10/14/19 9:40 PM, Rob Herring wrote: > On Mon, Oct 14, 2019 at 8:17 AM Thierry Reding <thierry.reding@gmail.com> wrote: >> >> On Wed, Oct 09, 2019 at 06:39:00PM -0500, Rob Herring wrote: >>> On Wed, Oct 09, 2019 at 10:43:41AM +0800, JC Kuo wrote: >>>> Extend the bindings to cover the set of features found in Tegra194. >>>> Note that, technically, there are four more supplies connected to the >>>> XUSB pad controller (DVDD_PEX, DVDD_PEX_PLL, HVDD_PEX and HVDD_PEX_PLL) >>>> , but the power sequencing requirements of Tegra194 require these to be >>>> under the control of the PMIC. >>>> >>>> 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. To deal with >>>> this, a new device node property "nvidia,disable-gen2" was added to >>>> Tegra194 that 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. >>> >>> I suspect this may be a common issue and we should have a common >>> property. Typically, this kind of property is in the controller though >>> and supports multiple speed limits. See PCI bindings for inspiration. >> >> Given that support for gen 2 speeds is dependent on signal trace length, >> it doesn't really make sense to restrict the whole controller to a given >> speed if only the signal trace for a single port exceeds the limit for >> which gen 2 would work. >> >> Also, the USB PHYs are in a different hardware block than the USB >> controller, so this really is a property of the PHY block, not the USB >> controller. > > Okay, but still should be common for USB PHYs IMO. > > Rob >
On Thu, Oct 17, 2019 at 03:48:52PM +0800, JC Kuo wrote: > Hi Thierry, Hi Rob, Hi Kishon, > Please let me know your thoughts of the below implementation. > > 1. Add a "bool disable_gen2" to "phy->attrs" structure. phy->attrs is pretty bus agnostic, so adding a USB-specific property doesn't sound like the right thing to do here. > 2. In _of_phy_get() of phy-core.c to add the follow to parse a generic property. > > phy->attrs.disable_gen2 = of_property_read_bool(args.np, > "usb-disable-gen2"); > 3. In individual phy driver, to add SOC/PHY specific programming accordingly. Could this perhaps be done using the ->set_mode() callback? We don't currently implement that, but we could implement it and then perhaps use the submode parameter to distinguish between USB 3.1 Gen 1 and USB 3.1 Gen 2. Perhaps a good mapping would look like this: USB 3.1 Gen 1: mode = PHY_MODE_USB_HOST_SS, submode = 0x0300 USB 3.1 Gen 2: mode = PHY_MODE_USB_HOST_SS, submode = 0x0301 The above basically reflects that USB 3.1 Gen 1 is really USB 3.0. This would also work with other speeds: USB 2.0: mode = PHY_MODE_USB_HOST_HS, submode = 0x0200 etc. I suppose to make this clearer we could add defines for the various submodes. It seems like submode may be intended to represent one of the interface modes defined by USBPHY_INTERFACE_MODE_*, but perhaps it can be repurposed for PHY_MODE_USB_HOST_SS? Thierry > > Thanks, > JC > > On 10/14/19 9:40 PM, Rob Herring wrote: > > On Mon, Oct 14, 2019 at 8:17 AM Thierry Reding <thierry.reding@gmail.com> wrote: > >> > >> On Wed, Oct 09, 2019 at 06:39:00PM -0500, Rob Herring wrote: > >>> On Wed, Oct 09, 2019 at 10:43:41AM +0800, JC Kuo wrote: > >>>> Extend the bindings to cover the set of features found in Tegra194. > >>>> Note that, technically, there are four more supplies connected to the > >>>> XUSB pad controller (DVDD_PEX, DVDD_PEX_PLL, HVDD_PEX and HVDD_PEX_PLL) > >>>> , but the power sequencing requirements of Tegra194 require these to be > >>>> under the control of the PMIC. > >>>> > >>>> 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. To deal with > >>>> this, a new device node property "nvidia,disable-gen2" was added to > >>>> Tegra194 that 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. > >>> > >>> I suspect this may be a common issue and we should have a common > >>> property. Typically, this kind of property is in the controller though > >>> and supports multiple speed limits. See PCI bindings for inspiration. > >> > >> Given that support for gen 2 speeds is dependent on signal trace length, > >> it doesn't really make sense to restrict the whole controller to a given > >> speed if only the signal trace for a single port exceeds the limit for > >> which gen 2 would work. > >> > >> Also, the USB PHYs are in a different hardware block than the USB > >> controller, so this really is a property of the PHY block, not the USB > >> controller. > > > > Okay, but still should be common for USB PHYs IMO. > > > > Rob > >
On Thu, Oct 17, 2019 at 03:48:52PM +0800, JC Kuo wrote: > Hi Thierry, Hi Rob, Hi Kishon, > Please let me know your thoughts of the below implementation. > > 1. Add a "bool disable_gen2" to "phy->attrs" structure. > 2. In _of_phy_get() of phy-core.c to add the follow to parse a generic property. > > phy->attrs.disable_gen2 = of_property_read_bool(args.np, > "usb-disable-gen2"); Regarding this, I'm not sure how Rob imagined the generic properties to work. Perhaps he was thinking about something like the max-link-speed property found in the PCI bindings. We could have something like this: - max-link-speed: If present this property specifies the USB generation supported on the PHY/port. Must be: 1: for USB 3.1 Gen 1 (a.k.a. USB 3.0) 2: for USB 3.1 Gen 2 I'm not sure if we need to consider anything prior to USB 3.0. I suppose we could do a similar mapping to what I proposed for the PHY ->set_mode callback: - max-link-speed: If present this property specifies the USB generation supported on the PHY/port. Must be: 0x0100: for USB 1.0 (Low-Speed) 0x0101: for USB 1.1 (Full-Speed) 0x0200: for USB 2.0 (Hi-Speed) 0x0300: for USB 3.0 (SuperSpeed) (a.k.a. USB 3.1 Gen 1) 0x0301: for USB 3.1 (SuperSpeed 10 Gbit/s) (a.k.a. USB 3.1 Gen 2) 0x0302: for USB 3.2 (SuperSpeed 20 Gbit/s) (a.k.a. USB 3.2 Gen 2 x 2) ... Or those could just be sequentially enumerated, like in the above example. Rob, any thoughts? Thierry > 3. In individual phy driver, to add SOC/PHY specific programming accordingly. > > Thanks, > JC > > On 10/14/19 9:40 PM, Rob Herring wrote: > > On Mon, Oct 14, 2019 at 8:17 AM Thierry Reding <thierry.reding@gmail.com> wrote: > >> > >> On Wed, Oct 09, 2019 at 06:39:00PM -0500, Rob Herring wrote: > >>> On Wed, Oct 09, 2019 at 10:43:41AM +0800, JC Kuo wrote: > >>>> Extend the bindings to cover the set of features found in Tegra194. > >>>> Note that, technically, there are four more supplies connected to the > >>>> XUSB pad controller (DVDD_PEX, DVDD_PEX_PLL, HVDD_PEX and HVDD_PEX_PLL) > >>>> , but the power sequencing requirements of Tegra194 require these to be > >>>> under the control of the PMIC. > >>>> > >>>> 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. To deal with > >>>> this, a new device node property "nvidia,disable-gen2" was added to > >>>> Tegra194 that 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. > >>> > >>> I suspect this may be a common issue and we should have a common > >>> property. Typically, this kind of property is in the controller though > >>> and supports multiple speed limits. See PCI bindings for inspiration. > >> > >> Given that support for gen 2 speeds is dependent on signal trace length, > >> it doesn't really make sense to restrict the whole controller to a given > >> speed if only the signal trace for a single port exceeds the limit for > >> which gen 2 would work. > >> > >> Also, the USB PHYs are in a different hardware block than the USB > >> controller, so this really is a property of the PHY block, not the USB > >> controller. > > > > Okay, but still should be common for USB PHYs IMO. > > > > Rob > >
On 17-10-2019 17:31, Thierry Reding wrote: > On Thu, Oct 17, 2019 at 03:48:52PM +0800, JC Kuo wrote: >> Hi Thierry, Hi Rob, Hi Kishon, >> Please let me know your thoughts of the below implementation. >> >> 1. Add a "bool disable_gen2" to "phy->attrs" structure. >> 2. In _of_phy_get() of phy-core.c to add the follow to parse a generic property. >> >> phy->attrs.disable_gen2 = of_property_read_bool(args.np, >> "usb-disable-gen2"); > > Regarding this, I'm not sure how Rob imagined the generic properties to > work. Perhaps he was thinking about something like the max-link-speed > property found in the PCI bindings. > > We could have something like this: > > - max-link-speed: > If present this property specifies the USB generation supported on > the PHY/port. Must be: > 1: for USB 3.1 Gen 1 (a.k.a. USB 3.0) > 2: for USB 3.1 Gen 2 > > I'm not sure if we need to consider anything prior to USB 3.0. I suppose > we could do a similar mapping to what I proposed for the PHY ->set_mode > callback: > > - max-link-speed: > If present this property specifies the USB generation supported on > the PHY/port. Must be: > 0x0100: for USB 1.0 (Low-Speed) > 0x0101: for USB 1.1 (Full-Speed) > 0x0200: for USB 2.0 (Hi-Speed) > 0x0300: for USB 3.0 (SuperSpeed) (a.k.a. USB 3.1 Gen 1) > 0x0301: for USB 3.1 (SuperSpeed 10 Gbit/s) (a.k.a. USB 3.1 Gen 2) > 0x0302: for USB 3.2 (SuperSpeed 20 Gbit/s) (a.k.a. USB 3.2 Gen 2 x 2) > ... > > Or those could just be sequentially enumerated, like in the above > example. > > Rob, any thoughts? > > Thierry > "Documentation/devicetree/bindings/usb/generic.txt" file already has dt-property named maximum-speed, which fulfills current requirement. So to disable gen2 feature simply add below entry to corresponding usb3 port entry. padctl@3520000 { status = "okay"; ports { usb3-3 { maximum-speed = "super-speed"; }; }; Read the property using API usb_get_maximum_speed. Thanks, Nagarjuna >> 3. In individual phy driver, to add SOC/PHY specific programming accordingly. >> >> Thanks, >> JC >> >> On 10/14/19 9:40 PM, Rob Herring wrote: >>> On Mon, Oct 14, 2019 at 8:17 AM Thierry Reding <thierry.reding@gmail.com> wrote: >>>> >>>> On Wed, Oct 09, 2019 at 06:39:00PM -0500, Rob Herring wrote: >>>>> On Wed, Oct 09, 2019 at 10:43:41AM +0800, JC Kuo wrote: >>>>>> Extend the bindings to cover the set of features found in Tegra194. >>>>>> Note that, technically, there are four more supplies connected to the >>>>>> XUSB pad controller (DVDD_PEX, DVDD_PEX_PLL, HVDD_PEX and HVDD_PEX_PLL) >>>>>> , but the power sequencing requirements of Tegra194 require these to be >>>>>> under the control of the PMIC. >>>>>> >>>>>> 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. To deal with >>>>>> this, a new device node property "nvidia,disable-gen2" was added to >>>>>> Tegra194 that 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. >>>>> >>>>> I suspect this may be a common issue and we should have a common >>>>> property. Typically, this kind of property is in the controller though >>>>> and supports multiple speed limits. See PCI bindings for inspiration. >>>> >>>> Given that support for gen 2 speeds is dependent on signal trace length, >>>> it doesn't really make sense to restrict the whole controller to a given >>>> speed if only the signal trace for a single port exceeds the limit for >>>> which gen 2 would work. >>>> >>>> Also, the USB PHYs are in a different hardware block than the USB >>>> controller, so this really is a property of the PHY block, not the USB >>>> controller. >>> >>> Okay, but still should be common for USB PHYs IMO. >>> >>> Rob >>>
diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt index 9fb682e47c29..59d870fa42e9 100644 --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt @@ -37,6 +37,7 @@ Required properties: - Tegra132: "nvidia,tegra132-xusb-padctl", "nvidia,tegra124-xusb-padctl" - Tegra210: "nvidia,tegra210-xusb-padctl" - Tegra186: "nvidia,tegra186-xusb-padctl" + - Tegra194: "nvidia,tegra194-xusb-padctl" - reg: Physical base address and length of the controller's registers. - resets: Must contain an entry for each entry in reset-names. - reset-names: Must include the following entries: @@ -62,6 +63,10 @@ For Tegra186: - vclamp-usb-supply: Bias rail for USB pad. Must supply 1.8 V. - vddio-hsic-supply: HSIC PHY power supply. Must supply 1.2 V. +For Tegra194: +- avdd-usb-supply: USB I/Os, VBUS, ID, REXT, D+/D- power supply. Must supply + 3.3 V. +- vclamp-usb-supply: Bias rail for USB pad. Must supply 1.8 V. Pad nodes: ========== @@ -154,6 +159,11 @@ For Tegra210, the list of valid PHY nodes is given below: - sata: sata-0 - functions: "usb3-ss", "sata" +For Tegra194, the list of valid PHY nodes is given below: +- usb2: usb2-0, usb2-1, usb2-2, usb2-3 + - functions: "xusb" +- usb3: usb3-0, usb3-1, usb3-2, usb3-3 + - functions: "xusb" Port nodes: =========== @@ -221,6 +231,9 @@ Optional properties: is internal. In the absence of this property the port is considered to be external. +- nvidia,disable-gen2: A boolean property whose presence determines that a port + should be limited to USB 3.1 Gen 1. This property is only for Tegra194. + For Tegra124 and Tegra132, the XUSB pad controller exposes the following ports: - 3x USB2: usb2-0, usb2-1, usb2-2 @@ -233,6 +246,9 @@ For Tegra210, the XUSB pad controller exposes the following ports: - 2x HSIC: hsic-0, hsic-1 - 4x super-speed USB: usb3-0, usb3-1, usb3-2, usb3-3 +For Tegra194, the XUSB pad controller exposes the following ports: +- 4x USB2: usb2-0, usb2-1, usb2-2, usb2-3 +- 4x super-speed USB: usb3-0, usb3-1, usb3-2, usb3-3 Examples: =========
Extend the bindings to cover the set of features found in Tegra194. Note that, technically, there are four more supplies connected to the XUSB pad controller (DVDD_PEX, DVDD_PEX_PLL, HVDD_PEX and HVDD_PEX_PLL) , but the power sequencing requirements of Tegra194 require these to be under the control of the PMIC. 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. To deal with this, a new device node property "nvidia,disable-gen2" was added to Tegra194 that 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@nvidia.com> --- Changes in v4: none Changes in v3: none Changes in v2: - fix a typo .../bindings/phy/nvidia,tegra124-xusb-padctl.txt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)