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

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

[Dmitry Baryshkov]

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
platforms. I did not enable UCSI for sc8180x yet, it has slightly
different implementation, which I'd like to get tested first.

Note: patch 2 depends on patch 1, otherwise people might end up with the
broken kernel and crashed firmware. The easiest way to handle this
dependency is to merge both patches through the USB tree.

Changes since v1:
- Check parent's compatible in the UCSI driver instead of using
  different AUX device names in the PMIC GLINK driver (Bjorn).

Dmitry Baryshkov (2):
  usb: typec: ucsi: fix UCSI on buggy Qualcomm devices
  soc: qcom: pmic_glink: enable UCSI by default

 drivers/soc/qcom/pmic_glink.c       | 19 +++++++++----------
 drivers/usb/typec/ucsi/ucsi.c       |  3 +++
 drivers/usb/typec/ucsi/ucsi.h       |  3 +++
 drivers/usb/typec/ucsi/ucsi_glink.c | 13 +++++++++++++
 4 files changed, 28 insertions(+), 10 deletions(-)

-- 
2.42.0

[PATCH v2 1/2] usb: typec: ucsi: fix UCSI on buggy Qualcomm devices

[Dmitry Baryshkov]

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 | 13 +++++++++++++
 3 files changed, 19 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..a94e2df6fd45 100644
--- a/drivers/usb/typec/ucsi/ucsi_glink.c
+++ b/drivers/usb/typec/ucsi/ucsi_glink.c
@@ -6,6 +6,7 @@
 #include <linux/auxiliary_bus.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/of_device.h>
 #include <linux/property.h>
 #include <linux/soc/qcom/pdr.h>
 #include <linux/usb/typec_mux.h>
@@ -296,11 +297,19 @@ static void pmic_glink_ucsi_destroy(void *data)
 	mutex_unlock(&ucsi->lock);
 }
 
+static const struct of_device_id pmic_glink_ucsi_of_quirks[] = {
+	{ .compatible = "qcom,sc8180x-pmic-glink", .data = (void *)UCSI_NO_PARTNER_PDOS, },
+	{ .compatible = "qcom,sc8280xp-pmic-glink", .data = (void *)UCSI_NO_PARTNER_PDOS, },
+	{ .compatible = "qcom,sm8350-pmic-glink", .data = (void *)UCSI_NO_PARTNER_PDOS, },
+	{}
+};
+
 static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
 				 const struct auxiliary_device_id *id)
 {
 	struct pmic_glink_ucsi *ucsi;
 	struct device *dev = &adev->dev;
+	const struct of_device_id *match;
 	struct fwnode_handle *fwnode;
 	int ret;
 
@@ -327,6 +336,10 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
 	if (ret)
 		return ret;
 
+	match = of_match_device(pmic_glink_ucsi_of_quirks, dev->parent);
+	if (match)
+		ucsi->ucsi->quirks = (unsigned long)match->data;
+
 	ucsi_set_drvdata(ucsi->ucsi, ucsi);
 
 	device_for_each_child_node(dev, fwnode) {
-- 
2.42.0

Re: [PATCH v2 1/2] usb: typec: ucsi: fix UCSI on buggy Qualcomm devices

[Neil Armstrong]

Hi,

On 25/10/2023 13:49, Dmitry Baryshkov wrote:
> On sevral Qualcomm platforms (SC8180X, SM8350, SC8280XP) a call to

    s/sevral/several/

> 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 | 13 +++++++++++++
>   3 files changed, 19 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..a94e2df6fd45 100644
> --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> @@ -6,6 +6,7 @@
>   #include <linux/auxiliary_bus.h>
>   #include <linux/module.h>
>   #include <linux/mutex.h>
> +#include <linux/of_device.h>
>   #include <linux/property.h>
>   #include <linux/soc/qcom/pdr.h>
>   #include <linux/usb/typec_mux.h>
> @@ -296,11 +297,19 @@ static void pmic_glink_ucsi_destroy(void *data)
>   	mutex_unlock(&ucsi->lock);
>   }
>   
> +static const struct of_device_id pmic_glink_ucsi_of_quirks[] = {
> +	{ .compatible = "qcom,sc8180x-pmic-glink", .data = (void *)UCSI_NO_PARTNER_PDOS, },
> +	{ .compatible = "qcom,sc8280xp-pmic-glink", .data = (void *)UCSI_NO_PARTNER_PDOS, },
> +	{ .compatible = "qcom,sm8350-pmic-glink", .data = (void *)UCSI_NO_PARTNER_PDOS, },
> +	{}
> +};
> +
>   static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
>   				 const struct auxiliary_device_id *id)
>   {
>   	struct pmic_glink_ucsi *ucsi;
>   	struct device *dev = &adev->dev;
> +	const struct of_device_id *match;
>   	struct fwnode_handle *fwnode;
>   	int ret;
>   
> @@ -327,6 +336,10 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
>   	if (ret)
>   		return ret;
>   
> +	match = of_match_device(pmic_glink_ucsi_of_quirks, dev->parent);
> +	if (match)
> +		ucsi->ucsi->quirks = (unsigned long)match->data;
> +
>   	ucsi_set_drvdata(ucsi->ucsi, ucsi);
>   
>   	device_for_each_child_node(dev, fwnode) {

With typo fixed:

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH v2 1/2] usb: typec: ucsi: fix UCSI on buggy Qualcomm devices

[Heikki Krogerus]

On Wed, Oct 25, 2023 at 02:49:29PM +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.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  drivers/usb/typec/ucsi/ucsi.c       |  3 +++
>  drivers/usb/typec/ucsi/ucsi.h       |  3 +++
>  drivers/usb/typec/ucsi/ucsi_glink.c | 13 +++++++++++++
>  3 files changed, 19 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..a94e2df6fd45 100644
> --- a/drivers/usb/typec/ucsi/ucsi_glink.c
> +++ b/drivers/usb/typec/ucsi/ucsi_glink.c
> @@ -6,6 +6,7 @@
>  #include <linux/auxiliary_bus.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/of_device.h>
>  #include <linux/property.h>
>  #include <linux/soc/qcom/pdr.h>
>  #include <linux/usb/typec_mux.h>
> @@ -296,11 +297,19 @@ static void pmic_glink_ucsi_destroy(void *data)
>  	mutex_unlock(&ucsi->lock);
>  }
>  
> +static const struct of_device_id pmic_glink_ucsi_of_quirks[] = {
> +	{ .compatible = "qcom,sc8180x-pmic-glink", .data = (void *)UCSI_NO_PARTNER_PDOS, },
> +	{ .compatible = "qcom,sc8280xp-pmic-glink", .data = (void *)UCSI_NO_PARTNER_PDOS, },
> +	{ .compatible = "qcom,sm8350-pmic-glink", .data = (void *)UCSI_NO_PARTNER_PDOS, },
> +	{}
> +};
> +
>  static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
>  				 const struct auxiliary_device_id *id)
>  {
>  	struct pmic_glink_ucsi *ucsi;
>  	struct device *dev = &adev->dev;
> +	const struct of_device_id *match;
>  	struct fwnode_handle *fwnode;
>  	int ret;
>  
> @@ -327,6 +336,10 @@ static int pmic_glink_ucsi_probe(struct auxiliary_device *adev,
>  	if (ret)
>  		return ret;
>  
> +	match = of_match_device(pmic_glink_ucsi_of_quirks, dev->parent);
> +	if (match)
> +		ucsi->ucsi->quirks = (unsigned long)match->data;
> +
>  	ucsi_set_drvdata(ucsi->ucsi, ucsi);
>  
>  	device_for_each_child_node(dev, fwnode) {
> -- 
> 2.42.0

-- 
heikki

[PATCH v2 2/2] soc: qcom: pmic_glink: enable UCSI by default

[Dmitry Baryshkov]

Now as the issue with the UCSI_GET_PDOS is worked around, enable UCSI
support for all PMIC_GLINK platforms except Qualcomm SC8180X. The
mentioned SoC has slightly different UCSI implementation, which I would
like be tested properly before enabling it.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/soc/qcom/pmic_glink.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index 914057331afd..71d8901a9389 100644
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -18,9 +18,6 @@ enum {
 	PMIC_GLINK_CLIENT_UCSI,
 };
 
-#define PMIC_GLINK_CLIENT_DEFAULT	(BIT(PMIC_GLINK_CLIENT_BATT) |	\
-					 BIT(PMIC_GLINK_CLIENT_ALTMODE))
-
 struct pmic_glink {
 	struct device *dev;
 	struct pdr_handle *pdr;
@@ -263,10 +260,10 @@ static int pmic_glink_probe(struct platform_device *pdev)
 	mutex_init(&pg->state_lock);
 
 	match_data = (unsigned long *)of_device_get_match_data(&pdev->dev);
-	if (match_data)
-		pg->client_mask = *match_data;
-	else
-		pg->client_mask = PMIC_GLINK_CLIENT_DEFAULT;
+	if (!match_data)
+		return -EINVAL;
+
+	pg->client_mask = *match_data;
 
 	if (pg->client_mask & BIT(PMIC_GLINK_CLIENT_UCSI)) {
 		ret = pmic_glink_add_aux_device(pg, &pg->ucsi_aux, "ucsi");
@@ -336,14 +333,16 @@ static void pmic_glink_remove(struct platform_device *pdev)
 	mutex_unlock(&__pmic_glink_lock);
 }
 
+static const unsigned long pmic_glink_sc8180x_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
+							    BIT(PMIC_GLINK_CLIENT_ALTMODE);
+
 static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
 							   BIT(PMIC_GLINK_CLIENT_ALTMODE) |
 							   BIT(PMIC_GLINK_CLIENT_UCSI);
 
 static const struct of_device_id pmic_glink_of_match[] = {
-	{ .compatible = "qcom,sm8450-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
-	{ .compatible = "qcom,sm8550-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
-	{ .compatible = "qcom,pmic-glink" },
+	{ .compatible = "qcom,sc8180x-pmic-glink", .data = &pmic_glink_sc8180x_client_mask },
+	{ .compatible = "qcom,pmic-glink", .data = &pmic_glink_sm8450_client_mask },
 	{}
 };
 MODULE_DEVICE_TABLE(of, pmic_glink_of_match);
-- 
2.42.0

Re: [PATCH v2 2/2] soc: qcom: pmic_glink: enable UCSI by default

[Neil Armstrong]

On 25/10/2023 13:49, Dmitry Baryshkov wrote:
> Now as the issue with the UCSI_GET_PDOS is worked around, enable UCSI
> support for all PMIC_GLINK platforms except Qualcomm SC8180X. The
> mentioned SoC has slightly different UCSI implementation, which I would
> like be tested properly before enabling it.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/soc/qcom/pmic_glink.c | 19 +++++++++----------
>   1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index 914057331afd..71d8901a9389 100644
> --- a/drivers/soc/qcom/pmic_glink.c
> +++ b/drivers/soc/qcom/pmic_glink.c
> @@ -18,9 +18,6 @@ enum {
>   	PMIC_GLINK_CLIENT_UCSI,
>   };
>   
> -#define PMIC_GLINK_CLIENT_DEFAULT	(BIT(PMIC_GLINK_CLIENT_BATT) |	\
> -					 BIT(PMIC_GLINK_CLIENT_ALTMODE))
> -
>   struct pmic_glink {
>   	struct device *dev;
>   	struct pdr_handle *pdr;
> @@ -263,10 +260,10 @@ static int pmic_glink_probe(struct platform_device *pdev)
>   	mutex_init(&pg->state_lock);
>   
>   	match_data = (unsigned long *)of_device_get_match_data(&pdev->dev);
> -	if (match_data)
> -		pg->client_mask = *match_data;
> -	else
> -		pg->client_mask = PMIC_GLINK_CLIENT_DEFAULT;
> +	if (!match_data)
> +		return -EINVAL;
> +
> +	pg->client_mask = *match_data;
>   
>   	if (pg->client_mask & BIT(PMIC_GLINK_CLIENT_UCSI)) {
>   		ret = pmic_glink_add_aux_device(pg, &pg->ucsi_aux, "ucsi");
> @@ -336,14 +333,16 @@ static void pmic_glink_remove(struct platform_device *pdev)
>   	mutex_unlock(&__pmic_glink_lock);
>   }
>   
> +static const unsigned long pmic_glink_sc8180x_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
> +							    BIT(PMIC_GLINK_CLIENT_ALTMODE);
> +
>   static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT_BATT) |
>   							   BIT(PMIC_GLINK_CLIENT_ALTMODE) |
>   							   BIT(PMIC_GLINK_CLIENT_UCSI);
>   
>   static const struct of_device_id pmic_glink_of_match[] = {
> -	{ .compatible = "qcom,sm8450-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
> -	{ .compatible = "qcom,sm8550-pmic-glink", .data = &pmic_glink_sm8450_client_mask },
> -	{ .compatible = "qcom,pmic-glink" },
> +	{ .compatible = "qcom,sc8180x-pmic-glink", .data = &pmic_glink_sc8180x_client_mask },
> +	{ .compatible = "qcom,pmic-glink", .data = &pmic_glink_sm8450_client_mask },
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, pmic_glink_of_match);

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

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

[Johan Hovold]

On Wed, Oct 25, 2023 at 02:49:28PM +0300, Dmitry Baryshkov wrote:
> 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
> platforms. I did not enable UCSI for sc8180x yet, it has slightly
> different implementation, which I'd like to get tested first.

Has no one tested this on sc8280xp/x13s before merging?

I see a bunch of errors with this series applied to 6.7-rc4:

[   11.999960] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
[   12.000430] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: ucsi_handle_connector_change: GET_CONNECTOR_STATUS failed (-110)
[   17.120515] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
[   17.124204] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
[   23.264792] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
[   23.264953] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)

Is it just broken or am I missing some undocumented dependency that is
only in linux-next?

Johan

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

[Dmitry Baryshkov]

On Fri, 8 Dec 2023 at 10:39, Johan Hovold <johan@kernel.org> wrote:
>
> On Wed, Oct 25, 2023 at 02:49:28PM +0300, Dmitry Baryshkov wrote:
> > 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
> > platforms. I did not enable UCSI for sc8180x yet, it has slightly
> > different implementation, which I'd like to get tested first.
>
> Has no one tested this on sc8280xp/x13s before merging?
>
> I see a bunch of errors with this series applied to 6.7-rc4:
>
> [   11.999960] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> [   12.000430] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: ucsi_handle_connector_change: GET_CONNECTOR_STATUS failed (-110)
> [   17.120515] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> [   17.124204] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> [   23.264792] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> [   23.264953] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)

Can you please post previous messages or is the first timeout the
first error from ucsi?

>
> Is it just broken or am I missing some undocumented dependency that is
> only in linux-next?
>
> Johan



-- 
With best wishes
Dmitry

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

[Johan Hovold]

On Fri, Dec 08, 2023 at 12:58:29PM +0200, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 10:39, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Wed, Oct 25, 2023 at 02:49:28PM +0300, Dmitry Baryshkov wrote:
> > > 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
> > > platforms. I did not enable UCSI for sc8180x yet, it has slightly
> > > different implementation, which I'd like to get tested first.
> >
> > Has no one tested this on sc8280xp/x13s before merging?
> >
> > I see a bunch of errors with this series applied to 6.7-rc4:
> >
> > [   11.999960] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > [   12.000430] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: ucsi_handle_connector_change: GET_CONNECTOR_STATUS failed (-110)
> > [   17.120515] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > [   17.124204] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> > [   23.264792] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > [   23.264953] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> 
> Can you please post previous messages or is the first timeout the
> first error from ucsi?

These are all the ucsi messages in the log (dmesg | grep ucsi).

The first error is sometimes GET_CONNECTOR_STATUS failed (-95) instead:

[    9.012421] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-95)
[   14.047379] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
[   14.050708] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
[   20.192382] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
[   20.192542] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)

I see that one if I boot with only the charger connected, the later -110
timeouts go away if I disconnect my r8152 ethernet adapter.

Johan

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

[Dmitry Baryshkov]

On Fri, 8 Dec 2023 at 13:09, Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, Dec 08, 2023 at 12:58:29PM +0200, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 10:39, Johan Hovold <johan@kernel.org> wrote:
> > >
> > > On Wed, Oct 25, 2023 at 02:49:28PM +0300, Dmitry Baryshkov wrote:
> > > > 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
> > > > platforms. I did not enable UCSI for sc8180x yet, it has slightly
> > > > different implementation, which I'd like to get tested first.
> > >
> > > Has no one tested this on sc8280xp/x13s before merging?
> > >
> > > I see a bunch of errors with this series applied to 6.7-rc4:
> > >
> > > [   11.999960] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > > [   12.000430] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: ucsi_handle_connector_change: GET_CONNECTOR_STATUS failed (-110)
> > > [   17.120515] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > > [   17.124204] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> > > [   23.264792] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > > [   23.264953] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> >
> > Can you please post previous messages or is the first timeout the
> > first error from ucsi?
>
> These are all the ucsi messages in the log (dmesg | grep ucsi).
>
> The first error is sometimes GET_CONNECTOR_STATUS failed (-95) instead:

Ack, thank you. This is pending on my side together with the UCSI
glink / altmode rework. I hope to have patches for that closer to the
NY.

>
> [    9.012421] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-95)
> [   14.047379] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> [   14.050708] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> [   20.192382] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> [   20.192542] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
>
> I see that one if I boot with only the charger connected, the later -110
> timeouts go away if I disconnect my r8152 ethernet adapter.
>
> Johan



-- 
With best wishes
Dmitry

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

[Johan Hovold]

On Fri, Dec 08, 2023 at 01:10:59PM +0200, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 13:09, Johan Hovold <johan@kernel.org> wrote:
> > On Fri, Dec 08, 2023 at 12:58:29PM +0200, Dmitry Baryshkov wrote:
> > > On Fri, 8 Dec 2023 at 10:39, Johan Hovold <johan@kernel.org> wrote:
> > > > On Wed, Oct 25, 2023 at 02:49:28PM +0300, Dmitry Baryshkov wrote:
> > > > > 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
> > > > > platforms. I did not enable UCSI for sc8180x yet, it has slightly
> > > > > different implementation, which I'd like to get tested first.
> > > >
> > > > Has no one tested this on sc8280xp/x13s before merging?
> > > >
> > > > I see a bunch of errors with this series applied to 6.7-rc4:
> > > >
> > > > [   11.999960] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > > > [   12.000430] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: ucsi_handle_connector_change: GET_CONNECTOR_STATUS failed (-110)
> > > > [   17.120515] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > > > [   17.124204] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> > > > [   23.264792] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > > > [   23.264953] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> > >
> > > Can you please post previous messages or is the first timeout the
> > > first error from ucsi?
> >
> > These are all the ucsi messages in the log (dmesg | grep ucsi).
> >
> > The first error is sometimes GET_CONNECTOR_STATUS failed (-95) instead:
> 
> Ack, thank you. This is pending on my side together with the UCSI
> glink / altmode rework. I hope to have patches for that closer to the
> NY.

What does that mean? That we shall revert these patches until that work
is finished? I don't want to have these errors littering the logs,
scaring users and possibly slowing down boot (those are five second
timeouts).

Also, if this was known issue, why wasn't it mentioned the cover letter
or commit messages?

Johan

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

[Dmitry Baryshkov]

On Fri, 8 Dec 2023 at 13:47, Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, Dec 08, 2023 at 01:10:59PM +0200, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 13:09, Johan Hovold <johan@kernel.org> wrote:
> > > On Fri, Dec 08, 2023 at 12:58:29PM +0200, Dmitry Baryshkov wrote:
> > > > On Fri, 8 Dec 2023 at 10:39, Johan Hovold <johan@kernel.org> wrote:
> > > > > On Wed, Oct 25, 2023 at 02:49:28PM +0300, Dmitry Baryshkov wrote:
> > > > > > 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
> > > > > > platforms. I did not enable UCSI for sc8180x yet, it has slightly
> > > > > > different implementation, which I'd like to get tested first.
> > > > >
> > > > > Has no one tested this on sc8280xp/x13s before merging?
> > > > >
> > > > > I see a bunch of errors with this series applied to 6.7-rc4:
> > > > >
> > > > > [   11.999960] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > > > > [   12.000430] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: ucsi_handle_connector_change: GET_CONNECTOR_STATUS failed (-110)
> > > > > [   17.120515] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > > > > [   17.124204] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> > > > > [   23.264792] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: timeout waiting for UCSI sync write response
> > > > > [   23.264953] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: GET_CONNECTOR_STATUS failed (-110)
> > > >
> > > > Can you please post previous messages or is the first timeout the
> > > > first error from ucsi?
> > >
> > > These are all the ucsi messages in the log (dmesg | grep ucsi).
> > >
> > > The first error is sometimes GET_CONNECTOR_STATUS failed (-95) instead:
> >
> > Ack, thank you. This is pending on my side together with the UCSI
> > glink / altmode rework. I hope to have patches for that closer to the
> > NY.
>
> What does that mean? That we shall revert these patches until that work
> is finished? I don't want to have these errors littering the logs,
> scaring users and possibly slowing down boot (those are five second
> timeouts).

Just send a patch disabling ucsi for sc8280xp.

>
> Also, if this was known issue, why wasn't it mentioned the cover letter
> or commit messages?

Surely it was not the known issue, otherwise I would not have sent the series.

-- 
With best wishes
Dmitry

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

[Johan Hovold]

On Fri, Dec 08, 2023 at 02:16:27PM +0200, Dmitry Baryshkov wrote:
> On Fri, 8 Dec 2023 at 13:47, Johan Hovold <johan@kernel.org> wrote:
> > On Fri, Dec 08, 2023 at 01:10:59PM +0200, Dmitry Baryshkov wrote:
> > > On Fri, 8 Dec 2023 at 13:09, Johan Hovold <johan@kernel.org> wrote:

> > > > The first error is sometimes GET_CONNECTOR_STATUS failed (-95) instead:
> > >
> > > Ack, thank you. This is pending on my side together with the UCSI
> > > glink / altmode rework. I hope to have patches for that closer to the
> > > NY.
> >
> > What does that mean? That we shall revert these patches until that work
> > is finished? I don't want to have these errors littering the logs,
> > scaring users and possibly slowing down boot (those are five second
> > timeouts).
> 
> Just send a patch disabling ucsi for sc8280xp.

Ok, will do.

Looks like that is indeed the only platform besides sc8180x which had
not yet been tested.

> > Also, if this was known issue, why wasn't it mentioned the cover letter
> > or commit messages?
> 
> Surely it was not the known issue, otherwise I would not have sent the series.

Ah, sorry, I misunderstood you then.

Johan

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

[Dmitry Baryshkov]

On Fri, 8 Dec 2023 at 14:25, Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, Dec 08, 2023 at 02:16:27PM +0200, Dmitry Baryshkov wrote:
> > On Fri, 8 Dec 2023 at 13:47, Johan Hovold <johan@kernel.org> wrote:
> > > On Fri, Dec 08, 2023 at 01:10:59PM +0200, Dmitry Baryshkov wrote:
> > > > On Fri, 8 Dec 2023 at 13:09, Johan Hovold <johan@kernel.org> wrote:
>
> > > > > The first error is sometimes GET_CONNECTOR_STATUS failed (-95) instead:
> > > >
> > > > Ack, thank you. This is pending on my side together with the UCSI
> > > > glink / altmode rework. I hope to have patches for that closer to the
> > > > NY.
> > >
> > > What does that mean? That we shall revert these patches until that work
> > > is finished? I don't want to have these errors littering the logs,
> > > scaring users and possibly slowing down boot (those are five second
> > > timeouts).
> >
> > Just send a patch disabling ucsi for sc8280xp.
>
> Ok, will do.
>
> Looks like that is indeed the only platform besides sc8180x which had
> not yet been tested.

And it has its own peculiarities which I didn't observe on other platforms.

>
> > > Also, if this was known issue, why wasn't it mentioned the cover letter
> > > or commit messages?
> >
> > Surely it was not the known issue, otherwise I would not have sent the series.
>
> Ah, sorry, I misunderstood you then.

No problem :-)

-- 
With best wishes
Dmitry

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

[Bjorn Andersson]

On Wed, 25 Oct 2023 14:49:28 +0300, Dmitry Baryshkov wrote:
> 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.
> 
> [...]

Applied, thanks!

[1/2] usb: typec: ucsi: fix UCSI on buggy Qualcomm devices
      commit: 1d103d6af241dbfc7e11eb9a46dff65db257a37f
[2/2] soc: qcom: pmic_glink: enable UCSI by default
      commit: 4db09e7b967b905ba3036a4d96e81c06b896b1bf

Best regards,
-- 
Bjorn Andersson <andersson@kernel.org>