From: Marc Zyngier <marc.zyngier@arm.com>
To: Matt Redfearn <matt.redfearn@imgtec.com>,
Ralf Baechle <ralf@linux-mips.org>,
Thomas Gleixner <tglx@linutronix.de>
Cc: linux-mips@linux-mips.org, lisa.parratt@imgtec.com,
linux-kernel@vger.kernel.org, Jason Cooper <jason@lakedaemon.net>
Subject: Re: [PATCH 1/6] irqchip: mips-gic: Add context saving for MIPS_REMOTEPROC
Date: Fri, 2 Sep 2016 11:54:04 +0100 [thread overview]
Message-ID: <57C95A4C.10703@arm.com> (raw)
In-Reply-To: <1472810395-21381-2-git-send-email-matt.redfearn@imgtec.com>
Hi Matt,
On 02/09/16 10:59, Matt Redfearn wrote:
> The MIPS remote processor driver allows non-Linux firmware to take
> control of and execute on one of the systems VPEs. If that VPE is
> brought back under Linux, it is necessary to ensure that all GIC
> interrupts are routed and masked as Linux expects them, as the firmware
> can have done anything it likes with the GIC configuration (hopefully
> just for that VPEs local interrupt sources, but allow for shared
> external interrupts as well).
>
> The configuration of shared and local CPU interrupts is maintained and
> updated every time a change is made. When a CPU is brought online, the
> saved configuration is restored.
>
> These functions will also be useful for restoring GIC context after a
> suspend to RAM.
>
> Signed-off-by: Matt Redfearn <matt.redfearn@imgtec.com>
> ---
>
> drivers/irqchip/irq-mips-gic.c | 185 +++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 178 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/irqchip/irq-mips-gic.c b/drivers/irqchip/irq-mips-gic.c
> index 83f498393a7f..5ba1fe1468ce 100644
> --- a/drivers/irqchip/irq-mips-gic.c
> +++ b/drivers/irqchip/irq-mips-gic.c
> @@ -8,6 +8,7 @@
> */
> #include <linux/bitmap.h>
> #include <linux/clocksource.h>
> +#include <linux/cpu.h>
> #include <linux/init.h>
> #include <linux/interrupt.h>
> #include <linux/irq.h>
> @@ -56,6 +57,47 @@ static unsigned int timer_cpu_pin;
> static struct irq_chip gic_level_irq_controller, gic_edge_irq_controller;
> DECLARE_BITMAP(ipi_resrv, GIC_MAX_INTRS);
>
> +#if defined(CONFIG_MIPS_RPROC)
> +#define CONTEXT_SAVING
> +#endif
> +
> +#ifdef CONTEXT_SAVING
This looks really cumbersome. Why not make everything depend on
CONFIG_MIPS_RPROC instead?
> +static struct {
> + unsigned mask: GIC_NUM_LOCAL_INTRS;
nit/personal taste: can't you make this a normal type instead of a
bitfield? Considering that GIC_NUM_LOCAL_INTRS is hardcoded to 7, I'd
rather see a u8... or even an unsigned long if you have to use bitmap
operations on it.
> +} gic_local_state[NR_CPUS];
This looks like this really should be a percpu variable.
> +
> +#define gic_save_local_rmask(vpe, i) (gic_local_state[vpe].mask &= i)
> +#define gic_save_local_smask(vpe, i) (gic_local_state[vpe].mask |= i)
> +
> +static struct {
> + unsigned vpe: 8;
> + unsigned pin: 4;
> +
> + unsigned polarity: 1;
> + unsigned trigger: 1;
> + unsigned dual_edge: 1;
> + unsigned mask: 1;
> +} gic_shared_state[GIC_MAX_INTRS];
> +
> +#define gic_save_shared_vpe(i, v) gic_shared_state[i].vpe = v
> +#define gic_save_shared_pin(i, p) gic_shared_state[i].pin = p
> +#define gic_save_shared_polarity(i, p) gic_shared_state[i].polarity = p
> +#define gic_save_shared_trigger(i, t) gic_shared_state[i].trigger = t
> +#define gic_save_shared_dual_edge(i, d) gic_shared_state[i].dual_edge = d
> +#define gic_save_shared_mask(i, m) gic_shared_state[i].mask = m
Why don't you make these static functions? The compiler will inline them
nicely, and that will save you fixing them (they all miss proper
bracketing of arguments).
> +
> +#else
> +#define gic_save_local_rmask(vpe, i)
> +#define gic_save_local_smask(vpe, i)
> +
> +#define gic_save_shared_vpe(i, v)
> +#define gic_save_shared_pin(i, p)
> +#define gic_save_shared_polarity(i, p)
> +#define gic_save_shared_trigger(i, t)
> +#define gic_save_shared_dual_edge(i, d)
> +#define gic_save_shared_mask(i, m)
Please make those a "do { } while(0)" construct, so that the trailing
semi-colon is properly swallowed.
> +#endif /* CONTEXT_SAVING */
> +
> static void __gic_irq_dispatch(void);
>
> static inline u32 gic_read32(unsigned int reg)
> @@ -105,52 +147,94 @@ static inline void gic_update_bits(unsigned int reg, unsigned long mask,
> gic_write(reg, regval);
> }
>
> -static inline void gic_reset_mask(unsigned int intr)
> +static inline void gic_write_reset_mask(unsigned int intr)
> {
> gic_write(GIC_REG(SHARED, GIC_SH_RMASK) + GIC_INTR_OFS(intr),
> 1ul << GIC_INTR_BIT(intr));
> }
>
> -static inline void gic_set_mask(unsigned int intr)
> +static inline void gic_reset_mask(unsigned int intr)
> +{
> + gic_save_shared_mask(intr, 0);
> + gic_write_reset_mask(intr);
> +}
> +
> +static inline void gic_write_set_mask(unsigned int intr)
> {
> gic_write(GIC_REG(SHARED, GIC_SH_SMASK) + GIC_INTR_OFS(intr),
> 1ul << GIC_INTR_BIT(intr));
> }
>
> -static inline void gic_set_polarity(unsigned int intr, unsigned int pol)
> +static inline void gic_set_mask(unsigned int intr)
> +{
> + gic_save_shared_mask(intr, 1);
> + gic_write_set_mask(intr);
> +}
> +
> +static inline void gic_write_polarity(unsigned int intr, unsigned int pol)
> {
> gic_update_bits(GIC_REG(SHARED, GIC_SH_SET_POLARITY) +
> GIC_INTR_OFS(intr), 1ul << GIC_INTR_BIT(intr),
> (unsigned long)pol << GIC_INTR_BIT(intr));
> }
>
> -static inline void gic_set_trigger(unsigned int intr, unsigned int trig)
> +static inline void gic_set_polarity(unsigned int intr, unsigned int pol)
> +{
> + gic_save_shared_polarity(intr, pol);
> + gic_write_polarity(intr, pol);
> +}
> +
> +static inline void gic_write_trigger(unsigned int intr, unsigned int trig)
> {
> gic_update_bits(GIC_REG(SHARED, GIC_SH_SET_TRIGGER) +
> GIC_INTR_OFS(intr), 1ul << GIC_INTR_BIT(intr),
> (unsigned long)trig << GIC_INTR_BIT(intr));
> }
>
> -static inline void gic_set_dual_edge(unsigned int intr, unsigned int dual)
> +static inline void gic_set_trigger(unsigned int intr, unsigned int trig)
> +{
> + gic_save_shared_trigger(intr, trig);
> + gic_write_trigger(intr, trig);
> +}
> +
> +static inline void gic_write_dual_edge(unsigned int intr, unsigned int dual)
> {
> gic_update_bits(GIC_REG(SHARED, GIC_SH_SET_DUAL) + GIC_INTR_OFS(intr),
> 1ul << GIC_INTR_BIT(intr),
> (unsigned long)dual << GIC_INTR_BIT(intr));
> }
>
> -static inline void gic_map_to_pin(unsigned int intr, unsigned int pin)
> +static inline void gic_set_dual_edge(unsigned int intr, unsigned int dual)
> +{
> + gic_save_shared_dual_edge(intr, dual);
> + gic_write_dual_edge(intr, dual);
> +}
> +
> +static inline void gic_write_map_to_pin(unsigned int intr, unsigned int pin)
> {
> gic_write32(GIC_REG(SHARED, GIC_SH_INTR_MAP_TO_PIN_BASE) +
> GIC_SH_MAP_TO_PIN(intr), GIC_MAP_TO_PIN_MSK | pin);
> }
>
> -static inline void gic_map_to_vpe(unsigned int intr, unsigned int vpe)
> +static inline void gic_map_to_pin(unsigned int intr, unsigned int pin)
> +{
> + gic_save_shared_pin(intr, pin);
> + gic_write_map_to_pin(intr, pin);
> +}
> +
> +static inline void gic_write_map_to_vpe(unsigned int intr, unsigned int vpe)
> {
> gic_write(GIC_REG(SHARED, GIC_SH_INTR_MAP_TO_VPE_BASE) +
> GIC_SH_MAP_TO_VPE_REG_OFF(intr, vpe),
> GIC_SH_MAP_TO_VPE_REG_BIT(vpe));
> }
>
> +static inline void gic_map_to_vpe(unsigned int intr, unsigned int vpe)
> +{
> + gic_save_shared_vpe(intr, vpe);
> + gic_write_map_to_vpe(intr, vpe);
> +}
> +
> #ifdef CONFIG_CLKSRC_MIPS_GIC
> cycle_t gic_read_count(void)
> {
> @@ -537,6 +621,7 @@ static void gic_mask_local_irq(struct irq_data *d)
> {
> int intr = GIC_HWIRQ_TO_LOCAL(d->hwirq);
>
> + gic_save_local_rmask(smp_processor_id(), (1 << intr));
> gic_write32(GIC_REG(VPE_LOCAL, GIC_VPE_RMASK), 1 << intr);
> }
>
> @@ -544,6 +629,7 @@ static void gic_unmask_local_irq(struct irq_data *d)
> {
> int intr = GIC_HWIRQ_TO_LOCAL(d->hwirq);
>
> + gic_save_local_smask(smp_processor_id(), (1 << intr));
> gic_write32(GIC_REG(VPE_LOCAL, GIC_VPE_SMASK), 1 << intr);
> }
>
> @@ -561,6 +647,7 @@ static void gic_mask_local_irq_all_vpes(struct irq_data *d)
>
> spin_lock_irqsave(&gic_lock, flags);
> for (i = 0; i < gic_vpes; i++) {
> + gic_save_local_rmask(mips_cm_vp_id(i), 1 << intr);
> gic_write(GIC_REG(VPE_LOCAL, GIC_VPE_OTHER_ADDR),
> mips_cm_vp_id(i));
> gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_RMASK), 1 << intr);
> @@ -576,6 +663,7 @@ static void gic_unmask_local_irq_all_vpes(struct irq_data *d)
>
> spin_lock_irqsave(&gic_lock, flags);
> for (i = 0; i < gic_vpes; i++) {
> + gic_save_local_smask(mips_cm_vp_id(i), 1 << intr);
> gic_write(GIC_REG(VPE_LOCAL, GIC_VPE_OTHER_ADDR),
> mips_cm_vp_id(i));
> gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_SMASK), 1 << intr);
> @@ -983,6 +1071,85 @@ static struct irq_domain_ops gic_ipi_domain_ops = {
> .match = gic_ipi_domain_match,
> };
>
> +#ifdef CONTEXT_SAVING
> +static void gic_restore_shared(void)
> +{
> + unsigned long flags;
> + int i;
> +
> + spin_lock_irqsave(&gic_lock, flags);
> + for (i = 0; i < gic_shared_intrs; i++) {
> + gic_write_polarity(i, gic_shared_state[i].polarity);
> + gic_write_trigger(i, gic_shared_state[i].trigger);
> + gic_write_dual_edge(i, gic_shared_state[i].dual_edge);
> + gic_write_map_to_vpe(i, gic_shared_state[i].vpe);
> + gic_write_map_to_pin(i, gic_shared_state[i].pin);
> +
> + if (gic_shared_state[i].mask)
> + gic_write_set_mask(i);
> + else
> + gic_write_reset_mask(i);
> + }
> + spin_unlock_irqrestore(&gic_lock, flags);
> +}
> +
> +static void gic_restore_local(unsigned int vpe)
> +{
> + int hw, virq, intr, mask;
> + unsigned long flags;
> +
> + for (hw = 0; hw < GIC_NUM_LOCAL_INTRS; hw++) {
> + intr = GIC_LOCAL_TO_HWIRQ(hw);
> + virq = irq_linear_revmap(gic_irq_domain, intr);
> + gic_local_irq_domain_map(gic_irq_domain, virq, hw);
> + }
> +
> + local_irq_save(flags);
> + gic_write(GIC_REG(VPE_LOCAL, GIC_VPE_OTHER_ADDR), vpe);
> +
> + /* Enable EIC mode if necessary */
> + gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_CTL), cpu_has_veic);
> +
> + /* Restore interrupt masks */
> + mask = gic_local_state[vpe].mask;
> + gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_RMASK), ~mask);
> + gic_write32(GIC_REG(VPE_OTHER, GIC_VPE_SMASK), mask);
> +
> + local_irq_restore(flags);
> +}
> +#endif /* CONTEXT_SAVING */
> +
> +#ifdef CONFIG_MIPS_RPROC
I'm even more confused now. How can you not have both CONFIG_MIPS_RPROC
and CONTEXT_SAVING defined at the same time?
> +/*
> + * The MIPS remote processor driver allows non-Linux firmware to take control
> + * of and execute on one of the systems VPEs. If that VPE is brought back under
> + * Linux, it is necessary to ensure that all GIC interrupts are routed and
> + * masked as Linux expects them, as the firmware can have done anything it
> + * likes with the GIC configuration (hopefully just for that VPEs local
> + * interrupt sources, but allow for shared external interrupts as well).
> + */
> +static int gic_cpu_notify(struct notifier_block *nfb, unsigned long action,
> + void *hcpu)
> +{
> + unsigned int cpu = mips_cm_vp_id((unsigned long)hcpu);
> +
> + switch (action) {
> + case CPU_UP_PREPARE:
> + case CPU_DOWN_FAILED:
> + gic_restore_shared();
> + gic_restore_local(cpu);
> + default:
> + break;
> + }
> +
> + return NOTIFY_OK;
> +}
> +
> +static struct notifier_block gic_cpu_notifier __refdata = {
> + .notifier_call = gic_cpu_notify
> +};
> +#endif /* CONFIG_MIPS_RPROC */
> +
> static void __init __gic_init(unsigned long gic_base_addr,
> unsigned long gic_addrspace_size,
> unsigned int cpu_vec, unsigned int irqbase,
> @@ -1082,6 +1249,10 @@ static void __init __gic_init(unsigned long gic_base_addr,
> }
>
> gic_basic_init();
> +
> +#ifdef CONFIG_MIPS_RPROC
> + register_hotcpu_notifier(&gic_cpu_notifier);
> +#endif /* CONFIG_MIPS_RPROC */
> }
>
> void __init gic_init(unsigned long gic_base_addr,
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
next prev parent reply other threads:[~2016-09-02 10:54 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-02 9:59 [PATCH 0/6] MIPS: Remote processor driver Matt Redfearn
2016-09-02 9:59 ` Matt Redfearn
2016-09-02 9:59 ` [PATCH 1/6] irqchip: mips-gic: Add context saving for MIPS_REMOTEPROC Matt Redfearn
2016-09-02 9:59 ` Matt Redfearn
2016-09-02 10:27 ` Matt Redfearn
2016-09-02 10:27 ` Matt Redfearn
2016-09-02 10:54 ` Marc Zyngier [this message]
2016-09-02 14:18 ` Matt Redfearn
2016-09-02 14:18 ` Matt Redfearn
2016-09-02 9:59 ` [PATCH 2/6] MIPS: tlb-r4k: If there are wired entries, don't use TLBINVF Matt Redfearn
2016-09-02 9:59 ` Matt Redfearn
2016-09-02 9:59 ` [PATCH 3/6] MIPS: smp.c: Introduce mechanism for freeing and allocating IPIs Matt Redfearn
2016-09-02 9:59 ` Matt Redfearn
2016-09-02 9:59 ` [PATCH 4/6] MIPS: CPS: Add VP(E) stealing Matt Redfearn
2016-09-02 9:59 ` Matt Redfearn
2016-09-02 9:59 ` [PATCH 5/6] remoteproc/MIPS: Add a remoteproc driver for MIPS Matt Redfearn
2016-09-02 9:59 ` Matt Redfearn
2016-09-12 8:49 ` Matt Redfearn
2016-09-12 8:49 ` Matt Redfearn
2016-09-02 9:59 ` [PATCH 6/6] MIPS: Deprecate VPE Loader Matt Redfearn
2016-09-02 9:59 ` Matt Redfearn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57C95A4C.10703@arm.com \
--to=marc.zyngier@arm.com \
--cc=jason@lakedaemon.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mips@linux-mips.org \
--cc=lisa.parratt@imgtec.com \
--cc=matt.redfearn@imgtec.com \
--cc=ralf@linux-mips.org \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.