[coreboot-gerrit] Change in coreboot[master]: soc/intel/common/block: Add Intel common systemagent support

Fri May 26 11:46:23 CEST 2017

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/systemagent.h
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/intelblocks/systemagent.h
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/systemagent/systemagent.c
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/systemagent/systemagent_early.c
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

-- 
To view, visit https://review.coreboot.org/19668
To unsubscribe, visit https://review.coreboot.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I969ff187e3d4199864cb2e9c9a13f4d04158e27c
Gerrit-PatchSet: 4
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: V Sowmya <v.sowmya at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Balaji Manigandan <balaji.manigandan at intel.com>
Gerrit-Reviewer: Barnali Sarkar <barnali.sarkar at intel.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-Reviewer: dhaval v sharma <dhaval.v.sharma at intel.com>
Gerrit-HasComments: Yes