| Message ID | 1412345120-24588-1-git-send-email-hdegoede@redhat.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: > A simple-framebuffer node represents a framebuffer setup by the firmware / > bootloader. Such a framebuffer may have a number of clocks in use, add a > property to communicate this to the OS. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > Reviewed-by: Mike Turquette <mturquette@linaro.org> > > -- > Changes in v2: > -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> > Changes in v3: > -Updated description to make clear simplefb deals with more then just memory NAK. "Fixing" the description is not what I meant and does not address my concerns. Currently, simplefb is configuration data. It is auxiliary data about how a chunk of memory is used. Using it or not has no side effects on the hardware setup, but you are changing that aspect. You are mixing in a hardware description that is simply inaccurate. The kernel has made the decision to turn off "unused" clocks. If its determination of what is unused is wrong, then it is not a problem to fix in DT. Rob > --- > Documentation/devicetree/bindings/video/simple-framebuffer.txt | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt > index 70c26f3..91176ee 100644 > --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt > +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt > @@ -1,8 +1,8 @@ > Simple Framebuffer > > -A simple frame-buffer describes a raw memory region that may be rendered to, > -with the assumption that the display hardware has already been set up to scan > -out from that buffer. > +A simple frame-buffer describes a frame-buffer setup by firmware or > +the bootloader, with the assumption that the display hardware has already > +been set up to scan out from the memory pointed to by the ref property. > > Required properties: > - compatible: "simple-framebuffer" > @@ -14,6 +14,9 @@ Required properties: > - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b). > - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r). > > +Optional properties: > +- clocks : List of clocks used by the framebuffer > + > Example: > > framebuffer { > -- > 2.1.0 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Rob, On Fri, Oct 3, 2014 at 5:57 PM, Rob Herring <robherring2@gmail.com> wrote: > On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> A simple-framebuffer node represents a framebuffer setup by the firmware / >> bootloader. Such a framebuffer may have a number of clocks in use, add a >> property to communicate this to the OS. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> Reviewed-by: Mike Turquette <mturquette@linaro.org> >> >> -- >> Changes in v2: >> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >> Changes in v3: >> -Updated description to make clear simplefb deals with more then just memory > > NAK. "Fixing" the description is not what I meant and does not address > my concerns. Currently, simplefb is configuration data. It is > auxiliary data about how a chunk of memory is used. Using it or not > has no side effects on the hardware setup, but you are changing that > aspect. You are mixing in a hardware description that is simply > inaccurate. > > The kernel has made the decision to turn off "unused" clocks. If its > determination of what is unused is wrong, then it is not a problem to > fix in DT. The kernel has made that decision because the driver hadn't told the kernel that those clocks had to be enabled. The only way for the driver to know which clocks to enable is by adding them to the description in DT. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 3, 2014 at 11:04 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Rob, > > On Fri, Oct 3, 2014 at 5:57 PM, Rob Herring <robherring2@gmail.com> wrote: >> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>> property to communicate this to the OS. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>> >>> -- >>> Changes in v2: >>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>> Changes in v3: >>> -Updated description to make clear simplefb deals with more then just memory >> >> NAK. "Fixing" the description is not what I meant and does not address >> my concerns. Currently, simplefb is configuration data. It is >> auxiliary data about how a chunk of memory is used. Using it or not >> has no side effects on the hardware setup, but you are changing that >> aspect. You are mixing in a hardware description that is simply >> inaccurate. >> >> The kernel has made the decision to turn off "unused" clocks. If its >> determination of what is unused is wrong, then it is not a problem to >> fix in DT. > > The kernel has made that decision because the driver hadn't told the > kernel that those clocks had to be enabled. > The only way for the driver to know which clocks to enable is by adding > them to the description in DT. Lack of a proper and complete driver is still a kernel problem. Now, if you want to accurately describe the display h/w in DT and you happen to use the simplefb driver, I don't really care. It just needs to be a separate binding. A driver is not the only solution here. The SOC or clock controller code could keep the clock on based on either the fact that the clock is already on or it is just hardcoded. This is not a problem unique to displays. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 10/03/2014 05:57 PM, Rob Herring wrote: > On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> A simple-framebuffer node represents a framebuffer setup by the firmware / >> bootloader. Such a framebuffer may have a number of clocks in use, add a >> property to communicate this to the OS. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> Reviewed-by: Mike Turquette <mturquette@linaro.org> >> >> -- >> Changes in v2: >> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >> Changes in v3: >> -Updated description to make clear simplefb deals with more then just memory > > NAK. "Fixing" the description is not what I meant and does not address > my concerns. Currently, simplefb is configuration data. It is > auxiliary data about how a chunk of memory is used. Using it or not > has no side effects on the hardware setup, but you are changing that > aspect. You are mixing in a hardware description that is simply > inaccurate. Memory is hardware too, what simplefb is is best seen as a virtual device, and even virtual devices have hardware resources they need, such as a chunk of memory, which the kernel should not touch other then use it as a framebuffer, likewise on some systems the virtual device needs clocks to keep running. > The kernel has made the decision to turn off "unused" clocks. If its > determination of what is unused is wrong, then it is not a problem to > fix in DT. No, it is up to DT to tell the kernel what clocks are used, that is how it works for any other device. I don't understand why some people keep insisting simplefb for some reason is o so very very special, because it is not special, it needs resources, and it needs to tell the kernel about this or bad things happen. More over it is a bit late to start making objections now. This has already been discussed to death for weeks now, and every argument against this patch has already been countered multiple times (including the one you are making now). Feel free to read the entire thread in the archives under the subject: "[PATCH 4/4] simplefb: add clock handling code" At one point in time we need to stop bickering and make a decision, that time has come now, so please lets get this discussion over with and merge this, so that we can all move on and spend out time in a more productive manner. Regards, Hans > > Rob > >> --- >> Documentation/devicetree/bindings/video/simple-framebuffer.txt | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt >> index 70c26f3..91176ee 100644 >> --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt >> +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt >> @@ -1,8 +1,8 @@ >> Simple Framebuffer >> >> -A simple frame-buffer describes a raw memory region that may be rendered to, >> -with the assumption that the display hardware has already been set up to scan >> -out from that buffer. >> +A simple frame-buffer describes a frame-buffer setup by firmware or >> +the bootloader, with the assumption that the display hardware has already >> +been set up to scan out from the memory pointed to by the ref property. >> >> Required properties: >> - compatible: "simple-framebuffer" >> @@ -14,6 +14,9 @@ Required properties: >> - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b). >> - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r). >> >> +Optional properties: >> +- clocks : List of clocks used by the framebuffer >> + >> Example: >> >> framebuffer { >> -- >> 2.1.0 >> > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 10/03/2014 06:19 PM, Rob Herring wrote: > On Fri, Oct 3, 2014 at 11:04 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> Hi Rob, >> >> On Fri, Oct 3, 2014 at 5:57 PM, Rob Herring <robherring2@gmail.com> wrote: >>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>> property to communicate this to the OS. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>> >>>> -- >>>> Changes in v2: >>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>> Changes in v3: >>>> -Updated description to make clear simplefb deals with more then just memory >>> >>> NAK. "Fixing" the description is not what I meant and does not address >>> my concerns. Currently, simplefb is configuration data. It is >>> auxiliary data about how a chunk of memory is used. Using it or not >>> has no side effects on the hardware setup, but you are changing that >>> aspect. You are mixing in a hardware description that is simply >>> inaccurate. >>> >>> The kernel has made the decision to turn off "unused" clocks. If its >>> determination of what is unused is wrong, then it is not a problem to >>> fix in DT. >> >> The kernel has made that decision because the driver hadn't told the >> kernel that those clocks had to be enabled. >> The only way for the driver to know which clocks to enable is by adding >> them to the description in DT. > > Lack of a proper and complete driver is still a kernel problem. Now, > if you want to accurately describe the display h/w in DT and you > happen to use the simplefb driver, I don't really care. It just needs > to be a separate binding. Please read the: "[PATCH 4/4] simplefb: add clock handling code" thread. The whole purpose we want to use simplefb for is to have a hardware agnostic driver for early boot messages. Not all devices have a usable serial console, so this is a must have for user-friendly debugging of boot problems. Basically the devicetree equivalent of vgacon / efifb. So we actually do not want to describe the hardware accurately, we want something generic, which simplefb gives us, we just want it to be a slightly more complete description then simplefb currently gives as, as the current description is too limited in practice, specifically the simpefb virtual device needs to accurately declare which clocks it uses, just like any other real hardware device in devicetree declares which clocks it uses. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 10/03/2014 05:57 PM, Rob Herring wrote: >> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>> property to communicate this to the OS. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>> >>> -- >>> Changes in v2: >>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>> Changes in v3: >>> -Updated description to make clear simplefb deals with more then just memory >> >> NAK. "Fixing" the description is not what I meant and does not address >> my concerns. Currently, simplefb is configuration data. It is >> auxiliary data about how a chunk of memory is used. Using it or not >> has no side effects on the hardware setup, but you are changing that >> aspect. You are mixing in a hardware description that is simply >> inaccurate. > > Memory is hardware too, what simplefb is is best seen as a virtual device, and > even virtual devices have hardware resources they need, such as a chunk of memory, > which the kernel should not touch other then use it as a framebuffer, likewise > on some systems the virtual device needs clocks to keep running. > >> The kernel has made the decision to turn off "unused" clocks. If its >> determination of what is unused is wrong, then it is not a problem to >> fix in DT. > > No, it is up to DT to tell the kernel what clocks are used, that is how it works > for any other device. I don't understand why some people keep insisting simplefb > for some reason is o so very very special, because it is not special, it needs > resources, and it needs to tell the kernel about this or bad things happen. No, the DT describes the connections of clocks from h/w block to h/w block. Their use is implied by the connection. And yes, as the other thread mentioned DT is more than just hardware information. However, what you are adding IS hardware information and clearly has a place somewhere else. And adding anything which is not hardware description gets much more scrutiny. > More over it is a bit late to start making objections now. This has already been > discussed to death for weeks now, and every argument against this patch has already > been countered multiple times (including the one you are making now). Feel free to > read the entire thread in the archives under the subject: > "[PATCH 4/4] simplefb: add clock handling code" You are on v2 and I hardly see any consensus on the v1 thread. Others have made suggestions which I would agree with and you've basically ignored them. > At one point in time we need to stop bickering and make a decision, that time has > come now, so please lets get this discussion over with and merge this, so that > we can all move on and spend out time in a more productive manner. Not an effective argument to get things merged. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: > On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 10/03/2014 05:57 PM, Rob Herring wrote: >>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>> property to communicate this to the OS. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>> >>>> -- >>>> Changes in v2: >>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>> Changes in v3: >>>> -Updated description to make clear simplefb deals with more then just memory >>> >>> NAK. "Fixing" the description is not what I meant and does not address >>> my concerns. Currently, simplefb is configuration data. It is >>> auxiliary data about how a chunk of memory is used. Using it or not >>> has no side effects on the hardware setup, but you are changing that >>> aspect. You are mixing in a hardware description that is simply >>> inaccurate. >> >> Memory is hardware too, what simplefb is is best seen as a virtual device, and >> even virtual devices have hardware resources they need, such as a chunk of memory, >> which the kernel should not touch other then use it as a framebuffer, likewise >> on some systems the virtual device needs clocks to keep running. >> >>> The kernel has made the decision to turn off "unused" clocks. If its >>> determination of what is unused is wrong, then it is not a problem to >>> fix in DT. >> >> No, it is up to DT to tell the kernel what clocks are used, that is how it works >> for any other device. I don't understand why some people keep insisting simplefb >> for some reason is o so very very special, because it is not special, it needs >> resources, and it needs to tell the kernel about this or bad things happen. > > No, the DT describes the connections of clocks from h/w block to h/w > block. Their use is implied by the connection. > > And yes, as the other thread mentioned DT is more than just hardware > information. However, what you are adding IS hardware information and > clearly has a place somewhere else. And adding anything which is not > hardware description gets much more scrutiny. > >> More over it is a bit late to start making objections now. This has already been >> discussed to death for weeks now, and every argument against this patch has already >> been countered multiple times (including the one you are making now). Feel free to >> read the entire thread in the archives under the subject: >> "[PATCH 4/4] simplefb: add clock handling code" > > You are on v2 and I hardly see any consensus on the v1 thread. Others > have made suggestions which I would agree with and you've basically > ignored them. > >> At one point in time we need to stop bickering and make a decision, that time has >> come now, so please lets get this discussion over with and merge this, so that >> we can all move on and spend out time in a more productive manner. > > Not an effective argument to get things merged. This is another way to solve the problem..... On Thu, Oct 2, 2014 at 9:46 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Oct 2, 2014 at 3:34 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >> Does the clock and regulator cleanup happen before drivers can load >> off from initrd? I didn't think it did but I might be wrong. > > Yes > > drivers/base/power/domain.c:late_initcall(genpd_poweroff_unused); > drivers/clk/clk.c:late_initcall_sync(clk_disable_unused); > drivers/regulator/core.c:late_initcall_sync(regulator_init_complete); What do you think about putting these calls onto an ioctl somewhere and then eliminating the late_initcall(..)? A tiny user space app could then hit that ioctl after all of the loadable device drivers are loaded. Add the command to make this ioctl call to busybox or udev. After all, it is not fatal if these calls aren't made, all they do is save power. Add a link in rc.d or somewhere similar to run this app at the appropriate time. Switching these over to an ioctl allows a window to be opened for device specific driver loading before the clock/regulator clean up happens. Now all of this mess of protecting clocks and regulator disappears. Instead get the device specific drivers written and loaded, then run the cleanup app which hits the ioctl(). All of the correct clock/regulators will be claimed and then this clean code will do the right thing. >From my perspective it appears that this cleanup is being done too early which then triggers a need to protect things from cleanup. > > Gr{oetje,eeting}s, > > Geert > > Rob > > -- > 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 Fri, Oct 3, 2014 at 10:55 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > On Thu, Oct 2, 2014 at 9:46 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >> On Thu, Oct 2, 2014 at 3:34 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>> Does the clock and regulator cleanup happen before drivers can load >>> off from initrd? I didn't think it did but I might be wrong. >> >> Yes >> >> drivers/base/power/domain.c:late_initcall(genpd_poweroff_unused); >> drivers/clk/clk.c:late_initcall_sync(clk_disable_unused); >> drivers/regulator/core.c:late_initcall_sync(regulator_init_complete); > > What do you think about putting these calls onto an ioctl somewhere > and then eliminating the late_initcall(..)? A tiny user space app > could then hit that ioctl after all of the loadable device drivers are > loaded. Add the command to make this ioctl call to busybox or udev. > After all, it is not fatal if these calls aren't made, all they do is > save power. Add a link in rc.d or somewhere similar to run this app at > the appropriate time. > > Switching these over to an ioctl allows a window to be opened for > device specific driver loading before the clock/regulator clean up > happens. > > Now all of this mess of protecting clocks and regulator disappears. > Instead get the device specific drivers written and loaded, then run > the cleanup app which hits the ioctl(). All of the correct > clock/regulators will be claimed and then this clean code will do the > right thing. > > From my perspective it appears that this cleanup is being done too > early which then triggers a need to protect things from cleanup. Not doing the cleanup doesn't help. If someone else calls clk_disable() on a clock which shares a parent with the clock you're silently using, that clock will still be disabled. This can happen at any time. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 3, 2014 at 5:26 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Fri, Oct 3, 2014 at 10:55 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >> On Thu, Oct 2, 2014 at 9:46 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: >>> On Thu, Oct 2, 2014 at 3:34 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>>> Does the clock and regulator cleanup happen before drivers can load >>>> off from initrd? I didn't think it did but I might be wrong. >>> >>> Yes >>> >>> drivers/base/power/domain.c:late_initcall(genpd_poweroff_unused); >>> drivers/clk/clk.c:late_initcall_sync(clk_disable_unused); >>> drivers/regulator/core.c:late_initcall_sync(regulator_init_complete); >> >> What do you think about putting these calls onto an ioctl somewhere >> and then eliminating the late_initcall(..)? A tiny user space app >> could then hit that ioctl after all of the loadable device drivers are >> loaded. Add the command to make this ioctl call to busybox or udev. >> After all, it is not fatal if these calls aren't made, all they do is >> save power. Add a link in rc.d or somewhere similar to run this app at >> the appropriate time. >> >> Switching these over to an ioctl allows a window to be opened for >> device specific driver loading before the clock/regulator clean up >> happens. >> >> Now all of this mess of protecting clocks and regulator disappears. >> Instead get the device specific drivers written and loaded, then run >> the cleanup app which hits the ioctl(). All of the correct >> clock/regulators will be claimed and then this clean code will do the >> right thing. >> >> From my perspective it appears that this cleanup is being done too >> early which then triggers a need to protect things from cleanup. > > Not doing the cleanup doesn't help. > > If someone else calls clk_disable() on a clock which shares a parent > with the clock you're silently using, that clock will still be disabled. > This can happen at any time. Could we start all of the regulators and clocks off with a reference count of one, but not do anything to change their state? Then this ioctl() would decrement that extra reference. Removing the extra reference would then disable everything that isn't claimed. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: > On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 10/03/2014 05:57 PM, Rob Herring wrote: >>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>> property to communicate this to the OS. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>> >>>> -- >>>> Changes in v2: >>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>> Changes in v3: >>>> -Updated description to make clear simplefb deals with more then just memory >>> >>> NAK. "Fixing" the description is not what I meant and does not address >>> my concerns. Currently, simplefb is configuration data. It is >>> auxiliary data about how a chunk of memory is used. Using it or not >>> has no side effects on the hardware setup, but you are changing that >>> aspect. You are mixing in a hardware description that is simply >>> inaccurate. >> >> Memory is hardware too, what simplefb is is best seen as a virtual device, and >> even virtual devices have hardware resources they need, such as a chunk of memory, >> which the kernel should not touch other then use it as a framebuffer, likewise >> on some systems the virtual device needs clocks to keep running. >> >>> The kernel has made the decision to turn off "unused" clocks. If its >>> determination of what is unused is wrong, then it is not a problem to >>> fix in DT. >> >> No, it is up to DT to tell the kernel what clocks are used, that is how it works >> for any other device. I don't understand why some people keep insisting simplefb >> for some reason is o so very very special, because it is not special, it needs >> resources, and it needs to tell the kernel about this or bad things happen. > > No, the DT describes the connections of clocks from h/w block to h/w > block. Their use is implied by the connection. > > And yes, as the other thread mentioned DT is more than just hardware > information. However, what you are adding IS hardware information and > clearly has a place somewhere else. And adding anything which is not > hardware description gets much more scrutiny. > >> More over it is a bit late to start making objections now. This has already been >> discussed to death for weeks now, and every argument against this patch has already >> been countered multiple times (including the one you are making now). Feel free to >> read the entire thread in the archives under the subject: >> "[PATCH 4/4] simplefb: add clock handling code" > > You are on v2 and I hardly see any consensus on the v1 thread. Others > have made suggestions which I would agree with and you've basically > ignored them. > >> At one point in time we need to stop bickering and make a decision, that time has >> come now, so please lets get this discussion over with and merge this, so that >> we can all move on and spend out time in a more productive manner. > > Not an effective argument to get things merged. If there is not good solution to deferring clock clean up in the kernel, another approach is to change how simple-framebuffer is described in the device tree.... Right now it is a stand-alone item that looks like a device node, but it isn't a device. framebuffer { compatible = "simple-framebuffer"; reg = <0x1d385000 (1600 * 1200 * 2)>; width = <1600>; height = <1200>; stride = <(1600 * 2)>; format = "r5g6b5"; }; How about something like this? reserved-memory { #address-cells = <1>; #size-cells = <1>; ranges; display_reserved: framebuffer@78000000 { reg = <0x78000000 (1600 * 1200 * 2)>; }; }; lcd0: lcd-controller@820000 { compatible = "marvell,dove-lcd"; reg = <0x820000 0x1000>; interrupts = <47>; clocks = <&si5351 0>; clock-names = "ext_ref_clk_1"; }; chosen { boot-framebuffer { compatible = "simple-framebuffer"; device = <&lcd0>; framebuffer = <&display_reserved>; width = <1600>; height = <1200>; stride = <(1600 * 2)>; format = "r5g6b5"; }; } This moves the definition of the boot framebuffer setup into the area where bootloader info is suppose to go. Then you can use the phandle to follow the actual device chains and protect the clocks and regulators. To make that work you are required to provide an accurate description of the real video hardware so that this chain can be followed. The main assumption here is that the syntax for clock and regulators is standardized enough that the generic simplefb code can run the phandle chain and discover them all. If they aren't then we messed up and simplefb gains some complexity to handle the irregular descriptions. That is something having a schema for device tree would have prevented. The compatible string should trigger simple-fb into making a 'struct device' that isn't hooked to any real hardware. Not clear that we should use a compatible = "" here. The device = <> points to the real hardware. Run that phandle chain and figure out what clocks need to be protected. Now when the driver for dove-lcd loads there won't be anything already attached to the hardware so there is no complex hand off needed. dove-lcd will just need to signal simple-fb to exit Exiting will decrement the ref counts on clocks/regulators. The memory region also allows for a ref count mechanism which allows it to be passed from simplefb to dove-lcd. If the ref count goes to zero when simplefb exits, it can be reclaimed. > > Rob > > -- > 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.
Hi, On 10/03/2014 10:08 PM, Rob Herring wrote: > On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 10/03/2014 05:57 PM, Rob Herring wrote: >>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>> property to communicate this to the OS. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>> >>>> -- >>>> Changes in v2: >>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>> Changes in v3: >>>> -Updated description to make clear simplefb deals with more then just memory >>> >>> NAK. "Fixing" the description is not what I meant and does not address >>> my concerns. Currently, simplefb is configuration data. It is >>> auxiliary data about how a chunk of memory is used. Using it or not >>> has no side effects on the hardware setup, but you are changing that >>> aspect. You are mixing in a hardware description that is simply >>> inaccurate. >> >> Memory is hardware too, what simplefb is is best seen as a virtual device, and >> even virtual devices have hardware resources they need, such as a chunk of memory, >> which the kernel should not touch other then use it as a framebuffer, likewise >> on some systems the virtual device needs clocks to keep running. >> >>> The kernel has made the decision to turn off "unused" clocks. If its >>> determination of what is unused is wrong, then it is not a problem to >>> fix in DT. >> >> No, it is up to DT to tell the kernel what clocks are used, that is how it works >> for any other device. I don't understand why some people keep insisting simplefb >> for some reason is o so very very special, because it is not special, it needs >> resources, and it needs to tell the kernel about this or bad things happen. > > No, the DT describes the connections of clocks from h/w block to h/w > block. Their use is implied by the connection. So normally DT describes HW aka clock connections, and your objection is that I cannot add HW description AKA clock connections to the simplefb node. > And yes, as the other thread mentioned DT is more than just hardware > information. However, what you are adding IS hardware information and > clearly has a place somewhere else. And adding anything which is not > hardware description gets much more scrutiny. But what I'm adding is not actually clock connections (aka HW description), the display engine has many clocks, and what is being added to the simplefb node is which clocks are *actually* used by the video mode setup for the simplefb, not the clock connections, not which clocks can be used by the hw-block_s_ involved in the display engine, but which ones are actually used for the specific mode (and output connector) setup for the simplefb. So according to your definition this is not HW description. This is usage description, just like the simplefb node contains which memory is used. >> More over it is a bit late to start making objections now. This has already been >> discussed to death for weeks now, and every argument against this patch has already >> been countered multiple times (including the one you are making now). Feel free to >> read the entire thread in the archives under the subject: >> "[PATCH 4/4] simplefb: add clock handling code" > > You are on v2 and I hardly see any consensus on the v1 thread. Others > have made suggestions which I would agree with and you've basically > ignored them. I have NOT ignored those, that is simply not true. I've even offered an alternative approach myself, which was quickly shot down, see: http://www.spinics.net/lists/arm-kernel/msg367559.html And being quickly shotdown is exactly what my alternative solution has in common with all the other alternatives. Non of them provide the simplicity nor robustness of simply adding clocks to the dt-node. And this is not surprising since adding clocks to the dt-node is exactly the right mechanism to tell the kernel that certain clocks are used for something. Mike Turquette, the clk maintainer, agrees that this is the right thing to do. Yet people keep arguing that simplefb is magically special and thus cannot have clocks. Arguments which to me after discussing this for weeks in a row are starting to sound like "you cannot do this because I say so" rather then proper technical arguments. It seems to me that you're arguing purely at semantics without looking at the use case in question at all. In the end creating a workable and working solution should always trump semantics. Note that I'm willing to look at alternatives, as long as those are not tons more complicated then the current proposal, and they don't introduce a number of trouble some unhandled corner cases which the current proposal does not introduce. Unfortunately all alternatives proposed sofar are both more complicated and introduce unhandled corner cases. Adding the clocks to the simplefb node is very KISS and for those who actually have looked at the requirements for the use-cases we're discussing here also feels like a natural fit, which usually is an indication that it is the right solution. So since adding clocks to simplefb seems problematic for some people, how about the following, we add a new firmwarefb binding, which is basically simplefb + clocks, then people who want simplefb to stay clean and elegant can use the clean and elegant simplefb bindings, and people who have a need to express clocks usage can do so using the firmwarefb binding. Would that be acceptable to you ? Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: > On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>> Hi, >>> >>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>> property to communicate this to the OS. >>>>> >>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>> >>>>> -- >>>>> Changes in v2: >>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>> Changes in v3: >>>>> -Updated description to make clear simplefb deals with more then just memory >>>> >>>> NAK. "Fixing" the description is not what I meant and does not address >>>> my concerns. Currently, simplefb is configuration data. It is >>>> auxiliary data about how a chunk of memory is used. Using it or not >>>> has no side effects on the hardware setup, but you are changing that >>>> aspect. You are mixing in a hardware description that is simply >>>> inaccurate. >>> >>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>> even virtual devices have hardware resources they need, such as a chunk of memory, >>> which the kernel should not touch other then use it as a framebuffer, likewise >>> on some systems the virtual device needs clocks to keep running. >>> >>>> The kernel has made the decision to turn off "unused" clocks. If its >>>> determination of what is unused is wrong, then it is not a problem to >>>> fix in DT. >>> >>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>> for any other device. I don't understand why some people keep insisting simplefb >>> for some reason is o so very very special, because it is not special, it needs >>> resources, and it needs to tell the kernel about this or bad things happen. >> >> No, the DT describes the connections of clocks from h/w block to h/w >> block. Their use is implied by the connection. >> >> And yes, as the other thread mentioned DT is more than just hardware >> information. However, what you are adding IS hardware information and >> clearly has a place somewhere else. And adding anything which is not >> hardware description gets much more scrutiny. >> >>> More over it is a bit late to start making objections now. This has already been >>> discussed to death for weeks now, and every argument against this patch has already >>> been countered multiple times (including the one you are making now). Feel free to >>> read the entire thread in the archives under the subject: >>> "[PATCH 4/4] simplefb: add clock handling code" >> >> You are on v2 and I hardly see any consensus on the v1 thread. Others >> have made suggestions which I would agree with and you've basically >> ignored them. >> >>> At one point in time we need to stop bickering and make a decision, that time has >>> come now, so please lets get this discussion over with and merge this, so that >>> we can all move on and spend out time in a more productive manner. >> >> Not an effective argument to get things merged. > > If there is not good solution to deferring clock clean up in the > kernel, another approach is to change how simple-framebuffer is > described in the device tree.... > > Right now it is a stand-alone item that looks like a device node, but > it isn't a device. > > framebuffer { > compatible = "simple-framebuffer"; > reg = <0x1d385000 (1600 * 1200 * 2)>; > width = <1600>; > height = <1200>; > stride = <(1600 * 2)>; > format = "r5g6b5"; > }; > > How about something like this? > > reserved-memory { > #address-cells = <1>; > #size-cells = <1>; > ranges; > > display_reserved: framebuffer@78000000 { > reg = <0x78000000 (1600 * 1200 * 2)>; > }; > }; > > lcd0: lcd-controller@820000 { > compatible = "marvell,dove-lcd"; > reg = <0x820000 0x1000>; > interrupts = <47>; > clocks = <&si5351 0>; > clock-names = "ext_ref_clk_1"; > }; > > chosen { > boot-framebuffer { > compatible = "simple-framebuffer"; > device = <&lcd0>; > framebuffer = <&display_reserved>; > width = <1600>; > height = <1200>; > stride = <(1600 * 2)>; > format = "r5g6b5"; > }; > } > > > This moves the definition of the boot framebuffer setup into the area > where bootloader info is suppose to go. Then you can use the phandle > to follow the actual device chains and protect the clocks and > regulators. To make that work you are required to provide an accurate > description of the real video hardware so that this chain can be > followed. This will not work, first of all multiple blocks may be involved, so the device = in the boot-framebuffer would need to be a list. That in itself is not a problem, the problem is that the blocks used may have multiple clocks, of which the setup mode likely uses only a few. So if we do things this way, we end up keeping way to many clocks enabled. This does nicely show why the clocks really belong in the simplefb node though. What you suggest above, would lead to simplefb having access to the hardware info / description of the display engine blocks, putting the clocks info in the display engine nodes, so keeping hardware description with hardware description as people want. But as said that does not tell the kernel which clocks are actually used for the setup mode, so it does not provide the info the kernel needs. What the kernel needs is not hardware description, but a list of clocks which are *actually used* by the setup mode. Thinking more about this the clocks property in the simplefb node is just like the reg property. Both are properties normally only found in real harware nodes, not in an informational node like simplefb. But for the kernel to be able to actually use the simplefb it needs to know which memory is used by the framebuffer. One could argue that a reg property is hardware description, and thus does not belong in the simplefb node, and that the kernel should just magically figure out which memory is used. Everyone sees that the kernel cannot magically figure out which memory is used by the simplefb. Yet people expect the kernel to somehow magically figure out which clocks are used, to avoid accidentally turning them off. This is not consistent, either the kernel needs to be told these things, and then it needs to be told all of them, or hardware node properties like a reg property do not belong in an informational node, and then the reg property should not be in the simplefb node either, and the kernel should somehow magically figure out where the memory lives, just like some people are expecting the kernel to magically figure out which clocks are used. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi, >>>> >>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>> property to communicate this to the OS. >>>>>> >>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>> >>>>>> -- >>>>>> Changes in v2: >>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>> Changes in v3: >>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>> >>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>> my concerns. Currently, simplefb is configuration data. It is >>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>> has no side effects on the hardware setup, but you are changing that >>>>> aspect. You are mixing in a hardware description that is simply >>>>> inaccurate. >>>> >>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>> on some systems the virtual device needs clocks to keep running. >>>> >>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>> determination of what is unused is wrong, then it is not a problem to >>>>> fix in DT. >>>> >>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>> for any other device. I don't understand why some people keep insisting simplefb >>>> for some reason is o so very very special, because it is not special, it needs >>>> resources, and it needs to tell the kernel about this or bad things happen. >>> >>> No, the DT describes the connections of clocks from h/w block to h/w >>> block. Their use is implied by the connection. >>> >>> And yes, as the other thread mentioned DT is more than just hardware >>> information. However, what you are adding IS hardware information and >>> clearly has a place somewhere else. And adding anything which is not >>> hardware description gets much more scrutiny. >>> >>>> More over it is a bit late to start making objections now. This has already been >>>> discussed to death for weeks now, and every argument against this patch has already >>>> been countered multiple times (including the one you are making now). Feel free to >>>> read the entire thread in the archives under the subject: >>>> "[PATCH 4/4] simplefb: add clock handling code" >>> >>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>> have made suggestions which I would agree with and you've basically >>> ignored them. >>> >>>> At one point in time we need to stop bickering and make a decision, that time has >>>> come now, so please lets get this discussion over with and merge this, so that >>>> we can all move on and spend out time in a more productive manner. >>> >>> Not an effective argument to get things merged. >> >> If there is not good solution to deferring clock clean up in the >> kernel, another approach is to change how simple-framebuffer is >> described in the device tree.... >> >> Right now it is a stand-alone item that looks like a device node, but >> it isn't a device. >> >> framebuffer { >> compatible = "simple-framebuffer"; >> reg = <0x1d385000 (1600 * 1200 * 2)>; >> width = <1600>; >> height = <1200>; >> stride = <(1600 * 2)>; >> format = "r5g6b5"; >> }; >> >> How about something like this? >> >> reserved-memory { >> #address-cells = <1>; >> #size-cells = <1>; >> ranges; >> >> display_reserved: framebuffer@78000000 { >> reg = <0x78000000 (1600 * 1200 * 2)>; >> }; >> }; >> >> lcd0: lcd-controller@820000 { >> compatible = "marvell,dove-lcd"; >> reg = <0x820000 0x1000>; >> interrupts = <47>; >> clocks = <&si5351 0>; >> clock-names = "ext_ref_clk_1"; >> }; >> >> chosen { >> boot-framebuffer { >> compatible = "simple-framebuffer"; >> device = <&lcd0>; >> framebuffer = <&display_reserved>; >> width = <1600>; >> height = <1200>; >> stride = <(1600 * 2)>; >> format = "r5g6b5"; >> }; >> } >> >> >> This moves the definition of the boot framebuffer setup into the area >> where bootloader info is suppose to go. Then you can use the phandle >> to follow the actual device chains and protect the clocks and >> regulators. To make that work you are required to provide an accurate >> description of the real video hardware so that this chain can be >> followed. > > This will not work, first of all multiple blocks may be involved, so > the device = in the boot-framebuffer would need to be a list. That in > itself is not a problem, the problem is that the blocks used may have > multiple clocks, of which the setup mode likely uses only a few. > > So if we do things this way, we end up keeping way to many clocks > enabled. That doesn't hurt anything. Simplefb is not going to enable anything, it is just protecting things from getting disabled. The whole point of simplefb is to assume that the BIOS set things up correctly. When the device specific driver loads everything will get sorted out and it will own the correct clocks. This does require a device specific driver to be written. Fairly easy to do by providing a device specific framebuffer driver while we wait on the KMS one. I do not support letting the life of simplefb extend past the initial boot window. For example - it is not going to came back after suspend/resume. It doesn't work for simplefb to just increment the ref count as a way of protecting the clocks/regulator. If you inc the ref count and the clock is off, the device specific driver can't turn it on. We have to build a new 'protected' mechanism for the clocks/regulators. When a clock/regulator is 'protected' it can't turn off until both the ref count is zero and the protected bit is off. 'protected' does not impact turning it on. And all of this is a problem up in the OS. Device trees are hardware descriptions, they are not meant to be manipulated into fixing OS level problems. > > This does nicely show why the clocks really belong in the simplefb > node though. What you suggest above, would lead to simplefb having > access to the hardware info / description of the display engine > blocks, putting the clocks info in the display engine nodes, so > keeping hardware description with hardware description as people > want. > > But as said that does not tell the kernel which clocks are actually > used for the setup mode, so it does not provide the info the kernel > needs. What the kernel needs is not hardware description, but a > list of clocks which are *actually used* by the setup mode. > > Thinking more about this the clocks property in the simplefb node > is just like the reg property. Both are properties normally only > found in real harware nodes, not in an informational node like > simplefb. > > But for the kernel to be able to actually use the simplefb it > needs to know which memory is used by the framebuffer. One could > argue that a reg property is hardware description, and thus does not > belong in the simplefb node, and that the kernel should just magically > figure out which memory is used. This is not a good parallel. That memory buffer has been given to the hardware so my example DT is wrong, That's why we need to review these things. It should be like this... The device tree is reflecting ownership, not state. If the buffer is not under the hardware when simplefb disappears it would get freed and the display controller still owns it and is still using it. >> reserved-memory { >> #address-cells = <1>; >> #size-cells = <1>; >> ranges; >> >> display_reserved: framebuffer@78000000 { >> reg = <0x78000000 (1600 * 1200 * 2)>; >> }; >> }; >> >> lcd0: lcd-controller@820000 { >> compatible = "marvell,dove-lcd"; >> reg = <0x820000 0x1000>; >> interrupts = <47>; >> clocks = <&si5351 0>; >> clock-names = "ext_ref_clk_1"; >> framebuffer = <&display_reserved>; >> }; >> >> chosen { >> boot-framebuffer { >> compatible = "simple-framebuffer"; >> device = <&lcd0>; >> width = <1600>; >> height = <1200>; >> stride = <(1600 * 2)>; >> format = "r5g6b5"; >> }; >> } It is questionable whether there should be a compatible string in the chosen section. That compatible string is loading in a parser that understands the section, not a device driver. The simplefb driver could key off from 'boot-framebuffer' instead of the compatible string. > > Everyone sees that the kernel cannot magically figure out which memory > is used by the simplefb. Yet people expect the kernel to somehow > magically figure out which clocks are used, to avoid accidentally turning > them off. > > This is not consistent, either the kernel needs to be told these things, > and then it needs to be told all of them, or hardware node properties > like a reg property do not belong in an informational node, and then the > reg property should not be in the simplefb node either, and the kernel > should somehow magically figure out where the memory lives, just like > some people are expecting the kernel to magically figure out which > clocks are used. > > Regards, > > Hans
Quoting jonsmirl@gmail.com (2014-10-03 14:50:24) > On Fri, Oct 3, 2014 at 5:26 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > On Fri, Oct 3, 2014 at 10:55 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > >> On Thu, Oct 2, 2014 at 9:46 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > >>> On Thu, Oct 2, 2014 at 3:34 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > >>>> Does the clock and regulator cleanup happen before drivers can load > >>>> off from initrd? I didn't think it did but I might be wrong. > >>> > >>> Yes > >>> > >>> drivers/base/power/domain.c:late_initcall(genpd_poweroff_unused); > >>> drivers/clk/clk.c:late_initcall_sync(clk_disable_unused); > >>> drivers/regulator/core.c:late_initcall_sync(regulator_init_complete); > >> > >> What do you think about putting these calls onto an ioctl somewhere > >> and then eliminating the late_initcall(..)? A tiny user space app > >> could then hit that ioctl after all of the loadable device drivers are > >> loaded. Add the command to make this ioctl call to busybox or udev. > >> After all, it is not fatal if these calls aren't made, all they do is > >> save power. Add a link in rc.d or somewhere similar to run this app at > >> the appropriate time. > >> > >> Switching these over to an ioctl allows a window to be opened for > >> device specific driver loading before the clock/regulator clean up > >> happens. > >> > >> Now all of this mess of protecting clocks and regulator disappears. > >> Instead get the device specific drivers written and loaded, then run > >> the cleanup app which hits the ioctl(). All of the correct > >> clock/regulators will be claimed and then this clean code will do the > >> right thing. > >> > >> From my perspective it appears that this cleanup is being done too > >> early which then triggers a need to protect things from cleanup. > > > > Not doing the cleanup doesn't help. > > > > If someone else calls clk_disable() on a clock which shares a parent > > with the clock you're silently using, that clock will still be disabled. > > This can happen at any time. > > Could we start all of the regulators and clocks off with a reference > count of one, but not do anything to change their state? Then this > ioctl() would decrement that extra reference. Removing the extra > reference would then disable everything that isn't claimed. No. That is too ugly. Regards, Mike > > > > > > Gr{oetje,eeting}s, > > > > Geert > > > > -- > > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > > > In personal conversations with technical people, I call myself a hacker. But > > when I'm talking to journalists I just say "programmer" or something like that. > > -- Linus Torvalds > > > > -- > Jon Smirl > jonsmirl@gmail.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: > On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> Hi, >>>>> >>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>> property to communicate this to the OS. >>>>>>> >>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>> >>>>>>> -- >>>>>>> Changes in v2: >>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>> Changes in v3: >>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>> >>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>> has no side effects on the hardware setup, but you are changing that >>>>>> aspect. You are mixing in a hardware description that is simply >>>>>> inaccurate. >>>>> >>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>> on some systems the virtual device needs clocks to keep running. >>>>> >>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>> fix in DT. >>>>> >>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>> for some reason is o so very very special, because it is not special, it needs >>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>> >>>> No, the DT describes the connections of clocks from h/w block to h/w >>>> block. Their use is implied by the connection. >>>> >>>> And yes, as the other thread mentioned DT is more than just hardware >>>> information. However, what you are adding IS hardware information and >>>> clearly has a place somewhere else. And adding anything which is not >>>> hardware description gets much more scrutiny. >>>> >>>>> More over it is a bit late to start making objections now. This has already been >>>>> discussed to death for weeks now, and every argument against this patch has already >>>>> been countered multiple times (including the one you are making now). Feel free to >>>>> read the entire thread in the archives under the subject: >>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>> >>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>> have made suggestions which I would agree with and you've basically >>>> ignored them. >>>> >>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>> come now, so please lets get this discussion over with and merge this, so that >>>>> we can all move on and spend out time in a more productive manner. >>>> >>>> Not an effective argument to get things merged. >>> >>> If there is not good solution to deferring clock clean up in the >>> kernel, another approach is to change how simple-framebuffer is >>> described in the device tree.... >>> >>> Right now it is a stand-alone item that looks like a device node, but >>> it isn't a device. >>> >>> framebuffer { >>> compatible = "simple-framebuffer"; >>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>> width = <1600>; >>> height = <1200>; >>> stride = <(1600 * 2)>; >>> format = "r5g6b5"; >>> }; >>> >>> How about something like this? >>> >>> reserved-memory { >>> #address-cells = <1>; >>> #size-cells = <1>; >>> ranges; >>> >>> display_reserved: framebuffer@78000000 { >>> reg = <0x78000000 (1600 * 1200 * 2)>; >>> }; >>> }; >>> >>> lcd0: lcd-controller@820000 { >>> compatible = "marvell,dove-lcd"; >>> reg = <0x820000 0x1000>; >>> interrupts = <47>; >>> clocks = <&si5351 0>; >>> clock-names = "ext_ref_clk_1"; >>> }; >>> >>> chosen { >>> boot-framebuffer { >>> compatible = "simple-framebuffer"; >>> device = <&lcd0>; >>> framebuffer = <&display_reserved>; >>> width = <1600>; >>> height = <1200>; >>> stride = <(1600 * 2)>; >>> format = "r5g6b5"; >>> }; >>> } >>> >>> >>> This moves the definition of the boot framebuffer setup into the area >>> where bootloader info is suppose to go. Then you can use the phandle >>> to follow the actual device chains and protect the clocks and >>> regulators. To make that work you are required to provide an accurate >>> description of the real video hardware so that this chain can be >>> followed. >> >> This will not work, first of all multiple blocks may be involved, so >> the device = in the boot-framebuffer would need to be a list. That in >> itself is not a problem, the problem is that the blocks used may have >> multiple clocks, of which the setup mode likely uses only a few. >> >> So if we do things this way, we end up keeping way to many clocks >> enabled. > > That doesn't hurt anything. <snip lots of stuff based on the above> Wrong, that does hurt things. As already discussed simply stopping the clocks from being disabled by the unused_clks mechanism is not enough, since clocks may be shared, and we must stop another device driver sharing the clock and doing clk_enable; probe; clk_disable; disabling the shared clk, which means calling clk_enable on it to mark it as being in use. So this will hurt cause it will cause us to enable a bunch of clks which should not be enabled. I've been thinking more about this, and I understand that people don't want to put hardware description in the simplefb node, but this is not hardware description. u-boot sets up the display-pipeline to scan out of a certain part of memory, in order to do this it writes the memory address to some registers in the display pipeline, so what it is passing is not hardware description (it is not passing all possible memory addresses for the framebuffer), but it is passing information about the state in which it has left the display pipeline, iow it is passing state information. Likewise when u-boot sets up the display-pipeline it choices certain clocks to use, just like it chooses a framebuffer address, and then it programs those clocks into registers. So what we want to pass along here is information about the way in which the display-pipeline has been configured, so we're passing along state information, not hardware description, just like with the framebuffer address. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> Hi, >>> >>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>> Hi, >>>>>> >>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>> property to communicate this to the OS. >>>>>>>> >>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>> >>>>>>>> -- >>>>>>>> Changes in v2: >>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>> Changes in v3: >>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>> >>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>> inaccurate. >>>>>> >>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>> on some systems the virtual device needs clocks to keep running. >>>>>> >>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>> fix in DT. >>>>>> >>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>> >>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>> block. Their use is implied by the connection. >>>>> >>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>> information. However, what you are adding IS hardware information and >>>>> clearly has a place somewhere else. And adding anything which is not >>>>> hardware description gets much more scrutiny. >>>>> >>>>>> More over it is a bit late to start making objections now. This has already been >>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>> read the entire thread in the archives under the subject: >>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>> >>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>> have made suggestions which I would agree with and you've basically >>>>> ignored them. >>>>> >>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>> we can all move on and spend out time in a more productive manner. >>>>> >>>>> Not an effective argument to get things merged. >>>> >>>> If there is not good solution to deferring clock clean up in the >>>> kernel, another approach is to change how simple-framebuffer is >>>> described in the device tree.... >>>> >>>> Right now it is a stand-alone item that looks like a device node, but >>>> it isn't a device. >>>> >>>> framebuffer { >>>> compatible = "simple-framebuffer"; >>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>> width = <1600>; >>>> height = <1200>; >>>> stride = <(1600 * 2)>; >>>> format = "r5g6b5"; >>>> }; >>>> >>>> How about something like this? >>>> >>>> reserved-memory { >>>> #address-cells = <1>; >>>> #size-cells = <1>; >>>> ranges; >>>> >>>> display_reserved: framebuffer@78000000 { >>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>> }; >>>> }; >>>> >>>> lcd0: lcd-controller@820000 { >>>> compatible = "marvell,dove-lcd"; >>>> reg = <0x820000 0x1000>; >>>> interrupts = <47>; >>>> clocks = <&si5351 0>; >>>> clock-names = "ext_ref_clk_1"; >>>> }; >>>> >>>> chosen { >>>> boot-framebuffer { >>>> compatible = "simple-framebuffer"; >>>> device = <&lcd0>; >>>> framebuffer = <&display_reserved>; >>>> width = <1600>; >>>> height = <1200>; >>>> stride = <(1600 * 2)>; >>>> format = "r5g6b5"; >>>> }; >>>> } >>>> >>>> >>>> This moves the definition of the boot framebuffer setup into the area >>>> where bootloader info is suppose to go. Then you can use the phandle >>>> to follow the actual device chains and protect the clocks and >>>> regulators. To make that work you are required to provide an accurate >>>> description of the real video hardware so that this chain can be >>>> followed. >>> >>> This will not work, first of all multiple blocks may be involved, so >>> the device = in the boot-framebuffer would need to be a list. That in >>> itself is not a problem, the problem is that the blocks used may have >>> multiple clocks, of which the setup mode likely uses only a few. >>> >>> So if we do things this way, we end up keeping way to many clocks >>> enabled. >> >> That doesn't hurt anything. > > <snip lots of stuff based on the above> > > Wrong, that does hurt things. As already discussed simply stopping the > clocks from being disabled by the unused_clks mechanism is not enough, > since clocks may be shared, and we must stop another device driver > sharing the clock and doing clk_enable; probe; clk_disable; disabling > the shared clk, which means calling clk_enable on it to mark it as > being in use. So this will hurt cause it will cause us to enable a bunch > of clks which should not be enabled. I said earlier that you would need to add a protected mechanism to clocks to handle this phase. When a clock/regulator is protected you can turn it on but you can't turn it off. When simplefb exits it will clear this protected status. When the protected status gets cleared treat it as a ref count decrement and turn off the clock/regulator if indicated to do so. If a clock is protected, all of it parents get the protected bit set too. Protected mode you can turn clocks on, but not off it is ref counted when it hits zero, look at the normal refcount and set that state Protected mode is not long lived. It only hangs around until the real device driver loads. > > I've been thinking more about this, and I understand that people don't > want to put hardware description in the simplefb node, but this is > not hardware description. > > u-boot sets up the display-pipeline to scan out of a certain part of > memory, in order to do this it writes the memory address to some registers > in the display pipeline, so what it is passing is not hardware description > (it is not passing all possible memory addresses for the framebuffer), but > it is passing information about the state in which it has left the display > pipeline, iow it is passing state information. Giving the buffer to a piece of hardware is more than setting state. The hardware now owns that buffer. That ownership needs to be managed so that if the hardware decides it doesn't want the buffer it can be returned to the global pool. That's why the buffer has to go into that reserved memory section, not the chosen section. An OS doesn't have to process chosen, it is just there for informational purposes. To keep the OS from thinking it owns the memory in that video buffer and can use it for OS purposes, it has to go into that reserved memory section. The clocks are different. We know exactly who owns those clocks, the graphics hardware. You want something like this where the state of the entire video path is encoded into the chosen section. But that info is going to vary all over the place with TV output, HDMI output, LCD panels, etc. simplefb isn't going to be simple any more. Plus the purposes of adding all of this complexity is just to handle the half second transition from boot loader control to real driver. reserved-memory { #address-cells = <1>; #size-cells = <1>; ranges; display_reserved: framebuffer@78000000 { reg = <0x78000000 (1600 * 1200 * 2)>; }; }; lcd0: lcd-controller@820000 { compatible = "marvell,dove-lcd"; reg = <0x820000 0x1000>; interrupts = <47>; framebuffer = <&display_reserved>; clocks = <&si5351 0>; clock-names = "ext_ref_clk_1"; }; chosen { boot-framebuffer { compatible = "simple-framebuffer"; state { device = <&lcd0>; width = <1600>; height = <1200>; stride = <(1600 * 2)>; format = "r5g6b5"; clocks = <&si5351 on 10mhz>; output = "hdmi"; state { device = <&hdmi> clocks = <&xyz on 12mhz>; } }; } > > Likewise when u-boot sets up the display-pipeline it choices certain > clocks to use, just like it chooses a framebuffer address, and then > it programs those clocks into registers. So what we want to pass along > here is information about the way in which the display-pipeline has > been configured, so we're passing along state information, > not hardware description, just like with the framebuffer address. > > Regards, > > Hans > > >
Hi, On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote: > On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi, >>>> >>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>>> property to communicate this to the OS. >>>>>>>>> >>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Changes in v2: >>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>> Changes in v3: >>>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>>> >>>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>>> inaccurate. >>>>>>> >>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>>> on some systems the virtual device needs clocks to keep running. >>>>>>> >>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>>> fix in DT. >>>>>>> >>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>>> >>>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>>> block. Their use is implied by the connection. >>>>>> >>>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>>> information. However, what you are adding IS hardware information and >>>>>> clearly has a place somewhere else. And adding anything which is not >>>>>> hardware description gets much more scrutiny. >>>>>> >>>>>>> More over it is a bit late to start making objections now. This has already been >>>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>>> read the entire thread in the archives under the subject: >>>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>>> >>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>>> have made suggestions which I would agree with and you've basically >>>>>> ignored them. >>>>>> >>>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>>> we can all move on and spend out time in a more productive manner. >>>>>> >>>>>> Not an effective argument to get things merged. >>>>> >>>>> If there is not good solution to deferring clock clean up in the >>>>> kernel, another approach is to change how simple-framebuffer is >>>>> described in the device tree.... >>>>> >>>>> Right now it is a stand-alone item that looks like a device node, but >>>>> it isn't a device. >>>>> >>>>> framebuffer { >>>>> compatible = "simple-framebuffer"; >>>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>>> width = <1600>; >>>>> height = <1200>; >>>>> stride = <(1600 * 2)>; >>>>> format = "r5g6b5"; >>>>> }; >>>>> >>>>> How about something like this? >>>>> >>>>> reserved-memory { >>>>> #address-cells = <1>; >>>>> #size-cells = <1>; >>>>> ranges; >>>>> >>>>> display_reserved: framebuffer@78000000 { >>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>> }; >>>>> }; >>>>> >>>>> lcd0: lcd-controller@820000 { >>>>> compatible = "marvell,dove-lcd"; >>>>> reg = <0x820000 0x1000>; >>>>> interrupts = <47>; >>>>> clocks = <&si5351 0>; >>>>> clock-names = "ext_ref_clk_1"; >>>>> }; >>>>> >>>>> chosen { >>>>> boot-framebuffer { >>>>> compatible = "simple-framebuffer"; >>>>> device = <&lcd0>; >>>>> framebuffer = <&display_reserved>; >>>>> width = <1600>; >>>>> height = <1200>; >>>>> stride = <(1600 * 2)>; >>>>> format = "r5g6b5"; >>>>> }; >>>>> } >>>>> >>>>> >>>>> This moves the definition of the boot framebuffer setup into the area >>>>> where bootloader info is suppose to go. Then you can use the phandle >>>>> to follow the actual device chains and protect the clocks and >>>>> regulators. To make that work you are required to provide an accurate >>>>> description of the real video hardware so that this chain can be >>>>> followed. >>>> >>>> This will not work, first of all multiple blocks may be involved, so >>>> the device = in the boot-framebuffer would need to be a list. That in >>>> itself is not a problem, the problem is that the blocks used may have >>>> multiple clocks, of which the setup mode likely uses only a few. >>>> >>>> So if we do things this way, we end up keeping way to many clocks >>>> enabled. >>> >>> That doesn't hurt anything. >> >> <snip lots of stuff based on the above> >> >> Wrong, that does hurt things. As already discussed simply stopping the >> clocks from being disabled by the unused_clks mechanism is not enough, >> since clocks may be shared, and we must stop another device driver >> sharing the clock and doing clk_enable; probe; clk_disable; disabling >> the shared clk, which means calling clk_enable on it to mark it as >> being in use. So this will hurt cause it will cause us to enable a bunch >> of clks which should not be enabled. > > I said earlier that you would need to add a protected mechanism to > clocks to handle this phase. When a clock/regulator is protected you > can turn it on but you can't turn it off. When simplefb exits it will > clear this protected status. When the protected status gets cleared > treat it as a ref count decrement and turn off the clock/regulator if > indicated to do so. If a clock is protected, all of it parents get the > protected bit set too. > > Protected mode > you can turn clocks on, but not off > it is ref counted > when it hits zero, look at the normal refcount and set that state > > Protected mode is not long lived. It only hangs around until the real > device driver loads. And that has already been nacked by the clk maintainer because it is too complicated, and I agree this is tons more complicated then simply adding the list of clocks to the simplefb node. >> I've been thinking more about this, and I understand that people don't >> want to put hardware description in the simplefb node, but this is >> not hardware description. >> >> u-boot sets up the display-pipeline to scan out of a certain part of >> memory, in order to do this it writes the memory address to some registers >> in the display pipeline, so what it is passing is not hardware description >> (it is not passing all possible memory addresses for the framebuffer), but >> it is passing information about the state in which it has left the display >> pipeline, iow it is passing state information. > > Giving the buffer to a piece of hardware is more than setting state. > The hardware now owns that buffer. That ownership needs to be managed > so that if the hardware decides it doesn't want the buffer it can be > returned to the global pool. > > That's why the buffer has to go into that reserved memory section, not > the chosen section. But is not in the reserved memory section, it is in the simplefb section, and we're stuck with this because of backward compatibility. An OS doesn't have to process chosen, it is just > there for informational purposes. To keep the OS from thinking it owns > the memory in that video buffer and can use it for OS purposes, it has > to go into that reserved memory section. > > The clocks are different. We know exactly who owns those clocks, the > graphics hardware. > > You want something like this where the state of the entire video path > is encoded into the chosen section. But that info is going to vary all > over the place with TV output, HDMI output, LCD panels, etc. simplefb > isn't going to be simple any more. Plus the purposes of adding all of > this complexity is just to handle the half second transition from boot > loader control to real driver. > > reserved-memory { > #address-cells = <1>; > #size-cells = <1>; > ranges; > > display_reserved: framebuffer@78000000 { > reg = <0x78000000 (1600 * 1200 * 2)>; > }; > }; > > lcd0: lcd-controller@820000 { > compatible = "marvell,dove-lcd"; > reg = <0x820000 0x1000>; > interrupts = <47>; > framebuffer = <&display_reserved>; > clocks = <&si5351 0>; > clock-names = "ext_ref_clk_1"; > }; > > chosen { > boot-framebuffer { > compatible = "simple-framebuffer"; > state { > device = <&lcd0>; > width = <1600>; > height = <1200>; > stride = <(1600 * 2)>; > format = "r5g6b5"; > clocks = <&si5351 on 10mhz>; Right, so here we get a list of clocks which are actually in use by the simplefb, just as I've been advocating all the time already. Note that the clock speed is not necessary, all the kernel needs to know is that it must not change it. So as you seem to agree that we need to pass some sort of clock state info, then all we need to agree on now is where to put it, as said adding the reg property to a reserved-memory node is out of the question because of backward compat, likewise storing width, height and format in a state sub-node are out of the question for the same reason. But other then that I'm fine with putting the simplefb node under chosen and putting clocks in there (without the state subnode) as you suggest above. So we seem to be in agreement here :) > output = "hdmi"; > state { > device = <&hdmi> > clocks = <&xyz on 12mhz>; > } That level of detail won't be necessary, simplefb is supposed to be simple, the kernel does not need to know which outputs there are, there will always be only one for simplefb. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote: >> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> Hi, >>> >>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> Hi, >>>>> >>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>>>> property to communicate this to the OS. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> Changes in v2: >>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>> Changes in v3: >>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>>>> >>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>>>> inaccurate. >>>>>>>> >>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>>>> on some systems the virtual device needs clocks to keep running. >>>>>>>> >>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>>>> fix in DT. >>>>>>>> >>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>>>> >>>>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>>>> block. Their use is implied by the connection. >>>>>>> >>>>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>>>> information. However, what you are adding IS hardware information and >>>>>>> clearly has a place somewhere else. And adding anything which is not >>>>>>> hardware description gets much more scrutiny. >>>>>>> >>>>>>>> More over it is a bit late to start making objections now. This has already been >>>>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>>>> read the entire thread in the archives under the subject: >>>>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>>>> >>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>>>> have made suggestions which I would agree with and you've basically >>>>>>> ignored them. >>>>>>> >>>>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>>>> we can all move on and spend out time in a more productive manner. >>>>>>> >>>>>>> Not an effective argument to get things merged. >>>>>> >>>>>> If there is not good solution to deferring clock clean up in the >>>>>> kernel, another approach is to change how simple-framebuffer is >>>>>> described in the device tree.... >>>>>> >>>>>> Right now it is a stand-alone item that looks like a device node, but >>>>>> it isn't a device. >>>>>> >>>>>> framebuffer { >>>>>> compatible = "simple-framebuffer"; >>>>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>>>> width = <1600>; >>>>>> height = <1200>; >>>>>> stride = <(1600 * 2)>; >>>>>> format = "r5g6b5"; >>>>>> }; >>>>>> >>>>>> How about something like this? >>>>>> >>>>>> reserved-memory { >>>>>> #address-cells = <1>; >>>>>> #size-cells = <1>; >>>>>> ranges; >>>>>> >>>>>> display_reserved: framebuffer@78000000 { >>>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>>> }; >>>>>> }; >>>>>> >>>>>> lcd0: lcd-controller@820000 { >>>>>> compatible = "marvell,dove-lcd"; >>>>>> reg = <0x820000 0x1000>; >>>>>> interrupts = <47>; >>>>>> clocks = <&si5351 0>; >>>>>> clock-names = "ext_ref_clk_1"; >>>>>> }; >>>>>> >>>>>> chosen { >>>>>> boot-framebuffer { >>>>>> compatible = "simple-framebuffer"; >>>>>> device = <&lcd0>; >>>>>> framebuffer = <&display_reserved>; >>>>>> width = <1600>; >>>>>> height = <1200>; >>>>>> stride = <(1600 * 2)>; >>>>>> format = "r5g6b5"; >>>>>> }; >>>>>> } >>>>>> >>>>>> >>>>>> This moves the definition of the boot framebuffer setup into the area >>>>>> where bootloader info is suppose to go. Then you can use the phandle >>>>>> to follow the actual device chains and protect the clocks and >>>>>> regulators. To make that work you are required to provide an accurate >>>>>> description of the real video hardware so that this chain can be >>>>>> followed. >>>>> >>>>> This will not work, first of all multiple blocks may be involved, so >>>>> the device = in the boot-framebuffer would need to be a list. That in >>>>> itself is not a problem, the problem is that the blocks used may have >>>>> multiple clocks, of which the setup mode likely uses only a few. >>>>> >>>>> So if we do things this way, we end up keeping way to many clocks >>>>> enabled. >>>> >>>> That doesn't hurt anything. >>> >>> <snip lots of stuff based on the above> >>> >>> Wrong, that does hurt things. As already discussed simply stopping the >>> clocks from being disabled by the unused_clks mechanism is not enough, >>> since clocks may be shared, and we must stop another device driver >>> sharing the clock and doing clk_enable; probe; clk_disable; disabling >>> the shared clk, which means calling clk_enable on it to mark it as >>> being in use. So this will hurt cause it will cause us to enable a bunch >>> of clks which should not be enabled. >> >> I said earlier that you would need to add a protected mechanism to >> clocks to handle this phase. When a clock/regulator is protected you >> can turn it on but you can't turn it off. When simplefb exits it will >> clear this protected status. When the protected status gets cleared >> treat it as a ref count decrement and turn off the clock/regulator if >> indicated to do so. If a clock is protected, all of it parents get the >> protected bit set too. >> >> Protected mode >> you can turn clocks on, but not off >> it is ref counted >> when it hits zero, look at the normal refcount and set that state >> >> Protected mode is not long lived. It only hangs around until the real >> device driver loads. > > And that has already been nacked by the clk maintainer because it is > too complicated, and I agree this is tons more complicated then simply > adding the list of clocks to the simplefb node. > >>> I've been thinking more about this, and I understand that people don't >>> want to put hardware description in the simplefb node, but this is >>> not hardware description. >>> >>> u-boot sets up the display-pipeline to scan out of a certain part of >>> memory, in order to do this it writes the memory address to some registers >>> in the display pipeline, so what it is passing is not hardware description >>> (it is not passing all possible memory addresses for the framebuffer), but >>> it is passing information about the state in which it has left the display >>> pipeline, iow it is passing state information. >> >> Giving the buffer to a piece of hardware is more than setting state. >> The hardware now owns that buffer. That ownership needs to be managed >> so that if the hardware decides it doesn't want the buffer it can be >> returned to the global pool. >> >> That's why the buffer has to go into that reserved memory section, not >> the chosen section. > > But is not in the reserved memory section, it is in the simplefb > section, and we're stuck with this because of backward compatibility. > > An OS doesn't have to process chosen, it is just >> there for informational purposes. To keep the OS from thinking it owns >> the memory in that video buffer and can use it for OS purposes, it has >> to go into that reserved memory section. >> >> The clocks are different. We know exactly who owns those clocks, the >> graphics hardware. >> >> You want something like this where the state of the entire video path >> is encoded into the chosen section. But that info is going to vary all >> over the place with TV output, HDMI output, LCD panels, etc. simplefb >> isn't going to be simple any more. Plus the purposes of adding all of >> this complexity is just to handle the half second transition from boot >> loader control to real driver. >> >> reserved-memory { >> #address-cells = <1>; >> #size-cells = <1>; >> ranges; >> >> display_reserved: framebuffer@78000000 { >> reg = <0x78000000 (1600 * 1200 * 2)>; >> }; >> }; >> >> lcd0: lcd-controller@820000 { >> compatible = "marvell,dove-lcd"; >> reg = <0x820000 0x1000>; >> interrupts = <47>; >> framebuffer = <&display_reserved>; >> clocks = <&si5351 0>; >> clock-names = "ext_ref_clk_1"; >> }; >> >> chosen { >> boot-framebuffer { >> compatible = "simple-framebuffer"; >> state { >> device = <&lcd0>; >> width = <1600>; >> height = <1200>; >> stride = <(1600 * 2)>; >> format = "r5g6b5"; >> clocks = <&si5351 on 10mhz>; > > Right, so here we get a list of clocks which are actually in use > by the simplefb, just as I've been advocating all the time already. > > Note that the clock speed is not necessary, all the kernel needs to > know is that it must not change it. > > So as you seem to agree that we need to pass some sort of clock state > info, then all we need to agree on now is where to put it, as said adding > the reg property to a reserved-memory node is out of the question because > of backward compat, likewise storing width, height and format in a state > sub-node are out of the question for the same reason. But other then that > I'm fine with putting the simplefb node under chosen and putting clocks > in there (without the state subnode) as you suggest above. > > So we seem to be in agreement here :) > >> output = "hdmi"; >> state { >> device = <&hdmi> >> clocks = <&xyz on 12mhz>; >> } > > That level of detail won't be necessary, simplefb is supposed to be > simple, the kernel does not need to know which outputs there are, there > will always be only one for simplefb. I don't agree, but you are going to do it any way so I'll try and help tkeep problem side effects I know of to a minimum. You are relying on the BIOS to provide detailed, accurate information. Relying on BIOSes to do that has historically been a very bad idea. If you go the way of putting this info into the chosen section you are going to have to mark the clocks/regulators in use for all of the outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be useful if the backlight turns off because simplefb hasn't grabbed it. This is the only real difference between the proposals - you want uboot to enumerate what needs to be protected. I don't trust the BIOS to do that reliably so I'd preferred to just protect everything in the device hardware chain until the device specific drivers load. ------------------------------------------------------- I also still believe this is a problem up in Linux that we shouldn't be using the device tree to fix. It seems to me that the need for something like a 'protected' mode is a generic problem that could be extended to all hardware. In protected mode things can be turned on but nothing can be turned off. Only after the kernel has all of the necessary drivers loaded would a small app run that hits an IOCTL and says, ok protected mode is over now clean up these things wasting power. Maybe it should be renamed 'boot' mode. To implement this the various 'turn off' functions would get a 'boot' mode flag. In boot mode they track the ref counts but don't turn things off when the ref count hits zero. Then that ioctl() that the user space app calls runs the chains of all of the clocks/regulators/etc and if the ref count is zero turns them off again and clears 'boot' mode. Doesn't matter if you turn off something again that is already off. > > Regards, > > Hans
Hi, On 10/05/2014 05:07 PM, jonsmirl@gmail.com wrote: > On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote: >>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi, >>>> >>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>> Hi, >>>>>> >>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>>>>> property to communicate this to the OS. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Changes in v2: >>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>> Changes in v3: >>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>>>>> >>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>>>>> inaccurate. >>>>>>>>> >>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>>>>> on some systems the virtual device needs clocks to keep running. >>>>>>>>> >>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>>>>> fix in DT. >>>>>>>>> >>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>>>>> >>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>>>>> block. Their use is implied by the connection. >>>>>>>> >>>>>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>>>>> information. However, what you are adding IS hardware information and >>>>>>>> clearly has a place somewhere else. And adding anything which is not >>>>>>>> hardware description gets much more scrutiny. >>>>>>>> >>>>>>>>> More over it is a bit late to start making objections now. This has already been >>>>>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>>>>> read the entire thread in the archives under the subject: >>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>>>>> >>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>>>>> have made suggestions which I would agree with and you've basically >>>>>>>> ignored them. >>>>>>>> >>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>>>>> we can all move on and spend out time in a more productive manner. >>>>>>>> >>>>>>>> Not an effective argument to get things merged. >>>>>>> >>>>>>> If there is not good solution to deferring clock clean up in the >>>>>>> kernel, another approach is to change how simple-framebuffer is >>>>>>> described in the device tree.... >>>>>>> >>>>>>> Right now it is a stand-alone item that looks like a device node, but >>>>>>> it isn't a device. >>>>>>> >>>>>>> framebuffer { >>>>>>> compatible = "simple-framebuffer"; >>>>>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>>>>> width = <1600>; >>>>>>> height = <1200>; >>>>>>> stride = <(1600 * 2)>; >>>>>>> format = "r5g6b5"; >>>>>>> }; >>>>>>> >>>>>>> How about something like this? >>>>>>> >>>>>>> reserved-memory { >>>>>>> #address-cells = <1>; >>>>>>> #size-cells = <1>; >>>>>>> ranges; >>>>>>> >>>>>>> display_reserved: framebuffer@78000000 { >>>>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>>>> }; >>>>>>> }; >>>>>>> >>>>>>> lcd0: lcd-controller@820000 { >>>>>>> compatible = "marvell,dove-lcd"; >>>>>>> reg = <0x820000 0x1000>; >>>>>>> interrupts = <47>; >>>>>>> clocks = <&si5351 0>; >>>>>>> clock-names = "ext_ref_clk_1"; >>>>>>> }; >>>>>>> >>>>>>> chosen { >>>>>>> boot-framebuffer { >>>>>>> compatible = "simple-framebuffer"; >>>>>>> device = <&lcd0>; >>>>>>> framebuffer = <&display_reserved>; >>>>>>> width = <1600>; >>>>>>> height = <1200>; >>>>>>> stride = <(1600 * 2)>; >>>>>>> format = "r5g6b5"; >>>>>>> }; >>>>>>> } >>>>>>> >>>>>>> >>>>>>> This moves the definition of the boot framebuffer setup into the area >>>>>>> where bootloader info is suppose to go. Then you can use the phandle >>>>>>> to follow the actual device chains and protect the clocks and >>>>>>> regulators. To make that work you are required to provide an accurate >>>>>>> description of the real video hardware so that this chain can be >>>>>>> followed. >>>>>> >>>>>> This will not work, first of all multiple blocks may be involved, so >>>>>> the device = in the boot-framebuffer would need to be a list. That in >>>>>> itself is not a problem, the problem is that the blocks used may have >>>>>> multiple clocks, of which the setup mode likely uses only a few. >>>>>> >>>>>> So if we do things this way, we end up keeping way to many clocks >>>>>> enabled. >>>>> >>>>> That doesn't hurt anything. >>>> >>>> <snip lots of stuff based on the above> >>>> >>>> Wrong, that does hurt things. As already discussed simply stopping the >>>> clocks from being disabled by the unused_clks mechanism is not enough, >>>> since clocks may be shared, and we must stop another device driver >>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling >>>> the shared clk, which means calling clk_enable on it to mark it as >>>> being in use. So this will hurt cause it will cause us to enable a bunch >>>> of clks which should not be enabled. >>> >>> I said earlier that you would need to add a protected mechanism to >>> clocks to handle this phase. When a clock/regulator is protected you >>> can turn it on but you can't turn it off. When simplefb exits it will >>> clear this protected status. When the protected status gets cleared >>> treat it as a ref count decrement and turn off the clock/regulator if >>> indicated to do so. If a clock is protected, all of it parents get the >>> protected bit set too. >>> >>> Protected mode >>> you can turn clocks on, but not off >>> it is ref counted >>> when it hits zero, look at the normal refcount and set that state >>> >>> Protected mode is not long lived. It only hangs around until the real >>> device driver loads. >> >> And that has already been nacked by the clk maintainer because it is >> too complicated, and I agree this is tons more complicated then simply >> adding the list of clocks to the simplefb node. >> >>>> I've been thinking more about this, and I understand that people don't >>>> want to put hardware description in the simplefb node, but this is >>>> not hardware description. >>>> >>>> u-boot sets up the display-pipeline to scan out of a certain part of >>>> memory, in order to do this it writes the memory address to some registers >>>> in the display pipeline, so what it is passing is not hardware description >>>> (it is not passing all possible memory addresses for the framebuffer), but >>>> it is passing information about the state in which it has left the display >>>> pipeline, iow it is passing state information. >>> >>> Giving the buffer to a piece of hardware is more than setting state. >>> The hardware now owns that buffer. That ownership needs to be managed >>> so that if the hardware decides it doesn't want the buffer it can be >>> returned to the global pool. >>> >>> That's why the buffer has to go into that reserved memory section, not >>> the chosen section. >> >> But is not in the reserved memory section, it is in the simplefb >> section, and we're stuck with this because of backward compatibility. >> >> An OS doesn't have to process chosen, it is just >>> there for informational purposes. To keep the OS from thinking it owns >>> the memory in that video buffer and can use it for OS purposes, it has >>> to go into that reserved memory section. >>> >>> The clocks are different. We know exactly who owns those clocks, the >>> graphics hardware. >>> >>> You want something like this where the state of the entire video path >>> is encoded into the chosen section. But that info is going to vary all >>> over the place with TV output, HDMI output, LCD panels, etc. simplefb >>> isn't going to be simple any more. Plus the purposes of adding all of >>> this complexity is just to handle the half second transition from boot >>> loader control to real driver. >>> >>> reserved-memory { >>> #address-cells = <1>; >>> #size-cells = <1>; >>> ranges; >>> >>> display_reserved: framebuffer@78000000 { >>> reg = <0x78000000 (1600 * 1200 * 2)>; >>> }; >>> }; >>> >>> lcd0: lcd-controller@820000 { >>> compatible = "marvell,dove-lcd"; >>> reg = <0x820000 0x1000>; >>> interrupts = <47>; >>> framebuffer = <&display_reserved>; >>> clocks = <&si5351 0>; >>> clock-names = "ext_ref_clk_1"; >>> }; >>> >>> chosen { >>> boot-framebuffer { >>> compatible = "simple-framebuffer"; >>> state { >>> device = <&lcd0>; >>> width = <1600>; >>> height = <1200>; >>> stride = <(1600 * 2)>; >>> format = "r5g6b5"; >>> clocks = <&si5351 on 10mhz>; >> >> Right, so here we get a list of clocks which are actually in use >> by the simplefb, just as I've been advocating all the time already. >> >> Note that the clock speed is not necessary, all the kernel needs to >> know is that it must not change it. >> >> So as you seem to agree that we need to pass some sort of clock state >> info, then all we need to agree on now is where to put it, as said adding >> the reg property to a reserved-memory node is out of the question because >> of backward compat, likewise storing width, height and format in a state >> sub-node are out of the question for the same reason. But other then that >> I'm fine with putting the simplefb node under chosen and putting clocks >> in there (without the state subnode) as you suggest above. >> >> So we seem to be in agreement here :) >> >>> output = "hdmi"; >>> state { >>> device = <&hdmi> >>> clocks = <&xyz on 12mhz>; >>> } >> >> That level of detail won't be necessary, simplefb is supposed to be >> simple, the kernel does not need to know which outputs there are, there >> will always be only one for simplefb. > > I don't agree, but you are going to do it any way so I'll try and help > tkeep problem side effects I know of to a minimum. You are relying on > the BIOS to provide detailed, accurate information. Relying on BIOSes > to do that has historically been a very bad idea. And here we come to the gist of your objections, I don't agree for various reasons: 1) Despite firmware sometimes having bugs, we simply have to trust the firmware, e.g. the reg property of simplefb could be set to point to some mmio rather then just the framebuffer, and writing that mmio could do real bad things, yet we trust it not to do that. 2) The firmware we're talking about here, at least for now, is u-boot which is fully FOSS and under our control. 3) "I don't trust foo" is not a technical argument, and decisions like this should be made on technical arguments. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 5, 2014 at 11:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote: >>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi, >>>> >>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>> Hi, >>>>>> >>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>>>>> property to communicate this to the OS. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Changes in v2: >>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>> Changes in v3: >>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>>>>> >>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>>>>> inaccurate. >>>>>>>>> >>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>>>>> on some systems the virtual device needs clocks to keep running. >>>>>>>>> >>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>>>>> fix in DT. >>>>>>>>> >>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>>>>> >>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>>>>> block. Their use is implied by the connection. >>>>>>>> >>>>>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>>>>> information. However, what you are adding IS hardware information and >>>>>>>> clearly has a place somewhere else. And adding anything which is not >>>>>>>> hardware description gets much more scrutiny. >>>>>>>> >>>>>>>>> More over it is a bit late to start making objections now. This has already been >>>>>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>>>>> read the entire thread in the archives under the subject: >>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>>>>> >>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>>>>> have made suggestions which I would agree with and you've basically >>>>>>>> ignored them. >>>>>>>> >>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>>>>> we can all move on and spend out time in a more productive manner. >>>>>>>> >>>>>>>> Not an effective argument to get things merged. >>>>>>> >>>>>>> If there is not good solution to deferring clock clean up in the >>>>>>> kernel, another approach is to change how simple-framebuffer is >>>>>>> described in the device tree.... >>>>>>> >>>>>>> Right now it is a stand-alone item that looks like a device node, but >>>>>>> it isn't a device. >>>>>>> >>>>>>> framebuffer { >>>>>>> compatible = "simple-framebuffer"; >>>>>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>>>>> width = <1600>; >>>>>>> height = <1200>; >>>>>>> stride = <(1600 * 2)>; >>>>>>> format = "r5g6b5"; >>>>>>> }; >>>>>>> >>>>>>> How about something like this? >>>>>>> >>>>>>> reserved-memory { >>>>>>> #address-cells = <1>; >>>>>>> #size-cells = <1>; >>>>>>> ranges; >>>>>>> >>>>>>> display_reserved: framebuffer@78000000 { >>>>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>>>> }; >>>>>>> }; >>>>>>> >>>>>>> lcd0: lcd-controller@820000 { >>>>>>> compatible = "marvell,dove-lcd"; >>>>>>> reg = <0x820000 0x1000>; >>>>>>> interrupts = <47>; >>>>>>> clocks = <&si5351 0>; >>>>>>> clock-names = "ext_ref_clk_1"; >>>>>>> }; >>>>>>> >>>>>>> chosen { >>>>>>> boot-framebuffer { >>>>>>> compatible = "simple-framebuffer"; >>>>>>> device = <&lcd0>; >>>>>>> framebuffer = <&display_reserved>; >>>>>>> width = <1600>; >>>>>>> height = <1200>; >>>>>>> stride = <(1600 * 2)>; >>>>>>> format = "r5g6b5"; >>>>>>> }; >>>>>>> } >>>>>>> >>>>>>> >>>>>>> This moves the definition of the boot framebuffer setup into the area >>>>>>> where bootloader info is suppose to go. Then you can use the phandle >>>>>>> to follow the actual device chains and protect the clocks and >>>>>>> regulators. To make that work you are required to provide an accurate >>>>>>> description of the real video hardware so that this chain can be >>>>>>> followed. >>>>>> >>>>>> This will not work, first of all multiple blocks may be involved, so >>>>>> the device = in the boot-framebuffer would need to be a list. That in >>>>>> itself is not a problem, the problem is that the blocks used may have >>>>>> multiple clocks, of which the setup mode likely uses only a few. >>>>>> >>>>>> So if we do things this way, we end up keeping way to many clocks >>>>>> enabled. >>>>> >>>>> That doesn't hurt anything. >>>> >>>> <snip lots of stuff based on the above> >>>> >>>> Wrong, that does hurt things. As already discussed simply stopping the >>>> clocks from being disabled by the unused_clks mechanism is not enough, >>>> since clocks may be shared, and we must stop another device driver >>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling >>>> the shared clk, which means calling clk_enable on it to mark it as >>>> being in use. So this will hurt cause it will cause us to enable a bunch >>>> of clks which should not be enabled. >>> >>> I said earlier that you would need to add a protected mechanism to >>> clocks to handle this phase. When a clock/regulator is protected you >>> can turn it on but you can't turn it off. When simplefb exits it will >>> clear this protected status. When the protected status gets cleared >>> treat it as a ref count decrement and turn off the clock/regulator if >>> indicated to do so. If a clock is protected, all of it parents get the >>> protected bit set too. >>> >>> Protected mode >>> you can turn clocks on, but not off >>> it is ref counted >>> when it hits zero, look at the normal refcount and set that state >>> >>> Protected mode is not long lived. It only hangs around until the real >>> device driver loads. >> >> And that has already been nacked by the clk maintainer because it is >> too complicated, and I agree this is tons more complicated then simply >> adding the list of clocks to the simplefb node. >> >>>> I've been thinking more about this, and I understand that people don't >>>> want to put hardware description in the simplefb node, but this is >>>> not hardware description. >>>> >>>> u-boot sets up the display-pipeline to scan out of a certain part of >>>> memory, in order to do this it writes the memory address to some registers >>>> in the display pipeline, so what it is passing is not hardware description >>>> (it is not passing all possible memory addresses for the framebuffer), but >>>> it is passing information about the state in which it has left the display >>>> pipeline, iow it is passing state information. >>> >>> Giving the buffer to a piece of hardware is more than setting state. >>> The hardware now owns that buffer. That ownership needs to be managed >>> so that if the hardware decides it doesn't want the buffer it can be >>> returned to the global pool. >>> >>> That's why the buffer has to go into that reserved memory section, not >>> the chosen section. >> >> But is not in the reserved memory section, it is in the simplefb >> section, and we're stuck with this because of backward compatibility. >> >> An OS doesn't have to process chosen, it is just >>> there for informational purposes. To keep the OS from thinking it owns >>> the memory in that video buffer and can use it for OS purposes, it has >>> to go into that reserved memory section. >>> >>> The clocks are different. We know exactly who owns those clocks, the >>> graphics hardware. >>> >>> You want something like this where the state of the entire video path >>> is encoded into the chosen section. But that info is going to vary all >>> over the place with TV output, HDMI output, LCD panels, etc. simplefb >>> isn't going to be simple any more. Plus the purposes of adding all of >>> this complexity is just to handle the half second transition from boot >>> loader control to real driver. >>> >>> reserved-memory { >>> #address-cells = <1>; >>> #size-cells = <1>; >>> ranges; >>> >>> display_reserved: framebuffer@78000000 { >>> reg = <0x78000000 (1600 * 1200 * 2)>; >>> }; >>> }; >>> >>> lcd0: lcd-controller@820000 { >>> compatible = "marvell,dove-lcd"; >>> reg = <0x820000 0x1000>; >>> interrupts = <47>; >>> framebuffer = <&display_reserved>; >>> clocks = <&si5351 0>; >>> clock-names = "ext_ref_clk_1"; >>> }; >>> >>> chosen { >>> boot-framebuffer { >>> compatible = "simple-framebuffer"; >>> state { >>> device = <&lcd0>; >>> width = <1600>; >>> height = <1200>; >>> stride = <(1600 * 2)>; >>> format = "r5g6b5"; >>> clocks = <&si5351 on 10mhz>; >> >> Right, so here we get a list of clocks which are actually in use >> by the simplefb, just as I've been advocating all the time already. >> >> Note that the clock speed is not necessary, all the kernel needs to >> know is that it must not change it. >> >> So as you seem to agree that we need to pass some sort of clock state >> info, then all we need to agree on now is where to put it, as said adding >> the reg property to a reserved-memory node is out of the question because >> of backward compat, likewise storing width, height and format in a state >> sub-node are out of the question for the same reason. But other then that >> I'm fine with putting the simplefb node under chosen and putting clocks >> in there (without the state subnode) as you suggest above. >> >> So we seem to be in agreement here :) >> >>> output = "hdmi"; >>> state { >>> device = <&hdmi> >>> clocks = <&xyz on 12mhz>; >>> } >> >> That level of detail won't be necessary, simplefb is supposed to be >> simple, the kernel does not need to know which outputs there are, there >> will always be only one for simplefb. > > I don't agree, but you are going to do it any way so I'll try and help > tkeep problem side effects I know of to a minimum. You are relying on > the BIOS to provide detailed, accurate information. Relying on BIOSes > to do that has historically been a very bad idea. > > If you go the way of putting this info into the chosen section you are > going to have to mark the clocks/regulators in use for all of the > outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be > useful if the backlight turns off because simplefb hasn't grabbed it. > > This is the only real difference between the proposals - you want > uboot to enumerate what needs to be protected. I don't trust the BIOS > to do that reliably so I'd preferred to just protect everything in the > device hardware chain until the device specific drivers load. > > ------------------------------------------------------- > > I also still believe this is a problem up in Linux that we shouldn't > be using the device tree to fix. > > It seems to me that the need for something like a 'protected' mode is > a generic problem that could be extended to all hardware. In protected > mode things can be turned on but nothing can be turned off. Only > after the kernel has all of the necessary drivers loaded would a small > app run that hits an IOCTL and says, ok protected mode is over now > clean up these things wasting power. What happens if some clock needs to be disabled? Like clocks that must be gated before setting a new clock rate or reparenting. The kernel supports these, and it wouldn't surprise me if some driver actually requires this. You might end up blocking that driver or breaking its probe function. And what if reset controls are asserted then de-asserted in the probe function? IIRC there are drivers that do this to reset the underlying hardware. > Maybe it should be renamed 'boot' mode. To implement this the various > 'turn off' functions would get a 'boot' mode flag. In boot mode they > track the ref counts but don't turn things off when the ref count hits > zero. Then that ioctl() that the user space app calls runs the chains > of all of the clocks/regulators/etc and if the ref count is zero turns > them off again and clears 'boot' mode. Doesn't matter if you turn off > something again that is already off. And what if something just happened to be left on that some driver wants to turn off? You are assuming everything not used is off, which is not always the case. ChenYu -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 5, 2014 at 11:16 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 10/05/2014 05:07 PM, jonsmirl@gmail.com wrote: >> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> Hi, >>> >>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote: >>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> Hi, >>>>> >>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>>>>>> property to communicate this to the OS. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Changes in v2: >>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>> Changes in v3: >>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>>>>>> >>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>>>>>> inaccurate. >>>>>>>>>> >>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>>>>>> on some systems the virtual device needs clocks to keep running. >>>>>>>>>> >>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>>>>>> fix in DT. >>>>>>>>>> >>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>>>>>> >>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>>>>>> block. Their use is implied by the connection. >>>>>>>>> >>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>>>>>> information. However, what you are adding IS hardware information and >>>>>>>>> clearly has a place somewhere else. And adding anything which is not >>>>>>>>> hardware description gets much more scrutiny. >>>>>>>>> >>>>>>>>>> More over it is a bit late to start making objections now. This has already been >>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>>>>>> read the entire thread in the archives under the subject: >>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>>>>>> >>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>>>>>> have made suggestions which I would agree with and you've basically >>>>>>>>> ignored them. >>>>>>>>> >>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>>>>>> we can all move on and spend out time in a more productive manner. >>>>>>>>> >>>>>>>>> Not an effective argument to get things merged. >>>>>>>> >>>>>>>> If there is not good solution to deferring clock clean up in the >>>>>>>> kernel, another approach is to change how simple-framebuffer is >>>>>>>> described in the device tree.... >>>>>>>> >>>>>>>> Right now it is a stand-alone item that looks like a device node, but >>>>>>>> it isn't a device. >>>>>>>> >>>>>>>> framebuffer { >>>>>>>> compatible = "simple-framebuffer"; >>>>>>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>>>>>> width = <1600>; >>>>>>>> height = <1200>; >>>>>>>> stride = <(1600 * 2)>; >>>>>>>> format = "r5g6b5"; >>>>>>>> }; >>>>>>>> >>>>>>>> How about something like this? >>>>>>>> >>>>>>>> reserved-memory { >>>>>>>> #address-cells = <1>; >>>>>>>> #size-cells = <1>; >>>>>>>> ranges; >>>>>>>> >>>>>>>> display_reserved: framebuffer@78000000 { >>>>>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>>>>> }; >>>>>>>> }; >>>>>>>> >>>>>>>> lcd0: lcd-controller@820000 { >>>>>>>> compatible = "marvell,dove-lcd"; >>>>>>>> reg = <0x820000 0x1000>; >>>>>>>> interrupts = <47>; >>>>>>>> clocks = <&si5351 0>; >>>>>>>> clock-names = "ext_ref_clk_1"; >>>>>>>> }; >>>>>>>> >>>>>>>> chosen { >>>>>>>> boot-framebuffer { >>>>>>>> compatible = "simple-framebuffer"; >>>>>>>> device = <&lcd0>; >>>>>>>> framebuffer = <&display_reserved>; >>>>>>>> width = <1600>; >>>>>>>> height = <1200>; >>>>>>>> stride = <(1600 * 2)>; >>>>>>>> format = "r5g6b5"; >>>>>>>> }; >>>>>>>> } >>>>>>>> >>>>>>>> >>>>>>>> This moves the definition of the boot framebuffer setup into the area >>>>>>>> where bootloader info is suppose to go. Then you can use the phandle >>>>>>>> to follow the actual device chains and protect the clocks and >>>>>>>> regulators. To make that work you are required to provide an accurate >>>>>>>> description of the real video hardware so that this chain can be >>>>>>>> followed. >>>>>>> >>>>>>> This will not work, first of all multiple blocks may be involved, so >>>>>>> the device = in the boot-framebuffer would need to be a list. That in >>>>>>> itself is not a problem, the problem is that the blocks used may have >>>>>>> multiple clocks, of which the setup mode likely uses only a few. >>>>>>> >>>>>>> So if we do things this way, we end up keeping way to many clocks >>>>>>> enabled. >>>>>> >>>>>> That doesn't hurt anything. >>>>> >>>>> <snip lots of stuff based on the above> >>>>> >>>>> Wrong, that does hurt things. As already discussed simply stopping the >>>>> clocks from being disabled by the unused_clks mechanism is not enough, >>>>> since clocks may be shared, and we must stop another device driver >>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling >>>>> the shared clk, which means calling clk_enable on it to mark it as >>>>> being in use. So this will hurt cause it will cause us to enable a bunch >>>>> of clks which should not be enabled. >>>> >>>> I said earlier that you would need to add a protected mechanism to >>>> clocks to handle this phase. When a clock/regulator is protected you >>>> can turn it on but you can't turn it off. When simplefb exits it will >>>> clear this protected status. When the protected status gets cleared >>>> treat it as a ref count decrement and turn off the clock/regulator if >>>> indicated to do so. If a clock is protected, all of it parents get the >>>> protected bit set too. >>>> >>>> Protected mode >>>> you can turn clocks on, but not off >>>> it is ref counted >>>> when it hits zero, look at the normal refcount and set that state >>>> >>>> Protected mode is not long lived. It only hangs around until the real >>>> device driver loads. >>> >>> And that has already been nacked by the clk maintainer because it is >>> too complicated, and I agree this is tons more complicated then simply >>> adding the list of clocks to the simplefb node. >>> >>>>> I've been thinking more about this, and I understand that people don't >>>>> want to put hardware description in the simplefb node, but this is >>>>> not hardware description. >>>>> >>>>> u-boot sets up the display-pipeline to scan out of a certain part of >>>>> memory, in order to do this it writes the memory address to some registers >>>>> in the display pipeline, so what it is passing is not hardware description >>>>> (it is not passing all possible memory addresses for the framebuffer), but >>>>> it is passing information about the state in which it has left the display >>>>> pipeline, iow it is passing state information. >>>> >>>> Giving the buffer to a piece of hardware is more than setting state. >>>> The hardware now owns that buffer. That ownership needs to be managed >>>> so that if the hardware decides it doesn't want the buffer it can be >>>> returned to the global pool. >>>> >>>> That's why the buffer has to go into that reserved memory section, not >>>> the chosen section. >>> >>> But is not in the reserved memory section, it is in the simplefb >>> section, and we're stuck with this because of backward compatibility. >>> >>> An OS doesn't have to process chosen, it is just >>>> there for informational purposes. To keep the OS from thinking it owns >>>> the memory in that video buffer and can use it for OS purposes, it has >>>> to go into that reserved memory section. >>>> >>>> The clocks are different. We know exactly who owns those clocks, the >>>> graphics hardware. >>>> >>>> You want something like this where the state of the entire video path >>>> is encoded into the chosen section. But that info is going to vary all >>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb >>>> isn't going to be simple any more. Plus the purposes of adding all of >>>> this complexity is just to handle the half second transition from boot >>>> loader control to real driver. >>>> >>>> reserved-memory { >>>> #address-cells = <1>; >>>> #size-cells = <1>; >>>> ranges; >>>> >>>> display_reserved: framebuffer@78000000 { >>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>> }; >>>> }; >>>> >>>> lcd0: lcd-controller@820000 { >>>> compatible = "marvell,dove-lcd"; >>>> reg = <0x820000 0x1000>; >>>> interrupts = <47>; >>>> framebuffer = <&display_reserved>; >>>> clocks = <&si5351 0>; >>>> clock-names = "ext_ref_clk_1"; >>>> }; >>>> >>>> chosen { >>>> boot-framebuffer { >>>> compatible = "simple-framebuffer"; >>>> state { >>>> device = <&lcd0>; >>>> width = <1600>; >>>> height = <1200>; >>>> stride = <(1600 * 2)>; >>>> format = "r5g6b5"; >>>> clocks = <&si5351 on 10mhz>; >>> >>> Right, so here we get a list of clocks which are actually in use >>> by the simplefb, just as I've been advocating all the time already. >>> >>> Note that the clock speed is not necessary, all the kernel needs to >>> know is that it must not change it. >>> >>> So as you seem to agree that we need to pass some sort of clock state >>> info, then all we need to agree on now is where to put it, as said adding >>> the reg property to a reserved-memory node is out of the question because >>> of backward compat, likewise storing width, height and format in a state >>> sub-node are out of the question for the same reason. But other then that >>> I'm fine with putting the simplefb node under chosen and putting clocks >>> in there (without the state subnode) as you suggest above. >>> >>> So we seem to be in agreement here :) >>> >>>> output = "hdmi"; >>>> state { >>>> device = <&hdmi> >>>> clocks = <&xyz on 12mhz>; >>>> } >>> >>> That level of detail won't be necessary, simplefb is supposed to be >>> simple, the kernel does not need to know which outputs there are, there >>> will always be only one for simplefb. >> >> I don't agree, but you are going to do it any way so I'll try and help >> tkeep problem side effects I know of to a minimum. You are relying on >> the BIOS to provide detailed, accurate information. Relying on BIOSes >> to do that has historically been a very bad idea. > > And here we come to the gist of your objections, I don't agree for > various reasons: > > 1) Despite firmware sometimes having bugs, we simply have to trust the firmware, > e.g. the reg property of simplefb could be set to point to some mmio rather then > just the framebuffer, and writing that mmio could do real bad things, yet we trust > it not to do that. > > 2) The firmware we're talking about here, at least for now, is u-boot which > is fully FOSS and under our control. > > 3) "I don't trust foo" is not a technical argument, and decisions like this > should be made on technical arguments. You also realize that uboot now is going to have to be compiled with the same DTS that the kernel is using so that the phandles will match up. And now you are going to have to recompile your uboot everytime that DTS changes. > > Regards, > > Hans
On Sun, Oct 5, 2014 at 11:17 AM, Chen-Yu Tsai <wens@csie.org> wrote: > On Sun, Oct 5, 2014 at 11:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>> Hi, >>> >>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote: >>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> Hi, >>>>> >>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>> Hi, >>>>>>>>>> >>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>>>>>> property to communicate this to the OS. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Changes in v2: >>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>> Changes in v3: >>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>>>>>> >>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>>>>>> inaccurate. >>>>>>>>>> >>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>>>>>> on some systems the virtual device needs clocks to keep running. >>>>>>>>>> >>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>>>>>> fix in DT. >>>>>>>>>> >>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>>>>>> >>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>>>>>> block. Their use is implied by the connection. >>>>>>>>> >>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>>>>>> information. However, what you are adding IS hardware information and >>>>>>>>> clearly has a place somewhere else. And adding anything which is not >>>>>>>>> hardware description gets much more scrutiny. >>>>>>>>> >>>>>>>>>> More over it is a bit late to start making objections now. This has already been >>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>>>>>> read the entire thread in the archives under the subject: >>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>>>>>> >>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>>>>>> have made suggestions which I would agree with and you've basically >>>>>>>>> ignored them. >>>>>>>>> >>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>>>>>> we can all move on and spend out time in a more productive manner. >>>>>>>>> >>>>>>>>> Not an effective argument to get things merged. >>>>>>>> >>>>>>>> If there is not good solution to deferring clock clean up in the >>>>>>>> kernel, another approach is to change how simple-framebuffer is >>>>>>>> described in the device tree.... >>>>>>>> >>>>>>>> Right now it is a stand-alone item that looks like a device node, but >>>>>>>> it isn't a device. >>>>>>>> >>>>>>>> framebuffer { >>>>>>>> compatible = "simple-framebuffer"; >>>>>>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>>>>>> width = <1600>; >>>>>>>> height = <1200>; >>>>>>>> stride = <(1600 * 2)>; >>>>>>>> format = "r5g6b5"; >>>>>>>> }; >>>>>>>> >>>>>>>> How about something like this? >>>>>>>> >>>>>>>> reserved-memory { >>>>>>>> #address-cells = <1>; >>>>>>>> #size-cells = <1>; >>>>>>>> ranges; >>>>>>>> >>>>>>>> display_reserved: framebuffer@78000000 { >>>>>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>>>>> }; >>>>>>>> }; >>>>>>>> >>>>>>>> lcd0: lcd-controller@820000 { >>>>>>>> compatible = "marvell,dove-lcd"; >>>>>>>> reg = <0x820000 0x1000>; >>>>>>>> interrupts = <47>; >>>>>>>> clocks = <&si5351 0>; >>>>>>>> clock-names = "ext_ref_clk_1"; >>>>>>>> }; >>>>>>>> >>>>>>>> chosen { >>>>>>>> boot-framebuffer { >>>>>>>> compatible = "simple-framebuffer"; >>>>>>>> device = <&lcd0>; >>>>>>>> framebuffer = <&display_reserved>; >>>>>>>> width = <1600>; >>>>>>>> height = <1200>; >>>>>>>> stride = <(1600 * 2)>; >>>>>>>> format = "r5g6b5"; >>>>>>>> }; >>>>>>>> } >>>>>>>> >>>>>>>> >>>>>>>> This moves the definition of the boot framebuffer setup into the area >>>>>>>> where bootloader info is suppose to go. Then you can use the phandle >>>>>>>> to follow the actual device chains and protect the clocks and >>>>>>>> regulators. To make that work you are required to provide an accurate >>>>>>>> description of the real video hardware so that this chain can be >>>>>>>> followed. >>>>>>> >>>>>>> This will not work, first of all multiple blocks may be involved, so >>>>>>> the device = in the boot-framebuffer would need to be a list. That in >>>>>>> itself is not a problem, the problem is that the blocks used may have >>>>>>> multiple clocks, of which the setup mode likely uses only a few. >>>>>>> >>>>>>> So if we do things this way, we end up keeping way to many clocks >>>>>>> enabled. >>>>>> >>>>>> That doesn't hurt anything. >>>>> >>>>> <snip lots of stuff based on the above> >>>>> >>>>> Wrong, that does hurt things. As already discussed simply stopping the >>>>> clocks from being disabled by the unused_clks mechanism is not enough, >>>>> since clocks may be shared, and we must stop another device driver >>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling >>>>> the shared clk, which means calling clk_enable on it to mark it as >>>>> being in use. So this will hurt cause it will cause us to enable a bunch >>>>> of clks which should not be enabled. >>>> >>>> I said earlier that you would need to add a protected mechanism to >>>> clocks to handle this phase. When a clock/regulator is protected you >>>> can turn it on but you can't turn it off. When simplefb exits it will >>>> clear this protected status. When the protected status gets cleared >>>> treat it as a ref count decrement and turn off the clock/regulator if >>>> indicated to do so. If a clock is protected, all of it parents get the >>>> protected bit set too. >>>> >>>> Protected mode >>>> you can turn clocks on, but not off >>>> it is ref counted >>>> when it hits zero, look at the normal refcount and set that state >>>> >>>> Protected mode is not long lived. It only hangs around until the real >>>> device driver loads. >>> >>> And that has already been nacked by the clk maintainer because it is >>> too complicated, and I agree this is tons more complicated then simply >>> adding the list of clocks to the simplefb node. >>> >>>>> I've been thinking more about this, and I understand that people don't >>>>> want to put hardware description in the simplefb node, but this is >>>>> not hardware description. >>>>> >>>>> u-boot sets up the display-pipeline to scan out of a certain part of >>>>> memory, in order to do this it writes the memory address to some registers >>>>> in the display pipeline, so what it is passing is not hardware description >>>>> (it is not passing all possible memory addresses for the framebuffer), but >>>>> it is passing information about the state in which it has left the display >>>>> pipeline, iow it is passing state information. >>>> >>>> Giving the buffer to a piece of hardware is more than setting state. >>>> The hardware now owns that buffer. That ownership needs to be managed >>>> so that if the hardware decides it doesn't want the buffer it can be >>>> returned to the global pool. >>>> >>>> That's why the buffer has to go into that reserved memory section, not >>>> the chosen section. >>> >>> But is not in the reserved memory section, it is in the simplefb >>> section, and we're stuck with this because of backward compatibility. >>> >>> An OS doesn't have to process chosen, it is just >>>> there for informational purposes. To keep the OS from thinking it owns >>>> the memory in that video buffer and can use it for OS purposes, it has >>>> to go into that reserved memory section. >>>> >>>> The clocks are different. We know exactly who owns those clocks, the >>>> graphics hardware. >>>> >>>> You want something like this where the state of the entire video path >>>> is encoded into the chosen section. But that info is going to vary all >>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb >>>> isn't going to be simple any more. Plus the purposes of adding all of >>>> this complexity is just to handle the half second transition from boot >>>> loader control to real driver. >>>> >>>> reserved-memory { >>>> #address-cells = <1>; >>>> #size-cells = <1>; >>>> ranges; >>>> >>>> display_reserved: framebuffer@78000000 { >>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>> }; >>>> }; >>>> >>>> lcd0: lcd-controller@820000 { >>>> compatible = "marvell,dove-lcd"; >>>> reg = <0x820000 0x1000>; >>>> interrupts = <47>; >>>> framebuffer = <&display_reserved>; >>>> clocks = <&si5351 0>; >>>> clock-names = "ext_ref_clk_1"; >>>> }; >>>> >>>> chosen { >>>> boot-framebuffer { >>>> compatible = "simple-framebuffer"; >>>> state { >>>> device = <&lcd0>; >>>> width = <1600>; >>>> height = <1200>; >>>> stride = <(1600 * 2)>; >>>> format = "r5g6b5"; >>>> clocks = <&si5351 on 10mhz>; >>> >>> Right, so here we get a list of clocks which are actually in use >>> by the simplefb, just as I've been advocating all the time already. >>> >>> Note that the clock speed is not necessary, all the kernel needs to >>> know is that it must not change it. >>> >>> So as you seem to agree that we need to pass some sort of clock state >>> info, then all we need to agree on now is where to put it, as said adding >>> the reg property to a reserved-memory node is out of the question because >>> of backward compat, likewise storing width, height and format in a state >>> sub-node are out of the question for the same reason. But other then that >>> I'm fine with putting the simplefb node under chosen and putting clocks >>> in there (without the state subnode) as you suggest above. >>> >>> So we seem to be in agreement here :) >>> >>>> output = "hdmi"; >>>> state { >>>> device = <&hdmi> >>>> clocks = <&xyz on 12mhz>; >>>> } >>> >>> That level of detail won't be necessary, simplefb is supposed to be >>> simple, the kernel does not need to know which outputs there are, there >>> will always be only one for simplefb. >> >> I don't agree, but you are going to do it any way so I'll try and help >> tkeep problem side effects I know of to a minimum. You are relying on >> the BIOS to provide detailed, accurate information. Relying on BIOSes >> to do that has historically been a very bad idea. >> >> If you go the way of putting this info into the chosen section you are >> going to have to mark the clocks/regulators in use for all of the >> outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be >> useful if the backlight turns off because simplefb hasn't grabbed it. >> >> This is the only real difference between the proposals - you want >> uboot to enumerate what needs to be protected. I don't trust the BIOS >> to do that reliably so I'd preferred to just protect everything in the >> device hardware chain until the device specific drivers load. >> >> ------------------------------------------------------- >> >> I also still believe this is a problem up in Linux that we shouldn't >> be using the device tree to fix. >> >> It seems to me that the need for something like a 'protected' mode is >> a generic problem that could be extended to all hardware. In protected >> mode things can be turned on but nothing can be turned off. Only >> after the kernel has all of the necessary drivers loaded would a small >> app run that hits an IOCTL and says, ok protected mode is over now >> clean up these things wasting power. > > What happens if some clock needs to be disabled? > Like clocks that must be gated before setting a new clock rate > or reparenting. The kernel supports these, and it wouldn't surprise me > if some driver actually requires this. You might end up blocking that driver > or breaking its probe function. Arggh, using those phandles in the chosen section means uboot is going to have to get recompiled every time the DTS changes. I think we need to come up with a scheme that doesn't need for uboot to be aware of phandles. Something like this... uboot adds the chosen section then Linux would error out if the framebuffer in the chosen section doesn't match the reserved memory it is expecting. Or make uboot smart enough to hunt down the reserved memory section and patch it like it does with dramsize. reserved-memory { #address-cells = <1>; #size-cells = <1>; ranges; display_reserved: framebuffer@78000000 { reg = <0x78000000 (1600 * 1200 * 2)>; }; }; lcd0: lcd-controller@820000 { compatible = "marvell,dove-lcd"; reg = <0x820000 0x1000>; interrupts = <47>; framebuffer = <&display_reserved>; clocks = <&si5351 0>; clock-names = "ext_ref_clk_1"; }; chosen { boot-framebuffer { framebuffer = <0x78000000>; width = <1600>; height = <1200>; stride = <(1600 * 2)>; format = "r5g6b5"; }; } > > And what if reset controls are asserted then de-asserted in the probe function? > IIRC there are drivers that do this to reset the underlying hardware. > >> Maybe it should be renamed 'boot' mode. To implement this the various >> 'turn off' functions would get a 'boot' mode flag. In boot mode they >> track the ref counts but don't turn things off when the ref count hits >> zero. Then that ioctl() that the user space app calls runs the chains >> of all of the clocks/regulators/etc and if the ref count is zero turns >> them off again and clears 'boot' mode. Doesn't matter if you turn off >> something again that is already off. > > And what if something just happened to be left on that some driver > wants to turn off? You are assuming everything not used is off, > which is not always the case. > > > ChenYu
On Sun, Oct 5, 2014 at 11:29 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > On Sun, Oct 5, 2014 at 11:17 AM, Chen-Yu Tsai <wens@csie.org> wrote: >> On Sun, Oct 5, 2014 at 11:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi, >>>> >>>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote: >>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>> Hi, >>>>>> >>>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>>>>>>> property to communicate this to the OS. >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>>>>>>> >>>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>>>>>>> inaccurate. >>>>>>>>>>> >>>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>>>>>>> on some systems the virtual device needs clocks to keep running. >>>>>>>>>>> >>>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>>>>>>> fix in DT. >>>>>>>>>>> >>>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>>>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>>>>>>> >>>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>>>>>>> block. Their use is implied by the connection. >>>>>>>>>> >>>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>>>>>>> information. However, what you are adding IS hardware information and >>>>>>>>>> clearly has a place somewhere else. And adding anything which is not >>>>>>>>>> hardware description gets much more scrutiny. >>>>>>>>>> >>>>>>>>>>> More over it is a bit late to start making objections now. This has already been >>>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>>>>>>> read the entire thread in the archives under the subject: >>>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>>>>>>> >>>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>>>>>>> have made suggestions which I would agree with and you've basically >>>>>>>>>> ignored them. >>>>>>>>>> >>>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>>>>>>> we can all move on and spend out time in a more productive manner. >>>>>>>>>> >>>>>>>>>> Not an effective argument to get things merged. >>>>>>>>> >>>>>>>>> If there is not good solution to deferring clock clean up in the >>>>>>>>> kernel, another approach is to change how simple-framebuffer is >>>>>>>>> described in the device tree.... >>>>>>>>> >>>>>>>>> Right now it is a stand-alone item that looks like a device node, but >>>>>>>>> it isn't a device. >>>>>>>>> >>>>>>>>> framebuffer { >>>>>>>>> compatible = "simple-framebuffer"; >>>>>>>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>>>>>>> width = <1600>; >>>>>>>>> height = <1200>; >>>>>>>>> stride = <(1600 * 2)>; >>>>>>>>> format = "r5g6b5"; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> How about something like this? >>>>>>>>> >>>>>>>>> reserved-memory { >>>>>>>>> #address-cells = <1>; >>>>>>>>> #size-cells = <1>; >>>>>>>>> ranges; >>>>>>>>> >>>>>>>>> display_reserved: framebuffer@78000000 { >>>>>>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>>>>>> }; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> lcd0: lcd-controller@820000 { >>>>>>>>> compatible = "marvell,dove-lcd"; >>>>>>>>> reg = <0x820000 0x1000>; >>>>>>>>> interrupts = <47>; >>>>>>>>> clocks = <&si5351 0>; >>>>>>>>> clock-names = "ext_ref_clk_1"; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> chosen { >>>>>>>>> boot-framebuffer { >>>>>>>>> compatible = "simple-framebuffer"; >>>>>>>>> device = <&lcd0>; >>>>>>>>> framebuffer = <&display_reserved>; >>>>>>>>> width = <1600>; >>>>>>>>> height = <1200>; >>>>>>>>> stride = <(1600 * 2)>; >>>>>>>>> format = "r5g6b5"; >>>>>>>>> }; >>>>>>>>> } >>>>>>>>> >>>>>>>>> >>>>>>>>> This moves the definition of the boot framebuffer setup into the area >>>>>>>>> where bootloader info is suppose to go. Then you can use the phandle >>>>>>>>> to follow the actual device chains and protect the clocks and >>>>>>>>> regulators. To make that work you are required to provide an accurate >>>>>>>>> description of the real video hardware so that this chain can be >>>>>>>>> followed. >>>>>>>> >>>>>>>> This will not work, first of all multiple blocks may be involved, so >>>>>>>> the device = in the boot-framebuffer would need to be a list. That in >>>>>>>> itself is not a problem, the problem is that the blocks used may have >>>>>>>> multiple clocks, of which the setup mode likely uses only a few. >>>>>>>> >>>>>>>> So if we do things this way, we end up keeping way to many clocks >>>>>>>> enabled. >>>>>>> >>>>>>> That doesn't hurt anything. >>>>>> >>>>>> <snip lots of stuff based on the above> >>>>>> >>>>>> Wrong, that does hurt things. As already discussed simply stopping the >>>>>> clocks from being disabled by the unused_clks mechanism is not enough, >>>>>> since clocks may be shared, and we must stop another device driver >>>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling >>>>>> the shared clk, which means calling clk_enable on it to mark it as >>>>>> being in use. So this will hurt cause it will cause us to enable a bunch >>>>>> of clks which should not be enabled. >>>>> >>>>> I said earlier that you would need to add a protected mechanism to >>>>> clocks to handle this phase. When a clock/regulator is protected you >>>>> can turn it on but you can't turn it off. When simplefb exits it will >>>>> clear this protected status. When the protected status gets cleared >>>>> treat it as a ref count decrement and turn off the clock/regulator if >>>>> indicated to do so. If a clock is protected, all of it parents get the >>>>> protected bit set too. >>>>> >>>>> Protected mode >>>>> you can turn clocks on, but not off >>>>> it is ref counted >>>>> when it hits zero, look at the normal refcount and set that state >>>>> >>>>> Protected mode is not long lived. It only hangs around until the real >>>>> device driver loads. >>>> >>>> And that has already been nacked by the clk maintainer because it is >>>> too complicated, and I agree this is tons more complicated then simply >>>> adding the list of clocks to the simplefb node. >>>> >>>>>> I've been thinking more about this, and I understand that people don't >>>>>> want to put hardware description in the simplefb node, but this is >>>>>> not hardware description. >>>>>> >>>>>> u-boot sets up the display-pipeline to scan out of a certain part of >>>>>> memory, in order to do this it writes the memory address to some registers >>>>>> in the display pipeline, so what it is passing is not hardware description >>>>>> (it is not passing all possible memory addresses for the framebuffer), but >>>>>> it is passing information about the state in which it has left the display >>>>>> pipeline, iow it is passing state information. >>>>> >>>>> Giving the buffer to a piece of hardware is more than setting state. >>>>> The hardware now owns that buffer. That ownership needs to be managed >>>>> so that if the hardware decides it doesn't want the buffer it can be >>>>> returned to the global pool. >>>>> >>>>> That's why the buffer has to go into that reserved memory section, not >>>>> the chosen section. >>>> >>>> But is not in the reserved memory section, it is in the simplefb >>>> section, and we're stuck with this because of backward compatibility. >>>> >>>> An OS doesn't have to process chosen, it is just >>>>> there for informational purposes. To keep the OS from thinking it owns >>>>> the memory in that video buffer and can use it for OS purposes, it has >>>>> to go into that reserved memory section. >>>>> >>>>> The clocks are different. We know exactly who owns those clocks, the >>>>> graphics hardware. >>>>> >>>>> You want something like this where the state of the entire video path >>>>> is encoded into the chosen section. But that info is going to vary all >>>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb >>>>> isn't going to be simple any more. Plus the purposes of adding all of >>>>> this complexity is just to handle the half second transition from boot >>>>> loader control to real driver. >>>>> >>>>> reserved-memory { >>>>> #address-cells = <1>; >>>>> #size-cells = <1>; >>>>> ranges; >>>>> >>>>> display_reserved: framebuffer@78000000 { >>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>> }; >>>>> }; >>>>> >>>>> lcd0: lcd-controller@820000 { >>>>> compatible = "marvell,dove-lcd"; >>>>> reg = <0x820000 0x1000>; >>>>> interrupts = <47>; >>>>> framebuffer = <&display_reserved>; >>>>> clocks = <&si5351 0>; >>>>> clock-names = "ext_ref_clk_1"; >>>>> }; >>>>> >>>>> chosen { >>>>> boot-framebuffer { >>>>> compatible = "simple-framebuffer"; >>>>> state { >>>>> device = <&lcd0>; >>>>> width = <1600>; >>>>> height = <1200>; >>>>> stride = <(1600 * 2)>; >>>>> format = "r5g6b5"; >>>>> clocks = <&si5351 on 10mhz>; >>>> >>>> Right, so here we get a list of clocks which are actually in use >>>> by the simplefb, just as I've been advocating all the time already. >>>> >>>> Note that the clock speed is not necessary, all the kernel needs to >>>> know is that it must not change it. >>>> >>>> So as you seem to agree that we need to pass some sort of clock state >>>> info, then all we need to agree on now is where to put it, as said adding >>>> the reg property to a reserved-memory node is out of the question because >>>> of backward compat, likewise storing width, height and format in a state >>>> sub-node are out of the question for the same reason. But other then that >>>> I'm fine with putting the simplefb node under chosen and putting clocks >>>> in there (without the state subnode) as you suggest above. >>>> >>>> So we seem to be in agreement here :) >>>> >>>>> output = "hdmi"; >>>>> state { >>>>> device = <&hdmi> >>>>> clocks = <&xyz on 12mhz>; >>>>> } >>>> >>>> That level of detail won't be necessary, simplefb is supposed to be >>>> simple, the kernel does not need to know which outputs there are, there >>>> will always be only one for simplefb. >>> >>> I don't agree, but you are going to do it any way so I'll try and help >>> tkeep problem side effects I know of to a minimum. You are relying on >>> the BIOS to provide detailed, accurate information. Relying on BIOSes >>> to do that has historically been a very bad idea. >>> >>> If you go the way of putting this info into the chosen section you are >>> going to have to mark the clocks/regulators in use for all of the >>> outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be >>> useful if the backlight turns off because simplefb hasn't grabbed it. >>> >>> This is the only real difference between the proposals - you want >>> uboot to enumerate what needs to be protected. I don't trust the BIOS >>> to do that reliably so I'd preferred to just protect everything in the >>> device hardware chain until the device specific drivers load. >>> >>> ------------------------------------------------------- >>> >>> I also still believe this is a problem up in Linux that we shouldn't >>> be using the device tree to fix. >>> >>> It seems to me that the need for something like a 'protected' mode is >>> a generic problem that could be extended to all hardware. In protected >>> mode things can be turned on but nothing can be turned off. Only >>> after the kernel has all of the necessary drivers loaded would a small >>> app run that hits an IOCTL and says, ok protected mode is over now >>> clean up these things wasting power. >> >> What happens if some clock needs to be disabled? >> Like clocks that must be gated before setting a new clock rate >> or reparenting. The kernel supports these, and it wouldn't surprise me >> if some driver actually requires this. You might end up blocking that driver >> or breaking its probe function. > > Arggh, using those phandles in the chosen section means uboot is going > to have to get recompiled every time the DTS changes. I think we need > to come up with a scheme that doesn't need for uboot to be aware of > phandles. Why is that? U-boot is perfectly capable of patching device tree blobs. Mainline u-boot already updates the memory node, and if needed, the reserved-memory node as well. Someone just has to write the (ugly) code to do it, which Luc has already done for clock phandles for sunxi. U-boot itself does not need to use the dtb, though that seems like the direction it's headed. > Something like this... > uboot adds the chosen section then Linux would error out if the > framebuffer in the chosen section doesn't match the reserved memory it > is expecting. Or make uboot smart enough to hunt down the reserved > memory section and patch it like it does with dramsize. And someone probably will. Why is that a problem? ChenYu > > reserved-memory { > #address-cells = <1>; > #size-cells = <1>; > ranges; > > display_reserved: framebuffer@78000000 { > reg = <0x78000000 (1600 * 1200 * 2)>; > }; > }; > > lcd0: lcd-controller@820000 { > compatible = "marvell,dove-lcd"; > reg = <0x820000 0x1000>; > interrupts = <47>; > framebuffer = <&display_reserved>; > clocks = <&si5351 0>; > clock-names = "ext_ref_clk_1"; > }; > > chosen { > boot-framebuffer { > framebuffer = <0x78000000>; > width = <1600>; > height = <1200>; > stride = <(1600 * 2)>; > format = "r5g6b5"; > }; > } > > > >> >> And what if reset controls are asserted then de-asserted in the probe function? >> IIRC there are drivers that do this to reset the underlying hardware. >> >>> Maybe it should be renamed 'boot' mode. To implement this the various >>> 'turn off' functions would get a 'boot' mode flag. In boot mode they >>> track the ref counts but don't turn things off when the ref count hits >>> zero. Then that ioctl() that the user space app calls runs the chains >>> of all of the clocks/regulators/etc and if the ref count is zero turns >>> them off again and clears 'boot' mode. Doesn't matter if you turn off >>> something again that is already off. >> >> And what if something just happened to be left on that some driver >> wants to turn off? You are assuming everything not used is off, >> which is not always the case. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Oct 5, 2014 at 11:36 AM, Chen-Yu Tsai <wens@csie.org> wrote: > On Sun, Oct 5, 2014 at 11:29 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >> On Sun, Oct 5, 2014 at 11:17 AM, Chen-Yu Tsai <wens@csie.org> wrote: >>> On Sun, Oct 5, 2014 at 11:07 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>> Hi, >>>>> >>>>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote: >>>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >>>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>>> Hi, >>>>>>>>>>>> >>>>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>>>>>>>> property to communicate this to the OS. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>>>> >>>>>>>>>>>>>> -- >>>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>>>>>>>> >>>>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>>>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>>>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>>>>>>>> inaccurate. >>>>>>>>>>>> >>>>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>>>>>>>> on some systems the virtual device needs clocks to keep running. >>>>>>>>>>>> >>>>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>>>>>>>> fix in DT. >>>>>>>>>>>> >>>>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>>>>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>>>>>>>> >>>>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>>>>>>>> block. Their use is implied by the connection. >>>>>>>>>>> >>>>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>>>>>>>> information. However, what you are adding IS hardware information and >>>>>>>>>>> clearly has a place somewhere else. And adding anything which is not >>>>>>>>>>> hardware description gets much more scrutiny. >>>>>>>>>>> >>>>>>>>>>>> More over it is a bit late to start making objections now. This has already been >>>>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>>>>>>>> read the entire thread in the archives under the subject: >>>>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>>>>>>>> >>>>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>>>>>>>> have made suggestions which I would agree with and you've basically >>>>>>>>>>> ignored them. >>>>>>>>>>> >>>>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>>>>>>>> we can all move on and spend out time in a more productive manner. >>>>>>>>>>> >>>>>>>>>>> Not an effective argument to get things merged. >>>>>>>>>> >>>>>>>>>> If there is not good solution to deferring clock clean up in the >>>>>>>>>> kernel, another approach is to change how simple-framebuffer is >>>>>>>>>> described in the device tree.... >>>>>>>>>> >>>>>>>>>> Right now it is a stand-alone item that looks like a device node, but >>>>>>>>>> it isn't a device. >>>>>>>>>> >>>>>>>>>> framebuffer { >>>>>>>>>> compatible = "simple-framebuffer"; >>>>>>>>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>>>>>>>> width = <1600>; >>>>>>>>>> height = <1200>; >>>>>>>>>> stride = <(1600 * 2)>; >>>>>>>>>> format = "r5g6b5"; >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> How about something like this? >>>>>>>>>> >>>>>>>>>> reserved-memory { >>>>>>>>>> #address-cells = <1>; >>>>>>>>>> #size-cells = <1>; >>>>>>>>>> ranges; >>>>>>>>>> >>>>>>>>>> display_reserved: framebuffer@78000000 { >>>>>>>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>>>>>>> }; >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> lcd0: lcd-controller@820000 { >>>>>>>>>> compatible = "marvell,dove-lcd"; >>>>>>>>>> reg = <0x820000 0x1000>; >>>>>>>>>> interrupts = <47>; >>>>>>>>>> clocks = <&si5351 0>; >>>>>>>>>> clock-names = "ext_ref_clk_1"; >>>>>>>>>> }; >>>>>>>>>> >>>>>>>>>> chosen { >>>>>>>>>> boot-framebuffer { >>>>>>>>>> compatible = "simple-framebuffer"; >>>>>>>>>> device = <&lcd0>; >>>>>>>>>> framebuffer = <&display_reserved>; >>>>>>>>>> width = <1600>; >>>>>>>>>> height = <1200>; >>>>>>>>>> stride = <(1600 * 2)>; >>>>>>>>>> format = "r5g6b5"; >>>>>>>>>> }; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> This moves the definition of the boot framebuffer setup into the area >>>>>>>>>> where bootloader info is suppose to go. Then you can use the phandle >>>>>>>>>> to follow the actual device chains and protect the clocks and >>>>>>>>>> regulators. To make that work you are required to provide an accurate >>>>>>>>>> description of the real video hardware so that this chain can be >>>>>>>>>> followed. >>>>>>>>> >>>>>>>>> This will not work, first of all multiple blocks may be involved, so >>>>>>>>> the device = in the boot-framebuffer would need to be a list. That in >>>>>>>>> itself is not a problem, the problem is that the blocks used may have >>>>>>>>> multiple clocks, of which the setup mode likely uses only a few. >>>>>>>>> >>>>>>>>> So if we do things this way, we end up keeping way to many clocks >>>>>>>>> enabled. >>>>>>>> >>>>>>>> That doesn't hurt anything. >>>>>>> >>>>>>> <snip lots of stuff based on the above> >>>>>>> >>>>>>> Wrong, that does hurt things. As already discussed simply stopping the >>>>>>> clocks from being disabled by the unused_clks mechanism is not enough, >>>>>>> since clocks may be shared, and we must stop another device driver >>>>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling >>>>>>> the shared clk, which means calling clk_enable on it to mark it as >>>>>>> being in use. So this will hurt cause it will cause us to enable a bunch >>>>>>> of clks which should not be enabled. >>>>>> >>>>>> I said earlier that you would need to add a protected mechanism to >>>>>> clocks to handle this phase. When a clock/regulator is protected you >>>>>> can turn it on but you can't turn it off. When simplefb exits it will >>>>>> clear this protected status. When the protected status gets cleared >>>>>> treat it as a ref count decrement and turn off the clock/regulator if >>>>>> indicated to do so. If a clock is protected, all of it parents get the >>>>>> protected bit set too. >>>>>> >>>>>> Protected mode >>>>>> you can turn clocks on, but not off >>>>>> it is ref counted >>>>>> when it hits zero, look at the normal refcount and set that state >>>>>> >>>>>> Protected mode is not long lived. It only hangs around until the real >>>>>> device driver loads. >>>>> >>>>> And that has already been nacked by the clk maintainer because it is >>>>> too complicated, and I agree this is tons more complicated then simply >>>>> adding the list of clocks to the simplefb node. >>>>> >>>>>>> I've been thinking more about this, and I understand that people don't >>>>>>> want to put hardware description in the simplefb node, but this is >>>>>>> not hardware description. >>>>>>> >>>>>>> u-boot sets up the display-pipeline to scan out of a certain part of >>>>>>> memory, in order to do this it writes the memory address to some registers >>>>>>> in the display pipeline, so what it is passing is not hardware description >>>>>>> (it is not passing all possible memory addresses for the framebuffer), but >>>>>>> it is passing information about the state in which it has left the display >>>>>>> pipeline, iow it is passing state information. >>>>>> >>>>>> Giving the buffer to a piece of hardware is more than setting state. >>>>>> The hardware now owns that buffer. That ownership needs to be managed >>>>>> so that if the hardware decides it doesn't want the buffer it can be >>>>>> returned to the global pool. >>>>>> >>>>>> That's why the buffer has to go into that reserved memory section, not >>>>>> the chosen section. >>>>> >>>>> But is not in the reserved memory section, it is in the simplefb >>>>> section, and we're stuck with this because of backward compatibility. >>>>> >>>>> An OS doesn't have to process chosen, it is just >>>>>> there for informational purposes. To keep the OS from thinking it owns >>>>>> the memory in that video buffer and can use it for OS purposes, it has >>>>>> to go into that reserved memory section. >>>>>> >>>>>> The clocks are different. We know exactly who owns those clocks, the >>>>>> graphics hardware. >>>>>> >>>>>> You want something like this where the state of the entire video path >>>>>> is encoded into the chosen section. But that info is going to vary all >>>>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb >>>>>> isn't going to be simple any more. Plus the purposes of adding all of >>>>>> this complexity is just to handle the half second transition from boot >>>>>> loader control to real driver. >>>>>> >>>>>> reserved-memory { >>>>>> #address-cells = <1>; >>>>>> #size-cells = <1>; >>>>>> ranges; >>>>>> >>>>>> display_reserved: framebuffer@78000000 { >>>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>>> }; >>>>>> }; >>>>>> >>>>>> lcd0: lcd-controller@820000 { >>>>>> compatible = "marvell,dove-lcd"; >>>>>> reg = <0x820000 0x1000>; >>>>>> interrupts = <47>; >>>>>> framebuffer = <&display_reserved>; >>>>>> clocks = <&si5351 0>; >>>>>> clock-names = "ext_ref_clk_1"; >>>>>> }; >>>>>> >>>>>> chosen { >>>>>> boot-framebuffer { >>>>>> compatible = "simple-framebuffer"; >>>>>> state { >>>>>> device = <&lcd0>; >>>>>> width = <1600>; >>>>>> height = <1200>; >>>>>> stride = <(1600 * 2)>; >>>>>> format = "r5g6b5"; >>>>>> clocks = <&si5351 on 10mhz>; >>>>> >>>>> Right, so here we get a list of clocks which are actually in use >>>>> by the simplefb, just as I've been advocating all the time already. >>>>> >>>>> Note that the clock speed is not necessary, all the kernel needs to >>>>> know is that it must not change it. >>>>> >>>>> So as you seem to agree that we need to pass some sort of clock state >>>>> info, then all we need to agree on now is where to put it, as said adding >>>>> the reg property to a reserved-memory node is out of the question because >>>>> of backward compat, likewise storing width, height and format in a state >>>>> sub-node are out of the question for the same reason. But other then that >>>>> I'm fine with putting the simplefb node under chosen and putting clocks >>>>> in there (without the state subnode) as you suggest above. >>>>> >>>>> So we seem to be in agreement here :) >>>>> >>>>>> output = "hdmi"; >>>>>> state { >>>>>> device = <&hdmi> >>>>>> clocks = <&xyz on 12mhz>; >>>>>> } >>>>> >>>>> That level of detail won't be necessary, simplefb is supposed to be >>>>> simple, the kernel does not need to know which outputs there are, there >>>>> will always be only one for simplefb. >>>> >>>> I don't agree, but you are going to do it any way so I'll try and help >>>> tkeep problem side effects I know of to a minimum. You are relying on >>>> the BIOS to provide detailed, accurate information. Relying on BIOSes >>>> to do that has historically been a very bad idea. >>>> >>>> If you go the way of putting this info into the chosen section you are >>>> going to have to mark the clocks/regulators in use for all of the >>>> outputs too -- hdmi, TV, LCD, backlights, etc, etc. Not going to be >>>> useful if the backlight turns off because simplefb hasn't grabbed it. >>>> >>>> This is the only real difference between the proposals - you want >>>> uboot to enumerate what needs to be protected. I don't trust the BIOS >>>> to do that reliably so I'd preferred to just protect everything in the >>>> device hardware chain until the device specific drivers load. >>>> >>>> ------------------------------------------------------- >>>> >>>> I also still believe this is a problem up in Linux that we shouldn't >>>> be using the device tree to fix. >>>> >>>> It seems to me that the need for something like a 'protected' mode is >>>> a generic problem that could be extended to all hardware. In protected >>>> mode things can be turned on but nothing can be turned off. Only >>>> after the kernel has all of the necessary drivers loaded would a small >>>> app run that hits an IOCTL and says, ok protected mode is over now >>>> clean up these things wasting power. >>> >>> What happens if some clock needs to be disabled? >>> Like clocks that must be gated before setting a new clock rate >>> or reparenting. The kernel supports these, and it wouldn't surprise me >>> if some driver actually requires this. You might end up blocking that driver >>> or breaking its probe function. >> >> Arggh, using those phandles in the chosen section means uboot is going >> to have to get recompiled every time the DTS changes. I think we need >> to come up with a scheme that doesn't need for uboot to be aware of >> phandles. > > Why is that? U-boot is perfectly capable of patching device tree blobs. > > Mainline u-boot already updates the memory node, and if needed, > the reserved-memory node as well. > > Someone just has to write the (ugly) code to do it, which Luc > has already done for clock phandles for sunxi. So uboot is going to contain code to hunt through the Linux provided DTB to find the correct phandles for the clocks/regulators and then patch those phandles into the chosen section? How is uboot going to reconcile it's concept of what those clock/regulators are with a Linux provided DTB that is constant state of flux? I think trying to get uboot to manipulate phandles in a Linux provided DTB is an incredibly fragile mechanism that will be prone to breakage. Much better to come with a scheme where uboot just inserts fixed strings into the DTB. That last example device tree I posted removed all of the phandles from the chosen section, but it relies on the kernel gaining 'boot' mode. > > U-boot itself does not need to use the dtb, though that seems > like the direction it's headed. > >> Something like this... >> uboot adds the chosen section then Linux would error out if the >> framebuffer in the chosen section doesn't match the reserved memory it >> is expecting. Or make uboot smart enough to hunt down the reserved >> memory section and patch it like it does with dramsize. > > And someone probably will. Why is that a problem? > > > ChenYu > > >> >> reserved-memory { >> #address-cells = <1>; >> #size-cells = <1>; >> ranges; >> >> display_reserved: framebuffer@78000000 { >> reg = <0x78000000 (1600 * 1200 * 2)>; >> }; >> }; >> >> lcd0: lcd-controller@820000 { >> compatible = "marvell,dove-lcd"; >> reg = <0x820000 0x1000>; >> interrupts = <47>; >> framebuffer = <&display_reserved>; >> clocks = <&si5351 0>; >> clock-names = "ext_ref_clk_1"; >> }; >> >> chosen { >> boot-framebuffer { >> framebuffer = <0x78000000>; >> width = <1600>; >> height = <1200>; >> stride = <(1600 * 2)>; >> format = "r5g6b5"; >> }; >> } >> >> >> >>> >>> And what if reset controls are asserted then de-asserted in the probe function? >>> IIRC there are drivers that do this to reset the underlying hardware. >>> >>>> Maybe it should be renamed 'boot' mode. To implement this the various >>>> 'turn off' functions would get a 'boot' mode flag. In boot mode they >>>> track the ref counts but don't turn things off when the ref count hits >>>> zero. Then that ioctl() that the user space app calls runs the chains >>>> of all of the clocks/regulators/etc and if the ref count is zero turns >>>> them off again and clears 'boot' mode. Doesn't matter if you turn off >>>> something again that is already off. >>> >>> And what if something just happened to be left on that some driver >>> wants to turn off? You are assuming everything not used is off, >>> which is not always the case.
Hi, On 10/05/2014 05:17 PM, jonsmirl@gmail.com wrote: > On Sun, Oct 5, 2014 at 11:16 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> Hi, >> >> On 10/05/2014 05:07 PM, jonsmirl@gmail.com wrote: >>> On Sun, Oct 5, 2014 at 10:27 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>> Hi, >>>> >>>> On 10/05/2014 02:52 PM, jonsmirl@gmail.com wrote: >>>>> On Sun, Oct 5, 2014 at 5:03 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>> Hi, >>>>>> >>>>>> On 10/04/2014 02:38 PM, jonsmirl@gmail.com wrote: >>>>>>> On Sat, Oct 4, 2014 at 5:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 10/04/2014 12:56 AM, jonsmirl@gmail.com wrote: >>>>>>>>> On Fri, Oct 3, 2014 at 4:08 PM, Rob Herring <robherring2@gmail.com> wrote: >>>>>>>>>> On Fri, Oct 3, 2014 at 12:34 PM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>> Hi, >>>>>>>>>>> >>>>>>>>>>> On 10/03/2014 05:57 PM, Rob Herring wrote: >>>>>>>>>>>> On Fri, Oct 3, 2014 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote: >>>>>>>>>>>>> A simple-framebuffer node represents a framebuffer setup by the firmware / >>>>>>>>>>>>> bootloader. Such a framebuffer may have a number of clocks in use, add a >>>>>>>>>>>>> property to communicate this to the OS. >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>>>>>>>>>>> Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>>> >>>>>>>>>>>>> -- >>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>> -Added Reviewed-by: Mike Turquette <mturquette@linaro.org> >>>>>>>>>>>>> Changes in v3: >>>>>>>>>>>>> -Updated description to make clear simplefb deals with more then just memory >>>>>>>>>>>> >>>>>>>>>>>> NAK. "Fixing" the description is not what I meant and does not address >>>>>>>>>>>> my concerns. Currently, simplefb is configuration data. It is >>>>>>>>>>>> auxiliary data about how a chunk of memory is used. Using it or not >>>>>>>>>>>> has no side effects on the hardware setup, but you are changing that >>>>>>>>>>>> aspect. You are mixing in a hardware description that is simply >>>>>>>>>>>> inaccurate. >>>>>>>>>>> >>>>>>>>>>> Memory is hardware too, what simplefb is is best seen as a virtual device, and >>>>>>>>>>> even virtual devices have hardware resources they need, such as a chunk of memory, >>>>>>>>>>> which the kernel should not touch other then use it as a framebuffer, likewise >>>>>>>>>>> on some systems the virtual device needs clocks to keep running. >>>>>>>>>>> >>>>>>>>>>>> The kernel has made the decision to turn off "unused" clocks. If its >>>>>>>>>>>> determination of what is unused is wrong, then it is not a problem to >>>>>>>>>>>> fix in DT. >>>>>>>>>>> >>>>>>>>>>> No, it is up to DT to tell the kernel what clocks are used, that is how it works >>>>>>>>>>> for any other device. I don't understand why some people keep insisting simplefb >>>>>>>>>>> for some reason is o so very very special, because it is not special, it needs >>>>>>>>>>> resources, and it needs to tell the kernel about this or bad things happen. >>>>>>>>>> >>>>>>>>>> No, the DT describes the connections of clocks from h/w block to h/w >>>>>>>>>> block. Their use is implied by the connection. >>>>>>>>>> >>>>>>>>>> And yes, as the other thread mentioned DT is more than just hardware >>>>>>>>>> information. However, what you are adding IS hardware information and >>>>>>>>>> clearly has a place somewhere else. And adding anything which is not >>>>>>>>>> hardware description gets much more scrutiny. >>>>>>>>>> >>>>>>>>>>> More over it is a bit late to start making objections now. This has already been >>>>>>>>>>> discussed to death for weeks now, and every argument against this patch has already >>>>>>>>>>> been countered multiple times (including the one you are making now). Feel free to >>>>>>>>>>> read the entire thread in the archives under the subject: >>>>>>>>>>> "[PATCH 4/4] simplefb: add clock handling code" >>>>>>>>>> >>>>>>>>>> You are on v2 and I hardly see any consensus on the v1 thread. Others >>>>>>>>>> have made suggestions which I would agree with and you've basically >>>>>>>>>> ignored them. >>>>>>>>>> >>>>>>>>>>> At one point in time we need to stop bickering and make a decision, that time has >>>>>>>>>>> come now, so please lets get this discussion over with and merge this, so that >>>>>>>>>>> we can all move on and spend out time in a more productive manner. >>>>>>>>>> >>>>>>>>>> Not an effective argument to get things merged. >>>>>>>>> >>>>>>>>> If there is not good solution to deferring clock clean up in the >>>>>>>>> kernel, another approach is to change how simple-framebuffer is >>>>>>>>> described in the device tree.... >>>>>>>>> >>>>>>>>> Right now it is a stand-alone item that looks like a device node, but >>>>>>>>> it isn't a device. >>>>>>>>> >>>>>>>>> framebuffer { >>>>>>>>> compatible = "simple-framebuffer"; >>>>>>>>> reg = <0x1d385000 (1600 * 1200 * 2)>; >>>>>>>>> width = <1600>; >>>>>>>>> height = <1200>; >>>>>>>>> stride = <(1600 * 2)>; >>>>>>>>> format = "r5g6b5"; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> How about something like this? >>>>>>>>> >>>>>>>>> reserved-memory { >>>>>>>>> #address-cells = <1>; >>>>>>>>> #size-cells = <1>; >>>>>>>>> ranges; >>>>>>>>> >>>>>>>>> display_reserved: framebuffer@78000000 { >>>>>>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>>>>>> }; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> lcd0: lcd-controller@820000 { >>>>>>>>> compatible = "marvell,dove-lcd"; >>>>>>>>> reg = <0x820000 0x1000>; >>>>>>>>> interrupts = <47>; >>>>>>>>> clocks = <&si5351 0>; >>>>>>>>> clock-names = "ext_ref_clk_1"; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> chosen { >>>>>>>>> boot-framebuffer { >>>>>>>>> compatible = "simple-framebuffer"; >>>>>>>>> device = <&lcd0>; >>>>>>>>> framebuffer = <&display_reserved>; >>>>>>>>> width = <1600>; >>>>>>>>> height = <1200>; >>>>>>>>> stride = <(1600 * 2)>; >>>>>>>>> format = "r5g6b5"; >>>>>>>>> }; >>>>>>>>> } >>>>>>>>> >>>>>>>>> >>>>>>>>> This moves the definition of the boot framebuffer setup into the area >>>>>>>>> where bootloader info is suppose to go. Then you can use the phandle >>>>>>>>> to follow the actual device chains and protect the clocks and >>>>>>>>> regulators. To make that work you are required to provide an accurate >>>>>>>>> description of the real video hardware so that this chain can be >>>>>>>>> followed. >>>>>>>> >>>>>>>> This will not work, first of all multiple blocks may be involved, so >>>>>>>> the device = in the boot-framebuffer would need to be a list. That in >>>>>>>> itself is not a problem, the problem is that the blocks used may have >>>>>>>> multiple clocks, of which the setup mode likely uses only a few. >>>>>>>> >>>>>>>> So if we do things this way, we end up keeping way to many clocks >>>>>>>> enabled. >>>>>>> >>>>>>> That doesn't hurt anything. >>>>>> >>>>>> <snip lots of stuff based on the above> >>>>>> >>>>>> Wrong, that does hurt things. As already discussed simply stopping the >>>>>> clocks from being disabled by the unused_clks mechanism is not enough, >>>>>> since clocks may be shared, and we must stop another device driver >>>>>> sharing the clock and doing clk_enable; probe; clk_disable; disabling >>>>>> the shared clk, which means calling clk_enable on it to mark it as >>>>>> being in use. So this will hurt cause it will cause us to enable a bunch >>>>>> of clks which should not be enabled. >>>>> >>>>> I said earlier that you would need to add a protected mechanism to >>>>> clocks to handle this phase. When a clock/regulator is protected you >>>>> can turn it on but you can't turn it off. When simplefb exits it will >>>>> clear this protected status. When the protected status gets cleared >>>>> treat it as a ref count decrement and turn off the clock/regulator if >>>>> indicated to do so. If a clock is protected, all of it parents get the >>>>> protected bit set too. >>>>> >>>>> Protected mode >>>>> you can turn clocks on, but not off >>>>> it is ref counted >>>>> when it hits zero, look at the normal refcount and set that state >>>>> >>>>> Protected mode is not long lived. It only hangs around until the real >>>>> device driver loads. >>>> >>>> And that has already been nacked by the clk maintainer because it is >>>> too complicated, and I agree this is tons more complicated then simply >>>> adding the list of clocks to the simplefb node. >>>> >>>>>> I've been thinking more about this, and I understand that people don't >>>>>> want to put hardware description in the simplefb node, but this is >>>>>> not hardware description. >>>>>> >>>>>> u-boot sets up the display-pipeline to scan out of a certain part of >>>>>> memory, in order to do this it writes the memory address to some registers >>>>>> in the display pipeline, so what it is passing is not hardware description >>>>>> (it is not passing all possible memory addresses for the framebuffer), but >>>>>> it is passing information about the state in which it has left the display >>>>>> pipeline, iow it is passing state information. >>>>> >>>>> Giving the buffer to a piece of hardware is more than setting state. >>>>> The hardware now owns that buffer. That ownership needs to be managed >>>>> so that if the hardware decides it doesn't want the buffer it can be >>>>> returned to the global pool. >>>>> >>>>> That's why the buffer has to go into that reserved memory section, not >>>>> the chosen section. >>>> >>>> But is not in the reserved memory section, it is in the simplefb >>>> section, and we're stuck with this because of backward compatibility. >>>> >>>> An OS doesn't have to process chosen, it is just >>>>> there for informational purposes. To keep the OS from thinking it owns >>>>> the memory in that video buffer and can use it for OS purposes, it has >>>>> to go into that reserved memory section. >>>>> >>>>> The clocks are different. We know exactly who owns those clocks, the >>>>> graphics hardware. >>>>> >>>>> You want something like this where the state of the entire video path >>>>> is encoded into the chosen section. But that info is going to vary all >>>>> over the place with TV output, HDMI output, LCD panels, etc. simplefb >>>>> isn't going to be simple any more. Plus the purposes of adding all of >>>>> this complexity is just to handle the half second transition from boot >>>>> loader control to real driver. >>>>> >>>>> reserved-memory { >>>>> #address-cells = <1>; >>>>> #size-cells = <1>; >>>>> ranges; >>>>> >>>>> display_reserved: framebuffer@78000000 { >>>>> reg = <0x78000000 (1600 * 1200 * 2)>; >>>>> }; >>>>> }; >>>>> >>>>> lcd0: lcd-controller@820000 { >>>>> compatible = "marvell,dove-lcd"; >>>>> reg = <0x820000 0x1000>; >>>>> interrupts = <47>; >>>>> framebuffer = <&display_reserved>; >>>>> clocks = <&si5351 0>; >>>>> clock-names = "ext_ref_clk_1"; >>>>> }; >>>>> >>>>> chosen { >>>>> boot-framebuffer { >>>>> compatible = "simple-framebuffer"; >>>>> state { >>>>> device = <&lcd0>; >>>>> width = <1600>; >>>>> height = <1200>; >>>>> stride = <(1600 * 2)>; >>>>> format = "r5g6b5"; >>>>> clocks = <&si5351 on 10mhz>; >>>> >>>> Right, so here we get a list of clocks which are actually in use >>>> by the simplefb, just as I've been advocating all the time already. >>>> >>>> Note that the clock speed is not necessary, all the kernel needs to >>>> know is that it must not change it. >>>> >>>> So as you seem to agree that we need to pass some sort of clock state >>>> info, then all we need to agree on now is where to put it, as said adding >>>> the reg property to a reserved-memory node is out of the question because >>>> of backward compat, likewise storing width, height and format in a state >>>> sub-node are out of the question for the same reason. But other then that >>>> I'm fine with putting the simplefb node under chosen and putting clocks >>>> in there (without the state subnode) as you suggest above. >>>> >>>> So we seem to be in agreement here :) >>>> >>>>> output = "hdmi"; >>>>> state { >>>>> device = <&hdmi> >>>>> clocks = <&xyz on 12mhz>; >>>>> } >>>> >>>> That level of detail won't be necessary, simplefb is supposed to be >>>> simple, the kernel does not need to know which outputs there are, there >>>> will always be only one for simplefb. >>> >>> I don't agree, but you are going to do it any way so I'll try and help >>> tkeep problem side effects I know of to a minimum. You are relying on >>> the BIOS to provide detailed, accurate information. Relying on BIOSes >>> to do that has historically been a very bad idea. >> >> And here we come to the gist of your objections, I don't agree for >> various reasons: >> >> 1) Despite firmware sometimes having bugs, we simply have to trust the firmware, >> e.g. the reg property of simplefb could be set to point to some mmio rather then >> just the framebuffer, and writing that mmio could do real bad things, yet we trust >> it not to do that. >> >> 2) The firmware we're talking about here, at least for now, is u-boot which >> is fully FOSS and under our control. >> >> 3) "I don't trust foo" is not a technical argument, and decisions like this >> should be made on technical arguments. > > You also realize that uboot now is going to have to be compiled with > the same DTS that the kernel is using so that the phandles will match > up. And now you are going to have to recompile your uboot everytime > that DTS changes. That is simply not true. Please stop spreading FUD about this without actually knowing how it works / having read the code. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt index 70c26f3..91176ee 100644 --- a/Documentation/devicetree/bindings/video/simple-framebuffer.txt +++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt @@ -1,8 +1,8 @@ Simple Framebuffer -A simple frame-buffer describes a raw memory region that may be rendered to, -with the assumption that the display hardware has already been set up to scan -out from that buffer. +A simple frame-buffer describes a frame-buffer setup by firmware or +the bootloader, with the assumption that the display hardware has already +been set up to scan out from the memory pointed to by the ref property. Required properties: - compatible: "simple-framebuffer" @@ -14,6 +14,9 @@ Required properties: - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b). - a8b8g8r8 (32-bit pixels, d[31:24]=a, d[23:16]=b, d[15:8]=g, d[7:0]=r). +Optional properties: +- clocks : List of clocks used by the framebuffer + Example: framebuffer {