[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