Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34545 )
Change subject: mediatek/mt8183: Init SPM driver ......................................................................
Patch Set 53:
(19 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 for the parts of this that are actually in the binary format, and nest the two.
edit: or rather, get rid of these first two members entirely because you can get the same thing from struct dyna_load_pcm_t (see other comment).
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.
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
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/in... PS53, Line 591: char path[128]; Why is this here? You don't seem to be using it.
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
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
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. You're only loading a single PCM blob per boot, you'll only ever fill out one of these elements. There's no reason why this needs to be an array *or* a global, you should be able to get by with just a single, stack-local dyna_load_pcm_t in spm_init().
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?
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 really add anything...)
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. Can you please add a big comment somewhere explaining the layout, e.g.:
/* * u16 firmware_size; -- counts in u32s * u8 firmware_code[firmware_size * 4]; * struct pcm_desc descriptor; * char version[]; */
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)?
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 260: printk(BIOS_INFO, "spm: firmware_size = %d\n", firmware_size); nit: Isn't this just the size of the PCM descriptor? Also, I think it would be clearer to print it in bytes (not dwords).
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 these (especially the snprintf() at the end).
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?
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 first, then descriptor)? Because it would be much easier to parse if you packaged it the other way round...
https://review.coreboot.org/c/coreboot/+/34545/53/src/soc/mediatek/mt8183/sp... PS53, Line 277: snprintf(dyna_load_pcm[index].version, Something needs to make sure the input string is terminated here. If you don't store the length of this in the format I guess the best you can do is
spm_bin[fw_size - 1] = '\0';
Also, is there any reason you're even copying this out? I don't really see it. If all you want to do is print it, I would just print it here and be done with it (or at least only pass a pointer to it around).
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 are stored in dyna_load_pcm_t, and dyna_load_pcm_t contains the struct pcm_desc, but the struct pcm_desc also has a pointer back to the other two things. Why do you need that?
It seems to me that you shouldn't need to store version and base in struct pcm_desc at all. You could just pass the struct dyna_load_pcm_t around to all the functions that currently take a struct pcm_desc, and then you have the version and data pointer in there already.
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 seems to be the only thing this does), just give it a return value.
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. you don't really need to print the function name and line), or move some of it to lower notification level. I'd say as a rule of thumb that the whole spm_init() should not print more than one line at INFO when there are no errors.