Re: Expanding Mailbox API
Hello Mudit and Gedare, I have returned from my mountain trip and digging through e-mails flood, some oversight possible. On Saturday 06 of August 2016 20:52:58 Gedare Bloom wrote: > It would make sense to me to refactor this code. I can't recall if > there was a compelling reason to have all of these different structs. I have no strong opinion there. The style has been introduced by Qiao Yang. It is probably inspired mostly by U-boot code. Linux kernel uses other style. There are two structs for each request. For example for BCM2835_MAILBOX_TAG_GET_BOARD_SERIAL operation there is bcm2835_mbox_tag_get_board_serial which is internal structure which requires correct alignment and padding for passing to the VideoCore and minimized/VideoCore exchange mechanism independent bcm2835_get_board_serial_entries structure which includes only data interesting for user. https://github.com/spark1729/rtems/commit/42442701293d4102f6987aae2c528d34c204f8d7 Form the amount of code perspective, it would be much easier to build/serialize requests directly in the code VideoCore access code (eliminate bcm2835_mbox_tag_get_board_serial definition). But as a documentation for hardware/VideoCore defined data format is definition of the structure quite appropriate format. The externally visible structure bcm2835_get_board_serial_entries has not so much reasons to exists for simple services returning one 32-bit numeric value. For the complex requests as video initialization, the structure is appropriate because number of parameters is huge and if there is need for some fields adjustment, function prototypes adaptation gets complex (pthreads attributes style allows long term source level compatibility better). Even for single value returning functions, it is good practice to return success/fail result and separating this information from actual data returned in referenced structure can be better. But for example for int bcm2835_mailbox_get_clock_rate( bcm2835_get_clock_rate_entries *_entries ); use of prototype in the form int bcm2835_mailbox_get_clock_rate( uint32_t clock_id, uint32_t *clock_rate_ptr); could be much more straightformward and user friendly. Question is if to left ptr to referenced location for the return value as last argument or use it as the first in this case. May it be for case get and set the last argument is more logical (what the first and in/out value as the last). This way next functions can be simplified bcm2835_mailbox_get_pitch bcm2835_mailbox_get_power_state bcm2835_mailbox_set_power_state bcm2835_mailbox_get_arm_memory bcm2835_mailbox_get_vc_memory bcm2835_mailbox_get_firmware_revision bcm2835_mailbox_get_board_model bcm2835_mailbox_get_board_revision bcm2835_mailbox_get_board_serial bcm2835_mailbox_get_clock_rate The structures style should be kept for bcm2835_mailbox_get_display_size bcm2835_mailbox_init_frame_buffer If style change is preferred, I change already included functions as I find some time. I would personally incline to commit proposed changes as they are to move development and testing forward and then do the simplification. There are more things to clean up. There is repeating functions calls sequence for each request which should be hidden in some combined function. bcm2835_mailbox_buffer_flush_and_invalidate( &buffer, sizeof( &buffer ) ); if ( bcm2835_mailbox_send_read_buffer( &buffer ) ) return -1; if ( !bcm2835_mailbox_buffer_suceeded( &buffer.hdr ) ) return -2; As for the Mudit's work, it would worth to check if he can switch to actual master (work seems to be based on Jul 19 master) and check if all his tests works still with it. Best wishes, Pavel ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH] arm/xilinx_zynq: Disable the MMU and the data and instruction cache on boot.
Hello Chris,
On Tuesday 09 of August 2016 09:19:51 Chris Johns wrote:
> The u-boot loader can enable the MMU plus the data and instruction caches.
> Disable them and if the data cache is enabled clear it before turn the
> caches off.
>
> Closes #2774.
> ---
> c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h | 4
> c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c | 2 +-
> 2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h
> b/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h index
> 01fdbb3..7734ddc 100644
> --- a/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h
> +++ b/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h
> @@ -166,6 +166,10 @@ arm_cp15_start_setup_mmu_and_cache(uint32_t
> ctrl_clear, uint32_t ctrl_set) {
>uint32_t ctrl = arm_cp15_get_control();
>
> + if ((ctrl & ARM_CP15_CTRL_C) != 0) {
> +arm_cp15_data_cache_clean_all_levels();
> + }
> +
I am not sure if this should be combined in arm_cp15_start_setup_mmu_and_cache
function. In the fact, the cache clean and disable can be required
for operations (for example interrupt vectors setup) between
bsp_start_hook_0 and bsp_start_hook_1 functions.
So I would incline to leave this code in the board specific
bsp_start_hook_0.
There can be even some problems with arm_cp15_data_cache_clean_all_levels
on on different ARM architecture variants.
The arm_cp15_start_setup_mmu_and_cache is used on all variants (ARM920T
for example) but arm_cp15_data_cache_clean_all_levels is ARMv7 specific
for now.
There is even problem on SMP. You need to setup MMU on the secondary CPUs
but arm_cp15_data_cache_clean_all_levels is highly problematic to be called
when other CPU is already running in MMU enabled state. You can corrupt
L2 cache content there because clean by set and way is not (according to my
ARMv7A+R manual reading) guaranteed to lead to snoop based caches
synchronization. Only L1 should be clean on the secondary CPUs.
So I have kept the code in bsp_start_hook_0 for RaspberryPi
https://git.rtems.org/rtems/tree/c/src/lib/libbsp/arm/raspberrypi/startup/bspstarthooks.c#n39
I use there RTEMS cache manager functions wrapping CP15 as well
because these are implemented such way that they are at least compatible
with ARMv& and ARMv6. Direct calling arm_cp15_data_cache_clean_all_levels
could be problematic on ARMv6. On the other hand, Zynq is ARMv7 only
so direct call should not be a problem
rtems_cache_flush_entire_data();
rtems_cache_invalidate_entire_data();
I call rtems_cache_invalidate_entire_data() later
even if the cache is disabled to ensure that there
are no stalled data in the cache from the loader which
enables and disables cache temporarily. If I remember
well, I have some problems with such stale content
in HYP mode startup case.
Sebastian, what is your opinion there?
I am not sure about consequences regarding SMP.
Best wishes,
Pavel
>ctrl &= ~ctrl_clear;
>ctrl |= ctrl_set;
>
> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c
> b/c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c index
> c7a1089..0918588 100644
> --- a/c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c
> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/startup/bspstartmmu.c
> @@ -41,7 +41,7 @@ BSP_START_TEXT_SECTION void
> zynq_setup_mmu_and_cache(void) __attribute__ ((weak) BSP_START_TEXT_SECTION
> void zynq_setup_mmu_and_cache(void)
> {
>uint32_t ctrl = arm_cp15_start_setup_mmu_and_cache(
> -ARM_CP15_CTRL_A,
> +ARM_CP15_CTRL_A | ARM_CP15_CTRL_C | ARM_CP15_CTRL_I | ARM_CP15_CTRL_M,
> ARM_CP15_CTRL_AFE | ARM_CP15_CTRL_Z
>);
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH] libbsp/arm: Add the TTB table to the default MMU set up as read/write.
On Tuesday 09 of August 2016 09:39:00 Chris Johns wrote:
> This lets the table be changed at runtime for dynamic loading and
> debugger support.
> ---
> c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h
> b/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h index
> 01fdbb3..a749f7d 100644
> --- a/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h
> +++ b/c/src/lib/libbsp/arm/shared/include/arm-cp15-start.h
> @@ -88,6 +88,10 @@ typedef struct {
> .begin = (uint32_t) bsp_section_nocachenoload_begin, \
> .end = (uint32_t) bsp_section_nocachenoload_end, \
> .flags = ARMV7_MMU_DEVICE \
> + }, { \
> +.begin = (uint32_t) bsp_translation_table_base, \
> +.end = (uint32_t) bsp_translation_table_end, \
> +.flags = ARMV7_MMU_DATA_READ_WRITE_CACHED \
>}
>
> BSP_START_DATA_SECTION extern const arm_cp15_start_section_config
ACK/+1 from my side
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH] arm/xilinx_zynq: Disable the MMU and the data and instruction cache on boot.
Hi Pavel, Thank you for taking the time to provide a really great explanation. On 14/08/2016 20:44, Pavel Pisa wrote: So I have kept the code in bsp_start_hook_0 for RaspberryPi https://git.rtems.org/rtems/tree/c/src/lib/libbsp/arm/raspberrypi/startup/bspstarthooks.c#n39 I added the code from the RPi hook_0 into arm_a9mpcore_start_hook_0 and it works. I am wondering if this should be moved to the cp15-start header as a function so we can share it. Chris ___ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel
[PATCH] arm/cortex-a: Fix cache flush/invalidate after u-boot.
This is a copy of the patch from Pavel to fix some strange behaviour with
data cache, instruction cache and MMU being enabled by u-boot on the
RaspberryPi.
Closes #2774.
---
.../libbsp/arm/shared/include/arm-a9mpcore-start.h | 29 ++
1 file changed, 29 insertions(+)
diff --git a/c/src/lib/libbsp/arm/shared/include/arm-a9mpcore-start.h
b/c/src/lib/libbsp/arm/shared/include/arm-a9mpcore-start.h
index 7d6185b..08a4d7b 100644
--- a/c/src/lib/libbsp/arm/shared/include/arm-a9mpcore-start.h
+++ b/c/src/lib/libbsp/arm/shared/include/arm-a9mpcore-start.h
@@ -129,8 +129,37 @@ BSP_START_TEXT_SECTION static inline void
arm_a9mpcore_start_hook_0(void)
volatile a9mpcore_scu *scu =
(volatile a9mpcore_scu *) BSP_ARM_A9MPCORE_SCU_BASE;
uint32_t cpu_id = arm_cortex_a9_get_multiprocessor_cpu_id();
+ uint32_t sctlr_val;
+ sctlr_val = arm_cp15_get_control();
+
+ /*
+ * Current U-boot loader seems to start kernel image
+ * with I and D caches on and MMU enabled.
+ * If RTEMS application image finds that cache is on
+ * during startup then disable caches.
+ */
+ if (sctlr_val & (ARM_CP15_CTRL_I | ARM_CP15_CTRL_C | ARM_CP15_CTRL_M)) {
+if (sctlr_val & (ARM_CP15_CTRL_C | ARM_CP15_CTRL_M)) {
+ /*
+ * If the data cache is on then ensure that it is clean
+ * before switching off to be extra carefull.
+ */
+ rtems_cache_flush_entire_data();
+ rtems_cache_invalidate_entire_data();
+}
+arm_cp15_flush_prefetch_buffer();
+sctlr_val &= ~(ARM_CP15_CTRL_I | ARM_CP15_CTRL_C | ARM_CP15_CTRL_M |
ARM_CP15_CTRL_A);
+arm_cp15_set_control(sctlr_val);
+ }
+ rtems_cache_invalidate_entire_data();
+ rtems_cache_invalidate_entire_instruction();
arm_cp15_branch_predictor_invalidate_all();
+ arm_cp15_tlb_invalidate();
+ arm_cp15_flush_prefetch_buffer();
+
+ /* Clear Translation Table Base Control Register */
+ arm_cp15_set_translation_table_base_control_register(0);
if (cpu_id == 0) {
arm_a9mpcore_start_scu_enable(scu);
--
2.4.6
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
Re: [PATCH] arm/cortex-a: Fix cache flush/invalidate after u-boot.
Hello Chris,
On Monday 15 of August 2016 07:30:56 Chris Johns wrote:
> This is a copy of the patch from Pavel to fix some strange behaviour with
> data cache, instruction cache and MMU being enabled by u-boot on the
> RaspberryPi.
>
> Closes #2774.
My code can have issues with SMP. If the arm_a9mpcore_start_hook_0
is called by secondary CPU then only local caches should be fully
invalidated.
So to be on safe side I suggest following changes to run
destructive invalidate only on CPU #0. May it be
rtems_cache_flush_entire_data
rtems_cache_invalidate_entire_data
rtems_cache_invalidate_entire_instruction
should be completely replaced for (cpu_id == 0) else block
by arm-cache-l1.h :
arm_cache_l1_clean_and_invalidate_entire_data
arm_cache_l1_invalidate_entire_instruction
which do not reach shared L2 cache and are guaranteed to not
disturb other cores operation.
If you test that chage is OK for Zynq with SMP then I reintroduce
change to RPi code (it would worth to have RPi2 SMP working for that
testing but that is near to bottom of my own TODO list).
> > May it be extend
> >
> >rtems/c/src/lib/libbsp/arm/xilinx-zynq/configure.ac
> >
> > with
> >
> >RTEMS_BSPOPTS_SET([BSP_START_IN_HYP_SUPPORT],[*],[1])
> >RTEMS_BSPOPTS_HELP([BSP_START_IN_HYP_SUPPORT], [Support start of BSP
> > in ARM HYP mode]) AM_CONDITIONAL(BSP_START_IN_HYP_SUPPORT,test
> > "$BSP_START_IN_HYP_SUPPORT" = "1")
> >
> > to support possible start in HYPervisor mode as well
>
> I saw this change but I do not fully understand the issues are that are
> being solved. I know recent RPi's enter in hypervisor mode. What boot
> loader does the RPi have?
>
> I saw this change but I do not fully understand the issues are that are
> being solved. I know recent RPi's enter in hypervisor mode. What boot
> loader does the RPi have?
It seems that recent RPi firmware enters kernel image (RTEMS application)
in HYP mode. Even if U-boot is used then it is enterred in HYP mode
and it pass control to loaded application in HYP mode.
RTEMS startup with BSP_START_IN_HYP_SUPPORT == 1 is implemented such
way that it detect CPU state and if HYP startup is used it
switches back to SVC. There should be no issues with loaders
starting in SVC as well as witch ARM architecture variants which
do not support HYP. But as for anything else, it worth to be
checked.
> .../libbsp/arm/shared/include/arm-a9mpcore-start.h | 29
> ++ 1 file changed, 29 insertions(+)
>
> diff --git a/c/src/lib/libbsp/arm/shared/include/arm-a9mpcore-start.h
> b/c/src/lib/libbsp/arm/shared/include/arm-a9mpcore-start.h index
> 7d6185b..08a4d7b 100644
> --- a/c/src/lib/libbsp/arm/shared/include/arm-a9mpcore-start.h
> +++ b/c/src/lib/libbsp/arm/shared/include/arm-a9mpcore-start.h
> @@ -129,8 +129,37 @@ BSP_START_TEXT_SECTION static inline void
> arm_a9mpcore_start_hook_0(void) volatile a9mpcore_scu *scu =
> (volatile a9mpcore_scu *) BSP_ARM_A9MPCORE_SCU_BASE;
>uint32_t cpu_id = arm_cortex_a9_get_multiprocessor_cpu_id();
> + uint32_t sctlr_val;
>
> + sctlr_val = arm_cp15_get_control();
> +
> + /*
> + * Current U-boot loader seems to start kernel image
> + * with I and D caches on and MMU enabled.
> + * If RTEMS application image finds that cache is on
> + * during startup then disable caches.
> + */
> + if (sctlr_val & (ARM_CP15_CTRL_I | ARM_CP15_CTRL_C | ARM_CP15_CTRL_M)) {
> +if (sctlr_val & (ARM_CP15_CTRL_C | ARM_CP15_CTRL_M)) {
> + /*
> + * If the data cache is on then ensure that it is clean
> + * before switching off to be extra carefull.
> + */
if (cpu_id == 0) {
> + rtems_cache_flush_entire_data();
> + rtems_cache_invalidate_entire_data();
} else {
arm_cache_l1_clean_and_invalidate_entire_data();
}
> +}
> +arm_cp15_flush_prefetch_buffer();
> +sctlr_val &= ~(ARM_CP15_CTRL_I | ARM_CP15_CTRL_C | ARM_CP15_CTRL_M |
> ARM_CP15_CTRL_A); +arm_cp15_set_control(sctlr_val);
> + }
if (cpu_id == 0) {
> +rtems_cache_invalidate_entire_data();
> +rtems_cache_invalidate_entire_instruction();
} else {
arm_cache_l1_invalidate_entire_data();
arm_cache_l1_invalidate_entire_instruction();
}
>arm_cp15_branch_predictor_invalidate_all();
> + arm_cp15_tlb_invalidate();
> + arm_cp15_flush_prefetch_buffer();
> +
> + /* Clear Translation Table Base Control Register */
> + arm_cp15_set_translation_table_base_control_register(0);
>
>if (cpu_id == 0) {
> arm_a9mpcore_start_scu_enable(scu);
___
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
