> -----Original Message----- > From: Marc Zyngier [mailto:marc.zyngier@xxxxxxx] > Sent: Monday, November 09, 2015 4:46 AM > To: Rivera Jose-B46482; gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Cc: Yoder Stuart-B08248; katz Itai-RM05202; Pan Lijun-B44306; Li Yang- > Leo-R58472; Wood Scott-B07421; agraf@xxxxxxx; Hamciuc Bogdan-BHAMCIU1; > Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794; > Schmitt Richard-B43082; dan.carpenter@xxxxxxxxxx; > jiang.liu@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v2 03/11] staging: fsl-mc: Added generic MSI support > for FSL-MC devices > > On 06/11/15 23:20, Jose Rivera wrote: > > [...] > > >>> +static void __fsl_mc_msi_write_msg(struct fsl_mc_device *mc_bus_dev, > >>> + struct fsl_mc_device_irq *mc_dev_irq) { > >>> + int error; > >>> + struct fsl_mc_device *owner_mc_dev = mc_dev_irq->mc_dev; > >>> + struct msi_desc *msi_desc = mc_dev_irq->msi_desc; > >>> + struct dprc_irq_cfg irq_cfg; > > [...] > > >>> + /* > >>> + * DPRCs and other objects use different commands to set up IRQs, > >>> + * so we have to differentiate them here. > >>> + */ > >>> + if (owner_mc_dev == mc_bus_dev) { > >>> + /* > >>> + * IRQ is for the mc_bus_dev's DPRC itself > >>> + */ > >>> + error = dprc_set_irq(mc_bus_dev->mc_io, > >>> + MC_CMD_FLAG_INTR_DIS | MC_CMD_FLAG_PRI, > >>> + mc_bus_dev->mc_handle, > >>> + mc_dev_irq->dev_irq_index, > >>> + &irq_cfg); > >>> + if (error < 0) { > >>> + dev_err(&owner_mc_dev->dev, > >>> + "dprc_set_irq() failed: %d\n", error); > >>> + } > >>> + } else { > >>> + error = dprc_set_obj_irq(mc_bus_dev->mc_io, > >>> + MC_CMD_FLAG_INTR_DIS | MC_CMD_FLAG_PRI, > >>> + mc_bus_dev->mc_handle, > >>> + owner_mc_dev->obj_desc.type, > >>> + owner_mc_dev->obj_desc.id, > >>> + mc_dev_irq->dev_irq_index, > >>> + &irq_cfg); > >>> + if (error < 0) { > >>> + dev_err(&owner_mc_dev->dev, > >>> + "dprc_obj_set_irq() failed: %d\n", error); > >>> + } > >>> + } > >> > >> It feels a bit odd that you have all of this under a single MSI > >> umbrella, and yet have to differentiate between them. Do they have a > >> different programming interface? Or is that because these > >> dprc_set_*_irq functions do some other stuff behind the scene (I'm too > lazy to check...)? > >> > > Due to MC firmware API limitations, dprc_set_obj_irq can only be used > > to set IRQs for the DPRC's children not for the DPRC itself. > > Right. So this makes me wonder whether or not you have the right approach > here. The logic behind the bus type was that devices with a common > programming interface would share a bus type (the odd duck being platform > which is used to implement anything else). > > Your description seems to suggest that only devices behind the DPRC share > that programming interface, and that the DPRC itself is the local weirdo. > Should it be using the platform-msi subsystem instead? Or is it just a > matter of firmware oddity? > It's really a MC API oddity-- both the DPRC and the devices behind it share the same MSI context (same ITT in the ITS) but they just require different APIs to set the MSI addr/data. > This is probably not a big deal, but it is worth keeping it in mind, > specially if that has visible consequences (in your device tree, for > example). > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
