Hi Oleksandr, Stefano, > On 14 Sep 2021, at 5:31 am, Oleksandr Andrushchenko > <[email protected]> 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 <[email protected]> >>>>> wrote: >>>>> >>>>> On Thu, 19 Aug 2021, Rahul Singh wrote: >>>>>> From: Oleksandr Andrushchenko <[email protected]> >>>>>> >>>>>> Add support for Xilinx ZynqMP PCI host controller to map the PCI config >>>>>> space to the XEN memory. >>>>>> >>>>>> Signed-off-by: Oleksandr Andrushchenko <[email protected]> >>>>>> --- >>>>>> 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 <[email protected]> >>>>> 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]
