Hi Vikas, On 1/29/21 6:24 PM, Vikas Gupta wrote: > Add msi support for Broadcom FlexRm device. > > Signed-off-by: Vikas Gupta <vikas.gupta@xxxxxxxxxxxx> > --- > .../platform/reset/vfio_platform_bcmflexrm.c | 72 ++++++++++++++++++- > 1 file changed, 70 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c b/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c > index 96064ef8f629..6ca4ca12575b 100644 > --- a/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c > +++ b/drivers/vfio/platform/reset/vfio_platform_bcmflexrm.c > @@ -21,7 +21,9 @@ > #include <linux/init.h> > #include <linux/io.h> > #include <linux/kernel.h> > +#include <linux/msi.h> > #include <linux/module.h> > +#include <linux/vfio.h> > > #include "../vfio_platform_private.h" > > @@ -33,6 +35,9 @@ > #define RING_VER 0x000 > #define RING_CONTROL 0x034 > #define RING_FLUSH_DONE 0x038 > +#define RING_MSI_ADDR_LS 0x03c > +#define RING_MSI_ADDR_MS 0x040 > +#define RING_MSI_DATA_VALUE 0x064 > > /* Register RING_CONTROL fields */ > #define CONTROL_FLUSH_SHIFT 5 > @@ -105,8 +110,71 @@ static int vfio_platform_bcmflexrm_reset(struct vfio_platform_device *vdev) > return ret; > } > > -module_vfio_reset_handler("brcm,iproc-flexrm-mbox", > - vfio_platform_bcmflexrm_reset); > +static u32 bcm_num_msi(struct vfio_platform_device *vdev) Please use the same prefix as for reset, ie. vfio_platform_bcmflexrm_get_msi > +{ > + struct vfio_platform_region *reg = &vdev->regions[0];> + > + return (reg->size / RING_REGS_SIZE); > +} > + > +static void bcm_write_msi(struct vfio_platform_device *vdev, vfio_platform_bcmflexrm_msi_write? > + struct msi_desc *desc, > + struct msi_msg *msg) > +{ > + int i; > + int hwirq = -1; > + int msi_src; > + void __iomem *ring; > + struct vfio_platform_region *reg = &vdev->regions[0]; > + > + if (!reg) > + return; why do you need this check? For this to be called, SET_IRQ IOCTL must have been called. This can only happen after vfio_platform_open which first calls vfio_platform_regions_init and then vfio_platform_irq_init > + > + for (i = 0; i < vdev->num_irqs; i++) > + if (vdev->irqs[i].type == VFIO_IRQ_TYPE_MSI) > + hwirq = vdev->irqs[i].ctx[0].hwirq; nit: It would have been easier to record in vdev the last index of wired interrupts and just add the index of the MSI > + > + if (hwirq < 0) > + return; > + > + msi_src = desc->irq - hwirq; > + > + if (!reg->ioaddr) { > + reg->ioaddr = ioremap(reg->addr, reg->size); > + if (!reg->ioaddr) pr_warn_once("")? > + return; > + } > + > + ring = reg->ioaddr + msi_src * RING_REGS_SIZE; > + > + writel_relaxed(msg->address_lo, ring + RING_MSI_ADDR_LS); > + writel_relaxed(msg->address_hi, ring + RING_MSI_ADDR_MS); > + writel_relaxed(msg->data, ring + RING_MSI_DATA_VALUE); > +} > + > +static struct vfio_platform_reset_node vfio_platform_bcmflexrm_reset_node = { > + .owner = THIS_MODULE, > + .compat = "brcm,iproc-flexrm-mbox", > + .of_reset = vfio_platform_bcmflexrm_reset, > + .of_get_msi = bcm_num_msi, > + .of_msi_write = bcm_write_msi > +}; > + > +static int __init vfio_platform_bcmflexrm_reset_module_init(void) > +{ > + __vfio_platform_register_reset(&vfio_platform_bcmflexrm_reset_node); > + > + return 0; > +} > + > +static void __exit vfio_platform_bcmflexrm_reset_module_exit(void) > +{ > + vfio_platform_unregister_reset("brcm,iproc-flexrm-mbox", > + vfio_platform_bcmflexrm_reset); > +} > + > +module_init(vfio_platform_bcmflexrm_reset_module_init); > +module_exit(vfio_platform_bcmflexrm_reset_module_exit); > > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Anup Patel <anup.patel@xxxxxxxxxxxx>"); > I think you should move the whole series in PATCH now. Thanks Eric