[Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Mathieu Rondonneau]

>From 6f985ebdd2c99257fb3873b9cb83ba6572dff60a Mon Sep 17 00:00:00 2001
From: Mathieu Rondonneau <mathieu.rondonneau@gmail.com>
Date: Fri, 13 Nov 2015 20:59:49 -0800
Subject: [PATCH] IPIPE patch for BCM2709.

 - kernel: https://github.com/raspberrypi/linux (rpi-3.18.y)
   - 1bb18c8f721ef674a447f3622273f2e2de7a205c
 - xenomai: git://git.xenomai.org/xenomai-3.git (stable-3.0.x)
   - cb72996a06ae4f29e403e3397324069f1983145e
---
 arch/arm/mach-bcm2709/armctrl.c                  | 20 ++++++++++++++++++++
 arch/arm/mach-bcm2709/bcm2708_gpio.c             | 13 ++++++++++++-
 arch/arm/mach-bcm2709/bcm2709.c                  |  9 +++++++++
 arch/arm/mach-bcm2709/include/mach/entry-macro.S | 12 ++++++++++--
 4 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-bcm2709/armctrl.c
b/arch/arm/mach-bcm2709/armctrl.c
index fc6cb8b..f69ceb8 100644
--- a/arch/arm/mach-bcm2709/armctrl.c
+++ b/arch/arm/mach-bcm2709/armctrl.c
@@ -29,6 +29,14 @@
 #include <asm/mach/irq.h>
 #include <mach/hardware.h>
 #include "armctrl.h"
+#include <linux/spinlock.h>
+
+#ifdef CONFIG_IPIPE
+#include <linux/ipipe.h>
+static IPIPE_DEFINE_RAW_SPINLOCK(irq_controller_lock);
+#else
+static DEFINE_RAW_SPINLOCK(irq_controller_lock);
+#endif

 /* For support of kernels >= 3.0 assume only one VIC for now*/
 static unsigned int remap_irqs[(INTERRUPT_ARASANSDIO + 1) -
INTERRUPT_JPEG] = {
@@ -54,6 +62,9 @@ static void armctrl_mask_irq(struct irq_data *d)
 		0
 	};
 	int i;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave_cond(&irq_controller_lock, flags);
 	if (d->irq >= FIQ_START) {
 		writel(0, __io_address(ARM_IRQ_FAST));
 	} else if (d->irq >= IRQ_ARM_LOCAL_CNTPSIRQ && d->irq <
IRQ_ARM_LOCAL_CNTPSIRQ + 4) {
@@ -79,6 +90,7 @@ static void armctrl_mask_irq(struct irq_data *d)
 	} else if (d->irq == INTERRUPT_ARM_LOCAL_PMU_FAST) {
 		writel(0xf, __io_address(ARM_LOCAL_PM_ROUTING_CLR));
 	} else { printk("%s: %d\n", __func__, d->irq); BUG(); }
+	raw_spin_unlock_irqrestore_cond(&irq_controller_lock, flags);
 }

 static void armctrl_unmask_irq(struct irq_data *d)
@@ -90,6 +102,9 @@ static void armctrl_unmask_irq(struct irq_data *d)
 		0
 	};
 	int i;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave_cond(&irq_controller_lock, flags);
 	if (d->irq >= FIQ_START) {
 		unsigned int data;
 		if (num_online_cpus() > 1) {
@@ -124,6 +139,7 @@ static void armctrl_unmask_irq(struct irq_data *d)
 	} else if (d->irq == INTERRUPT_ARM_LOCAL_PMU_FAST) {
 		writel(0xf, __io_address(ARM_LOCAL_PM_ROUTING_SET));
 	} else { printk("%s: %d\n", __func__, d->irq); BUG(); }
+	raw_spin_unlock_irqrestore_cond(&irq_controller_lock, flags);
 }

 #ifdef CONFIG_OF
@@ -333,6 +349,10 @@ static struct irq_chip armctrl_chip = {
 	.irq_mask = armctrl_mask_irq,
 	.irq_unmask = armctrl_unmask_irq,
 	.irq_set_wake = armctrl_set_wake,
+#ifdef CONFIG_IPIPE
+	.irq_hold = armctrl_mask_irq,
+	.irq_release = armctrl_unmask_irq,
+#endif
 };

 /**
diff --git a/arch/arm/mach-bcm2709/bcm2708_gpio.c
b/arch/arm/mach-bcm2709/bcm2708_gpio.c
index c1e9254..49485ab 100644
--- a/arch/arm/mach-bcm2709/bcm2708_gpio.c
+++ b/arch/arm/mach-bcm2709/bcm2708_gpio.c
@@ -24,6 +24,9 @@
 #include <linux/pinctrl/consumer.h>

 #include <linux/platform_data/bcm2708.h>
+#ifdef CONFIG_IPIPE
+#include <linux/ipipe.h>
+#endif

 #define BCM_GPIO_DRIVER_NAME "bcm2708_gpio"
 #define DRIVER_NAME BCM_GPIO_DRIVER_NAME
@@ -56,7 +59,11 @@ enum { GPIO_FSEL_INPUT, GPIO_FSEL_OUTPUT,
 	 * the GPIO code. This also makes the case of a GPIO routine call from
 	 * the IRQ code simpler.
 	 */
-static DEFINE_SPINLOCK(lock);	/* GPIO registers */
+#ifdef CONFIG_IPIPE
+static IPIPE_DEFINE_SPINLOCK(lock); /* GPIO registers */
+#else
+static DEFINE_SPINLOCK(lock); /* GPIO registers */
+#endif

 struct bcm2708_gpio {
 	struct list_head list;
@@ -294,7 +301,11 @@ static irqreturn_t bcm2708_gpio_interrupt(int irq,
void *dev_id)
 			if (!(level_bits & (1<<i)))
 				writel(1<<i,
 				       __io_address(GPIO_BASE) + GPIOEDS(bank));
+#ifdef CONFIG_IPIPE
+			ipipe_handle_demuxed_irq(gpio_to_irq(gpio));
+#else
 			generic_handle_irq(gpio_to_irq(gpio));
+#endif
 			/* ack level triggered IRQ after handling them */
 			if (level_bits & (1<<i))
 				writel(1<<i,
diff --git a/arch/arm/mach-bcm2709/bcm2709.c
b/arch/arm/mach-bcm2709/bcm2709.c
index 6bfd99e..4dd796c 100644
--- a/arch/arm/mach-bcm2709/bcm2709.c
+++ b/arch/arm/mach-bcm2709/bcm2709.c
@@ -1072,11 +1072,13 @@ static void __init bcm2709_timer_init(void)
 static void __init bcm2709_timer_init(void)
 {
 	extern void dc4_arch_timer_init(void);
+#ifndef CONFIG_IPIPE
 	// timer control
 	writel(0, __io_address(ARM_LOCAL_CONTROL));
 	// timer pre_scaler
 	writel(0x80000000, __io_address(ARM_LOCAL_PRESCALER)); // 19.2MHz
 	//writel(0x06AAAAAB, __io_address(ARM_LOCAL_PRESCALER)); // 1MHz
+#endif

 	if (use_dt)
 	{
@@ -1085,6 +1087,13 @@ static void __init bcm2709_timer_init(void)
 	}
 	else
 		dc4_arch_timer_init();
+
+#ifdef CONFIG_IPIPE
+	// timer control
+	writel(0, __io_address(ARM_LOCAL_CONTROL));
+	// timer pre_scaler
+	writel(0x80000000, __io_address(ARM_LOCAL_PRESCALER)); // 19.2MHz
+#endif
 }

 #endif
diff --git a/arch/arm/mach-bcm2709/include/mach/entry-macro.S
b/arch/arm/mach-bcm2709/include/mach/entry-macro.S
index 101d9f1..9e089c8 100644
--- a/arch/arm/mach-bcm2709/include/mach/entry-macro.S
+++ b/arch/arm/mach-bcm2709/include/mach/entry-macro.S
@@ -52,7 +52,11 @@
 	dsb
 	mov	r1, sp
 	adr	lr, BSYM(1b)
-	b	do_IPI
+#ifdef CONFIG_IPIPE
+        b	__ipipe_grab_ipi
+#else
+        b	do_IPI
+#endif

 1030:
 	/* check gpu interrupt */
@@ -107,7 +111,11 @@
 	@ routine called with r0 = irq number, r1 = struct pt_regs *
 	@
 	adr	lr, BSYM(1b)
-	b	asm_do_IRQ
+#ifdef CONFIG_IPIPE
+        b	__ipipe_grab_irq
+#else
+        b	asm_do_IRQ
+#endif

 1020:	@ EQ will be set if no irqs pending
 	.endm
-- 
2.6.2

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Gilles Chanteperdrix]

On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
> From 6f985ebdd2c99257fb3873b9cb83ba6572dff60a Mon Sep 17 00:00:00 2001
> From: Mathieu Rondonneau <mathieu.rondonneau@gmail.com>
> Date: Fri, 13 Nov 2015 20:59:49 -0800
> Subject: [PATCH] IPIPE patch for BCM2709.
> 
>  - kernel: https://github.com/raspberrypi/linux (rpi-3.18.y)
>    - 1bb18c8f721ef674a447f3622273f2e2de7a205c

As explained here:
http://xenomai.org/2014/09/porting-xenomai-dual-kernel-to-a-new-arm-soc/#Working_with_vendor_forks

If the patch is not for mainline Linux it should be sent as a couple
of pre and post patches.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Mathieu Rondonneau]

On 15-11-13 09:38 PM, Gilles Chanteperdrix wrote:
> On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
>> From 6f985ebdd2c99257fb3873b9cb83ba6572dff60a Mon Sep 17 00:00:00 2001
>> From: Mathieu Rondonneau <mathieu.rondonneau@gmail.com>
>> Date: Fri, 13 Nov 2015 20:59:49 -0800
>> Subject: [PATCH] IPIPE patch for BCM2709.
>>
>>  - kernel: https://github.com/raspberrypi/linux (rpi-3.18.y)
>>    - 1bb18c8f721ef674a447f3622273f2e2de7a205c
> 
> As explained here:
> http://xenomai.org/2014/09/porting-xenomai-dual-kernel-to-a-new-arm-soc/#Working_with_vendor_forks
> 
> If the patch is not for mainline Linux it should be sent as a couple
> of pre and post patches.
> 
there is no pre.patch required here, the ipipe patch from xenomai repo
apply with no conflict so I am not sure what to put in pre.patch.

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Gilles Chanteperdrix]

> +#include <linux/spinlock.h>
> +
> +#ifdef CONFIG_IPIPE
> +#include <linux/ipipe.h>
> +static IPIPE_DEFINE_RAW_SPINLOCK(irq_controller_lock);
> +#else
> +static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> +#endif

#ifdefery not needed here:
- you can always include linux/ipipe.h
- which will define IPIPE_DEFINE_RAW_SPINLOCK to be
DEFINE_RAW_SPINLOCK if CONFIG_IPIPE is disabled.

> 
>  /* For support of kernels >= 3.0 assume only one VIC for now*/
>  static unsigned int remap_irqs[(INTERRUPT_ARASANSDIO + 1) -
> INTERRUPT_JPEG] = {
> @@ -54,6 +62,9 @@ static void armctrl_mask_irq(struct irq_data *d)
>  		0
>  	};
>  	int i;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave_cond(&irq_controller_lock, flags);
>  	if (d->irq >= FIQ_START) {
>  		writel(0, __io_address(ARM_IRQ_FAST));
>  	} else if (d->irq >= IRQ_ARM_LOCAL_CNTPSIRQ && d->irq <
> IRQ_ARM_LOCAL_CNTPSIRQ + 4) {
> @@ -79,6 +90,7 @@ static void armctrl_mask_irq(struct irq_data *d)
>  	} else if (d->irq == INTERRUPT_ARM_LOCAL_PMU_FAST) {
>  		writel(0xf, __io_address(ARM_LOCAL_PM_ROUTING_CLR));
>  	} else { printk("%s: %d\n", __func__, d->irq); BUG(); }
> +	raw_spin_unlock_irqrestore_cond(&irq_controller_lock, flags);
>  }

Not sure why you need a spinlock here. If mainline does not need
one, chances are I-pipe does not need one either.

> 
>  static void armctrl_unmask_irq(struct irq_data *d)
> @@ -90,6 +102,9 @@ static void armctrl_unmask_irq(struct irq_data *d)
>  		0
>  	};
>  	int i;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave_cond(&irq_controller_lock, flags);
>  	if (d->irq >= FIQ_START) {
>  		unsigned int data;
>  		if (num_online_cpus() > 1) {
> @@ -124,6 +139,7 @@ static void armctrl_unmask_irq(struct irq_data *d)
>  	} else if (d->irq == INTERRUPT_ARM_LOCAL_PMU_FAST) {
>  		writel(0xf, __io_address(ARM_LOCAL_PM_ROUTING_SET));
>  	} else { printk("%s: %d\n", __func__, d->irq); BUG(); }
> +	raw_spin_unlock_irqrestore_cond(&irq_controller_lock, flags);
>  }

Same remark.

> 
>  #ifdef CONFIG_OF
> @@ -333,6 +349,10 @@ static struct irq_chip armctrl_chip = {
>  	.irq_mask = armctrl_mask_irq,
>  	.irq_unmask = armctrl_unmask_irq,
>  	.irq_set_wake = armctrl_set_wake,
> +#ifdef CONFIG_IPIPE
> +	.irq_hold = armctrl_mask_irq,
> +	.irq_release = armctrl_unmask_irq,
> +#endif
>  };
> 
>  /**
> diff --git a/arch/arm/mach-bcm2709/bcm2708_gpio.c
> b/arch/arm/mach-bcm2709/bcm2708_gpio.c
> index c1e9254..49485ab 100644
> --- a/arch/arm/mach-bcm2709/bcm2708_gpio.c
> +++ b/arch/arm/mach-bcm2709/bcm2708_gpio.c
> @@ -24,6 +24,9 @@
>  #include <linux/pinctrl/consumer.h>
> 
>  #include <linux/platform_data/bcm2708.h>
> +#ifdef CONFIG_IPIPE
> +#include <linux/ipipe.h>
> +#endif

Same as above.

> 
>  #define BCM_GPIO_DRIVER_NAME "bcm2708_gpio"
>  #define DRIVER_NAME BCM_GPIO_DRIVER_NAME
> @@ -56,7 +59,11 @@ enum { GPIO_FSEL_INPUT, GPIO_FSEL_OUTPUT,
>  	 * the GPIO code. This also makes the case of a GPIO routine call from
>  	 * the IRQ code simpler.
>  	 */
> -static DEFINE_SPINLOCK(lock);	/* GPIO registers */
> +#ifdef CONFIG_IPIPE
> +static IPIPE_DEFINE_SPINLOCK(lock); /* GPIO registers */
> +#else
> +static DEFINE_SPINLOCK(lock); /* GPIO registers */
> +#endif

Same as above.

> 
>  struct bcm2708_gpio {
>  	struct list_head list;
> @@ -294,7 +301,11 @@ static irqreturn_t bcm2708_gpio_interrupt(int irq,
> void *dev_id)
>  			if (!(level_bits & (1<<i)))
>  				writel(1<<i,
>  				       __io_address(GPIO_BASE) + GPIOEDS(bank));
> +#ifdef CONFIG_IPIPE
> +			ipipe_handle_demuxed_irq(gpio_to_irq(gpio));
> +#else
>  			generic_handle_irq(gpio_to_irq(gpio));
> +#endif

No #ifdef needed here, ipipe_handle_demuxed_irq is defined to
generic_handle_irq if CONFIG_IPIPE is disabled.

>  			/* ack level triggered IRQ after handling them */
>  			if (level_bits & (1<<i))
>  				writel(1<<i,
> diff --git a/arch/arm/mach-bcm2709/bcm2709.c
> b/arch/arm/mach-bcm2709/bcm2709.c
> index 6bfd99e..4dd796c 100644
> --- a/arch/arm/mach-bcm2709/bcm2709.c
> +++ b/arch/arm/mach-bcm2709/bcm2709.c
> @@ -1072,11 +1072,13 @@ static void __init bcm2709_timer_init(void)
>  static void __init bcm2709_timer_init(void)
>  {
>  	extern void dc4_arch_timer_init(void);
> +#ifndef CONFIG_IPIPE
>  	// timer control
>  	writel(0, __io_address(ARM_LOCAL_CONTROL));
>  	// timer pre_scaler
>  	writel(0x80000000, __io_address(ARM_LOCAL_PRESCALER)); // 19.2MHz
>  	//writel(0x06AAAAAB, __io_address(ARM_LOCAL_PRESCALER)); // 1MHz
> +#endif
> 
>  	if (use_dt)
>  	{
> @@ -1085,6 +1087,13 @@ static void __init bcm2709_timer_init(void)
>  	}
>  	else
>  		dc4_arch_timer_init();
> +
> +#ifdef CONFIG_IPIPE
> +	// timer control
> +	writel(0, __io_address(ARM_LOCAL_CONTROL));
> +	// timer pre_scaler
> +	writel(0x80000000, __io_address(ARM_LOCAL_PRESCALER)); // 19.2MHz
> +#endif
>  }

Why is that needed?


> 
>  #endif
> diff --git a/arch/arm/mach-bcm2709/include/mach/entry-macro.S
> b/arch/arm/mach-bcm2709/include/mach/entry-macro.S
> index 101d9f1..9e089c8 100644
> --- a/arch/arm/mach-bcm2709/include/mach/entry-macro.S
> +++ b/arch/arm/mach-bcm2709/include/mach/entry-macro.S
> @@ -52,7 +52,11 @@
>  	dsb
>  	mov	r1, sp
>  	adr	lr, BSYM(1b)
> -	b	do_IPI
> +#ifdef CONFIG_IPIPE
> +        b	__ipipe_grab_ipi
> +#else
> +        b	do_IPI
> +#endif
> 
>  1030:
>  	/* check gpu interrupt */
> @@ -107,7 +111,11 @@
>  	@ routine called with r0 = irq number, r1 = struct pt_regs *
>  	@
>  	adr	lr, BSYM(1b)
> -	b	asm_do_IRQ
> +#ifdef CONFIG_IPIPE
> +        b	__ipipe_grab_irq
> +#else
> +        b	asm_do_IRQ
> +#endif
> 
>  1020:	@ EQ will be set if no irqs pending
>  	.endm

I believe this way of defining irqs handlers has been deprecated for
a long long time. It prevents kernel to be built for multiple
platforms.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Gilles Chanteperdrix]

On Fri, Nov 13, 2015 at 10:06:56PM -0800, Mathieu Rondonneau wrote:
> On 15-11-13 09:38 PM, Gilles Chanteperdrix wrote:
> > On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
> >> From 6f985ebdd2c99257fb3873b9cb83ba6572dff60a Mon Sep 17 00:00:00 2001
> >> From: Mathieu Rondonneau <mathieu.rondonneau@gmail.com>
> >> Date: Fri, 13 Nov 2015 20:59:49 -0800
> >> Subject: [PATCH] IPIPE patch for BCM2709.
> >>
> >>  - kernel: https://github.com/raspberrypi/linux (rpi-3.18.y)
> >>    - 1bb18c8f721ef674a447f3622273f2e2de7a205c
> > 
> > As explained here:
> > http://xenomai.org/2014/09/porting-xenomai-dual-kernel-to-a-new-arm-soc/#Working_with_vendor_forks
> > 
> > If the patch is not for mainline Linux it should be sent as a couple
> > of pre and post patches.
> > 
> there is no pre.patch required here, the ipipe patch from xenomai repo
> apply with no conflict so I am not sure what to put in pre.patch.

Yes, sorry. Just read your patch. See comments in other post.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Gilles Chanteperdrix]

On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
>  #ifdef CONFIG_OF
> @@ -333,6 +349,10 @@ static struct irq_chip armctrl_chip = {
>  	.irq_mask = armctrl_mask_irq,
>  	.irq_unmask = armctrl_unmask_irq,
>  	.irq_set_wake = armctrl_set_wake,
> +#ifdef CONFIG_IPIPE
> +	.irq_hold = armctrl_mask_irq,
> +	.irq_release = armctrl_unmask_irq,
> +#endif

Also, as explained during your lengthy discussion, you do not need
to define irq_hold and irq_release here.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Mathieu Rondonneau]

On 15-11-14 04:39 AM, Gilles Chanteperdrix wrote:
> On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
>>  #ifdef CONFIG_OF
>> @@ -333,6 +349,10 @@ static struct irq_chip armctrl_chip = {
>>  	.irq_mask = armctrl_mask_irq,
>>  	.irq_unmask = armctrl_unmask_irq,
>>  	.irq_set_wake = armctrl_set_wake,
>> +#ifdef CONFIG_IPIPE
>> +	.irq_hold = armctrl_mask_irq,
>> +	.irq_release = armctrl_unmask_irq,
>> +#endif
> 
> Also, as explained during your lengthy discussion, you do not need
> to define irq_hold and irq_release here.
> 
Kernel does not boot if I remove them (also explained in our lengthy
discussion). I think that's the part we did not understood each other.
I am happy to remove them if there is another workaround to get the
kernel booting.

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Mathieu Rondonneau]

On 15-11-13 11:16 PM, Gilles Chanteperdrix wrote:
> On Fri, Nov 13, 2015 at 10:06:56PM -0800, Mathieu Rondonneau wrote:
>> On 15-11-13 09:38 PM, Gilles Chanteperdrix wrote:
>>> On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
>>>> From 6f985ebdd2c99257fb3873b9cb83ba6572dff60a Mon Sep 17 00:00:00 2001
>>>> From: Mathieu Rondonneau <mathieu.rondonneau@gmail.com>
>>>> Date: Fri, 13 Nov 2015 20:59:49 -0800
>>>> Subject: [PATCH] IPIPE patch for BCM2709.
>>>>
>>>>  - kernel: https://github.com/raspberrypi/linux (rpi-3.18.y)
>>>>    - 1bb18c8f721ef674a447f3622273f2e2de7a205c
>>>
>>> As explained here:
>>> http://xenomai.org/2014/09/porting-xenomai-dual-kernel-to-a-new-arm-soc/#Working_with_vendor_forks
>>>
>>> If the patch is not for mainline Linux it should be sent as a couple
>>> of pre and post patches.
>>>
>> there is no pre.patch required here, the ipipe patch from xenomai repo
>> apply with no conflict so I am not sure what to put in pre.patch.
> 
> Yes, sorry. Just read your patch. See comments in other post.
> 
No worries, thanks for confirming.

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Gilles Chanteperdrix]

On Sat, Nov 14, 2015 at 08:11:52AM -0800, Mathieu Rondonneau wrote:
> On 15-11-14 04:39 AM, Gilles Chanteperdrix wrote:
> > On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
> >>  #ifdef CONFIG_OF
> >> @@ -333,6 +349,10 @@ static struct irq_chip armctrl_chip = {
> >>  	.irq_mask = armctrl_mask_irq,
> >>  	.irq_unmask = armctrl_unmask_irq,
> >>  	.irq_set_wake = armctrl_set_wake,
> >> +#ifdef CONFIG_IPIPE
> >> +	.irq_hold = armctrl_mask_irq,
> >> +	.irq_release = armctrl_unmask_irq,
> >> +#endif
> > 
> > Also, as explained during your lengthy discussion, you do not need
> > to define irq_hold and irq_release here.
> > 
> Kernel does not boot if I remove them (also explained in our lengthy
> discussion). I think that's the part we did not understood each other.
> I am happy to remove them if there is another workaround to get the
> kernel booting.

I can not find the mail where you explain what happens when you
remove it. Could you explain it again, please?

Regards.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Mathieu Rondonneau]

On 15-11-14 10:26 AM, Gilles Chanteperdrix wrote:
> On Sat, Nov 14, 2015 at 08:11:52AM -0800, Mathieu Rondonneau wrote:
>> On 15-11-14 04:39 AM, Gilles Chanteperdrix wrote:
>>> On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
>>>>  #ifdef CONFIG_OF
>>>> @@ -333,6 +349,10 @@ static struct irq_chip armctrl_chip = {
>>>>  	.irq_mask = armctrl_mask_irq,
>>>>  	.irq_unmask = armctrl_unmask_irq,
>>>>  	.irq_set_wake = armctrl_set_wake,
>>>> +#ifdef CONFIG_IPIPE
>>>> +	.irq_hold = armctrl_mask_irq,
>>>> +	.irq_release = armctrl_unmask_irq,
>>>> +#endif
>>>
>>> Also, as explained during your lengthy discussion, you do not need
>>> to define irq_hold and irq_release here.
>>>
>> Kernel does not boot if I remove them (also explained in our lengthy
>> discussion). I think that's the part we did not understood each other.
>> I am happy to remove them if there is another workaround to get the
>> kernel booting.
> 
> I can not find the mail where you explain what happens when you
> remove it. Could you explain it again, please?
> 
> Regards.
> 

the doc refers to handle_fasteoi_irq
(http://xenomai.org/2014/09/porting-xenomai-dual-kernel-to-a-new-arm-soc/#flow_handler)
but I think there are more corner cases (else if cases). There seems to
be a case also for the handle_percpu_devid_irq for fasteoi behavior (to
call mask/unmask). The code allows that only when irq_hold is defined.

See the code from kernel/chip.c

} else if (handle == handle_percpu_irq ||
			   handle == handle_percpu_devid_irq) {
			if (irq_desc_get_chip(desc) &&
			    irq_desc_get_chip(desc)->irq_hold) {
				desc->ipipe_ack = __ipipe_ack_fasteoi_irq;
				desc->ipipe_end = __ipipe_end_fasteoi_irq;
			} else {
				desc->ipipe_ack = __ipipe_ack_percpu_irq;
				desc->ipipe_end = __ipipe_nop_irq;
			}
		}

In our earlier conversation you said this hack was for A9 only.
But it seems that this might be also needed for non-A9 (bcm2709) because
the mask and un-mask seems to be the one that are needed to be called.
this is the only reason I defined a irq_hold.

Does irq_ack and irq_eoi are required for handle_percpu_devid_irq?
the doc does not cover that part and current platform does not use
those. Removing the irq_hold then fails because irq not getting handled.

Currently handle_level_irq and handle_percpu_devid_irq share the same
irqchip, but that can not be the case if I define a irq_ack (irq_mask
will be called, then irq_ack, and then irq_unmask).

So far defining irq_hold seems to be what is required for this platform
in order to call the mask/unmask without having two irqchip.

I agree with you, this might look more like a hack.

Is there another way to do this?

Regards,
-Mathieu

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Mathieu Rondonneau]

On 15-11-13 11:15 PM, Gilles Chanteperdrix wrote:
>> +#include <linux/spinlock.h>
>> +
>> +#ifdef CONFIG_IPIPE
>> +#include <linux/ipipe.h>
>> +static IPIPE_DEFINE_RAW_SPINLOCK(irq_controller_lock);
>> +#else
>> +static DEFINE_RAW_SPINLOCK(irq_controller_lock);
>> +#endif
> 
> #ifdefery not needed here:
> - you can always include linux/ipipe.h
> - which will define IPIPE_DEFINE_RAW_SPINLOCK to be
> DEFINE_RAW_SPINLOCK if CONFIG_IPIPE is disabled.
> 
Will fix in next patch.

>>
>>  /* For support of kernels >= 3.0 assume only one VIC for now*/
>>  static unsigned int remap_irqs[(INTERRUPT_ARASANSDIO + 1) -
>> INTERRUPT_JPEG] = {
>> @@ -54,6 +62,9 @@ static void armctrl_mask_irq(struct irq_data *d)
>>  		0
>>  	};
>>  	int i;
>> +	unsigned long flags;
>> +
>> +	raw_spin_lock_irqsave_cond(&irq_controller_lock, flags);
>>  	if (d->irq >= FIQ_START) {
>>  		writel(0, __io_address(ARM_IRQ_FAST));
>>  	} else if (d->irq >= IRQ_ARM_LOCAL_CNTPSIRQ && d->irq <
>> IRQ_ARM_LOCAL_CNTPSIRQ + 4) {
>> @@ -79,6 +90,7 @@ static void armctrl_mask_irq(struct irq_data *d)
>>  	} else if (d->irq == INTERRUPT_ARM_LOCAL_PMU_FAST) {
>>  		writel(0xf, __io_address(ARM_LOCAL_PM_ROUTING_CLR));
>>  	} else { printk("%s: %d\n", __func__, d->irq); BUG(); }
>> +	raw_spin_unlock_irqrestore_cond(&irq_controller_lock, flags);
>>  }
> 
> Not sure why you need a spinlock here. If mainline does not need
> one, chances are I-pipe does not need one either.
>
It looks safer to me to have those. I can remove as I am unsure if they
are required.

>>
>>  static void armctrl_unmask_irq(struct irq_data *d)
>> @@ -90,6 +102,9 @@ static void armctrl_unmask_irq(struct irq_data *d)
>>  		0
>>  	};
>>  	int i;
>> +	unsigned long flags;
>> +
>> +	raw_spin_lock_irqsave_cond(&irq_controller_lock, flags);
>>  	if (d->irq >= FIQ_START) {
>>  		unsigned int data;
>>  		if (num_online_cpus() > 1) {
>> @@ -124,6 +139,7 @@ static void armctrl_unmask_irq(struct irq_data *d)
>>  	} else if (d->irq == INTERRUPT_ARM_LOCAL_PMU_FAST) {
>>  		writel(0xf, __io_address(ARM_LOCAL_PM_ROUTING_SET));
>>  	} else { printk("%s: %d\n", __func__, d->irq); BUG(); }
>> +	raw_spin_unlock_irqrestore_cond(&irq_controller_lock, flags);
>>  }
> 
> Same remark.
>
It looks safer to me to have those. I can remove as I am unsure if they
are required. Mainline should have this in my opinion. This is not ipipe
specific.
I think that's where the pre.patch might come in (to add mainline
specifics).
This could be clarified in the doc
(https://xenomai.org/2014/09/porting-xenomai-dual-kernel-to-a-new-arm-soc/#Working_with_vendor_forks)

>>
>>  #ifdef CONFIG_OF
>> @@ -333,6 +349,10 @@ static struct irq_chip armctrl_chip = {
>>  	.irq_mask = armctrl_mask_irq,
>>  	.irq_unmask = armctrl_unmask_irq,
>>  	.irq_set_wake = armctrl_set_wake,
>> +#ifdef CONFIG_IPIPE
>> +	.irq_hold = armctrl_mask_irq,
>> +	.irq_release = armctrl_unmask_irq,
>> +#endif
>>  };
>>
>>  /**
>> diff --git a/arch/arm/mach-bcm2709/bcm2708_gpio.c
>> b/arch/arm/mach-bcm2709/bcm2708_gpio.c
>> index c1e9254..49485ab 100644
>> --- a/arch/arm/mach-bcm2709/bcm2708_gpio.c
>> +++ b/arch/arm/mach-bcm2709/bcm2708_gpio.c
>> @@ -24,6 +24,9 @@
>>  #include <linux/pinctrl/consumer.h>
>>
>>  #include <linux/platform_data/bcm2708.h>
>> +#ifdef CONFIG_IPIPE
>> +#include <linux/ipipe.h>
>> +#endif
> 
> Same as above.
>
Will fix in next patch.

>>
>>  #define BCM_GPIO_DRIVER_NAME "bcm2708_gpio"
>>  #define DRIVER_NAME BCM_GPIO_DRIVER_NAME
>> @@ -56,7 +59,11 @@ enum { GPIO_FSEL_INPUT, GPIO_FSEL_OUTPUT,
>>  	 * the GPIO code. This also makes the case of a GPIO routine call from
>>  	 * the IRQ code simpler.
>>  	 */
>> -static DEFINE_SPINLOCK(lock);	/* GPIO registers */
>> +#ifdef CONFIG_IPIPE
>> +static IPIPE_DEFINE_SPINLOCK(lock); /* GPIO registers */
>> +#else
>> +static DEFINE_SPINLOCK(lock); /* GPIO registers */
>> +#endif
> 
> Same as above.
>
Will fix in next patch.

>>
>>  struct bcm2708_gpio {
>>  	struct list_head list;
>> @@ -294,7 +301,11 @@ static irqreturn_t bcm2708_gpio_interrupt(int irq,
>> void *dev_id)
>>  			if (!(level_bits & (1<<i)))
>>  				writel(1<<i,
>>  				       __io_address(GPIO_BASE) + GPIOEDS(bank));
>> +#ifdef CONFIG_IPIPE
>> +			ipipe_handle_demuxed_irq(gpio_to_irq(gpio));
>> +#else
>>  			generic_handle_irq(gpio_to_irq(gpio));
>> +#endif
> 
> No #ifdef needed here, ipipe_handle_demuxed_irq is defined to
> generic_handle_irq if CONFIG_IPIPE is disabled.
>
Will fix in next patch.

>>  			/* ack level triggered IRQ after handling them */
>>  			if (level_bits & (1<<i))
>>  				writel(1<<i,
>> diff --git a/arch/arm/mach-bcm2709/bcm2709.c
>> b/arch/arm/mach-bcm2709/bcm2709.c
>> index 6bfd99e..4dd796c 100644
>> --- a/arch/arm/mach-bcm2709/bcm2709.c
>> +++ b/arch/arm/mach-bcm2709/bcm2709.c
>> @@ -1072,11 +1072,13 @@ static void __init bcm2709_timer_init(void)
>>  static void __init bcm2709_timer_init(void)
>>  {
>>  	extern void dc4_arch_timer_init(void);
>> +#ifndef CONFIG_IPIPE
>>  	// timer control
>>  	writel(0, __io_address(ARM_LOCAL_CONTROL));
>>  	// timer pre_scaler
>>  	writel(0x80000000, __io_address(ARM_LOCAL_PRESCALER)); // 19.2MHz
>>  	//writel(0x06AAAAAB, __io_address(ARM_LOCAL_PRESCALER)); // 1MHz
>> +#endif
>>
>>  	if (use_dt)
>>  	{
>> @@ -1085,6 +1087,13 @@ static void __init bcm2709_timer_init(void)
>>  	}
>>  	else
>>  		dc4_arch_timer_init();
>> +
>> +#ifdef CONFIG_IPIPE
>> +	// timer control
>> +	writel(0, __io_address(ARM_LOCAL_CONTROL));
>> +	// timer pre_scaler
>> +	writel(0x80000000, __io_address(ARM_LOCAL_PRESCALER)); // 19.2MHz
>> +#endif
>>  }
> 
> Why is that needed?
> 
>
Bad handling in mainline to me, interrupt starts firing before handler
gets set (timer_init).

>>
>>  #endif
>> diff --git a/arch/arm/mach-bcm2709/include/mach/entry-macro.S
>> b/arch/arm/mach-bcm2709/include/mach/entry-macro.S
>> index 101d9f1..9e089c8 100644
>> --- a/arch/arm/mach-bcm2709/include/mach/entry-macro.S
>> +++ b/arch/arm/mach-bcm2709/include/mach/entry-macro.S
>> @@ -52,7 +52,11 @@
>>  	dsb
>>  	mov	r1, sp
>>  	adr	lr, BSYM(1b)
>> -	b	do_IPI
>> +#ifdef CONFIG_IPIPE
>> +        b	__ipipe_grab_ipi
>> +#else
>> +        b	do_IPI
>> +#endif
>>
>>  1030:
>>  	/* check gpu interrupt */
>> @@ -107,7 +111,11 @@
>>  	@ routine called with r0 = irq number, r1 = struct pt_regs *
>>  	@
>>  	adr	lr, BSYM(1b)
>> -	b	asm_do_IRQ
>> +#ifdef CONFIG_IPIPE
>> +        b	__ipipe_grab_irq
>> +#else
>> +        b	asm_do_IRQ
>> +#endif
>>
>>  1020:	@ EQ will be set if no irqs pending
>>  	.endm
> 
> I believe this way of defining irqs handlers has been deprecated for
> a long long time. It prevents kernel to be built for multiple
> platforms.
> 
bcm2709 redefines the arch_irq_handler_default from
arch/arm/include/asm/entry-macro-multi.S into this file to add some IPI
and videocore handling in the macro. I have just ported the ipipe
changes from the default arm macro to the bcm2709 one.

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Gilles Chanteperdrix]

On Sat, Nov 14, 2015 at 04:14:04PM -0800, Mathieu Rondonneau wrote:
> On 15-11-14 10:26 AM, Gilles Chanteperdrix wrote:
> > On Sat, Nov 14, 2015 at 08:11:52AM -0800, Mathieu Rondonneau wrote:
> >> On 15-11-14 04:39 AM, Gilles Chanteperdrix wrote:
> >>> On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
> >>>>  #ifdef CONFIG_OF
> >>>> @@ -333,6 +349,10 @@ static struct irq_chip armctrl_chip = {
> >>>>  	.irq_mask = armctrl_mask_irq,
> >>>>  	.irq_unmask = armctrl_unmask_irq,
> >>>>  	.irq_set_wake = armctrl_set_wake,
> >>>> +#ifdef CONFIG_IPIPE
> >>>> +	.irq_hold = armctrl_mask_irq,
> >>>> +	.irq_release = armctrl_unmask_irq,
> >>>> +#endif
> >>>
> >>> Also, as explained during your lengthy discussion, you do not need
> >>> to define irq_hold and irq_release here.
> >>>
> >> Kernel does not boot if I remove them (also explained in our lengthy
> >> discussion). I think that's the part we did not understood each other.
> >> I am happy to remove them if there is another workaround to get the
> >> kernel booting.
> > 
> > I can not find the mail where you explain what happens when you
> > remove it. Could you explain it again, please?
> > 
> > Regards.
> > 
> 
> the doc refers to handle_fasteoi_irq
> (http://xenomai.org/2014/09/porting-xenomai-dual-kernel-to-a-new-arm-soc/#flow_handler)
> but I think there are more corner cases (else if cases). There seems to
> be a case also for the handle_percpu_devid_irq for fasteoi behavior (to
> call mask/unmask). The code allows that only when irq_hold is defined.
> 
> See the code from kernel/chip.c
> 
> } else if (handle == handle_percpu_irq ||
> 			   handle == handle_percpu_devid_irq) {
> 			if (irq_desc_get_chip(desc) &&
> 			    irq_desc_get_chip(desc)->irq_hold) {
> 				desc->ipipe_ack = __ipipe_ack_fasteoi_irq;
> 				desc->ipipe_end = __ipipe_end_fasteoi_irq;
> 			} else {
> 				desc->ipipe_ack = __ipipe_ack_percpu_irq;
> 				desc->ipipe_end = __ipipe_nop_irq;
> 			}
> 		}
> 
> In our earlier conversation you said this hack was for A9 only.
> But it seems that this might be also needed for non-A9 (bcm2709) because
> the mask and un-mask seems to be the one that are needed to be called.
> this is the only reason I defined a irq_hold.
> 
> Does irq_ack and irq_eoi are required for handle_percpu_devid_irq?
> the doc does not cover that part and current platform does not use
> those. Removing the irq_hold then fails because irq not getting handled.
> 
> Currently handle_level_irq and handle_percpu_devid_irq share the same
> irqchip, but that can not be the case if I define a irq_ack (irq_mask
> will be called, then irq_ack, and then irq_unmask).
> 
> So far defining irq_hold seems to be what is required for this platform
> in order to call the mask/unmask without having two irqchip.
> 
> I agree with you, this might look more like a hack.
> 
> Is there another way to do this?

>From what I understand:
- on A9, the irqchip is shared between fasteoi irq and percpu irq
- on other platforms (where handling percpu irq was first
introduced), the irqchip is shared between percpu irq and another
flow handler, probably not level irqs
- on your platform, percpu irq shares the irqchip with level irqs,
__ipipe_ack_percpu_irq does not work because it does not mask the
irq, and this can not work with level irqs.

So, if implementing irq_hold works for this case, then let us keep
it that way. The porting guide simply need to be updated for this
case.

-- 
					    Gilles.
https://click-hack.org

Re: [Xenomai] [PATCH] IPIPE patch for BCM2709/RPI2.

[Mathieu Rondonneau]

On 15-11-15 10:43 AM, Gilles Chanteperdrix wrote:
> On Sat, Nov 14, 2015 at 04:14:04PM -0800, Mathieu Rondonneau wrote:
>> On 15-11-14 10:26 AM, Gilles Chanteperdrix wrote:
>>> On Sat, Nov 14, 2015 at 08:11:52AM -0800, Mathieu Rondonneau wrote:
>>>> On 15-11-14 04:39 AM, Gilles Chanteperdrix wrote:
>>>>> On Fri, Nov 13, 2015 at 09:31:23PM -0800, Mathieu Rondonneau wrote:
>>>>>>  #ifdef CONFIG_OF
>>>>>> @@ -333,6 +349,10 @@ static struct irq_chip armctrl_chip = {
>>>>>>  	.irq_mask = armctrl_mask_irq,
>>>>>>  	.irq_unmask = armctrl_unmask_irq,
>>>>>>  	.irq_set_wake = armctrl_set_wake,
>>>>>> +#ifdef CONFIG_IPIPE
>>>>>> +	.irq_hold = armctrl_mask_irq,
>>>>>> +	.irq_release = armctrl_unmask_irq,
>>>>>> +#endif
>>>>>
>>>>> Also, as explained during your lengthy discussion, you do not need
>>>>> to define irq_hold and irq_release here.
>>>>>
>>>> Kernel does not boot if I remove them (also explained in our lengthy
>>>> discussion). I think that's the part we did not understood each other.
>>>> I am happy to remove them if there is another workaround to get the
>>>> kernel booting.
>>>
>>> I can not find the mail where you explain what happens when you
>>> remove it. Could you explain it again, please?
>>>
>>> Regards.
>>>
>>
>> the doc refers to handle_fasteoi_irq
>> (http://xenomai.org/2014/09/porting-xenomai-dual-kernel-to-a-new-arm-soc/#flow_handler)
>> but I think there are more corner cases (else if cases). There seems to
>> be a case also for the handle_percpu_devid_irq for fasteoi behavior (to
>> call mask/unmask). The code allows that only when irq_hold is defined.
>>
>> See the code from kernel/chip.c
>>
>> } else if (handle == handle_percpu_irq ||
>> 			   handle == handle_percpu_devid_irq) {
>> 			if (irq_desc_get_chip(desc) &&
>> 			    irq_desc_get_chip(desc)->irq_hold) {
>> 				desc->ipipe_ack = __ipipe_ack_fasteoi_irq;
>> 				desc->ipipe_end = __ipipe_end_fasteoi_irq;
>> 			} else {
>> 				desc->ipipe_ack = __ipipe_ack_percpu_irq;
>> 				desc->ipipe_end = __ipipe_nop_irq;
>> 			}
>> 		}
>>
>> In our earlier conversation you said this hack was for A9 only.
>> But it seems that this might be also needed for non-A9 (bcm2709) because
>> the mask and un-mask seems to be the one that are needed to be called.
>> this is the only reason I defined a irq_hold.
>>
>> Does irq_ack and irq_eoi are required for handle_percpu_devid_irq?
>> the doc does not cover that part and current platform does not use
>> those. Removing the irq_hold then fails because irq not getting handled.
>>
>> Currently handle_level_irq and handle_percpu_devid_irq share the same
>> irqchip, but that can not be the case if I define a irq_ack (irq_mask
>> will be called, then irq_ack, and then irq_unmask).
>>
>> So far defining irq_hold seems to be what is required for this platform
>> in order to call the mask/unmask without having two irqchip.
>>
>> I agree with you, this might look more like a hack.
>>
>> Is there another way to do this?
> 
> From what I understand:
> - on A9, the irqchip is shared between fasteoi irq and percpu irq
> - on other platforms (where handling percpu irq was first
> introduced), the irqchip is shared between percpu irq and another
> flow handler, probably not level irqs
> - on your platform, percpu irq shares the irqchip with level irqs,
> __ipipe_ack_percpu_irq does not work because it does not mask the
> irq, and this can not work with level irqs.
> 
> So, if implementing irq_hold works for this case, then let us keep
> it that way. The porting guide simply need to be updated for this
> case.
> 
Sounds good to me,
Thanks,
-Mathieu