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: AQHRE01zz38hZKye70Cn4id08aSw9p6PUL6AgABMp6A=
Date: Fri, 6 Nov 2015 23:20:44 +0000
Message-ID: <DM2PR0301MB13097ADC9AC3E511B7FC9352FE280@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>
In-Reply-To: <563CE87B.9040107@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: 06 Nov 2015 23:20:44.4716
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR0301MB0656
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 8067
Lines: 227

> -----Original Message-----
> From: Marc Zyngier [mailto:marc.zyngier@arm.com]
> Sent: Friday, November 06, 2015 11:51 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 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@freescale.com>
> > ---
> > 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@freescale.com>
> > + *
> > + * 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...
--
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/