[PATCH v2 0/2] of: Register platform device for each framebuffer

[PATCH v2 0/2] of: Register platform device for each framebuffer

[Thomas Zimmermann]

Move the detection of OF framebuffers from fbdev into of platform code
and register a Linux platform device for each framebuffer. Allows for
DRM-based OF drivers and real hot-unplugging of the framebuffer.

This patchset has been tested with qemu's ppc64le emulation, which
provides a framebuffer via OF display node. If someone has an older
32-bit system with BootX available, please test.

v2:
	* integrate PPC code into generic platform setup (Rob)
	* keep !device workaround with a warning (Javier, Daniel)

Thomas Zimmermann (2):
  of: Create platform devices for OF framebuffers
  fbdev: Warn in hot-unplug workaround for framebuffers without device

 drivers/of/platform.c            | 88 +++++++++++++++++++++-------
 drivers/video/fbdev/core/fbmem.c | 10 ++--
 drivers/video/fbdev/offb.c       | 98 +++++++++++++++++++++-----------
 3 files changed, 136 insertions(+), 60 deletions(-)


base-commit: d97978df553d768e457cb68c637b2b0a6188b87c
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: 6e1032c6302461624f33194c8b8f37103a3fa6ef
prerequisite-patch-id: 3f204510fcbf9530d6540bd8e6128cce598988b6
prerequisite-patch-id: ab7611d28d07723ab1dd392dcf9a6345de3b1040
-- 
2.35.1

[PATCH v2 0/2] of: Register platform device for each framebuffer

[Thomas Zimmermann]

Move the detection of OF framebuffers from fbdev into of platform code
and register a Linux platform device for each framebuffer. Allows for
DRM-based OF drivers and real hot-unplugging of the framebuffer.

This patchset has been tested with qemu's ppc64le emulation, which
provides a framebuffer via OF display node. If someone has an older
32-bit system with BootX available, please test.

v2:
	* integrate PPC code into generic platform setup (Rob)
	* keep !device workaround with a warning (Javier, Daniel)

Thomas Zimmermann (2):
  of: Create platform devices for OF framebuffers
  fbdev: Warn in hot-unplug workaround for framebuffers without device

 drivers/of/platform.c            | 88 +++++++++++++++++++++-------
 drivers/video/fbdev/core/fbmem.c | 10 ++--
 drivers/video/fbdev/offb.c       | 98 +++++++++++++++++++++-----------
 3 files changed, 136 insertions(+), 60 deletions(-)


base-commit: d97978df553d768e457cb68c637b2b0a6188b87c
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
prerequisite-patch-id: 6e1032c6302461624f33194c8b8f37103a3fa6ef
prerequisite-patch-id: 3f204510fcbf9530d6540bd8e6128cce598988b6
prerequisite-patch-id: ab7611d28d07723ab1dd392dcf9a6345de3b1040
-- 
2.35.1

[PATCH v2 1/2] of: Create platform devices for OF framebuffers

[Thomas Zimmermann]

Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.

Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
further prevented real hot-unplugging. Adding a distinct platform
device for each OF framebuffer solves both problems. Specifically, a
DRM driver can now provide graphics output for modern userspace.

Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or BootX displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.

Tested with OF display nodes on qemu's ppc64le target.

v2:
	* run PPC code as part of existing initialization (Rob)
	* add a few more error warnings (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/of/platform.c      | 88 ++++++++++++++++++++++++++--------
 drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++-------------
 2 files changed, 132 insertions(+), 54 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index a16b74f32aa9..738ba2e2838c 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -507,7 +507,6 @@ int of_platform_default_populate(struct device_node *root,
 }
 EXPORT_SYMBOL_GPL(of_platform_default_populate);
 
-#ifndef CONFIG_PPC
 static const struct of_device_id reserved_mem_matches[] = {
 	{ .compatible = "qcom,rmtfs-mem" },
 	{ .compatible = "qcom,cmd-db" },
@@ -520,33 +519,81 @@ static const struct of_device_id reserved_mem_matches[] = {
 
 static int __init of_platform_default_populate_init(void)
 {
-	struct device_node *node;
-
 	device_links_supplier_sync_state_pause();
 
 	if (!of_have_populated_dt())
 		return -ENODEV;
 
-	/*
-	 * Handle certain compatibles explicitly, since we don't want to create
-	 * platform_devices for every node in /reserved-memory with a
-	 * "compatible",
-	 */
-	for_each_matching_node(node, reserved_mem_matches)
-		of_platform_device_create(node, NULL, NULL);
+	if (IS_ENABLED(CONFIG_PPC)) {
+		struct device_node *boot_display = NULL;
+		struct device_node *node;
+		struct platform_device *dev;
+		int ret;
+
+		/* Check if we have a MacOS display without a node spec */
+		if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
+			/*
+			 * The old code tried to work out which node was the MacOS
+			 * display based on the address. I'm dropping that since the
+			 * lack of a node spec only happens with old BootX versions
+			 * (users can update) and with this code, they'll still get
+			 * a display (just not the palette hacks).
+			 */
+			dev = platform_device_alloc("bootx-noscreen", 0);
+			if (WARN_ON(!dev))
+				return -ENOMEM;
+			ret = platform_device_add(dev);
+			if (WARN_ON(ret)) {
+				platform_device_put(dev);
+				return ret;
+			}
+		}
 
-	node = of_find_node_by_path("/firmware");
-	if (node) {
-		of_platform_populate(node, NULL, NULL, NULL);
-		of_node_put(node);
-	}
+		/*
+		 * For OF framebuffers, first create the device for the boot display,
+		 * then for the other framebuffers. Only fail for the boot display;
+		 * ignore errors for the rest.
+		 */
+		for_each_node_by_type(node, "display") {
+			if (!of_get_property(node, "linux,opened", NULL) ||
+			    !of_get_property(node, "linux,boot-display", NULL))
+				continue;
+			dev = of_platform_device_create(node, "of-display", NULL);
+			if (WARN_ON(!dev))
+				return -ENOMEM;
+			boot_display = node;
+			break;
+		}
+		for_each_node_by_type(node, "display") {
+			if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
+				continue;
+			of_platform_device_create(node, "of-display", NULL);
+		}
 
-	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
-	of_platform_device_create(node, NULL, NULL);
-	of_node_put(node);
+	} else {
+		struct device_node *node;
+
+		/*
+		 * Handle certain compatibles explicitly, since we don't want to create
+		 * platform_devices for every node in /reserved-memory with a
+		 * "compatible",
+		 */
+		for_each_matching_node(node, reserved_mem_matches)
+			of_platform_device_create(node, NULL, NULL);
 
-	/* Populate everything else. */
-	of_platform_default_populate(NULL, NULL, NULL);
+		node = of_find_node_by_path("/firmware");
+		if (node) {
+			of_platform_populate(node, NULL, NULL, NULL);
+			of_node_put(node);
+		}
+
+		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
+		of_platform_device_create(node, NULL, NULL);
+		of_node_put(node);
+
+		/* Populate everything else. */
+		of_platform_default_populate(NULL, NULL, NULL);
+	}
 
 	return 0;
 }
@@ -558,7 +605,6 @@ static int __init of_platform_sync_state_init(void)
 	return 0;
 }
 late_initcall_sync(of_platform_sync_state_init);
-#endif
 
 int of_platform_device_destroy(struct device *dev, void *data)
 {
diff --git a/drivers/video/fbdev/offb.c b/drivers/video/fbdev/offb.c
index afdb6aa48add..b1acb1ebebe9 100644
--- a/drivers/video/fbdev/offb.c
+++ b/drivers/video/fbdev/offb.c
@@ -386,10 +386,10 @@ static void offb_init_palette_hacks(struct fb_info *info, struct device_node *dp
 		FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_STATIC_PSEUDOCOLOR;
 }
 
-static void __init offb_init_fb(const char *name,
-				int width, int height, int depth,
-				int pitch, unsigned long address,
-				int foreign_endian, struct device_node *dp)
+static void offb_init_fb(struct platform_device *parent, const char *name,
+			 int width, int height, int depth,
+			 int pitch, unsigned long address,
+			 int foreign_endian, struct device_node *dp)
 {
 	unsigned long res_size = pitch * height;
 	struct offb_par *par = &default_par;
@@ -410,12 +410,13 @@ static void __init offb_init_fb(const char *name,
 		return;
 	}
 
-	info = framebuffer_alloc(sizeof(u32) * 16, NULL);
+	info = framebuffer_alloc(sizeof(u32) * 16, &parent->dev);
 
 	if (!info) {
 		release_mem_region(res_start, res_size);
 		return;
 	}
+	platform_set_drvdata(parent, info);
 
 	fix = &info->fix;
 	var = &info->var;
@@ -535,7 +536,8 @@ static void __init offb_init_fb(const char *name,
 }
 
 
-static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
+static void offb_init_nodriver(struct platform_device *parent, struct device_node *dp,
+			       int no_real_node)
 {
 	unsigned int len;
 	int i, width = 640, height = 480, depth = 8, pitch = 640;
@@ -650,46 +652,76 @@ static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
 		/* kludge for valkyrie */
 		if (of_node_name_eq(dp, "valkyrie"))
 			address += 0x1000;
-		offb_init_fb(no_real_node ? "bootx" : NULL,
+		offb_init_fb(parent, no_real_node ? "bootx" : NULL,
 			     width, height, depth, pitch, address,
 			     foreign_endian, no_real_node ? NULL : dp);
 	}
 }
 
-static int __init offb_init(void)
+static int offb_remove(struct platform_device *pdev)
 {
-	struct device_node *dp = NULL, *boot_disp = NULL;
+	struct fb_info *info = platform_get_drvdata(pdev);
 
-	if (fb_get_options("offb", NULL))
-		return -ENODEV;
+	if (info)
+		unregister_framebuffer(info);
 
-	/* Check if we have a MacOS display without a node spec */
-	if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL) != NULL) {
-		/* The old code tried to work out which node was the MacOS
-		 * display based on the address. I'm dropping that since the
-		 * lack of a node spec only happens with old BootX versions
-		 * (users can update) and with this code, they'll still get
-		 * a display (just not the palette hacks).
-		 */
-		offb_init_nodriver(of_chosen, 1);
-	}
+	return 0;
+}
 
-	for_each_node_by_type(dp, "display") {
-		if (of_get_property(dp, "linux,opened", NULL) &&
-		    of_get_property(dp, "linux,boot-display", NULL)) {
-			boot_disp = dp;
-			offb_init_nodriver(dp, 0);
-		}
-	}
-	for_each_node_by_type(dp, "display") {
-		if (of_get_property(dp, "linux,opened", NULL) &&
-		    dp != boot_disp)
-			offb_init_nodriver(dp, 0);
-	}
+static int offb_probe_bootx_noscreen(struct platform_device *pdev)
+{
+	offb_init_nodriver(pdev, of_chosen, 1);
 
 	return 0;
 }
 
+static struct platform_driver offb_driver_bootx_noscreen = {
+	.driver = {
+		.name = "bootx-noscreen",
+	},
+	.probe = offb_probe_bootx_noscreen,
+	.remove = offb_remove,
+};
+
+static int offb_probe_display(struct platform_device *pdev)
+{
+	offb_init_nodriver(pdev, pdev->dev.of_node, 0);
+
+	return 0;
+}
 
+static const struct of_device_id offb_of_match_display[] = {
+	{ .compatible = "display", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, offb_of_match_display);
+
+static struct platform_driver offb_driver_display = {
+	.driver = {
+		.name = "of-display",
+		.of_match_table = offb_of_match_display,
+	},
+	.probe = offb_probe_display,
+	.remove = offb_remove,
+};
+
+static int __init offb_init(void)
+{
+	if (fb_get_options("offb", NULL))
+		return -ENODEV;
+
+	platform_driver_register(&offb_driver_bootx_noscreen);
+	platform_driver_register(&offb_driver_display);
+
+	return 0;
+}
 module_init(offb_init);
+
+static void __exit offb_exit(void)
+{
+	platform_driver_unregister(&offb_driver_display);
+	platform_driver_unregister(&offb_driver_bootx_noscreen);
+}
+module_exit(offb_exit);
+
 MODULE_LICENSE("GPL");
-- 
2.35.1

[PATCH v2 1/2] of: Create platform devices for OF framebuffers

[Thomas Zimmermann]

Create a platform device for each OF-declared framebuffer and have
offb bind to these devices. Allows for real hot-unplugging and other
drivers besides offb.

Originally, offb created framebuffer devices while initializing its
module by parsing the OF device tree. No actual Linux device was set
up. This tied OF framebuffers to offb and makes writing other drivers
for the OF framebuffers complicated. The absence of a Linux device
further prevented real hot-unplugging. Adding a distinct platform
device for each OF framebuffer solves both problems. Specifically, a
DRM driver can now provide graphics output for modern userspace.

Some of the offb init code is now located in the OF initialization.
There's now also an implementation of of_platform_default_populate_init(),
which was missing before. The OF side creates different devices for
either OF display nodes or BootX displays as they require different
handling by the driver. The offb drivers picks up each type of device
and runs the appropriate fbdev initialization.

Tested with OF display nodes on qemu's ppc64le target.

v2:
	* run PPC code as part of existing initialization (Rob)
	* add a few more error warnings (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/of/platform.c      | 88 ++++++++++++++++++++++++++--------
 drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++-------------
 2 files changed, 132 insertions(+), 54 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index a16b74f32aa9..738ba2e2838c 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -507,7 +507,6 @@ int of_platform_default_populate(struct device_node *root,
 }
 EXPORT_SYMBOL_GPL(of_platform_default_populate);
 
-#ifndef CONFIG_PPC
 static const struct of_device_id reserved_mem_matches[] = {
 	{ .compatible = "qcom,rmtfs-mem" },
 	{ .compatible = "qcom,cmd-db" },
@@ -520,33 +519,81 @@ static const struct of_device_id reserved_mem_matches[] = {
 
 static int __init of_platform_default_populate_init(void)
 {
-	struct device_node *node;
-
 	device_links_supplier_sync_state_pause();
 
 	if (!of_have_populated_dt())
 		return -ENODEV;
 
-	/*
-	 * Handle certain compatibles explicitly, since we don't want to create
-	 * platform_devices for every node in /reserved-memory with a
-	 * "compatible",
-	 */
-	for_each_matching_node(node, reserved_mem_matches)
-		of_platform_device_create(node, NULL, NULL);
+	if (IS_ENABLED(CONFIG_PPC)) {
+		struct device_node *boot_display = NULL;
+		struct device_node *node;
+		struct platform_device *dev;
+		int ret;
+
+		/* Check if we have a MacOS display without a node spec */
+		if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
+			/*
+			 * The old code tried to work out which node was the MacOS
+			 * display based on the address. I'm dropping that since the
+			 * lack of a node spec only happens with old BootX versions
+			 * (users can update) and with this code, they'll still get
+			 * a display (just not the palette hacks).
+			 */
+			dev = platform_device_alloc("bootx-noscreen", 0);
+			if (WARN_ON(!dev))
+				return -ENOMEM;
+			ret = platform_device_add(dev);
+			if (WARN_ON(ret)) {
+				platform_device_put(dev);
+				return ret;
+			}
+		}
 
-	node = of_find_node_by_path("/firmware");
-	if (node) {
-		of_platform_populate(node, NULL, NULL, NULL);
-		of_node_put(node);
-	}
+		/*
+		 * For OF framebuffers, first create the device for the boot display,
+		 * then for the other framebuffers. Only fail for the boot display;
+		 * ignore errors for the rest.
+		 */
+		for_each_node_by_type(node, "display") {
+			if (!of_get_property(node, "linux,opened", NULL) ||
+			    !of_get_property(node, "linux,boot-display", NULL))
+				continue;
+			dev = of_platform_device_create(node, "of-display", NULL);
+			if (WARN_ON(!dev))
+				return -ENOMEM;
+			boot_display = node;
+			break;
+		}
+		for_each_node_by_type(node, "display") {
+			if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
+				continue;
+			of_platform_device_create(node, "of-display", NULL);
+		}
 
-	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
-	of_platform_device_create(node, NULL, NULL);
-	of_node_put(node);
+	} else {
+		struct device_node *node;
+
+		/*
+		 * Handle certain compatibles explicitly, since we don't want to create
+		 * platform_devices for every node in /reserved-memory with a
+		 * "compatible",
+		 */
+		for_each_matching_node(node, reserved_mem_matches)
+			of_platform_device_create(node, NULL, NULL);
 
-	/* Populate everything else. */
-	of_platform_default_populate(NULL, NULL, NULL);
+		node = of_find_node_by_path("/firmware");
+		if (node) {
+			of_platform_populate(node, NULL, NULL, NULL);
+			of_node_put(node);
+		}
+
+		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
+		of_platform_device_create(node, NULL, NULL);
+		of_node_put(node);
+
+		/* Populate everything else. */
+		of_platform_default_populate(NULL, NULL, NULL);
+	}
 
 	return 0;
 }
@@ -558,7 +605,6 @@ static int __init of_platform_sync_state_init(void)
 	return 0;
 }
 late_initcall_sync(of_platform_sync_state_init);
-#endif
 
 int of_platform_device_destroy(struct device *dev, void *data)
 {
diff --git a/drivers/video/fbdev/offb.c b/drivers/video/fbdev/offb.c
index afdb6aa48add..b1acb1ebebe9 100644
--- a/drivers/video/fbdev/offb.c
+++ b/drivers/video/fbdev/offb.c
@@ -386,10 +386,10 @@ static void offb_init_palette_hacks(struct fb_info *info, struct device_node *dp
 		FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_STATIC_PSEUDOCOLOR;
 }
 
-static void __init offb_init_fb(const char *name,
-				int width, int height, int depth,
-				int pitch, unsigned long address,
-				int foreign_endian, struct device_node *dp)
+static void offb_init_fb(struct platform_device *parent, const char *name,
+			 int width, int height, int depth,
+			 int pitch, unsigned long address,
+			 int foreign_endian, struct device_node *dp)
 {
 	unsigned long res_size = pitch * height;
 	struct offb_par *par = &default_par;
@@ -410,12 +410,13 @@ static void __init offb_init_fb(const char *name,
 		return;
 	}
 
-	info = framebuffer_alloc(sizeof(u32) * 16, NULL);
+	info = framebuffer_alloc(sizeof(u32) * 16, &parent->dev);
 
 	if (!info) {
 		release_mem_region(res_start, res_size);
 		return;
 	}
+	platform_set_drvdata(parent, info);
 
 	fix = &info->fix;
 	var = &info->var;
@@ -535,7 +536,8 @@ static void __init offb_init_fb(const char *name,
 }
 
 
-static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
+static void offb_init_nodriver(struct platform_device *parent, struct device_node *dp,
+			       int no_real_node)
 {
 	unsigned int len;
 	int i, width = 640, height = 480, depth = 8, pitch = 640;
@@ -650,46 +652,76 @@ static void __init offb_init_nodriver(struct device_node *dp, int no_real_node)
 		/* kludge for valkyrie */
 		if (of_node_name_eq(dp, "valkyrie"))
 			address += 0x1000;
-		offb_init_fb(no_real_node ? "bootx" : NULL,
+		offb_init_fb(parent, no_real_node ? "bootx" : NULL,
 			     width, height, depth, pitch, address,
 			     foreign_endian, no_real_node ? NULL : dp);
 	}
 }
 
-static int __init offb_init(void)
+static int offb_remove(struct platform_device *pdev)
 {
-	struct device_node *dp = NULL, *boot_disp = NULL;
+	struct fb_info *info = platform_get_drvdata(pdev);
 
-	if (fb_get_options("offb", NULL))
-		return -ENODEV;
+	if (info)
+		unregister_framebuffer(info);
 
-	/* Check if we have a MacOS display without a node spec */
-	if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL) != NULL) {
-		/* The old code tried to work out which node was the MacOS
-		 * display based on the address. I'm dropping that since the
-		 * lack of a node spec only happens with old BootX versions
-		 * (users can update) and with this code, they'll still get
-		 * a display (just not the palette hacks).
-		 */
-		offb_init_nodriver(of_chosen, 1);
-	}
+	return 0;
+}
 
-	for_each_node_by_type(dp, "display") {
-		if (of_get_property(dp, "linux,opened", NULL) &&
-		    of_get_property(dp, "linux,boot-display", NULL)) {
-			boot_disp = dp;
-			offb_init_nodriver(dp, 0);
-		}
-	}
-	for_each_node_by_type(dp, "display") {
-		if (of_get_property(dp, "linux,opened", NULL) &&
-		    dp != boot_disp)
-			offb_init_nodriver(dp, 0);
-	}
+static int offb_probe_bootx_noscreen(struct platform_device *pdev)
+{
+	offb_init_nodriver(pdev, of_chosen, 1);
 
 	return 0;
 }
 
+static struct platform_driver offb_driver_bootx_noscreen = {
+	.driver = {
+		.name = "bootx-noscreen",
+	},
+	.probe = offb_probe_bootx_noscreen,
+	.remove = offb_remove,
+};
+
+static int offb_probe_display(struct platform_device *pdev)
+{
+	offb_init_nodriver(pdev, pdev->dev.of_node, 0);
+
+	return 0;
+}
 
+static const struct of_device_id offb_of_match_display[] = {
+	{ .compatible = "display", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, offb_of_match_display);
+
+static struct platform_driver offb_driver_display = {
+	.driver = {
+		.name = "of-display",
+		.of_match_table = offb_of_match_display,
+	},
+	.probe = offb_probe_display,
+	.remove = offb_remove,
+};
+
+static int __init offb_init(void)
+{
+	if (fb_get_options("offb", NULL))
+		return -ENODEV;
+
+	platform_driver_register(&offb_driver_bootx_noscreen);
+	platform_driver_register(&offb_driver_display);
+
+	return 0;
+}
 module_init(offb_init);
+
+static void __exit offb_exit(void)
+{
+	platform_driver_unregister(&offb_driver_display);
+	platform_driver_unregister(&offb_driver_bootx_noscreen);
+}
+module_exit(offb_exit);
+
 MODULE_LICENSE("GPL");
-- 
2.35.1

Re: [PATCH v2 1/2] of: Create platform devices for OF framebuffers

[Rob Herring]

On Tue, Apr 19, 2022 at 12:04:04PM +0200, Thomas Zimmermann wrote:
> Create a platform device for each OF-declared framebuffer and have
> offb bind to these devices. Allows for real hot-unplugging and other
> drivers besides offb.
> 
> Originally, offb created framebuffer devices while initializing its
> module by parsing the OF device tree. No actual Linux device was set
> up. This tied OF framebuffers to offb and makes writing other drivers
> for the OF framebuffers complicated. The absence of a Linux device
> further prevented real hot-unplugging. Adding a distinct platform
> device for each OF framebuffer solves both problems. Specifically, a
> DRM driver can now provide graphics output for modern userspace.
> 
> Some of the offb init code is now located in the OF initialization.
> There's now also an implementation of of_platform_default_populate_init(),
> which was missing before. The OF side creates different devices for
> either OF display nodes or BootX displays as they require different
> handling by the driver. The offb drivers picks up each type of device
> and runs the appropriate fbdev initialization.
> 
> Tested with OF display nodes on qemu's ppc64le target.
> 
> v2:
> 	* run PPC code as part of existing initialization (Rob)
> 	* add a few more error warnings (Javier)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/of/platform.c      | 88 ++++++++++++++++++++++++++--------
>  drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++-------------
>  2 files changed, 132 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index a16b74f32aa9..738ba2e2838c 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -507,7 +507,6 @@ int of_platform_default_populate(struct device_node *root,
>  }
>  EXPORT_SYMBOL_GPL(of_platform_default_populate);
>  
> -#ifndef CONFIG_PPC
>  static const struct of_device_id reserved_mem_matches[] = {
>  	{ .compatible = "qcom,rmtfs-mem" },
>  	{ .compatible = "qcom,cmd-db" },
> @@ -520,33 +519,81 @@ static const struct of_device_id reserved_mem_matches[] = {
>  
>  static int __init of_platform_default_populate_init(void)
>  {
> -	struct device_node *node;
> -

As both if/else clauses need 'node', I'd keep this declared here.

>  	device_links_supplier_sync_state_pause();
>  
>  	if (!of_have_populated_dt())
>  		return -ENODEV;
>  
> -	/*
> -	 * Handle certain compatibles explicitly, since we don't want to create
> -	 * platform_devices for every node in /reserved-memory with a
> -	 * "compatible",
> -	 */
> -	for_each_matching_node(node, reserved_mem_matches)
> -		of_platform_device_create(node, NULL, NULL);
> +	if (IS_ENABLED(CONFIG_PPC)) {
> +		struct device_node *boot_display = NULL;
> +		struct device_node *node;
> +		struct platform_device *dev;
> +		int ret;
> +
> +		/* Check if we have a MacOS display without a node spec */
> +		if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
> +			/*
> +			 * The old code tried to work out which node was the MacOS
> +			 * display based on the address. I'm dropping that since the
> +			 * lack of a node spec only happens with old BootX versions
> +			 * (users can update) and with this code, they'll still get
> +			 * a display (just not the palette hacks).
> +			 */
> +			dev = platform_device_alloc("bootx-noscreen", 0);
> +			if (WARN_ON(!dev))
> +				return -ENOMEM;
> +			ret = platform_device_add(dev);
> +			if (WARN_ON(ret)) {
> +				platform_device_put(dev);
> +				return ret;
> +			}
> +		}
>  
> -	node = of_find_node_by_path("/firmware");
> -	if (node) {
> -		of_platform_populate(node, NULL, NULL, NULL);
> -		of_node_put(node);
> -	}
> +		/*
> +		 * For OF framebuffers, first create the device for the boot display,
> +		 * then for the other framebuffers. Only fail for the boot display;
> +		 * ignore errors for the rest.
> +		 */
> +		for_each_node_by_type(node, "display") {
> +			if (!of_get_property(node, "linux,opened", NULL) ||
> +			    !of_get_property(node, "linux,boot-display", NULL))
> +				continue;
> +			dev = of_platform_device_create(node, "of-display", NULL);
> +			if (WARN_ON(!dev))
> +				return -ENOMEM;
> +			boot_display = node;
> +			break;
> +		}
> +		for_each_node_by_type(node, "display") {
> +			if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
> +				continue;
> +			of_platform_device_create(node, "of-display", NULL);
> +		}
>  
> -	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> -	of_platform_device_create(node, NULL, NULL);
> -	of_node_put(node);
> +	} else {
> +		struct device_node *node;
> +
> +		/*
> +		 * Handle certain compatibles explicitly, since we don't want to create
> +		 * platform_devices for every node in /reserved-memory with a
> +		 * "compatible",
> +		 */
> +		for_each_matching_node(node, reserved_mem_matches)
> +			of_platform_device_create(node, NULL, NULL);
>  
> -	/* Populate everything else. */
> -	of_platform_default_populate(NULL, NULL, NULL);
> +		node = of_find_node_by_path("/firmware");
> +		if (node) {
> +			of_platform_populate(node, NULL, NULL, NULL);
> +			of_node_put(node);
> +		}
> +
> +		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> +		of_platform_device_create(node, NULL, NULL);
> +		of_node_put(node);

In v1, you supported "simple-framebuffer" on PPC. Don't we want to allow 
that? Maybe no one cares ATM, but that could change. Either way:

Reviewed-by: Rob Herring <robh@kernel.org>


> +
> +		/* Populate everything else. */
> +		of_platform_default_populate(NULL, NULL, NULL);
> +	}
>  
>  	return 0;
>  }
> @@ -558,7 +605,6 @@ static int __init of_platform_sync_state_init(void)
>  	return 0;
>  }
>  late_initcall_sync(of_platform_sync_state_init);
> -#endif
>  
>  int of_platform_device_destroy(struct device *dev, void *data)
>  {

Re: [PATCH v2 1/2] of: Create platform devices for OF framebuffers

[Rob Herring]

On Tue, Apr 19, 2022 at 12:04:04PM +0200, Thomas Zimmermann wrote:
> Create a platform device for each OF-declared framebuffer and have
> offb bind to these devices. Allows for real hot-unplugging and other
> drivers besides offb.
> 
> Originally, offb created framebuffer devices while initializing its
> module by parsing the OF device tree. No actual Linux device was set
> up. This tied OF framebuffers to offb and makes writing other drivers
> for the OF framebuffers complicated. The absence of a Linux device
> further prevented real hot-unplugging. Adding a distinct platform
> device for each OF framebuffer solves both problems. Specifically, a
> DRM driver can now provide graphics output for modern userspace.
> 
> Some of the offb init code is now located in the OF initialization.
> There's now also an implementation of of_platform_default_populate_init(),
> which was missing before. The OF side creates different devices for
> either OF display nodes or BootX displays as they require different
> handling by the driver. The offb drivers picks up each type of device
> and runs the appropriate fbdev initialization.
> 
> Tested with OF display nodes on qemu's ppc64le target.
> 
> v2:
> 	* run PPC code as part of existing initialization (Rob)
> 	* add a few more error warnings (Javier)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/of/platform.c      | 88 ++++++++++++++++++++++++++--------
>  drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++-------------
>  2 files changed, 132 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index a16b74f32aa9..738ba2e2838c 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -507,7 +507,6 @@ int of_platform_default_populate(struct device_node *root,
>  }
>  EXPORT_SYMBOL_GPL(of_platform_default_populate);
>  
> -#ifndef CONFIG_PPC
>  static const struct of_device_id reserved_mem_matches[] = {
>  	{ .compatible = "qcom,rmtfs-mem" },
>  	{ .compatible = "qcom,cmd-db" },
> @@ -520,33 +519,81 @@ static const struct of_device_id reserved_mem_matches[] = {
>  
>  static int __init of_platform_default_populate_init(void)
>  {
> -	struct device_node *node;
> -

As both if/else clauses need 'node', I'd keep this declared here.

>  	device_links_supplier_sync_state_pause();
>  
>  	if (!of_have_populated_dt())
>  		return -ENODEV;
>  
> -	/*
> -	 * Handle certain compatibles explicitly, since we don't want to create
> -	 * platform_devices for every node in /reserved-memory with a
> -	 * "compatible",
> -	 */
> -	for_each_matching_node(node, reserved_mem_matches)
> -		of_platform_device_create(node, NULL, NULL);
> +	if (IS_ENABLED(CONFIG_PPC)) {
> +		struct device_node *boot_display = NULL;
> +		struct device_node *node;
> +		struct platform_device *dev;
> +		int ret;
> +
> +		/* Check if we have a MacOS display without a node spec */
> +		if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
> +			/*
> +			 * The old code tried to work out which node was the MacOS
> +			 * display based on the address. I'm dropping that since the
> +			 * lack of a node spec only happens with old BootX versions
> +			 * (users can update) and with this code, they'll still get
> +			 * a display (just not the palette hacks).
> +			 */
> +			dev = platform_device_alloc("bootx-noscreen", 0);
> +			if (WARN_ON(!dev))
> +				return -ENOMEM;
> +			ret = platform_device_add(dev);
> +			if (WARN_ON(ret)) {
> +				platform_device_put(dev);
> +				return ret;
> +			}
> +		}
>  
> -	node = of_find_node_by_path("/firmware");
> -	if (node) {
> -		of_platform_populate(node, NULL, NULL, NULL);
> -		of_node_put(node);
> -	}
> +		/*
> +		 * For OF framebuffers, first create the device for the boot display,
> +		 * then for the other framebuffers. Only fail for the boot display;
> +		 * ignore errors for the rest.
> +		 */
> +		for_each_node_by_type(node, "display") {
> +			if (!of_get_property(node, "linux,opened", NULL) ||
> +			    !of_get_property(node, "linux,boot-display", NULL))
> +				continue;
> +			dev = of_platform_device_create(node, "of-display", NULL);
> +			if (WARN_ON(!dev))
> +				return -ENOMEM;
> +			boot_display = node;
> +			break;
> +		}
> +		for_each_node_by_type(node, "display") {
> +			if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
> +				continue;
> +			of_platform_device_create(node, "of-display", NULL);
> +		}
>  
> -	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> -	of_platform_device_create(node, NULL, NULL);
> -	of_node_put(node);
> +	} else {
> +		struct device_node *node;
> +
> +		/*
> +		 * Handle certain compatibles explicitly, since we don't want to create
> +		 * platform_devices for every node in /reserved-memory with a
> +		 * "compatible",
> +		 */
> +		for_each_matching_node(node, reserved_mem_matches)
> +			of_platform_device_create(node, NULL, NULL);
>  
> -	/* Populate everything else. */
> -	of_platform_default_populate(NULL, NULL, NULL);
> +		node = of_find_node_by_path("/firmware");
> +		if (node) {
> +			of_platform_populate(node, NULL, NULL, NULL);
> +			of_node_put(node);
> +		}
> +
> +		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> +		of_platform_device_create(node, NULL, NULL);
> +		of_node_put(node);

In v1, you supported "simple-framebuffer" on PPC. Don't we want to allow 
that? Maybe no one cares ATM, but that could change. Either way:

Reviewed-by: Rob Herring <robh@kernel.org>


> +
> +		/* Populate everything else. */
> +		of_platform_default_populate(NULL, NULL, NULL);
> +	}
>  
>  	return 0;
>  }
> @@ -558,7 +605,6 @@ static int __init of_platform_sync_state_init(void)
>  	return 0;
>  }
>  late_initcall_sync(of_platform_sync_state_init);
> -#endif
>  
>  int of_platform_device_destroy(struct device *dev, void *data)
>  {

Re: [PATCH v2 1/2] of: Create platform devices for OF framebuffers

[Rob Herring]

On Tue, Apr 19, 2022 at 12:04:04PM +0200, Thomas Zimmermann wrote:
> Create a platform device for each OF-declared framebuffer and have
> offb bind to these devices. Allows for real hot-unplugging and other
> drivers besides offb.
> 
> Originally, offb created framebuffer devices while initializing its
> module by parsing the OF device tree. No actual Linux device was set
> up. This tied OF framebuffers to offb and makes writing other drivers
> for the OF framebuffers complicated. The absence of a Linux device
> further prevented real hot-unplugging. Adding a distinct platform
> device for each OF framebuffer solves both problems. Specifically, a
> DRM driver can now provide graphics output for modern userspace.
> 
> Some of the offb init code is now located in the OF initialization.
> There's now also an implementation of of_platform_default_populate_init(),
> which was missing before. The OF side creates different devices for
> either OF display nodes or BootX displays as they require different
> handling by the driver. The offb drivers picks up each type of device
> and runs the appropriate fbdev initialization.
> 
> Tested with OF display nodes on qemu's ppc64le target.
> 
> v2:
> 	* run PPC code as part of existing initialization (Rob)
> 	* add a few more error warnings (Javier)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/of/platform.c      | 88 ++++++++++++++++++++++++++--------
>  drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++-------------
>  2 files changed, 132 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index a16b74f32aa9..738ba2e2838c 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -507,7 +507,6 @@ int of_platform_default_populate(struct device_node *root,
>  }
>  EXPORT_SYMBOL_GPL(of_platform_default_populate);
>  
> -#ifndef CONFIG_PPC
>  static const struct of_device_id reserved_mem_matches[] = {
>  	{ .compatible = "qcom,rmtfs-mem" },
>  	{ .compatible = "qcom,cmd-db" },
> @@ -520,33 +519,81 @@ static const struct of_device_id reserved_mem_matches[] = {
>  
>  static int __init of_platform_default_populate_init(void)
>  {
> -	struct device_node *node;
> -

As both if/else clauses need 'node', I'd keep this declared here.

>  	device_links_supplier_sync_state_pause();
>  
>  	if (!of_have_populated_dt())
>  		return -ENODEV;
>  
> -	/*
> -	 * Handle certain compatibles explicitly, since we don't want to create
> -	 * platform_devices for every node in /reserved-memory with a
> -	 * "compatible",
> -	 */
> -	for_each_matching_node(node, reserved_mem_matches)
> -		of_platform_device_create(node, NULL, NULL);
> +	if (IS_ENABLED(CONFIG_PPC)) {
> +		struct device_node *boot_display = NULL;
> +		struct device_node *node;
> +		struct platform_device *dev;
> +		int ret;
> +
> +		/* Check if we have a MacOS display without a node spec */
> +		if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
> +			/*
> +			 * The old code tried to work out which node was the MacOS
> +			 * display based on the address. I'm dropping that since the
> +			 * lack of a node spec only happens with old BootX versions
> +			 * (users can update) and with this code, they'll still get
> +			 * a display (just not the palette hacks).
> +			 */
> +			dev = platform_device_alloc("bootx-noscreen", 0);
> +			if (WARN_ON(!dev))
> +				return -ENOMEM;
> +			ret = platform_device_add(dev);
> +			if (WARN_ON(ret)) {
> +				platform_device_put(dev);
> +				return ret;
> +			}
> +		}
>  
> -	node = of_find_node_by_path("/firmware");
> -	if (node) {
> -		of_platform_populate(node, NULL, NULL, NULL);
> -		of_node_put(node);
> -	}
> +		/*
> +		 * For OF framebuffers, first create the device for the boot display,
> +		 * then for the other framebuffers. Only fail for the boot display;
> +		 * ignore errors for the rest.
> +		 */
> +		for_each_node_by_type(node, "display") {
> +			if (!of_get_property(node, "linux,opened", NULL) ||
> +			    !of_get_property(node, "linux,boot-display", NULL))
> +				continue;
> +			dev = of_platform_device_create(node, "of-display", NULL);
> +			if (WARN_ON(!dev))
> +				return -ENOMEM;
> +			boot_display = node;
> +			break;
> +		}
> +		for_each_node_by_type(node, "display") {
> +			if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
> +				continue;
> +			of_platform_device_create(node, "of-display", NULL);
> +		}
>  
> -	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> -	of_platform_device_create(node, NULL, NULL);
> -	of_node_put(node);
> +	} else {
> +		struct device_node *node;
> +
> +		/*
> +		 * Handle certain compatibles explicitly, since we don't want to create
> +		 * platform_devices for every node in /reserved-memory with a
> +		 * "compatible",
> +		 */
> +		for_each_matching_node(node, reserved_mem_matches)
> +			of_platform_device_create(node, NULL, NULL);
>  
> -	/* Populate everything else. */
> -	of_platform_default_populate(NULL, NULL, NULL);
> +		node = of_find_node_by_path("/firmware");
> +		if (node) {
> +			of_platform_populate(node, NULL, NULL, NULL);
> +			of_node_put(node);
> +		}
> +
> +		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
> +		of_platform_device_create(node, NULL, NULL);
> +		of_node_put(node);

In v1, you supported "simple-framebuffer" on PPC. Don't we want to allow 
that? Maybe no one cares ATM, but that could change. Either way:

Reviewed-by: Rob Herring <robh@kernel.org>


> +
> +		/* Populate everything else. */
> +		of_platform_default_populate(NULL, NULL, NULL);
> +	}
>  
>  	return 0;
>  }
> @@ -558,7 +605,6 @@ static int __init of_platform_sync_state_init(void)
>  	return 0;
>  }
>  late_initcall_sync(of_platform_sync_state_init);
> -#endif
>  
>  int of_platform_device_destroy(struct device *dev, void *data)
>  {

Re: [PATCH v2 1/2] of: Create platform devices for OF framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4758 bytes --]

Hi

Am 19.04.22 um 15:30 schrieb Rob Herring:
...
>> -#ifndef CONFIG_PPC
>>   static const struct of_device_id reserved_mem_matches[] = {
>>   	{ .compatible = "qcom,rmtfs-mem" },
>>   	{ .compatible = "qcom,cmd-db" },
>> @@ -520,33 +519,81 @@ static const struct of_device_id reserved_mem_matches[] = {
>>   
>>   static int __init of_platform_default_populate_init(void)
>>   {
>> -	struct device_node *node;
>> -
> 
> As both if/else clauses need 'node', I'd keep this declared here.

Ok.

> 
>>   	device_links_supplier_sync_state_pause();
>>   
>>   	if (!of_have_populated_dt())
>>   		return -ENODEV;
>>   
>> -	/*
>> -	 * Handle certain compatibles explicitly, since we don't want to create
>> -	 * platform_devices for every node in /reserved-memory with a
>> -	 * "compatible",
>> -	 */
>> -	for_each_matching_node(node, reserved_mem_matches)
>> -		of_platform_device_create(node, NULL, NULL);
>> +	if (IS_ENABLED(CONFIG_PPC)) {
>> +		struct device_node *boot_display = NULL;
>> +		struct device_node *node;
>> +		struct platform_device *dev;
>> +		int ret;
>> +
>> +		/* Check if we have a MacOS display without a node spec */
>> +		if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
>> +			/*
>> +			 * The old code tried to work out which node was the MacOS
>> +			 * display based on the address. I'm dropping that since the
>> +			 * lack of a node spec only happens with old BootX versions
>> +			 * (users can update) and with this code, they'll still get
>> +			 * a display (just not the palette hacks).
>> +			 */
>> +			dev = platform_device_alloc("bootx-noscreen", 0);
>> +			if (WARN_ON(!dev))
>> +				return -ENOMEM;
>> +			ret = platform_device_add(dev);
>> +			if (WARN_ON(ret)) {
>> +				platform_device_put(dev);
>> +				return ret;
>> +			}
>> +		}
>>   
>> -	node = of_find_node_by_path("/firmware");
>> -	if (node) {
>> -		of_platform_populate(node, NULL, NULL, NULL);
>> -		of_node_put(node);
>> -	}
>> +		/*
>> +		 * For OF framebuffers, first create the device for the boot display,
>> +		 * then for the other framebuffers. Only fail for the boot display;
>> +		 * ignore errors for the rest.
>> +		 */
>> +		for_each_node_by_type(node, "display") {
>> +			if (!of_get_property(node, "linux,opened", NULL) ||
>> +			    !of_get_property(node, "linux,boot-display", NULL))
>> +				continue;
>> +			dev = of_platform_device_create(node, "of-display", NULL);
>> +			if (WARN_ON(!dev))
>> +				return -ENOMEM;
>> +			boot_display = node;
>> +			break;
>> +		}
>> +		for_each_node_by_type(node, "display") {
>> +			if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
>> +				continue;
>> +			of_platform_device_create(node, "of-display", NULL);
>> +		}
>>   
>> -	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
>> -	of_platform_device_create(node, NULL, NULL);
>> -	of_node_put(node);
>> +	} else {
>> +		struct device_node *node;
>> +
>> +		/*
>> +		 * Handle certain compatibles explicitly, since we don't want to create
>> +		 * platform_devices for every node in /reserved-memory with a
>> +		 * "compatible",
>> +		 */
>> +		for_each_matching_node(node, reserved_mem_matches)
>> +			of_platform_device_create(node, NULL, NULL);
>>   
>> -	/* Populate everything else. */
>> -	of_platform_default_populate(NULL, NULL, NULL);
>> +		node = of_find_node_by_path("/firmware");
>> +		if (node) {
>> +			of_platform_populate(node, NULL, NULL, NULL);
>> +			of_node_put(node);
>> +		}
>> +
>> +		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
>> +		of_platform_device_create(node, NULL, NULL);
>> +		of_node_put(node);
> 
> In v1, you supported "simple-framebuffer" on PPC. Don't we want to allow
> that? Maybe no one cares ATM, but that could change. Either way:

Support for these framebuffers has always been mutually exclusive. The 
offb driver, which originally contained the code, depends on CONFIG_PPC. 
And PPC never supported simple-framebuffer anywhere.

> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Thank you.

Best regards
Thomas

> 
> 
>> +
>> +		/* Populate everything else. */
>> +		of_platform_default_populate(NULL, NULL, NULL);
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -558,7 +605,6 @@ static int __init of_platform_sync_state_init(void)
>>   	return 0;
>>   }
>>   late_initcall_sync(of_platform_sync_state_init);
>> -#endif
>>   
>>   int of_platform_device_destroy(struct device *dev, void *data)
>>   {

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/2] of: Create platform devices for OF framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4758 bytes --]

Hi

Am 19.04.22 um 15:30 schrieb Rob Herring:
...
>> -#ifndef CONFIG_PPC
>>   static const struct of_device_id reserved_mem_matches[] = {
>>   	{ .compatible = "qcom,rmtfs-mem" },
>>   	{ .compatible = "qcom,cmd-db" },
>> @@ -520,33 +519,81 @@ static const struct of_device_id reserved_mem_matches[] = {
>>   
>>   static int __init of_platform_default_populate_init(void)
>>   {
>> -	struct device_node *node;
>> -
> 
> As both if/else clauses need 'node', I'd keep this declared here.

Ok.

> 
>>   	device_links_supplier_sync_state_pause();
>>   
>>   	if (!of_have_populated_dt())
>>   		return -ENODEV;
>>   
>> -	/*
>> -	 * Handle certain compatibles explicitly, since we don't want to create
>> -	 * platform_devices for every node in /reserved-memory with a
>> -	 * "compatible",
>> -	 */
>> -	for_each_matching_node(node, reserved_mem_matches)
>> -		of_platform_device_create(node, NULL, NULL);
>> +	if (IS_ENABLED(CONFIG_PPC)) {
>> +		struct device_node *boot_display = NULL;
>> +		struct device_node *node;
>> +		struct platform_device *dev;
>> +		int ret;
>> +
>> +		/* Check if we have a MacOS display without a node spec */
>> +		if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
>> +			/*
>> +			 * The old code tried to work out which node was the MacOS
>> +			 * display based on the address. I'm dropping that since the
>> +			 * lack of a node spec only happens with old BootX versions
>> +			 * (users can update) and with this code, they'll still get
>> +			 * a display (just not the palette hacks).
>> +			 */
>> +			dev = platform_device_alloc("bootx-noscreen", 0);
>> +			if (WARN_ON(!dev))
>> +				return -ENOMEM;
>> +			ret = platform_device_add(dev);
>> +			if (WARN_ON(ret)) {
>> +				platform_device_put(dev);
>> +				return ret;
>> +			}
>> +		}
>>   
>> -	node = of_find_node_by_path("/firmware");
>> -	if (node) {
>> -		of_platform_populate(node, NULL, NULL, NULL);
>> -		of_node_put(node);
>> -	}
>> +		/*
>> +		 * For OF framebuffers, first create the device for the boot display,
>> +		 * then for the other framebuffers. Only fail for the boot display;
>> +		 * ignore errors for the rest.
>> +		 */
>> +		for_each_node_by_type(node, "display") {
>> +			if (!of_get_property(node, "linux,opened", NULL) ||
>> +			    !of_get_property(node, "linux,boot-display", NULL))
>> +				continue;
>> +			dev = of_platform_device_create(node, "of-display", NULL);
>> +			if (WARN_ON(!dev))
>> +				return -ENOMEM;
>> +			boot_display = node;
>> +			break;
>> +		}
>> +		for_each_node_by_type(node, "display") {
>> +			if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
>> +				continue;
>> +			of_platform_device_create(node, "of-display", NULL);
>> +		}
>>   
>> -	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
>> -	of_platform_device_create(node, NULL, NULL);
>> -	of_node_put(node);
>> +	} else {
>> +		struct device_node *node;
>> +
>> +		/*
>> +		 * Handle certain compatibles explicitly, since we don't want to create
>> +		 * platform_devices for every node in /reserved-memory with a
>> +		 * "compatible",
>> +		 */
>> +		for_each_matching_node(node, reserved_mem_matches)
>> +			of_platform_device_create(node, NULL, NULL);
>>   
>> -	/* Populate everything else. */
>> -	of_platform_default_populate(NULL, NULL, NULL);
>> +		node = of_find_node_by_path("/firmware");
>> +		if (node) {
>> +			of_platform_populate(node, NULL, NULL, NULL);
>> +			of_node_put(node);
>> +		}
>> +
>> +		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
>> +		of_platform_device_create(node, NULL, NULL);
>> +		of_node_put(node);
> 
> In v1, you supported "simple-framebuffer" on PPC. Don't we want to allow
> that? Maybe no one cares ATM, but that could change. Either way:

Support for these framebuffers has always been mutually exclusive. The 
offb driver, which originally contained the code, depends on CONFIG_PPC. 
And PPC never supported simple-framebuffer anywhere.

> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Thank you.

Best regards
Thomas

> 
> 
>> +
>> +		/* Populate everything else. */
>> +		of_platform_default_populate(NULL, NULL, NULL);
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -558,7 +605,6 @@ static int __init of_platform_sync_state_init(void)
>>   	return 0;
>>   }
>>   late_initcall_sync(of_platform_sync_state_init);
>> -#endif
>>   
>>   int of_platform_device_destroy(struct device *dev, void *data)
>>   {

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/2] of: Create platform devices for OF framebuffers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4758 bytes --]

Hi

Am 19.04.22 um 15:30 schrieb Rob Herring:
...
>> -#ifndef CONFIG_PPC
>>   static const struct of_device_id reserved_mem_matches[] = {
>>   	{ .compatible = "qcom,rmtfs-mem" },
>>   	{ .compatible = "qcom,cmd-db" },
>> @@ -520,33 +519,81 @@ static const struct of_device_id reserved_mem_matches[] = {
>>   
>>   static int __init of_platform_default_populate_init(void)
>>   {
>> -	struct device_node *node;
>> -
> 
> As both if/else clauses need 'node', I'd keep this declared here.

Ok.

> 
>>   	device_links_supplier_sync_state_pause();
>>   
>>   	if (!of_have_populated_dt())
>>   		return -ENODEV;
>>   
>> -	/*
>> -	 * Handle certain compatibles explicitly, since we don't want to create
>> -	 * platform_devices for every node in /reserved-memory with a
>> -	 * "compatible",
>> -	 */
>> -	for_each_matching_node(node, reserved_mem_matches)
>> -		of_platform_device_create(node, NULL, NULL);
>> +	if (IS_ENABLED(CONFIG_PPC)) {
>> +		struct device_node *boot_display = NULL;
>> +		struct device_node *node;
>> +		struct platform_device *dev;
>> +		int ret;
>> +
>> +		/* Check if we have a MacOS display without a node spec */
>> +		if (of_get_property(of_chosen, "linux,bootx-noscreen", NULL)) {
>> +			/*
>> +			 * The old code tried to work out which node was the MacOS
>> +			 * display based on the address. I'm dropping that since the
>> +			 * lack of a node spec only happens with old BootX versions
>> +			 * (users can update) and with this code, they'll still get
>> +			 * a display (just not the palette hacks).
>> +			 */
>> +			dev = platform_device_alloc("bootx-noscreen", 0);
>> +			if (WARN_ON(!dev))
>> +				return -ENOMEM;
>> +			ret = platform_device_add(dev);
>> +			if (WARN_ON(ret)) {
>> +				platform_device_put(dev);
>> +				return ret;
>> +			}
>> +		}
>>   
>> -	node = of_find_node_by_path("/firmware");
>> -	if (node) {
>> -		of_platform_populate(node, NULL, NULL, NULL);
>> -		of_node_put(node);
>> -	}
>> +		/*
>> +		 * For OF framebuffers, first create the device for the boot display,
>> +		 * then for the other framebuffers. Only fail for the boot display;
>> +		 * ignore errors for the rest.
>> +		 */
>> +		for_each_node_by_type(node, "display") {
>> +			if (!of_get_property(node, "linux,opened", NULL) ||
>> +			    !of_get_property(node, "linux,boot-display", NULL))
>> +				continue;
>> +			dev = of_platform_device_create(node, "of-display", NULL);
>> +			if (WARN_ON(!dev))
>> +				return -ENOMEM;
>> +			boot_display = node;
>> +			break;
>> +		}
>> +		for_each_node_by_type(node, "display") {
>> +			if (!of_get_property(node, "linux,opened", NULL) || node == boot_display)
>> +				continue;
>> +			of_platform_device_create(node, "of-display", NULL);
>> +		}
>>   
>> -	node = of_get_compatible_child(of_chosen, "simple-framebuffer");
>> -	of_platform_device_create(node, NULL, NULL);
>> -	of_node_put(node);
>> +	} else {
>> +		struct device_node *node;
>> +
>> +		/*
>> +		 * Handle certain compatibles explicitly, since we don't want to create
>> +		 * platform_devices for every node in /reserved-memory with a
>> +		 * "compatible",
>> +		 */
>> +		for_each_matching_node(node, reserved_mem_matches)
>> +			of_platform_device_create(node, NULL, NULL);
>>   
>> -	/* Populate everything else. */
>> -	of_platform_default_populate(NULL, NULL, NULL);
>> +		node = of_find_node_by_path("/firmware");
>> +		if (node) {
>> +			of_platform_populate(node, NULL, NULL, NULL);
>> +			of_node_put(node);
>> +		}
>> +
>> +		node = of_get_compatible_child(of_chosen, "simple-framebuffer");
>> +		of_platform_device_create(node, NULL, NULL);
>> +		of_node_put(node);
> 
> In v1, you supported "simple-framebuffer" on PPC. Don't we want to allow
> that? Maybe no one cares ATM, but that could change. Either way:

Support for these framebuffers has always been mutually exclusive. The 
offb driver, which originally contained the code, depends on CONFIG_PPC. 
And PPC never supported simple-framebuffer anywhere.

> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Thank you.

Best regards
Thomas

> 
> 
>> +
>> +		/* Populate everything else. */
>> +		of_platform_default_populate(NULL, NULL, NULL);
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -558,7 +605,6 @@ static int __init of_platform_sync_state_init(void)
>>   	return 0;
>>   }
>>   late_initcall_sync(of_platform_sync_state_init);
>> -#endif
>>   
>>   int of_platform_device_destroy(struct device *dev, void *data)
>>   {

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH v2 2/2] fbdev: Warn in hot-unplug workaround for framebuffers without device

[Thomas Zimmermann]

A workaround makes fbdev hot-unplugging work for framebuffers without
device. The only user for this feature was offb. As each OF framebuffer
now has an associated platform device, the workaround hould no longer
be triggered. Update it with a warning and rewrite the comment. Fbdev
drivers that trigger the hot-unplug workaround really need to be fixed.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/video/fbdev/core/fbmem.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index bc6ed750e915..84427470367b 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1577,14 +1577,12 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 			 * allocate the memory range.
 			 *
 			 * If it's not a platform device, at least print a warning. A
-			 * fix would add code to remove the device from the system.
+			 * fix would add code to remove the device from the system. For
+			 * framebuffers without any Linux device, print a warning as
+			 * well.
 			 */
 			if (!device) {
-				/* TODO: Represent each OF framebuffer as its own
-				 * device in the device hierarchy. For now, offb
-				 * doesn't have such a device, so unregister the
-				 * framebuffer as before without warning.
-				 */
+				pr_warn("fb%d: no device set\n", i);
 				do_unregister_framebuffer(registered_fb[i]);
 			} else if (dev_is_platform(device)) {
 				registered_fb[i]->forced_out = true;
-- 
2.35.1

[PATCH v2 2/2] fbdev: Warn in hot-unplug workaround for framebuffers without device

[Thomas Zimmermann]

A workaround makes fbdev hot-unplugging work for framebuffers without
device. The only user for this feature was offb. As each OF framebuffer
now has an associated platform device, the workaround hould no longer
be triggered. Update it with a warning and rewrite the comment. Fbdev
drivers that trigger the hot-unplug workaround really need to be fixed.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/video/fbdev/core/fbmem.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index bc6ed750e915..84427470367b 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1577,14 +1577,12 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 			 * allocate the memory range.
 			 *
 			 * If it's not a platform device, at least print a warning. A
-			 * fix would add code to remove the device from the system.
+			 * fix would add code to remove the device from the system. For
+			 * framebuffers without any Linux device, print a warning as
+			 * well.
 			 */
 			if (!device) {
-				/* TODO: Represent each OF framebuffer as its own
-				 * device in the device hierarchy. For now, offb
-				 * doesn't have such a device, so unregister the
-				 * framebuffer as before without warning.
-				 */
+				pr_warn("fb%d: no device set\n", i);
 				do_unregister_framebuffer(registered_fb[i]);
 			} else if (dev_is_platform(device)) {
 				registered_fb[i]->forced_out = true;
-- 
2.35.1

Re: [PATCH v2 2/2] fbdev: Warn in hot-unplug workaround for framebuffers without device

[Javier Martinez Canillas]

Hello Thomas,

Thanks a lot for re-spinning your series.

On 4/19/22 12:04, Thomas Zimmermann wrote:
> A workaround makes fbdev hot-unplugging work for framebuffers without
> device. The only user for this feature was offb. As each OF framebuffer
> now has an associated platform device, the workaround hould no longer
> be triggered. Update it with a warning and rewrite the comment. Fbdev
> drivers that trigger the hot-unplug workaround really need to be fixed.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH v2 2/2] fbdev: Warn in hot-unplug workaround for framebuffers without device

[Javier Martinez Canillas]

Hello Thomas,

Thanks a lot for re-spinning your series.

On 4/19/22 12:04, Thomas Zimmermann wrote:
> A workaround makes fbdev hot-unplugging work for framebuffers without
> device. The only user for this feature was offb. As each OF framebuffer
> now has an associated platform device, the workaround hould no longer
> be triggered. Update it with a warning and rewrite the comment. Fbdev
> drivers that trigger the hot-unplug workaround really need to be fixed.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Suggested-by: Javier Martinez Canillas <javierm@redhat.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Linux Engineering
Red Hat