On Thu, Aug 13, 2020 at 12:36 AM Gedare Bloom <ged...@rtems.org> wrote:
> I make a few comments below for you to consider. At a high level, I > think the interface looks reasonable, although plainly mimics the > freebsd naming conventions. I wonder if we should prefer our naming > conventions, which is to aim to use _ to separate different words, and > to limit use of abbreviations except as they help shorten very long > names? > > I might suggest something along these lines: > rtems_ofw_get_prop_len() instead of rtems_ofw_getproplen() for example. > I'll change them to RTEMS naming conventions. > > It will make mapping to FreeBSD a little more annoying, but will make > the RTEMS interface more RTEMS-friendly. > > Other small comments below. > > On Wed, Aug 12, 2020 at 10:11 AM G S Niteesh Babu <niteesh...@gmail.com> > wrote: > > > > --- > > bsps/shared/dev/ofw/ofw.c | 0 > > bsps/shared/dev/ofw/ofw.h | 437 ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 437 insertions(+) > > create mode 100644 bsps/shared/dev/ofw/ofw.c > > create mode 100644 bsps/shared/dev/ofw/ofw.h > > > > diff --git a/bsps/shared/dev/ofw/ofw.c b/bsps/shared/dev/ofw/ofw.c > > new file mode 100644 > > index 0000000000..e69de29bb2 > > diff --git a/bsps/shared/dev/ofw/ofw.h b/bsps/shared/dev/ofw/ofw.h > > new file mode 100644 > > index 0000000000..5a8526c892 > > --- /dev/null > > +++ b/bsps/shared/dev/ofw/ofw.h > > @@ -0,0 +1,437 @@ > > +/* SPDX-License-Identifier: BSD-2-Clause */ > > + > > +/** > > + * @file > > + * > > + * @ingroup ofw > > + * > > + * RTEMS FDT implementation of OpenFirmWare Interface. > > + */ > > + > > +/* > > + * Copyright (C) <2020> Niteesh Babu G S <niteesh...@gmail.com> > > no <> around year > Fixed. > > > + * > > + * 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 _OFW_H > > +#define _OFW_H > > + > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +#include <stdint.h> > > +#include <stddef.h> > > + > > +typedef uint32_t ihandle_t; > > +typedef uint32_t pcell_t; > > +typedef uint32_t phandle_t; > document the types? > I'll document them. I should document their use right? Like what ihandle_t, phandle_t represent. > + > > +/** > > + * @brief Gets the node that is next to @a node. > > Somewhere the data structure should be explained. Maybe it will take a > new section in our doco to document the new interface. > Once we finalize the interface. I'll add it to the doco. But I don't understand what you mean by data structures. > > + * > > + * @param[in] node Node offset > > + * > > + * @retval Peer node offset Successful operation. > > + * @retval 0 No peer node or invalid node handle > > + */ > > +phandle_t rtems_ofw_peer( > > + phandle_t node > > +); > > + > > +/** > > + * @brief Gets the node that is the child of @a node. > > + * > > + * @param[in] node Node offset > > + * > > + * @retval child node offset Successful operation. > > + * @retval 0 No child node or invalid node handle > > + */ > > +phandle_t rtems_ofw_child( > > + phandle_t node > > +); > > + > > +/** > > + * @brief Gets the node that is the parent of @a node. > > + * > > + * @param[in] node Node offset > > + * > > + * @retval child node offset Successful operation. > > + * @retval 0 No child node or invalid node handle > > + */ > > +phandle_t rtems_ofw_parent( > > + phandle_t node > > +); > > + > > +/** > > + * @brief Gets the length of the property mentioned in @a propname. > > + * > > + * @param[in] node Node offset > > + * @param[in] prop Property name > > + * > > + * @retval -1 Invalid node or property > > + * @retval Length of property on successful operation > > + */ > > +size_t rtems_ofw_getproplen( > > + phandle_t node, > > + const char *propname > > +); > > + > > + > 1 blank > Fixed. > > > +/** > > + * @brief Gets the value of property mentioned in @a propname. > > + * > > + * @param[in] node Node offset > > + * @param[in] prop Property name > > + * @param[out] buf The property value gets stored in this buffer > (Pre-allocated) > > + * @param[in] len Length of the buffer > > + > > + * @retval -1 Invalid node or property > > + * @retval Length of property on successful operation > > + */ > > +size_t rtems_ofw_getprop( > > + phandle_t node, > > + const char *propname, > > + void *buf, > > + size_t len > > +); > > + > > +/** > > + * @brief Gets the value of property mentioned in @a propname. > prop > Fixed. > > > + * > > + * Gets the value of property of @a node and converts the value into > the host's > > + * endiannes. > > + * > > + * @param[in] node Node offset > > + * @param[in] prop Property name > > + * @param[out] buf The property value gets stored in this > buffer(Pre-allocated) > > + * after converted to the host's endiannes > typo: endianness > more misspelled below > Sorry for the typo. I fixed them all. > > > + * @param[in] len Length of the buffer > > + * > > + * @retval -1 Invalid node or property > > + * @retval Length of property on successful operation > > + */ > > +size_t rtems_ofw_getencprop( > > + phandle_t node, > > + const char *prop, > > + pcell_t *buf, > > + size_t len > > +); > > + > > +/** > > + * @brief Checks if the property @a propname is present in @a node. > > + * > > + * @param[in] node Node offset > > + * @param[in] propname Property name > > + * > > + * @retval 0 Property not present. > > + * @retval 1 Property is present. > > + */ > > +int rtems_ofw_hasprop( > > + phandle_t node, > > + const char *propname > > +); > > + > > +/** > > + * @brief Searches for property @a propname in @a node. > > + * > > + * Searches the node and its parent recursively for the property and > fills the > > + * buffer with the first found value. > > + * > > + * @param[in] node Node offset > > + * @param[in] propname Property name > > + * @param[out] buf The property value gets stored in this buffer > (Pre-allocated) > > + * @param[in] len Length of the buffer > > + * > > + * @retval Length of the property if property is found. > > + * @retval -1 Property is not found. > > + */ > > +size_t rtems_ofw_searchprop( > > + phandle_t node, > > + const char *propname, > > + void *buf, > > + size_t len > > +); > > + > > +/** > > + * @brief Searches for property @a propname in @a node. > > + * > > + * Searches the node and its parent recursively for the property and > fills the > > + * buffer with the first value found after converting it to the > endiannes of the > > + * host. > > + * > > + * @param[in] node Node offset > > + * @param[in] propname Property name > > + * @param[out] buf The property value gets stored in this > buffer(Pre-allocated) > > + * after converted to the host's endiannes > > + * @param[in] len Length of the buffer > > + * > > + * @retval Length of the property if property is found. > > + * @retval -1 Property is not found. > > + */ > > +size_t rtems_ofw_searchencprop( > > + phandle_t node, > > + const char *propname, > > + pcell_t *buf, > > + size_t len > > +); > > + > > +/** > > + * @brief Gets the value of property mentioned in @a propname. > > + * > > + * Same as rtems_ofw_getprop, but the @a buf is allocated in this > routine and > > + * the user is responsible for freeing it. > > + * > > + * @param[in] node Node offset > > + * @param[in] propname Property name > > + * @param[out] buf The buffer is allocated in this routine and user is > > + * responsible for freeing. > > + * > > + * @retval -1 Property is not found. > > + * @retval Length of the property if property is found. > > + */ > > +size_t rtems_ofw_getprop_alloc( > > + phandle_t node, > > + const char *propname, > > + void **buf > > +); > > + > > +/** > > + * @brief Gets multiple values of the property @a propname. > > + * > > + * Same as rtems_ofw_getprop_alloc but it can read properties with > multiple > > + * values. > > + * For eg: reg = <0x1000 0x10 0x2000 0x20> > > + * > > + * @param[in] node Node offset > > + * @param[in] propname Property name > > + * @param[in] elsz Size of the single value > > + * @param[out] buf The buffer is allocated in this routine and user is > > + * responsible for freeing. > > + * > > + * @retval -1 Property is not found. > > + * @retval Number of values read. > > + */ > > +size_t rtems_ofw_getprop_alloc_multi( > > + phandle_t node, > > + const char *propname, > > + int elsz, > > + void **buf > > +); > > + > > +/** > > + * @brief Gets the value of property mentioned in @a propname. > > + * > > + * Same as rtems_ofw_getprop_alloc but the value stored in the buffer is > > + * converted into the host's endiannes. > > + * > > + * @param[in] node Node offset > > + * @param[in] propname Property name > > + * @param[out] buf The buffer is allocated in this routine and user is > > + * responsible for freeing. > > + * > > + * @retval -1 Property is not found. > > + * @retval Length of the property if property is found. > > + */ > > +size_t rtems_ofw_getencprop_alloc( > > + phandle_t node, > > + const char *propname, > > + void **buf > > +); > > + > > +/** > > + * @brief Gets multiple values of the property @a propname. > > + * > > + * Same as rtems_ofw_getprop_alloc_multi but the values stored in the > buffer > > + * are converted to the host's endiannes. > > + * > > + * @param[in] node Node offset > > + * @param[in] propname Property name > > + * @param[in] elsz Size of the single value > > + * @param[out] buf The buffer is allocated in this routine and user is > > + * responsible for freeing. > must they call rtems_ofw_prop_free? > FreeBSD has an OF_free which call's free with proper flags. In RTEMS we decided to ignore the flags and I blindly copied the whole interface. So I don't think a separate function for free is needed but it will make the API uniform. > > + * > > + * @retval -1 Property is not found. > > + * @retval Number of values read. > > + */ > > +size_t rtems_ofw_getencprop_alloc_multi( > > + phandle_t node, > > + const char *propname, > > + int elsz, > > + void **buf > > +); > > + > > +/** > > + * @brief Free's the buffers allocated by the rtems_ofw_*_alloc > functions. > > + * > > + * @param[in] buf Buffer to be freed > > + */ > > +void rtems_ofw_prop_free( > > + void *buf > > +); > > + > > +/** > > + * @brief Finds the next property of @a node. > > + * > > + * Finds the next property of the node and when @a propname is NULL it > returns > > + * the value in the first property. > > + * > > + * @param[in] node Node offset > > + * @param[in] propname Property name > > + * @param[out] buf The value of the next property gets stored in this > buffer > > + * (Pre-allocated) > > + * @param[in] len Length of the buffer > > + * > > + * @retval -1 node or previous property does not exist > > + * @retval 0 no more properties > > + * @retval 1 success > > + */ > > +int rtems_ofw_nextprop( > > + phandle_t node, > > + const char *propname, > > + char *buf, > > + size_t len > > +); > > + > > +/** > > + * @brief Sets the property @name of @a node to @a buf. > > + * > > + * @param[in] node Node offset > > + * @param[in] name Property name > > + * @param[in] buf Buffer containes the value to be set. > typo; contains > Fixed. > > + * @param[in] len Length of the buffer > > + * > > + * @retval -1 node does not exist > > + * @retval 0 on success > > + * @retval libFDT error codes on unsuccessful setting operation > > + */ > > +int rtems_ofw_setprop( > > + phandle_t node, > > + const char *name, > > + const void *buf, > > + size_t len > > +); > > + > > +/** > > + * @brief Converts a device specifier to a fully qualified path name. > > + * > > + * @param[in] dev device specifier > > + * @param[out] buf The path is filled into the buffer(Pre-allocated) > > + * @param[in] length of the buffer > > + * > > + * @retval -1 always. Unimplemented. > > + */ > > +size_t rtems_ofw_canon( > > + const char *dev, > > + char *buf, > > + size_t len > > +); > > + > > +/** > > + * @brief Finds the node at the given path > > + * > > + * @param[in] path to the node from root > > + * > > + * @retval -1 node does not exist > > + * @retval node handle on success > > + */ > > +phandle_t rtems_ofw_finddevice( > > + const char *path > > +); > > + > > +/** > > + * @brief This routine converts effective phandle of @a node to node > offset. > @a xref? > Fixed. > > + * > > + * @param[in] xref Node phandle > > + * > > + * @retval Node offset on success > > + * @retval Node phandle on failure > > + */ > > +phandle_t rtems_ofw_node_from_xref( > > + phandle_t xref > > +); > > + > > +/** > > + * @brief This routine converts node offset to effective phandle of @a > node. > > + * > > + * @retval Node phandle on success > > + * @retval Node offset on failure > > + */ > > +phandle_t rtems_ofw_xref_from_node( > > + phandle_t node > > +); > > + > > +/* > > + * instance handles(ihandle_t) as same as phandles in the FDT > implementation > > + * of OF interface. > > + */ > stray comments? > I'll add these comments to the type's documentation above. I also wanted to add a few other functions that are nor part of the OF interface but would be really beneficial for RTEMS. Can I add the following functions? 1) rtems_ofw_get_reg 2) rtems_ofw_status_okay 3) rtems_ofw_get_interrupts Since these functions are not part of OFW should I use FDT instead of OFW in the function names? eg: rtems_fdt_get_reg > + > > +/** > > + * @brief Converts @a instance handle to phandle. > > + * > > + * instance are same as node offsets in FDT. > > + * > > + * @param[in] instance Node offset > > + * > > + * @retval phandle of node on success. > > + * @retval instance of node on failure. > > + */ > > +phandle_t rtems_ofw_instance_to_package( > > + ihandle_t instance > > +); > > + > > +/** > > + * @brief Find the node's path from phandle. > > + * > > + * @param[in] node Node offset > > + * @param[out] buf Path is filled into this buffer(Pre-allocated). > > + * @param[in] len Length of the buffer. > > + * > > + * @retval -1 always. Unimplemented. > > + */ > > +size_t rtems_ofw_package_to_path( > > + phandle_t node, > > + char *buf, > > + size_t len > > +); > > + > > +/** > > + * @brief Find the node's path from ihandle. > > + * > > + * @param[in] instance Node offset > > + * @param[out] buf Path is filled into this buffer(Pre-allocated). > > + * @param[in] len Length of the buffer. > > + * > > + * @retval -1 always. Unimplemented. > > + */ > > +size_t rtems_ofw_instance_to_path( > > + ihandle_t instance, > > + char *buf, > > + size_t len > > +); > > + > > +#ifdef __cplusplus > > +} > > +#endif > > + > > +#endif /* _OFW_H */ > > \ No newline at end of file > > -- > > 2.17.1 > > >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel