| Message ID | 1490303069-13230-5-git-send-email-thierry.escande@collabora.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, Mar 23, 2017 at 10:04:28PM +0100, Thierry Escande wrote: > From: Julius Werner <jwerner@chromium.org> > > 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@chromium.org> > Signed-off-by: Thierry Escande <thierry.escande@collabora.com> > --- > .../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. > + - 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. > + 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 > > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@chromium.org> > > > > 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@chromium.org> > > Signed-off-by: Thierry Escande <thierry.escande@collabora.com> > > --- > > .../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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
...and again in plaintext, sorry about that. On Fri, Mar 24, 2017 at 12:32 PM, Julius Werner <jwerner@chromium.org> wrote: >> > 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"? > > > No. This binding is for the coreboot project in general and has nothing to > do with Google. coreboot is both the vendor and the product, so I think > "coreboot" would look better than "coreboot,coreboot". And yes, it is > supposed to represent every aspect of coreboot (right now the "coreboot > tables" are already sort of a catch-all data structure used by coreboot to > pass on any sort of info it wants later stages to know... and if we ever > have any additional things we'd like to pass on, we'd probably want to add > them to this binding as well). > >> >> > > + - 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. > > > See the old thread Brian linked for some arguments for and against this. I > think particularly because we want this node to represent every aspect of > coreboot (which I think makes more sense than spreading stuff all over the > place), treating it as reserved memory would not work well if we might later > add other fields. > > Also, since we didn't get any more responses the last time we tried to > upstream this and had schedules to keep, we had to go ahead with what we > had. So right now there are already millions of devices shipped with this > binding in firmware, and the only question we still have left to decide is > whether Linux wants to support them upstream or not. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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" + - 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. + 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. + +Example: + firmware { + ranges; + + coreboot { + compatible = "coreboot"; + reg = <0xfdfea000 0x264>, + <0xfdfea000 0x16000>; + } + };