Change in coreboot[master]: mb/google/drallion: Set cpu id to EC

[EricR Lai (Code Review)]

EricR Lai has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: mb/google/drallion: Set cpu id to EC
......................................................................

mb/google/drallion: Set cpu id to EC

Set CPU ID and cores to EC then EC will according to different
CPU to set different power table.

BUG=b:148126144
BRANCH=None
TEST=check EC can get correct CPU id and cores.

Signed-off-by: Eric Lai 
Change-Id: I23f5580b15a20a01e03a5f4c798e73574f874c9a
---
M src/mainboard/google/drallion/ramstage.c
1 file changed, 21 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/38566/1

diff --git a/src/mainboard/google/drallion/ramstage.c b/src/mainboard/google/drallion/ramstage.c
index 3855045..daee09d 100644
--- a/src/mainboard/google/drallion/ramstage.c
+++ b/src/mainboard/google/drallion/ramstage.c
@@ -14,14 +14,19 @@
  */

#include <arch/acpi.h>
+#include <arch/cpu.h>
 #include <boardid.h>
+#include <device/pci_ids.h>
+#include <device/pci_ops.h>
 #include <drivers/vpd/vpd.h>
+#include <ec/google/wilco/commands.h>
 #include <smbios.h>
 #include <soc/gpio.h>
 #include <soc/ramstage.h>
 #include <variant/gpio.h>
 #include <vendorcode/google/chromeos/chromeos.h>

+
 #define VPD_KEY_SYSTEM_SERIAL		"serial_number"
 #define VPD_KEY_MAINBOARD_SERIAL	"mlb_serial_number"
 #define VPD_SERIAL_LEN			64
@@ -59,6 +64,20 @@
    }
 }

+/* set cpu id to EC, default cores is 4 */
+static int set_cpu_id(void)
+{
+	uint16_t mch_id = 0;
+	uint8_t cpu_cores = 4;
+	const struct device *dev = pcidev_path_on_root(SA_DEVFN_ROOT);
+
+	mch_id = dev ? pci_read_config16(dev, PCI_DEVICE_ID) : 0xffff;
+	if (mch_id == PCI_DEVICE_ID_INTEL_CML_ULT_2_2)
+		cpu_cores = 2;
+
+	return wilco_ec_set_cpuid(cpu_get_cpuid(), cpu_cores, 0);
+}
+
 static void mainboard_init(void *chip_info)
 {
    const struct pad_config *gpio_table;
@@ -66,6 +85,8 @@

gpio_table = variant_gpio_table(&num_gpios);
    cnl_configure_pads(gpio_table, num_gpios);
+	if (set_cpu_id())
+		printk(BIOS_ERR, "wilco set cpu id failed\n");
 }

static void mainboard_enable(struct device *dev)

[EricR Lai (Code Review)]

Hello Selma Bensaid, Mathew King, Bora Guvendik,

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

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

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

Change subject: mb/google/drallion: Set cpu id to EC
......................................................................

mb/google/drallion: Set cpu id to EC

Set CPU ID and cores to EC then EC will according to different
CPU to set different power table.

BUG=b:148126144
BRANCH=None
TEST=check EC can get correct CPU id and cores.

Signed-off-by: Eric Lai 
Change-Id: I23f5580b15a20a01e03a5f4c798e73574f874c9a
---
M src/mainboard/google/drallion/ramstage.c
1 file changed, 20 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/38566/2

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: mb/google/drallion: Set cpu id to EC
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
File src/mainboard/google/drallion/ramstage.c:

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
PS2, Line 70: cpu_cores
Would get_cpu_topology() from <intelblocks/cpulib.h> work here?

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: mb/google/drallion: Set cpu id to EC
......................................................................

Patch Set 2:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
File src/mainboard/google/drallion/ramstage.c:

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
PS2, Line 69: 	uint16_t mch_id = 0;
Initialization not needed.

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
PS2, Line 88: 		printk(BIOS_ERR, "wilco set cpu id failed\n");
Error messages should be more elaborate for users.
> Failed to set CPU ID in Wilco EC. Only four cores might be used.

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: mb/google/drallion: Set cpu id to EC
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
File src/mainboard/google/drallion/ramstage.c:

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
PS2, Line 70: cpu_cores
> Would get_cpu_topology() from <intelblocks/cpulib. […]

I can find this function. Only have cpu_read_topology, but this function return bool for real and virtual cores are equal or not.

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: mb/google/drallion: Set cpu id to EC
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
File src/mainboard/google/drallion/ramstage.c:

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
PS2, Line 70: cpu_cores
> I can find this function. […]

Oh, I can pass the pointer to get the cores. I will check it, thanks.

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[EricR Lai (Code Review)]

Hello Selma Bensaid, Mathew King, Bora Guvendik, build bot (Jenkins),

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

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

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

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

ec/google/wilco: Set cpu id and cores to EC

Set CPU ID and cores to EC then EC will according to different
CPU to set different power table.

BUG=b:148126144
BRANCH=None
TEST=check EC can get correct CPU id and cores.

Signed-off-by: Eric Lai 
Change-Id: I23f5580b15a20a01e03a5f4c798e73574f874c9a
---
M src/ec/google/wilco/chip.c
1 file changed, 15 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/38566/3

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

Patch Set 3:

(3 comments)

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
File src/mainboard/google/drallion/ramstage.c:

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
PS2, Line 69: 	uint16_t mch_id = 0;
> Initialization not needed.

Done

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
PS2, Line 70: cpu_cores
> Oh, I can pass the pointer to get the cores. I will check it, thanks.

This function is works. And the virtual one will include all cores, ex: 4+2 will get 8 cores.

https://review.coreboot.org/c/coreboot/+/38566/2/src/mainboard/google/dralli... 
PS2, Line 88: 		printk(BIOS_ERR, "wilco set cpu id failed\n");
> Error messages should be more elaborate for users. […]

Done

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38566/3/src/ec/google/wilco/chip.c 
File src/ec/google/wilco/chip.c:

https://review.coreboot.org/c/coreboot/+/38566/3/src/ec/google/wilco/chip.c@... 
PS3, Line 170: 		printk(BIOS_ERR, "EC: use default cpu power table\n");
Use INFO instead ERR is better?

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[Selma Bensaid (Code Review)]

Selma Bensaid has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38566/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38566/2//COMMIT_MSG@9 
PS2, Line 9: EC will according to different
           : CPU to set different power table.
, EC will adapt power table according to the CPU ID and number of cores.

Is think correct Eric?

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[EricR Lai (Code Review)]

Hello Selma Bensaid, Mathew King, Bora Guvendik, build bot (Jenkins),

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

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

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

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

ec/google/wilco: Set cpu id and cores to EC

Set CPU ID and cores to EC then EC will adapt power table
according to the CPU ID and number of cores.

BUG=b:148126144
BRANCH=None
TEST=check EC can get correct CPU id and cores.

Signed-off-by: Eric Lai 
Change-Id: I23f5580b15a20a01e03a5f4c798e73574f874c9a
---
M src/ec/google/wilco/chip.c
1 file changed, 15 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/38566/4

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38566/2//COMMIT_MSG 
Commit Message:

https://review.coreboot.org/c/coreboot/+/38566/2//COMMIT_MSG@9 
PS2, Line 9: EC will according to different
           : CPU to set different power table.
> , EC will adapt power table according to the CPU ID and number of cores. […]

yes, I will fix this grammar. thx

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[EricR Lai (Code Review)]

EricR Lai has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

Patch Set 4:

Please have review, we verified this is works with new EC.

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[Duncan Laurie (Code Review)]

Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[Mathew King (Code Review)]

Mathew King has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: ec/google/wilco: Set cpu id and cores to EC

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38566 )

Change subject: ec/google/wilco: Set cpu id and cores to EC
......................................................................

ec/google/wilco: Set cpu id and cores to EC

Set CPU ID and cores to EC then EC will adapt power table
according to the CPU ID and number of cores.

BUG=b:148126144
BRANCH=None
TEST=check EC can get correct CPU id and cores.

Signed-off-by: Eric Lai 
Change-Id: I23f5580b15a20a01e03a5f4c798e73574f874c9a
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38566
Reviewed-by: Duncan Laurie 
Reviewed-by: Mathew King 
Tested-by: build bot (Jenkins) 
---
M src/ec/google/wilco/chip.c
1 file changed, 15 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Duncan Laurie: Looks good to me, approved
  Mathew King: Looks good to me, approved

diff --git a/src/ec/google/wilco/chip.c b/src/ec/google/wilco/chip.c
index b44cbd6..5729b4a 100644
--- a/src/ec/google/wilco/chip.c
+++ b/src/ec/google/wilco/chip.c
@@ -16,10 +16,13 @@
 #include <arch/acpi.h>
 #include <arch/acpi_device.h>
 #include <arch/acpigen.h>
+#include <arch/cpu.h>
 #include <bootstate.h>
 #include <cbmem.h>
+#include <console/console.h>
 #include <device/pnp.h>
 #include <ec/acpi/ec.h>
+#include <intelblocks/cpulib.h>
 #include <pc80/keyboard.h>
 #include <stdint.h>

@@ -124,6 +127,14 @@
 }
 BOOT_STATE_INIT_ENTRY(BS_OS_RESUME, BS_ON_ENTRY, wilco_ec_resume, NULL);

+static int wilco_set_cpu_id(void)
+{
+	uint32_t cpu_phy_cores, cpu_virtual_cores;
+
+	cpu_read_topology(&cpu_phy_cores, &cpu_virtual_cores);
+	return wilco_ec_set_cpuid(cpu_get_cpuid(), cpu_phy_cores, 0);
+}
+
 static void wilco_ec_init(struct device *dev)
 {
    if (!dev->enabled)
@@ -153,6 +164,10 @@

/* Turn on camera power */
    wilco_ec_send(KB_CAMERA, CAMERA_ON);
+
+	/* Set cpu id and phy cores */
+	if (wilco_set_cpu_id())
+		printk(BIOS_ERR, "EC: use default cpu power table\n");
 }

static void wilco_ec_resource(struct device *dev, int index,