On Sun, Apr 16, 2023 at 6:48 PM Aaron Nyholm
<aaron.nyh...@unfoldedeffective.com> wrote:
>
> 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.
>
I'm not sure there's much advantage one way or the other. You can
leave it as is for now.
> > 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?
>
A configure-time option may be viable, e.g., spec/build/cpukit/opt*
You should probably elevate that question to its own email thread to
get other opinions.
> 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
