Change in ...coreboot[master]: sdm845: Add USB support on cheza platform

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/25213 )

Change subject: sdm845: Add USB support on cheza platform
......................................................................

Patch Set 65:

(3 comments)

https://review.coreboot.org/#/c/25213/64/src/mainboard/google/cheza/romstage... 
File src/mainboard/google/cheza/romstage.c:

https://review.coreboot.org/#/c/25213/64/src/mainboard/google/cheza/romstage... 
PS64, Line 22: these resets
             : 	 * off early so they get
> "these resets" here i mean is core+ PHYs resets which is plural here. […]

Oh, right, okay, makes sense.

https://review.coreboot.org/#/c/25213/64/src/soc/qualcomm/sdm845/usb.c 
File src/soc/qualcomm/sdm845/usb.c:

https://review.coreboot.org/#/c/25213/64/src/soc/qualcomm/sdm845/usb.c@585 
PS64, Line 585: 	int serdes_tbl_num;
> we can not do this as we have differences in all the regsiter sets(serdes, tx,rx,pcs). […]

Hmmm... okay, crap, I didn't see all those other differences.

Then let's maybe follow a different strategy. Keep the lists separate as you have them right now, but also split up the usb_host_base array into two independent structures, and then write independent entry functions like this:

static struct usb_dwc3_cfg usb_port0 = {
   .usb_host_dwc3 = (void *)USB_HOST0_DWC3_BASE,
   ...
 };

static struct usb_dwc3_cfg usb_port1 = {
   .usb_host_dwc3 = (void *)USB_HOST1_DWC3_BASE,
   ...
 };

void reset_usb0(void)
 {
   reset_usb(&usb_port0, 0);
 }

void reset_usb0(void)
 {
   reset_usb(&usb_port1, 1);
 }

void setup_usb0(void)
 {
   setup_usb(&usb_port0, 0);
 }

void setup_usb0(void)
 {
   setup_usb(&usb_port1, 1);
 }

The advantage of this is that it allows the linker to eliminate the port data (including all those tables) for ports that are not being used, so it will at least allow us to save all the port0 stuff on Cheza.

https://review.coreboot.org/#/c/25213/64/src/soc/qualcomm/sdm845/usb.c@824 
PS64, Line 824: 			USB3_PCS_PHYSTATUS));
> Yes, SW wait untill phystatus become 0. […]

Okay, thanks, just wanted to double-check. ;) I (think I?) don't have access to your programming guide so I can't confirm these independently.