Re: [PATCH v2 2/4] usb: typec: Add attribute file showing the USB Modes of the partner

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi Abhishek,

Thank you for the review.

On Wed, Oct 09, 2024 at 03:16:34PM -0700, Abhishek Pandit-Subedi wrote:
> On Wed, Oct 9, 2024 at 8:46 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > This attribute file shows the supported USB modes (USB 2.0,
> > USB 3.0 and USB4) of the partner, and the currently active
> > mode.
> >
> > The active mode is determined primarily by checking the
> > speed of the enumerated USB device. When USB Power Delivery
> > is supported, the active USB mode should be always the mode
> > that was used with the Enter_USB Message, regardless of the
> > result of the USB enumeration. The port drivers can
> > separately assign the mode with a dedicated API.
> >
> > If USB Power Delivery Identity is supplied for the partner
> > device, the supported modes are extracted from it.
> >
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-typec |  14 +++
> >  drivers/usb/typec/class.c                   | 123 +++++++++++++++++++-
> >  drivers/usb/typec/class.h                   |   2 +
> >  include/linux/usb/typec.h                   |   5 +
> >  4 files changed, 140 insertions(+), 4 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > index 7c307f02d99e..866865ffeb66 100644
> > --- a/Documentation/ABI/testing/sysfs-class-typec
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -233,6 +233,20 @@ Description:
> >                 directory exists, it will have an attribute file for every VDO
> >                 in Discover Identity command result.
> >
> > +What:          /sys/class/typec/<port>-partner/usb_mode
> > +Date:          February 2024
> > +Contact:       Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > +Description:   The USB Modes that the partner device supports. The active mode
> > +               is disaplayed in brackets. The active USB mode can be changed by
> spelling: displayed

Thanks for catching that.

> > +               writing to this file when the port driver is able to send Data
> > +               Reset Message to the partner. That requires USB Power Delivery
> > +               contract between the partner and the port.
> > +
> > +               Valid values:
> > +               - usb2 (USB 2.0)
> > +               - usb3 (USB 3.2)
> > +               - usb4 (USB4)
> > +
> >  USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
> >
> >  Note: Electronically Marked Cables will have a device also for one cable plug
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 9c26d2ad40df..f25cc20a2024 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -618,6 +618,74 @@ EXPORT_SYMBOL_GPL(typec_unregister_altmode);
> >  /* ------------------------------------------------------------------------- */
> >  /* Type-C Partners */
> >
> > +/**
> > + * typec_partner_set_usb_mode - Assign active USB Mode for the partner
> > + * @partner: USB Type-C partner
> > + * @mode: USB Mode (USB2, USB3 or USB4)
> > + *
> > + * The port drivers can use this function to assign the active USB Mode to
> > + * @partner. The USB Mode can change for example due to Data Reset.
> > + */
> > +void typec_partner_set_usb_mode(struct typec_partner *partner, enum usb_mode mode)
> > +{
> > +       if (!partner || partner->usb_mode == mode)
> > +               return;
> > +
> > +       partner->usb_capability |= BIT(mode - 1);
> > +       partner->usb_mode = mode;
> > +       sysfs_notify(&partner->dev.kobj, NULL, "usb_mode");
> > +}
> > +EXPORT_SYMBOL_GPL(typec_partner_set_usb_mode);
> > +
> > +static ssize_t
> > +usb_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +       struct typec_partner *partner = to_typec_partner(dev);
> > +       int len = 0;
> > +       int i;
> > +
> > +       for (i = USB_MODE_USB2; i < USB_MODE_USB4 + 1; i++) {
> > +               if (!(BIT(i - 1) & partner->usb_capability))
> > +                       continue;
> > +
> > +               if (i == partner->usb_mode)
> > +                       len += sysfs_emit_at(buf, len, "[%s] ", usb_modes[i]);
> > +               else
> > +                       len += sysfs_emit_at(buf, len, "%s ", usb_modes[i]);
> > +       }
> > +
> > +       buf[len - 1] = '\n';
> > +       return len;
> > +}
> > +
> > +static ssize_t usb_mode_store(struct device *dev, struct device_attribute *attr,
> > +                             const char *buf, size_t size)
> > +{
> > +       struct typec_partner *partner = to_typec_partner(dev);
> > +       struct typec_port *port = to_typec_port(dev->parent);
> > +       int mode;
> > +       int ret;
> > +
> > +       if (!port->ops || !port->ops->data_reset)
> > +               return -EOPNOTSUPP;
> > +
> > +       mode = sysfs_match_string(usb_modes, buf);
> > +       if (mode < 0)
> > +               return mode;
> > +
> > +       if (mode == partner->usb_mode)
> > +               return size;
> > +
> > +       ret = port->ops->data_reset(port, mode);
> > +       if (ret)
> > +               return ret;
> > +
> > +       typec_partner_set_usb_mode(partner, mode);
> > +
> > +       return size;
> > +}
> > +static DEVICE_ATTR_RW(usb_mode);
> > +
> >  static ssize_t accessory_mode_show(struct device *dev,
> >                                    struct device_attribute *attr,
> >                                    char *buf)
> > @@ -664,6 +732,7 @@ static struct attribute *typec_partner_attrs[] = {
> >         &dev_attr_supports_usb_power_delivery.attr,
> >         &dev_attr_number_of_alternate_modes.attr,
> >         &dev_attr_type.attr,
> > +       &dev_attr_usb_mode.attr,
> >         &dev_attr_usb_power_delivery_revision.attr,
> >         NULL
> >  };
> > @@ -671,6 +740,14 @@ static struct attribute *typec_partner_attrs[] = {
> >  static umode_t typec_partner_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n)
> >  {
> >         struct typec_partner *partner = to_typec_partner(kobj_to_dev(kobj));
> > +       struct typec_port *port = to_typec_port(partner->dev.parent);
> > +
> > +       if (attr == &dev_attr_usb_mode.attr) {
> > +               if (!partner->usb_capability)
> > +                       return 0;
> > +               if (!port->ops || !port->ops->data_reset)
> > +                       return 0444;
> > +       }
> >
> >         if (attr == &dev_attr_number_of_alternate_modes.attr) {
> >                 if (partner->num_altmodes < 0)
> > @@ -744,10 +821,33 @@ static void typec_partner_unlink_device(struct typec_partner *partner, struct de
> >   */
> >  int typec_partner_set_identity(struct typec_partner *partner)
> >  {
> > -       if (!partner->identity)
> > +       u8 usb_capability = partner->usb_capability;
> > +       struct device *dev = &partner->dev;
> > +       struct usb_pd_identity *id;
> > +
> > +       id = get_pd_identity(dev);
> > +       if (!id)
> >                 return -EINVAL;
> >
> > -       typec_report_identity(&partner->dev);
> > +       if (to_typec_port(dev->parent)->data_role == TYPEC_HOST)  {
> > +               u32 devcap = PD_VDO_UFP_DEVCAP(id->vdo[0]);
> > +
> > +               if (devcap & (DEV_USB2_CAPABLE | DEV_USB2_BILLBOARD))
> > +                       usb_capability |= USB_CAPABILITY_USB2;
> > +               if (devcap & DEV_USB3_CAPABLE)
> > +                       usb_capability |= USB_CAPABILITY_USB3;
> > +               if (devcap & DEV_USB4_CAPABLE)
> > +                       usb_capability |= USB_CAPABILITY_USB4;
> > +       } else {
> > +               usb_capability = PD_VDO_DFP_HOSTCAP(id->vdo[0]);
> > +       }
> > +
> > +       if (partner->usb_capability != usb_capability) {
> > +               partner->usb_capability = usb_capability;
> > +               sysfs_notify(&dev->kobj, NULL, "usb_mode");
> > +       }
> > +
> > +       typec_report_identity(dev);
> >         return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(typec_partner_set_identity);
> > @@ -917,6 +1017,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
> >         partner->usb_pd = desc->usb_pd;
> >         partner->accessory = desc->accessory;
> >         partner->num_altmodes = -1;
> > +       partner->usb_capability = desc->usb_capability;
> >         partner->pd_revision = desc->pd_revision;
> >         partner->svdm_version = port->cap->svdm_version;
> >         partner->attach = desc->attach;
> > @@ -936,6 +1037,15 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
> >         partner->dev.type = &typec_partner_dev_type;
> >         dev_set_name(&partner->dev, "%s-partner", dev_name(&port->dev));
> >
> > +       if (port->usb2_dev) {
> > +               partner->usb_capability |= USB_CAPABILITY_USB2;
> > +               partner->usb_mode = USB_MODE_USB2;
> > +       }
> > +       if (port->usb3_dev) {
> > +               partner->usb_capability |= USB_CAPABILITY_USB2 | USB_CAPABILITY_USB3;
> > +               partner->usb_mode = USB_MODE_USB3;
> > +       }
> > +
> >         ret = device_register(&partner->dev);
> >         if (ret) {
> >                 dev_err(&port->dev, "failed to register partner (%d)\n", ret);
> > @@ -1935,13 +2045,18 @@ static void typec_partner_attach(struct typec_connector *con, struct device *dev
> >         struct typec_port *port = container_of(con, struct typec_port, con);
> >         struct typec_partner *partner = typec_get_partner(port);
> >         struct usb_device *udev = to_usb_device(dev);
> > +       enum usb_mode usb_mode;
> >
> > -       if (udev->speed < USB_SPEED_SUPER)
> > +       if (udev->speed < USB_SPEED_SUPER) {
> > +               usb_mode = USB_MODE_USB2;
> >                 port->usb2_dev = dev;
> > -       else
> > +       } else {
> > +               usb_mode = USB_MODE_USB3;
> >                 port->usb3_dev = dev;
> > +       }
> >
> >         if (partner) {
> > +               typec_partner_set_usb_mode(partner, usb_mode);
> >                 typec_partner_link_device(partner, dev);
> >                 put_device(&partner->dev);
> >         }
> > diff --git a/drivers/usb/typec/class.h b/drivers/usb/typec/class.h
> > index 85bc50aa54f7..b3076a24ad2e 100644
> > --- a/drivers/usb/typec/class.h
> > +++ b/drivers/usb/typec/class.h
> > @@ -35,6 +35,8 @@ struct typec_partner {
> >         int                             num_altmodes;
> >         u16                             pd_revision; /* 0300H = "3.0" */
> >         enum usb_pd_svdm_ver            svdm_version;
> > +       enum usb_mode                   usb_mode;
> > +       u8                              usb_capability;
> >
> >         struct usb_power_delivery       *pd;
> >
> > diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
> > index d8999472212d..c35221b0e164 100644
> > --- a/include/linux/usb/typec.h
> > +++ b/include/linux/usb/typec.h
> > @@ -220,6 +220,7 @@ struct typec_cable_desc {
> >   * @accessory: Audio, Debug or none.
> >   * @identity: Discover Identity command data
> >   * @pd_revision: USB Power Delivery Specification Revision if supported
> > + * @usb_capability: Supported USB Modes
> >   * @attach: Notification about attached USB device
> >   * @deattach: Notification about removed USB device
> >   *
> > @@ -237,6 +238,7 @@ struct typec_partner_desc {
> >         enum typec_accessory    accessory;
> >         struct usb_pd_identity  *identity;
> >         u16                     pd_revision; /* 0300H = "3.0" */
> > +       u8                      usb_capability;
> >
> >         void (*attach)(struct typec_partner *partner, struct device *dev);
> >         void (*deattach)(struct typec_partner *partner, struct device *dev);
> > @@ -252,6 +254,7 @@ struct typec_partner_desc {
> >   * @pd_get: Get available USB Power Delivery Capabilities.
> >   * @pd_set: Set USB Power Delivery Capabilities.
> >   * @usb_mode_set: Set the USB Mode to be used with Enter_USB message
> > + * @data_reset: Set new USB mode by using the Data Reset message
> >   */
> >  struct typec_operations {
> >         int (*try_role)(struct typec_port *port, int role);
> > @@ -263,6 +266,7 @@ struct typec_operations {
> >         struct usb_power_delivery **(*pd_get)(struct typec_port *port);
> >         int (*pd_set)(struct typec_port *port, struct usb_power_delivery *pd);
> >         int (*usb_mode_set)(struct typec_port *port, enum usb_mode mode);
> > +       int (*data_reset)(struct typec_port *port, enum usb_mode mode);
> 
> I have a problem with the naming of these two functions,
> "usb_mode_set" and "data_reset".
> 
> The former, usb_mode_set, seems to exist to set the default USB mode
> on data reset. The latter, data_reset, seems to really be Data Reset +
> Enter_USB.
> 
> The former, "usb_mode_set", seems mostly useful to in-kernel TCPM to
> be able to set the default USB response after a data reset. UCSI (and
> cros_ec_typec on ChromeOS) doesn't expose this level of control.
> 
> A better declaration may be:
> * int (*default_usb_mode_set)(struct typec_port *port, enum usb_mode mode);
> * int (*enter_usb_mode)(struct typec_port *port, enum usb_mode mode);

Makes sense. Let's change it like that.

Br,

-- 
heikki