* [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support
@ 2021-11-17 15:14 Hector Martin
2021-11-17 15:14 ` [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state Hector Martin
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Hector Martin @ 2021-11-17 15:14 UTC (permalink / raw)
To: Heikki Krogerus, Greg Kroah-Hartman, Sven Peter
Cc: Hector Martin, Guido Günther, Alyssa Rosenzweig, linux-usb,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state 2021-11-17 15:14 [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support Hector Martin @ 2021-11-17 15:14 ` Hector Martin 2021-11-17 16:03 ` Greg Kroah-Hartman 2021-11-18 13:05 ` Heikki Krogerus 2021-11-17 15:14 ` [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x Hector Martin 2021-11-17 18:19 ` [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support Sven Peter 2 siblings, 2 replies; 11+ messages in thread From: Hector Martin @ 2021-11-17 15:14 UTC (permalink / raw) To: Heikki Krogerus, Greg Kroah-Hartman, Sven Peter Cc: Hector Martin, Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state 2021-11-17 15:14 ` [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state Hector Martin @ 2021-11-17 16:03 ` Greg Kroah-Hartman 2021-11-17 16:07 ` Hector Martin 2021-11-18 13:05 ` Heikki Krogerus 1 sibling, 1 reply; 11+ messages in thread From: Greg Kroah-Hartman @ 2021-11-17 16:03 UTC (permalink / raw) To: Hector Martin Cc: Heikki Krogerus, Sven Peter, Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state 2021-11-17 16:03 ` Greg Kroah-Hartman @ 2021-11-17 16:07 ` Hector Martin 0 siblings, 0 replies; 11+ messages in thread From: Hector Martin @ 2021-11-17 16:07 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Heikki Krogerus, Sven Peter, Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state 2021-11-17 15:14 ` [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state Hector Martin 2021-11-17 16:03 ` Greg Kroah-Hartman @ 2021-11-18 13:05 ` Heikki Krogerus 1 sibling, 0 replies; 11+ messages in thread From: Heikki Krogerus @ 2021-11-18 13:05 UTC (permalink / raw) To: Hector Martin Cc: Greg Kroah-Hartman, Sven Peter, Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x 2021-11-17 15:14 [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support Hector Martin 2021-11-17 15:14 ` [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state Hector Martin @ 2021-11-17 15:14 ` Hector Martin 2021-11-17 16:04 ` Greg Kroah-Hartman 2021-11-18 13:12 ` Heikki Krogerus 2021-11-17 18:19 ` [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support Sven Peter 2 siblings, 2 replies; 11+ messages in thread From: Hector Martin @ 2021-11-17 15:14 UTC (permalink / raw) To: Heikki Krogerus, Greg Kroah-Hartman, Sven Peter Cc: Hector Martin, Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x 2021-11-17 15:14 ` [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x Hector Martin @ 2021-11-17 16:04 ` Greg Kroah-Hartman 2021-11-17 16:11 ` Hector Martin 2021-11-18 13:12 ` Heikki Krogerus 1 sibling, 1 reply; 11+ messages in thread From: Greg Kroah-Hartman @ 2021-11-17 16:04 UTC (permalink / raw) To: Hector Martin Cc: Heikki Krogerus, Sven Peter, Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x 2021-11-17 16:04 ` Greg Kroah-Hartman @ 2021-11-17 16:11 ` Hector Martin 0 siblings, 0 replies; 11+ messages in thread From: Hector Martin @ 2021-11-17 16:11 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Heikki Krogerus, Sven Peter, Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x 2021-11-17 15:14 ` [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x Hector Martin 2021-11-17 16:04 ` Greg Kroah-Hartman @ 2021-11-18 13:12 ` Heikki Krogerus 2021-11-20 3:08 ` Hector Martin 1 sibling, 1 reply; 11+ messages in thread From: Heikki Krogerus @ 2021-11-18 13:12 UTC (permalink / raw) To: Hector Martin Cc: Greg Kroah-Hartman, Sven Peter, Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x 2021-11-18 13:12 ` Heikki Krogerus @ 2021-11-20 3:08 ` Hector Martin 0 siblings, 0 replies; 11+ messages in thread From: Hector Martin @ 2021-11-20 3:08 UTC (permalink / raw) To: Heikki Krogerus Cc: Greg Kroah-Hartman, Sven Peter, Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support 2021-11-17 15:14 [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support Hector Martin 2021-11-17 15:14 ` [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state Hector Martin 2021-11-17 15:14 ` [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x Hector Martin @ 2021-11-17 18:19 ` Sven Peter 2 siblings, 0 replies; 11+ messages in thread From: Sven Peter @ 2021-11-17 18:19 UTC (permalink / raw) To: Hector Martin, Heikki Krogerus, Greg Kroah-Hartman Cc: Guido Günther, Alyssa Rosenzweig, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-11-20 3:08 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-17 15:14 [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support Hector Martin 2021-11-17 15:14 ` [PATCH 1/2] usb: typec: tipd: Fix typo in cd321x_switch_power_state Hector Martin 2021-11-17 16:03 ` Greg Kroah-Hartman 2021-11-17 16:07 ` Hector Martin 2021-11-18 13:05 ` Heikki Krogerus 2021-11-17 15:14 ` [PATCH 2/2] usb: typec: tipd: Fix initialization sequence for cd321x Hector Martin 2021-11-17 16:04 ` Greg Kroah-Hartman 2021-11-17 16:11 ` Hector Martin 2021-11-18 13:12 ` Heikki Krogerus 2021-11-20 3:08 ` Hector Martin 2021-11-17 18:19 ` [PATCH 0/2] usb: typec: tipd: Fixes for Apple M1 (CD321X) support Sven Peter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).