Taniya Das has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35496 )
Change subject: sc7180: Add clock driver ......................................................................
Patch Set 12:
(13 comments)
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/boo... File src/soc/qualcomm/sc7180/bootblock.c:
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/boo... PS9, Line 19: #include <soc/clock.h>
Twice?
Done
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... File src/soc/qualcomm/sc7180/clock.c:
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 34: 19200 * KHz
This can be SRC_XO_HZ after you define it (see other file). Maybe put /* 19. […]
Done
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 42: 19200 * KHz
This too.
Done
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 57: 300 * MHz
This can be GPLL0_EVEN_HZ.
Done
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 65: 0x00004805
This magic number needs to be split up into its individual fields and written with named constants i […]
Thanks Julius. I understand that we should use the individual bits. Would take a look at the new implementation.
But would it be okay to go about with the magic number for now?
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 89: static int clock_configure_rcg(struct sc7180_rcg *clk,
Why is this here? We didn't have this in the SDM845 code, and the registers are the same. […]
Done
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 176: clrbits_le32(&aoss->aoss_cc_apcs_misc, BIT(AOP_RESET_SHFT));
This is the code that's needed without the "bring up AOP in QC-SEC to lock down some stuff" hack tha […]
Yes, I am checking internally and would update.
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 202: int s = qup % QUP_WRAP1_S0;
Ohh... yeah... that was a bug in the SDM845 code. Good catch.
Ack
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 204: (struct sc7180_qupv3_clock *)
Cast should be unnecessary?
Done
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/inc... File src/soc/qualcomm/sc7180/include/soc/clock.h:
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/inc... PS9, Line 34:
nit: please align tabs
Done
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/inc... PS9, Line 35: #define QUP_WRAP_CORE_2X_19_2MHZ (19200 * KHz)
I think it makes more sense to call this SRC_XO_HZ, because it's really the frequency of the source […]
Done
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/inc... PS9, Line 49: struct sc7180_clock {
Hmmm... all of this doesn't really fit great (and didn't on SDM845 either). […]
Done
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/inc... PS9, Line 85: u32 qup_wrap0_core_2x_cbcr;
...with the above, you can just model this as an sc7180_clock.
Done