On Thu, Aug 13, 2020 at 4:31 AM Niteesh G. S. <niteesh...@gmail.com> wrote: > > 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. >> >
I thought of one more little complaint. These types are non-standard, and the _t is reserved for standards. Should we introduce our own type names here, and then also map them into the freeBSD type names? >> > > + >> > > +/** >> > > + * @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? > I'd put it in their doxygen. >> >> > 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
