Attention is currently required from: Martin Roth, chris wang. Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/50240 )
Change subject: soc/amd/picasso: add UPD for USB3 phy setting adjust ......................................................................
Patch Set 7:
(5 comments)
File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/50240/comment/781144cd_46590575 PS7, Line 75: #define USB3_PORT_COUNT 4 only true for dali/pollock. picasso has an additional usb3 port on xhci1, so this should be changed to 5; the devices with only 4 usb3 ports then just ignore the last one
https://review.coreboot.org/c/coreboot/+/50240/comment/c7e02b35_56d4f1e6 PS7, Line 258: uint8_t usb3_phy_en; i wonder if just having one setting to enable all usb3 port overrides would be sufficient. if the override is not selected, then just the defaults are used, no no board will break if it doesn't have all those settings in the devicetree, and if not then just all values have to be provided? not sure if having enable bits for the various settings is something that's needed. haven't looked closely at the fsp patch though, so i'm not sure
https://review.coreboot.org/c/coreboot/+/50240/comment/b340a6d3_dfaf3253 PS7, Line 258: usb3_phy_en usb3_phy_override would be a much more descriptive name. from the name i'd assume that it's for enabling the usb3 part of the usb ports, which isn't what it does
File src/vendorcode/amd/fsp/picasso/FspsUpd.h:
https://review.coreboot.org/c/coreboot/+/50240/comment/b39c3f8a_a06aa5cf PS7, Line 58: /** Offset 0x013D**/ uint8_t usb_3_port0_phy_tune[2]; : /** Offset 0x013F**/ uint8_t usb_3_port1_phy_tune[2]; : /** Offset 0x0141**/ uint8_t usb_3_port2_phy_tune[2]; : /** Offset 0x0143**/ uint8_t usb_3_port3_phy_tune[2]; you could do something like i did for the fch_usb_2_port_phy_tune setting above. that allows to just loop over the ports and apply the settings to the upd fields. i'm ok with both. feel free to keep it as it is right now and just ack this comment; that's someting that could be also done in the future.
https://review.coreboot.org/c/coreboot/+/50240/comment/6d8188ad_f783d3b7 PS7, Line 61: /** Offset 0x0143**/ uint8_t usb_3_port3_phy_tune[2]; there are 5 usb3 ports on the picasso chips; dali/pollock have 4 only. so there's the data structure for the last port missing