Attention is currently required from: Hung-Te Lin.
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68871 )
Change subject: mem_chip_info: Update to new format ......................................................................
Patch Set 4:
(5 comments)
Patchset:
PS4:
BTW, is it possible to have speed (frequency) in future?
Uff... I mean it would but I was really hoping I could finalize this now...
We were planning to only use this for part identification, so frequency isn't really needed for that. I guess it seems like a reasonable enough thing to add just as a precaution, if we still have time for that. I'm not really sure what the appropriate layer would be to model frequency here... I guess at the channel level? Can different channels theoretically run different frequencies? Or should it just be a toplevel value? (And for the data type I guess a `uint32_t frequency_mhz` -- or an enum? Is there going to be confusion between 1866 and 1867 depending on how people round? What should systems like Kevin put in where the frequency is technically 928MHz for SI reasons but the "common" frequency slot in that vicinity is 933?)
Also, of course, this would just be boot frequency and most devices have some form of dynamic frequency scaling these days (which also implies that the kernel must obviously know the possible frequency values through other means already anyway). Is that still a valuable thing to report here, then? I'm not even sure if it's guaranteed that this frequency is always the highest possible one.
File src/commonlib/bsd/include/commonlib/bsd/mem_chip_info.h:
https://review.coreboot.org/c/coreboot/+/68871/comment/d4b1a722_c171513c PS3, Line 72: struct_version
since it always takes them forever to make a new update […]
Well, changing this structure is also cumbersome for us independent of Qualcomm. The payload interface is supposed to be stable, so if we ever want to make changes here again (at least after we really start using it) we would have to have structs describing both layouts and code in depthcharge parsing both layouts etc... I hope we can avoid all that.
File src/soc/mediatek/common/memory.c:
https://review.coreboot.org/c/coreboot/+/68871/comment/1fc739cd_57e227f2 PS3, Line 129: ddr->mrr_info.mr8_density[r]
@yuping: right. […]
Ack
https://review.coreboot.org/c/coreboot/+/68871/comment/ca63d6fc_40c919f3 PS3, Line 150: size = sizeof(*mc) + : sizeof(struct mem_chip_entry) * CHANNEL_MAX * curr_ddr_info->mrr_info.rank_nums;
Is it possible to refactor mem_chip_info_size() to take a 'entries' parameter so both MTK and QC can […]
Done
https://review.coreboot.org/c/coreboot/+/68871/comment/3464a158_60e30c67 PS3, Line 155: fill_dram_info
can we do memset(mc, 0, size) first so there's no need to do reserve[0] = 0, reserve[1] = 0 in the f […]
Done
-
Julius Werner (Code Review)