On Thu, Oct 22, 2020 at 4:01 PM Tim Wawrzynczak via coreboot <coreboot@coreboot.org> wrote:


On Thu, Oct 22, 2020 at 2:01 PM Nico Huber <nico.h@gmx.de> wrote:
On 22.10.20 02:25, Tim Wawrzynczak wrote:
> On Wed, Oct 21, 2020 at 4:54 PM Nico Huber <nico.h@gmx.de> wrote:
>
>> On 22.10.20 00:29, Tim Wawrzynczak wrote:
>>> On Wed, Oct 21, 2020 at 4:00 PM Nico Huber <nico.h@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.

I moved the file. Those comments were sitting in there already as far as I remember: https://review.coreboot.org/c/coreboot/+/11592/

The specific things Tim was referring to are not in other tables, but they can be obtained from other sources. Real question is if we want to duplicate the act of requerying all the information and duplicating the code to do that. I would say that's not really great. The whole coreboot/libpayload split rears its head again in forcing duplicative work and code. That's a broader topic in general. 

 
>
>
>>>
>>> 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-leave@coreboot.org