From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Return-Path: Subject: Re: [RFC PATCH 1/3] pci, acpi: Match PCI config space accessors against platfrom specific ECAM quirks. To: Gabriele Paoloni , Jeffrey Hugo , Christopher Covington , Tomasz Nowicki , "helgaas@kernel.org" , "arnd@arndb.de" , "will.deacon@arm.com" , "catalin.marinas@arm.com" , "rafael@kernel.org" , "Lorenzo.Pieralisi@arm.com" , "okaya@codeaurora.org" , "jchandra@broadcom.com" References: <1464856864-18049-1-git-send-email-tn@semihalf.com> <1464856864-18049-2-git-send-email-tn@semihalf.com> <57519F09.2010201@codeaurora.org> <470f6d93-4c83-e271-7e99-7e23f3f76e71@codeaurora.org> Cc: "liudongdong (C)" , "linaro-acpi@lists.linaro.org" , "jcm@redhat.com" , "dhdang@apm.com" , "Liviu.Dudau@arm.com" , "ddaney@caviumnetworks.com" , "jeremy.linton@arm.com" , "linux-kernel@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "robert.richter@caviumnetworks.com" , "msalter@redhat.com" , "Suravee.Suthikulpanit@amd.com" , "linux-pci@vger.kernel.org" , Wangyijing , "mw@semihalf.com" , "andrea.gallo@linaro.org" , "linux-arm-kernel@lists.infradead.org" From: Hanjun Guo Message-ID: <8e00d6fa-7ed6-e69f-d41c-c114375c98ae@linaro.org> Date: Mon, 6 Jun 2016 15:54:08 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed List-ID: On 2016/6/6 15:27, Gabriele Paoloni wrote: > Hi Jeffrey >> On 6/3/2016 9:32 AM, Gabriele Paoloni wrote: >>> Hi Cov >>> >>>> Hi Tomasz, >>>> >>>> Thanks for your work on this. >>>> >>>> On 06/02/2016 04:41 AM, Tomasz Nowicki wrote: >>>>> Some platforms may not be fully compliant with generic set of PCI >>>> config >>>>> accessors. For these cases we implement the way to overwrite >>>> accessors >>>>> set. Algorithm traverses available quirk list, matches against >>>>> tuple and returns >> corresponding >>>>> PCI config ops. oem_id and oem_rev come from MCFG table standard >>>> header. >>>>> All quirks can be defined using DECLARE_ACPI_MCFG_FIXUP() macro and >>>>> kept self contained. Example: >>>>> >>>>> /* Custom PCI config ops */ >>>>> static struct pci_generic_ecam_ops foo_pci_ops = { >>>>> .bus_shift = 24, >>>>> .pci_ops = { >>>>> .map_bus = pci_ecam_map_bus, >>>>> .read = foo_ecam_config_read, >>>>> .write = foo_ecam_config_write, >>>>> } >>>>> }; >>>>> >>>>> DECLARE_ACPI_MCFG_FIXUP(&foo_pci_ops, , , >>>> , ); >>>>> >>>>> Signed-off-by: Tomasz Nowicki >>>>> --- >>>>> drivers/acpi/pci_mcfg.c | 32 >>>> ++++++++++++++++++++++++++++++++ >>>>> include/asm-generic/vmlinux.lds.h | 7 +++++++ >>>>> include/linux/pci-acpi.h | 19 +++++++++++++++++++ >>>>> 3 files changed, 58 insertions(+) >>>>> >>>>> diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c >>>>> index 1847f74..f3d4570 100644 >>>>> --- a/drivers/acpi/pci_mcfg.c >>>>> +++ b/drivers/acpi/pci_mcfg.c >>>>> @@ -22,11 +22,43 @@ >>>>> #include >>>>> #include >>>>> #include >>>>> +#include >>>>> >>>>> /* Root pointer to the mapped MCFG table */ >>>>> static struct acpi_table_mcfg *mcfg_table; >>>>> static int mcfg_entries; >>>>> >>>>> +extern struct pci_cfg_fixup __start_acpi_mcfg_fixups[]; >>>>> +extern struct pci_cfg_fixup __end_acpi_mcfg_fixups[]; >>>>> + >>>>> +struct pci_ecam_ops *pci_mcfg_get_ops(struct acpi_pci_root *root) >>>>> +{ >>>>> + int bus_num = root->secondary.start; >>>>> + int domain = root->segment; >>>>> + struct pci_cfg_fixup *f; >>>>> + >>>>> + if (!mcfg_table) >>>>> + return &pci_generic_ecam_ops; >>>>> + >>>>> + /* >>>>> + * Match against platform specific quirks and return >>>> corresponding >>>>> + * CAM ops. >>>>> + * >>>>> + * First match against PCI topology then use OEM ID >>>> and >>>>> + * OEM revision from MCFG table standard header. >>>>> + */ >>>>> + for (f = __start_acpi_mcfg_fixups; f < __end_acpi_mcfg_fixups; >>>> f++) { >>>>> + if ((f->domain == domain || f->domain == >>>> PCI_MCFG_DOMAIN_ANY) && >>>>> + (f->bus_num == bus_num || f->bus_num == >>>> PCI_MCFG_BUS_ANY) && >>>>> + (!strncmp(f->oem_id, mcfg_table->header.oem_id, >>>>> + ACPI_OEM_ID_SIZE)) && >>>>> + (f->oem_revision == mcfg_table->header.oem_revision)) >>>> >>>> Is this more likely to be updated between quirky and fixed platforms >>>> than oem_table_id? What do folks think about using oem_table_id >> instead >>>> of, or in addition to, oem_revision? >>> >>> From my understanding we need to stick to this mechanism as >> (otherwise) >>> there are platforms out in the field that would need a FW update. >>> >>> So I don't think that using oem_table_id "instead" is possible; about >>> "in addition" I think it is doable, but I do not see the advantage >> much. >>> I mean that if a platform gets fixed the oem revision should change >> too, >>> Right? >> >> Cov and I had a discussion about this, so hopefully I can bring a >> slightly different perspective that will make sense. >> >> We forsee a situation where we have platform A that needs a quirk, and >> platform B that does not. The OEM id is the same for both platforms as >> they are different platforms from the same OEM. Using the OEM revision >> field does not seem to be appropriate since these are different >> platforms and the revision field appears to be for the purpose of >> tracking differences within a single platform. Therefore, Cov is >> proposing using the OEM table id as a mechanism to distinguish platform >> A (needs quirk applied) vs platform B (no quirks) from the same OEM. > > Ah yes I see now... > > Probably it should be ok to have a check on all three OEM fields. Just for reference, x86 and IA64 use oem_id and oem_table_id to make a difference between different platforms, see acpi_madt_oem_check(char *oem_id, char *oem_table_id) for x86 and ia64, that can apply to ARM64 on MCFG too. Thanks Hanjun