Hi Jack,
Thanks for taking a look at the patch.
On Fri, Sep 3, 2021 at 11:05 AM Jack Pham <jackp@xxxxxxxxxxxxxx> wrote:
>
> On Thu, Sep 02, 2021 at 11:47:01PM -0700, Jack Pham wrote:
> > Hi Prashant,
> >
> > On Thu, Sep 02, 2021 at 02:34:58PM -0700, Prashant Malani wrote:
> > > Increase the max number of PDO objects to 13, to accommodate the extra
> > > PDOs added as a part of EPR (Extended Power Range) operation introduced
> > > in the USB PD Spec Rev 3.1, v 1.0. See Figure 6-54 for details.
> > >
> > > Signed-off-by: Prashant Malani <pmalani@xxxxxxxxxxxx>
> > > ---
> > > include/linux/usb/pd.h | 8 +++++++-
> > > 1 file changed, 7 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/usb/pd.h b/include/linux/usb/pd.h
> > > index 96b7ff66f074..7e8bdca1ce6e 100644
> > > --- a/include/linux/usb/pd.h
> > > +++ b/include/linux/usb/pd.h
> > > @@ -201,7 +201,13 @@ struct pd_message {
> > > } __packed;
> > >
> > > /* PDO: Power Data Object */
> > > -#define PDO_MAX_OBJECTS 7
> > > +
> > > +/*
> > > + * The EPR (Extended Power Range) structure is a superset of the SPR (Standard Power Range)
> > > + * capabilities structure, so set the max number of PDOs to 13 instead of 7. On SPR-only systems,
> > > + * objects 8 through 13 will just be empty.
> > > + */
> > > +#define PDO_MAX_OBJECTS 13
> >
> > Hmm this might break the recent change I made to UCSI in commit
> > 1f4642b72be7 ("usb: typec: ucsi: Retrieve all the PDOs instead of just
> > the first 4").
> >
> > 520 static void ucsi_get_src_pdos(struct ucsi_connector *con, int is_partner)
> > 521 {
> > 522 int ret;
> > 523
> > 524 /* UCSI max payload means only getting at most 4 PDOs at a time */
> > 525 ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
> > 526 if (ret < 0)
> > 527 return;
> > 528
> > 529 con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> > 530 if (con->num_pdos < UCSI_MAX_PDOS)
> > 531 return;
> > 532
> > 533 /* get the remaining PDOs, if any */
> > 534 ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
> > 535 PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> > ^^^^^^^^^^^^^^^
> > This routine calls the UCSI GET_PDOS command for up to 4 PDOs at a time
> > since that's the most the return payload can carry. Currently this
> > assumes that we'd only need to request the PPM at most twice to retrieve
> > all the PDOs for up to a maximum of 7 (first request for 4 then again if
> > needed for the remaining 3). I'm not sure if any existing UCSI FW would
> > be updatable to support more than 7 PDOs in the future, much less
> > support EPR. In fact, current UCSI 1.2 spec [1] Table 4-34 mentions PDO
>
> Sorry, forgot the footnote with the link to the spec:
> [1] https://www.intel.com/content/dam/www/public/us/en/documents/technical-specifications/usb-type-c-ucsi-spec.pdf
>
> > offset valid values are 0-7 and anything else "shall not be used", so I
> > don't know how UCSI will eventually cope with EPR without a spec update.
> >
> > So if this macro changes to 13 then this call would result in a call to
> > the UCSI GET_PDOS command passing num_pdos == 13-4 = 9 which would
> > probably result in an error from the PPM FW. So we might need to retain
> > the maximum value of 7 PDOs at least for UCSI here. Maybe that means
> > this UCSI driver needs to carry its own definition of
> > UCSI_MAX_TOTAL_PDOS=7 instead of using PDO_MAX_OBJECTS?
Thanks for pointing this out. We can perhaps just add another macro
for EPR_PDO_MAX_OBJECTS, and leave the current macro as is for now.
Best regards,
