Re: [PATCH 11 minor addeum] arm/raspberrypi: GPIO - using RTEMS interrupt lock during BSP
On 07/09/16 08:52, Pavel Pisa wrote:
The smpfatal08 test is quite a hack since it re-defines some global SMP
>support functions locally. In order to support this, the ARM BSPs must
>place the _CPU_SMP_Start_processor() function as the only global
>function into the bspsmp.c file.
I expected that
CFLAGS_OPTIMIZE_V += -ffunction-sections -fdata-sections
in BSP and linker -Wl,--gc-section could/sould resolve that.
So should I divide bspsmp.c into two files?
For example, bspsmp-startcpu.c, bspsmp-init.c.
What about
uint32_t _CPU_SMP_Initialize(void)
{
uint32_t cpu_count = (uint32_t)bsp_processor_count;
if ( cpu_count > 4 )
cpu_count = 4;
return cpu_count;
}
void _CPU_SMP_Finalize_initialization( uint32_t cpu_count )
{
/* Do nothing */
}
void _CPU_SMP_Prepare_start_multitasking( void )
{
/* Do nothing */
}
void _CPU_SMP_Send_interrupt( uint32_t target_cpu_index )
{
/* Generates IPI */
BCM2835_REG(BCM2836_MAILBOX_3_WRITE_SET_BASE +
0x10 * target_cpu_index) = 0x1;
}
should they go to the same compilation unit as _CPU_SMP_Start_processor
or bspsmp-init.c separate one.
Or I should not care about smpfatal08 or add required symbol to it.
It would be nice to get the smpfatal08 to work with this BSP. There is
already a special case for the the qoriq BSPs in this test, so maybe we
need something similar for the RPI.
--
Sebastian Huber, embedded brains GmbH
Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.
Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 11 minor addeum] arm/raspberrypi: GPIO - using RTEMS interrupt lock during BSP
Hello Sebastian,
On Wednesday 07 of September 2016 07:33:36 Sebastian Huber wrote:
> Hello Pavel,
>
> On 06/09/16 21:48, Pavel Pisa wrote:
> > Hello Sebastian,
> >
> > On Tuesday 06 of September 2016 20:33:08 Sebastian Huber wrote:
> >> The interrupt locks are simple interrupt disable/enable or spin locks.
> >> So, they always work.
> >
> > Allmost, on UP they are simple local IRQ disable.
> > But on SMP they are spinlock combined with IRQ disable.
> > But spinlock requires that corresponding memory location is initialized.
> > It is initialized to 0,0 as ticked lock or combination with some
> > debug/non-zero information depending on RTEMS_DEBUG, RTEMS_PROFILING etc.
> > The locks ends in BSS for zero case and I expect that it is source of
> > my problems.
>
> if you want to use something before the BSS initialization, then you can
> move it to the BSP_START_DATA_SECTION section or explicitly initialize
> it, e.g. in your case via rtems_interrupt_lock_initialize().
I skip this patch. I have found one other corner case for it usefullens
and it is changing value of output pins to safe state during BSP fatal
handling. If other CPU hard-faults when holding spinlock then fatal
handler can block. But again artificially constructed case.
> The rest of the patch series looks good.
Thanks
> The smpfatal08 test is quite a hack since it re-defines some global SMP
> support functions locally. In order to support this, the ARM BSPs must
> place the _CPU_SMP_Start_processor() function as the only global
> function into the bspsmp.c file.
I expected that
CFLAGS_OPTIMIZE_V += -ffunction-sections -fdata-sections
in BSP and linker -Wl,--gc-section could/sould resolve that.
So should I divide bspsmp.c into two files?
For example, bspsmp-startcpu.c, bspsmp-init.c.
What about
uint32_t _CPU_SMP_Initialize(void)
{
uint32_t cpu_count = (uint32_t)bsp_processor_count;
if ( cpu_count > 4 )
cpu_count = 4;
return cpu_count;
}
void _CPU_SMP_Finalize_initialization( uint32_t cpu_count )
{
/* Do nothing */
}
void _CPU_SMP_Prepare_start_multitasking( void )
{
/* Do nothing */
}
void _CPU_SMP_Send_interrupt( uint32_t target_cpu_index )
{
/* Generates IPI */
BCM2835_REG(BCM2836_MAILBOX_3_WRITE_SET_BASE +
0x10 * target_cpu_index) = 0x1;
}
should they go to the same compilation unit as _CPU_SMP_Start_processor
or bspsmp-init.c separate one.
Or I should not care about smpfatal08 or add required symbol to it.
Best wishes,
Pavel
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 11 minor addeum] arm/raspberrypi: GPIO - using RTEMS interrupt lock during BSP
Hello Pavel, On 06/09/16 21:48, Pavel Pisa wrote: Hello Sebastian, On Tuesday 06 of September 2016 20:33:08 Sebastian Huber wrote: The interrupt locks are simple interrupt disable/enable or spin locks. So, they always work. Allmost, on UP they are simple local IRQ disable. But on SMP they are spinlock combined with IRQ disable. But spinlock requires that corresponding memory location is initialized. It is initialized to 0,0 as ticked lock or combination with some debug/non-zero information depending on RTEMS_DEBUG, RTEMS_PROFILING etc. The locks ends in BSS for zero case and I expect that it is source of my problems. if you want to use something before the BSS initialization, then you can move it to the BSP_START_DATA_SECTION section or explicitly initialize it, e.g. in your case via rtems_interrupt_lock_initialize(). The rest of the patch series looks good. The smpfatal08 test is quite a hack since it re-defines some global SMP support functions locally. In order to support this, the ARM BSPs must place the _CPU_SMP_Start_processor() function as the only global function into the bspsmp.c file. -- Sebastian Huber, embedded brains GmbH Address : Dornierstr. 4, D-82178 Puchheim, Germany Phone : +49 89 189 47 41-16 Fax : +49 89 189 47 41-09 E-Mail : sebastian.hu...@embedded-brains.de PGP : Public key available on request. Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG. ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH 11 minor addeum] arm/raspberrypi: GPIO - using RTEMS interrupt lock during BSP
Hello Sebastian, On Tuesday 06 of September 2016 20:33:08 Sebastian Huber wrote: > The interrupt locks are simple interrupt disable/enable or spin locks. So, > they always work. Allmost, on UP they are simple local IRQ disable. But on SMP they are spinlock combined with IRQ disable. But spinlock requires that corresponding memory location is initialized. It is initialized to 0,0 as ticked lock or combination with some debug/non-zero information depending on RTEMS_DEBUG, RTEMS_PROFILING etc. The locks ends in BSS for zero case and I expect that it is source of my problems. I have experienced hard lock of RPi BSP on rpi_select_pin_function() in SMP build when I use it in bsp_start_hook_0(), this is before bsp_start_copy_sections(). I use that for hacking to enable JTAG early in boot for BSP debugging https://github.com/ppisa/rtems/blob/rtems-rpi-devel-160621/c/src/lib/libbsp/arm/raspberrypi/startup/bspstarthooks.c#L85 I am sure that deadlock has been in rpi_select_pin_function(). Removing JTAG setup helped. I am sure that skipping locks would help. But you are right that I have not analyzed where _System_state_Is_up( _System_state_Get() ) global variable is placed. If it is in BSS then it is only matter of luck that it does not match system up state when BSS is not initialized yet. So value of patch for real BSP use case is questionable. My use case is corner or even more obscure and can be solved by setup in u-boot or definition of the function variant without locking. Anyway, thanks for review. I expect that you are quite overloaded after return from vacations but I would be really happy if you can say your comments/ACK/NACK to the rest or RPi SMP series in next days. There is that problem with generic RTL, caches on ARM and RPi BSP mostly unusable with actual firmaware on 4.11 branch. I understand, that my proposal of backport of most of related fixes from master to 4.11 is sensitive question. Again your income/voting to debate (ideally with Joel, Chris and Gedare) could help in steering 4.11 branch. Best wishes, Pavel PS: Congratulation to "Rework thread priority management". This is big/fundamental step forward for RTEMS. ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
[PATCH 11 minor addeum] arm/raspberrypi: GPIO - using RTEMS interrupt lock during BSP
Locking can be used only when RTEMS reaches multitasking state _System_state_Is_up( _System_state_Get() ) --- c/src/lib/libbsp/arm/raspberrypi/gpio/rpi-gpio.c | 14 ++ 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/c/src/lib/libbsp/arm/raspberrypi/gpio/rpi-gpio.c b/c/src/lib/libbsp/arm/raspberrypi/gpio/rpi-gpio.c index 6c01d62..f7c8c36 100644 --- a/c/src/lib/libbsp/arm/raspberrypi/gpio/rpi-gpio.c +++ b/c/src/lib/libbsp/arm/raspberrypi/gpio/rpi-gpio.c @@ -19,6 +19,7 @@ #include #include #include +#include #include @@ -57,16 +58,21 @@ static rtems_status_code rpi_select_pin_function( /* Calculate the pin function select register address. */ volatile uint32_t *pin_addr = (uint32_t *) BCM2835_GPIO_REGS_BASE + (pin / 10); - uint32_t reg_old; uint32_t reg_new; + int use_locks; rtems_interrupt_lock_context lock_context; - rtems_interrupt_lock_acquire(_gpio_bsp_lock, _context); - reg_new = reg_old = *pin_addr; + use_locks = _System_state_Is_up( _System_state_Get() ); + if ( use_locks ) +rtems_interrupt_lock_acquire(_gpio_bsp_lock, _context); + + reg_new = *pin_addr; reg_new &= ~SELECT_PIN_FUNCTION(RPI_ALT_FUNC_MASK, pin); reg_new |= SELECT_PIN_FUNCTION(type, pin); *pin_addr = reg_new; - rtems_interrupt_lock_release(_gpio_bsp_lock, _context); + + if ( use_locks ) +rtems_interrupt_lock_release(_gpio_bsp_lock, _context); return RTEMS_SUCCESSFUL; } -- 1.9.1 ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
