Attention is currently required from: Jason Glenesk, Marshall Dawson, Chris Wang, chris wang. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/49932 )
Change subject: soc/amd/picasso: allow USB_PD port setting override ......................................................................
Patch Set 4: Code-Review-1
(8 comments)
File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/49932/comment/be99d671_a7f0e8d7 PS4, Line 63: __packed since the struct isn't used for raw register access, the layout in memory doesn't matter and it doesn't need to be marked as packed
File src/soc/amd/picasso/fch.c:
https://review.coreboot.org/c/coreboot/+/49932/comment/32e4615b_354d7ad2 PS4, Line 112: rfmux_config = read32((void *)(USB_PD_PORT_CONTROL)); this should always be 0. also this reads the register contents from the first mux override register. i'd just drop this
https://review.coreboot.org/c/coreboot/+/49932/comment/20b5e3c3_ff3992a9 PS4, Line 116: rfmux_config |= cfg->usb_pd_config_override[port].rfmux_config; when rfmux_override_en is set for more than one port, this might lead to unexpected/unwanted results, since for the second port it just ors the bits of the second one to the ones from the first one. it also assumes that the lowest 4 bits are always 0 in the register read; if some of those 4 bits are already set, the result might be unexpected. since the other bits aren't masked
https://review.coreboot.org/c/coreboot/+/49932/comment/65bb7f71_72fbecd6 PS4, Line 116: rfmux_config |= cfg->usb_pd_config_override[port].rfmux_config; : write32((void *)(USB_PD_PORT_CONTROL + (port)*PORT_OFFSET), : USB_PD_RFMUX_OVERRIDE + rfmux_config); i'd remove the read32 above and replace the code inside the if block with: write32((void *)(USB_PD_PORT_CONTROL + PD_PORT_MUX_OFFSET(port)), cfg->usb_pd_config_override[port].rfmux_config | USB_PD_RFMUX_OVERRIDE);
https://review.coreboot.org/c/coreboot/+/49932/comment/f2e001da_4dba6bdc PS4, Line 118: + | instead of +
File src/soc/amd/picasso/include/soc/i2c.h:
https://review.coreboot.org/c/coreboot/+/49932/comment/d18b3dad_4166df86 PS4, Line 24: #define I2C4_ADDR 0xFEDC6600 src/soc/amd/picasso/include/soc/iomap.h already defines APU_I2C4_BASE as 0xfedc6000, so i'd drop this and define USB_PD_PORT_CONTROL below as APU_I2C4_BASE + 0x600. or define the 0x600 offset as I2C4_USB_PD_CTRL_OFFSET and define USB_PD_PORT_CONTROL as APU_I2C4_BASE + I2C4_USB_PD_CTRL_OFFSET. the latter is probably a bit nicer
https://review.coreboot.org/c/coreboot/+/49932/comment/4467cc9f_d38f6365 PS4, Line 25: please use tabs to align the defines like in the rest of the file
https://review.coreboot.org/c/coreboot/+/49932/comment/f5779ac3_95f42cf2 PS4, Line 26: #define PORT_OFFSET 0x10 #define PD_PORT_MUX_OFFSET(x) (0x10 * (x)) would make the fch code easier to read