Re: [PATCH v2 11/17] xen/arm: PCI host bridge discovery within XEN on ARM

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Stefano,

> On 25 Sep 2021, at 12:26 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> wrote:
> 
> On Fri, 24 Sep 2021, Stefano Stabellini wrote:
>> On Fri, 24 Sep 2021, Rahul Singh wrote:
>>> Hi Stefano,
>>> 
>>>> On 23 Sep 2021, at 8:12 pm, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
>>>> wrote:
>>>> 
>>>> On Thu, 23 Sep 2021, Rahul Singh wrote:
>>>>>>> +            goto err_exit;
>>>>>>> +    }
>>>>>> 
>>>>>> This is unnecessary at the moment, right? Can we get rid of ops->init ?
>>>>> 
>>>>> No this is required for N1SDP board. Please check below patch.
>>>>> https://gitlab.com/rahsingh/xen-integration/-/commit/6379ba5764df33d57547087cff4ffc078dc515d5
>>>> 
>>>> OK
>>>> 
>>>> 
>>>>>>> +int pci_host_common_probe(struct dt_device_node *dev, const void *data)
>>>>>>> +{
>>>>>>> +    struct pci_host_bridge *bridge;
>>>>>>> +    struct pci_config_window *cfg;
>>>>>>> +    struct pci_ecam_ops *ops;
>>>>>>> +    const struct dt_device_match *of_id;
>>>>>>> +    int err;
>>>>>>> +
>>>>>>> +    if ( dt_device_for_passthrough(dev) )
>>>>>>> +        return 0;
>>>>>>> +
>>>>>>> +    of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
>>>>>>> +    ops = (struct pci_ecam_ops *) of_id->data;
>>>>>> 
>>>>>> Do we really need dt_match_node and dev->dev.of_match_table to get
>>>>>> dt_device_match.data?
>>>>>> 
>>>>> 
>>>>>> data is passed as a parameter to pci_host_common_probe, isn't it enough
>>>>>> to do:
>>>>>> 
>>>>>> ops = (struct pci_ecam_ops *) data;
>>>>> 
>>>>> As of now not required but in future we might need it if we implement 
>>>>> other ecam supported bridge
>>>>> 
>>>>> static const struct dt_device_match gen_pci_dt_match[] = {                
>>>>>       
>>>>>   { .compatible = "pci-host-ecam-generic",                                
>>>>>     
>>>>>     .data =       &pci_generic_ecam_ops },
>>>>> 
>>>>>   { .compatible = "pci-host-cam-generic",
>>>>>     .data = &gen_pci_cfg_cam_bus_ops },                                 
>>>>> 
>>>>>   { },                                                                    
>>>>>     
>>>>> };
>>>> 
>>>> Even if we add another ECAM-supported bridge, the following:
>>>> 
>>>> ops = (struct pci_ecam_ops *) data;
>>>> 
>>>> could still work, right? The probe function will directly receive as
>>>> parameter the .data pointer. You shouldn't need the indirection via
>>>> dt_match_node?
>>> 
>>> As per my understanding probe function will not get .data pointer.Probe 
>>> data argument is NULL in most of the cases in XEN
>>> Please have a look once dt_pci_init() -> device_init(..) call flow 
>>> implementation.
>> 
>> You are right. Looking at the code, nobody is currently using
>> dt_device_match.data and it is clear why: it is not passed to the
>> device_desc.init function at all. As it is today, it is basically
>> useless.
>> 
>> And there is only one case where device_init has a non-NULL data
>> parameter and it is in xen/drivers/char/arm-uart.c. All the others are
>> not even using the data parameter of device_init.
>> 
>> I think we need to change device_init so that dt_device_match.data can
>> be useful. Sorry for the scope-creep but I think we should do the
>> following:
>> 
>> - do not add of_match_table to struct device
>> 
>> - add one more parameter to device_desc.init:
>>  int (*init)(struct dt_device_node *dev, struct device_desc *desc, const 
>> void *data);
>> 
>> - change device_init to call desc->init with the right parameters:
>>  desc->init(dev, desc, data);
>> 
>> This way pci_host_common_probe is just going to get a desc directly as
>> parameter. I think it would make a lot more sense from an interface
>> perspective. It does require a change in all the DT_DEVICE_START.init
>> functions adding a struct device_desc *desc parameter, but it should be
>> a mechanical change.
>> 
>> Alternatively we could just change device_init to pass
>> device_desc.dt_match.data when the data parameter is NULL but it feels
>> like a hack.
>> 
>> 
>> What do you think?
> 
> 
> Another idea that doesn't require a device_desc.init change and also
> doesn't require a change to struct device is the following:
> 
> 
> diff --git a/xen/arch/arm/pci/pci-host-common.c 
> b/xen/arch/arm/pci/pci-host-common.c
> index a88f20175e..1aa0ef4c1e 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -205,8 +205,7 @@ int pci_host_common_probe(struct dt_device_node *dev, 
> const void *data)
>     if ( dt_device_for_passthrough(dev) )
>         return 0;
> 
> -    of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
> -    ops = (struct pci_ecam_ops *) of_id->data;
> +    ops = (struct pci_ecam_ops *) data;
> 
>     bridge = pci_alloc_host_bridge();
>     if ( !bridge )
> diff --git a/xen/arch/arm/pci/pci-host-generic.c 
> b/xen/arch/arm/pci/pci-host-generic.c
> index 6b3288d6f3..66fb843f49 100644
> --- a/xen/arch/arm/pci/pci-host-generic.c
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -20,15 +20,19 @@
> #include <asm/pci.h>
> 
> static const struct dt_device_match gen_pci_dt_match[] = {
> -    { .compatible = "pci-host-ecam-generic",
> -      .data =       &pci_generic_ecam_ops },
> -
> +    { .compatible = "pci-host-ecam-generic" },
>     { },
> };
> 
> +static int pci_host_generic_probe(struct dt_device_node *dev,
> +                                         const void *data)
> +{
> +    return pci_host_common_probe(dev, &pci_generic_ecam_ops);
> +}
> +
> DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
> .dt_match = gen_pci_dt_match,
> -.init = pci_host_common_probe,
> +.init = pci_host_generic_probe,
> DT_DEVICE_END
> 
> /*

I also think this is good idea to avoid modification for device_init(.) and 
struct device{} .
I will use this patch in next version.

Regards,
Rahul