<div dir="ltr">Chris,<div><br></div><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="m_9194902027365710539m_-2726649208165875943m_-5336791597524528872h5"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
+<br>
+#define ZYNQ_SLCR_BASE_ADDR ( 0xF8000000 )<br>
+/* NOTE: QEMU gives a value of 0 for the pss_idcode. */<br>
+#define ZYNQ_SLCR_PSS_IDCODE_OFF ( 0x530 )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_GE<wbr>T( reg ) BSP_FLD32GET( reg, 12, 16 )<br>
+#define ZYNQ_SLCR_LVL_SHFTR_EN_OFF ( 0x900 )<br>
+#define ZYNQ_SLCR_LVL_SHFTR_EN_DISABLE ( 0 )<br>
+#define ZYNQ_SLCR_LVL_SHFTR_EN_PS_TO_P<wbr>L ( 0xA )<br>
+#define ZYNQ_SLCR_LVL_SHFTR_EN_ALL ( 0xF )<br>
+#define ZYNQ_SLCR_LOCK_OFF ( 0x4 )<br>
+#define ZYNQ_SLCR_LOCK_KEY ( 0x767b )<br>
+#define ZYNQ_SLCR_UNLOCK_OFF ( 0x8 )<br>
+#define ZYNQ_SLCR_UNLOCK_KEY ( 0xdf0d )<br>
+#define ZYNQ_SLCR_FPGA_RST_CTRL_OFF ( 0x240 )<br>
</blockquote>
<br></div></div>
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.</blockquote><div><br></div><div>Sure, I agree.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>007s ( 0x03 )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>012s ( 0x1c )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>014s ( 0x08 )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>010 ( 0x02 )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>015 ( 0x1b )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>020 ( 0x07 )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>030 ( 0x0c )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>035 ( 0x12 )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>045 ( 0x11 )<br>
+#define ZYNQ_SLCR_PSS_IDCODE_DEVICE_7z<wbr>100 ( 0x16 )<br>
+<br>
</blockquote>
<br></span>
Should there be some register access functions provided? For example:<br>
<br>
static inline void zynq_slcr_write_32(const uint32_t reg,<br>
                                      const uint32_t value)<br>
{<br>
  volatile uint32_t* slcr;<br>
  slcr = (volatile uint32_t*)(ZYNQ_SLCR_BASE_ADDR + reg);<br>
  *slcr = value;<br>
}<br>
<br>
static inline uint32_t zynq_slcr_read_32(const uint32_t reg)<br>
{<br>
  volatile uint32_t* slcr;<br>
  return *slcr;<br>
}<br>
<br>
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.</blockquote><div><br></div><div>Makes sense, I will add register access functions.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="m_9194902027365710539m_-2726649208165875943m_-5336791597524528872h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+#ifdef __cplusplus<br>
+}<br>
+#endif /* __cplusplus */<br>
+<br>
+#endif /* LIBBSP_ARM_XILINX_ZYNQ_SLCR_RE<wbr>GS_H */<br>
diff --git a/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/include/zynq-slcr.h b/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/include/zynq-slcr.h<br>
new file mode 100644<br>
index 0000000..2303ff5<br>
--- /dev/null<br>
+++ b/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/include/zynq-slcr.h<br>
@@ -0,0 +1,70 @@<br>
+/**<br>
+ * @file<br>
+ * @ingroup zynq_slcr<br>
+ * @brief SLCR support.<br>
+ */<br>
+<br>
+/*<br>
+ *<br>
+ * Copyright (c) 2017<br>
+ *  NSF Center for High-Performance Reconfigurable Computing (CHREC),<br>
+ *  University of Pittsburgh.  All rights reserved.<br>
+ *<br>
+ * Redistribution and use in source and binary forms, with or without<br>
+ * modification, are permitted provided that the following conditions are<br>
+ * met:<br>
+ * 1. Redistributions of source code must retain the above copyright<br>
+ *    notice, this list of conditions and the following disclaimer.<br>
+ * 2. Redistributions in binary form must reproduce the above copyright<br>
+ *    notice, this list of conditions and the following disclaimer in the<br>
+ *    documentation and/or other materials provided with the distribution.<br>
+ *<br>
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS<br>
+ * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED<br>
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A<br>
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER<br>
+ * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,<br>
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,<br>
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR<br>
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF<br>
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING<br>
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS<br>
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.<br>
+ *<br>
+ * The views and conclusions contained in the software and documentation<br>
+ * are those of the authors and should not be interpreted as representing<br>
+ * official policies, either expressed or implied, of CHREC.<br>
+ *<br>
+ * Author: Patrick Gauvin <<a href="mailto:gauvin@hcs.ufl.edu" target="_blank">gauvin@hcs.ufl.edu</a>><br>
+ */<br>
+<br>
+/**<br>
+ * @defgroup zynq_slcr SLCR Support<br>
+ * @ingroup arm_zynq<br>
+ * @brief SLCR Support<br>
+ */<br>
+<br>
+#ifndef LIBBSP_ARM_XILINX_ZYNQ_SLCR_H<br>
+#define LIBBSP_ARM_XILINX_ZYNQ_SLCR_H<br>
+<br>
+#include <stdint.h><br>
+<br>
+#ifdef __cplusplus<br>
+extern "C" {<br>
+#endif /* __cplusplus */<br>
+<br>
+uint32_t zynq_slcr_pss_idcode_get( void );<br>
+<br>
+void zynq_slcr_fpga_clk_rst(<br>
+  uint32_t val<br>
+);<br>
+<br>
+void zynq_slcr_level_shifter_enable<wbr>(<br>
+  uint32_t val<br>
+);<br>
</blockquote>
<br></div></div>
Any chance you could provide info about the calls?</blockquote><div><br></div><div>Sure, I'll add Doxygen comments.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="m_9194902027365710539m_-2726649208165875943m_-5336791597524528872h5"><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+#ifdef __cplusplus<br>
+}<br>
+#endif /* __cplusplus */<br>
+<br>
+#endif /* LIBBSP_ARM_XILINX_ZYNQ_SLCR_H */<br>
diff --git a/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/<a href="http://preinstall.am" rel="noreferrer" target="_blank">preinstall.am</a> b/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/<a href="http://preinstall.am" rel="noreferrer" target="_blank">preinstall.am</a><br>
index a75c344..814095f 100644<br>
--- a/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/<a href="http://preinstall.am" rel="noreferrer" target="_blank">preinstall.am</a><br>
+++ b/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/<a href="http://preinstall.am" rel="noreferrer" target="_blank">preinstall.am</a><br>
@@ -150,6 +150,14 @@ $(PROJECT_INCLUDE)/bsp/zynq-ua<wbr>rt-regs.h: include/zynq-uart-regs.h $(PROJECT_INCL<br>
        $(INSTALL_DATA) $< $(PROJECT_INCLUDE)/bsp/zynq-ua<wbr>rt-regs.h<br>
 PREINSTALL_FILES += $(PROJECT_INCLUDE)/bsp/zynq-ua<wbr>rt-regs.h<br>
<br>
+$(PROJECT_INCLUDE)/bsp/zynq-s<wbr>lcr.h: include/zynq-slcr.h $(PROJECT_INCLUDE)/bsp/$(dirst<wbr>amp)<br>
+       $(INSTALL_DATA) $< $(PROJECT_INCLUDE)/bsp/zynq-sl<wbr>cr.h<br>
+PREINSTALL_FILES += $(PROJECT_INCLUDE)/bsp/zynq-sl<wbr>cr.h<br>
+<br>
+$(PROJECT_INCLUDE)/bsp/zynq-s<wbr>lcr-regs.h: include/zynq-slcr-regs.h $(PROJECT_INCLUDE)/bsp/$(dirst<wbr>amp)<br>
+       $(INSTALL_DATA) $< $(PROJECT_INCLUDE)/bsp/zynq-sl<wbr>cr-regs.h<br>
+PREINSTALL_FILES += $(PROJECT_INCLUDE)/bsp/zynq-sl<wbr>cr-regs.h<br>
+<br>
 $(PROJECT_INCLUDE)/libcpu/arm<wbr>-cp15.h: ../../../libcpu/arm/shared/inc<wbr>lude/arm-cp15.h $(PROJECT_INCLUDE)/libcpu/$(di<wbr>rstamp)<br>
        $(INSTALL_DATA) $< $(PROJECT_INCLUDE)/libcpu/arm-<wbr>cp15.h<br>
 PREINSTALL_FILES += $(PROJECT_INCLUDE)/libcpu/arm-<wbr>cp15.h<br>
diff --git a/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/slcr/zynq-slcr.c b/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/slcr/zynq-slcr.c<br>
new file mode 100644<br>
index 0000000..8b10722<br>
--- /dev/null<br>
+++ b/c/src/lib/libbsp/arm/xilinx-<wbr>zynq/slcr/zynq-slcr.c<br>
@@ -0,0 +1,90 @@<br>
+/*<br>
+ * SLCR Support Implementation<br>
+ *<br>
+ * At this point, only a few operations related to programming the FPGA are<br>
+ * supported.<br>
+ *<br>
+ * Copyright (c) 2017<br>
+ *  NSF Center for High-Performance Reconfigurable Computing (CHREC),<br>
+ *  University of Pittsburgh.  All rights reserved.<br>
+ *<br>
+ * Redistribution and use in source and binary forms, with or without<br>
+ * modification, are permitted provided that the following conditions are<br>
+ * met:<br>
+ * 1. Redistributions of source code must retain the above copyright<br>
+ *    notice, this list of conditions and the following disclaimer.<br>
+ * 2. Redistributions in binary form must reproduce the above copyright<br>
+ *    notice, this list of conditions and the following disclaimer in the<br>
+ *    documentation and/or other materials provided with the distribution.<br>
+ *<br>
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS<br>
+ * IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED<br>
+ * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A<br>
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER<br>
+ * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,<br>
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,<br>
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR<br>
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF<br>
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING<br>
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS<br>
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.<br>
+ *<br>
+ * The views and conclusions contained in the software and documentation<br>
+ * are those of the authors and should not be interpreted as representing<br>
+ * official policies, either expressed or implied, of CHREC.<br>
+ *<br>
+ * Author: Patrick Gauvin <<a href="mailto:gauvin@hcs.ufl.edu" target="_blank">gauvin@hcs.ufl.edu</a>><br>
+ */<br>
+#include <stdint.h><br>
+#include <bsp/zynq-slcr.h><br>
+#include <bsp/zynq-slcr-regs.h><br>
+<br>
+static void slcr_unlock( void )<br>
+{<br>
+  volatile uint32_t *unlock;<br>
+<br>
+  unlock = (uint32_t *)( ZYNQ_SLCR_BASE_ADDR + ZYNQ_SLCR_UNLOCK_OFF );<br>
+  *unlock = ZYNQ_SLCR_UNLOCK_KEY;<br>
+}<br>
</blockquote>
<br></div></div>
     zynq_slcr_write_32(ZYNQ_SLCR_<wbr>UNLOCK_OFF, ZYNQ_SLCR_UNLOCK_KEY);<br>
?? :)<br>
<br>
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.<span><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+<br>
+static void slcr_lock( void )<br>
+{<br>
+  volatile uint32_t *lock;<br>
+<br>
+  lock = (uint32_t *)( ZYNQ_SLCR_BASE_ADDR + ZYNQ_SLCR_LOCK_OFF );<br>
+  *lock = ZYNQ_SLCR_LOCK_KEY;<br>
</blockquote>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+}<br>
+<br>
+void zynq_slcr_fpga_clk_rst(<br>
+  uint32_t val<br>
+)<br>
+{<br>
+  volatile uint32_t *fpga_rst_ctrl;<br>
+<br>
+  fpga_rst_ctrl =<br>
+      (uint32_t *)( ZYNQ_SLCR_BASE_ADDR + ZYNQ_SLCR_FPGA_RST_CTRL_OFF );<br>
+  slcr_unlock();<br>
+  *fpga_rst_ctrl = 0xf & val;<br>
+  slcr_lock();<br>
</blockquote>
<br></span>
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?<br>
<br>
Note adding mask support will require a lock to be SMP safe.</blockquote><div><br></div><div>I see, masking sounds good. Since the lock will need to be initialized before the SLCR functions are called, should I turn this into a full driver like device config, or is it acceptable to call an SLCR init function during BSP startup?</div><div><br></div></div></div></div><div class="gmail_extra">Thank you for the quick feedback,</div><div class="gmail_extra"><br></div><div class="gmail_extra">Patrick</div></div>