RE: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi controller

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----Original Message-----
> From: Marc Zyngier <maz@xxxxxxxxxx>
> Sent: Thursday, July 21, 2022 10:28 AM
> To: Frank Li <frank.li@xxxxxxx>
> Cc: jdmason@xxxxxxxx; tglx@xxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kw@xxxxxxxxx; bhelgaas@xxxxxxxxxx;
> kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Peng Fan
> <peng.fan@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>;
> kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx <linux-
> imx@xxxxxxx>; kishon@xxxxxx; lorenzo.pieralisi@xxxxxxx;
> ntb@xxxxxxxxxxxxxxx
> Subject: Re: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> controller
> 
> Caution: EXT Email
> 
> On Thu, 21 Jul 2022 16:22:08 +0100,
> Frank Li <frank.li@xxxxxxx> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Marc Zyngier <maz@xxxxxxxxxx>
> > > Sent: Thursday, July 21, 2022 2:57 AM
> > > To: Frank Li <frank.li@xxxxxxx>
> > > Cc: jdmason@xxxxxxxx; tglx@xxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> > > krzysztof.kozlowski+dt@xxxxxxxxxx; shawnguo@xxxxxxxxxx;
> > > s.hauer@xxxxxxxxxxxxxx; kw@xxxxxxxxx; bhelgaas@xxxxxxxxxx;
> > > kernel@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-arm-
> > > kernel@xxxxxxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; Peng Fan
> > > <peng.fan@xxxxxxx>; Aisheng Dong <aisheng.dong@xxxxxxx>;
> > > kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; dl-linux-imx <linux-
> > > imx@xxxxxxx>; kishon@xxxxxx; lorenzo.pieralisi@xxxxxxx;
> > > ntb@xxxxxxxxxxxxxxx
> > > Subject: [EXT] Re: [PATCH v3 2/4] irqchip: imx mu worked as msi
> controller
> > >
> > > Caution: EXT Email
> > >
> > > On Wed, 20 Jul 2022 22:30:34 +0100,
> > > Frank Li <Frank.Li@xxxxxxx> wrote:
> > > >
> > > > MU support generate irq by write data to a register.
> > > > This patch make mu worked as msi controller.
> > > > So MU can do doorbell by using standard msi api.
> > > >
> > > > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > > > ---
> > > >  drivers/irqchip/Kconfig          |   7 +
> > > >  drivers/irqchip/Makefile         |   1 +
> > > >  drivers/irqchip/irq-imx-mu-msi.c | 462
> > > +++++++++++++++++++++++++++++++
> > > >  3 files changed, 470 insertions(+)
> > > >  create mode 100644 drivers/irqchip/irq-imx-mu-msi.c
> > > >
> > > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > > index 5e4e50122777d..4599471d880c0 100644
> > > > --- a/drivers/irqchip/Kconfig
> > > > +++ b/drivers/irqchip/Kconfig
> > > > @@ -470,6 +470,13 @@ config IMX_INTMUX
> > > >       help
> > > >         Support for the i.MX INTMUX interrupt multiplexer.
> > > >
> > > > +config IMX_MU_MSI
> > > > +     bool "i.MX MU work as MSI controller"
> > > > +     default y if ARCH_MXC
> > > > +     select IRQ_DOMAIN
> > > > +     help
> > > > +       MU work as MSI controller to do general doorbell
> > > > +
> > > >  config LS1X_IRQ
> > > >       bool "Loongson-1 Interrupt Controller"
> > > >       depends on MACH_LOONGSON32
> > > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > > > index 5d8e21d3dc6d8..870423746c783 100644
> > > > --- a/drivers/irqchip/Makefile
> > > > +++ b/drivers/irqchip/Makefile
> > > > @@ -98,6 +98,7 @@ obj-$(CONFIG_RISCV_INTC)            += irq-riscv-
> intc.o
> > > >  obj-$(CONFIG_SIFIVE_PLIC)            += irq-sifive-plic.o
> > > >  obj-$(CONFIG_IMX_IRQSTEER)           += irq-imx-irqsteer.o
> > > >  obj-$(CONFIG_IMX_INTMUX)             += irq-imx-intmux.o
> > > > +obj-$(CONFIG_IMX_MU_MSI)             += irq-imx-mu-msi.o
> > > >  obj-$(CONFIG_MADERA_IRQ)             += irq-madera.o
> > > >  obj-$(CONFIG_LS1X_IRQ)                       += irq-ls1x.o
> > > >  obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)    += irq-ti-sci-intr.o
> > > > diff --git a/drivers/irqchip/irq-imx-mu-msi.c b/drivers/irqchip/irq-imx-
> mu-
> > > msi.c
> > > > new file mode 100644
> > > > index 0000000000000..8277dba857759
> > > > --- /dev/null
> > > > +++ b/drivers/irqchip/irq-imx-mu-msi.c
> > > > @@ -0,0 +1,462 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/*
> > > > + * NXP MU worked as MSI controller
> > > > + *
> > > > + * Copyright (c) 2018 Pengutronix, Oleksij Rempel
> > > <o.rempel@xxxxxxxxxxxxxx>
> > > > + * Copyright 2022 NXP
> > > > + *   Frank Li <Frank.Li@xxxxxxx>
> > > > + *   Peng Fan <peng.fan@xxxxxxx>
> > > > + *
> > > > + * Based on drivers/mailbox/imx-mailbox.c
> > > > + */
> > > > +#include <linux/clk.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/msi.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/irq.h>
> > > > +#include <linux/irqchip/chained_irq.h>
> > > > +#include <linux/irqchip.h>
> > > > +#include <linux/irqdomain.h>
> > > > +#include <linux/of_irq.h>
> > > > +#include <linux/of_pci.h>
> > > > +#include <linux/of_platform.h>
> > > > +#include <linux/spinlock.h>
> > > > +#include <linux/dma-iommu.h>
> > > > +#include <linux/pm_runtime.h>
> > > > +#include <linux/pm_domain.h>
> > > > +
> > > > +
> > > > +#define IMX_MU_CHANS            4
> > > > +
> > > > +enum imx_mu_xcr {
> > > > +     IMX_MU_GIER,
> > > > +     IMX_MU_GCR,
> > > > +     IMX_MU_TCR,
> > > > +     IMX_MU_RCR,
> > > > +     IMX_MU_xCR_MAX,
> > > > +};
> > > > +
> > > > +enum imx_mu_xsr {
> > > > +     IMX_MU_SR,
> > > > +     IMX_MU_GSR,
> > > > +     IMX_MU_TSR,
> > > > +     IMX_MU_RSR,
> > > > +};
> > > > +
> > > > +enum imx_mu_type {
> > > > +     IMX_MU_V1 = BIT(0),
> > > > +     IMX_MU_V2 = BIT(1),
> > > > +     IMX_MU_V2_S4 = BIT(15),
> > > > +};
> > > > +
> > > > +/* Receive Interrupt Enable */
> > > > +#define IMX_MU_xCR_RIEn(type, x) (type & IMX_MU_V2 ? BIT(x) :
> BIT(24
> > > + (3 - (x))))
> > > > +#define IMX_MU_xSR_RFn(type, x) (type & IMX_MU_V2 ? BIT(x) :
> BIT(24 +
> > > (3 - (x))))
> > > > +
> > > > +struct imx_mu_dcfg {
> > > > +     enum imx_mu_type type;
> > > > +     u32     xTR;            /* Transmit Register0 */
> > > > +     u32     xRR;            /* Receive Register0 */
> > > > +     u32     xSR[4];         /* Status Registers */
> > > > +     u32     xCR[4];         /* Control Registers */
> > > > +};
> > > > +
> > > > +struct imx_mu_msi {
> > > > +     spinlock_t                      lock;
> > > > +     struct platform_device          *pdev;
> > > > +     struct irq_domain               *parent;
> > > > +     struct irq_domain               *msi_domain;
> > > > +     void __iomem                    *regs;
> > > > +     phys_addr_t                     msiir_addr;
> > > > +     const struct imx_mu_dcfg        *cfg;
> > > > +     unsigned long                   used;
> > > > +     u32                             gic_irq;
> > > > +     struct clk                      *clk;
> > > > +     struct device                   *pd_a;
> > > > +     struct device                   *pd_b;
> > > > +     struct device_link              *pd_link_a;
> > > > +     struct device_link              *pd_link_b;
> > > > +};
> > > > +
> > > > +static void imx_mu_write(struct imx_mu_msi *msi_data, u32 val, u32
> offs)
> > > > +{
> > > > +     iowrite32(val, msi_data->regs + offs);
> > > > +}
> > > > +
> > > > +static u32 imx_mu_read(struct imx_mu_msi *msi_data, u32 offs)
> > > > +{
> > > > +     return ioread32(msi_data->regs + offs);
> > > > +}
> > > > +
> > > > +static u32 imx_mu_xcr_rmw(struct imx_mu_msi *msi_data, enum
> > > imx_mu_xcr type, u32 set, u32 clr)
> > > > +{
> > > > +     unsigned long flags;
> > > > +     u32 val;
> > > > +
> > > > +     spin_lock_irqsave(&msi_data->lock, flags);
> > > > +     val = imx_mu_read(msi_data, msi_data->cfg->xCR[type]);
> > > > +     val &= ~clr;
> > > > +     val |= set;
> > > > +     imx_mu_write(msi_data, val, msi_data->cfg->xCR[type]);
> > > > +     spin_unlock_irqrestore(&msi_data->lock, flags);
> > > > +
> > > > +     return val;
> > > > +}
> > > > +
> > > > +static void imx_mu_msi_mask_irq(struct irq_data *data)
> > > > +{
> > > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> > > >parent_data);
> > >
> > > Urgh... No. Please don't go poking into the *parent* stuff. Implement
> > > the masking at the parent level, and use irq_chip_mask_parent() for
> > > this level, unless you can explain why you can't do otherwise.
> > >
> > > > +
> > > > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR, 0,
> > > IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq));
> > >
> > > How about making this readable and move the dereference inside the
> macro?
> > >
> > > > +}
> > > > +
> > > > +static void imx_mu_msi_unmask_irq(struct irq_data *data)
> > > > +{
> > > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data-
> > > >parent_data);
> > > > +
> > > > +     imx_mu_xcr_rmw(msi_data, IMX_MU_RCR,
> > > IMX_MU_xCR_RIEn(msi_data->cfg->type, data->hwirq), 0);
> > > > +}
> > > > +
> > > > +static struct irq_chip imx_mu_msi_irq_chip = {
> > > > +     .name = "MU-MSI",
> > > > +     .irq_mask       = imx_mu_msi_mask_irq,
> > > > +     .irq_unmask     = imx_mu_msi_unmask_irq,
> > > > +};
> > > > +
> > > > +static struct msi_domain_ops its_pmsi_ops = {
> > > > +};
> > >
> > > What does this have to do with the ITS?
> >
> > Without this, it will be crashed as access 0 address.
> 
> Because the *name* of the structure has an influence on the behaviour
> of the kernel?????
> 
> >
> > >
> > > > +
> > > > +static struct msi_domain_info imx_mu_msi_domain_info = {
> > > > +     .flags  = (MSI_FLAG_USE_DEF_DOM_OPS |
> > > > +                MSI_FLAG_USE_DEF_CHIP_OPS |
> > > > +                MSI_FLAG_PCI_MSIX),
> > >
> > > What does PCI_MSIX mean in this context? I really wish you used
> > > copy/paste a bit more carefully.
> > >
> > > > +     .ops    = &its_pmsi_ops,
> > > > +     .chip   = &imx_mu_msi_irq_chip,
> > > > +};
> > > > +
> > > > +static void imx_mu_msi_compose_msg(struct irq_data *data, struct
> > > msi_msg *msg)
> > > > +{
> > > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(data);
> > > > +
> > > > +     msg->address_hi = upper_32_bits(msi_data->msiir_addr);
> > > > +     msg->address_lo = lower_32_bits(msi_data->msiir_addr + 4 * data-
> > > >hwirq);
> > >
> > > This is a horrible way of writing this. you should compute the address
> > > first, and then apply the address split.
> > >
> > > > +     msg->data = data->hwirq;
> > > > +
> > > > +     iommu_dma_compose_msi_msg(irq_data_get_msi_desc(data),
> msg);
> > > > +}
> > > > +
> > > > +static int imx_mu_msi_set_affinity(struct irq_data *irq_data,
> > > > +                                const struct cpumask *mask, bool force)
> > > > +
> > > > +{
> > > > +     return IRQ_SET_MASK_OK;
> > > > +}
> > >
> > > Err... What effect does this have if you don't do anything?
> >
> > [Frank Li] Without this, it will be crashed as access 0 address.
> 
> Then you should fix that bug, because what you have here makes
> absolutely no sense.
> 
> >
> > >
> > > > +
> > > > +static struct irq_chip imx_mu_msi_parent_chip = {
> > > > +     .name                   = "MU",
> > > > +     .irq_compose_msi_msg    = imx_mu_msi_compose_msg,
> > > > +     .irq_set_affinity = imx_mu_msi_set_affinity,
> > > > +};
> > > > +
> > > > +static int imx_mu_msi_domain_irq_alloc(struct irq_domain *domain,
> > > > +                                     unsigned int virq,
> > > > +                                     unsigned int nr_irqs,
> > > > +                                     void *args)
> > > > +{
> > > > +     struct imx_mu_msi *msi_data = domain->host_data;
> > > > +     msi_alloc_info_t *info = args;
> > > > +     int pos, err = 0;
> > > > +
> > > > +     WARN_ON(nr_irqs != 1);
> > > > +
> > > > +     spin_lock(&msi_data->lock);
> > >
> > > Interrupt fires after lock acquisition, handler masks the interrupt.
> > > Deadlock.
> >
> > [Frank Li] you are right, it should be spin_lock_irqsave.
> >
> > >
> > > > +     pos = find_first_zero_bit(&msi_data->used, IMX_MU_CHANS);
> > > > +     if (pos < IMX_MU_CHANS)
> > > > +             __set_bit(pos, &msi_data->used);
> > > > +     else
> > > > +             err = -ENOSPC;
> > > > +     spin_unlock(&msi_data->lock);
> > > > +
> > > > +     if (err)
> > > > +             return err;
> > > > +
> > > > +     err = iommu_dma_prepare_msi(info->desc, msi_data->msiir_addr
> +
> > > pos * 4);
> > >
> > > Does this HW even have an IOMMU?
> >
> > [Frank Li] we have a platform with iommu.
> >
> > >
> > > > +     if (err)
> > > > +             return err;
> > > > +
> > > > +     irq_domain_set_info(domain, virq, pos,
> > > > +                         &imx_mu_msi_parent_chip, msi_data,
> > > > +                         handle_simple_irq, NULL, NULL);
> > >
> > > This is an edge interrupt. Please handle it like one.
> >
> > [Frank Li]  Not sure what your means?
> 
> A MSI is an edge interrupt. Use handle_edge_irq.
> 
> >
> > >
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static void imx_mu_msi_domain_irq_free(struct irq_domain *domain,
> > > > +                                    unsigned int virq, unsigned int nr_irqs)
> > > > +{
> > > > +     struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> > > > +     struct imx_mu_msi *msi_data = irq_data_get_irq_chip_data(d);
> > > > +
> > > > +     spin_lock(&msi_data->lock);
> > >
> > > Same problem.
> > >
> > > > +     __clear_bit(d->hwirq, &msi_data->used);
> > > > +     spin_unlock(&msi_data->lock);
> > > > +}
> > > > +
> > > > +static const struct irq_domain_ops imx_mu_msi_domain_ops = {
> > > > +     .alloc  = imx_mu_msi_domain_irq_alloc,
> > > > +     .free   = imx_mu_msi_domain_irq_free,
> > > > +};
> > > > +
> > > > +static void imx_mu_msi_irq_handler(struct irq_desc *desc)
> > > > +{
> > > > +     struct imx_mu_msi *msi_data = irq_desc_get_handler_data(desc);
> > > > +     u32 status;
> > > > +     int i;
> > > > +
> > > > +     status = imx_mu_read(msi_data, msi_data->cfg-
> >xSR[IMX_MU_RSR]);
> > > > +
> > > > +     chained_irq_enter(irq_desc_get_chip(desc), desc);
> > > > +     for (i = 0; i < IMX_MU_CHANS; i++) {
> > > > +             if (status & IMX_MU_xSR_RFn(msi_data->cfg->type, i)) {
> > > > +                     imx_mu_read(msi_data, msi_data->cfg->xRR + i * 4);
> > > > +                     generic_handle_domain_irq(msi_data->parent, i);
> > >
> > > Why the parent? You must start at the top of the hierarchy.

[Frank Li] Do you means that should be msi_data->msi_domain instead of msi_data->parent? 

> > >
> > > > +             }
> > > > +     }
> > > > +     chained_irq_exit(irq_desc_get_chip(desc), desc);
> > >
> > > If your MSIs are a chained interrupt, why do you even provide an
> > > affinity setting callback?
> >
> > [Frank Li]  it will be crash if no affinity setting callback.	
> 
> Then you have to fix your driver.

[Frank Li] After debug,  msi_domain_set_affinity() have not did null check for (parent->chip->irq_set_affinity). 
I think impact by using dummy set_affinity is minimized.  

int msi_domain_set_affinity(struct irq_data *irq_data,	
			    const struct cpumask *mask, bool force)
{
	struct irq_data *parent = irq_data->parent_data;
	struct msi_msg msg[2] = { [1] = { }, };
	int ret;

	ret = parent->chip->irq_set_affinity(parent, mask, force);
	if (ret >= 0 && ret != IRQ_SET_MASK_OK_DONE) {
		BUG_ON(irq_chip_compose_msi_msg(irq_data, msg));
		msi_check_level(irq_data->domain, msg);
		irq_chip_write_msi_msg(irq_data, msg);
	}

	return ret;
}
 

> 
>         M.
> 
> --
> Without deviation from the norm, progress is not possible.




[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux