Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34545 )
Change subject: mediatek/mt8183: Init SPM driver ......................................................................
Patch Set 54:
(17 comments)
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/in... File src/soc/mediatek/mt8183/include/soc/spm.h:
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/in... PS53, Line 574: const char *version; /* PCM code version */
Rather than doing complicated offsetof calculations in your code, please just define a sub-structure […]
"version" and "base" removed.
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/in... PS53, Line 576: const
Your code is writing into these, you shouldn't call them const.
Done
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/in... PS53, Line 577: sess These are also not used.
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/in... PS53, Line 590: dyna_load_pcm_t
nit: we generally don't end struct names in a _t
Renamed
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/in... PS53, Line 600: check_member(mtk_spm_regs, poweron_config_set, 0x0000);
nit: there's really no need to check the first member
Done
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/in... PS53, Line 601: check_member(mtk_spm_regs, spm_ack_chk_latch4, 0x0974);
nit: put right under the struct
Done
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... File src/soc/mediatek/mt8183/spm.c:
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 27: struct dyna_load_pcm_t dyna_load_pcm[DYNA_LOAD_PCM_MAX];
I don't understand the purpose of this struct. […]
Done
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 229: [DYNA_LOAD_PCM_MAX] = "pcm_path_max",
This doesn't seem to be used?
Removed
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 250: dyna_load_pcm_path[index]);
nit: just use file_name and it might fit on the same line? (Also, extra exclamation marks don't real […]
Done
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 255: offset = 0;
I'm really having a lot of trouble following the data layout you're parsing here. […]
Done
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 259: printk(BIOS_INFO, "spm: copy_size = %d\n", copy_size);
nit: This always prints "2", do you really need this (especially at INFO)?
Removed
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 263: offset += copy_size;
You should check that you're not overrunning the amount of data you loaded (fw_size) with all of the […]
Done
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 268: dyna_load_pcm[index].buf);
nit: This is always going to print the same address, do you really need that in production code?
@dawei Please respond to this.
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 270: /* start of pcm_desc */
I assume there's some external reason why this data format needs to be like this (firmware binary fi […]
@dawei Any comment on this?
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 281: pdesc->base = (u32 *)dyna_load_pcm[index].buf;
I am super confused why you have this complicated structure layout, where version and data pointer a […]
Done
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 283: dyna_load_pcm[index].ready = 1;
Does this do anything useful? If you want the caller to know whether the function succeeded (which s […]
Done
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 291: printk(BIOS_INFO, "%s done after %ld msecs.\n", __func__,
nit: would consider consolidating all this output a little (e.g. […]
@dawei Please address this.