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

[Julien Grall <julien@xen.org>]

On 10/09/2021 12:22, Rahul Singh wrote:
> Hi Julien

Hi Rahul,
>> On 7 Sep 2021, at 10:05 am, Julien Grall <julien@xen.org> 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@arm.com>
>>> + *
>>> + * 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