On Fri, 2019-08-09 at 13:18 +0100, Marc Zyngier wrote:
> On 09/08/2019 10:31, Lubomir Rintel wrote:
> > From: Andres Salomon <dilinger@xxxxxxxxxx>
> >
> > On mmp3, there's an extra set of ICU registers (ICU2) that handle
> > interrupts on the extra cores. When masking off interrupts on MP1,
> > these should be masked as well.
> >
> > We add a new interrupt controller via device tree to identify when we're
> > looking at an mmp3 machine via compatible field of "marvell,mmp3-intc".
> >
> > [lkundrak@xxxxx: Changed "mrvl,mmp3-intc" compatible strings to
> > "marvell,mmp3-intc". Tidied up the subject line a bit.]
> >
> > Signed-off-by: Andres Salomon <dilinger@xxxxxxxxxx>
> > Signed-off-by: Lubomir Rintel <lkundrak@xxxxx>
> >
> > ---
> > arch/arm/mach-mmp/regs-icu.h | 3 +++
> > drivers/irqchip/irq-mmp.c | 51 ++++++++++++++++++++++++++++++++++++
> > 2 files changed, 54 insertions(+)
> >
> > diff --git a/arch/arm/mach-mmp/regs-icu.h b/arch/arm/mach-mmp/regs-icu.h
> > index 0375d5a7fcb2b..410743d2b4020 100644
> > --- a/arch/arm/mach-mmp/regs-icu.h
> > +++ b/arch/arm/mach-mmp/regs-icu.h
> > @@ -11,6 +11,9 @@
> > #define ICU_VIRT_BASE (AXI_VIRT_BASE + 0x82000)
> > #define ICU_REG(x) (ICU_VIRT_BASE + (x))
> >
> > +#define ICU2_VIRT_BASE (AXI_VIRT_BASE + 0x84000)
> > +#define ICU2_REG(x) (ICU2_VIRT_BASE + (x))
> > +
> > #define ICU_INT_CONF(n) ICU_REG((n) << 2)
> > #define ICU_INT_CONF_MASK (0xf)
> >
> > diff --git a/drivers/irqchip/irq-mmp.c b/drivers/irqchip/irq-mmp.c
> > index cd8d2253f56d1..25497c75cc861 100644
> > --- a/drivers/irqchip/irq-mmp.c
> > +++ b/drivers/irqchip/irq-mmp.c
> > @@ -44,6 +44,7 @@ struct icu_chip_data {
> > unsigned int conf_enable;
> > unsigned int conf_disable;
> > unsigned int conf_mask;
> > + unsigned int conf2_mask;
> > unsigned int clr_mfp_irq_base;
> > unsigned int clr_mfp_hwirq;
> > struct irq_domain *domain;
> > @@ -53,9 +54,11 @@ struct mmp_intc_conf {
> > unsigned int conf_enable;
> > unsigned int conf_disable;
> > unsigned int conf_mask;
> > + unsigned int conf2_mask;
> > };
> >
> > static void __iomem *mmp_icu_base;
> > +static void __iomem *mmp_icu2_base;
> > static struct icu_chip_data icu_data[MAX_ICU_NR];
> > static int max_icu_nr;
> >
> > @@ -98,6 +101,16 @@ static void icu_mask_irq(struct irq_data *d)
> > r &= ~data->conf_mask;
> > r |= data->conf_disable;
> > writel_relaxed(r, mmp_icu_base + (hwirq << 2));
> > +
> > + if (data->conf2_mask) {
> > + /*
> > + * ICU1 (above) only controls PJ4 MP1; if using SMP,
> > + * we need to also mask the MP2 and MM cores via ICU2.
> > + */
> > + r = readl_relaxed(mmp_icu2_base + (hwirq << 2));
> > + r &= ~data->conf2_mask;
> > + writel_relaxed(r, mmp_icu2_base + (hwirq << 2));
> > + }
> > } else {
> > r = readl_relaxed(data->reg_mask) | (1 << hwirq);
> > writel_relaxed(r, data->reg_mask);
> > @@ -201,6 +214,14 @@ static const struct mmp_intc_conf mmp2_conf = {
> > MMP2_ICU_INT_ROUTE_PJ4_FIQ,
> > };
> >
> > +static struct mmp_intc_conf mmp3_conf = {
> > + .conf_enable = 0x20,
> > + .conf_disable = 0x0,
> > + .conf_mask = MMP2_ICU_INT_ROUTE_PJ4_IRQ |
> > + MMP2_ICU_INT_ROUTE_PJ4_FIQ,
> > + .conf2_mask = 0xf0,
> > +};
> > +
> > static void __exception_irq_entry mmp_handle_irq(struct pt_regs *regs)
> > {
> > int hwirq;
> > @@ -364,6 +385,14 @@ static int __init mmp_init_bases(struct device_node *node)
> > pr_err("Failed to get interrupt controller register\n");
> > return -ENOMEM;
> > }
> > + if (of_device_is_compatible(node, "marvell,mmp3-intc")) {
>
> Instead of harcoding the compatible property once more, why don't you
> simply pass a flag from mmpx_of_init()?
Will do so in next version.
> > + mmp_icu2_base = of_iomap(node, 1);
> > + if (!mmp_icu2_base) {
> > + pr_err("Failed to get interrupt controller register #2\n");
> > + iounmap(mmp_icu_base);
> > + return -ENOMEM;
> > + }
> > + }
> >
> > icu_data[0].virq_base = 0;
> > icu_data[0].domain = irq_domain_add_linear(node, nr_irqs,
> > @@ -386,6 +415,8 @@ static int __init mmp_init_bases(struct device_node *node)
> > irq_dispose_mapping(icu_data[0].virq_base + i);
> > }
> > irq_domain_remove(icu_data[0].domain);
> > + if (of_device_is_compatible(node, "marvell,mmp3-intc"))
> > + iounmap(mmp_icu2_base);
> > iounmap(mmp_icu_base);
> > return -EINVAL;
> > }
> > @@ -428,6 +459,26 @@ static int __init mmp2_of_init(struct device_node *node,
> > }
> > IRQCHIP_DECLARE(mmp2_intc, "mrvl,mmp2-intc", mmp2_of_init);
> >
> > +static int __init mmp3_of_init(struct device_node *node,
> > + struct device_node *parent)
> > +{
> > + int ret;
> > +
> > + ret = mmp_init_bases(node);
> > + if (ret < 0)
> > + return ret;
> > +
> > + icu_data[0].conf_enable = mmp3_conf.conf_enable;
> > + icu_data[0].conf_disable = mmp3_conf.conf_disable;
> > + icu_data[0].conf_mask = mmp3_conf.conf_mask;
> > + icu_data[0].conf2_mask = mmp3_conf.conf2_mask;
> > + irq_set_default_host(icu_data[0].domain);
>
> Why do you need this? On a fully DT-ified platform, there should be no
> notion of a default domain.
I didn't know. Pretty sure this was cargo-culted, because it's done
elsewhere too and also unnecessary. Will remove those cases too.
> > + set_handle_irq(mmp2_handle_irq);
> > + max_icu_nr = 1;
> > + return 0;
> > +}
> > +IRQCHIP_DECLARE(mmp3_intc, "marvell,mmp3-intc", mmp3_of_init);
> > +
> > static int __init mmp2_mux_of_init(struct device_node *node,
> > struct device_node *parent)
> > {
> >
>
> Thanks,
>
> M.
Thanks
Lubo
