Change in ...coreboot[master]: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50

[Werner Zeh (Code Review)]

Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32542

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

mb/siemens/mc_apl2: Limit SD-Card speed to DDR50

Due to PCB limitations the SD-Card interface is not able to operate
with the highest frequency reliably. The OS driver will switch to
the highest mode if a SD-Card is attached which supports this high
frequency mode. In order to work around this PCB limitation disable the
high frequency modes in the controller capabilities (SDR104 and HS400
mode) and leave SDR50 and DDR50 enabled.

Change-Id: Ia5fed5fb70b027de34170b49620927614a00fb7a
Signed-off-by: Werner Zeh 
---
M src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
1 file changed, 22 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/32542/1

diff --git a/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c b/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
index 0c2418a..1cbb689 100644
--- a/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
+++ b/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
@@ -23,11 +23,17 @@
 #include <timer.h>
 #include <timestamp.h>
 #include <baseboard/variants.h>
+#include <soc/pci_devs.h>
+
+#define SD_CAP_BYP		0x810
+#define  SD_CAP_BYP_EN		0x5A
+#define SD_CAP_BYP_REG1		0x814

void variant_mainboard_final(void)
 {
    struct device *dev;
    uint16_t cmd = 0;
+	void *base;

/* Set Master Enable for on-board PCI device. */
    dev = dev_find_device(PCI_VENDOR_ID_SIEMENS, 0x403e, 0);
@@ -36,6 +42,22 @@
    	cmd |= PCI_COMMAND_MASTER;
    	pci_write_config16(dev, PCI_COMMAND, cmd);
    }
+
+	/* Reduce SD-Card speed to DDR50 because of PCB constraints. */
+	dev = dev_find_slot(0, PCH_DEVFN_SDCARD);
+	if (dev) {
+		uint32_t reg;
+		base = (void *)(pci_read_config32(dev, PCI_BASE_ADDRESS_0) &
+					~PCI_BASE_ADDRESS_MEM_ATTR_MASK);
+		if (!base)
+			return;
+
+		write32(base + SD_CAP_BYP, SD_CAP_BYP_EN);
+		reg = read32(base + SD_CAP_BYP_REG1);
+		/* Disable HS400 and SDR104, keep SDR50 and DDR50 modes. */
+		reg &= ~0x20005800;
+		write32(base + SD_CAP_BYP_REG1, reg);
+	}
 }

static void wait_for_legacy_dev(void *unused)

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32542 )

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

Patch Set 1: Code-Review+2

[Kyösti Mälkki (Code Review)]

Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32542 )

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/32542/1/src/mainboard/siemens/mc_apl1/varian... 
File src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c:

https://review.coreboot.org/#/c/32542/1/src/mainboard/siemens/mc_apl1/varian... 
PS1, Line 47: 	dev = dev_find_slot(0, PCH_DEVFN_SDCARD);
Would pcidev_path_on_root(PCH_DEVFN_SDCARD) work here instead?

There are some aspects of dev_find_slot() why I would like to see it deprecated. One of them is that not only does the 'slot' aka 'dev' section of the PCI path BDF has to match, but also the '.fn' part.

https://review.coreboot.org/#/c/32542/1/src/mainboard/siemens/mc_apl1/varian... 
PS1, Line 55: 		write32(base + SD_CAP_BYP, SD_CAP_BYP_EN);
Looks like arithmetics performed on void pointer. AFAIR this is GCC extension not in the C standard but I don't remember if we generally approve or disapprove these.

[Werner Zeh (Code Review)]

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

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/32542/1/src/mainboard/siemens/mc_apl1/varian... 
File src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c:

https://review.coreboot.org/#/c/32542/1/src/mainboard/siemens/mc_apl1/varian... 
PS1, Line 47: 	dev = dev_find_slot(0, PCH_DEVFN_SDCARD);
> Would pcidev_path_on_root(PCH_DEVFN_SDCARD) work here instead? […]

Sure, pcidev_path_on_root() should be fine here as well. Will change.

https://review.coreboot.org/#/c/32542/1/src/mainboard/siemens/mc_apl1/varian... 
PS1, Line 55: 		write32(base + SD_CAP_BYP, SD_CAP_BYP_EN);
> Looks like arithmetics performed on void pointer. […]

Will change to uint32 arithmetic and cast to void* on write32/read32.

[Werner Zeh (Code Review)]

Hello Mario Scheithauer, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

mb/siemens/mc_apl2: Limit SD-Card speed to DDR50

Due to PCB limitations the SD-Card interface is not able to operate
with the highest frequency reliably. The OS driver will switch to
the highest mode if a SD-Card is attached which supports this high
frequency mode. In order to work around this PCB limitation disable the
high frequency modes in the controller capabilities (SDR104 and HS400
mode) and leave SDR50 and DDR50 enabled.

Change-Id: Ia5fed5fb70b027de34170b49620927614a00fb7a
Signed-off-by: Werner Zeh 
---
M src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
1 file changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/32542/2

[Mario Scheithauer (Code Review)]

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32542 )

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

Patch Set 2: Code-Review+2

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32542 )

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/32542/2/src/mainboard/siemens/mc_apl1/varian... 
File src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c:

https://review.coreboot.org/#/c/32542/2/src/mainboard/siemens/mc_apl1/varian... 
PS2, Line 49: pci_read_config32(dev
Use find_resource(dev) and res2mmio

[Werner Zeh (Code Review)]

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

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/32542/2/src/mainboard/siemens/mc_apl1/varian... 
File src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c:

https://review.coreboot.org/#/c/32542/2/src/mainboard/siemens/mc_apl1/varian... 
PS2, Line 49: pci_read_config32(dev
> Use find_resource(dev) and res2mmio

I had a look into find_resource(dev) and res2mmio. At the first glance it looked weird...but then, looking deeper, it turned out to be a nice way to do it. Will change.

[Werner Zeh (Code Review)]

Hello Patrick Rudolph, Mario Scheithauer, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

mb/siemens/mc_apl2: Limit SD-Card speed to DDR50

Due to PCB limitations the SD-Card interface is not able to operate
with the highest frequency reliably. The OS driver will switch to
the highest mode if a SD-Card is attached which supports this high
frequency mode. In order to work around this PCB limitation disable the
high frequency modes in the controller capabilities (SDR104 and HS400
mode) and leave SDR50 and DDR50 enabled.

Change-Id: Ia5fed5fb70b027de34170b49620927614a00fb7a
Signed-off-by: Werner Zeh 
---
M src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/32542/3

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32542 )

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

Patch Set 3: Code-Review+2

[Werner Zeh (Code Review)]

Werner Zeh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/32542 )

Change subject: mb/siemens/mc_apl2: Limit SD-Card speed to DDR50
......................................................................

mb/siemens/mc_apl2: Limit SD-Card speed to DDR50

Due to PCB limitations the SD-Card interface is not able to operate
with the highest frequency reliably. The OS driver will switch to
the highest mode if a SD-Card is attached which supports this high
frequency mode. In order to work around this PCB limitation disable the
high frequency modes in the controller capabilities (SDR104 and HS400
mode) and leave SDR50 and DDR50 enabled.

Change-Id: Ia5fed5fb70b027de34170b49620927614a00fb7a
Signed-off-by: Werner Zeh 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/32542
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
---
M src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
1 file changed, 20 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c b/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
index 0c2418a..f52091b 100644
--- a/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
+++ b/src/mainboard/siemens/mc_apl1/variants/mc_apl2/mainboard.c
@@ -23,6 +23,11 @@
 #include <timer.h>
 #include <timestamp.h>
 #include <baseboard/variants.h>
+#include <soc/pci_devs.h>
+
+#define SD_CAP_BYP		0x810
+#define  SD_CAP_BYP_EN		0x5A
+#define SD_CAP_BYP_REG1		0x814

void variant_mainboard_final(void)
 {
@@ -36,6 +41,21 @@
    	cmd |= PCI_COMMAND_MASTER;
    	pci_write_config16(dev, PCI_COMMAND, cmd);
    }
+
+	/* Reduce SD-Card speed to DDR50 because of PCB constraints. */
+	dev = pcidev_path_on_root(PCH_DEVFN_SDCARD);
+	if (dev) {
+		uint32_t reg;
+		struct resource *res = find_resource(dev, PCI_BASE_ADDRESS_0);
+		if (!res)
+			return;
+
+		write32(res2mmio(res, SD_CAP_BYP, 0), SD_CAP_BYP_EN);
+		reg = read32(res2mmio(res, SD_CAP_BYP_REG1, 0));
+		/* Disable HS400 and SDR104, keep SDR50 and DDR50 modes. */
+		reg &= ~0x20005800;
+		write32(res2mmio(res, SD_CAP_BYP_REG1, 0), reg);
+	}
 }

static void wait_for_legacy_dev(void *unused)