Change in coreboot[master]: soc/intel/common/block: Add Intel common systemagent support
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/19668 )
Change subject: soc/intel/common/block: Add Intel common systemagent support ......................................................................
Patch Set 4:
(42 comments)
https://review.coreboot.org/#/c/19668/4//COMMIT_MSG Commit Message:
PS4, Line 10: stemagent
systemagent ?
sorry my bad
https://review.coreboot.org/#/c/19668/4/src/soc/intel/apollolake/include/soc... File src/soc/intel/apollolake/include/soc/systemagent.h:
Line 47: #define SOC_SA_IMR_RESOURCES 20
This seems odd.
#define SOC_SA_IMR_RESOURCES 20 i have removed this as we are getting pointer to array so, size we are getting default.
https://review.coreboot.org/#/c/19668/4/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/systemagent.h:
PS4, Line 54: u32
uintptr_t
Done
PS4, Line 55: u32
uintptr_t
Done
PS4, Line 61: int
size_t
Done
PS4, Line 62: u32
uintptr_t and pretty much every where else.
Done
Line 64: void enable_bios_reset_cpl(void);
A lot of these function names are too generic please if that.
i thought those function names provide clear understanding about job description, any way, i'm bad at giving name, if you have some recommendation, please let me know.
PS4, Line 66: #if ENV_RAMSTAGE
why are we guarding declarations?
we can remove those guarding, but i thought of not including a code for a stage which not going to be in use for now from that stage. Move over this is the common header for all SA common code. Including the same for bootblock stage throwing error for device structure
Line 87: const char *description;
You are going to need to add some documentation for this.
i'm not sure, if we can add README kind of thing inside CB code. if so, i would rather prefer to mention for each IP to have the same README file. For an example: we would like to explain how this structure even work. i will add some comment here
PS4, Line 92:
tabs instead of spaces? Nothing lines up
Done
https://review.coreboot.org/#/c/19668/4/src/soc/intel/common/block/systemage... File src/soc/intel/common/block/systemagent/systemagent.c:
PS4, Line 27: #include <vendorcode/google/chromeos/chromeos.h>
remove
Done
PS4, Line 30: __attribute__((weak));
This is only to make things link? If the SoCs are selecting this common blo
yes, those are meant for linking between common code block and soc part.
Now we don;t need this as we are calling soc to get array pointer
PS4, Line 64: 1024 * 1024
MiB and all the other places. Plus this base calculation is not very good.
Done
PS4, Line 81: u32
uintptr_t
Done
PS4, Line 81: get_base_addr
These functions are named poorly for being global. They should be name spac
i have added "sa_" before all SA functions similar to fast_spi
PS4, Line 81: u32
size_t
Done
PS4, Line 127: ARRAY_SIZE(sa_fixed_resources)
You should rely on the soc returning a pointer to the array and filling in
Done
PS4, Line 161: mask = ((1ULL<<20)-1); : mask = ~mask;
mask = ~(1ULL * MiB - 1);
Done
PS4, Line 166: !(entry->offset < 0)
= 0
Done
Line 167: base_addr = pci_read_config32(dev, entry->reg) & ~1;
Why isn't this using the get_base_addr() function?
Done
PS4, Line 178: (uint64_t)
no cast needed
Done
PS4, Line 180: (uint64_t)
no cast needed
Done
PS4, Line 207: #if IS_ENABLED(CONFIG_SA_ENABLE_DPR)
No #if guards.
Done
PS4, Line 241: #if IS_ENABLED(CONFIG_SA_ENABLE_DPR)
if (IS_ENABLED(...))
Done
PS4, Line 245: uint32_t
uintptr_t
Done
Line 313: base_k = 4096 * 1024; /* 4GiB */
4 * (GiB / KiB)
Done
PS4, Line 314: >> 10
/ KiB
Done
PS4, Line 328: 0x100000
(1*MiB - 0xc0000) / KiB
Done
PS4, Line 330: chromeos_reserve_ram_oops(dev, index++);
remove
Done
PS4, Line 335: #if IS_ENABLED(CONFIG_SA_ENABLE_IMR)
Don't guard code with #if
Done
PS4, Line 341: uint32_t
size_t
Done
PS4, Line 350: ((~mask & 0x0fffffff) + 1)
outside parens not needed
Done
PS4, Line 370: sa_imr_resources[i].index
Isn't this already 'offset' ?
Done
PS4, Line 375: << 10
- KiB
Done
PS4, Line 376: << 10
- KiB
Done
PS4, Line 399: : #if IS_ENABLED(CONFIG_SA_ENABLE_IMR)
if (IS_ENABLED(...))
Done
https://review.coreboot.org/#/c/19668/4/src/soc/intel/common/block/systemage... File src/soc/intel/common/block/systemagent/systemagent_early.c:
PS4, Line 28: !ENV_RAMSTAGE
Why does this need to be guarded?
i had an opinion not to include any code which not getting used in that specific stage hence added guard. but more over, why i need this file even in ramstage because this file doing early SA init. its because of enable_bios_reset_cpl function getting called between different stage in SKL/KBL vs APL. hence i need to add complete file for ramstage. Also pci_io_write_config is bot available at ranstage, hence i need to limit with ENV_BOOTBLOCK token. if you think other wise, i can only guard bootblock_systemagent_early_init and open rest.
PS4, Line 68: u32
uintptr_t
Done
PS4, Line 68: set_base_addr
overly generic name
can we make like sa_set_base_address() / hb_set_base_address() in that way it will sound more for SA or host bridge?
PS4, Line 78: u32
uintptr_t
Done
PS4, Line 97: int
size_t
Done
PS4, Line 117: int
size_t
Done
-
Subrata Banik (Code Review)