Re: [kvm-unit-tests PATCH 01/11] arm/pci: Device tree PCI probing

[Andrew Jones <drjones@redhat.com>]

On Fri, Feb 05, 2016 at 12:48:50PM +0100, Alexander Gordeev wrote:
> On Wed, Jan 13, 2016 at 04:13:07PM +0100, Andrew Jones wrote:
> > > diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> > > new file mode 100644
> > > index 0000000..097ac2d
> > > --- /dev/null
> > > +++ b/lib/pci-host-generic.h
> > > @@ -0,0 +1,26 @@
> > > +#ifndef PCI_HOST_GENERIC_H
> > > +#define PCI_HOST_GENERIC_H
> > > +
> > > +struct pci_addr_range {
> > > +	phys_addr_t			start;
> > > +	phys_addr_t			size;
> > > +};
> > > +
> > > +struct pci_addr_space {
> > > +	struct pci_addr_range		cpu_range;
> > > +	struct pci_addr_range		pci_range;
> > > +	phys_addr_t			alloc;
> > 
> > alloc isn't used in this patch, but guess it will be later
> > 
> > > +	u32				of_flags;
> > > +};
> > > +
> > > +struct pci_host_bridge {
> > > +	struct pci_addr_range		cpu_range;
> > > +	int				bus;
> > > +	int				bus_max;
> > > +	int				nr_addr_spaces;
> > > +	struct pci_addr_space		addr_space[];
> > > +};
> > > +
> > > +#define PCI_ECAM_BUS_SIZE		(1 << 20)
> > 
> > Why not put this struct declaration (and supporting structs) in pci.h?
> > Linux has struct pci_host_bridge in include/linux/pci.h
> 
> I think we do not need any of these structures to get public.
> 
> My initial attempt was also an assessment of an idea to
> generalize PCI code across architectures. But my current
> judgement - an outcome is not worth it. With x86 PCI access
> so much different and only bus 0 function 0 "hierarchy" we
> only need the simplest required routines.
> 
> Hence, I would leave these in pci-host-generic.h (just not
> to blow pci-host-generic.c too big).
> 
> As result, just these routines would end up in pci.h:
> 
> 	bool pci_probe(void);
> 	void pci_free(void);	/* pci_shutdown() ? */ 
> 	int pci_bus_scan(void);
> 	int pci_testdev(void);
> 
> Thoughts?

Whichever you prefer. We either "bloat" pci.h or we "bloat" the lib
directory with another file :-) Actually, I still prefer pci.h, because
if arm unit tests will always need both of them, then they'll always
need two #include lines... OTOH, if you don't think unit tests will
need these structures, then they could just be defined right in
pci-host-generic.c (or pci.c if you consolidate the source files).

Thanks,
drew

> 
> > > +
> > > +#endif
> > > diff --git a/lib/pci.h b/lib/pci.h
> > > new file mode 100644
> > > index 0000000..22b8e31
> > > --- /dev/null
> > > +++ b/lib/pci.h
> > > @@ -0,0 +1,13 @@
> > > +#ifndef PCI_H
> > > +#define PCI_H
> > 
> > This file needs its copyright/gpl header. I also like to document
> > the lib here in the header file, e.g. see lib/devicetree.h and
> > lib/alloc.h
> > 
> > > +
> > > +#include "alloc.h"
> > > +
> > > +struct pci {
> > > +	void *sysdata;
> > 
> > The choice of sysdata for the name appears to come from the Linux
> > kernel. Why not also call 'struct pci' 'struct pci_dev' for full
> > consistency?
> > 
> > > +};
> > > +
> > > +extern struct pci *pci_dt_probe(void);
> > > +extern void pci_shutdown(struct pci *pci);
> > > +
> > > +#endif
> > > -- 
> > > 1.8.3.1
> > >