Attention is currently required from: Martin Roth, Felix Held. chris wang 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:
(4 comments)
File src/soc/amd/picasso/chip.h:
https://review.coreboot.org/c/coreboot/+/50240/comment/83138762_2efe22ef PS7, Line 75: #define USB3_PORT_COUNT 4
only true for dali/pollock. […]
Yes, check with the design guide, the USB3 phy setting should only apply to controller 0. so that only got 4 ports and in the Pollock-PI and Picasso-PI, it also only apply to 4 ports. I will rename the definition to make it clear.
https://review.coreboot.org/c/coreboot/+/50240/comment/c6eb884a_f9ca5c18 PS7, Line 258: uint8_t usb3_phy_en;
i wonder if just having one setting to enable all usb3 port overrides would be sufficient. […]
Yes, I add the default value in the tremyle and delboz's devtree, but agree with you that to have a setting for usb3 phy override might be better.
File src/vendorcode/amd/fsp/picasso/FspsUpd.h:
https://review.coreboot.org/c/coreboot/+/50240/comment/23e2bda6_3e0878ce 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. […]
those port should belong to controller 0 and in our Picasso-PI or Pollock-PI. it only to apply those port for tuning
https://review.coreboot.org/c/coreboot/+/50240/comment/7566596e_84e4f0a5 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. […]
Sure, so that need have a define like FSPS_UPD_USB2_PORT_COUNT. I will check that.
-
chris wang (Code Review)