Re: [PATCH v1 2/4] aarch64/versal: Add flash wrapper for Xilinx GQSPI

2023-10-19 Thread Kinsey Moore
Comments inline below.

On Thu, Oct 19, 2023 at 12:43 AM  wrote:

> From: Aaron Nyholm 
>
> ---
>  .../dev/spi/versal_xqspi_flash.c  | 296 ++
>  bsps/aarch64/xilinx-versal/include/bsp/irq.h  |   1 +
>  .../include/dev/spi/versal_xqspi_flash.h  |  49 +++
>  bsps/include/dev/spi/xqspipsu-flash-helper.h  |  24 ++
>  bsps/shared/dev/spi/xqspipsu-flash-helper.c   |  16 +
>  spec/build/bsps/aarch64/xilinx-versal/grp.yml |   2 +
>  .../aarch64/xilinx-versal/objxqspiflash.yml   |  24 ++
>  7 files changed, 412 insertions(+)
>  create mode 100644 bsps/aarch64/xilinx-versal/dev/spi/versal_xqspi_flash.c
>  create mode 100644
> bsps/aarch64/xilinx-versal/include/dev/spi/versal_xqspi_flash.h
>  create mode 100644 spec/build/bsps/aarch64/xilinx-versal/objxqspiflash.yml
>
> diff --git a/bsps/aarch64/xilinx-versal/dev/spi/versal_xqspi_flash.c
> b/bsps/aarch64/xilinx-versal/dev/spi/versal_xqspi_flash.c
> new file mode 100644
> index 00..7771af9dcd
> --- /dev/null
> +++ b/bsps/aarch64/xilinx-versal/dev/spi/versal_xqspi_flash.c
> @@ -0,0 +1,296 @@
> +/*
> + * Copyright (C) 2023, 2023 Aaron Nyholm
> + *
> + * 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.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +#include 
> +
> +
> +uint32_t xqspi_get_jedec_id(rtems_flashdev *flash);
> +
> +int xqspi_get_flash_type(
> +  rtems_flashdev *flash,
> +  rtems_flashdev_flash_type *type
> +);
> +
> +int xqspi_page_info_by_off(
> +  rtems_flashdev *flash,
> +  off_t search_offset,
> +  off_t *page_offset,
> +  size_t *page_size
> +);
> +
> +int xqspi_page_info_by_index(
> +  rtems_flashdev *flash,
> +  off_t search_index,
> +  off_t *page_offset,
> +  size_t *page_size
> +);
> +
> +int xqspi_page_count(
> +  rtems_flashdev *flash,
> +  int *page_count
> +);
> +
> +int xqspi_write_block_size(
> +  rtems_flashdev *flash,
> +  size_t *write_block_size
> +);
> +
> +int xqspi_read_wrapper(rtems_flashdev *flash,
> +  uintptr_t offset,
> +  size_t count,
> +  void *buffer
> +);
> +
> +int xqspi_write_wrapper(
> +  rtems_flashdev *flash,
> +  uintptr_t offset,
> +  size_t count,
> +  const void *buffer
> +);
> +
> +int xqspi_erase_wrapper(
> +  rtems_flashdev *flash,
> +  uintptr_t offset,
> +  size_t count
> +);
>

The above prototypes are unnecessary and the corresponding functions below
should be marked static so as not to require them.


> +
> +uint32_t xqspi_get_jedec_id(rtems_flashdev *flash) {
> +  return QspiPsu_NOR_Get_JEDEC_ID(flash->driver);
> +}
>

Side commentary: I don't really like that this NOR driver has as much
untracked internal state as it does, but that's how it came from Xilinx. An
alternative would be to read the 3 relevant bytes using QspiPsu_NOR_RDID.
Longer term I'd like to get that state pulled out into a NOR
management/wrapper struct that holds the raw XQspiPsu driver struct.


> +
> +int xqspi_get_flash_type(
> +  rtems_flashdev *flash,
> +  rtems_flashdev_flash_type *type
> +)
> +{
> +  *type = RTEMS_FLASHDEV_NOR;
> +  return 0;
> +}
> +
> +int xqspi_read_wrapper(
> +rtems_flashdev *flash,
> +uintptr_t offset,
> +size_t count,
> +void *buffer
> +)
> +{
> +  XQspiPsu *flash_driver = (XQspiPsu*)flash->driver;
> +  uint8_t *tmp_buffer;
> +  int status;
> +  int startAlign = 0;
> +
> +  /* Align offset to two byte boundary */
> +  if (offset%2) {
> +startAlign = 1;
> +offset = offset - 1;
> +count = count + 1;
> +  }
> +
> +  while (count > MAX_READ_SIZE) {
> +/* Read block and copy to buffer */
> +status = QspiPsu_NOR_Read(flash_driver, (uint32_t)offset,
> MAX_READ_SIZE, _buffer);
> +
> +if (status == 0) 

[PATCH v1 2/4] aarch64/versal: Add flash wrapper for Xilinx GQSPI

2023-10-18 Thread aaron . nyholm
From: Aaron Nyholm 

---
 .../dev/spi/versal_xqspi_flash.c  | 296 ++
 bsps/aarch64/xilinx-versal/include/bsp/irq.h  |   1 +
 .../include/dev/spi/versal_xqspi_flash.h  |  49 +++
 bsps/include/dev/spi/xqspipsu-flash-helper.h  |  24 ++
 bsps/shared/dev/spi/xqspipsu-flash-helper.c   |  16 +
 spec/build/bsps/aarch64/xilinx-versal/grp.yml |   2 +
 .../aarch64/xilinx-versal/objxqspiflash.yml   |  24 ++
 7 files changed, 412 insertions(+)
 create mode 100644 bsps/aarch64/xilinx-versal/dev/spi/versal_xqspi_flash.c
 create mode 100644 
bsps/aarch64/xilinx-versal/include/dev/spi/versal_xqspi_flash.h
 create mode 100644 spec/build/bsps/aarch64/xilinx-versal/objxqspiflash.yml

diff --git a/bsps/aarch64/xilinx-versal/dev/spi/versal_xqspi_flash.c 
b/bsps/aarch64/xilinx-versal/dev/spi/versal_xqspi_flash.c
new file mode 100644
index 00..7771af9dcd
--- /dev/null
+++ b/bsps/aarch64/xilinx-versal/dev/spi/versal_xqspi_flash.c
@@ -0,0 +1,296 @@
+/*
+ * Copyright (C) 2023, 2023 Aaron Nyholm
+ *
+ * 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.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+
+uint32_t xqspi_get_jedec_id(rtems_flashdev *flash);
+
+int xqspi_get_flash_type(
+  rtems_flashdev *flash,
+  rtems_flashdev_flash_type *type
+);
+
+int xqspi_page_info_by_off(
+  rtems_flashdev *flash,
+  off_t search_offset,
+  off_t *page_offset,
+  size_t *page_size
+);
+
+int xqspi_page_info_by_index(
+  rtems_flashdev *flash,
+  off_t search_index,
+  off_t *page_offset,
+  size_t *page_size
+);
+
+int xqspi_page_count(
+  rtems_flashdev *flash,
+  int *page_count
+);
+
+int xqspi_write_block_size(
+  rtems_flashdev *flash,
+  size_t *write_block_size
+);
+
+int xqspi_read_wrapper(rtems_flashdev *flash,
+  uintptr_t offset,
+  size_t count,
+  void *buffer
+);
+
+int xqspi_write_wrapper(
+  rtems_flashdev *flash,
+  uintptr_t offset,
+  size_t count,
+  const void *buffer
+);
+
+int xqspi_erase_wrapper(
+  rtems_flashdev *flash,
+  uintptr_t offset,
+  size_t count
+);
+
+uint32_t xqspi_get_jedec_id(rtems_flashdev *flash) {
+  return QspiPsu_NOR_Get_JEDEC_ID(flash->driver);
+}
+
+int xqspi_get_flash_type(
+  rtems_flashdev *flash,
+  rtems_flashdev_flash_type *type
+)
+{
+  *type = RTEMS_FLASHDEV_NOR;
+  return 0;
+}
+
+int xqspi_read_wrapper(
+rtems_flashdev *flash,
+uintptr_t offset,
+size_t count,
+void *buffer
+)
+{
+  XQspiPsu *flash_driver = (XQspiPsu*)flash->driver;
+  uint8_t *tmp_buffer;
+  int status;
+  int startAlign = 0;
+
+  /* Align offset to two byte boundary */
+  if (offset%2) {
+startAlign = 1;
+offset = offset - 1;
+count = count + 1;
+  }
+
+  while (count > MAX_READ_SIZE) {
+/* Read block and copy to buffer */
+status = QspiPsu_NOR_Read(flash_driver, (uint32_t)offset, MAX_READ_SIZE, 
_buffer);
+
+if (status == 0) {
+  memcpy(buffer, tmp_buffer + startAlign, MAX_READ_SIZE - startAlign);
+  /* Update count, offset and buffer pointer */
+  count = count - MAX_READ_SIZE;
+  buffer = buffer + MAX_READ_SIZE - startAlign;
+  offset = offset + MAX_READ_SIZE;
+  /* Clear startAlign once first block read */
+  if (startAlign) {
+startAlign = 0;
+  }
+} else {
+  return status;
+}
+  }
+
+  status = QspiPsu_NOR_Read(flash_driver, (uint32_t)offset, (uint32_t)count, 
_buffer);
+
+  if (status == 0) {
+memcpy(buffer, tmp_buffer + startAlign, count);
+  }
+  return status;
+}
+
+int xqspi_page_info_by_off(
+  rtems_flashdev *flash,
+  off_t search_offset,
+  off_t *page_offset,
+  size_t *page_size
+)
+{
+  *page_size = QspiPsu_NOR_Get_Sector_Size(flash->driver);
+  *page_offset = search_offset -