Change in coreboot[master]: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer

[陳建宏 (Code Review)]

陳建宏 has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/48941 )

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer

Boot on Kingston(EMMC64G-TA29/TX29-HP) and Hynix(H26M74002HMR) eMMC
will be failed. This is a workaround for improving the compatibiliy
between GL9763E(revision 03) and these two eMMC.

Signed-off-by: Renius Chen 
Change-Id: Iddb145ed6a9edb2d7a50248e64659cda78b88ae6
---
M src/drivers/genesyslogic/gl9763e/gl9763e.c
M src/drivers/genesyslogic/gl9763e/gl9763e.h
2 files changed, 16 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/48941/1

diff --git a/src/drivers/genesyslogic/gl9763e/gl9763e.c b/src/drivers/genesyslogic/gl9763e/gl9763e.c
index 48e520b..9b44215 100644
--- a/src/drivers/genesyslogic/gl9763e/gl9763e.c
+++ b/src/drivers/genesyslogic/gl9763e/gl9763e.c
@@ -12,6 +12,8 @@

static void gl9763e_init(struct device *dev)
 {
+	uint32_t ver;
+
    printk(BIOS_INFO, "GL9763E: init\n");
    pci_dev_init(dev);

@@ -25,6 +27,12 @@
    pci_update_config32(dev, PLL_CTL_2, ~PLL_CTL_2_MAX_SSC_MASK, MAX_SSC_30000PPM);
    /* Enable SSC */
    pci_or_config32(dev, PLL_CTL, PLL_CTL_SSC);
+	/* Check CHIP version */
+	ver = pci_read_config32(dev, HW_VER_2);
+	if ((ver & HW_VER_MASK) == REVISION_03) {
+		/* Set clock source for RX path */
+		pci_update_config32(dev, SDCLKRX_DLY, ~CLK_SRC_MASK, AFTER_OUTPUT_BUFF);
+	}
    /* Set VHS to read-only */
    pci_update_config32(dev, VHS, ~VHS_REV_MASK, VHS_REV_R);
 }
diff --git a/src/drivers/genesyslogic/gl9763e/gl9763e.h b/src/drivers/genesyslogic/gl9763e/gl9763e.h
index fd9c6ba..5cdaa68 100644
--- a/src/drivers/genesyslogic/gl9763e/gl9763e.h
+++ b/src/drivers/genesyslogic/gl9763e/gl9763e.h
@@ -21,3 +21,11 @@
 #define PLL_CTL_2			0x93C
 #define   PLL_CTL_2_MAX_SSC_MASK	(0xFFFF << 16)
 #define   MAX_SSC_30000PPM		(0xF5C3 << 16)
+
+#define HW_VER_2	0x8F8
+#define   HW_VER_MASK	0xFFFF
+#define   REVISION_03	0x0011
+
+#define SD_CLKRX_DLY			0x934
+#define   CLK_SRC_MASK			(0x3 << 24)
+#define   AFTER_OUTPUT_BUFF		(0x0 << 24)

[陳建宏 (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer

Boot on Kingston(EMMC64G-TA29/TX29-HP) and Hynix(H26M74002HMR) eMMC
will be failed. This is a workaround for improving the compatibiliy
between GL9763E(revision 03) and these two eMMC.

Signed-off-by: Renius Chen 
Change-Id: Iddb145ed6a9edb2d7a50248e64659cda78b88ae6
---
M src/drivers/genesyslogic/gl9763e/gl9763e.c
M src/drivers/genesyslogic/gl9763e/gl9763e.h
2 files changed, 16 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/48941/2

[Ben Chuang (Code Review)]

Ben Chuang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48941 )

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

Set Ready For Review

[陳建宏 (Code Review)]

Hello build bot (Jenkins),

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

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

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

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer

Booting on Kingston (EMMC64G-TA29/TX29-HP) and Hynix (H26M74002HMR) eMMC
currently fails due to R/W error. This is a workaround to finetune the
data latch timing by verdor-specific setting of GL9763E. For improving
the compatibility of GL9763E with these two eMMC.

Signed-off-by: Renius Chen 
Change-Id: Iddb145ed6a9edb2d7a50248e64659cda78b88ae6
---
M src/drivers/genesyslogic/gl9763e/gl9763e.c
M src/drivers/genesyslogic/gl9763e/gl9763e.h
2 files changed, 16 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/41/48941/3

[陳建宏 (Code Review)]

Attention is currently required from: Paul Menzel.
陳建宏 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48941 )

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

Patch Set 3:

(6 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48941/comment/eccef075_0eb9b9f8 
PS2, Line 9: Boot on Kingston(EMMC64G-TA29/TX29-HP) and Hynix(H26M74002HMR) eMMC
           : will be failed
> Boot … currently fails at ….

Done.

https://review.coreboot.org/c/coreboot/+/48941/comment/8fd929a1_64f6dfd1 
PS2, Line 9: Kingston(EMMC64G-TA29/TX29-HP) and Hynix(H26M74002HMR) eMMC
           : will be failed. This is a workaround for improving the compatibiliy
           : between GL9763E(revision 03)
> Please add a space before the (.

Done.

https://review.coreboot.org/c/coreboot/+/48941/comment/c6df72f9_54578851 
PS2, Line 10: This is a workaround for improving the compatibiliy
            : between GL9763E(revision 03) and these two eMMC.
> Please describe the workaround, and mention in what datasheet (name, revision) it’s described.

Added description of this workaround in the commit message .
It's mentioned in Genesyslogic's internal vendor-specific configuration space document of GL9763E.

https://review.coreboot.org/c/coreboot/+/48941/comment/0bdb0424_d131ed15 
PS2, Line 12:
> Do you have a bug report/issue for this? If so, please reference it here.

Please refer to https://issuetracker.google.com/issues/175354085.

Patchset:

PS3: 
Sorry for my late reply.
Thanks.

File src/drivers/genesyslogic/gl9763e/gl9763e.c:

https://review.coreboot.org/c/coreboot/+/48941/comment/f030bf76_f3a688e8 
PS2, Line 30: CHIP
> Lowercase, but the comment is superfluous in my opinion, as the code is clear.

Done.

[陳建宏 (Code Review)]

Attention is currently required from: Tim Wawrzynczak, Paul Menzel, YH Lin.
陳建宏 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48941 )

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

Patch Set 3:

(6 comments)

Commit Message:

https://review.coreboot.org/c/coreboot/+/48941/comment/f1a61afb_3dc98d28 
PS2, Line 9: Boot on Kingston(EMMC64G-TA29/TX29-HP) and Hynix(H26M74002HMR) eMMC
           : will be failed
> Done.

Done

https://review.coreboot.org/c/coreboot/+/48941/comment/2068eb35_791d3054 
PS2, Line 10: This is a workaround for improving the compatibiliy
            : between GL9763E(revision 03) and these two eMMC.
> Added description of this workaround in the commit message . […]

Done

https://review.coreboot.org/c/coreboot/+/48941/comment/ebc5f8d4_eef50662 
PS2, Line 9: Kingston(EMMC64G-TA29/TX29-HP) and Hynix(H26M74002HMR) eMMC
           : will be failed. This is a workaround for improving the compatibiliy
           : between GL9763E(revision 03)
> Done.

Done

https://review.coreboot.org/c/coreboot/+/48941/comment/212e69ef_f30c99a7 
PS2, Line 12:
> Please refer to https://issuetracker.google.com/issues/175354085.

Done

Patchset:

PS3: 
Uploaded Patchset 3

File src/drivers/genesyslogic/gl9763e/gl9763e.c:

https://review.coreboot.org/c/coreboot/+/48941/comment/fb74fc4b_c755c7bb 
PS2, Line 30: CHIP
> Done.

Done

[YH Lin (Code Review)]

Attention is currently required from: Caveh Jalali, Tim Wawrzynczak, Paul Menzel, 陳建宏, YH Lin.
YH Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48941 )

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

Patch Set 3: Code-Review+1

[Tim Wawrzynczak (Code Review)]

Attention is currently required from: Caveh Jalali, Paul Menzel, 陳建宏, YH Lin.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48941 )

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

Patch Set 3: Code-Review+1

(1 comment)

File src/drivers/genesyslogic/gl9763e/gl9763e.c:

https://review.coreboot.org/c/coreboot/+/48941/comment/f8fb7a74_8a623927 
PS3, Line 31: 	ver = pci_read_config32(dev, HW_VER_2);
            : 	if ((ver & HW_VER_MASK) == REVISION_03) {
            : 		/* Set clock source for RX path */
            : 		pci_update_config32(dev, SD_CLKRX_DLY, ~CLK_SRC_MASK, AFTER_OUTPUT_BUFF);
            : 	}
Is this specifically only to Volteer, or would this help other boards boot using those cards with GL9763E?

[陳建宏 (Code Review)]

Attention is currently required from: Caveh Jalali, Tim Wawrzynczak, Paul Menzel, YH Lin.
陳建宏 has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48941 )

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

Patch Set 3:

(2 comments)

Patchset:

PS3: 
Thanks.

File src/drivers/genesyslogic/gl9763e/gl9763e.c:

https://review.coreboot.org/c/coreboot/+/48941/comment/9295f521_b350e421 
PS3, Line 31: 	ver = pci_read_config32(dev, HW_VER_2);
            : 	if ((ver & HW_VER_MASK) == REVISION_03) {
            : 		/* Set clock source for RX path */
            : 		pci_update_config32(dev, SD_CLKRX_DLY, ~CLK_SRC_MASK, AFTER_OUTPUT_BUFF);
            : 	}
> Is this specifically only to Volteer, or would this help other boards boot using those cards with GL […]

This improves the compatibility of GL9763E with these eMMC cards, not only for a specific platform. Just because the booting failure was observed on Volteer.

[Tim Wawrzynczak (Code Review)]

Attention is currently required from: Caveh Jalali, Paul Menzel, 陳建宏, YH Lin.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48941 )

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

Patch Set 3: Code-Review+2

(1 comment)

File src/drivers/genesyslogic/gl9763e/gl9763e.c:

https://review.coreboot.org/c/coreboot/+/48941/comment/02ebc261_04275f03 
PS3, Line 31: 	ver = pci_read_config32(dev, HW_VER_2);
            : 	if ((ver & HW_VER_MASK) == REVISION_03) {
            : 		/* Set clock source for RX path */
            : 		pci_update_config32(dev, SD_CLKRX_DLY, ~CLK_SRC_MASK, AFTER_OUTPUT_BUFF);
            : 	}
> This improves the compatibility of GL9763E with these eMMC cards, not only for a specific platform. […]

Great, thank you!

[Tim Wawrzynczak (Code Review)]

Tim Wawrzynczak has submitted this change. ( https://review.coreboot.org/c/coreboot/+/48941 )

Change subject: drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer
......................................................................

drivers/genesyslogic/gl9763e: Fix boot on eMMC failed issue on Volteer

Booting on Kingston (EMMC64G-TA29/TX29-HP) and Hynix (H26M74002HMR) eMMC
currently fails due to R/W error. This is a workaround to finetune the
data latch timing by verdor-specific setting of GL9763E. For improving
the compatibility of GL9763E with these two eMMC.

Signed-off-by: Renius Chen 
Change-Id: Iddb145ed6a9edb2d7a50248e64659cda78b88ae6
Reviewed-on: https://review.coreboot.org/c/coreboot/+/48941
Tested-by: build bot (Jenkins) 
Reviewed-by: YH Lin 
Reviewed-by: Tim Wawrzynczak 
---
M src/drivers/genesyslogic/gl9763e/gl9763e.c
M src/drivers/genesyslogic/gl9763e/gl9763e.h
2 files changed, 16 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  YH Lin: Looks good to me, but someone else must approve
  Tim Wawrzynczak: Looks good to me, approved

diff --git a/src/drivers/genesyslogic/gl9763e/gl9763e.c b/src/drivers/genesyslogic/gl9763e/gl9763e.c
index 48e520b..d19cc4a 100644
--- a/src/drivers/genesyslogic/gl9763e/gl9763e.c
+++ b/src/drivers/genesyslogic/gl9763e/gl9763e.c
@@ -12,6 +12,8 @@

static void gl9763e_init(struct device *dev)
 {
+	uint32_t ver;
+
    printk(BIOS_INFO, "GL9763E: init\n");
    pci_dev_init(dev);

@@ -25,6 +27,12 @@
    pci_update_config32(dev, PLL_CTL_2, ~PLL_CTL_2_MAX_SSC_MASK, MAX_SSC_30000PPM);
    /* Enable SSC */
    pci_or_config32(dev, PLL_CTL, PLL_CTL_SSC);
+	/* Check chip version */
+	ver = pci_read_config32(dev, HW_VER_2);
+	if ((ver & HW_VER_MASK) == REVISION_03) {
+		/* Set clock source for RX path */
+		pci_update_config32(dev, SD_CLKRX_DLY, ~CLK_SRC_MASK, AFTER_OUTPUT_BUFF);
+	}
    /* Set VHS to read-only */
    pci_update_config32(dev, VHS, ~VHS_REV_MASK, VHS_REV_R);
 }
diff --git a/src/drivers/genesyslogic/gl9763e/gl9763e.h b/src/drivers/genesyslogic/gl9763e/gl9763e.h
index fd9c6ba..5cdaa68 100644
--- a/src/drivers/genesyslogic/gl9763e/gl9763e.h
+++ b/src/drivers/genesyslogic/gl9763e/gl9763e.h
@@ -21,3 +21,11 @@
 #define PLL_CTL_2			0x93C
 #define   PLL_CTL_2_MAX_SSC_MASK	(0xFFFF << 16)
 #define   MAX_SSC_30000PPM		(0xF5C3 << 16)
+
+#define HW_VER_2	0x8F8
+#define   HW_VER_MASK	0xFFFF
+#define   REVISION_03	0x0011
+
+#define SD_CLKRX_DLY			0x934
+#define   CLK_SRC_MASK			(0x3 << 24)
+#define   AFTER_OUTPUT_BUFF		(0x0 << 24)