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.
>
>
>>>
>>> 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 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.
_______________________________________________
coreboot mailing list -- coreboot@coreboot.org
To unsubscribe send an email to coreboot-le...@coreboot.org
