On Tue, 24 Oct 2023 at 19:42, Bjorn Andersson <quic_bjorande@xxxxxxxxxxx> wrote:
>
> On Tue, Oct 24, 2023 at 02:08:33AM +0300, Dmitry Baryshkov wrote:
> > On Tue, 24 Oct 2023 at 01:47, Bjorn Andersson <quic_bjorande@xxxxxxxxxxx> wrote:
> > >
> > > On Tue, Oct 24, 2023 at 12:47:26AM +0300, Dmitry Baryshkov wrote:
> > > > On sevral Qualcomm platforms (SC8180X, SM8350, SC8280XP) a call to
> > > > UCSI_GET_PDOS for non-PD partners will cause a firmware crash with no
> > > > easy way to recover from it. Since we have no easy way to determine
> > > > whether the partner really has PD support, shortcut UCSI_GET_PDOS on
> > > > such platforms. This allows us to enable UCSI support on such devices.
> > > >
> > >
> > > Really nice to see this. Thanks.
> > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > > ---
> > > > drivers/usb/typec/ucsi/ucsi.c | 3 +++
> > > > drivers/usb/typec/ucsi/ucsi.h | 3 +++
> > > > drivers/usb/typec/ucsi/ucsi_glink.c | 3 +++
> > > > 3 files changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> > > > index 61b64558f96c..5392ec698959 100644
> > > > --- a/drivers/usb/typec/ucsi/ucsi.c
> > > > +++ b/drivers/usb/typec/ucsi/ucsi.c
> > > > @@ -578,6 +578,9 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
> > > > u64 command;
> > > > int ret;
> > > >
> > > > + if (ucsi->quirks & UCSI_NO_PARTNER_PDOS)
> > > > + return 0;
> > > > +
> > > > command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);
> > > > command |= UCSI_GET_PDOS_PARTNER_PDO(is_partner);
> > > > command |= UCSI_GET_PDOS_PDO_OFFSET(offset);
> > > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> > > > index 474315a72c77..6478016d5cb8 100644
> > > > --- a/drivers/usb/typec/ucsi/ucsi.h
> > > > +++ b/drivers/usb/typec/ucsi/ucsi.h
> > > > @@ -317,6 +317,9 @@ struct ucsi {
> > > > #define EVENT_PENDING 0
> > > > #define COMMAND_PENDING 1
> > > > #define ACK_PENDING 2
> > > > +
> > > > + unsigned long quirks;
> > > > +#define UCSI_NO_PARTNER_PDOS BIT(0) /* Don't read partner's PDOs */
> > > > };
> > > >
> > > > #define UCSI_MAX_SVID 5
> > > > diff --git a/drivers/usb/typec/ucsi/ucsi_glink.c b/drivers/usb/typec/ucsi/ucsi_glink.c
> > > > index db6e248f8208..5c159e7b2b65 100644
> > > > --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> > > > +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> > > > @@ -327,6 +327,8 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
> > > > if (ret)
> > > > return ret;
> > > >
> > > > + ucsi->ucsi->quirks = id->driver_data;
> > > > +
> > > > ucsi_set_drvdata(ucsi->ucsi, ucsi);
> > > >
> > > > device_for_each_child_node(dev, fwnode) {
> > > > @@ -379,6 +381,7 @@ static void pmic_glink_ucsi_remove(struct auxiliary_device *adev)
> > > >
> > > > static const struct auxiliary_device_id pmic_glink_ucsi_id_table[] = {
> > > > { .name = "pmic_glink.ucsi", },
> > > > + { .name = "pmic_glink.ucsi-no-pdos", .driver_data = UCSI_NO_PARTNER_PDOS, },
> > >
> > > In altmode and battmgr drivers we apply quirks based on the compatible
> > > of the pmic_glink of_node.
> >
> > ... and I can't say that I like that. In typical drivers we perform
> > driver tuning by looking at the device's data (e.g. by using
> > of_device_is_compatible or by of_device_get_match_data. Checking the
> > parent device seems like breaking the layering.
>
> It felt like it was the cleaner option of the two when I did it. I think
> there was some variation of quirks which made me feel this would grow
> large - but I might misremember things now.
>
> > But if you insist, I can follow that approach.
>
> I insist that we should use the same mechanism of dealing with the
> quirks across the three parts, and following the existing approach
> doesn't seem too unreasonable...
The problem with the current approach is that it adds dependency
between patches. We can not apply patch2 without patch1 being in
place, since applying will enable buggy UCSI.
--
With best wishes
Dmitry
