Peter Lemenkov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
mb/lenovo/t420/devicetree: Use subsystemid inheritance
Change-Id: Ia321f2b974539ac1684173d767dd9eb64060364a Signed-off-by: Peter Lemenkov lemenkov@gmail.com --- M src/mainboard/lenovo/t420/devicetree.cb 1 file changed, 14 insertions(+), 36 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/84/37284/1
diff --git a/src/mainboard/lenovo/t420/devicetree.cb b/src/mainboard/lenovo/t420/devicetree.cb index 6deff60..53bd16f 100644 --- a/src/mainboard/lenovo/t420/devicetree.cb +++ b/src/mainboard/lenovo/t420/devicetree.cb @@ -37,13 +37,11 @@ register "pci_mmio_size" = "2048"
device domain 0 on - device pci 00.0 on - subsystemid 0x17aa 0x21ce - end # host bridge + subsystemid 0x17aa 0x21ce inherit + + device pci 00.0 on end # host bridge device pci 01.0 on end # PCIe Bridge for discrete graphics - device pci 02.0 on - subsystemid 0x17aa 0x21ce - end # Integrated Graphics Controller + device pci 02.0 on end # Integrated Graphics Controller
chip southbridge/intel/bd82x6x # Intel Series 6 Cougar Point PCH # GPI routing @@ -67,6 +65,7 @@
# Enable zero-based linear PCIe root port functions register "pcie_port_coalesce" = "1" + register "c2_latency" = "101" # c2 not supported
# device specific SPI configuration @@ -77,46 +76,30 @@ device pci 16.1 off end # Management Engine Interface 2 device pci 16.2 off end # Management Engine IDE-R device pci 16.3 off end # Management Engine KT - device pci 19.0 on - subsystemid 0x17aa 0x21ce - end # Intel Gigabit Ethernet - device pci 1a.0 on - subsystemid 0x17aa 0x21ce - end # USB Enhanced Host Controller #2 - device pci 1b.0 on - subsystemid 0x17aa 0x21ce - end # High Definition Audio Controller + device pci 19.0 on end # Intel Gigabit Ethernet + device pci 1a.0 on end # USB Enhanced Host Controller #2 + device pci 1b.0 on end # High Definition Audio Controller device pci 1c.0 off end # PCIe Port #1 - device pci 1c.1 on - subsystemid 0x17aa 0x21ce - end # PCIe Port #2 Integrated Wireless LAN + device pci 1c.1 on end # PCIe Port #2 Integrated Wireless LAN device pci 1c.2 off end # PCIe Port #3 device pci 1c.3 on - subsystemid 0x17aa 0x21ce smbios_slot_desc "7" "3" "ExpressCard Slot" "8" end # PCIe Port #4 ExpressCard device pci 1c.4 on - subsystemid 0x17aa 0x21ce chip drivers/ricoh/rce822 register "sdwppol" = "1" register "disable_mask" = "0x87" - device pci 00.0 on - subsystemid 0x17aa 0x21ce - end + device pci 00.0 on end end end # PCIe Port #5 (Ricoh SD & FW) device pci 1c.5 off end # PCIe Port #6 Intel Gigabit Ethernet PHY (not PCIe) device pci 1c.6 off end # PCIe Port #7 device pci 1c.7 off end # PCIe Port #8 - device pci 1d.0 on - subsystemid 0x17aa 0x21ce - end # USB Enhanced Host Controller #1 + device pci 1d.0 on end # USB Enhanced Host Controller #1 device pci 1e.0 off end # PCI bridge device pci 1f.0 on - subsystemid 0x17aa 0x21ce chip ec/lenovo/pmh7 - device pnp ff.1 on # dummy - end + device pnp ff.1 on end # dummy register "backlight_enable" = "0x01" register "dock_event_enable" = "0x01" end @@ -176,11 +159,8 @@ register "has_thinker1" = "1" end end # LPC Controller - device pci 1f.2 on - subsystemid 0x17aa 0x21ce - end # 6 port SATA AHCI Controller + device pci 1f.2 on end # 6 port SATA AHCI Controller device pci 1f.3 on - subsystemid 0x17aa 0x21ce # eeprom, 8 virtual devices, same chip chip drivers/i2c/at24rf08c device i2c 54 on end @@ -194,9 +174,7 @@ end end # SMBus Controller device pci 1f.5 off end # SATA Controller 2 - device pci 1f.6 on - subsystemid 0x17aa 0x21ce - end # Thermal + device pci 1f.6 on end # Thermal end end end
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... File src/mainboard/lenovo/t420/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... PS1, Line 79: device pci 19.0 on end # Intel Gigabit Ethernet Nit: add a space after each `on` so that all the instances of `end` are aligned
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
Patch Set 1: Code-Review+2
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... File src/mainboard/lenovo/t420/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... PS1, Line 79: device pci 19.0 on end # Intel Gigabit Ethernet
Nit: add a space after each `on` so that all the instances of `end` are aligned
I don't see what's to do here.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
Patch Set 1: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... File src/mainboard/lenovo/t420/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... PS1, Line 79: device pci 19.0 on end # Intel Gigabit Ethernet
I don't see what's to do here.
And why is it a reason to mark this as resolved?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... File src/mainboard/lenovo/t420/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... PS1, Line 79: device pci 19.0 on end # Intel Gigabit Ethernet
And why is it a reason to mark this as resolved?
We have an informal style guide that the "end" statements in such single line entries should be aligned. See $ git grep "on end" # two spaces
Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... File src/mainboard/lenovo/t420/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... PS1, Line 79: device pci 19.0 on end # Intel Gigabit Ethernet
We have an informal style guide that the "end" statements in such single line entries should be alig […]
looks like that isn't documented and linters doesn't complain. marking as invalid until proper documentation and linting exists.
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... File src/mainboard/lenovo/t420/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... PS1, Line 79: device pci 19.0 on end # Intel Gigabit Ethernet
looks like that isn't documented and linters doesn't complain. […]
so line #42, 43 and 44 needs two spaces? :)
I will not block this for one more whitespace
Peter Lemenkov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... File src/mainboard/lenovo/t420/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... PS1, Line 79: device pci 19.0 on end # Intel Gigabit Ethernet
so line #42, 43 and 44 needs two spaces? :) […]
Let's do it in (whitespace alignment) a separate commit later. I'd rather not to do it now especially considering that the majority of "on end #" has just one space.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
mb/lenovo/t420/devicetree: Use subsystemid inheritance
Change-Id: Ia321f2b974539ac1684173d767dd9eb64060364a Signed-off-by: Peter Lemenkov lemenkov@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/37284 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: HAOUAS Elyes ehaouas@noos.fr Reviewed-by: Patrick Rudolph siro@das-labor.org --- M src/mainboard/lenovo/t420/devicetree.cb 1 file changed, 14 insertions(+), 36 deletions(-)
Approvals: build bot (Jenkins): Verified HAOUAS Elyes: Looks good to me, approved Patrick Rudolph: Looks good to me, approved
diff --git a/src/mainboard/lenovo/t420/devicetree.cb b/src/mainboard/lenovo/t420/devicetree.cb index 6deff60..53bd16f 100644 --- a/src/mainboard/lenovo/t420/devicetree.cb +++ b/src/mainboard/lenovo/t420/devicetree.cb @@ -37,13 +37,11 @@ register "pci_mmio_size" = "2048"
device domain 0 on - device pci 00.0 on - subsystemid 0x17aa 0x21ce - end # host bridge + subsystemid 0x17aa 0x21ce inherit + + device pci 00.0 on end # host bridge device pci 01.0 on end # PCIe Bridge for discrete graphics - device pci 02.0 on - subsystemid 0x17aa 0x21ce - end # Integrated Graphics Controller + device pci 02.0 on end # Integrated Graphics Controller
chip southbridge/intel/bd82x6x # Intel Series 6 Cougar Point PCH # GPI routing @@ -67,6 +65,7 @@
# Enable zero-based linear PCIe root port functions register "pcie_port_coalesce" = "1" + register "c2_latency" = "101" # c2 not supported
# device specific SPI configuration @@ -77,46 +76,30 @@ device pci 16.1 off end # Management Engine Interface 2 device pci 16.2 off end # Management Engine IDE-R device pci 16.3 off end # Management Engine KT - device pci 19.0 on - subsystemid 0x17aa 0x21ce - end # Intel Gigabit Ethernet - device pci 1a.0 on - subsystemid 0x17aa 0x21ce - end # USB Enhanced Host Controller #2 - device pci 1b.0 on - subsystemid 0x17aa 0x21ce - end # High Definition Audio Controller + device pci 19.0 on end # Intel Gigabit Ethernet + device pci 1a.0 on end # USB Enhanced Host Controller #2 + device pci 1b.0 on end # High Definition Audio Controller device pci 1c.0 off end # PCIe Port #1 - device pci 1c.1 on - subsystemid 0x17aa 0x21ce - end # PCIe Port #2 Integrated Wireless LAN + device pci 1c.1 on end # PCIe Port #2 Integrated Wireless LAN device pci 1c.2 off end # PCIe Port #3 device pci 1c.3 on - subsystemid 0x17aa 0x21ce smbios_slot_desc "7" "3" "ExpressCard Slot" "8" end # PCIe Port #4 ExpressCard device pci 1c.4 on - subsystemid 0x17aa 0x21ce chip drivers/ricoh/rce822 register "sdwppol" = "1" register "disable_mask" = "0x87" - device pci 00.0 on - subsystemid 0x17aa 0x21ce - end + device pci 00.0 on end end end # PCIe Port #5 (Ricoh SD & FW) device pci 1c.5 off end # PCIe Port #6 Intel Gigabit Ethernet PHY (not PCIe) device pci 1c.6 off end # PCIe Port #7 device pci 1c.7 off end # PCIe Port #8 - device pci 1d.0 on - subsystemid 0x17aa 0x21ce - end # USB Enhanced Host Controller #1 + device pci 1d.0 on end # USB Enhanced Host Controller #1 device pci 1e.0 off end # PCI bridge device pci 1f.0 on - subsystemid 0x17aa 0x21ce chip ec/lenovo/pmh7 - device pnp ff.1 on # dummy - end + device pnp ff.1 on end # dummy register "backlight_enable" = "0x01" register "dock_event_enable" = "0x01" end @@ -176,11 +159,8 @@ register "has_thinker1" = "1" end end # LPC Controller - device pci 1f.2 on - subsystemid 0x17aa 0x21ce - end # 6 port SATA AHCI Controller + device pci 1f.2 on end # 6 port SATA AHCI Controller device pci 1f.3 on - subsystemid 0x17aa 0x21ce # eeprom, 8 virtual devices, same chip chip drivers/i2c/at24rf08c device i2c 54 on end @@ -194,9 +174,7 @@ end end # SMBus Controller device pci 1f.5 off end # SATA Controller 2 - device pci 1f.6 on - subsystemid 0x17aa 0x21ce - end # Thermal + device pci 1f.6 on end # Thermal end end end
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37284 )
Change subject: mb/lenovo/t420/devicetree: Use subsystemid inheritance ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... File src/mainboard/lenovo/t420/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37284/1/src/mainboard/lenovo/t420/d... PS1, Line 79: device pci 19.0 on end # Intel Gigabit Ethernet
Let's do it in (whitespace alignment) a separate commit later. […]
I was mostly commenting on where this request came from. I agree that a cleanup pass should be global and ideally be accompanied by some lint support (although that will be complicated to implement since it's context dependent)
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Peter Lemenkov (Code Review)