[V3,03/18] phy: tegra: xusb: Add usb-role-switch support
diff mbox series

Message ID 1577704195-2535-4-git-send-email-nkristam@nvidia.com
State Changes Requested
Headers show
Series
  • Tegra XUSB OTG support
Related show

Commit Message

Nagarjuna Kristam Dec. 30, 2019, 11:09 a.m. UTC
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 <nkristam@nvidia.com>
---
V3:
 - Driver aborts if usb-role-switch is not added in dt forotg/peripheral
   roles.
 - Added role name strings instead of enum values in debug prints.
 - Updated arguments and variable allignments as per Thierry inputs.
---
V2:
 - Removed dev_set_drvdata for port->dev.
 - Added of_platform_depopulate during error handling and driver removal.
---
 drivers/phy/tegra/Kconfig |  1 +
 drivers/phy/tegra/xusb.c  | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/phy/tegra/xusb.h  |  3 +++
 3 files changed, 61 insertions(+)

Comments

Thierry Reding Jan. 28, 2020, 5:32 p.m. UTC | #1
On Mon, Dec 30, 2019 at 04:39:40PM +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 <nkristam@nvidia.com>
> ---
> V3:
>  - Driver aborts if usb-role-switch is not added in dt forotg/peripheral
>    roles.
>  - Added role name strings instead of enum values in debug prints.
>  - Updated arguments and variable allignments as per Thierry inputs.
> ---
> V2:
>  - Removed dev_set_drvdata for port->dev.
>  - Added of_platform_depopulate during error handling and driver removal.
> ---
>  drivers/phy/tegra/Kconfig |  1 +
>  drivers/phy/tegra/xusb.c  | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/phy/tegra/xusb.h  |  3 +++
>  3 files changed, 61 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..11ea9b5 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;

This looks wrong. I don't think driver's are supposed to set this
because it basically means that the device is being attached to the
driver, but in this case it doesn't get probed by the driver and in
fact the ports don't match the pad controller, so they can't really
be driven by the same driver.

Is there any particular reason why you need this?

Thierry
Nagarjuna Kristam Jan. 29, 2020, 9:15 a.m. UTC | #2
On 28-01-2020 23:02, Thierry Reding wrote:
> On Mon, Dec 30, 2019 at 04:39:40PM +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<nkristam@nvidia.com>
>> ---
>> V3:
>>   - Driver aborts if usb-role-switch is not added in dt forotg/peripheral
>>     roles.
>>   - Added role name strings instead of enum values in debug prints.
>>   - Updated arguments and variable allignments as per Thierry inputs.
>> ---
>> V2:
>>   - Removed dev_set_drvdata for port->dev.
>>   - Added of_platform_depopulate during error handling and driver removal.
>> ---
>>   drivers/phy/tegra/Kconfig |  1 +
>>   drivers/phy/tegra/xusb.c  | 57 +++++++++++++++++++++++++++++++++++++++++++++++
>>   drivers/phy/tegra/xusb.h  |  3 +++
>>   3 files changed, 61 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..11ea9b5 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;
> This looks wrong. I don't think driver's are supposed to set this
> because it basically means that the device is being attached to the
> driver, but in this case it doesn't get probed by the driver and in
> fact the ports don't match the pad controller, so they can't really
> be driven by the same driver.
> 
> Is there any particular reason why you need this?
> 
> Thierry

Yes, port->dev.driver->owner is accessed in USB role switch driver in 
API usb_role_switch_get. If driver param is not updated, it causes NULL 
pointer exception. Based on your inputs, since this assignment is not 
supposed to used, I believe option available is to create a new 
device_driver structure and assign the same here.
Please share your thoughts.

- Nagarjuna
Thierry Reding Jan. 29, 2020, 9:26 a.m. UTC | #3
On Wed, Jan 29, 2020 at 02:45:38PM +0530, Nagarjuna Kristam wrote:
> 
> 
> On 28-01-2020 23:02, Thierry Reding wrote:
> > On Mon, Dec 30, 2019 at 04:39:40PM +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<nkristam@nvidia.com>
> > > ---
> > > V3:
> > >   - Driver aborts if usb-role-switch is not added in dt forotg/peripheral
> > >     roles.
> > >   - Added role name strings instead of enum values in debug prints.
> > >   - Updated arguments and variable allignments as per Thierry inputs.
> > > ---
> > > V2:
> > >   - Removed dev_set_drvdata for port->dev.
> > >   - Added of_platform_depopulate during error handling and driver removal.
> > > ---
> > >   drivers/phy/tegra/Kconfig |  1 +
> > >   drivers/phy/tegra/xusb.c  | 57 +++++++++++++++++++++++++++++++++++++++++++++++
> > >   drivers/phy/tegra/xusb.h  |  3 +++
> > >   3 files changed, 61 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..11ea9b5 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;
> > This looks wrong. I don't think driver's are supposed to set this
> > because it basically means that the device is being attached to the
> > driver, but in this case it doesn't get probed by the driver and in
> > fact the ports don't match the pad controller, so they can't really
> > be driven by the same driver.
> > 
> > Is there any particular reason why you need this?
> > 
> > Thierry
> 
> Yes, port->dev.driver->owner is accessed in USB role switch driver in API
> usb_role_switch_get. If driver param is not updated, it causes NULL pointer
> exception. Based on your inputs, since this assignment is not supposed to
> used, I believe option available is to create a new device_driver structure
> and assign the same here.
> Please share your thoughts.

Sounds to me more like what we want is to make the module_get() and
module_put() calls conditional to avoid the NULL pointer dereference.
Another common variant that I've seen in other subsystems is to make
drivers pass in an owner explicitly via some operations structure to
allow more fine-grained control over the reference count.

In this particular case one option to do this would be to add a struct
module *owner field to usb_role_switch_desc and that's copied to struct
usb_role_switch in usb_role_switch_register() so that that can be used
for reference counting (perhaps with the current code as fallback).

That would allow using simple devices (i.e. not bound to a driver) to
work with this framework as well.

Thierry

Patch
diff mbox series

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..11ea9b5 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)
@@ -541,6 +542,11 @@  static int tegra_xusb_port_init(struct tegra_xusb_port *port,
 
 static void tegra_xusb_port_unregister(struct tegra_xusb_port *port)
 {
+	if (!IS_ERR_OR_NULL(port->usb_role_sw)) {
+		of_platform_depopulate(&port->dev);
+		usb_role_switch_unregister(port->usb_role_sw);
+	}
+
 	device_unregister(&port->dev);
 }
 
@@ -551,11 +557,48 @@  static const char *const modes[] = {
 	[USB_DR_MODE_OTG] = "otg",
 };
 
+static const char * const usb_roles[] = {
+	[USB_ROLE_NONE]		= "none",
+	[USB_ROLE_HOST]		= "host",
+	[USB_ROLE_DEVICE]	= "device",
+};
+
+static int tegra_xusb_role_sw_set(struct device *dev, enum usb_role role)
+{
+	dev_dbg(dev, "%s: role %s\n", __func__, usb_roles[role]);
+
+	return 0;
+}
+
+static int tegra_xusb_setup_usb_role_switch(struct tegra_xusb_port *port)
+{
+	struct usb_role_switch_desc role_sx_desc = {
+		.fwnode = dev_fwnode(&port->dev),
+		.set = tegra_xusb_role_sw_set,
+	};
+	int err = 0;
+
+	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);
+		dev_err(&port->dev, "failed to register USB role switch: %d",
+			err);
+		return err;
+	}
+
+	/* populate connector entry */
+	of_platform_populate(port->dev.of_node, NULL, NULL, &port->dev);
+
+	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 +615,20 @@  static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port *usb2)
 		usb2->mode = USB_DR_MODE_HOST;
 	}
 
+	/* usb-role-switch property is mandatory for OTG/Peripheral modes */
+	if (usb2->mode == USB_DR_MODE_PERIPHERAL ||
+	    usb2->mode == USB_DR_MODE_OTG) {
+		if (of_property_read_bool(np, "usb-role-switch")) {
+			err = tegra_xusb_setup_usb_role_switch(port);
+			if (err < 0)
+				return err;
+		} else {
+			dev_err(&port->dev, "usb-role-switch not found for %s mode",
+				modes[usb2->mode]);
+			return -EINVAL;
+		}
+	}
+
 	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;
 };