Re: [PATCH 5/5] firmware: google memconsole: Add ARM/ARM64 support
Hi Thierry, In case this wasn't obvious from the build report: On Thu, Mar 23, 2017 at 10:04:29PM +0100, Thierry Escande wrote: > --- a/drivers/firmware/google/Kconfig > +++ b/drivers/firmware/google/Kconfig > @@ -1,6 +1,5 @@ > menuconfig GOOGLE_FIRMWARE > bool "Google Firmware Drivers" > - depends on X86 The build bots complained: you're relaxing the compile-time restriction here, so you should probably move it to the GOOGLE_SMI symbol, since that file uses X86 assembly. > default n > help > These firmware drivers are used by Google's servers. They are > @@ -21,7 +20,7 @@ config GOOGLE_SMI > > config GOOGLE_COREBOOT_TABLE > tristate > - depends on GOOGLE_COREBOOT_TABLE_ACPI > + depends on GOOGLE_COREBOOT_TABLE_ACPI || GOOGLE_COREBOOT_TABLE_OF > > config GOOGLE_COREBOOT_TABLE_ACPI > tristate "Coreboot Table Access - ACPI" > @@ -33,6 +32,16 @@ config GOOGLE_COREBOOT_TABLE_ACPI > pointer is accessed through the ACPI "GOOGCB00" object. > If unsure say N. > > +config GOOGLE_COREBOOT_TABLE_OF > + tristate "Coreboot Table Access - Device Tree" > + depends on OF > + select GOOGLE_COREBOOT_TABLE > + help > + This option enable the coreboot_table module, which provide other > + firmware modules to access coreboot table. The coreboot table pointer > + is accessed through the device tree node /firmware/coreboot. > + If unsure say N. > + > config GOOGLE_MEMCONSOLE > tristate > depends on GOOGLE_MEMCONSOLE_X86_LEGACY || GOOGLE_MEMCONSOLE_COREBOOT Brian
Re: [PATCH 5/5] firmware: google memconsole: Add ARM/ARM64 support
Hi Thierry, [auto build test ERROR on linus/master] [also build test ERROR on v4.11-rc3] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Thierry-Escande/firmware-google-memconsole/20170326-080828 config: ia64-allmodconfig (attached as .config) compiler: ia64-linux-gcc (GCC) 6.2.0 reproduce: wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=ia64 All errors (new ones prefixed by >>): drivers/firmware/google/gsmi.c: In function 'gsmi_exec.constprop': >> drivers/firmware/google/gsmi.c:198:3: error: inconsistent operand >> constraints in an 'asm' asm volatile ( ^~~ drivers/firmware/google/gsmi.c:213:3: error: inconsistent operand constraints in an 'asm' asm volatile ( ^~~ drivers/firmware/google/gsmi.c:229:3: error: inconsistent operand constraints in an 'asm' asm volatile ( ^~~ vim +/asm +198 drivers/firmware/google/gsmi.c 74c5b31c Mike Waychison 2011-04-29 182 /* 74c5b31c Mike Waychison 2011-04-29 183 * AH : Subfunction number 74c5b31c Mike Waychison 2011-04-29 184 * AL : Function number 74c5b31c Mike Waychison 2011-04-29 185 * EBX : Parameter block address 74c5b31c Mike Waychison 2011-04-29 186 * DX : SMI command port 74c5b31c Mike Waychison 2011-04-29 187 * 74c5b31c Mike Waychison 2011-04-29 188 * Three protocols here. See also the comment in gsmi_init(). 74c5b31c Mike Waychison 2011-04-29 189 */ 74c5b31c Mike Waychison 2011-04-29 190 if (gsmi_dev.handshake_type == GSMI_HANDSHAKE_CF) { 74c5b31c Mike Waychison 2011-04-29 191 /* 74c5b31c Mike Waychison 2011-04-29 192 * If handshake_type == HANDSHAKE_CF then set CF on the 74c5b31c Mike Waychison 2011-04-29 193 * way in and wait for the handler to clear it; this avoids 74c5b31c Mike Waychison 2011-04-29 194 * corrupting register state on those chipsets which have 74c5b31c Mike Waychison 2011-04-29 195 * a delay between writing the SMI trigger register and 74c5b31c Mike Waychison 2011-04-29 196 * entering SMM. 74c5b31c Mike Waychison 2011-04-29 197 */ 74c5b31c Mike Waychison 2011-04-29 @198 asm volatile ( 74c5b31c Mike Waychison 2011-04-29 199 "stc\n" 74c5b31c Mike Waychison 2011-04-29 200 "outb %%al, %%dx\n" 74c5b31c Mike Waychison 2011-04-29 201 "1: jc 1b\n" 74c5b31c Mike Waychison 2011-04-29 202 : "=a" (result) 74c5b31c Mike Waychison 2011-04-29 203 : "0" (cmd), 74c5b31c Mike Waychison 2011-04-29 204 "d" (gsmi_dev.smi_cmd), 74c5b31c Mike Waychison 2011-04-29 205 "b" (gsmi_dev.param_buf->address) 74c5b31c Mike Waychison 2011-04-29 206 : "memory", "cc" :: The code at line 198 was first introduced by commit :: 74c5b31c6618f01079212332b2e5f6c42f2d6307 driver: Google EFI SMI :: TO: Mike Waychison :: CC: Greg Kroah-Hartman --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH 5/5] firmware: google memconsole: Add ARM/ARM64 support
> What exactly is the "memory console"? Is it a log that coreboot writes into? Yes. It contains log messages, like coreboot's equivalent of dmesg.
Re: [PATCH 5/5] firmware: google memconsole: Add ARM/ARM64 support
On Fri, Mar 24, 2017 at 12:28:59PM +, Mark Rutland wrote: > On Thu, Mar 23, 2017 at 10:04:29PM +0100, Thierry Escande wrote: > > +static const struct of_device_id coreboot_of_match[] = { > > + { .compatible = "coreboot" }, > > + {}, > > +}; > > + > > +static struct platform_driver coreboot_table_of_driver = { > > + .probe = coreboot_table_of_probe, > > + .remove = coreboot_table_of_remove, > > + .driver = { > > + .name = "coreboot_table_of", > > + .of_match_table = coreboot_of_match, > > + }, > > +}; > > + > > +static int __init platform_coreboot_table_of_init(void) > > +{ > > + struct platform_device *pdev; > > + struct device_node *of_node; > > + > > + /* Limit device creation to the presence of /firmware/coreboot node */ > > + of_node = of_find_node_by_path("/firmware/coreboot"); > > + if (!of_node) > > + return -ENODEV; > > + > > I don't beleive that you need this module init function. Please use the > usual DT probing infrastrucutre instead, e.g. add: > > MODULE_DEVICE_TABLE(of, coreboot_of_match); > module_platform_driver(coreboot_table_of_driver); That doesn't work. If this node is placed under /firmware, which isn't a proper "bus", then we have to explicitly look for the sub-device. Due to this, the MODULE_DEVICE_TABLE() also isn't useful, because the /firmware/coreboot/ device won't be generated automatically, and so no matching uevent will occur. Brian
Re: [PATCH 5/5] firmware: google memconsole: Add ARM/ARM64 support
On Thu, Mar 23, 2017 at 10:04:29PM +0100, Thierry Escande wrote: > This patch expands the Google firmware memory console driver to also > work on certain tree based platforms running coreboot, such as ARM/ARM64 > Chromebooks. This patch now adds another path to find the coreboot table > through the device tree. In order to find that, a second level > bootloader must have installed the 'coreboot' compatible device tree > node that describes its base address and size. What exactly is the "memory console"? Is it a log that coreboot writes into? [...] > +static const struct of_device_id coreboot_of_match[] = { > + { .compatible = "coreboot" }, > + {}, > +}; > + > +static struct platform_driver coreboot_table_of_driver = { > + .probe = coreboot_table_of_probe, > + .remove = coreboot_table_of_remove, > + .driver = { > + .name = "coreboot_table_of", > + .of_match_table = coreboot_of_match, > + }, > +}; > + > +static int __init platform_coreboot_table_of_init(void) > +{ > + struct platform_device *pdev; > + struct device_node *of_node; > + > + /* Limit device creation to the presence of /firmware/coreboot node */ > + of_node = of_find_node_by_path("/firmware/coreboot"); > + if (!of_node) > + return -ENODEV; > + I don't beleive that you need this module init function. Please use the usual DT probing infrastrucutre instead, e.g. add: MODULE_DEVICE_TABLE(of, coreboot_of_match); module_platform_driver(coreboot_table_of_driver); Thanks, Mark.
[PATCH 5/5] firmware: google memconsole: Add ARM/ARM64 support
This patch expands the Google firmware memory console driver to also work on certain tree based platforms running coreboot, such as ARM/ARM64 Chromebooks. This patch now adds another path to find the coreboot table through the device tree. In order to find that, a second level bootloader must have installed the 'coreboot' compatible device tree node that describes its base address and size. This patch is a rework/split/merge of patches from the chromeos v4.4 kernel tree originally authored by: Wei-Ning Huang Julius Werner Brian Norris Signed-off-by: Thierry Escande --- drivers/firmware/google/Kconfig | 13 - drivers/firmware/google/Makefile| 1 + drivers/firmware/google/coreboot_table-of.c | 82 + 3 files changed, 94 insertions(+), 2 deletions(-) create mode 100644 drivers/firmware/google/coreboot_table-of.c diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig index 840bd62..baabd70 100644 --- a/drivers/firmware/google/Kconfig +++ b/drivers/firmware/google/Kconfig @@ -1,6 +1,5 @@ menuconfig GOOGLE_FIRMWARE bool "Google Firmware Drivers" - depends on X86 default n help These firmware drivers are used by Google's servers. They are @@ -21,7 +20,7 @@ config GOOGLE_SMI config GOOGLE_COREBOOT_TABLE tristate - depends on GOOGLE_COREBOOT_TABLE_ACPI + depends on GOOGLE_COREBOOT_TABLE_ACPI || GOOGLE_COREBOOT_TABLE_OF config GOOGLE_COREBOOT_TABLE_ACPI tristate "Coreboot Table Access - ACPI" @@ -33,6 +32,16 @@ config GOOGLE_COREBOOT_TABLE_ACPI pointer is accessed through the ACPI "GOOGCB00" object. If unsure say N. +config GOOGLE_COREBOOT_TABLE_OF + tristate "Coreboot Table Access - Device Tree" + depends on OF + select GOOGLE_COREBOOT_TABLE + help + This option enable the coreboot_table module, which provide other + firmware modules to access coreboot table. The coreboot table pointer + is accessed through the device tree node /firmware/coreboot. + If unsure say N. + config GOOGLE_MEMCONSOLE tristate depends on GOOGLE_MEMCONSOLE_X86_LEGACY || GOOGLE_MEMCONSOLE_COREBOOT diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile index 662a83e..bb952c6 100644 --- a/drivers/firmware/google/Makefile +++ b/drivers/firmware/google/Makefile @@ -2,6 +2,7 @@ obj-$(CONFIG_GOOGLE_SMI) += gsmi.o obj-$(CONFIG_GOOGLE_COREBOOT_TABLE)+= coreboot_table.o obj-$(CONFIG_GOOGLE_COREBOOT_TABLE_ACPI) += coreboot_table-acpi.o +obj-$(CONFIG_GOOGLE_COREBOOT_TABLE_OF) += coreboot_table-of.o obj-$(CONFIG_GOOGLE_MEMCONSOLE)+= memconsole.o obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT) += memconsole-coreboot.o obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c new file mode 100644 index 000..727acdc --- /dev/null +++ b/drivers/firmware/google/coreboot_table-of.c @@ -0,0 +1,82 @@ +/* + * coreboot_table-of.c + * + * Coreboot table access through open firmware. + * + * Copyright 2017 Google Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License v2.0 as published by + * the Free Software Foundation. + * + * 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 +#include +#include +#include +#include +#include + +#include "coreboot_table.h" + +static int coreboot_table_of_probe(struct platform_device *pdev) +{ + struct device_node *fw_dn = pdev->dev.of_node; + void __iomem *ptr; + + ptr = of_iomap(fw_dn, 0); + of_node_put(fw_dn); + if (!ptr) + return -ENOMEM; + + return coreboot_table_init(ptr); +} + +static int coreboot_table_of_remove(struct platform_device *pdev) +{ + return coreboot_table_exit(); +} + +static const struct of_device_id coreboot_of_match[] = { + { .compatible = "coreboot" }, + {}, +}; + +static struct platform_driver coreboot_table_of_driver = { + .probe = coreboot_table_of_probe, + .remove = coreboot_table_of_remove, + .driver = { + .name = "coreboot_table_of", + .of_match_table = coreboot_of_match, + }, +}; + +static int __init platform_coreboot_table_of_init(void) +{ + struct platform_device *pdev; + struct device_node *of_node; + + /* Limit device creation to the presence of /firmware/coreboot node */ + of_node = of_find_node_by_path("/firmware/coreboot"); + if (!of_node) + return -ENODEV; + + if (!of_match_node(coreboot_