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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

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.

Regards,
Rahul
 
> 
> If you are worried about gen_pci_cfg_cam_bus_ops not being a struct
> pci_ecam_ops: that problem can also be solved by making
> gen_pci_cfg_cam_bus_ops a struct containinig a struct pci_ecam_ops.