On 02.02.2009 19:02, Rudolf Marek wrote:
Hi,
that's an impressive piece of code!
Heh nice.
The following review is rather short and to-the-point. No offense intended.
Well yes as always ;)
A few comments about the general design: The global variable gencurrent is a bit irritating. How do you handle running on multiple cores?
The idea is that the create_acpi_tables always run on BSP. FOr the powernow I admin we will need to run get/set MSR on each CPU. I have in plan to move the powernow from the AMD board to more general place. For multi cores, they have same MSR so we dont need to solve that.
OK.
Same for len_stack. Besides that, both variables are in the global namespace and could use some acpi_ prefix or suffix.
Well I was quite hesitating if to use a global variable or introduce some pointer parameter to every function. Then I choose to have the global (and static) var for this. The reason is that the pointer propagation would be needed through all functions. Also I was not considering the multi thread support, because ACPI tables are always build by BSP. All pointers and stack is encapsulated, therefore not visible from outside. It should work fine.
Maybe add a comment that all these functions should not run on APs?
I think weak symbols are something that makes code harder to follow.
Yes, and I have read all mails until now. I decided to put here empty dummy routine with weak symbol. This routine will be hidden if someone creates "strong" version of same function in some other object file. The reason for this is to avoid having #ifdefs and dummy empty functions for all boards which don't need this.
The second possibility is to have just exactly one weak function and test its presence with if (name_fn) { name_fn)() } The linux kernel uses first case and I decided that this nicer, because if someone accidentally manages to create another weak function, it depends on luck which one will be called. The used scheme above assures that either the weak fn will be called or the strong one.
Hm. What about having boards #define HAVE_ACPI_SOME_FUNCTION
The { belongs on another line.
Hmm I already commit that, maybe I can fix it as trivial commit.
Yes please.
- return current;
+}
+void acpi_create_ssdt_generator(acpi_header_t *ssdt, char *oem_table_id)
Strange function name.
I have in plan to introduce acpi_create_ssdt which will link up the ssdt.aml code to this. This is the reason for this strange name.
OK.
+{
- unsigned long current=(unsigned long)ssdt+sizeof(acpi_header_t);
- memset((void *)ssdt, 0, sizeof(acpi_header_t));
- memcpy(&ssdt->signature, SSDT_NAME, 4);
Maybe strncpy instead?
Here it is still OK. because it is static. I think it would be better for oem_table_name.
"how many nesting levels do we support"
Hmm true maybe trival fix too.
Yes.
+#define ACPIGEN_LENSTACK_SIZE 10
+/* if you need to change this, change the acpigen_write_f and
- acpigen_patch_len */
Can you remove the empty line above?
I like them, you dont?
It is not clear if the comment belongs to the #define if there is an empty line in between.
+#define ACPIGEN_MAXLEN 0xfff
+#include <string.h> +#include <arch/acpigen.h> +#include <console/console.h>
+static char *gencurrent;
+char *len_stack[ACPIGEN_LENSTACK_SIZE]; +int ltop = 0;
These global variables need better names and some comments explaining what they do.
Whoops they should be static. Sorry about that. For this reason they have just names like this, because they should be static and not visible. I will fix them.
Grepping (and cscope) is easier if they have that prefix. It also avoids collisions if we ever change to unit-at-a-time compilation (like in v3).
+static int acpigen_write_len_f()
Needs a doxygen comment.
Yes in Alps I did not have doxygen installed. I will try to write some patch addressing the issues and also document this a bit. Also some words to ACPI wiki would be good...
Yes please. You know a lot about ACPI.
+{
- ASSERT(ltop < (ACPIGEN_LENSTACK_SIZE - 1))
- len_stack[ltop++] = gencurrent;
- acpigen_emit_byte(0);
- acpigen_emit_byte(0);
- return 2;
+}
+void acpigen_patch_len(int len)
Needs a doxygen comment.
+{
- ASSERT(len <= ACPIGEN_MAXLEN)
- ASSERT(ltop > 0)
- char *p = len_stack[--ltop];
- /* generate store length for 0xfff max */
- p[0] = (0x40 | (len & 0xf));
- p[1] = (len >> 4 & 0xff);
+}
+void acpigen_set_current(char *curr) {
Needs a doxygen comment.
- gencurrent = curr;
+}
+char *acpigen_get_current(void) {
Needs a doxygen comment.
Yes its true.
unsigned long acpi_fill_srat(unsigned long current); +unsigned long acpi_fill_ssdt_generator(unsigned long current, char *oem_table_id); +void acpi_create_ssdt_generator(acpi_header_t *ssdt, char *oem_table_id);
The two functions above have rather similar names and it's not immedately ofvious how they differ.
It follows the scheme we have right now. Create creates, fill is callback.
I didn't know that. OK.
acpigen_get_current is a really confusingly named function.
Hmm any idea of better name? it gets "current" which is used in all acpi code as current data pointer.
Is it really the current data pointer? The code seems to treat it as end-of-data pointer, not current data pointer.
"COREBOOT" please. That way, people can actually find out where the table came from. The fact that the data are generated dynamically is already visible from the compiler ID.
Here is small catch. The ACPI specs says that the OEM_TABLE_ID should have unique name, please note that the OEM_ID is set to coreboot.
Hm yes. Which OEM table ID is used by factory BIOS?
- for(i=sysconf.hc_possible_num; i<HC_POSSIBLE_NUM; i++) { // in
case we set array size to other than 8
Comment on a separate line, please.
Aha this is cut and paste from old code.
-> maybe fix it in the trivial patch with the other whitespace etc?
lenp += acpigen_write_dword(0x0);- }
- acpigen_patch_len(lenp - 1);
acpigen_patch_len is a name which does not really reveal function purpose.
Give me better one please, what it does its a patching of size when the size of the object is known.
Ah. I thought is is the length of the patch. Maybe acpigen_modify_len or acpigen_adjust_len (preferred)?
+int k8acpi_write_vars(void) +{
- int lens;
Maybe use len instead of lens?
Lens because its a Scope. lenp is for Package.
Ah. I thought it was the plural of len. Maybe use scopelen and packlen?
I know the original code did not have it, but can you please add TOM2?
Yes I was also thinking about this one. What is cool now, that we can easily add new stuff without worring to break the binary offset dependent patching ;)
Yes absolutely. That's what I thought.
I will try to address most of the issues also depending on your answers,
Thanks!
Regards, Carl-Daniel