| Message ID | 20191001113830.13028-2-mika.westerberg@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | thunderbolt: Add support for USB4 | expand |
On Tue, Oct 01, 2019 at 02:38:09PM +0300, Mika Westerberg wrote:
> We currently differentiate between SW CM and ICM by looking directly at
You should spell out what "SW CM" and "ICM" means please :)
thanks,
greg k-h
On Tue, Oct 01, 2019 at 02:10:05PM +0200, Greg Kroah-Hartman wrote: > On Tue, Oct 01, 2019 at 02:38:09PM +0300, Mika Westerberg wrote: > > We currently differentiate between SW CM and ICM by looking directly at > > You should spell out what "SW CM" and "ICM" means please :) Indeed, sorry about that. I will spell them out in next version. SW CM is Software Connection manager, essentially drivers/thunderbolt/tb.c. ICM is the Firmware Connection manager, essentially what is done in drivers/thunderbolt/icm.c. I think the I in ICM comes from Internal.
> -----Original Message----- > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Sent: Tuesday, October 1, 2019 7:47 AM > To: Greg Kroah-Hartman > Cc: linux-usb@vger.kernel.org; Andreas Noever; Michael Jamet; Yehezkel Bernat; > Rajmohan Mani; Nicholas Johnson; Lukas Wunner; Alan Stern; Limonciello, Mario; > Anthony Wong; linux-kernel@vger.kernel.org > Subject: Re: [RFC PATCH 01/22] thunderbolt: Introduce tb_switch_is_icm() > > > [EXTERNAL EMAIL] > > On Tue, Oct 01, 2019 at 02:10:05PM +0200, Greg Kroah-Hartman wrote: > > On Tue, Oct 01, 2019 at 02:38:09PM +0300, Mika Westerberg wrote: > > > We currently differentiate between SW CM and ICM by looking directly at > > > > You should spell out what "SW CM" and "ICM" means please :) > > Indeed, sorry about that. I will spell them out in next version. > > SW CM is Software Connection manager, essentially > drivers/thunderbolt/tb.c. In some of the documentation I've seen non-internal connection manager referred to as ECM, which I guess is an external connection manager? To be consistent with various documentations maybe that would be better than "SW CM". > > ICM is the Firmware Connection manager, essentially what is done in > drivers/thunderbolt/icm.c. I think the I in ICM comes from Internal.
On Tue, Oct 01, 2019 at 01:36:49PM +0000, Mario.Limonciello@dell.com wrote: > > -----Original Message----- > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > Sent: Tuesday, October 1, 2019 7:47 AM > > To: Greg Kroah-Hartman > > Cc: linux-usb@vger.kernel.org; Andreas Noever; Michael Jamet; Yehezkel Bernat; > > Rajmohan Mani; Nicholas Johnson; Lukas Wunner; Alan Stern; Limonciello, Mario; > > Anthony Wong; linux-kernel@vger.kernel.org > > Subject: Re: [RFC PATCH 01/22] thunderbolt: Introduce tb_switch_is_icm() > > > > > > [EXTERNAL EMAIL] > > > > On Tue, Oct 01, 2019 at 02:10:05PM +0200, Greg Kroah-Hartman wrote: > > > On Tue, Oct 01, 2019 at 02:38:09PM +0300, Mika Westerberg wrote: > > > > We currently differentiate between SW CM and ICM by looking directly at > > > > > > You should spell out what "SW CM" and "ICM" means please :) > > > > Indeed, sorry about that. I will spell them out in next version. > > > > SW CM is Software Connection manager, essentially > > drivers/thunderbolt/tb.c. > > In some of the documentation I've seen non-internal connection manager referred to > as ECM, which I guess is an external connection manager? To be consistent with various > documentations maybe that would be better than "SW CM". That's the first time I hear about ECM ;-) Here at Intel we use term "SW CM" and "FW CM" and IMHO they are better than ECM/ICM. But if people insist I can change them.
> -----Original Message----- > From: Mika Westerberg <mika.westerberg@linux.intel.com> > Sent: Tuesday, October 1, 2019 8:48 AM > To: Limonciello, Mario > Cc: gregkh@linuxfoundation.org; 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; stern@rowland.harvard.edu; anthony.wong@canonical.com; > linux-kernel@vger.kernel.org > Subject: Re: [RFC PATCH 01/22] thunderbolt: Introduce tb_switch_is_icm() > > > [EXTERNAL EMAIL] > > On Tue, Oct 01, 2019 at 01:36:49PM +0000, Mario.Limonciello@dell.com wrote: > > > -----Original Message----- > > > From: Mika Westerberg <mika.westerberg@linux.intel.com> > > > Sent: Tuesday, October 1, 2019 7:47 AM > > > To: Greg Kroah-Hartman > > > Cc: linux-usb@vger.kernel.org; Andreas Noever; Michael Jamet; Yehezkel > Bernat; > > > Rajmohan Mani; Nicholas Johnson; Lukas Wunner; Alan Stern; Limonciello, > Mario; > > > Anthony Wong; linux-kernel@vger.kernel.org > > > Subject: Re: [RFC PATCH 01/22] thunderbolt: Introduce tb_switch_is_icm() > > > > > > > > > [EXTERNAL EMAIL] > > > > > > On Tue, Oct 01, 2019 at 02:10:05PM +0200, Greg Kroah-Hartman wrote: > > > > On Tue, Oct 01, 2019 at 02:38:09PM +0300, Mika Westerberg wrote: > > > > > We currently differentiate between SW CM and ICM by looking directly at > > > > > > > > You should spell out what "SW CM" and "ICM" means please :) > > > > > > Indeed, sorry about that. I will spell them out in next version. > > > > > > SW CM is Software Connection manager, essentially > > > drivers/thunderbolt/tb.c. > > > > In some of the documentation I've seen non-internal connection manager > referred to > > as ECM, which I guess is an external connection manager? To be consistent with > various > > documentations maybe that would be better than "SW CM". > > That's the first time I hear about ECM ;-) > > Here at Intel we use term "SW CM" and "FW CM" and IMHO they are better > than ECM/ICM. But if people insist I can change them. I do agree with you, SW CM and FW CM are clearer than ECM/ICM, maybe just reference both in the comments so if someone is aware of ECM/ICM from some documents they can relate the two concepts.
On Tue, Oct 01, 2019 at 01:50:13PM +0000, Mario.Limonciello@dell.com wrote: > > Here at Intel we use term "SW CM" and "FW CM" and IMHO they are better > > than ECM/ICM. But if people insist I can change them. > > I do agree with you, SW CM and FW CM are clearer than ECM/ICM, maybe just reference > both in the comments so if someone is aware of ECM/ICM from some documents they > can relate the two concepts. Sure, I'll do that in the next version.
diff --git a/drivers/thunderbolt/lc.c b/drivers/thunderbolt/lc.c index ae1e92611c3e..af38076088f6 100644 --- a/drivers/thunderbolt/lc.c +++ b/drivers/thunderbolt/lc.c @@ -94,7 +94,7 @@ int tb_lc_configure_link(struct tb_switch *sw) struct tb_port *up, *down; int ret; - if (!sw->config.enabled || !tb_route(sw)) + if (!tb_route(sw) || tb_switch_is_icm(sw)) return 0; up = tb_upstream_port(sw); @@ -124,7 +124,7 @@ void tb_lc_unconfigure_link(struct tb_switch *sw) { struct tb_port *up, *down; - if (sw->is_unplugged || !sw->config.enabled || !tb_route(sw)) + if (sw->is_unplugged || !tb_route(sw) || tb_switch_is_icm(sw)) return; up = tb_upstream_port(sw); diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c index 5ea8db667e83..f9efd670d032 100644 --- a/drivers/thunderbolt/switch.c +++ b/drivers/thunderbolt/switch.c @@ -986,7 +986,7 @@ static int tb_plug_events_active(struct tb_switch *sw, bool active) u32 data; int res; - if (!sw->config.enabled) + if (tb_switch_is_icm(sw)) return 0; sw->config.plug_events_delay = 0xff; @@ -1710,7 +1710,7 @@ static int tb_switch_add_dma_port(struct tb_switch *sw) } /* Root switch DMA port requires running firmware */ - if (!tb_route(sw) && sw->config.enabled) + if (!tb_route(sw) && !tb_switch_is_icm(sw)) return 0; sw->dma_port = dma_port_alloc(sw); diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h index 6407d529871d..1565af2e48cb 100644 --- a/drivers/thunderbolt/tb.h +++ b/drivers/thunderbolt/tb.h @@ -591,6 +591,20 @@ static inline bool tb_switch_is_fr(const struct tb_switch *sw) } } +/** + * tb_switch_is_icm() - Is the switch handled by ICM firmware + * @sw: Switch to check + * + * In case there is a need to differentiate whether ICM firmware or SW CM + * is handling @sw this function can be called. It is valid to call this + * after tb_switch_alloc() and tb_switch_configure() has been called + * (latter only for SW CM case). + */ +static inline bool tb_switch_is_icm(const struct tb_switch *sw) +{ + return !sw->config.enabled; +} + int tb_wait_for_port(struct tb_port *port, bool wait_if_unplugged); int tb_port_add_nfc_credits(struct tb_port *port, int credits); int tb_port_set_initial_credits(struct tb_port *port, u32 credits);
We currently differentiate between SW CM and ICM by looking directly at the sw->config.enabled field which may be rather hard to understand for the casual reader. For this reason introduce a wrapper function with documentation that should make the intention more clear. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/thunderbolt/lc.c | 4 ++-- drivers/thunderbolt/switch.c | 4 ++-- drivers/thunderbolt/tb.h | 14 ++++++++++++++ 3 files changed, 18 insertions(+), 4 deletions(-)