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

Commit Message

Rahul Singh Aug. 19, 2021, 12:02 p.m. UTC

From: Oleksandr Andrushchenko <[e-mail]>

Add support for Xilinx ZynqMP PCI host controller to map the PCI config
space to the XEN memory.

Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
---
 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

Comments

Stefano Stabellini Sept. 9, 2021, 11:34 p.m. UTC | #1

On Thu, 19 Aug 2021, Rahul Singh wrote:
> From: Oleksandr Andrushchenko <[e-mail]>
> 
> Add support for Xilinx ZynqMP PCI host controller to map the PCI config
> space to the XEN memory.
> 
> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
> ---
>  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 <[e-mail]>

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.


> + * 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


> +    ops = (struct pci_ecam_ops *) of_id->data;
> +
> +    printk(XENLOG_INFO "Found PCI host bridge %s compatible:%s \n",
> +            dt_node_full_name(dev), of_id->compatible);
> +
> +    return pci_host_common_probe(dev, ops, 2);
> +}
> +
> +DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
> +.dt_match = gen_pci_dt_match,
> +.init = gen_pci_dt_init,
> +DT_DEVICE_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Rahul Singh Sept. 10, 2021, 12:01 p.m. UTC | #2

Hi Stefano,

> On 10 Sep 2021, at 12:34 am, Stefano Stabellini <[e-mail]> wrote:
> 
> On Thu, 19 Aug 2021, Rahul Singh wrote:
>> From: Oleksandr Andrushchenko <[e-mail]>
>> 
>> Add support for Xilinx ZynqMP PCI host controller to map the PCI config
>> space to the XEN memory.
>> 
>> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
>> ---
>> 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 <[e-mail]>
> 
> 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.

Regards,
Rahul

Oleksandr Andrushchenko Sept. 13, 2021, 2:46 p.m. UTC | #3

On 10.09.21 15:01, Rahul Singh wrote:
> Hi Stefano,
>
>> On 10 Sep 2021, at 12:34 am, Stefano Stabellini <[e-mail]> wrote:
>>
>> On Thu, 19 Aug 2021, Rahul Singh wrote:
>>> From: Oleksandr Andrushchenko <[e-mail]>
>>>
>>> Add support for Xilinx ZynqMP PCI host controller to map the PCI config
>>> space to the XEN memory.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
>>> ---
>>> 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 <[e-mail]>
>> 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

Stefano Stabellini Sept. 13, 2021, 9:02 p.m. UTC | #4

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 <[e-mail]> wrote:
> >>
> >> On Thu, 19 Aug 2021, Rahul Singh wrote:
> >>> From: Oleksandr Andrushchenko <[e-mail]>
> >>>
> >>> Add support for Xilinx ZynqMP PCI host controller to map the PCI config
> >>> space to the XEN memory.
> >>>
> >>> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
> >>> ---
> >>> 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 <[e-mail]>
> >> 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

Oleksandr Andrushchenko Sept. 14, 2021, 4:31 a.m. UTC | #5

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 <[e-mail]> wrote:
>>>>
>>>> On Thu, 19 Aug 2021, Rahul Singh wrote:
>>>>> From: Oleksandr Andrushchenko <[e-mail]>
>>>>>
>>>>> Add support for Xilinx ZynqMP PCI host controller to map the PCI config
>>>>> space to the XEN memory.
>>>>>
>>>>> Signed-off-by: Oleksandr Andrushchenko <[e-mail]>
>>>>> ---
>>>>> 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 <[e-mail]>
>>>> 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]

Rahul Singh Sept. 17, 2021, 7:39 a.m. UTC | #6

Hi Oleksandr, Stefano,

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

Patch

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 <[e-mail]>
+ *
+ * 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);
+    ops = (struct pci_ecam_ops *) of_id->data;
+
+    printk(XENLOG_INFO "Found PCI host bridge %s compatible:%s \n",
+            dt_node_full_name(dev), of_id->compatible);
+
+    return pci_host_common_probe(dev, ops, 2);
+}
+
+DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
+.dt_match = gen_pci_dt_match,
+.init = gen_pci_dt_init,
+DT_DEVICE_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */