Re: [PATCH] dmaengine: xgene-dma: Fix double IRQ issue by setting IRQ_DISABLE_UNLAZY flag
On Wed, 6 Jan 2016, Vinod Koul wrote:
> On Wed, Jan 06, 2016 at 02:51:07PM +0530, Rameshwar Sahu wrote:
> > >> @@ -1610,6 +1611,7 @@ static int xgene_dma_request_irqs(struct xgene_dma
> > >> *pdma)
> > >> /* Register DMA channel rx irq */
> > >> for (i = 0; i < XGENE_DMA_MAX_CHANNEL; i++) {
> > >> chan = >chan[i];
> > >> + irq_set_status_flags(chan->rx_irq, IRQ_DISABLE_UNLAZY);
> > >
> > > Why not use irq_settings_disable_unlazy(), at least read the reference you
> > > pointed out!
> >
> > irq_settings_disable_unlazy() is helper function to test
> > IRQ_DISABLE_UNLAZY flag is set or not, it's not for setting this flag.
> > FYI...
> > +static inline bool irq_settings_disable_unlazy(struct irq_desc *desc)
> > +{
> > + return desc->status_use_accessors & _IRQ_DISABLE_UNLAZY;
> > +}
>
> Ah yes, I saw clear API and assumed there would be set. Then I think we
> should add a set helper as well as the usage is intended for drivers to
> set this flag
>
> Thomas,
>
> Any reason why you didn't add a set helper, only test and clear?
Why would I? Those helpers are core internal and not usable in random drivers.
Drivers have irq_set_status_flags()/irq_clear_status_flags() ...
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] dmaengine: xgene-dma: Fix double IRQ issue by setting IRQ_DISABLE_UNLAZY flag
On Wed, 6 Jan 2016, Suman Tripathi wrote: > On Wed, Jan 6, 2016 at 3:35 PM, Thomas Gleixner <t...@linutronix.de> wrote: > > Why would I? Those helpers are core internal and not usable in random > > drivers. > > > > i think the problem is the name of the function. It should be something > > irq_check_settings_disable_unlazy And why is that relevant to a driver? Again, those helpers are core internal and well understood. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH (v4) 2/11] MIPS: bmips: Add bcm6345-l2-timer interrupt controller
Simon,
On Thu, 26 Nov 2015, Simon Arlott wrote:
> +static inline u32 bcm6345_timer_read_int_status(struct bcm6345_timer *timer)
> +{
> + if (timer->interrupt_bits == 32)
> + return __raw_readl(timer->base + timer->regs[TIMER_INT_STATUS]);
> + else
> + return __raw_readb(timer->base + timer->regs[TIMER_INT_STATUS]);
> +}
Instead of having that pile of conditionals you could just define two
functions and have a function pointer in struct bcm6345_timer which
you initialize at init time.
> +static inline void bcm6345_timer_write_control(struct bcm6345_timer *timer,
> + unsigned int id, u32 val)
> +{
> + if (id >= timer->nr_timers) {
> + WARN(1, "%s: %d >= %d", __func__, id, timer->nr_timers);
This is more than silly. You call that form the init function via:
for (i = 0; i < timer->nr_timers; i++)
Hmm?
> +static void bcm6345_timer_unmask(struct irq_data *d)
> +{
> + struct bcm6345_timer *timer = irq_data_get_irq_chip_data(d);
> + unsigned long flags;
> + u8 val;
> +
> + if (d->hwirq < timer->nr_timers) {
Again. You can have two different interrupt chips without that
completely undocumented and non obvious conditional.
BTW, how are those simple interrupts masked at all?
> + raw_spin_lock_irqsave(>lock, flags);
> + val = bcm6345_timer_read_int_enable(timer);
> + val |= BIT(d->hwirq);
> + bcm6345_timer_write_int_enable(timer, val);
> + raw_spin_unlock_irqrestore(>lock, flags);
> + }
> +}
> + raw_spin_lock_init(>lock);
> + timer->regs = regs;
> + timer->interrupt_bits = interrupt_bits;
> + timer->nr_timers = nr_timers;
> + timer->nr_interrupts = nr_timers + 1;
What is that extra interrupt about? For the casual reader this looks
like a bug ... Comments exist for a reason.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] MIPS: ath79: add irq chip ar7240-misc-intc
On Sat, 19 Sep 2015, Alexander Couzens wrote: > The ar7240 misc irq chip use ack handler > instead of ack_mask handler. All new ath79 chips use > the ar7240 misc irq chip > > Signed-off-by: Alexander Couzens <lyn...@fe80.eu> > Acked-by: Alban Bedel <al...@free.fr> > --- > .../interrupt-controller/qca,ath79-misc-intc.txt | 20 > ++-- > arch/mips/ath79/irq.c| 10 ++ That driver should probably move into drivers/irqchip. Other than that. Acked-by: Thomas Gleixner <t...@linutronix.de> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] MIPS: ath79: set missing irq ack handler for ar7100-misc-intc irq chip
On Sat, 19 Sep 2015, Alexander Couzens wrote: > The irq ack handler was forgotten while introducing OF support. > Only ar71xx and ar933x based devices require it. > > Signed-off-by: Alexander Couzens <lyn...@fe80.eu> > Acked-by: Alban Bedel <al...@free.fr> Acked-by: Thomas Gleixner <t...@linutronix.de> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c
On Thu, 27 Aug 2015, Hanjun Guo wrote: [1]: https://lkml.org/lkml/2015/7/29/236 If that is ok with you, we will introduce similar DECLARE_ thing for clock declare. Yes. Or we can drop this patch from this patch set, and clean up this patch when the ACPI_DECLARE() infrastructure is ready for upstream. Works either way. I just noticed that hard coded init thing and decided to rant about it :) Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c
On Thu, 27 Aug 2015, Hanjun Guo wrote:
On 08/26/2015 03:17 AM, Thomas Gleixner wrote:
On Wed, 26 Aug 2015, Fu Wei wrote:
/* Initialize per-processor generic timer */
-static int __init arch_timer_acpi_init(struct acpi_table_header
*table)
+void __init arch_timer_acpi_init(void)
{
And how is that supposed to work when we have next generation CPUs
which implement a different timer? You break multisystem kernels that
way.
Sorry, I think I missed some context here that I don't understand
why the code here will break multisystem kernels? I'm trying to
understand the problem here and update the code :)
yes, you are right, If there is a next generation CPUs which
implement a different timer, (maybe) this driver can not work.
we may need a new timer driver.
But,
(1) for now, aarch64 core always has the arch timer(this timer is
part of aarch64 architecture).
and the existing code make ARM64 kernel select ARM_ARCH_TIMER
(2) GTDT is designed for generic timer, so in this call
arch_timer_acpi_init we parse the gtdt info.
(3) once we have a ARM64 CPUs which implement a different timer, we
may need to select a right timer in the config stage.
and this timer may not be described in GTDT. So we can implement
another arch_timer_acpi_init by that time in new timer driver..
if the new time still uses GTDT(or new version GTDT), we may need to
update gtdt.c for new timer by that time.
That's simply wrong. You want to build kernels which run on both cpus
and the selection of the timer happens at runtime depending on the
ACPI info. We do the same thing with device tree.
I think the code can do that if I understand correctly. The code for
now is that we only support arch timer on ARM64, and this patch set
is adding SBSA watchdog timer support which need same function in
arch timer, so we move that function to common place.
We will load the driver (arch timer, memory mapped timer) when the
ACPI table defines them, which when new timer is coming, that will
defined in the ACPI table and load the driver as needed.
Please correct me if I misse something, thanks.
arch_timer_acpi_init() is called from the architecture boot code. So
how is that supposed to work with different timers?
Are you going to have bla_timer_acpi_init() and foo_timer_acpi_init()
calls as well?
Why not having a something like DT has: DECLARE_
and the arch_timer_acpi_init() using that to figure out which of the
timers to initialize.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c
On Wed, 26 Aug 2015, Fu Wei wrote:
/* Initialize per-processor generic timer */
-static int __init arch_timer_acpi_init(struct acpi_table_header *table)
+void __init arch_timer_acpi_init(void)
{
And how is that supposed to work when we have next generation CPUs
which implement a different timer? You break multisystem kernels that
way.
yes, you are right, If there is a next generation CPUs which
implement a different timer, (maybe) this driver can not work.
we may need a new timer driver.
But,
(1) for now, aarch64 core always has the arch timer(this timer is
part of aarch64 architecture).
and the existing code make ARM64 kernel select ARM_ARCH_TIMER
(2) GTDT is designed for generic timer, so in this call
arch_timer_acpi_init we parse the gtdt info.
(3) once we have a ARM64 CPUs which implement a different timer, we
may need to select a right timer in the config stage.
and this timer may not be described in GTDT. So we can implement
another arch_timer_acpi_init by that time in new timer driver..
if the new time still uses GTDT(or new version GTDT), we may need to
update gtdt.c for new timer by that time.
That's simply wrong. You want to build kernels which run on both cpus
and the selection of the timer happens at runtime depending on the
ACPI info. We do the same thing with device tree.
but before we really have this new timer, I think this code is OK to use.
I don't think so, but I leave this decision to the ARM64 maintainers.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v7 8/8] clocksource: simplify ACPI code in arm_arch_timer.c
On Tue, 25 Aug 2015, fu@linaro.org wrote:
You Cc the world and some more on your patch, but you fail to add the
maintainers of the clocksource code to the Cc list. Sigh.
From: Fu Wei fu@linaro.org
The patch update arm_arch_timer driver to use the function
provided by the new GTDT driver of ACPI.
By this way, arm_arch_timer.c can be simplified, and separate
all the ACPI GTDT knowledge from this timer driver.
That's not a proper changelog and this patch want's to be split in two:
1) Implement the new ACPI function
2) Make use of it
index 0aa135d..99505bb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -817,68 +817,30 @@ CLOCKSOURCE_OF_DECLARE(armv7_arch_timer_mem,
arm,armv7-timer-mem,
arch_timer_mem_init);
#ifdef CONFIG_ACPI
-static int __init map_generic_timer_interrupt(u32 interrupt, u32 flags)
-{
- int trigger, polarity;
-
- if (!interrupt)
- return 0;
-
- trigger = (flags ACPI_GTDT_INTERRUPT_MODE) ? ACPI_EDGE_SENSITIVE
- : ACPI_LEVEL_SENSITIVE;
-
- polarity = (flags ACPI_GTDT_INTERRUPT_POLARITY) ? ACPI_ACTIVE_LOW
- : ACPI_ACTIVE_HIGH;
-
- return acpi_register_gsi(NULL, interrupt, trigger, polarity);
-}
-
/* Initialize per-processor generic timer */
-static int __init arch_timer_acpi_init(struct acpi_table_header *table)
+void __init arch_timer_acpi_init(void)
{
And how is that supposed to work when we have next generation CPUs
which implement a different timer? You break multisystem kernels that
way.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] irqchip: bcm2836: Use a CPU notifier enable IPIs.
On Mon, 3 Aug 2015, Eric Anholt wrote:
Thomas Gleixner t...@linutronix.de writes:
On Mon, 27 Jul 2015, Eric Anholt wrote:
+/* Unmasks the IPI on the CPU wen it's first brought online. */
when
+static int bcm2836_arm_irqchip_cpu_notify(struct notifier_block *nfb,
+unsigned long action, void *hcpu)
+{
+ unsigned int cpu = (unsigned long)hcpu;
+ unsigned int int_reg = LOCAL_MAILBOX_INT_CONTROL0;
+ unsigned int mailbox = 0;
+
+ if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
+ bcm2836_arm_irqchip_unmask_per_cpu_irq(int_reg, mailbox, cpu);
Shouldn't you mask the irq on CPU_DYING?
I was just following what other drivers were doing. Is CPU_DYING the
only thing that needs masking?
CPPU_DYING is the counterpart of CPU_STARTING. It's called from the
CPU which goes down.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] irqchip: bcm2836: Use a CPU notifier enable IPIs.
On Mon, 27 Jul 2015, Eric Anholt wrote:
+/* Unmasks the IPI on the CPU wen it's first brought online. */
when
+static int bcm2836_arm_irqchip_cpu_notify(struct notifier_block *nfb,
+ unsigned long action, void *hcpu)
+{
+ unsigned int cpu = (unsigned long)hcpu;
+ unsigned int int_reg = LOCAL_MAILBOX_INT_CONTROL0;
+ unsigned int mailbox = 0;
+
+ if (action == CPU_STARTING || action == CPU_STARTING_FROZEN)
+ bcm2836_arm_irqchip_unmask_per_cpu_irq(int_reg, mailbox, cpu);
Shouldn't you mask the irq on CPU_DYING?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.
On Mon, 13 Jul 2015, Eric Anholt wrote:
+static void
+bcm2836_arm_irqchip_smp_init(void)
+{
+#ifdef CONFIG_SMP
+ int i;
+
+ /* unmask IPIs */
+ for_each_possible_cpu(i) {
+ bcm2836_arm_irqchip_unmask_per_cpu_irq(
+ LOCAL_MAILBOX_INT_CONTROL0, 0, i);
Do you really want to enable these interrupts on all possible cpus? Why so?
Shouldn't that be done at the point where a hotplugged CPU is starting ?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] arm64: mediatek: enable MTK_TIMER
On Fri, 17 Jul 2015, Matthias Brugger wrote: On Monday, July 13, 2015 05:32:47 PM Yingjoe Chen wrote: Enable MTK_TIMER for MediaTek plaform, which will be used as schedule clock. Signed-off-by: Yingjoe Chen yingjoe.c...@mediatek.com --- Applied, thanks. So you enable the current code w/o the modifications required for this to work? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] irqchip: Add bcm2836 interrupt controller for Raspberry Pi 2.
On Fri, 10 Jul 2015, Stephen Warren wrote:
On 07/07/2015 03:13 PM, Eric Anholt wrote:
+static struct arm_local_intc intc __read_mostly;
It'd be nice to give everything (types, functions, variables) a
consistent symbol prefix; bcm2836_arm_irqchip_ sounds like a good
bikeshed to me, but perhaps just propagating the above arm_local_ to the
functions too would be good, although that seems to risk symbol name
collisions with other ARM SoCs.
Which is irrelevant because its static.
+static void bcm2836_mask_per_cpu_irq(unsigned int reg, unsigned int bit)
+{
+ void __iomem *reg_base = intc.base + reg;
+ unsigned int i;
+
+ for (i = 0; i 4; i++)
Is 4 there the CPU count? Perhaps this should use one of the Linux
APIs to query the CPU count rather than hard-coding it?
Should per-CPU IRQs automatically be masked on all CPUs at once, or only
on the current CPU? A very quick look at the ARM GIC driver implies it
doesn't iterate over all CPUs when masking per-CPU IRQs.
Usually per cpu interrupts are only masked on the cpu which is calling
the function. The whole reason why per cpu interrupts exist is that
you can share the same interrupt number for all cores.
So masking all interrupts is not a good idea. In this case if a cpu is
hot unplugged, then all other cpus would not longer get timer
interrupts. Not what you really want, right?
+static void bcm2836_mask_gpu_irq(struct irq_data *d)
+{
+}
+
+static void bcm2836_unmask_gpu_irq(struct irq_data *d)
+{
+}
If the IRQs can't be masked, should these functions actually be implemented?
We have a few places in the core which expect at least mask/unmask to
be implemented.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/9] arm: twr-k70f120m: clock driver for Kinetis SoC
On Thu, 2 Jul 2015, Paul Osmialowski wrote: On Thu, 2 Jul 2015, Arnd Bergmann wrote: I wonder if you could move out the fixed rate clocks into their own nodes. Are they actually controlled by the same block? If they are just fixed, you can use the normal binding for fixed rate clocks and only describe the clocks that are related to the driver. In my view having these clocks grouped together looks more convincing. After all, they all share the same I/O regs in order to read configuration. The fact that they share a register is not making them a group. That's just a HW design decision and you need to deal with that by protecting the register access, but not by trying to group them artificially at the functional level. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/9] arm: twr-k70f120m: timer driver for Kinetis SoC
On Wed, 1 Jul 2015, Paul Osmialowski wrote: Hi Thomas, On Wed, 1 Jul 2015, Thomas Gleixner wrote: + clockevents_register_device( + kinetis_clockevent_tmrs[chan].evtdev); + + kinetis_pit_init(kinetis_clockevent_tmrs[chan], + (rate / HZ) - 1); + kinetis_pit_enable(kinetis_clockevent_tmrs[chan], 1); No point doing this. The core code has invoked the set_periodic call back via clockevents_register_device() already. As I removed this kinetis_pit_enable() line, the timer did not start, therefore the system became unusable. What could be possible reason for that? Well, you need to move both, the init and the enable into set_periodic(). Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/9] arm: twr-k70f120m: clock source drivers for Kinetis SoC
On Tue, 23 Jun 2015, Paul Osmialowski wrote:
+/*
+ * Clock event device set mode function
+ */
+static void kinetis_clockevent_tmr_set_mode(
+ enum clock_event_mode mode, struct clock_event_device *clk)
+{
+ struct kinetis_clock_event_ddata *pit =
+ container_of(clk, struct kinetis_clock_event_ddata, evtdev);
+
+ switch (mode) {
+ case CLOCK_EVT_MODE_PERIODIC:
+ kinetis_pit_enable(pit-base, 1);
+ break;
+ case CLOCK_EVT_MODE_ONESHOT:
+ case CLOCK_EVT_MODE_UNUSED:
+ case CLOCK_EVT_MODE_SHUTDOWN:
+ default:
+ kinetis_pit_enable(pit-base, 0);
+ }
+}
Please move to the new set_state_* interfaces. set_mode() is deprecated.
+static int kinetis_clockevent_tmr_set_next_event(
+ unsigned long delta, struct clock_event_device *c)
+{
+ struct kinetis_clock_event_ddata *pit =
+ container_of(c, struct kinetis_clock_event_ddata, evtdev);
+ unsigned long flags;
+
+ raw_local_irq_save(flags);
Pointless exercise. This is called with interrupts disabled.
+ kinetis_pit_init(pit-base, delta);
+ kinetis_pit_enable(pit-base, 1);
+ raw_local_irq_restore(flags);
+static struct irqaction kinetis_clockevent_irqaction[KINETIS_PIT_CHANNELS] =
{
+ {
+ .name = Kinetis Kernel Time Tick (pit0),
Please use oneword descriptive names not half sentences.
+ .flags = IRQF_TIMER | IRQF_IRQPOLL,
+ .dev_id = kinetis_clockevent_tmrs[0],
+ .handler = kinetis_clockevent_tmr_irq_handler,
+ }, {
+ .name = Kinetis Kernel Time Tick (pit1),
+ .flags = IRQF_TIMER | IRQF_IRQPOLL,
+ .dev_id = kinetis_clockevent_tmrs[1],
+ .handler = kinetis_clockevent_tmr_irq_handler,
+ }, {
+ .name = Kinetis Kernel Time Tick (pit2),
+ .flags = IRQF_TIMER | IRQF_IRQPOLL,
+ .dev_id = kinetis_clockevent_tmrs[2],
+ .handler = kinetis_clockevent_tmr_irq_handler,
+ }, {
+ .name = Kinetis Kernel Time Tick (pit3),
+ .flags = IRQF_TIMER | IRQF_IRQPOLL,
+ .dev_id = kinetis_clockevent_tmrs[3],
+ .handler = kinetis_clockevent_tmr_irq_handler,
+ },
Aside of that. Please use standard request_irq() there is no reason to
use setup_irq here.
+
+ setup_irq(irq, (kinetis_clockevent_irqaction[chan]));
request_irq(irq, handler, flags, name, kinetis_clockevent_tmrs[chan]);
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] clocksource: Add Pistachio clocksource-only driver
On Thu, 21 May 2015, Ezequiel Garcia wrote:
+static cycle_t clocksource_read_cycles(struct clocksource *cs)
+{
+ u32 counter, overflw;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(lock, flags);
Hmm. Is that lock really necessary to read that counter? The
clocksource is global. And if its actually used for timekeeping, the
lock can get heavy contended.
+ overflw = gpt_readl(TIMER_CURRENT_OVERFLOW_VALUE, 0);
+ counter = gpt_readl(TIMER_CURRENT_VALUE, 0);
+ raw_spin_unlock_irqrestore(lock, flags);
+
+ return ~(cycle_t)counter;
+}
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 6/7] clocksource: Add Pistachio clocksource-only driver
On Thu, 21 May 2015, Ezequiel Garcia wrote:
On 05/21/2015 07:00 PM, Thomas Gleixner wrote:
On Thu, 21 May 2015, Ezequiel Garcia wrote:
+static cycle_t clocksource_read_cycles(struct clocksource *cs)
+{
+ u32 counter, overflw;
+ unsigned long flags;
+
+ raw_spin_lock_irqsave(lock, flags);
Hmm. Is that lock really necessary to read that counter? The
clocksource is global. And if its actually used for timekeeping, the
lock can get heavy contended.
Yup, it is really (and sadly) necessary. The kernel hangs up completely
without it when the counter is accesed by more than one CPU.
Apparently, those two timer registers overflow and counter must be read
atomically.
Welcome to the wonderful world of useless timer hardware.
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/5] clocksource: st_lpc: Add LPC timer as a clocksource.
On Fri, 17 Apr 2015, Peter Griffin wrote:
+/* Low Power Timer */
+#define LPC_LPT_LSB_OFF 0x400
+#define LPC_LPT_MSB_OFF 0x404
+#define LPC_LPT_START_OFF0x408
+
+struct st_lpc {
+ struct clk *clk;
+ void __iomem *iomem_cs;
+};
+
+static struct st_lpc *st_lpc;
+
+static u64 notrace st_lpc_counter_read(void)
+{
+ u64 counter;
+ u32 lower;
+ u32 upper, old_upper;
+
+ upper = readl_relaxed(st_lpc-iomem_cs + LPC_LPT_MSB_OFF);
+ do {
+ old_upper = upper;
+ lower = readl_relaxed(st_lpc-iomem_cs + LPC_LPT_LSB_OFF);
+ upper = readl_relaxed(st_lpc-iomem_cs + LPC_LPT_MSB_OFF);
+ } while (upper != old_upper);
+
+ counter = upper;
+ counter = 32;
+ counter |= lower;
+ return counter;
What's the point of this exercise? The kernel can handle 32bit
clocksources nicely. So why do you want to artificially expand them to
64bit by adding useless loops and hoops to a hotpath?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/4] Add basic support for Mediatek MT8173 SoC
On Mon, 26 Jan 2015, Matthias Brugger wrote: Applied to v3.20-next/arm64. Yingjoe Chen (1): irqchip: mtk-sysirq: Get irq number from register resource size I just queued that irqchip patch in irq/core -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH RESEND v3 0/3] irqchip: vf610-mscm: add support for MSCM interrupt router
On Thu, 15 Jan 2015, Stefan Agner wrote: Splitted out version of the MSCM driver. My first driver based on the routeable domain support and was part of the Vybrid Cortex-M4 support patchset. So far the MSCM interrupt router was initialized by the boot loader and configured all interrupts for the Cortex-A5 CPU. There are two use cases where a proper driver is necessary: - To run Linux on the Cortex-M4. When the kernel is running on the non-preconfigured CPU, the interrupt router need to be configured properly. - To support deeper sleep modes: LPSTOP clears the interrupt router configuration, hence a driver needs to restore the configuration on resume. I created a seperate patchset for that driver which hopefully makes it easier to get it into mergeable state. Since I identified some registers likely to be used by other drivers (e.g. CPU ID or the CPU Generate Interrupt Register) I also added the syscon compatible string to make the registers available for other drivers in case needed. This resend version of this patchset is rebased on v3.19-rc4. Has the discussion with Marc on the original V3 set been resolved? What about the DT bindings? I'm relucant to pick that up w/o acks from the DT folks. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2] clockevents: rockchip: Add rockchip timer for rk3288
On Sun, 25 Jan 2015, Daniel Lezcano wrote:
+static inline void rk_timer_set_mode(enum clock_event_mode mode,
+ struct clock_event_device *ce)
+{
+ switch (mode) {
+ case CLOCK_EVT_MODE_PERIODIC:
+ rk_timer_disable(ce);
+ rk_timer_update_counter(rk_timer(ce)-freq / HZ - 1, ce);
+ rk_timer_enable(ce, TIMER_MODE_FREE_RUNNING);
Missing break. You disable the timer again right away ...
+ case CLOCK_EVT_MODE_ONESHOT:
+ case CLOCK_EVT_MODE_RESUME:
+ break;
+ case CLOCK_EVT_MODE_UNUSED:
+ case CLOCK_EVT_MODE_SHUTDOWN:
+ rk_timer_disable(ce);
+ break;
+ }
+}
+
+static irqreturn_t rk_timer_interrupt(int irq, void *dev_id)
+{
+ struct clock_event_device *ce = dev_id;
+
+ rk_timer_interrupt_clear(ce);
+
+ if (ce-mode == CLOCK_EVT_MODE_ONESHOT) {
+ rk_timer_disable(ce);
+ }
No need for the braces here.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/4] irqchip: mtk-sysirq: Get irq number from register resource size
On Thu, 22 Jan 2015, Matthias Brugger wrote: 2015-01-13 14:12 GMT+01:00 Matthias Brugger matthias@gmail.com: 2015-01-12 10:14 GMT+01:00 Eddie Huang eddie.hu...@mediatek.com: From: Yingjoe Chen yingjoe.c...@mediatek.com Originally mtk-sysirq hardcoded supported irq number to 224. This was fine since all SoCs before support the same number of irqs for intpol. However MT8173 intpol support 32 more irq pins, changes to get irq number from register resource size to suppor MT8173 properly. Signed-off-by: Yingjoe Chen yingjoe.c...@mediatek.com Signed-off-by: Eddie Huang eddie.hu...@mediatek.com --- drivers/irqchip/irq-mtk-sysirq.c | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) Jason, Thomas, will you take this patch through your branch? I will take the rest of the series. Best regards, Matthias -- motzblog.wordpress.com Hi Thomas, hi Jason, do you have any comments on this patch? Jason is swamped. I'm currently collecting stuff. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] irqchip: gic: Allow interrupt level to be set for PPIs.
On Thu, 22 Jan 2015, Marc Zyngier wrote: On Tue, Jan 20 2015 at 4:52:59 pm GMT, Liviu Dudau liviu.du...@arm.com wrote: During a recent cleanup of the arm64 DTs it has become clear that the handling of PPIs in _set_type() is incorrect. The ARM TRMs for GICv2 and later allow for implementation defined support for setting the edge or level type of the PPI interrupts and don't restrict the activation level of the signal. Current ARM implementations do restrict the PPI level type to IRQ_TYPE_LEVEL_LOW, but licensees of the IP can decide to shoot themselves in the foot at any time. Signed-off-by: Liviu Dudau liviu.du...@arm.com For the GIC and GICv3 parts: Acked-by: Marc Zyngier marc.zyng...@arm.com So I queue that if there are no further objections from Russell or the DT folks. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 0/5] ARM: at91: fix irq_pm_install_action WARNING
On Fri, 23 Jan 2015, Boris Brezillon wrote: - change the compatible string to clearly show that this chip is purely virtual So we probably want to do : s/dumb/virt/ for both DT and code to make it clear from the names as well. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V4 01/14] sh: Eliminate unused irq_reg_{readl,writel} accessors
On Mon, 19 Jan 2015, Geert Uytterhoeven wrote: Will you still do so, or shall I forward the patch to Andrew Morton? akpm please. -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 RESEND 2/8] irqchip: Supply new driver for STi based devices
On Fri, 23 Jan 2015, Lee Jones wrote: On Fri, 23 Jan 2015, Thomas Gleixner wrote: The only technical comment I have is: shouldn't all the stuff except the resume function be marked __init or is any of this required post init? It's not common to mark functions invoked at and affter *_probe() as __init. At least, not as far as I'm aware. Right, if the probe stuff is supposed to be functional post init, then we of course need to keep it around. But I guess we have no mechanism to distinguish the 'boot only' and 'keep post init' case. Might be worthwhile to investigate. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 RESEND 1/8] dt: bindings: Supply shared ST IRQ defines
On Fri, 23 Jan 2015, Lee Jones wrote: On Fri, 23 Jan 2015, Thomas Gleixner wrote: Can we please stop adding these pointless filenames all over the place? They are useless and wrong in a lot of cases. Less of a copy and paste and more and a `mv` without fixing up the file names. I guess they are silly, but they are rife, so someone must think they're a good idea. :) I'd rather say they are ripe for being banned :) Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 RESEND 1/8] dt: bindings: Supply shared ST IRQ defines
On Thu, 22 Jan 2015, Lee Jones wrote: These defines are used to allow values used for configuration to be easily human readable and will lessen the chance of logical mistakes. Signed-off-by: Lee Jones lee.jo...@linaro.org --- include/dt-bindings/interrupt-controller/irq-st.h | 30 +++ 1 file changed, 30 insertions(+) create mode 100644 include/dt-bindings/interrupt-controller/irq-st.h diff --git a/include/dt-bindings/interrupt-controller/irq-st.h b/include/dt-bindings/interrupt-controller/irq-st.h new file mode 100644 index 000..4c59ace --- /dev/null +++ b/include/dt-bindings/interrupt-controller/irq-st.h @@ -0,0 +1,30 @@ +/* + * include/linux/irqchip/irq-st.h Copy Paste? Can we please stop adding these pointless filenames all over the place? They are useless and wrong in a lot of cases. Aside of that, this needs an ack from the DT folks. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 RESEND 2/8] irqchip: Supply new driver for STi based devices
On Thu, 22 Jan 2015, Lee Jones wrote: This driver is used to enable System Configuration Register controlled External, CTI (Core Sight), PMU (Performance Management), and PL310 L2 Cache IRQs prior to use. I'm wondering how this is related to irq_chip, but well, I don't mind it being parked here. Though I really cannot say anything about this DT translation machinery for a single sysconfig register, other than it looks completely overengineered to me. The only technical comment I have is: shouldn't all the stuff except the resume function be marked __init or is any of this required post init? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Thu, 15 Jan 2015, Rob Herring wrote: On Thu, Jan 15, 2015 at 3:11 AM, Thomas Gleixner t...@linutronix.de wrote: On Wed, 14 Jan 2015, Rob Herring wrote: We do not change shared interrupts in any way. We provide an alternative mechanism for braindead hardware. And if the at91 folks are fine with the DT change, then it's their decision. Nothing forces this on everyone. We are changing how shared interrupts are described in DT. We don't need 2 ways to describe them. We could say this is only for AT91 and continue to describe shared interrupts as has been done forever. Then the next platform that hits this problem will have to go thru the same ABI breakage. Or we change DT practices to describe all shared interrupts with a demux node. Given the way DTs are incrementally created, it is not something we can check with review or tools, so we will still have the same ABI breakage problem. This is not describing the proper shared interrupts. This is a special case for a special case of braindamaged hardware. Whats wrong with doing that? We dont have to change that for all shared interrupts because 99% of them have a proper hardware implementation and are not affected by this. What's wrong with serving the AT91 with a proper solution, which does NOT inflict horrible hacks into the core code and does NOT weaken sanity checks and does NOT require irq chip specific knowledge in device drivers? There are probably ways to do this demux irqchip without a DT change. So far you have not provided any useful hint how to do so. What's the problem with a DT change for a single platform, if the maintainers are willing to take it and deal with the fallout? What's the solution for a platform that an ABI break is not okay and can't deal with the fallout? There is no other platform affected. This is a break for a specific set of devices and the 'fallout' is confined, well known and accepted. So what's your problem, really? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Wed, 14 Jan 2015, Rob Herring wrote: On Wed, Jan 14, 2015 at 4:36 AM, Thomas Gleixner t...@linutronix.de wrote: All attempts to work around that have resulted in horrible bandaids so far. That's why I guided Boris to implement this dummy demultiplexing mechanism. It solves the problem at hand nicely without adding nasty hackarounds into the suspend/resume code and inflicting platform knowledge on multi-platform device drivers. This change will break on old kernels with a new dtb. Would you be happy if a BIOS update required a new kernel? Fixing this for any platform requires a dtb update which may not be possible on some platforms. I don't have a problem with this breakage for 1 platform and the at91 guys may not care, but we'd ultimately be changing how all shared irqs are specified for all DT. Maybe we decide that this is how we want to describe things, but that needs much wider discussion and agreement. We do not change shared interrupts in any way. We provide an alternative mechanism for braindead hardware. And if the at91 folks are fine with the DT change, then it's their decision. Nothing forces this on everyone. If you have a proper solution for the problem at hand which - avoids the demux dummy - works straight forward with suspend/resume/wakeup - does not add horrible new APIs - does not add conditionals to the interrupt hotpath - does not inflict platform knowledge about interrupt chip details on drivers then I'm happy to take it. But as long as you can't come up with anything sane, the demux dummy is the best solution for this problem we've seen so far. What if during suspend you move all actions w/o IRQF_NO_SUSPEND to a suspended action list? This would leave only the actions with IRQF_NO_SUSPEND set in the active action list. The cost would be a pointer in irq_desc and moving the actions during suspend and resume. That's exactly what we want NOT. Because it prevents us to do proper sanity checks for IRQF_NO_SUSPEND. We've been there and I rejected it for that very reason. There are probably ways to do this demux irqchip without a DT change. What's the problem with a DT change for a single platform, if the maintainers are willing to take it and deal with the fallout? And in case of AT91 the new kernel will simply work with the old DT and just emit the same warning vs. the IRQF_NO_SUSPEND mismatch. Older kernels won't work with a new DT, but that's about it. Aside of that, I'm quite amused about your DT update worries. DTs break with every kernel version on very popular platforms in very weird and subtle ways. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/5] irqchip: add dumb demultiplexer implementation
On Tue, 13 Jan 2015, Boris Brezillon wrote:
+ ret = irq_set_handler_data(irq, demux);
+ if (ret) {
+ pr_err(Failed to assign handler data\n);
+ goto err_free_domain;
+ }
+
+ irq_set_chained_handler(irq, irq_dumb_demux_handler);
+
+ /*
+ * Disable the src irq (automatically enabled by
+ * irq_set_chained_handler) to prevent irqs from happening while
+ * nobody requested any of the demuxed irqs.
+ */
+ disable_irq(irq);
We rather prevent the startup of the irq line right away.
enum {
IRQ_CHAINED_NONE,
IRQ_CHAINED_STARTUP,
IRQ_CHAINED_NOSTARTUP,
};
-__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int is_chained,
- const char *name);
+__irq_set_handler(unsigned int irq, irq_flow_handler_t handle, int
chained_mode,
+ const char *name);
{
if (handle != handle_bad_irq chained_mode) {
irq_settings_set_noprobe(desc);
irq_settings_set_norequest(desc);
irq_settings_set_nothread(desc);
- irq_startup(desc, true);
+ if (chained_mode == IRQ_CHAINED_STARTUP)
+ irq_startup(desc, true);
}
}
Hmm?
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] irqchip: vf610-mscm: add support for MSCM interrupt router
On Tue, 16 Dec 2014, Marc Zyngier wrote: On 15/12/14 20:58, Stefan Agner wrote: The same line adjustment will break the 80 character border... But I agree, it's ugly the way it is now. Will put them in the same line. Never mind what checkpatch says. Readability trumps it anytime. If you want to obey checkpatch then move that stuff into a seperate inline function. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/12] irqchip: gic: define register_routable_domain_ops conditional
On Wed, 3 Dec 2014, Arnd Bergmann wrote: On Wednesday 03 December 2014 01:12:02 Stefan Agner wrote: The inline function register_routable_domain_ops is only usable if CONFIG_ARM_GIC is set. Make it depend on this configuration. This also allows other SoC interrupt controller to provide such a function. Signed-off-by: Stefan Agner ste...@agner.ch I don't think this is a good idea: either the interface is meant to be generic and should work with any interrupt controller, or it is specific to one irqchip and another irqchip should provide a different interface that has a nonconflicting name. I suppose you want the latter here, given that the declaration is part of the gic specific header file. That routable_domain stuff should have never been invented. In hindsight we should have done the stacked irq domains back then already, but in hindsight ... If you look at the crossbar scheme what we have now: GIC domain |--| | |-- private | non routable |-- peripheral | |-- peripheral | | |--| | |-- peripheral | routable |-- peripheral || |-- peripheral |--| | || |--| crossbar magic | || which is not how the hardware looks. The hardware actually looks like this: GIC domain |--| | |-- private | non routable |-- peripheral | |-- peripheral | | |--| |--| | | | |-- peripheral | routable |---| crossbar |-- peripheral | | | domain |-- peripheral |--| |--| So it is completely obvious that the peripheral interrupts which need to be routed through the crossbar belong to a different domain. We really should convert crossbar to that scheme and get rid of the routable domain stuff completely. With vybrid thats not different according to the diagram in the reference manual. GIC domain |--| | |-- private | non routable |-- peripheral | |-- peripheral | | |--| |--| | | | | | routable |---| IRQ router | | | | domain | | | | | |--| |--| | Shared state |-- CPU to CPU NVIC domain | and hardware |-- peripheral |--| | |-- peripheral | | |--| | | | | | routable |---| IRQ router | | | | domain | | | | | |--| |--| | | | |-- private | non routable |-- peripheral | |-- peripheral |--| The routable domain stuff is just an adhoc redirector which must be tied to a particular base irq chip implementation (i.e. GIC). With stacked irq domains there is no dependency on the underlying irq chip. No ifdeffery, nothing. So you can use the same router domain implementation for both the A5 and the M4 side. On the A5 side the router domain has the gic domain as parent and on the M4 side the nvic domain. The shared interrupts are allocated through the router domain which decides whether the interrupt can be assigned to a particular core or not. If it can be assigned it allocates the corresponding interrupt in the parent domain, sets up the routing and everything just works. See: http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/log/?h=irq/irqdomain-arm Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/12] irqchip: gic: define register_routable_domain_ops conditional
On Wed, 3 Dec 2014, Marc Zyngier wrote: On 03/12/14 17:28, Stefan Agner wrote: On 2014-12-03 14:04, Thomas Gleixner wrote: The shared interrupts are allocated through the router domain which decides whether the interrupt can be assigned to a particular core or not. If it can be assigned it allocates the corresponding interrupt in the parent domain, sets up the routing and everything just works. What do you mean by the shared state in the drawing above? Currently, I check whether a interrupt is already used by the other core by reading the register (do this configuration register reflect the shared state in your drawing?). I think that is basically it. It should only be the register that decides on the actual routing. BTW, how do you arbitrate between concurrent accesses to this register? Or is only the A5 allowed to change it? What I meant with 'shared state' is basically the configuration register space. Plus depending on the mechanism you want to use for correlating the routing between the A5 and the M4 some shared memory state, locking, IPC or whatever you need for this. http://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/log/?h=irq/irqdomain-arm Thanks for the link. So my work would involve to support domain hierarchy for NVIC (proper irq_domain_ops, introduce arm,routable-irqs property, anything else?) and then make use of the hierarchy code in my MSCM driver like for instance the mtk-sysirq driver...? I don't think we need the arm,routable-irq property at all, and I'm looking at refactoring the crossbar thingy to remove most of its entanglement with the GIC. All you need to know is the range of interrupts you're allowed to request through the routable domain. The inner-most irqchip shouldn't even know about it (after all, they are just wires coming in). It should be the duty of the outer-most irqchip (the router) to generate the correct request to the GIC/NVIC. So all the knowledge should be at the router level. The mtk-sysrq code is indeed a good example of what you can do. The gic-v2m MSI stuff is probably helpful as well. What is the state of the IRQ domain hierarchy, when will it go upstream? Scheduled for 3.19, if everything goes according to plan. Don't think we can go back anyway... ;-) Indeed. That would be a major headache... Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
Tony,
On Thu, 6 Nov 2014, Tony Lindgren wrote:
Any comments on the patch below? Let me know if you want to keep the
devm stuff out of kernel/irq/manage.c.
Sorry, this slipped through the cracks.
+static int setup_wakeirq(struct device *dev, unsigned int wakeirq,
+unsigned long wakeflags, bool devm)
+{
+ int ret;
+
+ if (!(dev wakeirq)) {
+ pr_err(Missing device or wakeirq for %s irq %d\n,
+ dev_name(dev), wakeirq);
+ return -EINVAL;
+ }
+
+ if (!(wakeflags
+ (IRQF_TRIGGER_LOW | IRQF_TRIGGER_HIGH | IRQF_ONESHOT))) {
+ pr_err(Invalid wakeirq for %s irq %d, must be level oneshot\n,
+ dev_name(dev), wakeirq);
This looks odd.
Why do you want to enforce LEVEL and ONESHOT? I can see the point for
ONESHOT, but I'm wondering about the requirement for level.
Now if you really want to enforce level AND oneshot, your check is
wrong as it will not trigger on
wakeflags = IRQF_TRIGGER_LOW;
wakeflags = IRQF_TRIGGER_HIGH;
wakeflags = IRQF_ONESHOT;
Not what you really want, right?
+int request_wake_irq(struct device *dev, unsigned int wakeirq,
+unsigned long wakeflags)
+{
+ return setup_wakeirq(dev, wakeirq, wakeflags, false);
+}
+EXPORT_SYMBOL(request_wake_irq);
_GPL please
+
+int devm_request_wake_irq(struct device *dev, unsigned int wakeirq,
+ unsigned long wakeflags)
+{
+ return setup_wakeirq(dev, wakeirq, wakeflags, false);
Shouldnt that have devm = true?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Thu, 13 Nov 2014, Tony Lindgren wrote:
Oops thanks for catching that. As the devres stuff is separate, I've
updated the patch to keep it that way by adding a minimal manage.h.
This avoids including internals.h in devres.c. Does that seem usable
for you?
What's wrong with internals.h? devres.c is core code, so it is not
affected of the ban to include internals.h :)
+/**
+ * init_disabled_wakeirq - initialize a wake-up interrupt for a device
+ * @dev: device to wake up on the wake-up interrupt
+ * @wakeirq: wake-up interrupt for the device
+ * @wakeflags: wake-up interrupt flags
+ *
+ * Note that the wake-up interrupt starts disabled. The wake-up interrupt
+ * is typically enabled from the device pm_runtime_suspend() and disabled
+ * again in the device pm_runtime_resume(). For runtime PM, the wake-up
+ * interrupt should be always enabled, and for device suspend and resume,
+ * the wake-up interrupt should be enabled depending on the device specific
+ * configuration for device_can_wakeup().
+ *
+ * Note also that we are not resending the lost device interrupts.
+ * We assume that the wake-up interrupt just needs to wake-up the device,
+ * and then device pm_runtime_resume() can deal with the situation.
+ *
+ * There are at least the following reasons to not resend the lost device
+ * interrupts automatically based on the wake-up interrupt:
+ *
+ * 1. There can be interrupt reentry issues calling the device interrupt
+ * based on the wake-up interrupt if done in the device driver. It
+ * could be done with check_irq_resend() after checking the device
+ * interrupt mask if we really wanted to though.
+ *
+ * 2. The device interrupt handler would need to be set up properly with
+ * pm_runtime_irq_safe(). Ideally you don't want to call pm_runtime
+ * calls from the device interrupt handler at all.
+ *
+ * 3. The IRQ subsystem may not know if it's safe to call the device
+ * interrupt unless the driver updates the interrupt status with
+ * disable_irq() and enable_irq() in addition to just disabling the
+ * interrupt at the hardware level in the device registers.
+ *
+ * So if replaying the lost device interrupts is absolutely needed from the
+ * hardware point of view, it's probably best to set up a completely
+ * separate wake-up interrupt handler for the wake-up interrupt in the
+ * device driver because of the reasons above.
Can we please kill this last paragraph? I'm already seeing the
gazillion of I think it is required to do so for my s special
chip implementations in random drivers which all get it wrong again.
So I'd rather provide a mechanism upfront which lets the driver know
that the wakeup interrupt originated from that device, i.e. let the
wake up handler call
pm_wakeup_irq(dev);
which calls:
pm_runtime_mark_last_busy(dev);
pm_request_resume(dev);
and aside of that tells the device via a flag or preferrably a
sequence counter that the wakeup irq has been triggered. So affected
devices can handle it based on that information w/o implementing the
next broken variant of wakeup irq handlers.
That also allows to remove the wakeflags check for level/edge.
+ */
+int init_disabled_wakeirq(struct device *dev, unsigned int wakeirq,
+ unsigned long wakeflags)
+{
+ if (!(dev wakeirq)) {
This is the second time I stumbled over this. While it is correct it
would be simpler to parse
if (!dev || !wakeirq) {
At least for my review damaged brain :)
+ pr_err(Missing device or wakeirq for %s irq %d\n,
+dev_name(dev), wakeirq);
+ return -EINVAL;
+ }
+
+ if (!(wakeflags IRQF_ONESHOT)) {
+ pr_err(Invalid wakeirq for %s irq %d, must be oneshot\n,
+dev_name(dev), wakeirq);
+ return -EINVAL;
+ }
Is there a reason why we force the wakeirq into a threaded handler?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 0/6] ARM: mediatek: Add support for interrupt polarity
On Sun, 9 Nov 2014, Jason Cooper wrote: Thomas, Marc, Could I get Acks for the core changes and gic changes when you have a moment? this depends on the core irqdomain changes which are not yet in tip. I'm waiting for Jiangs latest iteration and Marcs feedback on those. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Thu, 6 Nov 2014, Thomas Gleixner wrote: On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Actually, declaring msi_domain_ops as non-static, and allow other code to override the .alloc and .free? Why do you want to do that? I know why. Because you want to spare a level of hierarchy. But thats wrong simply because MSI itself is an interrupt chip at the device level. [ MSI ] --- [ GIC-MSI ] --- [ GIC ] So the MSI level only cares about the allocation of the virq space. GIC-MSI allocates out of the bitmap which handles the hard wired range of MSI capable GIC interrupts and GIC handles the underlying functionality. And this makes a lot of sense, if you think about interrupt remapping. If ARM ever grows that you simply insert it into the chain: [ MSI ] --- [ Remap] --- [ GIC-MSI ] --- [ GIC ] If you look at Jiangs x86 implementation it does exactly that. [ MSI ] --- [ Vector ] [ MSI ] --- [ Remap ] --- [ Vector ] And because ARM has this intermediate layer of GIC-MSI you need to represent it in the hierarchy whether you like it or not. If you'd try to bolt the GIC-MSI magic into the MSI layer itself, then interrupt remapping would never work. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Wed, 5 Nov 2014, Suravee Suthikulanit wrote: On 11/5/2014 6:05 PM, Suravee Suthikulanit wrote: - Overall, it seems that msi_domain_alloc() could be quite different across architectures. Would it be possible to declare this function as weak, and allow arch to override (similar to arch_setup_msi_irq)? Actually, declaring msi_domain_ops as non-static, and allow other code to override the .alloc and .free? Why do you want to do that? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Mon, 3 Nov 2014, Suravee Suthikulanit wrote: On 11/3/2014 4:51 PM, Thomas Gleixner wrote: On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote: + irq_domain_set_hwirq_and_chip(v2m-domain, virq, hwirq, + v2m_chip, v2m); + + irq_set_msi_desc(hwirq, desc); + irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING); Sure both calls work perfectly fine as long as virq == hwirq, right? I was running into an issue when calling the irq_domain_alloc_irq_parent(), it requires of_phandle_args pointer to be passed in. However, this does not work for GICv2m since it does not have interrupt information in the device tree. So, I decided at first to use direct (virq == hwirq) mapping, which simplifies the code a bit, but might not be ideal solution, as you pointed out. It's not only far from ideal. It's not a solution at all. Simply because there is no guarantee for virq == hwirq. An alternative would be to create a temporary struct of_phandle_args, and populate it with the interrupt information for the requested MSI. Then pass it to: -- irq_domain_alloc_irq_parent |-- gic_irq_domain_alloc |-- gic_irq_domain_xlate |-- gic_irq_domain_map However, this would still not be ideal if we want to support ACPI. Another Neither device tree nor ACPI has anything to do with MSI interrupts at runtime. All they do is to tell that there is a MSI controller and where the registers are and in the worst case fixups for a borked MSI_TYPER register. So either the TYPER reg or DT/ACPI gives you a fixed hwirq range which is reserved for MSI. And that's all you need, right? The MSI interrupt itself has no DT/ACPI information to use at allocation time simply because you CANNOT decribe a MSI device interrupt in DT/ACPI by any means. And you do not need any DT/ACPI information at that point. All you need is to pick one hwirq out of the existing fixed range and associate it to a newly allocated virq. That's the only information the underlying gic domain has to know about, because it needs to translate from the hwirq to the virq in the low level entry handler gic_handle_irq(). alternative would be coming up with a dedicate structure to be used here. I noticed on X86, it uses struct irq_alloc_info. May be that's what we also need here. It's a x86 concept to transport X86 specific information in order to avoid duplicated code all over the place. And x86 MSI support is a completely different beast than the thing you are dealing with. x86 has no concept of a fixed hwirq range for MSI. So no, just picking random stuff from random MSI implementations does not help at all. [...] I do not care at all how YOU waste your time. But I care very much about the fact that YOU are wasting MY precious time by exposing me to your patch trainwrecks. I don't intend to waste yours or anybody's precious time. Sorry if it takes a couple iterations to work out the issues. Also, I will try to put more comment in my code to make it more clear. Let me know what works best for you to work out the issues. By not sending obviously broken and half thought out patches in the first place. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Tue, 4 Nov 2014, Suravee Suthikulpanit wrote: And that's what I am trying to do here except that GIC is expecting that information to be passed to it via irq_domain_alloc_irqs(..., args) where args is struct of_phandle_args (e.g. in the kernel/irqdomain.c: irq_create_of_mapping). This works fine when specifying interrupt from DT, but that is not always the case. Currently, I can just create a fake of_phandle_args just to pass the hwirq information to GIC. -- gicv2m_setup_msi_irq() |struct of_phandle_args phan; |phan.np = NULL; |phan.args_count = 3; |phan.args[0] = 0; |phan.args[1] = hwirq - 32; |phan.args[2] = IRQ_TYPE_EDGE_RISING; |-- irq_domain_alloc_irqs(d, 1, NUMA_NO_NODE, phan); |-- gicv2m_domain_alloc(d, virq, nr_irqs, arg) |-- irq_domain_alloc_irqs_parent(d, virq, nr_irqs, arg); I am trying to figure out what would be a common data structure for this purpose that would work for both Dt and non-DT case (e.g. GICv2m MSI). Unless you think this is ok. You need to sort that out with Marc. It needs to be done in a way which is usable for the other potential use cases of stacked domains on top of GIC. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 00/14] genirq endian fixes; bcm7120/brcmstb IRQ updates
On Mon, 3 Nov 2014, Arnd Bergmann wrote:
On Saturday 01 November 2014 18:03:47 Kevin Cernekee wrote:
V2-V3:
- Move updated irq_reg_{readl,writel} functions back into linux/irq.h
so they can be called by irqchip drivers
- Add gc-reg_{readl,writel} function pointers so that irqchip
drivers like arch/sh/boards/mach-se/{7343,7722}/irq.c can override them
- CC: linux-sh list in lieu of Paul's defunct linux-sh.org email address
- Fix handling of zero L2 status in bcm7120-l2.c
- Rebase on Linus' head of tree
Looks all great. I also looked at the series now and am very happy
about how it turned out.
Does that translate to an Acked-by on the whole lot?
Jason, can you please pick that lot up with an Acked-by-me on the
changes to the existing infrastructure?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V10 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Mon, 3 Nov 2014, suravee.suthikulpa...@amd.com wrote:
+static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
+{
+ int pos;
+ struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
+
+ spin_lock(v2m-msi_cnt_lock);
Why do you need an extra lock here? Is that stuff not serialized from
the msi_chip layer already?
If not, why don't we have the serialization there instead of forcing
every callback to implement its own?
+ pos = irq - v2m-spi_start;
So this assumes that @irq is the hwirq number, right? How does the
calling function know about that? It should only have knowledge about
the virq number if I'm not missing something.
And if I'm missing something, then that msi_chip stuff is seriously
broken.
+ if (pos = 0 pos v2m-nr_spis)
So you simply avoid the clear bitmap instead of yelling loudly about
being called with completely wrong data?
I would not be surprised if that is related to my question above.
+ bitmap_clear(v2m-bm, pos, 1);
+static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
+ struct msi_desc *desc)
+{
+ int hwirq, virq, offset;
+ struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
+
+ if (!desc)
+ return -EINVAL;
Why on earth does every callback of msi_chip have to check for this?
+ spin_lock(v2m-msi_cnt_lock);
+ offset = bitmap_find_free_region(v2m-bm, v2m-nr_spis, 0);
+ spin_unlock(v2m-msi_cnt_lock);
+ if (offset 0)
+ return offset;
+
+ hwirq = v2m-spi_start + offset;
+ virq = __irq_domain_alloc_irqs(v2m-domain, hwirq,
+1, NUMA_NO_NODE, v2m, true);
+ if (virq 0) {
+ gicv2m_teardown_msi_irq(chip, hwirq);
+ return virq;
+ }
+
+ irq_domain_set_hwirq_and_chip(v2m-domain, virq, hwirq,
+ v2m_chip, v2m);
+
+ irq_set_msi_desc(hwirq, desc);
+ irq_set_irq_type(hwirq, IRQ_TYPE_EDGE_RISING);
Sure both calls work perfectly fine as long as virq == hwirq, right?
+ return 0;
Q: How does this populate virq to the caller?
A: Not at all
Q: How does the caller know which virq this function assigned?
A: Not at all
Q: How does the device driver know which virq to request?
A: Not at all
Q: Was this patch ever properly tested?
A: Not at all.
I do not care at all how YOU waste your time. But I care very much
about the fact that YOU are wasting MY precious time by exposing me to
your patch trainwrecks.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V9 PATCH 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Fri, 31 Oct 2014, suravee.suthikulpa...@amd.com wrote:
+/*
+ * alloc_msi_irq - Allocate MSIs from available MSI bitmap.
+ * @data: Pointer to v2m_data
+ * @nvec: Number of interrupts to allocate
+ * @irq: Pointer to the allocated irq
+ *
+ * Allocates interrupts only if the contiguous range of MSIs
+ * with specified nvec are available. Otherwise return the number
+ * of available interrupts. If none are available, then returns -ENOENT.
And the exact purpose of returning the number of available interrupts
is?
+ */
+static int alloc_msi_irq(struct v2m_data *data, int nvec, int *irq)
+{
+ int size = data-nr_spis;
+ int next = size, i = nvec, ret;
+
+ /* We should never allocate more than available nr_spis */
+ if (i = size)
+ i = size;
+
+ spin_lock(data-msi_cnt_lock);
+
+ for (; i 0; i--) {
+ next = bitmap_find_next_zero_area(data-bm,
+ size, 0, i, 0);
+ if (next size)
+ break;
+ }
That we need a pointless loop here.
+static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
+ struct msi_desc *desc)
+{
+ int hwirq = 0, virq, avail;
+ struct v2m_data *v2m = container_of(chip, struct v2m_data, msi_chip);
+
+ if (!desc) {
+ dev_err(pdev-dev,
+ MSI setup failed. Invalid msi descriptor\n);
+ return -EINVAL;
+ }
+
+ avail = alloc_msi_irq(v2m, 1, hwirq);
+ if (avail != 0) {
So that the caller can turn any non zero return value into -ENOSPC.
+ dev_err(pdev-dev,
+ MSI setup failed. Cannnot allocate IRQ\n);
+ return -ENOSPC;
+ }
Brilliant design.
+ virq = __irq_domain_alloc_irqs(v2m-domain, hwirq,
+1, NUMA_NO_NODE, v2m, true);
And surely the ability of alloc_msi_irq() to allocate a contiguous
vector space is required to satisfy an hardcoded allocation of ONE
interrupt.
What is guaranteeing that the caller only requests a single interrupt?
+err_out:
Single error exit which undoes the stuff in the same order it got
initialized is just plain wrong. Ever looked at proper error exits in
other kernel files?
+ of_pci_msi_chip_remove(v2m-msi_chip);
+ if (v2m-base)
+ iounmap(v2m-base);
+ if (v2m-bm)
+ kzfree(v2m-bm);
Of course you need to zero out the kzalloced bitmap before freeing it
in order not to leak the secrets of a zeroed buffer to the sneaky
black hats, right?
Oh well...
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 03/15] genirq: Generic chip: Move irq_reg_{readl,writel} accessors into generic-chip.c
On Wed, 29 Oct 2014, Kevin Cernekee wrote: This allows us to implement per-irqchip behavior when necessary, instead of hardcoding the behavior for all irqchip drivers at compile time. Signed-off-by: Kevin Cernekee cerne...@gmail.com --- include/linux/irq.h | 7 --- kernel/irq/generic-chip.c | 10 ++ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/include/linux/irq.h b/include/linux/irq.h index 03f48d9..8049e93 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -639,13 +639,6 @@ void arch_teardown_hwirq(unsigned int irq); void irq_init_desc(unsigned int irq); #endif -#ifndef irq_reg_writel -# define irq_reg_writel(val, addr) writel(val, addr) -#endif -#ifndef irq_reg_readl -# define irq_reg_readl(addr) readl(addr) -#endif - Brilliant patch that. # git grep -l irq_reg_readl drivers/irqchip/ drivers/irqchip/irq-atmel-aic.c drivers/irqchip/irq-atmel-aic5.c drivers/irqchip/irq-sunxi-nmi.c drivers/irqchip/irq-tb10x.c Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 03/15] genirq: Generic chip: Move irq_reg_{readl,writel} accessors into generic-chip.c
On Thu, 30 Oct 2014, Arnd Bergmann wrote: On Thursday 30 October 2014 09:43:02 Thomas Gleixner wrote: diff --git a/include/linux/irq.h b/include/linux/irq.h index 03f48d9..8049e93 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -639,13 +639,6 @@ void arch_teardown_hwirq(unsigned int irq); void irq_init_desc(unsigned int irq); #endif -#ifndef irq_reg_writel -# define irq_reg_writel(val, addr) writel(val, addr) -#endif -#ifndef irq_reg_readl -# define irq_reg_readl(addr) readl(addr) -#endif - Brilliant patch that. # git grep -l irq_reg_readl drivers/irqchip/ drivers/irqchip/irq-atmel-aic.c drivers/irqchip/irq-atmel-aic5.c drivers/irqchip/irq-sunxi-nmi.c drivers/irqchip/irq-tb10x.c Patch 1/15 changes these all. I think it's still broken because patch 2/15 is wrong, but it's not /that/ obviously broken. Oops. Missed that. -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 02/15] sh: Eliminate unused irq_reg_{readl,writel} accessors
On Thu, 30 Oct 2014, Arnd Bergmann wrote: On Wednesday 29 October 2014 19:17:55 Kevin Cernekee wrote: Defining these macros way down in arch/sh/.../irq.c doesn't cause kernel/irq/generic-chip.c to use them. As far as I can tell this code has no effect. Signed-off-by: Kevin Cernekee cerne...@gmail.com Actually it overrides the 32-bit accessors with 16-bit accessors, which does seem intentional and certainly has an effect. Not really. Neither arch/sh/boards/mach-se/7343/irq.c nor arch/sh/boards/mach-se/7722/irq.c actually use irq_reg_readl/writel. They simply define it. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 03/15] genirq: Generic chip: Move irq_reg_{readl,writel} accessors into generic-chip.c
On Thu, 30 Oct 2014, Thomas Gleixner wrote: On Thu, 30 Oct 2014, Arnd Bergmann wrote: On Thursday 30 October 2014 09:43:02 Thomas Gleixner wrote: diff --git a/include/linux/irq.h b/include/linux/irq.h index 03f48d9..8049e93 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -639,13 +639,6 @@ void arch_teardown_hwirq(unsigned int irq); void irq_init_desc(unsigned int irq); #endif -#ifndef irq_reg_writel -# define irq_reg_writel(val, addr) writel(val, addr) -#endif -#ifndef irq_reg_readl -# define irq_reg_readl(addr) readl(addr) -#endif - Brilliant patch that. # git grep -l irq_reg_readl drivers/irqchip/ drivers/irqchip/irq-atmel-aic.c drivers/irqchip/irq-atmel-aic5.c drivers/irqchip/irq-sunxi-nmi.c drivers/irqchip/irq-tb10x.c Patch 1/15 changes these all. I think it's still broken because patch 2/15 is wrong, but it's not /that/ obviously broken. Oops. Missed that. But looking at: drivers/irqchip/irq-sunxi-nmi.c It's using the generic chip. So while the generic chip uses the accessor function, the driver implementation which implements extra functionality will use something hardcoded. While it does not break the current setup, it's wrong nevertheless. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 05/15] genirq: Generic chip: Add big endian I/O accessors
On Thu, 30 Oct 2014, Arnd Bergmann wrote:
On Wednesday 29 October 2014 19:17:58 Kevin Cernekee wrote:
static LIST_HEAD(gc_list);
static DEFINE_RAW_SPINLOCK(gc_lock);
+static int is_big_endian(struct irq_chip_generic *gc)
+{
+ return !!(gc-domain-gc-gc_flags IRQ_GC_BE_IO);
+}
+
static void irq_reg_writel(struct irq_chip_generic *gc,
u32 val, int reg_offset)
{
- writel(val, gc-reg_base + reg_offset);
+ if (is_big_endian(gc))
+ iowrite32be(val, gc-reg_base + reg_offset);
+ else
+ writel(val, gc-reg_base + reg_offset);
}
What I had in mind was to use indirect function calls instead, like
#ifndef irq_reg_writel
static inline void irq_reg_writel_le(u32 val, void __iomem *addr)
{
return writel(val, addr);
}
#endif
#ifndef irq_reg_writel_be
static inline void irq_reg_writel_be(u32 val, void __iomem *addr)
{
return iowrite32_be(val, addr);
}
#endif
static inline void irq_reg_writel(struct irq_chip_generic *gc, u32 val, int
reg_offset)
{
if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP)
That's inside of the generic irq chip, so CONFIG_GENERIC_IRQ_CHIP is
always set when this is compiled.
!IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
return irq_reg_writel_le(val, gc-reg_base + reg_offset);
if (IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP)
!IS_ENABLED(CONFIG_GENERIC_IRQ_CHIP_BE))
s/!// ?
return irq_reg_writel_be(val, gc-reg_base + reg_offset);
I don't think the above will cover all combinations.
..._CHIP_BE ...CHIP_LE
N N ; Default behaviour: readl/writel
Y N ; ioread/write32be
N Y ; Default behaviour: readl/writel
Y Y ; Runtime selected
return gc-writel(val, gc-reg_base + reg_offset);
}
This would take the condition out of the callers.
So you trade a conditional for an indirect call. Not sure what's more
expensive. The indirect call is definitely a smaller text footprint,
so we should opt for this.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] irqchip: Allow irq_reg_{readl,writel} to use __raw_{readl_writel}
On Tue, 28 Oct 2014, Kevin Cernekee wrote: On big-endian systems readl/writel may perform an unwanted endian swap, breaking generic-chip.c. Let the platform code opt to use the __raw_ variants by selecting RAW_IRQ_ACCESSORS. This is required in order for bcm3384 to use GENERIC_IRQ_CHIP. Several existing irqchip drivers also use the __raw_ accessors directly, so it is a reasonably common requirement. How does that work with multi arch kernels? Thanks, tglx Signed-off-by: Kevin Cernekee cerne...@gmail.com --- drivers/irqchip/Kconfig | 3 +++ include/linux/irq.h | 13 + 2 files changed, 16 insertions(+) diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index b21f12f..6f0e80b 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -2,6 +2,9 @@ config IRQCHIP def_bool y depends on OF_IRQ +config RAW_IRQ_ACCESSORS + bool + config ARM_GIC bool select IRQ_DOMAIN diff --git a/include/linux/irq.h b/include/linux/irq.h index 03f48d9..ed1ea8a 100644 --- a/include/linux/irq.h +++ b/include/linux/irq.h @@ -639,6 +639,17 @@ void arch_teardown_hwirq(unsigned int irq); void irq_init_desc(unsigned int irq); #endif +#ifdef CONFIG_RAW_IRQ_ACCESSORS + +#ifndef irq_reg_writel +# define irq_reg_writel(val, addr) __raw_writel(val, addr) +#endif +#ifndef irq_reg_readl +# define irq_reg_readl(addr) __raw_readl(addr) +#endif + +#else + #ifndef irq_reg_writel # define irq_reg_writel(val, addr) writel(val, addr) #endif @@ -646,6 +657,8 @@ void irq_init_desc(unsigned int irq); # define irq_reg_readl(addr) readl(addr) #endif +#endif + /** * struct irq_chip_regs - register offsets for struct irq_gci * @enable: Enable register offset to reg_base -- 2.1.1 -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] irqchip: Allow irq_reg_{readl,writel} to use __raw_{readl_writel}
On Wed, 29 Oct 2014, Kevin Cernekee wrote:
Thomas:
How does that work with multi arch kernels?
I am assuming this question refers to e.g. CONFIG_ARCH_MULTIPLATFORM
If GENERIC_IRQ_CHIP is being used, the current implementation of
generic-chip.c will have to pick one global definition of
irq_reg_{readl,writel} for all supported SoCs.
One possibility is that individual irqchip drivers requiring special
accessors can pass in their own function pointers, similar to this:
struct sdhci_ops {
#ifdef CONFIG_MMC_SDHCI_IO_ACCESSORS
u32 (*read_l)(struct sdhci_host *host, int reg);
u16 (*read_w)(struct sdhci_host *host, int reg);
u8 (*read_b)(struct sdhci_host *host, int reg);
void (*write_l)(struct sdhci_host *host, u32 val, int reg);
void (*write_w)(struct sdhci_host *host, u16 val, int reg);
void (*write_b)(struct sdhci_host *host, u8 val, int reg);
#endif
and fall back to readl/writel if none are supplied. It would be nice
if this provided common definitions for the __raw_ and maybe the BE
variants too.
Or, we could add IRQ_GC_NATIVE_IO and/or IRQ_GC_BE_IO to enum irq_gc_flags.
I definitely prefer to have these options in the generic chip
implementation so we avoid that driver writers duplicate code in
creative and wrong ways.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] irqchip: Allow irq_reg_{readl,writel} to use __raw_{readl_writel}
On Wed, 29 Oct 2014, Arnd Bergmann wrote: On Wednesday 29 October 2014 13:09:47 Kevin Cernekee wrote: generic-chip.c already has a fair amount of indirection, with pointers to saved masks, user-specified register offsets, and such. Is there a concern that introducing, say, a pair of readl/writel function pointers, would cause an unacceptable performance drop? I don't know. Thomas' reply suggests that it isn't. Doing byteswap in software at a register access is usually free in terms of CPU cycles, but an indirect function call can be noticeable if we do that a lot. I did not say that it is free. I merily said that I prefer to have this solved at the core level rather than at the driver level. So you have several options to do so: 1) Indirections 2) Different functions for the different access modes 3) Alternatives #1 Is the simplest solution, but imposes the overhead of an indirect function call for something trivial #2 The most efficient and flexible way if you have to provide different access modes for different drivers. But it comes with the price of increasing the text foot print. #3 Smart and efficient, but requires that on a particular system all drivers use the same access mode. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/11] irqchip: Allow irq_reg_{readl,writel} to use __raw_{readl_writel}
On Wed, 29 Oct 2014, Arnd Bergmann wrote: Right. The option that I was explaining earlier basically combines #1 and #3: For all kernels on which we know the endianess of all generic-irqchip users at compile time, we hardcode that, and we use indirections of some sort for the cases where we build a kernel that needs both. Works for me. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 4/4] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
On Sun, 28 Sep 2014, Suravee Suthikulpanit wrote:
Jason/Thomas,
This patch comes from:
[V8 2/2] irqchip: gicv2m: Add supports for ARM GICv2m MSI(-X)
(https://lkml.org/lkml/2014/9/20/113)
It has been slightly modified to remove the multi-MSI supports for now
(I am waiting to discuss with Marc after he returned from vacation.), and will
be submitted separately.
Since this patch is independent from the multi-MSI stuff. Please let me know
if you would consider taking this separately.
Not without an explicit reviewed/acked from Marc for the GIC part and
a reviewed/acked from the DT folks.
Aside of that the conditional madness is just horrible:
+static inline
+struct gic_chip_data *irq_data_get_gic_chip_data(struct irq_data *d)
+{
+ struct gic_chip_data *gic_data;
+ struct msi_chip *mchip;
+ struct v2m_data *v2mdat;
+
+ /*
+* For MSI, irq_data.chip_data points to struct msi_chip.
+* For non-MSI, irq_data.chip_data points to struct gic_chip_data.
+*/
+ if (d-msi_desc) {
+ mchip = irq_data_get_irq_chip_data(d);
+ v2mdat = container_of(mchip, struct v2m_data, msi_chip);
+ gic_data = v2mdat-gic;
+ } else {
+ gic_data = irq_data_get_irq_chip_data(d);
+ }
+ return gic_data;
+}
For heavens sake, why are you insisting on duct-taping that into the
GIC proper instead of coming up with a proper layering?
https://lkml.org/lkml/2014/8/27/228
https://lkml.org/lkml/2014/8/26/707
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] [RFC] Using hierarchy irqdomian to implement MTK intpol.
On Thu, 25 Sep 2014, Joe.C wrote: From: Joe.C yingjoe.c...@mediatek.com Here's the first draft of using hierarchy irqdomain to implement MTK intpol support. I have tested it and intpol works fine. Before continue, I'd like to get your comments. This is based on Jiang's hierarchy irqdomian [1] and my mediatek SoC basic support [2]. Simplified block diagram for interrupt: - - ---| CIRQ |--|ARM GIC| ---| |--| | ---| |--| | ---| |--| | ---| |--| | - - In device tree, interrupt-parent for other devices is cirq, child of gic. This describe HW better and allow device to specify polarity as it is sent by the device. I'm using interrupt-parent to specify irq domain parent in cirq now. Maybe adding another name like 'interrupt-domain-parent' will be better. I leave that to the DT folks. Currently only set_type is implement in CIRQ, but I think this could extended to cover all function gic_arch_extn provided. Right. Marc, can you have a look at this whether you can see how that might replace the rest of the arch_extn hackery? At least the avoidance of the set_type mess looks pretty reasonable. In order to use hierarchy irq domain, gic can't use legacy irq domain anymore. If arm,hierarchy-irq-domain property is specified, GIC will work in hierarchy way and all interrupt must be set by device tree. One thing worth mention is arg in irq_domain_alloc_irqs. On x86, msi is using struct irq_alloc_info, to pass data between irq domain. In irq_create_of_mapping(), of_phandle_args is used. All allocs in irq_domain chain will need to use same interrupt-cells formats. Right, but we leave that to the architecture implementation. It needs to be consistent within an architecture, so we should change the void *arg to a well defined type. For OF architectures this would be of_phandle_args, other architectures can define their own struct type which allows to transport data between the domains. That ensures compile time type checking. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V8 1/2] irqchip: gic: Add support for multiple MSI for ARM64
On Tue, 23 Sep 2014, Suravee Suthikulpanit wrote: This patch implelments the ARM64 version of arch_setup_msi_irqs(), which does not return 1 for when PCI_CAP_ID_MSI and nvec 1. I can see that myself. What your changelog is missing is the reason WHY you think that copying that code from drivers/pci/msi.c and removing the PCI_CAP_ID_MSI and nvec 1 has any value. [Suravee] This is mainly be cause the weak version of arch_setup_msi_irqs() in the drivers/pci/msi.c doesn't support multi-MSI. Sorry for not being clear in the commit message. Groan. I asked you: WHY you think that copying that code from drivers/pci/msi.c and removing the PCI_CAP_ID_MSI and nvec 1 has any value. And your answer is that the function in drivers/pci/msi.c does not support Multi-MSI. Hell I know that myself. And there is a fricking good reason why allocating multi-MSI via for_each_msi() alloc_msi_irq(); is wrong. And while it might work by chance, there is no guarantee that it will work. It works for Multi-MSIX, but that has an additional X at the end and is a different beast when it comes to interrupts. I have no idea how crooked you are trying to work around that on the GIC side, but its going to be wrong and convoluted. Read and understand the MSI and MSI-X spec and the subtle differences of interrupt delivery. And if you groked that come back with a proper explanation why that patch makes sense or just go back to the drawing board and do it proper. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [V8 1/2] irqchip: gic: Add support for multiple MSI for ARM64
On Sat, 20 Sep 2014, suravee.suthikulpa...@amd.com wrote:
From: Suravee Suthikulpanit suravee.suthikulpa...@amd.com
This patch implelments the ARM64 version of arch_setup_msi_irqs(),
which does not return 1 for when PCI_CAP_ID_MSI and nvec 1.
I can see that myself. What your changelog is missing is the reason
WHY you think that copying that code from drivers/pci/msi.c and
removing the PCI_CAP_ID_MSI and nvec 1 has any value.
Signed-off-by: Suravee Suthikulpanit suravee.suthikulpa...@amd.com
Acked-by: Marc Zyngier marc.zyng...@arm.com
arch/arm64/kernel/Makefile | 1 +
arch/arm64/kernel/msi.c| 41 +
And that new function arm64_setup_msi_irqs is declared in which
header file exactly?
diff --git a/arch/arm64/kernel/msi.c b/arch/arm64/kernel/msi.c
new file mode 100644
index 000..a295862
--- /dev/null
+++ b/arch/arm64/kernel/msi.c
@@ -0,0 +1,41 @@
+/*
+ * ARM64 architectural MSI implemention
+ *
+ * Support for Message Signalelled Interrupts for systems that
+ * implement ARM Generic Interrupt Controller: GICv2m.
+ *
+ * Copyright (C) 2014 Advanced Micro Devices, Inc.
+ * Authors: Suravee Suthikulpanit suravee.suthikulpa...@amd.com
Copying stuff from A to B makes a real case for copyright, i.e. I'm
impressed by your ability to copy right.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include linux/irq.h
+#include linux/msi.h
+#include linux/pci.h
+
+/*
+ * ARM64 function for seting up MSI irqs.
+ * Based on driver/pci/msi.c: arch_setup_msi_irqs().
This is not based on. This is a verbatim copy with the omission of two
lines. Very creative that ...
+ *
+ * Note:
+ * Current implementation assumes that all interrupt controller used in
+ * ARM64 architecture _MUST_ supports multi-MSI.
Great assumption
+ */
+int arm64_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
What the heck is calling that code? The follow up patch does not and
due to lack of a declaration this patch is completely pointless.
So you add a new file with a pointless changelog and a boasting
copyright notice which adds completely useless functionality?
Well done.
At least you are consistent on the useless side of affairs:
+{
+ struct msi_desc *entry;
+ int ret;
+
+ list_for_each_entry(entry, dev-msi_list, list) {
+ ret = arch_setup_msi_irq(dev, entry);
Anyone who has the slightest idea how multi-MSI works will know that
this CANNOT work at all, but that's none of my business.
What's part of my business is to state that in my role as the
maintainer of all things related to interrupts this is the worst patch
I've seen in the last 10 years. Along with the saddening fact that it
carries the Acked-by of someone who should have known better.
At least if confirms my suspicion that ARM64 is a dignified successor
of the already infamous ARM32 universe.
I really can't bear the suspension to see the first 1500+ vendor patch
series of dubious quality supporting a real ARM64.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Thu, 18 Sep 2014, Nishanth Menon wrote:
On 17:57-20140918, Thomas Gleixner wrote:
I suppose I can improve the commit message to elaborate this better?
Will that help?
You also want to improve the comment in the empty handler.
+ */
+ return IRQ_NONE;
And it still does not explain WHY you think that returning IRQ_NONE is
the right thing to do here. You actually handle the interrupt, right?
Just because the handler is an NOP does not mean you did not handle
it.
+static int palmas_i2c_suspend(struct i2c_client *i2c, pm_message_t mesg)
+{
+ struct palmas *palmas = i2c_get_clientdata(i2c);
+ struct device *dev = i2c-dev;
+
+ if (!palmas-wakeirq)
+ return 0;
+
+ if (device_may_wakeup(dev))
+ enable_irq(palmas-wakeirq);
+
+ return 0;
+}
+
+static int palmas_i2c_resume(struct i2c_client *i2c)
+{
+ struct palmas *palmas = i2c_get_clientdata(i2c);
+ struct device *dev = i2c-dev;
+
+ if (!palmas-wakeirq)
+ return 0;
+
+ if (device_may_wakeup(dev))
+ disable_irq_nosync(palmas-wakeirq);
Again, why nosync?
true - nosync is not necessary at here. disable_irq is however necessary
as we are not interested in wakeup events for level changes.
We just use the enable/disable to control when we'd want to arm the pin
for waking up from suspend state.
And what is issuing the call to enable/disable_irq_wake()?
So if that interrupt is not marked proper then you can bring your
device into a wont resume state easily
start suspend
enable wakeirq
disable_device_irqs()
if (!iswakeup_irq())
disable_irq() // does not mask due to lazy masking
wakeirq fires
if (irq_is_disabled())
mask_irq();
transition into suspend
Now your pinctrl irq is masked at the HW level and wont wake the
machine up ever again.
So now looking at that pinctrl irq chip thing, which seems to be
designed to handle these kind of wakeups. That thing looks massivly
wrong as well, simply because it enforces to use
enable_irq/disable_irq().
So because the sole purpose of this chip is to handle the separate
wakeup style interrupt, it should actually NOT enable the interrupt in
the irq_unmask callback.
Simply because during normal operation nothing is interested in the
interrupt and any operation which might enable it (including request
irq) is just making the system handle completely pointless interrupts
and hoops and loops juggling with enable/disable irq.
So the right thing here is to have an empty unmask function and do the
actual unmask only in the irq_set_wake() callback. mask of course
needs to do what it says. The point is, that the following sequence of
code will just work w/o generating an interrupt on the wakeirq line
outside of the wake enabled context.
dev_init()
request_wakeirq();
suspend()
if (may_wake())
enable_irq_wake();
resume()
if (may_wake())
disable_irq_wake();
The other omap drivers using this have the same issue ... And of
course they are subtly different.
The uart one handles the actual device interrupt, which is violating
the general rule of possible interrupt reentrancy in the pm-runtime
case if the two interrupts are affine to two different cores. Yes,
it's protected by a lock and works by chance
The mmc one issues a disable_irq_nosync() in the wakeup irq handler
itself.
WHY does one driver need that and the other does not? You are not even
able to come up with a common scheme for OMAP. I don't want to see the
mess others are going to create when this stuff becomes more used.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Fri, 19 Sep 2014, Nishanth Menon wrote: On 08:37-20140919, Thomas Gleixner wrote: The other omap drivers using this have the same issue ... And of course they are subtly different. The uart one handles the actual device interrupt, which is violating the general rule of possible interrupt reentrancy in the pm-runtime case if the two interrupts are affine to two different cores. Yes, it's protected by a lock and works by chance The mmc one issues a disable_irq_nosync() in the wakeup irq handler itself. WHY does one driver need that and the other does not? You are not even able to come up with a common scheme for OMAP. I don't want to see the mess others are going to create when this stuff becomes more used. Thanks, tglx I think I understand your concern - I request Tony to comment about this. I mean, I can try and hook things like uart in other drivers (like https://patchwork.kernel.org/patch/4759171/ ), but w.r.t overall generic usage guideline wise, I would prefer Tony to comment. No, the uart and that i2c thing are just wrong. Assume the following device irq affine to cpu0 wakeup irq affine to cpu1 CPU 0 CPU 1 runtime suspend enable_wake(wakeup irq); wakeup interrupt is raised device interrupt is raised dev_handler(device) dev_handler(device) It might work due to locking, but it is nevertheless wrong. Interrupt handlers for devices are guaranteed not to be reentrant. And this brilliant stuff simply violates that guarantee. So, no. It's wrong even if it happens to work by chance. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Fri, 19 Sep 2014, Tony Lindgren wrote:
* Thomas Gleixner t...@linutronix.de [140919 10:37]:
From hardware point of view the wake-up events behave like interrupts
and could also be used as the only interrupt in some messed up cases.
That avoids all kinds of custom APIs from driver point.
The re-entrancy problem we've most likely had ever since we enabled
the PRCM interrupts, and maybe that's why I did not even consider
that part. I think before that we were calling the driver interrupt
after waking up from the PM code..
Anyways, how about the following to deal with the re-entrancy problem:
1. The wake-up interrupt handler must have a separate interrupt
handler that just calls tasklet_schedule()
2. The device interrupt handler also just calls tasklet_schedule()
3. The tasklet then does pm_runtime_get, handles the registers, and
so on.
Or would we still have a re-entrancy problem somewhere else with
that?
Why on earth are you wanting tasklets in there? That's just silly,
really.
The wakeup handler is supposed to bring the thing out of deep sleep
and nothing else. All you want it to do is to mask itself and save the
information that the real device irq is pending.
A stub handler for the wakeup irq is enough. We can have that in the
irq/pm core and all it would do is simply:
irqreturn_t handle_jinxed_wakeup_irq(unsigned irq, void *dev_id)
{
unsigned device_irq = get_dev_irq(dev_id);
force_mask(irq);
set_irq_pending(device_irq);
return HANDLED;
}
So on resume_device_irqs() the real device interrupt gets reenabled
and unmasked (if it was masked) and the interrupt gets resent either
in hardware (level or retrigger) or by the software resend mechanism.
That completely avoids tasklets, reentrant irq handlers and all other
crap which might be required.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Fri, 19 Sep 2014, Tony Lindgren wrote: * Thomas Gleixner t...@linutronix.de [140919 12:47]: Why on earth are you wanting tasklets in there? That's just silly, really. Lack of a framework on driver side to cope with this in a generic way? :p So instead of creating such a thing we rather have a completely ass backward workaround which spreads itself all over the tree? You SoC folks really need a quarterly sanity check. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 3/3] mfd: palmas: Add support for optional wakeup
On Thu, 18 Sep 2014, Nishanth Menon wrote:
+static irqreturn_t palmas_wake_irq(int irq, void *_palmas)
+{
+ /*
+ * Return Not handled so that interrupt is disabled.
And how is that interrupt disabled by returning IRQ_NONE? You mean it
gets disabled after it got reraised 10 times and the spurious
detector kills it?
+ * Level event ensures that the event is eventually handled
+ * by the appropriate chip handler already registered
Eventually handled? So eventually it's not handled?
+ */
+ return IRQ_NONE;
+}
+
int palmas_ext_control_req_config(struct palmas *palmas,
enum palmas_external_requestor_id id, int ext_ctrl, bool enable)
{
@@ -409,6 +420,7 @@ static void palmas_dt_to_pdata(struct i2c_client *i2c,
pdata-mux_from_pdata = 1;
pdata-pad2 = prop;
}
+ pdata-wakeirq = irq_of_parse_and_map(node, 1);
/* The default for this register is all masked */
ret = of_property_read_u32(node, ti,power-ctrl, prop);
@@ -521,6 +533,7 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
i2c_set_clientdata(i2c, palmas);
palmas-dev = i2c-dev;
palmas-irq = i2c-irq;
+ palmas-wakeirq = pdata-wakeirq;
match = of_match_device(of_palmas_match_tbl, i2c-dev);
@@ -587,6 +600,25 @@ static int palmas_i2c_probe(struct i2c_client *i2c,
if (ret 0)
goto err_i2c;
+ if (!palmas-wakeirq)
+ goto no_wake_irq;
+
+ ret = devm_request_irq(palmas-dev, palmas-wakeirq,
+palmas_wake_irq,
+pdata-irq_flags,
+dev_name(palmas-dev),
+palmas);
+ if (ret 0) {
+ dev_err(palmas-dev, Invalid wakeirq(%d) (res: %d), skiping\n,
+ palmas-wakeirq, ret);
+ palmas-wakeirq = 0;
+ } else {
+ /* We use wakeirq only during suspend-resume path */
+ device_set_wakeup_capable(palmas-dev, true);
+ disable_irq_nosync(palmas-wakeirq);
Urgh. Why nosysnc? And why do you want to do that at all?
irq_set_status_flags(irq, IRQ_NOAUTOEN);
Is what you want to set before requesting the irq.
+ }
+
+no_wake_irq:
no_irq:
slave = PALMAS_BASE_TO_SLAVE(PALMAS_PU_PD_OD_BASE);
addr = PALMAS_BASE_TO_REG(PALMAS_PU_PD_OD_BASE,
@@ -706,6 +738,34 @@ static int palmas_i2c_remove(struct i2c_client *i2c)
return 0;
}
+static int palmas_i2c_suspend(struct i2c_client *i2c, pm_message_t mesg)
+{
+ struct palmas *palmas = i2c_get_clientdata(i2c);
+ struct device *dev = i2c-dev;
+
+ if (!palmas-wakeirq)
+ return 0;
+
+ if (device_may_wakeup(dev))
+ enable_irq(palmas-wakeirq);
+
+ return 0;
+}
+
+static int palmas_i2c_resume(struct i2c_client *i2c)
+{
+ struct palmas *palmas = i2c_get_clientdata(i2c);
+ struct device *dev = i2c-dev;
+
+ if (!palmas-wakeirq)
+ return 0;
+
+ if (device_may_wakeup(dev))
+ disable_irq_nosync(palmas-wakeirq);
Again, why nosync?
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 01/16] MIPS: Provide a generic plat_irq_dispatch
On Fri, 5 Sep 2014, Andrew Bresticker wrote:
For platforms which boot with device-tree and use the MIPS CPU interrupt
controller binding, a generic plat_irq_dispatch() can be used since all
CPU interrupts should be mapped through the CPU IRQ domain. Implement a
plat_irq_dispatch() which simply handles the highest pending interrupt.
Signed-off-by: Andrew Bresticker abres...@chromium.org
Tested-by: Jonas Gorski j...@openwrt.org
---
No changes from v1.
---
arch/mips/kernel/irq_cpu.c | 28 +++-
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/arch/mips/kernel/irq_cpu.c b/arch/mips/kernel/irq_cpu.c
index e498f2b..9cf8459 100644
--- a/arch/mips/kernel/irq_cpu.c
+++ b/arch/mips/kernel/irq_cpu.c
@@ -116,6 +116,24 @@ void __init mips_cpu_irq_init(void)
}
#ifdef CONFIG_IRQ_DOMAIN
+static struct irq_domain *mips_intc_domain;
+
+asmlinkage void __weak plat_irq_dispatch(void)
+{
+ unsigned int pending = read_c0_cause() read_c0_status() ST0_IM;
+ unsigned int hw;
+ int irq;
+
+ if (!pending) {
+ spurious_interrupt();
+ return;
+ }
+
+ hw = fls(pending) - CAUSEB_IP - 1;
+ irq = irq_linear_revmap(mips_intc_domain, hw);
+ do_IRQ(irq);
Why are you not handling all pending interrupts in a loop?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 14/16] irqchip: mips-gic: Support local interrupts
On Fri, 5 Sep 2014, Andrew Bresticker wrote:
static void gic_mask_irq(struct irq_data *d)
{
- GIC_CLR_INTR_MASK(d-irq - gic_irq_base);
+ unsigned int irq = d-irq - gic_irq_base;
+
+ if (gic_is_local_irq(irq)) {
+ GICWRITE(GIC_REG(VPE_LOCAL, GIC_VPE_RMASK),
+ 1 GIC_INTR_BIT(gic_hw_to_local_irq(irq)));
+ } else {
+ GIC_CLR_INTR_MASK(irq);
+ }
}
static void gic_unmask_irq(struct irq_data *d)
{
- GIC_SET_INTR_MASK(d-irq - gic_irq_base);
+ unsigned int irq = d-irq - gic_irq_base;
+
+ if (gic_is_local_irq(irq)) {
+ GICWRITE(GIC_REG(VPE_LOCAL, GIC_VPE_SMASK),
+ 1 GIC_INTR_BIT(gic_hw_to_local_irq(irq)));
+ } else {
+ GIC_SET_INTR_MASK(irq);
+ }
Why are you adding a conditional in all these functions instead of
having two interrupt chips with separate callbacks and irqdata?
And looking at GIC_SET_INTR_MASK(irq) makes me shudder even more. The
whole thing can be replaced with the generic interrupt chip functions.
If you set it up proper, then there is not a single conditional or
runtime calculation of bitmasks, address offsets etc.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 15/16] MIPS: GIC: Use local interrupts for timer
On Fri, 5 Sep 2014, Andrew Bresticker wrote:
Instead of using GIC interrupt 0 for the timer (which was not even
handled correctly by the GIC irqchip code and could conflict with an
actual external interrupt), use the designated local interrupt for
the GIC timer.
Also, since the timer is a per-CPU interrupt, initialize it with
setup_percpu_irq() and enable it with enable_percpu_irq() instead
of using direct register writes.
Signed-off-by: Andrew Bresticker abres...@chromium.org
---
No changes from v1.
---
arch/mips/kernel/cevt-gic.c | 16 +++-
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/arch/mips/kernel/cevt-gic.c b/arch/mips/kernel/cevt-gic.c
index 6093716..cae72a4 100644
--- a/arch/mips/kernel/cevt-gic.c
+++ b/arch/mips/kernel/cevt-gic.c
@@ -68,7 +68,7 @@ int gic_clockevent_init(void)
if (!cpu_has_counter || !gic_frequency)
return -ENXIO;
- irq = MIPS_GIC_IRQ_BASE;
+ irq = MIPS_GIC_LOCAL_IRQ_BASE + GIC_LOCAL_INTR_COMPARE;
cd = per_cpu(gic_clockevent_device, cpu);
@@ -91,15 +91,13 @@ int gic_clockevent_init(void)
clockevents_register_device(cd);
- GICWRITE(GIC_REG(VPE_LOCAL, GIC_VPE_COMPARE_MAP), 0x8002);
- GICWRITE(GIC_REG(VPE_LOCAL, GIC_VPE_SMASK), GIC_VPE_SMASK_CMP_MSK);
+ if (!gic_timer_irq_installed) {
+ setup_percpu_irq(irq, gic_compare_irqaction);
+ irq_set_handler(irq, handle_percpu_irq);
+ gic_timer_irq_installed = 1;
+ }
- if (gic_timer_irq_installed)
- return 0;
+ enable_percpu_irq(irq, 0);
Please use a proper IRQ_TYPE constant instead of 0
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding
On Fri, 5 Sep 2014, Florian Fainelli wrote: On 09/05/2014 12:21 PM, Thomas Gleixner wrote: So if I understand correctly what you have is: /- GIC- Device-irq [routing] \- BC irq chip and you implement it as Device-irq [BC irq chip] [GIC] --- | - And the fwd mask is to tell the BC chip to use the GIC and which irq of the GIC, so it can fiddle with the GIC under the hood, right? The forward mask really is to tell the BCM7120 l2 interrupt controller: bypass me, and output the UART interrupts directly at the GIC level, so I think this does match your understanding. Not setting the forward mask means you would get the UART interrupts at the BCM7120 l2 interrupt controller level, and have to handle them here. Hope this helps clarify what this funky piece of hardware does. Sigh, this stacked interrupt chip nonsense is becoming a plague. So if you set that bit then the UART driver only sees the GIC as its interrupt controller and not the L2 thingy. So, the L2 chip only enables its interrupt unconditionally for that line and the enable/disable happens at the GIC level. If that's the case, that's fine with me. It's not pretty, but at least it does not involve L2 fiddling indirectly with the GIC. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 3/3] mfd: palmas: Add support for optional wakeup
On Fri, 5 Sep 2014, Nishanth Menon wrote: + if (!palmas-wakeirq) + goto no_wake_irq; + + ret = devm_request_irq(palmas-dev, palmas-wakeirq, +palmas_wake_irq, +IRQF_ONESHOT | pdata-irq_flags, Why is this marked IRQF_ONESHOT? +dev_name(palmas-dev), +palmas); + if (ret 0) + goto err_i2c; Why err and not doing the obvious clearing of palmas-wakeirq and keep at least the i2c functional? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Documentation: bcm7120-l2: Add Broadcom BCM7120-style L2 binding
You forgot to CC the device tree dudes. We want an ack on the bindings
before they materialize in Linus tree.
Thanks,
tglx
On Thu, 28 Aug 2014, Florian Fainelli wrote:
This patch adds the Device Tree binding document for the Broadcom
BCM7120-style Set-top-box Level 2 interrupt controller hardware.
Signed-off-by: Florian Fainelli f.faine...@gmail.com
---
.../interrupt-controller/brcm,bcm7120-l2-intc.txt | 44
++
1 file changed, 44 insertions(+)
create mode 100644
Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt
b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt
new file mode 100644
index ..3818ffed7347
--- /dev/null
+++
b/Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7120-l2-intc.txt
@@ -0,0 +1,44 @@
+Broadcom BCM7120-style Level 2 interrupt controller
+
+Required properties:
+
+- compatible: should be brcm,bcm7120-l2-intc
+- reg: specifies the base physical address and size of the registers
+- interrupt-controller: identifies the node as an interrupt controller
+- #interrupt-cells: specifies the number of cells needed to encode an
interrupt
+ source, should be 1.
+- interrupt-parent: specifies the phandle to the parent interrupt controller
+ this one is cascaded from
+- interrupts: specifies the interrupt line(s) in the interrupt-parent domain
+ to be used for cascading
+- brcm,int-map-mask: 32-bits bit mask describing how the interrupts at this
level
+ map to their respective parents. Should match exactly the number of
interrupts
+ specified in the 'interrupts' property.
+
+Optional properties:
+
+- interrupt-names: if present, the litteral names for the parent interrupts
+ specified in the 'interrupts' property.
+
+- brcm,irq-can-wake: if present, this means the L2 controller can be used as
a
+ wakeup source for system suspend/resume.
+
+- brcm,int-fwd-mask: if present, a 32-bits bit mask describing the interrupts
+ which need to be enabled in this controller to flow to the higher level
+ interrupt controller. This is typically needed for the UARTs interrupts to
+ flow through the top-level interrupt controller (e.g: ARM GIC on ARM-based
+ platforms).
+
+Example:
+
+irq0_intc: interrupt-controller@f0406800 {
+ compatible = brcm,bcm7120-l2-intc;
+ interrupt-parent = intc;
+ #interrupt-cells = 1;
+ reg = 0xf0406800 0x8;
+ interrupt-controller;
+ interrupts = 0x0 0x42 0x0, 0x0 0x40 0x0;
+ interrupt-names = upg_main, upg_bsc;
+ brcm,int-map-mask = 0xeb8, 0x140;
+ brcm,int-fwd-mask = 0x7;
+};
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present
On Wed, 27 Aug 2014, Marc Zyngier wrote:
As in the DT the actual IRQ polarity should be used, simply configuring
the HW IRQ polarity in the bootloader is not enough without telling the
GIC driver which polarity is supported on which IRQ, right?
Looking a bit closer at things, what you describe in DT is the IRQ
polarity the interrupt controller sees. Nothing else should interpret
that field.
So it is legal (IMO) to have a device with an interrupt specifier
describing a rising edge interrupt, and yet have the device generating a
falling edge, with Mediatek's special sauce doing the conversion in between.
Something will have to configure the polarity widget though, but that
can be left outside of the GIC.
This seems to become a popular topic and it looks like the whole GIC
extension thing is going to explode sooner than later.
We are currently discussing hierarchical irq domains to solve a
different issue in x86 land. See the related discussion here:
https://lkml.org/lkml/2014/8/1/67
Now looking at these GIC plus extra sauce problems, I wonder whether
this wouldn't be solvable in a similar way. If you look at it from the
HW perspective you have:
- -
---| MAGIC |--|ARM GIC|
---| |--| |
---| |--| |
---| |--| |
---| |--| |
- -
The MAGIC interrupt controller only provides functionality which is
not available from the ARM architected GIC but maps 1:1 to the ARM GIC
interrupt lines. So it looks like a variation to the more dynamic
mapping of MSI - Remap - CPU-Vector problem we need to solve on x86.
The idea is to have two irq domains: magic_domain and armgic_domain.
The magic_domain is the frontend facing the devices and the
armgic_domain is the parent domain. This is also reflected in
hierarchical data structures, i.e. irq_desc-irq_data will get a new
field parent_data, which points to the irq_data of the parent
interrupt controller, which is allocated separately when the interrupt
line is instantiated.
So in the above case the hotpath ack/eoi functions of the irq chip
associated to the device interrupt, i.e. magic_chip, would do:
irq_ack(struct irq_data *d)
{
struct irq_data *pd = d-parent_data;
pd-chip-irq_ack(pd);
}
Granted, that's another level of indirection, but this is going to be
more efficient than a boatload of conditional hooks in the GIC code
proper. Not to talk about the maintainability of the whole maze.
The irq_set_type() function would do:
irq_set_type(struct irq_data *d, int type)
{
struct irq_data *pd = d-parent_data;
gic_type = set_magic_chip_type(d, type);
return pd-chip-irq_set_type(d, gic_type);
}
Switching to this allows to completely avoid the gazillion of hooks in
the gic code and should work nicely with multiplatform kernels by
simpling hooking up the domain parent relation ship to the proper
magic domain or leave the armgic as the direct device interface in
case the SoC does not have the magic chip in front of the arm gic.
Thoughts?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present
On Wed, 27 Aug 2014, Marc Zyngier wrote: I very much like that kind of approach. Stacking domains seems to solve a number of issues at once: - NVIDIA's gic extension - TI's crossbar - ARM's GICv2m - Mediatek's glorified line inverter - ... and probably the next madness that's going to land tomorrow Its probably already there you just don't know about it yet :) I haven't completly groked the way we're going to allocate domains and irq_data structures, but this is definitely something worth All we have for now is a rough idea and some pseudo code in my lengthy reply to Jiang, but it shouldn't be hard to implement something useful. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v2 1/4] irqchip: gic: Change irq type check when extension is present
On Wed, 27 Aug 2014, Mark Rutland wrote:
On Wed, Aug 27, 2014 at 02:37:44PM +0100, Marc Zyngier wrote:
We need to work out how to drive the whole stacking from a DT
perspective: Mark, any idea?
Describing the lines the magic irqchip has to its parent irqchip is
simple, the standard interrupts property can handle that:
parent: parent {
...
#interrupt-cells = 1;
};
magic {
...
interrupt-parent = parent;
interrupts = 3, 6, 17, 2;
#interrupt-cells = 1;
};
The harder part is describing the configuration of each interrupt to the
magic irqchip (e.g. edge vs level triggered), which is inseparable form
the line identifier in the interrupt-specifier.
Do we really need to decribe that at the this level?
You have the parent ARMGIC and the maGIC with a specified (or even
dynamic) routing of the maGIC lines to the ARMGIC.
Now the device which uses a maGIC interrupt specifies the interrupt
type at the maGIC.
So the type setter function of the maGIC configures the maGIC hardware
in such a way that the output to the ARMGIC results in a type which
the ARMGIC can handle and calls the type setter function of the maGIC
with that type.
I don't think you need to decribe this in the magic/parent relation
ship at all. maGIC should know what kind of output it can provide to
ARMGIC so that should just work, unless I'm missing something.
If there interrupts don't share the same configuration then I'm not sure
how we describe that in the DT. That's especially problematic if the
assignment of parent line is dynamic (as with the crossbar iirc).
Why is this an issue?
There are two ways to solve that:
1) You can do a fully dynamic allocation at the parent level, which of
course requires that the whole parent range is dynamically
managed. That's what we are planning for the MSI/Remap/Vector
stacking
2) You define the irq lines at the parent which are available for
dynamic stacking and let the allocation code hand one out.
3) You tell the maGIC controller which lines of the parent are
available for it, so it can only stack onto those lines and instead
of letting the parent allocate a free one, the maGIC needs to map
its stuff to one of those ARMGIC lines.
Which one to chose depends on the particular maGIC incarnation, but
its not a big issue to select one ...
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 2/2] dts: spear: Add missing i2c1 interrupt
On Fri, 20 Jun 2014, Viresh Kumar wrote:
On Fri, Jun 20, 2014 at 3:26 AM, Thomas Gleixner t...@linutronix.de wrote:
Signed-off-by: Thomas Gleixner t...@linutronix.de
---
arch/arm/boot/dts/spear320.dtsi |5 +
1 file changed, 5 insertions(+)
Index: linux/arch/arm/boot/dts/spear320.dtsi
===
--- linux.orig/arch/arm/boot/dts/spear320.dtsi
+++ linux/arch/arm/boot/dts/spear320.dtsi
@@ -123,6 +123,11 @@
status = disabled;
};
+ i2c1: i2c@a700 {
+ interrupts = 21;
+ interrupt-parent = shirq;
+ };
Isn't this already available ?
i2c1: i2c@a700 {
#address-cells = 1;
#size-cells = 0;
compatible = snps,designware-i2c;
reg = 0xa700 0x1000;
interrupts = 21;
interrupt-parent = shirq;
status = disabled;
};
Oops yes, did this against an older tree and did not notice the
duplicate. So nothing to do here.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 1/2] dts: spear: Fix sdhci compatible
Make it match the bindings and the implementation.
Signed-off-by: Thomas Gleixner t...@linutronix.de
---
arch/arm/boot/dts/spear320.dtsi |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Index: linux/arch/arm/boot/dts/spear320.dtsi
===
--- linux.orig/arch/arm/boot/dts/spear320.dtsi
+++ linux/arch/arm/boot/dts/spear320.dtsi
@@ -48,7 +48,7 @@
};
sdhci@7000 {
- compatible = st,sdhci-spear;
+ compatible = st,spear300-sdhci;
reg = 0x7000 0x100;
interrupts = 10;
interrupt-parent = shirq;
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 0/2] dt: spear320: Make it work
The sdhci compatible does neither match the documentation nor the implementaion in the drivers and the i2c1 is missing an interrupt assignment. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[patch 2/2] dts: spear: Add missing i2c1 interrupt
Signed-off-by: Thomas Gleixner t...@linutronix.de
---
arch/arm/boot/dts/spear320.dtsi |5 +
1 file changed, 5 insertions(+)
Index: linux/arch/arm/boot/dts/spear320.dtsi
===
--- linux.orig/arch/arm/boot/dts/spear320.dtsi
+++ linux/arch/arm/boot/dts/spear320.dtsi
@@ -123,6 +123,11 @@
status = disabled;
};
+ i2c1: i2c@a700 {
+ interrupts = 21;
+ interrupt-parent = shirq;
+ };
+
gpiopinctrl: gpio@b300 {
compatible = st,spear-plgpio;
reg = 0xb300 0x1000;
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/7] clocksource: Add support for the Mediatek SoCs
On Sun, 15 Jun 2014, Daniel Lezcano wrote:
On 06/11/2014 08:14 PM, Thomas Gleixner wrote:
On Wed, 11 Jun 2014, Matthias Brugger wrote:
+static void mtk_clkevt_mode(enum clock_event_mode mode,
+ struct clock_event_device *clk)
+{
+ struct mtk_clock_event_device *evt = to_mtk_clk(clk);
+
+ mtk_clkevt_time_stop(evt, GPT_CLK_EVT);
+
+ switch (mode) {
+ case CLOCK_EVT_MODE_PERIODIC:
+ mtk_clkevt_time_setup(evt, evt-ticks_per_jiffy, GPT_CLK_EVT);
+ mtk_clkevt_time_start(evt, true, GPT_CLK_EVT);
+ break;
+ case CLOCK_EVT_MODE_ONESHOT:
+ mtk_clkevt_time_start(evt, false, GPT_CLK_EVT);
Why start the timer here? The code will call set next event right away.
+ break;
+ case CLOCK_EVT_MODE_UNUSED:
+ case CLOCK_EVT_MODE_SHUTDOWN:
+ default:
+ /* No more interrupts will occur as source is disabled */
+ break;
+ }
+}
Looks good otherwise.
Hi Thomas,
Can I consider the 8.1 patch (the one taking into account your comment) as
acked-by ?
Yes
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 00/19] irqchip: crossbar: driver fixes
On Fri, 13 Jun 2014, Jason Cooper wrote: On Fri, Jun 13, 2014 at 09:48:24AM -0700, Joe Perches wrote: On Fri, 2014-06-13 at 12:37 -0400, Jason Cooper wrote: On Fri, Jun 13, 2014 at 09:14:34AM -0700, Joe Perches wrote: On Fri, 2014-06-13 at 11:01 -0400, Jason Cooper wrote: Please format the subject lines like so: irqchip: crossbar: Set cb pointer ... ^ | \-- note the capitalization I suggest you don't make this a rule and focus on more important stuff instead. Says the one, who pesters people about typos in changelogs WTF? Jason is doing a great job in reviewing and he knows what to concentrate on. It's not my rule. ;-) Who's rule is it then? Thomas' Sentences start with an upper case letter. Our brain is trained on that rule when parsing a line. So for people who actually review patches by reading them instead of running a spell checker, consistent formatting more important than avoiding the random typo, which our brain just blends out in most of the cases. Unfortunately also when the typo is in actual code :( Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 2/7] clocksource: Add support for the Mediatek SoCs
On Wed, 11 Jun 2014, Matthias Brugger wrote:
+static void mtk_clkevt_mode(enum clock_event_mode mode,
+ struct clock_event_device *clk)
+{
+ struct mtk_clock_event_device *evt = to_mtk_clk(clk);
+
+ mtk_clkevt_time_stop(evt, GPT_CLK_EVT);
+
+ switch (mode) {
+ case CLOCK_EVT_MODE_PERIODIC:
+ mtk_clkevt_time_setup(evt, evt-ticks_per_jiffy, GPT_CLK_EVT);
+ mtk_clkevt_time_start(evt, true, GPT_CLK_EVT);
+ break;
+ case CLOCK_EVT_MODE_ONESHOT:
+ mtk_clkevt_time_start(evt, false, GPT_CLK_EVT);
Why start the timer here? The code will call set next event right away.
+ break;
+ case CLOCK_EVT_MODE_UNUSED:
+ case CLOCK_EVT_MODE_SHUTDOWN:
+ default:
+ /* No more interrupts will occur as source is disabled */
+ break;
+ }
+}
Looks good otherwise.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 01/21] irq: add devres version of OF IRQ mapping routines
On Wed, 4 Jun 2014, nyushche...@dev.rtsoft.ru wrote: From: Nikita Yushchenko nyushche...@dev.rtsoft.ru Many drivers use devres to manage their resources, and at the same time use irq_of_parse_and_map() / irq_dispose_mapping(). This creates problem on driver unload paths and on error paths: - it is invalid to call irq_dispose_mapping() while IRQ handler is still installed, - devres moves removal of IRQ handler out of driver, - without explicit devres support for IRQ mapping, irq_dispose_mapping() stays in driver and thus gets called while IRQ handler is still installed. This patch adds devm_irq_create_of_mapping() and devm_irq_of_parse_and_map() routines to be used by drivers for correct release of resources. Signed-off-by: Nikita Yushchenko nyushche...@dev.rtsoft.ru Reviewed-by: Thomas Gleixner t...@linutronix.de -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 00/14] irqchip: crossbar: driver fixes
On Tue, 3 Jun 2014, Sricharan R wrote: Please Cc all maintainers on such changes plus the relevant mailinglist. See MAINTAINERS. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/2] irqchip: add Broadcom Set Top Box Level-2 interrupt controller
On Thu, 22 May 2014, Florian Fainelli wrote:
+static void brcmstb_l2_intc_irq_handle(unsigned int irq, struct irq_desc
*desc)
+{
+ struct brcmstb_l2_intc_data *b = irq_desc_get_handler_data(desc);
+ struct irq_chip *chip = irq_get_chip(irq);
irq_desc_get_chip() is what you want here. irq_get_chip() is doing a
full lookop of desc, which is silly as you have desc already.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/3] ARM: sun7i/sun6i: irqchip: Irqchip driver for NMI controller
On Sat, 15 Mar 2014, Carlo Caione wrote: Allwinner A20/A31 SoCs have a special interrupt controller for managing NMI. Three register are present to (un)mask, control and acknowledge NMI. These two patches add a new irqchip driver in cascade with GIC. If I get an ack for the DT parts, I'll pick it up. -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 0/3] ARM: sun7i/sun6i: irqchip: Irqchip driver for NMI controller
On Wed, 19 Mar 2014, Maxime Ripard wrote: On Wed, Mar 19, 2014 at 12:13:56PM +0100, Thomas Gleixner wrote: On Sat, 15 Mar 2014, Carlo Caione wrote: Allwinner A20/A31 SoCs have a special interrupt controller for managing NMI. Three register are present to (un)mask, control and acknowledge NMI. These two patches add a new irqchip driver in cascade with GIC. If I get an ack for the DT parts, I'll pick it up. I had some comments on it, so Carlo will probably resubmit it. That's the resubmit as far as I can tell. -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] irqchip: sun4i: Don't mask + unmask for the non oneshot case
On Thu, 13 Mar 2014, Hans de Goede wrote:
Since sun4i and sun5i are single core SOCs there is no need to mask non
oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.
This is slightly wrong :)
Even on a SMP system there is no need to mask the interrupt when the
controller works like that sunxi one. If the controller is not broken
beyond repair then it does not deliver the same interrupt to a
different cpu. Such a thing would always deliver it to all cores and
those would race to grab the spinlock and mask it. I've seen such
horror, but don't ask how that performs.
The reason why you can spare the mask/unmask dance is that the
controller does not require any action and clears the interrupt when
the level goes back to inactive. That happens when the device handler
acks it at the device level.
Now there might be the case when the device reactivates the interrupt
before the RETI. But that does not matter as we run the primary
interrupt handlers with interrupts disabled.
Thanks,
tglx
Signed-off-by: Hans de Goede hdego...@redhat.com
---
drivers/irqchip/irq-sun4i.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index a0ed1ea..0a71990 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -74,8 +74,17 @@ static void sun4i_irq_unmask(struct irq_data *irqd)
sun4i_irq_base + SUN4I_IRQ_ENABLE_REG(reg));
}
+/*
+ * Since sun4i and sun5i are single core SOCs there is no need to mask non
+ * oneshot IRQs, to achieve this we use handle_fasteoi_irq with a dummy eoi.
+ */
+static void sun4i_irq_dummy_eoi(struct irq_data *irqd)
+{
+}
+
static struct irq_chip sun4i_irq_chip = {
.name = sun4i_irq,
+ .irq_eoi= sun4i_irq_dummy_eoi,
.irq_mask = sun4i_irq_mask,
.irq_unmask = sun4i_irq_unmask,
};
@@ -97,7 +106,7 @@ static int sun4i_irq_map(struct irq_domain *d, unsigned
int virq,
handle_fasteoi_irq);
else
irq_set_chip_and_handler(virq, sun4i_irq_chip,
- handle_level_irq);
+ handle_fasteoi_irq);
set_irq_flags(virq, IRQF_VALID | IRQF_PROBE);
--
1.9.0
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)
On Thu, 13 Mar 2014, Maxime Ripard wrote:
On Wed, Mar 12, 2014 at 06:17:07PM +0100, Hans de Goede wrote:
The ENMI needs to have the ack done *after* clearing the interrupt source,
otherwise we will get a spurious interrupt for each real interrupt. Switch
to the new handle_fasteoi_late_irq handler which gives us the desired
behavior.
Signed-off-by: Hans de Goede hdego...@redhat.com
---
drivers/irqchip/irq-sun4i.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index 8a2fbee..4b1c874 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -77,15 +77,22 @@ static void sun4i_irq_unmask(struct irq_data *irqd)
static struct irq_chip sun4i_irq_chip = {
.name = sun4i_irq,
.irq_ack= sun4i_irq_ack,
+ .irq_eoi= sun4i_irq_ack, /* For the ENMI */
Hmmm, I wonder if that actually does something.
There's been a patch floating around that I was sure was merged, but
apparently wasn't that remove sun4i_irq_ack, because the register we
were writing to are in read only, and it wasn't doing anything.
Well, it looks like it does something otherwise Hans would not see any
improvement of the situation.
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)
On Wed, 12 Mar 2014, Hans de Goede wrote:
The ENMI needs to have the ack done *after* clearing the interrupt source,
otherwise we will get a spurious interrupt for each real interrupt. Switch
to the new handle_fasteoi_late_irq handler which gives us the desired
behavior.
Signed-off-by: Hans de Goede hdego...@redhat.com
---
drivers/irqchip/irq-sun4i.c | 11 +--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-sun4i.c b/drivers/irqchip/irq-sun4i.c
index 8a2fbee..4b1c874 100644
--- a/drivers/irqchip/irq-sun4i.c
+++ b/drivers/irqchip/irq-sun4i.c
@@ -77,15 +77,22 @@ static void sun4i_irq_unmask(struct irq_data *irqd)
static struct irq_chip sun4i_irq_chip = {
.name = sun4i_irq,
.irq_ack= sun4i_irq_ack,
+ .irq_eoi= sun4i_irq_ack, /* For the ENMI */
.irq_mask = sun4i_irq_mask,
.irq_unmask = sun4i_irq_unmask,
+ .flags = IRQCHIP_EOI_THREADED, /* Only affects the ENMI */
That's not really true. The flags affect all interrupts which share
that chip.
};
static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
irq_hw_number_t hw)
{
- irq_set_chip_and_handler(virq, sun4i_irq_chip,
- handle_level_irq);
+ if (hw == 0) /* IRQ 0, the ENMI needs special handling */
+ irq_set_chip_and_handler(virq, sun4i_irq_chip,
+ handle_fasteoi_late_irq);
+ else
+ irq_set_chip_and_handler(virq, sun4i_irq_chip,
+ handle_level_irq);
I wonder what happens when you use the fasteoi handler for all of
them.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/4] irqchip: sun4i: Use handle_fasteoi_late_irq for the ENMI (irq 0)
On Thu, 13 Mar 2014, Hans de Goede wrote:
On 03/13/2014 03:46 PM, Thomas Gleixner wrote:
static int sun4i_irq_map(struct irq_domain *d, unsigned int virq,
irq_hw_number_t hw)
{
- irq_set_chip_and_handler(virq, sun4i_irq_chip,
- handle_level_irq);
+ if (hw == 0) /* IRQ 0, the ENMI needs special handling */
+ irq_set_chip_and_handler(virq, sun4i_irq_chip,
+ handle_fasteoi_late_irq);
+ else
+ irq_set_chip_and_handler(virq, sun4i_irq_chip,
+ handle_level_irq);
I wonder what happens when you use the fasteoi handler for all of
them.
As mentioned in my previous mail doing an ack (or an eio) seems to
be unnecessary for all but IRQ 0.
I do wonder if handle_level_irq is the right handle*irq function
to use in this case, since this is strictly used in the non smp
case I think that the mask / unmask done by handle_level_irq is
not necessary for non threaded handlers. So what would be the
correct handle*irq function to use in this case ?
Note the irqs are level irqs. IOW they may stay asserted while
the handler runs because of the handler and a new irq raising.
Right. You could be creative and use fasteoi plus an empty eoi
callback in the chip for irq 1-N. That way you only mask and unmask in
the threaded case.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors
On Mon, 3 Mar 2014, Stephen Warren wrote:
From: Stephen Warren swar...@nvidia.com
Some devices have configurable IRQ output polarities. Software might
use IRQ_TYPE_* to determine how to configure such a device's IRQ
output polarity in order to match how the IRQ controller input is
configured. If the board or SoC inverts the signal between the
device's IRQ output and controller's IRQ output, software must be
aware of this fact, in order to program the IRQ output to the correct
(i.e. opposite) polarity. This flag provides that information.
So what you're saying is:
Device IRQ output -- [Optional Inverter Logic] -- IRQ controller input.
And you're storing the information about the presence of the inverter
logic in the irq itself, but the core does not make any use of it and
you let the device driver deal with the outcome.
This sucks as all affected drivers have to implement the same sanity
logic for this.
Why don't you implement a core function which tells the driver which
polarity to select? That requires a few more changes, but I think it's
worth it for other reasons.
Right now the set_type logic requires the irq chip drivers to
implement sanity checking and default selections for TYPE_NONE. We can
be more clever about that and add this information to the irq chip
flags.
enum {
IRQ_CHIP_TYPES_MASK= 0x0f,
IRQ_CHIP_DEFAULT_MASK = 0xf0,
IRQ_CHIP_EXISTING_FLAGS
}
Now the irq_chip setup tells the core which types are available and
which one is the default fallback for TYPE_NONE.
So the core can do the sanity checks and we can kill quite some
repeated stuff from the irq chip implementations. For the inverted
logic case you can handle the inversion in the core as well, i.e. if a
driver requests IRQ_TYPE_LEVEL_HIGH you select IRQ_TYPE_LEVEL_LOW for
the chip, if possible.
For the case where the irq chip can only handle a single polarity you
can provide a core function to figure out to which polarity the driver
should set the device IRQ output line.
int irq_get_device_irq_polarity(int irq, int device_types)
{
/*
* Handle the inversion logic and select a proper
* device irq polarity from irq_chip(@irq)-flags and
* @device_types.
*
* Return a proper error code if no match.
*/
}
Let's look at an example:
irq_chip.flags = IRQ_TYPE_LEVEL_HIGH;
device_types = IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW;
Now for the non inverted case, this returns IRQ_TYPE_LEVEL_HIGH, for
the inverted case it returns IRQ_TYPE_LEVEL_LOW.
In both cases the irq_set_type() logic handles this correctly:
Non-Inverted case:
irq_set_type(irq, IRQ_TYPE_LEVEL_HIGH);
- Success
Inverted case:
irq_set_type(irq, IRQ_TYPE_LEVEL_LOW);
invert - IRQ_TYPE_LEVEL_HIGH
- Success
To make this work for interrupt chips which have no set_type callback
we can do the following in irq_set_type():
if (irq_is_inverted(irq))
type = invert(type);
ret = irq_check_type(chip, type);
if (ret 0 || !chip-irq_settype)
return ret;
return chip-irq_settype();
And irq_check_type() does:
if (!(chip-flags IRQ_CHIP_TYPES_MASK))
return chip-irq_settype ? 0 : -ENOTSUP;
if (*type == IRQ_TYPE_NONE)
*type == (chip-flags IRQ_CHIP_DEFAULT_MASK) 4;
return type_supported(chip-flags, *type);
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors
On Tue, 4 Mar 2014, Thomas Gleixner wrote:
On Mon, 3 Mar 2014, Stephen Warren wrote:
From: Stephen Warren swar...@nvidia.com
Some devices have configurable IRQ output polarities. Software might
use IRQ_TYPE_* to determine how to configure such a device's IRQ
output polarity in order to match how the IRQ controller input is
configured. If the board or SoC inverts the signal between the
device's IRQ output and controller's IRQ output, software must be
aware of this fact, in order to program the IRQ output to the correct
(i.e. opposite) polarity. This flag provides that information.
So what you're saying is:
Device IRQ output -- [Optional Inverter Logic] -- IRQ controller input.
And you're storing the information about the presence of the inverter
logic in the irq itself, but the core does not make any use of it and
you let the device driver deal with the outcome.
This sucks as all affected drivers have to implement the same sanity
logic for this.
Why don't you implement a core function which tells the driver which
polarity to select? That requires a few more changes, but I think it's
worth it for other reasons.
Right now the set_type logic requires the irq chip drivers to
implement sanity checking and default selections for TYPE_NONE. We can
be more clever about that and add this information to the irq chip
flags.
enum {
IRQ_CHIP_TYPES_MASK = 0x0f,
IRQ_CHIP_DEFAULT_MASK= 0xf0,
IRQ_CHIP_EXISTING_FLAGS
}
We need to extend the mask to indicate whether the chip supports
BOTH_EDGES. A chip can support FALLING and RISING, but not both at the
same time. For the set_type side the current BOTH = FALLING | RISING
is fine, but for checking the supported type it's not sufficient.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V3 1/5] genirq: define flag IRQ_SRC_DST_INVERTED, and accessors
On Tue, 4 Mar 2014, Stephen Warren wrote: On 03/04/2014 03:04 AM, Thomas Gleixner wrote: On Mon, 3 Mar 2014, Stephen Warren wrote: From: Stephen Warren swar...@nvidia.com Some devices have configurable IRQ output polarities. Software might use IRQ_TYPE_* to determine how to configure such a device's IRQ output polarity in order to match how the IRQ controller input is configured. If the board or SoC inverts the signal between the device's IRQ output and controller's IRQ output, software must be aware of this fact, in order to program the IRQ output to the correct (i.e. opposite) polarity. This flag provides that information. So what you're saying is: Device IRQ output -- [Optional Inverter Logic] -- IRQ controller input. And you're storing the information about the presence of the inverter logic in the irq itself, but the core does not make any use of it and you let the device driver deal with the outcome. This sucks as all affected drivers have to implement the same sanity logic for this. Why don't you implement a core function which tells the driver which polarity to select? That requires a few more changes, but I think it's worth it for other reasons. Right now the set_type logic requires the irq chip drivers to implement sanity checking and default selections for TYPE_NONE. We can be more clever about that and add this information to the irq chip flags. I don't see any such checking in drivers/irqchip/irq-gic.c; it rejects any type other than IRQ_TYPE_LEVEL_HIGH or IRQ_TYPE_EDGE_RISING, and I don't see any mention of TYPE_NONE in that file. Is the driver incomplete? No. The IRQ_TYPE_NONE is a hysterical leftover. Instead of adding all this extra logic to the core, what do you think of simply telling each driver that has a configurable interrupt output polarity exactly which polarity to use. This information would come from device tree or platform data. This is what I implemented in V1/V2 of this series: http://www.spinics.net/lists/devicetree/msg23648.html Is that any better, or do you definitely want the IRQ core to manage this? Oh, yes. Simply because any driver which is not aware of that inversion will trip over the issue that it requests by the best of its knowledge IRQ_TYPE_LEVEL_HIGH while it should actually request IRQ_TYPE_LEVEL_LOW due to the inversion. Are you really going to make all possibly affected drivers aware of that? Good luck! That's why we want to move such stuff to core code. Assume the following scenario: driverX which works perfectly fine on SoCA is reused for SoCB where the inverter sits between the device and the SoCB irq controller. With your DT scheme the whole thing falls flat on the nose simply because neither the driverX nor the core code is aware of the incompability. So driverX is happily using the IRQ_TYPE_LEVEL_HIGH setting which was used when the driver was written and the core works nicely with that because the irq chips supports IRQ_TYPE_LEVEL_HIGH. Just the fact that the inverter is in the hardware results in an infinite interrupt storm. Are you going to handle the bug reports and remote debug sessions for this kind of crap? Fuck no. You don't want to deal with this and that's why it is way better to build that kind of support into the core where everything which trips over the issue tells the random driver user/developer what's wrong. Dammit. I explained you very detailed WHY this is useful aside of the inverted logic situation. Is it that hard to understand? This usecase clearly shows a shortcoming at the core level along with a potential for consolidation. So why are you trying to convince me that this can be solved with some DT/device drivers hackery? When will you SoC folks ever understand that nothing on your SoCs/board is special? I'm telling that you for years but you simply refuse to get a gripe. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone
On Thu, 6 Feb 2014, Ivan Khoronzhuk wrote:
On 02/05/2014 10:27 PM, Thomas Gleixner wrote:
On Wed, 5 Feb 2014, Ivan Khoronzhuk wrote:
+ /* here we have to be sure the timer has been disabled */
Sigh. This is not a proper explanation for a barrier, really. You want
to explain what it serializes against what. i.e. you want to explain
why you are using the relaxed functions and avoid a separate non
relaxed variant in favour of an explicit barrier.
+ __iowmb();
The proper thing is to have an inline function key_stone_barrier() and
a full explanation of the issue in exactly that place instead of
handwaving comments here and there.
Thanks,
tglx
I can add new inline function like:
/**
* keystone_timer_barrier: write memory barrier
* use explicit barrier to avoid using readl/writel non relaxed function
* variants, because in our case relaxed variants hide the true places
* where barrier is needed.
*/
static inline void keystone_timer_barrier(void)
{
__iowmb();
}
and use it where it is needed.
Are you OK with it?
And I propose to leave comments under the barriers in order to be
able to understand why they are used.
Sounds good.
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V5 2/4] DRIVERS: IRQCHIP: CROSSBAR: Add support for Crossbar IP
On Mon, 3 Feb 2014, Sricharan R wrote: I already have your reviewed-by tag for the first patch in this series. Kevin was pointing out that irqchip maintainer tag is needed for this patch as well to be merged. We are planning to take this series through arm-soc tree. Can i please have your tag for this patch as well ? Acked-by-me -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone
On Tue, 4 Feb 2014, Ivan Khoronzhuk wrote: + keystone_timer_writel(off, TCR); + /* here we have to be sure the timer has been disabled */ + wmb(); We have explicit writew_relaxed and writew. Why open coding the barriers? Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone
On Tue, 4 Feb 2014, Ivan Khoronzhuk wrote: Please do not top post. It was so in v1. But it was decided to use explicit memory barriers, because we're always sure the memory barriers are there and that they're properly documented. Also in this case I don't need to add keystone readl/writel relaxed function variants and to use mixed calls of writel/writel_relaxed functions. See: http://www.spinics.net/lists/arm-kernel/msg294941.html Fair enough, but we want a proper explanation for explicit barriers in the code and not in some random discussion of patch version X on some random mailing list. Aside of that it should be iowmb(), but I might miss something ... Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe devicetree in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 4/9] clocksource/cadence_ttc: Use enable/disable_irq
On Fri, 6 Dec 2013, Sören Brinkmann wrote: On Thu, Nov 28, 2013 at 08:07:10PM +0100, Thomas Gleixner wrote: There is a solution to this. We can identify the broadcast device in the core and serialize all callers including interrupts on a different cpu against the update. So no need for the disable/enable_irq() dance. IIUC, and please correct me if I'm wrong, with the patch I'd simply call 'clockevents_update_freq() without having to disable IRQs. But I'm not sure whether periodic mode is covered. I found, that I had to reprogram the timer interval in my clock notifier callback when the timer frequency changes. I think 'clockevents_update_freq()' only handles oneshot mode. For that reason I call 'ttc_set_interval()' in the clock notifier in case the timer is in periodic mode. For that call we'd still have possible races. I guess the best solution would be to move that functionality into 'clockevents_update_freq()'? Indeed.
Re: ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)
On Thu, 5 Dec 2013, Russell King - ARM Linux wrote:
So there's not much point discussing this with you until you:
(a) calm down
Done so :)
(b) analyse it properly and work out the frequency under which each
class of IRQ (those = 32 and those 32) call into these functions.
Here you go:
The frequency of invoking the gic_arch_extn callbacks is exactly equal
to the frequency of interrupts in the system which go through the GIC
at least for mask/unmask/eoi. The frequency of calls per interrupt
depends on the interrupt type, but at minimum it is one.
So basically it does for any interrupt independent of = 32 or 32:
irq_fn(d)
{
do_something_common(d);
if (gic_arch_extn.fn)
gic_arch_extn.fn(d);
do_something_common(d);
}
which then does:
extn_fn(d)
{
if (this_irq_is_affected(d))
do_some_more(d);
}
So when this_irq_is_affected(d) boils down to
if (d-irq [] n)
then I agree, that it's debatable, whether the conditonal function
call and the simple this_irq_is_affected(d) check is worth to worry
about. Though there are people who care about two pointless
conditonals for various reasons.
But at the point when this_irq_is_affected(d) is doing a loop lookup,
then this really starts to smell badly.
Sure you might argue, that it's the problem of that particular patch
author to put a loop lookup into this_irq_is_affected().
Fair enough, though you have to admit that the design of the
gic_arch_extn actually enforces that, if you can't do a simple if irq
[] n check for whatever reason.
The alternative approach to that is to use different irq chip
implementations for interrupts affected by gic_arch_extn and those
which are not affected as we do in lot of other places do deal with
the subtle differences of particular interrupt lines. That definitely
would avoid that people try to stick more complex decision functions
than irq [] n into this_irq_is_affected().
Now looking at the locking szenario of GIC, it might eventually create
quite some duplicated code, which is undesirable as well. OTOH, the
locking requirements especially if I look at gic_[un]mask_irq and
gic_eoi_irq are not entirely clear to me from the code.
gic_[un]mask_irq(d)
{
mask = 1 SFT(gic_irq(d));
lock(controller_lock);
if (gic_arch_extn.irq_[un]mask)
gic_arch_extn.irq_[un]mask(d);
writel(mask, GIC_ENABLE_[CLEAR|SET]);
unlock(controller_lock);
}
while
gic_eoi_irq(d)
{
if (gic_arch_extn.irq_eoi) {
lock(controller_lock);
gic_arch_extn.irq_eoi(d);
unlock(controller_lock);
}
writel(gic_irq(d), GIC_EOI);
}
So is there a requirement to serialize the mask/unmask operations for
a particular interrupt line against mask/unmask operations on a
different core on some other interrupt line?
The operations for a particular interrupt line are already
serialized at the core code level.
The CLEAR/SET registers are designed to avoid locking in contrary to
the classic ENABLE reg, where you have to maintain consistency of
the full set of interrupts affected by that register.
So for the case where gic_arch_extn is not used, the locking is
completely pointless, right?
Or is this locking required to maintain consistency between the
gic_arch_extn.[un]mask and the GIC.[un]mask itself?
And even if the locking is required for this, then having two separate
chips with two different callbacks makes sense.
gic_mask_irq()
{
writel(mask, GIC_ENABLE_CLEAR);
}
gic_mask_extn_irq(d)
{
lock(controller_lock);
gic_arch_extn.irq_mask(d);
gic_mask_irq(d);
unlock(controller_lock);
}
And then have the gic_chip and gic_extn_chip set for the various
interrupt lines.
That avoids several things:
1) The locking for non gic_arch_extn interrupts
2) Two conditionals for gic_arch_extn interrupts
3) The enforcement of adding complex decision functions into the
gic_extn functions if there is no simple x N check possible.
I might have missed something as always, but at least I did a proper
analysis of the code as it is understandable to a mere mortal.
Thoughts?
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)
@all who feel responsible for gic_arch_extn
On Wed, 4 Dec 2013, Thomas Gleixner wrote:
I'm going to reply in a separate mail on this, because you have
brought this to my attention, but you are not responsible in the first
place for this brainfart.
Who came up with that gic_arch_extn concept in the first place?
It forces all GIC hotpath users to do:
hotpath_function(x)
{
do_hotpath_work();
if (random_arch_wants_crap())
random_arch_crap(x);
}
Brilliant design that. Even more so that we have only a few lonely
lusers of this brainfart. Lets look at these ordered by the output of
$ git grep -l gic_arch_extn
arch/arm/mach-imx/gpc.c
arch/arm/mach-omap2/omap-wakeupgen.c
arch/arm/mach-shmobile/intc-sh73a0.c
arch/arm/mach-shmobile/setup-r8a7779.c
arch/arm/mach-tegra/irq.c
arch/arm/mach-ux500/cpu.c
So looking at the first instance makes me go berserk already
arch/arm/mach-imx/gpc.c
This has the following repeating pattern:
imx_gpc_irq_XXX(struct irq_data *d)
{
if (d-irq 32)
return;
So the person who comitted that crime did notice, that the upper
layer calls this for all interrupts even those 32, but he could
not be arsed to sit down and avoid that.
Even worse this resulted in the following totaly misleading comment
above the irq number 32 check:
/* Sanity check for SPI irq */
if (d-irq 32)
This has nothing to do with sanity.
A sanity check is applied in case that something is expected to
be always correct, but where we want to catch the corner case
which we did not imagine yet.
So what is this (d-irq 32) check about?
It's a proof of incompetence because the only lame excuse of
implementing this nonsense is:
/*
* We are lazy and do that check on all irqs, but we could
* avoid that if we would register a different irq_chip for
* these irq lines.
*/
And I really stop here, because all other places using that nonsense
are more or less equally braindamaged.
I leave that as a an exercise to those who are responsible for the
initial implementation of gic_arch_extn and those who blindly used it.
FYI, this made me even more alert of drivers/irqchip/ being used as a
dump ground for random nonsense. It's on my high prio watch list now
and you better get your gear together and clean up the mess before I
go berserk on you.
Non-Subtle-Hint: Get rid of gic_arch_extn
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe devicetree in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
