[PATCH] Mailbox : Extending functionality
Pavel Pisa
ppisa4lists at pikron.com
Tue Aug 23 22:17:11 UTC 2016
Hello Mudit Jain,
I have gone through the patch in your repository rebased
on RTEMS master. I have only minor remarks to alignment/cache
problems prevention which has been added to other functions
in mainline. Please, update patch and if there are no
critical comment to the patch in next days, I push
it into mainline.
See the remark in patch below
On Saturday 20 of August 2016 16:06:20 Mudit Jain wrote:
> Adding functionality to get board serial,
> power state & clock rate
> ---
> c/src/lib/libbsp/arm/raspberrypi/include/vc.h | 17 +++++
> c/src/lib/libbsp/arm/raspberrypi/misc/vc.c | 75
> ++++++++++++++++++++++ c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h |
> 35 ++++++++++ 3 files changed, 127 insertions(+)
>
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> b/c/src/lib/libbsp/arm/raspberrypi/include/vc.h index 00414ff..e863732
> 100644
> --- a/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> +++ b/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> @@ -106,6 +106,8 @@ typedef struct {
> #define BCM2835_MAILBOX_POWER_STATE_NODEV ( 1 << 1 )
> int bcm2835_mailbox_set_power_state( bcm2835_set_power_state_entries
> *_entries );
>
> +int bcm2835_mailbox_get_power_state( bcm2835_set_power_state_entries
> *_entries ); +
> typedef struct {
> uint32_t base;
> size_t size;
> @@ -135,6 +137,21 @@ int bcm2835_mailbox_get_board_model(
> bcm2835_get_board_spec_entries *_entries );
>
> int bcm2835_mailbox_get_board_revision(
> bcm2835_get_board_spec_entries *_entries );
> +
> +typedef struct {
> + uint64_t board_serial;
> +} bcm2835_get_board_serial_entries;
> +
> +int bcm2835_mailbox_get_board_serial(
> + bcm2835_get_board_serial_entries *_entries );
> +
> +typedef struct {
> + uint32_t clock_id;
> + uint32_t clock_rate;
> +} bcm2835_get_clock_rate_entries;
> +
> +int bcm2835_mailbox_get_clock_rate(
> + bcm2835_get_clock_rate_entries *_entries );
> /** @} */
>
> #endif /* LIBBSP_ARM_RASPBERRYPI_VC_H */
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c
> b/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c index 0bec0c2..df92649 100644
> --- a/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c
> +++ b/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c
> @@ -254,6 +254,31 @@ int bcm2835_mailbox_get_cmdline(
> bcm2835_get_cmdline_entries *_entries ) return 0;
> }
>
> +int bcm2835_mailbox_get_power_state( bcm2835_set_power_state_entries
> *_entries ) +{
> + struct {
> + bcm2835_mbox_buf_hdr hdr;
> + bcm2835_mbox_tag_get_power_state get_power_state;
> + uint32_t end_tag;
> + } buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;
When BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE is applied only to variable
it does not apply to extending of the size of structure.
It is necessary put it in place where it affect not only placement
start address but even size
!! struct BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE {
bcm2835_mbox_buf_hdr hdr;
bcm2835_mbox_tag_get_power_state get_power_state;
uint32_t end_tag;
} buffer;
Even this arrangement is not enough to prevent some sharing
of last cache line when variable is on the stack. It can
be optimization problem some stack prefetch, I am not sure
!! struct BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE {
bcm2835_mbox_buf_hdr hdr;
bcm2835_mbox_tag_get_power_state get_power_state;
uint32_t end_tag;
!! uint32_t padding_reserve[16];
} buffer;
Adding padding which is equivalent to one cache line resolved
the problems. Generally it seems that placement of mailbox data
buffers on stack has not been optimal choice even that it
has straightforward logic (execution of function by firmware
with wait for the result (blocking) is conceptually equivalent
to calling regular C function with parameters on stack).
Concept has advantage that there are no problems with allocation
of heap or other memory during initial BSP setup when there
is nor heap ready to be used etc. But it seems too fragile at the
end, I am responsible for these problems because I have considered
idea to use stack as good during previous RPi GSoC project.
Anyway, I think that we should go current way for now.
> + BCM2835_MBOX_INIT_BUF( &buffer );
> + BCM2835_MBOX_INIT_TAG( &buffer.get_power_state,
> + BCM2835_MAILBOX_TAG_GET_POWER_STATE );
> + buffer.get_power_state.body.req.dev_id = _entries->dev_id;
> + bcm2835_mailbox_buffer_flush_and_invalidate( &buffer, sizeof( &buffer )
> ); +
> + if ( bcm2835_mailbox_send_read_buffer( &buffer ) )
> + return -1;
> +
> + _entries->dev_id = buffer.get_power_state.body.resp.dev_id;
> + _entries->state = buffer.get_power_state.body.resp.state;
> +
> + if ( !bcm2835_mailbox_buffer_suceeded( &buffer.hdr ) )
> + return -2;
> +
> + return 0;
> +}
> +
> int bcm2835_mailbox_set_power_state( bcm2835_set_power_state_entries
> *_entries ) {
> struct {
> @@ -398,3 +423,53 @@ int bcm2835_mailbox_get_board_revision(
>
> return 0;
> }
> +
> +int bcm2835_mailbox_get_board_serial(
> + bcm2835_get_board_serial_entries *_entries )
> +{
> + struct {
same
struct BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE {
> + bcm2835_mbox_buf_hdr hdr;
> + bcm2835_mbox_tag_get_board_serial get_board_serial;
> + uint32_t end_tag;
uint32_t padding_reserve[16];
> + } buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;
> + BCM2835_MBOX_INIT_BUF( &buffer );
> + BCM2835_MBOX_INIT_TAG_NO_REQ( &buffer.get_board_serial,
> + BCM2835_MAILBOX_TAG_GET_BOARD_SERIAL );
> + bcm2835_mailbox_buffer_flush_and_invalidate( &buffer, sizeof( &buffer )
> ); +
> + if ( bcm2835_mailbox_send_read_buffer( &buffer ) )
> + return -1;
> +
> + _entries->board_serial = buffer.get_board_serial.body.resp.board_serial;
> +
> + if ( !bcm2835_mailbox_buffer_suceeded( &buffer.hdr ) )
> + return -2;
> +
> + return 0;
> +}
> +
> +int bcm2835_mailbox_get_clock_rate(
> + bcm2835_get_clock_rate_entries *_entries )
> +{
> + struct {
struct BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE {
> + bcm2835_mbox_buf_hdr hdr;
> + bcm2835_mbox_tag_get_clock_rate get_clock_rate;
> + uint32_t end_tag;
uint32_t padding_reserve[16];
> + } buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;
> + BCM2835_MBOX_INIT_BUF( &buffer );
> + BCM2835_MBOX_INIT_TAG_NO_REQ( &buffer.get_clock_rate,
> + BCM2835_MAILBOX_TAG_GET_CLOCK_RATE );
> + buffer.get_clock_rate.body.req.clock_id = _entries->clock_id;
> + bcm2835_mailbox_buffer_flush_and_invalidate( &buffer, sizeof( &buffer )
> ); +
> + if ( bcm2835_mailbox_send_read_buffer( &buffer ) )
> + return -1;
> +
> + _entries->clock_id = buffer.get_clock_rate.body.resp.clock_id;
> + _entries->clock_rate = buffer.get_clock_rate.body.resp.clock_rate;
> +
> + if ( !bcm2835_mailbox_buffer_suceeded( &buffer.hdr ) )
> + return -2;
> +
> + return 0;
> +}
> diff --git a/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h
> b/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h index 8d1067b..48d1658
> 100644
> --- a/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h
> +++ b/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h
> @@ -179,6 +179,17 @@ typedef struct {
>
> #define BCM2835_MAILBOX_TAG_GET_BOARD_MAC 0x00010003
> #define BCM2835_MAILBOX_TAG_GET_BOARD_SERIAL 0x00010004
> +typedef struct {
> + bcm2835_mbox_tag_hdr tag_hdr;
> + union {
> + struct {
> + } req;
> + struct {
> + uint64_t board_serial;
> + } resp;
> + } body;
> +} bcm2835_mbox_tag_get_board_serial;
> +
> #define BCM2835_MAILBOX_TAG_GET_ARM_MEMORY 0x00010005
> typedef struct {
> bcm2835_mbox_tag_hdr tag_hdr;
> @@ -206,6 +217,18 @@ typedef struct {
> } bcm2835_mbox_tag_get_vc_memory;
>
> #define BCM2835_MAILBOX_TAG_GET_CLOCKS 0x00010007
> +typedef struct {
> + bcm2835_mbox_tag_hdr tag_hdr;
> + union {
> + struct {
> + uint32_t clock_id;
> + } req;
> + struct {
> + uint32_t clock_id;
> + uint32_t clock_rate;
> + } resp;
> + } body;
> +} bcm2835_mbox_tag_get_clock_rate;
>
> /* Config */
> #define BCM2835_MAILBOX_TAG_GET_CMD_LINE 0x00050001
> @@ -235,6 +258,18 @@ typedef struct {
> #define BCM2835_MAILBOX_POWER_UDID_CCP2TX 0x00000008
>
> #define BCM2835_MAILBOX_TAG_GET_POWER_STATE 0x00020001
> +typedef struct {
> + bcm2835_mbox_tag_hdr tag_hdr;
> + union {
> + struct {
> + uint32_t dev_id;
> + } req;
> + struct {
> + uint32_t dev_id;
> + uint32_t state;
> + } resp;
> + } body;
> +} bcm2835_mbox_tag_get_power_state;
>
> #define BCM2835_MAILBOX_POWER_STATE_RESP_ON (1 << 0)
> #define BCM2835_MAILBOX_POWER_STATE_RESP_NODEV (1 << 1)
Best wishes,
Pavel
More information about the devel
mailing list