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

Chris Johns chrisj at rtems.org
Thu Apr 20 23:41:55 UTC 2017


On 21/04/2017 07:52, Patrick Gauvin wrote:
> 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.

Thanks, this makes sense if time is important. I assume you want to 
scrub the fabric without resorting to a reset?

Would it make sense to provide an ioctl to scrub the fabric and to 
verify it? It would seem like a useful function other users may need.

In terms of the RTEMS implementation, I find event send and receive is 
simpler with a low overhead not needing to allocate a semaphore. The 
interrupt checks a task id variable and if not 0 sends an event and the 
receive blocks on the receive.

>
>
>
>         +{
>         +  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.
>

I am fine with this waiting until after the code has been merged. I can 
then help sort out secure loading. It is tricky to handle as you need a 
suitable key embedded in the device and that means a battery for the 
BBRAM to avoid needing to blow fuses. I have custom boards I can test 
this on.

>
>
>         +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.

Maybe it is simpler to track secure loading and if that happens do not 
allow a read back? I think it is safe to assume this driver is the only 
path to the PCAP.

>
>
>         +  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.
>

I found the doco. The details are in UG470 
(https://www.xilinx.com/support/documentation/user_guides/ug470_7Series_Config.pdf) 
Section 5.

Note, there are registers not documented here related to secure loading 
that are covered by an NDA. I have refused offers to that documentation. 
Currently the only tool I know that support those registers is `bootgen`.

Also to be full compliant we should detect the byte order and manage the 
swap but this can wait.

>
>
>         +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 );
>

I am wondering if it makes sense to have a couple of modes for the 
driver. One where a write is synchronous and does the above and one 
where you manually to follow this pattern?

I feel the simplified case is more common than the specialised case so a 
simplified write is more robust. If for some reason this pattern needs 
to change the impact to those who reconfigure or support partial loading 
is smaller.

>
>
>
>         +    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.

Great, I have it working on a couple of projects.

>
>
>
>
>         +  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.
>

Thanks.

>
>
>
>         +  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
>         <mailto: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.

Thanks

> The struct mapping bit me a couple times on this project alone.

I have been brought in late to a few projects over the years where this 
was part of the problems. A mix of reasons, hard to audit, inconsistent 
use of volatile, compiler issues, bit fields and the list goes on.

>
>
>         +/** @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.

Thanks.

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

Awesome, and thanks for submitting this driver.

> Thanks again for all the feedback,

No problem.

Chris


More information about the devel mailing list