Attention is currently required from: Ravi kumar, Sandeep Maheswaram, Paul Menzel, mturney mturney. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56091 )
Change subject: soc/qualcomm/common/usb: Add support for common USB driver ......................................................................
Patch Set 34:
(8 comments)
File src/soc/qualcomm/common/Kconfig:
https://review.coreboot.org/c/coreboot/+/56091/comment/71b923ab_32600b06 PS34, Line 15: endif Everything you're adding should be within this if-endif block.
https://review.coreboot.org/c/coreboot/+/56091/comment/78e160be_ff7d150d PS34, Line 31: default n If you follow my other comments I don't think you'll need any Kconfigs for this. Just having the SoC Makefile decide which file to compile in should be enough.
File src/soc/qualcomm/common/include/soc/usb/snps_usb_phy.h:
https://review.coreboot.org/c/coreboot/+/56091/comment/943b16ad_7754384a PS34, Line 41: #define QC_GENMASK(h, l) (BIT(h + 1) - BIT(l)) Use the existing GENMASK() macro from helpers.h rather than rolling your own. (I'm pretty sure I already mentioned this on another CL, please fix this globally across the stack so we don't have to ask for it again on every patch.)
https://review.coreboot.org/c/coreboot/+/56091/comment/e51d536a_eb4807a3 PS34, Line 43: #define SLEEPM BIT(0) Please don't define constants with no namespacing like this in global headers. Either move these to a .c file (or a private header that's only included by the specific .c file(s) belonging to this driver), or prefix these with SNPS_USB_ or the like.
File src/soc/qualcomm/common/include/soc/usb/usb_common.h:
https://review.coreboot.org/c/coreboot/+/56091/comment/f2b680cf_ccad0757 PS34, Line 3: #if CONFIG(QUSB_PHY) Please don't conditionally include things like this, it easily becomes confusing. You should just declare the prototype for hs_usb_phy_init() right in this header, then you shouldn't need to include either of these here. For the board_data, you can either use a union or you can just make it void * and then cast it to the respective type inside the function (although it doesn't look like you're really using the board_data for the snps anyway?).
https://review.coreboot.org/c/coreboot/+/56091/comment/5eafc299_92bcb591 PS34, Line 39: * USB BASE ADDRESSES What is this comment supposed to mean?
File src/soc/qualcomm/common/usb/qmp_usb_phy.c:
https://review.coreboot.org/c/coreboot/+/56091/comment/3b102a24_953db2d9 PS34, Line 7: #if CONFIG(QMPV4_USB_PHY) Please factor these out into separate files (e.g. a qmpv3_usb_phy.c and a qmpv4_usb_phy.c). It's better to duplicate a few lines than make the code this hard to follow through preprocessor guards.
File src/soc/qualcomm/common/usb/snps_usb_phy.c:
https://review.coreboot.org/c/coreboot/+/56091/comment/9e2f04de_41a3c410 PS34, Line 8: static void qcom_snps_hsphy_write_mask(u32 *offset, u32 mask, u32 val) We already have clrsetbits32() for this. (But generally, you shouldn't read-modify-write every single register on a peripheral that was just reset to power-on default values anyway, that's just a waste of time. Unless you really *need* to preserve the other bits that were set in there already, and you're in a situation where you really can't know at compie-time what those bits would be set to, just use write32().)
-
Julius Werner (Code Review)