Peichao Wang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43175 )
Change subject: mb/google/dedede/var/boten: Update devicetree ......................................................................
Patch Set 13:
(31 comments)
https://review.coreboot.org/c/coreboot/+/43175/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43175/2//COMMIT_MSG@7 PS2, Line 7: support touchpad for Boten
Update device tree for Boten
Done
https://review.coreboot.org/c/coreboot/+/43175/2//COMMIT_MSG@7 PS2, Line 7: mainboard
Done
https://review.coreboot.org/c/coreboot/+/43175/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43175/4//COMMIT_MSG@7 PS4, Line 7: mb/mainboard/dedede: Update device tree for Boten
mb/google/dedede/var/boten: Update devicetree
Done
https://review.coreboot.org/c/coreboot/+/43175/4//COMMIT_MSG@9 PS4, Line 9: Add device tree for touchpad
Done
Done
https://review.coreboot.org/c/coreboot/+/43175/4//COMMIT_MSG@9 PS4, Line 9: Add device tree for touchpad
It looks to me, you are adding more then just an entry for the touchpad. Please mention everything.
Done
https://review.coreboot.org/c/coreboot/+/43175/9//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/43175/9//COMMIT_MSG@9 PS9, Line 9: Add device tree for touchpad, touch screen, audio jack, enable USB2_[5][6] and I2C5
Maybe: […]
Done
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/boten/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... PS2, Line 41: "USB2_PORT_MID(OC_SKIP)" # Type-C Port C1
For Boten project, there is no C port in the DB. close it.
Done
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... PS2, Line 39: # USB Port Configuration : register "usb2_ports[0]" = "USB2_PORT_MID(OC_SKIP)" # Type-C Port C0 : register "usb2_ports[1]" = "USB2_PORT_MID(OC_SKIP)" # Type-C Port C1 : register "usb2_ports[2]" = "USB2_PORT_MID(OC_SKIP)" # Type-A Port A0 : register "usb2_ports[3]" = "USB2_PORT_MID(OC_SKIP)" # Type-A Port A1 : register "usb2_ports[4]" = "USB2_PORT_MID(OC_SKIP)" # Discrete Bluetooth : register "usb2_ports[5]" = "USB2_PORT_EMPTY" # Not Used : register "usb2_ports[6]" = "USB2_PORT_EMPTY" # Not Used : register "usb2_ports[7]" = "USB2_PORT_MID(OC_SKIP)" # Integrated Bluetooth
No need to re-configure everything in devicetree. […]
Done
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... PS2, Line 50: "USB3_PORT_DEFAULT(OC_SKIP)" # USB3/2 Type-C Port C1
No C port in the DB.
Done
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... PS2, Line 49: register "usb3_ports[0]" = "USB3_PORT_DEFAULT(OC_SKIP)" # USB3/2 Type-C Port C0 : register "usb3_ports[1]" = "USB3_PORT_DEFAULT(OC_SKIP)" # USB3/2 Type-C Port C1 : register "usb3_ports[2]" = "USB3_PORT_DEFAULT(OC_SKIP)" # USB3/1 Type-A Port A0 : register "usb3_ports[3]" = "USB3_PORT_DEFAULT(OC_SKIP)" # USB3/1 Type-A Port A1 : register "usb3_ports[4]" = "USB3_PORT_EMPTY" # Not Used : register "usb3_ports[5]" = "USB3_PORT_EMPTY" # Not Used
No need to re-configure everything in devicetree. […]
Done
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... PS2, Line 75: register "desc" = ""Right Type-C Port""
please close it
Done
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... PS2, Line 114: register "desc" = ""Right Type-C Port""
please close it
Done
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... PS2, Line 142: 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 : 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
Which of these 2 trackpads is used and verified in Boten. […]
Done
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... PS2, Line 161: device pci 15.2 on end # I2C 2
Please add touch screen description.
Done
https://review.coreboot.org/c/coreboot/+/43175/2/src/mainboard/google/dedede... PS2, Line 169: device pci 19.1 off end # I2C 5
Please enable this port since for P sensor
Done
https://review.coreboot.org/c/coreboot/+/43175/5/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/boten/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43175/5/src/mainboard/google/dedede... PS5, Line 12: #| I2C1 | Digitizer |
You can remove it, if Boten doesn't use I2C1.
Done
https://review.coreboot.org/c/coreboot/+/43175/5/src/mainboard/google/dedede... PS5, Line 14: #| I2C3 | Camera |
You can remove it, if Boten doesn't use I2C3.
Done
https://review.coreboot.org/c/coreboot/+/43175/5/src/mainboard/google/dedede... PS5, Line 25: .i2c[1] = { : .speed = I2C_SPEED_FAST, : },
You can remove it, if Boten doesn't use I2C1.
Done
https://review.coreboot.org/c/coreboot/+/43175/5/src/mainboard/google/dedede... PS5, Line 31: .i2c[3] = { : .speed = I2C_SPEED_FAST, : },
You can remove it, if Boten doesn't use I2C3.
Done
https://review.coreboot.org/c/coreboot/+/43175/5/src/mainboard/google/dedede... PS5, Line 47: register "usb2_ports[7]" = "USB2_PORT_MID(OC_SKIP)" # Integrated Bluetooth
As comments in previous runs, some definition like usb2_ports[0-3|7] are already in baseboard/device […]
Done
https://review.coreboot.org/c/coreboot/+/43175/5/src/mainboard/google/dedede... PS5, Line 54: register "usb3_ports[5]" = "USB3_PORT_EMPTY" # Not Used
Same as here and others.
Done
https://review.coreboot.org/c/coreboot/+/43175/5/src/mainboard/google/dedede... PS5, Line 58: [PchSerialIoIndexI2C1] = PchSerialIoPci,
If you don't use I2C1, it can be set to "PchSerialIoDisabled" and set […]
Done
https://review.coreboot.org/c/coreboot/+/43175/5/src/mainboard/google/dedede... PS5, Line 60: [PchSerialIoIndexI2C3] = PchSerialIoPci,
If you don't use I2C3, it can be set to "PchSerialIoDisabled" and set […]
Done
https://review.coreboot.org/c/coreboot/+/43175/8/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/boten/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43175/8/src/mainboard/google/dedede... PS8, Line 42: [PchSerialIoIndexI2C5] = PchSerialIoPci,
As I know, this one will be planed to P-Sensor later.
Done
https://review.coreboot.org/c/coreboot/+/43175/8/src/mainboard/google/dedede... PS8, Line 52: Right Type-A Port
I have other comment to suggest them to be amend for LTE usage.
Done
https://review.coreboot.org/c/coreboot/+/43175/8/src/mainboard/google/dedede... PS8, Line 83: 15
Nit: 0x15. Same for all the I2C addresses below for consistency.
Done
https://review.coreboot.org/c/coreboot/+/43175/9/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/boten/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43175/9/src/mainboard/google/dedede... PS9, Line 56: end
Do we want to change the desc of usb 2. […]
Done
https://review.coreboot.org/c/coreboot/+/43175/9/src/mainboard/google/dedede... PS9, Line 72: end
Do we want to change the desc of usb 3. […]
Done
https://review.coreboot.org/c/coreboot/+/43175/9/src/mainboard/google/dedede... PS9, Line 108: register "generic.enable_delay_ms" = "12"
Report_En(GP_A11) was unused.
Done
https://review.coreboot.org/c/coreboot/+/43175/12/src/mainboard/google/deded... File src/mainboard/google/dedede/variants/boten/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/43175/12/src/mainboard/google/deded... PS12, Line 14: #+-------------------+---------------------------+
Hi Ben, P-sensor CL: 43478 need to be rebased on it.
Done
https://review.coreboot.org/c/coreboot/+/43175/12/src/mainboard/google/deded... PS12, Line 29: }"
Should this be added? or you can add this in https://review.coreboot.org/c/coreboot/+/43478 ? […]
Done