Re: [PATCH v1 2/4] aarch64/versal: Add flash wrapper for Xilinx GQSPI
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, &tmp_buffer);
> +
> +if (status ==
[PATCH v1 2/4] aarch64/versal: Add flash wrapper for Xilinx GQSPI
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,
&tmp_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,
&tmp_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 - (se
