From: Meador Inge <meador_inge@mentor•com>
To: Benjamin Herrenschmidt <benh@kernel•crashing.org>
Cc: Hollis Blanchard <hollis_blanchard@mentor•com>,
devicetree-discuss@lists•ozlabs.org,
linuxppc-dev@lists•ozlabs.org
Subject: Re: [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property
Date: Thu, 10 Mar 2011 11:23:13 -0600 [thread overview]
Message-ID: <4D790901.9000701@mentor.com> (raw)
In-Reply-To: <1299036140.8833.818.camel@pasglop>
On 03/01/2011 09:22 PM, Benjamin Herrenschmidt wrote:
>
>> diff --git a/arch/powerpc/include/asm/mpic.h b/arch/powerpc/include/asm/mpic.h
>> index e000cce..7e1be12 100644
>> --- a/arch/powerpc/include/asm/mpic.h
>> +++ b/arch/powerpc/include/asm/mpic.h
>> @@ -325,6 +325,8 @@ struct mpic
>> #ifdef CONFIG_PM
>> struct mpic_irq_save *save_data;
>> #endif
>> +
>> + int cpu;
>> };
>
> Why ? The MPIC isn't specifically associated with a CPU and whatever we
> pick as default during boot isn't relevant later on, so I don't see why
> we should have global permanent state here.
I agree. I shouldn't have cached that. We should probably introduce a
helper function to get the cpuid, though. The:
unsigned int cpu = 0;
if (mpic->flags & MPIC_PRIMARY)
cpu = hard_smp_processor_id();
code is scattered in three places: '_mpic_cpu_write', '_mpic_cpu_read',
and 'mpic_init'.
>> /* Check if we have one of those nice broken MPICs with a flipped endian on
>> @@ -622,6 +631,14 @@ static unsigned int mpic_is_ipi(struct mpic *mpic, unsigned int irq)
>> return (src>= mpic->ipi_vecs[0]&& src<= mpic->ipi_vecs[3]);
>> }
>>
>> +/* Determine if the linux irq is a timer interrupt */
>> +static unsigned int mpic_is_timer_interrupt(struct mpic *mpic, unsigned int irq)
>> +{
>> + unsigned int src = mpic_irq_to_hw(irq);
>> +
>> + return (src>= mpic->timer_vecs[0]&& src<= mpic->timer_vecs[3]);
>> +}
>> +
>>
>> /* Convert a cpu mask from logical to physical cpu numbers. */
>> static inline u32 mpic_physmask(u32 cpumask)
>> @@ -967,6 +984,15 @@ static int mpic_host_map(struct irq_host *h, unsigned int virq,
>> if (hw>= mpic->irq_count)
>> return -EINVAL;
>>
>> + /* If the MPIC was reset, then all vectors have already been
>> + * initialized. Otherwise, the appropriate vector needs to be
>> + * initialized here to ensure that only used sources are setup with
>> + * a vector.
>> + */
>> + if (mpic->flags& MPIC_NO_RESET)
>> + if (!(mpic_is_ipi(mpic, hw) || mpic_is_timer_interrupt(mpic, hw)))
>> + mpic_init_vector(mpic, hw);
>> +
>
> The above isn't useful. Of those two registers you want to initialize,
> afaik, one (the destination) will be initialized by the core calling
> into set_affinity when the interrupt is requested, and the other one is
AFAIK, we can't rely on 'set_affinity' always being called. I don't
think it is called at all when !defined(CONFIG_SMP) and if it was, then
that would be an error:
/* include/linux/irq.h */
#else /* CONFIG_SMP */
static inline int irq_set_affinity(unsigned int irq,
const struct cpumask *m)
{
return -EINVAL;
}
> partially initialized in set_type, I'd say just modify set_type to
> initialize the source as well, and problem solved, no ?
The priority has to be initialized as well. They could both be done in
'mpic_set_irq_type', but that seems like a weird place since it has
nothing to do with actually setting the type.
Since we already have 'mpic_irq_set_priority' and 'mpic_set_vector',
perhaps a better option is to add 'mpic_set_destination' and put the
following in 'mpic_host_map' (using the cpuid helper function suggested
above):
/* Lazy source init when MPIC_NO_RESET */
if (!mpic_is_ipi(mpic, hw) && (mpic->flags & MPIC_NO_RESET)) {
mpic_set_vector(virq, hw);
mpic_set_destination(virq, mpic_cpuid(mpic));
mpic_irq_set_priority(virq, 8);
}
It is more overhead, but it reads well. Thoughts?
> Or is there a chance that the interrupt was left unmasked ?
There shouldn't be. The original idea was that either the boot program
would leave it masked or one of the AMP OSes would boot without
'pic-no-reset', which would mask everything. Most likely the boot program.
> I think it would be kosher to mask it in set_type unconditionally, I don't think it's ever supposed
> to be called for an enabled interrupt.
I will look into that.
Thanks,
--
Meador Inge | meador_inge AT mentor.com
Mentor Embedded | http://www.mentor.com/embedded-software
next prev parent reply other threads:[~2011-03-10 17:23 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-02-25 21:59 [PATCH v4 0/2] powerpc: Open PIC binding and "pic-no-reset" Meador Inge
2011-02-25 21:59 ` [PATCH v4 1/2] powerpc: document the Open PIC device tree binding Meador Inge
2011-02-28 7:44 ` Grant Likely
2011-02-25 21:59 ` [PATCH v4 2/2] powerpc: make MPIC honor the "pic-no-reset" device tree property Meador Inge
2011-03-02 3:22 ` Benjamin Herrenschmidt
2011-03-10 17:23 ` Meador Inge [this message]
2011-03-10 22:11 ` Benjamin Herrenschmidt
2011-03-10 22:13 ` Benjamin Herrenschmidt
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=4D790901.9000701@mentor.com \
--to=meador_inge@mentor$(echo .)com \
--cc=benh@kernel$(echo .)crashing.org \
--cc=devicetree-discuss@lists$(echo .)ozlabs.org \
--cc=hollis_blanchard@mentor$(echo .)com \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox