Akash Asthana has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29104 )
Change subject: sdm845: Port I2C driver ......................................................................
Patch Set 26:
(10 comments)
Thanks for reviewing, I have shared the code with Mike, Once it's released here I will update in the buganizer.
https://review.coreboot.org/#/c/29104/24/src/mainboard/google/cheza/mainboar... File src/mainboard/google/cheza/mainboard.c:
https://review.coreboot.org/#/c/29104/24/src/mainboard/google/cheza/mainboar... PS24, Line 42: i2c_init(12, 400 * KHz, 1);
This should be an enum (QUP_WRAP1_S4 or something like that). […]
As per a discussion with clock team they need this enum(QUP_WRAP1_S4...) for their functionality. So, instead of moving this enum to qcom_qup_se.h, I have defined a new enum(qup_se) in the common header(qcom_qup_se.h) and using it across the drivers.
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c File src/soc/qualcomm/sdm845/i2c.c:
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@46 PS24, Line 46: unsigned int hz, unsigned int idx
Let's not pass both Hz and an index. […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@101 PS24, Line 101: BITS_PER_WORD >> 4
How exactly does this work? When it says byte_granularity I would expect 1 means 1 byte, 2 means 2 b […]
geni_byte_granularity is used to configure the number of protocol words in each FIFO entry. Here is the mapping between granularity value and number of protocol words in each FIFO entry. 0 - 4*8, four words in each entry, max word size of 8 bits 1 - 2*16, two words in each entry, max word size of 16 bits 2 - 1*32, one word in each entry, max word size of 32 bits 3+ - undefined
It will work for BITS_PER_WORD == (16 or 32), however, in that case, we have to make necessary changes in FIFO PACKING CONFIGURATION because we have hardcoded the PACK_VECTORs assuming that BITS_PER_WORD is always 8. It will not work for BITS_PER_WORD = 64.
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@118 PS24, Line 118: setbits_le32(®s->geni_s_irq_enable, S_CMD_DONE_EN);
Why not combine this with the other write to that register above?
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@122 PS24, Line 122: static u32 wait_till_irq_set(unsigned int bus)
This is the exact same function as in the SPI code, right? Let's put it in qup. […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@187 PS24, Line 187: static void i2c_handle_error(unsigned int bus)
Actually... […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@228 PS24, Line 228: stopwatch_init_msecs_expire(&sw, 1000);
... […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@245 PS24, Line 245: (m_irq & M_CMD_DONE_EN)
nit: Can remove this part of the check. […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@247 PS24, Line 247: printk(BIOS_INFO, "%s:Error: Transfer failed\n", __func__);
nit: Please write as "ERROR: %s failed\n". […]
Done
https://review.coreboot.org/#/c/29104/24/src/soc/qualcomm/sdm845/i2c.c@261 PS24, Line 261: /* Set stretch = 0 for the last transfer */
Please also explain what that bit does and why it needs to be set only for the last transfer.
Done
-
Akash Asthana (Code Review)