Attention is currently required from: Jason Glenesk, Raul Rangel, Marshall Dawson, Chris Wang, Felix Held. chris wang 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 6:
(9 comments)
File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/49932/comment/f044e121_178ad777 PS4, Line 63: __packed
since the struct isn't used for raw register access, the layout in memory doesn't matter and it does […]
Done
File src/soc/amd/picasso/fch.c:
https://review.coreboot.org/c/coreboot/+/49932/comment/1579a111_8087f4e2 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. […]
Done
https://review.coreboot.org/c/coreboot/+/49932/comment/fb77998c_13148329 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 […]
Ack
https://review.coreboot.org/c/coreboot/+/49932/comment/d13083f6_ca335b3a 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: […]
Done
https://review.coreboot.org/c/coreboot/+/49932/comment/adc2c787_f757bff0 PS4, Line 118: +
| instead of +
Ack
File src/soc/amd/picasso/include/soc/i2c.h:
https://review.coreboot.org/c/coreboot/+/49932/comment/68ef0ac9_ede0ef4f PS5, Line 25: #define USB_PD_PORT_CONTROL APU_I2C4_BASE + I2C4_USB_PD_CTRL_OFFSET
Macros with complex values should be enclosed in parentheses
done
File src/soc/amd/picasso/include/soc/i2c.h:
https://review.coreboot.org/c/coreboot/+/49932/comment/3b8ab5a1_fe2925dd PS4, Line 24: #define I2C4_ADDR 0xFEDC6600
src/soc/amd/picasso/include/soc/iomap. […]
done
https://review.coreboot.org/c/coreboot/+/49932/comment/0f5fd233_417e02d1 PS4, Line 25:
please use tabs to align the defines like in the rest of the file
Ack
change to USB_PD_PORT_CONTROL (APU_I2C4_BASE + I2C4_USB_PD_CTRL_OFFSET)
https://review.coreboot.org/c/coreboot/+/49932/comment/2af70eee_7cee03f6 PS4, Line 26: #define PORT_OFFSET 0x10
#define PD_PORT_MUX_OFFSET(x) (0x10 * (x)) would make the fch code easier to read
Done
-
chris wang (Code Review)