Comments inline below.
On Thu, Oct 19, 2023 at 12:43 AM <aaron.nyh...@unfoldedeffective.com> wrote:
> From: Aaron Nyholm <aaron.nyh...@southerninnovation.com>
>
> ---
> .../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 0000000000..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 <bsp/irq.h>
> +#include <dev/spi/versal_xqspi_flash.h>
> +#include <dev/spi/xqspipsu.h>
> +#include <dev/spi/xqspipsu-flash-helper.h>
> +#include <dev/spi/xqspipsu_flash_config.h>
> +
> +#include <stdlib.h>
> +#include <string.h>
> +
> +
> +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 == 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 - (search_offset%((off_t)(*page_size)));
> + return 0;
> +}
> +
> +int xqspi_page_info_by_index(
> + rtems_flashdev *flash,
> + off_t search_index,
> + off_t *page_offset,
> + size_t *page_size
> +)
> +{
> + *page_size = QspiPsu_NOR_Get_Page_Size(flash->driver);
> + *page_offset = *page_size * search_index;
> + return 0;
> +}
>
You're mixing page size and sector size in these two functions.
+
> +int xqspi_page_count(
> + rtems_flashdev *flash,
> + int *page_count
> +)
> +{
> + *page_count = QspiPsu_NOR_Get_Device_Size(flash->driver) /
> + QspiPsu_NOR_Get_Page_Size(flash->driver);
> + return 0;
> +}
> +
> +int xqspi_write_block_size(
> + rtems_flashdev *flash,
> + size_t *write_block_size
> +)
> +{
> + *write_block_size = QspiPsu_NOR_Get_Page_Size(flash->driver);
> + return 0;
> +}
> +
> +int xqspi_write_wrapper(
> + rtems_flashdev *flash,
> + uintptr_t offset,
> + size_t count,
> + const void *buffer
> +)
> +{
> + XQspiPsu *flash_driver = (XQspiPsu*)flash->driver;
> + return QspiPsu_NOR_Write(flash_driver, (uint32_t)offset,
> (uint32_t)count, (void*)buffer);
> +}
> +
> +int xqspi_erase_wrapper(
> + rtems_flashdev *flash,
> + uintptr_t offset,
> + size_t count
> +)
> +{
> + XQspiPsu *flash_driver = (XQspiPsu*)flash->driver;
> + return QspiPsu_NOR_Erase(flash_driver, (uint32_t)offset,
> (uint32_t)count);
> +}
> +
> +rtems_flashdev* xqspi_flash_init(void)
>
I'd recommend that this be passed an initialized XQspiPsu pointer and this
flashdev adapter should be moved to bsps/shared so that it can be used by
any consumer of XQspiPsu. Is there anything preventing this?
+{
> + int status;
> + XQspiPsu *xQspiDev;
> + XQspiPsu_Config *xQspiDev_Config;
> +
> + xQspiDev = calloc(1, sizeof(XQspiPsu));
> + if (xQspiDev == NULL) {
> + return NULL;
> + }
> + xQspiDev_Config = calloc(1, sizeof(XQspiPsu_Config));
> + if (xQspiDev_Config == NULL) {
> + free(xQspiDev);
> + return NULL;
> + }
> +
> + xQspiDev_Config->DeviceId = 0;
> + xQspiDev_Config->BaseAddress = XQSPIPS_BASEADDR;
> + xQspiDev_Config->InputClockHz = VERSAL_QSPI_INPUT_CLOCK_HZ;
> + xQspiDev_Config->ConnectionMode = XQSPIPSU_CONNECTION_MODE_PARALLEL;
> + xQspiDev_Config->BusWidth = 2;
> + xQspiDev_Config->IsCacheCoherent = 0;
> +
> + status = XQspiPsu_CfgInitialize(xQspiDev, xQspiDev_Config,
> XQSPIPS_BASEADDR);
> +
> + if (status != 0) {
> + free(xQspiDev);
> + free(xQspiDev_Config);
> + return NULL;
> + }
> +
> + versal_xqspi_region_table *xtable =
> + calloc(1, sizeof(versal_xqspi_region_table));
> +
> + if (xtable == NULL) {
> + free(xQspiDev);
> + free(xQspiDev_Config);
> + return NULL;
> + }
> +
> + rtems_flashdev_region_table *ftable =
> + calloc(1, sizeof(rtems_flashdev_region_table));
> +
> + if (ftable == NULL) {
> + free(ftable);
> + free(xQspiDev);
> + free(xQspiDev_Config);
> + return NULL;
> + }
> +
> + ftable->regions =
> (rtems_flashdev_region*)(&(xtable->versal_xqspi_regions));
> + ftable->max_regions = VERSAL_XQSPI_MAX_REGIONS;
> + ftable->bit_allocator = &(xtable->versal_xqspi_bit_allocator);
> +
> + status = QspiPsu_NOR_Initialize(
> + xQspiDev,
> + VERSAL_IRQ_QSPI
> + );
> +
> + if (status != 0) {
> + free(xQspiDev);
> + free(xQspiDev_Config);
> + return NULL;
> + }
> +
> + rtems_flashdev *flash =
> rtems_flashdev_alloc_and_init(sizeof(rtems_flashdev));
> +
> + if (flash == NULL) {
> + free(xQspiDev);
> + free(xQspiDev_Config);
> + return NULL;
> + }
> +
> + flash->driver = xQspiDev;
> + flash->read = &xqspi_read_wrapper;
> + flash->write = &xqspi_write_wrapper;
> + flash->erase = &xqspi_erase_wrapper;
> + flash->jedec_id = &xqspi_get_jedec_id;
> + flash->flash_type = &xqspi_get_flash_type;
> + flash->page_info_by_offset = &xqspi_page_info_by_off;
> + flash->page_info_by_index = &xqspi_page_info_by_index;
> + flash->page_count = &xqspi_page_count;
> + flash->write_block_size = &xqspi_write_block_size;
> + flash->region_table = ftable;
>
This seems to be missing a destroy call to clean up the various allocations.
> +
> + return flash;
> +
> +}
> diff --git a/bsps/aarch64/xilinx-versal/include/bsp/irq.h
> b/bsps/aarch64/xilinx-versal/include/bsp/irq.h
> index b34bdfd345..6f4d387e8f 100644
> --- a/bsps/aarch64/xilinx-versal/include/bsp/irq.h
> +++ b/bsps/aarch64/xilinx-versal/include/bsp/irq.h
> @@ -61,6 +61,7 @@ extern "C" {
> #define VERSAL_IRQ_ETHERNET_0_WAKEUP 89
> #define VERSAL_IRQ_ETHERNET_1 90
> #define VERSAL_IRQ_ETHERNET_1_WAKEUP 91
> +#define VERSAL_IRQ_QSPI 157
>
> /** @} */
>
> diff --git
> a/bsps/aarch64/xilinx-versal/include/dev/spi/versal_xqspi_flash.h
> b/bsps/aarch64/xilinx-versal/include/dev/spi/versal_xqspi_flash.h
> new file mode 100644
> index 0000000000..fe0ffd3e71
> --- /dev/null
> +++ b/bsps/aarch64/xilinx-versal/include/dev/spi/versal_xqspi_flash.h
> @@ -0,0 +1,49 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/*
> + * 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.
> + */
> +
> +#ifndef LIBBSP_AARCH64_XILINX_VERSAL_XQSPI_FLASH_H
> +#define LIBBSP_AARCH64_XILINX_VERSAL_XQSPI_FLASH_H
> +
> +#include <dev/flash/flashdev.h>
> +
> +#define VERSAL_XQSPI_MAX_REGIONS 32
> +#define VERSAL_QSPI_INPUT_CLOCK_HZ 295833038
> +#define MAX_READ_SIZE 0x8000
> +
> +/*
> + * Initalize a qspi_flash device using Xilinx's qspi flash
> + * driver. Device still needs to be registered using
> + * qspi_flash_register in qspi_flash.c.
> +*/
> +rtems_flashdev* xqspi_flash_init(void);
> +
> +typedef struct versal_xqspi_region_table {
> + rtems_flashdev_region versal_xqspi_regions[VERSAL_XQSPI_MAX_REGIONS];
> + uint32_t versal_xqspi_bit_allocator;
> +} versal_xqspi_region_table;
> +
> +#endif /* LIBBSP_AARCH64_XILINX_VERSAL_XQSPI_FLASH_H */
> diff --git a/bsps/include/dev/spi/xqspipsu-flash-helper.h
> b/bsps/include/dev/spi/xqspipsu-flash-helper.h
> index 5e4233e64e..69660d28f7 100644
> --- a/bsps/include/dev/spi/xqspipsu-flash-helper.h
> +++ b/bsps/include/dev/spi/xqspipsu-flash-helper.h
> @@ -155,3 +155,27 @@ u32 QspiPsu_NOR_Get_Device_Size(XQspiPsu *QspiPsuPtr);
> *
>
> ******************************************************************************/
> u32 QspiPsu_NOR_Get_Sector_Size(XQspiPsu *QspiPsuPtr);
> +
>
> +/*****************************************************************************/
> +/**
> + *
> + * This function returns the page size of attached flash parts.
> + *
> + * @param QspiPsuPtr is a pointer to the QSPIPSU driver component to
> use.
> + *
> + * @return The page size of attached flash in bytes.
> + *
> +
> ******************************************************************************/
> +u32 QspiPsu_NOR_Get_Page_Size(XQspiPsu *QspiPsuPtr);
> +
>
> +/*****************************************************************************/
> +/**
> + *
> + * This function returns the JEDEC ID of attached flash parts.
> + *
> + * @param QspiPsuPtr is a pointer to the QSPIPSU driver component to
> use.
> + *
> + * @return The JEDEC ID of attached flash in bytes.
> + *
> +
> ******************************************************************************/
> +u32 QspiPsu_NOR_Get_JEDEC_ID(XQspiPsu *QspiPsuPtr);
> diff --git a/bsps/shared/dev/spi/xqspipsu-flash-helper.c
> b/bsps/shared/dev/spi/xqspipsu-flash-helper.c
> index c9d8273b87..1795c9756b 100644
> --- a/bsps/shared/dev/spi/xqspipsu-flash-helper.c
> +++ b/bsps/shared/dev/spi/xqspipsu-flash-helper.c
> @@ -2277,3 +2277,19 @@ u32 QspiPsu_NOR_Get_Device_Size(XQspiPsu
> *QspiPsuPtr)
> }
> return Flash_Config_Table[FCTIndex].FlashDeviceSize;
> }
> +
> +u32 QspiPsu_NOR_Get_Page_Size(XQspiPsu *QspiPsuPtr)
> +{
> + if(QspiPsuPtr->Config.ConnectionMode ==
> XQSPIPSU_CONNECTION_MODE_STACKED) {
> + return Flash_Config_Table[FCTIndex].PageSize * 2;
> + }
> + return Flash_Config_Table[FCTIndex].PageSize;
> +}
> +
> +u32 QspiPsu_NOR_Get_JEDEC_ID(XQspiPsu *QspiPsuPtr)
> +{
> + if(QspiPsuPtr->Config.ConnectionMode ==
> XQSPIPSU_CONNECTION_MODE_STACKED) {
> + return Flash_Config_Table[FCTIndex].jedec_id * 2;
> + }
> + return Flash_Config_Table[FCTIndex].jedec_id;
> +}
>
Copy and paste error?
> diff --git a/spec/build/bsps/aarch64/xilinx-versal/grp.yml
> b/spec/build/bsps/aarch64/xilinx-versal/grp.yml
> index badfa07fcc..098a8481db 100644
> --- a/spec/build/bsps/aarch64/xilinx-versal/grp.yml
> +++ b/spec/build/bsps/aarch64/xilinx-versal/grp.yml
> @@ -24,6 +24,8 @@ links:
> uid: abi
> - role: build-dependency
> uid: obj
> +- role: build-dependency
> + uid: objxqspiflash
> - role: build-dependency
> uid: optconirq
> - role: build-dependency
> diff --git a/spec/build/bsps/aarch64/xilinx-versal/objxqspiflash.yml
> b/spec/build/bsps/aarch64/xilinx-versal/objxqspiflash.yml
> new file mode 100644
> index 0000000000..6d84bfa8cd
> --- /dev/null
> +++ b/spec/build/bsps/aarch64/xilinx-versal/objxqspiflash.yml
> @@ -0,0 +1,24 @@
> +SPDX-License-Identifier: CC-BY-SA-4.0 OR BSD-2-Clause
> +build-type: objects
> +cflags: []
> +copyrights:
> +- Copyright (C) 2022 On-Line Applications Research (OAR)
> +cppflags: []
> +cxxflags: []
> +enabled-by: true
> +includes:
> +- bsps/include/dev/spi/
> +- bsps/include/xil/
> +- bsps/include/xil/${XIL_SUPPORT_PATH}/
> +install:
> +- destination: ${BSP_INCLUDEDIR}/bsp
> + source:
> + - bsps/aarch64/xilinx-versal/include/dev/spi/versal_xqspi_flash.h
> +links:
> +- role: build-dependency
> + uid: ../../objxilinxsupport
> +- role: build-dependency
> + uid: ../../objqspipsu
> +source:
> + - bsps/aarch64/xilinx-versal/dev/spi/versal_xqspi_flash.c
> +type: build
> --
> 2.25.1
>
> _______________________________________________
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
