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

[Oleksandr Andrushchenko <Oleksandr_Andrushchenko@epam.com>]

On 10.09.21 15:01, Rahul Singh wrote:
> Hi Stefano,
>
>> On 10 Sep 2021, at 12:34 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>
>> On Thu, 19 Aug 2021, Rahul Singh wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> 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@epam.com>
>>> ---
>>> 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@arm.com>
>> 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 <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 = "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.

>
> Regards,
> Rahul
>
>
Thanks,

Oleksandr