Re: [PATCH v1 07/14] xen/arm: Add support for Xilinx ZynqMP PCI host controller

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Oleksandr, Stefano,

> On 14 Sep 2021, at 5:31 am, Oleksandr Andrushchenko 
> <Oleksandr_Andrushchenko@xxxxxxxx> wrote:
> 
> 
> On 14.09.21 00:02, Stefano Stabellini wrote:
>> On Mon, 13 Sep 2021, Oleksandr Andrushchenko wrote:
>>> On 10.09.21 15:01, Rahul Singh wrote:
>>>> Hi Stefano,
>>>> 
>>>>> On 10 Sep 2021, at 12:34 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
>>>>> wrote:
>>>>> 
>>>>> On Thu, 19 Aug 2021, Rahul Singh wrote:
>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>>> 
>>>>>> Add support for Xilinx ZynqMP PCI host controller to map the PCI config
>>>>>> space to the XEN memory.
>>>>>> 
>>>>>> Signed-off-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>>> ---
>>>>>> xen/arch/arm/pci/Makefile          |  1 +
>>>>>> xen/arch/arm/pci/pci-host-zynqmp.c | 59 ++++++++++++++++++++++++++++++
>>>>>> 2 files changed, 60 insertions(+)
>>>>>> create mode 100644 xen/arch/arm/pci/pci-host-zynqmp.c
>>>>>> 
>>>>>> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
>>>>>> index 6f32fbbe67..1d045ade01 100644
>>>>>> --- a/xen/arch/arm/pci/Makefile
>>>>>> +++ b/xen/arch/arm/pci/Makefile
>>>>>> @@ -3,3 +3,4 @@ obj-y += pci-access.o
>>>>>> obj-y += pci-host-generic.o
>>>>>> obj-y += pci-host-common.o
>>>>>> obj-y += ecam.o
>>>>>> +obj-y += pci-host-zynqmp.o
>>>>>> diff --git a/xen/arch/arm/pci/pci-host-zynqmp.c 
>>>>>> b/xen/arch/arm/pci/pci-host-zynqmp.c
>>>>>> new file mode 100644
>>>>>> index 0000000000..fe103e3855
>>>>>> --- /dev/null
>>>>>> +++ b/xen/arch/arm/pci/pci-host-zynqmp.c
>>>>>> @@ -0,0 +1,59 @@
>>>>>> +/*
>>>>>> + * Copyright (C) 2020-2021 EPAM Systems
>>>>>> + *
>>>>>> + * Based on Linux drivers/pci/controller/pci-host-common.c
>>>>>> + * Based on Linux drivers/pci/controller/pci-host-generic.c
>>>>>> + * Based on xen/arch/arm/pci/pci-host-generic.c
>>>>>> + * Copyright (C) 2014 ARM Limited Will Deacon <will.deacon@xxxxxxx>
>>>>> Only one Copyright line per file is enough :-)
>>>>> 
>>>>> But actually all the Copyright lines with a name or a company name are
>>>>> not really required or useful, as the copyright is noted in full details
>>>>> in the commit messages (author and signed-off-by lines). I would remove
>>>>> them all from the new files added by this series.
>>>> Ok. Let me remove in next version.
>>>>>> + * 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 
>>>>>> <https://urldefense.com/v3/__http://www.gnu.org/licenses/__;!!GF_29dbcQIUBPA!lAdL_CvsuMuuX9ai6cwzm3NYiT1vwIIlxGU7nezSqq_nqJk40Zz-kT44LOsemcghJ_3j2CfflQ$
>>>>>>  [gnu[.]org]>.
>>>>>> + */
>>>>>> +
>>>>>> +#include <asm/device.h>
>>>>>> +#include <xen/pci.h>
>>>>>> +#include <asm/pci.h>
>>>>>> +
>>>>>> +static const struct dt_device_match gen_pci_dt_match[] = {
>>>>>> +    { .compatible = "xlnx,nwl-pcie-2.11",
>>>>>> +      .data =       &pci_generic_ecam_ops },
>>>>>> +    { },
>>>>>> +};
>>>>>> +
>>>>>> +static int gen_pci_dt_init(struct dt_device_node *dev, const void *data)
>>>>>> +{
>>>>>> +    const struct dt_device_match *of_id;
>>>>>> +    const struct pci_ecam_ops *ops;
>>>>>> +
>>>>>> +    of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node);
>>>>> This should be superfluous
>>>> Ack. I will remove the dt_match_node(..) in next version.
>>> I am not entirely sure we need this patch at all as the main reason for its 
>>> existence
>>> 
>>> was that we can run Xilinx QEMU for ZCU102. But, the final setup is not 
>>> going
>>> 
>>> to be functional as legacy IRQs are not supported and ITS is not a part of 
>>> ZynqMP.
>>> 
>>> So, QEMU allows to do a lot with PCI passthrough, but at the end of the day 
>>> one
>>> 
>>> won't have it working...
>>> 
>>> Please consider
>>> 
>>> If we decide to remove it then
>>> 
>>> int pci_host_common_probe(struct dt_device_node *dev,
>>>                            const struct pci_ecam_ops *ops,
>>>                            int ecam_reg_idx)
>>> 
>>> doesn't need the last parameter.
>> With my open source maintainer hat on, I don't see this patch as very
>> important; from that point of view I'd be happy for it to be dropped.
>> However, it would be good to have at least one non-default host bridge
>> (doesn't have to be the Xilinx bridge), otherwise it becomes difficult
>> to understand how the generic infrastructure introduced by this series
>> could be useful.
>> 
>> Moreover, your recent comment [1] made it even more evident that it
>> would be good to have at least two different drivers to spot
>> compatibility issues between them more easily.
> 
> Don't take me wrong here ;) I still do use Xilinx QEMU for most of
> 
> the tests, so it is good for me to have this patch in the tree. But,
> 
> to be fair, Xilinx QEMU won't give you a possibility to see the fully
> 
> functional system. This is why I say the patch can be dropped.
> 
> If we add some words in the commit message about this and have the
> 
> patch in the tree I'll be more than happy

I will have this patch in my series and I will add more comments about the 
patch.

Regards,
Rahul
> 
>> 
>> [1] 
>> https://urldefense.com/v3/__https://marc.info/?l=xen-devel&m=163154474008598__;!!GF_29dbcQIUBPA!lAdL_CvsuMuuX9ai6cwzm3NYiT1vwIIlxGU7nezSqq_nqJk40Zz-kT44LOsemcghJ_0bKs6zpA$
>>  [marc[.]info]