Hello Matthew Garrett,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/39567
to review the following change.
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
ec/51nb: add support for NPCE985LA0DX EC
Add support for the NPCE985LA0DX, as used on the 51NB X210 (to be added in a follow-on commit, and from which this was extracted).
Original source: https://review.coreboot.org/c/coreboot/+/32531/37
Change-Id: I5798fad7fd18083cde1aa647fd91ca9c5ce963b7 Signed-off-by: Matt DeVillier matt.devillier@gmail.com Signed-off-by: Matthew Garrett mjg59@google.com --- A src/ec/51nb/npce985la0dx/Kconfig A src/ec/51nb/npce985la0dx/Makefile.inc A src/ec/51nb/npce985la0dx/npce985la0dx.c 3 files changed, 111 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/39567/1
diff --git a/src/ec/51nb/npce985la0dx/Kconfig b/src/ec/51nb/npce985la0dx/Kconfig new file mode 100644 index 0000000..81c26da --- /dev/null +++ b/src/ec/51nb/npce985la0dx/Kconfig @@ -0,0 +1,40 @@ +## +## This file is part of the coreboot project. +## +## Copyright (C) 2019 Google LLC +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; version 2 of the License. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +config EC_51NB_NPCE985LA0DX + bool + default n + help + Support for the 51NB NPCE985LA0DX EC + +if EC_51NB_NPCE985LA0DX + +comment "Please select the following otherwise your laptop cannot be powered on." + +config EC_51NB_NPCE985LA0DX_FIRMWARE + bool "Add firmware image for 51NB NPCE985LA0DX EC" + depends on EC_51NB_NPCE985LA0DX + default n + help + Select this option to add the firmware blob for the 51NB EC. + You need this blob to power on your machine. + +config EC_51NB_NPCE985LA0DX_FW + string "51NB EC firmware path" + depends on EC_51NB_NPCE985LA0DX_FIRMWARE + default "ec.bin" + help + The path and filename of the file to use as 51NB firmware. +endif diff --git a/src/ec/51nb/npce985la0dx/Makefile.inc b/src/ec/51nb/npce985la0dx/Makefile.inc new file mode 100644 index 0000000..99208d7 --- /dev/null +++ b/src/ec/51nb/npce985la0dx/Makefile.inc @@ -0,0 +1,35 @@ +## +## This file is part of the coreboot project. +## +## Copyright (C) 2017 Google LLC +## +## This program is free software; you can redistribute it and/or modify +## it under the terms of the GNU General Public License as published by +## the Free Software Foundation; version 2 of the License. +## +## This program is distributed in the hope that it will be useful, +## but WITHOUT ANY WARRANTY; without even the implied warranty of +## MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +## GNU General Public License for more details. +## + +ifeq ($(CONFIG_EC_51NB_NPCE985LA0DX),y) + +files_added:: +ifeq ($(CONFIG_EC_51NB_NPCE985LA0DX_FIRMWARE),y) + $(CBFSTOOL) $(obj)/coreboot.rom write -r EC -f $(CONFIG_EC_51NB_NPCE985LA0DX_FW) --fill-upward +endif + +build_complete:: +ifeq ($(CONFIG_EC_51NB_NPCE985LA0DX_FIRMWARE),) + printf "\n** WARNING **\n" + printf "You haven't added the firmware blobs for 51NB EC.\n" + printf "You may be unable to power on your laptop without these blobs.\n" + printf "Please select the following option to add them:\n\n" + printf " Chipset --->\n" + printf " [*] Add firmware images for 51NB EC\n\n" +endif + +ramstage-y += npce985la0dx.c + +endif diff --git a/src/ec/51nb/npce985la0dx/npce985la0dx.c b/src/ec/51nb/npce985la0dx/npce985la0dx.c new file mode 100644 index 0000000..7339195 --- /dev/null +++ b/src/ec/51nb/npce985la0dx/npce985la0dx.c @@ -0,0 +1,36 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2019 Google LLC + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <device/pnp.h> + +/* + * This embedded controller looks awfully like a SuperIO chip. LDNs 5 and 6 + * need to be enabled to turn on the keyboard and mouse controller, and LDN + * 0x11 needs to be enabled to turn on ACPI embedded controller + * functionality. + */ +static struct pnp_info dev_infos[] = { + { NULL, 0x05 }, { NULL, 0x06 }, { NULL, 0x11 } +}; + +static void ec_51nb_npce985la0dx_ops_enable(struct device *dev) +{ + pnp_enable_devices(dev, &pnp_ops, ARRAY_SIZE(dev_infos), dev_infos); +} + +struct chip_operations ec_51nb_npce985la0dx_ops = { + CHIP_NAME("51NB EC") + .enable_dev = ec_51nb_npce985la0dx_ops_enable, +};
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39567 )
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
Patch Set 1: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39567 )
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
Patch Set 1: Code-Review+1
looks good to me. the sio components of the ec lack some stuff in dev_infos (i'd expect some io and irq settings there), but i'm ok with adding that in a later patch, so let's get this merged for now
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39567 )
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
Patch Set 1: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... File src/ec/51nb/npce985la0dx/npce985la0dx.c:
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... PS1, Line 14: */ Use SPDX header?
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... PS1, Line 19: SuperIO Super I/O
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... PS1, Line 22: * functionality. Fits on the line above.
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Paul Menzel, Angel Pons, Matthew Garrett, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39567
to look at the new patch set (#2).
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
ec/51nb: add support for NPCE985LA0DX EC
Add support for the NPCE985LA0DX, as used on the 51NB X210 (to be added in a follow-on commit, and from which this was extracted).
Original source: https://review.coreboot.org/c/coreboot/+/32531/37
Change-Id: I5798fad7fd18083cde1aa647fd91ca9c5ce963b7 Signed-off-by: Matt DeVillier matt.devillier@gmail.com Signed-off-by: Matthew Garrett mjg59@google.com --- A src/ec/51nb/npce985la0dx/Kconfig A src/ec/51nb/npce985la0dx/Makefile.inc A src/ec/51nb/npce985la0dx/npce985la0dx.c 3 files changed, 110 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/39567/2
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39567 )
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... File src/ec/51nb/npce985la0dx/npce985la0dx.c:
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... PS1, Line 14: */
Use SPDX header?
what's the current guidance on using SPDX headers? Should I go thru and change all of them in this patch series?
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... PS1, Line 19: SuperIO
Super I/O
Done
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... PS1, Line 22: * functionality.
Fits on the line above.
Done
Hello build bot (Jenkins), Patrick Georgi, Martin Roth, Paul Menzel, Angel Pons, Matthew Garrett, Felix Held,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39567
to look at the new patch set (#3).
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
ec/51nb: add support for NPCE985LA0DX EC
Add support for the NPCE985LA0DX, as used on the 51NB X210 (to be added in a follow-on commit, and from which this was extracted).
Original source: https://review.coreboot.org/c/coreboot/+/32531/37
Change-Id: I5798fad7fd18083cde1aa647fd91ca9c5ce963b7 Signed-off-by: Matt DeVillier matt.devillier@gmail.com Signed-off-by: Matthew Garrett mjg59@google.com --- A src/ec/51nb/npce985la0dx/Kconfig A src/ec/51nb/npce985la0dx/Makefile.inc A src/ec/51nb/npce985la0dx/npce985la0dx.c 3 files changed, 74 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/39567/3
Matt DeVillier has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39567 )
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... File src/ec/51nb/npce985la0dx/npce985la0dx.c:
https://review.coreboot.org/c/coreboot/+/39567/1/src/ec/51nb/npce985la0dx/np... PS1, Line 14: */
what's the current guidance on using SPDX headers? Should I go thru and change all of them in this p […]
Done
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39567 )
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
Patch Set 3: Code-Review+2
Felix Held has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39567 )
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
Patch Set 3: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39567 )
Change subject: ec/51nb: add support for NPCE985LA0DX EC ......................................................................
ec/51nb: add support for NPCE985LA0DX EC
Add support for the NPCE985LA0DX, as used on the 51NB X210 (to be added in a follow-on commit, and from which this was extracted).
Original source: https://review.coreboot.org/c/coreboot/+/32531/37
Change-Id: I5798fad7fd18083cde1aa647fd91ca9c5ce963b7 Signed-off-by: Matt DeVillier matt.devillier@gmail.com Signed-off-by: Matthew Garrett mjg59@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39567 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com Reviewed-by: Felix Held felix-coreboot@felixheld.de --- A src/ec/51nb/npce985la0dx/Kconfig A src/ec/51nb/npce985la0dx/Makefile.inc A src/ec/51nb/npce985la0dx/npce985la0dx.c 3 files changed, 74 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Felix Held: Looks good to me, approved Angel Pons: Looks good to me, approved
diff --git a/src/ec/51nb/npce985la0dx/Kconfig b/src/ec/51nb/npce985la0dx/Kconfig new file mode 100644 index 0000000..caa5624 --- /dev/null +++ b/src/ec/51nb/npce985la0dx/Kconfig @@ -0,0 +1,28 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# This file is part of the coreboot project. + +config EC_51NB_NPCE985LA0DX + bool + default n + help + Support for the 51NB NPCE985LA0DX EC + +if EC_51NB_NPCE985LA0DX + +comment "Please select the following otherwise your laptop cannot be powered on." + +config EC_51NB_NPCE985LA0DX_FIRMWARE + bool "Add firmware image for 51NB NPCE985LA0DX EC" + depends on EC_51NB_NPCE985LA0DX + default n + help + Select this option to add the firmware blob for the 51NB EC. + You need this blob to power on your machine. + +config EC_51NB_NPCE985LA0DX_FW + string "51NB EC firmware path" + depends on EC_51NB_NPCE985LA0DX_FIRMWARE + default "ec.bin" + help + The path and filename of the file to use as 51NB firmware. +endif diff --git a/src/ec/51nb/npce985la0dx/Makefile.inc b/src/ec/51nb/npce985la0dx/Makefile.inc new file mode 100644 index 0000000..810b324 --- /dev/null +++ b/src/ec/51nb/npce985la0dx/Makefile.inc @@ -0,0 +1,23 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# This file is part of the coreboot project. + +ifeq ($(CONFIG_EC_51NB_NPCE985LA0DX),y) + +files_added:: +ifeq ($(CONFIG_EC_51NB_NPCE985LA0DX_FIRMWARE),y) + $(CBFSTOOL) $(obj)/coreboot.rom write -r EC -f $(CONFIG_EC_51NB_NPCE985LA0DX_FW) --fill-upward +endif + +build_complete:: +ifeq ($(CONFIG_EC_51NB_NPCE985LA0DX_FIRMWARE),) + printf "\n** WARNING **\n" + printf "You haven't added the firmware blobs for 51NB EC.\n" + printf "You may be unable to power on your laptop without these blobs.\n" + printf "Please select the following option to add them:\n\n" + printf " Chipset --->\n" + printf " [*] Add firmware images for 51NB EC\n\n" +endif + +ramstage-y += npce985la0dx.c + +endif diff --git a/src/ec/51nb/npce985la0dx/npce985la0dx.c b/src/ec/51nb/npce985la0dx/npce985la0dx.c new file mode 100644 index 0000000..0e0fcd1 --- /dev/null +++ b/src/ec/51nb/npce985la0dx/npce985la0dx.c @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* This file is part of the coreboot project. */ + +#include <device/pnp.h> + +/* + * This embedded controller looks awfully like a Super I/O chip. LDNs 5 and 6 + * need to be enabled to turn on the keyboard and mouse controller, and LDN + * 0x11 needs to be enabled to turn on ACPI embedded controller functionality. + */ +static struct pnp_info dev_infos[] = { + { NULL, 0x05 }, { NULL, 0x06 }, { NULL, 0x11 } +}; + +static void ec_51nb_npce985la0dx_ops_enable(struct device *dev) +{ + pnp_enable_devices(dev, &pnp_ops, ARRAY_SIZE(dev_infos), dev_infos); +} + +struct chip_operations ec_51nb_npce985la0dx_ops = { + CHIP_NAME("51NB EC") + .enable_dev = ec_51nb_npce985la0dx_ops_enable, +};
-
Angel Pons (Code Review) -
Felix Held (Code Review)
-
Matt DeVillier (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)