[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On Fri, 24 Sep 2021, 23:42 Stefano Stabellini, <sstabellini@xxxxxxxxxx> 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 = "" },                                 
> >>
> >>    { },                                                                       
> >> };
> >
> > 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.

IIRC it is used by the SMMU driver. But you need to lookup for the desc manually in each init callback.

If I am not mistaken, this is how Linux is dealing with it as well. However...


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?

... I like the idea of passing desc parameter (we could also simply pass desc.data in an argument named "priv").

Cheers,

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.