[PATCH] bsps/shared/ofw: Added and Documented RTEMS OFW interface
Niteesh G. S.
niteesh.gs at gmail.com
Thu Aug 13 10:31:05 UTC 2020
Hello,
On Thu, Aug 13, 2020 at 12:09 PM Christian Mauderer <
christian.mauderer at 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 <gedare at rtems.org
> > <mailto:gedare at 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.gs at gmail.com <mailto:niteesh.gs at 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.gs at gmail.com
> > <mailto:niteesh.gs at 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.mauderer at 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20200813/640b370d/attachment-0001.html>
More information about the devel
mailing list