Hi all,
With the exception of the first patch which is fix, this series enables
Thunderbolt on Intel Ice Lake. Biggest difference from the previous
controllers is that the Thunderbolt controller is now integrated as part of
the SoC. The firmware messages pretty much follow Titan Ridge but there are
some differences as well (such as the new RTD3 veto notification). Also Ice
Lake does not implement security levels so DMA protection is handled by IOMMU.
This is v5.4 material but I'm sending it out now because I will be on
vacation next 4 weeks mostly without internet access. When I get back I'll
gather all the comments and update the series accordingly.
Thanks!
Mika Westerberg (8):
thunderbolt: Correct path indices for PCIe tunnel
thunderbolt: Move NVM upgrade support flag to struct icm
thunderbolt: Use 32-bit writes when writing ring producer/consumer
thunderbolt: Do not fail adding switch if some port is not implemented
thunderbolt: Hide switch attributes that are not set
thunderbolt: Expose active parts of NVM even if upgrade is not supported
thunderbolt: Add support for Intel Ice Lake
ACPI / property: Add two new Thunderbolt property GUIDs to the list
drivers/acpi/property.c | 6 +
drivers/thunderbolt/ctl.c | 23 ++-
drivers/thunderbolt/icm.c | 169 +++++++++++++++++--
drivers/thunderbolt/nhi.c | 300 ++++++++++++++++++++++++++++++++-
drivers/thunderbolt/nhi.h | 2 +
drivers/thunderbolt/nhi_regs.h | 25 +++
drivers/thunderbolt/switch.c | 52 ++++--
drivers/thunderbolt/tb_msgs.h | 16 +-
drivers/thunderbolt/tunnel.c | 4 +-
include/linux/thunderbolt.h | 2 +
10 files changed, 553 insertions(+), 46 deletions(-)
--
2.20.1
PCIe tunnel path indices got mixed up when we added support for tunnels
between switches that are not adjacent. This did not affect the
functionality as it is just an index but fix it now nevertheless to make
the code easier to understand.
Reported-by: Rajmohan Mani <[email protected]>
Fixes: 8c7acaaf020f ("thunderbolt: Extend tunnel creation to more than 2 adjacent switches")
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/thunderbolt/tunnel.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/thunderbolt/tunnel.c b/drivers/thunderbolt/tunnel.c
index 31d0234837e4..5a99234826e7 100644
--- a/drivers/thunderbolt/tunnel.c
+++ b/drivers/thunderbolt/tunnel.c
@@ -211,7 +211,7 @@ struct tb_tunnel *tb_tunnel_alloc_pci(struct tb *tb, struct tb_port *up,
return NULL;
}
tb_pci_init_path(path);
- tunnel->paths[TB_PCI_PATH_UP] = path;
+ tunnel->paths[TB_PCI_PATH_DOWN] = path;
path = tb_path_alloc(tb, up, TB_PCI_HOPID, down, TB_PCI_HOPID, 0,
"PCIe Up");
@@ -220,7 +220,7 @@ struct tb_tunnel *tb_tunnel_alloc_pci(struct tb *tb, struct tb_port *up,
return NULL;
}
tb_pci_init_path(path);
- tunnel->paths[TB_PCI_PATH_DOWN] = path;
+ tunnel->paths[TB_PCI_PATH_UP] = path;
return tunnel;
}
--
2.20.1
This is depends on the controller and on the platform/CPU we are
running. Move it to struct icm so we can set it per controller.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/thunderbolt/icm.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/thunderbolt/icm.c b/drivers/thunderbolt/icm.c
index fbdcef56a676..2a56d9478b34 100644
--- a/drivers/thunderbolt/icm.c
+++ b/drivers/thunderbolt/icm.c
@@ -55,6 +55,7 @@
* @safe_mode: ICM is in safe mode
* @max_boot_acl: Maximum number of preboot ACL entries (%0 if not supported)
* @rpm: Does the controller support runtime PM (RTD3)
+ * @can_upgrade_nvm: Can the NVM firmware be upgrade on this controller
* @is_supported: Checks if we can support ICM on this controller
* @cio_reset: Trigger CIO reset
* @get_mode: Read and return the ICM firmware mode (optional)
@@ -74,6 +75,7 @@ struct icm {
int vnd_cap;
bool safe_mode;
bool rpm;
+ bool can_upgrade_nvm;
bool (*is_supported)(struct tb *tb);
int (*cio_reset)(struct tb *tb);
int (*get_mode)(struct tb *tb);
@@ -1913,12 +1915,7 @@ static int icm_start(struct tb *tb)
if (IS_ERR(tb->root_switch))
return PTR_ERR(tb->root_switch);
- /*
- * NVM upgrade has not been tested on Apple systems and they
- * don't provide images publicly either. To be on the safe side
- * prevent root switch NVM upgrade on Macs for now.
- */
- tb->root_switch->no_nvm_upgrade = x86_apple_machine;
+ tb->root_switch->no_nvm_upgrade = !icm->can_upgrade_nvm;
tb->root_switch->rpm = icm->rpm;
ret = tb_switch_add(tb->root_switch);
@@ -2021,6 +2018,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
switch (nhi->pdev->device) {
case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI:
case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI:
+ icm->can_upgrade_nvm = true;
icm->is_supported = icm_fr_is_supported;
icm->get_route = icm_fr_get_route;
icm->save_devices = icm_fr_save_devices;
@@ -2038,6 +2036,13 @@ struct tb *icm_probe(struct tb_nhi *nhi)
case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI:
case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI:
icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
+ /*
+ * NVM upgrade has not been tested on Apple systems and
+ * they don't provide images publicly either. To be on
+ * the safe side prevent root switch NVM upgrade on Macs
+ * for now.
+ */
+ icm->can_upgrade_nvm = !x86_apple_machine;
icm->is_supported = icm_ar_is_supported;
icm->cio_reset = icm_ar_cio_reset;
icm->get_mode = icm_ar_get_mode;
@@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
+ icm->can_upgrade_nvm = true;
icm->is_supported = icm_ar_is_supported;
icm->cio_reset = icm_tr_cio_reset;
icm->get_mode = icm_ar_get_mode;
--
2.20.1
Ice Lake Thunderbolt controller NVM firmware is part of the BIOS image
which means it is not writable through the DMA port anymore. However, we
can still read it so we can keep nvm_version and active parts of NVM.
This way users still can find out the active NVM version and other
potentially useful information directly from Linux.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/thunderbolt/switch.c | 28 +++++++++++++++++++---------
1 file changed, 19 insertions(+), 9 deletions(-)
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index e84067084dcd..e318e9714a38 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -364,12 +364,14 @@ static int tb_switch_nvm_add(struct tb_switch *sw)
nvm->active = nvm_dev;
}
- nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
- if (IS_ERR(nvm_dev)) {
- ret = PTR_ERR(nvm_dev);
- goto err_nvm_active;
+ if (!sw->no_nvm_upgrade) {
+ nvm_dev = register_nvmem(sw, nvm->id, NVM_MAX_SIZE, false);
+ if (IS_ERR(nvm_dev)) {
+ ret = PTR_ERR(nvm_dev);
+ goto err_nvm_active;
+ }
+ nvm->non_active = nvm_dev;
}
- nvm->non_active = nvm_dev;
sw->nvm = nvm;
return 0;
@@ -398,7 +400,8 @@ static void tb_switch_nvm_remove(struct tb_switch *sw)
if (!nvm->authenticating)
nvm_clear_auth_status(sw);
- nvmem_unregister(nvm->non_active);
+ if (nvm->non_active)
+ nvmem_unregister(nvm->non_active);
if (nvm->active)
nvmem_unregister(nvm->active);
ida_simple_remove(&nvm_ida, nvm->id);
@@ -1355,8 +1358,11 @@ static umode_t switch_attr_is_visible(struct kobject *kobj,
sw->security_level == TB_SECURITY_SECURE)
return attr->mode;
return 0;
- } else if (attr == &dev_attr_nvm_authenticate.attr ||
- attr == &dev_attr_nvm_version.attr) {
+ } else if (attr == &dev_attr_nvm_authenticate.attr) {
+ if (sw->dma_port && !sw->no_nvm_upgrade)
+ return attr->mode;
+ return 0;
+ } else if (attr == &dev_attr_nvm_version.attr) {
if (sw->dma_port)
return attr->mode;
return 0;
@@ -1707,13 +1713,17 @@ static int tb_switch_add_dma_port(struct tb_switch *sw)
break;
}
- if (sw->no_nvm_upgrade)
+ /* Root switch DMA port requires running firmware */
+ if (!tb_route(sw) && sw->config.enabled)
return 0;
sw->dma_port = dma_port_alloc(sw);
if (!sw->dma_port)
return 0;
+ if (sw->no_nvm_upgrade)
+ return 0;
+
/*
* Check status of the previous flash authentication. If there
* is one we need to power cycle the switch in any case to make
--
2.20.1
Thunderbolt host routers may not always contain DROM that includes
device identification information. This is mostly needed for Ice Lake
systems but some Falcon Ridge controllers on PCs also do not have DROM.
In that case hide the identification attributes.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/thunderbolt/switch.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index eac62ff1b85c..e84067084dcd 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -1337,7 +1337,19 @@ static umode_t switch_attr_is_visible(struct kobject *kobj,
struct device *dev = container_of(kobj, struct device, kobj);
struct tb_switch *sw = tb_to_switch(dev);
- if (attr == &dev_attr_key.attr) {
+ if (attr == &dev_attr_device.attr) {
+ if (!sw->device)
+ return 0;
+ } else if (attr == &dev_attr_device_name.attr) {
+ if (!sw->device_name)
+ return 0;
+ } else if (attr == &dev_attr_vendor.attr) {
+ if (!sw->vendor)
+ return 0;
+ } else if (attr == &dev_attr_vendor_name.attr) {
+ if (!sw->vendor_name)
+ return 0;
+ } else if (attr == &dev_attr_key.attr) {
if (tb_route(sw) &&
sw->tb->security_level == TB_SECURITY_SECURE &&
sw->security_level == TB_SECURITY_SECURE)
--
2.20.1
The register access should be using 32-bit reads/writes according to the
datasheet. With the previous generation hardware 16-bit writes have been
working but starting with ICL this is not the case anymore so fix
producer/consumer register update to use correct width register address.
Signed-off-by: Mika Westerberg <[email protected]>
---
drivers/thunderbolt/nhi.c | 26 ++++++++++++++++++++++----
1 file changed, 22 insertions(+), 4 deletions(-)
diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
index 27fbe62c7ddd..09242653da67 100644
--- a/drivers/thunderbolt/nhi.c
+++ b/drivers/thunderbolt/nhi.c
@@ -143,9 +143,24 @@ static void __iomem *ring_options_base(struct tb_ring *ring)
return io;
}
-static void ring_iowrite16desc(struct tb_ring *ring, u32 value, u32 offset)
+static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
{
- iowrite16(value, ring_desc_base(ring) + offset);
+ u32 val;
+
+ val = ioread32(ring_desc_base(ring) + 8);
+ val &= 0x0000ffff;
+ val |= prod << 16;
+ iowrite32(val, ring_desc_base(ring) + 8);
+}
+
+static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
+{
+ u32 val;
+
+ val = ioread32(ring_desc_base(ring) + 8);
+ val &= 0xffff0000;
+ val |= cons;
+ iowrite32(val, ring_desc_base(ring) + 8);
}
static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
@@ -197,7 +212,10 @@ static void ring_write_descriptors(struct tb_ring *ring)
descriptor->sof = frame->sof;
}
ring->head = (ring->head + 1) % ring->size;
- ring_iowrite16desc(ring, ring->head, ring->is_tx ? 10 : 8);
+ if (ring->is_tx)
+ ring_iowrite_prod(ring, ring->head);
+ else
+ ring_iowrite_cons(ring, ring->head);
}
}
@@ -662,7 +680,7 @@ void tb_ring_stop(struct tb_ring *ring)
ring_iowrite32options(ring, 0, 0);
ring_iowrite64desc(ring, 0, 0);
- ring_iowrite16desc(ring, 0, ring->is_tx ? 10 : 8);
+ ring_iowrite32desc(ring, 0, 8);
ring_iowrite32desc(ring, 0, 12);
ring->head = 0;
ring->tail = 0;
--
2.20.1
On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
<[email protected]> wrote:
>
> @@ -1913,12 +1915,7 @@ static int icm_start(struct tb *tb)
> if (IS_ERR(tb->root_switch))
> return PTR_ERR(tb->root_switch);
>
> - /*
> - * NVM upgrade has not been tested on Apple systems and they
> - * don't provide images publicly either. To be on the safe side
> - * prevent root switch NVM upgrade on Macs for now.
> - */
> - tb->root_switch->no_nvm_upgrade = x86_apple_machine;
> + tb->root_switch->no_nvm_upgrade = !icm->can_upgrade_nvm;
> tb->root_switch->rpm = icm->rpm;
>
> ret = tb_switch_add(tb->root_switch);
> @@ -2021,6 +2018,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> switch (nhi->pdev->device) {
> case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_2C_NHI:
> case PCI_DEVICE_ID_INTEL_FALCON_RIDGE_4C_NHI:
> + icm->can_upgrade_nvm = true;
> icm->is_supported = icm_fr_is_supported;
> icm->get_route = icm_fr_get_route;
> icm->save_devices = icm_fr_save_devices;
> @@ -2038,6 +2036,13 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_4C_NHI:
> case PCI_DEVICE_ID_INTEL_ALPINE_RIDGE_C_2C_NHI:
> icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> + /*
> + * NVM upgrade has not been tested on Apple systems and
> + * they don't provide images publicly either. To be on
> + * the safe side prevent root switch NVM upgrade on Macs
> + * for now.
> + */
> + icm->can_upgrade_nvm = !x86_apple_machine;
> icm->is_supported = icm_ar_is_supported;
> icm->cio_reset = icm_ar_cio_reset;
> icm->get_mode = icm_ar_get_mode;
> @@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
> case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
> icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> + icm->can_upgrade_nvm = true;
Shouldn't this be also !x86_apple_machine just like AR?
(For FR, we don't use ICM on Apple machines, as much as I remember, so it's fine
to enable it there unconditionally for ICM code path.)
> icm->is_supported = icm_ar_is_supported;
> icm->cio_reset = icm_tr_cio_reset;
> icm->get_mode = icm_ar_get_mode;
> --
> 2.20.1
>
On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
<[email protected]> wrote:
>
> The register access should be using 32-bit reads/writes according to the
> datasheet. With the previous generation hardware 16-bit writes have been
> working but starting with ICL this is not the case anymore so fix
> producer/consumer register update to use correct width register address.
>
> Signed-off-by: Mika Westerberg <[email protected]>
> ---
> drivers/thunderbolt/nhi.c | 26 ++++++++++++++++++++++----
> 1 file changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> index 27fbe62c7ddd..09242653da67 100644
> --- a/drivers/thunderbolt/nhi.c
> +++ b/drivers/thunderbolt/nhi.c
> @@ -143,9 +143,24 @@ static void __iomem *ring_options_base(struct tb_ring *ring)
> return io;
> }
>
> -static void ring_iowrite16desc(struct tb_ring *ring, u32 value, u32 offset)
> +static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
> {
> - iowrite16(value, ring_desc_base(ring) + offset);
> + u32 val;
> +
> + val = ioread32(ring_desc_base(ring) + 8);
> + val &= 0x0000ffff;
> + val |= prod << 16;
> + iowrite32(val, ring_desc_base(ring) + 8);
> +}
> +
> +static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
> +{
> + u32 val;
> +
> + val = ioread32(ring_desc_base(ring) + 8);
> + val &= 0xffff0000;
> + val |= cons;
> + iowrite32(val, ring_desc_base(ring) + 8);
> }
>
> static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
> @@ -197,7 +212,10 @@ static void ring_write_descriptors(struct tb_ring *ring)
> descriptor->sof = frame->sof;
> }
> ring->head = (ring->head + 1) % ring->size;
> - ring_iowrite16desc(ring, ring->head, ring->is_tx ? 10 : 8);
> + if (ring->is_tx)
> + ring_iowrite_prod(ring, ring->head);
> + else
> + ring_iowrite_cons(ring, ring->head);
Really a matter of taste, but maybe you want to consider having a single
function, with a 3rd parameter, bool is_tx.
The calls here will be unified to:
ring_iowrite(ring, ring->head, ring->is_tx);
(No condition is needed here).
The implementation uses the new parameter to decide which part of the register
to mask, reducing the code duplication (in my eyes):
val = ioread32(ring_desc_base(ring) + 8);
if (is_tx) {
val &= 0x0000ffff;
val |= value << 16;
} else {
val &= 0xffff0000;
val |= value;
}
iowrite32(val, ring_desc_base(ring) + 8);
I'm not sure if it improves the readability or makes it worse. Your call.
On Fri, Jul 05, 2019 at 12:57:52PM +0300, Mika Westerberg wrote:
> Hi all,
>
> With the exception of the first patch which is fix, this series enables
> Thunderbolt on Intel Ice Lake. Biggest difference from the previous
> controllers is that the Thunderbolt controller is now integrated as part of
> the SoC. The firmware messages pretty much follow Titan Ridge but there are
> some differences as well (such as the new RTD3 veto notification). Also Ice
> Lake does not implement security levels so DMA protection is handled by IOMMU.
>
> This is v5.4 material but I'm sending it out now because I will be on
> vacation next 4 weeks mostly without internet access. When I get back I'll
> gather all the comments and update the series accordingly.
>
> Thanks!
>
> Mika Westerberg (8):
> thunderbolt: Correct path indices for PCIe tunnel
> thunderbolt: Move NVM upgrade support flag to struct icm
> thunderbolt: Use 32-bit writes when writing ring producer/consumer
> thunderbolt: Do not fail adding switch if some port is not implemented
> thunderbolt: Hide switch attributes that are not set
> thunderbolt: Expose active parts of NVM even if upgrade is not supported
> thunderbolt: Add support for Intel Ice Lake
> ACPI / property: Add two new Thunderbolt property GUIDs to the list
Forgot to Cc Raanan and Raj, now added. Sorry about that. The patch
series can also be viewed here:
https://lore.kernel.org/lkml/[email protected]/T/#m9cb5a393dfc79f1c2212d0787b6bad5b689db6bd
On Fri, Jul 05, 2019 at 01:52:49PM +0300, Yehezkel Bernat wrote:
> > @@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> > case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
> > case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
> > icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> > + icm->can_upgrade_nvm = true;
>
> Shouldn't this be also !x86_apple_machine just like AR?
> (For FR, we don't use ICM on Apple machines, as much as I remember, so it's fine
> to enable it there unconditionally for ICM code path.)
Yes, good point. I'll fix it up.
On Fri, Jul 05, 2019 at 02:09:44PM +0300, Yehezkel Bernat wrote:
> On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
> <[email protected]> wrote:
> >
> > The register access should be using 32-bit reads/writes according to the
> > datasheet. With the previous generation hardware 16-bit writes have been
> > working but starting with ICL this is not the case anymore so fix
> > producer/consumer register update to use correct width register address.
> >
> > Signed-off-by: Mika Westerberg <[email protected]>
> > ---
> > drivers/thunderbolt/nhi.c | 26 ++++++++++++++++++++++----
> > 1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > index 27fbe62c7ddd..09242653da67 100644
> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > @@ -143,9 +143,24 @@ static void __iomem *ring_options_base(struct tb_ring *ring)
> > return io;
> > }
> >
> > -static void ring_iowrite16desc(struct tb_ring *ring, u32 value, u32 offset)
> > +static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
> > {
> > - iowrite16(value, ring_desc_base(ring) + offset);
> > + u32 val;
> > +
> > + val = ioread32(ring_desc_base(ring) + 8);
> > + val &= 0x0000ffff;
> > + val |= prod << 16;
> > + iowrite32(val, ring_desc_base(ring) + 8);
> > +}
> > +
> > +static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
> > +{
> > + u32 val;
> > +
> > + val = ioread32(ring_desc_base(ring) + 8);
> > + val &= 0xffff0000;
> > + val |= cons;
> > + iowrite32(val, ring_desc_base(ring) + 8);
> > }
> >
> > static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
> > @@ -197,7 +212,10 @@ static void ring_write_descriptors(struct tb_ring *ring)
> > descriptor->sof = frame->sof;
> > }
> > ring->head = (ring->head + 1) % ring->size;
> > - ring_iowrite16desc(ring, ring->head, ring->is_tx ? 10 : 8);
> > + if (ring->is_tx)
> > + ring_iowrite_prod(ring, ring->head);
> > + else
> > + ring_iowrite_cons(ring, ring->head);
>
> Really a matter of taste, but maybe you want to consider having a single
> function, with a 3rd parameter, bool is_tx.
> The calls here will be unified to:
> ring_iowrite(ring, ring->head, ring->is_tx);
> (No condition is needed here).
I like the idea. We could even make it
ring_iowrite_prod_cons(ring);
and then use ring->head and ring->is_tx inside.
On Fri, Jul 5, 2019 at 1:33 PM Mika Westerberg
<[email protected]> wrote:
>
> On Fri, Jul 05, 2019 at 12:57:52PM +0300, Mika Westerberg wrote:
> > Hi all,
> >
> > With the exception of the first patch which is fix, this series enables
> > Thunderbolt on Intel Ice Lake. Biggest difference from the previous
> > controllers is that the Thunderbolt controller is now integrated as part of
> > the SoC. The firmware messages pretty much follow Titan Ridge but there are
> > some differences as well (such as the new RTD3 veto notification). Also Ice
> > Lake does not implement security levels so DMA protection is handled by IOMMU.
> >
> > This is v5.4 material but I'm sending it out now because I will be on
> > vacation next 4 weeks mostly without internet access. When I get back I'll
> > gather all the comments and update the series accordingly.
> >
> > Thanks!
> >
> > Mika Westerberg (8):
> > thunderbolt: Correct path indices for PCIe tunnel
> > thunderbolt: Move NVM upgrade support flag to struct icm
> > thunderbolt: Use 32-bit writes when writing ring producer/consumer
> > thunderbolt: Do not fail adding switch if some port is not implemented
> > thunderbolt: Hide switch attributes that are not set
> > thunderbolt: Expose active parts of NVM even if upgrade is not supported
> > thunderbolt: Add support for Intel Ice Lake
> > ACPI / property: Add two new Thunderbolt property GUIDs to the list
>
> Forgot to Cc Raanan and Raj, now added. Sorry about that. The patch
> series can also be viewed here:
>
> https://lore.kernel.org/lkml/[email protected]/T/#m9cb5a393dfc79f1c2212d0787b6bad5b689db6bd
Besides a few comments, LGTM.
For Thunderbolt patches,
Reviewed-by: Yehezkel Bernat <[email protected]>
From: Yehezkel Bernat
> Sent: 05 July 2019 12:10
> On Fri, Jul 5, 2019 at 12:58 PM Mika Westerberg
> <[email protected]> wrote:
> >
> > The register access should be using 32-bit reads/writes according to the
> > datasheet. With the previous generation hardware 16-bit writes have been
> > working but starting with ICL this is not the case anymore so fix
> > producer/consumer register update to use correct width register address.
> >
> > Signed-off-by: Mika Westerberg <[email protected]>
> > ---
> > drivers/thunderbolt/nhi.c | 26 ++++++++++++++++++++++----
> > 1 file changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/thunderbolt/nhi.c b/drivers/thunderbolt/nhi.c
> > index 27fbe62c7ddd..09242653da67 100644
> > --- a/drivers/thunderbolt/nhi.c
> > +++ b/drivers/thunderbolt/nhi.c
> > @@ -143,9 +143,24 @@ static void __iomem *ring_options_base(struct tb_ring *ring)
> > return io;
> > }
> >
> > -static void ring_iowrite16desc(struct tb_ring *ring, u32 value, u32 offset)
> > +static void ring_iowrite_prod(struct tb_ring *ring, u16 prod)
> > {
> > - iowrite16(value, ring_desc_base(ring) + offset);
> > + u32 val;
> > +
> > + val = ioread32(ring_desc_base(ring) + 8);
> > + val &= 0x0000ffff;
> > + val |= prod << 16;
> > + iowrite32(val, ring_desc_base(ring) + 8);
> > +}
> > +
> > +static void ring_iowrite_cons(struct tb_ring *ring, u16 cons)
> > +{
> > + u32 val;
> > +
> > + val = ioread32(ring_desc_base(ring) + 8);
> > + val &= 0xffff0000;
> > + val |= cons;
> > + iowrite32(val, ring_desc_base(ring) + 8);
> > }
> >
> > static void ring_iowrite32desc(struct tb_ring *ring, u32 value, u32 offset)
> > @@ -197,7 +212,10 @@ static void ring_write_descriptors(struct tb_ring *ring)
> > descriptor->sof = frame->sof;
> > }
> > ring->head = (ring->head + 1) % ring->size;
> > - ring_iowrite16desc(ring, ring->head, ring->is_tx ? 10 : 8);
> > + if (ring->is_tx)
> > + ring_iowrite_prod(ring, ring->head);
> > + else
> > + ring_iowrite_cons(ring, ring->head);
>
> Really a matter of taste, but maybe you want to consider having a single
> function, with a 3rd parameter, bool is_tx.
> The calls here will be unified to:
> ring_iowrite(ring, ring->head, ring->is_tx);
> (No condition is needed here).
>
> The implementation uses the new parameter to decide which part of the register
> to mask, reducing the code duplication (in my eyes):
>
> val = ioread32(ring_desc_base(ring) + 8);
> if (is_tx) {
> val &= 0x0000ffff;
> val |= value << 16;
> } else {
> val &= 0xffff0000;
> val |= value;
> }
> iowrite32(val, ring_desc_base(ring) + 8);
>
> I'm not sure if it improves the readability or makes it worse. Your call.
Gah, that is all horrid beyond belief.
If a 32bit write is valid then the hardware must not be updating
the other 16 bits.
In which case the driver knows what they should be.
So it can do a single 32bit write of the required value.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
> -----Original Message-----
> From: Mika Westerberg <[email protected]>
> Sent: Friday, July 5, 2019 5:58 AM
> To: Yehezkel Bernat
> Cc: LKML; Andreas Noever; Michael Jamet; Rafael J . Wysocki; Len Brown; Lukas
> Wunner; Limonciello, Mario; Anthony Wong; [email protected]
> Subject: Re: [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct
> icm
>
>
> [EXTERNAL EMAIL]
>
> On Fri, Jul 05, 2019 at 01:52:49PM +0300, Yehezkel Bernat wrote:
> > > @@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> > > case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
> > > case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
> > > icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> > > + icm->can_upgrade_nvm = true;
> >
> > Shouldn't this be also !x86_apple_machine just like AR?
> > (For FR, we don't use ICM on Apple machines, as much as I remember, so it's fine
> > to enable it there unconditionally for ICM code path.)
>
> Yes, good point. I'll fix it up.
Another thought - does the TR or AR ID's setting can_upgrade_nvm to !x86_apple_machine
show up in anything like a dock or is it only host controllers? If it's in docks, then it might be worth
only blocking on apple if it's a host.
On Tue, Jul 09, 2019 at 03:11:15PM +0000, [email protected] wrote:
> > -----Original Message-----
> > From: Mika Westerberg <[email protected]>
> > Sent: Friday, July 5, 2019 5:58 AM
> > To: Yehezkel Bernat
> > Cc: LKML; Andreas Noever; Michael Jamet; Rafael J . Wysocki; Len Brown; Lukas
> > Wunner; Limonciello, Mario; Anthony Wong; [email protected]
> > Subject: Re: [PATCH 2/8] thunderbolt: Move NVM upgrade support flag to struct
> > icm
> >
> >
> > [EXTERNAL EMAIL]
> >
> > On Fri, Jul 05, 2019 at 01:52:49PM +0300, Yehezkel Bernat wrote:
> > > > @@ -2054,6 +2059,7 @@ struct tb *icm_probe(struct tb_nhi *nhi)
> > > > case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_2C_NHI:
> > > > case PCI_DEVICE_ID_INTEL_TITAN_RIDGE_4C_NHI:
> > > > icm->max_boot_acl = ICM_AR_PREBOOT_ACL_ENTRIES;
> > > > + icm->can_upgrade_nvm = true;
> > >
> > > Shouldn't this be also !x86_apple_machine just like AR?
> > > (For FR, we don't use ICM on Apple machines, as much as I remember, so it's fine
> > > to enable it there unconditionally for ICM code path.)
> >
> > Yes, good point. I'll fix it up.
>
> Another thought - does the TR or AR ID's setting can_upgrade_nvm to !x86_apple_machine
> show up in anything like a dock or is it only host controllers? If it's in docks, then it might be worth
> only blocking on apple if it's a host.
It affects only hosts so on Apple system you can't upgrade host NVM but
docks and other devices you can.
From: Mika Westerberg
> Sent: 07 August 2019 17:14
> To: David Laight
>
> On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > Really a matter of taste, but maybe you want to consider having a single
> > > function, with a 3rd parameter, bool is_tx.
> > > The calls here will be unified to:
> > > ring_iowrite(ring, ring->head, ring->is_tx);
> > > (No condition is needed here).
> > >
> > > The implementation uses the new parameter to decide which part of the register
> > > to mask, reducing the code duplication (in my eyes):
> > >
> > > val = ioread32(ring_desc_base(ring) + 8);
> > > if (is_tx) {
> > > val &= 0x0000ffff;
> > > val |= value << 16;
> > > } else {
> > > val &= 0xffff0000;
> > > val |= value;
> > > }
> > > iowrite32(val, ring_desc_base(ring) + 8);
> > >
> > > I'm not sure if it improves the readability or makes it worse. Your call.
> >
> > Gah, that is all horrid beyond belief.
> > If a 32bit write is valid then the hardware must not be updating
> > the other 16 bits.
> > In which case the driver knows what they should be.
> > So it can do a single 32bit write of the required value.
>
> I'm not entirely sure I understand what you say above. Can you shed some
> light on this by a concrete example how it should look like? :-)
The driver must know both the tx and rx ring values, so:
iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);
The ioread32() is likely to be very slow - you only want to do them
if absolutely necessary.
The speed of the iowrite32() doesn't matter (much) since it is almost
certainly 'posted' and execution continues while the bus cycle is
in progress.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > Really a matter of taste, but maybe you want to consider having a single
> > function, with a 3rd parameter, bool is_tx.
> > The calls here will be unified to:
> > ring_iowrite(ring, ring->head, ring->is_tx);
> > (No condition is needed here).
> >
> > The implementation uses the new parameter to decide which part of the register
> > to mask, reducing the code duplication (in my eyes):
> >
> > val = ioread32(ring_desc_base(ring) + 8);
> > if (is_tx) {
> > val &= 0x0000ffff;
> > val |= value << 16;
> > } else {
> > val &= 0xffff0000;
> > val |= value;
> > }
> > iowrite32(val, ring_desc_base(ring) + 8);
> >
> > I'm not sure if it improves the readability or makes it worse. Your call.
>
> Gah, that is all horrid beyond belief.
> If a 32bit write is valid then the hardware must not be updating
> the other 16 bits.
> In which case the driver knows what they should be.
> So it can do a single 32bit write of the required value.
I'm not entirely sure I understand what you say above. Can you shed some
light on this by a concrete example how it should look like? :-)
From: 'Mika Westerberg' [mailto:[email protected]]
> Sent: 07 August 2019 17:36
>
> On Wed, Aug 07, 2019 at 04:22:26PM +0000, David Laight wrote:
> > From: Mika Westerberg
> > > Sent: 07 August 2019 17:14
> > > To: David Laight
> > >
> > > On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > > > Really a matter of taste, but maybe you want to consider having a single
> > > > > function, with a 3rd parameter, bool is_tx.
> > > > > The calls here will be unified to:
> > > > > ring_iowrite(ring, ring->head, ring->is_tx);
> > > > > (No condition is needed here).
> > > > >
> > > > > The implementation uses the new parameter to decide which part of the register
> > > > > to mask, reducing the code duplication (in my eyes):
> > > > >
> > > > > val = ioread32(ring_desc_base(ring) + 8);
> > > > > if (is_tx) {
> > > > > val &= 0x0000ffff;
> > > > > val |= value << 16;
> > > > > } else {
> > > > > val &= 0xffff0000;
> > > > > val |= value;
> > > > > }
> > > > > iowrite32(val, ring_desc_base(ring) + 8);
> > > > >
> > > > > I'm not sure if it improves the readability or makes it worse. Your call.
> > > >
> > > > Gah, that is all horrid beyond belief.
> > > > If a 32bit write is valid then the hardware must not be updating
> > > > the other 16 bits.
> > > > In which case the driver knows what they should be.
> > > > So it can do a single 32bit write of the required value.
> > >
> > > I'm not entirely sure I understand what you say above. Can you shed some
> > > light on this by a concrete example how it should look like? :-)
> >
> > The driver must know both the tx and rx ring values, so:
> > iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);
> >
>
> I see. However, prod or cons side gets updated by the hardware as it
> processes buffers and other side is only updated by the driver. I'm not
> sure the above works here.
If the hardware updates the other half of the 32bit word it doesn't ever work.
In that case you must do 16bit writes.
If the hardware is ignoring the byte-enables it is broken and unusable.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Wed, Aug 07, 2019 at 04:22:26PM +0000, David Laight wrote:
> From: Mika Westerberg
> > Sent: 07 August 2019 17:14
> > To: David Laight
> >
> > On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > > Really a matter of taste, but maybe you want to consider having a single
> > > > function, with a 3rd parameter, bool is_tx.
> > > > The calls here will be unified to:
> > > > ring_iowrite(ring, ring->head, ring->is_tx);
> > > > (No condition is needed here).
> > > >
> > > > The implementation uses the new parameter to decide which part of the register
> > > > to mask, reducing the code duplication (in my eyes):
> > > >
> > > > val = ioread32(ring_desc_base(ring) + 8);
> > > > if (is_tx) {
> > > > val &= 0x0000ffff;
> > > > val |= value << 16;
> > > > } else {
> > > > val &= 0xffff0000;
> > > > val |= value;
> > > > }
> > > > iowrite32(val, ring_desc_base(ring) + 8);
> > > >
> > > > I'm not sure if it improves the readability or makes it worse. Your call.
> > >
> > > Gah, that is all horrid beyond belief.
> > > If a 32bit write is valid then the hardware must not be updating
> > > the other 16 bits.
> > > In which case the driver knows what they should be.
> > > So it can do a single 32bit write of the required value.
> >
> > I'm not entirely sure I understand what you say above. Can you shed some
> > light on this by a concrete example how it should look like? :-)
>
> The driver must know both the tx and rx ring values, so:
> iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);
>
I see. However, prod or cons side gets updated by the hardware as it
processes buffers and other side is only updated by the driver. I'm not
sure the above works here.
> The ioread32() is likely to be very slow - you only want to do them
> if absolutely necessary.
> The speed of the iowrite32() doesn't matter (much) since it is almost
> certainly 'posted' and execution continues while the bus cycle is
> in progress.
OK thanks for the explanation.
On Wed, Aug 07, 2019 at 04:41:30PM +0000, David Laight wrote:
> From: 'Mika Westerberg' [mailto:[email protected]]
> > Sent: 07 August 2019 17:36
> >
> > On Wed, Aug 07, 2019 at 04:22:26PM +0000, David Laight wrote:
> > > From: Mika Westerberg
> > > > Sent: 07 August 2019 17:14
> > > > To: David Laight
> > > >
> > > > On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > > > > Really a matter of taste, but maybe you want to consider having a single
> > > > > > function, with a 3rd parameter, bool is_tx.
> > > > > > The calls here will be unified to:
> > > > > > ring_iowrite(ring, ring->head, ring->is_tx);
> > > > > > (No condition is needed here).
> > > > > >
> > > > > > The implementation uses the new parameter to decide which part of the register
> > > > > > to mask, reducing the code duplication (in my eyes):
> > > > > >
> > > > > > val = ioread32(ring_desc_base(ring) + 8);
> > > > > > if (is_tx) {
> > > > > > val &= 0x0000ffff;
> > > > > > val |= value << 16;
> > > > > > } else {
> > > > > > val &= 0xffff0000;
> > > > > > val |= value;
> > > > > > }
> > > > > > iowrite32(val, ring_desc_base(ring) + 8);
> > > > > >
> > > > > > I'm not sure if it improves the readability or makes it worse. Your call.
> > > > >
> > > > > Gah, that is all horrid beyond belief.
> > > > > If a 32bit write is valid then the hardware must not be updating
> > > > > the other 16 bits.
> > > > > In which case the driver knows what they should be.
> > > > > So it can do a single 32bit write of the required value.
> > > >
> > > > I'm not entirely sure I understand what you say above. Can you shed some
> > > > light on this by a concrete example how it should look like? :-)
> > >
> > > The driver must know both the tx and rx ring values, so:
> > > iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);
> > >
> >
> > I see. However, prod or cons side gets updated by the hardware as it
> > processes buffers and other side is only updated by the driver. I'm not
> > sure the above works here.
>
> If the hardware updates the other half of the 32bit word it doesn't ever work.
>
> In that case you must do 16bit writes.
> If the hardware is ignoring the byte-enables it is broken and unusable.
It is quite usable as I'm running this code on real hardware ;-) but
32-bit access is needed.
The low or high 16-bits are read-only depending on the ring (Tx or Rx)
and updated by the hardware. It ignores writes to that part so we could
do this for Tx ring:
iowrite32(prod << 16, ring_desc_base(ring) + 8);
and this for Rx ring:
iowrite32(cons, ring_desc_base(ring) + 8);
Do you see any issues with this?
From: 'Mika Westerberg'
> Sent: 08 August 2019 10:58
> On Wed, Aug 07, 2019 at 04:41:30PM +0000, David Laight wrote:
> > From: 'Mika Westerberg' [mailto:[email protected]]
> > > Sent: 07 August 2019 17:36
> > >
> > > On Wed, Aug 07, 2019 at 04:22:26PM +0000, David Laight wrote:
> > > > From: Mika Westerberg
> > > > > Sent: 07 August 2019 17:14
> > > > > To: David Laight
> > > > >
> > > > > On Fri, Jul 05, 2019 at 04:04:19PM +0000, David Laight wrote:
> > > > > > > Really a matter of taste, but maybe you want to consider having a single
> > > > > > > function, with a 3rd parameter, bool is_tx.
> > > > > > > The calls here will be unified to:
> > > > > > > ring_iowrite(ring, ring->head, ring->is_tx);
> > > > > > > (No condition is needed here).
> > > > > > >
> > > > > > > The implementation uses the new parameter to decide which part of the register
> > > > > > > to mask, reducing the code duplication (in my eyes):
> > > > > > >
> > > > > > > val = ioread32(ring_desc_base(ring) + 8);
> > > > > > > if (is_tx) {
> > > > > > > val &= 0x0000ffff;
> > > > > > > val |= value << 16;
> > > > > > > } else {
> > > > > > > val &= 0xffff0000;
> > > > > > > val |= value;
> > > > > > > }
> > > > > > > iowrite32(val, ring_desc_base(ring) + 8);
> > > > > > >
> > > > > > > I'm not sure if it improves the readability or makes it worse. Your call.
> > > > > >
> > > > > > Gah, that is all horrid beyond belief.
> > > > > > If a 32bit write is valid then the hardware must not be updating
> > > > > > the other 16 bits.
> > > > > > In which case the driver knows what they should be.
> > > > > > So it can do a single 32bit write of the required value.
> > > > >
> > > > > I'm not entirely sure I understand what you say above. Can you shed some
> > > > > light on this by a concrete example how it should look like? :-)
> > > >
> > > > The driver must know both the tx and rx ring values, so:
> > > > iowrite32(tx_val << 16 | rx_val, ring_desc_base(ring) + 8);
> > > >
> > >
> > > I see. However, prod or cons side gets updated by the hardware as it
> > > processes buffers and other side is only updated by the driver. I'm not
> > > sure the above works here.
> >
> > If the hardware updates the other half of the 32bit word it doesn't ever work.
> >
> > In that case you must do 16bit writes.
> > If the hardware is ignoring the byte-enables it is broken and unusable.
>
> It is quite usable as I'm running this code on real hardware ;-) but
> 32-bit access is needed.
>
> The low or high 16-bits are read-only depending on the ring (Tx or Rx)
> and updated by the hardware. It ignores writes to that part so we could
> do this for Tx ring:
>
> iowrite32(prod << 16, ring_desc_base(ring) + 8);
>
> and this for Rx ring:
>
> iowrite32(cons, ring_desc_base(ring) + 8);
>
> Do you see any issues with this?
No, just comment that the hardware ignores the write to the other bytes.
You probably want separate functions - to remove the mispredicted branch.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)