Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966224AbbKFRu5 (ORCPT ); Fri, 6 Nov 2015 12:50:57 -0500 Received: from foss.arm.com ([217.140.101.70]:43274 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932978AbbKFRu4 (ORCPT ); Fri, 6 Nov 2015 12:50:56 -0500 Message-ID: <563CE87B.9040107@arm.com> Date: Fri, 06 Nov 2015 17:50:51 +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: "J. German Rivera" , gregkh@linuxfoundation.org, arnd@arndb.de, devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org CC: stuart.yoder@freescale.com, itai.katz@freescale.com, lijun.pan@freescale.com, leoli@freescale.com, scottwood@freescale.com, agraf@suse.de, bhamciu1@freescale.com, R89243@freescale.com, bhupesh.sharma@freescale.com, nir.erez@freescale.com, richard.schmitt@freescale.com, 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> In-Reply-To: <1446234217-25512-4-git-send-email-German.Rivera@freescale.com> 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: 6584 Lines: 205 On 30/10/15 19:43, J. German Rivera wrote: > Created an MSI domain for the fsl-mc bus-- including functions > to create a domain, find a domain, alloc/free domain irqs, and > bus specific overrides for domain and irq_chip ops. > > Signed-off-by: J. German Rivera > --- > Changes in v2: none > > drivers/staging/fsl-mc/bus/Kconfig | 1 + > drivers/staging/fsl-mc/bus/Makefile | 1 + > drivers/staging/fsl-mc/bus/mc-msi.c | 276 ++++++++++++++++++++++++++++ > drivers/staging/fsl-mc/include/mc-private.h | 17 ++ > drivers/staging/fsl-mc/include/mc.h | 17 ++ > 5 files changed, 312 insertions(+) > create mode 100644 drivers/staging/fsl-mc/bus/mc-msi.c > > diff --git a/drivers/staging/fsl-mc/bus/Kconfig b/drivers/staging/fsl-mc/bus/Kconfig > index 0d779d9..c498ac6 100644 > --- a/drivers/staging/fsl-mc/bus/Kconfig > +++ b/drivers/staging/fsl-mc/bus/Kconfig > @@ -9,6 +9,7 @@ > config FSL_MC_BUS > tristate "Freescale Management Complex (MC) bus driver" > depends on OF && ARM64 > + select GENERIC_MSI_IRQ_DOMAIN > help > Driver to enable the bus infrastructure for the Freescale > QorIQ Management Complex (fsl-mc). The fsl-mc is a hardware > diff --git a/drivers/staging/fsl-mc/bus/Makefile b/drivers/staging/fsl-mc/bus/Makefile > index 25433a9..a5f2ba4 100644 > --- a/drivers/staging/fsl-mc/bus/Makefile > +++ b/drivers/staging/fsl-mc/bus/Makefile > @@ -13,5 +13,6 @@ mc-bus-driver-objs := mc-bus.o \ > dpmng.o \ > dprc-driver.o \ > mc-allocator.o \ > + mc-msi.o \ > dpmcp.o \ > dpbp.o > diff --git a/drivers/staging/fsl-mc/bus/mc-msi.c b/drivers/staging/fsl-mc/bus/mc-msi.c > new file mode 100644 > index 0000000..81b53e3 > --- /dev/null > +++ b/drivers/staging/fsl-mc/bus/mc-msi.c > @@ -0,0 +1,276 @@ > +/* > + * Freescale Management Complex (MC) bus driver MSI support > + * > + * Copyright (C) 2015 Freescale Semiconductor, Inc. > + * Author: German Rivera > + * > + * This file is licensed under the terms of the GNU General Public > + * License version 2. This program is licensed "as is" without any > + * warranty of any kind, whether express or implied. > + */ > + > +#include "../include/mc-private.h" > +#include > +#include > +#include > +#include > +#include > +#include > +#include "../include/mc-sys.h" > +#include "dprc-cmd.h" > + > +static void fsl_mc_msi_set_desc(msi_alloc_info_t *arg, > + struct msi_desc *desc) > +{ > + arg->desc = desc; > + arg->hwirq = (irq_hw_number_t)desc->fsl_mc.msi_index; > +} > + > +static void fsl_mc_msi_update_dom_ops(struct msi_domain_info *info) > +{ > + struct msi_domain_ops *ops = info->ops; > + > + if (WARN_ON(!ops)) > + return; > + > + if (!ops->set_desc) > + ops->set_desc = fsl_mc_msi_set_desc; When would that be overridden by the MSI controller? > +} > + > +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; > + > + /* > + * msi_desc->msg.address is 0x0 when this function is invoked in > + * the free_irq() code path. In this case, for the MC, we don't > + * really need to "unprogram" the MSI, so we just return. > + */ > + if (msi_desc->msg.address_lo == 0x0 && msi_desc->msg.address_hi == 0x0) > + return; > + > + if (WARN_ON(!owner_mc_dev)) > + return; > + > + irq_cfg.paddr = ((u64)msi_desc->msg.address_hi << 32) | > + msi_desc->msg.address_lo; This should really be a phys_addr_t. > + irq_cfg.val = msi_desc->msg.data; > + irq_cfg.user_irq_id = msi_desc->irq; > + > + /* > + * 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...)? > +} > + > +/* > + * NOTE: This function is invoked with interrupts disabled > + */ > +static void fsl_mc_msi_write_msg(struct irq_data *irq_data, > + struct msi_msg *msg) > +{ > + struct msi_desc *msi_desc = irq_data_get_msi_desc(irq_data); > + struct fsl_mc_device *mc_bus_dev = to_fsl_mc_device(msi_desc->dev); > + struct fsl_mc_bus *mc_bus = to_fsl_mc_bus(mc_bus_dev); > + struct fsl_mc_device_irq *mc_dev_irq = > + &mc_bus->irq_resources[msi_desc->fsl_mc.msi_index]; > + > + WARN_ON(mc_dev_irq->msi_desc != msi_desc); > + msi_desc->msg = *msg; I wonder why we don't do that in core code... > + > + /* > + * Program the MSI (paddr, value) pair in the device: > + */ > + __fsl_mc_msi_write_msg(mc_bus_dev, mc_dev_irq); > +} > + > +static void fsl_mc_msi_update_chip_ops(struct msi_domain_info *info) > +{ > + struct irq_chip *chip = info->chip; > + > + if (WARN_ON((!chip))) > + return; > + > + if (!chip->irq_write_msi_msg) > + chip->irq_write_msi_msg = fsl_mc_msi_write_msg; Same as above. Do you foresee any circumstances where you you wouldn't use this function? My hunch is that you should warn if chip->irq_write_msi_msg is provided by the caller. > +} [...] 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/