Message-Id: <FE61DD1C-51DA-4869-8A62-C019C3D70577@konsulko.com>
Date:	Wed, 18 Feb 2015 17:53:50 +0200
From:	Pantelis Antoniou <pantelis.antoniou@...sulko.com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Grant Likely <grant.likely@...retlab.ca>,
	Matt Porter <matt.porter@...aro.org>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Guenter Roeck <linux@...ck-us.net>,
	Ludovic Desroches <ludovic.desroches@...el.com>,
	Rob Herring <robherring2@...il.com>,
	Tony Lindgren <tony@...mide.com>,
	Nicolas Ferre <nicolas.ferre@...el.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/4] of: DT quirks infrastructure

Hi Mark,

> On Feb 18, 2015, at 17:41 , Mark Rutland <mark.rutland@....com> wrote:
> 
> Hi Pantelis,
> 
> On Wed, Feb 18, 2015 at 02:59:34PM +0000, Pantelis Antoniou wrote:
>> Implement a method of applying DT quirks early in the boot sequence.
>> 
>> A DT quirk is a subtree of the boot DT that can be applied to
>> a target in the base DT resulting in a modification of the live
>> tree. The format of the quirk nodes is that of a device tree overlay.
>> 
>> For details please refer to Documentation/devicetree/quirks.txt
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@...sulko.com>
>> ---
>> Documentation/devicetree/quirks.txt | 101 ++++++++++
>> drivers/of/dynamic.c                | 358 ++++++++++++++++++++++++++++++++++++
>> include/linux/of.h                  |  16 ++
>> 3 files changed, 475 insertions(+)
>> create mode 100644 Documentation/devicetree/quirks.txt
>> 
>> diff --git a/Documentation/devicetree/quirks.txt b/Documentation/devicetree/quirks.txt
>> new file mode 100644
>> index 0000000..789319a
>> --- /dev/null
>> +++ b/Documentation/devicetree/quirks.txt
>> @@ -0,0 +1,101 @@
>> +A Device Tree quirk is the way which allows modification of the
>> +boot device tree under the control of a per-platform specific method.
>> +
>> +Take for instance the case of a board family that comprises of a
>> +number of different board revisions, all being incremental changes
>> +after an initial release.
>> +
>> +Since all board revisions must be supported via a single software image
>> +the only way to support this scheme is by having a different DTB for each
>> +revision with the bootloader selecting which one to use at boot time.
> 
> Not necessarily at boot time. The boards don't have to run the exact
> same FW/bootloader binary, so the relevant DTB could be programmed onto
> each board.
> 

That has not been the case in any kind of board I’ve worked with.

A special firmware image that requires a different programming step at
the factory to select the correct DTB for each is always one more thing
that can go wrong.

>> +While this may in theory work, in practice it is very cumbersome
>> +for the following reasons:
>> +
>> +1. The act of selecting a different boot device tree blob requires
>> +a reasonably advanced bootloader with some kind of configuration or
>> +scripting capabilities. Sadly this is not the case many times, the
>> +bootloader is extremely dumb and can only use a single dt blob.
> 
> You can have several bootloader builds, or even a single build with
> something like appended DTB to get an appropriate DTB if the same binary
> will otherwise work across all variants of a board.
> 

No, the same DTB will not work across all the variants of a board.

> So it's not necessarily true that you need a complex bootloader.
> 

>> +2. On many instances boot time is extremely critical; in some cases
>> +there are hard requirements like having working video feeds in under
>> +2 seconds from power-up. This leaves an extremely small time budget for
>> +boot-up, as low as 500ms to kernel entry. The sanest way to get there
>> +is by removing the standard bootloader from the normal boot sequence
>> +altogether by having a very small boot shim that loads the kernel and
>> +immediately jumps to kernel, like falcon-boot mode in u-boot does.
> 
> Given my previous comments above I don't see why this is relevant.
> You're already passing _some_ DTB here, so if you can organise for the
> board to statically provide a sane DTB that's fine, or you can resort to
> appended DTB if it's not possible to update the board configuration.
> 

You’re missing the point. I can’t use the same DTB for each revision of the
board. Each board is similar but it’s not identical.

>> +3. Having different DTBs/DTSs for different board revisions easily leads to
>> +drift between versions. Since no developer is expected to have every single
>> +board revision at hand, things are easy to get out of sync, with board versions
>> +failing to boot even though the kernel is up to date.
> 
> I'm not sure I follow. Surely if you don't have every board revision at
> hand you can't test quirks exhaustively either?
> 

It’s one less thing to worry about. For example in the current mainline kernel
already there is a drift between the beaglebone white and the beaglebone black.

Having the same DTS is just easier to keep things in sync.

> Additionally you face the problem that two boards of the same variant
> could have different base DTBs that you would need to test that each
> board's quirks worked for a range of base DTBs.
> 

This is not a valid case. This patch is about boards that have the same base DTB.

>> +4. One final problem is the static way that device tree works.
>> +For example it might be desirable for various boards to have a way to
>> +selectively configure the boot device tree, possibly by the use of command
>> +line options.  For instance a device might be disabled if a given command line
>> +option is present, different configuration to various devices for debugging
>> +purposes can be selected and so on. Currently the only way to do so is by
>> +recompiling the DTS and installing, which is an chore for developers and
>> +a completely unreasonable expectation from end-users.
> 
> I'm not sure I follow here.
> 
> Which devices do you envisage this being the case for?
> 
> Outside of debug scenarios when would you envisage we do this?
> 

We already have to do this on the beaglebone black. The onboard EMMC and HDMI
devices conflict with any capes that use the same pins. So you have to
have a way to disable them so that the attached cape will work.
 
> For the debug case it seems reasonable to have command line parameters
> to get the kernel to do what we want. Just because there's a device in
> the DTB that's useful in a debug scenario doesn't mean we have to use it
> by default.

I don’t follow. Users need this functionality to work. I.e. pass a command
line option to use different OPPs etc. Real world usage is messy.

> 
>> +Device Tree quirks solve all those problems by having an in-kernel interface
>> +which per-board/platform method can use to selectively modify the device tree
>> +right after unflattening.
>> +
>> +A DT quirk is a subtree of the boot DT that can be applied to
>> +a target in the base DT resulting in a modification of the live
>> +tree. The format of the quirk nodes is that of a device tree overlay.
>> +
>> +As an example the following DTS contains a quirk.
>> +
>> +/ {
>> +       foo: foo-node {
>> +               bar = <10>;
>> +       };
>> +
>> +       select-quirk = <&quirk>;
>> +
>> +       quirk: quirk {
>> +               fragment@0 {
>> +                       target = <&foo>;
>> +                       __overlay {
>> +                               bar = <0xf00>;
>> +                               baz = <11>;
>> +                       };
>> +               };
>> +       };
>> +};
>> +
>> +The quirk when applied would result at the following tree:
>> +
>> +/ {
>> +       foo: foo-node {
>> +               bar = <0xf00>;
>> +               baz = <11>;
>> +       };
>> +
>> +       select-quirk = <&quirk>;
>> +
>> +       quirk: quirk {
>> +               fragment@0 {
>> +                       target = <&foo>;
>> +                       __overlay {
>> +                               bar = <0xf00>;
>> +                               baz = <11>;
>> +                       };
>> +               };
>> +       };
>> +
>> +};
>> +
>> +The two public method used to accomplish this are of_quirk_apply_by_node()
>> +and of_quirk_apply_by_phandle();
>> +
>> +To apply the quirk, a per-platform method can retrieve the phandle from the
>> +select-quirk property and pass it to the of_quirk_apply_by_phandle() node.
>> +
>> +The method which the per-platform method is using to select the quirk to apply
>> +is out of the scope of the DT quirk definition, but possible methods include
>> +and are not limited to: revision encoding in a GPIO input range, board id
>> +located in external or internal EEPROM or flash, DMI board ids, etc.
> 
> It seems rather unfortunate that to get a useful device tree we have to
> resort to board-specific mechanisms. That means yet more platform code,
> which is rather unfortunate. This would also require any other DT users
> to understand and potentially have to sanitize any quirks (e.g. in the
> case of Xen).

The original internal version of the patches used early platform devices and
a generic firmware quirk mechanism, but I was directed to the per-platform
method instead. It is perfectly doable to go back at the original implementation
but I need to get the ball rolling with a discussion about the internals.
 
> 
> Mark.

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists