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

[Marc Zyngier <marc.zyngier@arm.com>]

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?

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...