[PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support

[PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support

[Hector Martin]

Hi folks,

These two fixes make tipd work properly on Apple M1 devices, in
particular in the case where the bootloader hasn't initialized
the controllers yet.

We normally do it in m1n1 (so the machine can charge and so bootloaders
get working USB without needing this driver), but that was causing this
codepath to never get properly exercised, so we never caught it. I
noticed on the new machines with 3+1 ports, since m1n1 was only
initializing 2 and the other 2 were failing to initialize.

Hector Martin (2):
  usb: typec: tipd: Fix typo in cd321x_switch_power_state
  usb: typec: tipd: Fix initialization sequence for cd321x

 drivers/usb/typec/tipd/core.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

-- 
2.33.0

[PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state

[Hector Martin]

SPSS should've been SSPS.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/usb/typec/tipd/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index fb8ef12bbe9c..4da5a0b2aed2 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -653,7 +653,7 @@ static int cd321x_switch_power_state(struct tps6598x *tps, u8 target_state)
 	if (state == target_state)
 		return 0;
 
-	ret = tps6598x_exec_cmd(tps, "SPSS", sizeof(u8), &target_state, 0, NULL);
+	ret = tps6598x_exec_cmd(tps, "SSPS", sizeof(u8), &target_state, 0, NULL);
 	if (ret)
 		return ret;
 
-- 
2.33.0

[PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x

[Hector Martin]

The power state switch needs to happen first, as that
kickstarts the firmware into normal mode.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/usb/typec/tipd/core.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 4da5a0b2aed2..6d27a5b5e3ca 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -707,6 +707,7 @@ static int tps6598x_probe(struct i2c_client *client)
 	u32 conf;
 	u32 vid;
 	int ret;
+	u64 mask1;
 
 	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
 	if (!tps)
@@ -730,11 +731,6 @@ static int tps6598x_probe(struct i2c_client *client)
 	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		tps->i2c_protocol = true;
 
-	/* Make sure the controller has application firmware running */
-	ret = tps6598x_check_mode(tps);
-	if (ret)
-		return ret;
-
 	if (np && of_device_is_compatible(np, "apple,cd321x")) {
 		/* Switch CD321X chips to the correct system power state */
 		ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
@@ -742,24 +738,27 @@ static int tps6598x_probe(struct i2c_client *client)
 			return ret;
 
 		/* CD321X chips have all interrupts masked initially */
-		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
-					APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
-					APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
-					APPLE_CD_REG_INT_PLUG_EVENT);
-		if (ret)
-			return ret;
+		mask1 = APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
+			APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
+			APPLE_CD_REG_INT_PLUG_EVENT;
 
 		irq_handler = cd321x_interrupt;
 	} else {
 		/* Enable power status, data status and plug event interrupts */
-		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
-				       TPS_REG_INT_POWER_STATUS_UPDATE |
-				       TPS_REG_INT_DATA_STATUS_UPDATE |
-				       TPS_REG_INT_PLUG_EVENT);
-		if (ret)
-			return ret;
+		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
+			TPS_REG_INT_DATA_STATUS_UPDATE |
+			TPS_REG_INT_PLUG_EVENT;
 	}
 
+	/* Make sure the controller has application firmware running */
+	ret = tps6598x_check_mode(tps);
+	if (ret)
+		return ret;
+
+	ret = tps6598x_write64(tps, TPS_REG_INT_MASK1, mask1);
+	if (ret)
+		return ret;
+
 	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
 	if (ret < 0)
 		return ret;
-- 
2.33.0

Re: [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state

[Greg Kroah-Hartman]

On Thu, Nov 18, 2021 at 12:14:49AM +0900, Hector Martin wrote:
> SPSS should've been SSPS.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/usb/typec/tipd/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index fb8ef12bbe9c..4da5a0b2aed2 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -653,7 +653,7 @@ static int cd321x_switch_power_state(struct tps6598x *tps, u8 target_state)
>  	if (state == target_state)
>  		return 0;
>  
> -	ret = tps6598x_exec_cmd(tps, "SPSS", sizeof(u8), &target_state, 0, NULL);
> +	ret = tps6598x_exec_cmd(tps, "SSPS", sizeof(u8), &target_state, 0, NULL);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.33.0
> 

This looks like a "Fix" commit.  What commit id does this fix?

thanks,

greg k-h

Re: [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x

[Greg Kroah-Hartman]

On Thu, Nov 18, 2021 at 12:14:50AM +0900, Hector Martin wrote:
> The power state switch needs to happen first, as that
> kickstarts the firmware into normal mode.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/usb/typec/tipd/core.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)

Same question here, what commit id does this fix?

thanks,

greg k-h

Re: [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state

[Hector Martin]

On 18/11/2021 01.03, Greg Kroah-Hartman wrote:
> On Thu, Nov 18, 2021 at 12:14:49AM +0900, Hector Martin wrote:
>> SPSS should've been SSPS.
>>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> ---
>>   drivers/usb/typec/tipd/core.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>> index fb8ef12bbe9c..4da5a0b2aed2 100644
>> --- a/drivers/usb/typec/tipd/core.c
>> +++ b/drivers/usb/typec/tipd/core.c
>> @@ -653,7 +653,7 @@ static int cd321x_switch_power_state(struct tps6598x *tps, u8 target_state)
>>   	if (state == target_state)
>>   		return 0;
>>   
>> -	ret = tps6598x_exec_cmd(tps, "SPSS", sizeof(u8), &target_state, 0, NULL);
>> +	ret = tps6598x_exec_cmd(tps, "SSPS", sizeof(u8), &target_state, 0, NULL);
>>   	if (ret)
>>   		return ret;
>>   
>> -- 
>> 2.33.0
>>
> 
> This looks like a "Fix" commit.  What commit id does this fix?
> 
> thanks,
> 
> greg k-h
> 

That should be:

Fixes: c9c14be664cf ("usb: typec: tipd: Switch CD321X power state to S0")

Thanks,
-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x

[Hector Martin]

On 18/11/2021 01.04, Greg Kroah-Hartman wrote:
> On Thu, Nov 18, 2021 at 12:14:50AM +0900, Hector Martin wrote:
>> The power state switch needs to happen first, as that
>> kickstarts the firmware into normal mode.
>>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> ---
>>   drivers/usb/typec/tipd/core.c | 33 ++++++++++++++++-----------------
>>   1 file changed, 16 insertions(+), 17 deletions(-)
> 
> Same question here, what commit id does this fix?
> 
> thanks,
> 
> greg k-h
> 

Logically speaking, this fixes the same commit too (though it does it by
moving everything around the hunk it introduced instead), so:

Fixes: c9c14be664cf ("usb: typec: tipd: Switch CD321X power state to S0")

Thanks,
-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support

[Sven Peter]

On Wed, Nov 17, 2021, at 16:14, Hector Martin wrote:
> Hi folks,
>
> These two fixes make tipd work properly on Apple M1 devices, in
> particular in the case where the bootloader hasn't initialized
> the controllers yet.
>
> We normally do it in m1n1 (so the machine can charge and so bootloaders
> get working USB without needing this driver), but that was causing this
> codepath to never get properly exercised, so we never caught it. I

My boot process usually is iBoot -> m1n1 on nvme -> m1n1 chainloaded over usb.
I thought I exercised this path by turning off the init in m1n1. I didn't take
into account that this would only affect the one loaded over usb and that
the one on nvme would still intitialize everything.
Thanks for fixing this!

With the Fixes tags feel free to add

Reviewed-by: Sven Peter <sven@svenpeter.dev>

to both patches.

Sven

Re: [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state

[Heikki Krogerus]

On Thu, Nov 18, 2021 at 12:14:49AM +0900, Hector Martin wrote:
> SPSS should've been SSPS.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

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

> ---
>  drivers/usb/typec/tipd/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index fb8ef12bbe9c..4da5a0b2aed2 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -653,7 +653,7 @@ static int cd321x_switch_power_state(struct tps6598x *tps, u8 target_state)
>  	if (state == target_state)
>  		return 0;
>  
> -	ret = tps6598x_exec_cmd(tps, "SPSS", sizeof(u8), &target_state, 0, NULL);
> +	ret = tps6598x_exec_cmd(tps, "SSPS", sizeof(u8), &target_state, 0, NULL);
>  	if (ret)
>  		return ret;
>  
> -- 
> 2.33.0

-- 
heikki

Re: [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x

[Heikki Krogerus]

Hi,

On Thu, Nov 18, 2021 at 12:14:50AM +0900, Hector Martin wrote:
> The power state switch needs to happen first, as that
> kickstarts the firmware into normal mode.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

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

Please resend these with the appropriate Fixes tag included.

> ---
>  drivers/usb/typec/tipd/core.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
> index 4da5a0b2aed2..6d27a5b5e3ca 100644
> --- a/drivers/usb/typec/tipd/core.c
> +++ b/drivers/usb/typec/tipd/core.c
> @@ -707,6 +707,7 @@ static int tps6598x_probe(struct i2c_client *client)
>  	u32 conf;
>  	u32 vid;
>  	int ret;
> +	u64 mask1;
>  
>  	tps = devm_kzalloc(&client->dev, sizeof(*tps), GFP_KERNEL);
>  	if (!tps)
> @@ -730,11 +731,6 @@ static int tps6598x_probe(struct i2c_client *client)
>  	if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
>  		tps->i2c_protocol = true;
>  
> -	/* Make sure the controller has application firmware running */
> -	ret = tps6598x_check_mode(tps);
> -	if (ret)
> -		return ret;
> -
>  	if (np && of_device_is_compatible(np, "apple,cd321x")) {
>  		/* Switch CD321X chips to the correct system power state */
>  		ret = cd321x_switch_power_state(tps, TPS_SYSTEM_POWER_STATE_S0);
> @@ -742,24 +738,27 @@ static int tps6598x_probe(struct i2c_client *client)
>  			return ret;
>  
>  		/* CD321X chips have all interrupts masked initially */
> -		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
> -					APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> -					APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> -					APPLE_CD_REG_INT_PLUG_EVENT);
> -		if (ret)
> -			return ret;
> +		mask1 = APPLE_CD_REG_INT_POWER_STATUS_UPDATE |
> +			APPLE_CD_REG_INT_DATA_STATUS_UPDATE |
> +			APPLE_CD_REG_INT_PLUG_EVENT;
>  
>  		irq_handler = cd321x_interrupt;
>  	} else {
>  		/* Enable power status, data status and plug event interrupts */
> -		ret = tps6598x_write64(tps, TPS_REG_INT_MASK1,
> -				       TPS_REG_INT_POWER_STATUS_UPDATE |
> -				       TPS_REG_INT_DATA_STATUS_UPDATE |
> -				       TPS_REG_INT_PLUG_EVENT);
> -		if (ret)
> -			return ret;
> +		mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
> +			TPS_REG_INT_DATA_STATUS_UPDATE |
> +			TPS_REG_INT_PLUG_EVENT;
>  	}
>  
> +	/* Make sure the controller has application firmware running */
> +	ret = tps6598x_check_mode(tps);
> +	if (ret)
> +		return ret;
> +
> +	ret = tps6598x_write64(tps, TPS_REG_INT_MASK1, mask1);
> +	if (ret)
> +		return ret;
> +
>  	ret = tps6598x_read32(tps, TPS_REG_STATUS, &status);
>  	if (ret < 0)
>  		return ret;
> -- 
> 2.33.0

thanks,

-- 
heikki

Re: [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x

[Hector Martin]

On 18/11/2021 22.12, Heikki Krogerus wrote:
> Hi,
> 
> On Thu, Nov 18, 2021 at 12:14:50AM +0900, Hector Martin wrote:
>> The power state switch needs to happen first, as that
>> kickstarts the firmware into normal mode.
>>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
> 
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Please resend these with the appropriate Fixes tag included.

Done, thanks for the review!

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub