From: Jose Rivera <German.Rivera@freescale.com>
To: Marc Zyngier <marc.zyngier@arm.com>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "arnd@arndb.de" <arnd@arndb.de>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC: Stuart Yoder <stuart.yoder@freescale.com>,
        Katz Itai <itai.katz@freescale.com>,
        Lijun Pan <Lijun.Pan@freescale.com>, Li Leo <LeoLi@freescale.com>,
        Scott Wood <scottwood@freescale.com>, "agraf@suse.de" <agraf@suse.de>,
        Hamciuc Bogdan <bhamciu1@freescale.com>,
        Marginean Alexandru <R89243@freescale.com>,
        Sharma Bhupesh <bhupesh.sharma@freescale.com>,
        "Erez Nir" <nir.erez@freescale.com>,
        Richard Schmitt <richard.schmitt@freescale.com>,
        "dan.carpenter@oracle.com" <dan.carpenter@oracle.com>,
        "jiang.liu@linux.intel.com" <jiang.liu@linux.intel.com>
Subject: RE: [PATCH v2 03/11] staging: fsl-mc: Added generic MSI support for
 FSL-MC devices
Thread-Topic: [PATCH v2 03/11] staging: fsl-mc: Added generic MSI support for
 FSL-MC devices
Thread-Index: AQHRE01zz38hZKye70Cn4id08aSw9p6PUL6AgABMp6CAA/OjgIAAsIQw
Date: Mon, 9 Nov 2015 21:18:45 +0000
Message-ID: <DM2PR0301MB1309BE69516E98AE9E455CC9FE150@DM2PR0301MB1309.namprd03.prod.outlook.com>
References: <1446234217-25512-1-git-send-email-German.Rivera@freescale.com>
 <1446234217-25512-4-git-send-email-German.Rivera@freescale.com>
 <563CE87B.9040107@arm.com>
 <DM2PR0301MB13097ADC9AC3E511B7FC9352FE280@DM2PR0301MB1309.namprd03.prod.outlook.com>
 <56407967.1030506@arm.com>
In-Reply-To: <56407967.1030506@arm.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Nov 2015 21:18:45.3938
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR0301MB1310
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3707
Lines: 94

> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Monday, November 09, 2015 4:46 AM
> To: Rivera Jose-B46482; gregkh@linuxfoundation.org; arnd@arndb.de;
> devel@driverdev.osuosl.org; linux-kernel@vger.kernel.org
> Cc: Yoder Stuart-B08248; katz Itai-RM05202; Pan Lijun-B44306; Li Yang-
> Leo-R58472; Wood Scott-B07421; agraf@suse.de; Hamciuc Bogdan-BHAMCIU1;
> Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794;
> Schmitt Richard-B43082; dan.carpenter@oracle.com;
> jiang.liu@linux.intel.com
> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/