| Message ID | 20210115163311.391332-1-kyletso@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f75a1025c0b94d58de56c9421104ed6e987ddcd2 |
| Headers | show |
| Series | usb: typec: tcpm: Create legacy PDOs for PD2 connection | expand |
Hi Kyle, On Sat, Jan 16, 2021 at 12:33:11AM +0800, Kyle Tso wrote: > If the port partner is PD2, the PDOs of the local port should follow the > format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs > and transform them into PD2 format before sending them to the PD2 port > partner. I guess it's not possible for the system to supply separate static PDOs for each PD revision? > Signed-off-by: Kyle Tso <kyletso@google.com> > --- > drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------ > include/linux/usb/pd.h | 1 + > 2 files changed, 53 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 22a85b396f69..1220ab1ed47d 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role) > return 0; > } > > +/* > + * Transform the PDO to be compliant to PD rev2.0. > + * Return 0 if the PDO type is not defined in PD rev2.0. > + * Otherwise, return the converted PDO. > + */ > +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role) > +{ > + switch (pdo_type(pdo)) { > + case PDO_TYPE_FIXED: > + if (role == TYPEC_SINK) > + return pdo & ~PDO_FIXED_FRS_CURR_MASK; > + else > + return pdo & ~PDO_FIXED_UNCHUNK_EXT; > + case PDO_TYPE_VAR: > + case PDO_TYPE_BATT: > + return pdo; > + case PDO_TYPE_APDO: > + default: > + return 0; > + } > +} > + > static int tcpm_pd_send_source_caps(struct tcpm_port *port) > { > struct pd_message msg; > - int i; > + u32 pdo; > + unsigned int i, nr_pdo = 0; Side note. I think this driver uses the "reverse christmas tree" style with the variables. > memset(&msg, 0, sizeof(msg)); > - if (!port->nr_src_pdo) { > + > + for (i = 0; i < port->nr_src_pdo; i++) { > + if (port->negotiated_rev >= PD_REV30) { > + msg.payload[nr_pdo++] = cpu_to_le32(port->src_pdo[i]); > + } else { > + pdo = tcpm_forge_legacy_pdo(port, port->src_pdo[i], TYPEC_SOURCE); > + if (pdo) > + msg.payload[nr_pdo++] = cpu_to_le32(pdo); > + } > + } Why not just generate the PD_REV20 PDOs during probe time and then just use the ones that fit the negotiated_rev here? > + > + if (!nr_pdo) { > /* No source capabilities defined, sink only */ > msg.header = PD_HEADER_LE(PD_CTRL_REJECT, > port->pwr_role, > @@ -930,10 +964,8 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port) > port->data_role, > port->negotiated_rev, > port->message_id, > - port->nr_src_pdo); > + nr_pdo); > } > - for (i = 0; i < port->nr_src_pdo; i++) > - msg.payload[i] = cpu_to_le32(port->src_pdo[i]); > > return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg); > } > @@ -941,10 +973,22 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port) > static int tcpm_pd_send_sink_caps(struct tcpm_port *port) > { > struct pd_message msg; > - int i; > + u32 pdo; > + unsigned int i, nr_pdo = 0; > > memset(&msg, 0, sizeof(msg)); > - if (!port->nr_snk_pdo) { > + > + for (i = 0; i < port->nr_snk_pdo; i++) { > + if (port->negotiated_rev >= PD_REV30) { > + msg.payload[nr_pdo++] = cpu_to_le32(port->snk_pdo[i]); > + } else { > + pdo = tcpm_forge_legacy_pdo(port, port->snk_pdo[i], TYPEC_SINK); > + if (pdo) > + msg.payload[nr_pdo++] = cpu_to_le32(pdo); > + } > + } ditto. > + if (!nr_pdo) { > /* No sink capabilities defined, source only */ > msg.header = PD_HEADER_LE(PD_CTRL_REJECT, > port->pwr_role, > @@ -957,10 +1001,8 @@ static int tcpm_pd_send_sink_caps(struct tcpm_port *port) > port->data_role, > port->negotiated_rev, > port->message_id, > - port->nr_snk_pdo); > + nr_pdo); > } > - for (i = 0; i < port->nr_snk_pdo; i++) > - msg.payload[i] = cpu_to_le32(port->snk_pdo[i]); > > return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg); > } > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h > index bb9a782e1411..88f64bce5dea 100644 > --- a/include/linux/usb/pd.h > +++ b/include/linux/usb/pd.h > @@ -225,6 +225,7 @@ enum pd_pdo_type { > #define PDO_FIXED_EXTPOWER BIT(27) /* Externally powered */ > #define PDO_FIXED_USB_COMM BIT(26) /* USB communications capable */ > #define PDO_FIXED_DATA_SWAP BIT(25) /* Data role swap supported */ > +#define PDO_FIXED_UNCHUNK_EXT BIT(24) /* Unchunked Extended Message supported (Source) */ > #define PDO_FIXED_FRS_CURR_MASK (BIT(24) | BIT(23)) /* FR_Swap Current (Sink) */ > #define PDO_FIXED_FRS_CURR_SHIFT 23 > #define PDO_FIXED_VOLT_SHIFT 10 /* 50mV units */ thanks,
On Thu, Jan 21, 2021 at 4:41 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > Hi Kyle, > > On Sat, Jan 16, 2021 at 12:33:11AM +0800, Kyle Tso wrote: > > If the port partner is PD2, the PDOs of the local port should follow the > > format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs > > and transform them into PD2 format before sending them to the PD2 port > > partner. > > I guess it's not possible for the system to supply separate static > PDOs for each PD revision? > We can do that for sure. But a problem is that if there are more PD revisions in the future, we will need to add more PDO arrays. For backward compatibility, the new revision usually uses the previously-reserved bits for the new features. From my perspective, the better way to achieve the backward compatibility is to just clear the bits if those are reserved in the previous revision. I can submit another patch which adds another PDO array for PD2 if you think it is more appropriate. > > Signed-off-by: Kyle Tso <kyletso@google.com> > > --- > > drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------ > > include/linux/usb/pd.h | 1 + > > 2 files changed, 53 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > index 22a85b396f69..1220ab1ed47d 100644 > > --- a/drivers/usb/typec/tcpm/tcpm.c > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role) > > return 0; > > } > > > > +/* > > + * Transform the PDO to be compliant to PD rev2.0. > > + * Return 0 if the PDO type is not defined in PD rev2.0. > > + * Otherwise, return the converted PDO. > > + */ > > +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role) > > +{ > > + switch (pdo_type(pdo)) { > > + case PDO_TYPE_FIXED: > > + if (role == TYPEC_SINK) > > + return pdo & ~PDO_FIXED_FRS_CURR_MASK; > > + else > > + return pdo & ~PDO_FIXED_UNCHUNK_EXT; > > + case PDO_TYPE_VAR: > > + case PDO_TYPE_BATT: > > + return pdo; > > + case PDO_TYPE_APDO: > > + default: > > + return 0; > > + } > > +} > > + > > static int tcpm_pd_send_source_caps(struct tcpm_port *port) > > { > > struct pd_message msg; > > - int i; > > + u32 pdo; > > + unsigned int i, nr_pdo = 0; > > Side note. I think this driver uses the "reverse christmas tree" > style with the variables. I will change the order (if there is a next version) thanks, Kyle
On 1/15/21 8:33 AM, Kyle Tso wrote: > If the port partner is PD2, the PDOs of the local port should follow the > format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs > and transform them into PD2 format before sending them to the PD2 port > partner. > > Signed-off-by: Kyle Tso <kyletso@google.com> I don't like this too much, but I don't have a better idea. Thus, Reviewed-by: Guenter Roeck <linux@roeckus.net> Guenter > --- > drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------ > include/linux/usb/pd.h | 1 + > 2 files changed, 53 insertions(+), 10 deletions(-) > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > index 22a85b396f69..1220ab1ed47d 100644 > --- a/drivers/usb/typec/tcpm/tcpm.c > +++ b/drivers/usb/typec/tcpm/tcpm.c > @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role) > return 0; > } > > +/* > + * Transform the PDO to be compliant to PD rev2.0. > + * Return 0 if the PDO type is not defined in PD rev2.0. > + * Otherwise, return the converted PDO. > + */ > +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role) > +{ > + switch (pdo_type(pdo)) { > + case PDO_TYPE_FIXED: > + if (role == TYPEC_SINK) > + return pdo & ~PDO_FIXED_FRS_CURR_MASK; > + else > + return pdo & ~PDO_FIXED_UNCHUNK_EXT; > + case PDO_TYPE_VAR: > + case PDO_TYPE_BATT: > + return pdo; > + case PDO_TYPE_APDO: > + default: > + return 0; > + } > +} > + > static int tcpm_pd_send_source_caps(struct tcpm_port *port) > { > struct pd_message msg; > - int i; > + u32 pdo; > + unsigned int i, nr_pdo = 0; > > memset(&msg, 0, sizeof(msg)); > - if (!port->nr_src_pdo) { > + > + for (i = 0; i < port->nr_src_pdo; i++) { > + if (port->negotiated_rev >= PD_REV30) { > + msg.payload[nr_pdo++] = cpu_to_le32(port->src_pdo[i]); > + } else { > + pdo = tcpm_forge_legacy_pdo(port, port->src_pdo[i], TYPEC_SOURCE); > + if (pdo) > + msg.payload[nr_pdo++] = cpu_to_le32(pdo); > + } > + } > + > + if (!nr_pdo) { > /* No source capabilities defined, sink only */ > msg.header = PD_HEADER_LE(PD_CTRL_REJECT, > port->pwr_role, > @@ -930,10 +964,8 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port) > port->data_role, > port->negotiated_rev, > port->message_id, > - port->nr_src_pdo); > + nr_pdo); > } > - for (i = 0; i < port->nr_src_pdo; i++) > - msg.payload[i] = cpu_to_le32(port->src_pdo[i]); > > return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg); > } > @@ -941,10 +973,22 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port) > static int tcpm_pd_send_sink_caps(struct tcpm_port *port) > { > struct pd_message msg; > - int i; > + u32 pdo; > + unsigned int i, nr_pdo = 0; > > memset(&msg, 0, sizeof(msg)); > - if (!port->nr_snk_pdo) { > + > + for (i = 0; i < port->nr_snk_pdo; i++) { > + if (port->negotiated_rev >= PD_REV30) { > + msg.payload[nr_pdo++] = cpu_to_le32(port->snk_pdo[i]); > + } else { > + pdo = tcpm_forge_legacy_pdo(port, port->snk_pdo[i], TYPEC_SINK); > + if (pdo) > + msg.payload[nr_pdo++] = cpu_to_le32(pdo); > + } > + } > + > + if (!nr_pdo) { > /* No sink capabilities defined, source only */ > msg.header = PD_HEADER_LE(PD_CTRL_REJECT, > port->pwr_role, > @@ -957,10 +1001,8 @@ static int tcpm_pd_send_sink_caps(struct tcpm_port *port) > port->data_role, > port->negotiated_rev, > port->message_id, > - port->nr_snk_pdo); > + nr_pdo); > } > - for (i = 0; i < port->nr_snk_pdo; i++) > - msg.payload[i] = cpu_to_le32(port->snk_pdo[i]); > > return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg); > } > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h > index bb9a782e1411..88f64bce5dea 100644 > --- a/include/linux/usb/pd.h > +++ b/include/linux/usb/pd.h > @@ -225,6 +225,7 @@ enum pd_pdo_type { > #define PDO_FIXED_EXTPOWER BIT(27) /* Externally powered */ > #define PDO_FIXED_USB_COMM BIT(26) /* USB communications capable */ > #define PDO_FIXED_DATA_SWAP BIT(25) /* Data role swap supported */ > +#define PDO_FIXED_UNCHUNK_EXT BIT(24) /* Unchunked Extended Message supported (Source) */ > #define PDO_FIXED_FRS_CURR_MASK (BIT(24) | BIT(23)) /* FR_Swap Current (Sink) */ > #define PDO_FIXED_FRS_CURR_SHIFT 23 > #define PDO_FIXED_VOLT_SHIFT 10 /* 50mV units */ >
Hi, On Thu, Jan 21, 2021 at 05:48:46PM +0800, Kyle Tso wrote: > On Thu, Jan 21, 2021 at 4:41 PM Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > > > Hi Kyle, > > > > On Sat, Jan 16, 2021 at 12:33:11AM +0800, Kyle Tso wrote: > > > If the port partner is PD2, the PDOs of the local port should follow the > > > format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs > > > and transform them into PD2 format before sending them to the PD2 port > > > partner. > > > > I guess it's not possible for the system to supply separate static > > PDOs for each PD revision? > > > We can do that for sure. But a problem is that if there are more PD > revisions in the future, we will need to add more PDO arrays. > For backward compatibility, the new revision usually uses the > previously-reserved bits for the new features. > >From my perspective, the better way to achieve the backward > compatibility is to just clear the bits if those are reserved in the > previous revision. I was trying to think of something better for this, but I got nothing. I'm not completely comfortable with this, but never mind. Let's just go with this. > I can submit another patch which adds another PDO array for PD2 if you > think it is more appropriate. Not necessary. Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > Signed-off-by: Kyle Tso <kyletso@google.com> > > > --- > > > drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------ > > > include/linux/usb/pd.h | 1 + > > > 2 files changed, 53 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c > > > index 22a85b396f69..1220ab1ed47d 100644 > > > --- a/drivers/usb/typec/tcpm/tcpm.c > > > +++ b/drivers/usb/typec/tcpm/tcpm.c > > > @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role) > > > return 0; > > > } > > > > > > +/* > > > + * Transform the PDO to be compliant to PD rev2.0. > > > + * Return 0 if the PDO type is not defined in PD rev2.0. > > > + * Otherwise, return the converted PDO. > > > + */ > > > +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role) > > > +{ > > > + switch (pdo_type(pdo)) { > > > + case PDO_TYPE_FIXED: > > > + if (role == TYPEC_SINK) > > > + return pdo & ~PDO_FIXED_FRS_CURR_MASK; > > > + else > > > + return pdo & ~PDO_FIXED_UNCHUNK_EXT; > > > + case PDO_TYPE_VAR: > > > + case PDO_TYPE_BATT: > > > + return pdo; > > > + case PDO_TYPE_APDO: > > > + default: > > > + return 0; > > > + } > > > +} > > > + > > > static int tcpm_pd_send_source_caps(struct tcpm_port *port) > > > { > > > struct pd_message msg; > > > - int i; > > > + u32 pdo; > > > + unsigned int i, nr_pdo = 0; > > > > Side note. I think this driver uses the "reverse christmas tree" > > style with the variables. > > I will change the order (if there is a next version) > > thanks, > Kyle
diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c index 22a85b396f69..1220ab1ed47d 100644 --- a/drivers/usb/typec/tcpm/tcpm.c +++ b/drivers/usb/typec/tcpm/tcpm.c @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role) return 0; } +/* + * Transform the PDO to be compliant to PD rev2.0. + * Return 0 if the PDO type is not defined in PD rev2.0. + * Otherwise, return the converted PDO. + */ +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role) +{ + switch (pdo_type(pdo)) { + case PDO_TYPE_FIXED: + if (role == TYPEC_SINK) + return pdo & ~PDO_FIXED_FRS_CURR_MASK; + else + return pdo & ~PDO_FIXED_UNCHUNK_EXT; + case PDO_TYPE_VAR: + case PDO_TYPE_BATT: + return pdo; + case PDO_TYPE_APDO: + default: + return 0; + } +} + static int tcpm_pd_send_source_caps(struct tcpm_port *port) { struct pd_message msg; - int i; + u32 pdo; + unsigned int i, nr_pdo = 0; memset(&msg, 0, sizeof(msg)); - if (!port->nr_src_pdo) { + + for (i = 0; i < port->nr_src_pdo; i++) { + if (port->negotiated_rev >= PD_REV30) { + msg.payload[nr_pdo++] = cpu_to_le32(port->src_pdo[i]); + } else { + pdo = tcpm_forge_legacy_pdo(port, port->src_pdo[i], TYPEC_SOURCE); + if (pdo) + msg.payload[nr_pdo++] = cpu_to_le32(pdo); + } + } + + if (!nr_pdo) { /* No source capabilities defined, sink only */ msg.header = PD_HEADER_LE(PD_CTRL_REJECT, port->pwr_role, @@ -930,10 +964,8 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port) port->data_role, port->negotiated_rev, port->message_id, - port->nr_src_pdo); + nr_pdo); } - for (i = 0; i < port->nr_src_pdo; i++) - msg.payload[i] = cpu_to_le32(port->src_pdo[i]); return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg); } @@ -941,10 +973,22 @@ static int tcpm_pd_send_source_caps(struct tcpm_port *port) static int tcpm_pd_send_sink_caps(struct tcpm_port *port) { struct pd_message msg; - int i; + u32 pdo; + unsigned int i, nr_pdo = 0; memset(&msg, 0, sizeof(msg)); - if (!port->nr_snk_pdo) { + + for (i = 0; i < port->nr_snk_pdo; i++) { + if (port->negotiated_rev >= PD_REV30) { + msg.payload[nr_pdo++] = cpu_to_le32(port->snk_pdo[i]); + } else { + pdo = tcpm_forge_legacy_pdo(port, port->snk_pdo[i], TYPEC_SINK); + if (pdo) + msg.payload[nr_pdo++] = cpu_to_le32(pdo); + } + } + + if (!nr_pdo) { /* No sink capabilities defined, source only */ msg.header = PD_HEADER_LE(PD_CTRL_REJECT, port->pwr_role, @@ -957,10 +1001,8 @@ static int tcpm_pd_send_sink_caps(struct tcpm_port *port) port->data_role, port->negotiated_rev, port->message_id, - port->nr_snk_pdo); + nr_pdo); } - for (i = 0; i < port->nr_snk_pdo; i++) - msg.payload[i] = cpu_to_le32(port->snk_pdo[i]); return tcpm_pd_transmit(port, TCPC_TX_SOP, &msg); } diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h index bb9a782e1411..88f64bce5dea 100644 --- a/include/linux/usb/pd.h +++ b/include/linux/usb/pd.h @@ -225,6 +225,7 @@ enum pd_pdo_type { #define PDO_FIXED_EXTPOWER BIT(27) /* Externally powered */ #define PDO_FIXED_USB_COMM BIT(26) /* USB communications capable */ #define PDO_FIXED_DATA_SWAP BIT(25) /* Data role swap supported */ +#define PDO_FIXED_UNCHUNK_EXT BIT(24) /* Unchunked Extended Message supported (Source) */ #define PDO_FIXED_FRS_CURR_MASK (BIT(24) | BIT(23)) /* FR_Swap Current (Sink) */ #define PDO_FIXED_FRS_CURR_SHIFT 23 #define PDO_FIXED_VOLT_SHIFT 10 /* 50mV units */
If the port partner is PD2, the PDOs of the local port should follow the format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs and transform them into PD2 format before sending them to the PD2 port partner. Signed-off-by: Kyle Tso <kyletso@google.com> --- drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------ include/linux/usb/pd.h | 1 + 2 files changed, 53 insertions(+), 10 deletions(-)