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 5:
(24 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:
Line 62: size_t count);
Comment your exposed functions. Really his entire file needs better comment
Done
Line 67: #if ENV_RAMSTAGE
I really hate guarding things like this, and I've yet to see a good reason
Done
Line 70: int soc_add_fixed_mmio_resources(device_t dev);
Don't use device_t here. use struct device *.
Done
Line 76: size_t size;
What's the significance of this filed? Like functions, these exposed struct
Done
PS5, Line 85: int
bool?
Done
PS5, Line 94: BAR
at REG in pci space? Otherwise it's purely a value found at reg?
Done
https://review.coreboot.org/#/c/19668/5/src/soc/intel/common/block/systemage... File src/soc/intel/common/block/systemagent/systemagent.c:
Line 43: return 0;
we should use index as return
Done
Line 46: __attribute__((weak)) void soc_add_imr_resources(device_t dev, int index)
got it, you want to unify behavior, i had focused on function caling order.
Done
Line 55: #define DPR_SIZE_MASK 0xff0
Yes, I know. It was more about how this doesn't apply to all chipsets. A co
Done
Line 57: int sa_get_pcie_base_addr(device_t dev, unsigned int index, uintptr_t *base,
But you don't have to.
Done
PS5, Line 84: if(
space between if and (
Done
PS5, Line 93: len
i guess this is a miss, we don't need to get the len, purpose for this API
Done
PS5, Line 115: *len)
But we know base and length for nearly all the entries in such a table. We
Done
Line 156: IORESOURCE_ASSIGNED;
mmio_resource()
Done
Line 166: static void read_map_entry(device_t dev, const struct sa_map_entry *entry,
But why complicate this further? Why not provide an array of
Done
Line 306: IORESOURCE_ASSIGNED;
Why isn't reserved_ram_resource() and ram_resource() being used?
Done
PS5, Line 310: SA_TSEG_REG
That would be helpful documentation in the header file about the surface ar
Done
PS5, Line 337: (0xa0000 / KiB)
parens not needed
Done
Line 338: (0xc0000 - 0xa0000) / KiB);
same line
Done
PS5, Line 339: (0xc0000 / KiB),
parens not needed
Done
PS5, Line 365: const struct sa_mmio_descriptor *sa_imr_resources
All that was used out of this structure is a index and an optional descript
Done
Line 384: resource->base = (base & 0x0fffffff) * KiB;
Why isn't reserved_ram_resource being used here? Is this so one can spew ou
Done
Line 422: int index = 0;
No reason to set this to 0. It's assigned below.
Done
https://review.coreboot.org/#/c/19668/5/src/soc/intel/common/block/systemage... File src/soc/intel/common/block/systemagent/systemagent_early.c:
Line 111: (unsigned long)base);
So we're going to see that these messages spewed out on every boot? What is
i have now guarded against SA_DEBUG, to avoid every boot more debug log. now its more over user debug interest.
-
Subrata Banik (Code Review)