Messages in this thread | | | Date | Mon, 2 Jun 2008 16:19:29 +0200 | From | Robert Richter <> | Subject | Re: [PATCH] x86: Add PCI extended config space access for AMD Barcelona |
| |
Arjan,
On 28.05.08 12:02:53, Arjan van de Ven wrote: > Comment 1: > Can we make the 256/4096 thing conditional on actually having the > feature somehow? (while not making the code TOO ugly)
In the first version I had 2 functions also. The patch have had lots of duplicate code or inline functions. Since the conditional check is already in raw_pci_* I decided to not implement an additional check and use only one function.
int raw_pci_read(unsigned int domain, unsigned int bus, unsigned int devfn, int reg, int len, u32 *val) { if (reg < 256 && raw_pci_ops) return raw_pci_ops->read(domain, bus, devfn, reg, len, val); if (raw_pci_ext_ops) return raw_pci_ext_ops->read(domain, bus, devfn, reg, len, val); return -EINVAL; }
That leaves as a difference to the basic access is the shift left of bits 8:11 in the PCI_CONF1_ADDRESS macro. Functional the new macro is the same and the overhead for this is small. So I see keeping all code in one function as the best solution.
> Comment 2: > The cpu_has_XXX is a bit dubious; while it's dependent on your cpu > model right now, I'm a bit hesitant to consider a PCI feature something > that belongs in the cpu_has_XXX namespace. (Yes I know PCI is moving > into the cpu package, but on a logical level it seems just the wrong > place). > Do we need a platform_has_XXX namespace for things like this?
An alternative implementation would be here to use a check something like pci_probe & PCI_HAS_EXT_CFG. If needed, I will send an updated patch.
-Robert
-- Advanced Micro Devices, Inc. Operating System Research Center email: robert.richter@amd.com
| |