[PATCH] usb: typec: ucsi: Only get source PDOs from the actual source

[PATCH] usb: typec: ucsi: Only get source PDOs from the actual source

[Jack Pham]

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(-)

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;
-- 
2.24.0

Re: [PATCH] usb: typec: ucsi: Only get source PDOs from the actual source

[Heikki Krogerus]

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,

-- 
heikki

Re: [PATCH] usb: typec: ucsi: Only get source PDOs from the actual source

[Jack Pham]

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

Re: [PATCH] usb: typec: ucsi: Only get source PDOs from the actual source

[Heikki Krogerus]

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,

-- 
heikki