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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@xxxxxxxx>]

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

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