On 10/06/2020 17:11, Niteesh G. S. wrote:
> On Sat, Jun 6, 2020 at 5:40 PM Christian Mauderer <o...@c-mauderer.de
> <mailto:o...@c-mauderer.de>> wrote:
>
> On 04/06/2020 19:43, G S Niteesh Babu wrote:
> > The following files have been ported to RTEMS
> > 1) openfirm.h
> > 2) openfirm.c
> > 3) ofw_fdt.c
> > ---
> > cpukit/libfreebsd/dev/ofw/ofw_fdt.c | 117
> ++++++++++++++++++++++++++-
> > cpukit/libfreebsd/dev/ofw/openfirm.c | 59 +++++++++++++-
> > cpukit/libfreebsd/dev/ofw/openfirm.h | 18 +++++
> > 3 files changed, 191 insertions(+), 3 deletions(-)
> >
> > diff --git a/cpukit/libfreebsd/dev/ofw/ofw_fdt.c
> b/cpukit/libfreebsd/dev/ofw/ofw_fdt.c
> > index e4f72e8142..5341440789 100644
> > --- a/cpukit/libfreebsd/dev/ofw/ofw_fdt.c
> > +++ b/cpukit/libfreebsd/dev/ofw/ofw_fdt.c
> > @@ -29,10 +29,13 @@
> > * SUCH DAMAGE.
> > */
> >
> > +#ifndef __rtems__
> > #include <sys/cdefs.h>
> > __FBSDID("$FreeBSD$");
> > +#endif /* __rtems__ */
> >
> > #include <sys/param.h>
> > +#ifndef __rtems__
> > #include <sys/kernel.h>
> > #include <sys/malloc.h>
> > #include <sys/systm.h>
> > @@ -45,9 +48,18 @@ __FBSDID("$FreeBSD$");
> > #include <dev/ofw/ofwvar.h>
> > #include <dev/ofw/openfirm.h>
> > #include <dev/ofw/ofw_bus_subr.h>
> > +#endif /* __rtems__ */
> >
> > #include "ofw_if.h"
> > -
>
> I'm not really happy about lines starting with "-" in the porting
> commits. I think we should apply the same rules like in libbsd: Only add
> stuff. Exception: The one at the end of the file is OK (where git tells
> you "\ No newline at end of file"). You can't really avoid that.
>
>
> I fixed this.
>
>
>
> > +#ifdef __rtems__
> > +#include <rtems.h>
> > +#include <rtems/sysinit.h>
> > +#include <libfdt.h>
> > +#include <assert.h>
> > +#include "openfirm.h"
> > +#endif /* __rtems__ */
> > +
> > +#ifndef __rtems__
> > #ifdef DEBUG
> > #define debugf(fmt, args...) do { printf("%s(): ", __func__);
> \
> > printf(fmt,##args); } while (0)
> > @@ -63,6 +75,7 @@ __FBSDID("$FreeBSD$");
> > #define FDT_MARVELL
> > #endif
> > #endif
> > +#endif /* __rtems__ */
> >
> > static int ofw_fdt_init(ofw_t, void *);
> > static phandle_t ofw_fdt_peer(ofw_t, phandle_t);
> > @@ -78,6 +91,7 @@ static ssize_t ofw_fdt_canon(ofw_t, const char
> *, char *, size_t);
> > static phandle_t ofw_fdt_finddevice(ofw_t, const char *);
> > static ssize_t ofw_fdt_instance_to_path(ofw_t, ihandle_t, char *,
> size_t);
> > static ssize_t ofw_fdt_package_to_path(ofw_t, phandle_t, char *,
> size_t);
> > +#ifndef __rtems__
> > static int ofw_fdt_interpret(ofw_t, const char *, int, cell_t *);
> >
> > static ofw_method_t ofw_fdt_methods[] = {
> > @@ -104,9 +118,11 @@ static ofw_def_t ofw_fdt = {
> > 0
> > };
> > OFW_DEF(ofw_fdt);
> > +#endif /* __rtems__ */
> >
> > static void *fdtp = NULL;
> >
> > +#ifndef __rtems__
> > static int
> > sysctl_handle_dtb(SYSCTL_HANDLER_ARGS)
> > {
> > @@ -127,6 +143,24 @@ sysctl_register_fdt_oid(void *arg)
> > sysctl_handle_dtb, "", "Device Tree Blob");
> > }
> > SYSINIT(dtb_oid, SI_SUB_KMEM, SI_ORDER_ANY,
> sysctl_register_fdt_oid, NULL);
> > +#else /* __rtems__ */
> > +const void* bsp_fdt_get(void);
> > +static void
> > +ofw_init(void)
> > +{
> > + int rv;
> > + const void *fdt;
> > +
> > + fdt = bsp_fdt_get();
> > + rv = ofw_fdt_init(NULL, fdt);
> > + assert(rv == 0);
>
> Is assert really the only error handling possible here? Asserts are
> allways a bit difficult because they stop the entire system. In which
> cases would it hit?
>
>
> The assert is reached when the FDT is not valid. The check is done
> using fdt_check_header. Shutting the system would be the only way
> right?
It's in a low level driver function and if it fails, there is most
likely much else to do. So OK.
> I agree to assert is not the right way to quit. Can we quit with
> a proper error code like BSP_FATAL_INVALID_FDT or do we have
> something like that already?
I don't know of an existing one. Maybe add one. But maybe start a new
group in the bsp/fatal.h and make that a LIBFREEBSD_FATAL_INVALID_FDT.
I'm sure there will be other cases too sooner or later.
>
>
> > +}
> > +RTEMS_SYSINIT_ITEM(
> > + ofw_init,
> > + RTEMS_SYSINIT_BSP_START,
> > + RTEMS_SYSINIT_ORDER_FIRST
> > +);
> > +#endif /* __rtems__ */
> >
> > static int
> > ofw_fdt_init(ofw_t ofw, void *data)
> > @@ -297,7 +331,11 @@ ofw_fdt_getprop(ofw_t ofw, phandle_t package,
> const char *propname, void *buf,
> > if (prop == NULL)
> > return (-1);
> >
> > +#ifndef __rtems__
> > bcopy(prop, buf, min(len, buflen));
> > +#else /* __rtems__ */
> > + memcpy(buf, prop, MIN(len, buflen));
> > +#endif /* __rtems__ */
> >
> > return (len);
> > }
> > @@ -407,6 +445,7 @@ ofw_fdt_package_to_path(ofw_t ofw, phandle_t
> package, char *buf, size_t len)
> > return (-1);
> > }
> >
> > +#ifndef __rtems__
> > #if defined(FDT_MARVELL)
> > static int
> > ofw_fdt_fixup(ofw_t ofw)
> > @@ -476,4 +515,78 @@ ofw_fdt_interpret(ofw_t ofw, const char *cmd,
> int nret, cell_t *retvals)
> > #else
> > return (0);
> > #endif
> > -}
> > \ No newline at end of file
> > +}
> > +#endif /* __rtems__ */
> > +
> > +#ifdef __rtems__
> > +int OFW_INIT(ofw_t ofw_obj, void *cookie)
>
> Again: Please use FreeBSD style. The return value should be on it's onw
> line during the definition (not during declaration).
>
> > +{
>
> There should be one empty line if you have no local variables (I'm not
> really consequent here too).
>
> > + return ofw_fdt_init(ofw_obj, cookie);
>
> And brackets around everything that is returned.
>
>
> I fixed all.
>
>
>
> > +}
> > +
> > +phandle_t OFW_PEER(ofw_t ofw_obj, phandle_t node)
> > +{
> > + return ofw_fdt_peer(ofw_obj, node);
> > +}
> > +
> > +phandle_t OFW_CHILD(ofw_t ofw_obj, phandle_t node)
> > +{
> > + return ofw_fdt_child(ofw_obj, node);
> > +}
> > +
> > +phandle_t OFW_PARENT(ofw_t ofw_obj, phandle_t node)
> > +{
> > + return ofw_fdt_parent(ofw_obj, node);
> > +}
> > +
> > +phandle_t OFW_INSTANCE_TO_PACKAGE(ofw_t ofw_obj, ihandle_t instance)
> > +{
> > + return ofw_fdt_instance_to_package(ofw_obj, instance);
> > +}
> > +
> > +ssize_t OFW_GETPROPLEN(ofw_t ofw_obj, phandle_t package, const
> char *propname)
> > +{
> > + return ofw_fdt_getproplen(ofw_obj, package, propname);
> > +}
> > +
> > +ssize_t OFW_GETPROP(ofw_t ofw_obj, phandle_t package, const char
> *propname,
> > + void *buf, size_t buflen
> > +)
> > +{
> > + return ofw_fdt_getprop(ofw_obj, package, propname, buf, buflen);
> > +}
> > +
> > +int OFW_NEXTPROP(ofw_t ofw_obj, phandle_t package, const char
> *prev, char *buf,
> > + size_t size)
> > +{
> > + return ofw_fdt_nextprop(ofw_obj, package, prev, buf, size);
> > +}
> > +
> > +int OFW_SETPROP(ofw_t ofw_obj, phandle_t package, const char
> *propname,
> > + const void *buf, size_t len)
> > +{
> > + return ofw_fdt_setprop(ofw_obj, package, propname, buf, len);
> > +}
> > +
> > +ssize_t OFW_CANON(ofw_t ofw_obj, const char *device, char *buf,
> size_t len)
> > +{
> > + return ofw_fdt_canon(ofw_obj, device, buf, len);
> > +}
> > +
> > +phandle_t OFW_FINDDEVICE(ofw_t ofw_obj, const char *device)
> > +{
> > + return ofw_fdt_finddevice(ofw_obj, device);
> > +}
> > +
> > +ssize_t OFW_INSTANCE_TO_PATH(ofw_t ofw_obj, ihandle_t instance,
> char *buf,
> > + size_t len)
> > +{
> > + return ofw_fdt_instance_to_path(ofw_obj, instance, buf, len);
> > +}
> > +
> > +ssize_t OFW_PACKAGE_TO_PATH(ofw_t ofw_obj, phandle_t package,
> char *buf,
> > + size_t len)
> > +{
> > + return ofw_fdt_package_to_path(ofw_obj, package, buf, len);
> > +}
> > +#endif /* __rtems__ */
> > \ No newline at end of file
> > diff --git a/cpukit/libfreebsd/dev/ofw/openfirm.c
> b/cpukit/libfreebsd/dev/ofw/openfirm.c
> > index 50ebf2cf16..646c006e18 100644
> > --- a/cpukit/libfreebsd/dev/ofw/openfirm.c
> > +++ b/cpukit/libfreebsd/dev/ofw/openfirm.c
> > @@ -57,27 +57,39 @@
> > * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > */
> >
> > +#ifndef __rtems__
> > #include <sys/cdefs.h>
>
> Don't we have a sys/cdefs.h? Or is that from libbsd in my installation?
>
> It is not present in RTEMS but is present in newlib. What is this header
> used for? Do we really need it?
That header contains a lot of helper defines. Like variable attributes
and simmilar. I'm not sure why it was included in openfirm.c. But I
assume that the author of the file did include it for some reason. So if
we have it and the code compiles with the file: Why remove it. If it
makes problems but the code compiles without the file the #ifndef is OK.
>
> > __FBSDID("$FreeBSD$");
> >
> > #include "opt_platform.h"
> > +#endif /* __rtems__ */
> >
> > #include <sys/param.h>
> > +#ifndef __rtems__
> > #include <sys/kernel.h>
> > #include <sys/lock.h>
> > #include <sys/malloc.h>
> > #include <sys/mutex.h>
> > #include <sys/queue.h>
> > #include <sys/systm.h>
> > +#endif /* __rtems__ */
> > #include <sys/endian.h>
> >
> > +#ifndef __rtems__
> > #include <machine/stdarg.h>
> >
> > #include <dev/ofw/ofwvar.h>
> > #include <dev/ofw/openfirm.h>
> > +#endif /* __rtems__ */
> >
> > #include "ofw_if.h"
> > +#ifdef __rtems__
> > +#include <assert.h>
>
> Why do you need assert.h here?
>
>
> I missed it while cleaning.
>
>
>
> > +#include "openfirm.h"
> > +#include "../../rtems-freebsd-helper.h"
> > +#endif /* __rtems__ */
> >
> > +#ifndef __rtems__
> > static void OF_putchar(int c, void *arg);
> >
> > MALLOC_DEFINE(M_OFWPROP, "openfirm", "Open Firmware properties");
> > @@ -85,7 +97,9 @@ MALLOC_DEFINE(M_OFWPROP, "openfirm", "Open
> Firmware properties");
> > static ihandle_t stdout;
> >
> > static ofw_def_t *ofw_def_impl = NULL;
> > +#endif /* __rtems__ */
> > static ofw_t ofw_obj;
> > +#ifndef __rtems__
> > static struct ofw_kobj ofw_kernel_obj;
> > static struct kobj_ops ofw_kernel_kops;
> >
> > @@ -224,14 +238,18 @@ OF_install(char *name, int prio)
> >
> > return (FALSE);
> > }
> > +#endif /* __rtems__ */
> >
> > /* Initializer */
> > int
> > OF_init(void *cookie)
> > {
> > +#ifndef __rtems__
> > phandle_t chosen;
> > +#endif /* __rtems__ */
> > int rv;
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> >
> > @@ -242,17 +260,21 @@ OF_init(void *cookie)
> > */
> > kobj_class_compile_static(ofw_def_impl, &ofw_kernel_kops);
> > kobj_init_static((kobj_t)ofw_obj, ofw_def_impl);
> > +#endif /* __rtems__ */
> >
> > rv = OFW_INIT(ofw_obj, cookie);
> >
> > +#ifndef __rtems__
> > if ((chosen = OF_finddevice("/chosen")) != -1)
> > if (OF_getencprop(chosen, "stdout", &stdout,
> > sizeof(stdout)) == -1)
> > stdout = -1;
> > +#endif /* __rtems__ */
> >
> > return (rv);
> > }
> >
> > +#ifndef __rtems__
> > static void
> > OF_putchar(int c, void *arg __unused)
> > {
> > @@ -314,6 +336,7 @@ OF_interpret(const char *cmd, int nreturns, ...)
> >
> > return (status);
> > }
> > +#endif /* __rtems__ */
> >
> > /*
> > * Device tree functions
> > @@ -324,8 +347,10 @@ phandle_t
> > OF_peer(phandle_t node)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (0);
> > +#endif /* __rtems__ */
> >
> > return (OFW_PEER(ofw_obj, node));
> > }
> > @@ -335,8 +360,10 @@ phandle_t
> > OF_child(phandle_t node)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (0);
> > +#endif /* __rtems__ */
> >
> > return (OFW_CHILD(ofw_obj, node));
> > }
> > @@ -346,8 +373,10 @@ phandle_t
> > OF_parent(phandle_t node)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (0);
> > +#endif /* __rtems__ */
> >
> > return (OFW_PARENT(ofw_obj, node));
> > }
> > @@ -357,8 +386,10 @@ phandle_t
> > OF_instance_to_package(ihandle_t instance)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> > +#endif /* __rtems__ */
> >
> > return (OFW_INSTANCE_TO_PACKAGE(ofw_obj, instance));
> > }
> > @@ -368,8 +399,10 @@ ssize_t
> > OF_getproplen(phandle_t package, const char *propname)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> > +#endif /* __rtems__ */
> >
> > return (OFW_GETPROPLEN(ofw_obj, package, propname));
> > }
> > @@ -387,8 +420,10 @@ ssize_t
> > OF_getprop(phandle_t package, const char *propname, void *buf,
> size_t buflen)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> > +#endif /* __rtems__ */
> >
> > return (OFW_GETPROP(ofw_obj, package, propname, buf, buflen));
> > }
> > @@ -532,8 +567,10 @@ int
> > OF_nextprop(phandle_t package, const char *previous, char *buf,
> size_t size)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> > +#endif /* __rtems__ */
> >
> > return (OFW_NEXTPROP(ofw_obj, package, previous, buf, size));
> > }
> > @@ -543,8 +580,10 @@ int
> > OF_setprop(phandle_t package, const char *propname, const void
> *buf, size_t len)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> > +#endif /* __rtems__ */
> >
> > return (OFW_SETPROP(ofw_obj, package, propname, buf,len));
> > }
> > @@ -554,8 +593,10 @@ ssize_t
> > OF_canon(const char *device, char *buf, size_t len)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> > +#endif /* __rtems__ */
> >
> > return (OFW_CANON(ofw_obj, device, buf, len));
> > }
> > @@ -565,8 +606,10 @@ phandle_t
> > OF_finddevice(const char *device)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> > +#endif /* __rtems__ */
> >
> > return (OFW_FINDDEVICE(ofw_obj, device));
> > }
> > @@ -576,8 +619,10 @@ ssize_t
> > OF_instance_to_path(ihandle_t instance, char *buf, size_t len)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> > +#endif /* __rtems__ */
> >
> > return (OFW_INSTANCE_TO_PATH(ofw_obj, instance, buf, len));
> > }
> > @@ -587,8 +632,10 @@ ssize_t
> > OF_package_to_path(phandle_t package, char *buf, size_t len)
> > {
> >
> > +#ifndef __rtems__
> > if (ofw_def_impl == NULL)
> > return (-1);
> > +#endif /* __rtems__ */
> >
> > return (OFW_PACKAGE_TO_PATH(ofw_obj, package, buf, len));
> > }
> > @@ -626,14 +673,18 @@ OF_child_xref_phandle(phandle_t parent,
> phandle_t xref)
> > phandle_t
> > OF_node_from_xref(phandle_t xref)
> > {
> > +#ifndef __rtems__
> > struct xrefinfo *xi;
> > +#endif /* __rtems__ */
> > phandle_t node;
> >
> > +#ifndef __rtems__
> > if (xref_init_done) {
> > if ((xi = xrefinfo_find(xref, FIND_BY_XREF)) == NULL)
> > return (xref);
> > return (xi->node);
> > }
> > +#endif /* __rtems__ */
> >
> > if ((node = OF_child_xref_phandle(OF_peer(0), xref)) == -1)
> > return (xref);
> > @@ -643,14 +694,18 @@ OF_node_from_xref(phandle_t xref)
> > phandle_t
> > OF_xref_from_node(phandle_t node)
> > {
> > +#ifndef __rtems__
> > struct xrefinfo *xi;
> > +#endif /* __rtems__ */
> > phandle_t xref;
> >
> > +#ifndef __rtems__
> > if (xref_init_done) {
> > if ((xi = xrefinfo_find(node, FIND_BY_NODE)) == NULL)
> > return (node);
> > return (xi->xref);
> > }
> > +#endif /* __rtems__ */
> >
> > if (OF_getencprop(node, "phandle", &xref, sizeof(xref)) == -1 &&
> > OF_getencprop(node, "ibm,phandle", &xref, sizeof(xref))
> == -1 &&
> > @@ -659,6 +714,7 @@ OF_xref_from_node(phandle_t node)
> > return (xref);
> > }
> >
> > +#ifndef __rtems__
> > device_t
> > OF_device_from_xref(phandle_t xref)
> > {
> > @@ -845,4 +901,5 @@ OF_exit()
> >
> > for (;;) /* just in case */
> > ;
> > -}
> > \ No newline at end of file
> > +}
> > +#endif /* __rtems__ */
> > \ No newline at end of file
> > diff --git a/cpukit/libfreebsd/dev/ofw/openfirm.h
> b/cpukit/libfreebsd/dev/ofw/openfirm.h
> > index 74a7075367..00c0d4dd88 100644
> > --- a/cpukit/libfreebsd/dev/ofw/openfirm.h
> > +++ b/cpukit/libfreebsd/dev/ofw/openfirm.h
> > @@ -63,7 +63,13 @@
> > #define _DEV_OPENFIRM_H_
> >
> > #include <sys/types.h>
> > +#ifndef __rtems__
> > #include <machine/_bus.h>
> > +#else /* __rtems__ */
> > +#include <rtems.h>
> > +#include <libfdt.h>
>
> Maybe I missed it but for what do you need libfdt.h here?
>
> Again, missed it while cleaning.
>
>
> > +#include "../../rtems-freebsd-helper.h"
> > +#endif /* __rtems__ */
> >
> > /*
> > * Prototypes for Open Firmware Interface Routines
> > @@ -73,7 +79,12 @@ typedef uint32_t ihandle_t;
> > typedef uint32_t phandle_t;
> > typedef uint32_t pcell_t;
> >
> > +#ifdef __rtems__
> > +typedef uint32_t cell_t;
> > +#endif /* __rtems__ */
> > +
> > #ifdef _KERNEL
> > +#ifndef __rtems__
> > #include <sys/malloc.h>
> >
> > #include <machine/ofw_machdep.h>
> > @@ -86,8 +97,10 @@ MALLOC_DECLARE(M_OFWPROP);
> > */
> >
> > boolean_t OF_install(char *name, int prio);
> > +#endif /* __rtems__*/
> > int OF_init(void *cookie);
> >
> > +#ifndef __rtems__
> > /*
> > * Known Open Firmware interface names
> > */
> > @@ -100,6 +113,7 @@ int OF_init(void *cookie);
> > /* Generic functions */
> > int OF_test(const char *name);
> > void OF_printf(const char *fmt, ...);
> > +#endif /* __rtems__ */
> >
> > /* Device tree functions */
> > phandle_t OF_peer(phandle_t node);
> > @@ -141,6 +155,7 @@ ssize_t OF_package_to_path(phandle_t
> node, char *buf, size_t len);
> > phandle_t OF_node_from_xref(phandle_t xref);
> > phandle_t OF_xref_from_node(phandle_t node);
> >
> > +#ifndef __rtems__
> > /*
> > * When properties contain references to other nodes using xref
> handles it is
> > * often necessary to use interfaces provided by the driver for
> the referenced
> > @@ -158,9 +173,11 @@ void OF_close(ihandle_t instance);
> > ssize_t OF_read(ihandle_t instance, void *buf,
> size_t len);
> > ssize_t OF_write(ihandle_t instance, const void
> *buf, size_t len);
> > int OF_seek(ihandle_t instance, uint64_t where);
> > +#endif /* __rtems__ */
> >
> > phandle_t OF_instance_to_package(ihandle_t instance);
> > ssize_t OF_instance_to_path(ihandle_t instance, char
> *buf, size_t len);
> > +#ifndef __rtems__
> > int OF_call_method(const char *method, ihandle_t instance,
> > int nargs, int nreturns, ...);
> >
> > @@ -183,5 +200,6 @@ int OF_interpret(const char
> *cmd, int nreturns, ...);
> > int OF_decode_addr(phandle_t dev, int regno,
> bus_space_tag_t *ptag,
> > bus_space_handle_t *phandle, bus_size_t *sz);
> >
> > +#endif /* __rtems__ */
> > #endif /* _KERNEL */
> > #endif /* _DEV_OPENFIRM_H_ */
> > \ No newline at end of file
> >
>
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
