[PATCH v2 0/2] firmware: coreboot: Fix probe and simplify code

[PATCH v2 0/2] firmware: coreboot: Fix probe and simplify code

[Stephen Boyd]

The first patch is a trimmed down version of patch#1 that leaves
the of_iomap() in place and just fixes the real problem of probe
not working on DT based platforms. The second patch collapses everything
into the core file to hopefully simplify code at the expense of mildly
changing the behavior on DT based platforms.

Stephen Boyd (2):
  firmware: coreboot: Let OF core populate platform device
  firmware: coreboot: Collapse platform drivers into bus core

 drivers/firmware/google/Kconfig               | 28 +++---
 drivers/firmware/google/Makefile              |  2 -
 drivers/firmware/google/coreboot_table-acpi.c | 88 -----------------
 drivers/firmware/google/coreboot_table-of.c   | 82 ----------------
 drivers/firmware/google/coreboot_table.c      | 96 ++++++++++++++++---
 drivers/firmware/google/coreboot_table.h      |  6 --
 6 files changed, 96 insertions(+), 206 deletions(-)
 delete mode 100644 drivers/firmware/google/coreboot_table-acpi.c
 delete mode 100644 drivers/firmware/google/coreboot_table-of.c

-- 
Sent by a computer through tubes

[PATCH v2 1/2] firmware: coreboot: Let OF core populate platform device

[Stephen Boyd]

Now that the /firmware/coreboot node in DT is populated by the core DT
platform code with commit 3aa0582fdb82 ("of: platform: populate
/firmware/ node from of_platform_default_populate_init()") we should and
can remove the platform device creation here. Otherwise, the
of_platform_device_create() call will fail, the coreboot of driver won't
be registered, and this driver will never bind. At the same time, we
should move this driver to use MODULE_DEVICE_TABLE so that module
auto-load works properly when the coreboot device is auto-populated and
we should drop the of_node handling that was presumably placed here to
hold a reference to the DT node created during module init that no
longer happens.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Cc: Sudeep Holla <Sudeep.Holla@arm.com>
Fixes: 3aa0582fdb82 ("of: platform: populate /firmware/ node from of_platform_default_populate_init()")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table-of.c | 28 +++------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
index f15bf404c579..9b90c0fa4a0b 100644
--- a/drivers/firmware/google/coreboot_table-of.c
+++ b/drivers/firmware/google/coreboot_table-of.c
@@ -19,7 +19,6 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
-#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 
 #include "coreboot_table.h"
@@ -30,7 +29,6 @@ static int coreboot_table_of_probe(struct platform_device *pdev)
 	void __iomem *ptr;
 
 	ptr = of_iomap(fw_dn, 0);
-	of_node_put(fw_dn);
 	if (!ptr)
 		return -ENOMEM;
 
@@ -44,8 +42,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
 
 static const struct of_device_id coreboot_of_match[] = {
 	{ .compatible = "coreboot" },
-	{},
+	{}
 };
+MODULE_DEVICE_TABLE(of, coreboot_of_match);
 
 static struct platform_driver coreboot_table_of_driver = {
 	.probe = coreboot_table_of_probe,
@@ -55,28 +54,7 @@ static struct platform_driver coreboot_table_of_driver = {
 		.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_of_match, of_node))
-		return -ENODEV;
-
-	pdev = of_platform_device_create(of_node, "coreboot_table_of", NULL);
-	if (!pdev)
-		return -ENODEV;
-
-	return platform_driver_register(&coreboot_table_of_driver);
-}
-
-module_init(platform_coreboot_table_of_init);
+module_platform_driver(coreboot_table_of_driver);
 
 MODULE_AUTHOR("Google, Inc.");
 MODULE_LICENSE("GPL");
-- 
Sent by a computer through tubes

[PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core

[Stephen Boyd]

The DT based and ACPI based platform drivers here do the same thing; map
some memory and hand it over to the coreboot bus to populate devices.
The only major difference is that the DT based driver doesn't map the
coreboot table header to figure out how large of a region to map for the
whole coreboot table and it uses of_iomap() instead of ioremap_cache().
A cached or non-cached mapping shouldn't matter here and mapping some
smaller region first before mapping the whole table is just more work
but should be OK. In the end, we can remove two files and combine the
code all in one place making it easier to reason about things.

We leave the old Kconfigs in place for a little while longer but make
them hidden and default to the previously hidden config option. This way
users can upgrade without having to know to reselect this config in the
future. Later on we can remove the old hidden configs.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Cc: Sudeep Holla <Sudeep.Holla@arm.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/Kconfig               | 28 +++---
 drivers/firmware/google/Makefile              |  2 -
 drivers/firmware/google/coreboot_table-acpi.c | 88 -----------------
 drivers/firmware/google/coreboot_table-of.c   | 60 ------------
 drivers/firmware/google/coreboot_table.c      | 96 ++++++++++++++++---
 drivers/firmware/google/coreboot_table.h      |  6 --
 6 files changed, 96 insertions(+), 184 deletions(-)
 delete mode 100644 drivers/firmware/google/coreboot_table-acpi.c
 delete mode 100644 drivers/firmware/google/coreboot_table-of.c

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index a456a000048b..42bea4dfdc4b 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -19,28 +19,22 @@ config GOOGLE_SMI
 	  variables.
 
 config GOOGLE_COREBOOT_TABLE
-	tristate
-	depends on GOOGLE_COREBOOT_TABLE_ACPI || GOOGLE_COREBOOT_TABLE_OF
-
-config GOOGLE_COREBOOT_TABLE_ACPI
-	tristate "Coreboot Table Access - ACPI"
-	depends on ACPI
-	select GOOGLE_COREBOOT_TABLE
+	tristate "Coreboot Table Access"
+	depends on ACPI || OF
 	help
 	  This option enables the coreboot_table module, which provides other
-	  firmware modules to access to the coreboot table. The coreboot table
-	  pointer is accessed through the ACPI "GOOGCB00" object.
+	  firmware modules access to the coreboot table. The coreboot table
+	  pointer is accessed through the ACPI "GOOGCB00" object or the
+	  device tree node /firmware/coreboot.
 	  If unsure say N.
 
+config GOOGLE_COREBOOT_TABLE_ACPI
+	tristate
+	default GOOGLE_COREBOOT_TABLE
+
 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.
+	tristate
+	default GOOGLE_COREBOOT_TABLE
 
 config GOOGLE_MEMCONSOLE
 	tristate
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d0b3fba96194..d17caded5d88 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -2,8 +2,6 @@
 
 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_FRAMEBUFFER_COREBOOT)  += framebuffer-coreboot.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
diff --git a/drivers/firmware/google/coreboot_table-acpi.c b/drivers/firmware/google/coreboot_table-acpi.c
deleted file mode 100644
index 77197fe3d42f..000000000000
--- a/drivers/firmware/google/coreboot_table-acpi.c
+++ /dev/null
@@ -1,88 +0,0 @@
-/*
- * coreboot_table-acpi.c
- *
- * Using ACPI to locate Coreboot table and provide coreboot table access.
- *
- * 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 <linux/acpi.h>
-#include <linux/device.h>
-#include <linux/err.h>
-#include <linux/init.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-
-#include "coreboot_table.h"
-
-static int coreboot_table_acpi_probe(struct platform_device *pdev)
-{
-	phys_addr_t phyaddr;
-	resource_size_t len;
-	struct coreboot_table_header __iomem *header = NULL;
-	struct resource *res;
-	void __iomem *ptr = NULL;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -EINVAL;
-
-	len = resource_size(res);
-	if (!res->start || !len)
-		return -EINVAL;
-
-	phyaddr = res->start;
-	header = ioremap_cache(phyaddr, sizeof(*header));
-	if (header == NULL)
-		return -ENOMEM;
-
-	ptr = ioremap_cache(phyaddr,
-			    header->header_bytes + header->table_bytes);
-	iounmap(header);
-	if (!ptr)
-		return -ENOMEM;
-
-	return coreboot_table_init(&pdev->dev, ptr);
-}
-
-static int coreboot_table_acpi_remove(struct platform_device *pdev)
-{
-	return coreboot_table_exit();
-}
-
-static const struct acpi_device_id cros_coreboot_acpi_match[] = {
-	{ "GOOGCB00", 0 },
-	{ "BOOT0000", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, cros_coreboot_acpi_match);
-
-static struct platform_driver coreboot_table_acpi_driver = {
-	.probe = coreboot_table_acpi_probe,
-	.remove = coreboot_table_acpi_remove,
-	.driver = {
-		.name = "coreboot_table_acpi",
-		.acpi_match_table = ACPI_PTR(cros_coreboot_acpi_match),
-	},
-};
-
-static int __init coreboot_table_acpi_init(void)
-{
-	return platform_driver_register(&coreboot_table_acpi_driver);
-}
-
-module_init(coreboot_table_acpi_init);
-
-MODULE_AUTHOR("Google, Inc.");
-MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
deleted file mode 100644
index 9b90c0fa4a0b..000000000000
--- a/drivers/firmware/google/coreboot_table-of.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
- * 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 <linux/device.h>
-#include <linux/io.h>
-#include <linux/module.h>
-#include <linux/of_address.h>
-#include <linux/platform_device.h>
-
-#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);
-	if (!ptr)
-		return -ENOMEM;
-
-	return coreboot_table_init(&pdev->dev, 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" },
-	{}
-};
-MODULE_DEVICE_TABLE(of, coreboot_of_match);
-
-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,
-	},
-};
-module_platform_driver(coreboot_table_of_driver);
-
-MODULE_AUTHOR("Google, Inc.");
-MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 19db5709ae28..9a1d066ec9ad 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -16,12 +16,15 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 
 #include "coreboot_table.h"
@@ -70,12 +73,6 @@ static struct bus_type coreboot_bus_type = {
 	.remove		= coreboot_bus_remove,
 };
 
-static int __init coreboot_bus_init(void)
-{
-	return bus_register(&coreboot_bus_type);
-}
-module_init(coreboot_bus_init);
-
 static void coreboot_device_release(struct device *dev)
 {
 	struct coreboot_device *device = CB_DEV(dev);
@@ -97,7 +94,7 @@ void coreboot_driver_unregister(struct coreboot_driver *driver)
 }
 EXPORT_SYMBOL(coreboot_driver_unregister);
 
-int coreboot_table_init(struct device *dev, void __iomem *ptr)
+static int coreboot_table_init(struct device *dev, void __iomem *ptr)
 {
 	int i, ret;
 	void *ptr_entry;
@@ -140,19 +137,96 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
 
 	return ret;
 }
-EXPORT_SYMBOL(coreboot_table_init);
 
-int coreboot_table_exit(void)
+static int coreboot_table_probe(struct platform_device *pdev)
+{
+	phys_addr_t phyaddr;
+	resource_size_t len;
+	struct coreboot_table_header __iomem *header = NULL;
+	struct resource *res;
+	void __iomem *ptr = NULL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+
+	len = resource_size(res);
+	if (!res->start || !len)
+		return -EINVAL;
+
+	phyaddr = res->start;
+	header = ioremap_cache(phyaddr, sizeof(*header));
+	if (header == NULL)
+		return -ENOMEM;
+
+	ptr = ioremap_cache(phyaddr,
+			    header->header_bytes + header->table_bytes);
+	iounmap(header);
+	if (!ptr)
+		return -ENOMEM;
+
+	return coreboot_table_init(&pdev->dev, ptr);
+}
+
+static int coreboot_table_remove(struct platform_device *pdev)
 {
 	if (ptr_header) {
 		bus_unregister(&coreboot_bus_type);
 		iounmap(ptr_header);
 		ptr_header = NULL;
 	}
-
 	return 0;
 }
-EXPORT_SYMBOL(coreboot_table_exit);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id cros_coreboot_acpi_match[] = {
+	{ "GOOGCB00", 0 },
+	{ "BOOT0000", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, cros_coreboot_acpi_match);
+#endif
+
+#ifdef CONFIG_OF
+static const struct of_device_id coreboot_of_match[] = {
+	{ .compatible = "coreboot" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, coreboot_of_match);
+#endif
+
+static struct platform_driver coreboot_table_driver = {
+	.probe = coreboot_table_probe,
+	.remove = coreboot_table_remove,
+	.driver = {
+		.name = "coreboot_table",
+		.acpi_match_table = ACPI_PTR(cros_coreboot_acpi_match),
+		.of_match_table = of_match_ptr(coreboot_of_match),
+	},
+};
+
+static int __init coreboot_init(void)
+{
+	int ret;
+
+	ret = bus_register(&coreboot_bus_type);
+	if (ret)
+		return ret;
+
+	ret = platform_driver_register(&coreboot_table_driver);
+	if (ret)
+		bus_unregister(&coreboot_bus_type);
+
+	return ret;
+}
+module_init(coreboot_init);
+
+static void __exit coreboot_exit(void)
+{
+	platform_driver_unregister(&coreboot_table_driver);
+	bus_unregister(&coreboot_bus_type);
+}
+module_exit(coreboot_exit);
 
 MODULE_AUTHOR("Google, Inc.");
 MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index 8ad95a94481b..71a9de6b15fa 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -91,10 +91,4 @@ int coreboot_driver_register(struct coreboot_driver *driver);
 /* Unregister a driver that uses the data from a coreboot table. */
 void coreboot_driver_unregister(struct coreboot_driver *driver);
 
-/* Initialize coreboot table module given a pointer to iomem */
-int coreboot_table_init(struct device *dev, void __iomem *ptr);
-
-/* Cleanup coreboot table module */
-int coreboot_table_exit(void);
-
 #endif /* __COREBOOT_TABLE_H */
-- 
Sent by a computer through tubes

Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core

[Julius Werner]

> +config GOOGLE_COREBOOT_TABLE_ACPI
> +       tristate
> +       default GOOGLE_COREBOOT_TABLE

I don't think this helps in upgrading (as your commit message says)
unless you also keep the 'select GOOGLE_COREBOOT_TABLE' here, right?

> -int coreboot_table_init(struct device *dev, void __iomem *ptr)
> +static int coreboot_table_init(struct device *dev, void __iomem *ptr)

nit: There's little reason to keep coreboot_table_init() a separate
function now. Could maybe compact the code a little more if you merge
it into probe()? (Also could then do the signature sanity check before
trusting the length values to map the whole thing, which is probably a
good idea.)

>         if (ptr_header) {
>                 bus_unregister(&coreboot_bus_type);
>                 iounmap(ptr_header);

Could ptr_header be handled by devm now, somehow? Also, don't you have
two bus_unregister() now (here and in coreboot_exit())? Or is that
intentional?

> +static struct platform_driver coreboot_table_driver = {
> +       .probe = coreboot_table_probe,
> +       .remove = coreboot_table_remove,
> +       .driver = {
> +               .name = "coreboot_table",
> +               .acpi_match_table = ACPI_PTR(cros_coreboot_acpi_match),
> +               .of_match_table = of_match_ptr(coreboot_of_match),

Who takes precedence if they both exist? Will we have two
coreboot_table busses? (That would probably not be so good...)

Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core

[Stephen Boyd]

Quoting Julius Werner (2018-08-08 12:07:30)
> > +config GOOGLE_COREBOOT_TABLE_ACPI
> > +       tristate
> > +       default GOOGLE_COREBOOT_TABLE
> 
> I don't think this helps in upgrading (as your commit message says)
> unless you also keep the 'select GOOGLE_COREBOOT_TABLE' here, right?

Oh yes should be select, not default.

> 
> > -int coreboot_table_init(struct device *dev, void __iomem *ptr)
> > +static int coreboot_table_init(struct device *dev, void __iomem *ptr)
> 
> nit: There's little reason to keep coreboot_table_init() a separate
> function now. Could maybe compact the code a little more if you merge
> it into probe()? (Also could then do the signature sanity check before
> trusting the length values to map the whole thing, which is probably a
> good idea.)

Sure. I can make another patch for squashing that all together.

> 
> >         if (ptr_header) {
> >                 bus_unregister(&coreboot_bus_type);
> >                 iounmap(ptr_header);
> 
> Could ptr_header be handled by devm now, somehow?

Yes. It hasn't been devmified yet because that would be more things in
one big patch. This is quickly blowing up!

> Also, don't you have
> two bus_unregister() now (here and in coreboot_exit())? Or is that
> intentional?

That's nice. I didn't notice that module_init() was registering the bus
and then platform drivers could remove the bus later with the driver
unbind. I'll move them both into the driver bind/unbind path, in another
patch.

> 
> > +static struct platform_driver coreboot_table_driver = {
> > +       .probe = coreboot_table_probe,
> > +       .remove = coreboot_table_remove,
> > +       .driver = {
> > +               .name = "coreboot_table",
> > +               .acpi_match_table = ACPI_PTR(cros_coreboot_acpi_match),
> > +               .of_match_table = of_match_ptr(coreboot_of_match),
> 
> Who takes precedence if they both exist? Will we have two
> coreboot_table busses? (That would probably not be so good...)

I'm not aware of a system that has both ACPI and devicetree, so this
isn't a problem.

Re: [PATCH v2 1/2] firmware: coreboot: Let OF core populate platform device

[Sudeep Holla]

On Wed, Aug 08, 2018 at 10:24:13AM -0700, Stephen Boyd wrote:
> Now that the /firmware/coreboot node in DT is populated by the core DT
> platform code with commit 3aa0582fdb82 ("of: platform: populate
> /firmware/ node from of_platform_default_populate_init()") we should and
> can remove the platform device creation here. Otherwise, the
> of_platform_device_create() call will fail, the coreboot of driver won't
> be registered, and this driver will never bind. At the same time, we
> should move this driver to use MODULE_DEVICE_TABLE so that module
> auto-load works properly when the coreboot device is auto-populated and
> we should drop the of_node handling that was presumably placed here to
> hold a reference to the DT node created during module init that no
> longer happens.
>
> Cc: Wei-Ning Huang <wnhuang@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Samuel Holland <samuel@sholland.org>
> Cc: Sudeep Holla <Sudeep.Holla@arm.com>

Reviewed-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6656 bytes --]

Hi Stephen,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.18-rc8 next-20180808]
[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/Stephen-Boyd/firmware-coreboot-Fix-probe-and-simplify-code/20180810-015624
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.2.0 make.cross ARCH=mips 

All error/warnings (new ones prefixed by >>):

   In file included from arch/mips/include/asm/page.h:194:0,
                    from include/linux/mmzone.h:21,
                    from include/linux/gfp.h:6,
                    from include/linux/slab.h:15,
                    from include/linux/resource_ext.h:19,
                    from include/linux/acpi.h:26,
                    from drivers/firmware//google/coreboot_table.c:19:
   drivers/firmware//google/coreboot_table.c: In function 'coreboot_table_probe':
>> arch/mips/include/asm/io.h:277:35: error: '_page_cachable_default' undeclared (first use in this function)
     __ioremap_mode((offset), (size), _page_cachable_default)
                                      ^
>> arch/mips/include/asm/io.h:278:23: note: in expansion of macro 'ioremap_cachable'
    #define ioremap_cache ioremap_cachable
                          ^~~~~~~~~~~~~~~~
>> drivers/firmware//google/coreboot_table.c:158:11: note: in expansion of macro 'ioremap_cache'
     header = ioremap_cache(phyaddr, sizeof(*header));
              ^~~~~~~~~~~~~
   arch/mips/include/asm/io.h:277:35: note: each undeclared identifier is reported only once for each function it appears in
     __ioremap_mode((offset), (size), _page_cachable_default)
                                      ^
>> arch/mips/include/asm/io.h:278:23: note: in expansion of macro 'ioremap_cachable'
    #define ioremap_cache ioremap_cachable
                          ^~~~~~~~~~~~~~~~
>> drivers/firmware//google/coreboot_table.c:158:11: note: in expansion of macro 'ioremap_cache'
     header = ioremap_cache(phyaddr, sizeof(*header));
              ^~~~~~~~~~~~~
--
   In file included from arch/mips/include/asm/page.h:194:0,
                    from include/linux/mmzone.h:21,
                    from include/linux/gfp.h:6,
                    from include/linux/slab.h:15,
                    from include/linux/resource_ext.h:19,
                    from include/linux/acpi.h:26,
                    from drivers/firmware/google/coreboot_table.c:19:
   drivers/firmware/google/coreboot_table.c: In function 'coreboot_table_probe':
>> arch/mips/include/asm/io.h:277:35: error: '_page_cachable_default' undeclared (first use in this function)
     __ioremap_mode((offset), (size), _page_cachable_default)
                                      ^
>> arch/mips/include/asm/io.h:278:23: note: in expansion of macro 'ioremap_cachable'
    #define ioremap_cache ioremap_cachable
                          ^~~~~~~~~~~~~~~~
   drivers/firmware/google/coreboot_table.c:158:11: note: in expansion of macro 'ioremap_cache'
     header = ioremap_cache(phyaddr, sizeof(*header));
              ^~~~~~~~~~~~~
   arch/mips/include/asm/io.h:277:35: note: each undeclared identifier is reported only once for each function it appears in
     __ioremap_mode((offset), (size), _page_cachable_default)
                                      ^
>> arch/mips/include/asm/io.h:278:23: note: in expansion of macro 'ioremap_cachable'
    #define ioremap_cache ioremap_cachable
                          ^~~~~~~~~~~~~~~~
   drivers/firmware/google/coreboot_table.c:158:11: note: in expansion of macro 'ioremap_cache'
     header = ioremap_cache(phyaddr, sizeof(*header));
              ^~~~~~~~~~~~~

vim +/_page_cachable_default +277 arch/mips/include/asm/io.h

^1da177e include/asm-mips/io.h      Linus Torvalds    2005-04-16  260  
^1da177e include/asm-mips/io.h      Linus Torvalds    2005-04-16  261  /*
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  262   * ioremap_cachable -	map bus memory into CPU space
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  263   * @offset:	    bus address of the memory
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  264   * @size:	    size of the resource to map
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  265   *
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  266   * ioremap_nocache performs a platform specific sequence of operations to
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  267   * make bus memory CPU accessible via the readb/readw/readl/writeb/
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  268   * writew/writel functions and the other mmio helpers. The returned
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  269   * address is not guaranteed to be usable directly as a virtual
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  270   * address.
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  271   *
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  272   * This version of ioremap ensures that the memory is marked cachable by
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  273   * the CPU.  Also enables full write-combining.	 Useful for some
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  274   * memory-like regions on I/O busses.
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  275   */
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  276  #define ioremap_cachable(offset, size)					\
35133692 include/asm-mips/io.h      Chris Dearman     2007-09-19 @277  	__ioremap_mode((offset), (size), _page_cachable_default)
a68f3768 arch/mips/include/asm/io.h Maciej W. Rozycki 2016-01-09 @278  #define ioremap_cache ioremap_cachable
778e2ac5 include/asm-mips/io.h      Ralf Baechle      2006-02-28  279  

:::::: The code at line 277 was first introduced by commit
:::::: 351336929ccf222ae38ff0cb7a8dd5fd5c6236a0 [MIPS] Allow setting of the cache attribute at run time.

:::::: TO: Chris Dearman <chris@mips.com>
:::::: CC: Ralf Baechle <ralf@linux-mips.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56394 bytes --]

Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core

[kbuild test robot]

Hi Stephen,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v4.18-rc8 next-20180809]
[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/Stephen-Boyd/firmware-coreboot-Fix-probe-and-simplify-code/20180810-015624
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   drivers/firmware/google/coreboot_table.c:113:22: sparse: cast removes address space of expression
   drivers/firmware/google/coreboot_table.c:115:39: sparse: incorrect type in argument 2 (different address spaces) @@    expected void const volatile [noderef] <asn:2>*addr @@    got noderef] <asn:2>*addr @@
   drivers/firmware/google/coreboot_table.c:115:39:    expected void const volatile [noderef] <asn:2>*addr
   drivers/firmware/google/coreboot_table.c:115:39:    got void *[assigned] ptr_entry
   drivers/firmware/google/coreboot_table.c:127:47: sparse: incorrect type in argument 2 (different address spaces) @@    expected void const volatile [noderef] <asn:2>*addr @@    got noderef] <asn:2>*addr @@
   drivers/firmware/google/coreboot_table.c:127:47:    expected void const volatile [noderef] <asn:2>*addr
   drivers/firmware/google/coreboot_table.c:127:47:    got void *[assigned] ptr_entry
>> drivers/firmware/google/coreboot_table.c:163:29: sparse: dereference of noderef expression
   drivers/firmware/google/coreboot_table.c:163:52: sparse: dereference of noderef expression

vim +163 drivers/firmware/google/coreboot_table.c

    96	
    97	static int coreboot_table_init(struct device *dev, void __iomem *ptr)
    98	{
    99		int i, ret;
   100		void *ptr_entry;
   101		struct coreboot_device *device;
   102		struct coreboot_table_entry entry;
   103		struct coreboot_table_header header;
   104	
   105		ptr_header = ptr;
   106		memcpy_fromio(&header, ptr_header, sizeof(header));
   107	
   108		if (strncmp(header.signature, "LBIO", sizeof(header.signature))) {
   109			pr_warn("coreboot_table: coreboot table missing or corrupt!\n");
   110			return -ENODEV;
   111		}
   112	
 > 113		ptr_entry = (void *)ptr_header + header.header_bytes;
   114		for (i = 0; i < header.table_entries; i++) {
   115			memcpy_fromio(&entry, ptr_entry, sizeof(entry));
   116	
   117			device = kzalloc(sizeof(struct device) + entry.size, GFP_KERNEL);
   118			if (!device) {
   119				ret = -ENOMEM;
   120				break;
   121			}
   122	
   123			dev_set_name(&device->dev, "coreboot%d", i);
   124			device->dev.parent = dev;
   125			device->dev.bus = &coreboot_bus_type;
   126			device->dev.release = coreboot_device_release;
   127			memcpy_fromio(&device->entry, ptr_entry, entry.size);
   128	
   129			ret = device_register(&device->dev);
   130			if (ret) {
   131				put_device(&device->dev);
   132				break;
   133			}
   134	
   135			ptr_entry += entry.size;
   136		}
   137	
   138		return ret;
   139	}
   140	
   141	static int coreboot_table_probe(struct platform_device *pdev)
   142	{
   143		phys_addr_t phyaddr;
   144		resource_size_t len;
   145		struct coreboot_table_header __iomem *header = NULL;
   146		struct resource *res;
   147		void __iomem *ptr = NULL;
   148	
   149		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   150		if (!res)
   151			return -EINVAL;
   152	
   153		len = resource_size(res);
   154		if (!res->start || !len)
   155			return -EINVAL;
   156	
   157		phyaddr = res->start;
   158		header = ioremap_cache(phyaddr, sizeof(*header));
   159		if (header == NULL)
   160			return -ENOMEM;
   161	
   162		ptr = ioremap_cache(phyaddr,
 > 163				    header->header_bytes + header->table_bytes);
   164		iounmap(header);
   165		if (!ptr)
   166			return -ENOMEM;
   167	
   168		return coreboot_table_init(&pdev->dev, ptr);
   169	}
   170	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation