This series adds a work around for enabling PCIe ASPM and for setting PCIe
LTR values on VMD reserved root ports on select platforms. While
configuration of these capabilities is usually done by BIOS, on these
platforms these capabilities will not be configured because the ports are
not visible to BIOS. This was part of an initial design that expected the
driver to completely handle the ports, including power management. However
on Linux those ports are still managed by the PCIe core, which has the
expectation that they adhere to device standards including BIOS
configuration, leading to this problem.
The target platforms are Tiger Lake, Alder Lake, and Raptor Lake though the
latter has already implemented support for configuring the LTR values.
Meteor Lake is expected add BIOS ASPM support, eliminating the future need
for this work around.
Note, the driver programs the LTRs because BIOS would also normally do this
for devices that do not set them by default. Without this, SoC power
management would be blocked on those platform. This SoC specific value is
the maximum latency required to allow the SoC to enter the deepest power
state.
This patch addresses the following open bugzillas on VMD enabled laptops
that cannot enter low power states.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=212355
Link: https://bugzilla.kernel.org/show_bug.cgi?id=215063
Link: https://bugzilla.kernel.org/show_bug.cgi?id=213717
David E. Box (3):
PCI: vmd: Use PCI_VDEVICE in device list
PCI: vmd: Add vmd_device_data
PCI: vmd: Add quirk to configure PCIe ASPM and LTR
Michael Bottini (1):
PCI/ASPM: Add pci_enable_link_state()
drivers/pci/controller/vmd.c | 158 +++++++++++++++++++++++++++--------
drivers/pci/pcie/aspm.c | 54 ++++++++++++
include/linux/pci.h | 7 ++
3 files changed, 186 insertions(+), 33 deletions(-)
base-commit: 247f34f7b80357943234f93f247a1ae6b6c3a740
--
2.25.1
Add vmd_device_data to allow adding additional info for driver data.
Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Jon Derrick <[email protected]>
Reviewed-by: Nirmal Patel <[email protected]>
---
V7
- Moved PCI_VDEVICE into separate earlier patch.
V6
- Inline the declarations for driver data in the vmd_ids list.
Suggested by Jonathan
V5
- New patch
drivers/pci/controller/vmd.c | 80 +++++++++++++++++++++++++-----------
1 file changed, 55 insertions(+), 25 deletions(-)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 9dedca714c18..a53d88fd820c 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -68,6 +68,10 @@ enum vmd_features {
VMD_FEAT_CAN_BYPASS_MSI_REMAP = (1 << 4),
};
+struct vmd_device_data {
+ enum vmd_features features;
+};
+
static DEFINE_IDA(vmd_instance_ida);
/*
@@ -709,11 +713,12 @@ static void vmd_copy_host_bridge_flags(struct pci_host_bridge *root_bridge,
vmd_bridge->native_dpc = root_bridge->native_dpc;
}
-static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
+static int vmd_enable_domain(struct vmd_dev *vmd, struct vmd_device_data *info)
{
struct pci_sysdata *sd = &vmd->sysdata;
struct resource *res;
u32 upper_bits;
+ unsigned long features = info->features;
unsigned long flags;
LIST_HEAD(resources);
resource_size_t offset[2] = {0};
@@ -882,7 +887,8 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
- unsigned long features = (unsigned long) id->driver_data;
+ struct vmd_device_data *info = (struct vmd_device_data *)id->driver_data;
+ unsigned long features = info->features;
struct vmd_dev *vmd;
int err;
@@ -927,7 +933,7 @@ static int vmd_probe(struct pci_dev *dev, const struct pci_device_id *id)
spin_lock_init(&vmd->cfg_lock);
pci_set_drvdata(dev, vmd);
- err = vmd_enable_domain(vmd, features);
+ err = vmd_enable_domain(vmd, info);
if (err)
goto out_release_instance;
@@ -995,35 +1001,59 @@ static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
static const struct pci_device_id vmd_ids[] = {
{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
- .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,},
+ (kernel_ulong_t)&(struct vmd_device_data) {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,
+ },
+ },
{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
- .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW |
- VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_CAN_BYPASS_MSI_REMAP,},
+ (kernel_ulong_t)&(struct vmd_device_data) {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW |
+ VMD_FEAT_HAS_BUS_RESTRICTIONS |
+ VMD_FEAT_CAN_BYPASS_MSI_REMAP,
+ },
+ },
{PCI_VDEVICE(INTEL, 0x467f),
- .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
- VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_OFFSET_FIRST_VECTOR,},
+ (kernel_ulong_t)&(struct vmd_device_data) {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+ VMD_FEAT_HAS_BUS_RESTRICTIONS |
+ VMD_FEAT_OFFSET_FIRST_VECTOR,
+ },
+ },
{PCI_VDEVICE(INTEL, 0x4c3d),
- .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
- VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_OFFSET_FIRST_VECTOR,},
+ (kernel_ulong_t)&(struct vmd_device_data) {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+ VMD_FEAT_HAS_BUS_RESTRICTIONS |
+ VMD_FEAT_OFFSET_FIRST_VECTOR,
+ },
+ },
{PCI_VDEVICE(INTEL, 0xa77f),
- .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
- VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_OFFSET_FIRST_VECTOR,},
+ (kernel_ulong_t)&(struct vmd_device_data) {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+ VMD_FEAT_HAS_BUS_RESTRICTIONS |
+ VMD_FEAT_OFFSET_FIRST_VECTOR,
+ },
+ },
{PCI_VDEVICE(INTEL, 0x7d0b),
- .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
- VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_OFFSET_FIRST_VECTOR,},
+ (kernel_ulong_t)&(struct vmd_device_data) {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+ VMD_FEAT_HAS_BUS_RESTRICTIONS |
+ VMD_FEAT_OFFSET_FIRST_VECTOR,
+ },
+ },
{PCI_VDEVICE(INTEL, 0xad0b),
- .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
- VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_OFFSET_FIRST_VECTOR,},
+ (kernel_ulong_t)&(struct vmd_device_data) {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+ VMD_FEAT_HAS_BUS_RESTRICTIONS |
+ VMD_FEAT_OFFSET_FIRST_VECTOR,
+ },
+ },
{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
- .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
- VMD_FEAT_HAS_BUS_RESTRICTIONS |
- VMD_FEAT_OFFSET_FIRST_VECTOR,},
+ (kernel_ulong_t)&(struct vmd_device_data) {
+ .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
+ VMD_FEAT_HAS_BUS_RESTRICTIONS |
+ VMD_FEAT_OFFSET_FIRST_VECTOR,
+ },
+ },
{0,}
};
MODULE_DEVICE_TABLE(pci, vmd_ids);
--
2.25.1
Refactor the PCI ID list to use PCI_VDEVICE.
Signed-off-by: David E. Box <[email protected]>
Reviewed-by: Jon Derrick <[email protected]>
Reviewed-by: Nirmal Patel <[email protected]>
---
V7 - New Patch. Separate patch suggested by Lorenzo
drivers/pci/controller/vmd.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index e06e9f4fc50f..9dedca714c18 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -994,33 +994,33 @@ static int vmd_resume(struct device *dev)
static SIMPLE_DEV_PM_OPS(vmd_dev_pm_ops, vmd_suspend, vmd_resume);
static const struct pci_device_id vmd_ids[] = {
- {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
+ {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_201D),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP,},
- {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
+ {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_28C0),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
VMD_FEAT_CAN_BYPASS_MSI_REMAP,},
- {PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x467f),
+ {PCI_VDEVICE(INTEL, 0x467f),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
VMD_FEAT_OFFSET_FIRST_VECTOR,},
- {PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x4c3d),
+ {PCI_VDEVICE(INTEL, 0x4c3d),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
VMD_FEAT_OFFSET_FIRST_VECTOR,},
- {PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xa77f),
+ {PCI_VDEVICE(INTEL, 0xa77f),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
VMD_FEAT_OFFSET_FIRST_VECTOR,},
- {PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0x7d0b),
+ {PCI_VDEVICE(INTEL, 0x7d0b),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
VMD_FEAT_OFFSET_FIRST_VECTOR,},
- {PCI_DEVICE(PCI_VENDOR_ID_INTEL, 0xad0b),
+ {PCI_VDEVICE(INTEL, 0xad0b),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
VMD_FEAT_OFFSET_FIRST_VECTOR,},
- {PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
+ {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
.driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
VMD_FEAT_OFFSET_FIRST_VECTOR,},
--
2.25.1
On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
> On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
> > On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
> >> Add vmd_device_data to allow adding additional info for driver data.
> >
> >> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> >> - .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >> - VMD_FEAT_HAS_BUS_RESTRICTIONS |
> >> - VMD_FEAT_OFFSET_FIRST_VECTOR,},
> >> + (kernel_ulong_t)&(struct vmd_device_data) {
> >> + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> >> + VMD_FEAT_HAS_BUS_RESTRICTIONS |
> >> + VMD_FEAT_OFFSET_FIRST_VECTOR,
> >> + },
> >> + },
> >
> > It looks like these devices come in families where several device IDs
> > share the same features. I think this would be more readable if you
> > defined each family outside this table and simply referenced the
> > family here. E.g., you could do something like:
> >
> > static struct vmd_device_data vmd_v1 = {
> > .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > VMD_FEAT_OFFSET_FIRST_VECTOR,
> > };
>
> I seem to recall it being similar to this in one of the previous revisions
> It's fine with me either way
Indeed it was:
https://lore.kernel.org/r/[email protected]
I'd forgotten that.
At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
that used the 467f data. The current series adds two more (0x7d0b
0x0ad0b). Maybe the "vmd_467f_data" name could have been more
descriptive, but the code was definitely shorter:
+ { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
+ { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
+ { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
+ { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B), (kernel_ulong_t)&vmd_467f_data },
I do wish pci_device_id.driver_data were a void pointer, as it is for
of_device_id, which makes it much more natural to express [1], but
that ship has long sailed.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
> > {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> > .driver_data = (kernel_ulong_t) &vmd_v1,
> >
> > Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> > instead of repeating it a half dozen times.
> >
> >> {0,}
> >> };
> >> MODULE_DEVICE_TABLE(pci, vmd_ids);
> >> --
> >> 2.25.1
> >>
On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
> Add vmd_device_data to allow adding additional info for driver data.
> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> - .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> - VMD_FEAT_HAS_BUS_RESTRICTIONS |
> - VMD_FEAT_OFFSET_FIRST_VECTOR,},
> + (kernel_ulong_t)&(struct vmd_device_data) {
> + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> + VMD_FEAT_HAS_BUS_RESTRICTIONS |
> + VMD_FEAT_OFFSET_FIRST_VECTOR,
> + },
> + },
It looks like these devices come in families where several device IDs
share the same features. I think this would be more readable if you
defined each family outside this table and simply referenced the
family here. E.g., you could do something like:
static struct vmd_device_data vmd_v1 = {
.features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
VMD_FEAT_HAS_BUS_RESTRICTIONS |
VMD_FEAT_OFFSET_FIRST_VECTOR,
};
{PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
.driver_data = (kernel_ulong_t) &vmd_v1,
Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
instead of repeating it a half dozen times.
> {0,}
> };
> MODULE_DEVICE_TABLE(pci, vmd_ids);
> --
> 2.25.1
>
On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
> On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
>> Add vmd_device_data to allow adding additional info for driver data.
>
>> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>> - .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>> - VMD_FEAT_HAS_BUS_RESTRICTIONS |
>> - VMD_FEAT_OFFSET_FIRST_VECTOR,},
>> + (kernel_ulong_t)&(struct vmd_device_data) {
>> + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>> + VMD_FEAT_HAS_BUS_RESTRICTIONS |
>> + VMD_FEAT_OFFSET_FIRST_VECTOR,
>> + },
>> + },
>
> It looks like these devices come in families where several device IDs
> share the same features. I think this would be more readable if you
> defined each family outside this table and simply referenced the
> family here. E.g., you could do something like:
>
> static struct vmd_device_data vmd_v1 = {
> .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> VMD_FEAT_HAS_BUS_RESTRICTIONS |
> VMD_FEAT_OFFSET_FIRST_VECTOR,
> };
I seem to recall it being similar to this in one of the previous revisions
It's fine with me either way
>
> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> .driver_data = (kernel_ulong_t) &vmd_v1,
>
> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> instead of repeating it a half dozen times.
>
>> {0,}
>> };
>> MODULE_DEVICE_TABLE(pci, vmd_ids);
>> --
>> 2.25.1
>>
On Fri, 2022-10-28 at 14:40 -0500, Bjorn Helgaas wrote:
> On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
> > On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
> > > On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
> > > > Add vmd_device_data to allow adding additional info for driver data.
> > > > {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> > > > - .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > > - VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > > - VMD_FEAT_OFFSET_FIRST_VECTOR,},
> > > > + (kernel_ulong_t)&(struct vmd_device_data) {
> > > > + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > > + VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > > + VMD_FEAT_OFFSET_FIRST_VECTOR,
> > > > + },
> > > > + },
> > >
> > > It looks like these devices come in families where several device IDs
> > > share the same features. I think this would be more readable if you
> > > defined each family outside this table and simply referenced the
> > > family here. E.g., you could do something like:
> > >
> > > static struct vmd_device_data vmd_v1 = {
> > > .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > > VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > > VMD_FEAT_OFFSET_FIRST_VECTOR,
> > > };
> >
> > I seem to recall it being similar to this in one of the previous revisions
> > It's fine with me either way
>
> Indeed it was:
> https://lore.kernel.org/r/[email protected]
> I'd forgotten that.
>
> At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
> that used the 467f data. The current series adds two more (0x7d0b
> 0x0ad0b). Maybe the "vmd_467f_data" name could have been more
> descriptive, but the code was definitely shorter:
>
> + { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
> + { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
> + { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> (kernel_ulong_t)&vmd_467f_data },
I prefer this too but don't know what's the best name. Could just be by the
platform that started this grouping, e.g. vmd_tgl_data for Tiger Lake. What do
you think Jonathan?
David
>
> I do wish pci_device_id.driver_data were a void pointer, as it is for
> of_device_id, which makes it much more natural to express [1], but
> that ship has long sailed.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
>
> > > {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> > > .driver_data = (kernel_ulong_t) &vmd_v1,
> > >
> > > Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> > > instead of repeating it a half dozen times.
> > >
> > > > {0,}
> > > > };
> > > > MODULE_DEVICE_TABLE(pci, vmd_ids);
> > > > --
> > > > 2.25.1
> > > >
On 10/28/2022 2:14 PM, Jonathan Derrick wrote:
>
> On 10/28/2022 3:22 PM, David E. Box wrote:
>> On Fri, 2022-10-28 at 14:40 -0500, Bjorn Helgaas wrote:
>>> On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
>>>> On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
>>>>> On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
>>>>>> Add vmd_device_data to allow adding additional info for driver data.
>>>>>> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>>>> - .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>>> - VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>>> - VMD_FEAT_OFFSET_FIRST_VECTOR,},
>>>>>> + (kernel_ulong_t)&(struct vmd_device_data) {
>>>>>> + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>>> + VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>>> + VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>>> + },
>>>>>> + },
>>>>> It looks like these devices come in families where several device IDs
>>>>> share the same features. I think this would be more readable if you
>>>>> defined each family outside this table and simply referenced the
>>>>> family here. E.g., you could do something like:
>>>>>
>>>>> static struct vmd_device_data vmd_v1 = {
>>>>> .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>> VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>> VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>> };
>>>> I seem to recall it being similar to this in one of the previous revisions
>>>> It's fine with me either way
>>> Indeed it was:
>>> https://lore.kernel.org/r/[email protected]
>>> I'd forgotten that.
>>>
>>> At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
>>> that used the 467f data. The current series adds two more (0x7d0b
>>> 0x0ad0b). Maybe the "vmd_467f_data" name could have been more
>>> descriptive, but the code was definitely shorter:
>>>
>>> + { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
>>> + { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
>>> + { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
>>> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>> (kernel_ulong_t)&vmd_467f_data },
>> I prefer this too but don't know what's the best name. Could just be by the
>> platform that started this grouping, e.g. vmd_tgl_data for Tiger Lake. What do
>> you think Jonathan?
>>
>> David
> vmd_client_data ? (meaning product class; client vs enterprise)
Product class would make sense. We can keep adding new IDs.
>
>>> I do wish pci_device_id.driver_data were a void pointer, as it is for
>>> of_device_id, which makes it much more natural to express [1], but
>>> that ship has long sailed.
>>>
>>> [1]
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
>>>
>>>>> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>>> .driver_data = (kernel_ulong_t) &vmd_v1,
>>>>>
>>>>> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
>>>>> instead of repeating it a half dozen times.
>>>>>
>>>>>> {0,}
>>>>>> };
>>>>>> MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>>>> --
>>>>>> 2.25.1
>>>>>>
On 10/28/2022 3:22 PM, David E. Box wrote:
> On Fri, 2022-10-28 at 14:40 -0500, Bjorn Helgaas wrote:
>> On Fri, Oct 28, 2022 at 02:18:48PM -0500, Jonathan Derrick wrote:
>>> On 10/28/2022 2:13 PM, Bjorn Helgaas wrote:
>>>> On Mon, Oct 24, 2022 at 05:44:10PM -0700, David E. Box wrote:
>>>>> Add vmd_device_data to allow adding additional info for driver data.
>>>>> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>>> - .driver_data = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>> - VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>> - VMD_FEAT_OFFSET_FIRST_VECTOR,},
>>>>> + (kernel_ulong_t)&(struct vmd_device_data) {
>>>>> + .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>>> + VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>>> + VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>>> + },
>>>>> + },
>>>>
>>>> It looks like these devices come in families where several device IDs
>>>> share the same features. I think this would be more readable if you
>>>> defined each family outside this table and simply referenced the
>>>> family here. E.g., you could do something like:
>>>>
>>>> static struct vmd_device_data vmd_v1 = {
>>>> .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
>>>> VMD_FEAT_HAS_BUS_RESTRICTIONS |
>>>> VMD_FEAT_OFFSET_FIRST_VECTOR,
>>>> };
>>>
>>> I seem to recall it being similar to this in one of the previous revisions
>>> It's fine with me either way
>>
>> Indeed it was:
>> https://lore.kernel.org/r/[email protected]
>> I'd forgotten that.
>>
>> At the time there were four devices (0x467f 0x4c3d 0xa77f 0x9a0b)
>> that used the 467f data. The current series adds two more (0x7d0b
>> 0x0ad0b). Maybe the "vmd_467f_data" name could have been more
>> descriptive, but the code was definitely shorter:
>>
>> + { PCI_VDEVICE(INTEL, 0x467f), (kernel_ulong_t)&vmd_467f_data },
>> + { PCI_VDEVICE(INTEL, 0x4c3d), (kernel_ulong_t)&vmd_467f_data },
>> + { PCI_VDEVICE(INTEL, 0xa77f), (kernel_ulong_t)&vmd_467f_data },
>> + { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>> (kernel_ulong_t)&vmd_467f_data },
>
> I prefer this too but don't know what's the best name. Could just be by the
> platform that started this grouping, e.g. vmd_tgl_data for Tiger Lake. What do
> you think Jonathan?
>
> David
vmd_client_data ? (meaning product class; client vs enterprise)
>
>>
>> I do wish pci_device_id.driver_data were a void pointer, as it is for
>> of_device_id, which makes it much more natural to express [1], but
>> that ship has long sailed.
>>
>> [1]
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-kirin.c?id=v6.0#n768
>>
>>>> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
>>>> .driver_data = (kernel_ulong_t) &vmd_v1,
>>>>
>>>> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
>>>> instead of repeating it a half dozen times.
>>>>
>>>>> {0,}
>>>>> };
>>>>> MODULE_DEVICE_TABLE(pci, vmd_ids);
>>>>> --
>>>>> 2.25.1
>>>>>
>
On Fri, Oct 28, 2022 at 02:13:08PM -0500, Bjorn Helgaas wrote:
> It looks like these devices come in families where several device IDs
> share the same features. I think this would be more readable if you
> defined each family outside this table and simply referenced the
> family here. E.g., you could do something like:
>
> static struct vmd_device_data vmd_v1 = {
> .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> VMD_FEAT_HAS_BUS_RESTRICTIONS |
> VMD_FEAT_OFFSET_FIRST_VECTOR,
> };
>
> {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> .driver_data = (kernel_ulong_t) &vmd_v1,
>
> Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> instead of repeating it a half dozen times.
I wonder why we need the ltr field at all. For those that set it
is always the same value, so it could just be a quirk flag to set it.
Tat being said I think thegrouping makes a lot of sense, but I'd just
do it with a #define for the set of common quirk flags.
On Mon, 2022-10-31 at 00:04 -0700, Christoph Hellwig wrote:
> On Fri, Oct 28, 2022 at 02:13:08PM -0500, Bjorn Helgaas wrote:
> > It looks like these devices come in families where several device IDs
> > share the same features. I think this would be more readable if you
> > defined each family outside this table and simply referenced the
> > family here. E.g., you could do something like:
> >
> > static struct vmd_device_data vmd_v1 = {
> > .features = VMD_FEAT_HAS_MEMBAR_SHADOW_VSCAP |
> > VMD_FEAT_HAS_BUS_RESTRICTIONS |
> > VMD_FEAT_OFFSET_FIRST_VECTOR,
> > };
> >
> > {PCI_VDEVICE(INTEL, PCI_DEVICE_ID_INTEL_VMD_9A0B),
> > .driver_data = (kernel_ulong_t) &vmd_v1,
> >
> > Then you can add VMD_FEAT_BIOS_PM_QUIRK and the .ltr value in one place
> > instead of repeating it a half dozen times.
>
> I wonder why we need the ltr field at all. For those that set it
> is always the same value, so it could just be a quirk flag to set it.
Yeah, this makes sense particularly since this isn't intended as a permanent
fix. I'll get rid of it.
>
> Tat being said I think thegrouping makes a lot of sense, but I'd just
> do it with a #define for the set of common quirk flags.
Works for me. I'll create a VMD_FEATS_CLIENT group but I'll keep the ltr quirk
separate since future client systems won't be using it.
David