Akash Asthana has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/27483 )
Change subject: sdm845: Add SPI QUP driver ......................................................................
Patch Set 46:
(7 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/27483/46/src/mainboard/google/cheza/bootbloc... File src/mainboard/google/cheza/bootblock.c:
https://review.coreboot.org/#/c/27483/46/src/mainboard/google/cheza/bootbloc... PS46, Line 24: qup_spi_init(10, 1010 * KHz);
Please add comments describing which devices (EC and H1) these are connected to.
Done
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/bootblock.c File src/soc/qualcomm/sdm845/bootblock.c:
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/bootblock.c... PS46, Line 4: * Copyright (C) 2018-2019, The Linux Foundation. All rights reserved.
nit: file not actually changing anymore?
We moved qup_spi_init from this file to cheza/bootblock.c in 2019.
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/include/soc... File src/soc/qualcomm/sdm845/include/soc/qcom_qup_se.h:
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/include/soc... PS46, Line 203: union proto_rx_trans_len {
nit: is there a point in having this as a union? Why not just name the field tx_trans_len? (Goes for […]
I am not sure whether I get your suggestion correctly. In union proto_rx_trans_len we have listed 3 registers 1) uart_tx_trans_len (No. of tx byte to transfer in UART) 2,3)i2c/spi_rx_trans_len (No. of rx byte to transfer in I2C/SPI).
Why not just name the field tx_trans_len?
I don't think that it would be appropriate, because we also use that same register to configure rx_trans_len for I2C/SPI. We need union for these cases where we use exactly same register in 2 very different context.
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c File src/soc/qualcomm/sdm845/spi_qup.c:
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c@5... PS46, Line 58: extern struct qup qup[16];
This declaration should go in qcom_qup_se.h, not here.
Done
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c@1... PS46, Line 195: setup_fifo_params(slave);
Sorry if I asked this before and forgot again... […]
Yes, We need to do this again on every transfer. I have kept only the minimum neccassary configuration which we need to do for every transfer in this function.
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c@2... PS46, Line 237: if (!(m_irq & M_CMD_DONE_EN) || ((m_irq & M_CMD_DONE_EN)
!A || (A && B) can be simplified to !A || B. So this can just be […]
Done
https://review.coreboot.org/#/c/27483/46/src/soc/qualcomm/sdm845/spi_qup.c@2... PS46, Line 273: assert(((DEFAULT_SE_CLK - speed_hz * div) <= div * KHz) && (div > 0));
nit: better put this at the top, right after div is calculated (before writing it to m_clk_cfg). […]
Done
-
Akash Asthana (Code Review)