On Fri, Apr 08, 2016 at 06:09:38PM +0200, Martin Pieuchot wrote:
> On 08/04/16(Fri) 17:14, Patrick Wildt wrote:
> > On Fri, Apr 08, 2016 at 04:34:25PM +0200, Martin Pieuchot wrote:
> > > On 08/04/16(Fri) 14:46, Patrick Wildt wrote:
> > > > Hi,
> > > >
> > > > so that we can easily check if a node is compatible or to find nodes
> > > > that are compatible, I would like to add helpers to the fdt routines.
> > > >
> > > > This way the drivers can check if they "match" to a node by simply
> > > > calling:
> > > >
> > > > if (fdt_node_compatible(ma->ma_node, "samsung,exynos4210-ehci"))
> > > > return (1);
> > > >
> > > > Sometimes it's helpful to find any node that is compatible. It can
> > > > be helpful for instance on finding the early uart. Simplified example:
> > > >
> > > > if ((node = fdt_find_compatible("arm,pl011")) != NULL)
> > > > pl011cnattach(...);
> > > >
> > > > Thoughts? ok?
> > >
> > > Always hard to comment without seeing which code would use that.
> > >
> > > Is a custom function really needed? Why not do like sparc64 and macppc,
> > > for example your ehci_match() could be:
> > >
> > > {
> > > char compat[32];
> > > ...
> > >
> > > if (OF_getprop(ma->ma_node, "compatible", compat, sizeof(compat)) == -1)
> > > return 0;
> > >
> > > if (strcmp(compat, "samsung,exynos4210-ehci") == 0
> > > return 1;
> > >
> > > return 0;
> > > }
> > >
> >
> > The compatible attribute can be a list of strings:
>
> Yes, that's also true for macppc.
>
> > Implementing parsing multiple strings in a buffer and checking it in
> > every _match function seemed a bit too much overhead.
>
> Seems or is overhead? How is the reality? From my experience dealing
> with Apple device-tree most of the time you just use one of the
> compatible entry.
You usually look for one entry in an attribute with only one string.
Though it's happens often that the attribute has multiple strings and
the one I'm looking for isn't in the first.
In one tree I have 63 calls of fdt_node_compatible. 9 drivers check for
multiple strings.
>
> > Imagine 20 drivers
> > doing the same code over and over again.
>
> So? What I find really nice about FDT is that you can use the OF_* API.
> That means we can write a driver for ARMv7 the same way we do it for
> sparc64 and macppc. So it's easier to learn an maintain. If you
> introduce fdt-only functions, we lose this advantage.
>
> Now there might be a real benefit to having such function, but in that
> case I'd suggest to implement it in the OF_* API.
Yes, that would harm the OF_* API. If there's a way to implement it
using the OF_* API, I would not be against that.
>
> For the moment I haven't seen any code that in my opinion justify this.
>
> > Especially if you're also are not looking for one compatible, but
> > multiple compatibles:
>
> You have some example, I don't find the code hard to follow:
>
> macppc: aoa, wdc@obio, mpcpci
> sparc64: com@ebus, comkbd, comms, ofwiic, pcfiic, pyro, schizo
>
Ok, so here's one example: The platform code for FDT on ARMv7, which
has a bit of code to find an early console to use. It's a heavier user
of finding compatible nodes and matching them.
I could also implement the proposed wrappers for fdt_machdep.c and keep
it outside of sys/dev/ofw/fdt.c, but I'm not sure such "generic" functions
should be put into MD code.
diff --git sys/arch/armv7/fdt/fdt_machdep.c sys/arch/armv7/fdt/fdt_machdep.c
new file mode 100644
index 0000000..3f973f6
--- /dev/null
+++ b/sys/arch/armv7/fdt/fdt_machdep.c
@@ -0,0 +1,173 @@
+/*
+ * Copyright (c) 2014 Patrick Wildt <patr...@blueri.se>
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include <sys/param.h>
+#include <sys/types.h>
+#include <sys/systm.h>
+#include <sys/termios.h>
+
+#include <machine/bus.h>
+
+#include <dev/ofw/fdt.h>
+
+#include <dev/ic/comreg.h>
+#include <dev/ic/comvar.h>
+
+#include <arm/cortex/smc.h>
+#include <arm/armv7/armv7var.h>
+#include <armv7/armv7/armv7var.h>
+#include <armv7/armv7/armv7_machdep.h>
+
+#include <armv7/vexpress/pl011var.h>
+#include <armv7/imx/imxuartvar.h>
+#include <armv7/exynos/exdisplayvar.h>
+
+extern int comcnspeed;
+extern int comcnmode;
+
+void
+fdt_platform_smc_write(bus_space_tag_t iot, bus_space_handle_t ioh, bus_size_t
off,
+ uint32_t op, uint32_t val)
+{
+ bus_space_write_4(iot, ioh, off, val);
+}
+
+int
+fdt_find_stdout(char *compatible, struct fdt_memory *mem)
+{
+ char *stdout_path;
+ char *stdout_options;
+ void *node;
+
+ if (compatible == NULL || mem == NULL)
+ return 1;
+
+ if ((node = fdt_find_node("/chosen")) != NULL &&
+ (fdt_node_property(node, "stdout-path", &stdout_path) ||
+ fdt_node_property(node, "linux,stdout-path", &stdout_path))) {
+ if ((stdout_options = strchr(stdout_path, ':')) != NULL)
+ *stdout_options = '\0';
+ if ((node = fdt_find_node(stdout_path)) != NULL &&
+ fdt_node_compatible(node, compatible))
+ return fdt_get_memory_address(node, 0, mem);
+ }
+
+ if ((node = fdt_find_compatible(compatible)) != NULL)
+ return fdt_get_memory_address(node, 0, mem);
+
+ return 1;
+}
+
+void
+fdt_platform_init_cons(void)
+{
+ struct fdt_memory mem;
+ uint32_t freq;
+
+ /*
+ * XXX: Without having all clocks attached, we cannot look up
+ * XXX: the frequency. Thus make an educated guess.
+ */
+ if (fdt_find_compatible("marvell,armada-370-xp"))
+ freq = 250000000;
+ else if (fdt_find_compatible("allwinner,sun7i-a20") ||
+ fdt_find_compatible("allwinner,sun6i-a31"))
+ freq = 24000000;
+ else
+ freq = COM_FREQ;
+
+#if notyet
+ if (!fdt_find_stdout("simple-framebuffer", &mem))
+ exdisplay_cnattach(&armv7_bs_tag, mem.addr, mem.size);
+#endif
+
+ if (!fdt_find_stdout("arm,pl011", &mem))
+ pl011cnattach(&armv7_bs_tag, mem.addr, comcnspeed, comcnmode);
+
+ if (!fdt_find_stdout("snps,dw-apb-uart", &mem))
+ comcnattach(&armv7_a4x_bs_tag, mem.addr, comcnspeed,
+ freq, comcnmode);
+
+ if (!fdt_find_stdout("fsl,ns16550", &mem) ||
+ !fdt_find_stdout("fsl,16550-FIFO64", &mem))
+ comcnattach(&armv7_bs_tag, mem.addr, comcnspeed,
+ 150000000, comcnmode);
+
+ if (!fdt_find_stdout("fsl,imx6q-uart", &mem))
+ imxuartcnattach(&armv7_bs_tag, mem.addr, comcnspeed, comcnmode);
+
+ comdefaultrate = comcnspeed;
+}
+
+void (*fdt_platform_watchdog_reset_fn)(void);
+void
+fdt_platform_watchdog_reset(void)
+{
+ if (fdt_platform_watchdog_reset_fn != NULL)
+ fdt_platform_watchdog_reset_fn();
+}
+
+void
+fdt_platform_powerdown(void)
+{
+
+}
+
+const char *
+fdt_platform_board_name(void)
+{
+ void *node = fdt_next_node(0);
+ char *compatible;
+
+ if (!fdt_node_property(node, "compatible", &compatible))
+ return "unknown";
+ else
+ return compatible;
+}
+
+void
+fdt_platform_disable_l2_if_needed(void)
+{
+
+}
+
+void
+fdt_platform_board_init(void)
+{
+
+}
+
+struct armv7_platform fdt_platform = {
+ .boot_name = "OpenBSD/fdt",
+ .board_name = fdt_platform_board_name,
+ .board_init = fdt_platform_board_init,
+ .smc_write = fdt_platform_smc_write,
+ .init_cons = fdt_platform_init_cons,
+ .watchdog_reset = fdt_platform_watchdog_reset,
+ .powerdown = fdt_platform_powerdown,
+ .disable_l2_if_needed = fdt_platform_disable_l2_if_needed,
+};
+
+struct armv7_platform *
+fdt_platform_match(void)
+{
+ /* If we're running without fdt, do not attach. */
+ /* XXX: Find a better way. */
+ if (fdt_next_node(0) == NULL)
+ return (NULL);
+
+ return (&fdt_platform);
+}
