Xixi Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44699 )
Change subject: soc/mediatek/mt8192: Add dram control register define and bits define ......................................................................
Patch Set 7:
(6 comments)
https://review.coreboot.org/c/coreboot/+/44699/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44699/6//COMMIT_MSG@8 PS6, Line 8:
Can more common code be used?
On architecure, mt8192 is different from mt8183 or mt8173, so no common code can use now.
https://review.coreboot.org/c/coreboot/+/44699/6//COMMIT_MSG@8 PS6, Line 8:
Taking (copied(?)) from where?
mt8192 is a new design, is not copied from mt8183 or mt8173.
https://review.coreboot.org/c/coreboot/+/44699/6/src/soc/mediatek/mt8192/inc... File src/soc/mediatek/mt8192/include/soc/dramc_pi_api.h:
https://review.coreboot.org/c/coreboot/+/44699/6/src/soc/mediatek/mt8192/inc... PS6, Line 128: DQS_8PH_DEGREE_45,
Sort it?
Ack
https://review.coreboot.org/c/coreboot/+/44699/6/src/soc/mediatek/mt8192/inc... PS6, Line 157: u8 chn;
Why not unsigned int? The architecture native type has to be used anyway?
The size of structure member should be 1 or 2 or 4 bytes. Eg: mode register as above "struct mr_values".
Another way, u8 is enough form channel info.
For portable, if unsigned int, maybe we can't assure the size by different compiler.
https://review.coreboot.org/c/coreboot/+/44699/6/src/soc/mediatek/mt8192/inc... PS6, Line 158: u8 rank;
Ditto?
As above.
https://review.coreboot.org/c/coreboot/+/44699/6/src/soc/mediatek/mt8192/inc... PS6, Line 159: u8 fsp;
Please add a comment, what *fsp* is.
Ack