[PATCH] bsps/shared/ofw: Added and Documented RTEMS OFW interface
Gedare Bloom
gedare at rtems.org
Thu Aug 13 14:16:28 UTC 2020
On Thu, Aug 13, 2020 at 4:31 AM Niteesh G. S. <niteesh.gs at gmail.com> wrote:
>
> 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.
>> >
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.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.
More information about the devel
mailing list