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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * 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?
> +}
> +
> +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...
> -----Original Message-----
> From: Marc Zyngier [mailto:[email protected]]
> Sent: Friday, November 06, 2015 11:51 AM
> To: Rivera Jose-B46482; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: Yoder Stuart-B08248; katz Itai-RM05202; Pan Lijun-B44306; Li Yang-
> Leo-R58472; Wood Scott-B07421; [email protected]; Hamciuc Bogdan-BHAMCIU1;
> Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794;
> Schmitt Richard-B43082; [email protected];
> [email protected]
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> > + *
> > + * 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...
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...
> -----Original Message-----
> From: Marc Zyngier [mailto:[email protected]]
> Sent: Monday, November 09, 2015 4:46 AM
> To: Rivera Jose-B46482; [email protected]; [email protected];
> [email protected]; [email protected]
> Cc: Yoder Stuart-B08248; katz Itai-RM05202; Pan Lijun-B44306; Li Yang-
> Leo-R58472; Wood Scott-B07421; [email protected]; Hamciuc Bogdan-BHAMCIU1;
> Marginean Alexandru-R89243; Sharma Bhupesh-B45370; Erez Nir-RM30794;
> Schmitt Richard-B43082; [email protected];
> [email protected]
> 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...