Hi Gedare,
Thanks for the feedback you’ve provided. I have a couple questions.
You suggest refactoring the callouts into another sturct, I’m happy to do so. I
originally chose not to as I was was basing the high level structure off the
i2c API already in RTEMS. Just clarifying it’s worth doing.
> 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.
As for the RTEMS_FLASHDEV_MAX_REGIONS being set at 32 I chose the value as it
seamed a good balance between memory usage and function. I wanted to avoid
using malloc as your previous feedback suggested. If configurable size is
necessary what’s the best way to approach it without using malloc?
Thanks,
Aaron
> On 15 Apr 2023, at 01:58, Gedare Bloom <ged...@rtems.org> wrote:
>
> I focused my review on the API. See below.
>
> On Thu, Apr 6, 2023 at 1:08 AM <aaron.nyh...@unfoldedeffective.com> wrote:
>>
>> From: Aaron Nyholm <aaron.nyh...@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@rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
