Julius Werner 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)
Great, this looks almost ready to go. I suggest rebasing it before the blob-dependent patches so we can land it.
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.
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?
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 the others as well...)
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.
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... do we really need to do this again on every transfer? Wouldn't it be enough to do it once in qup_spi_init()?
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
if (!(m_irq & M_CMD_DONE_EN) || tx_rem_bytes || rx_rem_bytes)
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). Also maybe add an explaining comment (e.g. /* make sure div can hit target frequency within +/- 1KHz range */).
-
Julius Werner (Code Review)