| Message ID | 20211027064842.6901-1-quic_jackp@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: typec: ucsi: Only get source PDOs from the actual source | expand |
On Tue, Oct 26, 2021 at 11:48:42PM -0700, Jack Pham wrote: > The intent of ucsi_get_src_pdos() is to obtain the source's PDOs > in order to provide the power_supply object the required data to > report the mininum, maximum and currently operating voltage & > current should a PD contract be in place. > > If however, the port is operating as a PD source, this call would > invoke GET_PDOS on the partner causing the PPM to send a > Get_Source_Caps PD message to the port partner which may not make > sense especially if the partner is not a dual-power role capable > device. Further, it has been observed that certain DisplayPort > adapter cables (which are power sink-only devices) even fail to > bring up the display link after receiving a Get_Source_Caps > message, suggesting they can't cope well with an unsupported PD > message to the point that it renders them functionally inoperable. > > Fix this by checking the connector status flags for the power > direction and use this to decide whether to send the GET_PDOs > query to the partner or the port. This also helps to make the > power_supply VOLTAGE_{MIN,MAX,NOW} and CURRENT_{MAX,NOW} > properties more consistent when the port is in source mode. > > Signed-off-by: Jack Pham <quic_jackp@quicinc.com> > --- > Hi Heikki, > > Was wrestling with how exactly to do this. The other approach I was > thinking was to not even do GET_PDOs at all if operating as a source, > but that would also mean we'd need to add similar checking to the > VOLTAGE/CURRENT property getters in psy.c so that they would not > return incorrect/stale data. Since the ONLINE property will already > be 0 anyway it may make more sense to invalidate the rest of the props? > > The patch below is concise though...so that's what I went with ;) Would it still make sense / help if we had separate power supplies registered for the port-source, port-sink, partner-source and partner-sink? I also think we need to unify these power supplies so that tcpm and ucsi (and others) always register the same power supplies. thanks,
Hi Heikki, On Thu, Oct 28, 2021 at 02:55:32PM +0300, Heikki Krogerus wrote: > On Tue, Oct 26, 2021 at 11:48:42PM -0700, Jack Pham wrote: > > The intent of ucsi_get_src_pdos() is to obtain the source's PDOs > > in order to provide the power_supply object the required data to > > report the mininum, maximum and currently operating voltage & > > current should a PD contract be in place. > > > > If however, the port is operating as a PD source, this call would > > invoke GET_PDOS on the partner causing the PPM to send a > > Get_Source_Caps PD message to the port partner which may not make > > sense especially if the partner is not a dual-power role capable > > device. Further, it has been observed that certain DisplayPort > > adapter cables (which are power sink-only devices) even fail to > > bring up the display link after receiving a Get_Source_Caps > > message, suggesting they can't cope well with an unsupported PD > > message to the point that it renders them functionally inoperable. > > > > Fix this by checking the connector status flags for the power > > direction and use this to decide whether to send the GET_PDOs > > query to the partner or the port. This also helps to make the > > power_supply VOLTAGE_{MIN,MAX,NOW} and CURRENT_{MAX,NOW} > > properties more consistent when the port is in source mode. > > > > Signed-off-by: Jack Pham <quic_jackp@quicinc.com> > > --- > > Hi Heikki, > > > > Was wrestling with how exactly to do this. The other approach I was > > thinking was to not even do GET_PDOs at all if operating as a source, > > but that would also mean we'd need to add similar checking to the > > VOLTAGE/CURRENT property getters in psy.c so that they would not > > return incorrect/stale data. Since the ONLINE property will already > > be 0 anyway it may make more sense to invalidate the rest of the props? > > > > The patch below is concise though...so that's what I went with ;) > > Would it still make sense / help if we had separate power supplies > registered for the port-source, port-sink, partner-source and > partner-sink? The name "power_supply" of the class to me implies a source :). So I can see perhaps having separate port-source and partner-source supplies making sense; they would effectively be mutually exclusive as only one of the pair would be "online" at a time when a partner is present depending on the power direction. But I'm not sure I see a use for having port-sink/partner-sink objects though. While the VOLTAGE_NOW / CURRENT_NOW properties might be redundantly reporting the same value from corresponding -source objects, would there be different MIN/MAX values which are obtained from Get_Sink_Caps? > I also think we need to unify these power supplies so that tcpm and > ucsi (and others) always register the same power supplies. I like that idea too! Thanks, Jack
On Tue, Nov 02, 2021 at 12:01:31AM -0700, Jack Pham wrote: > Hi Heikki, > > On Thu, Oct 28, 2021 at 02:55:32PM +0300, Heikki Krogerus wrote: > > On Tue, Oct 26, 2021 at 11:48:42PM -0700, Jack Pham wrote: > > > The intent of ucsi_get_src_pdos() is to obtain the source's PDOs > > > in order to provide the power_supply object the required data to > > > report the mininum, maximum and currently operating voltage & > > > current should a PD contract be in place. > > > > > > If however, the port is operating as a PD source, this call would > > > invoke GET_PDOS on the partner causing the PPM to send a > > > Get_Source_Caps PD message to the port partner which may not make > > > sense especially if the partner is not a dual-power role capable > > > device. Further, it has been observed that certain DisplayPort > > > adapter cables (which are power sink-only devices) even fail to > > > bring up the display link after receiving a Get_Source_Caps > > > message, suggesting they can't cope well with an unsupported PD > > > message to the point that it renders them functionally inoperable. > > > > > > Fix this by checking the connector status flags for the power > > > direction and use this to decide whether to send the GET_PDOs > > > query to the partner or the port. This also helps to make the > > > power_supply VOLTAGE_{MIN,MAX,NOW} and CURRENT_{MAX,NOW} > > > properties more consistent when the port is in source mode. > > > > > > Signed-off-by: Jack Pham <quic_jackp@quicinc.com> > > > --- > > > Hi Heikki, > > > > > > Was wrestling with how exactly to do this. The other approach I was > > > thinking was to not even do GET_PDOs at all if operating as a source, > > > but that would also mean we'd need to add similar checking to the > > > VOLTAGE/CURRENT property getters in psy.c so that they would not > > > return incorrect/stale data. Since the ONLINE property will already > > > be 0 anyway it may make more sense to invalidate the rest of the props? > > > > > > The patch below is concise though...so that's what I went with ;) > > > > Would it still make sense / help if we had separate power supplies > > registered for the port-source, port-sink, partner-source and > > partner-sink? > > The name "power_supply" of the class to me implies a source :). So I > can see perhaps having separate port-source and partner-source supplies > making sense; they would effectively be mutually exclusive as only one > of the pair would be "online" at a time when a partner is present > depending on the power direction. > > But I'm not sure I see a use for having port-sink/partner-sink objects > though. While the VOLTAGE_NOW / CURRENT_NOW properties might be > redundantly reporting the same value from corresponding -source objects, > would there be different MIN/MAX values which are obtained from > Get_Sink_Caps? A power-supply can be a supply, supplicant or both, and that aligns perfectly with source/sink IMO. By taking advantage of that, you could display the whole power-supply chain to the user space. So if you had a port-sink power-supply online and partner-source online - the partner supplying the port - then in reality the port-sink power-supply would/could also be the "supply" for another power-supply, for example the battery. That's the idea with the power-supply chain. Nevertheless, I guess there does not have to be separate source and sink power-supplies. If we just had separate port and partner power-supplies, we could also possibly use the "status" property to see which one is sink and which is the source (charging or discharging). Though, I don't think that would be ideal. > > I also think we need to unify these power supplies so that tcpm and > > ucsi (and others) always register the same power supplies. > > I like that idea too! thanks,
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 6aa28384f77f..406d757266ae 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -582,9 +582,13 @@ static int ucsi_get_pdos(struct ucsi_connector *con, int is_partner, static int ucsi_get_src_pdos(struct ucsi_connector *con) { int ret; + int partner; + + /* Get PDOs from whomever is the source */ + partner = (con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK; /* UCSI max payload means only getting at most 4 PDOs at a time */ - ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS); + ret = ucsi_get_pdos(con, partner, con->src_pdos, 0, UCSI_MAX_PDOS); if (ret < 0) return ret; @@ -593,7 +597,7 @@ static int ucsi_get_src_pdos(struct ucsi_connector *con) return 0; /* get the remaining PDOs, if any */ - ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS, + ret = ucsi_get_pdos(con, partner, con->src_pdos, UCSI_MAX_PDOS, PDO_MAX_OBJECTS - UCSI_MAX_PDOS); if (ret < 0) return ret;
The intent of ucsi_get_src_pdos() is to obtain the source's PDOs in order to provide the power_supply object the required data to report the mininum, maximum and currently operating voltage & current should a PD contract be in place. If however, the port is operating as a PD source, this call would invoke GET_PDOS on the partner causing the PPM to send a Get_Source_Caps PD message to the port partner which may not make sense especially if the partner is not a dual-power role capable device. Further, it has been observed that certain DisplayPort adapter cables (which are power sink-only devices) even fail to bring up the display link after receiving a Get_Source_Caps message, suggesting they can't cope well with an unsupported PD message to the point that it renders them functionally inoperable. Fix this by checking the connector status flags for the power direction and use this to decide whether to send the GET_PDOs query to the partner or the port. This also helps to make the power_supply VOLTAGE_{MIN,MAX,NOW} and CURRENT_{MAX,NOW} properties more consistent when the port is in source mode. Signed-off-by: Jack Pham <quic_jackp@quicinc.com> --- Hi Heikki, Was wrestling with how exactly to do this. The other approach I was thinking was to not even do GET_PDOs at all if operating as a source, but that would also mean we'd need to add similar checking to the VOLTAGE/CURRENT property getters in psy.c so that they would not return incorrect/stale data. Since the ONLINE property will already be 0 anyway it may make more sense to invalidate the rest of the props? The patch below is concise though...so that's what I went with ;) Jack drivers/usb/typec/ucsi/ucsi.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)