> -----Original Message----- > From: Marc Zyngier [mailto:marc.zyngier@xxxxxxx] > Sent: Friday, November 06, 2015 11:51 AM > To: Rivera Jose-B46482; gregkh@xxxxxxxxxxxxxxxxxxx; arnd@xxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Cc: Yoder Stuart-B08248; katz Itai-RM05202; Pan Lijun-B44306; Li Yang- > Leo-R58472; Wood Scott-B07421; agraf@xxxxxxx; Hamciuc Bogdan-BHAMCIU1; > Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794; > Schmitt Richard-B43082; dan.carpenter@xxxxxxxxxx; > jiang.liu@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v2 03/11] staging: fsl-mc: Added generic MSI support > for FSL-MC devices > > 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 <German.Rivera@xxxxxxxxxxxxx> > > --- > > 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 <German.Rivera@xxxxxxxxxxxxx> > > + * > > + * 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 <linux/of_device.h> > > +#include <linux/of_address.h> > > +#include <linux/irqchip/arm-gic-v3.h> #include <linux/irq.h> #include > > +<linux/irqdomain.h> #include <linux/msi.h> #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? > It should not be set by the MSI controller. As you suggested in another, I will add A WARN_ON if set_desc is not NULL. > > +} > > + > > +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. > I'll change the type of paddr in the dprc_irq_cfg struct to be phys_addr_t instead of u64. > > + 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...)? > 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. > > +} > > + > > +/* > > + * 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. > I agree with you. The caller should not set the irq_write_msi_msg function. I'll add a WARN_ON to catch that. > > +} > > [...] > > Thanks, > > M. > -- > Jazz is not dead. It just smells funny... _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
