| Message ID | 20240724201116.2094059-1-jthies@google.com |
|---|---|
| Headers | show |
| Series | usb: typec: ucsi: Expand power supply support | expand |
On Wed, Jul 24, 2024 at 08:11:13PM GMT, Jameson Thies wrote: > Add status to UCSI power supply driver properties based on the port's > connection and power direction states. > > Signed-off-by: Jameson Thies <jthies@google.com> Please CC Power Supply maintainers for this patchset (added to cc). At least per the Documentation/ABI/testing/sysfs-class-power, the status property applies to batteries, while UCSI psy device is a charger. This is logical, as there might be multiple reasons why the battery is not being charging even when the supply is online. > --- > Changes in V2: > - None. > > drivers/usb/typec/ucsi/psy.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c > index e623d80e177c..d0b52cee41d2 100644 > --- a/drivers/usb/typec/ucsi/psy.c > +++ b/drivers/usb/typec/ucsi/psy.c > @@ -29,6 +29,7 @@ static enum power_supply_property ucsi_psy_props[] = { > POWER_SUPPLY_PROP_CURRENT_MAX, > POWER_SUPPLY_PROP_CURRENT_NOW, > POWER_SUPPLY_PROP_SCOPE, > + POWER_SUPPLY_PROP_STATUS, > }; > > static int ucsi_psy_get_scope(struct ucsi_connector *con, > @@ -51,6 +52,20 @@ static int ucsi_psy_get_scope(struct ucsi_connector *con, > return 0; > } > > +static int ucsi_psy_get_status(struct ucsi_connector *con, > + union power_supply_propval *val) > +{ > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > + if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > + if ((con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK) > + val->intval = POWER_SUPPLY_STATUS_CHARGING; > + else > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > + } > + > + return 0; > +} > + > static int ucsi_psy_get_online(struct ucsi_connector *con, > union power_supply_propval *val) > { > @@ -249,6 +264,8 @@ static int ucsi_psy_get_prop(struct power_supply *psy, > return ucsi_psy_get_current_now(con, val); > case POWER_SUPPLY_PROP_SCOPE: > return ucsi_psy_get_scope(con, val); > + case POWER_SUPPLY_PROP_STATUS: > + return ucsi_psy_get_status(con, val); > default: > return -EINVAL; > } > -- > 2.45.2.1089.g2a221341d9-goog >
On Wed, Jul 24, 2024 at 08:11:15PM GMT, Jameson Thies wrote: > Add POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX as a property to the UCSI > power supply driver. When set to a positive value, enable the > connector's sink path. When set to a negative value, disable the > connector's sink path. [Also added power supply maintainers to cc] This really looks like a hack. As a user I'd expect that if I set the charging limit, it really gets applied. In other words, I'd expect to be able to limit 60W PSY to provide just 15W by limiting the current. This is not the case with this patch. Please provide rational for this change, i.e. why using the existing typec property isn't enough for your use case. > > Signed-off-by: Jameson Thies <jthies@google.com> > --- > Changes in V2: > - Added SET_SINK_PATH call when handling charge_control_limit_max > update. Updated commit message. > > drivers/usb/typec/ucsi/psy.c | 52 +++++++++++++++++++++++++++++++++++ > drivers/usb/typec/ucsi/ucsi.c | 15 ++++++++++ > drivers/usb/typec/ucsi/ucsi.h | 5 ++++ > 3 files changed, 72 insertions(+) > > diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c > index d708f9eb1654..28265feb9d13 100644 > --- a/drivers/usb/typec/ucsi/psy.c > +++ b/drivers/usb/typec/ucsi/psy.c > @@ -30,6 +30,7 @@ static enum power_supply_property ucsi_psy_props[] = { > POWER_SUPPLY_PROP_CURRENT_NOW, > POWER_SUPPLY_PROP_SCOPE, > POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX, > }; > > static int ucsi_psy_get_scope(struct ucsi_connector *con, > @@ -275,11 +276,60 @@ static int ucsi_psy_get_prop(struct power_supply *psy, > return ucsi_psy_get_scope(con, val); > case POWER_SUPPLY_PROP_STATUS: > return ucsi_psy_get_status(con, val); > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX: > + val->intval = 0; > + return 0; > + default: > + return -EINVAL; > + } > +} > + > +static int ucsi_psy_set_charge_control_limit_max(struct ucsi_connector *con, > + const union power_supply_propval *val) > +{ > + /* > + * Writing a negative value to the charge control limit max implies the > + * port should not accept charge. Disable the sink path for a negative > + * charge control limit, and enable the sink path for a positive charge > + * control limit. If the requested charge port is a source, update the > + * power role. > + */ > + int ret; > + bool sink_path = false; > + > + if (val->intval >= 0) { > + sink_path = true; > + if (!con->typec_cap.ops || !con->typec_cap.ops->pr_set) > + return -EINVAL; > + > + ret = con->typec_cap.ops->pr_set(con->port, TYPEC_SINK); Should there be a call to pr_set(TYPEC_SOURCE) if the value is negative? > + if (ret < 0) > + return ret; > + } > + > + return ucsi_set_sink_path(con, sink_path); You are calling SET_SINK_PATH uncoditionally, while this command was not defined for UCSI 1.x. Also it is an error to call it when device is in power source mode or if there is no partner connected. Last, but not least, the property value survives between PSY reconnects (which is expected), however after reconnecting the partner device, the value won't be reprogrammed by the UCSI driver, resulting in the inconsistency between the sysfs and actual system state. > +} > + > +static int ucsi_psy_set_prop(struct power_supply *psy, > + enum power_supply_property psp, > + const union power_supply_propval *val) > +{ > + struct ucsi_connector *con = power_supply_get_drvdata(psy); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX: > + return ucsi_psy_set_charge_control_limit_max(con, val); > default: > return -EINVAL; > } > } > > +static int ucsi_psy_prop_is_writeable(struct power_supply *psy, > + enum power_supply_property psp) > +{ > + return psp == POWER_SUPPLY_PROP_CHARGE_CONTROL_LIMIT_MAX; > +} > + > static enum power_supply_usb_type ucsi_psy_usb_types[] = { > POWER_SUPPLY_USB_TYPE_C, > POWER_SUPPLY_USB_TYPE_PD, > @@ -308,6 +358,8 @@ int ucsi_register_port_psy(struct ucsi_connector *con) > con->psy_desc.properties = ucsi_psy_props; > con->psy_desc.num_properties = ARRAY_SIZE(ucsi_psy_props); > con->psy_desc.get_property = ucsi_psy_get_prop; > + con->psy_desc.set_property = ucsi_psy_set_prop; > + con->psy_desc.property_is_writeable = ucsi_psy_prop_is_writeable; > > con->psy = power_supply_register(dev, &con->psy_desc, &psy_cfg); > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index dcd3765cc1f5..02663fdebdd9 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -1545,6 +1545,21 @@ static const struct typec_operations ucsi_ops = { > .pr_set = ucsi_pr_swap > }; > > +int ucsi_set_sink_path(struct ucsi_connector *con, bool sink_path) > +{ > + struct ucsi *ucsi = con->ucsi; > + u64 command; > + int ret; > + > + command = UCSI_SET_SINK_PATH | UCSI_CONNECTOR_NUMBER(con->num); > + command |= UCSI_SET_SINK_PATH_SINK_PATH(sink_path); > + ret = ucsi_send_command(ucsi, command, NULL, 0); > + if (ret < 0) > + dev_err(con->ucsi->dev, "SET_SINK_PATH failed (%d)\n", ret); > + > + return ret; > +} > + > /* Caller must call fwnode_handle_put() after use */ > static struct fwnode_handle *ucsi_find_fwnode(struct ucsi_connector *con) > { > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index 57129f3c0814..6a958eac5703 100644 > --- a/drivers/usb/typec/ucsi/ucsi.h > +++ b/drivers/usb/typec/ucsi/ucsi.h > @@ -114,6 +114,7 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); > #define UCSI_GET_CONNECTOR_STATUS 0x12 > #define UCSI_GET_ERROR_STATUS 0x13 > #define UCSI_GET_PD_MESSAGE 0x15 > +#define UCSI_SET_SINK_PATH 0x1c > > #define UCSI_CONNECTOR_NUMBER(_num_) ((u64)(_num_) << 16) > #define UCSI_COMMAND(_cmd_) ((_cmd_) & 0xff) > @@ -187,6 +188,9 @@ void ucsi_connector_change(struct ucsi *ucsi, u8 num); > #define UCSI_GET_PD_MESSAGE_TYPE_IDENTITY 4 > #define UCSI_GET_PD_MESSAGE_TYPE_REVISION 5 > > +/* SET_SINK_PATH command bits */ > +#define UCSI_SET_SINK_PATH_SINK_PATH(_r_) (((_r_) ? 1 : 0) << 23) > + > /* -------------------------------------------------------------------------- */ > > /* Error information returned by PPM in response to GET_ERROR_STATUS command. */ > @@ -492,6 +496,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command, > > void ucsi_altmode_update_active(struct ucsi_connector *con); > int ucsi_resume(struct ucsi *ucsi); > +int ucsi_set_sink_path(struct ucsi_connector *con, bool sink_path); > > void ucsi_notify_common(struct ucsi *ucsi, u32 cci); > int ucsi_sync_control_common(struct ucsi *ucsi, u64 command); > -- > 2.45.2.1089.g2a221341d9-goog >
Hi Dmitry, thank you for your feedback on this patch. The intention here is to allow power source selection through the UCSI driver. The existing typec operation wouldn't work here because setting the power roles alone won't set the charge source. That's also why there is no pr_set(TYPEC_SOURCE) call for a negative value. It should disable charging from that port, but it doesn't need to change the power role. But I take your point that writing positive/negative values to charge_control_limit_max is not an intuitive way to enable this functionality. Thanks for catching this issues with UCSI version and inconsistencies between sysfs and system state. I need to revisit the design here. I'll remove this patch from the V3 series and take another look at how we could implement power source selection. Thanks, Jameson
Hi, On Thu, Jul 25, 2024 at 06:31:00AM GMT, Dmitry Baryshkov wrote: > On Wed, Jul 24, 2024 at 08:11:13PM GMT, Jameson Thies wrote: > > Add status to UCSI power supply driver properties based on the port's > > connection and power direction states. > > > > Signed-off-by: Jameson Thies <jthies@google.com> > > Please CC Power Supply maintainers for this patchset (added to cc). Thanks. I guess I should add something like this to MAINTAINERS considering the power-supply bits are in its own file for UCSI: UCSI POWER-SUPPLY API R: Sebastian Reichel <sre@kernel.org> L: linux-pm@vger.kernel.org F: drivers/usb/typec/ucsi/psy.c > At least per the Documentation/ABI/testing/sysfs-class-power, the status > property applies to batteries, while UCSI psy device is a charger. This > is logical, as there might be multiple reasons why the battery is not > being charging even when the supply is online. Correct. These status bits are not meant for chargers. E.g. POWER_SUPPLY_STATUS_NOT_CHARGING has a very specific meaning, that a battery is neither charged nor discharged. Since the device is running that can only happen when a charger is connected, but not charging (or the device has two batteries). For AC the power-supply API has been designed only taking the SINK role into account. At some point USB was added and some time later people added reverse mode to their USB chargers for OTG mode. So far this has been handled by registering a regulator and using that to switch the mode. This made sense for OTG, but USB-C PD makes things even more complex. I am open to ideas how to properly handle the source role in the power-supply API, but let's not overload the status property for it. Please make sure to CC me on any follow-up series. -- Sebastian > > > --- > > Changes in V2: > > - None. > > > > drivers/usb/typec/ucsi/psy.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c > > index e623d80e177c..d0b52cee41d2 100644 > > --- a/drivers/usb/typec/ucsi/psy.c > > +++ b/drivers/usb/typec/ucsi/psy.c > > @@ -29,6 +29,7 @@ static enum power_supply_property ucsi_psy_props[] = { > > POWER_SUPPLY_PROP_CURRENT_MAX, > > POWER_SUPPLY_PROP_CURRENT_NOW, > > POWER_SUPPLY_PROP_SCOPE, > > + POWER_SUPPLY_PROP_STATUS, > > }; > > > > static int ucsi_psy_get_scope(struct ucsi_connector *con, > > @@ -51,6 +52,20 @@ static int ucsi_psy_get_scope(struct ucsi_connector *con, > > return 0; > > } > > > > +static int ucsi_psy_get_status(struct ucsi_connector *con, > > + union power_supply_propval *val) > > +{ > > + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING; > > + if (con->status.flags & UCSI_CONSTAT_CONNECTED) { > > + if ((con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK) > > + val->intval = POWER_SUPPLY_STATUS_CHARGING; > > + else > > + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; > > + } > > + > > + return 0; > > +} > > + > > static int ucsi_psy_get_online(struct ucsi_connector *con, > > union power_supply_propval *val) > > { > > @@ -249,6 +264,8 @@ static int ucsi_psy_get_prop(struct power_supply *psy, > > return ucsi_psy_get_current_now(con, val); > > case POWER_SUPPLY_PROP_SCOPE: > > return ucsi_psy_get_scope(con, val); > > + case POWER_SUPPLY_PROP_STATUS: > > + return ucsi_psy_get_status(con, val); > > default: > > return -EINVAL; > > } > > -- > > 2.45.2.1089.g2a221341d9-goog > > > > -- > With best wishes > Dmitry
On Fri, Jul 26, 2024 at 11:30:37PM GMT, Sebastian Reichel wrote: > Hi, > > On Thu, Jul 25, 2024 at 06:31:00AM GMT, Dmitry Baryshkov wrote: > > On Wed, Jul 24, 2024 at 08:11:13PM GMT, Jameson Thies wrote: > > > Add status to UCSI power supply driver properties based on the port's > > > connection and power direction states. > > > > > > Signed-off-by: Jameson Thies <jthies@google.com> > > > > Please CC Power Supply maintainers for this patchset (added to cc). > > Thanks. I guess I should add something like this to MAINTAINERS > considering the power-supply bits are in its own file for UCSI: > > UCSI POWER-SUPPLY API > R: Sebastian Reichel <sre@kernel.org> > L: linux-pm@vger.kernel.org > F: drivers/usb/typec/ucsi/psy.c Or maybe extract a separate USB PD PSY driver, which can get used by other Type-C port managers (I didn't evalue if it makes sense, i.e. if the TCPM drivers can provide data, I just assume they can).