RE: [PATCH v2 03/11] staging: fsl-mc: Added generic MSI support for FSL-MC devices

[Jose Rivera]

> -----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...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/