[PATCH v2 1/3] cpukit/flash: Add API for Flash devices

[Gedare Bloom]

I focused my review on the API. See below.

On Thu, Apr 6, 2023 at 1:08 AM <aaron.nyholm at unfoldedeffective.com> wrote:
>
> From: Aaron Nyholm <aaron.nyholm at southerninnovation.com>
>
[...]
> diff --git a/cpukit/include/dev/flash/flashdev.h b/cpukit/include/dev/flash/flashdev.h
> new file mode 100644
> index 0000000000..5d53ea7b97
> --- /dev/null
> +++ b/cpukit/include/dev/flash/flashdev.h
> @@ -0,0 +1,437 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +
> +/**
> + * @file
> + *
> + * @ingroup Flash
> + *
> + * @brief Generic Flash API
> + */
> +
> +/*
> + * Copyright (C) 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 _DEV_FLASHDEV_H
> +#define _DEV_FLASHDEV_H
> +
> +#include <rtems/thread.h>
> +#include <sys/types.h>
> +
> +typedef struct rtems_flashdev rtems_flashdev;
> +
> +/**
> + * @defgroup Generic Flash API
> + *
> + * @ingroup RTEMSDeviceDrivers
> + *
> + * @brief Generic Flash API to wrap specific device drivers.
> + *
> + * @{
> + */
> +
> +/* IOCTL Calls */
> +
> +/**
> + * @brief Obtains the flash device.
> + *
> + * This command has no argument.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_OBTAIN 0
> +/**
> + * @brief Releases the flash device.
> + *
> + * This command has no argument.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_RELEASE 1
> +/**
> + * @brief Returns the JEDEC ID of the flash device.
> + *
> + * @param[out] jedec_id Pointer to uint32_t in which the JEDEC ID is
> + * returned in.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_JEDEC_ID 2
> +/**
> + * @brief Erases flash device.
> + *
> + * @param[in] erase_args Pointer to rtems_flashdev_ioctl_erase_args struct
> + * containing offset and size of erase to be performed.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_ERASE 3
> +/**
> + * @brief Set a region that limits read, write and erase calls to within it.
> + * Regions are file descriptor specific and limited to a single region per
> + * file descriptor and 32 regions total per flash device. Regions can be
> + * changed or updated by calling this IOCTL again.
> + *
> + * @param[in] region Pointer to rtems_flashdev_ioctl_region struct containing
> + * base and length of defined region.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_REGION_SET 4
> +/**
> + * @brief Removes the set region on the file descriptor.
> + *
> + * This command has no argument.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_REGION_UNSET 5
> +/**
> + * @brief Returns the type of flash device (e.g. NOR or NAND).
> + *
> + * @param[out] flash_type Pointer to integer which is set to the flash
> + * type macro value.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_TYPE 6
> +
> +/**
> + * @brief Get the size and address of flash page at given offset
> + *
> + * The offset ignores the region limiting. To find page of region
> + * limited offset add the base of the region to the desired offset.
> + *
> + * @param[in,out] rtems_flashdev_ioctl_page_info arg Pointer to struct
> + * with offset and space for return values.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_PAGEINFO_BY_OFFSET 7
> +
> +/**
> + * @brief Get the size and address of nth flash page where n is index passed in.
> + *
> + * The index ignores the region limiting.
> + *
> + * @param[in,out] rtems_flashdev_ioctl_page_info arg Pointer to struct
> + * with index and space for return values.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_PAGEINFO_BY_INDEX 8
> +
> +/**
> + * @brief Get the number of pages in flash device.
> + *
> + * @param[out] count Integer containing the number of pages.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_PAGE_COUNT 9
> +
> +/**
> + * @brief Get the minimum write size supported by the driver.
> + *
> + * @param[out] count Integer containing the minimum write size.
> + */
> +#define RTEMS_FLASHDEV_IOCTL_WRITE_BLOCK_SIZE 10
> +
> +/* Flash Types */
> +/**
> + * @brief Returned from IOCTL call if flash is of type NOR.
> + */
> +#define RTEMS_FLASHDEV_TYPE_NOR 0
> +
> +/**
> + * @brief Returned from IOCTL call if flash is of type NAND.
> + */
> +#define RTEMS_FLASHDEV_TYPE_NAND 1
> +
An enum might be better.

> +/**
> + * @brief The maximum number of region limited file descriptors
> + * allowed to be open at once.
> + */
> +#define RTEMS_FLASHDEV_MAX_REGIONS 32
> +
Why is 32 a good default constant limit? should this be configurable?

> +/**
> + * @brief General definition for on flash device.
> + */
> +typedef struct rtems_flashdev_region {
> +  /**
> +   * @brief Base of region.
> +   */
> +  off_t offset;
The off_t type is normally used for file offsets. Are these regions
file-mapped? If not, a different type should probably be chosen,
perhaps just a primitive.

> +  /**
> +   * @brief Length of region.
> +   */
> +  size_t size;
> +} rtems_flashdev_ioctl_region,
> +  rtems_flashdev_ioctl_erase_args,
> +  rtems_flashdev_returned_page_info;
Do we really need to use aliases for the same structure? Why not just
keep everything consistent as a "rtems_flashdev_region"?

> +
> +/**
> + * @brief Page information returned from IOCTL calls.
> + */
> +typedef struct rtems_flashdev_ioctl_page_info {
> +  /**
> +   * @brief Offset or index to find page at.
> +   */
> +  off_t offset_or_index;
ditto. The name is a bit wordy "offset_or_index" -- these are kind of
synonyms anyway.

> +
> +  /**
> +   * @brief Information returned about the page. Including the
> +   * base offset and size of page.
> +   */
> +  rtems_flashdev_returned_page_info return_page_info;
> +} rtems_flashdev_ioctl_page_info;
> +
> +/**
> + * @brief Flash device.
> + */
> +struct rtems_flashdev {
> +  /**
> +   * @brief Call to the device driver to read the flash device.
> +   *
> +   * @param[in] flash Pointer to flash device.
> +   * @param[in] offset Address to read from.
> +   * @param[in] count Number of bytes to read.
> +   * @param[out] buffer Buffer for data to be read into.
> +   *
> +   * @retval 0 Successful operation.
> +   * @retval 1 Failed operation.
> +   */
> +  int ( *read )(
> +    rtems_flashdev *flash,
> +    uint32_t offset,
This should be uintptr_t if it's an address. That should accommodate
also 4G+ flash devices in 64-bit architectures. Maybe it should be
called uintptr_t addr; After all, an address is typically just an
offset from 0. This API is overusing the word offset to mean several
different things that is confusing.

> +    uint32_t count,
> +    void *buffer
> +  );
> +
> +  /**
> +   * @brief Call to the device driver to write to the flash device.
> +   *
> +   * @param[in] flash Pointer to flash device.
> +   * @param[in] offset Address to write to.
> +   * @param[in] count Number of bytes to read.
> +   * @param[in] buffer Buffer for data to be written from.
> +   *
> +   * @retval 0 Successful operation.
> +   * @retval 1 Failed operation.
> +   */
> +  int ( *write )(
> +    rtems_flashdev *flash,
> +    uint32_t offset,
> +    uint32_t count,
> +    const void *buffer
> +  );
> +
> +  /**
> +   * @brief Call to the device driver to erase the flash device.
> +   *
> +   * @param[in] flash Pointer to flash device.
> +   * @param[in] offset Address to erase at.
> +   * @param[in] count Number of bytes to erase.
> +   *
> +   * @retval 0 Successful operation.
> +   * @retval 1 Failed operation.
> +   */
> +  int ( *erase )(
> +    rtems_flashdev *flash,
> +    uint32_t offset,
> +    uint32_t count
> +  );
> +
> +  /**
> +   * @brief Call to the device driver to return the JEDEC ID.
> +   *
> +   * @param[in] flash The flash device.
> +   *
> +   * @retval JEDEC ID.
> +   */
> +  uint32_t ( *jedec_id )(
> +    rtems_flashdev *flash
> +  );
> +
> +  /**
> +   * @brief Call to the device driver to return the flash type.
> +   *
> +   * @param[in] flash The flash device.
> +   *
> +   * @retval The RTEMS_FLASHDEV_TYPE_* for the given flash type.
> +   */
> +  int ( *flash_type )(
> +    rtems_flashdev *flash
> +  );
> +
> +  /**
> +   * @brief Call to device driver to get size and offset of page at
> +   * given offset
> +   *
> +   * @param[in] flash The flash device
> +   * @param[in] search_offset The offset of the page which info is to be
> +   * returned.
> +   * @param[out] page_offset The offset of the start of the page
> +   * @param[out] page_size The size of the page
> +   *
> +   * @retval 0 Success.
> +   * @retval non-zero Failed.
> +   */
> +  int ( *page_info_by_off )(
Spell out offset, if you'll keep the word offset. "Off" is a real
word, so shortening "Offset" to "Off" is confusing. Rule #1 of API
design is don't be confusing.

> +    rtems_flashdev *flash,
> +    off_t search_offset,
> +    off_t *page_offset,
> +    size_t *page_size
> +  );
> +
> +  /**
> +   * @brief Call to device driver to get size and offset of page at
> +   * given offset
Why this @brief is same as the previous one? There should be something
different. (Maybe you meant "at given index"?)

> +   *
> +   * @param[in] flash The flash device
> +   * @param[in] search_index The index of the page which info is to be returned.
> +   * @param[out] page_offset The offset of the start of the page
> +   * @param[out] page_size The size of the page
> +   *
> +   * @retval 0 Sucess.
> +   * @retval non-zero Failed.
> +   */
> +  int ( *page_info_by_index )(
> +    rtems_flashdev *flashdev,
> +    off_t search_index,
> +    off_t *page_offset,
> +    size_t *page_size
> +  );
> +
> +  /**
> +   * @brief Call to device driver to return the number of pages on the flash
> +   * device.
> +   *
> +   * @param[out] page_count The number of pages on the flash device.
> +   *
> +   * @retval 0 Success.
> +   * @retval non-zero Failed.
> +   */
> +  int ( *page_count )(
> +    rtems_flashdev *flashdev,
> +    int *page_count
> +  );
> +
> +  /**
> +   * @brief Call to device driver to return the minimum write size of the
> +   * flash device.
> +   *
> +   * @param[out] write_block_size The mimimum write size of the flash device.
typo "mimimum"

> +   *
> +   * @retval 0 Success.
> +   * @retval non-zero Failed.
> +   */
> +  int ( *write_block_size )(
> +    rtems_flashdev *flashdev,
> +    size_t *write_block_size
> +  );
> +
> +  /**
> +   * @brief Destroys the flash device.
> +   *
> +   * @param[in] flash Pointer to flash device.
> +   */
> +  void ( *destroy )(
> +    rtems_flashdev *flashdev
> +  );
> +

You might consider refactoring the callouts to another struct that is
included here (either by value or by reference), just to separate the
function pointers from the data storage. Not strictly necessary, but
it would simplify this struct greatly, and should make implementation
of different drivers easier by consolidating the definitions of these
callouts.

> +  /**
> +   * @brief Pointer to device driver.
> +   */
> +  void *driver;
Is there a type/struct for this?

> +
> +  /**
> +   * @brief Mutex to protect the flash device access.
> +   */
> +  rtems_recursive_mutex mutex;
> +
> +  /**
> +   * @brief Bit allocator for regions.
> +   */
> +  uint32_t region_bit_allocator;
> +
> +  /**
> +   * @brief Regions stored.
> +   */
> +  rtems_flashdev_ioctl_region regions[ RTEMS_FLASHDEV_MAX_REGIONS ];

As noted above, this could be configurable indeed it could use a
zero-size array here and the size can be determined at allocation
time. If that is helpful.

> +};
> +
> +/**
> + * @brief Allocate and initialize the flash device.
> + *
> + * After a successful allocation and initialization of the flash device
> + * it must be destoryed via rtems_flashdev_destroy_and_free().
typo destoryed

> + *
> + * @param[in] size The number of bytes to allocate.
> + *
> + * @retval NULL Failed to set up flash device.
> + * @retval non-NULL The new flash device.
> + */
> +rtems_flashdev *rtems_flashdev_alloc_and_init(
> +  size_t size
> +);
> +
> +/**
> + * @brief Initialize the flash device.
> + *
> + * After a successful initialization of the flash device it must be
> + * destroyed via rtems_flashdev_destory().
> + *
> + * After initalization and before registration read, write, erase, jedec_id
typo: initalization

> + * and flash_type functions need to be set in the flashdev.
> + *
> + * @param[in] flash The flash device to initialize.
> + *
> + * @retval 1 Failed to set up flash device.
> + * @retval 0 Successful set up of flash device.
> + */
> +int rtems_flashdev_init(
> +  rtems_flashdev *flash
> +);
> +
> +/**
> + * @brief Register the flash device.
> + *
> + * This function always claims ownership of the flash device.
> + *
> + * After initalization and before registration read, write, erase, jedec_id
ditto.

> + * and flash_type functions need to be set in the flashdev.
> + *
> + * @param[in] flash The flash device.
> + * @param[in] flash_path The path to the flash device file.
> + *
> + * @retval 0 Successful operation.
> + * @retval non-zero Failed operation.
> + */
> +int rtems_flashdev_register(
> +  rtems_flashdev *flash,
> +  const char *flash_path
> +);
> +
> +/**
> + * @brief Destorys the flash device.
typo

> + *
> + * @param[in] flash The flash device.
> + */
> +void rtems_flashdev_destroy(
> +  rtems_flashdev *flash
> +);
> +
> +/**
> + * @brief Destorys the flash device and frees it's memory.
typo Destorys
typo: it's -> its

> + *
> + * @param[in] flash The flash device.
> + */
> +void rtems_flashdev_destroy_and_free(
> +  rtems_flashdev *flash
> +);
> +
> +/** @} */
> +
> +#endif /* _DEV_FLASHDEV_H */
> diff --git a/spec/build/cpukit/librtemscpu.yml b/spec/build/cpukit/librtemscpu.yml
> index 2c969aa5f6..86c5c8ff2b 100644
> --- a/spec/build/cpukit/librtemscpu.yml
> +++ b/spec/build/cpukit/librtemscpu.yml
> @@ -52,6 +52,9 @@ install:
>  - destination: ${BSP_INCLUDEDIR}/dev/spi
>    source:
>    - cpukit/include/dev/spi/spi.h
> +- destination: ${BSP_INCLUDEDIR}/dev/flash
> +  source:
> +  - cpukit/include/dev/flash/flashdev.h
>  - destination: ${BSP_INCLUDEDIR}/dev/can
>    source:
>    - cpukit/include/dev/can/can.h
> @@ -522,6 +525,7 @@ links:
>  - role: build-dependency
>    uid: vckey
>  source:
> +- cpukit/dev/flash/flashdev.c
>  - cpukit/dev/i2c/eeprom.c
>  - cpukit/dev/i2c/fpga-i2c-slave.c
>  - cpukit/dev/i2c/gpio-nxp-pca9535.c
> --
> 2.25.1
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel