[PATCH 2/2] bsp/xilinx-zynq: Add device configuration driver

Patrick Gauvin pggauvin at gmail.com
Thu Apr 20 21:52:34 UTC 2017


Chris,


>> +
>> +static drvdata data;
>> +/* TODO: Abstract DMA buffer retrieval better */
>>
>
> Agreed.
>
>
> +static uint8_t *dma_buf = NULL;
>> +
>> +/* Check if bit is set in reg (and not masked by mask), and if it is,
>> write
>> + * that bit to reg, returning true. Otherwise return false.
>> + */
>> +static inline bool check_and_set(
>> +  volatile uint32_t *reg,
>> +  uint32_t           mask,
>> +  uint32_t           bit
>> +)
>> +{
>> +  if ( *reg & bit & ~mask )
>> +  {
>> +    *reg = bit;
>> +    return true;
>> +  }
>> +  return false;
>> +}
>> +
>> +/* Only one semaphore is used since only one interrupt is unmasked at a
>> time.
>> + * The interrupt is cleared and masked after it is caught here.
>> + */
>> +static void zynq_devcfg_isr(
>> +  void *args
>> +)
>>
>
> Why are you wanting to use interrupts rather than a simple sleep and poll?
> I am ok with a sychronous read/write driver interface here.
>
> Do we need interrupts and the complexity they have here when loading the
> fabric is so critical to any other part of a system and must be done early?


I moved to interrupts since I wanted a way to implement timeouts that
allowed the driver to continue as soon as an operation completes. This is
mostly to benefit applications that will read from and write to the PCAP
during operation, after the bitstream is initially loaded, like an FPGA
scrubber.


>
> +{
>> +  uint32_t intrs[] = {
>> +    ZYNQ_DEVCFG_INT_DMA_DONE_INT,
>> +    ZYNQ_DEVCFG_INT_PCFG_INIT_PE_INT,
>> +    ZYNQ_DEVCFG_INT_PCFG_INIT_NE_INT,
>> +    ZYNQ_DEVCFG_INT_PCFG_DONE_INT,
>> +    ZYNQ_DEVCFG_INT_PSS_CFG_RESET_B_INT
>> +  };
>> +  volatile uint32_t *int_sts = &data.regs->int_sts;
>> +  volatile uint32_t *int_mask = &data.regs->int_mask;
>> +
>> +  (void) args;
>> +
>> +  for ( size_t i = 0; i < RTEMS_ARRAY_SIZE( intrs ); ++i )
>> +    if ( check_and_set( int_sts, *int_mask, intrs[i] ) )
>> +    {
>> +      *int_mask |= intrs[i];
>> +      rtems_semaphore_release( data.sem_id_int_wait );
>> +      return;
>> +    }
>> +}
>> +
>> +static size_t get_bitstream_len( void )
>> +{
>> +  switch ( ZYNQ_SLCR_PSS_IDCODE_DEVICE_GET( zynq_slcr_pss_idcode_get()
>> ) )
>> +  {
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z007s:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z007s;
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z012s:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z012s;
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z014s:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z014s;
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z010:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z010;
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z015:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z015;
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z020:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z020;
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z030:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z030;
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z035:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z035;
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z045:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z045;
>> +  case ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z100:
>> +    return ZYNQ_DEVCFG_BITSTREAM_LEN_7z100;
>> +  default:
>> +    return 0;
>> +  }
>> +}
>>
>
> See below.
>
> +
>> +/**
>> + * @brief Create an aligned buffer for the bitstream.
>> + *
>> + * @param Desired length of the buffer in bytes.
>> + *
>> + * @retval NULL malloc failed.
>> + */
>> +static uint8_t *dma_buf_get(
>> +  size_t len
>> +)
>> +{
>> +  dma_buf = malloc( len + ZYNQ_DEVCFG_PCAP_DMA_ALIGN );
>> +  if ( NULL == dma_buf )
>> +    return NULL;
>> +  if ( (size_t)dma_buf % ZYNQ_DEVCFG_PCAP_DMA_ALIGN != 0 )
>> +  {
>> +    return dma_buf + ZYNQ_DEVCFG_PCAP_DMA_ALIGN
>> +        - ( (size_t)dma_buf % ZYNQ_DEVCFG_PCAP_DMA_ALIGN );
>> +  }
>> +  return dma_buf;
>>
>
> This and the next function need to be fixed to remove the static global.
>
>
OK.


>
> +}
>> +
>> +static void dma_buf_release( void )
>> +{
>> +  free( dma_buf );
>> +}
>> +
>> +/**
>> + * @brief Initiates a PCAP DMA transfer.
>> + *
>> + * @param src[in] For programming the FPGA, this is the location of the
>> + * bitstream data. For readback, it is the location of the PL readback
>> command
>> + * sequence.
>> + * @param src_len Typically the length of bitstream in dwords, or the
>> number of
>> + * PL commands. The user must check this value for correctness.
>> + * @param dst[in,out] For programming the FPGA use
>> ZYNQ_DEVCFG_BITSTREAM_ADDR,
>> + * for readback this is where the readback data is stored.
>> + * @param dst_len Typically the Length of bitstream in dwords, or the
>> number of
>> + * readback words expected. The user must check this value for
>> correctness.
>> + *
>> + * @retval 0 Transfer was started.
>> + * @retval -1 src_len or dst_len invalid.
>> + * @retval -2 The DMA queue was full.
>> + */
>> +static int pcap_dma_xfer(
>> +  volatile zynq_devcfg_regs *regs,
>>
>
> There is only instance of these registers and they fixed in the address
> space, so why have the argument? It is not like we have a 1..n instances
> and need to vary the address.


Good point, I'll go through and remove zynq_devcfg_regs as arguments.


>
>
> +  uint32_t                  *src,
>> +  size_t                     src_len,
>> +  uint32_t                  *dst,
>> +  size_t                     dst_len
>> +)
>> +{
>> +#ifdef ZYNQ_DEVCFG_DEBUG
>> +  printf( "DMA TRANSFER REQUESTED:\n" );
>> +  printf( "Source: %p\n", src );
>> +  printf( "Source length: %zu\n", src_len );
>> +  printf( "Destination: %p\n", dst );
>> +  printf( "Destination length: %zu\n", dst_len );
>> +#endif /* ZYNQ_DEVCFG_DEBUG */
>> +
>> +  if ( ZYNQ_DEVCFG_DMA_SRC_LEN_LEN( src_len ) != src_len )
>> +    return -1;
>> +  if ( ZYNQ_DEVCFG_DMA_DEST_LEN_LEN( dst_len ) != dst_len )
>> +    return -1;
>> +
>> +  /* Check if the command queue is full */
>> +  if ( ZYNQ_DEVCFG_STATUS_DMA_CMD_Q_F( regs->status ) )
>> +  {
>> +    WARN( "Zynq DMA queue full\n" );
>> +    return -2;
>> +  }
>> +
>> +  /* Order is important */
>> +  regs->dma_src_addr = (uint32_t)src;
>> +  regs->dma_dst_addr = (uint32_t)dst;
>> +  regs->dma_src_len = ZYNQ_DEVCFG_DMA_SRC_LEN_LEN( src_len );
>> +  regs->dma_dest_len = ZYNQ_DEVCFG_DMA_DEST_LEN_LEN( dst_len );
>> +
>> +  return 0;
>> +}
>> +
>> +static int pcap_dma_xfer_wait_and_check(
>> +  volatile zynq_devcfg_regs *regs
>>
>
> Same.
>
> +)
>> +{
>> +  uint32_t          int_sts;
>> +  rtems_status_code status;
>> +
>> +  /* NOTE: The ISR will handle acknowledging the transfer. */
>> +  regs->int_mask &= ~ZYNQ_DEVCFG_INT_DMA_DONE_INT;
>> +  status = rtems_semaphore_obtain(
>> +    data.sem_id_int_wait,
>> +    RTEMS_WAIT,
>> +    INT_TIMEOUT
>> +  );
>> +  if ( RTEMS_SUCCESSFUL != status )
>> +  {
>> +    regs->int_mask |= ZYNQ_DEVCFG_INT_DMA_DONE_INT;
>> +    WARN( "DMA timed out\n" );
>> +    return -1;
>> +  }
>> +
>> +  /* TODO: More descriptive error handling */
>>
>
> Yes please. We need more than -1 and -2.
>
>
> +  int_sts = regs->int_sts;
>> +  if (
>> +    ZYNQ_DEVCFG_INT_AXI_WERR_INT_GET( int_sts )
>> +    || ZYNQ_DEVCFG_INT_AXI_RTO_INT_GET( int_sts )
>> +    || ZYNQ_DEVCFG_INT_AXI_RERR_INT_GET( int_sts )
>> +    || ZYNQ_DEVCFG_INT_RX_FIFO_OV_INT_GET( int_sts )
>> +    || ZYNQ_DEVCFG_INT_DMA_CMD_ERR_INT_GET( int_sts )
>> +    || ZYNQ_DEVCFG_INT_DMA_Q_OV_INT_GET( int_sts )
>> +    || ZYNQ_DEVCFG_INT_P2D_LEN_ERR_INT_GET( int_sts )
>> +    || ZYNQ_DEVCFG_INT_PCFG_HMAC_ERR_INT_GET( int_sts )
>> +  )
>> +    return -2;
>> +
>> +  return 0;
>> +}
>> +
>> +/**
>> + * @brief Configure the PCAP controller.
>> + */
>> +static void pl_init(
>> +  volatile zynq_devcfg_regs *regs
>> +)
>> +{
>> +  regs->ctrl = ZYNQ_DEVCFG_CTRL_PCAP_MODE( 1 ) |
>> +      ZYNQ_DEVCFG_CTRL_PCAP_PR( ZYNQ_DEVCFG_CTRL_PCAP_PR_PCAP ) |
>> +      ZYNQ_DEVCFG_CTRL_RESERVED_BITS | regs->ctrl;
>> +  /* Disable loopback */
>> +  regs->mctrl = ZYNQ_DEVCFG_MCTRL_SET(
>> +    regs->mctrl,
>> +    ~ZYNQ_DEVCFG_MCTRL_INT_PCAP_LPBK( 1 ) & regs->mctrl
>> +  );
>> +  /* Clear all interrupts */
>> +  regs->int_sts = ZYNQ_DEVCFG_INT_ALL;
>> +
>> +  if ( !data.secure )
>> +  {
>> +    if ( ZYNQ_DEVCFG_CTRL_QUARTER_PCAP_RATE_EN( regs->ctrl ) )
>> +      regs->ctrl = ( ~ZYNQ_DEVCFG_CTRL_QUARTER_PCAP_RATE_EN( 1 ) &
>> regs->ctrl )
>> +          | ZYNQ_DEVCFG_CTRL_RESERVED_BITS;
>> +  }
>> +  else
>> +  {
>> +    if ( !ZYNQ_DEVCFG_CTRL_QUARTER_PCAP_RATE_EN( regs->ctrl ) )
>> +      regs->ctrl = ZYNQ_DEVCFG_CTRL_QUARTER_PCAP_RATE_EN( 1 ) |
>> regs->ctrl
>> +          | ZYNQ_DEVCFG_CTRL_RESERVED_BITS;
>>
>
> Have you tested secure loading?


No, I have not yet.


>
>> +rtems_device_driver zynq_devcfg_read(
>> +  rtems_device_major_number  major,
>> +  rtems_device_minor_number  minor,
>> +  void                      *args
>> +)
>> +{
>> +  rtems_libio_rw_args_t *rw_args;
>> +  int                    status;
>> +  rtems_status_code      rstatus;
>> +  rtems_status_code      final_status;
>> +  uint8_t               *data_buf;
>> +
>> +  (void) major;
>> +  (void) minor;
>> +  rw_args = args;
>> +  rw_args->bytes_moved = 0;
>> +
>> +  rstatus = rtems_semaphore_obtain( data.sem_id_internal, RTEMS_NO_WAIT,
>> 0 );
>> +  if ( RTEMS_SUCCESSFUL != rstatus )
>> +  {
>> +    final_status = RTEMS_RESOURCE_IN_USE;
>> +    goto err_obtain;
>> +  }
>> +
>> +  if ( rw_args->count < 4 )
>> +  {
>> +    final_status = RTEMS_INVALID_SIZE;
>> +    goto err_insane;
>> +  }
>> +  /* TODO: It might be valid to read configuration registers while the
>> PL is
>> +   * not programmed.
>> +   */
>> +  /* PCFG_DONE must be asserted before readback */
>> +  if ( !ZYNQ_DEVCFG_INT_PCFG_DONE_INT_GET( data.regs->int_sts ) )
>> +  {
>> +    WARN( "read attempted when FPGA configuration not done\n" );
>> +    final_status = RTEMS_IO_ERROR;
>> +    goto err_insane;
>> +  }
>> +
>> +  if ( 0 != (size_t)rw_args->buffer % ZYNQ_DEVCFG_PCAP_DMA_ALIGN )
>> +    data_buf = dma_buf_get( rw_args->count );
>> +  else
>> +    data_buf = (uint8_t *)rw_args->buffer;
>> +
>> +  status = pcap_dma_xfer(
>> +    data.regs,
>> +    (uint32_t *)ZYNQ_DEVCFG_BITSTREAM_ADDR,
>> +    rw_args->count / 4,
>> +    (uint32_t *)( (uint32_t)data_buf | 1 ),
>> +    rw_args->count / 4
>> +  );
>>
>
> What happens if the fabric is loaded with a secured bitfile? Is the
> hardware nice about you wanting to read a secured image or does it just
> stop and you need a reset?
>
> What is the use case for a read? I only ask because you must have loaded
> the fabric in the first place so your application should have that file
> somewhere.
>
>
Now that you mention it, I remember reading something about a lockdown
related to this, I'll have to check the TRM.

The primary use case for reads that I see is an FPGA scrubber which checks
and corrects configuration errors. At least in our lab, this is the primary
use.


> +  if ( status )
>> +  {
>> +    WARN( "DMA setup FAILED\n" );
>> +    final_status = RTEMS_IO_ERROR;
>> +    goto err_dma;
>> +  }
>> +  else
>> +  {
>> +    status = pcap_dma_xfer_wait_and_check( data.regs );
>> +    if ( status )
>> +    {
>> +      WARN( "DMA FAILED\n" );
>> +      final_status = RTEMS_IO_ERROR;
>> +      goto err_dma;
>> +    }
>> +  }
>> +
>> +  /* Ensure stale data is not read */
>> +  rtems_cache_invalidate_multiple_data_lines( data_buf, rw_args->count
>> );
>> +
>> +  final_status = RTEMS_SUCCESSFUL;
>> +  rw_args->bytes_moved = rw_args->count;
>> +  if ( data_buf != (uint8_t *)rw_args->buffer )
>> +    memcpy( rw_args->buffer, data_buf, rw_args->count );
>> +err_dma:
>> +  if ( data_buf != (uint8_t *)rw_args->buffer )
>> +    dma_buf_release();
>>
>
> dma_buf_release(data_buf) ?


:) In retrospect, I'm not sure why I made the DMA buffer pointer global.


>
>
> +  rstatus = rtems_semaphore_release( data.sem_id_internal );
>> +err_insane:
>> +  if ( RTEMS_SUCCESSFUL != rstatus )
>> +    final_status = RTEMS_INTERNAL_ERROR;
>> +err_obtain:
>> +  return final_status;
>> +}
>> +
>> +rtems_device_driver zynq_devcfg_write(
>> +  rtems_device_major_number  major,
>> +  rtems_device_minor_number  minor,
>> +  void                      *args
>> +)
>> +{
>> +  rtems_libio_rw_args_t *rw_args;
>> +  int                    status;
>> +  rtems_status_code      rstatus;
>> +  rtems_status_code      final_status;
>> +  uint8_t               *data_buf;
>> +
>> +  (void) major;
>> +  (void) minor;
>> +  rw_args = args;
>> +  rw_args->bytes_moved = 0;
>> +
>> +  rstatus = rtems_semaphore_obtain( data.sem_id_internal, RTEMS_NO_WAIT,
>> 0 );
>> +  if ( RTEMS_SUCCESSFUL != rstatus )
>> +  {
>> +    final_status = RTEMS_RESOURCE_IN_USE;
>> +    goto err_obtain;
>> +  }
>>
>
> I suggest a check of the data for a valid series of dummy words, the bus
> width auto detect and a sync word. This sequence is used by the PCAP loader
> to sync and align the bit stream to be processed.
>
> For example here is a function that determines the endian format of the
> image before it is loaded:
>
> static bool image_check_endian(const uint32_t* image)
> {
>   /* Xilinx binary format header */
>   const uint32_t bin_format[] = {
>     0xffffffff, /* Dummy words */
>     0xffffffff,
>     0x000000bb, /* Bus width auto detect, word 1 */
>     0x11220044, /* Bus width auto detect, word 2 */
>     0xffffffff,
>     0xffffffff,
>     0xaa995566, /* Sync word */
>   };
>   size_t i;
>
>   for (i = 0; i < (sizeof(bin_format) / sizeof(uint32_t)); i++) {
>     if (image[i] != bin_format[i]) {
>       return false;
>     }
>   }
>
>   return true;
>
> }
>

This is a good check to have, however I'll need to check that the commands
used with the PCAP after a full programming have a similar header.


>
>> +rtems_device_driver zynq_devcfg_control(
>> +  rtems_device_major_number  major,
>> +  rtems_device_minor_number  minor,
>> +  void                      *args
>> +)
>> +{
>> +  rtems_libio_ioctl_args_t *ioctl_args;
>> +  char                     *str;
>> +  int                       status;
>> +  rtems_status_code         rstatus;
>> +  rtems_status_code         final_status;
>> +
>> +  (void) major;
>> +  (void) minor;
>> +  ioctl_args = args;
>> +
>> +  rstatus = rtems_semaphore_obtain( data.sem_id_internal, RTEMS_NO_WAIT,
>> 0 );
>> +  if ( RTEMS_UNSATISFIED == rstatus )
>> +  {
>> +    ioctl_args->ioctl_return = -1;
>> +    return RTEMS_RESOURCE_IN_USE;
>> +  }
>> +  else if ( RTEMS_SUCCESSFUL != rstatus )
>> +  {
>> +    ioctl_args->ioctl_return = -1;
>> +    return RTEMS_INTERNAL_ERROR;
>> +  }
>> +
>> +  final_status = RTEMS_SUCCESSFUL;
>> +  ioctl_args->ioctl_return = 0;
>> +  switch ( ioctl_args->command ) {
>> +  case ZYNQ_DEVCFG_IOCTL_VERSION:
>> +    str = ioctl_args->buffer;
>> +    switch( ZYNQ_DEVCFG_MCTRL_PS_VERSION_GET( data.regs->mctrl ) ) {
>> +      case ZYNQ_DEVCFG_MCTRL_PS_VERSION_1_0:
>> +        strncpy( str, "1.0", ZYNQ_DEVCFG_IOCTL_VERSION_MAX_LEN );
>> +        break;
>> +      case ZYNQ_DEVCFG_MCTRL_PS_VERSION_2_0:
>> +        strncpy( str, "2.0", ZYNQ_DEVCFG_IOCTL_VERSION_MAX_LEN );
>> +        break;
>> +      case ZYNQ_DEVCFG_MCTRL_PS_VERSION_3_0:
>> +        strncpy( str, "3.0", ZYNQ_DEVCFG_IOCTL_VERSION_MAX_LEN );
>> +        break;
>> +      case ZYNQ_DEVCFG_MCTRL_PS_VERSION_3_1:
>> +        strncpy( str, "3.1", ZYNQ_DEVCFG_IOCTL_VERSION_MAX_LEN );
>> +        break;
>> +      default:
>> +        strncpy( str, "???", ZYNQ_DEVCFG_IOCTL_VERSION_MAX_LEN );
>> +        break;
>> +    }
>> +    break;
>> +  case ZYNQ_DEVCFG_IOCTL_FPGA_PROGRAM_PRE:
>> +    pl_init( data.regs );
>> +    /* Hold FPGA clocks in reset */
>> +    zynq_slcr_fpga_clk_rst( 0xf );
>> +    /* Enable PS to PL level shifters */
>> +    zynq_slcr_level_shifter_enable( ZYNQ_SLCR_LVL_SHFTR_EN_DISABLE );
>> +    zynq_slcr_level_shifter_enable( ZYNQ_SLCR_LVL_SHFTR_EN_PS_TO_PL );
>> +    status = pl_clear( data.regs );
>> +    if ( 0 != status )
>> +    {
>> +      ioctl_args->ioctl_return = -1;
>> +      final_status = RTEMS_UNSATISFIED;
>> +    }
>>
>
> Why are the load phases broken out like this? A use case type of example
> would be nice.


The load phases are broken out to keep the write function generic enough to
be used after the initial configuration of the FPGA. Here is an example of
an initial load, minus error checking:

ioctl ( fd, ZYNQ_DEVCFG_IOCTL_FPGA_PROGRAM_PRE );
write( fd, BITSTREAM_DATA, BITSTREAM_LEN );
ioctl ( fd, ZYNQ_DEVCFG_IOCTL_FPGA_PROGRAM_WAIT_DONE );
ioctl ( fd, ZYNQ_DEVCFG_IOCTL_FPGA_PROGRAM_POST );


>
>
> +    break;
>> +  case ZYNQ_DEVCFG_IOCTL_FPGA_PROGRAM_POST:
>> +    /* Enable all PS-PL level shifters */
>> +    zynq_slcr_level_shifter_enable( ZYNQ_SLCR_LVL_SHFTR_EN_ALL );
>> +    /* Release FPGA clocks from reset */
>> +    zynq_slcr_fpga_clk_rst( 0 );
>> +    break;
>> +  case ZYNQ_DEVCFG_IOCTL_FPGA_PROGRAM_WAIT_DONE:
>> +    data.regs->int_mask &= ~ZYNQ_DEVCFG_INT_PCFG_DONE_INT;
>> +    status = rtems_semaphore_obtain(
>> +      data.sem_id_int_wait,
>> +      RTEMS_WAIT,
>> +      INT_TIMEOUT
>> +    );
>> +    if ( RTEMS_SUCCESSFUL != status )
>> +    {
>> +      ioctl_args->ioctl_return = -1;
>> +      data.regs->int_mask |= ZYNQ_DEVCFG_INT_PCFG_DONE_INT;
>> +      final_status = RTEMS_TIMEOUT;
>> +    }
>>
>
> I am still not seeing the need for asychronous operation.
>
> +    break;
>> +  case ZYNQ_DEVCFG_IOCTL_SET_SECURE:
>> +    data.secure = *(bool *)ioctl_args->buffer;
>> +    break;
>>
>
> We can take this off line, but it is easier to detect the secure bit in
> the bitfile and so this call is not needed. We can also not bother to load
> a secure bitfile if the Zynq is not secure.
>
> While on the topic of secure modes, data can be decrypt by passing it in
> and then out of the PCAN. When I tested this a few years ago I found it
> only worked with the OCM. We should support doing this as it is a
> requirement on some system that data be secured with the same AES key as
> the bitfile. I am not sure how this would fit into the device model being
> used here, I suppose the returning data is buffered until read or the
> device is closed.


OK, I'm open to talking more about secure mode offline. It sounds like
you've used it extensively.


>
>
> +  case ZYNQ_DEVCFG_IOCTL_BITSTREAM_LEN_GET:
>> +    *(size_t *)ioctl_args->buffer = get_bitstream_len();
>> +    break;
>>
>
> What is purpose of this?


I made that function originally when I was checking the length of the input
data. After I realized I needed to support more than just full-bitstream
programming, and that the input data was just commands, it really isn't
that useful anymore, since the user will already know the length of their
commands anyway. I'll take this out, along with the get_bitstream_len
function.


>
>
> +  default:
>> +    ioctl_args->ioctl_return = -1;
>> +    final_status = RTEMS_INVALID_NAME; /* Maps to EINVAL */
>> +    break;
>> +  }
>> +
>> +  rstatus = rtems_semaphore_release( data.sem_id_internal );
>> +  if ( rstatus != RTEMS_SUCCESSFUL )
>> +    WARN( "Failed to release internal semaphore\n" );
>> +  return final_status;
>> +}
>> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-devcfg-regs.h
>> b/c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-devcfg-regs.h
>> new file mode 100644
>> index 0000000..d3601d6
>> --- /dev/null
>> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-devcfg-regs.h
>> @@ -0,0 +1,194 @@
>> +/**
>> + * @file
>> + * @ingroup zynq_devcfg
>> + * @brief Device configuration interface register definitions.
>> + */
>> +
>> +/*
>> + * Copyright (c) 2016
>> + *  NSF Center for High-Performance Reconfigurable Computing (CHREC),
>> + *  University of Florida.  All rights reserved.
>> + * Copyright (c) 2017
>> + *  NSF Center for High-Performance Reconfigurable Computing (CHREC),
>> + *  University of Pittsburgh.  All rights reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions are
>> + * met:
>> + * 1. Redistributions of source code must retain the above copyright
>> + *    notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + *    notice, this list of conditions and the following disclaimer in the
>> + *    documentation and/or other materials provided with the
>> distribution.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> "AS
>> + * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
>> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
>> + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> OWNER
>> + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> SPECIAL,
>> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
>> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + *
>> + * The views and conclusions contained in the software and documentation
>> + * are those of the authors and should not be interpreted as representing
>> + * official policies, either expressed or implied, of CHREC.
>> + *
>> + * Author: Patrick Gauvin <gauvin at hcs.ufl.edu>
>> + */
>> +
>> +/**
>> + * @defgroup zynq_devcfg_regs Device Configuration Interface Register
>> Definitions
>> + * @ingroup zynq_devcfg
>> + * @brief Device Configuration Interface Register Definitions
>> + */
>> +
>> +#ifndef LIBBSP_ARM_XILINX_ZYNQ_DEVCFG_REGS_H
>> +#define LIBBSP_ARM_XILINX_ZYNQ_DEVCFG_REGS_H
>> +
>> +#include <bsp/utility.h>
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif /* __cplusplus */
>> +
>> +/* Zynq-7000 series devcfg address */
>> +#define ZYNQ_DEVCFG_BASE_ADDR 0xF8007000UL
>> +/* For use with the PCAP DMA */
>> +#define ZYNQ_DEVCFG_BITSTREAM_ADDR 0xFFFFFFFFUL
>> +/* PCAP DMA transfers must be 64-byte aligned */
>> +#define ZYNQ_DEVCFG_PCAP_DMA_ALIGN 64
>> +#define ZYNQ_DEVCFG_INTERRUPT_VECTOR 40
>> +
>> +typedef struct {
>> +  uint32_t ctrl;
>> +#define ZYNQ_DEVCFG_CTRL_FORCE_RST( val ) BSP_FLD32( val, 31, 31 )
>> +#define ZYNQ_DEVCFG_CTRL_FORCE_RST_GET( reg ) BSP_FLD32GET( reg, 31, 31
>> )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_PROG_B_GET( reg ) BSP_FLD32GET( reg, 30,
>> 30 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_PROG_B( val ) BSP_FLD32( val, 30, 30 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_POR_CNT_4K_GET( reg ) BSP_FLD32GET( reg,
>> 29, 29 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_POR_CNT_4K( val ) BSP_FLD32( val, 29, 29 )
>> +#define ZYNQ_DEVCFG_CTRL_PCAP_PR( val ) BSP_FLD32( val, 27, 27 )
>> +#define ZYNQ_DEVCFG_CTRL_PCAP_PR_GET( reg ) BSP_FLD32GET( reg, 27, 27 )
>> +#define ZYNQ_DEVCFG_CTRL_PCAP_PR_ICAP ( 0 )
>> +#define ZYNQ_DEVCFG_CTRL_PCAP_PR_PCAP ( 1 )
>> +#define ZYNQ_DEVCFG_CTRL_PCAP_MODE( val ) BSP_FLD32( val, 26, 26 )
>> +#define ZYNQ_DEVCFG_CTRL_PCAP_MODE_GET( reg ) BSP_FLD32GET( reg, 26, 26
>> )
>> +#define ZYNQ_DEVCFG_CTRL_QUARTER_PCAP_RATE_EN( val ) BSP_FLD32( val,
>> 25, 25 )
>> +#define ZYNQ_DEVCFG_CTRL_QUARTER_PCAP_RATE_EN_GET( reg ) \
>> +  BSP_FLD32GET( reg, 25, 25 )
>> +#define ZYNQ_DEVCFG_CTRL_MULTIBOOT_EN( val ) BSP_FLD32( val, 24, 24 )
>> +#define ZYNQ_DEVCFG_CTRL_MULTIBOOT_EN_GET( reg ) BSP_FLD32GET( reg, 24,
>> 24 )
>> +#define ZYNQ_DEVCFG_CTRL_JTAG_CHAIN_DIS( val ) BSP_FLD32( val, 23, 23 )
>> +#define ZYNQ_DEVCFG_CTRL_JTAG_CHAIN_DIS_GET( reg ) BSP_FLD32GET( reg,
>> 23, 23 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_AES_FUSE( val ) BSP_FLD32( val, 12, 12 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_AES_FUSE_GET( reg ) BSP_FLD32GET( reg,
>> 12, 12 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_AES_FUSE_BBRAM ( 0 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_AES_FUSE_EFUSE ( 1 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_AES_EN( val ) BSP_FLD32( val, 9, 11 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_AES_EN_GET( reg ) BSP_FLD32GET( reg, 9,
>> 11 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_AES_EN_ENABLE ( 0x3 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_AES_EN_DISABLE ( 0x0 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_AES_EN_LOCKDOWN ( 0x1 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_SEU_EN( val ) BSP_FLD32( val, 8, 8 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_SEU_EN_GET( reg ) BSP_FLD32GET( reg, 8, 8
>> )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_SEC_EN_GET( reg ) BSP_FLD32GET( reg, 7, 7
>> )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_SPNIDEN( val ) BSP_FLD32( val, 6, 6 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_SPNIDEN_GET( reg ) BSP_FLD32GET( reg, 6,
>> 6 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_SPIDEN( val ) BSP_FLD32( val, 5, 5 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_SPIDEN_GET( reg ) BSP_FLD32GET( reg, 5, 5
>> )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_NIDEN( val ) BSP_FLD32( val, 4, 4 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_NIDEN_GET( reg ) BSP_FLD32GET( reg, 4, 4 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_DBGEN( val ) BSP_FLD32( val, 3, 3 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_DBGEN_GET( reg ) BSP_FLD32GET( reg, 3, 3 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_DAP_EN( val ) BSP_FLD32( val, 0, 2 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_DAP_EN_GET( reg ) BSP_FLD32GET( reg, 0, 2
>> )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_DAP_EN_ENABLE ( 0x3 )
>> +#define ZYNQ_DEVCFG_CTRL_PCFG_DAP_EN_BYPASS ( 0x0 )
>> +#define ZYNQ_DEVCFG_CTRL_RESERVED_BITS ( 0x6000 )
>> +  uint32_t lock;
>> +  uint32_t cfg;
>> +  /* int_sts and int_mask directly overlap, so they share the
>> ZYNQ_DEVCFG_INT_*
>> +   * macros */
>> +  uint32_t int_sts;
>> +  uint32_t int_mask;
>> +#define ZYNQ_DEVCFG_INT_PSS_CFG_RESET_B_INT BSP_BIT32( 27 )
>> +#define ZYNQ_DEVCFG_INT_PSS_CFG_RESET_B_INT_GET( reg ) \
>> +  BSP_FLD32GET( reg, 27, 27 )
>> +#define ZYNQ_DEVCFG_INT_AXI_WERR_INT_GET( reg ) BSP_FLD32GET( reg, 22,
>> 22 )
>> +#define ZYNQ_DEVCFG_INT_AXI_RTO_INT_GET( reg ) BSP_FLD32GET( reg, 21,
>> 21 )
>> +#define ZYNQ_DEVCFG_INT_AXI_RERR_INT_GET( reg ) BSP_FLD32GET( reg, 20,
>> 20 )
>> +#define ZYNQ_DEVCFG_INT_RX_FIFO_OV_INT_GET( reg ) \
>> +  BSP_FLD32GET( reg, 18, 18 )
>> +#define ZYNQ_DEVCFG_INT_DMA_CMD_ERR_INT_GET( reg ) \
>> +  BSP_FLD32GET( reg, 15, 15 )
>> +#define ZYNQ_DEVCFG_INT_DMA_Q_OV_INT_GET( reg ) BSP_FLD32GET( reg, 14,
>> 14 )
>> +#define ZYNQ_DEVCFG_INT_DMA_DONE_INT BSP_BIT32( 13 )
>> +#define ZYNQ_DEVCFG_INT_DMA_DONE_INT_GET( reg ) BSP_FLD32GET( reg, 13,
>> 13 )
>> +#define ZYNQ_DEVCFG_INT_D_P_DONE_INT BSP_BIT32( 12 )
>> +#define ZYNQ_DEVCFG_INT_D_P_DONE_INT_GET( reg ) BSP_FLD32GET( reg, 12,
>> 12 )
>> +#define ZYNQ_DEVCFG_INT_P2D_LEN_ERR_INT_GET( reg ) \
>> +  BSP_FLD32GET( reg, 11, 11 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_HMAC_ERR_INT_GET( reg ) \
>> +  BSP_FLD32GET( reg, 6, 6 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_SEU_ERR_INT_GET( reg ) \
>> +  BSP_FLD32GET( reg, 5, 5 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_POR_B_INT_GET( reg ) BSP_FLD32GET( reg, 4,
>> 4 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_CFG_RST_INT_GET( reg ) \
>> +  BSP_FLD32GET( reg, 3, 3 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_DONE_INT BSP_BIT32( 2 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_DONE_INT_GET( reg ) BSP_FLD32GET( reg, 2,
>> 2 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_INIT_PE_INT BSP_BIT32( 1 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_INIT_PE_INT_GET( reg ) \
>> +  BSP_FLD32GET( reg, 1, 1 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_INIT_NE_INT BSP_BIT32( 0 )
>> +#define ZYNQ_DEVCFG_INT_PCFG_INIT_NE_INT_GET( reg ) \
>> +  BSP_FLD32GET( reg, 0, 0 )
>> +#define ZYNQ_DEVCFG_INT_ALL ( 0xf8f7f87f )
>> +  uint32_t status;
>> +#define ZYNQ_DEVCFG_STATUS_DMA_CMD_Q_F( val ) BSP_FLD32( val, 31, 31 )
>> +#define ZYNQ_DEVCFG_STATUS_DMA_CMD_Q_F_GET( reg ) BSP_FLD32GET( reg,
>> 31, 31 )
>> +#define ZYNQ_DEVCFG_STATUS_PCFG_INIT_GET( reg ) BSP_FLD32GET( reg, 4, 4
>> )
>> +  uint32_t dma_src_addr;
>> +  uint32_t dma_dst_addr;
>> +  uint32_t dma_src_len;
>> +#define ZYNQ_DEVCFG_DMA_SRC_LEN_LEN( val ) BSP_FLD32( val, 0, 26 )
>> +  uint32_t dma_dest_len; /* (sic) */
>> +#define ZYNQ_DEVCFG_DMA_DEST_LEN_LEN( val ) BSP_FLD32( val, 0, 26 )
>> +  uint32_t reserved0;
>> +  uint32_t multiboot_addr;
>> +  uint32_t reserved1;
>> +  uint32_t unlock;
>> +  uint32_t reserved2[18];
>> +  uint32_t mctrl;
>> +#define ZYNQ_DEVCFG_MCTRL_PS_VERSION_GET( reg ) BSP_FLD32GET( reg, 28,
>> 31 )
>> +#define ZYNQ_DEVCFG_MCTRL_PS_VERSION_1_0 0x0
>> +#define ZYNQ_DEVCFG_MCTRL_PS_VERSION_2_0 0x1
>> +#define ZYNQ_DEVCFG_MCTRL_PS_VERSION_3_0 0x2
>> +#define ZYNQ_DEVCFG_MCTRL_PS_VERSION_3_1 0x3
>> +#define ZYNQ_DEVCFG_MCTRL_PCFG_POR_B_GET( reg ) BSP_FLD32GET( reg, 8, 8
>> )
>> +#define ZYNQ_DEVCFG_MCTRL_INT_PCAP_LPBK_GET( reg ) BSP_FLD32GET( reg,
>> 4, 4 )
>> +#define ZYNQ_DEVCFG_MCTRL_INT_PCAP_LPBK( val ) BSP_FLD32( val, 4, 4 )
>> +#define ZYNQ_DEVCFG_MCTRL_RESERVED_SET_BITS ( 0x800000 )
>> +#define ZYNQ_DEVCFG_MCTRL_RESERVED_UNSET_BITS ( 0x3 )
>> +#define ZYNQ_DEVCFG_MCTRL_SET( reg, val ) ( ( ( reg ) & \
>> +  ~ZYNQ_DEVCFG_MCTRL_RESERVED_UNSET_BITS ) | \
>> +  ZYNQ_DEVCFG_MCTRL_RESERVED_SET_BITS | ( val ) )
>> +  uint32_t reserved3[32];
>> +  uint32_t xadcif_cfg;
>> +  uint32_t xadcif_int_sts;
>> +  uint32_t xadcif_int_mask;
>> +  uint32_t xadcif_msts;
>> +  uint32_t xadcif_cmdfifo;
>> +  uint32_t xadcif_rdfifo;
>> +  uint32_t xadcif_mctrl;
>> +} zynq_devcfg_regs;
>>
>
> <burb> that is a lot of work for accessing a subset of the avaliable
> registers (I do not like struct mapping to hardware, too many scars). I can
> only hope these values are correct given the TRM is defined in absolute
> addresses and offsets.
>
>
I'll consider moving to the get/set method you proposed for the SLCR patch.
The struct mapping bit me a couple times on this project alone.


>
>> +/** @brief Bitstream lengths in bytes */
>> +enum zynq_devcfg_bitstream_length {
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z007s = 16669920UL / 8,
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z012s = 28085344UL / 8,
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z014s = 32364512UL / 8,
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z010 = 16669920UL / 8,
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z015 = 28085344UL / 8,
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z020 = 32364512UL / 8,
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z030 = 47839328UL / 8,
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z035 = 106571232UL / 8,
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z045 = 106571232UL / 8,
>> +  ZYNQ_DEVCFG_BITSTREAM_LEN_7z100 = 139330784UL / 8
>>
>
> Where did these figures come from?
>
> I would be careful here. A bitfile is a sequence of words that are
> interpreted. The hardware has registers and op codes and so the length of
> data can vary including the number of dummy values, or sync bytes that
> appear in the bit sequence. For example a secure bitfile has a bit set in
> one of the internal register that selects the secure mode.
>
> I will send you some code offline I have that decodes the format. There is
> a Xilinx UG on the topic, I forget the number.
>
>
I pulled those numbers from the TRM. They end up being equivalent to the
length of the BIN-format bitstream that Vivado outputs. I'll be removing
this as I mentioned above since it no longer has a use.

I'll plan on sending out a v2 patch early next week.

Thanks again for all the feedback,

Patrick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20170420/d8abb3ec/attachment-0001.html>


More information about the devel mailing list