| Message ID | 1596751055-12316-1-git-send-email-tuanphan@os.amperecomputing.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI/ACPI: Add Ampere Altra SOC MCFG quirk | expand |
On Thu, Aug 06, 2020 at 02:57:34PM -0700, Tuan Phan wrote: > Ampere Altra SOC supports only 32-bit ECAM reading. Therefore, > add an MCFG quirk for the platform. This is interesting. So this host bridge supports sub 32-bit config *writes*, but not reads? I actually don't know whether that complies with the spec or not. If config registers are not allowed to have side effects on read, this *would* be compliant. PCIe r5.0, sec 7.4, doesn't list any register types with read side effects, so there shouldn't be any in the registers defined by the spec. But I would think device-specific registers could do whatever they wanted, e.g., reading an interrupt status register or something could clear it. And I think sec 7.2.2 about ECAM implicitly requires sub 32-bit accesses because it mentions the access size and byte enables. Is this a one-off situation where future hardware will allow sub 32-bit reads and writes? We don't want a stream of quirks for future devices. > Signed-off-by: Tuan Phan <tuanphan@os.amperecomputing.com> > --- > drivers/acpi/pci_mcfg.c | 20 ++++++++++++++++++++ > drivers/pci/ecam.c | 10 ++++++++++ > include/linux/pci-ecam.h | 1 + > 3 files changed, 31 insertions(+) > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > index 54b36b7ad47d..e526571e0ebd 100644 > --- a/drivers/acpi/pci_mcfg.c > +++ b/drivers/acpi/pci_mcfg.c > @@ -142,6 +142,26 @@ static struct mcfg_fixup mcfg_quirks[] = { > XGENE_V2_ECAM_MCFG(4, 0), > XGENE_V2_ECAM_MCFG(4, 1), > XGENE_V2_ECAM_MCFG(4, 2), > + > +#define ALTRA_ECAM_QUIRK(rev, seg) \ > + { "Ampere", "Altra ", rev, seg, MCFG_BUS_ANY, &pci_32b_read_ops } > + > + ALTRA_ECAM_QUIRK(1, 0), > + ALTRA_ECAM_QUIRK(1, 1), > + ALTRA_ECAM_QUIRK(1, 2), > + ALTRA_ECAM_QUIRK(1, 3), > + ALTRA_ECAM_QUIRK(1, 4), > + ALTRA_ECAM_QUIRK(1, 5), > + ALTRA_ECAM_QUIRK(1, 6), > + ALTRA_ECAM_QUIRK(1, 7), > + ALTRA_ECAM_QUIRK(1, 8), > + ALTRA_ECAM_QUIRK(1, 9), > + ALTRA_ECAM_QUIRK(1, 10), > + ALTRA_ECAM_QUIRK(1, 11), > + ALTRA_ECAM_QUIRK(1, 12), > + ALTRA_ECAM_QUIRK(1, 13), > + ALTRA_ECAM_QUIRK(1, 14), > + ALTRA_ECAM_QUIRK(1, 15), > }; > > static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c > index 8f065a42fc1a..b54d32a31669 100644 > --- a/drivers/pci/ecam.c > +++ b/drivers/pci/ecam.c > @@ -168,4 +168,14 @@ const struct pci_ecam_ops pci_32b_ops = { > .write = pci_generic_config_write32, > } > }; > + > +/* ECAM ops for 32-bit read only (non-compliant) */ > +const struct pci_ecam_ops pci_32b_read_ops = { > + .bus_shift = 20, > + .pci_ops = { > + .map_bus = pci_ecam_map_bus, > + .read = pci_generic_config_read32, > + .write = pci_generic_config_write, > + } > +}; > #endif > diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h > index 1af5cb02ef7f..033ce74f02e8 100644 > --- a/include/linux/pci-ecam.h > +++ b/include/linux/pci-ecam.h > @@ -51,6 +51,7 @@ extern const struct pci_ecam_ops pci_generic_ecam_ops; > > #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) > extern const struct pci_ecam_ops pci_32b_ops; /* 32-bit accesses only */ > +extern const struct pci_ecam_ops pci_32b_read_ops; /* 32-bit read only */ > extern const struct pci_ecam_ops hisi_pcie_ops; /* HiSilicon */ > extern const struct pci_ecam_ops thunder_pem_ecam_ops; /* Cavium ThunderX 1.x & 2.x */ > extern const struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */ > -- > 2.18.4 >
> On Aug 6, 2020, at 3:27 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Thu, Aug 06, 2020 at 02:57:34PM -0700, Tuan Phan wrote: >> Ampere Altra SOC supports only 32-bit ECAM reading. Therefore, >> add an MCFG quirk for the platform. > > This is interesting. So this host bridge supports sub 32-bit config > *writes*, but not reads? > > I actually don't know whether that complies with the spec or not. If > config registers are not allowed to have side effects on read, this > *would* be compliant. > > PCIe r5.0, sec 7.4, doesn't list any register types with read side > effects, so there shouldn't be any in the registers defined by the > spec. But I would think device-specific registers could do whatever > they wanted, e.g., reading an interrupt status register or something > could clear it. > > And I think sec 7.2.2 about ECAM implicitly requires sub 32-bit > accesses because it mentions the access size and byte enables. > > Is this a one-off situation where future hardware will allow sub > 32-bit reads and writes? We don't want a stream of quirks for future > devices. Hi Bjorn, Actually, this is a silicon bug inside current Altra Soc. Our SOC supports sub 32-bit reads and writes. But using byte read for some devices like AMD graphic card causing corrupted data due to host controller HW errata. Future devices will have this issue fixed so this quirk applies only for current Altra Soc. So you think I can drop the “non-compliant” as there are no read side effects for all registers? > >> Signed-off-by: Tuan Phan <tuanphan@os.amperecomputing.com> >> --- >> drivers/acpi/pci_mcfg.c | 20 ++++++++++++++++++++ >> drivers/pci/ecam.c | 10 ++++++++++ >> include/linux/pci-ecam.h | 1 + >> 3 files changed, 31 insertions(+) >> >> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c >> index 54b36b7ad47d..e526571e0ebd 100644 >> --- a/drivers/acpi/pci_mcfg.c >> +++ b/drivers/acpi/pci_mcfg.c >> @@ -142,6 +142,26 @@ static struct mcfg_fixup mcfg_quirks[] = { >> XGENE_V2_ECAM_MCFG(4, 0), >> XGENE_V2_ECAM_MCFG(4, 1), >> XGENE_V2_ECAM_MCFG(4, 2), >> + >> +#define ALTRA_ECAM_QUIRK(rev, seg) \ >> + { "Ampere", "Altra ", rev, seg, MCFG_BUS_ANY, &pci_32b_read_ops } >> + >> + ALTRA_ECAM_QUIRK(1, 0), >> + ALTRA_ECAM_QUIRK(1, 1), >> + ALTRA_ECAM_QUIRK(1, 2), >> + ALTRA_ECAM_QUIRK(1, 3), >> + ALTRA_ECAM_QUIRK(1, 4), >> + ALTRA_ECAM_QUIRK(1, 5), >> + ALTRA_ECAM_QUIRK(1, 6), >> + ALTRA_ECAM_QUIRK(1, 7), >> + ALTRA_ECAM_QUIRK(1, 8), >> + ALTRA_ECAM_QUIRK(1, 9), >> + ALTRA_ECAM_QUIRK(1, 10), >> + ALTRA_ECAM_QUIRK(1, 11), >> + ALTRA_ECAM_QUIRK(1, 12), >> + ALTRA_ECAM_QUIRK(1, 13), >> + ALTRA_ECAM_QUIRK(1, 14), >> + ALTRA_ECAM_QUIRK(1, 15), >> }; >> >> static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; >> diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c >> index 8f065a42fc1a..b54d32a31669 100644 >> --- a/drivers/pci/ecam.c >> +++ b/drivers/pci/ecam.c >> @@ -168,4 +168,14 @@ const struct pci_ecam_ops pci_32b_ops = { >> .write = pci_generic_config_write32, >> } >> }; >> + >> +/* ECAM ops for 32-bit read only (non-compliant) */ >> +const struct pci_ecam_ops pci_32b_read_ops = { >> + .bus_shift = 20, >> + .pci_ops = { >> + .map_bus = pci_ecam_map_bus, >> + .read = pci_generic_config_read32, >> + .write = pci_generic_config_write, >> + } >> +}; >> #endif >> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h >> index 1af5cb02ef7f..033ce74f02e8 100644 >> --- a/include/linux/pci-ecam.h >> +++ b/include/linux/pci-ecam.h >> @@ -51,6 +51,7 @@ extern const struct pci_ecam_ops pci_generic_ecam_ops; >> >> #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) >> extern const struct pci_ecam_ops pci_32b_ops; /* 32-bit accesses only */ >> +extern const struct pci_ecam_ops pci_32b_read_ops; /* 32-bit read only */ >> extern const struct pci_ecam_ops hisi_pcie_ops; /* HiSilicon */ >> extern const struct pci_ecam_ops thunder_pem_ecam_ops; /* Cavium ThunderX 1.x & 2.x */ >> extern const struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */ >> -- >> 2.18.4 >>
On Thu, Aug 06, 2020 at 03:40:41PM -0700, Tuan Phan wrote: > > > On Aug 6, 2020, at 3:27 PM, Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > On Thu, Aug 06, 2020 at 02:57:34PM -0700, Tuan Phan wrote: > >> Ampere Altra SOC supports only 32-bit ECAM reading. Therefore, > >> add an MCFG quirk for the platform. > > > > This is interesting. So this host bridge supports sub 32-bit config > > *writes*, but not reads? > > > > I actually don't know whether that complies with the spec or not. If > > config registers are not allowed to have side effects on read, this > > *would* be compliant. > > > > PCIe r5.0, sec 7.4, doesn't list any register types with read side > > effects, so there shouldn't be any in the registers defined by the > > spec. But I would think device-specific registers could do whatever > > they wanted, e.g., reading an interrupt status register or something > > could clear it. > > > > And I think sec 7.2.2 about ECAM implicitly requires sub 32-bit > > accesses because it mentions the access size and byte enables. > > > > Is this a one-off situation where future hardware will allow sub > > 32-bit reads and writes? We don't want a stream of quirks for future > > devices. > > Actually, this is a silicon bug inside current Altra Soc. Our SOC > supports sub 32-bit reads and writes. But using byte read for some > devices like AMD graphic card causing corrupted data due to host > controller HW errata. > > Future devices will have this issue fixed so this quirk applies only > for current Altra Soc. > > So you think I can drop the “non-compliant” as there are no read > side effects for all registers? No, I actually said that I don't think any *architected* config registers have read side effects, but I think it *is* allowed for device-specific registers to have read side effects. Glad this will be fixed in future devices! > >> Signed-off-by: Tuan Phan <tuanphan@os.amperecomputing.com> > >> --- > >> drivers/acpi/pci_mcfg.c | 20 ++++++++++++++++++++ > >> drivers/pci/ecam.c | 10 ++++++++++ > >> include/linux/pci-ecam.h | 1 + > >> 3 files changed, 31 insertions(+) > >> > >> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > >> index 54b36b7ad47d..e526571e0ebd 100644 > >> --- a/drivers/acpi/pci_mcfg.c > >> +++ b/drivers/acpi/pci_mcfg.c > >> @@ -142,6 +142,26 @@ static struct mcfg_fixup mcfg_quirks[] = { > >> XGENE_V2_ECAM_MCFG(4, 0), > >> XGENE_V2_ECAM_MCFG(4, 1), > >> XGENE_V2_ECAM_MCFG(4, 2), > >> + > >> +#define ALTRA_ECAM_QUIRK(rev, seg) \ > >> + { "Ampere", "Altra ", rev, seg, MCFG_BUS_ANY, &pci_32b_read_ops } > >> + > >> + ALTRA_ECAM_QUIRK(1, 0), > >> + ALTRA_ECAM_QUIRK(1, 1), > >> + ALTRA_ECAM_QUIRK(1, 2), > >> + ALTRA_ECAM_QUIRK(1, 3), > >> + ALTRA_ECAM_QUIRK(1, 4), > >> + ALTRA_ECAM_QUIRK(1, 5), > >> + ALTRA_ECAM_QUIRK(1, 6), > >> + ALTRA_ECAM_QUIRK(1, 7), > >> + ALTRA_ECAM_QUIRK(1, 8), > >> + ALTRA_ECAM_QUIRK(1, 9), > >> + ALTRA_ECAM_QUIRK(1, 10), > >> + ALTRA_ECAM_QUIRK(1, 11), > >> + ALTRA_ECAM_QUIRK(1, 12), > >> + ALTRA_ECAM_QUIRK(1, 13), > >> + ALTRA_ECAM_QUIRK(1, 14), > >> + ALTRA_ECAM_QUIRK(1, 15), > >> }; > >> > >> static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > >> diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c > >> index 8f065a42fc1a..b54d32a31669 100644 > >> --- a/drivers/pci/ecam.c > >> +++ b/drivers/pci/ecam.c > >> @@ -168,4 +168,14 @@ const struct pci_ecam_ops pci_32b_ops = { > >> .write = pci_generic_config_write32, > >> } > >> }; > >> + > >> +/* ECAM ops for 32-bit read only (non-compliant) */ > >> +const struct pci_ecam_ops pci_32b_read_ops = { > >> + .bus_shift = 20, > >> + .pci_ops = { > >> + .map_bus = pci_ecam_map_bus, > >> + .read = pci_generic_config_read32, > >> + .write = pci_generic_config_write, > >> + } > >> +}; > >> #endif > >> diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h > >> index 1af5cb02ef7f..033ce74f02e8 100644 > >> --- a/include/linux/pci-ecam.h > >> +++ b/include/linux/pci-ecam.h > >> @@ -51,6 +51,7 @@ extern const struct pci_ecam_ops pci_generic_ecam_ops; > >> > >> #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) > >> extern const struct pci_ecam_ops pci_32b_ops; /* 32-bit accesses only */ > >> +extern const struct pci_ecam_ops pci_32b_read_ops; /* 32-bit read only */ > >> extern const struct pci_ecam_ops hisi_pcie_ops; /* HiSilicon */ > >> extern const struct pci_ecam_ops thunder_pem_ecam_ops; /* Cavium ThunderX 1.x & 2.x */ > >> extern const struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */ > >> -- > >> 2.18.4 > >> >
On Thu, Aug 06, 2020 at 02:57:34PM -0700, Tuan Phan wrote: > Ampere Altra SOC supports only 32-bit ECAM reading. Therefore, > add an MCFG quirk for the platform. > > Signed-off-by: Tuan Phan <tuanphan@os.amperecomputing.com> Applied to pci/enumeration for v5.10, thanks! > --- > drivers/acpi/pci_mcfg.c | 20 ++++++++++++++++++++ > drivers/pci/ecam.c | 10 ++++++++++ > include/linux/pci-ecam.h | 1 + > 3 files changed, 31 insertions(+) > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > index 54b36b7ad47d..e526571e0ebd 100644 > --- a/drivers/acpi/pci_mcfg.c > +++ b/drivers/acpi/pci_mcfg.c > @@ -142,6 +142,26 @@ static struct mcfg_fixup mcfg_quirks[] = { > XGENE_V2_ECAM_MCFG(4, 0), > XGENE_V2_ECAM_MCFG(4, 1), > XGENE_V2_ECAM_MCFG(4, 2), > + > +#define ALTRA_ECAM_QUIRK(rev, seg) \ > + { "Ampere", "Altra ", rev, seg, MCFG_BUS_ANY, &pci_32b_read_ops } > + > + ALTRA_ECAM_QUIRK(1, 0), > + ALTRA_ECAM_QUIRK(1, 1), > + ALTRA_ECAM_QUIRK(1, 2), > + ALTRA_ECAM_QUIRK(1, 3), > + ALTRA_ECAM_QUIRK(1, 4), > + ALTRA_ECAM_QUIRK(1, 5), > + ALTRA_ECAM_QUIRK(1, 6), > + ALTRA_ECAM_QUIRK(1, 7), > + ALTRA_ECAM_QUIRK(1, 8), > + ALTRA_ECAM_QUIRK(1, 9), > + ALTRA_ECAM_QUIRK(1, 10), > + ALTRA_ECAM_QUIRK(1, 11), > + ALTRA_ECAM_QUIRK(1, 12), > + ALTRA_ECAM_QUIRK(1, 13), > + ALTRA_ECAM_QUIRK(1, 14), > + ALTRA_ECAM_QUIRK(1, 15), > }; > > static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c > index 8f065a42fc1a..b54d32a31669 100644 > --- a/drivers/pci/ecam.c > +++ b/drivers/pci/ecam.c > @@ -168,4 +168,14 @@ const struct pci_ecam_ops pci_32b_ops = { > .write = pci_generic_config_write32, > } > }; > + > +/* ECAM ops for 32-bit read only (non-compliant) */ > +const struct pci_ecam_ops pci_32b_read_ops = { > + .bus_shift = 20, > + .pci_ops = { > + .map_bus = pci_ecam_map_bus, > + .read = pci_generic_config_read32, > + .write = pci_generic_config_write, > + } > +}; > #endif > diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h > index 1af5cb02ef7f..033ce74f02e8 100644 > --- a/include/linux/pci-ecam.h > +++ b/include/linux/pci-ecam.h > @@ -51,6 +51,7 @@ extern const struct pci_ecam_ops pci_generic_ecam_ops; > > #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) > extern const struct pci_ecam_ops pci_32b_ops; /* 32-bit accesses only */ > +extern const struct pci_ecam_ops pci_32b_read_ops; /* 32-bit read only */ > extern const struct pci_ecam_ops hisi_pcie_ops; /* HiSilicon */ > extern const struct pci_ecam_ops thunder_pem_ecam_ops; /* Cavium ThunderX 1.x & 2.x */ > extern const struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */ > -- > 2.18.4 >
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index 54b36b7ad47d..e526571e0ebd 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -142,6 +142,26 @@ static struct mcfg_fixup mcfg_quirks[] = { XGENE_V2_ECAM_MCFG(4, 0), XGENE_V2_ECAM_MCFG(4, 1), XGENE_V2_ECAM_MCFG(4, 2), + +#define ALTRA_ECAM_QUIRK(rev, seg) \ + { "Ampere", "Altra ", rev, seg, MCFG_BUS_ANY, &pci_32b_read_ops } + + ALTRA_ECAM_QUIRK(1, 0), + ALTRA_ECAM_QUIRK(1, 1), + ALTRA_ECAM_QUIRK(1, 2), + ALTRA_ECAM_QUIRK(1, 3), + ALTRA_ECAM_QUIRK(1, 4), + ALTRA_ECAM_QUIRK(1, 5), + ALTRA_ECAM_QUIRK(1, 6), + ALTRA_ECAM_QUIRK(1, 7), + ALTRA_ECAM_QUIRK(1, 8), + ALTRA_ECAM_QUIRK(1, 9), + ALTRA_ECAM_QUIRK(1, 10), + ALTRA_ECAM_QUIRK(1, 11), + ALTRA_ECAM_QUIRK(1, 12), + ALTRA_ECAM_QUIRK(1, 13), + ALTRA_ECAM_QUIRK(1, 14), + ALTRA_ECAM_QUIRK(1, 15), }; static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c index 8f065a42fc1a..b54d32a31669 100644 --- a/drivers/pci/ecam.c +++ b/drivers/pci/ecam.c @@ -168,4 +168,14 @@ const struct pci_ecam_ops pci_32b_ops = { .write = pci_generic_config_write32, } }; + +/* ECAM ops for 32-bit read only (non-compliant) */ +const struct pci_ecam_ops pci_32b_read_ops = { + .bus_shift = 20, + .pci_ops = { + .map_bus = pci_ecam_map_bus, + .read = pci_generic_config_read32, + .write = pci_generic_config_write, + } +}; #endif diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h index 1af5cb02ef7f..033ce74f02e8 100644 --- a/include/linux/pci-ecam.h +++ b/include/linux/pci-ecam.h @@ -51,6 +51,7 @@ extern const struct pci_ecam_ops pci_generic_ecam_ops; #if defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) extern const struct pci_ecam_ops pci_32b_ops; /* 32-bit accesses only */ +extern const struct pci_ecam_ops pci_32b_read_ops; /* 32-bit read only */ extern const struct pci_ecam_ops hisi_pcie_ops; /* HiSilicon */ extern const struct pci_ecam_ops thunder_pem_ecam_ops; /* Cavium ThunderX 1.x & 2.x */ extern const struct pci_ecam_ops pci_thunder_ecam_ops; /* Cavium ThunderX 1.x */
Ampere Altra SOC supports only 32-bit ECAM reading. Therefore, add an MCFG quirk for the platform. Signed-off-by: Tuan Phan <tuanphan@os.amperecomputing.com> --- drivers/acpi/pci_mcfg.c | 20 ++++++++++++++++++++ drivers/pci/ecam.c | 10 ++++++++++ include/linux/pci-ecam.h | 1 + 3 files changed, 31 insertions(+)