Change in coreboot[master]: amd/picasso: rework USB2 PHY tune parameter handling

[Felix Held (Code Review)]

Felix Held has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43783 )

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

amd/picasso: rework USB2 PHY tune parameter handling

Change-Id: I67f23c0602e345fbd806e661a4462cf07f93ef64
Signed-off-by: Felix Held 
---
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
M src/vendorcode/amd/fsp/picasso/FspsUpd.h
3 files changed, 10 insertions(+), 19 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/43783/1

diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h
index d3c9a07..266faad 100644
--- a/src/soc/amd/picasso/chip.h
+++ b/src/soc/amd/picasso/chip.h
@@ -37,6 +37,8 @@
    uint8_t	tx_res_tune;
 };

+#define USB2_PHY_TUNE_COUNT	6
+
 struct soc_amd_picasso_config {
    struct soc_amd_common_config common_config;
    /*
@@ -134,12 +136,7 @@
    uint8_t xhci0_force_gen1;

uint8_t has_usb2_phy_tune_params;
-	struct usb2_phy_tune usb_2_port_0_tune_params;
-	struct usb2_phy_tune usb_2_port_1_tune_params;
-	struct usb2_phy_tune usb_2_port_2_tune_params;
-	struct usb2_phy_tune usb_2_port_3_tune_params;
-	struct usb2_phy_tune usb_2_port_4_tune_params;
-	struct usb2_phy_tune usb_2_port_5_tune_params;
+	struct usb2_phy_tune usb_2_port_tune_params[USB2_PHY_TUNE_COUNT];
 };

typedef struct soc_amd_picasso_config config_t;
diff --git a/src/soc/amd/picasso/fsp_params.c b/src/soc/amd/picasso/fsp_params.c
index d280bff..b0986c8 100644
--- a/src/soc/amd/picasso/fsp_params.c
+++ b/src/soc/amd/picasso/fsp_params.c
@@ -98,17 +98,15 @@
 static void fsp_usb_oem_customization(FSP_S_CONFIG *scfg,
    		const struct soc_amd_picasso_config *cfg)
 {
-	size_t num = sizeof(struct usb2_phy_tune);
+	ASSERT(FSPS_UPD_USB2_PORT_COUNT == USB2_PHY_TUNE_COUNT);

scfg->xhci0_force_gen1 = cfg->xhci0_force_gen1;

if (cfg->has_usb2_phy_tune_params) {
-		memcpy(scfg->fch_usb_2_port0_phy_tune, &cfg->usb_2_port_0_tune_params, num);
-		memcpy(scfg->fch_usb_2_port1_phy_tune, &cfg->usb_2_port_1_tune_params, num);
-		memcpy(scfg->fch_usb_2_port2_phy_tune, &cfg->usb_2_port_2_tune_params, num);
-		memcpy(scfg->fch_usb_2_port3_phy_tune, &cfg->usb_2_port_3_tune_params, num);
-		memcpy(scfg->fch_usb_2_port4_phy_tune, &cfg->usb_2_port_4_tune_params, num);
-		memcpy(scfg->fch_usb_2_port5_phy_tune, &cfg->usb_2_port_5_tune_params, num);
+		for (size_t i = 0; i < FSPS_UPD_USB2_PORT_COUNT; i++) {
+			memcpy(scfg->fch_usb_2_port_phy_tune[i], &cfg->usb_2_port_tune_params[i],
+				sizeof(scfg->fch_usb_2_port_phy_tune[0]));
+		}
    }
 }

diff --git a/src/vendorcode/amd/fsp/picasso/FspsUpd.h b/src/vendorcode/amd/fsp/picasso/FspsUpd.h
index 4298b11..5adbb81 100644
--- a/src/vendorcode/amd/fsp/picasso/FspsUpd.h
+++ b/src/vendorcode/amd/fsp/picasso/FspsUpd.h
@@ -11,6 +11,7 @@

#define FSPS_UPD_DXIO_DESCRIPTOR_COUNT	8
 #define FSPS_UPD_DDI_DESCRIPTOR_COUNT	4
+#define FSPS_UPD_USB2_PORT_COUNT	6

typedef struct __packed {
    /** Offset 0x0020**/	uint32_t                    emmc0_mode;
@@ -21,12 +22,7 @@
    /** Offset 0x00D0**/	uint8_t                     unused2[16];
    /** Offset 0x00E0**/	uint8_t                     fch_usb_version_major;
    /** Offset 0x00E1**/	uint8_t                     fch_usb_version_minor;
-	/** Offset 0x00E2**/	uint8_t                     fch_usb_2_port0_phy_tune[9];
-	/** Offset 0x00EB**/	uint8_t                     fch_usb_2_port1_phy_tune[9];
-	/** Offset 0x00F4**/	uint8_t                     fch_usb_2_port2_phy_tune[9];
-	/** Offset 0x00FD**/	uint8_t                     fch_usb_2_port3_phy_tune[9];
-	/** Offset 0x0106**/	uint8_t                     fch_usb_2_port4_phy_tune[9];
-	/** Offset 0x010F**/	uint8_t                     fch_usb_2_port5_phy_tune[9];
+	/** Offset 0x00E2**/	uint8_t                     fch_usb_2_port_phy_tune[FSPS_UPD_USB2_PORT_COUNT][9];
    /** Offset 0x0118**/	uint8_t                     fch_usb_device_removable;
    /** Offset 0x0119**/	uint8_t                     fch_usb_3_port_force_gen1;
    /** Offset 0x011A**/	uint8_t                     fch_usb_u3_rx_det_wa_enable;

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43783 )

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

Patch Set 1: Code-Review+2

[build bot (Jenkins) (Code Review)]

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43783 )

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43783/1/src/soc/amd/picasso/fsp_par... 
File src/soc/amd/picasso/fsp_params.c:

https://review.coreboot.org/c/coreboot/+/43783/1/src/soc/amd/picasso/fsp_par... 
PS1, Line 107: 			memcpy(scfg->fch_usb_2_port_phy_tune[i], &cfg->usb_2_port_tune_params[i],
line over 96 characters

[Felix Held (Code Review)]

Hello build bot (Jenkins), Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/43783

to look at the new patch set (#2).

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

amd/picasso: rework USB2 PHY tune parameter handling

Change-Id: I67f23c0602e345fbd806e661a4462cf07f93ef64
Signed-off-by: Felix Held 
---
M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb
M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
M src/vendorcode/amd/fsp/picasso/FspsUpd.h
5 files changed, 23 insertions(+), 31 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/43783/2

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43783 )

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

Patch Set 2: Code-Review+1

[Felix Held (Code Review)]

Hello build bot (Jenkins), Angel Pons, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/43783

to look at the new patch set (#3).

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

amd/picasso: rework USB2 PHY tune parameter handling

BUG=b:161923068

Change-Id: I67f23c0602e345fbd806e661a4462cf07f93ef64
Signed-off-by: Felix Held 
---
M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb
M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
M src/vendorcode/amd/fsp/picasso/FspsUpd.h
5 files changed, 23 insertions(+), 31 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/43783/3

[Felix Held (Code Review)]

Hello build bot (Jenkins), Angel Pons, Aaron Durbin,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/43783

to look at the new patch set (#4).

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

amd/picasso: rework USB2 PHY tune parameter handling

BUG=b:161923068

Change-Id: I67f23c0602e345fbd806e661a4462cf07f93ef64
Signed-off-by: Felix Held 
---
M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb
M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
M src/vendorcode/amd/fsp/picasso/FspsUpd.h
5 files changed, 23 insertions(+), 31 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/83/43783/4

[Angel Pons (Code Review)]

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43783 )

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

Patch Set 4: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43783 )

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

Patch Set 4: Code-Review+2

(1 comment)

https://review.coreboot.org/c/coreboot/+/43783/4/src/soc/amd/picasso/chip.h 
File src/soc/amd/picasso/chip.h:

https://review.coreboot.org/c/coreboot/+/43783/4/src/soc/amd/picasso/chip.h@... 
PS4, Line 139: USB_PORT_COUNT
Just curious: Can't we directly use `FSPS_UPD_USB2_PORT_COUNT` here?

[Felix Held (Code Review)]

Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43783 )

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43783/4/src/soc/amd/picasso/chip.h 
File src/soc/amd/picasso/chip.h:

https://review.coreboot.org/c/coreboot/+/43783/4/src/soc/amd/picasso/chip.h@... 
PS4, Line 139: USB_PORT_COUNT
> Just curious: Can't we directly use `FSPS_UPD_USB2_PORT_COUNT` here?

I wasn't sure if it would be a good idea to pull in the FspsUpd header here. FSPS_UPD_USB2_PORT_COUNT should definitely stay in FspsUpd.h

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43783 )

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/43783/4/src/soc/amd/picasso/chip.h 
File src/soc/amd/picasso/chip.h:

https://review.coreboot.org/c/coreboot/+/43783/4/src/soc/amd/picasso/chip.h@... 
PS4, Line 139: USB_PORT_COUNT
> I wasn't sure if it would be a good idea to pull in the FspsUpd header here. […]

Agreed FSPS_UPD_USB2_PORT_COUTN should stay in FspsUpd.h. I was just wondering if we should reuse the same macro here. Anyways, we can go ahead with USB_PORT_COUNT.

[Felix Held (Code Review)]

Felix Held has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43783 )

Change subject: amd/picasso: rework USB2 PHY tune parameter handling
......................................................................

amd/picasso: rework USB2 PHY tune parameter handling

BUG=b:161923068

Change-Id: I67f23c0602e345fbd806e661a4462cf07f93ef64
Signed-off-by: Felix Held 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/43783
Reviewed-by: Angel Pons 
Reviewed-by: Furquan Shaikh 
Tested-by: build bot (Jenkins) 
---
M src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb
M src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb
M src/soc/amd/picasso/chip.h
M src/soc/amd/picasso/fsp_params.c
M src/vendorcode/amd/fsp/picasso/FspsUpd.h
5 files changed, 23 insertions(+), 31 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved
  Angel Pons: Looks good to me, approved

diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb
index 4a01a12..6f72c85 100644
--- a/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb
+++ b/src/mainboard/google/zork/variants/baseboard/devicetree_dalboz.cb
@@ -54,7 +54,7 @@
    register "has_usb2_phy_tune_params" = "1"

# Controller0 Port0 Default
-	register "usb_2_port_0_tune_params" = "{
+	register "usb_2_port_tune_params[0]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -67,7 +67,7 @@
    }"

# Controller0 Port1 Default
-	register "usb_2_port_1_tune_params" = "{
+	register "usb_2_port_tune_params[1]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -80,7 +80,7 @@
    }"

# Controller0 Port2 Default
-	register "usb_2_port_2_tune_params" = "{
+	register "usb_2_port_tune_params[2]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -93,7 +93,7 @@
    }"

# Controller0 Port3 Default
-	register "usb_2_port_3_tune_params" = "{
+	register "usb_2_port_tune_params[3]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -106,7 +106,7 @@
    }"

# Controller1 Port0 Default
-	register "usb_2_port_4_tune_params" = "{
+	register "usb_2_port_tune_params[4]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -119,7 +119,7 @@
    }"

# Controller1 Port1 Default
-	register "usb_2_port_5_tune_params" = "{
+	register "usb_2_port_tune_params[5]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
diff --git a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb
index 986c444..d43063c 100644
--- a/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb
+++ b/src/mainboard/google/zork/variants/baseboard/devicetree_trembyle.cb
@@ -54,7 +54,7 @@
    register "has_usb2_phy_tune_params" = "1"

# Controller0 Port0 Default
-	register "usb_2_port_0_tune_params" = "{
+	register "usb_2_port_tune_params[0]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -67,7 +67,7 @@
    }"

# Controller0 Port1 Default
-	register "usb_2_port_1_tune_params" = "{
+	register "usb_2_port_tune_params[1]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -80,7 +80,7 @@
    }"

# Controller0 Port2 Default
-	register "usb_2_port_2_tune_params" = "{
+	register "usb_2_port_tune_params[2]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -93,7 +93,7 @@
    }"

# Controller0 Port3 Default
-	register "usb_2_port_3_tune_params" = "{
+	register "usb_2_port_tune_params[3]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -106,7 +106,7 @@
    }"

# Controller1 Port0 Default
-	register "usb_2_port_4_tune_params" = "{
+	register "usb_2_port_tune_params[4]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
@@ -119,7 +119,7 @@
    }"

# Controller1 Port1 Default
-	register "usb_2_port_5_tune_params" = "{
+	register "usb_2_port_tune_params[5]" = "{
    	.com_pds_tune = 0x03,
    	.sq_rx_tune = 0x3,
    	.tx_fsls_tune = 0x3,
diff --git a/src/soc/amd/picasso/chip.h b/src/soc/amd/picasso/chip.h
index d3c9a07..64258b2 100644
--- a/src/soc/amd/picasso/chip.h
+++ b/src/soc/amd/picasso/chip.h
@@ -37,6 +37,8 @@
    uint8_t	tx_res_tune;
 };

+#define USB_PORT_COUNT	6
+
 struct soc_amd_picasso_config {
    struct soc_amd_common_config common_config;
    /*
@@ -134,12 +136,7 @@
    uint8_t xhci0_force_gen1;

uint8_t has_usb2_phy_tune_params;
-	struct usb2_phy_tune usb_2_port_0_tune_params;
-	struct usb2_phy_tune usb_2_port_1_tune_params;
-	struct usb2_phy_tune usb_2_port_2_tune_params;
-	struct usb2_phy_tune usb_2_port_3_tune_params;
-	struct usb2_phy_tune usb_2_port_4_tune_params;
-	struct usb2_phy_tune usb_2_port_5_tune_params;
+	struct usb2_phy_tune usb_2_port_tune_params[USB_PORT_COUNT];
 };

typedef struct soc_amd_picasso_config config_t;
diff --git a/src/soc/amd/picasso/fsp_params.c b/src/soc/amd/picasso/fsp_params.c
index d280bff..8e6703e 100644
--- a/src/soc/amd/picasso/fsp_params.c
+++ b/src/soc/amd/picasso/fsp_params.c
@@ -98,17 +98,16 @@
 static void fsp_usb_oem_customization(FSP_S_CONFIG *scfg,
    		const struct soc_amd_picasso_config *cfg)
 {
-	size_t num = sizeof(struct usb2_phy_tune);
+	ASSERT(FSPS_UPD_USB2_PORT_COUNT == USB_PORT_COUNT);

scfg->xhci0_force_gen1 = cfg->xhci0_force_gen1;

if (cfg->has_usb2_phy_tune_params) {
-		memcpy(scfg->fch_usb_2_port0_phy_tune, &cfg->usb_2_port_0_tune_params, num);
-		memcpy(scfg->fch_usb_2_port1_phy_tune, &cfg->usb_2_port_1_tune_params, num);
-		memcpy(scfg->fch_usb_2_port2_phy_tune, &cfg->usb_2_port_2_tune_params, num);
-		memcpy(scfg->fch_usb_2_port3_phy_tune, &cfg->usb_2_port_3_tune_params, num);
-		memcpy(scfg->fch_usb_2_port4_phy_tune, &cfg->usb_2_port_4_tune_params, num);
-		memcpy(scfg->fch_usb_2_port5_phy_tune, &cfg->usb_2_port_5_tune_params, num);
+		for (size_t i = 0; i < FSPS_UPD_USB2_PORT_COUNT; i++) {
+			memcpy(scfg->fch_usb_2_port_phy_tune[i],
+				&cfg->usb_2_port_tune_params[i],
+				sizeof(scfg->fch_usb_2_port_phy_tune[0]));
+		}
    }
 }

diff --git a/src/vendorcode/amd/fsp/picasso/FspsUpd.h b/src/vendorcode/amd/fsp/picasso/FspsUpd.h
index 4298b11..5adbb81 100644
--- a/src/vendorcode/amd/fsp/picasso/FspsUpd.h
+++ b/src/vendorcode/amd/fsp/picasso/FspsUpd.h
@@ -11,6 +11,7 @@

#define FSPS_UPD_DXIO_DESCRIPTOR_COUNT	8
 #define FSPS_UPD_DDI_DESCRIPTOR_COUNT	4
+#define FSPS_UPD_USB2_PORT_COUNT	6

typedef struct __packed {
    /** Offset 0x0020**/	uint32_t                    emmc0_mode;
@@ -21,12 +22,7 @@
    /** Offset 0x00D0**/	uint8_t                     unused2[16];
    /** Offset 0x00E0**/	uint8_t                     fch_usb_version_major;
    /** Offset 0x00E1**/	uint8_t                     fch_usb_version_minor;
-	/** Offset 0x00E2**/	uint8_t                     fch_usb_2_port0_phy_tune[9];
-	/** Offset 0x00EB**/	uint8_t                     fch_usb_2_port1_phy_tune[9];
-	/** Offset 0x00F4**/	uint8_t                     fch_usb_2_port2_phy_tune[9];
-	/** Offset 0x00FD**/	uint8_t                     fch_usb_2_port3_phy_tune[9];
-	/** Offset 0x0106**/	uint8_t                     fch_usb_2_port4_phy_tune[9];
-	/** Offset 0x010F**/	uint8_t                     fch_usb_2_port5_phy_tune[9];
+	/** Offset 0x00E2**/	uint8_t                     fch_usb_2_port_phy_tune[FSPS_UPD_USB2_PORT_COUNT][9];
    /** Offset 0x0118**/	uint8_t                     fch_usb_device_removable;
    /** Offset 0x0119**/	uint8_t                     fch_usb_3_port_force_gen1;
    /** Offset 0x011A**/	uint8_t                     fch_usb_u3_rx_det_wa_enable;