| Message ID | 20241114-usb-typec-controller-enhancements-v2-3-362376856aea@zuehlke.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: hd3ss3220: enhance driver with port type, power opmode, and role preference settings | expand |
Hi Oliver, I'm sorry, I noticed a problem with this... On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote: > The TI HD3SS3220 Type-C controller supports configuring the port type > it will operate as through the MODE_SELECT field of the General > Control Register. > > Configure the port type based on the fwnode property "power-role" > during probe, and through the port_type_set typec_operation. > > The MODE_SELECT field can only be changed when the controller is in > unattached state, so follow the sequence recommended by the datasheet to: > 1. disable termination on CC pins to disable the controller > 2. change the mode > 3. re-enable termination > > This will effectively cause a connected device to disconnect > for the duration of the mode change. Changing the type of the port is really problematic, and IMO we should actually never support that. Consider for example, if your port is sink only, then the platform almost certainly can't drive the VBUS. This patch would still allow the port to be changed to source port. Sorry for not realising this in v1. I think what you want here is just a power role swap. Currently power role swap is only supported when USB PD is supported in the class code, but since the USB Type-C specification quite clearly states that power role and data role swap can be optionally supported even when USB PD is not supported (section 2.3.3) we need to fix that: diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c index 58f40156de56..ee81909565a4 100644 --- a/drivers/usb/typec/class.c +++ b/drivers/usb/typec/class.c @@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct device *dev, return -EOPNOTSUPP; } - if (port->pwr_opmode != TYPEC_PWR_MODE_PD) { - dev_dbg(dev, "partner unable to swap power role\n"); - return -EIO; - } - ret = sysfs_match_string(typec_roles, buf); if (ret < 0) return ret; After that it should be possible to do power role swap also in this driver when the port is DRP capable. > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com> > --- > drivers/usb/typec/hd3ss3220.c | 66 ++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 65 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c > index e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a01f311fb60b4284da 100644 > --- a/drivers/usb/typec/hd3ss3220.c > +++ b/drivers/usb/typec/hd3ss3220.c > @@ -35,10 +35,16 @@ > #define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS BIT(4) > > /* Register HD3SS3220_REG_GEN_CTRL*/ > +#define HD3SS3220_REG_GEN_CTRL_DISABLE_TERM BIT(0) > #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK (BIT(2) | BIT(1)) > #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT 0x00 > #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK BIT(1) > #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC (BIT(2) | BIT(1)) > +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK (BIT(5) | BIT(4)) > +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DEFAULT 0x00 > +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP BIT(5) > +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP BIT(4) > +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP (BIT(5) | BIT(4)) > > struct hd3ss3220 { > struct device *dev; > @@ -75,6 +81,52 @@ static int hd3ss3220_set_power_opmode(struct hd3ss3220 *hd3ss3220, int power_opm > current_mode); > } > > +static int hd3ss3220_set_port_type(struct hd3ss3220 *hd3ss3220, int type) > +{ > + int mode_select, err; > + > + switch (type) { > + case TYPEC_PORT_SRC: > + mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP; > + break; > + case TYPEC_PORT_SNK: > + mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP; > + break; > + case TYPEC_PORT_DRP: > + mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP; > + break; > + default: > + dev_err(hd3ss3220->dev, "bad port type: %d\n", type); > + return -EINVAL; > + } > + > + /* Disable termination before changing MODE_SELECT as required by datasheet */ > + err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, > + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, > + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM); > + if (err < 0) { > + dev_err(hd3ss3220->dev, "Failed to disable port for mode change: %d\n", err); > + return err; > + } > + > + err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, > + HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK, > + mode_select); > + if (err < 0) { > + dev_err(hd3ss3220->dev, "Failed to change mode: %d\n", err); > + regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, > + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0); > + return err; > + } > + > + err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, > + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0); > + if (err < 0) > + dev_err(hd3ss3220->dev, "Failed to re-enable port after mode change: %d\n", err); > + > + return err; > +} > + > static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref) > { > return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port *port, enum typec_data_role role) > return ret; > } > > +static int hd3ss3220_port_type_set(struct typec_port *port, enum typec_port_type type) > +{ > + struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port); > + > + return hd3ss3220_set_port_type(hd3ss3220, type); > +} This wrapper seems completely useless. You only need one function here for the callback. > static const struct typec_operations hd3ss3220_ops = { > - .dr_set = hd3ss3220_dr_set > + .dr_set = hd3ss3220_dr_set, > + .port_type_set = hd3ss3220_port_type_set, > }; So here I think you should implement the pr_set callback instead. Let me kwno wh > static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220) > @@ -273,6 +333,10 @@ static int hd3ss3220_probe(struct i2c_client *client) > if (ret != 0 && ret != -EINVAL && ret != -ENXIO) > goto err_put_role; > > + ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type); > + if (ret < 0) > + goto err_put_role; > + > hd3ss3220->port = typec_register_port(&client->dev, &typec_cap); > if (IS_ERR(hd3ss3220->port)) { > ret = PTR_ERR(hd3ss3220->port); > thanks,
Hello Heikki, Thanks for reviewing. > -----Original Message----- > From: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Sent: Monday, November 18, 2024 12:50 PM > To: Facklam, Olivér <oliver.facklam@zuehlke.com> > Cc: Biju Das <biju.das@bp.renesas.com>; Greg Kroah-Hartman > <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org; linux- > kernel@vger.kernel.org; von Heyl, Benedict > <benedict.vonheyl@zuehlke.com>; Först, Mathis > <mathis.foerst@zuehlke.com>; Glettig, Michael > <Michael.Glettig@zuehlke.com> > Subject: Re: [PATCH v2 3/4] usb: typec: hd3ss3220: support configuring port > type > > Hi Oliver, > > I'm sorry, I noticed a problem with this... > > On Thu, Nov 14, 2024 at 09:02:08AM +0100, Oliver Facklam wrote: > > The TI HD3SS3220 Type-C controller supports configuring the port type > > it will operate as through the MODE_SELECT field of the General > > Control Register. > > > > Configure the port type based on the fwnode property "power-role" > > during probe, and through the port_type_set typec_operation. > > > > The MODE_SELECT field can only be changed when the controller is in > > unattached state, so follow the sequence recommended by the datasheet > to: > > 1. disable termination on CC pins to disable the controller > > 2. change the mode > > 3. re-enable termination > > > > This will effectively cause a connected device to disconnect > > for the duration of the mode change. > > Changing the type of the port is really problematic, and IMO we should > actually never support that. Could you clarify why you think it is problematic? > > Consider for example, if your port is sink only, then the platform > almost certainly can't drive the VBUS. This patch would still allow > the port to be changed to source port. In my testing, it appeared to me that when registering a type-c port with "typec_cap.type = TYPEC_PORT_SNK" (for example), then the type-c class disables the port_type_store functionality: if (port->cap->type != TYPEC_PORT_DRP || !port->ops || !port->ops->port_type_set) { dev_dbg(dev, "changing port type not supported\n"); return -EOPNOTSUPP; } So to my understanding, a platform which cannot drive VBUS should simply set the fwnode `power-role="sink"`. Since patch 2/4 correctly parses this property, wouldn't that solve this case? > > Sorry for not realising this in v1. > > I think what you want here is just a power role swap. Currently power > role swap is only supported when USB PD is supported in the class > code, but since the USB Type-C specification quite clearly states that > power role and data role swap can be optionally supported even when > USB PD is not supported (section 2.3.3) we need to fix that: My interpretation of section 2.3.3 is that the 2 mechanisms allowing power role swap are: - USB PD (after initial connection) - "as part of the initial connection process": to me this is simply referring to the Try.SRC / Try.SNK mechanism, for which we already have the "try_role" callback. Maybe I'm misunderstanding what the intentions are behind each of the typec_operations, so if you could clarify that (or give some pointer), that would be appreciated. My understanding: - "try_role": set Try.SRC / Try.SNK / no preference for a dual-role port for initial connection - "pr_set" / "dr_set" / "vconn_set": swap power and data role resp. after the initial connection using USB-PD. - "port_type_set": configure what port type to operate as, i.e. which initial connection state machine from the USB-C standard to apply for the next connection Please correct me if any of these are incorrect. > > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c > index 58f40156de56..ee81909565a4 100644 > --- a/drivers/usb/typec/class.c > +++ b/drivers/usb/typec/class.c > @@ -1535,11 +1535,6 @@ static ssize_t power_role_store(struct device > *dev, > return -EOPNOTSUPP; > } > > - if (port->pwr_opmode != TYPEC_PWR_MODE_PD) { > - dev_dbg(dev, "partner unable to swap power role\n"); > - return -EIO; > - } > - > ret = sysfs _match_string(typec_roles, buf); > if (ret < 0) > return ret; > > > After that it should be possible to do power role swap also in this > driver when the port is DRP capable. > > > Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com> > > --- > > drivers/usb/typec/hd3ss3220.c | 66 > ++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 65 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/typec/hd3ss3220.c > b/drivers/usb/typec/hd3ss3220.c > > index > e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a > 01f311fb60b4284da 100644 > > --- a/drivers/usb/typec/hd3ss3220.c > > +++ b/drivers/usb/typec/hd3ss3220.c [...] > > @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port > *port, enum typec_data_role role) > > return ret; > > } > > > > +static int hd3ss3220_port_type_set(struct typec_port *port, enum > typec_port_type type) > > +{ > > + struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port); > > + > > + return hd3ss3220_set_port_type(hd3ss3220, type); > > +} > > This wrapper seems completely useless. You only need one function here > for the callback. The wrapper is to extract the struct hd3ss3220 from the typec_port. The underlying hd3ss3220_set_port_type I am also using during probe to configure initial port type. One point worth mentioning here is that if the MODE_SELECT register is not configured, the chip will operate according to a default which is chosen by an external pin (sorry if this was not detailed enough in commit msg) From the datasheet: ------------------- | PORT | 4 | I | Tri-level input pin to indicate port mode. The state of this pin is sampled when HD3SS3220's ENn_CC is asserted low, and VDD5 is active. This pin is also sampled following a I2C_SOFT_RESET. H - DFP (Pull-up to VDD5 if DFP mode is desired) NC - DRP (Leave unconnected if DRP mode is desired) L - UFP (Pull-down or tie to GND if UFP mode is desired) In our use case, it was not desirable to leave this default based on wiring, and it makes more sense to me to allow the configuration to come from the fwnode property. Hence the port type setting in probe(). > > > static const struct typec_operations hd3ss3220_ops = { > > - .dr_set = hd3ss3220_dr_set > > + .dr_set = hd3ss3220_dr_set, > > + .port_type_set = hd3ss3220_port_type_set, > > }; > > So here I think you should implement the pr_set callback instead. I can do that, but based on the MODE_SELECT register description, it seems to me that this setting is fundamentally changing the operation mode of the chip, i.e. the state machine that is being run for initial connection. So there would have to be a way of "resetting" it to be a dual-role port again, which the "pr_set" callback doesn't seem to have? This register can be written to set the HD3SS3220 mode operation. The ADDR pin must be set to I2C mode. If the default is maintained, HD3SS3220 shall operate according to the PORT pin levels and modes. The MODE_SELECT can only be changed when in the unattached state. 00 - DRP mode (start from unattached.SNK) (default) 01 - UFP mode (unattached.SNK) 10 - DFP mode (unattached.SRC) 11 - DRP mode (start from unattached.SNK) > > Let me kwno wh > > > static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220) > > @@ -273,6 +333,10 @@ static int hd3ss3220_probe(struct i2c_client > *client) > > if (ret != 0 && ret != -EINVAL && ret != -ENXIO) > > goto err_put_role; > > > > + ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type); > > + if (ret < 0) > > + goto err_put_role; > > + > > hd3ss3220->port = typec_register_port(&client->dev, &typec_cap); > > if (IS_ERR(hd3ss3220->port)) { > > ret = PTR_ERR(hd3ss3220->port); > > > > thanks, > > -- > heikki Thanks! Oliver
diff --git a/drivers/usb/typec/hd3ss3220.c b/drivers/usb/typec/hd3ss3220.c index e581272bb47de95dee8363a5491f543354fcbbf8..e3e9b1597e3b09b82f0726a01f311fb60b4284da 100644 --- a/drivers/usb/typec/hd3ss3220.c +++ b/drivers/usb/typec/hd3ss3220.c @@ -35,10 +35,16 @@ #define HD3SS3220_REG_CN_STAT_CTRL_INT_STATUS BIT(4) /* Register HD3SS3220_REG_GEN_CTRL*/ +#define HD3SS3220_REG_GEN_CTRL_DISABLE_TERM BIT(0) #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_MASK (BIT(2) | BIT(1)) #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_DEFAULT 0x00 #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SNK BIT(1) #define HD3SS3220_REG_GEN_CTRL_SRC_PREF_DRP_TRY_SRC (BIT(2) | BIT(1)) +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK (BIT(5) | BIT(4)) +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DEFAULT 0x00 +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP BIT(5) +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP BIT(4) +#define HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP (BIT(5) | BIT(4)) struct hd3ss3220 { struct device *dev; @@ -75,6 +81,52 @@ static int hd3ss3220_set_power_opmode(struct hd3ss3220 *hd3ss3220, int power_opm current_mode); } +static int hd3ss3220_set_port_type(struct hd3ss3220 *hd3ss3220, int type) +{ + int mode_select, err; + + switch (type) { + case TYPEC_PORT_SRC: + mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DFP; + break; + case TYPEC_PORT_SNK: + mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_UFP; + break; + case TYPEC_PORT_DRP: + mode_select = HD3SS3220_REG_GEN_CTRL_MODE_SELECT_DRP; + break; + default: + dev_err(hd3ss3220->dev, "bad port type: %d\n", type); + return -EINVAL; + } + + /* Disable termination before changing MODE_SELECT as required by datasheet */ + err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM); + if (err < 0) { + dev_err(hd3ss3220->dev, "Failed to disable port for mode change: %d\n", err); + return err; + } + + err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, + HD3SS3220_REG_GEN_CTRL_MODE_SELECT_MASK, + mode_select); + if (err < 0) { + dev_err(hd3ss3220->dev, "Failed to change mode: %d\n", err); + regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0); + return err; + } + + err = regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, + HD3SS3220_REG_GEN_CTRL_DISABLE_TERM, 0); + if (err < 0) + dev_err(hd3ss3220->dev, "Failed to re-enable port after mode change: %d\n", err); + + return err; +} + static int hd3ss3220_set_source_pref(struct hd3ss3220 *hd3ss3220, int src_pref) { return regmap_update_bits(hd3ss3220->regmap, HD3SS3220_REG_GEN_CTRL, @@ -131,8 +183,16 @@ static int hd3ss3220_dr_set(struct typec_port *port, enum typec_data_role role) return ret; } +static int hd3ss3220_port_type_set(struct typec_port *port, enum typec_port_type type) +{ + struct hd3ss3220 *hd3ss3220 = typec_get_drvdata(port); + + return hd3ss3220_set_port_type(hd3ss3220, type); +} + static const struct typec_operations hd3ss3220_ops = { - .dr_set = hd3ss3220_dr_set + .dr_set = hd3ss3220_dr_set, + .port_type_set = hd3ss3220_port_type_set, }; static void hd3ss3220_set_role(struct hd3ss3220 *hd3ss3220) @@ -273,6 +333,10 @@ static int hd3ss3220_probe(struct i2c_client *client) if (ret != 0 && ret != -EINVAL && ret != -ENXIO) goto err_put_role; + ret = hd3ss3220_set_port_type(hd3ss3220, typec_cap.type); + if (ret < 0) + goto err_put_role; + hd3ss3220->port = typec_register_port(&client->dev, &typec_cap); if (IS_ERR(hd3ss3220->port)) { ret = PTR_ERR(hd3ss3220->port);
The TI HD3SS3220 Type-C controller supports configuring the port type it will operate as through the MODE_SELECT field of the General Control Register. Configure the port type based on the fwnode property "power-role" during probe, and through the port_type_set typec_operation. The MODE_SELECT field can only be changed when the controller is in unattached state, so follow the sequence recommended by the datasheet to: 1. disable termination on CC pins to disable the controller 2. change the mode 3. re-enable termination This will effectively cause a connected device to disconnect for the duration of the mode change. Signed-off-by: Oliver Facklam <oliver.facklam@zuehlke.com> --- drivers/usb/typec/hd3ss3220.c | 66 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-)