Hello, On Thu, Aug 13, 2020 at 12:09 PM Christian Mauderer < christian.maude...@embedded-brains.de> wrote:
> Hello Niteesh, > > On 13/08/2020 07:05, Niteesh G. S. wrote: > > On Thu, Aug 13, 2020 at 12:36 AM Gedare Bloom <ged...@rtems.org > > <mailto: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 <mailto: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. > > Maybe a short notice that the intention is to be compatible with the > FreeBSD OpenFirmWare interface would be nice. So if someone changes > something he knows that he has to take a look at FreeBSD too. > OK. I'll add one. > > > + */ > > > + > > > +/* > > > + * Copyright (C) <2020> Niteesh Babu G S <niteesh...@gmail.com > > <mailto: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. > > > > That will also make the API future proof. If someone somewhen decides to > change the implementation, the user already uses "rtems_ofw_prop_free" > instead of a plain "free" everywhere. > It should actually be rtems_ofw_free but I missed to remove the _prop_ while copy-pasting. > > > > > + * > > > + * @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. > > > Maybe make clear in the description that these don't have a mapping that > matches FreeBSD's OFW interface. > Are stray comments fine here? Or should I add it to the doco? > > 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 > > Be carefull with "rtems_fdt". There is already a wrapper for libfdt with > these prefixes in cpukit/libmisc/rtems-fdt/rtems-fdt.c. And if you use > the types from this file, you should keep the prefix as "rtems_ofw". > OK. > Best regards > > Christian > > > > > > + > > > +/** > > > + * @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 > > > > > > > -- > -------------------------------------------- > embedded brains GmbH > Herr Christian Mauderer > Dornierstr. 4 > D-82178 Puchheim > Germany > email: christian.maude...@embedded-brains.de > Phone: +49-89-18 94 741 - 18 > Fax: +49-89-18 94 741 - 08 > PGP: Public key available on request. > > Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG. >
_______________________________________________ devel mailing list devel@rtems.org http://lists.rtems.org/mailman/listinfo/devel