Hi Oleksandr,
> On 17 Sep 2021, at 7:13 am, Oleksandr Andrushchenko
> <oleksandr_andrushche...@epam.com> wrote:
>
> Hi, Rahul!
>
> On 15.09.21 23:33, Stefano Stabellini wrote:
>> On Wed, 15 Sep 2021, Rahul Singh wrote:
>>> Hi Oleksandr, Stefano,
>>>
>>>> On 15 Sep 2021, at 6:30 am, Oleksandr Andrushchenko
>>>> <oleksandr_andrushche...@epam.com> wrote:
>>>>
>>>> Hi, Rahul!
>>>>
>>>> On 14.09.21 17:24, Oleksandr Andrushchenko wrote:
>>>>> }
>>>>>>> +static int pci_ecam_register_mmio_handler(struct domain *d,
>>>>>>> + struct pci_host_bridge
>>>>>>> *bridge,
>>>>>>> + const struct
>>>>>>> mmio_handler_ops *ops)
>>>>>>> +{
>>>>>>> + struct pci_config_window *cfg = bridge->sysdata;
>>>>>>> +
>>>>>>> + register_mmio_handler(d, ops, cfg->phys_addr, cfg->size, NULL);
>>>>>>> + return 0;
>>>>>>> +}
>>>>>> Given that struct pci_config_window is generic (it is not specific to
>>>>>> one bridge), I wonder if we even need the .register_mmio_handler
>>>>>> callback here.
>>>>>>
>>>>>> In fact,pci_host_bridge->sysdata doesn't even need to be a void*, it
>>>>>> could be a struct pci_config_window*, right?
>>>>> Rahul, this actually may change your series.
>>>>>
>>>>> Do you think we can do that?
>>>>>
>>>> This is the only change requested that left unanswered by now.
>>>>
>>>> Will it be possible that you change the API accordingly, so I can
>>>>
>>>> implement as Stefano suggests?
>>> We need pci_host_bridge->sysdata as void* in case we need to implement the
>>> new non-ecam PCI controller in XEN.
>>> Please have a look once in Linux code[1] how bridge->sysdata will be used.
>>> struct pci_config_window is used only for
>>> ecam supported host controller. Different PCI host controller will have
>>> different PCI interface to access the PCI controller.
>>>
>>> [1]
>>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-rcar-host.c*n309__;Iw!!GF_29dbcQIUBPA!kjkv6KIlvXOEgVaS6BNPRo0gyABihhO0XmNHRPFJaFAGhhTEuK1mIsWqPs0cXEipzkT_MmA-Eg$
>>> [git[.]kernel[.]org]
>>>
>>> I think we need bridge->sysdata in future to implement the new PCI
>>> controller.
>> In my opinion the pci_config_window is too important of an information
>> to be left inside an opaque pointer, especially when the info under
>> pci_config_window is both critical and vendor-neutral.
>>
>> My preference would be something like this:
>>
>>
>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index 9c28a4bdc4..c80d846da3 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -55,7 +55,6 @@ struct pci_config_window {
>> uint8_t busn_start;
>> uint8_t busn_end;
>> void __iomem *win;
>> - const struct pci_ecam_ops *ops;
>> };
>>
>> /*
>> @@ -68,7 +67,8 @@ struct pci_host_bridge {
>> uint16_t segment; /* Segment number */
>> u8 bus_start; /* Bus start of this bridge. */
>> u8 bus_end; /* Bus end of this bridge. */
>> - void *sysdata; /* Pointer to the config space window*/
>> + struct pci_config_window* cfg; /* Pointer to the bridge config window
>> */
>> + void *sysdata; /* Pointer to bridge private data */
>> const struct pci_ops *ops;
>> };
>>
>>
>> As a reference the attached patch builds. However, I had to remove const
>> where struct pci_ecam_ops *ops is used.
>
> I'd like to know which route we go with this as this is now the last
>
> thing which stops me from sending v2 of this series.
I will modify the code as per Stefano request and will send the next version.
Regards,
Rahul
>
> Thank you,
>
> Oleksandr
