Setup console from Device Tree (3) #1433

jforissier · 2017-03-21T16:58:36Z

This is the third rework of my proof-of-concept code to use DT in OP-TEE to configure devices, namely: the console UART.

jforissier · 2017-03-28T11:29:35Z

This PR can be logically split in 3 parts:

The first 4 commits deal with basic DT support for OP-TEE drivers,
The 5th and 6th commits add DT support for the PL011 serial driver,
The 7th commit reads the console configuration from DT (PL011 only)

jforissier · 2017-04-24T14:25:21Z

Rebased and Travis errors (checkpatch) fixed. Any feedback?

etienne-lms · 2017-04-25T07:04:42Z

core/arch/arm/kernel/generic_boot.c

+ * for which we have a compatible driver. If so, switch the console to
+ * this device.
+ */
+static void configure_console_from_dt(unsigned long phys_fdt)


could move out of generic_boot.c ? maybe core/kernel/console.c.

etienne-lms · 2017-04-25T07:13:41Z

core/kernel/dt.c

+	return (!strncmp(st, "ok", len));
+}
+
+int _fdt_get_status(const void *fdt, int offs, int *status)


could return void or status.

OK, will return status

etienne-lms · 2017-04-25T07:19:32Z

core/kernel/dt.c

+	/* Both "ok" and "okay" are valid */
+	if (len > 2)
+		len = 2;
+	return (!strncmp(st, "ok", len));


I think it's better to strictly test "ok" and "okay".
Monir: null string returning OK status could also be handle in this function.

I think it's better to strictly test "ok" and "okay".

Yep.

Monir: null string returning OK status could also be handle in this function.

That wouldn't help much -- would not work for secure-status which defaults to the same value as status when null (i.e., not always "okay").

etienne-lms · 2017-04-25T07:34:25Z

core/include/kernel/dt.h

+	return NULL;
+}
+
+#define __dt_driver


If CFG_DT is not defined, i could be nice to not define neither __dt_driver to insure one cannot implement DT dependent resources without enabling CFG_DT=y.

etienne-lms · 2017-04-25T07:44:06Z

core/arch/arm/kernel/generic_boot.c

+		 * means we don't want any console output
+		 */
+		IMSG("Switching off console");
+		register_serial_console(NULL);


This mandates a secure-chosen stdout-path in the DT. I think it is too restrictive.
One could define a DT aware uart driver in OP-TEE and expect the DT parser to find the related node, and register the console upon value of the property "secure-status".

Note that if /secure-chosen does not exist in the DT, the function does nothing (see a few lines above).

Also, there could be several UART devices with secure-status = okay, which one would you use as the console? This is really the job of the /[secure-]chosen node: pass some configuration parameters from the firmware to the [secure] OS. In addition, there might be additional configuration parameter (baud rate, parity) which really belong in /[secure-]chosen/stdout-path. This is well documented already (https://www.kernel.org/doc/Documentation/devicetree/bindings/chosen.txt).

etienne-lms · 2017-04-27T09:07:44Z

core/kernel/console.c

+				break;
+			}
+		}
+	}


This seems quite generic to DT parsing in OP-TEE. Could it be moved to your new core/kernel/dt.c.
(It look a lot like the dt_find_driver() you implemented)

etienne-lms · 2017-04-27T09:09:52Z

core/kernel/console.c

+		goto out;
+	}
+
+	IMSG("Switching console to device: %s", uart);


Minor: I would move this trace either before the new device init (before console_flush()) or after the new device registering.

Not after the new console has registered, because I want it to appear on the "old" console (assuming they're different) to help the user understand where the subsequent output will go.

About moving it before console_flush(): we don't know yet if the init of the new console is OK (and we don't want to print the message if it failed). Would you want a console_flush() after this DMSG()? Seems pretty useless.

I was wondering if old and new console are the same, won't it mess in some way if core it pushing traces after the device init but before its registration.

Hmmm no, I don't think so. register_serial_console() only sets a pointer.

etienne-lms · 2017-04-27T09:10:20Z

core/kernel/console.c

+ */
+void configure_console_from_dt(unsigned long phys_fdt)
+{
+	const struct dt_driver *drv, *found = NULL;


minor: 1 line per variable.

etienne-lms · 2017-04-27T09:19:43Z

core/kernel/dt.c

+	assert(cpu_mmu_enabled());
+
+	st = _fdt_get_status(fdt, offs);
+	if (st == -1 || st == DT_STATUS_DISABLED)


minor: We don't expect st == -1 when CFG_DT is enable.

etienne-lms

Look ok to me. Still few minor comment (there will always be).
Yet: Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

It is mergeable, due to deps on node "/secure-chosen" ?.

etienne-lms · 2017-04-27T14:05:49Z

core/kernel/console.c

+		goto out;
+	}
+
+	IMSG("Switching console to device: %s", uart);


etienne-lms · 2017-04-27T14:07:15Z

core/kernel/dt.c

+			st = fdt_node_check_compatible(fdt, offs,
+							dm->compatible);
+			if (!st)
+				return drv;


minor: could get rid of st

etienne-lms · 2017-04-27T14:13:53Z

core/include/kernel/dt.h

+ */
+#define DT_STATUS_DISABLED 0
+#define DT_STATUS_OK_NSEC  1
+#define DT_STATUS_OK_SEC   2


since DT_STATUS_OK_NSEC/SEC are bit fields, it would be more explicit with

#define DT_STATUS_OK_NSEC BIT(0) #define DT_STATUS_OK_NSEC BIT(1)

jforissier · 2017-04-27T15:30:27Z

Additional comments addressed, rebased on top of master. Will add the review tags later. Since I am introducing new framework code, I'd like Reviewed-bys or Acked-bys from @jenswi-linaro and/or @jbech-linaro especially.

@etienne-lms this can safely be merged, because the dependency on /secure-chosen is not a 'hard' one: the code will do nothing when the node is not present.

jenswi-linaro · 2017-04-27T15:41:24Z

core/include/kernel/dt.h

+	const void *driver;
+};
+
+#define __dt_driver __attribute__((__section__(".rodata.dtdrv")))


Please base it on __section() instead

jenswi-linaro · 2017-04-27T15:43:34Z

core/include/kernel/dt.h

+
+#define __dt_driver __attribute__((__section__(".rodata.dtdrv")))
+
+extern struct dt_driver __rodata_dtdrv_start, __rodata_dtdrv_end;


Would it work to make them const?

jenswi-linaro · 2017-04-27T15:48:10Z

core/kernel/dt.c

+
+static bool is_okay(const char *st, int len)
+{
+	return (!strncmp(st, "ok", len) || !strncmp(st, "okay", len));


Outer parenthesis not needed.

jenswi-linaro · 2017-04-27T17:03:48Z

core/kernel/dt.c

+	else
+		mtype = MEM_AREA_IO_NSEC;
+
+	vbase = (vaddr_t)phys_to_virt(pbase, mtype);


What about pbase + sz - 1?

What do you mean?

So you check that the base address is mapped, but not the entire space needed for the device.

Ah, yes, I assumed the memory would normally be mapped only through this function, which is obviously wrong. I'll fix.

jenswi-linaro · 2017-04-28T07:42:15Z

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Use the functions provided by libfdt instead of our own. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

When CFG_DT=y, a linker section is created (.rodata.dtdrv) to hold all the DT-compatible drivers. The table can later be queried at runtime. Some manipulation functions are exported in <kernel/dt.h>. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Maps a device into memory from its FDT node. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Add struct serial_driver which will be useful to UART drivers that want to create devices from a DT node. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

…path If CFG_DT=y, check the Device Tree's /secure-chosen node and look for the stdout-path property. Adjust the console output accordingly. The DT bindings for this property have been proposed on the LKML [1]. [1] https://www.spinics.net/lists/arm-kernel/msg566034.html Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (QEMU) Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

jforissier mentioned this pull request Mar 21, 2017

Setup console from Device Tree #1388

Closed

jforissier force-pushed the dt3 branch 2 times, most recently from 38a3a1c to 5f064f0 Compare April 24, 2017 13:56

etienne-lms reviewed Apr 25, 2017

View reviewed changes

jforissier force-pushed the dt3 branch from a58a9af to 97baf25 Compare April 27, 2017 08:37

etienne-lms reviewed Apr 27, 2017

View reviewed changes

jforissier force-pushed the dt3 branch 2 times, most recently from d0e630b to 17b0d8b Compare April 27, 2017 13:50

etienne-lms reviewed Apr 27, 2017

View reviewed changes

jforissier force-pushed the dt3 branch from 17b0d8b to c1891d4 Compare April 27, 2017 15:19

jenswi-linaro reviewed Apr 27, 2017

View reviewed changes

jforissier added 7 commits April 28, 2017 10:31

dt: Add FDT manipulation functions

4012bad

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

dt: Add dt_map_dev()

8b80e7b

Maps a device into memory from its FDT node. Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

pl011: dt: Add DT support

5879461

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

jforissier force-pushed the dt3 branch from 97060f9 to 2da57cc Compare April 28, 2017 08:33

jforissier merged commit 4dc31c5 into OP-TEE:master Apr 28, 2017

jforissier deleted the dt3 branch April 28, 2017 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup console from Device Tree (3) #1433

Setup console from Device Tree (3) #1433

jforissier commented Mar 21, 2017

jforissier commented Mar 28, 2017

jforissier commented Apr 24, 2017

etienne-lms Apr 25, 2017

jforissier Apr 27, 2017

etienne-lms Apr 25, 2017

jforissier Apr 27, 2017

etienne-lms Apr 25, 2017

jforissier Apr 27, 2017

etienne-lms Apr 25, 2017

jforissier Apr 27, 2017

etienne-lms Apr 25, 2017

jforissier Apr 27, 2017

etienne-lms Apr 27, 2017

etienne-lms Apr 27, 2017

jforissier Apr 27, 2017

etienne-lms Apr 27, 2017

jforissier Apr 27, 2017

etienne-lms Apr 27, 2017

etienne-lms Apr 27, 2017

etienne-lms Apr 27, 2017

etienne-lms left a comment

etienne-lms Apr 27, 2017

etienne-lms Apr 27, 2017

etienne-lms Apr 27, 2017

jforissier commented Apr 27, 2017

jenswi-linaro Apr 27, 2017

jenswi-linaro Apr 27, 2017

jenswi-linaro Apr 27, 2017

jenswi-linaro Apr 27, 2017

jforissier Apr 28, 2017

jenswi-linaro Apr 28, 2017

jforissier Apr 28, 2017

jenswi-linaro commented Apr 28, 2017


		#define __dt_driver __attribute__((__section__(".rodata.dtdrv")))

		extern struct dt_driver __rodata_dtdrv_start, __rodata_dtdrv_end;

Setup console from Device Tree (3) #1433

Setup console from Device Tree (3) #1433

Conversation

jforissier commented Mar 21, 2017

jforissier commented Mar 28, 2017

jforissier commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jforissier commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenswi-linaro commented Apr 28, 2017