[2/9] thunderbolt: Add USB4 port devices

Create devices for each USB4 port. This is needed when we add retimer
access when there is no device connected but may be useful for other
purposes too following what USB subsystem does. This exports a single
attribute "link" that shows the type of the USB4 link (or "none" if
there is no cable connected).

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 .../ABI/testing/sysfs-bus-thunderbolt         |   7 ++
 drivers/thunderbolt/Makefile                  |   2 +-
 drivers/thunderbolt/retimer.c                 |  15 ++-
 drivers/thunderbolt/switch.c                  |  17 ++-
 drivers/thunderbolt/tb.h                      |  30 +++++
 drivers/thunderbolt/usb4.c                    |  54 +++++++++
 drivers/thunderbolt/usb4_port.c               | 112 ++++++++++++++++++
 7 files changed, 229 insertions(+), 8 deletions(-)
 create mode 100644 drivers/thunderbolt/usb4_port.c

On Wed, May 19, 2021 at 05:12:52PM +0300, Mika Westerberg wrote:
> Create devices for each USB4 port. This is needed when we add retimer
> access when there is no device connected but may be useful for other
> purposes too following what USB subsystem does. This exports a single
> attribute "link" that shows the type of the USB4 link (or "none" if
> there is no cable connected).

<snip>

> +/*
> + * USB4 port device
> + *
> + * Copyright (C) 2021, Intel Corporation
> + * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "tb.h"
> +
> +static ssize_t link_show(struct device *dev, struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct usb4_port *usb4 = tb_to_usb4_port_device(dev);
> +	struct tb_port *port = usb4->port;
> +	struct tb *tb = port->sw->tb;
> +	const char *link;
> +
> +	if (mutex_lock_interruptible(&tb->lock))
> +		return -ERESTARTSYS;
> +
> +	if (tb_is_upstream_port(port))
> +		link = port->sw->link_usb4 ? "usb4" : "tbt";
> +	else if (tb_port_has_remote(port))
> +		link = port->remote->sw->link_usb4 ? "usb4" : "tbt";
> +	else
> +		link = "none";
> +
> +	mutex_unlock(&tb->lock);
> +
> +	return sysfs_emit(buf, "%s\n", link);
> +}
> +static DEVICE_ATTR_RO(link);
> +
> +static struct attribute *common_attrs[] = {
> +	&dev_attr_link.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group common_group = {
> +	.attrs = common_attrs,
> +};
> +
> +static const struct attribute_group *usb4_port_device_groups[] = {
> +	&common_group,
> +	NULL
> +};
> +
> +static void usb4_port_device_release(struct device *dev)
> +{
> +	struct usb4_port *usb4 = container_of(dev, struct usb4_port, dev);
> +
> +	kfree(usb4);
> +}
> +
> +struct device_type usb4_port_device_type = {
> +	.name = "usb4_port",
> +	.groups = usb4_port_device_groups,
> +	.release = usb4_port_device_release,
> +};

I noticed that in the next patch you add acpi_bus_type for these
ports, but is that really necessary? Why not just:

int usb4_port_fwnode_match(struct tb_port *port, struct fwnode_handle *fwnode)
{
        if (is_acpi_device_node(fwnode))
                return acpi_device_adr(to_acpi_device_node(fwnode)) == port->port;

        return 0;
}

> +/**
> + * usb4_port_device_add() - Add USB4 port device
> + * @port: Lane 0 adapter port to add the USB4 port
> + *
> + * Creates and registers a USB4 port device for @port. Returns the new
> + * USB4 port device pointer or ERR_PTR() in case of error.
> + */
> +struct usb4_port *usb4_port_device_add(struct tb_port *port)
> +{

        struct fwnode_handle *child;

> +	struct usb4_port *usb4;
> +	int ret;
> +
> +	usb4 = kzalloc(sizeof(*usb4), GFP_KERNEL);
> +	if (!usb4)
> +		return ERR_PTR(-ENOMEM);
> +
> +	usb4->port = port;
> +	usb4->dev.type = &usb4_port_device_type;
> +	usb4->dev.parent = &port->sw->dev;
> +	dev_set_name(&usb4->dev, "usb4_port%d", port->port);

and then here something like this (feel free to improve this part):

        device_for_each_child_node(&port->sw->dev, child) {
                if (usb4_port_fwnode_match(port, child)) {
                        usb4->dev.fwnode = child;
                        break;
                }
        }

Or maybe I'm missing something?

> +	ret = device_register(&usb4->dev);
> +	if (ret) {
> +		put_device(&usb4->dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	pm_runtime_no_callbacks(&usb4->dev);
> +	pm_runtime_set_active(&usb4->dev);
> +	pm_runtime_enable(&usb4->dev);
> +	pm_runtime_set_autosuspend_delay(&usb4->dev, TB_AUTOSUSPEND_DELAY);
> +	pm_runtime_mark_last_busy(&usb4->dev);
> +	pm_runtime_use_autosuspend(&usb4->dev);
> +
> +	return usb4;
> +}

thanks,

Hi Heikki,

On Wed, May 19, 2021 at 06:14:47PM +0300, Heikki Krogerus wrote:
> On Wed, May 19, 2021 at 05:12:52PM +0300, Mika Westerberg wrote:
> > Create devices for each USB4 port. This is needed when we add retimer
> > access when there is no device connected but may be useful for other
> > purposes too following what USB subsystem does. This exports a single
> > attribute "link" that shows the type of the USB4 link (or "none" if
> > there is no cable connected).
> 
> <snip>
> 
> > +/*
> > + * USB4 port device
> > + *
> > + * Copyright (C) 2021, Intel Corporation
> > + * Author: Mika Westerberg <mika.westerberg@linux.intel.com>
> > + */
> > +
> > +#include <linux/acpi.h>
> > +#include <linux/pm_runtime.h>
> > +
> > +#include "tb.h"
> > +
> > +static ssize_t link_show(struct device *dev, struct device_attribute *attr,
> > +			 char *buf)
> > +{
> > +	struct usb4_port *usb4 = tb_to_usb4_port_device(dev);
> > +	struct tb_port *port = usb4->port;
> > +	struct tb *tb = port->sw->tb;
> > +	const char *link;
> > +
> > +	if (mutex_lock_interruptible(&tb->lock))
> > +		return -ERESTARTSYS;
> > +
> > +	if (tb_is_upstream_port(port))
> > +		link = port->sw->link_usb4 ? "usb4" : "tbt";
> > +	else if (tb_port_has_remote(port))
> > +		link = port->remote->sw->link_usb4 ? "usb4" : "tbt";
> > +	else
> > +		link = "none";
> > +
> > +	mutex_unlock(&tb->lock);
> > +
> > +	return sysfs_emit(buf, "%s\n", link);
> > +}
> > +static DEVICE_ATTR_RO(link);
> > +
> > +static struct attribute *common_attrs[] = {
> > +	&dev_attr_link.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group common_group = {
> > +	.attrs = common_attrs,
> > +};
> > +
> > +static const struct attribute_group *usb4_port_device_groups[] = {
> > +	&common_group,
> > +	NULL
> > +};
> > +
> > +static void usb4_port_device_release(struct device *dev)
> > +{
> > +	struct usb4_port *usb4 = container_of(dev, struct usb4_port, dev);
> > +
> > +	kfree(usb4);
> > +}
> > +
> > +struct device_type usb4_port_device_type = {
> > +	.name = "usb4_port",
> > +	.groups = usb4_port_device_groups,
> > +	.release = usb4_port_device_release,
> > +};
> 
> I noticed that in the next patch you add acpi_bus_type for these
> ports, but is that really necessary? Why not just:
> 
> int usb4_port_fwnode_match(struct tb_port *port, struct fwnode_handle *fwnode)
> {
>         if (is_acpi_device_node(fwnode))
>                 return acpi_device_adr(to_acpi_device_node(fwnode)) == port->port;
> 
>         return 0;
> }

I have to say I might be missing some new additions to fwnode front but
the acpi_bus_type is used to match the ACPI nodes to usb4_ports and also
to routers. I see that this one may work for the former but not sure
about the latter.

I guess we could do similar for routers too in switch.c.

However, I would like to keep ACPI specific code in acpi.c if possible
but if this is the preferred way then no problem doing what you say :)

> > +/**
> > + * usb4_port_device_add() - Add USB4 port device
> > + * @port: Lane 0 adapter port to add the USB4 port
> > + *
> > + * Creates and registers a USB4 port device for @port. Returns the new
> > + * USB4 port device pointer or ERR_PTR() in case of error.
> > + */
> > +struct usb4_port *usb4_port_device_add(struct tb_port *port)
> > +{
> 
>         struct fwnode_handle *child;
> 
> > +	struct usb4_port *usb4;
> > +	int ret;
> > +
> > +	usb4 = kzalloc(sizeof(*usb4), GFP_KERNEL);
> > +	if (!usb4)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	usb4->port = port;
> > +	usb4->dev.type = &usb4_port_device_type;
> > +	usb4->dev.parent = &port->sw->dev;
> > +	dev_set_name(&usb4->dev, "usb4_port%d", port->port);
> 
> and then here something like this (feel free to improve this part):
> 
>         device_for_each_child_node(&port->sw->dev, child) {
>                 if (usb4_port_fwnode_match(port, child)) {
>                         usb4->dev.fwnode = child;
>                         break;
>                 }
>         }
> 
> Or maybe I'm missing something?
> 
> > +	ret = device_register(&usb4->dev);
> > +	if (ret) {
> > +		put_device(&usb4->dev);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	pm_runtime_no_callbacks(&usb4->dev);
> > +	pm_runtime_set_active(&usb4->dev);
> > +	pm_runtime_enable(&usb4->dev);
> > +	pm_runtime_set_autosuspend_delay(&usb4->dev, TB_AUTOSUSPEND_DELAY);
> > +	pm_runtime_mark_last_busy(&usb4->dev);
> > +	pm_runtime_use_autosuspend(&usb4->dev);
> > +
> > +	return usb4;
> > +}
> 
> thanks,
> 
> -- 
> heikki

Hi,

On Wed, May 19, 2021 at 06:40:05PM +0300, Mika Westerberg wrote:
> > I noticed that in the next patch you add acpi_bus_type for these
> > ports, but is that really necessary? Why not just:
> > 
> > int usb4_port_fwnode_match(struct tb_port *port, struct fwnode_handle *fwnode)
> > {
> >         if (is_acpi_device_node(fwnode))
> >                 return acpi_device_adr(to_acpi_device_node(fwnode)) == port->port;
> > 
> >         return 0;
> > }
> 
> I have to say I might be missing some new additions to fwnode front but
> the acpi_bus_type is used to match the ACPI nodes to usb4_ports and also
> to routers. I see that this one may work for the former but not sure
> about the latter.

Yeah, I noticed that afterwards.

> I guess we could do similar for routers too in switch.c.

Forget about it.

> However, I would like to keep ACPI specific code in acpi.c if possible
> but if this is the preferred way then no problem doing what you say :)

Well, that's actually the thing. The matching is the only ACPI (or any
firmware type) specific thing that we need when assigning the firmware
nodes to the device. Note also that the above function works already
even with CONFIG_ACPI=n. That's why I though that there is no reason
not to just make this firmware node type agnostic from the start.

But since your acpi_bus_type handles also the routers, and also since
the node hierarchy with the separate UFP and DFP ports and everything
did not look straightforward, it's probable easier to just use that.

So sorry again for the noise.

> > > +/**
> > > + * usb4_port_device_add() - Add USB4 port device
> > > + * @port: Lane 0 adapter port to add the USB4 port
> > > + *
> > > + * Creates and registers a USB4 port device for @port. Returns the new
> > > + * USB4 port device pointer or ERR_PTR() in case of error.
> > > + */
> > > +struct usb4_port *usb4_port_device_add(struct tb_port *port)
> > > +{
> > 
> >         struct fwnode_handle *child;
> > 
> > > +	struct usb4_port *usb4;
> > > +	int ret;
> > > +
> > > +	usb4 = kzalloc(sizeof(*usb4), GFP_KERNEL);
> > > +	if (!usb4)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	usb4->port = port;
> > > +	usb4->dev.type = &usb4_port_device_type;
> > > +	usb4->dev.parent = &port->sw->dev;
> > > +	dev_set_name(&usb4->dev, "usb4_port%d", port->port);
> > 
> > and then here something like this (feel free to improve this part):
> > 
> >         device_for_each_child_node(&port->sw->dev, child) {
> >                 if (usb4_port_fwnode_match(port, child)) {
> >                         usb4->dev.fwnode = child;
> >                         break;
> >                 }
> >         }
> > 
> > Or maybe I'm missing something?
> > 
> > > +	ret = device_register(&usb4->dev);
> > > +	if (ret) {
> > > +		put_device(&usb4->dev);
> > > +		return ERR_PTR(ret);
> > > +	}
> > > +
> > > +	pm_runtime_no_callbacks(&usb4->dev);
> > > +	pm_runtime_set_active(&usb4->dev);
> > > +	pm_runtime_enable(&usb4->dev);
> > > +	pm_runtime_set_autosuspend_delay(&usb4->dev, TB_AUTOSUSPEND_DELAY);
> > > +	pm_runtime_mark_last_busy(&usb4->dev);
> > > +	pm_runtime_use_autosuspend(&usb4->dev);
> > > +
> > > +	return usb4;
> > > +}

thanks,