[PATCH 1/2] bsp/xilinx-zynq: Add SLCR driver

Chris Johns chrisj at rtems.org
Thu Apr 20 06:00:43 UTC 2017


Thanks for submitting this driver. My comments are below.

Chris

On 20/04/2017 13:06, Patrick Gauvin wrote:
> ---
>  c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am       |  5 ++
>  .../arm/xilinx-zynq/include/zynq-slcr-regs.h       | 84 ++++++++++++++++++++
>  .../lib/libbsp/arm/xilinx-zynq/include/zynq-slcr.h | 70 +++++++++++++++++
>  c/src/lib/libbsp/arm/xilinx-zynq/preinstall.am     |  8 ++
>  c/src/lib/libbsp/arm/xilinx-zynq/slcr/zynq-slcr.c  | 90 ++++++++++++++++++++++
>  5 files changed, 257 insertions(+)
>  create mode 100644 c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-slcr-regs.h
>  create mode 100644 c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-slcr.h
>  create mode 100644 c/src/lib/libbsp/arm/xilinx-zynq/slcr/zynq-slcr.c
>
> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am b/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
> index 1f9ed59..08024b9 100644
> --- a/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/Makefile.am
> @@ -49,6 +49,8 @@ include_bsp_HEADERS += include/i2c.h
>  include_bsp_HEADERS += include/irq.h
>  include_bsp_HEADERS += include/zynq-uart.h
>  include_bsp_HEADERS += include/zynq-uart-regs.h
> +include_bsp_HEADERS += include/zynq-slcr.h
> +include_bsp_HEADERS += include/zynq-slcr-regs.h
>
>  include_libcpu_HEADERS = ../../../libcpu/arm/shared/include/arm-cp15.h
>
> @@ -118,6 +120,9 @@ libbsp_a_SOURCES += ../shared/arm-a9mpcore-clock-config.c
>  # I2C
>  libbsp_a_SOURCES += i2c/cadence-i2c.c
>
> +# System Level Control Registers
> +libbsp_a_SOURCES += slcr/zynq-slcr.c
> +
>  # Cache
>  libbsp_a_SOURCES += ../../../libcpu/shared/src/cache_manager.c
>  libbsp_a_SOURCES += ../shared/include/arm-cache-l1.h
> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-slcr-regs.h b/c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-slcr-regs.h
> new file mode 100644
> index 0000000..a4c8fdf
> --- /dev/null
> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-slcr-regs.h
> @@ -0,0 +1,84 @@
> +/**
> + * @file
> + * @ingroup zynq_slcr
> + * @brief SLCR register definitions.
> + */
> +
> +/*
> + * 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_slcr_regs SLCR Register Definitions
> + * @ingroup zynq_slcr
> + * @brief SLCR Register Definitions
> + */
> +
> +#ifndef LIBBSP_ARM_XILINX_ZYNQ_SLCR_REGS_H
> +#define LIBBSP_ARM_XILINX_ZYNQ_SLCR_REGS_H
> +
> +#include <bsp/utility.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +#define ZYNQ_SLCR_BASE_ADDR ( 0xF8000000 )
> +/* NOTE: QEMU gives a value of 0 for the pss_idcode. */
> +#define ZYNQ_SLCR_PSS_IDCODE_OFF ( 0x530 )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_GET( reg ) BSP_FLD32GET( reg, 12, 16 )
> +#define ZYNQ_SLCR_LVL_SHFTR_EN_OFF ( 0x900 )
> +#define ZYNQ_SLCR_LVL_SHFTR_EN_DISABLE ( 0 )
> +#define ZYNQ_SLCR_LVL_SHFTR_EN_PS_TO_PL ( 0xA )
> +#define ZYNQ_SLCR_LVL_SHFTR_EN_ALL ( 0xF )
> +#define ZYNQ_SLCR_LOCK_OFF ( 0x4 )
> +#define ZYNQ_SLCR_LOCK_KEY ( 0x767b )
> +#define ZYNQ_SLCR_UNLOCK_OFF ( 0x8 )
> +#define ZYNQ_SLCR_UNLOCK_KEY ( 0xdf0d )
> +#define ZYNQ_SLCR_FPGA_RST_CTRL_OFF ( 0x240 )

Can I please suggest the offsets for the registers be grouped together 
and then the values, offsets etc for each register grouped together? It 
makes it easier to extend what you have started to define here.

> +
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z007s ( 0x03 )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z012s ( 0x1c )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z014s ( 0x08 )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z010 ( 0x02 )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z015 ( 0x1b )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z020 ( 0x07 )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z030 ( 0x0c )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z035 ( 0x12 )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z045 ( 0x11 )
> +#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z100 ( 0x16 )
> +

Should there be some register access functions provided? For example:

static inline void zynq_slcr_write_32(const uint32_t reg,
                                       const uint32_t value)
{
   volatile uint32_t* slcr;
   slcr = (volatile uint32_t*)(ZYNQ_SLCR_BASE_ADDR + reg);
   *slcr = value;
}

static inline uint32_t zynq_slcr_read_32(const uint32_t reg)
{
   volatile uint32_t* slcr;
   return *slcr;
}

I suggest this because working with the Zynq sometime requires you trace 
the register accesses and this approach allows this. A weakness of a 
mapped struct is not being able to do that.

> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* LIBBSP_ARM_XILINX_ZYNQ_SLCR_REGS_H */
> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-slcr.h b/c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-slcr.h
> new file mode 100644
> index 0000000..2303ff5
> --- /dev/null
> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/include/zynq-slcr.h
> @@ -0,0 +1,70 @@
> +/**
> + * @file
> + * @ingroup zynq_slcr
> + * @brief SLCR support.
> + */
> +
> +/*
> + *
> + * 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_slcr SLCR Support
> + * @ingroup arm_zynq
> + * @brief SLCR Support
> + */
> +
> +#ifndef LIBBSP_ARM_XILINX_ZYNQ_SLCR_H
> +#define LIBBSP_ARM_XILINX_ZYNQ_SLCR_H
> +
> +#include <stdint.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +uint32_t zynq_slcr_pss_idcode_get( void );
> +
> +void zynq_slcr_fpga_clk_rst(
> +  uint32_t val
> +);
> +
> +void zynq_slcr_level_shifter_enable(
> +  uint32_t val
> +);

Any chance you could provide info about the calls?

> +
> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
> +
> +#endif /* LIBBSP_ARM_XILINX_ZYNQ_SLCR_H */
> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/preinstall.am b/c/src/lib/libbsp/arm/xilinx-zynq/preinstall.am
> index a75c344..814095f 100644
> --- a/c/src/lib/libbsp/arm/xilinx-zynq/preinstall.am
> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/preinstall.am
> @@ -150,6 +150,14 @@ $(PROJECT_INCLUDE)/bsp/zynq-uart-regs.h: include/zynq-uart-regs.h $(PROJECT_INCL
>  	$(INSTALL_DATA) $< $(PROJECT_INCLUDE)/bsp/zynq-uart-regs.h
>  PREINSTALL_FILES += $(PROJECT_INCLUDE)/bsp/zynq-uart-regs.h
>
> +$(PROJECT_INCLUDE)/bsp/zynq-slcr.h: include/zynq-slcr.h $(PROJECT_INCLUDE)/bsp/$(dirstamp)
> +	$(INSTALL_DATA) $< $(PROJECT_INCLUDE)/bsp/zynq-slcr.h
> +PREINSTALL_FILES += $(PROJECT_INCLUDE)/bsp/zynq-slcr.h
> +
> +$(PROJECT_INCLUDE)/bsp/zynq-slcr-regs.h: include/zynq-slcr-regs.h $(PROJECT_INCLUDE)/bsp/$(dirstamp)
> +	$(INSTALL_DATA) $< $(PROJECT_INCLUDE)/bsp/zynq-slcr-regs.h
> +PREINSTALL_FILES += $(PROJECT_INCLUDE)/bsp/zynq-slcr-regs.h
> +
>  $(PROJECT_INCLUDE)/libcpu/arm-cp15.h: ../../../libcpu/arm/shared/include/arm-cp15.h $(PROJECT_INCLUDE)/libcpu/$(dirstamp)
>  	$(INSTALL_DATA) $< $(PROJECT_INCLUDE)/libcpu/arm-cp15.h
>  PREINSTALL_FILES += $(PROJECT_INCLUDE)/libcpu/arm-cp15.h
> diff --git a/c/src/lib/libbsp/arm/xilinx-zynq/slcr/zynq-slcr.c b/c/src/lib/libbsp/arm/xilinx-zynq/slcr/zynq-slcr.c
> new file mode 100644
> index 0000000..8b10722
> --- /dev/null
> +++ b/c/src/lib/libbsp/arm/xilinx-zynq/slcr/zynq-slcr.c
> @@ -0,0 +1,90 @@
> +/*
> + * SLCR Support Implementation
> + *
> + * At this point, only a few operations related to programming the FPGA are
> + * supported.
> + *
> + * 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>
> + */
> +#include <stdint.h>
> +#include <bsp/zynq-slcr.h>
> +#include <bsp/zynq-slcr-regs.h>
> +
> +static void slcr_unlock( void )
> +{
> +  volatile uint32_t *unlock;
> +
> +  unlock = (uint32_t *)( ZYNQ_SLCR_BASE_ADDR + ZYNQ_SLCR_UNLOCK_OFF );
> +  *unlock = ZYNQ_SLCR_UNLOCK_KEY;
> +}

      zynq_slcr_write_32(ZYNQ_SLCR_UNLOCK_OFF, ZYNQ_SLCR_UNLOCK_KEY);
?? :)

Doing this lets a simple grep can find all the accesses, plus we do not 
need to remember or check all the accesses are volatile.

> +
> +static void slcr_lock( void )
> +{
> +  volatile uint32_t *lock;
> +
> +  lock = (uint32_t *)( ZYNQ_SLCR_BASE_ADDR + ZYNQ_SLCR_LOCK_OFF );
> +  *lock = ZYNQ_SLCR_LOCK_KEY;

> +}
> +
> +void zynq_slcr_fpga_clk_rst(
> +  uint32_t val
> +)
> +{
> +  volatile uint32_t *fpga_rst_ctrl;
> +
> +  fpga_rst_ctrl =
> +      (uint32_t *)( ZYNQ_SLCR_BASE_ADDR + ZYNQ_SLCR_FPGA_RST_CTRL_OFF );
> +  slcr_unlock();
> +  *fpga_rst_ctrl = 0xf & val;
> +  slcr_lock();

I so not like the reset value being written to like this. I know you 
need to set the lower 4 bits to 1 when loading the fabric via PCAP but 
there are 4 independent resets and I have applications that have 
individual control once loaded. What about a mask be provided and only 
the bits we are interested in are set or cleared depending on the mask?

Note adding mask support will require a lock to be SMP safe.

> +}
> +
> +void zynq_slcr_level_shifter_enable(
> +  uint32_t val
> +)
> +{
> +  volatile uint32_t *lvl_shftr_en;
> +
> +  lvl_shftr_en =
> +      (uint32_t *)( ZYNQ_SLCR_BASE_ADDR + ZYNQ_SLCR_LVL_SHFTR_EN_OFF );
> +  slcr_unlock();
> +  *lvl_shftr_en = val;
> +  slcr_lock();
> +}
> +
> +uint32_t zynq_slcr_pss_idcode_get ( void )
> +{
> +  volatile uint32_t *pss_idcode;
> +
> +  pss_idcode = (uint32_t *)( ZYNQ_SLCR_BASE_ADDR + ZYNQ_SLCR_PSS_IDCODE_OFF );
> +  return *pss_idcode;
> +}
>


More information about the devel mailing list