'arm' shouldn't be in the subject of this patch.
On Thu, Feb 11, 2016 at 09:25:02AM +0100, Alexander Gordeev wrote:
> Scan bus 0 only and function 0 only on each device.
>
> Operations pci_config_read*/pci_config_write* are just
> wrappers over read*/write* accessors and only needed
> to emphasize PCI configuration space access.
>
> Cc: Thomas Huth <thuth@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> lib/pci-host-generic.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++-
> lib/pci-host-generic.h | 34 +++++++++++++++++++++++
> 2 files changed, 107 insertions(+), 1 deletion(-)
>
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> index 9bc6642..fd3bb34 100644
> --- a/lib/pci-host-generic.c
> +++ b/lib/pci-host-generic.c
> @@ -38,6 +38,18 @@ static pci_res_type_t flags_to_type(u32 of_flags)
> return ((of_flags & 0x40000000) >> 28) | ((of_flags >> 24) & 0x03);
> }
>
> +int find_next_dev(struct gen_pci *pci, int dev)
> +{
> + for (dev++; dev < PCI_NUM_DEVICES; dev++) {
> + void *conf = dev_conf(pci, dev);
> +
> + if (pci_config_readl(conf, PCI_VENDOR_ID) != ((u32)~0))
> + return dev;
> + }
> +
> + return -1;
> +}
find_next_dev, used by for_each_pci_dev, seems a bit crufty to me. Why
not just have
for (i = 0; i < 256; ++i)
if ([get pci-vendor-id] == 0xffff)
continue;
everywhere you need to iterate the devices?
> +
> static u32 from_fdt32(fdt32_t val)
> {
> return fdt32_to_cpu(val);
> @@ -199,20 +211,80 @@ static struct gen_pci *gen_pci_probe(void)
> return pci;
> }
>
> +static void pci_dev_print(void *conf)
> +{
> + u16 vendor_id = pci_config_readw(conf, PCI_VENDOR_ID);
> + u16 device_id = pci_config_readw(conf, PCI_DEVICE_ID);
> + u8 header = pci_config_readb(conf, PCI_HEADER_TYPE);
> + u8 progif = pci_config_readb(conf, PCI_CLASS_PROG);
> + u8 subcl = pci_config_readb(conf, PCI_CLASS_DEVICE);
> + u8 class = pci_config_readb(conf, PCI_CLASS_DEVICE + 1);
> +
> + printf("conf %p vendor_id %04x device_id %04x type %d "
> + "progif %02x class %02x subcl %02x\n",
> + conf, vendor_id, device_id, header,
> + progif, class, subcl);
> +}
This should also be in pci.c, and globally accessible for unittests.
It also needs to be written to use the global 'struct pci *pci'
object.
> +
> +static int pci_bus_scan(struct gen_pci *pci)
> +{
> + int dev;
> + int nr_dev = 0;
> +
> + if (!pci)
> + return -1;
This check is unnecessary, as this is a private function. We don't
have any other error conditions, so this should be a void function.
> +
> + for_each_pci_dev(pci, dev) {
> + void *conf = dev_conf(pci, dev);
> + pci_dev_print(conf);
We shouldn't print on every unittest boot. But providing the means to
print is good. Perhaps we need a compile-time flag providing a verbose
mode?
> +
> + /* We are only interested in normal PCI devices */
> + if (pci_config_readb(conf, PCI_HEADER_TYPE) !=
> + PCI_HEADER_TYPE_NORMAL)
> + continue;
> +
> + pci_config_writel(PCI_COMMAND_SERR, conf, PCI_COMMAND);
> + nr_dev++;
> + }
> +
> + return nr_dev;
> +}
> +
> bool pci_probe(void)
This should return the pci pointer.
> {
> struct gen_pci *pci = get_pci();
Don't need the get_pci() call, we have access to the private
pointer directly.
>
> assert(pci == NULL);
> pci = gen_pci_probe();
> + if (!pci)
> + goto probe;
return NULL;
> set_pci(pci);
>
> - return (pci != NULL);
> + if (pci_bus_scan(pci) < 0)
> + goto scan;
This is a void function, no need for goto.
return pci;
No need for labels below.
> +
> + return true;
> +
> +scan:
> + pci_shutdown();
> +
> +probe:
> + return false;
> }
>
> void pci_shutdown(void)
> {
> struct gen_pci *pci = get_pci();
get_pci not needed.
> + int dev;
> +
> + if (!pci)
> + return;
> +
> + for_each_pci_dev(pci, dev) {
> + void *conf = dev_conf(pci, dev);
> +
> + pci_config_writel(PCI_COMMAND_INTX_DISABLE, conf, PCI_COMMAND);
> + }
Should be written in pci.c using pci_config_read/write functions.
>
> set_pci(NULL);
> free(pci);
> diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> index f4ae3e4..1d86b43 100644
> --- a/lib/pci-host-generic.h
> +++ b/lib/pci-host-generic.h
> @@ -69,5 +69,39 @@ typedef enum pci_res_type {
> *
> */
> #define PCI_ECAM_BUS_SIZE (1 << 20)
> +#define PCI_NUM_DEVICES (PCI_ECAM_BUS_SIZE / (1 << 15))
Weird way to write 32 :-)
> +#define PCI_ECAM_CONFIG_SIZE (1 << 12)
> +
> +#define for_each_pci_dev(pci, dev) \
> + for (dev = find_next_dev(pci, -1); \
> + dev >= 0; \
> + dev = find_next_dev(pci, dev))
> +
> +static void* dev_conf(struct gen_pci *pci, int dev)
This function needs pci_ prefixing it, and I would name it
something like pci_get_config
> +{
> + return (void*)pci->cpu_range.start + (dev * 8) * PCI_ECAM_CONFIG_SIZE;
Wait. This should be the implementation of that nice comment about
cfg_offset, right? Shouldn't it be
static void *pci_get_conf(struct gen_pci *pci, int dev, int register)
{
pci_host_bridge *host = container_of(pci, struct pci_host_bridge, pci);
unsigned offset = /* bus << 20 | */ dev << 15 | /* function << 12 | */ register;
return host->cpu_range.start + offset;
}
Of course (dev * 8 * 4096) results in the same thing (not including
register) as (dev << 15)...
(I actually would rather not have the extra cpu_range part in the
pci_host_bridge either. I don't think it gains much in readability,
to just doing host->addr, or some such.)
> +}
> +
> +static u8 pci_config_readb(const void *conf, int off)
> +{
> + return readb(conf + off);
> +}
> +
> +static u16 pci_config_readw(const void *conf, int off)
> +{
> + return readw(conf + off);
> +}
> +
> +static u32 pci_config_readl(const void *conf, int off)
> +{
> + return readl(conf + off);
> +}
> +
> +static void pci_config_writel(u32 val, void *conf, int off)
> +{
> + writel(val, conf + off);
> +}
There's no point to these wrappers. We need a way to get the config
address, like "pci_get_config", but then we can just use readl/writel
directly from within lib/pci-host-generic.c. lib/pci.c should always
us something like pci_config_read(), which is implemented in the arch-
specific header lib/<ARCH>/asm/pci.h, and thus can be something that
uses readl/writel directly too when we're an arch using pci-host-bridge.
> +
> +extern int find_next_dev(struct gen_pci *pci, int dev);
>
> #endif
> --
> 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
