Re: [PATCH v1 05/14] xen/arm: PCI host bridge discovery within XEN on ARM

[Julien Grall <julien@xxxxxxx>]

On 10/09/2021 12:22, Rahul Singh wrote:
> Hi Julien
Hi Rahul,
> > On 7 Sep 2021, at 10:05 am, Julien Grall <julien@xxxxxxx> wrote:
> > On 19/08/2021 13:02, Rahul Singh wrote:
> > > +#include <xen/rwlock.h>
> > > +#include <xen/sched.h>
> > > +#include <xen/vmap.h>
> > > +
> > > +/*
> > > + * List for all the pci host bridges.
> > > + */
> > > +
> > > +static LIST_HEAD(pci_host_bridges);
> > > +
> > > +static atomic_t domain_nr = ATOMIC_INIT(-1);
> > > +
> > > +bool dt_pci_parse_bus_range(struct dt_device_node *dev,
> > > +                            struct pci_config_window *cfg)
> > Aside, "pci_config_window", the function is not Arm specific. Would it be possible to 
> > consider to introduce "struct resource" in Xen so this function can be moved in 
> > common/device_tree.c?
> I can introduce the "struct resource” but I am not sure whether "struct 
> resource” will be
> useful later point in time. What I prefer as of now, we can have this function 
> and we can
> move this to common/device_tree.c once we have the requirement for "struct 
> resource”.
TBH, I wasn't asking about using "struct resource". This was simply a 
suggestion how to make the code common.
What I am more interested is moving this function in common code so it 
can be re-used by RISC-v in the future. I don't particularly mind 
whether this is using "struct resource" or a different structure.
> > > diff --git a/xen/arch/arm/pci/pci-host-generic.c 
> > > b/xen/arch/arm/pci/pci-host-generic.c
> > > new file mode 100644
> > > index 0000000000..13d0f7f999
> > > --- /dev/null
> > > +++ b/xen/arch/arm/pci/pci-host-generic.c
> > > @@ -0,0 +1,55 @@
> > > +/*
> > > + * Copyright (C) 2021 Arm Ltd.
> > > + *
> > > + * Based on Linux drivers/pci/controller/pci-host-common.c
> > > + * Based on Linux drivers/pci/controller/pci-host-generic.c
> > > + * Copyright (C) 2014 ARM Limited Will Deacon <will.deacon@xxxxxxx>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + *
> > > + * This program is distributed in the hope that it will be useful,
> > > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > > + * GNU General Public License for more details.
> > > + *
> > > + * You should have received a copy of the GNU General Public License
> > > + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> > > + */
> > > +
> > > +#include <asm/device.h>
> > > +#include <xen/pci.h>
> > > +#include <asm/pci.h>
> > > +
> > > +static const struct dt_device_match gen_pci_dt_match[] = {
> > > +    { .compatible = "pci-host-ecam-generic" },
> > > +    { },
> > > +};
> > > +
> > > +static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
> > > +{
> > > +    const struct dt_device_match *of_id;
> > > +
> > > +    of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
> > This seems to be a bit pointless to me as you already know the compatible 
> > (there is only one possible...).
>   As of now we are only implementing the "pci-host-ecam-generic” compatible 
> PCI, but in future we might
> need to implement the other compatible like Linux see as below.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-host-generic.c#L59
Right... You know the node matched and you don't need any data. So this 
is a bit pointless to have to walk again the structure for just printing 
the compatible that matched.
Cheers,

--
Julien Grall