On Tue, Oct 01, 2019 at 02:43:15PM +0000, Mario.Limonciello@xxxxxxxx wrote:
> > -----Original Message-----
> > From: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > Sent: Tuesday, October 1, 2019 6:39 AM
> > To: linux-usb@xxxxxxxxxxxxxxx
> > Cc: Andreas Noever; Michael Jamet; Mika Westerberg; Yehezkel Bernat; Rajmohan
> > Mani; Nicholas Johnson; Lukas Wunner; Greg Kroah-Hartman; Alan Stern;
> > Limonciello, Mario; Anthony Wong; linux-kernel@xxxxxxxxxxxxxxx
> > Subject: [RFC PATCH 22/22] thunderbolt: Do not start firmware unless asked by the
> > user
> >
> >
> > [EXTERNAL EMAIL]
> >
> > Since now we can do pretty much the same thing in the software
> > connection manager than the firmware would do, there is no point
> > starting it by default. Instead we can just continue using the software
> > connection manager.
> >
> > Make it possible for user to switch between the two by adding a module
> > pararameter (start_icm) which is by default false. Having this ability
> > to enable the firmware may be useful at least when debugging possible
> > issues with the software connection manager implementation.
>
> If the host system firmware didn't start the ICM, does that mean SW connection
> manager would just take over even on systems with discrete AR/TR controllers?
Yes. This is pretty much the case with Apple systems now.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> > ---
> > drivers/thunderbolt/icm.c | 14 +++++++++++---
> > drivers/thunderbolt/tb.c | 4 ----
> > 2 files changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
> > index 9c9c6ea2b790..c4a2de0f2a44 100644
> > --- a/drivers/thunderbolt/icm.c
> > +++ b/drivers/thunderbolt/icm.c
> > @@ -11,6 +11,7 @@
> >
> > #include <linux/delay.h>
> > #include <linux/mutex.h>
> > +#include <linux/moduleparam.h>
> > #include <linux/pci.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/platform_data/x86/apple.h>
> > @@ -43,6 +44,10 @@
> > #define ICM_APPROVE_TIMEOUT 10000 /* ms */
> > #define ICM_MAX_LINK 4
> >
> > +static bool start_icm;
> > +module_param(start_icm, bool, 0444);
> > +MODULE_PARM_DESC(start_icm, "start ICM firmware if it is not running (default:
> > false)");
> > +
> > /**
> > * struct icm - Internal connection manager private data
> > * @request_lock: Makes sure only one message is send to ICM at time
> > @@ -1353,13 +1358,16 @@ static bool icm_ar_is_supported(struct tb *tb)
> > {
> > struct pci_dev *upstream_port;
> > struct icm *icm = tb_priv(tb);
> > + u32 val;
> >
> > /*
> > * Starting from Alpine Ridge we can use ICM on Apple machines
> > * as well. We just need to reset and re-enable it first.
>
> This comment doesn't really seem as relevant anymore. The meaning of it
> has nothing to do with Apple anymore.
Actually it is still relevant. For USB4 the intent is to have FW/SW CM
switch in ACPI spec instead. But that is still under discussion.
> > + * However, only start it if explicitly asked by the user.
> > */
> > - if (!x86_apple_machine)
> > - return true;
> > + val = ioread32(tb->nhi->iobase + REG_FW_STS);
> > + if (!(val & REG_FW_STS_ICM_EN) && !start_icm)
> > + return false;
>
> This code is already in icm_firmware_start. Maybe split it to a small function
> So you can just have the more readable
>
> If (!icm_firmware_running(tb) && !start_icm))
Good point. I'll do that.
