Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35496 )
Change subject: sc7180: Add clock driver ......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/c/coreboot/+/35496/15/src/soc/qualcomm/sc7180/bo... File src/soc/qualcomm/sc7180/bootblock.c:
https://review.coreboot.org/c/coreboot/+/35496/15/src/soc/qualcomm/sc7180/bo... PS15, Line 19: #include <soc/clock.h> Still twice
https://review.coreboot.org/c/coreboot/+/35496/15/src/soc/qualcomm/sc7180/cl... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/35496/15/src/soc/qualcomm/sc7180/cl... PS15, Line 82: write32(&((struct sc7180_clock_mnd *)clk)->m, m & Please do something like
struct sc7180_clock_mnd *mnd = (struct sc7180_clock_mnd *)clk;
at the top of the function and then just use that.
https://review.coreboot.org/c/coreboot/+/35496/15/src/soc/qualcomm/sc7180/cl... PS15, Line 183: &gcc->qup_wrap1_s[s]; nit: maybe fit both sides of the colon on one line now?
https://review.coreboot.org/c/coreboot/+/35496/15/src/soc/qualcomm/sc7180/cl... PS15, Line 185: clock_configure((struct sc7180_clock *)&qup_clk->clk, qup_cfg, hz, Please pass &qup_clk->clk.clock rather than casting (maybe we should fix those names a bit to look less weird next to each other).
https://review.coreboot.org/c/coreboot/+/35496/15/src/soc/qualcomm/sc7180/in... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/35496/15/src/soc/qualcomm/sc7180/in... PS15, Line 58: struct sc7180_clock_mnd clk; nit: Maybe call this 'mnd' or 'mnd_clk' to be easier to tell it apart from the struct sc7180_clock it contains.
https://review.coreboot.org/c/coreboot/+/35496/15/src/soc/qualcomm/sc7180/in... PS15, Line 110: }; nit: it would be good to add some offset checks for this structure so we can be certain we don't mess up the ranges when we make changes like this. something like
check_member(sc7180_gcc, qup_wrap0_bcr, 0x17000); check_member(sc7180_gcc, qup_wrap1_bcr, 0x18000); check_member(sc7180_gcc, usb3_phy_prim_bcr, 0x50000);