No 'arm' in subject please.
On Thu, Feb 11, 2016 at 09:25:04AM +0100, Alexander Gordeev wrote:
> These two functions is a prerequisite for the following
> pci-testdev test.
>
> Cc: Thomas Huth <thuth@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> lib/pci-host-generic.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 167d0db..9bbf232 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -419,6 +419,67 @@ static int pci_bus_scan(struct gen_pci *pci)
> return nr_dev;
> }
>
> +static pcidevaddr_t encode_addr(int bus, int dev, int fn)
> +{
> + assert(bus < 256 && dev < 32 && fn < 8);
> + return bus << 16 | dev << 11 | fn;
Wait. This is for CAM, not ECAM (and it's missing the fn shift).
ECAM should be
return bus << 20 | dev << 15 | fn << 12 | register;
> +}
> +
> +static void decode_addr(pcidevaddr_t bdf, int *bus, int *dev, int *fn)
> +{
> + *bus = (bdf >> 16) & 0xff;
> + *dev = (bdf >> 11) & 0x1f;
> + *fn = (bdf >> 8) & 0x03;
Here you have the fn shift, but this is still CAM. And what about
register?
> +}
> +
> +pcidevaddr_t pci_find_dev(u16 vendor_id, u16 device_id)
We have this function already in pci.c. We need to implement a
new pci_config_read, rather than a new pci_find_dev.
ARM's and PowerPC's pci_config_read will call into a pci-host-generic
config function that will use pci_get() to get the pci object, which
will lead to the pci-host-bridge object, which will be used by readl.
> +{
> + struct gen_pci *pci = get_pci();
> + int dev;
> +
> + if (!pci)
> + return PCIDEVADDR_INVALID;
Not that this function should be here anyway, but in cases like this
I think an error message + abort() is better. The unittest shouldn't
be allowed to continue if it tries to use something like pci_find_dev
without running pci_probe first.
> +
> + for_each_pci_dev(pci, dev) {
> + void *conf = dev_conf(pci, dev);
> +
> + if (vendor_id == pci_config_readw(conf, PCI_VENDOR_ID) &&
> + device_id == pci_config_readw(conf, PCI_DEVICE_ID))
> + return encode_addr(0, dev, 0);
> + }
> +
> + return PCIDEVADDR_INVALID;
> +}
> +
> +unsigned long pci_bar_addr(pcidevaddr_t bdf, int bar)
> +{
> + struct gen_pci *pci = get_pci();
> + void *conf;
> + struct pci_addr_space *res;
> + pci_res_type_t type;
> + phys_addr_t addr;
> + bool is64;
> + int ret, bus, dev, fn;
> +
> + if (!pci)
> + return ~0;
> +
> + decode_addr(bdf, &bus, &dev, &fn);
> + assert(!bus && !fn); /* We support bus 0 and function 0 only */
Why take a 'bdf' instead of just a dev if only bar=0 and fn=0 are supported.
> +
> + conf = dev_conf(pci, dev);
> + ret = pci_config_readb(conf, PCI_HEADER_TYPE);
> + assert(ret == PCI_HEADER_TYPE_NORMAL);
> +
> + ret = pci_get_bar(conf, bar, &type, &addr, NULL, &is64);
> + assert(ret);
> +
> + res = pci_find_res(pci, type);
> + assert(res);
> +
> + return res->cpu_range.start + (addr - res->pci_range.start);
> +}
I don't understand everything here. So this is where the translation
happens, I take it. Is it normal to have to search the resources for
the type? The pci_bar_addr function we already have in pci.c checks
the BAR with PCI_BASE_ADDRESS_SPACE_IO and knows what type it is. With
the translation going on, it does seem like it'd be hard to merge with
what x86 is using. However, I can't help but think something is wrong
if they're so different, or is this a PCI vs. PCIe thing that requires
a new function?
> +
> bool pci_probe(void)
> {
> struct gen_pci *pci = get_pci();
> --
> 1.8.3.1
>
Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
