|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] RE: [RFCv4,12/35] plat/platform_bus: Add probe/match interface for platform devices
Hi Razvan
> -----Original Message-----
> From: Razvan Virtan <virtanrazvan@xxxxxxxxx>
> Sent: Saturday, March 20, 2021 1:46 AM
> To: Justin He <Justin.He@xxxxxxx>
> Cc: minios-devel@xxxxxxxxxxxxx
> Subject: Re: [RFCv4,12/35] plat/platform_bus: Add probe/match interface for
> platform devices
>
> I've found some minor typos and I have some questions regarding the
> pf_find_driver()
> function from "platform_bus.c". I'll let them here, as inline comments.
>
> diff --git a/plat/common/platform_bus.c b/plat/common/platform_bus.c
> index 1410e71..fecb389 100644
> --- a/plat/common/platform_bus.c
> +++ b/plat/common/platform_bus.c
> @@ -52,8 +52,10 @@ struct pf_bus_handler {
> };
> static struct pf_bus_handler pfh;
>
> -static const char * const pf_device_list[] = {
>
> Typo: "*pf_device_compatible_list[]"
>
> +static const char *pf_device_compatilbe_list[] = {
> "virtio,mmio",
> + "pci-host-ecam-generic",
> + NULL
> };
>
> static inline int pf_device_id_match(const struct pf_device_id *id0,
> @@ -67,14 +69,22 @@ static inline int pf_device_id_match(const struct
> pf_device_id *id0,
> return rc;
> }
>
> -static inline struct pf_driver *pf_find_driver(struct pf_device_id *id)
> +static inline struct pf_driver *pf_find_driver(const char *compatible)
> {
> struct pf_driver *drv;
> + struct pf_device_id id;
>
> UK_TAILQ_FOREACH(drv, &pfh.drv_list, next) {
> - if (pf_device_id_match(id, drv->device_ids)) {
> - uk_pr_debug("pf driver found devid=%d\n", drv-
> >device_ids->device_id);
> - return drv; /* driver found */
> + if (!drv->match)
> + continue;
> +
> + id.device_id = (uint16_t)drv->match(compatible);
>
> In both the virtio_mmio and pci_ecam implementations, driver match()
> function returns
> -1 when the "compatible" parameter isn't found in the driver's match table.
> Therefore, I think the condition in this statement should be "if
> (id.device_id > 0)".
Yes, thanks. More than that, I'd like to change it as
(id.device_id >= PLATFORM_DEVICE_ID_START)
>
> + if (id.device_id) {
>
> What would happen if a platform driver would support more than one device?
> For example, the implementation of "virtio_mmio_match_table[]" from
> "virtio_mmio.c"
> would allow, theoretically, more than one supported devices. If that would
> happen,
> I think "drv->device_ids" should become an array of ids and we should call
> "pf_device_id_match()"
> on all of it's elements.
> Would it be possible to need a platform driver like that in the future? (if
> it would
> be possible, I think we should adapt the implementation)
>
Yes, I will change device_ids to ids array
> + if (pf_device_id_match(&id, drv->device_ids)) {
> + uk_pr_debug("pf driver found devid(0x%x)\n",
> id.device_id);
> +
> + return drv;
> + }
> }
> }
>
> @@ -84,104 +94,96 @@ static inline struct pf_driver *pf_find_driver(struct
> pf_device_id *id)
> }
>
> static inline int pf_driver_add_device(struct pf_driver *drv,
> - struct pf_device_id *devid,
> - __u64 dev_base,
> - int dev_irq)
> + struct pf_device *dev)
> {
> - struct pf_device *dev;
> int ret;
>
> UK_ASSERT(drv != NULL);
> UK_ASSERT(drv->add_dev != NULL);
> + UK_ASSERT(dev != NULL);
>
> - dev = (struct pf_device *) uk_calloc(pfh.a, 1, sizeof(*dev));
> - if (!dev) {
> - uk_pr_err("Platform : Failed to initialize: Out of memory!\n");
> - return -ENOMEM;
> - }
> -
> - memcpy(&dev->id, devid, sizeof(dev->id));
> - uk_pr_debug("pf_driver_add_device dev->id=%d\n", dev->id.device_id);
> -
> - dev->base = dev_base;
> - dev->irq = dev_irq;
> + uk_pr_debug("pf_driver_add_device devid=%d\n", dev->id.device_id);
>
> ret = drv->add_dev(dev);
> if (ret < 0) {
> uk_pr_err("Platform Failed to initialize device driver\n");
> - uk_free(pfh.a, dev);
> }
>
> return ret;
> }
>
> +static inline int pf_driver_probe_device(struct pf_driver *drv,
> + struct pf_device *dev)
> +{
> + int ret;
> +
> + UK_ASSERT(drv != NULL && dev != NULL);
> + UK_ASSERT(drv->probe != NULL);
> +
> + uk_pr_info("pf_driver_probe_device devid=%d\n", dev->id.device_id);
> +
> + ret = drv->probe(dev);
> + if (ret < 0) {
> + uk_pr_err("Platform Failed to probe device driver\n");
> +
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int pf_probe(void)
> {
> - struct pf_device_id devid;
> struct pf_driver *drv;
> - int i;
> - int end_offset = -1;
> + int idx = 0;
> int ret = -ENODEV;
> - const fdt32_t *prop;
> - int type, hwirq, prop_len;
> - __u64 reg_base;
> - __phys_addr dev_base;
> - int dev_irq;
> + struct pf_device *dev;
> + int fdt_pf = -1;
>
> uk_pr_info("Probe PF\n");
>
> - /* We only support virtio_mmio as a platform device here.
> - * A loop here is needed for finding drivers if more devices
> - */
> - devid.device_id = VIRTIO_MMIO_ID;
> + /* Search all the platform bus devices provided by fdt */
> + do {
>
> Typo: "pf_device_compatible_list"
Okay
>
> + fdt_pf =
> fdt_node_offset_idx_by_compatible_list(_libkvmplat_cfg.dtb,
> + fdt_pf,
> pf_device_compatilbe_list,
> &idx);
> + if (fdt_pf < 0) {
> + uk_pr_info("End of searching platform devices\n");
> + break;
> + }
>
> - drv = pf_find_driver(&devid);
> - if (!drv) {
> - uk_pr_info("<no driver>\n");
> - return -ENODEV;
> - }
> + /* Alloc dev */
> + dev = (struct pf_device *) uk_calloc(pfh.a, 1, sizeof(*dev));
> + if (!dev) {
> + uk_pr_err("Platform : Failed to initialize: Out of
> memory!\n");
> + return -ENOMEM;
> + }
>
> - uk_pr_info("driver %p\n", drv);
> -
> - /* qemu creates virtio devices in reverse order */
> - for (i = 0; i < UK_MAX_VIRTIO_MMIO_DEVICE; i++) {
> - end_offset = fdt_node_offset_by_compatible_list(fdt_start,
> - end_offset,
> - pf_device_list);
> - if (end_offset == -FDT_ERR_NOTFOUND) {
> - uk_pr_info("device not found in fdt\n");
> - goto error_exit;
> - } else {
> - prop = fdt_getprop(fdt_start, end_offset, "interrupts",
> &prop_len);
> - if (!prop) {
> - uk_pr_err("irq of device not found in fdt\n");
> - goto error_exit;
> - }
> + dev->fdt_offset = fdt_pf;
>
> - type = fdt32_to_cpu(prop[0]);
> - hwirq = fdt32_to_cpu(prop[1]);
> + /* Find drv with compatible-id match table */
>
> Typo: "pf_device_compatible_list"
>
> + drv = pf_find_driver(pf_device_compatilbe_list[idx]);
> + if (!drv) {
> + uk_free(pfh.a, dev);
> + continue;
> + }
>
> - prop = fdt_getprop(fdt_start, end_offset, "reg",
> &prop_len);
> - if (!prop) {
> - uk_pr_err("reg of device not found in fdt\n");
> - goto error_exit;
> - }
> + dev->id = *(struct pf_device_id *)drv->device_ids;
> + uk_pr_info("driver %p\n", drv);
>
> - /* only care about base addr, ignore the size */
> - reg_base = fdt32_to_cpu(prop[0]);
> - reg_base = reg_base << 32 | fdt32_to_cpu(prop[1]);
> + ret = pf_driver_probe_device(drv, dev);
> + if (ret < 0) {
> + uk_free(pfh.a, dev);
> + continue;
> }
>
> - dev_base = reg_base;
> - dev_irq = gic_irq_translate(type, hwirq);
> -
> - ret = pf_driver_add_device(drv, &devid, dev_base, dev_irq);
> - }
> + ret = pf_driver_add_device(drv, dev);
> + if (ret < 0) {
> + uk_pr_err("Platform Failed to initialize device driver,
> ret(%d)\n",ret);
> + uk_free(pfh.a, dev);
> + }
> + } while (1);
>
> return ret;
> -
> -error_exit:
> - return -ENODEV;
> }
>
>
> @@ -232,4 +234,4 @@ static struct pf_bus_handler pfh = {
> .b.init = pf_init,
> .b.probe = pf_probe
> };
> -UK_BUS_REGISTER(&pfh.b);
> +UK_BUS_REGISTER_PRIORITY(&pfh.b, 1);
>
> Reviewed-by: Razvan Virtan <virtanrazvan@xxxxxxxxx>
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |