[PATCH 0/7] usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms

[PATCH 0/7] usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms

[Dmitry Baryshkov]

Fix several issues discovered while debugging UCSI implementation on
Qualcomm platforms (ucsi_glink). With these patches I was able to
get a working Type-C port managament implementation. Tested on SC8280XP
(Lenovo X13s laptop) and SM8350-HDK.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (7):
      usb: typec: ucsi: fix race condition in connection change ACK'ing
      usb: typec: ucsi: acknowledge the UCSI_CCI_NOT_SUPPORTED
      usb: typec: ucsi: make ACK_CC_CI rules more obvious
      usb: typec: ucsi: allow non-partner GET_PDOS for Qualcomm devices
      usb: typec: ucsi: limit the UCSI_NO_PARTNER_PDOS even further
      usb: typec: ucsi: properly register partner's PD device
      soc: qcom: pmic_glink: reenable UCSI on sc8280xp

 drivers/soc/qcom/pmic_glink.c |  1 -
 drivers/usb/typec/ucsi/ucsi.c | 51 +++++++++++++++++++++++++++++++++----------
 2 files changed, 39 insertions(+), 13 deletions(-)
---
base-commit: ea86f16f605361af3779af0e0b4be18614282091
change-id: 20240312-qcom-ucsi-fixes-6578d236b60b

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH 1/7] usb: typec: ucsi: fix race condition in connection change ACK'ing

[Dmitry Baryshkov]

The code to handle connection change events contains a race: there is an
open window for notifications to arrive between clearing EVENT_PENDING
bit and sending the ACK_CC_CI command to acknowledge the connection
change. This is mostly not an issue, but on Qualcomm platforms when the
PPM receives ACK_CC_CI with the ConnectorChange bit set if there is no
pending reported Connector Change, it responds with the CommandCompleted
+ NotSupported notifications, completely breaking UCSI state machine.

Fix this by reading out CCI after ACK_CC_CI and scheduling the work if
there is a connector change reported.

Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index cf52cb34d285..4abb752c6806 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -61,12 +61,28 @@ static int ucsi_acknowledge_command(struct ucsi *ucsi)
 
 static int ucsi_acknowledge_connector_change(struct ucsi *ucsi)
 {
+	unsigned int con_num;
 	u64 ctrl;
+	u32 cci;
+	int ret;
 
 	ctrl = UCSI_ACK_CC_CI;
 	ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
 
-	return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
+	ret = ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
+	if (ret)
+		return ret;
+
+	clear_bit(EVENT_PENDING, &ucsi->flags);
+	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
+	if (ret)
+		return ret;
+
+	con_num = UCSI_CCI_CONNECTOR(cci);
+	if (con_num)
+		ucsi_connector_change(ucsi, con_num);
+
+	return 0;
 }
 
 static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
@@ -1215,8 +1231,6 @@ static void ucsi_handle_connector_change(struct work_struct *work)
 	if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
 		ucsi_partner_task(con, ucsi_check_altmodes, 1, 0);
 
-	clear_bit(EVENT_PENDING, &con->ucsi->flags);
-
 	mutex_lock(&ucsi->ppm_lock);
 	ret = ucsi_acknowledge_connector_change(ucsi);
 	mutex_unlock(&ucsi->ppm_lock);

-- 
2.39.2

[PATCH 2/7] usb: typec: ucsi: acknowledge the UCSI_CCI_NOT_SUPPORTED

[Dmitry Baryshkov]

When the PPM reports UCSI_CCI_NOT_SUPPORTED for the command, the flag
remains set and no further commands are allowed to be processed until
OPM acknowledges failed command completion using ACK_CC_CI. Add missing
call to ucsi_acknowledge_command().

Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 4abb752c6806..bde4f03b9aa2 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -167,8 +167,10 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 	if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
 		return -EIO;
 
-	if (cci & UCSI_CCI_NOT_SUPPORTED)
-		return -EOPNOTSUPP;
+	if (cci & UCSI_CCI_NOT_SUPPORTED) {
+		ret = ucsi_acknowledge_command(ucsi);
+		return ret ? ret : -EOPNOTSUPP;
+	}
 
 	if (cci & UCSI_CCI_ERROR) {
 		if (cmd == UCSI_GET_ERROR_STATUS)

-- 
2.39.2

[PATCH 3/7] usb: typec: ucsi: make ACK_CC_CI rules more obvious

[Dmitry Baryshkov]

It is pretty easy to miss a call to usb_acknowledge_command() in
the error handling inside ucsi_exec_command(). For example
UCSI_CCI_ERROR had this call hidden inside ucsi_read_error().

Move this call and add a comment to make the rules regarding
usb_acknowledge_command() calls more obvious.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index bde4f03b9aa2..05a44e346e85 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -92,11 +92,6 @@ static int ucsi_read_error(struct ucsi *ucsi)
 	u16 error;
 	int ret;
 
-	/* Acknowledge the command that failed */
-	ret = ucsi_acknowledge_command(ucsi);
-	if (ret)
-		return ret;
-
 	ret = ucsi_exec_command(ucsi, UCSI_GET_ERROR_STATUS);
 	if (ret < 0)
 		return ret;
@@ -167,14 +162,27 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
 	if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
 		return -EIO;
 
+	/*
+	 * All error cases below must acknowledge the command completion,
+	 * otherwise PPM will be stuck and won't process commands anymore.
+	 *
+	 * In non-error case the command is acknowledged after reading Data
+	 * from the controller.
+	 */
+
 	if (cci & UCSI_CCI_NOT_SUPPORTED) {
 		ret = ucsi_acknowledge_command(ucsi);
 		return ret ? ret : -EOPNOTSUPP;
 	}
 
 	if (cci & UCSI_CCI_ERROR) {
+		ret = ucsi_acknowledge_command(ucsi);
+		if (ret)
+			return ret;
+
 		if (cmd == UCSI_GET_ERROR_STATUS)
 			return -EIO;
+
 		return ucsi_read_error(ucsi);
 	}
 

-- 
2.39.2

[PATCH 4/7] usb: typec: ucsi: allow non-partner GET_PDOS for Qualcomm devices

[Dmitry Baryshkov]

The name and description of the USB_NO_PARTNER_PDOS quirk specifies that
only retrieving PDOS of the attached device is crashing. Retrieving PDOS
of the UCSI device works. Fix the condition to limit the workaround only
to is_partner cases.

Fixes: 1d103d6af241 ("usb: typec: ucsi: fix UCSI on buggy Qualcomm devices")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 05a44e346e85..011d52bf34f6 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -641,7 +641,8 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
 	u64 command;
 	int ret;
 
-	if (ucsi->quirks & UCSI_NO_PARTNER_PDOS)
+	if (is_partner &&
+	    ucsi->quirks & UCSI_NO_PARTNER_PDOS)
 		return 0;
 
 	command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);

-- 
2.39.2

[PATCH 5/7] usb: typec: ucsi: limit the UCSI_NO_PARTNER_PDOS even further

[Dmitry Baryshkov]

Reading Partner Source PDOs for the consumer Connectors appears to be
working. Permit getting PDOs in this case in order to populate
capabilities of the connected power supply in the sysfs.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 011d52bf34f6..72d368433b1f 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -642,7 +642,9 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
 	int ret;
 
 	if (is_partner &&
-	    ucsi->quirks & UCSI_NO_PARTNER_PDOS)
+	    ucsi->quirks & UCSI_NO_PARTNER_PDOS &&
+	    ((con->status.flags & UCSI_CONSTAT_PWR_DIR) ||
+	     !is_source(role)))
 		return 0;
 
 	command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);

-- 
2.39.2

[PATCH 6/7] usb: typec: ucsi: properly register partner's PD device

[Dmitry Baryshkov]

Use typec_partner_usb_power_delivery_register() to register PD device
for Type-C partner so that the PD device is nested under the partner's
device in sysfs.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/usb/typec/ucsi/ucsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 72d368433b1f..78e04b7701c8 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -833,7 +833,7 @@ static int ucsi_register_partner_pdos(struct ucsi_connector *con)
 	if (con->partner_pd)
 		return 0;
 
-	con->partner_pd = usb_power_delivery_register(NULL, &desc);
+	con->partner_pd = typec_partner_usb_power_delivery_register(con->partner, &desc);
 	if (IS_ERR(con->partner_pd))
 		return PTR_ERR(con->partner_pd);
 

-- 
2.39.2

[PATCH 7/7] soc: qcom: pmic_glink: reenable UCSI on sc8280xp

[Dmitry Baryshkov]

Now as all UCSI issues have been fixed, reenable UCSI subdevice on the
Qualcomm SC8280XP platform.

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

diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
index f913e9bd57ed..e5a591733a0f 100644
--- a/drivers/soc/qcom/pmic_glink.c
+++ b/drivers/soc/qcom/pmic_glink.c
@@ -343,7 +343,6 @@ static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT
 
 static const struct of_device_id pmic_glink_of_match[] = {
 	{ .compatible = "qcom,sc8180x-pmic-glink", .data = &pmic_glink_sc8180x_client_mask },
-	{ .compatible = "qcom,sc8280xp-pmic-glink", .data = &pmic_glink_sc8180x_client_mask },
 	{ .compatible = "qcom,pmic-glink", .data = &pmic_glink_sm8450_client_mask },
 	{}
 };

-- 
2.39.2

Re: [PATCH 1/7] usb: typec: ucsi: fix race condition in connection change ACK'ing

[Heikki Krogerus]

Hi Dmitry,

On Wed, Mar 13, 2024 at 05:54:11AM +0200, Dmitry Baryshkov wrote:
> The code to handle connection change events contains a race: there is an
> open window for notifications to arrive between clearing EVENT_PENDING
> bit and sending the ACK_CC_CI command to acknowledge the connection
> change. This is mostly not an issue, but on Qualcomm platforms when the
> PPM receives ACK_CC_CI with the ConnectorChange bit set if there is no
> pending reported Connector Change, it responds with the CommandCompleted
> + NotSupported notifications, completely breaking UCSI state machine.
> 
> Fix this by reading out CCI after ACK_CC_CI and scheduling the work if
> there is a connector change reported.
 
> Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

UCSI specification quite clearly states that the PPM must wait until
OPM has acknowledged the notification before sending the next
notification, so this looks like a workaround for Qualcomm specific
issue. Ideally it would have been isolated - now this is done on
every platform.

I'm a little bit uncomfortable with the unconditional reading of the
CCI field. On most systems reading the field will clear it completely.
Hopefully that will not cause more problems.

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

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 20 +++++++++++++++++---
>  1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index cf52cb34d285..4abb752c6806 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -61,12 +61,28 @@ static int ucsi_acknowledge_command(struct ucsi *ucsi)
>  
>  static int ucsi_acknowledge_connector_change(struct ucsi *ucsi)
>  {
> +	unsigned int con_num;
>  	u64 ctrl;
> +	u32 cci;
> +	int ret;
>  
>  	ctrl = UCSI_ACK_CC_CI;
>  	ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
>  
> -	return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
> +	ret = ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
> +	if (ret)
> +		return ret;
> +
> +	clear_bit(EVENT_PENDING, &ucsi->flags);
> +	ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> +	if (ret)
> +		return ret;
> +
> +	con_num = UCSI_CCI_CONNECTOR(cci);
> +	if (con_num)
> +		ucsi_connector_change(ucsi, con_num);
> +
> +	return 0;
>  }
>  
>  static int ucsi_exec_command(struct ucsi *ucsi, u64 command);
> @@ -1215,8 +1231,6 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>  	if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
>  		ucsi_partner_task(con, ucsi_check_altmodes, 1, 0);
>  
> -	clear_bit(EVENT_PENDING, &con->ucsi->flags);
> -
>  	mutex_lock(&ucsi->ppm_lock);
>  	ret = ucsi_acknowledge_connector_change(ucsi);
>  	mutex_unlock(&ucsi->ppm_lock);
> 
> -- 
> 2.39.2

-- 
heikki

Re: [PATCH 2/7] usb: typec: ucsi: acknowledge the UCSI_CCI_NOT_SUPPORTED

[Heikki Krogerus]

On Wed, Mar 13, 2024 at 05:54:12AM +0200, Dmitry Baryshkov wrote:
> When the PPM reports UCSI_CCI_NOT_SUPPORTED for the command, the flag
> remains set and no further commands are allowed to be processed until
> OPM acknowledges failed command completion using ACK_CC_CI. Add missing
> call to ucsi_acknowledge_command().
> 
> Fixes: bdc62f2bae8f ("usb: typec: ucsi: Simplified registration and I/O API")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 4abb752c6806..bde4f03b9aa2 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -167,8 +167,10 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
>  	if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
>  		return -EIO;
>  
> -	if (cci & UCSI_CCI_NOT_SUPPORTED)
> -		return -EOPNOTSUPP;
> +	if (cci & UCSI_CCI_NOT_SUPPORTED) {
> +		ret = ucsi_acknowledge_command(ucsi);
> +		return ret ? ret : -EOPNOTSUPP;
> +	}
>  
>  	if (cci & UCSI_CCI_ERROR) {
>  		if (cmd == UCSI_GET_ERROR_STATUS)
> 
> -- 
> 2.39.2

-- 
heikki

Re: [PATCH 3/7] usb: typec: ucsi: make ACK_CC_CI rules more obvious

[Heikki Krogerus]

On Wed, Mar 13, 2024 at 05:54:13AM +0200, Dmitry Baryshkov wrote:
> It is pretty easy to miss a call to usb_acknowledge_command() in
> the error handling inside ucsi_exec_command(). For example
> UCSI_CCI_ERROR had this call hidden inside ucsi_read_error().
> 
> Move this call and add a comment to make the rules regarding
> usb_acknowledge_command() calls more obvious.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index bde4f03b9aa2..05a44e346e85 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -92,11 +92,6 @@ static int ucsi_read_error(struct ucsi *ucsi)
>  	u16 error;
>  	int ret;
>  
> -	/* Acknowledge the command that failed */
> -	ret = ucsi_acknowledge_command(ucsi);
> -	if (ret)
> -		return ret;
> -
>  	ret = ucsi_exec_command(ucsi, UCSI_GET_ERROR_STATUS);
>  	if (ret < 0)
>  		return ret;
> @@ -167,14 +162,27 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
>  	if (!(cci & UCSI_CCI_COMMAND_COMPLETE))
>  		return -EIO;
>  
> +	/*
> +	 * All error cases below must acknowledge the command completion,
> +	 * otherwise PPM will be stuck and won't process commands anymore.
> +	 *
> +	 * In non-error case the command is acknowledged after reading Data
> +	 * from the controller.
> +	 */
> +
>  	if (cci & UCSI_CCI_NOT_SUPPORTED) {
>  		ret = ucsi_acknowledge_command(ucsi);
>  		return ret ? ret : -EOPNOTSUPP;
>  	}
>  
>  	if (cci & UCSI_CCI_ERROR) {
> +		ret = ucsi_acknowledge_command(ucsi);
> +		if (ret)
> +			return ret;
> +
>  		if (cmd == UCSI_GET_ERROR_STATUS)
>  			return -EIO;
> +
>  		return ucsi_read_error(ucsi);
>  	}
>  
> 
> -- 
> 2.39.2

-- 
heikki

Re: [PATCH 4/7] usb: typec: ucsi: allow non-partner GET_PDOS for Qualcomm devices

[Heikki Krogerus]

On Wed, Mar 13, 2024 at 05:54:14AM +0200, Dmitry Baryshkov wrote:
> The name and description of the USB_NO_PARTNER_PDOS quirk specifies that
> only retrieving PDOS of the attached device is crashing. Retrieving PDOS
> of the UCSI device works. Fix the condition to limit the workaround only
> to is_partner cases.
> 
> Fixes: 1d103d6af241 ("usb: typec: ucsi: fix UCSI on buggy Qualcomm devices")
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 05a44e346e85..011d52bf34f6 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -641,7 +641,8 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
>  	u64 command;
>  	int ret;
>  
> -	if (ucsi->quirks & UCSI_NO_PARTNER_PDOS)
> +	if (is_partner &&
> +	    ucsi->quirks & UCSI_NO_PARTNER_PDOS)
>  		return 0;
>  
>  	command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);
> 
> -- 
> 2.39.2

-- 
heikki

Re: [PATCH 5/7] usb: typec: ucsi: limit the UCSI_NO_PARTNER_PDOS even further

[Heikki Krogerus]

On Wed, Mar 13, 2024 at 05:54:15AM +0200, Dmitry Baryshkov wrote:
> Reading Partner Source PDOs for the consumer Connectors appears to be
> working. Permit getting PDOs in this case in order to populate
> capabilities of the connected power supply in the sysfs.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 011d52bf34f6..72d368433b1f 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -642,7 +642,9 @@ static int ucsi_read_pdos(struct ucsi_connector *con,
>  	int ret;
>  
>  	if (is_partner &&
> -	    ucsi->quirks & UCSI_NO_PARTNER_PDOS)
> +	    ucsi->quirks & UCSI_NO_PARTNER_PDOS &&
> +	    ((con->status.flags & UCSI_CONSTAT_PWR_DIR) ||
> +	     !is_source(role)))
>  		return 0;
>  
>  	command = UCSI_COMMAND(UCSI_GET_PDOS) | UCSI_CONNECTOR_NUMBER(con->num);
> 
> -- 
> 2.39.2

-- 
heikki

Re: [PATCH 6/7] usb: typec: ucsi: properly register partner's PD device

[Heikki Krogerus]

On Wed, Mar 13, 2024 at 05:54:16AM +0200, Dmitry Baryshkov wrote:
> Use typec_partner_usb_power_delivery_register() to register PD device
> for Type-C partner so that the PD device is nested under the partner's
> device in sysfs.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

> ---
>  drivers/usb/typec/ucsi/ucsi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 72d368433b1f..78e04b7701c8 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -833,7 +833,7 @@ static int ucsi_register_partner_pdos(struct ucsi_connector *con)
>  	if (con->partner_pd)
>  		return 0;
>  
> -	con->partner_pd = usb_power_delivery_register(NULL, &desc);
> +	con->partner_pd = typec_partner_usb_power_delivery_register(con->partner, &desc);
>  	if (IS_ERR(con->partner_pd))
>  		return PTR_ERR(con->partner_pd);
>  
> 
> -- 
> 2.39.2

-- 
heikki

Re: [PATCH 7/7] soc: qcom: pmic_glink: reenable UCSI on sc8280xp

[Heikki Krogerus]

On Wed, Mar 13, 2024 at 05:54:17AM +0200, Dmitry Baryshkov wrote:
> Now as all UCSI issues have been fixed, reenable UCSI subdevice on the
> Qualcomm SC8280XP platform.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

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

> ---
>  drivers/soc/qcom/pmic_glink.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/pmic_glink.c b/drivers/soc/qcom/pmic_glink.c
> index f913e9bd57ed..e5a591733a0f 100644
> --- a/drivers/soc/qcom/pmic_glink.c
> +++ b/drivers/soc/qcom/pmic_glink.c
> @@ -343,7 +343,6 @@ static const unsigned long pmic_glink_sm8450_client_mask = BIT(PMIC_GLINK_CLIENT
>  
>  static const struct of_device_id pmic_glink_of_match[] = {
>  	{ .compatible = "qcom,sc8180x-pmic-glink", .data = &pmic_glink_sc8180x_client_mask },
> -	{ .compatible = "qcom,sc8280xp-pmic-glink", .data = &pmic_glink_sc8180x_client_mask },
>  	{ .compatible = "qcom,pmic-glink", .data = &pmic_glink_sm8450_client_mask },
>  	{}
>  };
> 
> -- 
> 2.39.2

-- 
heikki

Re: [PATCH 0/7] usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms

[Johan Hovold]

On Wed, Mar 13, 2024 at 05:54:10AM +0200, Dmitry Baryshkov wrote:
> Fix several issues discovered while debugging UCSI implementation on
> Qualcomm platforms (ucsi_glink). With these patches I was able to
> get a working Type-C port managament implementation. Tested on SC8280XP
> (Lenovo X13s laptop) and SM8350-HDK.

> Dmitry Baryshkov (7):
>       usb: typec: ucsi: fix race condition in connection change ACK'ing
>       usb: typec: ucsi: acknowledge the UCSI_CCI_NOT_SUPPORTED
>       usb: typec: ucsi: make ACK_CC_CI rules more obvious
>       usb: typec: ucsi: allow non-partner GET_PDOS for Qualcomm devices
>       usb: typec: ucsi: limit the UCSI_NO_PARTNER_PDOS even further
>       usb: typec: ucsi: properly register partner's PD device

>       soc: qcom: pmic_glink: reenable UCSI on sc8280xp

I just gave this series a quick spin on my X13s and it seems there are
still some issues that needs to be resolved before merging at least the
final patch in this series:

[    7.786167] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
[    7.786445] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
[    7.883493] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
[    7.883614] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
[    7.905194] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
[    7.905295] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
[    7.913340] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
[    7.913409] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)

I see these errors on boot both with and without my charger and ethernet
device connected. 

I'm afraid I won't have to time to help debug this myself at least for
another week.

Johan

Re: [PATCH 0/7] usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms

[Dmitry Baryshkov]

On Fri, 22 Mar 2024 at 14:16, Johan Hovold <johan@kernel.org> wrote:
>
> On Wed, Mar 13, 2024 at 05:54:10AM +0200, Dmitry Baryshkov wrote:
> > Fix several issues discovered while debugging UCSI implementation on
> > Qualcomm platforms (ucsi_glink). With these patches I was able to
> > get a working Type-C port managament implementation. Tested on SC8280XP
> > (Lenovo X13s laptop) and SM8350-HDK.
>
> > Dmitry Baryshkov (7):
> >       usb: typec: ucsi: fix race condition in connection change ACK'ing
> >       usb: typec: ucsi: acknowledge the UCSI_CCI_NOT_SUPPORTED
> >       usb: typec: ucsi: make ACK_CC_CI rules more obvious
> >       usb: typec: ucsi: allow non-partner GET_PDOS for Qualcomm devices
> >       usb: typec: ucsi: limit the UCSI_NO_PARTNER_PDOS even further
> >       usb: typec: ucsi: properly register partner's PD device
>
> >       soc: qcom: pmic_glink: reenable UCSI on sc8280xp
>
> I just gave this series a quick spin on my X13s and it seems there are
> still some issues that needs to be resolved before merging at least the
> final patch in this series:
>
> [    7.786167] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> [    7.786445] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> [    7.883493] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> [    7.883614] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> [    7.905194] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> [    7.905295] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> [    7.913340] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> [    7.913409] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
>
> I see these errors on boot both with and without my charger and ethernet
> device connected.

Just to doublecheck: do you have latest adsp installed? Do you have
your bootloaders updated?

If you back up the patch #5 ("limit the UCSI_NO_PARTNER_PDOS even
further"), does it still break for you?


>
> I'm afraid I won't have to time to help debug this myself at least for
> another week.
>
> Johan



--
With best wishes
Dmitry

Re: [PATCH 0/7] usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms

[Johan Hovold]

On Fri, Mar 22, 2024 at 03:39:36PM +0200, Dmitry Baryshkov wrote:
> On Fri, 22 Mar 2024 at 14:16, Johan Hovold <johan@kernel.org> wrote:
> > On Wed, Mar 13, 2024 at 05:54:10AM +0200, Dmitry Baryshkov wrote:

> > > Dmitry Baryshkov (7):
> > >       usb: typec: ucsi: fix race condition in connection change ACK'ing
> > >       usb: typec: ucsi: acknowledge the UCSI_CCI_NOT_SUPPORTED
> > >       usb: typec: ucsi: make ACK_CC_CI rules more obvious
> > >       usb: typec: ucsi: allow non-partner GET_PDOS for Qualcomm devices
> > >       usb: typec: ucsi: limit the UCSI_NO_PARTNER_PDOS even further
> > >       usb: typec: ucsi: properly register partner's PD device
> >
> > >       soc: qcom: pmic_glink: reenable UCSI on sc8280xp
> >
> > I just gave this series a quick spin on my X13s and it seems there are
> > still some issues that needs to be resolved before merging at least the
> > final patch in this series:
> >
> > [    7.786167] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > [    7.786445] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)

> > I see these errors on boot both with and without my charger and ethernet
> > device connected.
> 
> Just to doublecheck: do you have latest adsp installed?

Yes, there has only been one adsp fw released to linux-firmware and
that's the one I'm using.

If this depends on anything newer that should have been mentioned in
the commit message and we obviously can't enable UCSI until that
firmware has been released.

> Do you have your bootloaders updated?

Yes.

> If you back up the patch #5 ("limit the UCSI_NO_PARTNER_PDOS even
> further"), does it still break for you?

I don't understand what you mean by "back up the patch #5". Revert
(drop) patch 5?

The errors are still there with patch 5 reverted.

Johan

Re: [PATCH 0/7] usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms

[Dmitry Baryshkov]

On Fri, 22 Mar 2024 at 14:16, Johan Hovold <johan@kernel.org> wrote:
>
> On Wed, Mar 13, 2024 at 05:54:10AM +0200, Dmitry Baryshkov wrote:
> > Fix several issues discovered while debugging UCSI implementation on
> > Qualcomm platforms (ucsi_glink). With these patches I was able to
> > get a working Type-C port managament implementation. Tested on SC8280XP
> > (Lenovo X13s laptop) and SM8350-HDK.
>
> > Dmitry Baryshkov (7):
> >       usb: typec: ucsi: fix race condition in connection change ACK'ing
> >       usb: typec: ucsi: acknowledge the UCSI_CCI_NOT_SUPPORTED
> >       usb: typec: ucsi: make ACK_CC_CI rules more obvious
> >       usb: typec: ucsi: allow non-partner GET_PDOS for Qualcomm devices
> >       usb: typec: ucsi: limit the UCSI_NO_PARTNER_PDOS even further
> >       usb: typec: ucsi: properly register partner's PD device
>
> >       soc: qcom: pmic_glink: reenable UCSI on sc8280xp
>
> I just gave this series a quick spin on my X13s and it seems there are
> still some issues that needs to be resolved before merging at least the
> final patch in this series:
>
> [    7.786167] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> [    7.786445] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> [    7.883493] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> [    7.883614] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> [    7.905194] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> [    7.905295] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> [    7.913340] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> [    7.913409] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)

I have traced what is causing these messages. During UCSI startup the
ucsi_register_port() function queries for PDOs associated with the
on-board USB-C port. This is allowed by the spec. Qualcomm firmware
detects that there is no PD-device connected and instead of returning
corresponding set of PDOs returns Eerror Indicator set to 1b but then
it returns zero error status in response to GET_ERROR_STATUS, causing
"unknown error 0" code. I have checked the PPM, it doesn't even have
the code to set the error status properly in this case (not to mention
that asking for device's PDOs should not be an error, unless the
command is inappropriate for the target.

Thus said, I think the driver is behaving correctly. Granted that
these messages are harmless, we can ignore them for now. I'll later
check if we can update PD information for the device's ports when PD
device is attached. I have verified that once the PD device is
attached, corresponding GET_PDOS command returns correct set of PD
objects. Ccurrently the driver registers usb_power_delivery devices,
but with neither source nor sink set of capabilities.

An alternative option is to drop patches 4 and 5, keeping
'NO_PARTNER_PDOS' quirk equivalent to 'don't send GET_PDOS at all'.
However I'd like to abstain from this option, since it doesn't allow
us to check PD capabilities of the attached device.

Heikki, Johan, WDYT?

For reference, here is a trace of relevant messages exchanged over the
GLINK interface during UCSI bootstrap:

[   11.030838] write: 00000000: 10 00 01 00 07 00 00 00

GET_PDOS(connection 1, Source, 3 PDOs)

[   11.044171] write ack: 0
[   11.044263] notify: 00000000: 00 00 00 c0 00 00 00 00

Command Complete, Error

[   11.044458] read: 00000000: 00 01 00 00 00 00 00 c0 00 00 00 00 00 00 00 00
[   11.044460] read: 00000010: e7 3f 00 00 00 00 00 00 02 00 20 01 00 03 30 01
[   11.044462] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   11.059790] read: 00000000: 00 01 00 00 00 00 00 c0 00 00 00 00 00 00 00 00
[   11.059797] read: 00000010: e7 3f 00 00 00 00 00 00 02 00 20 01 00 03 30 01
[   11.059801] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   11.059814] write: 00000000: 04 00 02 00 00 00 00 00

Ack_CC command

[   11.075509] write ack: 0
[   11.075544] notify: 00000000: 00 00 00 20 00 00 00 00

Ack for Ack_CC

[   11.091828] read: 00000000: 00 01 00 00 00 00 00 20 00 00 00 00 00 00 00 00
[   11.091864] read: 00000010: e7 3f 00 00 00 00 00 00 02 00 20 01 00 03 30 01
[   11.091879] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   11.092339] write: 00000000: 13 00 00 00 00 00 00 00

GET_ERROR_STATUS

[   11.106398] write ack: 0
[   11.106435] notify: 00000000: 00 10 00 80 00 00 00 00

command complete, 0x10 bytes of response

[   11.122729] read: 00000000: 00 01 00 00 00 10 00 80 00 00 00 00 00 00 00 00
[   11.122758] read: 00000010: 00 00 00 00 00 00 00 00 02 00 20 01 00 03 30 01
[   11.122770] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Zero response data


[   11.137523] read: 00000000: 00 01 00 00 00 10 00 80 00 00 00 00 00 00 00 00
[   11.137548] read: 00000010: 00 00 00 00 00 00 00 00 02 00 20 01 00 03 30 01
[   11.137559] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   11.153028] read: 00000000: 00 01 00 00 00 10 00 80 00 00 00 00 00 00 00 00
[   11.153064] read: 00000010: 00 00 00 00 00 00 00 00 02 00 20 01 00 03 30 01
[   11.153080] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   11.153492] write: 00000000: 04 00 02 00 00 00 00 00

Ack_CC for the GET_ERROR_STATUS command

[   11.169060] write ack: 0
[   11.169108] notify: 00000000: 00 00 00 20 00 00 00 00

Ack for ACK_CC

[   11.184114] read: 00000000: 00 01 00 00 00 00 00 20 00 00 00 00 00 00 00 00
[   11.184140] read: 00000010: 00 00 00 00 00 00 00 00 02 00 20 01 00 03 30 01
[   11.184152] read: 00000020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[   11.184548] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
[   11.184695] ------------[ cut here ]------------
[   11.184703] WARNING: CPU: 2 PID: 28 at
drivers/usb/typec/ucsi/ucsi.c:140 ucsi_exec_command+0x284/0x328
[typec_ucsi]
[   11.185488] Call trace:
[   11.185494]  ucsi_exec_command+0x284/0x328 [typec_ucsi]
[   11.185519]  ucsi_send_command+0x54/0x118 [typec_ucsi]
[   11.185543]  ucsi_read_pdos+0x5c/0xdc [typec_ucsi]
[   11.185567]  ucsi_get_pdos+0x30/0xa4 [typec_ucsi]
[   11.185590]  ucsi_init_work+0x3bc/0x82c [typec_ucsi]
[   11.185614]  process_one_work+0x148/0x2a0
[   11.185638]  worker_thread+0x2fc/0x40c
[   11.185655]  kthread+0x110/0x114
[   11.185668]  ret_from_fork+0x10/0x20

Then comes the same log for the Connector=1, Sink PDOs, Connector=2
Source PDOs and finally Connector=2 Sink PDOs.


-- 
With best wishes
Dmitry

Re: [PATCH 0/7] usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms

[Johan Hovold]

On Mon, Mar 25, 2024 at 10:56:21PM +0200, Dmitry Baryshkov wrote:
> On Fri, 22 Mar 2024 at 14:16, Johan Hovold <johan@kernel.org> wrote:

> > I just gave this series a quick spin on my X13s and it seems there are
> > still some issues that needs to be resolved before merging at least the
> > final patch in this series:
> >
> > [    7.786167] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > [    7.786445] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > [    7.883493] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > [    7.883614] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > [    7.905194] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > [    7.905295] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > [    7.913340] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > [    7.913409] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> 
> I have traced what is causing these messages. During UCSI startup the
> ucsi_register_port() function queries for PDOs associated with the
> on-board USB-C port. This is allowed by the spec. Qualcomm firmware
> detects that there is no PD-device connected and instead of returning
> corresponding set of PDOs returns Eerror Indicator set to 1b but then
> it returns zero error status in response to GET_ERROR_STATUS, causing
> "unknown error 0" code. I have checked the PPM, it doesn't even have
> the code to set the error status properly in this case (not to mention
> that asking for device's PDOs should not be an error, unless the
> command is inappropriate for the target.
> 
> Thus said, I think the driver is behaving correctly. Granted that
> these messages are harmless, we can ignore them for now. I'll later
> check if we can update PD information for the device's ports when PD
> device is attached. I have verified that once the PD device is
> attached, corresponding GET_PDOS command returns correct set of PD
> objects. Ccurrently the driver registers usb_power_delivery devices,
> but with neither source nor sink set of capabilities.
> 
> An alternative option is to drop patches 4 and 5, keeping
> 'NO_PARTNER_PDOS' quirk equivalent to 'don't send GET_PDOS at all'.
> However I'd like to abstain from this option, since it doesn't allow
> us to check PD capabilities of the attached device.
> 
> Heikki, Johan, WDYT?

Whatever you do, you need to suppress those errors above before enabling
anything more on sc8280xp (e.g. even if it means adding a quirk to the
driver).

Johan

Re: [PATCH 0/7] usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms

[Dmitry Baryshkov]

On Tue, 26 Mar 2024 at 10:41, Johan Hovold <johan@kernel.org> wrote:
>
> On Mon, Mar 25, 2024 at 10:56:21PM +0200, Dmitry Baryshkov wrote:
> > On Fri, 22 Mar 2024 at 14:16, Johan Hovold <johan@kernel.org> wrote:
>
> > > I just gave this series a quick spin on my X13s and it seems there are
> > > still some issues that needs to be resolved before merging at least the
> > > final patch in this series:
> > >
> > > [    7.786167] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > > [    7.786445] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > > [    7.883493] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > > [    7.883614] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > > [    7.905194] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > > [    7.905295] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > > [    7.913340] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > > [    7.913409] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> >
> > I have traced what is causing these messages. During UCSI startup the
> > ucsi_register_port() function queries for PDOs associated with the
> > on-board USB-C port. This is allowed by the spec. Qualcomm firmware
> > detects that there is no PD-device connected and instead of returning
> > corresponding set of PDOs returns Eerror Indicator set to 1b but then
> > it returns zero error status in response to GET_ERROR_STATUS, causing
> > "unknown error 0" code. I have checked the PPM, it doesn't even have
> > the code to set the error status properly in this case (not to mention
> > that asking for device's PDOs should not be an error, unless the
> > command is inappropriate for the target.
> >
> > Thus said, I think the driver is behaving correctly. Granted that
> > these messages are harmless, we can ignore them for now. I'll later
> > check if we can update PD information for the device's ports when PD
> > device is attached. I have verified that once the PD device is
> > attached, corresponding GET_PDOS command returns correct set of PD
> > objects. Ccurrently the driver registers usb_power_delivery devices,
> > but with neither source nor sink set of capabilities.
> >
> > An alternative option is to drop patches 4 and 5, keeping
> > 'NO_PARTNER_PDOS' quirk equivalent to 'don't send GET_PDOS at all'.
> > However I'd like to abstain from this option, since it doesn't allow
> > us to check PD capabilities of the attached device.
> >
> > Heikki, Johan, WDYT?
>
> Whatever you do, you need to suppress those errors above before enabling
> anything more on sc8280xp (e.g. even if it means adding a quirk to the
> driver).

Why? The errors are legitimate.


-- 
With best wishes
Dmitry

Re: [PATCH 0/7] usb: typec: ucsi: fix several issues manifesting on Qualcomm platforms

[Dmitry Baryshkov]

On Tue, 26 Mar 2024 at 12:22, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Tue, 26 Mar 2024 at 10:41, Johan Hovold <johan@kernel.org> wrote:
> >
> > On Mon, Mar 25, 2024 at 10:56:21PM +0200, Dmitry Baryshkov wrote:
> > > On Fri, 22 Mar 2024 at 14:16, Johan Hovold <johan@kernel.org> wrote:
> >
> > > > I just gave this series a quick spin on my X13s and it seems there are
> > > > still some issues that needs to be resolved before merging at least the
> > > > final patch in this series:
> > > >
> > > > [    7.786167] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > > > [    7.786445] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > > > [    7.883493] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > > > [    7.883614] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > > > [    7.905194] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > > > [    7.905295] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > > > [    7.913340] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: unknown error 0
> > > > [    7.913409] ucsi_glink.pmic_glink_ucsi pmic_glink.ucsi.0: UCSI_GET_PDOS failed (-5)
> > >
> > > I have traced what is causing these messages. During UCSI startup the
> > > ucsi_register_port() function queries for PDOs associated with the
> > > on-board USB-C port. This is allowed by the spec. Qualcomm firmware
> > > detects that there is no PD-device connected and instead of returning
> > > corresponding set of PDOs returns Eerror Indicator set to 1b but then
> > > it returns zero error status in response to GET_ERROR_STATUS, causing
> > > "unknown error 0" code. I have checked the PPM, it doesn't even have
> > > the code to set the error status properly in this case (not to mention
> > > that asking for device's PDOs should not be an error, unless the
> > > command is inappropriate for the target.
> > >
> > > Thus said, I think the driver is behaving correctly. Granted that
> > > these messages are harmless, we can ignore them for now. I'll later
> > > check if we can update PD information for the device's ports when PD
> > > device is attached. I have verified that once the PD device is
> > > attached, corresponding GET_PDOS command returns correct set of PD
> > > objects. Ccurrently the driver registers usb_power_delivery devices,
> > > but with neither source nor sink set of capabilities.
> > >
> > > An alternative option is to drop patches 4 and 5, keeping
> > > 'NO_PARTNER_PDOS' quirk equivalent to 'don't send GET_PDOS at all'.
> > > However I'd like to abstain from this option, since it doesn't allow
> > > us to check PD capabilities of the attached device.
> > >
> > > Heikki, Johan, WDYT?
> >
> > Whatever you do, you need to suppress those errors above before enabling
> > anything more on sc8280xp (e.g. even if it means adding a quirk to the
> > driver).
>
> Why? The errors are legitimate.

Just to expand my answer. We have the 'driver should be quiet by
default' policy. Which usually means that there should be no 'foo
registered' or 'foo bound' messages if everything goes smooth. We do
not have 'don't warn about manufacturer issues' or 'don't barf if
firmware returns an error' kind of requirements. We can be nicer and
skip something if we know that it may return an error. But we don't
have to.


--
With best wishes
Dmitry