<div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-size:small">On Thu, Aug 13, 2020 at 12:36 AM Gedare Bloom <<a href="mailto:gedare@rtems.org">gedare@rtems.org</a>> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">I make a few comments below for you to consider. At a high level, I<br>
think the interface looks reasonable, although plainly mimics the<br>
freebsd naming conventions. I wonder if we should prefer our naming<br>
conventions, which is to aim to use _ to separate different words, and<br>
to limit use of abbreviations except as they help shorten very long<br>
names?<br>
<br>
I might suggest something along these lines:<br>
rtems_ofw_get_prop_len() instead of rtems_ofw_getproplen() for example.<br></blockquote><div><br></div><div class="gmail_default" style="font-size:small">I'll change them to RTEMS naming conventions.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
It will make mapping to FreeBSD a little more annoying, but will make<br>
the RTEMS interface more RTEMS-friendly.<br>
<br>
Other small comments below.<br>
<br>
On Wed, Aug 12, 2020 at 10:11 AM G S Niteesh Babu <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>> wrote:<br>
><br>
> ---<br>
>  bsps/shared/dev/ofw/ofw.c |   0<br>
>  bsps/shared/dev/ofw/ofw.h | 437 ++++++++++++++++++++++++++++++++++++++<br>
>  2 files changed, 437 insertions(+)<br>
>  create mode 100644 bsps/shared/dev/ofw/ofw.c<br>
>  create mode 100644 bsps/shared/dev/ofw/ofw.h<br>
><br>
> diff --git a/bsps/shared/dev/ofw/ofw.c b/bsps/shared/dev/ofw/ofw.c<br>
> new file mode 100644<br>
> index 0000000000..e69de29bb2<br>
> diff --git a/bsps/shared/dev/ofw/ofw.h b/bsps/shared/dev/ofw/ofw.h<br>
> new file mode 100644<br>
> index 0000000000..5a8526c892<br>
> --- /dev/null<br>
> +++ b/bsps/shared/dev/ofw/ofw.h<br>
> @@ -0,0 +1,437 @@<br>
> +/* SPDX-License-Identifier: BSD-2-Clause */<br>
> +<br>
> +/**<br>
> + * @file<br>
> + *<br>
> + * @ingroup ofw<br>
> + *<br>
> + * RTEMS FDT implementation of OpenFirmWare Interface.<br>
> + */<br>
> +<br>
> +/*<br>
> + * Copyright (C) <2020> Niteesh Babu G S <<a href="mailto:niteesh.gs@gmail.com" target="_blank">niteesh.gs@gmail.com</a>><br>
<br>
no <> around year<br></blockquote><div><span class="gmail_default" style="font-size:small">Fixed.</span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + *<br>
> + * Redistribution and use in source and binary forms, with or without<br>
> + * modification, are permitted provided that the following conditions<br>
> + * are met:<br>
> + * 1. Redistributions of source code must retain the above copyright<br>
> + *    notice, this list of conditions and the following disclaimer.<br>
> + * 2. Redistributions in binary form must reproduce the above copyright<br>
> + *    notice, this list of conditions and the following disclaimer in the<br>
> + *    documentation and/or other materials provided with the distribution.<br>
> + *<br>
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"<br>
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE<br>
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE<br>
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE<br>
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR<br>
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF<br>
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS<br>
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN<br>
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)<br>
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE<br>
> + * POSSIBILITY OF SUCH DAMAGE.<br>
> + */<br>
> +<br>
> +#ifndef _OFW_H<br>
> +#define _OFW_H<br>
> +<br>
> +#ifdef __cplusplus<br>
> +extern "C" {<br>
> +#endif<br>
> +<br>
> +#include <stdint.h><br>
> +#include <stddef.h><br>
> +<br>
> +typedef uint32_t  ihandle_t;<br>
> +typedef uint32_t  pcell_t;<br>
> +typedef uint32_t  phandle_t;<br>
document the types?<br></blockquote><div><span class="gmail_default" style="font-size:small"></span></div><div><span class="gmail_default" style="font-size:small">I'll document them. I should document their use right? Like what</span></div><div><span class="gmail_default" style="font-size:small">ihandle_t, phandle_t represent.</span></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +/**<br>
> + * @brief Gets the node that is next to @a node.<br>
<br>
Somewhere the data structure should be explained. Maybe it will take a<br>
new section in our doco to document the new interface.<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">Once we finalize the interface. I'll add it to the doco.</div><div class="gmail_default" style="font-size:small">But I don't understand what you mean by data structures.</div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + *<br>
> + * @param[in] node Node offset<br>
> + *<br>
> + * @retval Peer node offset Successful operation.<br>
> + * @retval 0 No peer node or invalid node handle<br>
> + */<br>
> +phandle_t rtems_ofw_peer(<br>
> +  phandle_t   node<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Gets the node that is the child of @a node.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + *<br>
> + * @retval child node offset Successful operation.<br>
> + * @retval 0 No child node or invalid node handle<br>
> + */<br>
> +phandle_t rtems_ofw_child(<br>
> +  phandle_t   node<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Gets the node that is the parent of @a node.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + *<br>
> + * @retval child node offset Successful operation.<br>
> + * @retval 0 No child node or invalid node handle<br>
> + */<br>
> +phandle_t rtems_ofw_parent(<br>
> +  phandle_t   node<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Gets the length of the property mentioned in @a propname.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] prop Property name<br>
> + *<br>
> + * @retval -1 Invalid node or property<br>
> + * @retval  Length of property on successful operation<br>
> + */<br>
> +size_t rtems_ofw_getproplen(<br>
> +  phandle_t   node,<br>
> +  const char *propname<br>
> +);<br>
> +<br>
> +<br>
1 blank<br></blockquote><div><span class="gmail_default" style="font-size:small">Fixed.</span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> +/**<br>
> + * @brief Gets the value of property mentioned in @a propname.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] prop Property name<br>
> + * @param[out] buf The property value gets stored in this buffer (Pre-allocated)<br>
> + * @param[in] len Length of the buffer<br>
> +<br>
> + * @retval -1 Invalid node or property<br>
> + * @retval  Length of property on successful operation<br>
> + */<br>
> +size_t rtems_ofw_getprop(<br>
> +  phandle_t    node,<br>
> +  const char  *propname,<br>
> +  void        *buf,<br>
> +  size_t       len<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Gets the value of property mentioned in @a propname.<br>
prop<br></blockquote><div><span class="gmail_default" style="font-size:small">Fixed.</span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + *<br>
> + * Gets the value of property of @a node and converts the value into the host's<br>
> + * endiannes.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] prop Property name<br>
> + * @param[out] buf The property value gets stored in this buffer(Pre-allocated)<br>
> + * after converted to the host's endiannes<br>
typo: endianness<br>
more misspelled below<br></blockquote><div><span class="gmail_default" style="font-size:small">Sorry for the typo. I fixed them all.</span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> + * @param[in] len Length of the buffer<br>
> + *<br>
> + * @retval -1 Invalid node or property<br>
> + * @retval  Length of property on successful operation<br>
> + */<br>
> +size_t rtems_ofw_getencprop(<br>
> +  phandle_t    node,<br>
> +  const char  *prop,<br>
> +  pcell_t     *buf,<br>
> +  size_t       len<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Checks if the property @a propname is present in @a node.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] propname Property name<br>
> + *<br>
> + *  @retval  0 Property not present.<br>
> + *  @retval  1 Property is present.<br>
> + */<br>
> +int rtems_ofw_hasprop(<br>
> +  phandle_t    node,<br>
> +  const char  *propname<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Searches for property @a propname in @a node.<br>
> + *<br>
> + * Searches the node and its parent recursively for the property and fills the<br>
> + * buffer with the first found value.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] propname Property name<br>
> + * @param[out] buf The property value gets stored in this buffer (Pre-allocated)<br>
> + * @param[in] len Length of the buffer<br>
> + *<br>
> + * @retval  Length of the property if property is found.<br>
> + * @retval  -1 Property is not found.<br>
> + */<br>
> +size_t rtems_ofw_searchprop(<br>
> +  phandle_t    node,<br>
> +  const char  *propname,<br>
> +  void        *buf,<br>
> +  size_t       len<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Searches for property @a propname in @a node.<br>
> + *<br>
> + * Searches the node and its parent recursively for the property and fills the<br>
> + * buffer with the first value found after converting it to the endiannes of the<br>
> + * host.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] propname Property name<br>
> + * @param[out] buf The property value gets stored in this buffer(Pre-allocated)<br>
> + * after converted to the host's endiannes<br>
> + * @param[in] len Length of the buffer<br>
> + *<br>
> + * @retval  Length of the property if property is found.<br>
> + * @retval  -1 Property is not found.<br>
> + */<br>
> +size_t rtems_ofw_searchencprop(<br>
> +  phandle_t    node,<br>
> +  const char  *propname,<br>
> +  pcell_t     *buf,<br>
> +  size_t       len<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Gets the value of property mentioned in @a propname.<br>
> + *<br>
> + * Same as rtems_ofw_getprop, but the @a buf is allocated in this routine and<br>
> + * the user is responsible for freeing it.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] propname Property name<br>
> + * @param[out] buf The buffer is allocated in this routine and user is<br>
> + * responsible for freeing.<br>
> + *<br>
> + * @retval  -1 Property is not found.<br>
> + * @retval  Length of the property if property is found.<br>
> + */<br>
> +size_t rtems_ofw_getprop_alloc(<br>
> +  phandle_t    node,<br>
> +  const char  *propname,<br>
> +  void       **buf<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Gets multiple values of the property @a propname.<br>
> + *<br>
> + * Same as rtems_ofw_getprop_alloc but it can read properties with multiple<br>
> + * values.<br>
> + * For eg: reg = <0x1000 0x10 0x2000 0x20><br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] propname Property name<br>
> + * @param[in] elsz Size of the single value<br>
> + * @param[out] buf The buffer is allocated in this routine and user is<br>
> + * responsible for freeing.<br>
> + *<br>
> + * @retval  -1 Property is not found.<br>
> + * @retval   Number of values read.<br>
> + */<br>
> +size_t rtems_ofw_getprop_alloc_multi(<br>
> +  phandle_t    node,<br>
> +  const char  *propname,<br>
> +  int          elsz,<br>
> +  void       **buf<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Gets the value of property mentioned in @a propname.<br>
> + *<br>
> + * Same as rtems_ofw_getprop_alloc but the value stored in the buffer is<br>
> + * converted into the host's endiannes.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] propname Property name<br>
> + * @param[out] buf The buffer is allocated in this routine and user is<br>
> + * responsible for freeing.<br>
> + *<br>
> + * @retval  -1 Property is not found.<br>
> + * @retval  Length of the property if property is found.<br>
> + */<br>
> +size_t rtems_ofw_getencprop_alloc(<br>
> +  phandle_t    node,<br>
> +  const char  *propname,<br>
> +  void       **buf<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Gets multiple values of the property @a propname.<br>
> + *<br>
> + * Same as rtems_ofw_getprop_alloc_multi but the values stored in the buffer<br>
> + * are converted to the host's endiannes.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] propname Property name<br>
> + * @param[in] elsz Size of the single value<br>
> + * @param[out] buf The buffer is allocated in this routine and user is<br>
> + * responsible for freeing.<br>
must they call rtems_ofw_prop_free?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">FreeBSD has an OF_free which call's free with proper flags. </div><div class="gmail_default" style="font-size:small">In RTEMS we decided to ignore the flags and I blindly copied the</div><div class="gmail_default" style="font-size:small">whole interface. So I don't think a separate function for free is</div><div class="gmail_default" style="font-size:small">needed but it will make the API uniform.</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + *<br>
> + * @retval  -1 Property is not found.<br>
> + * @retval   Number of values read.<br>
> + */<br>
> +size_t rtems_ofw_getencprop_alloc_multi(<br>
> +  phandle_t     node,<br>
> +  const char   *propname,<br>
> +  int           elsz,<br>
> +  void        **buf<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Free's the buffers allocated by the rtems_ofw_*_alloc functions.<br>
> + *<br>
> + * @param[in] buf Buffer to be freed<br>
> + */<br>
> +void rtems_ofw_prop_free(<br>
> +  void   *buf<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Finds the next property of @a node.<br>
> + *<br>
> + * Finds the next property of the node and when @a propname is NULL it returns<br>
> + * the value in the first property.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] propname Property name<br>
> + * @param[out] buf The value of the next property gets stored in this buffer<br>
> + * (Pre-allocated)<br>
> + * @param[in] len Length of the buffer<br>
> + *<br>
> + * @retval  -1 node or previous property does not exist<br>
> + * @retval   0 no more properties<br>
> + * @retval   1 success<br>
> + */<br>
> +int rtems_ofw_nextprop(<br>
> +  phandle_t    node,<br>
> +  const char  *propname,<br>
> +  char        *buf,<br>
> +  size_t       len<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Sets the property @name of @a node to @a buf.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[in] name Property name<br>
> + * @param[in] buf Buffer containes the value to be set.<br>
typo; contains<br></blockquote><div><span class="gmail_default" style="font-size:small">Fixed.</span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + * @param[in] len Length of the buffer<br>
> + *<br>
> + * @retval  -1 node does not exist<br>
> + * @retval   0 on success<br>
> + * @retval   libFDT error codes on unsuccessful setting operation<br>
> + */<br>
> +int rtems_ofw_setprop(<br>
> +  phandle_t    node,<br>
> +  const char  *name,<br>
> +  const void  *buf,<br>
> +  size_t       len<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Converts a device specifier to a fully qualified path name.<br>
> + *<br>
> + * @param[in] dev device specifier<br>
> + * @param[out] buf The path is filled into the buffer(Pre-allocated)<br>
> + * @param[in] length of the buffer<br>
> + *<br>
> + * @retval  -1 always. Unimplemented.<br>
> + */<br>
> +size_t rtems_ofw_canon(<br>
> +  const char  *dev,<br>
> +  char        *buf,<br>
> +  size_t       len<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Finds the node at the given path<br>
> + *<br>
> + * @param[in] path to the node from root<br>
> + *<br>
> + * @retval  -1 node does not exist<br>
> + * @retval   node handle on success<br>
> + */<br>
> +phandle_t rtems_ofw_finddevice(<br>
> +  const char  *path<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief This routine converts effective phandle of @a node to node offset.<br>
@a xref?<br></blockquote><div><span class="gmail_default" style="font-size:small">Fixed.</span> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> + *<br>
> + * @param[in] xref Node phandle<br>
> + *<br>
> + * @retval Node offset on success<br>
> + * @retval Node phandle on failure<br>
> + */<br>
> +phandle_t rtems_ofw_node_from_xref(<br>
> +  phandle_t   xref<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief This routine converts node offset to effective phandle of @a node.<br>
> + *<br>
> + * @retval Node phandle on success<br>
> + * @retval Node offset on failure<br>
> + */<br>
> +phandle_t rtems_ofw_xref_from_node(<br>
> +  phandle_t   node<br>
> +);<br>
> +<br>
> +/*<br>
> + * instance handles(ihandle_t) as same as phandles in the FDT implementation<br>
> + * of OF interface.<br>
> + */<br>
stray comments?<br></blockquote><div><br></div><div><div class="gmail_default" style="font-size:small">I'll add these comments to the type's documentation above.</div></div><div><br></div><div><div class="gmail_default" style="font-size:small">I also wanted to add a few other functions that are nor part</div><div class="gmail_default" style="font-size:small">of the OF interface but would be really beneficial for RTEMS.</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Can I add the following functions?</div><div class="gmail_default" style="font-size:small">1) rtems_ofw_get_reg</div><div class="gmail_default" style="font-size:small">2) rtems_ofw_status_okay</div><div class="gmail_default" style="font-size:small">3) rtems_ofw_get_interrupts</div><div class="gmail_default" style="font-size:small"><br></div><div class="gmail_default" style="font-size:small">Since these functions are not part of OFW should I use FDT</div><div class="gmail_default" style="font-size:small">instead of OFW in the function names?</div><div class="gmail_default" style="font-size:small">eg: rtems_fdt_get_reg <br></div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +<br>
> +/**<br>
> + * @brief Converts @a instance handle to phandle.<br>
> + *<br>
> + * instance are same as node offsets in FDT.<br>
> + *<br>
> + * @param[in] instance Node offset<br>
> + *<br>
> + * @retval phandle of node on success.<br>
> + * @retval instance of node on failure.<br>
> + */<br>
> +phandle_t rtems_ofw_instance_to_package(<br>
> +  ihandle_t   instance<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Find the node's path from phandle.<br>
> + *<br>
> + * @param[in] node Node offset<br>
> + * @param[out] buf Path is filled into this buffer(Pre-allocated).<br>
> + * @param[in] len Length of the buffer.<br>
> + *<br>
> + * @retval -1 always. Unimplemented.<br>
> + */<br>
> +size_t rtems_ofw_package_to_path(<br>
> +  phandle_t   node,<br>
> +  char       *buf,<br>
> +  size_t      len<br>
> +);<br>
> +<br>
> +/**<br>
> + * @brief Find the node's path from ihandle.<br>
> + *<br>
> + * @param[in] instance Node offset<br>
> + * @param[out] buf Path is filled into this buffer(Pre-allocated).<br>
> + * @param[in] len Length of the buffer.<br>
> + *<br>
> + * @retval -1 always. Unimplemented.<br>
> + */<br>
> +size_t rtems_ofw_instance_to_path(<br>
> +  ihandle_t  instance,<br>
> +  char      *buf,<br>
> +  size_t     len<br>
> +);<br>
> +<br>
> +#ifdef __cplusplus<br>
> +}<br>
> +#endif<br>
> +<br>
> +#endif /* _OFW_H */<br>
> \ No newline at end of file<br>
> --<br>
> 2.17.1<br>
><br>
</blockquote></div></div>