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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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 <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.

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.

[1] https://marc.info/?l=xen-devel&m=163154474008598