Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19668 )
Change subject: soc/intel/common/block: Add Intel common systemagent support ......................................................................
Patch Set 5:
(12 comments)
https://review.coreboot.org/#/c/19668/5/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
PS5, Line 77: int What are the expected return values? I believe you are only checking for 0/non-zero?
PS5, Line 74: struct sa_mmio_descriptor { : unsigned int index; : size_t size; : int (*get_resource)(device_t dev, unsigned int index, : uintptr_t *base, size_t *size); : const char *description; : }; The other mmio_descriptor above is named _set_. Should this be _get_. Is there a way the two can be combined into just one?
PS5, Line 83: int Why not unsigned?
PS5, Line 85: int bool?
PS5, Line 86: int bool?
PS5, Line 124: int uintptr_t?
Can you please add comments as to what these functions are expected to return?
https://review.coreboot.org/#/c/19668/5/src/soc/intel/common/block/systemage... File src/soc/intel/common/block/systemagent/systemagent.c:
PS5, Line 84: if(*len) { How would this ever evaluate to false? You are assigning *len in the switch block above even in default case.
PS5, Line 93: len
i guess this is a miss, we don't need to get the len, purpose for this API
It's confusing. The above function is also named as _get_*_base_addr but sets len, but this function does not.
PS5, Line 197: value |= ~mask; value &= ~mask;
PS5, Line 251: sa_read_map_entries(dev, sa_map_entry, &mc_values[0]); : sa_report_map_entries(dev, sa_map_entry, &mc_values[0]); Why not report it while you are actually reading it?
PS5, Line 397: int index; : : index = sa_add_fixed_mmio_resources(dev, sa_fixed_resources, count); : : return index; return sa_add_fixed_mmio_resources(dev, sa_fixed_resources, count);
PS5, Line 407: int resource_index; : : resource_index = sa_add_dram_resources(dev, index, sa_map_entry); : : return resource_index; return sa_add_dram_resources(dev, index, sa_map_entry);