Re: [RFC PATCH 22/22] thunderbolt: Do not start firmware unless asked by the user

[Mika Westerberg]

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.