lkml.org 
[lkml]   [2015]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v2 03/11] staging: fsl-mc: Added generic MSI support for FSL-MC devices
Date
> -----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...


\
 
 \ /
  Last update: 2015-11-09 22:41    [W:0.548 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site