Re: [PATCH 4/5] firmware: Add coreboot device tree binding documentation

[Brian Norris]

Hi Mark,

Julius can correct me if I'm wrong on any of this, but a quick note that
probably should have prefaced this:

These bindings were submitted years go

https://patchwork.ozlabs.org/patch/359672/
[PATCH] firmware: Add device tree binding for coreboot

discussed, and resubmitted seemingly according to several reviewers'
suggestions here

https://patchwork.kernel.org/patch/4362661/
[PATCH v2] firmware: Add device tree binding for coreboot

after which point we heard no response.

So these bindings have been baked into our coreboot firmware ever since
and are impossible to change on existing devices. It's *possible* we
could make modifications for new products, but it's really a non-starter
for this binding to be rejected at this point. (Or at least, if you want
this stuff to work in mainline for any Chromebook made in the last
several years.) If it's rejected, Chromebooks will just have to continue
shipping with forked drivers in our downstream tree.

Again, IMO that doesn't mean that this binding can't be improved or
augmented, but I think it's worth very serious consideration that the
legacy binding be considered and documented, even if it's noted as
"deprecated" or something like that.

On Fri, Mar 24, 2017 at 12:21:46PM +0000, Mark Rutland wrote:
> On Thu, Mar 23, 2017 at 10:04:28PM +0100, Thierry Escande wrote:
> > From: Julius Werner <jwerner@xxxxxxxxxxxx>
> > 
> > This patch adds documentation describing a device tree binding for the
> > coreboot firmware. It is meant to be dynamically added during boot and
> > contains address definitions for the coreboot table (a list of
> > variable-sized descriptors providing information about various compile-
> > and run-time generated firmware parameters) and the CBMEM area (the
> > structure containing most run-time resident memory regions set up by
> > coreboot).
> > 
> > These definitions allow kernel drivers to easily access data contained
> > in and pointed to by these regions (such as coreboot's in-memory log).
> > (An example implementation can be seen in the following patch)
> > 
> > Signed-off-by: Julius Werner <jwerner@xxxxxxxxxxxx>
> > Signed-off-by: Thierry Escande <thierry.escande@xxxxxxxxxxxxx>
> > ---
> >  .../devicetree/bindings/firmware/coreboot.txt      | 33 ++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/firmware/coreboot.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/firmware/coreboot.txt b/Documentation/devicetree/bindings/firmware/coreboot.txt
> > new file mode 100644
> > index 0000000..4c95570
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/firmware/coreboot.txt
> > @@ -0,0 +1,33 @@
> > +COREBOOT firmware information
> > +
> > +The device tree node to communicate the location of coreboot's memory-resident
> > +bookkeeping structures to the kernel. Since coreboot itself cannot boot a
> > +device-tree-based kernel (yet), this node needs to be inserted by a
> > +second-stage bootloader (a coreboot "payload").
> > +
> > +Required properties:
> > + - compatible: Should be "coreboot"
> 
> Devicetree bindings should be in vendor,prefix format. This doesn't
> represent every aspect of coreboot, so it needs a more descriptive
> string.

Any particular suggestion? I suppose this is Google's interpretation of
coreboot tables, so "google,coreboot"?

> > + - reg: Address and length of the following two memory regions, in order:
> > +	1.) The coreboot table. This is a list of variable-sized descriptors
> > +	that contain various compile- and run-time generated firmware
> > +	parameters. It is identified by the magic string "LBIO" in its first
> > +	four bytes.
> > +	See coreboot's src/commonlib/include/commonlib/coreboot_tables.h for
> > +	details.
> 
> Given this is a memory region, it should be described under the
> reserved-memory node.

We've painted this bikeshed before. I guess the result was either to use
/reserved-memory or /memreserve/. I believe we've been using
/memreserve/. I suppose we could document a method to use either, but
the main "agreement" was to use the /firmware/coreboot path instead of
/reserved-memory/coreboot.

(And yes, I suppose /memreserve/ should be noted here.)

Brian

> > +	2.) The CBMEM area. This is a downward-growing memory region used by
> > +	coreboot to dynamically allocate data structures that remain resident.
> > +	It may or may not include the coreboot table as one of its members. It
> > +	is identified by a root node descriptor with the magic number
> > +	0xc0389481 that resides in the topmost 8 bytes of the area.
> > +	See coreboot's src/include/imd.h for details.
> 
> I beleive likewise here.
> 
> Thanks,
> Mark.
> 
> > +
> > +Example:
> > +	firmware {
> > +		ranges;
> > +
> > +		coreboot {
> > +			compatible = "coreboot";
> > +			reg = <0xfdfea000 0x264>,
> > +			      <0xfdfea000 0x16000>;
> > +		}
> > +	};
> > -- 
> > 2.7.4