Hello Tim Chen,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/47013
to review the following change.
Change subject: mb/google/dedede/var/metaknight: Add device settings ......................................................................
mb/google/dedede/var/metaknight: Add device settings
Add the configuration in device tree: 1. Add HDA,speaker codec and speaker amp setting 2. Add Elan, Raydium and Goodix touchscreen setting 3. Add user/world facing camera usb setting 4 Add Synaptics and Elan Touchpad setting 5. Add WiFi configuration 6. Add DPTF setting
BUG=b:169813211 BRANCH=None TEST=build metaknight firmware
Change-Id: Ie034724ea5c6edff6cdba694605d3238f66be910 Signed-off-by: Tim Chen tim-chen@quanta.corp-partner.google.com --- M src/mainboard/google/dedede/variants/metaknight/overridetree.cb 1 file changed, 175 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/13/47013/1
diff --git a/src/mainboard/google/dedede/variants/metaknight/overridetree.cb b/src/mainboard/google/dedede/variants/metaknight/overridetree.cb index 404024b..af7de71 100644 --- a/src/mainboard/google/dedede/variants/metaknight/overridetree.cb +++ b/src/mainboard/google/dedede/variants/metaknight/overridetree.cb @@ -1,5 +1,9 @@ chip soc/intel/jasperlake
+ # USB Port Configuration + register "usb2_ports[5]" = "USB2_PORT_MID(OC_SKIP)" # Camera + register "usb2_ports[6]" = "USB2_PORT_MID(OC_SKIP)" # World Facing Camera + # Intel Common SoC Config #+-------------------+---------------------------+ #| Field | Value | @@ -11,7 +15,7 @@ #| I2C0 | Trackpad | #| I2C1 | Digitizer | #| I2C2 | Touchscreen | - #| I2C3 | Camera | + #| I2C3 | TBD | #| I2C4 | Audio | #+-------------------+---------------------------+ register "common_soc_config" = "{ @@ -36,7 +40,176 @@ }, }"
+ register "power_limits_config" = "{ + .tdp_pl1_override = 7, + .tdp_pl2_override = 12, + }" + + register "tcc_offset" = "15" # TCC of 90C + device domain 0 on - device pci 15.0 on end + device pci 04.0 on + chip drivers/intel/dptf + register "options.tsr[0].desc" = ""Memory"" + register "options.tsr[1].desc" = ""Ambient"" + + register "policies.passive[0]" = "DPTF_PASSIVE(CPU, CPU, 90, 5000)" + register "policies.passive[1]" = "DPTF_PASSIVE(CPU, TEMP_SENSOR_0, 70, 6000)" + register "policies.passive[2]" = "DPTF_PASSIVE(CPU, TEMP_SENSOR_1, 60, 5000)" + + register "policies.critical[0]" = "DPTF_CRITICAL(CPU, 105, SHUTDOWN)" + register "policies.critical[1]" = "DPTF_CRITICAL(TEMP_SENSOR_0, 80, SHUTDOWN)" + register "policies.critical[2]" = "DPTF_CRITICAL(TEMP_SENSOR_1, 80, SHUTDOWN)" + + register "controls.power_limits.pl1" = "{ + .min_power = 3000, + .max_power = 7000, + .time_window_min = 1 * MSECS_PER_SEC, + .time_window_max = 1 * MSECS_PER_SEC, + .granularity = 200,}" + register "controls.power_limits.pl2" = "{ + .min_power = 7000, + .max_power = 12000, + .time_window_min = 1 * MSECS_PER_SEC, + .time_window_max = 1 * MSECS_PER_SEC, + .granularity = 1000,}" + + ## Charger Performance Control (Control, mA) + register "controls.charger_perf[0]" = "{ 255, 3000 }" + register "controls.charger_perf[1]" = "{ 24, 1500 }" + register "controls.charger_perf[2]" = "{ 16, 1000 }" + register "controls.charger_perf[3]" = "{ 8, 500 }" + + device generic 0 on end + end + end # SA Thermal device + device pci 14.0 on + chip drivers/usb/acpi + device usb 0.0 on + chip drivers/usb/acpi + register "desc" = ""Camera"" + register "type" = "UPC_TYPE_INTERNAL" + device usb 2.5 on end + end + chip drivers/usb/acpi + register "desc" = ""Camera"" + register "type" = "UPC_TYPE_INTERNAL" + device usb 2.6 on end + end + end + end + end # USB xHCI + device pci 15.0 on + chip drivers/i2c/hid + register "generic.hid" = ""PNP0C50"" + register "generic.desc" = ""Synaptics Touchpad"" + register "generic.irq" = "ACPI_IRQ_WAKE_EDGE_LOW(GPP_B3_IRQ)" + register "generic.wake" = "GPE0_DW0_03" + register "generic.probed" = "1" + register "hid_desc_reg_offset" = "0x20" + device i2c 0x2c on end + end + chip drivers/i2c/generic + register "hid" = ""ELAN0000"" + register "desc" = ""ELAN Touchpad"" + register "irq" = "ACPI_IRQ_WAKE_EDGE_LOW(GPP_B3_IRQ)" + register "wake" = "GPE0_DW0_03" + register "probed" = "1" + device i2c 15 on end + end + end # I2C 0 + device pci 15.2 on + chip drivers/i2c/hid + register "generic.hid" = ""GDIX0000"" + register "generic.desc" = ""Goodix Touchscreen"" + register "generic.irq" = "ACPI_IRQ_EDGE_LOW(GPP_D4_IRQ)" + register "generic.probed" = "1" + register "generic.reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D5)" + register "generic.reset_delay_ms" = "120" + register "generic.reset_off_delay_ms" = "2" + register "generic.enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_D6)" + register "generic.enable_delay_ms" = "12" + register "generic.has_power_resource" = "1" + register "generic.disable_gpio_export_in_crs" = "1" + register "hid_desc_reg_offset" = "0x01" + device i2c 0x5d on end + end + chip drivers/i2c/hid + register "generic.hid" = ""ELAN6915"" + register "generic.desc" = ""ELAN Touchscreen"" + register "generic.irq" = "ACPI_IRQ_EDGE_LOW(GPP_D4_IRQ)" + register "generic.probed" = "1" + register "generic.reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D5)" + register "generic.reset_delay_ms" = "20" + register "generic.reset_off_delay_ms" = "2" + register "generic.stop_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_A11)" + register "generic.stop_delay_ms" = "280" + register "generic.stop_off_delay_ms" = "2" + register "generic.enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_D6)" + register "generic.enable_delay_ms" = "1" + register "generic.has_power_resource" = "1" + register "generic.disable_gpio_export_in_crs" = "1" + register "hid_desc_reg_offset" = "0x01" + device i2c 15 on end + end + chip drivers/i2c/generic + register "hid" = ""ELAN0001"" + register "desc" = ""ELAN Touchscreen"" + register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_D4_IRQ)" + register "probed" = "1" + register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D5)" + register "reset_delay_ms" = "20" + register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_D6)" + register "enable_delay_ms" = "1" + register "has_power_resource" = "1" + device i2c 10 on end + end + chip drivers/i2c/generic + register "hid" = ""RAYD0001"" + register "desc" = ""Raydium Touchscreen"" + register "irq" = "ACPI_IRQ_EDGE_LOW(GPP_D4_IRQ)" + register "probed" = "1" + register "reset_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_LOW(GPP_D5)" + register "reset_delay_ms" = "1" + register "enable_gpio" = "ACPI_GPIO_OUTPUT_ACTIVE_HIGH(GPP_D6)" + register "enable_delay_ms" = "50" + register "has_power_resource" = "1" + register "disable_gpio_export_in_crs" = "1" + device i2c 39 on end + end + end # I2C 2 + device pci 15.3 off end # I2C 3 + device pci 19.0 on + chip drivers/i2c/generic + register "hid" = ""10EC5682"" + register "name" = ""RT58"" + register "desc" = ""Realtek RT5682"" + register "irq_gpio" = "ACPI_GPIO_IRQ_EDGE_BOTH(GPP_D16)" + register "property_count" = "1" + register "property_list[0].type" = "ACPI_DP_TYPE_INTEGER" + register "property_list[0].name" = ""realtek,jd-src"" + register "property_list[0].integer" = "1" + device i2c 1a on end + end + chip drivers/i2c/generic + register "hid" = ""10EC1015"" + register "desc" = ""Realtek SPK AMP L"" + register "uid" = "0" + device i2c 28 on end + end + chip drivers/i2c/generic + register "hid" = ""10EC1015"" + register "desc" = ""Realtek SPK AMP R"" + register "uid" = "1" + device i2c 29 on end + end + end + device pci 1f.3 on end # Intel HDA + device pci 1c.7 on + chip drivers/wifi/generic + register "wake" = "GPE0_DW2_03" + device pci 00.0 on end + end + end # PCI Express Root Port 8 - WLAN end end
Paul Fagerburg has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47013 )
Change subject: mb/google/dedede/var/metaknight: Add device settings ......................................................................
Patch Set 1: Code-Review+1
Tim Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47013 )
Change subject: mb/google/dedede/var/metaknight: Add device settings ......................................................................
Patch Set 1:
Hi Google team, Is there any concerns on this CL? If not could you help to +2? Thank you.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47013 )
Change subject: mb/google/dedede/var/metaknight: Add device settings ......................................................................
Patch Set 1:
(8 comments)
https://review.coreboot.org/c/coreboot/+/47013/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47013/1//COMMIT_MSG@9 PS1, Line 9: Add the configuration in device tree: I would recommend splitting each device configuration into a CL of their own.
https://review.coreboot.org/c/coreboot/+/47013/1//COMMIT_MSG@14 PS1, Line 14: 5. Add WiFi configuration Is metaknight using Discrete WiFi or CNVi?
https://review.coreboot.org/c/coreboot/+/47013/1//COMMIT_MSG@15 PS1, Line 15: 6. Add DPTF setting Is the DPTF value tuned already. If not, I would recommend you to drop it and use the baseboard DPTF configuration for now. Once you have the tuned value, it can be added.
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/metaknight/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... PS1, Line 35: .i2c[3] = { : .speed = I2C_SPEED_FAST, : }, Can be removed.
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... PS1, Line 41: }" Disable I2C3
register "SerialIoI2cMode" = "{ [PchSerialIoIndexI2C0] = PchSerialIoPci, [PchSerialIoIndexI2C1] = PchSerialIoPci, [PchSerialIoIndexI2C2] = PchSerialIoPci, [PchSerialIoIndexI2C3] = PchSerialIoDisabled, [PchSerialIoIndexI2C4] = PchSerialIoPci, [PchSerialIoIndexI2C5] = PchSerialIoPci, }"
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... PS1, Line 90: Camera User Facing Camera
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... PS1, Line 95: Camera World Facing Camera
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... PS1, Line 106: "ACPI_IRQ_WAKE_EDGE_LOW(GPP_B3_IRQ)" We recently learnt that HID over I2C uses Level triggered IRQs. So needs to be fixed. Same for all the I2C Human Interface Devices.
Tim Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47013 )
Change subject: mb/google/dedede/var/metaknight: Add device settings ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47013/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47013/1//COMMIT_MSG@14 PS1, Line 14: 5. Add WiFi configuration
Is metaknight using Discrete WiFi or CNVi?
metaknight is using CNVi.
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/metaknight/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... PS1, Line 106: "ACPI_IRQ_WAKE_EDGE_LOW(GPP_B3_IRQ)"
We recently learnt that HID over I2C uses Level triggered IRQs. So needs to be fixed. […]
I should change this to ACPI_IRQ_WAKE_LEVEL_LOW(GPP_B3_IRQ) right?
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47013 )
Change subject: mb/google/dedede/var/metaknight: Add device settings ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47013/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/47013/1//COMMIT_MSG@14 PS1, Line 14: 5. Add WiFi configuration
metaknight is using CNVi.
The WiFi configuration that you added in overridetree.cb is for discrete WiFi and is not required. CNVi configuration is already part of baseboard devicetree.cb.
Also please disable the USB2 port i.e. port 2.4 which is being used for Discrete Bluetooth.
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/metaknight/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... PS1, Line 106: "ACPI_IRQ_WAKE_EDGE_LOW(GPP_B3_IRQ)"
I should change this to ACPI_IRQ_WAKE_LEVEL_LOW(GPP_B3_IRQ) right?
Yes. For ELAN0000 Touchpad, since it is not a HID over I2C device please consult the data sheet for any recommendations.
Similarly for the Touchscreen devices, specifically for the HID over I2C, configure the IRQ lines as level triggered i.e. register "generic.irq" = "ACPI_IRQ_LEVEL_LOW(GPP_D4_IRQ)"
Sumeet R Pawnikar has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47013 )
Change subject: mb/google/dedede/var/metaknight: Add device settings ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/metaknight/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... PS1, Line 56: register "policies.passive[0]" = "DPTF_PASSIVE(CPU, CPU, 90, 5000)" : register "policies.passive[1]" = "DPTF_PASSIVE(CPU, TEMP_SENSOR_0, 70, 6000)" : register "policies.passive[2]" = "DPTF_PASSIVE(CPU, TEMP_SENSOR_1, 60, 5000)" Can you please update above passive policies in this below structured manner,
register "policies.passive" = "{ [0] = DPTF_PASSIVE(CPU, CPU, 90, 5000), [1] = DPTF_PASSIVE(CPU, TEMP_SENSOR_0, 70, 6000), [2] = DPTF_PASSIVE(CPU, TEMP_SENSOR_1, 60, 5000)}"
https://review.coreboot.org/c/coreboot/+/47013/1/src/mainboard/google/dedede... PS1, Line 60: register "policies.critical[0]" = "DPTF_CRITICAL(CPU, 105, SHUTDOWN)" : register "policies.critical[1]" = "DPTF_CRITICAL(TEMP_SENSOR_0, 80, SHUTDOWN)" : register "policies.critical[2]" = "DPTF_CRITICAL(TEMP_SENSOR_1, 80, SHUTDOWN)" Here also the same way as per above comment,
register "policies.critical" = "{ [0] = DPTF_CRITICAL(CPU, 105, SHUTDOWN), [1] = DPTF_CRITICAL(TEMP_SENSOR_0, 80, SHUTDOWN), [2] = DPTF_CRITICAL(TEMP_SENSOR_1, 80, SHUTDOWN)}"
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/47013?usp=email )
Change subject: mb/google/dedede/var/metaknight: Add device settings ......................................................................
Abandoned
-
Karthik Ramasubramanian (Code Review) -
Paul Fagerburg (Code Review)
-
Stefan Reinauer (Code Review)
-
Sumeet R Pawnikar (Code Review)
-
Tim Chen (Code Review)