[PATCH] Adding functionalities to Mailbox RPi

Mudit Jain muditjain18011995 at gmail.com
Fri Jun 24 10:02:36 UTC 2016


Hi Gedare,

When I use uncrustify, it makes a lot of changes to the previous code as
well. We can have another patch over this for the cosmetic changes.

I myself, did have doubts regarding the empty struct and having two structs
that are basically identical, however previously it was implemented in the
similar fashion so I went on with it.
For example : *bcm2835_mbox_tag_get_arm_memory* &
*bcm2835_mbox_tag_get_vc_memory.
*This is the same for a lot of the mailbox code.

I can change it if that is suggested.

Thanks.
Mudit

On Thu, Jun 23, 2016 at 9:51 PM, Gedare Bloom <gedare at rtems.org> wrote:

> On Mon, Jun 20, 2016 at 5:17 PM, Mudit Jain <muditjain18011995 at gmail.com>
> wrote:
> > From: muditj <spark1729 at yahoo.com>
> >
> > Added functions for retrieving firmware revision,
> > board model and board revision.
> > ---
> >  c/src/lib/libbsp/arm/raspberrypi/include/vc.h      | 19 +++++++
> >  c/src/lib/libbsp/arm/raspberrypi/misc/vc.c         | 60
> ++++++++++++++++++++++
> >  c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h | 20 ++++++++
> >  3 files changed, 99 insertions(+)
> >
> > diff --git a/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> b/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> > index 4e91fde..dbf9812 100644
> > --- a/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> > +++ b/c/src/lib/libbsp/arm/raspberrypi/include/vc.h
> > @@ -135,6 +135,25 @@ typedef struct
> >
> >  int
> >  bcm2835_mailbox_get_vc_memory(bcm2835_get_vc_memory_entries* _entries);
> > +
> > +typedef struct
> > +{
> Put { on previous line. Be consistent and consider using a tool to
> check style conformance:
> https://devel.rtems.org/wiki/Developer/Coding/Conventions#Tools
>
> > +  uint32_t fw_rev;
> > +} bcm2835_mailbox_get_fw_rev_entries;
> > +
> > +int
> >
> +bcm2835_mailbox_get_firmware_revision(bcm2835_mailbox_get_fw_rev_entries*
> _entries);
> > +
> > +typedef struct
> > +{
> > +  uint32_t spec;
> > +} bcm2835_get_board_spec_entries;
> > +
> > +int
> > +bcm2835_mailbox_get_board_model(bcm2835_get_board_spec_entries*
> _entries);
> > +
> > +int
> > +bcm2835_mailbox_get_board_revision(bcm2835_get_board_spec_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 91f9174..d27668d 100644
> > --- a/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c
> > +++ b/c/src/lib/libbsp/arm/raspberrypi/misc/vc.c
> > @@ -278,3 +278,63 @@
> bcm2835_mailbox_get_vc_memory(bcm2835_get_vc_memory_entries* _entries)
> >      return -2;
> >    return 0;
> >  }
> > +
> > +int
> >
> +bcm2835_mailbox_get_firmware_revision(bcm2835_mailbox_get_fw_rev_entries*
> _entries)
> > +{
> > +  struct{
> > +    bcm2835_mbox_buf_hdr hdr;
> > +    bcm2835_mbox_tag_get_fw_rev get_fw_rev;
> > +    uint32_t end_tag;
> > +  }buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;
> Add some white space around brackets. Generally, this is a dense
> function to read. Also, not sure why there is locally defined struct
> here, should this struct be defined elsewhere? It looks like the exact
> same struct is used below with slightly different field names only.
>
> > +  BCM2835_MBOX_INIT_BUF(&buffer);
> > +  BCM2835_MBOX_INIT_TAG_NO_REQ(&buffer.get_fw_rev,
> > +    BCM2835_MAILBOX_TAG_FIRMWARE_REVISION);
> > +  bcm2835_mailbox_buffer_flush_and_invalidate(&buffer, sizeof(&buffer));
> does sizeof(&buffer) work as you expect? or do you want sizeof(buffer)?
>
> > +  if (bcm2835_mailbox_send_read_buffer(&buffer))
> > +    return -1;
> > +  _entries->fw_rev = buffer.get_fw_rev.body.resp.rev;
> > +  if( !bcm2835_mailbox_buffer_suceeded(&buffer.hdr) )
> > +    return -2;
> It would be nice to have defined error codes.
>
> The same comments apply below.
>
> > +  return 0;
> > +}
> > +
> > +int
> > +bcm2835_mailbox_get_board_model(bcm2835_get_board_spec_entries*
> _entries)
> > +{
> > +  struct{
> > +    bcm2835_mbox_buf_hdr hdr;
> > +    bcm2835_mbox_tag_get_board_spec get_board_model;
> > +    uint32_t end_tag;
> > +  }buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;
> > +  BCM2835_MBOX_INIT_BUF(&buffer);
> > +  BCM2835_MBOX_INIT_TAG_NO_REQ(&buffer.get_board_model,
> > +    BCM2835_MAILBOX_TAG_GET_BOARD_MODEL);
> > +  bcm2835_mailbox_buffer_flush_and_invalidate(&buffer, sizeof(&buffer));
> > +  if (bcm2835_mailbox_send_read_buffer(&buffer))
> > +    return -1;
> > +  _entries->spec = buffer.get_board_model.body.resp.spec;
> > +  if( !bcm2835_mailbox_buffer_suceeded(&buffer.hdr) )
> > +    return -2;
> > +  return 0;
> > +}
> > +
> > +int
> > +bcm2835_mailbox_get_board_revision(bcm2835_get_board_spec_entries*
> _entries)
> > +{
> > +  struct{
> > +    bcm2835_mbox_buf_hdr hdr;
> > +    bcm2835_mbox_tag_get_board_spec get_board_revision;
> > +    uint32_t end_tag;
> > +  }buffer BCM2835_MBOX_BUF_ALIGN_ATTRIBUTE;
> > +  BCM2835_MBOX_INIT_BUF(&buffer);
> > +  BCM2835_MBOX_INIT_TAG_NO_REQ(&buffer.get_board_revision,
> > +    BCM2835_MAILBOX_TAG_GET_BOARD_VERSION);
> > +  bcm2835_mailbox_buffer_flush_and_invalidate(&buffer, sizeof(&buffer));
> > +  if (bcm2835_mailbox_send_read_buffer(&buffer))
> > +    return -1;
> > +  _entries->spec = buffer.get_board_revision.body.resp.spec;
> > +  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 f3d5a28..1b2bf92 100644
> > --- a/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h
> > +++ b/c/src/lib/libbsp/arm/raspberrypi/misc/vc_defines.h
> > @@ -135,10 +135,30 @@ typedef struct {
> >
> >  /* Video Core */
> >  #define BCM2835_MAILBOX_TAG_FIRMWARE_REVISION   0x00000001
> > +typedef struct {
> > +  bcm2835_mbox_tag_hdr tag_hdr;
> > +  union {
> > +    struct {
> > +    } req;
> an empty struct? what's the point for this?
>
> > +    struct {
> > +      uint32_t rev;
> > +    } resp;
> > +  } body;
> > +} bcm2835_mbox_tag_get_fw_rev;
> >
> >  /* Hardware */
> >  #define BCM2835_MAILBOX_TAG_GET_BOARD_MODEL     0x00010001
> >  #define BCM2835_MAILBOX_TAG_GET_BOARD_VERSION   0x00010002
> > +typedef struct {
> > +  bcm2835_mbox_tag_hdr tag_hdr;
> > +  union {
> > +    struct {
> > +    } req;
> > +    struct {
> > +      uint32_t spec;
> > +    } resp;
> > +  } body;
> > +} bcm2835_mbox_tag_get_board_spec;
> >
> These two structs are basically identical. Do we care to distinguish them?
>
> >  #if (BSP_IS_RPI2 == 1)
> >  #define BCM2836_MAILBOX_BOARD_V_2_B             0x4
> > --
> > 1.9.1
> >
> > _______________________________________________
> > devel mailing list
> > devel at rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20160624/19c1fa4a/attachment-0002.html>


More information about the devel mailing list