On Thu, Oct 22, 2020 at 2:01 PM Nico Huber <nic...@gmx.de> wrote: > On 22.10.20 02:25, Tim Wawrzynczak wrote: > > On Wed, Oct 21, 2020 at 4:54 PM Nico Huber <nic...@gmx.de> wrote: > > > >> On 22.10.20 00:29, Tim Wawrzynczak wrote: > >>> On Wed, Oct 21, 2020 at 4:00 PM Nico Huber <nic...@gmx.de> wrote: > >>> > >>>> On 21.10.20 21:19, Tim Wawrzynczak via coreboot wrote: > >>>>> Currently there are 3 different "strapping" entries in the coreboot > >>>> tables, > >>>>> and with the recent addition of fw_config ( > >>>>> https://review.coreboot.org/c/coreboot/+/41209), we would also like > to > >>>> add > >>>>> the 64-bit fw_config field (updated here > >>>>> https://review.coreboot.org/c/coreboot/+/45939) to the coreboot > table > >> as > >>>>> well. > >>>>> > >>>>> In this patch (https://review.coreboot.org/c/coreboot/+/46605), I am > >>>>> proposing to deprecate the 3 current "strapping" entries (board ID, > ram > >>>>> code and SKU ID), and add them all to 1 entry, containing board ID, > ram > >>>>> code, SKU ID as well as fw_config. This saves the overhead of > parsing 4 > >>>>> different entries to obtain board configuration information. > >>>>> > >>>>> Would like to hear any thoughts on this, > >>>> > >>>> I'm actually very confused about these things and how they are > supposed > >>>> to be used. Is it correct to say that there are / would be coreboot > >>>> table entries with board-specific encodings? > >>> > >>> > >>> There already are :) Board ID, SKU ID, and RAM Code are inherently > >>> mainboard (family) or vendor specific > >>> conventions. I see FW_CONFIG as yet another one of these strapping > >> fields. > >>> > >>> > >>>> Wouldn't it be better to > >>>> decode the infos first and put that into tables so generic drivers can > >>>> consume them? > >>> > >>> > >>> That's an interesting thought, Nico. Can I assume you're talking about > >> the > >>> coreboot table here? > >> > >> Yes? Are you? ;) > >> > > > > I wasn't sure if you were implying I decode the ACPI tables :P (which I > > obviously > > can't do on ARM, although I guess they have a DT). > > Not my intention. But actually: if the information you need in > Depthcharge is available in these other tables, you should use > them by definition. The comment in `coreboot_tables.h` suggests > that it's only for information that isn't available otherwise. > I don't fully agree with it, but whoever wrote it had a point. >
Adding an ACPI parser to depthcharge is way overkill, IMHO. git blame shows that comment was from Aaron, maybe he could chime in here. > > > > > >>> > >>> What I'm trying to accomplish here is to be able to pass the FW_CONFIG > >>> value from coreboot > >>> to the payload, in my case, obviously depthcharge. You can see some of > >> our > >>> uses for fw_config > >>> in coreboot already, in mb/google/volteer for example. Some of the > fields > >>> are distinguishing which > >>> daughterboard or audio device is on a given mainboard, which in these > >> cases > >>> is not enumerable > >>> information, hence my thought to pass the fw_config value to the > payload. > >>> This alleviates needing > >>> the payload to know where this information came from, as coreboot has > >>> already done the work > >>> to figure that out. > >> > >> The question is, does depthcharge need to know the daughterboard id? or > >> does it actually need to know something that is implied by that id and > >> could be written into the table explicitly? > >> > > > > Sure, the ID contains implicit information. It would be possible to add a > > whole bunch of > > new entry types into the coreboot table, but which way to take that? > > Let's look at volteer as a strawman; I want to to able to tell > depthcharge > > which audio chip & interface (I2S vs. SNDW) it has. I could go two > > different ways with that: > > 1) Create an audio device table, which just contains board- or vendor- > > specific IDs in it, and doesn't really shift the issue away from > > board-specific encodings > > I don't see why such a table would have to be board specific. Even if > these chip models don't have a unique id, couldn't we enumerate them > globally in coreboot? > > > 2) reinvent ACPI / DT and encode the device structure & properties in a > > binary structure > > or even add ACPI / DT parsing support to the payload, but that seems like > > overkill. > > It depends much on the scale (which I don't see yet). And also, if > you want to write code that supports individual devices or code that > supports the ecosystem. > I have thought some more about this, and I think this is an approach that is worth exploring some more... thanks for chatting about this, Nico :) I will think about this some more and bring back some thoughts later... > I guess I'm not familiar enough with Depthcharge to make any good > argument here. > > Nico > > PS. Just remembered the patch to split `static.h` up. So I guess > Depthcharge is / will be compiled per-board? That seems rather > special for a payload. > Agreed, this is a problem that has been low-priority for us, but I think the time may be coming due for something there soon. IMHO, our proliferation of build targets for depthcharge has gotten too high for all it's doing.
_______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
