| Message ID | 1407914239-12054-4-git-send-email-libv@skynet.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote: > The next commit will handle clocks correctly, so that these do not get > automatically disabled on certain SoC simplefb implementations. As a > result, the removal of this simplefb driver, and the release of the > clocks, is rather final, and only a full display driver can work after > this. So, it makes sense to also flag the dt node as disabled, even > though it has no real value today. > > Signed-off-by: Luc Verhaegen <libv@skynet.be> Please, no. Drivers should not be modifying the device tree without and exceptionally good reason for doing so. Drivers are to treat the DT as immutable. * the exception is an overlay driver which add new devices to the kernel. Definitely not the case here. g. > --- > drivers/video/fbdev/simplefb.c | 43 ++++++++++++++++++++++++++++++++++++--- > 1 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c > index 72a4f20..74c4b2a 100644 > --- a/drivers/video/fbdev/simplefb.c > +++ b/drivers/video/fbdev/simplefb.c > @@ -138,6 +138,32 @@ static int simplefb_parse_dt(struct platform_device *pdev, > return 0; > } > > +/* > + * Make sure that nothing tries to inadvertedly re-use this node... > + */ > +static int > +simplefb_dt_disable(struct platform_device *pdev) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct property *property; > + int ret; > + > + property = kzalloc(sizeof(struct property), GFP_KERNEL); > + if (!property) > + return -ENOMEM; > + > + property->name = "status"; > + property->value = "disabled"; > + property->length = strlen(property->value) + 1; > + > + ret = of_update_property(np, property); > + if (ret) > + dev_err(&pdev->dev, "%s: failed to update property: %d\n", > + __func__, ret); > + > + return ret; > +} > + > static int simplefb_parse_pd(struct platform_device *pdev, > struct simplefb_params *params) > { > @@ -187,17 +213,20 @@ static int simplefb_probe(struct platform_device *pdev) > ret = simplefb_parse_dt(pdev, ¶ms); > > if (ret) > - return ret; > + goto error_dt_disable; > > mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!mem) { > dev_err(&pdev->dev, "No memory resource\n"); > - return -EINVAL; > + ret = -EINVAL; > + goto error_dt_disable; > } > > info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev); > - if (!info) > - return -ENOMEM; > + if (!info) { > + ret = -ENOMEM; > + goto error_dt_disable; > + } > platform_set_drvdata(pdev, info); > > par = info->par; > @@ -260,6 +289,10 @@ static int simplefb_probe(struct platform_device *pdev) > error_fb_release: > framebuffer_release(info); > > + error_dt_disable: > + if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node) > + simplefb_dt_disable(pdev); > + > return ret; > } > > @@ -269,6 +302,8 @@ static int simplefb_remove(struct platform_device *pdev) > > unregister_framebuffer(info); > framebuffer_release(info); > + if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node) > + simplefb_dt_disable(pdev); > > return 0; > } > -- > 1.7.7 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote: >> The next commit will handle clocks correctly, so that these do not get >> automatically disabled on certain SoC simplefb implementations. As a >> result, the removal of this simplefb driver, and the release of the >> clocks, is rather final, and only a full display driver can work after >> this. So, it makes sense to also flag the dt node as disabled, even >> though it has no real value today. >> >> Signed-off-by: Luc Verhaegen <libv@skynet.be> > > Please, no. > > Drivers should not be modifying the device tree without and > exceptionally good reason for doing so. Drivers are to treat the DT as > immutable. > > * the exception is an overlay driver which add new devices to the > kernel. Definitely not the case here. Why? I think we have exactly that case: * DT describes the real hw properly and those parts are immutable * Additionally, bootloaders create firmware-framebuffers and create simple-framebuffer devices for them. Those are valid as long as no driver reconfigured the real hw. * Once a real hw-driver loads, it might destroy the existing framebuffers, thus, it should also destroy the platform device. * If the real hw-driver is unloaded, it might re-create the FB and thus create a new (or enable the old) platform device. Or, in a nutshell: A "simple-framebuffer" device is basically a platform-device for framebuffers. Framebuffers can be created and destroyed during runtime. The reason we create platform-devices for them, is to allow dummy drivers to be probed. Real hw-drivers obviously bind to the parent bus device. Other ideas are obviously welcome, but so far all of the other ideas sounded like big hacks (like remove_conflicting_framebuffers() so far..). Thanks David
On Wed, Aug 13, 2014 at 9:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely > <grant.likely@secretlab.ca> wrote: >> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote: >>> The next commit will handle clocks correctly, so that these do not get >>> automatically disabled on certain SoC simplefb implementations. As a >>> result, the removal of this simplefb driver, and the release of the >>> clocks, is rather final, and only a full display driver can work after >>> this. So, it makes sense to also flag the dt node as disabled, even >>> though it has no real value today. >>> >>> Signed-off-by: Luc Verhaegen <libv@skynet.be> >> >> Please, no. >> >> Drivers should not be modifying the device tree without and >> exceptionally good reason for doing so. Drivers are to treat the DT as >> immutable. >> >> * the exception is an overlay driver which add new devices to the >> kernel. Definitely not the case here. > > Why? The majority of the DT code is based on the assumption of a static tree. Pantelis has been working on being able to modify it at runtime with overlays, but he has had to go through a lot of rework because it is not a trivial task. When you get into modifying the DT, you need to have a lot more understanding of the side effects to changing the tree. The DT structure also has a lifecycle that can go beyond the current lifecycle of the kernel. The kexec tool will extract the current tree from the kernel, make the appropriate modifications, and use that to boot the next kernel. Allowing any driver to modify the tree has side effects beyond just the current kernel. In this specific case, it will interact badly with the work Pantelis is doing to make platform devices work with overlays. Modifying the status property will cause the associated struct device to get removed in the middle of probing a driver for that device! That will most likely cause an oops. Besides, Luc straight out *said*: "...even though it has no real value today". In what circumstance is that justification for modifying the tree? > I think we have exactly that case: > * DT describes the real hw properly and those parts are immutable > * Additionally, bootloaders create firmware-framebuffers and > create simple-framebuffer devices for them. Those are > valid as long as no driver reconfigured the real hw. > * Once a real hw-driver loads, it might destroy the existing > framebuffers, thus, it should also destroy the platform device. > * If the real hw-driver is unloaded, it might re-create the FB > and thus create a new (or enable the old) platform device. > > Or, in a nutshell: A "simple-framebuffer" device is basically a > platform-device for framebuffers. Framebuffers can be created and > destroyed during runtime. The reason we create platform-devices for > them, is to allow dummy drivers to be probed. Real hw-drivers > obviously bind to the parent bus device. > > Other ideas are obviously welcome, but so far all of the other ideas > sounded like big hacks (like remove_conflicting_framebuffers() so > far..). > > Thanks > David
Hi On Wed, Aug 13, 2014 at 11:23 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Wed, Aug 13, 2014 at 9:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote: >> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely >> <grant.likely@secretlab.ca> wrote: >>> * the exception is an overlay driver which add new devices to the >>> kernel. Definitely not the case here. >> >> Why? > > The majority of the DT code is based on the assumption of a static > tree. Pantelis has been working on being able to modify it at runtime > with overlays, but he has had to go through a lot of rework because it > is not a trivial task. When you get into modifying the DT, you need to > have a lot more understanding of the side effects to changing the > tree. The DT structure also has a lifecycle that can go beyond the > current lifecycle of the kernel. The kexec tool will extract the > current tree from the kernel, make the appropriate modifications, and > use that to boot the next kernel. Allowing any driver to modify the > tree has side effects beyond just the current kernel. Ok, fair enough. So we leave the DT untouched. That still allows calling device_add() / device_del() on platform-devices, right? > In this specific case, it will interact badly with the work Pantelis > is doing to make platform devices work with overlays. Modifying the > status property will cause the associated struct device to get removed > in the middle of probing a driver for that device! That will most > likely cause an oops. > > Besides, Luc straight out *said*: "...even though it has no real value > today". In what circumstance is that justification for modifying the > tree? Sorry, I wasn't clear enough: I'm not arguing in favor of this patch. I just want to figure out what to do once we implement hardware-handover for graphics devices on non-x86 (which this series is kinda preparing for). This patch just reminded me, that we could do this on a DT level, instead of driver-core level. But I'm fine with avoiding that, if you warn about complications. Thanks David
On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote: > > The majority of the DT code is based on the assumption of a static > tree. Pantelis has been working on being able to modify it at runtime > with overlays, but he has had to go through a lot of rework because it > is not a trivial task. When you get into modifying the DT, you need to > have a lot more understanding of the side effects to changing the > tree. The DT structure also has a lifecycle that can go beyond the > current lifecycle of the kernel. The kexec tool will extract the > current tree from the kernel, make the appropriate modifications, and > use that to boot the next kernel. Allowing any driver to modify the > tree has side effects beyond just the current kernel. > > In this specific case, it will interact badly with the work Pantelis > is doing to make platform devices work with overlays. Modifying the > status property will cause the associated struct device to get removed > in the middle of probing a driver for that device! That will most > likely cause an oops. > > Besides, Luc straight out *said*: "...even though it has no real value > today". In what circumstance is that justification for modifying the > tree? With that sentence i meant that given the current state of things, it has no real value. It has no value currently as re-probing simplefb is not going to happen. But it's not a big leap to turn simplefb into a proper module. Not that that makes much sense, but that's never stopped anyone. To me it seemed simple, dt is what drives simplefb, so dt then also becomes responsible for making sure that simplefb or another driver does not attempt to blindly use this info again. The way this is implemented i do not care for in any way, i just knew that i could not do nothing here, given the catastrophic effect disabling the clocks has on simplefb on sunxi. Given the discussion that errupted here, i'd say that this does need some resolution, and altering the dt is going to have to be part of the solution. In any case, i will gladly drop this patch, as it is not absolutely necessary. But it should be very clear that there is no going back on this dt node after the clocks were released once. Luc Verhaegen.
On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote: > On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote: > > > > The majority of the DT code is based on the assumption of a static > > tree. Pantelis has been working on being able to modify it at runtime > > with overlays, but he has had to go through a lot of rework because it > > is not a trivial task. When you get into modifying the DT, you need to > > have a lot more understanding of the side effects to changing the > > tree. The DT structure also has a lifecycle that can go beyond the > > current lifecycle of the kernel. The kexec tool will extract the > > current tree from the kernel, make the appropriate modifications, and > > use that to boot the next kernel. Allowing any driver to modify the > > tree has side effects beyond just the current kernel. > > > > In this specific case, it will interact badly with the work Pantelis > > is doing to make platform devices work with overlays. Modifying the > > status property will cause the associated struct device to get removed > > in the middle of probing a driver for that device! That will most > > likely cause an oops. > > > > Besides, Luc straight out *said*: "...even though it has no real value > > today". In what circumstance is that justification for modifying the > > tree? > > With that sentence i meant that given the current state of things, it > has no real value. > > It has no value currently as re-probing simplefb is not going to happen. > But it's not a big leap to turn simplefb into a proper module. Not that > that makes much sense, but that's never stopped anyone. > > To me it seemed simple, dt is what drives simplefb, so dt then also > becomes responsible for making sure that simplefb or another driver does > not attempt to blindly use this info again. The way this is implemented > i do not care for in any way, i just knew that i could not do nothing > here, given the catastrophic effect disabling the clocks has on simplefb > on sunxi. Given the discussion that errupted here, i'd say that this > does need some resolution, and altering the dt is going to have to be > part of the solution. > > In any case, i will gladly drop this patch, as it is not absolutely > necessary. But it should be very clear that there is no going back on > this dt node after the clocks were released once. > > Luc Verhaegen. What about approaching this from the other end? U-Boot could add a property named "once-only" or so. Luc Verhaegen.
On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote: > On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote: >> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote: >> > >> > The majority of the DT code is based on the assumption of a static >> > tree. Pantelis has been working on being able to modify it at runtime >> > with overlays, but he has had to go through a lot of rework because it >> > is not a trivial task. When you get into modifying the DT, you need to >> > have a lot more understanding of the side effects to changing the >> > tree. The DT structure also has a lifecycle that can go beyond the >> > current lifecycle of the kernel. The kexec tool will extract the >> > current tree from the kernel, make the appropriate modifications, and >> > use that to boot the next kernel. Allowing any driver to modify the >> > tree has side effects beyond just the current kernel. >> > >> > In this specific case, it will interact badly with the work Pantelis >> > is doing to make platform devices work with overlays. Modifying the >> > status property will cause the associated struct device to get removed >> > in the middle of probing a driver for that device! That will most >> > likely cause an oops. >> > >> > Besides, Luc straight out *said*: "...even though it has no real value >> > today". In what circumstance is that justification for modifying the >> > tree? >> >> With that sentence i meant that given the current state of things, it >> has no real value. >> >> It has no value currently as re-probing simplefb is not going to happen. >> But it's not a big leap to turn simplefb into a proper module. Not that >> that makes much sense, but that's never stopped anyone. >> >> To me it seemed simple, dt is what drives simplefb, so dt then also >> becomes responsible for making sure that simplefb or another driver does >> not attempt to blindly use this info again. The way this is implemented >> i do not care for in any way, i just knew that i could not do nothing >> here, given the catastrophic effect disabling the clocks has on simplefb >> on sunxi. Given the discussion that errupted here, i'd say that this >> does need some resolution, and altering the dt is going to have to be >> part of the solution. >> >> In any case, i will gladly drop this patch, as it is not absolutely >> necessary. But it should be very clear that there is no going back on >> this dt node after the clocks were released once. >> >> Luc Verhaegen. > > What about approaching this from the other end? U-Boot could add a > property named "once-only" or so. Device tree is supposed to be a static description of the hardware usable on all operating systems. It is the wrong mechanism for communicating between uboot and the kernel. Use something like atags or the kernel command line to tell the kernel that the console has already been set up. The switch over from simple to KMS should not be done via a node add/del to the device tree either. No one has removed the device from the system, the device tree should not be changing. Some Linux mechanism inside the kernel needs to handle that transition. Somehow simple needs to hang onto the clocks, let go of the device, load KMS, then exit? > > Luc Verhaegen. > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
On 08/13/2014 02:49 AM, David Herrmann wrote: > Hi > > On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely > <grant.likely@secretlab.ca> wrote: >> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote: >>> The next commit will handle clocks correctly, so that these do not get >>> automatically disabled on certain SoC simplefb implementations. As a >>> result, the removal of this simplefb driver, and the release of the >>> clocks, is rather final, and only a full display driver can work after >>> this. So, it makes sense to also flag the dt node as disabled, even >>> though it has no real value today. >>> >>> Signed-off-by: Luc Verhaegen <libv@skynet.be> >> >> Please, no. >> >> Drivers should not be modifying the device tree without and >> exceptionally good reason for doing so. Drivers are to treat the DT as >> immutable. >> >> * the exception is an overlay driver which add new devices to the >> kernel. Definitely not the case here. > > Why? I think we have exactly that case: > * DT describes the real hw properly and those parts are immutable > * Additionally, bootloaders create firmware-framebuffers and > create simple-framebuffer devices for them. Those are > valid as long as no driver reconfigured the real hw. > * Once a real hw-driver loads, it might destroy the existing > framebuffers, thus, it should also destroy the platform device. > * If the real hw-driver is unloaded, it might re-create the FB > and thus create a new (or enable the old) platform device. My intention was always that a bootloader's addition of a simplefb node to the DT would be user-configurable or driven by the original DT content. As such, there shouldn't ever be both a DT node describing the "real" HW and simplefb. In other words, if the DT already has the "real" DT node, the bootloader should automatically (or under user command) not add the simpefb node.
On Wed, Aug 13, 2014 at 12:44 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 08/13/2014 02:49 AM, David Herrmann wrote: >> >> Hi >> >> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely >> <grant.likely@secretlab.ca> wrote: >>> >>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote: >>>> >>>> The next commit will handle clocks correctly, so that these do not get >>>> automatically disabled on certain SoC simplefb implementations. As a >>>> result, the removal of this simplefb driver, and the release of the >>>> clocks, is rather final, and only a full display driver can work after >>>> this. So, it makes sense to also flag the dt node as disabled, even >>>> though it has no real value today. >>>> >>>> Signed-off-by: Luc Verhaegen <libv@skynet.be> >>> >>> >>> Please, no. >>> >>> Drivers should not be modifying the device tree without and >>> exceptionally good reason for doing so. Drivers are to treat the DT as >>> immutable. >>> >>> * the exception is an overlay driver which add new devices to the >>> kernel. Definitely not the case here. >> >> >> Why? I think we have exactly that case: >> * DT describes the real hw properly and those parts are immutable >> * Additionally, bootloaders create firmware-framebuffers and >> create simple-framebuffer devices for them. Those are >> valid as long as no driver reconfigured the real hw. >> * Once a real hw-driver loads, it might destroy the existing >> framebuffers, thus, it should also destroy the platform device. >> * If the real hw-driver is unloaded, it might re-create the FB >> and thus create a new (or enable the old) platform device. > > > My intention was always that a bootloader's addition of a simplefb node to > the DT would be user-configurable or driven by the original DT content. As > such, there shouldn't ever be both a DT node describing the "real" HW and > simplefb. In other words, if the DT already has the "real" DT node, the > bootloader should automatically (or under user command) not add the simpefb > node. DT is just the wrong mechanism to signal this, use an ATAG or kernel command line parameter. real hardware has compatible = "real-hardware-name, simplefb" simplefb is built into kernel. It will attach to the device because of the compatible string. Then it can look at the command line and see if the bootloader also supported simplefb and already set things up. Then some mechanism will have to be designed to arrange a handoff between simplefb and the chip specific KMS driver. But that's a Linux problem, not a DT one. > > > -- > You received this message because you are subscribed to the Google Groups > "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
On 08/13/2014 11:26 AM, jonsmirl@gmail.com wrote: > On Wed, Aug 13, 2014 at 12:44 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 08/13/2014 02:49 AM, David Herrmann wrote: >>> >>> Hi >>> >>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely >>> <grant.likely@secretlab.ca> wrote: >>>> >>>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote: >>>>> >>>>> The next commit will handle clocks correctly, so that these do not get >>>>> automatically disabled on certain SoC simplefb implementations. As a >>>>> result, the removal of this simplefb driver, and the release of the >>>>> clocks, is rather final, and only a full display driver can work after >>>>> this. So, it makes sense to also flag the dt node as disabled, even >>>>> though it has no real value today. >>>>> >>>>> Signed-off-by: Luc Verhaegen <libv@skynet.be> >>>> >>>> >>>> Please, no. >>>> >>>> Drivers should not be modifying the device tree without and >>>> exceptionally good reason for doing so. Drivers are to treat the DT as >>>> immutable. >>>> >>>> * the exception is an overlay driver which add new devices to the >>>> kernel. Definitely not the case here. >>> >>> >>> Why? I think we have exactly that case: >>> * DT describes the real hw properly and those parts are immutable >>> * Additionally, bootloaders create firmware-framebuffers and >>> create simple-framebuffer devices for them. Those are >>> valid as long as no driver reconfigured the real hw. >>> * Once a real hw-driver loads, it might destroy the existing >>> framebuffers, thus, it should also destroy the platform device. >>> * If the real hw-driver is unloaded, it might re-create the FB >>> and thus create a new (or enable the old) platform device. >> >> >> My intention was always that a bootloader's addition of a simplefb node to >> the DT would be user-configurable or driven by the original DT content. As >> such, there shouldn't ever be both a DT node describing the "real" HW and >> simplefb. In other words, if the DT already has the "real" DT node, the >> bootloader should automatically (or under user command) not add the simpefb >> node. > > DT is just the wrong mechanism to signal this, use an ATAG or kernel > command line parameter. > > real hardware has compatible = "real-hardware-name, simplefb" > > simplefb is built into kernel. It will attach to the device because of > the compatible string. Then it can look at the command line and see if > the bootloader also supported simplefb and already set things up. > > Then some mechanism will have to be designed to arrange a handoff > between simplefb and the chip specific KMS driver. But that's a Linux > problem, not a DT one. Having a single DT node that conforms to both the binding for "real-hardware-name" and "simplefb" doesn't seem like a good approach. What if the properties required by the two bindings conflict in some way? The approach you advocate certainly hasn't ever been used AFAIK.
On Wed, Aug 13, 2014 at 1:34 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 08/13/2014 11:26 AM, jonsmirl@gmail.com wrote: >> >> On Wed, Aug 13, 2014 at 12:44 PM, Stephen Warren <swarren@wwwdotorg.org> >> wrote: >>> >>> On 08/13/2014 02:49 AM, David Herrmann wrote: >>>> >>>> >>>> Hi >>>> >>>> On Wed, Aug 13, 2014 at 10:40 AM, Grant Likely >>>> <grant.likely@secretlab.ca> wrote: >>>>> >>>>> >>>>> On Wed, Aug 13, 2014 at 8:17 AM, Luc Verhaegen <libv@skynet.be> wrote: >>>>>> >>>>>> >>>>>> The next commit will handle clocks correctly, so that these do not get >>>>>> automatically disabled on certain SoC simplefb implementations. As a >>>>>> result, the removal of this simplefb driver, and the release of the >>>>>> clocks, is rather final, and only a full display driver can work after >>>>>> this. So, it makes sense to also flag the dt node as disabled, even >>>>>> though it has no real value today. >>>>>> >>>>>> Signed-off-by: Luc Verhaegen <libv@skynet.be> >>>>> >>>>> >>>>> >>>>> Please, no. >>>>> >>>>> Drivers should not be modifying the device tree without and >>>>> exceptionally good reason for doing so. Drivers are to treat the DT as >>>>> immutable. >>>>> >>>>> * the exception is an overlay driver which add new devices to the >>>>> kernel. Definitely not the case here. >>>> >>>> >>>> >>>> Why? I think we have exactly that case: >>>> * DT describes the real hw properly and those parts are immutable >>>> * Additionally, bootloaders create firmware-framebuffers and >>>> create simple-framebuffer devices for them. Those are >>>> valid as long as no driver reconfigured the real hw. >>>> * Once a real hw-driver loads, it might destroy the existing >>>> framebuffers, thus, it should also destroy the platform device. >>>> * If the real hw-driver is unloaded, it might re-create the FB >>>> and thus create a new (or enable the old) platform device. >>> >>> >>> >>> My intention was always that a bootloader's addition of a simplefb node >>> to >>> the DT would be user-configurable or driven by the original DT content. >>> As >>> such, there shouldn't ever be both a DT node describing the "real" HW and >>> simplefb. In other words, if the DT already has the "real" DT node, the >>> bootloader should automatically (or under user command) not add the >>> simpefb >>> node. >> >> >> DT is just the wrong mechanism to signal this, use an ATAG or kernel >> command line parameter. >> >> real hardware has compatible = "real-hardware-name, simplefb" >> >> simplefb is built into kernel. It will attach to the device because of >> the compatible string. Then it can look at the command line and see if >> the bootloader also supported simplefb and already set things up. >> >> Then some mechanism will have to be designed to arrange a handoff >> between simplefb and the chip specific KMS driver. But that's a Linux >> problem, not a DT one. > > > Having a single DT node that conforms to both the binding for > "real-hardware-name" and "simplefb" doesn't seem like a good approach. What > if the properties required by the two bindings conflict in some way? The > approach you advocate certainly hasn't ever been used AFAIK. I believe we do have something like this - SPI core implements a lot of core DT functions. All SPI nodes use this core. Then hardware specific attributes are added. The conflicts would need to get sorted out. That's why we should have a schema in place for device tree. Then the properties for specific video hardware would inherit from the properties for simplefb.
On Wed, Aug 13, 2014 at 1:54 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote: >> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote: >>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote: >>> > >>> > The majority of the DT code is based on the assumption of a static >>> > tree. Pantelis has been working on being able to modify it at runtime >>> > with overlays, but he has had to go through a lot of rework because it >>> > is not a trivial task. When you get into modifying the DT, you need to >>> > have a lot more understanding of the side effects to changing the >>> > tree. The DT structure also has a lifecycle that can go beyond the >>> > current lifecycle of the kernel. The kexec tool will extract the >>> > current tree from the kernel, make the appropriate modifications, and >>> > use that to boot the next kernel. Allowing any driver to modify the >>> > tree has side effects beyond just the current kernel. >>> > >>> > In this specific case, it will interact badly with the work Pantelis >>> > is doing to make platform devices work with overlays. Modifying the >>> > status property will cause the associated struct device to get removed >>> > in the middle of probing a driver for that device! That will most >>> > likely cause an oops. >>> > >>> > Besides, Luc straight out *said*: "...even though it has no real value >>> > today". In what circumstance is that justification for modifying the >>> > tree? >>> >>> With that sentence i meant that given the current state of things, it >>> has no real value. >>> >>> It has no value currently as re-probing simplefb is not going to happen. >>> But it's not a big leap to turn simplefb into a proper module. Not that >>> that makes much sense, but that's never stopped anyone. >>> >>> To me it seemed simple, dt is what drives simplefb, so dt then also >>> becomes responsible for making sure that simplefb or another driver does >>> not attempt to blindly use this info again. The way this is implemented >>> i do not care for in any way, i just knew that i could not do nothing >>> here, given the catastrophic effect disabling the clocks has on simplefb >>> on sunxi. Given the discussion that errupted here, i'd say that this >>> does need some resolution, and altering the dt is going to have to be >>> part of the solution. >>> >>> In any case, i will gladly drop this patch, as it is not absolutely >>> necessary. But it should be very clear that there is no going back on >>> this dt node after the clocks were released once. >>> >>> Luc Verhaegen. >> >> What about approaching this from the other end? U-Boot could add a >> property named "once-only" or so. > > Device tree is supposed to be a static description of the hardware > usable on all operating systems. It is the wrong mechanism for > communicating between uboot and the kernel. Use something like atags > or the kernel command line to tell the kernel that the console has > already been set up. Not accurate. While it is primarily hardware description, it is also used for firmware communication. There is loads of precedence for this. The /chosen node is the most significant example, but there are other places where the tree is used to provide state. For example, the current-speed property on UART nodes. > The switch over from simple to KMS should not be done via a node > add/del to the device tree either. No one has removed the device from > the system, the device tree should not be changing. The simple-framebuffer binding appears to be insufficient in this regard in that it doesn't have any linkage with the actual device providing the framebuffer. Ideally, I would put the simple framebuffer state directly into the video device node and use the chosen node to point to the stdout device (probably with the stdout-path property). Then the driver already knows it can just ignore the simple properties because it owns the device node when it binds. That said, simple-framebuffer as it stands is in use so we're not going to deprecate it. I would like to see an addition that specifies how a controller can be associated with a simple framebuffer node. BTW, Is anyone currently using the simple framebuffer for early console? For early console we would want to start using it well before setting up platform devices. g.
On Wed, Aug 13, 2014 at 08:14:37PM +0100, Grant Likely wrote: > > BTW, Is anyone currently using the simple framebuffer for early > console? For early console we would want to start using it well before > setting up platform devices. The code that sets up simplefb for sunxi is primarily for providing a console in u-boot (using the ancient cfbconsole infrastructure). From what i can tell, the u-boot code for rpi is about showing a splash screen (using the much newer lcd infrastructure). With u-boot showing a console, it really seemed only a small step to add simplefb. And quite a few people in our sunxi community are interested in it, primarily for u-boot and early console, and only secondarily as a stop-gap for a full driver. Luc Verhaegen.
On 08/13/2014 01:25 PM, Luc Verhaegen wrote: > On Wed, Aug 13, 2014 at 08:14:37PM +0100, Grant Likely wrote: >> >> BTW, Is anyone currently using the simple framebuffer for early >> console? For early console we would want to start using it well before >> setting up platform devices. > > The code that sets up simplefb for sunxi is primarily for providing a > console in u-boot (using the ancient cfbconsole infrastructure). From > what i can tell, the u-boot code for rpi is about showing a splash > screen (using the much newer lcd infrastructure). There's no splash screen on the Pi in the upstream U-Boot code. The LCD displays the U-Boot console/stdout. If USB support for the Pi's USB host is ever sent upstream, that will allow the user to use USB/LCD for U-Boot control, rather than serial.
On Wed, Aug 13, 2014 at 8:25 PM, Luc Verhaegen <libv@skynet.be> wrote: > On Wed, Aug 13, 2014 at 08:14:37PM +0100, Grant Likely wrote: >> >> BTW, Is anyone currently using the simple framebuffer for early >> console? For early console we would want to start using it well before >> setting up platform devices. > > The code that sets up simplefb for sunxi is primarily for providing a > console in u-boot (using the ancient cfbconsole infrastructure). From > what i can tell, the u-boot code for rpi is about showing a splash > screen (using the much newer lcd infrastructure). > > With u-boot showing a console, it really seemed only a small step to add > simplefb. And quite a few people in our sunxi community are interested > in it, primarily for u-boot and early console, and only secondarily as a > stop-gap for a full driver. Both of which make sense and should be supported, so I agree we need to find a solution. The problem I think comes down to the handoff mechanism. There are a lot of different video controllers which could all be configured for a simple framebuffer. I don't think the "run this only once" test is the right approach. Sometimes the simple framebuffer will never be torn down. It is conceivable that a simple framebuffer will get /added/ at runtime with an overlay, or even rebound to the driver. The simple framebuffer driver really needs to be explicitly told from outside itself that it is being taken over since it doesn't actually have the information to know when a framebuffer becomes invalid. Only the real video driver can provide that information. How does the sunxi driver currently take over from the simplefb? g.
On Wed, Aug 13, 2014 at 01:58:36PM -0600, Stephen Warren wrote: > On 08/13/2014 01:25 PM, Luc Verhaegen wrote: >> >> The code that sets up simplefb for sunxi is primarily for providing a >> console in u-boot (using the ancient cfbconsole infrastructure). From >> what i can tell, the u-boot code for rpi is about showing a splash >> screen (using the much newer lcd infrastructure). > > There's no splash screen on the Pi in the upstream U-Boot code. The LCD > displays the U-Boot console/stdout. If USB support for the Pi's USB host > is ever sent upstream, that will allow the user to use USB/LCD for > U-Boot control, rather than serial. Ah ok, i hadn't immediately found a tie with the lcd code and cfbconsole code, so apparently i didn't dig down far enough. Luc Verhaegen.
On Wed, Aug 13, 2014 at 09:00:07PM +0100, Grant Likely wrote: > > Both of which make sense and should be supported, so I agree we need > to find a solution. > > The problem I think comes down to the handoff mechanism. There are a > lot of different video controllers which could all be configured for a > simple framebuffer. > > I don't think the "run this only once" test is the right approach. > Sometimes the simple framebuffer will never be torn down. It is > conceivable that a simple framebuffer will get /added/ at runtime with > an overlay, or even rebound to the driver. The simple framebuffer > driver really needs to be explicitly told from outside itself that it > is being taken over since it doesn't actually have the information to > know when a framebuffer becomes invalid. Only the real video driver > can provide that information. There really are two separate issues here: 1) making sure that nothing tries to use the freshly died simplefb again. 2) doing a clean handover to another, usually hw specific, driver. 1 is what i hoped to superficially solve with this patch. 2 can be very smart about things as the replacement driver actually should know the hw. I kind of like the idea of an "only-once" (there must be a better name for this) property. This allows the driver/device infrastructure to handle things much more cleanly, and allows me to state this from u-boot for sunxi, while Stephen wouldn't need to for rpi. I currently don't know how or where this could be handled, i just know that this smells like a decent solution which could solve my concern while at least avoiding the probe issue you mentioned earlier. > How does the sunxi driver currently take over from the simplefb? Not at all. A big blob of KMS code exists for our sunxi-3.4 kernel, where i can load the original display driver quickly and easily. This tree predates most of DT. I was rather hoping that the simplefb stop-gap didn't require me to have fully engineered everything yesterday already. Luc Verhaegen.
On Wed, Aug 13, 2014 at 3:14 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Wed, Aug 13, 2014 at 1:54 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >> On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote: >>> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote: >>>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote: >>>> > >>>> > The majority of the DT code is based on the assumption of a static >>>> > tree. Pantelis has been working on being able to modify it at runtime >>>> > with overlays, but he has had to go through a lot of rework because it >>>> > is not a trivial task. When you get into modifying the DT, you need to >>>> > have a lot more understanding of the side effects to changing the >>>> > tree. The DT structure also has a lifecycle that can go beyond the >>>> > current lifecycle of the kernel. The kexec tool will extract the >>>> > current tree from the kernel, make the appropriate modifications, and >>>> > use that to boot the next kernel. Allowing any driver to modify the >>>> > tree has side effects beyond just the current kernel. >>>> > >>>> > In this specific case, it will interact badly with the work Pantelis >>>> > is doing to make platform devices work with overlays. Modifying the >>>> > status property will cause the associated struct device to get removed >>>> > in the middle of probing a driver for that device! That will most >>>> > likely cause an oops. >>>> > >>>> > Besides, Luc straight out *said*: "...even though it has no real value >>>> > today". In what circumstance is that justification for modifying the >>>> > tree? >>>> >>>> With that sentence i meant that given the current state of things, it >>>> has no real value. >>>> >>>> It has no value currently as re-probing simplefb is not going to happen. >>>> But it's not a big leap to turn simplefb into a proper module. Not that >>>> that makes much sense, but that's never stopped anyone. >>>> >>>> To me it seemed simple, dt is what drives simplefb, so dt then also >>>> becomes responsible for making sure that simplefb or another driver does >>>> not attempt to blindly use this info again. The way this is implemented >>>> i do not care for in any way, i just knew that i could not do nothing >>>> here, given the catastrophic effect disabling the clocks has on simplefb >>>> on sunxi. Given the discussion that errupted here, i'd say that this >>>> does need some resolution, and altering the dt is going to have to be >>>> part of the solution. >>>> >>>> In any case, i will gladly drop this patch, as it is not absolutely >>>> necessary. But it should be very clear that there is no going back on >>>> this dt node after the clocks were released once. >>>> >>>> Luc Verhaegen. >>> >>> What about approaching this from the other end? U-Boot could add a >>> property named "once-only" or so. >> >> Device tree is supposed to be a static description of the hardware >> usable on all operating systems. It is the wrong mechanism for >> communicating between uboot and the kernel. Use something like atags >> or the kernel command line to tell the kernel that the console has >> already been set up. > > Not accurate. While it is primarily hardware description, it is also > used for firmware communication. There is loads of precedence for > this. The /chosen node is the most significant example, but there are > other places where the tree is used to provide state. For example, the > current-speed property on UART nodes. I do seem to recall you telling me a long time ago that those chosen nodes were a mistake (or maybe it was Matt Sealey). I'm pretty wary of opening to door to device trees carrying a bunch of state. Five years from now the DT is going to look like a Christmas tree. > >> The switch over from simple to KMS should not be done via a node >> add/del to the device tree either. No one has removed the device from >> the system, the device tree should not be changing. > > The simple-framebuffer binding appears to be insufficient in this > regard in that it doesn't have any linkage with the actual device > providing the framebuffer. Ideally, I would put the simple framebuffer > state directly into the video device node and use the chosen node to > point to the stdout device (probably with the stdout-path property). > Then the driver already knows it can just ignore the simple properties > because it owns the device node when it binds. > > That said, simple-framebuffer as it stands is in use so we're not > going to deprecate it. I would like to see an addition that specifies > how a controller can be associated with a simple framebuffer node. > > BTW, Is anyone currently using the simple framebuffer for early > console? For early console we would want to start using it well before > setting up platform devices. > > g.
On Wed, Aug 13, 2014 at 9:41 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > On Wed, Aug 13, 2014 at 3:14 PM, Grant Likely <grant.likely@secretlab.ca> wrote: >> On Wed, Aug 13, 2014 at 1:54 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>> On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote: >>>> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote: >>>>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote: >>>>> > >>>>> > The majority of the DT code is based on the assumption of a static >>>>> > tree. Pantelis has been working on being able to modify it at runtime >>>>> > with overlays, but he has had to go through a lot of rework because it >>>>> > is not a trivial task. When you get into modifying the DT, you need to >>>>> > have a lot more understanding of the side effects to changing the >>>>> > tree. The DT structure also has a lifecycle that can go beyond the >>>>> > current lifecycle of the kernel. The kexec tool will extract the >>>>> > current tree from the kernel, make the appropriate modifications, and >>>>> > use that to boot the next kernel. Allowing any driver to modify the >>>>> > tree has side effects beyond just the current kernel. >>>>> > >>>>> > In this specific case, it will interact badly with the work Pantelis >>>>> > is doing to make platform devices work with overlays. Modifying the >>>>> > status property will cause the associated struct device to get removed >>>>> > in the middle of probing a driver for that device! That will most >>>>> > likely cause an oops. >>>>> > >>>>> > Besides, Luc straight out *said*: "...even though it has no real value >>>>> > today". In what circumstance is that justification for modifying the >>>>> > tree? >>>>> >>>>> With that sentence i meant that given the current state of things, it >>>>> has no real value. >>>>> >>>>> It has no value currently as re-probing simplefb is not going to happen. >>>>> But it's not a big leap to turn simplefb into a proper module. Not that >>>>> that makes much sense, but that's never stopped anyone. >>>>> >>>>> To me it seemed simple, dt is what drives simplefb, so dt then also >>>>> becomes responsible for making sure that simplefb or another driver does >>>>> not attempt to blindly use this info again. The way this is implemented >>>>> i do not care for in any way, i just knew that i could not do nothing >>>>> here, given the catastrophic effect disabling the clocks has on simplefb >>>>> on sunxi. Given the discussion that errupted here, i'd say that this >>>>> does need some resolution, and altering the dt is going to have to be >>>>> part of the solution. >>>>> >>>>> In any case, i will gladly drop this patch, as it is not absolutely >>>>> necessary. But it should be very clear that there is no going back on >>>>> this dt node after the clocks were released once. >>>>> >>>>> Luc Verhaegen. >>>> >>>> What about approaching this from the other end? U-Boot could add a >>>> property named "once-only" or so. >>> >>> Device tree is supposed to be a static description of the hardware >>> usable on all operating systems. It is the wrong mechanism for >>> communicating between uboot and the kernel. Use something like atags >>> or the kernel command line to tell the kernel that the console has >>> already been set up. >> >> Not accurate. While it is primarily hardware description, it is also >> used for firmware communication. There is loads of precedence for >> this. The /chosen node is the most significant example, but there are >> other places where the tree is used to provide state. For example, the >> current-speed property on UART nodes. > > I do seem to recall you telling me a long time ago that those chosen > nodes were a mistake (or maybe it was Matt Sealey). I'm pretty wary of > opening to door to device trees carrying a bunch of state. Five years > from now the DT is going to look like a Christmas tree. Wasn't me. Carrying state in the DT provided by firmware is perfectly reasonable in my opinion. g.
On Thu, Aug 14, 2014 at 6:15 AM, Grant Likely <grant.likely@secretlab.ca> wrote: > On Wed, Aug 13, 2014 at 9:41 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >> On Wed, Aug 13, 2014 at 3:14 PM, Grant Likely <grant.likely@secretlab.ca> wrote: >>> On Wed, Aug 13, 2014 at 1:54 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>>> On Wed, Aug 13, 2014 at 6:19 AM, Luc Verhaegen <libv@skynet.be> wrote: >>>>> On Wed, Aug 13, 2014 at 11:45:24AM +0200, Luc Verhaegen wrote: >>>>>> On Wed, Aug 13, 2014 at 10:23:14AM +0100, Grant Likely wrote: >>>>>> > >>>>>> > The majority of the DT code is based on the assumption of a static >>>>>> > tree. Pantelis has been working on being able to modify it at runtime >>>>>> > with overlays, but he has had to go through a lot of rework because it >>>>>> > is not a trivial task. When you get into modifying the DT, you need to >>>>>> > have a lot more understanding of the side effects to changing the >>>>>> > tree. The DT structure also has a lifecycle that can go beyond the >>>>>> > current lifecycle of the kernel. The kexec tool will extract the >>>>>> > current tree from the kernel, make the appropriate modifications, and >>>>>> > use that to boot the next kernel. Allowing any driver to modify the >>>>>> > tree has side effects beyond just the current kernel. >>>>>> > >>>>>> > In this specific case, it will interact badly with the work Pantelis >>>>>> > is doing to make platform devices work with overlays. Modifying the >>>>>> > status property will cause the associated struct device to get removed >>>>>> > in the middle of probing a driver for that device! That will most >>>>>> > likely cause an oops. >>>>>> > >>>>>> > Besides, Luc straight out *said*: "...even though it has no real value >>>>>> > today". In what circumstance is that justification for modifying the >>>>>> > tree? >>>>>> >>>>>> With that sentence i meant that given the current state of things, it >>>>>> has no real value. >>>>>> >>>>>> It has no value currently as re-probing simplefb is not going to happen. >>>>>> But it's not a big leap to turn simplefb into a proper module. Not that >>>>>> that makes much sense, but that's never stopped anyone. >>>>>> >>>>>> To me it seemed simple, dt is what drives simplefb, so dt then also >>>>>> becomes responsible for making sure that simplefb or another driver does >>>>>> not attempt to blindly use this info again. The way this is implemented >>>>>> i do not care for in any way, i just knew that i could not do nothing >>>>>> here, given the catastrophic effect disabling the clocks has on simplefb >>>>>> on sunxi. Given the discussion that errupted here, i'd say that this >>>>>> does need some resolution, and altering the dt is going to have to be >>>>>> part of the solution. >>>>>> >>>>>> In any case, i will gladly drop this patch, as it is not absolutely >>>>>> necessary. But it should be very clear that there is no going back on >>>>>> this dt node after the clocks were released once. >>>>>> >>>>>> Luc Verhaegen. >>>>> >>>>> What about approaching this from the other end? U-Boot could add a >>>>> property named "once-only" or so. >>>> >>>> Device tree is supposed to be a static description of the hardware >>>> usable on all operating systems. It is the wrong mechanism for >>>> communicating between uboot and the kernel. Use something like atags >>>> or the kernel command line to tell the kernel that the console has >>>> already been set up. >>> >>> Not accurate. While it is primarily hardware description, it is also >>> used for firmware communication. There is loads of precedence for >>> this. The /chosen node is the most significant example, but there are >>> other places where the tree is used to provide state. For example, the >>> current-speed property on UART nodes. >> >> I do seem to recall you telling me a long time ago that those chosen >> nodes were a mistake (or maybe it was Matt Sealey). I'm pretty wary of >> opening to door to device trees carrying a bunch of state. Five years >> from now the DT is going to look like a Christmas tree. > > Wasn't me. Carrying state in the DT provided by firmware is perfectly > reasonable in my opinion. So what are the rules going to be? Once the OS is up and starts changing things the state in the DT and the OS are going to diverge. How does this get handled for a kernel driver that can load/unload? Should the kernel remove those state nodes as soon as it alters the state? > > g.
tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl@gmail.com: > So what are the rules going to be? Once the OS is up and starts > changing things the state in the DT and the OS are going to diverge. > How does this get handled for a kernel driver that can load/unload? > Should the kernel remove those state nodes as soon as it alters the > state? hardware that is no longer there should also not be represented/active in DT. A simple fb that have been torn down is no more, no different from never having been there. It can be argued that it never was in the first place (i.e. that it is not actually hardware) but that it another can of worms and both have their benefits and drawbacks. One thing is certain however, as far as simplefb is concerned it is hardware, not really any different from a persistent framebuffer with hardwired settings in hardware. Regarding kexec, it's the responsibility of the kernel doing kexec to make sure DT and hardware matches for the next started kernel. With hardware that can be reconfigured it is not safe to assume that the DT can be passed as-is from before the kernel reconfigured hardware. If the currently running kernel wants to set up a framebuffer for simplefb use by the next kernel than it's free to do so, but in either case it needs to provide the right information to next kernel. If it was given a simplefb node, but then killed the simplefb then no simplefb node should be provided to the next kernel. If it was not given a simplefb node, but have configured the hardware suitable for simplefb node then it may provide a simplefb node. A UART that have changed rate no longer have that rate. Same applies to numerous other items as well. If a kernel remaps IRQs, bus addresses, or whatever in hardware aspects where such changes is possible then DT needs to be updated as well. A bit less common than a simplefb being destroyed by driver reconfiguring the framebuffer, but if you look at it from a little distance then the problem is exactly the same. Hardware is not 100% static, and therefore hardware description also can not be 100% static. And hardware is getting more and more dynamically reconfigurable. A DT that contains false information is generally worse than no DT / not having that information at all. Regards Henrik
On Thu, Aug 14, 2014 at 8:45 PM, Henrik Nordström <henrik@henriknordstrom.net> wrote: > tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl@gmail.com: > >> So what are the rules going to be? Once the OS is up and starts >> changing things the state in the DT and the OS are going to diverge. >> How does this get handled for a kernel driver that can load/unload? >> Should the kernel remove those state nodes as soon as it alters the >> state? > > hardware that is no longer there should also not be represented/active > in DT. > > A simple fb that have been torn down is no more, no different from never > having been there. It can be argued that it never was in the first place > (i.e. that it is not actually hardware) but that it another can of worms > and both have their benefits and drawbacks. One thing is certain > however, as far as simplefb is concerned it is hardware, not really any > different from a persistent framebuffer with hardwired settings in > hardware. How does the kernel know what clocks to protect? A list in the simplefb node? This list of clocks is a reason for adding simplefb to the compatible list for the real hardware. That way it won't be duplicated. The issues with parameter conflicts between simplefb and the real hardware can be dealt with. If the real hardware wants to add the simplefb compatible field it will need to get its parameters sorted out so that they don't conflict. Clock syntax is standardized in the DTS so it shouldn't be a problem. You can also argue that simplefb should never occur in a DTS file. It is something that uboot will add. And then the kernel will remove when simplefb loads KMS. That logic should apply to all of these dynamic fields. You don't want stale state data in the DT. > > Regarding kexec, it's the responsibility of the kernel doing kexec to > make sure DT and hardware matches for the next started kernel. With > hardware that can be reconfigured it is not safe to assume that the DT > can be passed as-is from before the kernel reconfigured hardware. If the > currently running kernel wants to set up a framebuffer for simplefb use > by the next kernel than it's free to do so, but in either case it needs > to provide the right information to next kernel. > > If it was given a simplefb node, but then killed the simplefb then no > simplefb node should be provided to the next kernel. If it was not given > a simplefb node, but have configured the hardware suitable for simplefb > node then it may provide a simplefb node. > > A UART that have changed rate no longer have that rate. > > Same applies to numerous other items as well. If a kernel remaps IRQs, > bus addresses, or whatever in hardware aspects where such changes is > possible then DT needs to be updated as well. A bit less common than a > simplefb being destroyed by driver reconfiguring the framebuffer, but if > you look at it from a little distance then the problem is exactly the > same. Hardware is not 100% static, and therefore hardware description > also can not be 100% static. And hardware is getting more and more > dynamically reconfigurable. > > A DT that contains false information is generally worse than no DT / not > having that information at all. > > Regards > Henrik >
On Thu, Aug 14, 2014 at 09:27:14PM -0400, jonsmirl@gmail.com wrote: > On Thu, Aug 14, 2014 at 8:45 PM, Henrik Nordström > <henrik@henriknordstrom.net> wrote: > > tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl@gmail.com: > > > >> So what are the rules going to be? Once the OS is up and starts > >> changing things the state in the DT and the OS are going to diverge. > >> How does this get handled for a kernel driver that can load/unload? > >> Should the kernel remove those state nodes as soon as it alters the > >> state? > > > > hardware that is no longer there should also not be represented/active > > in DT. > > > > A simple fb that have been torn down is no more, no different from never > > having been there. It can be argued that it never was in the first place > > (i.e. that it is not actually hardware) but that it another can of worms > > and both have their benefits and drawbacks. One thing is certain > > however, as far as simplefb is concerned it is hardware, not really any > > different from a persistent framebuffer with hardwired settings in > > hardware. > > How does the kernel know what clocks to protect? A list in the > simplefb node? This list of clocks is a reason for adding simplefb to > the compatible list for the real hardware. That way it won't be > duplicated. > > The issues with parameter conflicts between simplefb and the real > hardware can be dealt with. If the real hardware wants to add the > simplefb compatible field it will need to get its parameters sorted > out so that they don't conflict. Clock syntax is standardized in the > DTS so it shouldn't be a problem. It shouldn't be, but apparently, some disagree. Anyway, I'm not sure having the simplefb compatible would work in this use-case, mainly for two reasons: - Most likely, the bindings are going to be very different. Not only about which properties you'll have, but also what you will place in these properties. reg for example have the memory address of the buffer in the simplefb case, while in the KMS driver case, it would have the memory address of the registers. - There's will be a single driver probed. So if you want to go down the hand over road (which is a bit premature at this point if you ask me, but anyway), you will have no driver probed to hand over to. Maxime
On Fri, Aug 15, 2014 at 2:43 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Thu, Aug 14, 2014 at 09:27:14PM -0400, jonsmirl@gmail.com wrote: >> On Thu, Aug 14, 2014 at 8:45 PM, Henrik Nordström >> <henrik@henriknordstrom.net> wrote: >> > tor 2014-08-14 klockan 08:07 -0400 skrev jonsmirl@gmail.com: >> > >> >> So what are the rules going to be? Once the OS is up and starts >> >> changing things the state in the DT and the OS are going to diverge. >> >> How does this get handled for a kernel driver that can load/unload? >> >> Should the kernel remove those state nodes as soon as it alters the >> >> state? >> > >> > hardware that is no longer there should also not be represented/active >> > in DT. >> > >> > A simple fb that have been torn down is no more, no different from never >> > having been there. It can be argued that it never was in the first place >> > (i.e. that it is not actually hardware) but that it another can of worms >> > and both have their benefits and drawbacks. One thing is certain >> > however, as far as simplefb is concerned it is hardware, not really any >> > different from a persistent framebuffer with hardwired settings in >> > hardware. >> >> How does the kernel know what clocks to protect? A list in the >> simplefb node? This list of clocks is a reason for adding simplefb to >> the compatible list for the real hardware. That way it won't be >> duplicated. >> >> The issues with parameter conflicts between simplefb and the real >> hardware can be dealt with. If the real hardware wants to add the >> simplefb compatible field it will need to get its parameters sorted >> out so that they don't conflict. Clock syntax is standardized in the >> DTS so it shouldn't be a problem. > > It shouldn't be, but apparently, some disagree. > > Anyway, I'm not sure having the simplefb compatible would work in this > use-case, mainly for two reasons: > - Most likely, the bindings are going to be very different. Not only > about which properties you'll have, but also what you will place > in these properties. reg for example have the memory address of > the buffer in the simplefb case, while in the KMS driver case, it > would have the memory address of the registers. > - There's will be a single driver probed. So if you want to go down > the hand over road (which is a bit premature at this point if you > ask me, but anyway), you will have no driver probed to hand over > to. The graphic drivers have already been down this road before. Simplefb should be a driver library not a driver. DRM is also a driver library. So to implement simplefb whip up a real skeleton driver for the video hardware that parses the DT and then pokes the appropriate info into the simplefb library. Initially that driver will just include the calls out to the simplefb library. Later on it can get KMS implemented. This real driver will get bound to the hardware and have a dependency that loads the simplefb driver library. Now there is no handover problem. Only a single driver is ever attached to the hardware. When it wants to go into KMS mode it makes a call into the simplefb library to shut down whatever it is doing. But you still need a mechanism for uboot to signal that it has set up simplefb. Adding a dynamic node like this might work... video0: video@01c21800 { compatible = "allwinner,sun4i-a10-video"; clocks = <&apb0_gates 6>, <&ir0_clk>; clock-names = "apb", "video"; interrupts = <0 5 4>; reg = <0x01c21800 0x40>; status = "enabled"; simplefb { new namespace for simplefb parameters } }; I would have suggested a driver library earlier but it has been a while since I worked on DRM and I forgot about it. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c index 72a4f20..74c4b2a 100644 --- a/drivers/video/fbdev/simplefb.c +++ b/drivers/video/fbdev/simplefb.c @@ -138,6 +138,32 @@ static int simplefb_parse_dt(struct platform_device *pdev, return 0; } +/* + * Make sure that nothing tries to inadvertedly re-use this node... + */ +static int +simplefb_dt_disable(struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.of_node; + struct property *property; + int ret; + + property = kzalloc(sizeof(struct property), GFP_KERNEL); + if (!property) + return -ENOMEM; + + property->name = "status"; + property->value = "disabled"; + property->length = strlen(property->value) + 1; + + ret = of_update_property(np, property); + if (ret) + dev_err(&pdev->dev, "%s: failed to update property: %d\n", + __func__, ret); + + return ret; +} + static int simplefb_parse_pd(struct platform_device *pdev, struct simplefb_params *params) { @@ -187,17 +213,20 @@ static int simplefb_probe(struct platform_device *pdev) ret = simplefb_parse_dt(pdev, ¶ms); if (ret) - return ret; + goto error_dt_disable; mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!mem) { dev_err(&pdev->dev, "No memory resource\n"); - return -EINVAL; + ret = -EINVAL; + goto error_dt_disable; } info = framebuffer_alloc(sizeof(struct simplefb_par), &pdev->dev); - if (!info) - return -ENOMEM; + if (!info) { + ret = -ENOMEM; + goto error_dt_disable; + } platform_set_drvdata(pdev, info); par = info->par; @@ -260,6 +289,10 @@ static int simplefb_probe(struct platform_device *pdev) error_fb_release: framebuffer_release(info); + error_dt_disable: + if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node) + simplefb_dt_disable(pdev); + return ret; } @@ -269,6 +302,8 @@ static int simplefb_remove(struct platform_device *pdev) unregister_framebuffer(info); framebuffer_release(info); + if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node) + simplefb_dt_disable(pdev); return 0; }
The next commit will handle clocks correctly, so that these do not get automatically disabled on certain SoC simplefb implementations. As a result, the removal of this simplefb driver, and the release of the clocks, is rather final, and only a full display driver can work after this. So, it makes sense to also flag the dt node as disabled, even though it has no real value today. Signed-off-by: Luc Verhaegen <libv@skynet.be> --- drivers/video/fbdev/simplefb.c | 43 ++++++++++++++++++++++++++++++++++++--- 1 files changed, 39 insertions(+), 4 deletions(-)