[PATCH] firmware: coreboot: Let OF core populate platform device

[PATCH] 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 can
move this driver to use platform device APIs instead of DT specific ones
and drop the of_node handling that was presumably placed there to hold a
reference to the DT node created during module init.

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: Thierry Escande <thierry.escande@collabora.com>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table-of.c | 39 +++++----------------
 1 file changed, 9 insertions(+), 30 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
index f15bf404c579..270f112bdc54 100644
--- a/drivers/firmware/google/coreboot_table-of.c
+++ b/drivers/firmware/google/coreboot_table-of.c
@@ -18,21 +18,20 @@
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/module.h>
-#include <linux/of_address.h>
-#include <linux/of_platform.h>
+#include <linux/mod_devicetable.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;
+	struct resource *res;
 
-	ptr = of_iomap(fw_dn, 0);
-	of_node_put(fw_dn);
-	if (!ptr)
-		return -ENOMEM;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ptr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ptr))
+		return PTR_ERR(ptr);
 
 	return coreboot_table_init(&pdev->dev, ptr);
 }
@@ -44,8 +43,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 +55,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

Re: [PATCH] firmware: coreboot: Let OF core populate platform device

[Sudeep Holla]

On Mon, Aug 6, 2018 at 6:13 PM Stephen Boyd <swboyd@chromium.org> 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 can
> move this driver to use platform device APIs instead of DT specific ones
> and drop the of_node handling that was presumably placed there to hold a
> reference to the DT node created during module init.
>

Nice cleanup.

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

--
Regards,
Sudeep

Re: [PATCH] firmware: coreboot: Let OF core populate platform device

[Brian Norris]

On Mon, Aug 06, 2018 at 10:10:47AM -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

Does this deserve a 'Fixes' tag?

Fixes: 3aa0582fdb82 ("of: platform: populate /firmware/ node from of_platform_default_populate_init()")

The coreboot-of driver was in 4.12, and it seems to have broken in 4.16,
along with the above patch.

> 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 can
> move this driver to use platform device APIs instead of DT specific ones
> and drop the of_node handling that was presumably placed there to hold a
> reference to the DT node created during module init.
> 
> Cc: Wei-Ning Huang <wnhuang@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>

Reviewed-by: Brian Norris <briannorris@chromium.org>

> Cc: Samuel Holland <samuel@sholland.org>
> Cc: Thierry Escande <thierry.escande@collabora.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/firmware/google/coreboot_table-of.c | 39 +++++----------------
>  1 file changed, 9 insertions(+), 30 deletions(-)

Re: [PATCH] firmware: coreboot: Let OF core populate platform device

[Stephen Boyd]

Quoting Brian Norris (2018-08-06 10:38:08)
> On Mon, Aug 06, 2018 at 10:10:47AM -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
> 
> Does this deserve a 'Fixes' tag?
> 
> Fixes: 3aa0582fdb82 ("of: platform: populate /firmware/ node from of_platform_default_populate_init()")
> 
> The coreboot-of driver was in 4.12, and it seems to have broken in 4.16,
> along with the above patch.

Yes. A fixes tag should be added.

> 
> > 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 can
> > move this driver to use platform device APIs instead of DT specific ones
> > and drop the of_node handling that was presumably placed there to hold a
> > reference to the DT node created during module init.
> > 
> > Cc: Wei-Ning Huang <wnhuang@chromium.org>
> > Cc: Julius Werner <jwerner@chromium.org>
> > Cc: Brian Norris <briannorris@chromium.org>
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 

Thanks!

Re: [PATCH] firmware: coreboot: Let OF core populate platform device

[Julius Werner]

Thanks for the quick fix!

Reviewed-by: Julius Werner <jwerner@chromium.org>

Re: [PATCH] firmware: coreboot: Let OF core populate platform device

[Brian Norris]

Hi Stephen,

Although I already 'Reviewed' this, I noticed another problem when I
went to look again:

On Mon, Aug 06, 2018 at 10:10:47AM -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 can
> move this driver to use platform device APIs instead of DT specific ones
> and drop the of_node handling that was presumably placed there to hold a
> reference to the DT node created during module init.
> 
> 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: Thierry Escande <thierry.escande@collabora.com>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/firmware/google/coreboot_table-of.c | 39 +++++----------------
>  1 file changed, 9 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
> index f15bf404c579..270f112bdc54 100644
> --- a/drivers/firmware/google/coreboot_table-of.c
> +++ b/drivers/firmware/google/coreboot_table-of.c
> @@ -18,21 +18,20 @@
>  #include <linux/device.h>
>  #include <linux/io.h>
>  #include <linux/module.h>
> -#include <linux/of_address.h>
> -#include <linux/of_platform.h>
> +#include <linux/mod_devicetable.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;
> +	struct resource *res;
>  
> -	ptr = of_iomap(fw_dn, 0);
> -	of_node_put(fw_dn);
> -	if (!ptr)
> -		return -ENOMEM;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ptr = devm_ioremap_resource(&pdev->dev, res);

You're making this a devm_* ioremap, but coreboot_table_exit() will
handle unmapping this for you already. So you're introducing a potential
double-unmap.

> +	if (IS_ERR(ptr))
> +		return PTR_ERR(ptr);
>  
>  	return coreboot_table_init(&pdev->dev, ptr);
>  }
> @@ -44,8 +43,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id coreboot_of_match[] = {
>  	{ .compatible = "coreboot" },
> -	{},
> +	{}

...and since I had a *real* comment, I'll mention this bit I noticed
previously...this diff seems a bit superfluous. I understand we don't
really want to extend this list beyond the NULL terminator, but
normally, I'd expect we leave it alone.

Brian

>  };
> +MODULE_DEVICE_TABLE(of, coreboot_of_match);
>  
>  static struct platform_driver coreboot_table_of_driver = {
>  	.probe = coreboot_table_of_probe,
> @@ -55,28 +55,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
> 

Re: [PATCH] firmware: coreboot: Let OF core populate platform device

[Stephen Boyd]

Quoting Brian Norris (2018-08-07 14:41:04)
> On Mon, Aug 06, 2018 at 10:10:47AM -0700, Stephen Boyd wrote:
> > diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
> > index f15bf404c579..270f112bdc54 100644
> > --- a/drivers/firmware/google/coreboot_table-of.c
> > +++ b/drivers/firmware/google/coreboot_table-of.c
> > @@ -18,21 +18,20 @@
> >  #include <linux/device.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > -#include <linux/of_address.h>
> > -#include <linux/of_platform.h>
> > +#include <linux/mod_devicetable.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;
> > +     struct resource *res;
> >  
> > -     ptr = of_iomap(fw_dn, 0);
> > -     of_node_put(fw_dn);
> > -     if (!ptr)
> > -             return -ENOMEM;
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     ptr = devm_ioremap_resource(&pdev->dev, res);
> 
> You're making this a devm_* ioremap, but coreboot_table_exit() will
> handle unmapping this for you already. So you're introducing a potential
> double-unmap.

Thanks! I'll split this patch into essential and non-essential pieces
then and layer shortening patches on top.

> 
> > +     if (IS_ERR(ptr))
> > +             return PTR_ERR(ptr);
> >  
> >       return coreboot_table_init(&pdev->dev, ptr);
> >  }
> > @@ -44,8 +43,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
> >  
> >  static const struct of_device_id coreboot_of_match[] = {
> >       { .compatible = "coreboot" },
> > -     {},
> > +     {}
> 
> ...and since I had a *real* comment, I'll mention this bit I noticed
> previously...this diff seems a bit superfluous. I understand we don't
> really want to extend this list beyond the NULL terminator, but
> normally, I'd expect we leave it alone.
> 

I was adding MODULE_DEVICE_TABLE nearby. I'll leave the removal here
unless you really hate the style cleanup.

> >  };
> > +MODULE_DEVICE_TABLE(of, coreboot_of_match);
> >  

Re: [PATCH] firmware: coreboot: Let OF core populate platform device

[Brian Norris]

On Tue, Aug 07, 2018 at 11:00:50PM -0700, Stephen Boyd wrote:
> Quoting Brian Norris (2018-08-07 14:41:04)
> > On Mon, Aug 06, 2018 at 10:10:47AM -0700, Stephen Boyd wrote:
> > > @@ -44,8 +43,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
> > >  
> > >  static const struct of_device_id coreboot_of_match[] = {
> > >       { .compatible = "coreboot" },
> > > -     {},
> > > +     {}
> > 
> > ...and since I had a *real* comment, I'll mention this bit I noticed
> > previously...this diff seems a bit superfluous. I understand we don't
> > really want to extend this list beyond the NULL terminator, but
> > normally, I'd expect we leave it alone.
> > 
> 
> I was adding MODULE_DEVICE_TABLE nearby. I'll leave the removal here
> unless you really hate the style cleanup.

My ugly and imprecise grepping tells me you're going to have a fun time
enforcing your sense of style, but good luck ;)

$ git grep -n -A 10 'of_device_id.*\[\] = ' | grep '{\s*}$' | wc -l
1020
$ git grep -n -A 10 'of_device_id.*\[\] = ' | grep '{\s*},$' | wc -l
1348