by Thierry Reding

[permalink] [raw]

Subject: Re: [PATCH 01/18] dt-bindings: phy: tegra-xusb: Add usb-role-switch

On Fri, Dec 06, 2019 at 04:20:04PM +0530, Nagarjuna Kristam wrote:
> Add usb-role-switch property for Tegra210 and Tegra186 platforms. This
> entry is used by XUSB pad controller driver to register for role changes
> for OTG/Peripheral capable USB 2 ports.
>
> Signed-off-by: Nagarjuna Kristam <[email protected]>
> ---
> Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> index 9fb682e..0f19ed6 100644
> --- a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> @@ -179,6 +179,10 @@ Optional properties:
> is internal. In the absence of this property the port is considered to be
> external.
> - vbus-supply: phandle to a regulator supplying the VBUS voltage.
> +- usb-role-switch: boolean property to indicate use of USB Role Switch.

That first sentence here seems a bit useless and vague. It doesn't
really convey anything other than the name already does. Perhaps
something like:

Boolean property to indicate that the port support OTG. If
present, the port supports switching between USB host and
peripheral roles.

> + This property is MUST for OTG,Peripheral capable USB 2 ports. Connector

If this is mandatory, why not add it to the list of required properties?
I guess since it's only mandatory for ports that support OTG, perhaps we
could add a section "Required properties for OTG capable ports:" or
something like that? Then you can also omit the second sentence in the
description.

> + should be added as subnode, see connector.txt. vbus-gpio in connector is

There's no file called "connector.txt". Are you referring to

Documentation/devicetree/bindings/connector/usb-connector.txt

? Also, that file calls the property "vbus-gpios" and lists it as
optional. What would happen if we don't specify it? Doesn't that just
mean that we can't support role detection?

> + Mandatory.

"mandatory"

Thierry

>
> ULPI ports:
> -----------
> --
> 2.7.4
>

Attachments:

(No filename) (2.22 kB)
signature.asc (849.00 B)
Download all attachments

2019-12-06 14:55:33

by Thierry Reding

[permalink] [raw]

Subject: Re: [PATCH 03/18] phy: tegra: xusb: Add usb-role-switch support

On Fri, Dec 06, 2019 at 04:20:06PM +0530, Nagarjuna Kristam wrote:
> If usb-role-switch property is present in USB 2 port, register
> usb-role-switch to receive usb role changes.
>
> Signed-off-by: Nagarjuna Kristam <[email protected]>
> ---
> drivers/phy/tegra/Kconfig | 1 +
> drivers/phy/tegra/xusb.c | 40 ++++++++++++++++++++++++++++++++++++++++
> drivers/phy/tegra/xusb.h | 3 +++
> 3 files changed, 44 insertions(+)
>
> diff --git a/drivers/phy/tegra/Kconfig b/drivers/phy/tegra/Kconfig
> index f9817c3..df07c4d 100644
> --- a/drivers/phy/tegra/Kconfig
> +++ b/drivers/phy/tegra/Kconfig
> @@ -2,6 +2,7 @@
> config PHY_TEGRA_XUSB
> tristate "NVIDIA Tegra XUSB pad controller driver"
> depends on ARCH_TEGRA
> + select USB_CONN_GPIO
> help
> Choose this option if you have an NVIDIA Tegra SoC.
>
> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
> index f98ec39..da60a63 100644
> --- a/drivers/phy/tegra/xusb.c
> +++ b/drivers/phy/tegra/xusb.c
> @@ -523,6 +523,7 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
> port->dev.type = &tegra_xusb_port_type;
> port->dev.of_node = of_node_get(np);
> port->dev.parent = padctl->dev;
> + port->dev.driver = padctl->dev->driver;
>
> err = dev_set_name(&port->dev, "%s-%u", name, index);
> if (err < 0)
> @@ -532,6 +533,7 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
> if (err < 0)
> goto unregister;
>
> + dev_set_drvdata(&port->dev, port);

You never seem to use dev_get_drvdata() to get at this. Also, you can
get at it via container_of(), so this is only marginally useful to begin
with.

> return 0;
>
> unregister:
> @@ -541,6 +543,7 @@ static int tegra_xusb_port_init(struct tegra_xusb_port *port,
>
> static void tegra_xusb_port_unregister(struct tegra_xusb_port *port)
> {
> + usb_role_switch_unregister(port->usb_role_sw);
> device_unregister(&port->dev);
> }
>
> @@ -551,11 +554,39 @@ static const char *const modes[] = {
> [USB_DR_MODE_OTG] = "otg",
> };
>
> +static int tegra_xusb_role_sw_set(struct device *dev, enum usb_role role)
> +{
> + dev_dbg(dev, "%s calling notifier for role is %d\n", __func__, role);
> +
> + return 0;
> +}
> +
> +static int tegra_xusb_setup_usb_role_switch(struct tegra_xusb_port *port)
> +{
> + int err = 0;
> + struct usb_role_switch_desc role_sx_desc = {
> + .set = tegra_xusb_role_sw_set,
> + .fwnode = dev_fwnode(&port->dev),
> + };
> +
> + port->usb_role_sw = usb_role_switch_register(&port->dev,
> + &role_sx_desc);
> + if (IS_ERR(port->usb_role_sw)) {
> + err = PTR_ERR(port->usb_role_sw);
> + if (err != EPROBE_DEFER)
> + dev_err(&port->dev, "Failed to register USB role SW: %d",
> + err);
> + }
> +
> + return err;
> +}
> +
> static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
> {
> struct tegra_xusb_port *port = &usb2->base;
> struct device_node *np = port->dev.of_node;
> const char *mode;
> + int err;
>
> usb2->internal = of_property_read_bool(np, "nvidia,internal");
>
> @@ -572,6 +603,15 @@ static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
> usb2->mode = USB_DR_MODE_HOST;
> }
>
> + if (of_property_read_bool(np, "usb-role-switch")) {
> + /* populate connector entry */
> + of_platform_populate(np, NULL, NULL, &port->dev);

I think we want to clean this up on failure, don't we? Otherwise we
might end up trying to register the same platform device multiple times.
Also, do we want to depopulate when the port is removed again?

Have you tried unloading and loading the driver to see if that works?

Thierry

> +
> + err = tegra_xusb_setup_usb_role_switch(port);
> + if (err < 0)
> + return err;
> + }
> +
> usb2->supply = devm_regulator_get(&port->dev, "vbus");
> return PTR_ERR_OR_ZERO(usb2->supply);
> }
> diff --git a/drivers/phy/tegra/xusb.h b/drivers/phy/tegra/xusb.h
> index da94fcc..9f27899 100644
> --- a/drivers/phy/tegra/xusb.h
> +++ b/drivers/phy/tegra/xusb.h
> @@ -12,6 +12,7 @@
> #include <linux/workqueue.h>
>
> #include <linux/usb/otg.h>
> +#include <linux/usb/role.h>
>
> /* legacy entry points for backwards-compatibility */
> int tegra_xusb_padctl_legacy_probe(struct platform_device *pdev);
> @@ -266,6 +267,8 @@ struct tegra_xusb_port {
> struct list_head list;
> struct device dev;
>
> + struct usb_role_switch *usb_role_sw;
> +
> const struct tegra_xusb_port_ops *ops;
> };
>
> --
> 2.7.4
>

Attachments:

(No filename) (4.50 kB)
signature.asc (849.00 B)
Download all attachments

2019-12-06 14:58:27

On Fri, Dec 06, 2019 at 04:20:05PM +0530, Nagarjuna Kristam wrote:
> Add device-tree binding documentation for the XUSB device mode controller
> present on Tegra210 and Tegra186 SoC. This controller supports the USB 3.0
> specification.
>
> Signed-off-by: Nagarjuna Kristam <[email protected]>
> ---

Next time version your patches correctly and include version history
here so I don't have to go look up why my R-by is missing.

> .../devicetree/bindings/usb/nvidia,tegra-xudc.yaml | 204 +++++++++++++++++++++
> 1 file changed, 204 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml
>
> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml
> new file mode 100644
> index 0000000..b23c451
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra-xudc.yaml
> @@ -0,0 +1,204 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings please:

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/usb/nvidia,tegra-xudc.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: Device tree binding for NVIDIA Tegra XUSB device mode controller (XUDC)
> +
> +description:
> + The Tegra XUDC controller supports both USB 2.0 HighSpeed/FullSpeed and
> + USB 3.0 SuperSpeed protocols.
> +
> +maintainers:
> + - Nagarjuna Kristam <[email protected]>
> + - JC Kuo <[email protected]>
> + - Thierry Reding <[email protected]>
> +
> +properties:
> + compatible:
> + oneOf:
> + - items:
> + - const: nvidia,tegra210-xudc # For Tegra210
> + - items:
> + - const: nvidia,tegra186-xudc # For Tegra186

Use 'enum' instead of oneOf+const.

> +
> + interrupts:
> + maxItems: 1
> + description: Must contain the XUSB device interrupt.

Don't need a description for a single interrupt line.

> +
> + power-domains:
> + maxItems: 2
> + description:
> + A list of PM domain specifiers that reference each power-domain
> + used by the XUSB device mode controller. This list must comprise of a
> + specifier for the XUSBA and XUSBB power-domains.
> + See ../power/power_domain.txt and ../arm/tegra/nvidia,tegra20-pmc.txt
> + for details.

Just need:

items:
- description: XUSBA power-domain
- description: XUSBB power-domain

> +
> + power-domains-names:
> + maxItems: 2
> + description:
> + A list of names that represent each of the specifiers in
> + the 'power-domains' property.

That's every 'power-domains-names'.

> + items:
> + - const: ss
> + - const: dev

Okay, but those names don't match up with XUSBA and XUSBB. Names should
be meaningful or local to the module, not the provider if that helps.

> +
> + nvidia,xusb-padctl:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description:
> + phandle to the XUSB pad controller that is used to configure the USB pads
> + used by the XUDC controller.
> +
> + phys:
> + minItems: 1
> + description:
> + Must contain an entry for each entry in phy-names.
> + See ../phy/phy-bindings.txt for details.
> +
> + phy-names:
> + minItems: 1
> + items:
> + - const: usb2-0
> + - const: usb2-1
> + - const: usb2-2
> + - const: usb2-3
> + - const: usb3-0
> + - const: usb3-1
> + - const: usb3-2
> + - const: usb3-3
> +
> + avddio-usb-supply:
> + description: PCIe/USB3 analog logic power supply. Must supply 1.05 V.
> +
> + hvdd-usb-supply:
> + description: USB controller power supply. Must supply 3.3 V.
> +
> +required:
> + - compatible
> + - power-domains
> + - power-domain-names
> + - nvidia,xusb-padctl
> + - phys
> + - phy-names
> +
> +allOf:
> + - if:
> + properties:
> + compatible:
> + items:
> + const: nvidia,tegra210-xudc
> +
> + then:
> + properties:
> + reg:
> + maxItems: 3
> + items:
> + - description: XUSB device controller registers
> + - description: XUSB device PCI Config registers
> + - description: XUSB device registers.
> + reg-names:
> + maxItems: 3
> + items:
> + - const: base
> + - const: fpci
> + - const: ipfs

As these are a superset of tegra186, you can move this up and tegra186
just needs to define 'maxItems: 2'.

> + clocks:
> + description:
> + Must contain an entry for all clocks used. See ../clock/clock-bindings.txt
> + for details.
> + maxItems: 5
> + items:
> + - description: Clock to enable core XUSB dev clock.
> + - description: Clock to enable XUSB super speed clock.
> + - description: Clock to enable XUSB super speed dev clock.
> + - description: Clock to enable XUSB high speed dev clock.
> + - description: Clock to enable XUSB full speed dev clock.
> + clock-names:
> + items:
> + - const: dev
> + - const: ss
> + - const: ss_src
> + - const: hs_src
> + - const: fs_src

I would re-order the last 2 entries so that you can do the same thing as
I said for 'reg'.

> + required:
> + - reg
> + - reg-names
> + - clocks
> + - clock-names

No need for these to be under the if. They are always required and
don't have to be where defined by 'properties'.

> + - avddio-usb-supply
> + - hvdd-usb-supply
> + - if:
> + properties:
> + compatible:
> + contains:
> + const: nvidia,tegra186-xudc
> +
> + then:
> + properties:
> + reg:
> + maxItems: 2
> + items:
> + - description: XUSB device controller registers
> + - description: XUSB device PCI Config registers
> + reg-names:
> + maxItems: 2
> + items:
> + - const: base
> + - const: fpci
> + clocks:
> + description:
> + Must contain an entry for all clocks used. See ../clock/clock-bindings.txt
> + for details.
> + maxItems: 4
> + items:
> + - description: Clock to enable core XUSB dev clock.
> + - description: Clock to enable XUSB super speed clock.
> + - description: Clock to enable XUSB super speed dev clock.
> + - description: Clock to enable XUSB full speed dev clock.
> + clock-names:
> + items:
> + - const: dev
> + - const: ss
> + - const: ss_src
> + - const: fs_src
> + required:
> + - reg
> + - reg-names
> + - clocks
> + - clock-names
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/tegra210-car.h>
> + #include <dt-bindings/gpio/tegra-gpio.h>
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> + usb@700d0000 {
> + compatible = "nvidia,tegra210-xudc";
> + reg = <0x0 0x700d0000 0x0 0x8000>,
> + <0x0 0x700d8000 0x0 0x1000>,
> + <0x0 0x700d9000 0x0 0x1000>;
> + reg-names = "base", "fpci", "ipfs";
> +
> + interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
> +
> + clocks = <&tegra_car TEGRA210_CLK_XUSB_DEV>,
> + <&tegra_car TEGRA210_CLK_XUSB_SS>,
> + <&tegra_car TEGRA210_CLK_XUSB_SSP_SRC>,
> + <&tegra_car TEGRA210_CLK_XUSB_HS_SRC>,
> + <&tegra_car TEGRA210_CLK_XUSB_FS_SRC>;
> + clock-names = "dev", "ss", "ss_src", "hs_src", "fs_src";
> +
> + power-domains = <&pd_xusbdev>, <&pd_xusbss>;
> + power-domain-names = "dev", "ss";
> +
> + nvidia,xusb-padctl = <&padctl>;
> +
> + phys = <&micro_b>;
> + phy-names = "usb2-0";
> +
> + avddio-usb-supply = <&vdd_pex_1v05>;
> + hvdd-usb-supply = <&vdd_3v3_sys>;
> + };
> --
> 2.7.4
>