Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752979AbbKIKqP (ORCPT ); Mon, 9 Nov 2015 05:46:15 -0500 Received: from foss.arm.com ([217.140.101.70]:49935 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752561AbbKIKqF (ORCPT ); Mon, 9 Nov 2015 05:46:05 -0500 Message-ID: <56407967.1030506@arm.com> Date: Mon, 09 Nov 2015 10:45:59 +0000 From: Marc Zyngier Organization: ARM Ltd User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 MIME-Version: 1.0 To: Jose Rivera , "gregkh@linuxfoundation.org" , "arnd@arndb.de" , "devel@driverdev.osuosl.org" , "linux-kernel@vger.kernel.org" CC: Stuart Yoder , Katz Itai , Lijun Pan , Li Leo , Scott Wood , "agraf@suse.de" , Hamciuc Bogdan , Marginean Alexandru , Sharma Bhupesh , Erez Nir , Richard Schmitt , "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 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> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2735 Lines: 76 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... -- 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/