Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35496 )
Change subject: sc7180: Add clock driver ......................................................................
Patch Set 9:
(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?
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.2KHz */ as a comment next to it.
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 42: 19200 * KHz This too.
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 57: 300 * MHz This can be GPLL0_EVEN_HZ.
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 instead.
Note that we just introduced a fancy new API to make accessing registers with bit fields easier and safer... see a demonstration from another SoC in CB:35471. With that, rather than doing
#define SOMEBIT_SHIFT 5 #define SOMEFIELD_SHIFT 7 #define SOMEFIELD_MASK 0xf
clrsetbits_le32(®, 1 << SOMEBIT_SHIFT | SOMEFIELD_MASK << SOMEFIELD_SHIFT, 1 << SOMEBIT_SHIFT | 2 << SOMEFIELD_SHIFT);
you can just do
DECLARE_BIT(SOMEBIT, 5) DECLARE_FIELD(SOMEFIELD, 10, 7)
SET32_BITFIELDS(®, SOMEBIT, 1, SOMEFIELD, 2);
We're still testing this out so it's not a requirement yet, but feel free to use it if you want to. I think it makes drivers that access a lot of registers with bitfields much easier to follow. (For consistency you should try to only use one paradigm per file though... so if you introduce it here you should also use it for other registers with multiple fields like gpll.user_ctl or rcg.cfg.)
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.
Just use clock_configure() instead... the convention is that if the clk_cfg does not have an 'm' value, it's a pure RCG clock (without MND registers), and it should work fine for this case.
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 that we had in SDM845. Is that hack no longer needed on this SoC? Please explain why.
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.
https://review.coreboot.org/c/coreboot/+/35496/9/src/soc/qualcomm/sc7180/clo... PS9, Line 204: (struct sc7180_qupv3_clock *) Cast should be unnecessary?
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
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 oscillator (regardless of whether you use it for the QUPs or something else).
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). Let's remodel this a bit. I'd suggest you get rid of sc7180_rcg (because there's never an RCG without CBCR), you change this to just
struct sc7180_clock { u32 cbcr; u32 rcg_cmd; u32 rcg_cfg; }
and then you add another
struct sc7180_clock_mnd { struct sc7180_clock clock; u32 m; u32 n; u32 d_2; }
configure_clock() would take a struct sc7180_clock, but cast it to sc7180_clock_mnd if the 'm' field in clk_cfg is non-zero.
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.