Attention is currently required from: Shelley Chen, Ravi kumar, Paul Menzel, mturney mturney, Julius Werner. Taniya Das has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56588 )
Change subject: soc: common: clock: Add support for common clock driver ......................................................................
Patch Set 2:
(14 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/56588/comment/3058b864_9dcf5193 PS2, Line 7: Add support for common clock driver
Add common clock driver
Ack
https://review.coreboot.org/c/coreboot/+/56588/comment/ab2aebba_811cf13f PS2, Line 10: root clock generator(RCG)
Please add a space before the (.
Ack
https://review.coreboot.org/c/coreboot/+/56588/comment/15ded691_6ea66518 PS2, Line 14: Zonda/Agera
Please clarify what these names mean.
The are the PLL types used for CPU.
https://review.coreboot.org/c/coreboot/+/56588/comment/bf5df6e6_9961601d PS2, Line 19: SC7180 clock driver is also refactored to use the common clock : driver APIs.
How did you verify it works the same? (Is there some test or log message?)
We have our test code which will set the desired frequencies and then we can measure(debugger connected tool) the frequencies requested.
https://review.coreboot.org/c/coreboot/+/56588/comment/9049150c_77eb7e31 PS2, Line 21:
Please mention the datasheet name and revision.
Sorry Paul, we do not have any such data/revision which we can add here.
Patchset:
PS2:
Yes, it's true that it still needs to compile and boot correctly in between the two patches. […]
Sure, will move the changes accordingly.
File src/soc/qualcomm/common/clock.c:
https://review.coreboot.org/c/coreboot/+/56588/comment/0c359740_9721b5e0 PS2, Line 20: /* Set clock vote bit */
The comment is not needed, as the code line is self-explaining.
Done
https://review.coreboot.org/c/coreboot/+/56588/comment/d6963867_9e333b77 PS2, Line 25: return CB_ERR;
If console works, can an error be logged?
Done
https://review.coreboot.org/c/coreboot/+/56588/comment/a11c66a7_470c981c PS2, Line 84: idx
Can a native type be used?
Done
https://review.coreboot.org/c/coreboot/+/56588/comment/2c0b65ff_d5aa6bee PS2, Line 100: RCG*/
Missing space.
Done
https://review.coreboot.org/c/coreboot/+/56588/comment/3ce4a425_e36a0210 PS2, Line 111: idx
unsigned int?
Done
https://review.coreboot.org/c/coreboot/+/56588/comment/d20625da_dbf338be PS2, Line 183: udelay(10);
Why the delay? Please add a comment.
Done
https://review.coreboot.org/c/coreboot/+/56588/comment/6c9cf6f3_106ea6d3 PS2, Line 188: printk(BIOS_ERR, "ERROR: PLL did not lock!\n");
Please not the consequence in the message. […]
Yes the CPU would be on a default general purpose PLL which would be at a very low frequency.
https://review.coreboot.org/c/coreboot/+/56588/comment/ef82d078_a9ea423c PS2, Line 199: udelay(5);
Why the delay? Please add a comment.
Done