| Message ID | 20231023215327.695720-2-dmitry.baryshkov@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: add workaround for several Qualcomm platforms | expand |
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 >
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 > >
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
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.
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.
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
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, }, {}, }; MODULE_DEVICE_TABLE(auxiliary, pmic_glink_ucsi_id_table);
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. 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(+)