Hi coreboot community,
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, - Tim
On Wed, Oct 21, 2020 at 1:20 PM Tim Wawrzynczak via coreboot < coreboot@coreboot.org> wrote:
Hi coreboot community,
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.
Not all of these entities are sourced through the same mechanism (gpios vs cbi). As such any time you want to read one of those fields you'll be unconditionally needing to obtain all of those while at the time the user may only want one field. And then this can happen from multiple stages.
I would say that since these things aren't all related to answering the question someone may want to answer that we shouldn't go down this path. Width of fields, e.g., could be different across platforms since not everything is consistent.
Would like to hear any thoughts on this,
- Tim
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On Wed, Oct 21, 2020 at 1:50 PM Aaron Durbin adurbin@google.com wrote:
On Wed, Oct 21, 2020 at 1:20 PM Tim Wawrzynczak via coreboot < coreboot@coreboot.org> wrote:
Hi coreboot community,
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.
Not all of these entities are sourced through the same mechanism (gpios vs cbi). As such any time you want to read one of those fields you'll be unconditionally needing to obtain all of those while at the time the user may only want one field. And then this can happen from multiple stages.
The patch I'm proposing doesn't change whether or not the functions `board_id()`, `ram_code()`, `sku_id()` are called or not, or their definition. It will, however, currently waste 12 bytes of space if none of the weak functions are overridden. However, if a board uses all 4 IDs, then this patch saves 20 bytes of space in the table on redundant tags/sizes in the records. I guess this is the tradeoff here.
I would say that since these things aren't all related to answering the question someone may want to answer that we shouldn't go down this path. Width of fields, e.g., could be different across platforms since not everything is consistent.
I'm not sure I follow; board_id, ram_code and sku_id are all currently defined as 32 bits in coreboot/libpayload.
Would like to hear any thoughts on this,
- Tim
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
On Wed, Oct 21, 2020 at 2:37 PM Tim Wawrzynczak twawrzynczak@google.com wrote:
On Wed, Oct 21, 2020 at 1:50 PM Aaron Durbin adurbin@google.com wrote:
On Wed, Oct 21, 2020 at 1:20 PM Tim Wawrzynczak via coreboot < coreboot@coreboot.org> wrote:
Hi coreboot community,
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.
Not all of these entities are sourced through the same mechanism (gpios vs cbi). As such any time you want to read one of those fields you'll be unconditionally needing to obtain all of those while at the time the user may only want one field. And then this can happen from multiple stages.
The patch I'm proposing doesn't change whether or not the functions `board_id()`, `ram_code()`, `sku_id()` are called or not, or their definition. It will, however, currently waste 12 bytes of space if none of the weak functions are overridden. However, if a board uses all 4 IDs, then this patch saves 20 bytes of space in the table on redundant tags/sizes in the records. I guess this is the tradeoff here.
I would say that since these things aren't all related to answering the question someone may want to answer that we shouldn't go down this path. Width of fields, e.g., could be different across platforms since not everything is consistent.
I'm not sure I follow; board_id, ram_code and sku_id are all currently defined as 32 bits in coreboot/libpayload.
Sorry. I missed that this topic was pertaining to coreboot tables -- not runtime in coreboot. And I didn't read the patch. :)
I think it doesn't matter how the tables are encoded.
Would like to hear any thoughts on this,
- Tim
coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
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? Wouldn't it be better to decode the infos first and put that into tables so generic drivers can consume them?
Generally, I wouldn't assume / want board-specific drivers outside of coreboot. It seems board-specific table entries would invite people to write such.
Sorry if I completely misunderstood the intention of these entries.
Nico
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?
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.
Generally, I wouldn't assume / want board-specific drivers outside of coreboot. It seems board-specific table entries would invite people to write such
I don't think this is encouraging board-specific drivers; just trying to pass information to the payload here, that's all.
Sorry if I completely misunderstood the intention of these entries.
I just hope I have explained myself well enough for others to understand :)
Nico _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-leave@coreboot.org
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? ;)
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?
Generally, I wouldn't assume / want board-specific drivers outside of coreboot. It seems board-specific table entries would invite people to write such
I don't think this is encouraging board-specific drivers; just trying to pass information to the payload here, that's all.
Wouldn't that payload then naturally contain a board-specific driver? I mean is it just displaying or storing the information or is it acting on it?
Sorry if I completely misunderstood the intention of these entries.
I just hope I have explained myself well enough for others to understand :)
I guess I understand what you are doing. I just don't think it's a good idea.
Nico
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).
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 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.
Generally, I wouldn't assume / want board-specific drivers outside of
coreboot. It seems board-specific table entries would invite people to write such
I don't think this is encouraging board-specific drivers; just trying to pass information to the payload here, that's all.
Wouldn't that payload then naturally contain a board-specific driver? I mean is it just displaying or storing the information or is it acting on it?
The information has to live somewhere; it can live in a declarative table, or it can live in code.
Sorry if I completely misunderstood the intention of these entries.
I just hope I have explained myself well enough for others to understand
:)
I guess I understand what you are doing. I just don't think it's a good idea.
I am always open to suggestions on a better way to do something :)
Nico
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.
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:
- 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?
- 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.
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.
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:
- 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?
- 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.
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:
- 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?
- 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
Generally, I wouldn't assume / want board-specific drivers outside of coreboot. It seems board-specific table entries would invite people to write such
I don't think this is encouraging board-specific drivers; just trying to pass information to the payload here, that's all.
Wouldn't that payload then naturally contain a board-specific driver? I mean is it just displaying or storing the information or is it acting on it?
Yes, payloads contain board-specific drivers. Isn't that how it's always been? We have always had a split of responsibility between coreboot and the payload -- for example, USB, storage devices, audio devices, etc. have always only been initialized in the payload and coreboot generally doesn't know anything about them. How else is the payload supposed to do that other than hardcoding board-specific knowledge? On Arm platforms generally none of these devices are enumerable, so the payload needs to know what address to find the eMMC controller at, needs to have a driver to talk to it and needs to know the maximum speed supported by the eMMC part on that board. We do all of this by hardcoding board-specific information, and occasionally (e.g. when multi-sourcing eMMC parts on what's otherwise the same board) it is convenient to support multiple board SKUs in the same image by looking up this information in a table indexed by a strapping ID. When the device this information belongs to is wholly controlled and initialized by the payload and coreboot doesn't know anything about it, I think it makes sense for that table to also be in the payload. (Note that on Arm devices we take this even further, and those strapping IDs are also used to look up the right kernel device tree to make sure the kernel can have the right information about those strapping-dependent devices. It's board-specific information all the way down.)
We've been using this set of strapping IDs for many years, Tim's patch is just more efficiently organizing a set of existing concepts that had been slowly added piece by piece over time. I hope it shouldn't be too controversial.
Hi Julius,
wasn't sure if you are addressing me personally. I'll assume these questions are meant for me as you replied to my mail.
On 26.10.20 23:27, Julius Werner wrote:
Generally, I wouldn't assume / want board-specific drivers outside of coreboot. It seems board-specific table entries would invite people to write such
I don't think this is encouraging board-specific drivers; just trying to pass information to the payload here, that's all.
Wouldn't that payload then naturally contain a board-specific driver? I mean is it just displaying or storing the information or is it acting on it?
Yes, payloads contain board-specific drivers.
Did anybody deny that? Your "Yes" seems to imply it.
Isn't that how it's always been?
Considering Linux on a PC as the first payload, I think no.
We have always had a split of responsibility between coreboot and the payload -- for example, USB, storage devices, audio devices, etc. have always only been initialized in the payload and coreboot generally doesn't know anything about them.
Let's digest this slowly, as I really don't know anything but x86 well and don't want to make false assumptions. Do you mean initialization as something that needs to be done so the OS can operate the device (this is what Intel would call "silicon init"). Or just so that the payload can operate the device?
How else is the payload supposed to do that other than hardcoding board-specific knowledge?
More generic tables, for instance? On x86 we have ACPI, and you also mentioned kernel device trees. And I assumed so far that the intention of the coreboot tables also was something like this: to explicitly state the information a driver needs, and not implied by an id.
(I wasn't sure if this question was rhetorical. Let me know if you need a more elaborate answer.)
On Arm platforms generally none of these devices are enumerable, so the payload needs to know what address to find the eMMC controller at, needs to have a driver to talk to it and needs to know the maximum speed supported by the eMMC part on that board. We do all of this by hardcoding board-specific information, and occasionally (e.g. when multi-sourcing eMMC parts on what's otherwise the same board) it is convenient to support multiple board SKUs in the same image by looking up this information in a table indexed by a strapping ID. When the device this information belongs to is wholly controlled and initialized by the payload and coreboot doesn't know anything about it, I think it makes sense for that table to also be in the payload.
Thanks, I assumed that you do it like this (with exception of the definition of "initialization"; question above) but never looked.
(Note that on Arm devices we take this even further, and those strapping IDs are also used to look up the right kernel device tree to make sure the kernel can have the right information about those strapping-dependent devices. It's board-specific information all the way down.)
Is it the kernel that looks up the correct device tree or is this still done in the payload?
We've been using this set of strapping IDs for many years, Tim's patch is just more efficiently organizing a set of existing concepts that had been slowly added piece by piece over time. I hope it shouldn't be too controversial.
Sorry if I made the impression. I don't think it's controversial. It seems like the right step for the path taken. I didn't understand why this path was taken and Tim asked for comments, hence I thought it's a good opportunity to discuss that as well. Not to criticize or bike- shed anything, but just to have talked about it and learn from one another :)
Nico
Hi Julius,
wasn't sure if you are addressing me personally. I'll assume these questions are meant for me as you replied to my mail.
I got the impression that the core discussion in this thread was centered around whether payloads may contain board-specific drivers (or hardcode board-specific information), vs. coreboot having to gather all that information and pass it to the payload in a board-agnostic way. I just tied in to your earlier email to respond to that general question, rather than the more specific details about parsing ACPI tables or whatever that were discussed later. I just wanted to say that I think hardcoding board-specific stuff in payloads is perfectly fine and something we have been doing for a long time (I guess "always" is too long a timeframe for a project as old as coreboot, but at least depthcharge has done this for as long as it existed).
We have always had a split of responsibility between coreboot and the payload -- for example, USB, storage devices, audio devices, etc. have always only been initialized in the payload and coreboot generally doesn't know anything about them.
Let's digest this slowly, as I really don't know anything but x86 well and don't want to make false assumptions. Do you mean initialization as something that needs to be done so the OS can operate the device (this is what Intel would call "silicon init"). Or just so that the payload can operate the device?
I mean just for allowing the payload to operate the device. I agree that the "silicon init" part has traditionally all been in coreboot, but sometimes you have devices that don't require any "silicon init" (and therefore coreboot doesn't really know or care about it), but the payload may still want to use it, and therefore may need to know chipset- or board-specific details about it.
How else is the payload supposed to do that other than hardcoding board-specific knowledge?
More generic tables, for instance? On x86 we have ACPI, and you also mentioned kernel device trees. And I assumed so far that the intention of the coreboot tables also was something like this: to explicitly state the information a driver needs, and not implied by an id.
Using generic tables is fine when they exist, but they don't always and I don't think coreboot needs to invent them wherever they don't. In practice we often use these ID numbers to enable workarounds for some odd and subtle bugs on certain board revisions that we've never quite encountered in that form before -- if we were trying to design a new generic table for each of those cases all the time, we'd be adding three coreboot table entry types per platform that we bring up. These IDs are usually not the first resort for tying things together, we just like to have them available as the last.
Also, you still need to hardcode *some* board-specific information anyway. For example, you can't just bundle every audio codec driver that you support -- you'd be bloating your payload size unnecessarily. You want to only compile in the one this board is actually using, which is a form of hardcoding. If you're going that far already, what difference does it make if you hardcode a little more information (e.g. what I2C bus it's connected on) when there's no good existing mechanism to pass that information through from coreboot?
(Note that on Arm devices we take this even further, and those strapping IDs are also used to look up the right kernel device tree to make sure the kernel can have the right information about those strapping-dependent devices. It's board-specific information all the way down.)
Is it the kernel that looks up the correct device tree or is this still done in the payload?
The kernel image has a bunch of possible device trees bundled in it and the payload picks the right one from those based on an identifying string containing these IDs (e.g. "google,bob-rev2-sku1"). The payload looks it up but the device tree itself (including that identifying string) is defined in the kernel repo, so the kernel also "knows" about these IDs (at least in its source repository, not in the resulting executable).
We've been using this set of strapping IDs for many years, Tim's patch is just more efficiently organizing a set of existing concepts that had been slowly added piece by piece over time. I hope it shouldn't be too controversial.
Sorry if I made the impression. I don't think it's controversial. It seems like the right step for the path taken. I didn't understand why this path was taken and Tim asked for comments, hence I thought it's a good opportunity to discuss that as well. Not to criticize or bike- shed anything, but just to have talked about it and learn from one another :)
Okay, fair enough. Angel commented on the CL that this email thread needs to get resolved before the patch can land, so I wanted to try to help resolve it. Sounds like we're all okay with the patch moving forward then.
Hi list,
On Wed, Oct 28, 2020 at 10:17 PM Julius Werner jwerner@chromium.org wrote:
Okay, fair enough. Angel commented on the CL that this email thread needs to get resolved before the patch can land, so I wanted to try to help resolve it.
No, I never said that. I merely pointed out that discussion was taking place outside of Gerrit.
Best regards, Angel
-
Aaron Durbin -
Angel Pons -
Julius Werner -
Nico Huber -
Tim Wawrzynczak