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
The power state switch needs to happen first, as that
kickstarts the firmware into normal mode.
Signed-off-by: Hector Martin <[email protected]>
---
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
SPSS should've been SSPS.
Signed-off-by: Hector Martin <[email protected]>
---
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
On Thu, Nov 18, 2021 at 12:14:49AM +0900, Hector Martin wrote:
> SPSS should've been SSPS.
>
> Signed-off-by: Hector Martin <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
>> ---
>> 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 ([email protected])
Public Key: https://mrcn.st/pub
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 <[email protected]>
>> ---
>> 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 ([email protected])
Public Key: https://mrcn.st/pub
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 <[email protected]>
to both patches.
Sven
On Thu, Nov 18, 2021 at 12:14:49AM +0900, Hector Martin wrote:
> SPSS should've been SSPS.
>
> Signed-off-by: Hector Martin <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
> ---
> 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
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 <[email protected]>
Reviewed-by: Heikki Krogerus <[email protected]>
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
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 <[email protected]>
>
> Reviewed-by: Heikki Krogerus <[email protected]>
>
> Please resend these with the appropriate Fixes tag included.
Done, thanks for the review!
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub