mbox series

[0/2] usb: typec: ucsi: add workaround for several Qualcomm platforms

Message ID 20231023215327.695720-1-dmitry.baryshkov@linaro.org
Headers show
Series usb: typec: ucsi: add workaround for several Qualcomm platforms | expand

Message

Dmitry Baryshkov Oct. 23, 2023, 9:47 p.m. UTC
The UCSI firmware on Qualcomm SC8180X, SC8280XP and SM8350 are buggy.
Submitting UCSI_GET_PDOS command for partners which do not actually
support PD and do not have PDOs causes firmware to crash, preventing
further UCSI activity. Firmware on newer platforms have fixed this
issue. In order to still be able to use UCSI functionality on the
mentioned platforms (e.g. to be able to handle USB role switching),
apply a workaround that completely shortcuts UCSI_GET_PDOS command for
the USB-C partner.

This has been tested on sm8350 only, but should apply to other two
platforms.

Dmitry Baryshkov (2):
  usb: typec: ucsi: fix UCSI on buggy Qualcomm devices
  soc: qcom: pmic-glink: enable UCSI on older devices

 drivers/soc/qcom/pmic_glink.c       | 7 +++++++
 drivers/usb/typec/ucsi/ucsi.c       | 3 +++
 drivers/usb/typec/ucsi/ucsi.h       | 3 +++
 drivers/usb/typec/ucsi/ucsi_glink.c | 3 +++
 4 files changed, 16 insertions(+)

Comments

Bjorn Andersson Oct. 23, 2023, 10:47 p.m. UTC | #1
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@linaro.org>
> ---
>  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.

Could we do the same here, instead of mixing the two schemes?

Regards,
Bjorn

>  	{},
>  };
>  MODULE_DEVICE_TABLE(auxiliary, pmic_glink_ucsi_id_table);
> -- 
> 2.42.0
>
Dmitry Baryshkov Oct. 23, 2023, 11:08 p.m. UTC | #2
On Tue, 24 Oct 2023 at 01:47, Bjorn Andersson <quic_bjorande@quicinc.com> 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@linaro.org>
> > ---
> >  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.
But if you insist, I can follow that approach.

>
> Could we do the same here, instead of mixing the two schemes?
>
> Regards,
> Bjorn
>
> >       {},
> >  };
> >  MODULE_DEVICE_TABLE(auxiliary, pmic_glink_ucsi_id_table);
> > --
> > 2.42.0
> >
Bjorn Andersson Oct. 24, 2023, 4:42 p.m. UTC | #3
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@quicinc.com> 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@linaro.org>
> > > ---
> > >  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...

Thanks,
Bjorn
Dmitry Baryshkov Oct. 24, 2023, 7:36 p.m. UTC | #4
On Tue, 24 Oct 2023 at 19:42, Bjorn Andersson <quic_bjorande@quicinc.com> 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@quicinc.com> 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@linaro.org>
> > > > ---
> > > >  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...

Ack, let me send v2 and hope for the best from USB maintainers then.
Dmitry Baryshkov Oct. 24, 2023, 8:13 p.m. UTC | #5
On Tue, 24 Oct 2023 at 19:42, Bjorn Andersson <quic_bjorande@quicinc.com> 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@quicinc.com> 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@linaro.org>
> > > > ---
> > > >  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.
Bjorn Andersson Oct. 24, 2023, 8:36 p.m. UTC | #6
On Tue, Oct 24, 2023 at 11:13:55PM +0300, Dmitry Baryshkov wrote:
> On Tue, 24 Oct 2023 at 19:42, Bjorn Andersson <quic_bjorande@quicinc.com> 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@quicinc.com> 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@linaro.org>
> > > > > ---
> > > > >  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.
> 

Good point. Please describe this dependency when you respin the patches,
and we can take them together through the USB tree.

Regards,
Bjorn