| Message ID | 20220413092454.1073-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | of: Register platform device for each framebuffer | expand |
Hello Thomas, Thanks for working on this. On 4/13/22 11:24, 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 > prevented real hot-unplugging. Adding a distinct platform device for > each OF framebuffer solves both problems. Specifically, a DRM drivers > can now provide graphics output with 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. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- [snip] > + 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); Shouldn't check for the return value here too ? Other than this small nit, it looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi Am 13.04.22 um 12:45 schrieb Javier Martinez Canillas: > Hello Thomas, > > Thanks for working on this. > > On 4/13/22 11:24, 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 >> prevented real hot-unplugging. Adding a distinct platform device for >> each OF framebuffer solves both problems. Specifically, a DRM drivers >> can now provide graphics output with 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. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- > > [snip] > >> + 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); > > Shouldn't check for the return value here too ? Failing is probably not useful, as it's not the main FB used for booting. Printing an error message wouldn't hurt, I guess. I'll also update the new driver registration in offb with an error message. > > Other than this small nit, it looks good to me. > > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > Thanks. Best regards Thomas
On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann <tzimmermann@suse.de> 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 > prevented real hot-unplugging. Adding a distinct platform device for > each OF framebuffer solves both problems. Specifically, a DRM drivers > can now provide graphics output with 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. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/of/platform.c | 73 ++++++++++++++++++++++++++-- > drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++------------- > 2 files changed, 134 insertions(+), 37 deletions(-) > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index a16b74f32aa9..4c63b9a73587 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root, > } > EXPORT_SYMBOL(of_platform_bus_probe); > > +static int __init of_platform_populate_framebuffers(void) > +{ > + struct device_node *boot_display = NULL; > + struct device_node *node; > + struct platform_device *dev; > + int ret; > + > + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); > + of_platform_device_create(node, NULL, NULL); > + of_node_put(node); > + The rest is PPC only, so bail out here if !PPC. > + /* 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; > + } > + } > + > + /* > + * 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); > + } > + > + return 0; > +} > + > /** > * of_platform_populate() - Populate platform_devices from device tree data > * @root: parent of the first level to probe or NULL for the root of the tree > @@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void) > of_node_put(node); > } > > - node = of_get_compatible_child(of_chosen, "simple-framebuffer"); > - of_platform_device_create(node, NULL, NULL); > - of_node_put(node); > + of_platform_populate_framebuffers(); > > /* Populate everything else. */ > of_platform_default_populate(NULL, NULL, NULL); I'm pretty sure it's just this call that's the problem for PPC though none of the above existed when adding this caused a regression. Can we remove the ifdef and just make this call conditional on !IS_ENABLED(CONFIG_PPC). > @@ -551,6 +603,20 @@ static int __init of_platform_default_populate_init(void) > return 0; > } > arch_initcall_sync(of_platform_default_populate_init); > +#else > +static int __init of_platform_default_populate_init(void) > +{ > + device_links_supplier_sync_state_pause(); > + > + if (!of_have_populated_dt()) > + return -ENODEV; > + > + of_platform_populate_framebuffers(); > + > + return 0; > +} > +arch_initcall_sync(of_platform_default_populate_init); > +#endif > > static int __init of_platform_sync_state_init(void) > { > @@ -558,7 +624,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) > {
Hi Am 13.04.22 um 14:51 schrieb Rob Herring: > On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann <tzimmermann@suse.de> 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 >> prevented real hot-unplugging. Adding a distinct platform device for >> each OF framebuffer solves both problems. Specifically, a DRM drivers >> can now provide graphics output with 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. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/of/platform.c | 73 ++++++++++++++++++++++++++-- >> drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++------------- >> 2 files changed, 134 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c >> index a16b74f32aa9..4c63b9a73587 100644 >> --- a/drivers/of/platform.c >> +++ b/drivers/of/platform.c >> @@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root, >> } >> EXPORT_SYMBOL(of_platform_bus_probe); >> >> +static int __init of_platform_populate_framebuffers(void) >> +{ >> + struct device_node *boot_display = NULL; >> + struct device_node *node; >> + struct platform_device *dev; >> + int ret; >> + >> + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); >> + of_platform_device_create(node, NULL, NULL); >> + of_node_put(node); >> + > > The rest is PPC only, so bail out here if !PPC. > >> + /* 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; >> + } >> + } >> + >> + /* >> + * 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); >> + } >> + >> + return 0; >> +} >> + >> /** >> * of_platform_populate() - Populate platform_devices from device tree data >> * @root: parent of the first level to probe or NULL for the root of the tree >> @@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void) >> of_node_put(node); >> } >> >> - node = of_get_compatible_child(of_chosen, "simple-framebuffer"); >> - of_platform_device_create(node, NULL, NULL); >> - of_node_put(node); >> + of_platform_populate_framebuffers(); >> >> /* Populate everything else. */ >> of_platform_default_populate(NULL, NULL, NULL); > > I'm pretty sure it's just this call that's the problem for PPC though > none of the above existed when adding this caused a regression. Can we > remove the ifdef and just make this call conditional on > !IS_ENABLED(CONFIG_PPC). Together with the changes in of_platform_populate_framebuffers(), the code is more or less an "if-else" depending on PPC. I'll drop of_platform_populate_framebuffers() from the patch and make a separate implementation of of_platform_default_populate_init for PPC. Seems like the easiest solution to me. Best regards Thomas > > >> @@ -551,6 +603,20 @@ static int __init of_platform_default_populate_init(void) >> return 0; >> } >> arch_initcall_sync(of_platform_default_populate_init); >> +#else >> +static int __init of_platform_default_populate_init(void) >> +{ >> + device_links_supplier_sync_state_pause(); >> + >> + if (!of_have_populated_dt()) >> + return -ENODEV; >> + >> + of_platform_populate_framebuffers(); >> + >> + return 0; >> +} >> +arch_initcall_sync(of_platform_default_populate_init); >> +#endif >> >> static int __init of_platform_sync_state_init(void) >> { >> @@ -558,7 +624,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) >> {
On 4/13/22 19:58, Thomas Zimmermann wrote: > Hi [snip] >>> >>> /* Populate everything else. */ >>> of_platform_default_populate(NULL, NULL, NULL); >> >> I'm pretty sure it's just this call that's the problem for PPC though >> none of the above existed when adding this caused a regression. Can we >> remove the ifdef and just make this call conditional on >> !IS_ENABLED(CONFIG_PPC). > > Together with the changes in of_platform_populate_framebuffers(), the > code is more or less an "if-else" depending on PPC. I'll drop > of_platform_populate_framebuffers() from the patch and make a separate > implementation of of_platform_default_populate_init for PPC. Seems like > the easiest solution to me. > That sounds reasonable to me as well. Feel free to retain my R-B tag when posting v2.
On Wed, Apr 13, 2022 at 12:58 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > > Am 13.04.22 um 14:51 schrieb Rob Herring: > > On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann <tzimmermann@suse.de> 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 > >> prevented real hot-unplugging. Adding a distinct platform device for > >> each OF framebuffer solves both problems. Specifically, a DRM drivers > >> can now provide graphics output with 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. > >> > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > >> --- > >> drivers/of/platform.c | 73 ++++++++++++++++++++++++++-- > >> drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++------------- > >> 2 files changed, 134 insertions(+), 37 deletions(-) > >> > >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c > >> index a16b74f32aa9..4c63b9a73587 100644 > >> --- a/drivers/of/platform.c > >> +++ b/drivers/of/platform.c > >> @@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root, > >> } > >> EXPORT_SYMBOL(of_platform_bus_probe); > >> > >> +static int __init of_platform_populate_framebuffers(void) > >> +{ > >> + struct device_node *boot_display = NULL; > >> + struct device_node *node; > >> + struct platform_device *dev; > >> + int ret; > >> + > >> + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); > >> + of_platform_device_create(node, NULL, NULL); > >> + of_node_put(node); > >> + > > > > The rest is PPC only, so bail out here if !PPC. > > > >> + /* 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; > >> + } > >> + } > >> + > >> + /* > >> + * 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); > >> + } > >> + > >> + return 0; > >> +} > >> + > >> /** > >> * of_platform_populate() - Populate platform_devices from device tree data > >> * @root: parent of the first level to probe or NULL for the root of the tree > >> @@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void) > >> of_node_put(node); > >> } > >> > >> - node = of_get_compatible_child(of_chosen, "simple-framebuffer"); > >> - of_platform_device_create(node, NULL, NULL); > >> - of_node_put(node); > >> + of_platform_populate_framebuffers(); > >> > >> /* Populate everything else. */ > >> of_platform_default_populate(NULL, NULL, NULL); > > > > I'm pretty sure it's just this call that's the problem for PPC though > > none of the above existed when adding this caused a regression. Can we > > remove the ifdef and just make this call conditional on > > !IS_ENABLED(CONFIG_PPC). > > Together with the changes in of_platform_populate_framebuffers(), the > code is more or less an "if-else" depending on PPC. I'll drop > of_platform_populate_framebuffers() from the patch and make a separate > implementation of of_platform_default_populate_init for PPC. Seems like > the easiest solution to me. That just moves us farther from PPC ever using of_platform_default_populate_init(). But I don't know that anyone in PPC cares about that, so fine I guess. Rob
eOn Wed, Apr 13, 2022 at 1:46 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Wed, Apr 13, 2022 at 12:58 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > > > > Hi > > > > Am 13.04.22 um 14:51 schrieb Rob Herring: > > > On Wed, Apr 13, 2022 at 4:24 AM Thomas Zimmermann <tzimmermann@suse.de> 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 > > >> prevented real hot-unplugging. Adding a distinct platform device for > > >> each OF framebuffer solves both problems. Specifically, a DRM drivers > > >> can now provide graphics output with 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. > > >> > > >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > > >> --- > > >> drivers/of/platform.c | 73 ++++++++++++++++++++++++++-- > > >> drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++------------- > > >> 2 files changed, 134 insertions(+), 37 deletions(-) > > >> > > >> diff --git a/drivers/of/platform.c b/drivers/of/platform.c > > >> index a16b74f32aa9..4c63b9a73587 100644 > > >> --- a/drivers/of/platform.c > > >> +++ b/drivers/of/platform.c > > >> @@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root, > > >> } > > >> EXPORT_SYMBOL(of_platform_bus_probe); > > >> > > >> +static int __init of_platform_populate_framebuffers(void) > > >> +{ > > >> + struct device_node *boot_display = NULL; > > >> + struct device_node *node; > > >> + struct platform_device *dev; > > >> + int ret; > > >> + > > >> + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); > > >> + of_platform_device_create(node, NULL, NULL); > > >> + of_node_put(node); > > >> + > > > > > > The rest is PPC only, so bail out here if !PPC. > > > > > >> + /* 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; > > >> + } > > >> + } > > >> + > > >> + /* > > >> + * 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); > > >> + } > > >> + > > >> + return 0; > > >> +} > > >> + > > >> /** > > >> * of_platform_populate() - Populate platform_devices from device tree data > > >> * @root: parent of the first level to probe or NULL for the root of the tree > > >> @@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void) > > >> of_node_put(node); > > >> } > > >> > > >> - node = of_get_compatible_child(of_chosen, "simple-framebuffer"); > > >> - of_platform_device_create(node, NULL, NULL); > > >> - of_node_put(node); > > >> + of_platform_populate_framebuffers(); > > >> > > >> /* Populate everything else. */ > > >> of_platform_default_populate(NULL, NULL, NULL); > > > > > > I'm pretty sure it's just this call that's the problem for PPC though > > > none of the above existed when adding this caused a regression. Can we > > > remove the ifdef and just make this call conditional on > > > !IS_ENABLED(CONFIG_PPC). > > > > Together with the changes in of_platform_populate_framebuffers(), the > > code is more or less an "if-else" depending on PPC. I'll drop > > of_platform_populate_framebuffers() from the patch and make a separate > > implementation of of_platform_default_populate_init for PPC. Seems like > > the easiest solution to me. > > That just moves us farther from PPC ever using > of_platform_default_populate_init(). But I don't know that anyone in > PPC cares about that, so fine I guess. Actually, no. Make it work with IS_ENABLED(CONFIG_PPC) rather than an #ifdef. Currently, I don't have to build this (or any of drivers/of/) for PPC because it is IS_ENABLED(CONFIG_PPC) everywhere. Yes, there's an #ifdef already, but there's not an #else and no PPC only code compiled. Rob
Hi Am 13.04.22 um 14:51 schrieb Rob Herring: ... >> + >> /** >> * of_platform_populate() - Populate platform_devices from device tree data >> * @root: parent of the first level to probe or NULL for the root of the tree >> @@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void) >> of_node_put(node); >> } >> >> - node = of_get_compatible_child(of_chosen, "simple-framebuffer"); >> - of_platform_device_create(node, NULL, NULL); >> - of_node_put(node); >> + of_platform_populate_framebuffers(); >> >> /* Populate everything else. */ >> of_platform_default_populate(NULL, NULL, NULL); > > I'm pretty sure it's just this call that's the problem for PPC though > none of the above existed when adding this caused a regression. Can we > remove the ifdef and just make this call conditional on > !IS_ENABLED(CONFIG_PPC). That didn't work. The boot process stops at some point. I'll send you an updated patch that covers most of the function with IS_ENABLED(CONFIG_PPC) Best regards Thomas > > >> @@ -551,6 +603,20 @@ static int __init of_platform_default_populate_init(void) >> return 0; >> } >> arch_initcall_sync(of_platform_default_populate_init); >> +#else >> +static int __init of_platform_default_populate_init(void) >> +{ >> + device_links_supplier_sync_state_pause(); >> + >> + if (!of_have_populated_dt()) >> + return -ENODEV; >> + >> + of_platform_populate_framebuffers(); >> + >> + return 0; >> +} >> +arch_initcall_sync(of_platform_default_populate_init); >> +#endif >> >> static int __init of_platform_sync_state_init(void) >> { >> @@ -558,7 +624,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/of/platform.c b/drivers/of/platform.c index a16b74f32aa9..4c63b9a73587 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -447,6 +447,60 @@ int of_platform_bus_probe(struct device_node *root, } EXPORT_SYMBOL(of_platform_bus_probe); +static int __init of_platform_populate_framebuffers(void) +{ + struct device_node *boot_display = NULL; + struct device_node *node; + struct platform_device *dev; + int ret; + + node = of_get_compatible_child(of_chosen, "simple-framebuffer"); + of_platform_device_create(node, NULL, NULL); + of_node_put(node); + + /* 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; + } + } + + /* + * 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); + } + + return 0; +} + /** * of_platform_populate() - Populate platform_devices from device tree data * @root: parent of the first level to probe or NULL for the root of the tree @@ -541,9 +595,7 @@ static int __init of_platform_default_populate_init(void) of_node_put(node); } - node = of_get_compatible_child(of_chosen, "simple-framebuffer"); - of_platform_device_create(node, NULL, NULL); - of_node_put(node); + of_platform_populate_framebuffers(); /* Populate everything else. */ of_platform_default_populate(NULL, NULL, NULL); @@ -551,6 +603,20 @@ static int __init of_platform_default_populate_init(void) return 0; } arch_initcall_sync(of_platform_default_populate_init); +#else +static int __init of_platform_default_populate_init(void) +{ + device_links_supplier_sync_state_pause(); + + if (!of_have_populated_dt()) + return -ENODEV; + + of_platform_populate_framebuffers(); + + return 0; +} +arch_initcall_sync(of_platform_default_populate_init); +#endif static int __init of_platform_sync_state_init(void) { @@ -558,7 +624,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");
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 prevented real hot-unplugging. Adding a distinct platform device for each OF framebuffer solves both problems. Specifically, a DRM drivers can now provide graphics output with 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. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/of/platform.c | 73 ++++++++++++++++++++++++++-- drivers/video/fbdev/offb.c | 98 +++++++++++++++++++++++++------------- 2 files changed, 134 insertions(+), 37 deletions(-)