| Message ID | 20191001113830.13028-23-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thunderbolt: Add support for USB4 | expand |
> -----Original Message----- > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Sent: Tuesday, October 1, 2019 6:39 AM > To: linux-usb@vger.kernel.org > 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@vger.kernel.org > 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? > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > 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. > + * 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)) > > /* > * Find the upstream PCIe port in case we need to do reset > @@ -2224,7 +2232,7 @@ struct tb *icm_probe(struct tb_nhi *nhi) > > case PCI_DEVICE_ID_INTEL_ICL_NHI0: > case PCI_DEVICE_ID_INTEL_ICL_NHI1: > - icm->is_supported = icm_ar_is_supported; > + icm->is_supported = icm_fr_is_supported; > icm->driver_ready = icm_icl_driver_ready; > icm->set_uuid = icm_icl_set_uuid; > icm->device_connected = icm_icl_device_connected; > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > index 6c468ba96e9a..aebf2c10aa85 100644 > --- a/drivers/thunderbolt/tb.c > +++ b/drivers/thunderbolt/tb.c > @@ -9,7 +9,6 @@ > #include <linux/slab.h> > #include <linux/errno.h> > #include <linux/delay.h> > -#include <linux/platform_data/x86/apple.h> > > #include "tb.h" > #include "tb_regs.h" > @@ -1117,9 +1116,6 @@ struct tb *tb_probe(struct tb_nhi *nhi) > struct tb_cm *tcm; > struct tb *tb; > > - if (!x86_apple_machine) > - return NULL; > - > tb = tb_domain_alloc(nhi, sizeof(*tcm)); > if (!tb) > return NULL; > -- > 2.23.0
On Tue, Oct 01, 2019 at 02:43:15PM +0000, Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > Sent: Tuesday, October 1, 2019 6:39 AM > > To: linux-usb@vger.kernel.org > > 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@vger.kernel.org > > 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@linux.intel.com> > > --- > > 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.
> -----Original Message----- > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Sent: Tuesday, October 1, 2019 9:59 AM > To: Limonciello, Mario > Cc: linux-usb@vger.kernel.org; andreas.noever@gmail.com; > michael.jamet@intel.com; YehezkelShB@gmail.com; rajmohan.mani@intel.com; > nicholas.johnson-opensource@outlook.com.au; lukas@wunner.de; > gregkh@linuxfoundation.org; stern@rowland.harvard.edu; > anthony.wong@canonical.com; linux-kernel@vger.kernel.org > Subject: Re: [RFC PATCH 22/22] thunderbolt: Do not start firmware unless asked by > the user > > > [EXTERNAL EMAIL] > > On Tue, Oct 01, 2019 at 02:43:15PM +0000, Mario.Limonciello@dell.com wrote: > > > -----Original Message----- > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > Sent: Tuesday, October 1, 2019 6:39 AM > > > To: linux-usb@vger.kernel.org > > > 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@vger.kernel.org > > > 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. Potentially if system firmware started ICM can we accomplish the same thing by resetting AR/TR that normally use ICM and then SW CM would take over? Or is the ICM started up automatically when the controller is power cycled based on something in the NVM? I'm trying to find a way that I can usefully exercise some of this stuff on pre-USB4 controllers like AR/TR/ICL-TBT. > > > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > --- > > > 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. Like read a hint from an ACPI table that indicates which one driver should use? The idea being early USB4 systems would test and ship with this bit set to FW CM and later USB4 systems can have it set to SW CM? I like that idea, but I think that you almost want the module parameter to indicate which CM you want to "use" instead so you would override what was set in ACPI either way, but default to auto. cm=auto TBT3 and less: follow NVM behavior USB4: follow ACPI table, either use FW or SW CM. cm=icm Start up ICM, or error out if you can't. cm=sw Stop ICM if it's running, and initialize using kernel CM. That would certainly allow running this across some more configurations really easily then. > > > > + * 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.
On Tue, Oct 01, 2019 at 04:53:54PM +0000, Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > Sent: Tuesday, October 1, 2019 9:59 AM > > To: Limonciello, Mario > > Cc: linux-usb@vger.kernel.org; andreas.noever@gmail.com; > > michael.jamet@intel.com; YehezkelShB@gmail.com; rajmohan.mani@intel.com; > > nicholas.johnson-opensource@outlook.com.au; lukas@wunner.de; > > gregkh@linuxfoundation.org; stern@rowland.harvard.edu; > > anthony.wong@canonical.com; linux-kernel@vger.kernel.org > > Subject: Re: [RFC PATCH 22/22] thunderbolt: Do not start firmware unless asked by > > the user > > > > > > [EXTERNAL EMAIL] > > > > On Tue, Oct 01, 2019 at 02:43:15PM +0000, Mario.Limonciello@dell.com wrote: > > > > -----Original Message----- > > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > Sent: Tuesday, October 1, 2019 6:39 AM > > > > To: linux-usb@vger.kernel.org > > > > 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@vger.kernel.org > > > > 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. > > Potentially if system firmware started ICM can we accomplish the same thing by > resetting AR/TR that normally use ICM and then SW CM would take over? > > Or is the ICM started up automatically when the controller is power cycled based > on something in the NVM? It is something in the NVM :) > I'm trying to find a way that I can usefully exercise some of this stuff on pre-USB4 > controllers like AR/TR/ICL-TBT. Only way I can think of is to find yourself a Mac ;-) > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > > --- > > > > 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. > > Like read a hint from an ACPI table that indicates which one driver should use? Yes. I think it will be extension to _OSC but as it is still under discussion so subject to change. > The idea being early USB4 systems would test and ship with this bit set to > FW CM and later USB4 systems can have it set to SW CM? Correct. > I like that idea, but I think that you almost want the module parameter to indicate > which CM you want to "use" instead so you would override what was set in ACPI > either way, but default to auto. > > cm=auto > TBT3 and less: follow NVM behavior > USB4: follow ACPI table, either use FW or SW CM. > > cm=icm > Start up ICM, or error out if you can't. > > cm=sw > Stop ICM if it's running, and initialize using kernel CM. > > That would certainly allow running this across some more configurations really easily then. The point with start_icm is to act as "chicken bit" in case SW CM (Linux driver) does not work properly. Nothing else. User can then switch back to FW CM on their Apple system and get more "working" system hopefully reporting this to us so we can try to figure out what's wrong with the driver :) I don't think it is good idea to have a module parameter to control anything else that is supposed to be set in the BIOS/boot firmware.
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. + * 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; /* * Find the upstream PCIe port in case we need to do reset @@ -2224,7 +2232,7 @@ struct tb *icm_probe(struct tb_nhi *nhi) case PCI_DEVICE_ID_INTEL_ICL_NHI0: case PCI_DEVICE_ID_INTEL_ICL_NHI1: - icm->is_supported = icm_ar_is_supported; + icm->is_supported = icm_fr_is_supported; icm->driver_ready = icm_icl_driver_ready; icm->set_uuid = icm_icl_set_uuid; icm->device_connected = icm_icl_device_connected; diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c index 6c468ba96e9a..aebf2c10aa85 100644 --- a/drivers/thunderbolt/tb.c +++ b/drivers/thunderbolt/tb.c @@ -9,7 +9,6 @@ #include <linux/slab.h> #include <linux/errno.h> #include <linux/delay.h> -#include <linux/platform_data/x86/apple.h> #include "tb.h" #include "tb_regs.h" @@ -1117,9 +1116,6 @@ struct tb *tb_probe(struct tb_nhi *nhi) struct tb_cm *tcm; struct tb *tb; - if (!x86_apple_machine) - return NULL; - tb = tb_domain_alloc(nhi, sizeof(*tcm)); if (!tb) return NULL;
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. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/thunderbolt/icm.c | 14 +++++++++++--- drivers/thunderbolt/tb.c | 4 ---- 2 files changed, 11 insertions(+), 7 deletions(-)