| Message ID | 1407914239-12054-5-git-send-email-libv@skynet.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/13/2014 01:17 AM, Luc Verhaegen wrote: > This claims and enables clocks listed in the simple framebuffer dt node. > This is needed so that the display engine, in case the required clocks > are known by the kernel code and are described in the dt, will remain > properly enabled. I think this make simplefb not simple any more, which rather goes against the whole point of it. I specifically conceived simplefb to know about nothing more than the memory address and pixel layout of the memory buffer. I certainly don't like the idea of expanding simplefb to anything beyond that, and IIRC *not* extending is was a condition agreed when it was first merged. If more knowledge than that is required, then there needs to be a HW-specific driver to manage any clocks/resets/video registers, etc. The correct way to handle this without a complete DRM/KMS/... driver is to avoid the clocks in question being turned off at boot. I thought there was a per-clock flag to prevent disabling an unused clock? If not, perhaps the clock driver should force the clock to be enabled (perhaps only if the DRM/KMS driver isn't enabled?). For example, the Tegra clock driver has a clock initialization table which IIRC was used for this purpose before we got a DRM/KMS driver. That way, all the details are kept inside the kernel code, and don't end up influencing the DT representation of simplefb.
On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > > I think this make simplefb not simple any more, which rather goes > against the whole point of it. > > I specifically conceived simplefb to know about nothing more than the > memory address and pixel layout of the memory buffer. I certainly don't > like the idea of expanding simplefb to anything beyond that, and IIRC > *not* extending is was a condition agreed when it was first merged. If > more knowledge than that is required, then there needs to be a > HW-specific driver to manage any clocks/resets/video registers, etc. Yes. Simplefb quickly becomes anything but, doesn't it. Perhaps DenialFB would've been a better name for it ;p > The correct way to handle this without a complete DRM/KMS/... driver is > to avoid the clocks in question being turned off at boot. I thought > there was a per-clock flag to prevent disabling an unused clock? If not, > perhaps the clock driver should force the clock to be enabled (perhaps > only if the DRM/KMS driver isn't enabled?). For example, the Tegra clock > driver has a clock initialization table which IIRC was used for this > purpose before we got a DRM/KMS driver. That way, all the details are > kept inside the kernel code, and don't end up influencing the DT > representation of simplefb. How was simplefb handled on tegra? Where is the code for that? I didn't see anything in u-boot for instance. But the code for handling clocks where they are supposed to be handled is pretty generic from where i sit. Luc Verhaegen.
On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > On 08/13/2014 01:17 AM, Luc Verhaegen wrote: > >This claims and enables clocks listed in the simple framebuffer dt node. > >This is needed so that the display engine, in case the required clocks > >are known by the kernel code and are described in the dt, will remain > >properly enabled. > > I think this make simplefb not simple any more, which rather goes > against the whole point of it. > > I specifically conceived simplefb to know about nothing more than > the memory address and pixel layout of the memory buffer. I > certainly don't like the idea of expanding simplefb to anything > beyond that, and IIRC *not* extending is was a condition agreed when > it was first merged. If more knowledge than that is required, then > there needs to be a HW-specific driver to manage any > clocks/resets/video registers, etc. I'm sorry, but how is that not simple? clocks enabling is step 1 in a driver in order to communicate somehow with the controller. Reset is a different story, because arguably, if simplefb is there, the controller is already out of reset. And I don't see why video registers are coming into the discussion here. The code Luc posted doesn't access any register, at all. It just makes sure the needed controller keep going. > The correct way to handle this without a complete DRM/KMS/... driver > is to avoid the clocks in question being turned off at boot. Which is exactly what this code does, using the generic DT bindings to express dependency for a given clock. How is this wrong? > I thought there was a per-clock flag to prevent disabling an unused > clock? No, last time I heard, Mike Turquette was against it. > If not, perhaps the clock driver should force the clock to be > enabled (perhaps only if the DRM/KMS driver isn't enabled?). I'm sorry, but I'm not going to take any code that will do that in our clock driver. I'm not going to have a huge list of ifdef depending on configuration options to know which clock to enable, especially when clk_get should have the consumer device as an argument. > For example, the Tegra clock driver has a clock initialization table > which IIRC was used for this purpose before we got a DRM/KMS driver. > That way, all the details are kept inside the kernel code, and don't > end up influencing the DT representation of simplefb. I don't really see how the optional usage of a generic property influences badly the DT representation of simplefb. Maxime
Hi, On 08/13/2014 07:01 PM, Maxime Ripard wrote: > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >> On 08/13/2014 01:17 AM, Luc Verhaegen wrote: >>> This claims and enables clocks listed in the simple framebuffer dt node. >>> This is needed so that the display engine, in case the required clocks >>> are known by the kernel code and are described in the dt, will remain >>> properly enabled. >> >> I think this make simplefb not simple any more, which rather goes >> against the whole point of it. >> >> I specifically conceived simplefb to know about nothing more than >> the memory address and pixel layout of the memory buffer. I >> certainly don't like the idea of expanding simplefb to anything >> beyond that, and IIRC *not* extending is was a condition agreed when >> it was first merged. If more knowledge than that is required, then >> there needs to be a HW-specific driver to manage any >> clocks/resets/video registers, etc. > > I'm sorry, but how is that not simple? clocks enabling is step 1 in a > driver in order to communicate somehow with the controller. Reset is a > different story, because arguably, if simplefb is there, the > controller is already out of reset. > > And I don't see why video registers are coming into the discussion > here. The code Luc posted doesn't access any register, at all. It just > makes sure the needed controller keep going. > >> The correct way to handle this without a complete DRM/KMS/... driver >> is to avoid the clocks in question being turned off at boot. > > Which is exactly what this code does, using the generic DT bindings to > express dependency for a given clock. How is this wrong? > >> I thought there was a per-clock flag to prevent disabling an unused >> clock? > > No, last time I heard, Mike Turquette was against it. > >> If not, perhaps the clock driver should force the clock to be >> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > > I'm sorry, but I'm not going to take any code that will do that in our > clock driver. > > I'm not going to have a huge list of ifdef depending on configuration > options to know which clock to enable, especially when clk_get should > have the consumer device as an argument. > >> For example, the Tegra clock driver has a clock initialization table >> which IIRC was used for this purpose before we got a DRM/KMS driver. >> That way, all the details are kept inside the kernel code, and don't >> end up influencing the DT representation of simplefb. > > I don't really see how the optional usage of a generic property > influences badly the DT representation of simplefb. +1 to all that Maxime said. Also as can be seen in other discussion on this patch set, simplefb should not be seen as something orthogonal to having a full kms driver. So just write a full kms driver is not the answer IMHO. What we want is for a bootloader setup console to be available through simplefb bindings so that the kernel can show output without depending on module loading, and thus can show errors if things go bad before a kms driver gets loaded. And no build kms into the kernel is not the answer. We've all been working hard to be able to build more generic kernels, so as to get generic distro support. And generic distros will build kms as modules, as there are simply to many different kms drivers to build them all in. Regards, Hans
Op 14 aug. 2014, om 11:37 heeft Hans de Goede <hdegoede@redhat.com> het volgende geschreven: > Hi, > > On 08/13/2014 07:01 PM, Maxime Ripard wrote: >> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >>> On 08/13/2014 01:17 AM, Luc Verhaegen wrote: >>>> This claims and enables clocks listed in the simple framebuffer dt node. >>>> This is needed so that the display engine, in case the required clocks >>>> are known by the kernel code and are described in the dt, will remain >>>> properly enabled. >>> >>> I think this make simplefb not simple any more, which rather goes >>> against the whole point of it. >>> >>> I specifically conceived simplefb to know about nothing more than >>> the memory address and pixel layout of the memory buffer. I >>> certainly don't like the idea of expanding simplefb to anything >>> beyond that, and IIRC *not* extending is was a condition agreed when >>> it was first merged. If more knowledge than that is required, then >>> there needs to be a HW-specific driver to manage any >>> clocks/resets/video registers, etc. >> >> I'm sorry, but how is that not simple? clocks enabling is step 1 in a >> driver in order to communicate somehow with the controller. Reset is a >> different story, because arguably, if simplefb is there, the >> controller is already out of reset. >> >> And I don't see why video registers are coming into the discussion >> here. The code Luc posted doesn't access any register, at all. It just >> makes sure the needed controller keep going. >> >>> The correct way to handle this without a complete DRM/KMS/... driver >>> is to avoid the clocks in question being turned off at boot. >> >> Which is exactly what this code does, using the generic DT bindings to >> express dependency for a given clock. How is this wrong? >> >>> I thought there was a per-clock flag to prevent disabling an unused >>> clock? >> >> No, last time I heard, Mike Turquette was against it. >> >>> If not, perhaps the clock driver should force the clock to be >>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >> >> I'm sorry, but I'm not going to take any code that will do that in our >> clock driver. >> >> I'm not going to have a huge list of ifdef depending on configuration >> options to know which clock to enable, especially when clk_get should >> have the consumer device as an argument. >> >>> For example, the Tegra clock driver has a clock initialization table >>> which IIRC was used for this purpose before we got a DRM/KMS driver. >>> That way, all the details are kept inside the kernel code, and don't >>> end up influencing the DT representation of simplefb. >> >> I don't really see how the optional usage of a generic property >> influences badly the DT representation of simplefb. > > +1 to all that Maxime said. > > Also as can be seen in other discussion on this patch set, simplefb > should not be seen as something orthogonal to having a full kms driver. > > So just write a full kms driver is not the answer IMHO. What we want > is for a bootloader setup console to be available through simplefb > bindings so that the kernel can show output without depending on > module loading, and thus can show errors if things go bad before > a kms driver gets loaded. > > And no build kms into the kernel is not the answer. We've all been > working hard to be able to build more generic kernels, so as to get > generic distro support. And generic distros will build kms as modules, > as there are simply to many different kms drivers to build them all > in. How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel? I know from experience that it's not possible on x86 especially with efifb in the mix, but I wonder what the situation on ARM is. I only have TI, sunxi and exynos boards to test on and building in both TI drm drivers and the exynos one seems to work. Note that I'm not talking about the non-DRM abortions that maskerade as graphics drivers for ARM SoCs, only proper DRM ones. regards, Koen
On Thu, Aug 14, 2014 at 12:31:50PM +0200, Koen Kooi wrote: > > How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel? I know from experience that it's not possible on x86 especially with efifb in the mix, but I wonder what the situation on ARM is. I only have TI, sunxi and exynos boards to test on and building in both TI drm drivers and the exynos one seems to work. > Note that I'm not talking about the non-DRM abortions that maskerade as graphics drivers for ARM SoCs, only proper DRM ones. > > regards, > > Koen A quick check tells me that my current sunxi-3.4 sunxi_drm.ko measures 1.7MB, for currently about 8kloc (the binary seems excessively big though), which is just display. This still lacks many key features (lcd gpio, backlight pwm, lvds, ints), so it will grow ~1kloc still. Also, i constantly am re-loading this (how else does one develop any sizable amount of code?), so it does that cleanly and correctly, which seems quite contrary to your experience with ARM drm drivers. Luc Verhaegen.
Hi, On 08/14/2014 12:31 PM, Koen Kooi wrote: > > Op 14 aug. 2014, om 11:37 heeft Hans de Goede <hdegoede@redhat.com> het volgende geschreven: > >> Hi, >> >> On 08/13/2014 07:01 PM, Maxime Ripard wrote: >>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >>>> On 08/13/2014 01:17 AM, Luc Verhaegen wrote: >>>>> This claims and enables clocks listed in the simple framebuffer dt node. >>>>> This is needed so that the display engine, in case the required clocks >>>>> are known by the kernel code and are described in the dt, will remain >>>>> properly enabled. >>>> >>>> I think this make simplefb not simple any more, which rather goes >>>> against the whole point of it. >>>> >>>> I specifically conceived simplefb to know about nothing more than >>>> the memory address and pixel layout of the memory buffer. I >>>> certainly don't like the idea of expanding simplefb to anything >>>> beyond that, and IIRC *not* extending is was a condition agreed when >>>> it was first merged. If more knowledge than that is required, then >>>> there needs to be a HW-specific driver to manage any >>>> clocks/resets/video registers, etc. >>> >>> I'm sorry, but how is that not simple? clocks enabling is step 1 in a >>> driver in order to communicate somehow with the controller. Reset is a >>> different story, because arguably, if simplefb is there, the >>> controller is already out of reset. >>> >>> And I don't see why video registers are coming into the discussion >>> here. The code Luc posted doesn't access any register, at all. It just >>> makes sure the needed controller keep going. >>> >>>> The correct way to handle this without a complete DRM/KMS/... driver >>>> is to avoid the clocks in question being turned off at boot. >>> >>> Which is exactly what this code does, using the generic DT bindings to >>> express dependency for a given clock. How is this wrong? >>> >>>> I thought there was a per-clock flag to prevent disabling an unused >>>> clock? >>> >>> No, last time I heard, Mike Turquette was against it. >>> >>>> If not, perhaps the clock driver should force the clock to be >>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >>> >>> I'm sorry, but I'm not going to take any code that will do that in our >>> clock driver. >>> >>> I'm not going to have a huge list of ifdef depending on configuration >>> options to know which clock to enable, especially when clk_get should >>> have the consumer device as an argument. >>> >>>> For example, the Tegra clock driver has a clock initialization table >>>> which IIRC was used for this purpose before we got a DRM/KMS driver. >>>> That way, all the details are kept inside the kernel code, and don't >>>> end up influencing the DT representation of simplefb. >>> >>> I don't really see how the optional usage of a generic property >>> influences badly the DT representation of simplefb. >> >> +1 to all that Maxime said. >> >> Also as can be seen in other discussion on this patch set, simplefb >> should not be seen as something orthogonal to having a full kms driver. >> >> So just write a full kms driver is not the answer IMHO. What we want >> is for a bootloader setup console to be available through simplefb >> bindings so that the kernel can show output without depending on >> module loading, and thus can show errors if things go bad before >> a kms driver gets loaded. >> >> And no build kms into the kernel is not the answer. We've all been >> working hard to be able to build more generic kernels, so as to get >> generic distro support. And generic distros will build kms as modules, >> as there are simply to many different kms drivers to build them all >> in. > > How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel? I don't know how many we've today, but I do know that we will have twice as more next year, and 4x as more the year after that, etc. IOW this does not scale. Regards, Hans
Hi, On 08/14/2014 12:57 PM, Luc Verhaegen wrote: > On Thu, Aug 14, 2014 at 12:31:50PM +0200, Koen Kooi wrote: >> >> How many DRM drivers are there on ARM and what's the size impact of building them all into the kernel? I know from experience that it's not possible on x86 especially with efifb in the mix, but I wonder what the situation on ARM is. I only have TI, sunxi and exynos boards to test on and building in both TI drm drivers and the exynos one seems to work. >> Note that I'm not talking about the non-DRM abortions that maskerade as graphics drivers for ARM SoCs, only proper DRM ones. >> >> regards, >> >> Koen > > A quick check tells me that my current sunxi-3.4 sunxi_drm.ko measures > 1.7MB That likely is with debug-info, try running "strip --strip-debug" on the .ko file. Note don't use plain "strip" that ruins kernel modules. Regards, Hans p.s. I'm leaving home (and the internet) for vacation tomorrow. I'll be back on Mon Aug 25.
On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: [...] > > If not, perhaps the clock driver should force the clock to be > > enabled (perhaps only if the DRM/KMS driver isn't enabled?). > > I'm sorry, but I'm not going to take any code that will do that in our > clock driver. > > I'm not going to have a huge list of ifdef depending on configuration > options to know which clock to enable, especially when clk_get should > have the consumer device as an argument. Are you saying is that you want to solve a platform-specific problem by pushing code into simple, generic drivers so that your platform code can stay "clean"? Thierry
On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > [...] > > > If not, perhaps the clock driver should force the clock to be > > > enabled (perhaps only if the DRM/KMS driver isn't enabled?). > > > > I'm sorry, but I'm not going to take any code that will do that in our > > clock driver. > > > > I'm not going to have a huge list of ifdef depending on configuration > > options to know which clock to enable, especially when clk_get should > > have the consumer device as an argument. > > Are you saying is that you want to solve a platform-specific problem by > pushing code into simple, generic drivers so that your platform code can > stay "clean"? Are you saying that this driver would become "dirty" with such a patch? If so, we really have an issue in the kernel.
On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > > On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > > > On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > > [...] > > > > If not, perhaps the clock driver should force the clock to be > > > > enabled (perhaps only if the DRM/KMS driver isn't enabled?). > > > > > > I'm sorry, but I'm not going to take any code that will do that in our > > > clock driver. > > > > > > I'm not going to have a huge list of ifdef depending on configuration > > > options to know which clock to enable, especially when clk_get should > > > have the consumer device as an argument. > > > > Are you saying is that you want to solve a platform-specific problem by > > pushing code into simple, generic drivers so that your platform code can > > stay "clean"? > > Are you saying that this driver would become "dirty" with such a patch? Yes. Others have said the same and even provided alternative solutions on how to solve what's seemingly a platform-specific problem in a platform-specific way. > If so, we really have an issue in the kernel. Can you elaborate? Thierry
Hi, On 08/25/2014 03:39 PM, Thierry Reding wrote: > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >>> [...] >>>>> If not, perhaps the clock driver should force the clock to be >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >>>> >>>> I'm sorry, but I'm not going to take any code that will do that in our >>>> clock driver. >>>> >>>> I'm not going to have a huge list of ifdef depending on configuration >>>> options to know which clock to enable, especially when clk_get should >>>> have the consumer device as an argument. >>> >>> Are you saying is that you want to solve a platform-specific problem by >>> pushing code into simple, generic drivers so that your platform code can >>> stay "clean"? >> >> Are you saying that this driver would become "dirty" with such a patch? > > Yes. Others have said the same and even provided alternative solutions > on how to solve what's seemingly a platform-specific problem in a > platform-specific way. This is not platform specific, any platform with a complete clock driver will suffer from the same problem (the clock driver disabling unclaimed ahb gates, and thus killing the video output) if it wants to use simplefb for early console support. I can only assume that this problem was never hit on tegra because when kms support (and thus also a clock driver for the video plls) was introduced simplefb support was dropped at the same time, or the gates are not being disabled for some other reason. As for the suggestion to simply never disable the plls / ahb gates by blocking them from ever being disabled in the sunxi clock driver, that is not really a solution either, as we want to be able to turn these things off to safe power on screen blank once control has been turned over to the kms driver. And while at it let me also tackle the don't use simplefb only use kms argument, that means that the clocks will be turned off until the kms module loads, which will cause noticable screen flicker / video output resync, something which we've been trying to get rid of for years now. And no, build in the kms driver is not an answer either. That works nicely for firmware, but not for generic Linux distributions supporting a wide range of boards. Regards, Hans
On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: > On 08/25/2014 03:39 PM, Thierry Reding wrote: > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > >>> [...] > >>>>> If not, perhaps the clock driver should force the clock to be > >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > >>>> > >>>> I'm sorry, but I'm not going to take any code that will do that in our > >>>> clock driver. > >>>> > >>>> I'm not going to have a huge list of ifdef depending on configuration > >>>> options to know which clock to enable, especially when clk_get should > >>>> have the consumer device as an argument. > >>> > >>> Are you saying is that you want to solve a platform-specific problem by > >>> pushing code into simple, generic drivers so that your platform code can > >>> stay "clean"? > >> > >> Are you saying that this driver would become "dirty" with such a patch? > > > > Yes. Others have said the same and even provided alternative solutions > > on how to solve what's seemingly a platform-specific problem in a > > platform-specific way. > > This is not platform specific, any platform with a complete clock driver > will suffer from the same problem (the clock driver disabling unclaimed > ahb gates, and thus killing the video output) if it wants to use simplefb > for early console support. It is platform specific in that your platform may require certain clocks to remain on. The next platform may require power domains to remain on during boot and yet another one may rely on regulators to stay on during boot. By your argument simplefb will need to be taught to handle pretty much every type of resource that the kernel has. > As for the suggestion to simply never disable the plls / ahb gates by blocking > them from ever being disabled in the sunxi clock driver, that is not really > a solution either, as we want to be able to turn these things off to safe > power on screen blank once control has been turned over to the kms driver. Then perhaps part of the hand-off procedure between simplefb and DRM/KMS should involve marking PLLs or "gates" as properly managed. > And while at it let me also tackle the don't use simplefb only use kms argument, > that means that the clocks will be turned off until the kms module loads, which > will cause noticable screen flicker / video output resync, something which we've > been trying to get rid of for years now. > > And no, build in the kms driver is not an answer either. That works nicely for > firmware, but not for generic Linux distributions supporting a wide range > of boards. Odd... I didn't offer any of those two as solutions to the problem. Thierry
On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: >> On 08/25/2014 03:39 PM, Thierry Reding wrote: >> > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: >> >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: >> >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: >> >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >> >>> [...] >> >>>>> If not, perhaps the clock driver should force the clock to be >> >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >> >>>> >> >>>> I'm sorry, but I'm not going to take any code that will do that in our >> >>>> clock driver. >> >>>> >> >>>> I'm not going to have a huge list of ifdef depending on configuration >> >>>> options to know which clock to enable, especially when clk_get should >> >>>> have the consumer device as an argument. >> >>> >> >>> Are you saying is that you want to solve a platform-specific problem by >> >>> pushing code into simple, generic drivers so that your platform code can >> >>> stay "clean"? >> >> >> >> Are you saying that this driver would become "dirty" with such a patch? >> > >> > Yes. Others have said the same and even provided alternative solutions >> > on how to solve what's seemingly a platform-specific problem in a >> > platform-specific way. >> >> This is not platform specific, any platform with a complete clock driver >> will suffer from the same problem (the clock driver disabling unclaimed >> ahb gates, and thus killing the video output) if it wants to use simplefb >> for early console support. > > It is platform specific in that your platform may require certain clocks > to remain on. The next platform may require power domains to remain on > during boot and yet another one may rely on regulators to stay on during > boot. By your argument simplefb will need to be taught to handle pretty > much every type of resource that the kernel has. Why can't simplefb be a driver library that is called from a device specific device driver that only claims the clocks (or regulators)? Then build all of these device specific drivers into the generic ARM kernel. They will be quite small since all they do is claim the clocks (or regulator). Maybe we can even figure out some protocol for removing the unused ones from memory later. Later during the boot process the device specific driver can load its KMS code which has also been implemented as a driver library. Maybe use E_PROBE_DEFER to do this. Match on the device ID, claim the clocks, defer until the full KMS library can be loaded. > >> As for the suggestion to simply never disable the plls / ahb gates by blocking >> them from ever being disabled in the sunxi clock driver, that is not really >> a solution either, as we want to be able to turn these things off to safe >> power on screen blank once control has been turned over to the kms driver. > > Then perhaps part of the hand-off procedure between simplefb and DRM/KMS > should involve marking PLLs or "gates" as properly managed. > >> And while at it let me also tackle the don't use simplefb only use kms argument, >> that means that the clocks will be turned off until the kms module loads, which >> will cause noticable screen flicker / video output resync, something which we've >> been trying to get rid of for years now. >> >> And no, build in the kms driver is not an answer either. That works nicely for >> firmware, but not for generic Linux distributions supporting a wide range >> of boards. > > Odd... I didn't offer any of those two as solutions to the problem. > > Thierry
Hi, On 08/25/2014 04:16 PM, Thierry Reding wrote: > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: >> On 08/25/2014 03:39 PM, Thierry Reding wrote: >>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: >>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: >>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: >>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >>>>> [...] >>>>>>> If not, perhaps the clock driver should force the clock to be >>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >>>>>> >>>>>> I'm sorry, but I'm not going to take any code that will do that in our >>>>>> clock driver. >>>>>> >>>>>> I'm not going to have a huge list of ifdef depending on configuration >>>>>> options to know which clock to enable, especially when clk_get should >>>>>> have the consumer device as an argument. >>>>> >>>>> Are you saying is that you want to solve a platform-specific problem by >>>>> pushing code into simple, generic drivers so that your platform code can >>>>> stay "clean"? >>>> >>>> Are you saying that this driver would become "dirty" with such a patch? >>> >>> Yes. Others have said the same and even provided alternative solutions >>> on how to solve what's seemingly a platform-specific problem in a >>> platform-specific way. >> >> This is not platform specific, any platform with a complete clock driver >> will suffer from the same problem (the clock driver disabling unclaimed >> ahb gates, and thus killing the video output) if it wants to use simplefb >> for early console support. > > It is platform specific in that your platform may require certain clocks > to remain on. The next platform may require power domains to remain on > during boot and yet another one may rely on regulators to stay on during > boot. By your argument simplefb will need to be taught to handle pretty > much every type of resource that the kernel has. > >> As for the suggestion to simply never disable the plls / ahb gates by blocking >> them from ever being disabled in the sunxi clock driver, that is not really >> a solution either, as we want to be able to turn these things off to safe >> power on screen blank once control has been turned over to the kms driver. > > Then perhaps part of the hand-off procedure between simplefb and DRM/KMS > should involve marking PLLs or "gates" as properly managed. And by your earlier argument also power domains, regulators, etc. So now we need to add code to each of the clock core, power-domain core, regulator core, etc. to have them now about this initially unmanaged state thing you're introducing, as well as modify all involved clock / regulator / etc. drivers to mark certain resources as unmanaged. Or we add a single simple and clean patch to the simplefb driver for dealing with clocks, and worry about all the other hypothetical problems later... Regards, Hans
Hi, On 08/25/2014 04:23 PM, jonsmirl@gmail.com wrote: > On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: >> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: >>> On 08/25/2014 03:39 PM, Thierry Reding wrote: >>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: >>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: >>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: >>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >>>>>> [...] >>>>>>>> If not, perhaps the clock driver should force the clock to be >>>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >>>>>>> >>>>>>> I'm sorry, but I'm not going to take any code that will do that in our >>>>>>> clock driver. >>>>>>> >>>>>>> I'm not going to have a huge list of ifdef depending on configuration >>>>>>> options to know which clock to enable, especially when clk_get should >>>>>>> have the consumer device as an argument. >>>>>> >>>>>> Are you saying is that you want to solve a platform-specific problem by >>>>>> pushing code into simple, generic drivers so that your platform code can >>>>>> stay "clean"? >>>>> >>>>> Are you saying that this driver would become "dirty" with such a patch? >>>> >>>> Yes. Others have said the same and even provided alternative solutions >>>> on how to solve what's seemingly a platform-specific problem in a >>>> platform-specific way. >>> >>> This is not platform specific, any platform with a complete clock driver >>> will suffer from the same problem (the clock driver disabling unclaimed >>> ahb gates, and thus killing the video output) if it wants to use simplefb >>> for early console support. >> >> It is platform specific in that your platform may require certain clocks >> to remain on. The next platform may require power domains to remain on >> during boot and yet another one may rely on regulators to stay on during >> boot. By your argument simplefb will need to be taught to handle pretty >> much every type of resource that the kernel has. > > Why can't simplefb be a driver library that is called from a device > specific device driver that only claims the clocks (or regulators)? > Then build all of these device specific drivers into the generic ARM > kernel. They will be quite small since all they do is claim the clocks > (or regulator). Maybe we can even figure out some protocol for > removing the unused ones from memory later. > > Later during the boot process the device specific driver can load its > KMS code which has also been implemented as a driver library. Maybe > use E_PROBE_DEFER to do this. Match on the device ID, claim the > clocks, defer until the full KMS library can be loaded. There is no need for all this complexity, all that is needed is for the simplefb driver to be thought to claim + enable any clocks listed in its dt node. Then once we want to do a handover, all is needed is a single simplefb unregister call, at which point simplefb will disable the clocks and release them. Note that this will be a nop as they should already be claimed and enabled by the kms driver at this time. Regards, Hans
On Mon, Aug 25, 2014 at 04:23:59PM +0200, Hans de Goede wrote: > Hi, > > On 08/25/2014 04:16 PM, Thierry Reding wrote: > > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: > >> On 08/25/2014 03:39 PM, Thierry Reding wrote: > >>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > >>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > >>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > >>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > >>>>> [...] > >>>>>>> If not, perhaps the clock driver should force the clock to be > >>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > >>>>>> > >>>>>> I'm sorry, but I'm not going to take any code that will do that in our > >>>>>> clock driver. > >>>>>> > >>>>>> I'm not going to have a huge list of ifdef depending on configuration > >>>>>> options to know which clock to enable, especially when clk_get should > >>>>>> have the consumer device as an argument. > >>>>> > >>>>> Are you saying is that you want to solve a platform-specific problem by > >>>>> pushing code into simple, generic drivers so that your platform code can > >>>>> stay "clean"? > >>>> > >>>> Are you saying that this driver would become "dirty" with such a patch? > >>> > >>> Yes. Others have said the same and even provided alternative solutions > >>> on how to solve what's seemingly a platform-specific problem in a > >>> platform-specific way. > >> > >> This is not platform specific, any platform with a complete clock driver > >> will suffer from the same problem (the clock driver disabling unclaimed > >> ahb gates, and thus killing the video output) if it wants to use simplefb > >> for early console support. > > > > It is platform specific in that your platform may require certain clocks > > to remain on. The next platform may require power domains to remain on > > during boot and yet another one may rely on regulators to stay on during > > boot. By your argument simplefb will need to be taught to handle pretty > > much every type of resource that the kernel has. > > > >> As for the suggestion to simply never disable the plls / ahb gates by blocking > >> them from ever being disabled in the sunxi clock driver, that is not really > >> a solution either, as we want to be able to turn these things off to safe > >> power on screen blank once control has been turned over to the kms driver. > > > > Then perhaps part of the hand-off procedure between simplefb and DRM/KMS > > should involve marking PLLs or "gates" as properly managed. > > And by your earlier argument also power domains, regulators, etc. So now we need > to add code to each of the clock core, power-domain core, regulator core, etc. to > have them now about this initially unmanaged state thing you're introducing, as > well as modify all involved clock / regulator / etc. drivers to mark certain > resources as unmanaged. Hmm... that's true. But we already have a way to deal with exactly this situation for regulators. There's a property called regulator-boot-on which a bootloader should set whet it has enabled a given regulator. It can of course also be set statically in a DTS if it's know upfront that a bootloader will always enable it. Perhaps what we need is a similar property for clocks so that the clock framework will not inadvertently turn off a clock that's still being used. Thierry
On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote: > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: > > On 08/25/2014 03:39 PM, Thierry Reding wrote: > > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > > >>> [...] > > >>>>> If not, perhaps the clock driver should force the clock to be > > >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > > >>>> > > >>>> I'm sorry, but I'm not going to take any code that will do that in our > > >>>> clock driver. > > >>>> > > >>>> I'm not going to have a huge list of ifdef depending on configuration > > >>>> options to know which clock to enable, especially when clk_get should > > >>>> have the consumer device as an argument. > > >>> > > >>> Are you saying is that you want to solve a platform-specific problem by > > >>> pushing code into simple, generic drivers so that your platform code can > > >>> stay "clean"? > > >> > > >> Are you saying that this driver would become "dirty" with such a patch? > > > > > > Yes. Others have said the same and even provided alternative solutions > > > on how to solve what's seemingly a platform-specific problem in a > > > platform-specific way. > > > > This is not platform specific, any platform with a complete clock driver > > will suffer from the same problem (the clock driver disabling unclaimed > > ahb gates, and thus killing the video output) if it wants to use simplefb > > for early console support. > > It is platform specific in that your platform may require certain clocks > to remain on. The platform doesn't. simplefb does. simplefb is the obvious consumer for these clocks, and given the current API and abstraction we have, it should be the one claiming the clocks too. > The next platform may require power domains to remain on during boot > and yet another one may rely on regulators to stay on during > boot. By your argument simplefb will need to be taught to handle > pretty much every type of resource that the kernel has. And I wouldn't find anything wrong with that. We're already doing so for any generic driver in the kernel (AHCI, EHCI comes to my mind first, there's probably a lot of others). Why wouldn't we do as such for this one?
On Mon, Aug 25, 2014 at 10:23:26AM -0400, jonsmirl@gmail.com wrote: > On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: > > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: > >> On 08/25/2014 03:39 PM, Thierry Reding wrote: > >> > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > >> >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > >> >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > >> >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > >> >>> [...] > >> >>>>> If not, perhaps the clock driver should force the clock to be > >> >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > >> >>>> > >> >>>> I'm sorry, but I'm not going to take any code that will do that in our > >> >>>> clock driver. > >> >>>> > >> >>>> I'm not going to have a huge list of ifdef depending on configuration > >> >>>> options to know which clock to enable, especially when clk_get should > >> >>>> have the consumer device as an argument. > >> >>> > >> >>> Are you saying is that you want to solve a platform-specific problem by > >> >>> pushing code into simple, generic drivers so that your platform code can > >> >>> stay "clean"? > >> >> > >> >> Are you saying that this driver would become "dirty" with such a patch? > >> > > >> > Yes. Others have said the same and even provided alternative solutions > >> > on how to solve what's seemingly a platform-specific problem in a > >> > platform-specific way. > >> > >> This is not platform specific, any platform with a complete clock driver > >> will suffer from the same problem (the clock driver disabling unclaimed > >> ahb gates, and thus killing the video output) if it wants to use simplefb > >> for early console support. > > > > It is platform specific in that your platform may require certain clocks > > to remain on. The next platform may require power domains to remain on > > during boot and yet another one may rely on regulators to stay on during > > boot. By your argument simplefb will need to be taught to handle pretty > > much every type of resource that the kernel has. > > Why can't simplefb be a driver library that is called from a device > specific device driver that only claims the clocks (or regulators)? > Then build all of these device specific drivers into the generic ARM > kernel. They will be quite small since all they do is claim the clocks > (or regulator). Maybe we can even figure out some protocol for > removing the unused ones from memory later. > > Later during the boot process the device specific driver can load its > KMS code which has also been implemented as a driver library. Maybe > use E_PROBE_DEFER to do this. Match on the device ID, claim the > clocks, defer until the full KMS library can be loaded. That sounds like the most scalable solution so far. On the other hand, as I understand it, the simplefb driver was designed to take over the framebuffer set up by firmware, so it's somewhat odd that the driver would have to deal with resources in the first place. If we push the resource problem into the respective subsystems we keep the simplefb driver completely hardware agnostic. And we'll also be solving this problem for other types of drivers at the same time. Firmware may after all initialize clocks and other resources for other types of devices too. Handling resources in the drivers would therefore imply that every driver needs to cope with this. Thierry
On Mon, Aug 25, 2014 at 04:58:54PM +0200, Maxime Ripard wrote: > On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote: > > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: > > > On 08/25/2014 03:39 PM, Thierry Reding wrote: > > > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > > > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > > > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > > > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > > > >>> [...] > > > >>>>> If not, perhaps the clock driver should force the clock to be > > > >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > > > >>>> > > > >>>> I'm sorry, but I'm not going to take any code that will do that in our > > > >>>> clock driver. > > > >>>> > > > >>>> I'm not going to have a huge list of ifdef depending on configuration > > > >>>> options to know which clock to enable, especially when clk_get should > > > >>>> have the consumer device as an argument. > > > >>> > > > >>> Are you saying is that you want to solve a platform-specific problem by > > > >>> pushing code into simple, generic drivers so that your platform code can > > > >>> stay "clean"? > > > >> > > > >> Are you saying that this driver would become "dirty" with such a patch? > > > > > > > > Yes. Others have said the same and even provided alternative solutions > > > > on how to solve what's seemingly a platform-specific problem in a > > > > platform-specific way. > > > > > > This is not platform specific, any platform with a complete clock driver > > > will suffer from the same problem (the clock driver disabling unclaimed > > > ahb gates, and thus killing the video output) if it wants to use simplefb > > > for early console support. > > > > It is platform specific in that your platform may require certain clocks > > to remain on. > > The platform doesn't. simplefb does. simplefb is the obvious consumer > for these clocks, and given the current API and abstraction we have, > it should be the one claiming the clocks too. No. simplefb just wants to write to some memory that hardware has been set up to scan out. The platform requires that the clocks be on. Other platforms may not even allow turning off the clocks. > > The next platform may require power domains to remain on during boot > > and yet another one may rely on regulators to stay on during > > boot. By your argument simplefb will need to be taught to handle > > pretty much every type of resource that the kernel has. > > And I wouldn't find anything wrong with that. We're already doing so > for any generic driver in the kernel (AHCI, EHCI comes to my mind > first, there's probably a lot of others). Why wouldn't we do as such > for this one? Yes, and we've had similar discussions in those subsystems too. Thierry
On Mon, Aug 25, 2014 at 04:53:06PM +0200, Thierry Reding wrote: > Hmm... that's true. But we already have a way to deal with exactly this > situation for regulators. There's a property called regulator-boot-on > which a bootloader should set whet it has enabled a given regulator. It > can of course also be set statically in a DTS if it's know upfront that > a bootloader will always enable it. Perhaps what we need is a similar > property for clocks so that the clock framework will not inadvertently > turn off a clock that's still being used. Except that such a property won't work either. Regulators with regulator-boot-on will still be disabled if there's no one to claim it. Just like what happens currently for the clocks. Maxime
On Mon, Aug 25, 2014 at 10:53 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Aug 25, 2014 at 04:23:59PM +0200, Hans de Goede wrote: >> Hi, >> >> On 08/25/2014 04:16 PM, Thierry Reding wrote: >> > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: >> >> On 08/25/2014 03:39 PM, Thierry Reding wrote: >> >>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: >> >>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: >> >>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: >> >>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >> >>>>> [...] >> >>>>>>> If not, perhaps the clock driver should force the clock to be >> >>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >> >>>>>> >> >>>>>> I'm sorry, but I'm not going to take any code that will do that in our >> >>>>>> clock driver. >> >>>>>> >> >>>>>> I'm not going to have a huge list of ifdef depending on configuration >> >>>>>> options to know which clock to enable, especially when clk_get should >> >>>>>> have the consumer device as an argument. >> >>>>> >> >>>>> Are you saying is that you want to solve a platform-specific problem by >> >>>>> pushing code into simple, generic drivers so that your platform code can >> >>>>> stay "clean"? >> >>>> >> >>>> Are you saying that this driver would become "dirty" with such a patch? >> >>> >> >>> Yes. Others have said the same and even provided alternative solutions >> >>> on how to solve what's seemingly a platform-specific problem in a >> >>> platform-specific way. >> >> >> >> This is not platform specific, any platform with a complete clock driver >> >> will suffer from the same problem (the clock driver disabling unclaimed >> >> ahb gates, and thus killing the video output) if it wants to use simplefb >> >> for early console support. >> > >> > It is platform specific in that your platform may require certain clocks >> > to remain on. The next platform may require power domains to remain on >> > during boot and yet another one may rely on regulators to stay on during >> > boot. By your argument simplefb will need to be taught to handle pretty >> > much every type of resource that the kernel has. >> > >> >> As for the suggestion to simply never disable the plls / ahb gates by blocking >> >> them from ever being disabled in the sunxi clock driver, that is not really >> >> a solution either, as we want to be able to turn these things off to safe >> >> power on screen blank once control has been turned over to the kms driver. >> > >> > Then perhaps part of the hand-off procedure between simplefb and DRM/KMS >> > should involve marking PLLs or "gates" as properly managed. >> >> And by your earlier argument also power domains, regulators, etc. So now we need >> to add code to each of the clock core, power-domain core, regulator core, etc. to >> have them now about this initially unmanaged state thing you're introducing, as >> well as modify all involved clock / regulator / etc. drivers to mark certain >> resources as unmanaged. > > Hmm... that's true. But we already have a way to deal with exactly this > situation for regulators. There's a property called regulator-boot-on > which a bootloader should set whet it has enabled a given regulator. It > can of course also be set statically in a DTS if it's know upfront that > a bootloader will always enable it. Perhaps what we need is a similar > property for clocks so that the clock framework will not inadvertently > turn off a clock that's still being used. There should probably be a generic 'boot-initialized;' property that can be added to any DT device node. Then uboot can add that property to the device node for anything it has turned on. You could even use it to add more info 'boot-initialized = <"9600 8 N 1">;' That passes the info in an OS agnostic manner. > > Thierry
On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote: > > No. simplefb just wants to write to some memory that hardware has been > set up to scan out. The platform requires that the clocks be on. Simplefb also requires that the memory is there and is persistent. Fine for discrete graphics cards, fine for rpi where most things are hidden from the ARM core anyway, not so fine for anybody else. Luc Verhaegen.
On Mon, Aug 25, 2014 at 04:27:04PM +0200, Hans de Goede wrote: > Hi, > > On 08/25/2014 04:23 PM, jonsmirl@gmail.com wrote: > > On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding > > <thierry.reding@gmail.com> wrote: > >> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: > >>> On 08/25/2014 03:39 PM, Thierry Reding wrote: > >>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > >>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > >>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > >>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > >>>>>> [...] > >>>>>>>> If not, perhaps the clock driver should force the clock to be > >>>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > >>>>>>> > >>>>>>> I'm sorry, but I'm not going to take any code that will do that in our > >>>>>>> clock driver. > >>>>>>> > >>>>>>> I'm not going to have a huge list of ifdef depending on configuration > >>>>>>> options to know which clock to enable, especially when clk_get should > >>>>>>> have the consumer device as an argument. > >>>>>> > >>>>>> Are you saying is that you want to solve a platform-specific problem by > >>>>>> pushing code into simple, generic drivers so that your platform code can > >>>>>> stay "clean"? > >>>>> > >>>>> Are you saying that this driver would become "dirty" with such a patch? > >>>> > >>>> Yes. Others have said the same and even provided alternative solutions > >>>> on how to solve what's seemingly a platform-specific problem in a > >>>> platform-specific way. > >>> > >>> This is not platform specific, any platform with a complete clock driver > >>> will suffer from the same problem (the clock driver disabling unclaimed > >>> ahb gates, and thus killing the video output) if it wants to use simplefb > >>> for early console support. > >> > >> It is platform specific in that your platform may require certain clocks > >> to remain on. The next platform may require power domains to remain on > >> during boot and yet another one may rely on regulators to stay on during > >> boot. By your argument simplefb will need to be taught to handle pretty > >> much every type of resource that the kernel has. > > > > Why can't simplefb be a driver library that is called from a device > > specific device driver that only claims the clocks (or regulators)? > > Then build all of these device specific drivers into the generic ARM > > kernel. They will be quite small since all they do is claim the clocks > > (or regulator). Maybe we can even figure out some protocol for > > removing the unused ones from memory later. > > > > Later during the boot process the device specific driver can load its > > KMS code which has also been implemented as a driver library. Maybe > > use E_PROBE_DEFER to do this. Match on the device ID, claim the > > clocks, defer until the full KMS library can be loaded. > > There is no need for all this complexity, all that is needed is for the > simplefb driver to be thought to claim + enable any clocks listed in > its dt node. Out of curiosity, how does this work in practice? How does the bootloader create this entry? Does it scan the DT to see which clocks the real hardware device references and then simply copies them to the simplefb node? Thierry
On Mon, Aug 25, 2014 at 05:12:58PM +0200, Thierry Reding wrote: > > Out of curiosity, how does this work in practice? How does the > bootloader create this entry? Does it scan the DT to see which clocks > the real hardware device references and then simply copies them to the > simplefb node? > > Thierry https://www.mail-archive.com/linux-sunxi@googlegroups.com/msg06619.html Luc Verhaegen.
On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote: > On Mon, Aug 25, 2014 at 04:58:54PM +0200, Maxime Ripard wrote: > > On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote: > > > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: > > > > On 08/25/2014 03:39 PM, Thierry Reding wrote: > > > > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > > > > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > > > > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > > > > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > > > > >>> [...] > > > > >>>>> If not, perhaps the clock driver should force the clock to be > > > > >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > > > > >>>> > > > > >>>> I'm sorry, but I'm not going to take any code that will do that in our > > > > >>>> clock driver. > > > > >>>> > > > > >>>> I'm not going to have a huge list of ifdef depending on configuration > > > > >>>> options to know which clock to enable, especially when clk_get should > > > > >>>> have the consumer device as an argument. > > > > >>> > > > > >>> Are you saying is that you want to solve a platform-specific problem by > > > > >>> pushing code into simple, generic drivers so that your platform code can > > > > >>> stay "clean"? > > > > >> > > > > >> Are you saying that this driver would become "dirty" with such a patch? > > > > > > > > > > Yes. Others have said the same and even provided alternative solutions > > > > > on how to solve what's seemingly a platform-specific problem in a > > > > > platform-specific way. > > > > > > > > This is not platform specific, any platform with a complete clock driver > > > > will suffer from the same problem (the clock driver disabling unclaimed > > > > ahb gates, and thus killing the video output) if it wants to use simplefb > > > > for early console support. > > > > > > It is platform specific in that your platform may require certain clocks > > > to remain on. > > > > The platform doesn't. simplefb does. simplefb is the obvious consumer > > for these clocks, and given the current API and abstraction we have, > > it should be the one claiming the clocks too. > > No. simplefb just wants to write to some memory that hardware has been > set up to scan out. The platform requires that the clocks be on. Other > platforms may not even allow turning off the clocks. Like what? the rpi? Come on. Just because the videocore is some black box we know nothing about doesn't mean we should use it as an example. Any decent enough SoC, with a decent support in the kernel will have clocks for this, and I really wonder how simplefb will behave once its clocks will be turned off... > > > The next platform may require power domains to remain on during boot > > > and yet another one may rely on regulators to stay on during > > > boot. By your argument simplefb will need to be taught to handle > > > pretty much every type of resource that the kernel has. > > > > And I wouldn't find anything wrong with that. We're already doing so > > for any generic driver in the kernel (AHCI, EHCI comes to my mind > > first, there's probably a lot of others). Why wouldn't we do as such > > for this one? > > Yes, and we've had similar discussions in those subsystems too. Similar discussion, with different outcomes it seems. Maxime
Hi, Am 25.08.2014 15:47, schrieb Hans de Goede: > On 08/25/2014 03:39 PM, Thierry Reding wrote: >> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: >>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: >>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: >>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >>>>>> If not, perhaps the clock driver should force the clock to be >>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). [...] > As for the suggestion to simply never disable the plls / ahb gates by blocking > them from ever being disabled in the sunxi clock driver, that is not really > a solution either, as we want to be able to turn these things off to safe > power on screen blank once control has been turned over to the kms driver. Without wanting to take sides on the simplefb matter, can't you just use clk_ignore_unused in bootargs to work around the issue at hand? That's what I do on the Spring Chromebook. Cheers, Andreas
On Mon, Aug 25, 2014 at 11:12 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Aug 25, 2014 at 04:27:04PM +0200, Hans de Goede wrote: >> Hi, >> >> On 08/25/2014 04:23 PM, jonsmirl@gmail.com wrote: >> > On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding >> > <thierry.reding@gmail.com> wrote: >> >> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: >> >>> On 08/25/2014 03:39 PM, Thierry Reding wrote: >> >>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: >> >>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: >> >>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: >> >>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >> >>>>>> [...] >> >>>>>>>> If not, perhaps the clock driver should force the clock to be >> >>>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >> >>>>>>> >> >>>>>>> I'm sorry, but I'm not going to take any code that will do that in our >> >>>>>>> clock driver. >> >>>>>>> >> >>>>>>> I'm not going to have a huge list of ifdef depending on configuration >> >>>>>>> options to know which clock to enable, especially when clk_get should >> >>>>>>> have the consumer device as an argument. >> >>>>>> >> >>>>>> Are you saying is that you want to solve a platform-specific problem by >> >>>>>> pushing code into simple, generic drivers so that your platform code can >> >>>>>> stay "clean"? >> >>>>> >> >>>>> Are you saying that this driver would become "dirty" with such a patch? >> >>>> >> >>>> Yes. Others have said the same and even provided alternative solutions >> >>>> on how to solve what's seemingly a platform-specific problem in a >> >>>> platform-specific way. >> >>> >> >>> This is not platform specific, any platform with a complete clock driver >> >>> will suffer from the same problem (the clock driver disabling unclaimed >> >>> ahb gates, and thus killing the video output) if it wants to use simplefb >> >>> for early console support. >> >> >> >> It is platform specific in that your platform may require certain clocks >> >> to remain on. The next platform may require power domains to remain on >> >> during boot and yet another one may rely on regulators to stay on during >> >> boot. By your argument simplefb will need to be taught to handle pretty >> >> much every type of resource that the kernel has. >> > >> > Why can't simplefb be a driver library that is called from a device >> > specific device driver that only claims the clocks (or regulators)? >> > Then build all of these device specific drivers into the generic ARM >> > kernel. They will be quite small since all they do is claim the clocks >> > (or regulator). Maybe we can even figure out some protocol for >> > removing the unused ones from memory later. >> > >> > Later during the boot process the device specific driver can load its >> > KMS code which has also been implemented as a driver library. Maybe >> > use E_PROBE_DEFER to do this. Match on the device ID, claim the >> > clocks, defer until the full KMS library can be loaded. >> >> There is no need for all this complexity, all that is needed is for the >> simplefb driver to be thought to claim + enable any clocks listed in >> its dt node. > > Out of curiosity, how does this work in practice? How does the > bootloader create this entry? Does it scan the DT to see which clocks > the real hardware device references and then simply copies them to the > simplefb node? That's why this is such a problem. There is a general rule in Linux - one device, one driver. All of the kernel device driver code is written around this assumption. We're trying to make two drivers for one device and the kernel is not designed to support that. There shouldn't be an independent 'simplefb' node. That is creating two device descriptions for a single device. Adding 'chosen' information to the devices nodes might work. video@1345 { compatible = "sunxi-a20-video"; reg = <...>; clocks = <...>; chosen { compatible = "simplefb"; // dynamically added by uboot buffer = <0x4564>; // dynamically added by uboot }; }; This will initialize the simplefb driver without attaching it to any specific hardware. It can then look in its parent node for clocks and regulators and claim them. > > Thierry
On Mon, Aug 25, 2014 at 05:09:00PM +0200, Luc Verhaegen wrote: > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote: > > > > No. simplefb just wants to write to some memory that hardware has been > > set up to scan out. The platform requires that the clocks be on. > > Simplefb also requires that the memory is there and is persistent. Fine > for discrete graphics cards, fine for rpi where most things are hidden > from the ARM core anyway, not so fine for anybody else. I don't understand. This patch series isn't changing anything about the memory aspects of the driver yet it's working for you on sunxi, isn't it? So it can't be all that broken. Thierry
On Mon, Aug 25, 2014 at 05:22:32PM +0200, Maxime Ripard wrote: > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote: > > On Mon, Aug 25, 2014 at 04:58:54PM +0200, Maxime Ripard wrote: > > > On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote: > > > > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: > > > > > On 08/25/2014 03:39 PM, Thierry Reding wrote: > > > > > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > > > > > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > > > > > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > > > > > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > > > > > >>> [...] > > > > > >>>>> If not, perhaps the clock driver should force the clock to be > > > > > >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > > > > > >>>> > > > > > >>>> I'm sorry, but I'm not going to take any code that will do that in our > > > > > >>>> clock driver. > > > > > >>>> > > > > > >>>> I'm not going to have a huge list of ifdef depending on configuration > > > > > >>>> options to know which clock to enable, especially when clk_get should > > > > > >>>> have the consumer device as an argument. > > > > > >>> > > > > > >>> Are you saying is that you want to solve a platform-specific problem by > > > > > >>> pushing code into simple, generic drivers so that your platform code can > > > > > >>> stay "clean"? > > > > > >> > > > > > >> Are you saying that this driver would become "dirty" with such a patch? > > > > > > > > > > > > Yes. Others have said the same and even provided alternative solutions > > > > > > on how to solve what's seemingly a platform-specific problem in a > > > > > > platform-specific way. > > > > > > > > > > This is not platform specific, any platform with a complete clock driver > > > > > will suffer from the same problem (the clock driver disabling unclaimed > > > > > ahb gates, and thus killing the video output) if it wants to use simplefb > > > > > for early console support. > > > > > > > > It is platform specific in that your platform may require certain clocks > > > > to remain on. > > > > > > The platform doesn't. simplefb does. simplefb is the obvious consumer > > > for these clocks, and given the current API and abstraction we have, > > > it should be the one claiming the clocks too. > > > > No. simplefb just wants to write to some memory that hardware has been > > set up to scan out. The platform requires that the clocks be on. Other > > platforms may not even allow turning off the clocks. > > Like what? the rpi? Come on. Just because the videocore is some black > box we know nothing about doesn't mean we should use it as an example. You make it sound like the Raspberry Pi is somehow less important than sunxi. > Any decent enough SoC, with a decent support in the kernel will have > clocks for this, and I really wonder how simplefb will behave once its > clocks will be turned off... There are other devices besides ARM SoCs that may want to use this driver and that don't have clock support. But you're missing my point. What I'm saying is that the simplefb driver is meant to serve as a way to take over whatever framebuffer a firmware set up. Therefore I think it makes the most sense to assume that nothing needs to be controlled in any way since already been set up by firmware. Eventually there should be a driver that takes over from simplefb that knows how to properly handle the device's specifics, but that's not simplefb. The goal of this patch series is to keep clocks from being turned off. But that's not what it does. What it does is turn clocks on to prevent them from being turned off. In my opinion that's a workaround for a deficiency in the kernel (and the firmware/kernel interface) and I think it should be fixed at the root. So a much better solution would be to establish a way for firmware to communicate to the kernel that a given resource has been enabled by firmware and shouldn't be disabled. Such a solution can be implement for all types of resources and can be reused by all drivers since they don't have to worry about these details. Thierry
On Mon, Aug 25, 2014 at 05:07:05PM +0200, Maxime Ripard wrote: > On Mon, Aug 25, 2014 at 04:53:06PM +0200, Thierry Reding wrote: > > Hmm... that's true. But we already have a way to deal with exactly this > > situation for regulators. There's a property called regulator-boot-on > > which a bootloader should set whet it has enabled a given regulator. It > > can of course also be set statically in a DTS if it's know upfront that > > a bootloader will always enable it. Perhaps what we need is a similar > > property for clocks so that the clock framework will not inadvertently > > turn off a clock that's still being used. > > Except that such a property won't work either. Of course it won't work if it's designed not to work. The solution to that is to design it in a way that it works and does exactly what we want it to do. > Regulators with regulator-boot-on will still be disabled if there's no > one to claim it. Just like what happens currently for the clocks. I was somewhat surprised by this, but it can indeed easily be verified. It seems to me somewhat at odds with the definition of such a property. Mark, You've probably not read the whole backlog, but the discussion revolves around hand-off of resources from firmware to kernel (via DT in this case). If firmware initializes a device (display controller in this particular case) and enables resources needed by the device to work properly then in order to properly hand over resources from firmware to kernel we need a way to communicate the state of these resources. For regulators the regulator-boot-on property is specified to do exactly that. However the implementation will automatically disable a regulator marked boot-on if it hasn't been claimed by any driver after the initcall stage completes. I find that rather odd since I always assumed that a regulator marked boot-on would not be touched by the core at all, assuming that firmware set it up properly and that it would be required (even if not explicitly claimed). The issue that this causes is that we can't properly hand-off devices initialized by firmware because the regulator will be disabled after the initcall stage and then enabled when the driver loads. In case of display the result will usually be flicker. The same applies to other types of resources (in this case clocks). Two categories of drivers have this issue: drivers built as modules (or that defer probing) and therefore won't be probed until after initcalls have run and generic low-level drivers that take over firmware devices (simplefb in this case) that don't know anything about the resource that the devices need. Also Cc'ing Mike, perhaps he has ideas on how to solve this problem for clocks specifically. Thierry
On Mon, Aug 25, 2014 at 05:18:22PM +0200, Luc Verhaegen wrote: > On Mon, Aug 25, 2014 at 05:12:58PM +0200, Thierry Reding wrote: > > > > Out of curiosity, how does this work in practice? How does the > > bootloader create this entry? Does it scan the DT to see which clocks > > the real hardware device references and then simply copies them to the > > simplefb node? > > > > Thierry > > https://www.mail-archive.com/linux-sunxi@googlegroups.com/msg06619.html That looks like a royal pain. Again, I think it'd be much simpler (but not less code, unfortunately) to do this on a per-resource basis. That way these low-level firmware drivers in the kernel can stay trivial, keeping the real complexity where they belong: in hardware-specific drivers such as DRM/KMS. Thierry
On 26 August 2014 10:04, Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Aug 25, 2014 at 05:22:32PM +0200, Maxime Ripard wrote: >> On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote: >> > On Mon, Aug 25, 2014 at 04:58:54PM +0200, Maxime Ripard wrote: >> > > On Mon, Aug 25, 2014 at 04:16:29PM +0200, Thierry Reding wrote: >> > > > On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: >> > > > > On 08/25/2014 03:39 PM, Thierry Reding wrote: >> > > > > > On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: >> > > > > >> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: >> > > > > >>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: >> > > > > >>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >> > > > > >>> [...] >> > > > > >>>>> If not, perhaps the clock driver should force the clock to be >> > > > > >>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >> > > > > >>>> >> > > > > >>>> I'm sorry, but I'm not going to take any code that will do that in our >> > > > > >>>> clock driver. >> > > > > >>>> >> > > > > >>>> I'm not going to have a huge list of ifdef depending on configuration >> > > > > >>>> options to know which clock to enable, especially when clk_get should >> > > > > >>>> have the consumer device as an argument. >> > > > > >>> >> > > > > >>> Are you saying is that you want to solve a platform-specific problem by >> > > > > >>> pushing code into simple, generic drivers so that your platform code can >> > > > > >>> stay "clean"? >> > > > > >> >> > > > > >> Are you saying that this driver would become "dirty" with such a patch? >> > > > > > >> > > > > > Yes. Others have said the same and even provided alternative solutions >> > > > > > on how to solve what's seemingly a platform-specific problem in a >> > > > > > platform-specific way. >> > > > > >> > > > > This is not platform specific, any platform with a complete clock driver >> > > > > will suffer from the same problem (the clock driver disabling unclaimed >> > > > > ahb gates, and thus killing the video output) if it wants to use simplefb >> > > > > for early console support. >> > > > >> > > > It is platform specific in that your platform may require certain clocks >> > > > to remain on. >> > > >> > > The platform doesn't. simplefb does. simplefb is the obvious consumer >> > > for these clocks, and given the current API and abstraction we have, >> > > it should be the one claiming the clocks too. >> > >> > No. simplefb just wants to write to some memory that hardware has been >> > set up to scan out. The platform requires that the clocks be on. Other >> > platforms may not even allow turning off the clocks. >> >> Like what? the rpi? Come on. Just because the videocore is some black >> box we know nothing about doesn't mean we should use it as an example. > > You make it sound like the Raspberry Pi is somehow less important than > sunxi. No. It's just bad example for driver development because on rpi you are not in control of the hardware. Then the issue of enabling and disabling clock becomes moot because you do not have access to clock at all. > >> Any decent enough SoC, with a decent support in the kernel will have >> clocks for this, and I really wonder how simplefb will behave once its >> clocks will be turned off... > > There are other devices besides ARM SoCs that may want to use this > driver and that don't have clock support. Then the clock support part should be compiled out in that case. Just like any other driver that has code that is disabled when some kernel susbsystem is compiled out. > > But you're missing my point. What I'm saying is that the simplefb driver > is meant to serve as a way to take over whatever framebuffer a firmware > set up. Therefore I think it makes the most sense to assume that nothing > needs to be controlled in any way since already been set up by firmware. > Eventually there should be a driver that takes over from simplefb that > knows how to properly handle the device's specifics, but that's not > simplefb. The simplefb driver is not controlling anything but is using resources set up by firmware. Currently DT has no way to express that state so the proposed solution is to state in DT that simplefb controls the resources that are needed for the framebuffer on the platform. Which resources are that is platform specific and that platform specific is handled by the bootloader that fills in what resources simplefb so 'controls' in its DT node. This way the resource is not disabled by the generic kernel framework such as the clock framework. > > The goal of this patch series is to keep clocks from being turned off. > But that's not what it does. What it does is turn clocks on to prevent > them from being turned off. In my opinion that's a workaround for a > deficiency in the kernel (and the firmware/kernel interface) and I think > it should be fixed at the root. So a much better solution would be to > establish a way for firmware to communicate to the kernel that a given > resource has been enabled by firmware and shouldn't be disabled. That's not the case here. If you later decide you do not want a framebuffer you should be able to stop simplefb, disable the clock that it claimed and reclaim the fb memory even if you do not replace it with a full KMS driver. That way you would not need a special 'headless server bootloader' so long as you do not mind your server os installation enables the display temporarily during boot. Thanks Michal
On Tue, Aug 26, 2014 at 10:26:27AM +0200, Thierry Reding wrote: > On Mon, Aug 25, 2014 at 05:07:05PM +0200, Maxime Ripard wrote: > > Regulators with regulator-boot-on will still be disabled if there's no > > one to claim it. Just like what happens currently for the clocks. > I was somewhat surprised by this, but it can indeed easily be verified. > It seems to me somewhat at odds with the definition of such a property. That depends what you think it should do - it's there for handover from the bootloader in cases where we can't read the state. > that. However the implementation will automatically disable a regulator > marked boot-on if it hasn't been claimed by any driver after the > initcall stage completes. > I find that rather odd since I always assumed that a regulator marked > boot-on would not be touched by the core at all, assuming that firmware > set it up properly and that it would be required (even if not explicitly > claimed). No, there's a separate always-on property if we don't want to disable. We can't assume that the "proper" setup is that the supply should be left on. > Two categories of drivers have this issue: drivers built as modules (or > that defer probing) and therefore won't be probed until after initcalls We really need a later initcall to manage handover to userspace (probably triggered by a combination of userspace saying it's done doing initial module enumeration and the deferred probe grinding to a halt). > have run and generic low-level drivers that take over firmware devices > (simplefb in this case) that don't know anything about the resource that > the devices need. I don't understand this use case, sorry - it sounds like a buggy system design and/or integration. If the firmware is managing the resource then Linux really shouldn't be talking to it at all without coordinating with firmware. How can such a system work otherwise?
Hi, On 08/25/2014 05:49 PM, jonsmirl@gmail.com wrote: > On Mon, Aug 25, 2014 at 11:12 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: >> On Mon, Aug 25, 2014 at 04:27:04PM +0200, Hans de Goede wrote: >>> Hi, >>> >>> On 08/25/2014 04:23 PM, jonsmirl@gmail.com wrote: >>>> On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding >>>> <thierry.reding@gmail.com> wrote: >>>>> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote: >>>>>> On 08/25/2014 03:39 PM, Thierry Reding wrote: >>>>>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: >>>>>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: >>>>>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: >>>>>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: >>>>>>>>> [...] >>>>>>>>>>> If not, perhaps the clock driver should force the clock to be >>>>>>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). >>>>>>>>>> >>>>>>>>>> I'm sorry, but I'm not going to take any code that will do that in our >>>>>>>>>> clock driver. >>>>>>>>>> >>>>>>>>>> I'm not going to have a huge list of ifdef depending on configuration >>>>>>>>>> options to know which clock to enable, especially when clk_get should >>>>>>>>>> have the consumer device as an argument. >>>>>>>>> >>>>>>>>> Are you saying is that you want to solve a platform-specific problem by >>>>>>>>> pushing code into simple, generic drivers so that your platform code can >>>>>>>>> stay "clean"? >>>>>>>> >>>>>>>> Are you saying that this driver would become "dirty" with such a patch? >>>>>>> >>>>>>> Yes. Others have said the same and even provided alternative solutions >>>>>>> on how to solve what's seemingly a platform-specific problem in a >>>>>>> platform-specific way. >>>>>> >>>>>> This is not platform specific, any platform with a complete clock driver >>>>>> will suffer from the same problem (the clock driver disabling unclaimed >>>>>> ahb gates, and thus killing the video output) if it wants to use simplefb >>>>>> for early console support. >>>>> >>>>> It is platform specific in that your platform may require certain clocks >>>>> to remain on. The next platform may require power domains to remain on >>>>> during boot and yet another one may rely on regulators to stay on during >>>>> boot. By your argument simplefb will need to be taught to handle pretty >>>>> much every type of resource that the kernel has. >>>> >>>> Why can't simplefb be a driver library that is called from a device >>>> specific device driver that only claims the clocks (or regulators)? >>>> Then build all of these device specific drivers into the generic ARM >>>> kernel. They will be quite small since all they do is claim the clocks >>>> (or regulator). Maybe we can even figure out some protocol for >>>> removing the unused ones from memory later. >>>> >>>> Later during the boot process the device specific driver can load its >>>> KMS code which has also been implemented as a driver library. Maybe >>>> use E_PROBE_DEFER to do this. Match on the device ID, claim the >>>> clocks, defer until the full KMS library can be loaded. >>> >>> There is no need for all this complexity, all that is needed is for the >>> simplefb driver to be thought to claim + enable any clocks listed in >>> its dt node. >> >> Out of curiosity, how does this work in practice? How does the >> bootloader create this entry? Does it scan the DT to see which clocks >> the real hardware device references and then simply copies them to the >> simplefb node? > > That's why this is such a problem. There is a general rule in Linux - > one device, one driver. A general rule with exceptions such as, drumroll ... , video devices, e.g. efifb vs kms drivers are the exact same thing, or good old vgacon vs kms drivers. Or any other special firmware interfaces for early output (be it serial or video) vs a later loaded / initialized more complete hardware specific driver. Also in the usb world there are quite a few examples where your once device one driver rule does not hold either, but that is not really relevant. Regards, Hans
On Tue, Aug 26, 2014 at 10:00:35AM +0100, Mark Brown wrote: > On Tue, Aug 26, 2014 at 10:26:27AM +0200, Thierry Reding wrote: > > On Mon, Aug 25, 2014 at 05:07:05PM +0200, Maxime Ripard wrote: > > > > Regulators with regulator-boot-on will still be disabled if there's no > > > one to claim it. Just like what happens currently for the clocks. > > > I was somewhat surprised by this, but it can indeed easily be verified. > > It seems to me somewhat at odds with the definition of such a property. > > That depends what you think it should do - it's there for handover from > the bootloader in cases where we can't read the state. True. The description leaves a lot of room for interpretation, though. - regulator-boot-on: bootloader/firmware enabled regulator > > that. However the implementation will automatically disable a regulator > > marked boot-on if it hasn't been claimed by any driver after the > > initcall stage completes. > > > I find that rather odd since I always assumed that a regulator marked > > boot-on would not be touched by the core at all, assuming that firmware > > set it up properly and that it would be required (even if not explicitly > > claimed). > > No, there's a separate always-on property if we don't want to disable. But always-on means that it can't ever be disabled. In this case what we'd need is a "don't disable automatically because it's needed, but we may want to switch it off at a later point to save power." > We can't assume that the "proper" setup is that the supply should be > left on. Right, we can't assume it, but if we're given appropriate hints I think it's fair to keep resources set up by firmware untouched. > > Two categories of drivers have this issue: drivers built as modules (or > > that defer probing) and therefore won't be probed until after initcalls > > We really need a later initcall to manage handover to userspace > (probably triggered by a combination of userspace saying it's done doing > initial module enumeration and the deferred probe grinding to a halt). Yes, perhaps that would be another option. > > have run and generic low-level drivers that take over firmware devices > > (simplefb in this case) that don't know anything about the resource that > > the devices need. > > I don't understand this use case, sorry - it sounds like a buggy system > design and/or integration. If the firmware is managing the resource > then Linux really shouldn't be talking to it at all without coordinating > with firmware. How can such a system work otherwise? The firmware isn't really managing resources. It's setting up state that the kernel shouldn't be modifying. Currently the kernel assumes that no firmware exists and just disables everything that's not being used. That is a reasonable default, but it also limits what we can do. I think if we provided a good interface to communicate state between firmware and kernel then we could easily do this kind of hand-off. Thierry
On Tue, Aug 26, 2014 at 11:18:54AM +0200, Thierry Reding wrote: > On Tue, Aug 26, 2014 at 10:00:35AM +0100, Mark Brown wrote: > > > I find that rather odd since I always assumed that a regulator marked > > > boot-on would not be touched by the core at all, assuming that firmware > > > set it up properly and that it would be required (even if not explicitly > > > claimed). > > No, there's a separate always-on property if we don't want to disable. > But always-on means that it can't ever be disabled. In this case what > we'd need is a "don't disable automatically because it's needed, but we > may want to switch it off at a later point to save power." Such a property wouldn't make much sense, it should also apply to the driver at which point it's just the same as always-on. > > We can't assume that the "proper" setup is that the supply should be > > left on. > Right, we can't assume it, but if we're given appropriate hints I think > it's fair to keep resources set up by firmware untouched. I really can't see any sensible way to do that in an OS neutral thing like DT. > > > have run and generic low-level drivers that take over firmware devices > > > (simplefb in this case) that don't know anything about the resource that > > > the devices need. > > I don't understand this use case, sorry - it sounds like a buggy system > > design and/or integration. If the firmware is managing the resource > > then Linux really shouldn't be talking to it at all without coordinating > > with firmware. How can such a system work otherwise? > The firmware isn't really managing resources. It's setting up state that > the kernel shouldn't be modifying. Currently the kernel assumes that no > firmware exists and just disables everything that's not being used. That > is a reasonable default, but it also limits what we can do. I think if > we provided a good interface to communicate state between firmware and > kernel then we could easily do this kind of hand-off. I'm afraid that's confusing me even further. If the "firmware devices" don't know anything about the resources as you say then presumably they need to be always on since they obviously won't be able to control them and there is going to be no handoff?
On Tue, Aug 26, 2014 at 11:06:12AM +0100, Mark Brown wrote: > On Tue, Aug 26, 2014 at 11:18:54AM +0200, Thierry Reding wrote: > > On Tue, Aug 26, 2014 at 10:00:35AM +0100, Mark Brown wrote: > > > > > I find that rather odd since I always assumed that a regulator marked > > > > boot-on would not be touched by the core at all, assuming that firmware > > > > set it up properly and that it would be required (even if not explicitly > > > > claimed). > > > > No, there's a separate always-on property if we don't want to disable. > > > But always-on means that it can't ever be disabled. In this case what > > we'd need is a "don't disable automatically because it's needed, but we > > may want to switch it off at a later point to save power." > > Such a property wouldn't make much sense, it should also apply to the > driver at which point it's just the same as always-on. > > > > We can't assume that the "proper" setup is that the supply should be > > > left on. > > > Right, we can't assume it, but if we're given appropriate hints I think > > it's fair to keep resources set up by firmware untouched. > > I really can't see any sensible way to do that in an OS neutral thing > like DT. It should be possible to simply define a property for this that the firmware can add to the device tree. There are other subsystems for example where we don't touch resources unless they are explicitly accessed. GPIO and pinmux are two examples of those. The reason is precisely so that we don't undo anything that the firmware already set up and that may be needed during boot. > > > > have run and generic low-level drivers that take over firmware devices > > > > (simplefb in this case) that don't know anything about the resource that > > > > the devices need. > > > > I don't understand this use case, sorry - it sounds like a buggy system > > > design and/or integration. If the firmware is managing the resource > > > then Linux really shouldn't be talking to it at all without coordinating > > > with firmware. How can such a system work otherwise? > > > The firmware isn't really managing resources. It's setting up state that > > the kernel shouldn't be modifying. Currently the kernel assumes that no > > firmware exists and just disables everything that's not being used. That > > is a reasonable default, but it also limits what we can do. I think if > > we provided a good interface to communicate state between firmware and > > kernel then we could easily do this kind of hand-off. > > I'm afraid that's confusing me even further. If the "firmware devices" > don't know anything about the resources as you say then presumably they > need to be always on since they obviously won't be able to control them > and there is going to be no handoff? How this works with simplefb is that the firmware (U-Boot for example) sets up everything needed to scan out a framebuffer. The firmware will typically use this for a boot splash or a console. It's akin to the standard VGA console on x86. Before booting the kernel it will modify the DTB and insert a node that contains information about the framebuffer that was set up. The node contains properties that define the resolution, the format and the physical address of the framebuffer memory. An in-kernel driver will then be able to use that information very early in the boot process to show the console (to replace the serial console that's usually not available on consumer devices). Later on when a real driver can be loaded it should take over the resources from simplefb and remove it. So the firmware does control the resources to a point where the display hardware is initialized and then it will happily forget about them. The device will keep scanning out the framebuffer and simplefb can update it with boot messages. To prevent the console from suddenly disappearing the kernel needs to make sure that the resources aren't inadvertently turned off at some more or less random point in time. The resources don't need to be always on, only as long as there isn't a proper driver to manage them. Once that driver has requested the resources it will likely want to control them to save power. Therefore I think the most straightforward way would be for the kernel not to touch any of these resources until they've been explicitly requested. Even if we don't want to rely on firmware doing the right thing by default, I think it's still valuable to take hints from firmware when it provides them so rather than defaulting to not touching any resources what I proposed is to mark resources that shouldn't be touched. Thierry
On Tue, Aug 26, 2014 at 12:55:06PM +0200, Thierry Reding wrote: > On Tue, Aug 26, 2014 at 11:06:12AM +0100, Mark Brown wrote: > > On Tue, Aug 26, 2014 at 11:18:54AM +0200, Thierry Reding wrote: > > > On Tue, Aug 26, 2014 at 10:00:35AM +0100, Mark Brown wrote: > > > > We can't assume that the "proper" setup is that the supply should be > > > > left on. > > > Right, we can't assume it, but if we're given appropriate hints I think > > > it's fair to keep resources set up by firmware untouched. > > I really can't see any sensible way to do that in an OS neutral thing > > like DT. > It should be possible to simply define a property for this that the > firmware can add to the device tree. > There are other subsystems for example where we don't touch resources > unless they are explicitly accessed. GPIO and pinmux are two examples of > those. The reason is precisely so that we don't undo anything that the > firmware already set up and that may be needed during boot. GPIOs and pinmuxes are quite different in both how they get set up (you don't typically have their configuration set in ROM and shoehorned into the board which is what we're dealing with for a lot of PMICs) and in the consequences of leaving them alone. I really don't think defining a custom property to work around a timing issue in a particular OS startup sequence is the best way forwards here - we should solve our startup sequencing issue rather than layer on hacks, otherwise we're adding fragility and work to the system. The flag would need to be explicitly added and then it means we're stuck with the behaviour even if the system gets smarter. > Before booting the kernel it will modify the DTB and insert a node that > contains information about the framebuffer that was set up. The node > contains properties that define the resolution, the format and the > physical address of the framebuffer memory. An in-kernel driver will > then be able to use that information very early in the boot process to > show the console (to replace the serial console that's usually not > available on consumer devices). Later on when a real driver can be > loaded it should take over the resources from simplefb and remove it. OK, so this is not meaningfully different to just loading the driver normally - it's exactly the same scenario.
On Tue, Aug 26, 2014 at 09:52:49AM +0200, Thierry Reding wrote: > On Mon, Aug 25, 2014 at 05:09:00PM +0200, Luc Verhaegen wrote: > > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote: > > > > > > No. simplefb just wants to write to some memory that hardware has been > > > set up to scan out. The platform requires that the clocks be on. > > > > Simplefb also requires that the memory is there and is persistent. Fine > > for discrete graphics cards, fine for rpi where most things are hidden > > from the ARM core anyway, not so fine for anybody else. > > I don't understand. This patch series isn't changing anything about the > memory aspects of the driver yet it's working for you on sunxi, isn't > it? So it can't be all that broken. > > Thierry Oh, i had to go wrestle with UBoot options and reserve 8MB off the top, which the kernel never gets told about. A nice throwback to x86 IGP bioses, a past i had thought i had left behind forgood. Luc Verhaegen.
On Tue, Aug 26, 2014 at 02:02:58PM +0200, Luc Verhaegen wrote: > On Tue, Aug 26, 2014 at 09:52:49AM +0200, Thierry Reding wrote: > > On Mon, Aug 25, 2014 at 05:09:00PM +0200, Luc Verhaegen wrote: > > > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote: > > > > > > > > No. simplefb just wants to write to some memory that hardware has been > > > > set up to scan out. The platform requires that the clocks be on. > > > > > > Simplefb also requires that the memory is there and is persistent. Fine > > > for discrete graphics cards, fine for rpi where most things are hidden > > > from the ARM core anyway, not so fine for anybody else. > > > > I don't understand. This patch series isn't changing anything about the > > memory aspects of the driver yet it's working for you on sunxi, isn't > > it? So it can't be all that broken. > > > > Thierry > > Oh, i had to go wrestle with UBoot options and reserve 8MB off the top, > which the kernel never gets told about. A nice throwback to x86 IGP > bioses, a past i had thought i had left behind forgood. Can you not use the reserved memory code (drivers/of/of_reserved_mem.c in the kernel)? I think that's the generally accepted way to do this with DT. Thierry
On Tue, Aug 26, 2014 at 02:21:16PM +0200, Thierry Reding wrote: > On Tue, Aug 26, 2014 at 02:02:58PM +0200, Luc Verhaegen wrote: > > On Tue, Aug 26, 2014 at 09:52:49AM +0200, Thierry Reding wrote: > > > On Mon, Aug 25, 2014 at 05:09:00PM +0200, Luc Verhaegen wrote: > > > > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote: > > > > > > > > > > No. simplefb just wants to write to some memory that hardware has been > > > > > set up to scan out. The platform requires that the clocks be on. > > > > > > > > Simplefb also requires that the memory is there and is persistent. Fine > > > > for discrete graphics cards, fine for rpi where most things are hidden > > > > from the ARM core anyway, not so fine for anybody else. > > > > > > I don't understand. This patch series isn't changing anything about the > > > memory aspects of the driver yet it's working for you on sunxi, isn't > > > it? So it can't be all that broken. > > > > > > Thierry > > > > Oh, i had to go wrestle with UBoot options and reserve 8MB off the top, > > which the kernel never gets told about. A nice throwback to x86 IGP > > bioses, a past i had thought i had left behind forgood. > > Can you not use the reserved memory code (drivers/of/of_reserved_mem.c > in the kernel)? I think that's the generally accepted way to do this > with DT. > > Thierry It was mentioned to me, and I probably could do that, but this seemed even more DT wrangling from within u-boot, and it didn't seem finished yet. Plus, all of this already was way more wrangling than i bargained for, especially with the promise of "simple"-fb. Then there is the really awkward way in which one gets to define clocks outside of a dts/compiler (you've seen the code, right?), i really did not feel like repeating something like that. Luc Verhaegen.
Hi, On Mon, Aug 25, 2014 at 05:25:18PM +0200, Andreas Färber wrote: > Hi, > > Am 25.08.2014 15:47, schrieb Hans de Goede: > > On 08/25/2014 03:39 PM, Thierry Reding wrote: > >> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote: > >>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote: > >>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote: > >>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote: > >>>>>> If not, perhaps the clock driver should force the clock to be > >>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?). > [...] > > As for the suggestion to simply never disable the plls / ahb gates by blocking > > them from ever being disabled in the sunxi clock driver, that is not really > > a solution either, as we want to be able to turn these things off to safe > > power on screen blank once control has been turned over to the kms driver. > > Without wanting to take sides on the simplefb matter, can't you just use > clk_ignore_unused in bootargs to work around the issue at hand? > That's what I do on the Spring Chromebook. Leaving *all* the clocks enabled just to workaround a driver failing to grab its clocks doesn't look like a solution either. Maxime
On Tue, Aug 26, 2014 at 10:40:27AM +0200, Thierry Reding wrote: > On Mon, Aug 25, 2014 at 05:18:22PM +0200, Luc Verhaegen wrote: > > On Mon, Aug 25, 2014 at 05:12:58PM +0200, Thierry Reding wrote: > > > > > > Out of curiosity, how does this work in practice? How does the > > > bootloader create this entry? Does it scan the DT to see which clocks > > > the real hardware device references and then simply copies them to the > > > simplefb node? > > > > > > Thierry > > > > https://www.mail-archive.com/linux-sunxi@googlegroups.com/msg06619.html > > That looks like a royal pain. Again, I think it'd be much simpler (but > not less code, unfortunately) to do this on a per-resource basis. That > way these low-level firmware drivers in the kernel can stay trivial, > keeping the real complexity where they belong: in hardware-specific > drivers such as DRM/KMS. So we have to write a DRM/KMS driver in order to have display working while waiting for a DRM/KMS driver to be worked on? Come on... Maxime
On Tue, Aug 26, 2014 at 03:22:07PM +0200, Maxime Ripard wrote: > On Tue, Aug 26, 2014 at 10:40:27AM +0200, Thierry Reding wrote: > > On Mon, Aug 25, 2014 at 05:18:22PM +0200, Luc Verhaegen wrote: > > > On Mon, Aug 25, 2014 at 05:12:58PM +0200, Thierry Reding wrote: > > > > > > > > Out of curiosity, how does this work in practice? How does the > > > > bootloader create this entry? Does it scan the DT to see which clocks > > > > the real hardware device references and then simply copies them to the > > > > simplefb node? > > > > > > > > Thierry > > > > > > https://www.mail-archive.com/linux-sunxi@googlegroups.com/msg06619.html > > > > That looks like a royal pain. Again, I think it'd be much simpler (but > > not less code, unfortunately) to do this on a per-resource basis. That > > way these low-level firmware drivers in the kernel can stay trivial, > > keeping the real complexity where they belong: in hardware-specific > > drivers such as DRM/KMS. > > So we have to write a DRM/KMS driver in order to have display working > while waiting for a DRM/KMS driver to be worked on? Come on... Perhaps you should go and read this thread again. That's not at all what I've been saying. Thierry
On Tue, Aug 26, 2014 at 10:04:33AM +0200, Thierry Reding wrote: > > > No. simplefb just wants to write to some memory that hardware has been > > > set up to scan out. The platform requires that the clocks be on. Other > > > platforms may not even allow turning off the clocks. > > > > Like what? the rpi? Come on. Just because the videocore is some black > > box we know nothing about doesn't mean we should use it as an example. > > You make it sound like the Raspberry Pi is somehow less important than > sunxi. No. What I mean is that it seems like we are somehow punished, or at least blamed, for having a better and more complete kernel support. > > Any decent enough SoC, with a decent support in the kernel will have > > clocks for this, and I really wonder how simplefb will behave once its > > clocks will be turned off... > > There are other devices besides ARM SoCs that may want to use this > driver and that don't have clock support. And in this case, with this patch, simplefb will not claim any clock, nor will fail probing. > But you're missing my point. What I'm saying is that the simplefb driver > is meant to serve as a way to take over whatever framebuffer a firmware > set up. Therefore I think it makes the most sense to assume that nothing > needs to be controlled in any way since already been set up by firmware. > Eventually there should be a driver that takes over from simplefb that > knows how to properly handle the device's specifics, but that's not > simplefb. I guess such a hand over if it were to happen in the kernel would involve the second driver claiming the resources before the first one release them. How is that different in this case? > The goal of this patch series is to keep clocks from being turned off. > But that's not what it does. What it does is turn clocks on to prevent > them from being turned off. In my opinion that's a workaround for a > deficiency in the kernel (and the firmware/kernel interface) and I think > it should be fixed at the root. So a much better solution would be to > establish a way for firmware to communicate to the kernel that a given > resource has been enabled by firmware and shouldn't be disabled. Such a > solution can be implement for all types of resources and can be reused > by all drivers since they don't have to worry about these details. Mike Turquette repeatedly said that he was against such a DT property: https://lkml.org/lkml/2014/5/12/693
On Tue, Aug 26, 2014 at 03:53:41PM +0200, Maxime Ripard wrote: > On Tue, Aug 26, 2014 at 10:04:33AM +0200, Thierry Reding wrote: > > > > No. simplefb just wants to write to some memory that hardware has been > > > > set up to scan out. The platform requires that the clocks be on. Other > > > > platforms may not even allow turning off the clocks. > > > > > > Like what? the rpi? Come on. Just because the videocore is some black > > > box we know nothing about doesn't mean we should use it as an example. > > > > You make it sound like the Raspberry Pi is somehow less important than > > sunxi. > > No. What I mean is that it seems like we are somehow punished, or at > least blamed, for having a better and more complete kernel support. This isn't a competition. Nobody's punishing or blaming anyone. This is about finding the best solution for the problem at hand. > > > Any decent enough SoC, with a decent support in the kernel will have > > > clocks for this, and I really wonder how simplefb will behave once its > > > clocks will be turned off... > > > > There are other devices besides ARM SoCs that may want to use this > > driver and that don't have clock support. > > And in this case, with this patch, simplefb will not claim any clock, > nor will fail probing. > > > But you're missing my point. What I'm saying is that the simplefb driver > > is meant to serve as a way to take over whatever framebuffer a firmware > > set up. Therefore I think it makes the most sense to assume that nothing > > needs to be controlled in any way since already been set up by firmware. > > Eventually there should be a driver that takes over from simplefb that > > knows how to properly handle the device's specifics, but that's not > > simplefb. > > I guess such a hand over if it were to happen in the kernel would > involve the second driver claiming the resources before the first one > release them. How is that different in this case? It's different in that that driver will be hardware specific and know exactly what clock and other resources are required. It will have a device-specific binding. > > The goal of this patch series is to keep clocks from being turned off. > > But that's not what it does. What it does is turn clocks on to prevent > > them from being turned off. In my opinion that's a workaround for a > > deficiency in the kernel (and the firmware/kernel interface) and I think > > it should be fixed at the root. So a much better solution would be to > > establish a way for firmware to communicate to the kernel that a given > > resource has been enabled by firmware and shouldn't be disabled. Such a > > solution can be implement for all types of resources and can be reused > > by all drivers since they don't have to worry about these details. > > Mike Turquette repeatedly said that he was against such a DT property: > https://lkml.org/lkml/2014/5/12/693 Mike says in that email that he's opposing the addition of a property for clocks that is the equivalent of regulator-always-on. That's not what this is about. If at all it'd be a property to mark a clock that should not be disabled by default because it's essential. Adding Mike on this subthread too. Either way, Mark already suggested a different alternative in another subthread, namely to add a new kind of checkpoint at which subsystems can call a "disable unused" function that's called later than a late initcall. This is not going to fix things for you immediately because the clocks will still be switched off (only later) if you don't have a real driver that's claiming the clocks. But you can work around that for now by making the relevant clocks always on and remove that workaround once a real driver is loaded that knows how to handle them properly. Thierry
On Tue, Aug 26, 2014 at 02:33:57PM +0200, Luc Verhaegen wrote: > On Tue, Aug 26, 2014 at 02:21:16PM +0200, Thierry Reding wrote: > > On Tue, Aug 26, 2014 at 02:02:58PM +0200, Luc Verhaegen wrote: > > > On Tue, Aug 26, 2014 at 09:52:49AM +0200, Thierry Reding wrote: > > > > On Mon, Aug 25, 2014 at 05:09:00PM +0200, Luc Verhaegen wrote: > > > > > On Mon, Aug 25, 2014 at 05:05:04PM +0200, Thierry Reding wrote: > > > > > > > > > > > > No. simplefb just wants to write to some memory that hardware has been > > > > > > set up to scan out. The platform requires that the clocks be on. > > > > > > > > > > Simplefb also requires that the memory is there and is persistent. Fine > > > > > for discrete graphics cards, fine for rpi where most things are hidden > > > > > from the ARM core anyway, not so fine for anybody else. > > > > > > > > I don't understand. This patch series isn't changing anything about the > > > > memory aspects of the driver yet it's working for you on sunxi, isn't > > > > it? So it can't be all that broken. > > > > > > > > Thierry > > > > > > Oh, i had to go wrestle with UBoot options and reserve 8MB off the top, > > > which the kernel never gets told about. A nice throwback to x86 IGP > > > bioses, a past i had thought i had left behind forgood. > > > > Can you not use the reserved memory code (drivers/of/of_reserved_mem.c > > in the kernel)? I think that's the generally accepted way to do this > > with DT. > > > > Thierry > > It was mentioned to me, and I probably could do that, but this seemed > even more DT wrangling from within u-boot, and it didn't seem finished > yet. Plus, all of this already was way more wrangling than i bargained > for, especially with the promise of "simple"-fb. Then there is the > really awkward way in which one gets to define clocks outside of a > dts/compiler (you've seen the code, right?), i really did not feel like > repeating something like that. It could most probably be turned into fairly generic and reusable code so that others can use it. And it would give you a way of recovering those 8 MiB of memory that you'd otherwise loose. Thierry
tis 2014-08-26 klockan 12:55 +0200 skrev Thierry Reding: > So the firmware does control the resources to a point where the display > hardware is initialized and then it will happily forget about them. The > device will keep scanning out the framebuffer and simplefb can update it > with boot messages. To prevent the console from suddenly disappearing > the kernel needs to make sure that the resources aren't inadvertently > turned off at some more or less random point in time. Which imho is until simplefb is unbound from the device. Separately to this there needs to be a in-kernel handover procedure where a new driver (i.e. video kms driver) can bind to a device and take over resources, but it's completely unrelated to who owns the hardware resources while simplefb is the active driver for the device. It is not clear to me where the hardware resources should be listed in DT, being it a simplefb node or part of the actual hardware device node properly marked as dynamic boot defaults or something else? It's somewhere inbetween hardware and virtual device, and somewhat volatile. As far as simplefb is concerned it is a hardware desription of the framebuffer, but for a kms driver it's no more than firmware handover of boottime settings and ceases to exists once the kms driver have reconfigured the hardware. What I can do is to list some properties that these settings need to provide: 1. Handover of hardware settings from bootloader to kernel. 2. Hardware description for simplefb use. It's at this level not really any different from any other hardware. 3. Volatile and removed when kms driver have reconfigured the hardware or it's been decided that the (virtual) device should not be active. Regarding 3 and kexec as mentioned earlier. A new set of properties may be added again for kexec usage to describe the current framebuffer for the new kernel but op to the system which prepares for kexec to determine. Should be no different in mechanism from bootloader to kernel handover (the kexecing kernel is the bootloader in kexec case). Regards Henrik
Hi, On 08/26/2014 04:35 PM, Thierry Reding wrote: > On Tue, Aug 26, 2014 at 03:53:41PM +0200, Maxime Ripard wrote: >> On Tue, Aug 26, 2014 at 10:04:33AM +0200, Thierry Reding wrote: >>>>> No. simplefb just wants to write to some memory that hardware has been >>>>> set up to scan out. The platform requires that the clocks be on. Other >>>>> platforms may not even allow turning off the clocks. >>>> >>>> Like what? the rpi? Come on. Just because the videocore is some black >>>> box we know nothing about doesn't mean we should use it as an example. >>> >>> You make it sound like the Raspberry Pi is somehow less important than >>> sunxi. >> >> No. What I mean is that it seems like we are somehow punished, or at >> least blamed, for having a better and more complete kernel support. > > This isn't a competition. Nobody's punishing or blaming anyone. This is > about finding the best solution for the problem at hand. > >>>> Any decent enough SoC, with a decent support in the kernel will have >>>> clocks for this, and I really wonder how simplefb will behave once its >>>> clocks will be turned off... >>> >>> There are other devices besides ARM SoCs that may want to use this >>> driver and that don't have clock support. >> >> And in this case, with this patch, simplefb will not claim any clock, >> nor will fail probing. >> >>> But you're missing my point. What I'm saying is that the simplefb driver >>> is meant to serve as a way to take over whatever framebuffer a firmware >>> set up. Therefore I think it makes the most sense to assume that nothing >>> needs to be controlled in any way since already been set up by firmware. >>> Eventually there should be a driver that takes over from simplefb that >>> knows how to properly handle the device's specifics, but that's not >>> simplefb. >> >> I guess such a hand over if it were to happen in the kernel would >> involve the second driver claiming the resources before the first one >> release them. How is that different in this case? > > It's different in that that driver will be hardware specific and know > exactly what clock and other resources are required. It will have a > device-specific binding. > >>> The goal of this patch series is to keep clocks from being turned off. >>> But that's not what it does. What it does is turn clocks on to prevent >>> them from being turned off. In my opinion that's a workaround for a >>> deficiency in the kernel (and the firmware/kernel interface) and I think >>> it should be fixed at the root. So a much better solution would be to >>> establish a way for firmware to communicate to the kernel that a given >>> resource has been enabled by firmware and shouldn't be disabled. Such a >>> solution can be implement for all types of resources and can be reused >>> by all drivers since they don't have to worry about these details. >> >> Mike Turquette repeatedly said that he was against such a DT property: >> https://lkml.org/lkml/2014/5/12/693 > > Mike says in that email that he's opposing the addition of a property > for clocks that is the equivalent of regulator-always-on. That's not > what this is about. If at all it'd be a property to mark a clock that > should not be disabled by default because it's essential. > > Adding Mike on this subthread too. > > Either way, Mark already suggested a different alternative in another > subthread, namely to add a new kind of checkpoint at which subsystems > can call a "disable unused" function that's called later than a late > initcall. This is not going to fix things for you immediately because > the clocks will still be switched off (only later) if you don't have > a real driver that's claiming the clocks. But you can work around that > for now by making the relevant clocks always on and remove that > workaround once a real driver is loaded that knows how to handle them > properly. This will simply not work, and is a ton more complicated then simply teaching simplefb about a clocks property, which is a really simple and clean patch. First of all let me explain why this won't work. When should those subsystems call this "later then a late initcall" call ? the kms driver may very well be a kernel module, which will be loaded by userspace, so we would need this to be triggered by userspace, but when ? When the initrd is done? What then if the kms driver is not in the initrd, so maybe when udev is done enumerating devices, but what then if the kernel is still enumerating hardware at this time? Will we just put a sleep 30 in our initscripts to make sure the kernel is done scanning all busses, will that be long enough? Maybe we need to re-introduce the scsi_wait_done kernel module which was added as an ugly hack to allow userspace to wait for the kernel to have finished scanning scsi (and ata / sata) busses, for controllers found at the time the module was load. Which never worked reliable, because it would be possible that the controller itself still needed to be discovered. We've spend years cleaning up userspace enough to be able to kill scsi_wait_done. We've already been down this road of waiting for hw enumeration to be done, and the problem is that on modern systems *it is never done*. So second of all, Thierry, what exactly is the technical argument against adding support for dealing with clocks to simplefb ? I've heard a lot of people in favor of it, and only pretty much you against it, and your argument so far seems to boil down to "I don't like it", which is not really a technical sound argument IMHO. Moreover all the alternatives seem to be horribly complicated and most of them also seem to have several issues which will make them simply not work reliable. Regards, Hans
On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > > > > Any decent enough SoC, with a decent support in the kernel will have > > > > clocks for this, and I really wonder how simplefb will behave once its > > > > clocks will be turned off... > > > > > > There are other devices besides ARM SoCs that may want to use this > > > driver and that don't have clock support. > > > > And in this case, with this patch, simplefb will not claim any clock, > > nor will fail probing. > > > > > But you're missing my point. What I'm saying is that the simplefb driver > > > is meant to serve as a way to take over whatever framebuffer a firmware > > > set up. Therefore I think it makes the most sense to assume that nothing > > > needs to be controlled in any way since already been set up by firmware. > > > Eventually there should be a driver that takes over from simplefb that > > > knows how to properly handle the device's specifics, but that's not > > > simplefb. > > > > I guess such a hand over if it were to happen in the kernel would > > involve the second driver claiming the resources before the first one > > release them. How is that different in this case? > > It's different in that that driver will be hardware specific and know > exactly what clock and other resources are required. It will have a > device-specific binding. Except that you made simplefb a generic driver. So we have two choices here: either we don't anything but the trivial case, and no one with a rather more advanced hardware will be able to use it, or we try to grab any resource that might be of use, which means clocks, regulators, reserved memory region, or whatever, so that we pretty much cover all cases. It really is as simple as that. > > > The goal of this patch series is to keep clocks from being turned off. > > > But that's not what it does. What it does is turn clocks on to prevent > > > them from being turned off. In my opinion that's a workaround for a > > > deficiency in the kernel (and the firmware/kernel interface) and I think > > > it should be fixed at the root. So a much better solution would be to > > > establish a way for firmware to communicate to the kernel that a given > > > resource has been enabled by firmware and shouldn't be disabled. Such a > > > solution can be implement for all types of resources and can be reused > > > by all drivers since they don't have to worry about these details. > > > > Mike Turquette repeatedly said that he was against such a DT property: > > https://lkml.org/lkml/2014/5/12/693 > > Mike says in that email that he's opposing the addition of a property > for clocks that is the equivalent of regulator-always-on. That's not > what this is about. If at all it'd be a property to mark a clock that > should not be disabled by default because it's essential. It's just semantic. How is "a clock that should not be disabled by default because it's essential" not a clock that stays always on? Plus, you should read the mail further. It's clearly said that consumer drivers should call clk_prepare_enable themselves, and that the only exception to that is the case where we don't have such a driver (and only valid as a temporary exception, which I guess you'll soon turn into temporary-until-a-drm-kms-driver-is-merged). > Adding Mike on this subthread too. > > Either way, Mark already suggested a different alternative in another > subthread, namely to add a new kind of checkpoint at which subsystems > can call a "disable unused" function that's called later than a late > initcall. This is not going to fix things for you immediately because > the clocks will still be switched off (only later) if you don't have > a real driver that's claiming the clocks. Great, I'm glad we found a solution for a completely unrelated issue. > But you can work around that for now by making the relevant clocks > always on and remove that workaround once a real driver is loaded > that knows how to handle them properly. So, let me get this straight. The clock provider driver should behave as a clock consumer because it knows that in some cases, it might not have any willingful enough consumer? Doesn't that even ring your this-is-a-clear-abstraction-violation-bell just a tiny bit?
On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > > > > > Any decent enough SoC, with a decent support in the kernel will have > > > > > clocks for this, and I really wonder how simplefb will behave once its > > > > > clocks will be turned off... > > > > > > > > There are other devices besides ARM SoCs that may want to use this > > > > driver and that don't have clock support. > > > > > > And in this case, with this patch, simplefb will not claim any clock, > > > nor will fail probing. > > > > > > > But you're missing my point. What I'm saying is that the simplefb driver > > > > is meant to serve as a way to take over whatever framebuffer a firmware > > > > set up. Therefore I think it makes the most sense to assume that nothing > > > > needs to be controlled in any way since already been set up by firmware. > > > > Eventually there should be a driver that takes over from simplefb that > > > > knows how to properly handle the device's specifics, but that's not > > > > simplefb. > > > > > > I guess such a hand over if it were to happen in the kernel would > > > involve the second driver claiming the resources before the first one > > > release them. How is that different in this case? > > > > It's different in that that driver will be hardware specific and know > > exactly what clock and other resources are required. It will have a > > device-specific binding. > > Except that you made simplefb a generic driver. So we have two choices > here: either we don't anything but the trivial case, and no one with a > rather more advanced hardware will be able to use it, or we try to > grab any resource that might be of use, which means clocks, > regulators, reserved memory region, or whatever, so that we pretty > much cover all cases. It really is as simple as that. No, it should be even simpler. simplefb shouldn't have to know any of this. It's just taking what firmware has set up and uses that. It's a stop-gap solution to provide information on the display until a real driver can be loaded and initializes the display hardware properly. > > > > The goal of this patch series is to keep clocks from being turned off. > > > > But that's not what it does. What it does is turn clocks on to prevent > > > > them from being turned off. In my opinion that's a workaround for a > > > > deficiency in the kernel (and the firmware/kernel interface) and I think > > > > it should be fixed at the root. So a much better solution would be to > > > > establish a way for firmware to communicate to the kernel that a given > > > > resource has been enabled by firmware and shouldn't be disabled. Such a > > > > solution can be implement for all types of resources and can be reused > > > > by all drivers since they don't have to worry about these details. > > > > > > Mike Turquette repeatedly said that he was against such a DT property: > > > https://lkml.org/lkml/2014/5/12/693 > > > > Mike says in that email that he's opposing the addition of a property > > for clocks that is the equivalent of regulator-always-on. That's not > > what this is about. If at all it'd be a property to mark a clock that > > should not be disabled by default because it's essential. > > It's just semantic. How is "a clock that should not be disabled by > default because it's essential" not a clock that stays always on? Because a clock that should not be disabled by default can be turned off when appropriate. A clock that is always on can't be turned off. > Plus, you should read the mail further. It's clearly said that > consumer drivers should call clk_prepare_enable themselves, and that > the only exception to that is the case where we don't have such a > driver (and only valid as a temporary exception, which I guess you'll > soon turn into temporary-until-a-drm-kms-driver-is-merged). Exactly. simplefb is only a temporary measure. It's not meant to be used as a full-blown framebuffer driver. There are two use-cases where it is an appropriate solution: 1) As a stop-gap solution for when the platform doesn't support a full display driver yet. In that case you will want simplefb to stay around forever. 2) To get early boot output before a full display driver can be loaded in which case simplefb should go away after the display driver has taken over. In case of 2) the most straightforward solution is to not disable any clocks until all drivers have had a chance to claim them. When the full driver has been probed it should have claimed the clocks so they would no longer get disabled. For 1) I think it's fair to say that it's only a temporary solution to get something on the screen. There won't be any kind of acceleration at all, no power saving. Nothing but a dumb framebuffer. In that case it should be a simple matter of keeping a select number of clocks always on in the clock driver until support is more complete and it can be properly handled. It's not like it's going to make a difference anyway since simplefb won't ever disable them either. > > Adding Mike on this subthread too. > > > > Either way, Mark already suggested a different alternative in another > > subthread, namely to add a new kind of checkpoint at which subsystems > > can call a "disable unused" function that's called later than a late > > initcall. This is not going to fix things for you immediately because > > the clocks will still be switched off (only later) if you don't have > > a real driver that's claiming the clocks. > > Great, I'm glad we found a solution for a completely unrelated issue. It's not at all unrelated. See above. > > But you can work around that for now by making the relevant clocks > > always on and remove that workaround once a real driver is loaded > > that knows how to handle them properly. > > So, let me get this straight. The clock provider driver should behave > as a clock consumer because it knows that in some cases, it might not > have any willingful enough consumer? Doesn't that even ring your > this-is-a-clear-abstraction-violation-bell just a tiny bit? No. The clock driver should simply not allow the clocks to be disabled if it isn't safe to do so. Thierry
On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote: > It is not clear to me where the hardware resources should be listed in > DT, being it a simplefb node or part of the actual hardware device node > properly marked as dynamic boot defaults or something else? It's > somewhere inbetween hardware and virtual device, and somewhat volatile. > As far as simplefb is concerned it is a hardware desription of the > framebuffer, but for a kms driver it's no more than firmware handover of > boottime settings and ceases to exists once the kms driver have > reconfigured the hardware. Is simplefb something that should be in the device tree distinctly in the first place - shouldn't it be a subset of the functionality of the video nodes? It's the same hardware being driven differently.
Hi, On 08/27/2014 08:54 AM, Thierry Reding wrote: > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: >> On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: >>>>>> Any decent enough SoC, with a decent support in the kernel will have >>>>>> clocks for this, and I really wonder how simplefb will behave once its >>>>>> clocks will be turned off... >>>>> >>>>> There are other devices besides ARM SoCs that may want to use this >>>>> driver and that don't have clock support. >>>> >>>> And in this case, with this patch, simplefb will not claim any clock, >>>> nor will fail probing. >>>> >>>>> But you're missing my point. What I'm saying is that the simplefb driver >>>>> is meant to serve as a way to take over whatever framebuffer a firmware >>>>> set up. Therefore I think it makes the most sense to assume that nothing >>>>> needs to be controlled in any way since already been set up by firmware. >>>>> Eventually there should be a driver that takes over from simplefb that >>>>> knows how to properly handle the device's specifics, but that's not >>>>> simplefb. >>>> >>>> I guess such a hand over if it were to happen in the kernel would >>>> involve the second driver claiming the resources before the first one >>>> release them. How is that different in this case? >>> >>> It's different in that that driver will be hardware specific and know >>> exactly what clock and other resources are required. It will have a >>> device-specific binding. >> >> Except that you made simplefb a generic driver. So we have two choices >> here: either we don't anything but the trivial case, and no one with a >> rather more advanced hardware will be able to use it, or we try to >> grab any resource that might be of use, which means clocks, >> regulators, reserved memory region, or whatever, so that we pretty >> much cover all cases. It really is as simple as that. > > No, it should be even simpler. simplefb shouldn't have to know any of > this. It's just taking what firmware has set up and uses that. It's a > stop-gap solution to provide information on the display until a real > driver can be loaded and initializes the display hardware properly. > >>>>> The goal of this patch series is to keep clocks from being turned off. >>>>> But that's not what it does. What it does is turn clocks on to prevent >>>>> them from being turned off. In my opinion that's a workaround for a >>>>> deficiency in the kernel (and the firmware/kernel interface) and I think >>>>> it should be fixed at the root. So a much better solution would be to >>>>> establish a way for firmware to communicate to the kernel that a given >>>>> resource has been enabled by firmware and shouldn't be disabled. Such a >>>>> solution can be implement for all types of resources and can be reused >>>>> by all drivers since they don't have to worry about these details. >>>> >>>> Mike Turquette repeatedly said that he was against such a DT property: >>>> https://lkml.org/lkml/2014/5/12/693 >>> >>> Mike says in that email that he's opposing the addition of a property >>> for clocks that is the equivalent of regulator-always-on. That's not >>> what this is about. If at all it'd be a property to mark a clock that >>> should not be disabled by default because it's essential. >> >> It's just semantic. How is "a clock that should not be disabled by >> default because it's essential" not a clock that stays always on? > > Because a clock that should not be disabled by default can be turned off > when appropriate. A clock that is always on can't be turned off. > >> Plus, you should read the mail further. It's clearly said that >> consumer drivers should call clk_prepare_enable themselves, and that >> the only exception to that is the case where we don't have such a >> driver (and only valid as a temporary exception, which I guess you'll >> soon turn into temporary-until-a-drm-kms-driver-is-merged). > > Exactly. simplefb is only a temporary measure. It's not meant to be used > as a full-blown framebuffer driver. There are two use-cases where it is > an appropriate solution: > > 1) As a stop-gap solution for when the platform doesn't support a full > display driver yet. In that case you will want simplefb to stay > around forever. > > 2) To get early boot output before a full display driver can be loaded > in which case simplefb should go away after the display driver has > taken over. > > In case of 2) the most straightforward solution is to not disable any > clocks until all drivers have had a chance to claim them. When the full > driver has been probed it should have claimed the clocks so they would > no longer get disabled. Please read my long reply from yesterday evening why this will never work, as there is not a well defined moment when "all drivers have had a chance to claim them" I've been doing low level Linux development for 10 years now and I've worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past. We've been down this road before, and all it leads to is pain, some more pain and then even more pain. Followed by the conclusion that everything needed to be refactored to use a hotplug model, and that instead of waiting for hardware / disk-enumeration to be done (so we can mount filesystems), the right thing to do is to actually wait for the block device(s) holding said filesystems to show up. The same applies here, the right thing to do is to wait for the kms driver claiming the clocks to actually show up. And the easiest and cleanest way to do that is to have simplefb claim the clocks, and have it release them when the kms driver loads and asks simplefb to unload. Oh and one more thing, this whole make a list of special clocks which should be disabled later model is flawed too, because it assumes a fixed list of clocks, but which clocks actually get used may very well depend on whether e.g. a vga or hdmi monitor is attached, so u-boot would then still need to communicate the list of clocks actually used somehow, and if it needs to pass a list of clocks, the most natural way to do so is through a clocks property... Regards, Hans
tis 2014-08-26 klockan 15:53 +0200 skrev Maxime Ripard: > > it should be fixed at the root. So a much better solution would be to > > establish a way for firmware to communicate to the kernel that a given > > resource has been enabled by firmware and shouldn't be disabled. Such a > > solution can be implement for all types of resources and can be reused > > by all drivers since they don't have to worry about these details. > > Mike Turquette repeatedly said that he was against such a DT property: > https://lkml.org/lkml/2014/5/12/693 And not really what we want here. The clocks should be turned off if not in use, i.e. when simplefb is unbound from the device and no other driver has taken over. Maybe even in the case where there is no simplefb driver in-kernel, but if the use case is to be generalized then this kind of resources need to be held until explicitly released to allow resources to be held until their driver is loaded, which may happen long after kernel bootup. But again, for many use cases it is a virtual hardware property, not really hardware in it's own but still smells a lot like hardware. These parameters is only valid until the hardware have been reconfigured. Regards Henrik
On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote: > On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote: > > > It is not clear to me where the hardware resources should be listed in > > DT, being it a simplefb node or part of the actual hardware device node > > properly marked as dynamic boot defaults or something else? It's > > somewhere inbetween hardware and virtual device, and somewhat volatile. > > As far as simplefb is concerned it is a hardware desription of the > > framebuffer, but for a kms driver it's no more than firmware handover of > > boottime settings and ceases to exists once the kms driver have > > reconfigured the hardware. > > Is simplefb something that should be in the device tree distinctly in > the first place - shouldn't it be a subset of the functionality of the > video nodes? It's the same hardware being driven differently. Therorically, yes, but that would mean knowing beforehand what the final binding will look like, even before submitting the driver. Since the bindings are always reviewed, and most of the time changed slightly, that wouldn't work very well with the DT as a stable ABI policy I guess. Maxime
Hi Maxime, On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote: >> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote: >> >> > It is not clear to me where the hardware resources should be listed in >> > DT, being it a simplefb node or part of the actual hardware device node >> > properly marked as dynamic boot defaults or something else? It's >> > somewhere inbetween hardware and virtual device, and somewhat volatile. >> > As far as simplefb is concerned it is a hardware desription of the >> > framebuffer, but for a kms driver it's no more than firmware handover of >> > boottime settings and ceases to exists once the kms driver have >> > reconfigured the hardware. >> >> Is simplefb something that should be in the device tree distinctly in >> the first place - shouldn't it be a subset of the functionality of the >> video nodes? It's the same hardware being driven differently. > > Therorically, yes, but that would mean knowing beforehand what the > final binding will look like, even before submitting the driver. Since > the bindings are always reviewed, and most of the time changed > slightly, that wouldn't work very well with the DT as a stable ABI > policy I guess. If you don't know how the bindings for a device will look like at the time of writing your DTS, you're always screwed, whether you add a simpefb node or not. If you know how the bindings look like, just add the device, with an extra "linux,simplefb" compatibility value. If you don't know how the bindings look like, do your utter best in guessing. Your DTS must be amended later anyway, either because you guessed wrong[*] (in case you added a node to have simplefb working), or because you have to add a real device node (in case you didn't add one for simplefb). [*] Actually you may have guessed right, in which case it'll continue working as-is, and everybody will be happy. Whether you want to keep backwards-compatibility in your future driver with the "guessed wrong" node is up to you. 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 Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > > > > > > Any decent enough SoC, with a decent support in the kernel will have > > > > > > clocks for this, and I really wonder how simplefb will behave once its > > > > > > clocks will be turned off... > > > > > > > > > > There are other devices besides ARM SoCs that may want to use this > > > > > driver and that don't have clock support. > > > > > > > > And in this case, with this patch, simplefb will not claim any clock, > > > > nor will fail probing. > > > > > > > > > But you're missing my point. What I'm saying is that the simplefb driver > > > > > is meant to serve as a way to take over whatever framebuffer a firmware > > > > > set up. Therefore I think it makes the most sense to assume that nothing > > > > > needs to be controlled in any way since already been set up by firmware. > > > > > Eventually there should be a driver that takes over from simplefb that > > > > > knows how to properly handle the device's specifics, but that's not > > > > > simplefb. > > > > > > > > I guess such a hand over if it were to happen in the kernel would > > > > involve the second driver claiming the resources before the first one > > > > release them. How is that different in this case? > > > > > > It's different in that that driver will be hardware specific and know > > > exactly what clock and other resources are required. It will have a > > > device-specific binding. > > > > Except that you made simplefb a generic driver. So we have two choices > > here: either we don't anything but the trivial case, and no one with a > > rather more advanced hardware will be able to use it, or we try to > > grab any resource that might be of use, which means clocks, > > regulators, reserved memory region, or whatever, so that we pretty > > much cover all cases. It really is as simple as that. > > No, it should be even simpler. simplefb shouldn't have to know any of > this. It's just taking what firmware has set up and uses that. It's a > stop-gap solution to provide information on the display until a real > driver can be loaded and initializes the display hardware properly. That was not the original intention of this driver then. Stephen's commit log (commit 26549c8d36a6) was even mentionning some use cases: " Examples use-cases include: * The built-in LCD panels on the Samsung ARM chromebook, and Tegra devices, and likely many other ARM or embedded systems. These cannot yet be supported using a full graphics driver, since the panel control should be provided by the CDF (Common Display Framework), which has been stuck in design/review for quite some time. One could support these panels using custom SoC-specific code, but there is a desire to use common infra-structure rather than having each SoC vendor invent their own code, hence the desire to wait for CDF. * Hardware for which a full graphics driver is not yet available, and the path to obtain one upstream isn't yet clear. For example, the Raspberry Pi. * Any hardware in early stages of upstreaming, before a full graphics driver has been tackled. This driver can provide a graphical boot console (even full X support) much earlier in the upstreaming process, thus making new SoC or board support more generally useful earlier. " We're clearly in the use cases 2 and 3. I understand that you're suggesting a 4th use-case, which seems totally valid, but let's not forget the reasons why it has been merged in the first place. > > > > > The goal of this patch series is to keep clocks from being turned off. > > > > > But that's not what it does. What it does is turn clocks on to prevent > > > > > them from being turned off. In my opinion that's a workaround for a > > > > > deficiency in the kernel (and the firmware/kernel interface) and I think > > > > > it should be fixed at the root. So a much better solution would be to > > > > > establish a way for firmware to communicate to the kernel that a given > > > > > resource has been enabled by firmware and shouldn't be disabled. Such a > > > > > solution can be implement for all types of resources and can be reused > > > > > by all drivers since they don't have to worry about these details. > > > > > > > > Mike Turquette repeatedly said that he was against such a DT property: > > > > https://lkml.org/lkml/2014/5/12/693 > > > > > > Mike says in that email that he's opposing the addition of a property > > > for clocks that is the equivalent of regulator-always-on. That's not > > > what this is about. If at all it'd be a property to mark a clock that > > > should not be disabled by default because it's essential. > > > > It's just semantic. How is "a clock that should not be disabled by > > default because it's essential" not a clock that stays always on? > > Because a clock that should not be disabled by default can be turned off > when appropriate. A clock that is always on can't be turned off. If a clock is essential, then it should never be disabled. Or we don't share the same meaning of essential. > > Plus, you should read the mail further. It's clearly said that > > consumer drivers should call clk_prepare_enable themselves, and that > > the only exception to that is the case where we don't have such a > > driver (and only valid as a temporary exception, which I guess you'll > > soon turn into temporary-until-a-drm-kms-driver-is-merged). > > Exactly. simplefb is only a temporary measure. It's not meant to be used > as a full-blown framebuffer driver. There are two use-cases where it is > an appropriate solution: > > 1) As a stop-gap solution for when the platform doesn't support a full > display driver yet. In that case you will want simplefb to stay > around forever. > > 2) To get early boot output before a full display driver can be loaded > in which case simplefb should go away after the display driver has > taken over. > > In case of 2) the most straightforward solution is to not disable any > clocks until all drivers have had a chance to claim them. When the full > driver has been probed it should have claimed the clocks so they would > no longer get disabled. > > For 1) I think it's fair to say that it's only a temporary solution to > get something on the screen. There won't be any kind of acceleration at > all, no power saving. Nothing but a dumb framebuffer. In that case it > should be a simple matter of keeping a select number of clocks always on > in the clock driver until support is more complete and it can be > properly handled. It's not like it's going to make a difference anyway > since simplefb won't ever disable them either. We do agree on that. The only thing we seem to disagree on is how to keep these clocks running. > > > Adding Mike on this subthread too. > > > > > > Either way, Mark already suggested a different alternative in another > > > subthread, namely to add a new kind of checkpoint at which subsystems > > > can call a "disable unused" function that's called later than a late > > > initcall. This is not going to fix things for you immediately because > > > the clocks will still be switched off (only later) if you don't have > > > a real driver that's claiming the clocks. > > > > Great, I'm glad we found a solution for a completely unrelated issue. > > It's not at all unrelated. See above. Kind of, these patches were about the first use case you mentionned, and your solution tackles the second one. > > > But you can work around that for now by making the relevant clocks > > > always on and remove that workaround once a real driver is loaded > > > that knows how to handle them properly. > > > > So, let me get this straight. The clock provider driver should behave > > as a clock consumer because it knows that in some cases, it might not > > have any willingful enough consumer? Doesn't that even ring your > > this-is-a-clear-abstraction-violation-bell just a tiny bit? > > No. The clock driver should simply not allow the clocks to be disabled > if it isn't safe to do so. Again, we do seem to differ on our interpretation of an essential clock. To me, it is perfectly safe to disable the clocks. The system will still be responding, the memory will be there, the CPU will keep running, and we do have a way to recover from that disabled to clock (for example to enable it back). Plus, again, in Mike's mail, it's clearly said that adding hacks like this to the clock driver should only be considered in the case where we don't have a consuming driver, which is not our case here. Maxime
Hi! On Wed, Aug 27, 2014 at 10:37:36AM +0200, Geert Uytterhoeven wrote: > Hi Maxime, > > On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote: > >> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote: > >> > >> > It is not clear to me where the hardware resources should be listed in > >> > DT, being it a simplefb node or part of the actual hardware device node > >> > properly marked as dynamic boot defaults or something else? It's > >> > somewhere inbetween hardware and virtual device, and somewhat volatile. > >> > As far as simplefb is concerned it is a hardware desription of the > >> > framebuffer, but for a kms driver it's no more than firmware handover of > >> > boottime settings and ceases to exists once the kms driver have > >> > reconfigured the hardware. > >> > >> Is simplefb something that should be in the device tree distinctly in > >> the first place - shouldn't it be a subset of the functionality of the > >> video nodes? It's the same hardware being driven differently. > > > > Therorically, yes, but that would mean knowing beforehand what the > > final binding will look like, even before submitting the driver. Since > > the bindings are always reviewed, and most of the time changed > > slightly, that wouldn't work very well with the DT as a stable ABI > > policy I guess. > > If you don't know how the bindings for a device will look like at the time of > writing your DTS, you're always screwed, whether you add a simpefb > node or not. > > If you know how the bindings look like, just add the device, with an extra > "linux,simplefb" compatibility value. > If you don't know how the bindings look like, do your utter best in > guessing. Your DTS must be amended later anyway, either because > you guessed wrong[*] (in case you added a node to have simplefb > working), or because you have to add a real device node (in case you > didn't add one for simplefb). Let's be conservative and consider the case where we would guess wrong. If we just rely on a simplefb node, when reviewing and integrating the "new" bindings to describe accureately the various IPs involved in the display path, we would obviously create new compatibles for them. Since it's new compatibles, we can come up with any binding we'd like, without have to consider the backward compatibility, since it's a new binding. Then, we just remove the simplefb, all is good. If we were to try to create our bindings for all the IPs involved, and were not pleased with the binding anymore when merging the driver, then we would have to break the bindings, since we don't introduce a new compatible anymore, but modifying an existing one. Maxime
On Wed, Aug 27, 2014 at 10:55 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: >> >> Is simplefb something that should be in the device tree distinctly in >> >> the first place - shouldn't it be a subset of the functionality of the >> >> video nodes? It's the same hardware being driven differently. >> > >> > Therorically, yes, but that would mean knowing beforehand what the >> > final binding will look like, even before submitting the driver. Since >> > the bindings are always reviewed, and most of the time changed >> > slightly, that wouldn't work very well with the DT as a stable ABI >> > policy I guess. >> >> If you don't know how the bindings for a device will look like at the time of >> writing your DTS, you're always screwed, whether you add a simpefb >> node or not. >> >> If you know how the bindings look like, just add the device, with an extra >> "linux,simplefb" compatibility value. >> If you don't know how the bindings look like, do your utter best in >> guessing. Your DTS must be amended later anyway, either because >> you guessed wrong[*] (in case you added a node to have simplefb >> working), or because you have to add a real device node (in case you >> didn't add one for simplefb). > > Let's be conservative and consider the case where we would guess > wrong. > > If we just rely on a simplefb node, when reviewing and integrating the > "new" bindings to describe accureately the various IPs involved in the > display path, we would obviously create new compatibles for > them. Since it's new compatibles, we can come up with any binding we'd > like, without have to consider the backward compatibility, since it's > a new binding. > > Then, we just remove the simplefb, all is good. I would keep the simplefb compatible value. Else you break compatibility with old kernels that don't have your new driver. 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
Can you please fix you mail setup. You're addressing me directly in this email, yet I'm neither in To: nor Cc: headers. That's really annoying. On Tue, Aug 26, 2014 at 09:59:00PM +0200, Hans de Goede wrote: > Hi, > > On 08/26/2014 04:35 PM, Thierry Reding wrote: > >On Tue, Aug 26, 2014 at 03:53:41PM +0200, Maxime Ripard wrote: > >>On Tue, Aug 26, 2014 at 10:04:33AM +0200, Thierry Reding wrote: > >>>>>No. simplefb just wants to write to some memory that hardware has been > >>>>>set up to scan out. The platform requires that the clocks be on. Other > >>>>>platforms may not even allow turning off the clocks. > >>>> > >>>>Like what? the rpi? Come on. Just because the videocore is some black > >>>>box we know nothing about doesn't mean we should use it as an example. > >>> > >>>You make it sound like the Raspberry Pi is somehow less important than > >>>sunxi. > >> > >>No. What I mean is that it seems like we are somehow punished, or at > >>least blamed, for having a better and more complete kernel support. > > > >This isn't a competition. Nobody's punishing or blaming anyone. This is > >about finding the best solution for the problem at hand. > > > >>>>Any decent enough SoC, with a decent support in the kernel will have > >>>>clocks for this, and I really wonder how simplefb will behave once its > >>>>clocks will be turned off... > >>> > >>>There are other devices besides ARM SoCs that may want to use this > >>>driver and that don't have clock support. > >> > >>And in this case, with this patch, simplefb will not claim any clock, > >>nor will fail probing. > >> > >>>But you're missing my point. What I'm saying is that the simplefb driver > >>>is meant to serve as a way to take over whatever framebuffer a firmware > >>>set up. Therefore I think it makes the most sense to assume that nothing > >>>needs to be controlled in any way since already been set up by firmware. > >>>Eventually there should be a driver that takes over from simplefb that > >>>knows how to properly handle the device's specifics, but that's not > >>>simplefb. > >> > >>I guess such a hand over if it were to happen in the kernel would > >>involve the second driver claiming the resources before the first one > >>release them. How is that different in this case? > > > >It's different in that that driver will be hardware specific and know > >exactly what clock and other resources are required. It will have a > >device-specific binding. > > > >>>The goal of this patch series is to keep clocks from being turned off. > >>>But that's not what it does. What it does is turn clocks on to prevent > >>>them from being turned off. In my opinion that's a workaround for a > >>>deficiency in the kernel (and the firmware/kernel interface) and I think > >>>it should be fixed at the root. So a much better solution would be to > >>>establish a way for firmware to communicate to the kernel that a given > >>>resource has been enabled by firmware and shouldn't be disabled. Such a > >>>solution can be implement for all types of resources and can be reused > >>>by all drivers since they don't have to worry about these details. > >> > >>Mike Turquette repeatedly said that he was against such a DT property: > >>https://lkml.org/lkml/2014/5/12/693 > > > >Mike says in that email that he's opposing the addition of a property > >for clocks that is the equivalent of regulator-always-on. That's not > >what this is about. If at all it'd be a property to mark a clock that > >should not be disabled by default because it's essential. > > > >Adding Mike on this subthread too. > > > >Either way, Mark already suggested a different alternative in another > >subthread, namely to add a new kind of checkpoint at which subsystems > >can call a "disable unused" function that's called later than a late > >initcall. This is not going to fix things for you immediately because > >the clocks will still be switched off (only later) if you don't have > >a real driver that's claiming the clocks. But you can work around that > >for now by making the relevant clocks always on and remove that > >workaround once a real driver is loaded that knows how to handle them > >properly. > > This will simply not work, and is a ton more complicated then > simply teaching simplefb about a clocks property, which is a really simple > and clean patch. > > First of all let me explain why this won't work. When should those > subsystems call this "later then a late initcall" call ? the kms driver > may very well be a kernel module, which will be loaded by userspace, > so we would need this to be triggered by userspace, but when ? > > When the initrd is done? What then if the kms driver is not in the initrd, > so maybe when udev is done enumerating devices, but what then if the kernel > is still enumerating hardware at this time? Usually an initrd knows how to wait for all or a given set of devices to be probed. udev is pretty good for that. > Will we just put a sleep 30 > in our initscripts to make sure the kernel is done scanning all busses, > will that be long enough? Maybe we need to re-introduce the scsi_wait_done > kernel module which was added as an ugly hack to allow userspace to wait > for the kernel to have finished scanning scsi (and ata / sata) busses, > for controllers found at the time the module was load. Which never worked > reliable, because it would be possible that the controller itself still > needed to be discovered. We've spend years cleaning up userspace enough > to be able to kill scsi_wait_done. We've already been down this road of > waiting for hw enumeration to be done, and the problem is that on > modern systems *it is never done*. Those are mostly integration issues. If you don't load the driver from initrd then I don't think anybody will complain when there's flicker when it finally gets loaded. The whole point of this is to have early access to the framebuffer and get display and graphics going as soon as possible, so please don't pull in hypothetical scenarios where people manually load drivers half an hour after the system has booted. > So second of all, Thierry, what exactly is the technical argument against > adding support for dealing with clocks to simplefb ? I've already stated technical arguments against it. You could just go back and read the thread again, or would you rather want me to repeat them for your convenience? > I've heard a lot of people in favor of it, Of course you have, all the people in favour of it are sunxi people that really want to see this merged because it gives them working display after U-Boot. > and only pretty much you against it, At least Stephen also spoke out about it. But this quickly devolved into the kind of thread you want to get out of as quickly as possible, so I can't blame him for not continuing this discussion. In fact I'm very much regretting getting into this in the first place. You clearly have no intention to even consider any possible other solution that what was posted, so it's pretty useless to try and convince you. > and your argument so far seems to boil down to "I don't like it", I'm not surprised that you think so since you seem to be very selective in what you read (or register) and what you don't. > is not really a technical sound argument IMHO. Moreover all the > alternatives seem to be horribly complicated and most of them also > seem to have several issues which will make them simply not work > reliable. I'm not an unreasonable person, or at least I like to think so, and if there really isn't a better solution then I won't object to this patch any longer. But as it stands I am not convinced that this is the best solution, so I think we should keep the discussion going for a bit and see if we really can't come up with something that will fix this for a more general case rather than just your particular use-case. It's evidently a recurring problem that others suffer from, too. Thierry
On Wed, Aug 27, 2014 at 10:00:23AM +0200, Hans de Goede wrote: > Hi, > > On 08/27/2014 08:54 AM, Thierry Reding wrote: > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > >> On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > >>>>>> Any decent enough SoC, with a decent support in the kernel will have > >>>>>> clocks for this, and I really wonder how simplefb will behave once its > >>>>>> clocks will be turned off... > >>>>> > >>>>> There are other devices besides ARM SoCs that may want to use this > >>>>> driver and that don't have clock support. > >>>> > >>>> And in this case, with this patch, simplefb will not claim any clock, > >>>> nor will fail probing. > >>>> > >>>>> But you're missing my point. What I'm saying is that the simplefb driver > >>>>> is meant to serve as a way to take over whatever framebuffer a firmware > >>>>> set up. Therefore I think it makes the most sense to assume that nothing > >>>>> needs to be controlled in any way since already been set up by firmware. > >>>>> Eventually there should be a driver that takes over from simplefb that > >>>>> knows how to properly handle the device's specifics, but that's not > >>>>> simplefb. > >>>> > >>>> I guess such a hand over if it were to happen in the kernel would > >>>> involve the second driver claiming the resources before the first one > >>>> release them. How is that different in this case? > >>> > >>> It's different in that that driver will be hardware specific and know > >>> exactly what clock and other resources are required. It will have a > >>> device-specific binding. > >> > >> Except that you made simplefb a generic driver. So we have two choices > >> here: either we don't anything but the trivial case, and no one with a > >> rather more advanced hardware will be able to use it, or we try to > >> grab any resource that might be of use, which means clocks, > >> regulators, reserved memory region, or whatever, so that we pretty > >> much cover all cases. It really is as simple as that. > > > > No, it should be even simpler. simplefb shouldn't have to know any of > > this. It's just taking what firmware has set up and uses that. It's a > > stop-gap solution to provide information on the display until a real > > driver can be loaded and initializes the display hardware properly. > > > >>>>> The goal of this patch series is to keep clocks from being turned off. > >>>>> But that's not what it does. What it does is turn clocks on to prevent > >>>>> them from being turned off. In my opinion that's a workaround for a > >>>>> deficiency in the kernel (and the firmware/kernel interface) and I think > >>>>> it should be fixed at the root. So a much better solution would be to > >>>>> establish a way for firmware to communicate to the kernel that a given > >>>>> resource has been enabled by firmware and shouldn't be disabled. Such a > >>>>> solution can be implement for all types of resources and can be reused > >>>>> by all drivers since they don't have to worry about these details. > >>>> > >>>> Mike Turquette repeatedly said that he was against such a DT property: > >>>> https://lkml.org/lkml/2014/5/12/693 > >>> > >>> Mike says in that email that he's opposing the addition of a property > >>> for clocks that is the equivalent of regulator-always-on. That's not > >>> what this is about. If at all it'd be a property to mark a clock that > >>> should not be disabled by default because it's essential. > >> > >> It's just semantic. How is "a clock that should not be disabled by > >> default because it's essential" not a clock that stays always on? > > > > Because a clock that should not be disabled by default can be turned off > > when appropriate. A clock that is always on can't be turned off. > > > >> Plus, you should read the mail further. It's clearly said that > >> consumer drivers should call clk_prepare_enable themselves, and that > >> the only exception to that is the case where we don't have such a > >> driver (and only valid as a temporary exception, which I guess you'll > >> soon turn into temporary-until-a-drm-kms-driver-is-merged). > > > > Exactly. simplefb is only a temporary measure. It's not meant to be used > > as a full-blown framebuffer driver. There are two use-cases where it is > > an appropriate solution: > > > > 1) As a stop-gap solution for when the platform doesn't support a full > > display driver yet. In that case you will want simplefb to stay > > around forever. > > > > 2) To get early boot output before a full display driver can be loaded > > in which case simplefb should go away after the display driver has > > taken over. > > > > In case of 2) the most straightforward solution is to not disable any > > clocks until all drivers have had a chance to claim them. When the full > > driver has been probed it should have claimed the clocks so they would > > no longer get disabled. > > Please read my long reply from yesterday evening why this will never > work, as there is not a well defined moment when "all drivers have had a > chance to claim them" > > I've been doing low level Linux development for 10 years now and I've > worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past. Ah, so this is now officially a pissing contest, is it? Thierry
On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: [...] > > > > > Mike Turquette repeatedly said that he was against such a DT property: > > > > > https://lkml.org/lkml/2014/5/12/693 > > > > > > > > Mike says in that email that he's opposing the addition of a property > > > > for clocks that is the equivalent of regulator-always-on. That's not > > > > what this is about. If at all it'd be a property to mark a clock that > > > > should not be disabled by default because it's essential. > > > > > > It's just semantic. How is "a clock that should not be disabled by > > > default because it's essential" not a clock that stays always on? > > > > Because a clock that should not be disabled by default can be turned off > > when appropriate. A clock that is always on can't be turned off. > > If a clock is essential, then it should never be disabled. Or we don't > share the same meaning of essential. Essential for the particular use-case. > > > > But you can work around that for now by making the relevant clocks > > > > always on and remove that workaround once a real driver is loaded > > > > that knows how to handle them properly. > > > > > > So, let me get this straight. The clock provider driver should behave > > > as a clock consumer because it knows that in some cases, it might not > > > have any willingful enough consumer? Doesn't that even ring your > > > this-is-a-clear-abstraction-violation-bell just a tiny bit? > > > > No. The clock driver should simply not allow the clocks to be disabled > > if it isn't safe to do so. > > Again, we do seem to differ on our interpretation of an essential > clock. To me, it is perfectly safe to disable the clocks. The system > will still be responding, the memory will be there, the CPU will keep > running, and we do have a way to recover from that disabled to clock > (for example to enable it back). > > Plus, again, in Mike's mail, it's clearly said that adding hacks like > this to the clock driver should only be considered in the case where > we don't have a consuming driver, which is not our case here. Well, that depends on what you mean by the consuming driver. simplefb isn't a traditional driver in that sense. There will eventually, in a more fully-featured system, be a driver that properly consumes the clocks. Now, since this is only a temporary measure I think it's one of the cases where having this encoded in the clock driver would be appropriate. Thierry
On Wed, Aug 27, 2014 at 10:55:38AM +0200, Maxime Ripard wrote: > Hi! > > On Wed, Aug 27, 2014 at 10:37:36AM +0200, Geert Uytterhoeven wrote: > > Hi Maxime, > > > > On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard > > <maxime.ripard@free-electrons.com> wrote: > > > On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote: > > >> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote: > > >> > > >> > It is not clear to me where the hardware resources should be listed in > > >> > DT, being it a simplefb node or part of the actual hardware device node > > >> > properly marked as dynamic boot defaults or something else? It's > > >> > somewhere inbetween hardware and virtual device, and somewhat volatile. > > >> > As far as simplefb is concerned it is a hardware desription of the > > >> > framebuffer, but for a kms driver it's no more than firmware handover of > > >> > boottime settings and ceases to exists once the kms driver have > > >> > reconfigured the hardware. > > >> > > >> Is simplefb something that should be in the device tree distinctly in > > >> the first place - shouldn't it be a subset of the functionality of the > > >> video nodes? It's the same hardware being driven differently. > > > > > > Therorically, yes, but that would mean knowing beforehand what the > > > final binding will look like, even before submitting the driver. Since > > > the bindings are always reviewed, and most of the time changed > > > slightly, that wouldn't work very well with the DT as a stable ABI > > > policy I guess. > > > > If you don't know how the bindings for a device will look like at the time of > > writing your DTS, you're always screwed, whether you add a simpefb > > node or not. > > > > If you know how the bindings look like, just add the device, with an extra > > "linux,simplefb" compatibility value. > > If you don't know how the bindings look like, do your utter best in > > guessing. Your DTS must be amended later anyway, either because > > you guessed wrong[*] (in case you added a node to have simplefb > > working), or because you have to add a real device node (in case you > > didn't add one for simplefb). > > Let's be conservative and consider the case where we would guess > wrong. > > If we just rely on a simplefb node, when reviewing and integrating the > "new" bindings to describe accureately the various IPs involved in the > display path, we would obviously create new compatibles for > them. Since it's new compatibles, we can come up with any binding we'd > like, without have to consider the backward compatibility, since it's > a new binding. > > Then, we just remove the simplefb, all is good. > > If we were to try to create our bindings for all the IPs involved, and > were not pleased with the binding anymore when merging the driver, > then we would have to break the bindings, since we don't introduce a > new compatible anymore, but modifying an existing one. It's usually a bad idea to merge a binding without an appropriate implementation thereof in a driver. It's been done in the past and very often has resulted in bindings that turned out unusable. You could start out small and only describe the various individual IP blocks that exist in the hardware with reg, interrupts, clock, etc. properties, most of which should be known up front. Then you could try to find out where simplefb fits best. Thierry
ons 2014-08-27 klockan 10:55 +0200 skrev Maxime Ripard: > If we just rely on a simplefb node, when reviewing and integrating the > "new" bindings to describe accureately the various IPs involved in the > display path, we would obviously create new compatibles for > them. Since it's new compatibles, we can come up with any binding we'd > like, without have to consider the backward compatibility, since it's > a new binding. My gut feeling is that the attributes needed by simplefb belong in the actual hardware node somehow, with a standard layout so it can be applied on all compatible video devices. The complication is that these attributes is volatile and not really set/defined by hardware. Similar to UART baud rate (and kernel command line if you like). Regards Henrik
On Wed, Aug 27, 2014 at 11:05:29AM +0200, Geert Uytterhoeven wrote: > On Wed, Aug 27, 2014 at 10:55 AM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > >> >> Is simplefb something that should be in the device tree distinctly in > >> >> the first place - shouldn't it be a subset of the functionality of the > >> >> video nodes? It's the same hardware being driven differently. > >> > > >> > Therorically, yes, but that would mean knowing beforehand what the > >> > final binding will look like, even before submitting the driver. Since > >> > the bindings are always reviewed, and most of the time changed > >> > slightly, that wouldn't work very well with the DT as a stable ABI > >> > policy I guess. > >> > >> If you don't know how the bindings for a device will look like at the time of > >> writing your DTS, you're always screwed, whether you add a simpefb > >> node or not. > >> > >> If you know how the bindings look like, just add the device, with an extra > >> "linux,simplefb" compatibility value. > >> If you don't know how the bindings look like, do your utter best in > >> guessing. Your DTS must be amended later anyway, either because > >> you guessed wrong[*] (in case you added a node to have simplefb > >> working), or because you have to add a real device node (in case you > >> didn't add one for simplefb). > > > > Let's be conservative and consider the case where we would guess > > wrong. > > > > If we just rely on a simplefb node, when reviewing and integrating the > > "new" bindings to describe accureately the various IPs involved in the > > display path, we would obviously create new compatibles for > > them. Since it's new compatibles, we can come up with any binding we'd > > like, without have to consider the backward compatibility, since it's > > a new binding. > > > > Then, we just remove the simplefb, all is good. > > I would keep the simplefb compatible value. Else you break compatibility > with old kernels that don't have your new driver. Yes, true. Since the simplefb will be injected by u-boot, it will be there anyway.
Hi, On 08/27/2014 11:31 AM, Thierry Reding wrote: > Can you please fix you mail setup. You're addressing me directly in this > email, yet I'm neither in To: nor Cc: headers. That's really annoying. Sorry, I'll pay closer attention that you're in the To when I'm directly addressing you next time. > On Tue, Aug 26, 2014 at 09:59:00PM +0200, Hans de Goede wrote: <snip> >>> Either way, Mark already suggested a different alternative in another >>> subthread, namely to add a new kind of checkpoint at which subsystems >>> can call a "disable unused" function that's called later than a late >>> initcall. This is not going to fix things for you immediately because >>> the clocks will still be switched off (only later) if you don't have >>> a real driver that's claiming the clocks. But you can work around that >>> for now by making the relevant clocks always on and remove that >>> workaround once a real driver is loaded that knows how to handle them >>> properly. >> >> This will simply not work, and is a ton more complicated then >> simply teaching simplefb about a clocks property, which is a really simple >> and clean patch. >> >> First of all let me explain why this won't work. When should those >> subsystems call this "later then a late initcall" call ? the kms driver >> may very well be a kernel module, which will be loaded by userspace, >> so we would need this to be triggered by userspace, but when ? >> >> When the initrd is done? What then if the kms driver is not in the initrd, >> so maybe when udev is done enumerating devices, but what then if the kernel >> is still enumerating hardware at this time? > > Usually an initrd knows how to wait for all or a given set of devices to > be probed. udev is pretty good for that. udev does not have a clue how to wait for all devices, it can be used to wait for a specific device to show up, that is true, but this requires adding specific logic for this (class of) device to the initrd init process, which is really sub-optimal. >> Will we just put a sleep 30 >> in our initscripts to make sure the kernel is done scanning all busses, >> will that be long enough? Maybe we need to re-introduce the scsi_wait_done >> kernel module which was added as an ugly hack to allow userspace to wait >> for the kernel to have finished scanning scsi (and ata / sata) busses, >> for controllers found at the time the module was load. Which never worked >> reliable, because it would be possible that the controller itself still >> needed to be discovered. We've spend years cleaning up userspace enough >> to be able to kill scsi_wait_done. We've already been down this road of >> waiting for hw enumeration to be done, and the problem is that on >> modern systems *it is never done*. > > Those are mostly integration issues. If you don't load the driver from > initrd then I don't think anybody will complain when there's flicker > when it finally gets loaded. The whole point of this is to have early > access to the framebuffer and get display and graphics going as soon as > possible, so please don't pull in hypothetical scenarios where people > manually load drivers half an hour after the system has booted. In another mail in this thread you've mentioned 2 uses for simplefb: 1) Serving as *the* fb until a kms driver is present 2) Allowing for output of early kernel boot messages. I agree that those are the 2 use-cases we're dealing with here. Note that the whole discussion we're having hear about disabling the clocks once hardware probing is "done", is moot for 1. One possible way mentioned of dealing with 1. is to mark these clocks as always on for now, and then we need to drop this hack (I cannot call this anything else then a hack), once we get kms, meaning interdependencies between the dropping patch and the kms adding patches, etc. All not very pretty. For 2. We have what you call "mostly integration issues". Allow me to copy and paste in another bit of this thread here: >> I've been doing low level Linux development for 10 years now and I've >> worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past. > > Ah, so this is now officially a pissing contest, is it? No I'm merely trying to explain that I've a ton of experience with what you seem to think of as merely integration issues, and they are a royal PITA, and thus to be avoided at all costs. Really, please just trust me on this one, doing the whole add some later then late init call type is a bad idea. >> So second of all, Thierry, what exactly is the technical argument against >> adding support for dealing with clocks to simplefb ? > > I've already stated technical arguments against it. You could just go > back and read the thread again, or would you rather want me to repeat > them for your convenience? It is a long thread, IIRC your main arguments against this are: 1) You don't want this kind of platform-specific knowledge in simplefb 2) That simplefb is supposed to be simple, and this is not simple As for 1. several people have already argued that clocks as such are an abstract concept, found one many platforms and as such are not platform specific. Any generic hw driver dealing with embedded platforms, be it ahci, ohci, ehci, etc. has ended up adding abstract support for things like clocks (and other resources). I say abstract support here, since to all these drivers the support was added in a way that no platform specific knowledge is necessary, they just deal with clocks not caring about the specific underlying clock implementation, clock names, etc. This really is unavoidable. The framebuffer memory needed was mentioned as another example of a resource in this thread. You suggested using the new mem-reserve framework for this. I agree that that is the right thing to do, but even then we still need a way to tie this reserved mem to the simplefb, so that the kernel can know when it is no longer used and can be added to the general memory pool. If you look at how almost all dt bindings work, then the dt node for a device specifies the resources needed, I don't see why simplefb would be so special that it should be different here. I agree that it is important to get the abstractions right here, but to me it seems that the right abstraction is to be consistent with how all other devices are abstracted and to add needed resources to the dt node for the simplefb. As for 2. Yes this will add some more code to simplefb, more code but code which is not complicated in anyway, so the simplefb code would become larger but still stay simple. Were as all the suggested alternatives sofar are at least an order of magnitude more complicated, crossing many subsystems, demanding coordination with userspace, etc. >> I've heard a lot of people in favor of it, > > Of course you have, all the people in favour of it are sunxi people that > really want to see this merged because it gives them working display > after U-Boot. I don't think that that is entirely fair towards Maxime, yes Maxime is a sunxi person, but he usually pushed back hard against anything he considers not clean enough, even if it would enable highly desirable functionality. > >> and only pretty much you against it, > > At least Stephen also spoke out about it. But this quickly devolved into > the kind of thread you want to get out of as quickly as possible, so I > can't blame him for not continuing this discussion. In fact I'm very > much regretting getting into this in the first place. You clearly have > no intention to even consider any possible other solution that what was > posted, so it's pretty useless to try and convince you. > >> and your argument so far seems to boil down to "I don't like it", > > I'm not surprised that you think so since you seem to be very selective > in what you read (or register) and what you don't. See above, I've summarized the 2 arguments which you've made as "I don't like it" before since to me they seem to be subjective arguments, you say it is too platform specific, but where is the divide line for something being _too_ platform specific ? I realize in the end everything is pretty much subjective. I apologize for the "I don't like it" characterization that was not fair of me. > >> is not really a technical sound argument IMHO. Moreover all the >> alternatives seem to be horribly complicated and most of them also >> seem to have several issues which will make them simply not work >> reliable. > > I'm not an unreasonable person, or at least I like to think so, and if > there really isn't a better solution then I won't object to this patch > any longer. That is good to hear. > But as it stands I am not convinced that this is the best solution I'm very much open to a better solution, but so far everything suggested is so much more complicated, that I don;t consider any of the suggestions done so far better. > so I think we should keep the discussion going for a bit and > see if we really can't come up with something that will fix this for a > more general case rather than just your particular use-case. It's > evidently a recurring problem that others suffer from, too. Yes needing resources despite the device being behind some generic interface where the main driver code for that interface has no knowledge about such resources sofar, because e.g. on PC-s this was not necessary is a recurring problem, which has been solved already 3 times at least, see the ohci-, ehci- and ahci-platform drivers, and for all 3 the conclusion was that the best solution was to simple add the resources to the dt-node, and add code to claim and enable them. Regards, Hans > > Thierry >
On Wed, Aug 27, 2014 at 6:35 AM, Hans de Goede <hdegoede@redhat.com> wrote: >> On 08/27/2014 11:31 AM, Thierry Reding wrote: snip > In another mail in this thread you've mentioned 2 uses for simplefb: > > 1) Serving as *the* fb until a kms driver is present > 2) Allowing for output of early kernel boot messages. > > I agree that those are the 2 use-cases we're dealing with here. > > Note that the whole discussion we're having hear about disabling the clocks > once hardware probing is "done", is moot for 1. One possible way mentioned > of dealing with 1. is to mark these clocks as always on for now, and then we > need to drop this hack (I cannot call this anything else then a hack), once > we get kms, meaning interdependencies between the dropping patch and the kms > adding patches, etc. All not very pretty. > > For 2. We have what you call "mostly integration issues". Allow me to copy > and paste in another bit of this thread here: These are two independent problems. > 1) Serving as *the* fb until a kms driver is present Why can't we whip together a real device specific fbdev driver and quit trying to run without a device specific driver in place? History has shown that to be bad idea except during early boot when there isn't a choice. fbdev drivers are pretty simple things and you don't have to make one that implements every feature. fbdev driver don't even have to implement mode setting. They can just live with the mode initially provided and refuse to change it. Communicating this initial mode info on the kernel command line is already supported so we don't even need a device tree change. This is the transition tool until KMS gets written. But it needs to be a device specific fbdev implementation, not simplefb. That's because the device specific driver is going to claim those clocks and regulators. > 2) Allowing for output of early kernel boot messages. For this to work the kernel needs to know two things - address of the framebuffer and the x/y layout. Then just nuke this display when the kernel finishes the boot process. One of two things will happen. 1) the fbdev driver has been loaded. It has claimed the clocks, taken care of memory, etc. If KMS is around, load it instead. When kernel tries to nuke the display nothing will happen. 2) Your display goes blank because the kernel has disabled clocks and reclaimed the memory. Take that as a hint to write a fbdev driver. pages more snipped....
Hi All, On Wed, Aug 27, 2014 at 6:37 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Maxime, > > On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: >> On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote: >>> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote: >>> >>> > It is not clear to me where the hardware resources should be listed in >>> > DT, being it a simplefb node or part of the actual hardware device node >>> > properly marked as dynamic boot defaults or something else? It's >>> > somewhere inbetween hardware and virtual device, and somewhat volatile. >>> > As far as simplefb is concerned it is a hardware desription of the >>> > framebuffer, but for a kms driver it's no more than firmware handover of >>> > boottime settings and ceases to exists once the kms driver have >>> > reconfigured the hardware. >>> >>> Is simplefb something that should be in the device tree distinctly in >>> the first place - shouldn't it be a subset of the functionality of the >>> video nodes? It's the same hardware being driven differently. >> >> Therorically, yes, but that would mean knowing beforehand what the >> final binding will look like, even before submitting the driver. Since >> the bindings are always reviewed, and most of the time changed >> slightly, that wouldn't work very well with the DT as a stable ABI >> policy I guess. > > If you don't know how the bindings for a device will look like at the time of > writing your DTS, you're always screwed, whether you add a simpefb > node or not. > > If you know how the bindings look like, just add the device, with an extra > "linux,simplefb" compatibility value. > If you don't know how the bindings look like, do your utter best in > guessing. Your DTS must be amended later anyway, either because > you guessed wrong[*] (in case you added a node to have simplefb > working), or because you have to add a real device node (in case you > didn't add one for simplefb). > > [*] Actually you may have guessed right, in which case it'll continue > working as-is, and everybody will be happy. > Whether you want to keep backwards-compatibility in your future driver > with the "guessed wrong" node is up to you. I apologise if I'm stepping on anyone's toes or horribly misrepresenting device tree's capabilities, but couldn't we start out with something like: disp@whatever { compatible = "sunxi,sun4i-disp"; clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>; } as our binding? u-boot could then set up a framebuffer and mangle this to: disp@whatever { compatible = "sunxi,sun4i-disp", "linux,simplefb"; clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>; // simplefb stuff } Hell, if we have a reserved memory driver, couldn't we then mangle this to something like: disp@whatever { compatible = "sunxi,sun4i-disp", "linux,simplefb"; clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>; // simplefb stuff // reserved mem stuff } simplefb is modified to be smart enough to grab the clocks (that's a small amount of code that almost every DT-enabled driver has, right?) and if we're going to use the reserved memory driver to ensure nobody messes with it's in-memory framebuffer, it'll have to deal with that too. Eventually sunxi-kms gets submitted, and it takes the "sunxi,sun4i-disp" compatible, which we've already defined, however we need some more stuff so we just add more stuff to the binding and end up with something like: disp@whatever { compatible = "sunxi,sun4i-disp"; clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>; // sunxi-kms stuff } I believe there'd be no backwards compatibility issues as we're only adding, not subtracting or modifying. We already know which clocks are required for the display IP, right? And if not, we can add them. I believe this minimises the mangling u-boot has to do (at least initially) and is forward and backward compatible. That'd work, right? Thanks,
Hi, On 08/27/2014 02:44 PM, jonsmirl@gmail.com wrote: <snip> >> 1) Serving as *the* fb until a kms driver is present > > Why can't we whip together a real device specific fbdev driver and > quit trying to run without a device specific driver in place? History > has shown that to be bad idea except during early boot when there > isn't a choice. Writing a kms driver and getting it upstream is not a simple task, and starting with a too limited implementation is dangerous as we need to get the devicetree binding first from the get go. IOW this is not really a realistic answer to problem 1. <snip> >> 2) Allowing for output of early kernel boot messages. > > For this to work the kernel needs to know two things - address of the > framebuffer and the x/y layout. Then just nuke this display when the > kernel finishes the boot process. Please read my earlier mails on how completely flawed it is to write "when the kernel finishes the boot process". The problem is that there is no way to really determine when this happens. Regards, Hans
On Wed, Aug 27, 2014 at 12:35:24PM +0200, Hans de Goede wrote: > Hi, > > On 08/27/2014 11:31 AM, Thierry Reding wrote: > > Can you please fix you mail setup. You're addressing me directly in this > > email, yet I'm neither in To: nor Cc: headers. That's really annoying. > > Sorry, I'll pay closer attention that you're in the To when I'm directly > addressing you next time. > > > On Tue, Aug 26, 2014 at 09:59:00PM +0200, Hans de Goede wrote: > > <snip> > > >>> Either way, Mark already suggested a different alternative in another > >>> subthread, namely to add a new kind of checkpoint at which subsystems > >>> can call a "disable unused" function that's called later than a late > >>> initcall. This is not going to fix things for you immediately because > >>> the clocks will still be switched off (only later) if you don't have > >>> a real driver that's claiming the clocks. But you can work around that > >>> for now by making the relevant clocks always on and remove that > >>> workaround once a real driver is loaded that knows how to handle them > >>> properly. > >> > >> This will simply not work, and is a ton more complicated then > >> simply teaching simplefb about a clocks property, which is a really simple > >> and clean patch. > >> > >> First of all let me explain why this won't work. When should those > >> subsystems call this "later then a late initcall" call ? the kms driver > >> may very well be a kernel module, which will be loaded by userspace, > >> so we would need this to be triggered by userspace, but when ? > >> > >> When the initrd is done? What then if the kms driver is not in the initrd, > >> so maybe when udev is done enumerating devices, but what then if the kernel > >> is still enumerating hardware at this time? > > > > Usually an initrd knows how to wait for all or a given set of devices to > > be probed. udev is pretty good for that. > > udev does not have a clue how to wait for all devices, it can be used to > wait for a specific device to show up, that is true, but this requires > adding specific logic for this (class of) device to the initrd init process, > which is really sub-optimal. I thought pretty much every distribution that did anything remotely related to early DRM/KMS like plymouth and whatnot already had this specific logic. It's been a while since I dealt with all that, but something roughly like this used to be present in pretty much all distributions: udevadm trigger --subsystem-match=graphics \ --subsystem-match=drm \ --subsystem-match=tty udevadm settle The effect of that being that udevadm settle would block as long as devices were still being probed. Admittedly this was all long before deferred probing was introduced, so I have no idea how well it plays with it. But optimizing deferred probing is an entirely different topic currently under discussion that should help sorting this out, too. > >> Will we just put a sleep 30 > >> in our initscripts to make sure the kernel is done scanning all busses, > >> will that be long enough? Maybe we need to re-introduce the scsi_wait_done > >> kernel module which was added as an ugly hack to allow userspace to wait > >> for the kernel to have finished scanning scsi (and ata / sata) busses, > >> for controllers found at the time the module was load. Which never worked > >> reliable, because it would be possible that the controller itself still > >> needed to be discovered. We've spend years cleaning up userspace enough > >> to be able to kill scsi_wait_done. We've already been down this road of > >> waiting for hw enumeration to be done, and the problem is that on > >> modern systems *it is never done*. > > > > Those are mostly integration issues. If you don't load the driver from > > initrd then I don't think anybody will complain when there's flicker > > when it finally gets loaded. The whole point of this is to have early > > access to the framebuffer and get display and graphics going as soon as > > possible, so please don't pull in hypothetical scenarios where people > > manually load drivers half an hour after the system has booted. > > In another mail in this thread you've mentioned 2 uses for simplefb: > > 1) Serving as *the* fb until a kms driver is present > 2) Allowing for output of early kernel boot messages. > > I agree that those are the 2 use-cases we're dealing with here. > > Note that the whole discussion we're having hear about disabling the clocks > once hardware probing is "done", is moot for 1. One possible way mentioned > of dealing with 1. is to mark these clocks as always on for now, and then we > need to drop this hack (I cannot call this anything else then a hack), once > we get kms, meaning interdependencies between the dropping patch and the kms > adding patches, etc. All not very pretty. I don't think it's that bad. When your real driver is merged you can still keep the clocks always on for a release cycle if you worry about any fallout. It's unlikely to going to be your last driver that you merge and people aren't immediately going to build products based on it, so it's unlikely that anyone will notice the wasted power. > For 2. We have what you call "mostly integration issues". Allow me to copy > and paste in another bit of this thread here: > > >> I've been doing low level Linux development for 10 years now and I've > >> worked a lot on storage (raid, iscsi, fcoe support, etc.) in the past. > > > > Ah, so this is now officially a pissing contest, is it? > > No I'm merely trying to explain that I've a ton of experience with what > you seem to think of as merely integration issues, and they are a royal PITA, > and thus to be avoided at all costs. Really, please just trust me on this > one, doing the whole add some later then late init call type is a bad idea. Well, you could equally just trust me on this one when I say adding clock support for this driver is bad. Stalemate. > >> So second of all, Thierry, what exactly is the technical argument against > >> adding support for dealing with clocks to simplefb ? > > > > I've already stated technical arguments against it. You could just go > > back and read the thread again, or would you rather want me to repeat > > them for your convenience? > > It is a long thread, IIRC your main arguments against this are: > > 1) You don't want this kind of platform-specific knowledge in simplefb > 2) That simplefb is supposed to be simple, and this is not simple > > As for 1. several people have already argued that clocks as such are > an abstract concept, found one many platforms and as such are not > platform specific. That alone doesn't justify this patch. SoCs often have a requirement to enable clocks in a specific order, for example. Other systems may need to coordinate clocks with resets in a specific order or enable power domains and such. All that is very SoC specific and if we don't make it very clear what this driver assumes about the state of the system, then we can't object to anybody adding support for those later on either. The result of that is going to be a driver that needs to handle all kinds of combinations of resources. So while clocks themselves are fairly generic the way in which they need to be used is highly platform-specific. Let me also reiterate what I've said a couple of times already. The issue here isn't that simplefb needs to manage these clocks in any way. Firmware has already set them up so that display works. The reason why you need this patch is so that the clock framework doesn't automatically turn them off. With the proposed patch the driver enables these clocks, but that's not what it needs to do, they are already on. It's a work around to prevent the clock framework into not disabling them. So as far as I'm concerned this points to a fundamental issue with how the clock framework handles unused clocks. Therefore that is the problem that should be solved, not worked around. > Any generic hw driver dealing with embedded platforms, be it ahci, ohci, > ehci, etc. has ended up adding abstract support for things like clocks > (and other resources). I say abstract support here, since to all these > drivers the support was added in a way that no platform specific knowledge > is necessary, they just deal with clocks not caring about the specific > underlying clock implementation, clock names, etc. Yes, I know that. You and I both had a very similar discussion not so long ago. But the above aren't quite the same as simplefb. The devices follow at least a well-known standard (EHCI, OHCI, AHCI), so there's bound to be more commonalities between them than between the various display devices that simplefb potentially can support. Interestingly though, if you look at what hardware really is supported by the generic drivers that deal with embedded platforms, there isn't all that much diversity (I've manually stripped out non-DTS occurrences since they aren't relevant for this discussion): $ git grep -n generic-ohci arch/arm/boot/dts/sun4i-a10.dtsi:451: compatible = "allwinner,sun4i-a10-ohci", "generic-ohci"; arch/arm/boot/dts/sun4i-a10.dtsi:492: compatible = "allwinner,sun4i-a10-ohci", "generic-ohci"; arch/arm/boot/dts/sun5i-a10s.dtsi:403: compatible = "allwinner,sun5i-a10s-ohci", "generic-ohci"; arch/arm/boot/dts/sun5i-a13.dtsi:376: compatible = "allwinner,sun5i-a13-ohci", "generic-ohci"; arch/arm/boot/dts/sun6i-a31.dtsi:410: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci"; arch/arm/boot/dts/sun6i-a31.dtsi:432: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci"; arch/arm/boot/dts/sun6i-a31.dtsi:443: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci"; arch/arm/boot/dts/sun7i-a20.dtsi:535: compatible = "allwinner,sun7i-a20-ohci", "generic-ohci"; arch/arm/boot/dts/sun7i-a20.dtsi:576: compatible = "allwinner,sun7i-a20-ohci", "generic-ohci"; arch/powerpc/boot/dts/akebono.dts:144: compatible = "ibm,476gtr-ohci", "generic-ohci"; arch/powerpc/boot/dts/akebono.dts:151: compatible = "ibm,476gtr-ohci", "generic-ohci"; $ git grep -n generic-ehci arch/arm/boot/dts/rk3288.dtsi:199: compatible = "generic-ehci"; arch/arm/boot/dts/rk3288.dtsi:210: compatible = "generic-ehci"; arch/arm/boot/dts/sun4i-a10.dtsi:441: compatible = "allwinner,sun4i-a10-ehci", "generic-ehci"; arch/arm/boot/dts/sun4i-a10.dtsi:482: compatible = "allwinner,sun4i-a10-ehci", "generic-ehci"; arch/arm/boot/dts/sun5i-a10s.dtsi:393: compatible = "allwinner,sun5i-a10s-ehci", "generic-ehci"; arch/arm/boot/dts/sun5i-a13.dtsi:366: compatible = "allwinner,sun5i-a13-ehci", "generic-ehci"; arch/arm/boot/dts/sun6i-a31.dtsi:399: compatible = "allwinner,sun6i-a31-ehci", "generic-ehci"; arch/arm/boot/dts/sun6i-a31.dtsi:421: compatible = "allwinner,sun6i-a31-ehci", "generic-ehci"; arch/arm/boot/dts/sun7i-a20.dtsi:525: compatible = "allwinner,sun7i-a20-ehci", "generic-ehci"; arch/arm/boot/dts/sun7i-a20.dtsi:566: compatible = "allwinner,sun7i-a20-ehci", "generic-ehci"; arch/powerpc/boot/dts/akebono.dts:130: compatible = "ibm,476gtr-ehci", "generic-ehci"; For generic-ahci there aren't any matches, but here are a few that are marked compatible with it using different compatible strings: $ git grep -n snps,spear-ahci arch/arm/boot/dts/spear1310.dtsi:60: compatible = "snps,spear-ahci"; arch/arm/boot/dts/spear1310.dtsi:69: compatible = "snps,spear-ahci"; arch/arm/boot/dts/spear1310.dtsi:78: compatible = "snps,spear-ahci"; arch/arm/boot/dts/spear1340.dtsi:43: compatible = "snps,spear-ahci"; $ git grep -n snps,exynos5440-ahci arch/arm/boot/dts/exynos5440.dtsi:241: compatible = "snps,exynos5440-ahci"; $ git grep -n 'ibm,476gtr-ahci' arch/powerpc/boot/dts/akebono.dts:123: compatible = "ibm,476gtr-ahci"; $ git grep -n 'snps,dwc-ahci' arch/arm/boot/dts/dra7.dtsi:1049: compatible = "snps,dwc-ahci"; arch/arm/boot/dts/exynos5250.dtsi:265: compatible = "snps,dwc-ahci"; arch/arm/boot/dts/omap5.dtsi:919: compatible = "snps,dwc-ahci"; That looks fairly homogenous to me. There are also things like this (from the ahci_platform.c driver): if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci")) hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ; With more supported hardware there's bound to be more quirks like that. So there's a couple of SoCs and boards that actually are generic enough to work with a generic driver. And then there's a whole bunch of other drivers for hardware that's compliant with the same standard yet needs different drivers. To me that's a clear indication that there isn't enough genericity to warrant a generic driver in the first place. The commonality is in the functionality and defined by the standard registers. But there's little to no commonality in how that interface is glued into the SoC. Luckily the above subsystems implement the standard hardware programming in a library, so that non-generic drivers can still make use of most of the generic code. > This really is unavoidable. The framebuffer memory needed was mentioned > as another example of a resource in this thread. You suggested using the > new mem-reserve framework for this. I agree that that is the right thing > to do, but even then we still need a way to tie this reserved mem to > the simplefb, so that the kernel can know when it is no longer used > and can be added to the general memory pool. Yes, memory handling seems like it isn't handled optimally by simplefb. To be fair the simplefb driver predates the reserved-memory bindings. I think it might be worthwhile to investigate on how to extend the binding to make use of that, though. I think the assumption currently is that the framebuffer memory will be lost forever (it needs to be carved out of the physical memory that the kernel gets to see). I couldn't find any code that returned reserved-memory to the kernel, but it's certainly a feature that I think we want. Especially if the firmware framebuffer is large (as in 1080p). > If you look at how almost all dt bindings work, then the dt node for > a device specifies the resources needed, I don't see why simplefb would > be so special that it should be different here. I agree that it is > important to get the abstractions right here, but to me it seems that > the right abstraction is to be consistent with how all other devices > are abstracted and to add needed resources to the dt node for the > simplefb. But simplefb is fundamentally different from other devices. It isn't a physical device at all. It's a virtual device that reuses resources as set up by some other piece of code (firmware). It implies that there's nothing that needs to be managed. It should only create a framebuffer with the given parameters and allow the kernel (and userspace) to render into it. The only way you can deal with such virtual, completely generic devices is by being very explicit about the requirements. For simplefb the assumptions are that firmware set everything up and passes information about what it set up to the kernel so that it can be reused. None of the resources need to be explicitly managed because they have all been properly configured. For that reason, again, I think the right way is for the kernel not to switch off any of the used resources. If you want to equate simplefb to other drivers then it is fundamentally broken anyway. Given only what's in the DTB the simplefb driver won't be able to do anything useful. Consider what would happen if the firmware didn't set up all the resources. Then the DT is missing resets, power supplies and all that to make it work. > As for 2. Yes this will add some more code to simplefb, more code but > code which is not complicated in anyway, so the simplefb code would > become larger but still stay simple. Were as all the suggested > alternatives sofar are at least an order of magnitude more complicated, > crossing many subsystems, demanding coordination with userspace, etc. I'm not objecting to this patch because of the complexity, but because it is an illusion that the code is in any way generic. > > so I think we should keep the discussion going for a bit and > > see if we really can't come up with something that will fix this for a > > more general case rather than just your particular use-case. It's > > evidently a recurring problem that others suffer from, too. > > Yes needing resources despite the device being behind some generic > interface where the main driver code for that interface has no knowledge > about such resources sofar, because e.g. on PC-s this was not necessary > is a recurring problem, which has been solved already 3 times at least, > see the ohci-, ehci- and ahci-platform drivers, and for all 3 the conclusion > was that the best solution was to simple add the resources to the dt-node, > and add code to claim and enable them. And as I pointed out above in all three cases the generic platform driver is only marginally useful because many SoCs have specific requirements that make them incompatible with the generic driver. Thierry
On Wed, Aug 27, 2014 at 8:50 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 08/27/2014 02:44 PM, jonsmirl@gmail.com wrote: > > <snip> > >>> 1) Serving as *the* fb until a kms driver is present >> >> Why can't we whip together a real device specific fbdev driver and >> quit trying to run without a device specific driver in place? History >> has shown that to be bad idea except during early boot when there >> isn't a choice. > > Writing a kms driver and getting it upstream is not a simple task, and > starting with a too limited implementation is dangerous as we need to get > the devicetree binding first from the get go. I didn't say write a kms driver, I said write a fbdev driver. Look in drivers/video/fbdev. fbdev drivers are used on many embedded systems. Its what we used for years before kms came along. > > IOW this is not really a realistic answer to problem 1. > > <snip> > >>> 2) Allowing for output of early kernel boot messages. >> >> For this to work the kernel needs to know two things - address of the >> framebuffer and the x/y layout. Then just nuke this display when the >> kernel finishes the boot process. > > Please read my earlier mails on how completely flawed it is to write > "when the kernel finishes the boot process". The problem is that there > is no way to really determine when this happens. Let me rephrase that, when the existing code in the clk system and regulator systems decide to do their clean up the display will get nuked. > > Regards, > > Hans > > -- > 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 Wed, Aug 27, 2014 at 02:50:18PM +0200, Hans de Goede wrote: > Hi, > > On 08/27/2014 02:44 PM, jonsmirl@gmail.com wrote: > > <snip> > > >> 1) Serving as *the* fb until a kms driver is present > > > > Why can't we whip together a real device specific fbdev driver and > > quit trying to run without a device specific driver in place? History > > has shown that to be bad idea except during early boot when there > > isn't a choice. > > Writing a kms driver and getting it upstream is not a simple task, and > starting with a too limited implementation is dangerous as we need to get > the devicetree binding first from the get go. > > IOW this is not really a realistic answer to problem 1. > > <snip> > > >> 2) Allowing for output of early kernel boot messages. > > > > For this to work the kernel needs to know two things - address of the > > framebuffer and the x/y layout. Then just nuke this display when the > > kernel finishes the boot process. > > Please read my earlier mails on how completely flawed it is to write > "when the kernel finishes the boot process". The problem is that there > is no way to really determine when this happens. That doesn't mean we can't create new ways to signal this. One option would be for userspace to signal this, another would be to have critical subsystems broadcast such events so that things like the clock framework can listen for those and react accordingly. The fact remains that disabling all unused clocks unconditinally at late_initcall time is not (always) the right thing to do. What this really is is a policy decision and traditionally we don't like to dictate policy in the kernel, so having something like an optional feature that would allow userspace to notify the kernel that it's initialized doesn't sound like a bad idea. This doesn't necessarily have to mean that all possible devices have been probed (users could choose to blacklist modules, etc.) but just an indication that the system is done with essential tasks (for desktop distributions I would assume that means the graphical login manager has started). Thierry
Hi Julian, On Wed, Aug 27, 2014 at 10:45:14PM +1000, Julian Calaby wrote: > Hi All, > > On Wed, Aug 27, 2014 at 6:37 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > Hi Maxime, > > > > On Wed, Aug 27, 2014 at 10:22 AM, Maxime Ripard > > <maxime.ripard@free-electrons.com> wrote: > >> On Wed, Aug 27, 2014 at 08:40:57AM +0100, Mark Brown wrote: > >>> On Tue, Aug 26, 2014 at 08:40:09PM +0200, Henrik Nordström wrote: > >>> > >>> > It is not clear to me where the hardware resources should be listed in > >>> > DT, being it a simplefb node or part of the actual hardware device node > >>> > properly marked as dynamic boot defaults or something else? It's > >>> > somewhere inbetween hardware and virtual device, and somewhat volatile. > >>> > As far as simplefb is concerned it is a hardware desription of the > >>> > framebuffer, but for a kms driver it's no more than firmware handover of > >>> > boottime settings and ceases to exists once the kms driver have > >>> > reconfigured the hardware. > >>> > >>> Is simplefb something that should be in the device tree distinctly in > >>> the first place - shouldn't it be a subset of the functionality of the > >>> video nodes? It's the same hardware being driven differently. > >> > >> Therorically, yes, but that would mean knowing beforehand what the > >> final binding will look like, even before submitting the driver. Since > >> the bindings are always reviewed, and most of the time changed > >> slightly, that wouldn't work very well with the DT as a stable ABI > >> policy I guess. > > > > If you don't know how the bindings for a device will look like at the time of > > writing your DTS, you're always screwed, whether you add a simpefb > > node or not. > > > > If you know how the bindings look like, just add the device, with an extra > > "linux,simplefb" compatibility value. > > If you don't know how the bindings look like, do your utter best in > > guessing. Your DTS must be amended later anyway, either because > > you guessed wrong[*] (in case you added a node to have simplefb > > working), or because you have to add a real device node (in case you > > didn't add one for simplefb). > > > > [*] Actually you may have guessed right, in which case it'll continue > > working as-is, and everybody will be happy. > > Whether you want to keep backwards-compatibility in your future driver > > with the "guessed wrong" node is up to you. > > I apologise if I'm stepping on anyone's toes or horribly > misrepresenting device tree's capabilities, but couldn't we start out > with something like: > > disp@whatever { > compatible = "sunxi,sun4i-disp"; > clocks = <&stuff 1>, <&stuff 2>, <&stuff 3>; Unfortunately, it's slightly more complicated than that, since there is several clocks involved, that feed several different IPs that need to stay on. In this patch, and given the current code we have, we need to have the gates for the HDMI controller, the LCD controller and the display backend, which are three different IPs, with one clock each, that would be described by three different nodes. Maxime
Hi, On Wed, Aug 27, 2014 at 11:31:24AM +0200, Thierry Reding wrote: > > and only pretty much you against it, > > At least Stephen also spoke out about it. But this quickly devolved into > the kind of thread you want to get out of as quickly as possible, so I > can't blame him for not continuing this discussion. In fact I'm very > much regretting getting into this in the first place. You clearly have > no intention to even consider any possible other solution that what was > posted, so it's pretty useless to try and convince you. No. I'm ready to consider any solution that doesn't involve hardcoding these clocks in the kernel clock driver. I believe that is a pretty similar, yet opposite, stance than the one you're currently making. Maxime
Hi, On 08/27/2014 02:56 PM, Thierry Reding wrote: > On Wed, Aug 27, 2014 at 12:35:24PM +0200, Hans de Goede wrote: <snip> >>>> So second of all, Thierry, what exactly is the technical argument against >>>> adding support for dealing with clocks to simplefb ? >>> >>> I've already stated technical arguments against it. You could just go >>> back and read the thread again, or would you rather want me to repeat >>> them for your convenience? >> >> It is a long thread, IIRC your main arguments against this are: >> >> 1) You don't want this kind of platform-specific knowledge in simplefb >> 2) That simplefb is supposed to be simple, and this is not simple >> >> As for 1. several people have already argued that clocks as such are >> an abstract concept, found one many platforms and as such are not >> platform specific. > > That alone doesn't justify this patch. SoCs often have a requirement to > enable clocks in a specific order, for example. Other systems may need > to coordinate clocks with resets in a specific order or enable power > domains and such. All that is very SoC specific and if we don't make it > very clear what this driver assumes about the state of the system, then > we can't object to anybody adding support for those later on either. The > result of that is going to be a driver that needs to handle all kinds of > combinations of resources. The problems your talking about here all have to do with ordering how things are enabled, but what we're talking about here is keeping things enabled which are already enabled by the bootloader, so there is no ordering problem. > So while clocks themselves are fairly generic the way in which they need > to be used is highly platform-specific. > > Let me also reiterate what I've said a couple of times already. The > issue here isn't that simplefb needs to manage these clocks in any way. > Firmware has already set them up so that display works. The reason why > you need this patch is so that the clock framework doesn't automatically > turn them off. With the proposed patch the driver enables these clocks, > but that's not what it needs to do, they are already on. It's a work > around to prevent the clock framework into not disabling them. > > So as far as I'm concerned this points to a fundamental issue with how > the clock framework handles unused clocks. Therefore that is the problem > that should be solved, not worked around. Hmm I see, in my mind the problem is not that the clk framework disables unused clocks, but that no one is marking the clocks in question as used. Someone should mark these clocks as used so that they do not get disabled. We could add a clk_mark_in_use function and have the simplefb patch call that instead of clk_prepare_and_enable, or maybe even better just only claim the clks and teach the clk framework to not disable claimed clk in its cleanup run. That would make it clear that simplefb is not enabling anything, just claiming resource its need to avoid them getting removed from underneath simplefb, would that work for you ? >> Any generic hw driver dealing with embedded platforms, be it ahci, ohci, >> ehci, etc. has ended up adding abstract support for things like clocks >> (and other resources). I say abstract support here, since to all these >> drivers the support was added in a way that no platform specific knowledge >> is necessary, they just deal with clocks not caring about the specific >> underlying clock implementation, clock names, etc. > > Yes, I know that. You and I both had a very similar discussion not so > long ago. But the above aren't quite the same as simplefb. The devices > follow at least a well-known standard (EHCI, OHCI, AHCI), so there's > bound to be more commonalities between them than between the various > display devices that simplefb potentially can support. > > Interestingly though, if you look at what hardware really is supported > by the generic drivers that deal with embedded platforms, there isn't > all that much diversity (I've manually stripped out non-DTS occurrences > since they aren't relevant for this discussion): That is because the generic platform drivers were only added recently to stop the insanity where each new soc got its own ohci-soc.c / ehci-soc.c file. <snip> > arch/arm/boot/dts/rk3288.dtsi:199: compatible = "generic-ehci"; > arch/arm/boot/dts/rk3288.dtsi:210: compatible = "generic-ehci"; (offtopic) And I see that despite the recent addition, we actually already have our first non sunxi user, good, I didn't even know the rockchip guys were using this :) And I know that there are quite a few more in the pipeline (waiting for their usb phy and dt patches to get merged). <snip> > So there's a couple of SoCs and boards that actually are generic enough > to work with a generic driver. And then there's a whole bunch of other > drivers for hardware that's compliant with the same standard yet needs > different drivers. No, there are a lot of other drivers which were written before someone decided that having 10-20 drivers which were 90% copy and paste of each other was a bad idea, but we're really going offtopic here. <snip> >> This really is unavoidable. The framebuffer memory needed was mentioned >> as another example of a resource in this thread. You suggested using the >> new mem-reserve framework for this. I agree that that is the right thing >> to do, but even then we still need a way to tie this reserved mem to >> the simplefb, so that the kernel can know when it is no longer used >> and can be added to the general memory pool. > > Yes, memory handling seems like it isn't handled optimally by simplefb. > To be fair the simplefb driver predates the reserved-memory bindings. I > think it might be worthwhile to investigate on how to extend the binding > to make use of that, though. I think the assumption currently is that > the framebuffer memory will be lost forever (it needs to be carved out > of the physical memory that the kernel gets to see). I couldn't find any > code that returned reserved-memory to the kernel, but it's certainly a > feature that I think we want. Especially if the firmware framebuffer is > large (as in 1080p). Ack. >> If you look at how almost all dt bindings work, then the dt node for >> a device specifies the resources needed, I don't see why simplefb would >> be so special that it should be different here. I agree that it is >> important to get the abstractions right here, but to me it seems that >> the right abstraction is to be consistent with how all other devices >> are abstracted and to add needed resources to the dt node for the >> simplefb. > > But simplefb is fundamentally different from other devices. It isn't a > physical device at all. It's a virtual device that reuses resources as > set up by some other piece of code (firmware). It implies that there's > nothing that needs to be managed. It should only create a framebuffer > with the given parameters and allow the kernel (and userspace) to render > into it. > > The only way you can deal with such virtual, completely generic devices > is by being very explicit about the requirements. For simplefb the > assumptions are that firmware set everything up and passes information > about what it set up to the kernel so that it can be reused. > None of the resources need to be explicitly managed because they have all > been properly configured. For that reason, again, I think the right way is > for the kernel not to switch off any of the used resources. The key word here is "the used resources" to me this is not about simlefb managing resources, but marking them as used as long as it needs them, like it will need to do for the reserved mem chunk. <snip> >> As for 2. Yes this will add some more code to simplefb, more code but >> code which is not complicated in anyway, so the simplefb code would >> become larger but still stay simple. Were as all the suggested >> alternatives sofar are at least an order of magnitude more complicated, >> crossing many subsystems, demanding coordination with userspace, etc. > > I'm not objecting to this patch because of the complexity, but because > it is an illusion that the code is in any way generic. Ok. Regards, Hans
On Wed, Aug 27, 2014 at 02:56:14PM +0200, Thierry Reding wrote: > > >> So second of all, Thierry, what exactly is the technical argument against > > >> adding support for dealing with clocks to simplefb ? > > > > > > I've already stated technical arguments against it. You could just go > > > back and read the thread again, or would you rather want me to repeat > > > them for your convenience? > > > > It is a long thread, IIRC your main arguments against this are: > > > > 1) You don't want this kind of platform-specific knowledge in simplefb > > 2) That simplefb is supposed to be simple, and this is not simple > > > > As for 1. several people have already argued that clocks as such are > > an abstract concept, found one many platforms and as such are not > > platform specific. > > That alone doesn't justify this patch. SoCs often have a requirement to > enable clocks in a specific order, for example. Other systems may need > to coordinate clocks with resets in a specific order or enable power > domains and such. All that is very SoC specific and if we don't make it > very clear what this driver assumes about the state of the system, then > we can't object to anybody adding support for those later on either. The > result of that is going to be a driver that needs to handle all kinds of > combinations of resources. Which is not an issue in our case, since the firmware enabled them already. > So while clocks themselves are fairly generic the way in which they need > to be used is highly platform-specific. > > Let me also reiterate what I've said a couple of times already. The > issue here isn't that simplefb needs to manage these clocks in any way. > Firmware has already set them up so that display works. The reason why > you need this patch is so that the clock framework doesn't automatically > turn them off. With the proposed patch the driver enables these clocks, > but that's not what it needs to do, they are already on. It's a work > around to prevent the clock framework into not disabling them. > > So as far as I'm concerned this points to a fundamental issue with how > the clock framework handles unused clocks. Therefore that is the problem > that should be solved, not worked around. Well, this is again a philosophical difference them. I find like the right thing to do for the clock framework to disable the clocks that are enabled if no one reports them as used. That also comes down to all the drivers that require a clock to stay enabled should report to the clock framework to tell that it uses this clock. Fortunately, we have everything in place to do so. > > Any generic hw driver dealing with embedded platforms, be it ahci, ohci, > > ehci, etc. has ended up adding abstract support for things like clocks > > (and other resources). I say abstract support here, since to all these > > drivers the support was added in a way that no platform specific knowledge > > is necessary, they just deal with clocks not caring about the specific > > underlying clock implementation, clock names, etc. > > Yes, I know that. You and I both had a very similar discussion not so > long ago. But the above aren't quite the same as simplefb. The devices > follow at least a well-known standard (EHCI, OHCI, AHCI), so there's > bound to be more commonalities between them than between the various > display devices that simplefb potentially can support. > > Interestingly though, if you look at what hardware really is supported > by the generic drivers that deal with embedded platforms, there isn't > all that much diversity (I've manually stripped out non-DTS occurrences > since they aren't relevant for this discussion): > > $ git grep -n generic-ohci > arch/arm/boot/dts/sun4i-a10.dtsi:451: compatible = "allwinner,sun4i-a10-ohci", "generic-ohci"; > arch/arm/boot/dts/sun4i-a10.dtsi:492: compatible = "allwinner,sun4i-a10-ohci", "generic-ohci"; > arch/arm/boot/dts/sun5i-a10s.dtsi:403: compatible = "allwinner,sun5i-a10s-ohci", "generic-ohci"; > arch/arm/boot/dts/sun5i-a13.dtsi:376: compatible = "allwinner,sun5i-a13-ohci", "generic-ohci"; > arch/arm/boot/dts/sun6i-a31.dtsi:410: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci"; > arch/arm/boot/dts/sun6i-a31.dtsi:432: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci"; > arch/arm/boot/dts/sun6i-a31.dtsi:443: compatible = "allwinner,sun6i-a31-ohci", "generic-ohci"; > arch/arm/boot/dts/sun7i-a20.dtsi:535: compatible = "allwinner,sun7i-a20-ohci", "generic-ohci"; > arch/arm/boot/dts/sun7i-a20.dtsi:576: compatible = "allwinner,sun7i-a20-ohci", "generic-ohci"; > arch/powerpc/boot/dts/akebono.dts:144: compatible = "ibm,476gtr-ohci", "generic-ohci"; > arch/powerpc/boot/dts/akebono.dts:151: compatible = "ibm,476gtr-ohci", "generic-ohci"; > > $ git grep -n generic-ehci > arch/arm/boot/dts/rk3288.dtsi:199: compatible = "generic-ehci"; > arch/arm/boot/dts/rk3288.dtsi:210: compatible = "generic-ehci"; > arch/arm/boot/dts/sun4i-a10.dtsi:441: compatible = "allwinner,sun4i-a10-ehci", "generic-ehci"; > arch/arm/boot/dts/sun4i-a10.dtsi:482: compatible = "allwinner,sun4i-a10-ehci", "generic-ehci"; > arch/arm/boot/dts/sun5i-a10s.dtsi:393: compatible = "allwinner,sun5i-a10s-ehci", "generic-ehci"; > arch/arm/boot/dts/sun5i-a13.dtsi:366: compatible = "allwinner,sun5i-a13-ehci", "generic-ehci"; > arch/arm/boot/dts/sun6i-a31.dtsi:399: compatible = "allwinner,sun6i-a31-ehci", "generic-ehci"; > arch/arm/boot/dts/sun6i-a31.dtsi:421: compatible = "allwinner,sun6i-a31-ehci", "generic-ehci"; > arch/arm/boot/dts/sun7i-a20.dtsi:525: compatible = "allwinner,sun7i-a20-ehci", "generic-ehci"; > arch/arm/boot/dts/sun7i-a20.dtsi:566: compatible = "allwinner,sun7i-a20-ehci", "generic-ehci"; > arch/powerpc/boot/dts/akebono.dts:130: compatible = "ibm,476gtr-ehci", "generic-ehci"; > > For generic-ahci there aren't any matches, but here are a few that are > marked compatible with it using different compatible strings: > > $ git grep -n snps,spear-ahci > arch/arm/boot/dts/spear1310.dtsi:60: compatible = "snps,spear-ahci"; > arch/arm/boot/dts/spear1310.dtsi:69: compatible = "snps,spear-ahci"; > arch/arm/boot/dts/spear1310.dtsi:78: compatible = "snps,spear-ahci"; > arch/arm/boot/dts/spear1340.dtsi:43: compatible = "snps,spear-ahci"; > > $ git grep -n snps,exynos5440-ahci > arch/arm/boot/dts/exynos5440.dtsi:241: compatible = "snps,exynos5440-ahci"; > > $ git grep -n 'ibm,476gtr-ahci' > arch/powerpc/boot/dts/akebono.dts:123: compatible = "ibm,476gtr-ahci"; > > $ git grep -n 'snps,dwc-ahci' > arch/arm/boot/dts/dra7.dtsi:1049: compatible = "snps,dwc-ahci"; > arch/arm/boot/dts/exynos5250.dtsi:265: compatible = "snps,dwc-ahci"; > arch/arm/boot/dts/omap5.dtsi:919: compatible = "snps,dwc-ahci"; > > That looks fairly homogenous to me. There are also things like this > (from the ahci_platform.c driver): > > if (of_device_is_compatible(dev->of_node, "hisilicon,hisi-ahci")) > hpriv->flags |= AHCI_HFLAG_NO_FBS | AHCI_HFLAG_NO_NCQ; > > With more supported hardware there's bound to be more quirks like that. > > So there's a couple of SoCs and boards that actually are generic enough > to work with a generic driver. And then there's a whole bunch of other > drivers for hardware that's compliant with the same standard yet needs > different drivers. To me that's a clear indication that there isn't > enough genericity to warrant a generic driver in the first place. > > The commonality is in the functionality and defined by the standard > registers. But there's little to no commonality in how that interface is > glued into the SoC. Luckily the above subsystems implement the standard > hardware programming in a library, so that non-generic drivers can still > make use of most of the generic code. To be fair, these additions are a couple of release old, and are quite recent. You can't really make any judgement based on barely 2 releases of history. > > If you look at how almost all dt bindings work, then the dt node for > > a device specifies the resources needed, I don't see why simplefb would > > be so special that it should be different here. I agree that it is > > important to get the abstractions right here, but to me it seems that > > the right abstraction is to be consistent with how all other devices > > are abstracted and to add needed resources to the dt node for the > > simplefb. > > But simplefb is fundamentally different from other devices. It isn't a > physical device at all. It's a virtual device that reuses resources as > set up by some other piece of code (firmware). It implies that there's > nothing that needs to be managed. It should only create a framebuffer > with the given parameters and allow the kernel (and userspace) to render > into it. Nothing should be managed, but everything should stay as is. Again, this is something that we all seem to agree on, yet differ completely on how to implement that. > The only way you can deal with such virtual, completely generic devices > is by being very explicit about the requirements. For simplefb the > assumptions are that firmware set everything up and passes information > about what it set up to the kernel so that it can be reused. None of the > resources need to be explicitly managed because they have all been > properly configured. For that reason, again, I think the right way is > for the kernel not to switch off any of the used resources. So, you're saying that the firmware should inform the kernel about what clocks it has set up? I do agree on that too. And it looks like we reached an agreement then, since it's exactly what this patch is relying on. > If you want to equate simplefb to other drivers then it is fundamentally > broken anyway. Given only what's in the DTB the simplefb driver won't be > able to do anything useful. Consider what would happen if the firmware > didn't set up all the resources. Then the DT is missing resets, power > supplies and all that to make it work. That would mean that the framebuffer wasn't working in the first place, which breaks the initial assumption, doesn't it? > > > so I think we should keep the discussion going for a bit and > > > see if we really can't come up with something that will fix this for a > > > more general case rather than just your particular use-case. It's > > > evidently a recurring problem that others suffer from, too. > > > > Yes needing resources despite the device being behind some generic > > interface where the main driver code for that interface has no knowledge > > about such resources sofar, because e.g. on PC-s this was not necessary > > is a recurring problem, which has been solved already 3 times at least, > > see the ohci-, ehci- and ahci-platform drivers, and for all 3 the conclusion > > was that the best solution was to simple add the resources to the dt-node, > > and add code to claim and enable them. > > And as I pointed out above in all three cases the generic platform > driver is only marginally useful because many SoCs have specific > requirements that make them incompatible with the generic driver. s/marginally useful/marginally used/
On Wed, Aug 27, 2014 at 03:44:46PM +0200, Hans de Goede wrote: > Hi, > > On 08/27/2014 02:56 PM, Thierry Reding wrote: > > On Wed, Aug 27, 2014 at 12:35:24PM +0200, Hans de Goede wrote: > > <snip> > > >>>> So second of all, Thierry, what exactly is the technical argument against > >>>> adding support for dealing with clocks to simplefb ? > >>> > >>> I've already stated technical arguments against it. You could just go > >>> back and read the thread again, or would you rather want me to repeat > >>> them for your convenience? > >> > >> It is a long thread, IIRC your main arguments against this are: > >> > >> 1) You don't want this kind of platform-specific knowledge in simplefb > >> 2) That simplefb is supposed to be simple, and this is not simple > >> > >> As for 1. several people have already argued that clocks as such are > >> an abstract concept, found one many platforms and as such are not > >> platform specific. > > > > That alone doesn't justify this patch. SoCs often have a requirement to > > enable clocks in a specific order, for example. Other systems may need > > to coordinate clocks with resets in a specific order or enable power > > domains and such. All that is very SoC specific and if we don't make it > > very clear what this driver assumes about the state of the system, then > > we can't object to anybody adding support for those later on either. The > > result of that is going to be a driver that needs to handle all kinds of > > combinations of resources. > > The problems your talking about here all have to do with ordering how > things are enabled, but what we're talking about here is keeping things > enabled which are already enabled by the bootloader, so there is no > ordering problem. See what I did there? I tricked you into saying what I've been saying all along. =) It's about keeping things enabled which have been enabled by the bootloader. That's the root of the problem. > > So while clocks themselves are fairly generic the way in which they need > > to be used is highly platform-specific. > > > > Let me also reiterate what I've said a couple of times already. The > > issue here isn't that simplefb needs to manage these clocks in any way. > > Firmware has already set them up so that display works. The reason why > > you need this patch is so that the clock framework doesn't automatically > > turn them off. With the proposed patch the driver enables these clocks, > > but that's not what it needs to do, they are already on. It's a work > > around to prevent the clock framework into not disabling them. > > > > So as far as I'm concerned this points to a fundamental issue with how > > the clock framework handles unused clocks. Therefore that is the problem > > that should be solved, not worked around. > > Hmm I see, in my mind the problem is not that the clk framework disables > unused clocks, but that no one is marking the clocks in question as used. > Someone should mark these clocks as used so that they do not get disabled. > > We could add a clk_mark_in_use function and have the simplefb patch call > that instead of clk_prepare_and_enable, or maybe even better just only > claim the clks and teach the clk framework to not disable claimed clk > in its cleanup run. That would make it clear that simplefb is not enabling > anything, just claiming resource its need to avoid them getting removed > from underneath simplefb, would that work for you ? That would be more accurate, but it boils down to the same problem where we still need a driver to claim the resources in some way whereas the problem is fundamentally one where the bootloader should be telling the kernel not to disable it. It's in fact the bootloader that's claiming the resources. > >> Any generic hw driver dealing with embedded platforms, be it ahci, ohci, > >> ehci, etc. has ended up adding abstract support for things like clocks > >> (and other resources). I say abstract support here, since to all these > >> drivers the support was added in a way that no platform specific knowledge > >> is necessary, they just deal with clocks not caring about the specific > >> underlying clock implementation, clock names, etc. > > > > Yes, I know that. You and I both had a very similar discussion not so > > long ago. But the above aren't quite the same as simplefb. The devices > > follow at least a well-known standard (EHCI, OHCI, AHCI), so there's > > bound to be more commonalities between them than between the various > > display devices that simplefb potentially can support. > > > > Interestingly though, if you look at what hardware really is supported > > by the generic drivers that deal with embedded platforms, there isn't > > all that much diversity (I've manually stripped out non-DTS occurrences > > since they aren't relevant for this discussion): > > That is because the generic platform drivers were only added recently > to stop the insanity where each new soc got its own ohci-soc.c / > ehci-soc.c file. Insanity is subjective. > > arch/arm/boot/dts/rk3288.dtsi:199: compatible = "generic-ehci"; > > arch/arm/boot/dts/rk3288.dtsi:210: compatible = "generic-ehci"; > > (offtopic) And I see that despite the recent addition, we actually already have our > first non sunxi user, good, I didn't even know the rockchip guys were using this :) FWIW, the rk3288 entry is broken. It should really contain an SoC specific compatible string as well. > And I know that there are quite a few more in the pipeline (waiting for their > usb phy and dt patches to get merged). As far as I'm concerned that's all a mistake (and catastrophy waiting to happen), but hey, there's only so many hours in a day so I've got to pick my battles. > > So there's a couple of SoCs and boards that actually are generic enough > > to work with a generic driver. And then there's a whole bunch of other > > drivers for hardware that's compliant with the same standard yet needs > > different drivers. > > No, there are a lot of other drivers which were written before someone > decided that having 10-20 drivers which were 90% copy and paste of each > other was a bad idea, but we're really going offtopic here. The copy and paste is for code that's platform specific. The clocks have different names, resets are different, supplies are different. The fact that many can currently use the same driver is likely just coincidence rather than design and it's entirely possible that at some point they'll add support for a more advanced feature that makes them incompatible with the rest of the generic drivers. And then you have a big mess because you need to add quirks all over the place. And this isn't all that far off-topic, since simplefb also needs to deal with this kind of situation. And what I've been arguing is that in order to really be generic it has to make assumptions, one of which is that it uses only resources that it doesn't need to explicitly handle. > >> If you look at how almost all dt bindings work, then the dt node for > >> a device specifies the resources needed, I don't see why simplefb would > >> be so special that it should be different here. I agree that it is > >> important to get the abstractions right here, but to me it seems that > >> the right abstraction is to be consistent with how all other devices > >> are abstracted and to add needed resources to the dt node for the > >> simplefb. > > > > But simplefb is fundamentally different from other devices. It isn't a > > physical device at all. It's a virtual device that reuses resources as > > set up by some other piece of code (firmware). It implies that there's > > nothing that needs to be managed. It should only create a framebuffer > > with the given parameters and allow the kernel (and userspace) to render > > into it. > > > > The only way you can deal with such virtual, completely generic devices > > is by being very explicit about the requirements. For simplefb the > > assumptions are that firmware set everything up and passes information > > about what it set up to the kernel so that it can be reused. > > > None of the resources need to be explicitly managed because they have all > > been properly configured. For that reason, again, I think the right way is > > for the kernel not to switch off any of the used resources. > > The key word here is "the used resources" to me this is not about simlefb > managing resources, but marking them as used as long as it needs them, like > it will need to do for the reserved mem chunk. The difference between the clocks and the memory resource is that the driver needs to directly access the memory (it needs to map it and provide a userspace mapping for it) whereas it doesn't need to touch the clocks (except to workaround a Linux-specific implementation detail). Thierry
On Wed, Aug 27, 2014 at 03:56:09PM +0200, Maxime Ripard wrote: > On Wed, Aug 27, 2014 at 02:56:14PM +0200, Thierry Reding wrote: > > > >> So second of all, Thierry, what exactly is the technical argument against > > > >> adding support for dealing with clocks to simplefb ? > > > > > > > > I've already stated technical arguments against it. You could just go > > > > back and read the thread again, or would you rather want me to repeat > > > > them for your convenience? > > > > > > It is a long thread, IIRC your main arguments against this are: > > > > > > 1) You don't want this kind of platform-specific knowledge in simplefb > > > 2) That simplefb is supposed to be simple, and this is not simple > > > > > > As for 1. several people have already argued that clocks as such are > > > an abstract concept, found one many platforms and as such are not > > > platform specific. > > > > That alone doesn't justify this patch. SoCs often have a requirement to > > enable clocks in a specific order, for example. Other systems may need > > to coordinate clocks with resets in a specific order or enable power > > domains and such. All that is very SoC specific and if we don't make it > > very clear what this driver assumes about the state of the system, then > > we can't object to anybody adding support for those later on either. The > > result of that is going to be a driver that needs to handle all kinds of > > combinations of resources. > > Which is not an issue in our case, since the firmware enabled them > already. Exactly, so why do you need them at all? You shouldn't have to. > > So there's a couple of SoCs and boards that actually are generic enough > > to work with a generic driver. And then there's a whole bunch of other > > drivers for hardware that's compliant with the same standard yet needs > > different drivers. To me that's a clear indication that there isn't > > enough genericity to warrant a generic driver in the first place. > > > > The commonality is in the functionality and defined by the standard > > registers. But there's little to no commonality in how that interface is > > glued into the SoC. Luckily the above subsystems implement the standard > > hardware programming in a library, so that non-generic drivers can still > > make use of most of the generic code. > > To be fair, these additions are a couple of release old, and are quite > recent. You can't really make any judgement based on barely 2 releases > of history. Okay, fair enough. Time will tell. It's still kind of odd to declare a driver to be generic without attempting to make the majority of existing drivers work with it. > > > If you look at how almost all dt bindings work, then the dt node for > > > a device specifies the resources needed, I don't see why simplefb would > > > be so special that it should be different here. I agree that it is > > > important to get the abstractions right here, but to me it seems that > > > the right abstraction is to be consistent with how all other devices > > > are abstracted and to add needed resources to the dt node for the > > > simplefb. > > > > But simplefb is fundamentally different from other devices. It isn't a > > physical device at all. It's a virtual device that reuses resources as > > set up by some other piece of code (firmware). It implies that there's > > nothing that needs to be managed. It should only create a framebuffer > > with the given parameters and allow the kernel (and userspace) to render > > into it. > > Nothing should be managed, but everything should stay as is. Again, > this is something that we all seem to agree on, yet differ completely > on how to implement that. I'm arguing that if everything should stay as is, then nobody should have to do anything. > > The only way you can deal with such virtual, completely generic devices > > is by being very explicit about the requirements. For simplefb the > > assumptions are that firmware set everything up and passes information > > about what it set up to the kernel so that it can be reused. None of the > > resources need to be explicitly managed because they have all been > > properly configured. For that reason, again, I think the right way is > > for the kernel not to switch off any of the used resources. > > So, you're saying that the firmware should inform the kernel about > what clocks it has set up? Yes. The problem is that even if we tell the kernel that clocks have been set up it may still decide to disable them when they are unused. That's the whole purpose of clk_disable_unused() as I understand it. Whatever mechanism we introduce needs to specifically imply that the clock must stay enabled. > > If you want to equate simplefb to other drivers then it is fundamentally > > broken anyway. Given only what's in the DTB the simplefb driver won't be > > able to do anything useful. Consider what would happen if the firmware > > didn't set up all the resources. Then the DT is missing resets, power > > supplies and all that to make it work. > > That would mean that the framebuffer wasn't working in the first > place, which breaks the initial assumption, doesn't it? Exactly. simplefb gets away with an incomplete description of the hardware in DT because of these assumptions. So you can't draw parallels to other drivers since they don't make these assumptions and need to be described completely. Thierry
On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: > On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: > > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: > > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > [...] > > > > > > Mike Turquette repeatedly said that he was against such a DT property: > > > > > > https://lkml.org/lkml/2014/5/12/693 > > > > > > > > > > Mike says in that email that he's opposing the addition of a property > > > > > for clocks that is the equivalent of regulator-always-on. That's not > > > > > what this is about. If at all it'd be a property to mark a clock that > > > > > should not be disabled by default because it's essential. > > > > > > > > It's just semantic. How is "a clock that should not be disabled by > > > > default because it's essential" not a clock that stays always on? > > > > > > Because a clock that should not be disabled by default can be turned off > > > when appropriate. A clock that is always on can't be turned off. > > > > If a clock is essential, then it should never be disabled. Or we don't > > share the same meaning of essential. > > Essential for the particular use-case. So, how would the clock driver would know about which use case we're in? How would it know about which display engine is currently running? How would it know about which video output is being set? Currently, we have two separate display engines, which can each output either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every one of these combinations would require different clocks. What clocks will we put in the driver? All of them? Maxime
Hello, On 27 August 2014 17:42, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: >> On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: >> > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: >> > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: >> > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: >> [...] >> > > > > > Mike Turquette repeatedly said that he was against such a DT property: >> > > > > > https://lkml.org/lkml/2014/5/12/693 >> > > > > >> > > > > Mike says in that email that he's opposing the addition of a property >> > > > > for clocks that is the equivalent of regulator-always-on. That's not >> > > > > what this is about. If at all it'd be a property to mark a clock that >> > > > > should not be disabled by default because it's essential. >> > > > >> > > > It's just semantic. How is "a clock that should not be disabled by >> > > > default because it's essential" not a clock that stays always on? >> > > >> > > Because a clock that should not be disabled by default can be turned off >> > > when appropriate. A clock that is always on can't be turned off. >> > >> > If a clock is essential, then it should never be disabled. Or we don't >> > share the same meaning of essential. >> >> Essential for the particular use-case. > > So, how would the clock driver would know about which use case we're > in? How would it know about which display engine is currently running? > How would it know about which video output is being set? > > Currently, we have two separate display engines, which can each output > either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every > one of these combinations would require different clocks. What clocks > will we put in the driver? All of them? > since simplefb cannot be extended how about adding, say, dtfb which claims the resources from dt and then instantiates a simplefb once the resources are claimed? That is have a dtfb which has the clocks assigned and has simplefb as child dt node. Thanks Michal
On Wed, Aug 27, 2014 at 10:57:29PM +0200, Michal Suchanek wrote: > Hello, > > On 27 August 2014 17:42, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > > On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: > >> On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: > >> > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: > >> > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > >> > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > >> [...] > >> > > > > > Mike Turquette repeatedly said that he was against such a DT property: > >> > > > > > https://lkml.org/lkml/2014/5/12/693 > >> > > > > > >> > > > > Mike says in that email that he's opposing the addition of a property > >> > > > > for clocks that is the equivalent of regulator-always-on. That's not > >> > > > > what this is about. If at all it'd be a property to mark a clock that > >> > > > > should not be disabled by default because it's essential. > >> > > > > >> > > > It's just semantic. How is "a clock that should not be disabled by > >> > > > default because it's essential" not a clock that stays always on? > >> > > > >> > > Because a clock that should not be disabled by default can be turned off > >> > > when appropriate. A clock that is always on can't be turned off. > >> > > >> > If a clock is essential, then it should never be disabled. Or we don't > >> > share the same meaning of essential. > >> > >> Essential for the particular use-case. > > > > So, how would the clock driver would know about which use case we're > > in? How would it know about which display engine is currently running? > > How would it know about which video output is being set? > > > > Currently, we have two separate display engines, which can each output > > either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every > > one of these combinations would require different clocks. What clocks > > will we put in the driver? All of them? > > > > since simplefb cannot be extended how about adding, say, dtfb which > claims the resources from dt and then instantiates a simplefb once the > resources are claimed? That is have a dtfb which has the clocks > assigned and has simplefb as child dt node. I don't see how that changes anything. All you do is add another layer of indirection. The fundamental problem remains the same and isn't solved. Thierry
On Wed, Aug 27, 2014 at 05:42:21PM +0200, Maxime Ripard wrote: > On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: > > On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: > > > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: > > > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > > > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > > [...] > > > > > > > Mike Turquette repeatedly said that he was against such a DT property: > > > > > > > https://lkml.org/lkml/2014/5/12/693 > > > > > > > > > > > > Mike says in that email that he's opposing the addition of a property > > > > > > for clocks that is the equivalent of regulator-always-on. That's not > > > > > > what this is about. If at all it'd be a property to mark a clock that > > > > > > should not be disabled by default because it's essential. > > > > > > > > > > It's just semantic. How is "a clock that should not be disabled by > > > > > default because it's essential" not a clock that stays always on? > > > > > > > > Because a clock that should not be disabled by default can be turned off > > > > when appropriate. A clock that is always on can't be turned off. > > > > > > If a clock is essential, then it should never be disabled. Or we don't > > > share the same meaning of essential. > > > > Essential for the particular use-case. > > So, how would the clock driver would know about which use case we're > in? How would it know about which display engine is currently running? > How would it know about which video output is being set? > > Currently, we have two separate display engines, which can each output > either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every > one of these combinations would require different clocks. What clocks > will we put in the driver? All of them? Ideally the solution wouldn't involve hard-coding this into the clock driver at all. There should be a way for firmware to communicate to the kernel that a given clock shouldn't be disabled. Then since firmware already knows what it set up it can tell the kernel to not touch those. Thierry
On 28 August 2014 12:11, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 27, 2014 at 05:42:21PM +0200, Maxime Ripard wrote: >> On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: >> > On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: >> > > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: >> > > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: >> > > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: >> > [...] >> > > > > > > Mike Turquette repeatedly said that he was against such a DT property: >> > > > > > > https://lkml.org/lkml/2014/5/12/693 >> > > > > > >> > > > > > Mike says in that email that he's opposing the addition of a property >> > > > > > for clocks that is the equivalent of regulator-always-on. That's not >> > > > > > what this is about. If at all it'd be a property to mark a clock that >> > > > > > should not be disabled by default because it's essential. >> > > > > >> > > > > It's just semantic. How is "a clock that should not be disabled by >> > > > > default because it's essential" not a clock that stays always on? >> > > > >> > > > Because a clock that should not be disabled by default can be turned off >> > > > when appropriate. A clock that is always on can't be turned off. >> > > >> > > If a clock is essential, then it should never be disabled. Or we don't >> > > share the same meaning of essential. >> > >> > Essential for the particular use-case. >> >> So, how would the clock driver would know about which use case we're >> in? How would it know about which display engine is currently running? >> How would it know about which video output is being set? >> >> Currently, we have two separate display engines, which can each output >> either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every >> one of these combinations would require different clocks. What clocks >> will we put in the driver? All of them? > > Ideally the solution wouldn't involve hard-coding this into the clock > driver at all. There should be a way for firmware to communicate to the > kernel that a given clock shouldn't be disabled. Then since firmware > already knows what it set up it can tell the kernel to not touch those. > And that way is that it inserts into the simplefb node or whatever node the list of clocks that it programmed in order to make the framebuffer work. Do you know a better, more generic way than that? Thanks Michal
Hi, On 08/27/2014 04:16 PM, Thierry Reding wrote: <snip> >> The problems your talking about here all have to do with ordering how >> things are enabled, but what we're talking about here is keeping things >> enabled which are already enabled by the bootloader, so there is no >> ordering problem. > > See what I did there? I tricked you into saying what I've been saying > all along. =) He he :) > It's about keeping things enabled which have been enabled > by the bootloader. That's the root of the problem. Ok so we all seem to largely agree on most things here: 1) Somehow some clks must be marked used so as to not get disabled 2) We don't want to hardcode these clocks into the kernel (sunxi) clk driver, instead the bootloader should tell the kernel about these clocks. So the only point of discussion left seems to be how to do 2... <snip> >> Hmm I see, in my mind the problem is not that the clk framework disables >> unused clocks, but that no one is marking the clocks in question as used. >> Someone should mark these clocks as used so that they do not get disabled. >> >> We could add a clk_mark_in_use function and have the simplefb patch call >> that instead of clk_prepare_and_enable, or maybe even better just only >> claim the clks and teach the clk framework to not disable claimed clk >> in its cleanup run. That would make it clear that simplefb is not enabling >> anything, just claiming resource its need to avoid them getting removed >> from underneath simplefb, would that work for you ? > > That would be more accurate, but it boils down to the same problem where > we still need a driver to claim the resources in some way whereas the > problem is fundamentally one where the bootloader should be telling the > kernel not to disable it. It's in fact the bootloader that's claiming > the resources. Yes, but those resources do not belong to the bootloader in a sense that traditional bootloader / firmware claimed resources (e.g. acpi reserved resources) do. These traditional resources are claimed for ever. Where as these resources are claimed by the bootloader to keep the simplefb it provides working, as soon as the simplefb is no longer used, they become unused. <snip off-topic generic-ehci discussion> >> No, there are a lot of other drivers which were written before someone >> decided that having 10-20 drivers which were 90% copy and paste of each >> other was a bad idea, but we're really going offtopic here. > > The copy and paste is for code that's platform specific. The clocks have > different names, resets are different, supplies are different. The fact > that many can currently use the same driver is likely just coincidence > rather than design and it's entirely possible that at some point they'll > add support for a more advanced feature that makes them incompatible > with the rest of the generic drivers. And then you have a big mess > because you need to add quirks all over the place. > > And this isn't all that far off-topic, since simplefb also needs to deal > with this kind of situation. And what I've been arguing is that in order > to really be generic it has to make assumptions, one of which is that it > uses only resources that it doesn't need to explicitly handle. I can understand that you're worried about generic ?hci drivers dealing with clocks / resets / etc. As there may be strict ordering requirements there, but for simplefb that is not the case. All we're asking for is for a way to communicate 2 things to the kernel: 1) These resources are in use (we are not asking the kernel to do anything with them, rather the opposite, we're asking to leave them alone so no ordering issues) 2) Tie these resources to simplefb so that the kernel can know when they are no longer in use, and it may e.g. re-use the memory To me the most logical and also most "correct" way of modelling this is to put the resources inside the simplefb dt node. <snip> >> The key word here is "the used resources" to me this is not about simlefb >> managing resources, but marking them as used as long as it needs them, like >> it will need to do for the reserved mem chunk. > > The difference between the clocks and the memory resource is that the > driver needs to directly access the memory (it needs to map it and > provide a userspace mapping for it) whereas it doesn't need to touch the > clocks (except to workaround a Linux-specific implementation detail). Erm, no, the need to map the memory and the memory being a resource which may be released are an orthogonal problem. E.g. a system with dedicated framebuffer memory won't need to use a reserved main-memory chunk, nor need to worry about returning that mem when simplefb is no longer in use. Regards, Hans
On Thu, Aug 28, 2014 at 8:15 AM, Hans de Goede <hdegoede@redhat.com> wrote: > Hi, > > On 08/27/2014 04:16 PM, Thierry Reding wrote: > > <snip> > >>> The problems your talking about here all have to do with ordering how >>> things are enabled, but what we're talking about here is keeping things >>> enabled which are already enabled by the bootloader, so there is no >>> ordering problem. >> >> See what I did there? I tricked you into saying what I've been saying >> all along. =) > > He he :) > >> It's about keeping things enabled which have been enabled >> by the bootloader. That's the root of the problem. > > Ok so we all seem to largely agree on most things here: > > 1) Somehow some clks must be marked used so as to not get disabled > 2) We don't want to hardcode these clocks into the kernel (sunxi) clk > driver, instead the bootloader should tell the kernel about these clocks. > > So the only point of discussion left seems to be how to do 2... Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and whip together a device specific driver that claims the proper resources? And just implement the minimal about of fbdev possible? fbdev already is a driver library. --- An independent issue is the early printk equivalent. For that you need an address and the x/y layout > > <snip> > >>> Hmm I see, in my mind the problem is not that the clk framework disables >>> unused clocks, but that no one is marking the clocks in question as used. >>> Someone should mark these clocks as used so that they do not get disabled. >>> >>> We could add a clk_mark_in_use function and have the simplefb patch call >>> that instead of clk_prepare_and_enable, or maybe even better just only >>> claim the clks and teach the clk framework to not disable claimed clk >>> in its cleanup run. That would make it clear that simplefb is not enabling >>> anything, just claiming resource its need to avoid them getting removed >>> from underneath simplefb, would that work for you ? >> >> That would be more accurate, but it boils down to the same problem where >> we still need a driver to claim the resources in some way whereas the >> problem is fundamentally one where the bootloader should be telling the >> kernel not to disable it. It's in fact the bootloader that's claiming >> the resources. > > Yes, but those resources do not belong to the bootloader in a sense > that traditional bootloader / firmware claimed resources (e.g. acpi > reserved resources) do. These traditional resources are claimed for ever. > > Where as these resources are claimed by the bootloader to keep the simplefb > it provides working, as soon as the simplefb is no longer used, they become > unused. > > <snip off-topic generic-ehci discussion> > >>> No, there are a lot of other drivers which were written before someone >>> decided that having 10-20 drivers which were 90% copy and paste of each >>> other was a bad idea, but we're really going offtopic here. >> >> The copy and paste is for code that's platform specific. The clocks have >> different names, resets are different, supplies are different. The fact >> that many can currently use the same driver is likely just coincidence >> rather than design and it's entirely possible that at some point they'll >> add support for a more advanced feature that makes them incompatible >> with the rest of the generic drivers. And then you have a big mess >> because you need to add quirks all over the place. >> >> And this isn't all that far off-topic, since simplefb also needs to deal >> with this kind of situation. And what I've been arguing is that in order >> to really be generic it has to make assumptions, one of which is that it >> uses only resources that it doesn't need to explicitly handle. > > I can understand that you're worried about generic ?hci drivers dealing with > clocks / resets / etc. As there may be strict ordering requirements there, > but for simplefb that is not the case. > > All we're asking for is for a way to communicate 2 things to the kernel: > > 1) These resources are in use (we are not asking the kernel to do anything > with them, rather the opposite, we're asking to leave them alone so no > ordering issues) > > 2) Tie these resources to simplefb so that the kernel can know when they > are no longer in use, and it may e.g. re-use the memory > > To me the most logical and also most "correct" way of modelling this is to > put the resources inside the simplefb dt node. > > <snip> > >>> The key word here is "the used resources" to me this is not about simlefb >>> managing resources, but marking them as used as long as it needs them, like >>> it will need to do for the reserved mem chunk. >> >> The difference between the clocks and the memory resource is that the >> driver needs to directly access the memory (it needs to map it and >> provide a userspace mapping for it) whereas it doesn't need to touch the >> clocks (except to workaround a Linux-specific implementation detail). > > Erm, no, the need to map the memory and the memory being a resource > which may be released are an orthogonal problem. E.g. a system with > dedicated framebuffer memory won't need to use a reserved main-memory > chunk, nor need to worry about returning that mem when simplefb is no > longer in use. > > Regards, > > Hans > > -- > 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 Thu, Aug 28, 2014 at 3:22 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >> 2) We don't want to hardcode these clocks into the kernel (sunxi) clk >> driver, instead the bootloader should tell the kernel about these clocks. >> >> So the only point of discussion left seems to be how to do 2... > > Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and > whip together a device specific driver that claims the proper > resources? And just implement the minimal about of fbdev possible? > fbdev already is a driver library. Like... drivers/video/fbdev/offb.c? 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 Thu, Aug 28, 2014 at 10:20 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Aug 28, 2014 at 3:22 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>> 2) We don't want to hardcode these clocks into the kernel (sunxi) clk >>> driver, instead the bootloader should tell the kernel about these clocks. >>> >>> So the only point of discussion left seems to be how to do 2... >> >> Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and >> whip together a device specific driver that claims the proper >> resources? And just implement the minimal about of fbdev possible? >> fbdev already is a driver library. > > Like... drivers/video/fbdev/offb.c? I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton and use it as a template for making device specific versions of it. I don't see why there is so much resistance to just making device specific fb drivers. Whenever the KMS driver gets written just disable the device specific fb driver in the build. > > 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 Thu, Aug 28, 2014 at 4:33 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > On Thu, Aug 28, 2014 at 10:20 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Thu, Aug 28, 2014 at 3:22 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>>> 2) We don't want to hardcode these clocks into the kernel (sunxi) clk >>>> driver, instead the bootloader should tell the kernel about these clocks. >>>> >>>> So the only point of discussion left seems to be how to do 2... >>> >>> Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and >>> whip together a device specific driver that claims the proper >>> resources? And just implement the minimal about of fbdev possible? >>> fbdev already is a driver library. >> >> Like... drivers/video/fbdev/offb.c? > > I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton > and use it as a template for making device specific versions of it. > > I don't see why there is so much resistance to just making device > specific fb drivers. Whenever the KMS driver gets written just > disable the device specific fb driver in the build. I explicitly named offb, because it already supports living with the video mode initialized by Open Firmware, which is passed to the kernel in a device tree.
On Thu, Aug 28, 2014 at 10:37 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Aug 28, 2014 at 4:33 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >> On Thu, Aug 28, 2014 at 10:20 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Thu, Aug 28, 2014 at 3:22 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>>>> 2) We don't want to hardcode these clocks into the kernel (sunxi) clk >>>>> driver, instead the bootloader should tell the kernel about these clocks. >>>>> >>>>> So the only point of discussion left seems to be how to do 2... >>>> >>>> Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and >>>> whip together a device specific driver that claims the proper >>>> resources? And just implement the minimal about of fbdev possible? >>>> fbdev already is a driver library. >>> >>> Like... drivers/video/fbdev/offb.c? >> >> I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton >> and use it as a template for making device specific versions of it. >> >> I don't see why there is so much resistance to just making device >> specific fb drivers. Whenever the KMS driver gets written just >> disable the device specific fb driver in the build. > > I explicitly named offb, because it already supports living with the > video mode initialized by Open Firmware, which is passed to the kernel > in a device tree. Not sure how that works. It from a PowerMac arch/powerpc/platforms/powermac/bootx_init.c arch/powerpc/kernel/prom_init.c > > -- > 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 28 August 2014 16:33, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > On Thu, Aug 28, 2014 at 10:20 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Thu, Aug 28, 2014 at 3:22 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>>> 2) We don't want to hardcode these clocks into the kernel (sunxi) clk >>>> driver, instead the bootloader should tell the kernel about these clocks. >>>> >>>> So the only point of discussion left seems to be how to do 2... >>> >>> Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and >>> whip together a device specific driver that claims the proper >>> resources? And just implement the minimal about of fbdev possible? >>> fbdev already is a driver library. >> >> Like... drivers/video/fbdev/offb.c? > > I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton > and use it as a template for making device specific versions of it. > > I don't see why there is so much resistance to just making device > specific fb drivers. Whenever the KMS driver gets written just > disable the device specific fb driver in the build. Except that is not the goal here. The simplefb or whatever replacement is supposed to stay as a generic driver compiled into kernel whereas the complete platform-specific driver is supposed to be provided as module and loaded at the init system's leasure sometime during boot. This way you can have generic distribution kernel which supports many devices but does not have built-in support for every graphics hardware. Thanks Michal
On Thu, Aug 28, 2014 at 12:25 PM, Michal Suchanek <hramrach@gmail.com> wrote: > On 28 August 2014 16:33, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >> On Thu, Aug 28, 2014 at 10:20 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Thu, Aug 28, 2014 at 3:22 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>>>> 2) We don't want to hardcode these clocks into the kernel (sunxi) clk >>>>> driver, instead the bootloader should tell the kernel about these clocks. >>>>> >>>>> So the only point of discussion left seems to be how to do 2... >>>> >>>> Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and >>>> whip together a device specific driver that claims the proper >>>> resources? And just implement the minimal about of fbdev possible? >>>> fbdev already is a driver library. >>> >>> Like... drivers/video/fbdev/offb.c? >> >> I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton >> and use it as a template for making device specific versions of it. >> >> I don't see why there is so much resistance to just making device >> specific fb drivers. Whenever the KMS driver gets written just >> disable the device specific fb driver in the build. > > Except that is not the goal here. The simplefb or whatever replacement > is supposed to stay as a generic driver compiled into kernel whereas There is no generic solution to this problem as this entire thread has illustrated. The clocks/regulators needed by each SOC vary. So there are two solutions.. 1) modify simplefb to have some kind of heuristic that tries to guess what needs to be protected. A heuristic that is probably going to fail on every new SOC. 2) Spend a day implementing a device specific fbdev driver that does the correct thing all of the time. These drivers would sit in initrd and load before the clock/regulator clean up shuts everything off. Use the existing simplefb code as a template for doing this. > the complete platform-specific driver is supposed to be provided as > module and loaded at the init system's leasure sometime during boot. > This way you can have generic distribution kernel which supports many > devices but does not have built-in support for every graphics > hardware. > > Thanks > > Michal > > -- > 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 Thu, Aug 28, 2014 at 12:34:58PM -0400, jonsmirl@gmail.com wrote: > > 2) Spend a day implementing a device specific fbdev driver that does > the correct thing all of the time. These drivers would sit in initrd > and load before the clock/regulator clean up shuts everything off. Use > the existing simplefb code as a template for doing this. If that finally ends this discussion, i'll more than happily do it. I'm sure that we've all could've done this a hundred times in the time we've wasted here. Good call Geert. Luc Verhaegen.
On 28 August 2014 18:34, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > On Thu, Aug 28, 2014 at 12:25 PM, Michal Suchanek <hramrach@gmail.com> wrote: >> On 28 August 2014 16:33, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>> On Thu, Aug 28, 2014 at 10:20 AM, Geert Uytterhoeven >>> <geert@linux-m68k.org> wrote: >>>> On Thu, Aug 28, 2014 at 3:22 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: >>>>>> 2) We don't want to hardcode these clocks into the kernel (sunxi) clk >>>>>> driver, instead the bootloader should tell the kernel about these clocks. >>>>>> >>>>>> So the only point of discussion left seems to be how to do 2... >>>>> >>>>> Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and >>>>> whip together a device specific driver that claims the proper >>>>> resources? And just implement the minimal about of fbdev possible? >>>>> fbdev already is a driver library. >>>> >>>> Like... drivers/video/fbdev/offb.c? >>> >>> I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton >>> and use it as a template for making device specific versions of it. >>> >>> I don't see why there is so much resistance to just making device >>> specific fb drivers. Whenever the KMS driver gets written just >>> disable the device specific fb driver in the build. >> >> Except that is not the goal here. The simplefb or whatever replacement >> is supposed to stay as a generic driver compiled into kernel whereas > > There is no generic solution to this problem as this entire thread has > illustrated. The clocks/regulators needed by each SOC vary. > > So there are two solutions.. > 1) modify simplefb to have some kind of heuristic that tries to guess > what needs to be protected. A heuristic that is probably going to fail > on every new SOC. You do not need a heuristic when the bootloader that enabled the clocks and regulators can tell you which. And this patch was initially about adding support to simplefb to read from DT where u-boot fills in the clocks that are needed for HDMI output to keep running on sunxi. > > 2) Spend a day implementing a device specific fbdev driver that does > the correct thing all of the time. These drivers would sit in initrd > and load before the clock/regulator clean up shuts everything off. Use > the existing simplefb code as a template for doing this. But then you need to spend a day implementing such driver for every SoC and it will not work before initrd is loaded. In contrast simplefb with resource management support can work on any platform where the bootloader enables framebuffer and then can be compiled into generic kernel and work before even initrd is loaded. Thanks Michal
On Thu, Aug 28, 2014 at 12:11:41PM +0200, Thierry Reding wrote: > On Wed, Aug 27, 2014 at 05:42:21PM +0200, Maxime Ripard wrote: > > On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: > > > On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: > > > > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: > > > > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > > > > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > > > [...] > > > > > > > > Mike Turquette repeatedly said that he was against such a DT property: > > > > > > > > https://lkml.org/lkml/2014/5/12/693 > > > > > > > > > > > > > > Mike says in that email that he's opposing the addition of a property > > > > > > > for clocks that is the equivalent of regulator-always-on. That's not > > > > > > > what this is about. If at all it'd be a property to mark a clock that > > > > > > > should not be disabled by default because it's essential. > > > > > > > > > > > > It's just semantic. How is "a clock that should not be disabled by > > > > > > default because it's essential" not a clock that stays always on? > > > > > > > > > > Because a clock that should not be disabled by default can be turned off > > > > > when appropriate. A clock that is always on can't be turned off. > > > > > > > > If a clock is essential, then it should never be disabled. Or we don't > > > > share the same meaning of essential. > > > > > > Essential for the particular use-case. > > > > So, how would the clock driver would know about which use case we're > > in? How would it know about which display engine is currently running? > > How would it know about which video output is being set? > > > > Currently, we have two separate display engines, which can each output > > either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every > > one of these combinations would require different clocks. What clocks > > will we put in the driver? All of them? > > Ideally the solution wouldn't involve hard-coding this into the clock > driver at all. Cool, so we do agree on that too :) > There should be a way for firmware to communicate to the kernel that > a given clock shouldn't be disabled. And this patch was such an attempt. I guess it wasn't that far off then. > Then since firmware already knows what it set up it can tell the > kernel to not touch those. Somehow, I've been raised kernel-wise into thinking that you can never fully trust your firmware. Or at least that you should have a way to recover from any bug/bad behaviour from it. Moreover, the way I see it, there's a major flaw in having an attribute in the clock node: you don't even know if the clock is ever going to be used. If simplefb is not compiled in, you won't claim the clocks, and they will be disabled, which is imho a good thing. This case wouldn't be covered with an attribute at the clock node, because you don't have a link to what device/feature actually uses it in the system, and so you have to make the assumption that it will be used. And you will end up with clocks with a rather high rate running for nothing.
On 28 August 2014 12:08, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 27, 2014 at 10:57:29PM +0200, Michal Suchanek wrote: >> Hello, >> >> On 27 August 2014 17:42, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: >> > On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: >> >> On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: >> >> > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: >> >> > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: >> >> > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: >> >> [...] >> >> > > > > > Mike Turquette repeatedly said that he was against such a DT property: >> >> > > > > > https://lkml.org/lkml/2014/5/12/693 >> >> > > > > >> >> > > > > Mike says in that email that he's opposing the addition of a property >> >> > > > > for clocks that is the equivalent of regulator-always-on. That's not >> >> > > > > what this is about. If at all it'd be a property to mark a clock that >> >> > > > > should not be disabled by default because it's essential. >> >> > > > >> >> > > > It's just semantic. How is "a clock that should not be disabled by >> >> > > > default because it's essential" not a clock that stays always on? >> >> > > >> >> > > Because a clock that should not be disabled by default can be turned off >> >> > > when appropriate. A clock that is always on can't be turned off. >> >> > >> >> > If a clock is essential, then it should never be disabled. Or we don't >> >> > share the same meaning of essential. >> >> >> >> Essential for the particular use-case. >> > >> > So, how would the clock driver would know about which use case we're >> > in? How would it know about which display engine is currently running? >> > How would it know about which video output is being set? >> > >> > Currently, we have two separate display engines, which can each output >> > either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every >> > one of these combinations would require different clocks. What clocks >> > will we put in the driver? All of them? >> > >> >> since simplefb cannot be extended how about adding, say, dtfb which >> claims the resources from dt and then instantiates a simplefb once the >> resources are claimed? That is have a dtfb which has the clocks >> assigned and has simplefb as child dt node. > > I don't see how that changes anything. All you do is add another layer > of indirection. The fundamental problem remains the same and isn't > solved. It keeps clock code out of simplefb and provides driver for the kind of framebuffer set up by firmware that exists on sunxi, exynos, and probably many other SoCs. That is a framebuffer that needs some clocks and possibly regulators enabled to keep running because the reality of the platform is that it has clocks and regulators unlike some other platforms that do not. These clocks and regulators are used but not configured by the framebuffer and should be reclaimed when firmware framebuffer is disabled. This is the same as the chunk of memory used by simplefb which is currently lost but should be reclaimed when simplefb is disabled. This memory is not 'reserved for firmware' and unusable but reserved for framebuffer and the regulators are not 'always on' or 'should never be disabled' but should be enabled while framebuffer is used. As far as I can tell in DT this is expressed by creating a DT node associated with the framebuffer driver that tells the kernel that this memory, clocks and regulators are associated with the framebuffer driver and can be reclaimed if this driver is stopped or not enabled in the kernel at all. If you are going to be asinine about simplefb not getting support for managing any resource other than the memory chunk then another layer of indirection is required for platforms that have more resources to manage. If there is another way to associate resources with a driver in DT then please enlighten me. AFAICT simplefb is the framebuffer driver already in kernel closest to the driver that is required for sunxi - simplefb also relies on firmware to set up the framebuffer but unlike vesafb or efifb it already has DT integration. So the most efficient way to implement framebuffer for sunxi is to extend simplefb or if necessary add another layer of indirection under simplefb. If there is a better fitting driver in the kernel then please enlighten me and the developer that wrote this patch what driver it would be. Thanks Michal
On Fri, Aug 29, 2014 at 07:13:22AM +0200, Michal Suchanek wrote: > On 28 August 2014 12:08, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Wed, Aug 27, 2014 at 10:57:29PM +0200, Michal Suchanek wrote: > >> Hello, > >> > >> On 27 August 2014 17:42, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > >> > On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: > >> >> On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: > >> >> > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: > >> >> > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > >> >> > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > >> >> [...] > >> >> > > > > > Mike Turquette repeatedly said that he was against such a DT property: > >> >> > > > > > https://lkml.org/lkml/2014/5/12/693 > >> >> > > > > > >> >> > > > > Mike says in that email that he's opposing the addition of a property > >> >> > > > > for clocks that is the equivalent of regulator-always-on. That's not > >> >> > > > > what this is about. If at all it'd be a property to mark a clock that > >> >> > > > > should not be disabled by default because it's essential. > >> >> > > > > >> >> > > > It's just semantic. How is "a clock that should not be disabled by > >> >> > > > default because it's essential" not a clock that stays always on? > >> >> > > > >> >> > > Because a clock that should not be disabled by default can be turned off > >> >> > > when appropriate. A clock that is always on can't be turned off. > >> >> > > >> >> > If a clock is essential, then it should never be disabled. Or we don't > >> >> > share the same meaning of essential. > >> >> > >> >> Essential for the particular use-case. > >> > > >> > So, how would the clock driver would know about which use case we're > >> > in? How would it know about which display engine is currently running? > >> > How would it know about which video output is being set? > >> > > >> > Currently, we have two separate display engines, which can each output > >> > either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every > >> > one of these combinations would require different clocks. What clocks > >> > will we put in the driver? All of them? > >> > > >> > >> since simplefb cannot be extended how about adding, say, dtfb which > >> claims the resources from dt and then instantiates a simplefb once the > >> resources are claimed? That is have a dtfb which has the clocks > >> assigned and has simplefb as child dt node. > > > > I don't see how that changes anything. All you do is add another layer > > of indirection. The fundamental problem remains the same and isn't > > solved. > > It keeps clock code out of simplefb and provides driver for the kind > of framebuffer set up by firmware that exists on sunxi, exynos, and > probably many other SoCs. That is a framebuffer that needs some clocks > and possibly regulators enabled to keep running because the reality of > the platform is that it has clocks and regulators unlike some other > platforms that do not. > > These clocks and regulators are used but not configured by the > framebuffer and should be reclaimed when firmware framebuffer is > disabled. This is the same as the chunk of memory used by simplefb > which is currently lost but should be reclaimed when simplefb is > disabled. > > This memory is not 'reserved for firmware' and unusable but reserved > for framebuffer and the regulators are not 'always on' or 'should > never be disabled' but should be enabled while framebuffer is used. > > As far as I can tell in DT this is expressed by creating a DT node > associated with the framebuffer driver that tells the kernel that this > memory, clocks and regulators are associated with the framebuffer > driver and can be reclaimed if this driver is stopped or not enabled > in the kernel at all. If you are going to be asinine about simplefb > not getting support for managing any resource other than the memory > chunk then another layer of indirection is required for platforms that > have more resources to manage. > > If there is another way to associate resources with a driver in DT > then please enlighten me. > > AFAICT simplefb is the framebuffer driver already in kernel closest to > the driver that is required for sunxi - simplefb also relies on > firmware to set up the framebuffer but unlike vesafb or efifb it > already has DT integration. So the most efficient way to implement > framebuffer for sunxi is to extend simplefb or if necessary add > another layer of indirection under simplefb. If there is a better > fitting driver in the kernel then please enlighten me and the > developer that wrote this patch what driver it would be. I think simplefb is exactly the right driver to use for this case. But I don't think making it manage all the resources is the right thing to do. While it's fairly easy to do it, it's also something that needs to be done for every other driver with similar requirements. Consider for example what happens if you want to play some welcome sound in the bootloader and keep it playing while the kernel is booting. You'd need to repeat exactly what this driver does to keep clocks for audio hardware running. And there are possibly other similar use-cases as well. One other issue with simplefb is that it isn't a real device to begin with. It's completely virtual, so in the same way that it doesn't program any device-specific registers I don't think it should claim any resources. Thierry
On Thu, Aug 28, 2014 at 10:46:03PM +0200, Maxime Ripard wrote: > On Thu, Aug 28, 2014 at 12:11:41PM +0200, Thierry Reding wrote: [...] > > Then since firmware already knows what it set up it can tell the > > kernel to not touch those. > > Somehow, I've been raised kernel-wise into thinking that you can never > fully trust your firmware. Or at least that you should have a way to > recover from any bug/bad behaviour from it. If you don't trust your firmware then you shouldn't be taking over a device that it's set up for you. Rather you should write a proper driver that sets things up from the start. This whole "we don't trust firmware" isn't going to work if we want to have hand-off from firmware to kernel. And it's already been decided in other threads that moving more code out of the kernel and into firmware is a requirement (c.f. ARM64). Also in this case you wrote the firmware, so why wouldn't you trust it? > Moreover, the way I see it, there's a major flaw in having an > attribute in the clock node: you don't even know if the clock is ever > going to be used. > > If simplefb is not compiled in, you won't claim the clocks, and they > will be disabled, which is imho a good thing. This case wouldn't be > covered with an attribute at the clock node, because you don't have a > link to what device/feature actually uses it in the system, and so you > have to make the assumption that it will be used. And you will end up > with clocks with a rather high rate running for nothing. That's completely hypothetical. If simplefb isn't enabled and the clock isn't claimed there's probably still going to be some other driver taking over eventually. If there isn't, what's the point of firmware setting up display in the first case? Thierry
On 29 August 2014 08:19, Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Aug 29, 2014 at 07:13:22AM +0200, Michal Suchanek wrote: >> On 28 August 2014 12:08, Thierry Reding <thierry.reding@gmail.com> wrote: >> > On Wed, Aug 27, 2014 at 10:57:29PM +0200, Michal Suchanek wrote: >> >> Hello, >> >> >> >> On 27 August 2014 17:42, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: >> >> > On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: >> >> >> On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: >> >> >> > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: >> >> >> > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: >> >> >> > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: >> >> >> [...] >> >> >> > > > > > Mike Turquette repeatedly said that he was against such a DT property: >> >> >> > > > > > https://lkml.org/lkml/2014/5/12/693 >> >> >> > > > > >> >> >> > > > > Mike says in that email that he's opposing the addition of a property >> >> >> > > > > for clocks that is the equivalent of regulator-always-on. That's not >> >> >> > > > > what this is about. If at all it'd be a property to mark a clock that >> >> >> > > > > should not be disabled by default because it's essential. >> >> >> > > > >> >> >> > > > It's just semantic. How is "a clock that should not be disabled by >> >> >> > > > default because it's essential" not a clock that stays always on? >> >> >> > > >> >> >> > > Because a clock that should not be disabled by default can be turned off >> >> >> > > when appropriate. A clock that is always on can't be turned off. >> >> >> > >> >> >> > If a clock is essential, then it should never be disabled. Or we don't >> >> >> > share the same meaning of essential. >> >> >> >> >> >> Essential for the particular use-case. >> >> > >> >> > So, how would the clock driver would know about which use case we're >> >> > in? How would it know about which display engine is currently running? >> >> > How would it know about which video output is being set? >> >> > >> >> > Currently, we have two separate display engines, which can each output >> >> > either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every >> >> > one of these combinations would require different clocks. What clocks >> >> > will we put in the driver? All of them? >> >> > >> >> >> >> since simplefb cannot be extended how about adding, say, dtfb which >> >> claims the resources from dt and then instantiates a simplefb once the >> >> resources are claimed? That is have a dtfb which has the clocks >> >> assigned and has simplefb as child dt node. >> > >> > I don't see how that changes anything. All you do is add another layer >> > of indirection. The fundamental problem remains the same and isn't >> > solved. >> >> It keeps clock code out of simplefb and provides driver for the kind >> of framebuffer set up by firmware that exists on sunxi, exynos, and >> probably many other SoCs. That is a framebuffer that needs some clocks >> and possibly regulators enabled to keep running because the reality of >> the platform is that it has clocks and regulators unlike some other >> platforms that do not. >> >> These clocks and regulators are used but not configured by the >> framebuffer and should be reclaimed when firmware framebuffer is >> disabled. This is the same as the chunk of memory used by simplefb >> which is currently lost but should be reclaimed when simplefb is >> disabled. >> >> This memory is not 'reserved for firmware' and unusable but reserved >> for framebuffer and the regulators are not 'always on' or 'should >> never be disabled' but should be enabled while framebuffer is used. >> >> As far as I can tell in DT this is expressed by creating a DT node >> associated with the framebuffer driver that tells the kernel that this >> memory, clocks and regulators are associated with the framebuffer >> driver and can be reclaimed if this driver is stopped or not enabled >> in the kernel at all. If you are going to be asinine about simplefb >> not getting support for managing any resource other than the memory >> chunk then another layer of indirection is required for platforms that >> have more resources to manage. >> >> If there is another way to associate resources with a driver in DT >> then please enlighten me. >> >> AFAICT simplefb is the framebuffer driver already in kernel closest to >> the driver that is required for sunxi - simplefb also relies on >> firmware to set up the framebuffer but unlike vesafb or efifb it >> already has DT integration. So the most efficient way to implement >> framebuffer for sunxi is to extend simplefb or if necessary add >> another layer of indirection under simplefb. If there is a better >> fitting driver in the kernel then please enlighten me and the >> developer that wrote this patch what driver it would be. > > I think simplefb is exactly the right driver to use for this case. But I > don't think making it manage all the resources is the right thing to do. And what is supposed to manage resources used by simplefb when not simplefb? > While it's fairly easy to do it, it's also something that needs to be > done for every other driver with similar requirements. Other drivers that require clocks do manage them, yes. And some drivers that did not do that in the past were extended for that. > Consider for > example what happens if you want to play some welcome sound in the > bootloader and keep it playing while the kernel is booting. You'd need > to repeat exactly what this driver does to keep clocks for audio > hardware running. And there are possibly other similar use-cases as > well. Why would you want to do that? If the kernel boots fast the welcome sound can be played from userspace when all drivers all in place. If it boots slow you can just play the sound and let the kernel boot. If you implement enough of audio driver that the kernel can actually change the startup sound then there is not much left out. You cannot just place random data in a buffer but have to synchronize with the hardware for audio playback to work. > > One other issue with simplefb is that it isn't a real device to begin > with. It's completely virtual, so in the same way that it doesn't > program any device-specific registers I don't think it should claim any > resources. OK, so the framebuffer memory that simplefb uses for scanout should not be claimed by it ever, either? So how is it supposed to be handled? Reserved as unusable memory forever as it is now? Thanks Michal
On Thu, Aug 28, 2014 at 02:15:40PM +0200, Hans de Goede wrote: > On 08/27/2014 04:16 PM, Thierry Reding wrote: [...] > >> Hmm I see, in my mind the problem is not that the clk framework disables > >> unused clocks, but that no one is marking the clocks in question as used. > >> Someone should mark these clocks as used so that they do not get disabled. > >> > >> We could add a clk_mark_in_use function and have the simplefb patch call > >> that instead of clk_prepare_and_enable, or maybe even better just only > >> claim the clks and teach the clk framework to not disable claimed clk > >> in its cleanup run. That would make it clear that simplefb is not enabling > >> anything, just claiming resource its need to avoid them getting removed > >> from underneath simplefb, would that work for you ? > > > > That would be more accurate, but it boils down to the same problem where > > we still need a driver to claim the resources in some way whereas the > > problem is fundamentally one where the bootloader should be telling the > > kernel not to disable it. It's in fact the bootloader that's claiming > > the resources. > > Yes, but those resources do not belong to the bootloader in a sense > that traditional bootloader / firmware claimed resources (e.g. acpi > reserved resources) do. These traditional resources are claimed for ever. I thought that at least on x86 there was a way for the kernel to reclaim memory set apart for an early framebuffer. > Where as these resources are claimed by the bootloader to keep the simplefb > it provides working, as soon as the simplefb is no longer used, they become > unused. Right. And when simplefb goes away it is because a real driver is taking over, in which case it will claim the resources explicitly. > > The copy and paste is for code that's platform specific. The clocks have > > different names, resets are different, supplies are different. The fact > > that many can currently use the same driver is likely just coincidence > > rather than design and it's entirely possible that at some point they'll > > add support for a more advanced feature that makes them incompatible > > with the rest of the generic drivers. And then you have a big mess > > because you need to add quirks all over the place. > > > > And this isn't all that far off-topic, since simplefb also needs to deal > > with this kind of situation. And what I've been arguing is that in order > > to really be generic it has to make assumptions, one of which is that it > > uses only resources that it doesn't need to explicitly handle. > > I can understand that you're worried about generic ?hci drivers dealing with > clocks / resets / etc. As there may be strict ordering requirements there, > but for simplefb that is not the case. > > All we're asking for is for a way to communicate 2 things to the kernel: > > 1) These resources are in use (we are not asking the kernel to do anything > with them, rather the opposite, we're asking to leave them alone so no > ordering issues) Right. That's the issue that needs to be solved. We still only disagree on how it should be solved. > 2) Tie these resources to simplefb so that the kernel can know when they > are no longer in use, and it may e.g. re-use the memory For the memory there shouldn't be a problem because it's already in the DT node anyway. It has to be there so that simplefb knows where to write to. For all the other resources, if 1) is solved properly then 2) becomes a non-issue. > To me the most logical and also most "correct" way of modelling this is to > put the resources inside the simplefb dt node. Except that simplefb isn't a real device, so there's a hard time justifying its existence in DT as it is. Claiming resources from a virtual device doesn't sound correct to me at all. > >> The key word here is "the used resources" to me this is not about simlefb > >> managing resources, but marking them as used as long as it needs them, like > >> it will need to do for the reserved mem chunk. > > > > The difference between the clocks and the memory resource is that the > > driver needs to directly access the memory (it needs to map it and > > provide a userspace mapping for it) whereas it doesn't need to touch the > > clocks (except to workaround a Linux-specific implementation detail). > > Erm, no, the need to map the memory and the memory being a resource > which may be released are an orthogonal problem. E.g. a system with > dedicated framebuffer memory won't need to use a reserved main-memory > chunk, nor need to worry about returning that mem when simplefb is no > longer in use. I would think the memory should still be reserved anyway to make sure nothing else is writing over it. And it's in the device tree anyway because the driver needs to know where to put framebuffer content. So the point I was trying to make is that we can't treat the memory in the same way as clocks because it needs to be explicitly managed. Whereas clocks don't. The driver is simply too generic to know what to do with the clocks. It doesn't know what frequency they should be running at or what they're used for, so by any definition of what DT should describe they're useless for this virtual device. Furthermore it's fairly likely that as your kernel support progresses you'll find that the driver all of a sudden needs to manage some other type of resource that you just haven't needed until now because it may default to being always on. Then you'll have a hard time keeping backwards-compatibility and will have to resort to the kinds of hacks that you don't want to see in the kernel. So it really boils down to the DT needing to describe a complete device with all the dependencies. And just clocks aren't the complete description. But if you want the complete description then you're going to need a complete driver as well and simplefb is out of the picture anyway. Once again, the current, basic form of the binding allows for a completely generic implementation of the driver because it makes assumptions about firmware setting things up the right way so that the driver doesn't have to. Thierry
On Fri, Aug 29, 2014 at 08:48:42AM +0200, Michal Suchanek wrote: > On 29 August 2014 08:19, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Fri, Aug 29, 2014 at 07:13:22AM +0200, Michal Suchanek wrote: > >> On 28 August 2014 12:08, Thierry Reding <thierry.reding@gmail.com> wrote: > >> > On Wed, Aug 27, 2014 at 10:57:29PM +0200, Michal Suchanek wrote: > >> >> Hello, > >> >> > >> >> On 27 August 2014 17:42, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > >> >> > On Wed, Aug 27, 2014 at 11:52:48AM +0200, Thierry Reding wrote: > >> >> >> On Wed, Aug 27, 2014 at 10:45:26AM +0200, Maxime Ripard wrote: > >> >> >> > On Wed, Aug 27, 2014 at 08:54:41AM +0200, Thierry Reding wrote: > >> >> >> > > On Tue, Aug 26, 2014 at 11:02:48PM +0200, Maxime Ripard wrote: > >> >> >> > > > On Tue, Aug 26, 2014 at 04:35:51PM +0200, Thierry Reding wrote: > >> >> >> [...] > >> >> >> > > > > > Mike Turquette repeatedly said that he was against such a DT property: > >> >> >> > > > > > https://lkml.org/lkml/2014/5/12/693 > >> >> >> > > > > > >> >> >> > > > > Mike says in that email that he's opposing the addition of a property > >> >> >> > > > > for clocks that is the equivalent of regulator-always-on. That's not > >> >> >> > > > > what this is about. If at all it'd be a property to mark a clock that > >> >> >> > > > > should not be disabled by default because it's essential. > >> >> >> > > > > >> >> >> > > > It's just semantic. How is "a clock that should not be disabled by > >> >> >> > > > default because it's essential" not a clock that stays always on? > >> >> >> > > > >> >> >> > > Because a clock that should not be disabled by default can be turned off > >> >> >> > > when appropriate. A clock that is always on can't be turned off. > >> >> >> > > >> >> >> > If a clock is essential, then it should never be disabled. Or we don't > >> >> >> > share the same meaning of essential. > >> >> >> > >> >> >> Essential for the particular use-case. > >> >> > > >> >> > So, how would the clock driver would know about which use case we're > >> >> > in? How would it know about which display engine is currently running? > >> >> > How would it know about which video output is being set? > >> >> > > >> >> > Currently, we have two separate display engines, which can each output > >> >> > either to 4 different outputs (HDMI, RGB/LVDS, 2 DSI). Each and every > >> >> > one of these combinations would require different clocks. What clocks > >> >> > will we put in the driver? All of them? > >> >> > > >> >> > >> >> since simplefb cannot be extended how about adding, say, dtfb which > >> >> claims the resources from dt and then instantiates a simplefb once the > >> >> resources are claimed? That is have a dtfb which has the clocks > >> >> assigned and has simplefb as child dt node. > >> > > >> > I don't see how that changes anything. All you do is add another layer > >> > of indirection. The fundamental problem remains the same and isn't > >> > solved. > >> > >> It keeps clock code out of simplefb and provides driver for the kind > >> of framebuffer set up by firmware that exists on sunxi, exynos, and > >> probably many other SoCs. That is a framebuffer that needs some clocks > >> and possibly regulators enabled to keep running because the reality of > >> the platform is that it has clocks and regulators unlike some other > >> platforms that do not. > >> > >> These clocks and regulators are used but not configured by the > >> framebuffer and should be reclaimed when firmware framebuffer is > >> disabled. This is the same as the chunk of memory used by simplefb > >> which is currently lost but should be reclaimed when simplefb is > >> disabled. > >> > >> This memory is not 'reserved for firmware' and unusable but reserved > >> for framebuffer and the regulators are not 'always on' or 'should > >> never be disabled' but should be enabled while framebuffer is used. > >> > >> As far as I can tell in DT this is expressed by creating a DT node > >> associated with the framebuffer driver that tells the kernel that this > >> memory, clocks and regulators are associated with the framebuffer > >> driver and can be reclaimed if this driver is stopped or not enabled > >> in the kernel at all. If you are going to be asinine about simplefb > >> not getting support for managing any resource other than the memory > >> chunk then another layer of indirection is required for platforms that > >> have more resources to manage. > >> > >> If there is another way to associate resources with a driver in DT > >> then please enlighten me. > >> > >> AFAICT simplefb is the framebuffer driver already in kernel closest to > >> the driver that is required for sunxi - simplefb also relies on > >> firmware to set up the framebuffer but unlike vesafb or efifb it > >> already has DT integration. So the most efficient way to implement > >> framebuffer for sunxi is to extend simplefb or if necessary add > >> another layer of indirection under simplefb. If there is a better > >> fitting driver in the kernel then please enlighten me and the > >> developer that wrote this patch what driver it would be. > > > > I think simplefb is exactly the right driver to use for this case. But I > > don't think making it manage all the resources is the right thing to do. > > And what is supposed to manage resources used by simplefb when not simplefb? Nothing should be managing them. There's no need to manage them at all. The issue that this patch works around is that the clock framework is trying to be smart and turning all unused clocks off and making incomplete assumptions about what "all unused clocks" means. > > While it's fairly easy to do it, it's also something that needs to be > > done for every other driver with similar requirements. > > Other drivers that require clocks do manage them, yes. And some > drivers that did not do that in the past were extended for that. Not generic drivers. Not drivers that take over something that firmware has set up. > > Consider for > > example what happens if you want to play some welcome sound in the > > bootloader and keep it playing while the kernel is booting. You'd need > > to repeat exactly what this driver does to keep clocks for audio > > hardware running. And there are possibly other similar use-cases as > > well. > > Why would you want to do that? If the kernel boots fast the welcome > sound can be played from userspace when all drivers all in place. In that case you don't need simplefb either. Just use a real driver. > If it boots slow you can just play the sound and let the kernel boot. And have audio interrupted by the kernel booting and disabling the clocks? > > One other issue with simplefb is that it isn't a real device to begin > > with. It's completely virtual, so in the same way that it doesn't > > program any device-specific registers I don't think it should claim any > > resources. > > OK, so the framebuffer memory that simplefb uses for scanout should > not be claimed by it ever, either? No. simplefb needs to manage the framebuffer memory because it needs explicit access to it. The contents of that memory need to be modified. The frequency of the clocks don't need to be modified. They don't need to be enabled or disabled either. Thierry
On Thu, Aug 28, 2014 at 12:34:58PM -0400, jonsmirl@gmail.com wrote: > On Thu, Aug 28, 2014 at 12:25 PM, Michal Suchanek <hramrach@gmail.com> wrote: > > On 28 August 2014 16:33, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > >> On Thu, Aug 28, 2014 at 10:20 AM, Geert Uytterhoeven > >> <geert@linux-m68k.org> wrote: > >>> On Thu, Aug 28, 2014 at 3:22 PM, jonsmirl@gmail.com <jonsmirl@gmail.com> wrote: > >>>>> 2) We don't want to hardcode these clocks into the kernel (sunxi) clk > >>>>> driver, instead the bootloader should tell the kernel about these clocks. > >>>>> > >>>>> So the only point of discussion left seems to be how to do 2... > >>>> > >>>> Wouldn't it be a lot simpler just to use existing fbdev (not KMS) and > >>>> whip together a device specific driver that claims the proper > >>>> resources? And just implement the minimal about of fbdev possible? > >>>> fbdev already is a driver library. > >>> > >>> Like... drivers/video/fbdev/offb.c? > >> > >> I'd probably reclassify drivers/video/fbdev/simplefb.c as a skeleton > >> and use it as a template for making device specific versions of it. > >> > >> I don't see why there is so much resistance to just making device > >> specific fb drivers. Whenever the KMS driver gets written just > >> disable the device specific fb driver in the build. > > > > Except that is not the goal here. The simplefb or whatever replacement > > is supposed to stay as a generic driver compiled into kernel whereas > > There is no generic solution to this problem as this entire thread has > illustrated. The clocks/regulators needed by each SOC vary. There is a generic solution. Genericity only works if you define a well defined set of assumptions. In this case the assumptions are that some firmware sets up display hardware to scan out from a memory region and communicates the location, layout and format of that memory region so that a generic driver can write into that framebuffer. The generic solution here works because we've eliminated the platform specificities and let firmware take care of it. > So there are two solutions.. > 1) modify simplefb to have some kind of heuristic that tries to guess > what needs to be protected. A heuristic that is probably going to fail > on every new SOC. > > 2) Spend a day implementing a device specific fbdev driver that does > the correct thing all of the time. These drivers would sit in initrd > and load before the clock/regulator clean up shuts everything off. Use > the existing simplefb code as a template for doing this. There's a third possibility: find a way to prevent the clock framework (and any other resource framework for that matter) from forcefully disabling things that they shouldn't. Thierry
Hi, On 08/29/2014 09:01 AM, Thierry Reding wrote: > On Thu, Aug 28, 2014 at 02:15:40PM +0200, Hans de Goede wrote: >> On 08/27/2014 04:16 PM, Thierry Reding wrote: > [...] >>>> Hmm I see, in my mind the problem is not that the clk framework disables >>>> unused clocks, but that no one is marking the clocks in question as used. >>>> Someone should mark these clocks as used so that they do not get disabled. >>>> >>>> We could add a clk_mark_in_use function and have the simplefb patch call >>>> that instead of clk_prepare_and_enable, or maybe even better just only >>>> claim the clks and teach the clk framework to not disable claimed clk >>>> in its cleanup run. That would make it clear that simplefb is not enabling >>>> anything, just claiming resource its need to avoid them getting removed >>>> from underneath simplefb, would that work for you ? >>> >>> That would be more accurate, but it boils down to the same problem where >>> we still need a driver to claim the resources in some way whereas the >>> problem is fundamentally one where the bootloader should be telling the >>> kernel not to disable it. It's in fact the bootloader that's claiming >>> the resources. >> >> Yes, but those resources do not belong to the bootloader in a sense >> that traditional bootloader / firmware claimed resources (e.g. acpi >> reserved resources) do. These traditional resources are claimed for ever. > > I thought that at least on x86 there was a way for the kernel to reclaim > memory set apart for an early framebuffer. > >> Where as these resources are claimed by the bootloader to keep the simplefb >> it provides working, as soon as the simplefb is no longer used, they become >> unused. > > Right. And when simplefb goes away it is because a real driver is taking > over, in which case it will claim the resources explicitly. > >>> The copy and paste is for code that's platform specific. The clocks have >>> different names, resets are different, supplies are different. The fact >>> that many can currently use the same driver is likely just coincidence >>> rather than design and it's entirely possible that at some point they'll >>> add support for a more advanced feature that makes them incompatible >>> with the rest of the generic drivers. And then you have a big mess >>> because you need to add quirks all over the place. >>> >>> And this isn't all that far off-topic, since simplefb also needs to deal >>> with this kind of situation. And what I've been arguing is that in order >>> to really be generic it has to make assumptions, one of which is that it >>> uses only resources that it doesn't need to explicitly handle. >> >> I can understand that you're worried about generic ?hci drivers dealing with >> clocks / resets / etc. As there may be strict ordering requirements there, >> but for simplefb that is not the case. >> >> All we're asking for is for a way to communicate 2 things to the kernel: >> >> 1) These resources are in use (we are not asking the kernel to do anything >> with them, rather the opposite, we're asking to leave them alone so no >> ordering issues) > > Right. That's the issue that needs to be solved. We still only disagree > on how it should be solved. > >> 2) Tie these resources to simplefb so that the kernel can know when they >> are no longer in use, and it may e.g. re-use the memory > > For the memory there shouldn't be a problem because it's already in the > DT node anyway. It has to be there so that simplefb knows where to write > to. The memory information currently in the dt node is not complete enough to reclaim memory, e.g. the sunxi driver always reserves 8M, but the simplefb reg entry only describes the part actually used for the framebuffer. > For all the other resources, if 1) is solved properly then 2) becomes a > non-issue. > >> To me the most logical and also most "correct" way of modelling this is to >> put the resources inside the simplefb dt node. > > Except that simplefb isn't a real device, so there's a hard time > justifying its existence in DT as it is. Claiming resources from a > virtual device doesn't sound correct to me at all. Yet you want it to claim memory that does not seem consistent to me. > >>>> The key word here is "the used resources" to me this is not about simlefb >>>> managing resources, but marking them as used as long as it needs them, like >>>> it will need to do for the reserved mem chunk. >>> >>> The difference between the clocks and the memory resource is that the >>> driver needs to directly access the memory (it needs to map it and >>> provide a userspace mapping for it) whereas it doesn't need to touch the >>> clocks (except to workaround a Linux-specific implementation detail). >> >> Erm, no, the need to map the memory and the memory being a resource >> which may be released are an orthogonal problem. E.g. a system with >> dedicated framebuffer memory won't need to use a reserved main-memory >> chunk, nor need to worry about returning that mem when simplefb is no >> longer in use. > > I would think the memory should still be reserved anyway to make sure > nothing else is writing over it. And it's in the device tree anyway > because the driver needs to know where to put framebuffer content. No, the address were to write framebuffer contents currently is in the simplefb node, not the actually backing memory info. I can fully see some crazy hardware where a chunk of main memory is reserved for some funky video engine, and then that chunk of memory gets accessed through io-mem addresses for framebuffer access (e.g. the hardware may need this to know when the fb is dirtied). > So > the point I was trying to make is that we can't treat the memory in the > same way as clocks because it needs to be explicitly managed. Whereas > clocks don't. The driver is simply too generic to know what to do with > the clocks. It doesn't know what frequency they should be running at or > what they're used for, so by any definition of what DT should describe > they're useless for this virtual device. I don't see why you keep insisting that the memory is so special, it is just another resource which we need to claim, and tie to the simplefb node so that it can be re-used (or disabled) when simplefb is no longer used (it could e.g. be unbound explicitly on headless systems). > Furthermore it's fairly likely that as your kernel support progresses > you'll find that the driver all of a sudden needs to manage some other > type of resource that you just haven't needed until now because it may > default to being always on. Then you'll have a hard time keeping > backwards-compatibility and will have to resort to the kinds of hacks > that you don't want to see in the kernel. kernel support? This is about u-boot's video code and the simplefb bindings. > So it really boils down to the DT needing to describe a complete device > with all the dependencies. And just clocks aren't the complete > description. But if you want the complete description then you're going > to need a complete driver as well and simplefb is out of the picture > anyway. Yes we eventually need a full kernel driver, with its own bindings, that is not what this is about. > Once again, the current, basic form of the binding allows for a > completely generic implementation of the driver because it makes > assumptions about firmware setting things up the right way so that the > driver doesn't have to. Except that it does not work because it does not claim the necessary resources. We seem to be at a point where this discussion is just going in circles, and since there seems to be no reasonable alternative to the proposed solution of adding the clocks to the simplefb node, can you please just stop blocking progress on this? Your objection to this has been duly noted, but no one is forcing you or other people to actually use the clocks property in your implementation of simplefb. So if your 101% certain that this is a bad idea, can you please just let us (the sunxi people) make our own mistakes and learn from those ? As we actually plan to use the clocks property, we will be the ones which will have to deal with any issues. For the record, I don't think this is a bad idea myself, but that just brings us full circle again. I'm willing to look into just marking the clocks as used, instead of actually enabling them. Regards, Hans
On Fri, Aug 29, 2014 at 09:23:41AM +0200, Hans de Goede wrote: > On 08/29/2014 09:01 AM, Thierry Reding wrote: > > On Thu, Aug 28, 2014 at 02:15:40PM +0200, Hans de Goede wrote: [...] > >> 2) Tie these resources to simplefb so that the kernel can know when they > >> are no longer in use, and it may e.g. re-use the memory > > > > For the memory there shouldn't be a problem because it's already in the > > DT node anyway. It has to be there so that simplefb knows where to write > > to. > > The memory information currently in the dt node is not complete enough to > reclaim memory, e.g. the sunxi driver always reserves 8M, but the simplefb > reg entry only describes the part actually used for the framebuffer. You can easily fix that. > >> To me the most logical and also most "correct" way of modelling this is to > >> put the resources inside the simplefb dt node. > > > > Except that simplefb isn't a real device, so there's a hard time > > justifying its existence in DT as it is. Claiming resources from a > > virtual device doesn't sound correct to me at all. > > Yet you want it to claim memory that does not seem consistent to me. Because it needs to explicitly access that memory. It does not need to explicitly access the clocks. > >>>> The key word here is "the used resources" to me this is not about simlefb > >>>> managing resources, but marking them as used as long as it needs them, like > >>>> it will need to do for the reserved mem chunk. > >>> > >>> The difference between the clocks and the memory resource is that the > >>> driver needs to directly access the memory (it needs to map it and > >>> provide a userspace mapping for it) whereas it doesn't need to touch the > >>> clocks (except to workaround a Linux-specific implementation detail). > >> > >> Erm, no, the need to map the memory and the memory being a resource > >> which may be released are an orthogonal problem. E.g. a system with > >> dedicated framebuffer memory won't need to use a reserved main-memory > >> chunk, nor need to worry about returning that mem when simplefb is no > >> longer in use. > > > > I would think the memory should still be reserved anyway to make sure > > nothing else is writing over it. And it's in the device tree anyway > > because the driver needs to know where to put framebuffer content. > > No, the address were to write framebuffer contents currently is in the > simplefb node, not the actually backing memory info. I can fully see > some crazy hardware where a chunk of main memory is reserved for some > funky video engine, and then that chunk of memory gets accessed through > io-mem addresses for framebuffer access (e.g. the hardware may need this > to know when the fb is dirtied). We're drifting off into the realm of hypotheses here. Of course if you can't make the hardware work with these simple assumptions because it's too whacky then that's just tough luck. > > So > > the point I was trying to make is that we can't treat the memory in the > > same way as clocks because it needs to be explicitly managed. Whereas > > clocks don't. The driver is simply too generic to know what to do with > > the clocks. It doesn't know what frequency they should be running at or > > what they're used for, so by any definition of what DT should describe > > they're useless for this virtual device. > > I don't see why you keep insisting that the memory is so special, it is > just another resource which we need to claim, It's the only resource which we need to claim. Because it is the only resource that needs to be explicitly accessed. simplefb is useless if you don't write to that memory. > and tie to the simplefb node > so that it can be re-used (or disabled) when simplefb is no longer used > (it could e.g. be unbound explicitly on headless systems). On headless systems the firmware shouldn't be setting up a framebuffer in the first place. > > Furthermore it's fairly likely that as your kernel support progresses > > you'll find that the driver all of a sudden needs to manage some other > > type of resource that you just haven't needed until now because it may > > default to being always on. Then you'll have a hard time keeping > > backwards-compatibility and will have to resort to the kinds of hacks > > that you don't want to see in the kernel. > > kernel support? This is about u-boot's video code and the simplefb bindings. And the kernel doesn't need to use the simplefb bindings? Why are we even having this conversation, then? > > So it really boils down to the DT needing to describe a complete device > > with all the dependencies. And just clocks aren't the complete > > description. But if you want the complete description then you're going > > to need a complete driver as well and simplefb is out of the picture > > anyway. > > Yes we eventually need a full kernel driver, with its own bindings, that > is not what this is about. It is to some degree. The whole point of simplefb is to provide a useful graphical output until a full driver is in place. The way to achieve that is by taking advantage of the firmware setting things up for you. Which is what I've been saying all along. If the firmware has already set things up for you, you shouldn't need to do anything special like enabling clocks because they are already on. Oh, and let me restate that that's not actually what the patch is doing. It isn't enabling clocks at all. It's merely a workaround to keep them running. > > Once again, the current, basic form of the binding allows for a > > completely generic implementation of the driver because it makes > > assumptions about firmware setting things up the right way so that the > > driver doesn't have to. > > Except that it does not work because it does not claim the necessary > resources. Wrong. The reason it doesn't work is because the clock framework disables the clocks. That's a Linux-specific implementation detail and therefore shouldn't influence the binding in any way. > We seem to be at a point where this discussion is just going in circles, > and since there seems to be no reasonable alternative to the proposed > solution of adding the clocks to the simplefb node, can you please just > stop blocking progress on this? A couple of different solutions to the problem were proposed during the discussion. It's just that nobody's been willing to take a step back and look at this from a different perspective. > Your objection to this has been duly noted, Right. Ignored is how I would put it. > but no one is forcing you or > other people to actually use the clocks property in your implementation > of simplefb. So if your 101% certain that this is a bad idea, can you > please just let us (the sunxi people) make our own mistakes and learn > from those ? Ugh... This isn't just about letting you make your own mistakes. This is about solving things in a wrong way (and device specific way) and baking something into an ABI that really doesn't need to be there. And more specifically if we let clocks be "managed" by simplefb we can no longer claim that it is simple and therefore can't block adding all other sorts of device-specific hacks to it. > As we actually plan to use the clocks property, we will be > the ones which will have to deal with any issues. Wrong. Everyone that potentially wants to use the driver will have to deal with this. Anyway, I am getting tired of this whole discussion and as you already said we're not making any progress. You really should repost this series to the devicetree mailing list and give people there a chance to look at the proposed changes. If everybody else is fine with it my objections will be overruled anyway. I'll just have to sort out/live with the mess when I want to use simplefb for hand-off. Thierry
On Fri, Aug 29, 2014 at 08:29:33AM +0200, Thierry Reding wrote: > On Thu, Aug 28, 2014 at 10:46:03PM +0200, Maxime Ripard wrote: > > On Thu, Aug 28, 2014 at 12:11:41PM +0200, Thierry Reding wrote: > [...] > > > Then since firmware already knows what it set up it can tell the > > > kernel to not touch those. > > > > Somehow, I've been raised kernel-wise into thinking that you can never > > fully trust your firmware. Or at least that you should have a way to > > recover from any bug/bad behaviour from it. > > If you don't trust your firmware then you shouldn't be taking over a > device that it's set up for you. Rather you should write a proper driver > that sets things up from the start. > > This whole "we don't trust firmware" isn't going to work if we want to > have hand-off from firmware to kernel. And it's already been decided in > other threads that moving more code out of the kernel and into firmware > is a requirement (c.f. ARM64). Except that in ARM64 case, it has been asked before having any SoC/boards out. We're definitely not in this situation there. > Also in this case you wrote the firmware, so why wouldn't you trust it? We partly wrote the firmware, on some of the available SoCs. Some other just have allwinner's own. But yeah, in this case they wouldn't even set up the framebuffer in the first place. > > Moreover, the way I see it, there's a major flaw in having an > > attribute in the clock node: you don't even know if the clock is ever > > going to be used. > > > > If simplefb is not compiled in, you won't claim the clocks, and they > > will be disabled, which is imho a good thing. This case wouldn't be > > covered with an attribute at the clock node, because you don't have a > > link to what device/feature actually uses it in the system, and so you > > have to make the assumption that it will be used. And you will end up > > with clocks with a rather high rate running for nothing. > > That's completely hypothetical. If simplefb isn't enabled and the clock > isn't claimed there's probably still going to be some other driver > taking over eventually. If there isn't, what's the point of firmware > setting up display in the first case? And now, here is a new requirement for the firmware: that in addition to being reliable, that it must not be dumb, or take shortcuts, like setting up the framebuffer in every case, no matter wether there's a screen or not. Maxime
On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote: > I would think the memory should still be reserved anyway to make sure > nothing else is writing over it. And it's in the device tree anyway > because the driver needs to know where to put framebuffer content. So > the point I was trying to make is that we can't treat the memory in the > same way as clocks because it needs to be explicitly managed. Whereas > clocks don't. The driver is simply too generic to know what to do with > the clocks. You agreed on the fact that the only thing we need to do with the clocks is claim them. Really, I don't find what's complicated there (or not generic). > It doesn't know what frequency they should be running at We don't care about that. Just like we don't care about which frequency is the memory bus running at. It will just run at whatever frequency is appropriate. > or what they're used for And we don't care about that either. You're not interested in what output the framebuffer is setup to use, which is pretty much the same here, this is the same thing here. > so by any definition of what DT should describe they're useless for > this virtual device. > > Furthermore it's fairly likely that as your kernel support progresses > you'll find that the driver all of a sudden needs to manage some other > type of resource that you just haven't needed until now because it may > default to being always on. Then you'll have a hard time keeping > backwards-compatibility and will have to resort to the kinds of hacks > that you don't want to see in the kernel. Not such a hard time. An older DT wouldn't define the new requirements anyway, so they wouldn't be used, and we would end up in pretty much the current case. Maxime
On Fri, Aug 29, 2014 at 03:57:08PM +0200, Maxime Ripard wrote: > On Fri, Aug 29, 2014 at 08:29:33AM +0200, Thierry Reding wrote: > > On Thu, Aug 28, 2014 at 10:46:03PM +0200, Maxime Ripard wrote: > > > On Thu, Aug 28, 2014 at 12:11:41PM +0200, Thierry Reding wrote: > > [...] > > > > Then since firmware already knows what it set up it can tell the > > > > kernel to not touch those. > > > > > > Somehow, I've been raised kernel-wise into thinking that you can never > > > fully trust your firmware. Or at least that you should have a way to > > > recover from any bug/bad behaviour from it. > > > > If you don't trust your firmware then you shouldn't be taking over a > > device that it's set up for you. Rather you should write a proper driver > > that sets things up from the start. > > > > This whole "we don't trust firmware" isn't going to work if we want to > > have hand-off from firmware to kernel. And it's already been decided in > > other threads that moving more code out of the kernel and into firmware > > is a requirement (c.f. ARM64). > > Except that in ARM64 case, it has been asked before having any > SoC/boards out. We're definitely not in this situation there. You're still in the situation where you need to trust your firmware, so my point remains valid. > > Also in this case you wrote the firmware, so why wouldn't you trust it? > > We partly wrote the firmware, on some of the available SoCs. Some > other just have allwinner's own. But yeah, in this case they wouldn't > even set up the framebuffer in the first place. Even if you didn't write it, you could still trust it provided that you had the source code for it. The whole argument of "never trust firmware" is based on the assumption that you can't fix it. And even if that's the case, it's still perfectly acceptable to not trust firmware. But if you don't trust firmware then you really shouldn't be using simplefb in the first place and do everything in the kernel. > > > Moreover, the way I see it, there's a major flaw in having an > > > attribute in the clock node: you don't even know if the clock is ever > > > going to be used. > > > > > > If simplefb is not compiled in, you won't claim the clocks, and they > > > will be disabled, which is imho a good thing. This case wouldn't be > > > covered with an attribute at the clock node, because you don't have a > > > link to what device/feature actually uses it in the system, and so you > > > have to make the assumption that it will be used. And you will end up > > > with clocks with a rather high rate running for nothing. > > > > That's completely hypothetical. If simplefb isn't enabled and the clock > > isn't claimed there's probably still going to be some other driver > > taking over eventually. If there isn't, what's the point of firmware > > setting up display in the first case? > > And now, here is a new requirement for the firmware: that in addition > to being reliable, that it must not be dumb, or take shortcuts, like > setting up the framebuffer in every case, no matter wether there's a > screen or not. Firmware should never do that anyway. The whole point of firmware is to be device-specific and therefore it already knows when there's a need to activate a framebuffer or not. Thierry
On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote: > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote: > > I would think the memory should still be reserved anyway to make sure > > nothing else is writing over it. And it's in the device tree anyway > > because the driver needs to know where to put framebuffer content. So > > the point I was trying to make is that we can't treat the memory in the > > same way as clocks because it needs to be explicitly managed. Whereas > > clocks don't. The driver is simply too generic to know what to do with > > the clocks. > > You agreed on the fact that the only thing we need to do with the > clocks is claim them. Really, I don't find what's complicated there > (or not generic). That's not what I agreed on. What I said is that the only thing we need to do with the clocks is nothing. They are already in the state that they need to be. > > It doesn't know what frequency they should be running at > > We don't care about that. Just like we don't care about which > frequency is the memory bus running at. It will just run at whatever > frequency is appropriate. Exactly. And you shouldn't have to care about them at all. Firmware has already configured the clocks to run at the correct frequencies, and it has made sure that they are enabled. > > or what they're used for > > And we don't care about that either. You're not interested in what > output the framebuffer is setup to use, which is pretty much the same > here, this is the same thing here. That's precisely what I've been saying. The only thing that simplefb cares about is the memory it should be using and the format of the pixels (and how many of them) it writes into that memory. Everything else is assumed to have been set up. Including clocks. > > so by any definition of what DT should describe they're useless for > > this virtual device. > > > > Furthermore it's fairly likely that as your kernel support progresses > > you'll find that the driver all of a sudden needs to manage some other > > type of resource that you just haven't needed until now because it may > > default to being always on. Then you'll have a hard time keeping > > backwards-compatibility and will have to resort to the kinds of hacks > > that you don't want to see in the kernel. > > Not such a hard time. An older DT wouldn't define the new requirements > anyway, so they wouldn't be used, and we would end up in pretty much > the current case. Except that you have firmware in the wild that sets up an incomplete simplefb node and if you don't want to break compatibility you need to provide fallbacks for the resources that aren't listed in the DT node. And given that those fallbacks are all very board specific you'll need to find ways to keep them enabled if you want to keep existing setups working. Thierry
On 29 August 2014 16:38, Thierry Reding <thierry.reding@gmail.com> wrote: > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote: >> On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote: >> > I would think the memory should still be reserved anyway to make sure >> > nothing else is writing over it. And it's in the device tree anyway >> > because the driver needs to know where to put framebuffer content. So >> > the point I was trying to make is that we can't treat the memory in the >> > same way as clocks because it needs to be explicitly managed. Whereas >> > clocks don't. The driver is simply too generic to know what to do with >> > the clocks. >> >> You agreed on the fact that the only thing we need to do with the >> clocks is claim them. Really, I don't find what's complicated there >> (or not generic). > > That's not what I agreed on. What I said is that the only thing we need > to do with the clocks is nothing. They are already in the state that > they need to be. > >> > It doesn't know what frequency they should be running at >> >> We don't care about that. Just like we don't care about which >> frequency is the memory bus running at. It will just run at whatever >> frequency is appropriate. > > Exactly. And you shouldn't have to care about them at all. Firmware has > already configured the clocks to run at the correct frequencies, and it > has made sure that they are enabled. > >> > or what they're used for >> >> And we don't care about that either. You're not interested in what >> output the framebuffer is setup to use, which is pretty much the same >> here, this is the same thing here. > > That's precisely what I've been saying. The only thing that simplefb > cares about is the memory it should be using and the format of the > pixels (and how many of them) it writes into that memory. Everything > else is assumed to have been set up. > > Including clocks. And for simplefb to work it should claim the memory and clock so that nothing else in the kernel uses them or reclaims them as unused. That currently the memory is reserved by excluding it from memory usable by the kernel is a hack, not proper resource management. And so is keeping all clocks enabled in case something used them. > >> > so by any definition of what DT should describe they're useless for >> > this virtual device. >> > >> > Furthermore it's fairly likely that as your kernel support progresses >> > you'll find that the driver all of a sudden needs to manage some other >> > type of resource that you just haven't needed until now because it may >> > default to being always on. Then you'll have a hard time keeping >> > backwards-compatibility and will have to resort to the kinds of hacks >> > that you don't want to see in the kernel. >> >> Not such a hard time. An older DT wouldn't define the new requirements >> anyway, so they wouldn't be used, and we would end up in pretty much >> the current case. > > Except that you have firmware in the wild that sets up an incomplete > simplefb node and if you don't want to break compatibility you need to > provide fallbacks for the resources that aren't listed in the DT node. > And given that those fallbacks are all very board specific you'll need > to find ways to keep them enabled if you want to keep existing setups > working. > Except in this case either the simplefb does not need clocks the user had to set up the kernel argument to not disable unused clocks so the user of such firmware is well aware of that limitation. Thanks Michal
On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote: > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote: > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote: > > > I would think the memory should still be reserved anyway to make sure > > > nothing else is writing over it. And it's in the device tree anyway > > > because the driver needs to know where to put framebuffer content. So > > > the point I was trying to make is that we can't treat the memory in the > > > same way as clocks because it needs to be explicitly managed. Whereas > > > clocks don't. The driver is simply too generic to know what to do with > > > the clocks. > > > > You agreed on the fact that the only thing we need to do with the > > clocks is claim them. Really, I don't find what's complicated there > > (or not generic). > > That's not what I agreed on. What I said is that the only thing we need > to do with the clocks is nothing. They are already in the state that > they need to be. Claim was probably a poor choice of words, but still. We have to keep the clock running, and both the solution you've been giving and this patch do so in a generic way. > > > It doesn't know what frequency they should be running at > > > > We don't care about that. Just like we don't care about which > > frequency is the memory bus running at. It will just run at whatever > > frequency is appropriate. > > Exactly. And you shouldn't have to care about them at all. Firmware has > already configured the clocks to run at the correct frequencies, and it > has made sure that they are enabled. > > > > or what they're used for > > > > And we don't care about that either. You're not interested in what > > output the framebuffer is setup to use, which is pretty much the same > > here, this is the same thing here. > > That's precisely what I've been saying. The only thing that simplefb > cares about is the memory it should be using and the format of the > pixels (and how many of them) it writes into that memory. Everything > else is assumed to have been set up. > > Including clocks. We're really discussing in circles here. Mike? Your opinion would be very valuable. > > > so by any definition of what DT should describe they're useless for > > > this virtual device. > > > > > > Furthermore it's fairly likely that as your kernel support progresses > > > you'll find that the driver all of a sudden needs to manage some other > > > type of resource that you just haven't needed until now because it may > > > default to being always on. Then you'll have a hard time keeping > > > backwards-compatibility and will have to resort to the kinds of hacks > > > that you don't want to see in the kernel. > > > > Not such a hard time. An older DT wouldn't define the new requirements > > anyway, so they wouldn't be used, and we would end up in pretty much > > the current case. > > Except that you have firmware in the wild that sets up an incomplete > simplefb node and if you don't want to break compatibility you need to > provide fallbacks for the resources that aren't listed in the DT node. > And given that those fallbacks are all very board specific you'll need > to find ways to keep them enabled if you want to keep existing setups > working. How would an *optional* property break those users? If you don't need any clock to be kept running (or are hiding them under the carpet), of course you don't need such a property. Maxime
Quoting Maxime Ripard (2014-09-02 02:25:08) > On Fri, Aug 29, 2014 at 04:38:14PM +0200, Thierry Reding wrote: > > On Fri, Aug 29, 2014 at 04:12:44PM +0200, Maxime Ripard wrote: > > > On Fri, Aug 29, 2014 at 09:01:17AM +0200, Thierry Reding wrote: > > > > I would think the memory should still be reserved anyway to make sure > > > > nothing else is writing over it. And it's in the device tree anyway > > > > because the driver needs to know where to put framebuffer content. So > > > > the point I was trying to make is that we can't treat the memory in the > > > > same way as clocks because it needs to be explicitly managed. Whereas > > > > clocks don't. The driver is simply too generic to know what to do with > > > > the clocks. > > > > > > You agreed on the fact that the only thing we need to do with the > > > clocks is claim them. Really, I don't find what's complicated there > > > (or not generic). > > > > That's not what I agreed on. What I said is that the only thing we need > > to do with the clocks is nothing. They are already in the state that > > they need to be. > > Claim was probably a poor choice of words, but still. We have to keep > the clock running, and both the solution you've been giving and this > patch do so in a generic way. > > > > > It doesn't know what frequency they should be running at > > > > > > We don't care about that. Just like we don't care about which > > > frequency is the memory bus running at. It will just run at whatever > > > frequency is appropriate. > > > > Exactly. And you shouldn't have to care about them at all. Firmware has > > already configured the clocks to run at the correct frequencies, and it > > has made sure that they are enabled. > > > > > > or what they're used for > > > > > > And we don't care about that either. You're not interested in what > > > output the framebuffer is setup to use, which is pretty much the same > > > here, this is the same thing here. > > > > That's precisely what I've been saying. The only thing that simplefb > > cares about is the memory it should be using and the format of the > > pixels (and how many of them) it writes into that memory. Everything > > else is assumed to have been set up. > > > > Including clocks. > > We're really discussing in circles here. > > Mike? > -EHIGHLATENCYRESPONSE I forgot about this thread. Sorry. In an attempt to provide the least helpful answer possible, I will stay clear of all of the stuff relating to "how simple should simplefb be" and the related reserved memory discussion. A few times in this thread it is stated that we can't prevent unused clocks from being disabled. That is only partially true. The clock framework DOES provide a flag to prevent UNUSED clocks from being disabled at late_initcall-time by a clock "garbage collector" mechanism. Maxime and others familiar with the clock framework are aware of this. What the framework doesn't do is to allow for a magic flag in DT, in platform_data or elsewhere that says, "don't let me get turned off until the right driver claims me". That would be an external or alternative method for preventing a clock from being disabled. We have a method for preventing clocks from being disabled. It is as follows: struct clk *my_clk = clk_get(...); clk_prepare_enable(my_clk); That is how it should be done. Period. With that said I think that Luc's approach is very sensible. I'm not sure what purpose in the universe DT is supposed to serve if not for _this_exact_case_. We have a nice abstracted driver, usable by many people. The hardware details of how it is hooked up at the board level can all be hidden behind stable DT bindings that everyone already uses. What more could you want? Regards, Mike > Your opinion would be very valuable. > > > > > so by any definition of what DT should describe they're useless for > > > > this virtual device. > > > > > > > > Furthermore it's fairly likely that as your kernel support progresses > > > > you'll find that the driver all of a sudden needs to manage some other > > > > type of resource that you just haven't needed until now because it may > > > > default to being always on. Then you'll have a hard time keeping > > > > backwards-compatibility and will have to resort to the kinds of hacks > > > > that you don't want to see in the kernel. > > > > > > Not such a hard time. An older DT wouldn't define the new requirements > > > anyway, so they wouldn't be used, and we would end up in pretty much > > > the current case. > > > > Except that you have firmware in the wild that sets up an incomplete > > simplefb node and if you don't want to break compatibility you need to > > provide fallbacks for the resources that aren't listed in the DT node. > > And given that those fallbacks are all very board specific you'll need > > to find ways to keep them enabled if you want to keep existing setups > > working. > > How would an *optional* property break those users? > > If you don't need any clock to be kept running (or are hiding them > under the carpet), of course you don't need such a property. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com
diff --git a/drivers/video/fbdev/simplefb.c b/drivers/video/fbdev/simplefb.c index 74c4b2a..481dbfd 100644 --- a/drivers/video/fbdev/simplefb.c +++ b/drivers/video/fbdev/simplefb.c @@ -26,6 +26,7 @@ #include <linux/module.h> #include <linux/platform_data/simplefb.h> #include <linux/platform_device.h> +#include <linux/clk-provider.h> static struct fb_fix_screeninfo simplefb_fix = { .id = "simple", @@ -191,8 +192,101 @@ static int simplefb_parse_pd(struct platform_device *pdev, return 0; } +/* + * Clock handling code. + * + * Here we handle the clocks property of our "simple-framebuffer" dt node. + * This is necessary so that we can make sure that any clocks needed by + * the display engine that the bootloader set up for us (and for which it + * provided a simplefb dt node), stay up, for the life of the simplefb + * driver. + * + * When the driver unloads, we cleanly disable, and then release the clocks. + */ +struct simplefb_clock { + struct list_head list; + struct clk *clock; +}; + +/* + * We only complain about errors here, no action is taken as the most likely + * error can only happen due to a mismatch between the bootloader which set + * up simplefb, and the clock definitions in the device tree. Chances are + * that there are no adverse effects, and if there are, a clean teardown of + * the fb probe will not help us much either. So just complain and carry on, + * and hope that the user actually gets a working fb at the end of things. + */ +static void +simplefb_clocks_init(struct platform_device *pdev, struct list_head *list) +{ + struct device_node *np = pdev->dev.of_node; + int clock_count, i; + + INIT_LIST_HEAD(list); + + if (dev_get_platdata(&pdev->dev) || !np) + return; + + clock_count = of_clk_get_parent_count(np); + for (i = 0; i < clock_count; i++) { + struct simplefb_clock *entry; + struct clk *clock = of_clk_get(np, i); + int ret; + + if (IS_ERR(clock)) { + dev_err(&pdev->dev, "%s: clock %d not found: %ld\n", + __func__, i, PTR_ERR(clock)); + continue; + } + + ret = clk_prepare_enable(clock); + if (ret) { + dev_err(&pdev->dev, + "%s: failed to enable clock %d: %d\n", + __func__, i, ret); + clk_put(clock); + continue; + } + + entry = kzalloc(sizeof(struct simplefb_clock), GFP_KERNEL); + if (!entry) { + dev_err(&pdev->dev, + "%s: failed to alloc clock %d list entry.\n", + __func__, i); + clk_disable_unprepare(clock); + clk_put(clock); + continue; + } + + entry->clock = clock; + /* + * add to the front of the list, so we disable clocks in the + * correct order. + */ + list_add(&entry->list, list); + } +} + +static void +simplefb_clocks_destroy(struct list_head *list) +{ + struct list_head *pos, *n; + + list_for_each_safe(pos, n, list) { + struct simplefb_clock *entry = + container_of(pos, struct simplefb_clock, list); + + list_del(&entry->list); + + clk_disable_unprepare(entry->clock); + clk_put(entry->clock); + kfree(entry); + } +} + struct simplefb_par { u32 palette[PSEUDO_PALETTE_SIZE]; + struct list_head clock_list[1]; }; static int simplefb_probe(struct platform_device *pdev) @@ -265,6 +359,8 @@ static int simplefb_probe(struct platform_device *pdev) } info->pseudo_palette = (void *) par->palette; + simplefb_clocks_init(pdev, par->clock_list); + dev_info(&pdev->dev, "framebuffer at 0x%lx, 0x%x bytes, mapped to 0x%p\n", info->fix.smem_start, info->fix.smem_len, info->screen_base); @@ -276,14 +372,15 @@ static int simplefb_probe(struct platform_device *pdev) ret = register_framebuffer(info); if (ret < 0) { dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret); - goto error_unmap; + goto error_clocks; } dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node); return 0; - error_unmap: + error_clocks: + simplefb_clocks_destroy(par->clock_list); iounmap(info->screen_base); error_fb_release: @@ -299,8 +396,10 @@ static int simplefb_probe(struct platform_device *pdev) static int simplefb_remove(struct platform_device *pdev) { struct fb_info *info = platform_get_drvdata(pdev); + struct simplefb_par *par = info->par; unregister_framebuffer(info); + simplefb_clocks_destroy(par->clock_list); framebuffer_release(info); if (!dev_get_platdata(&pdev->dev) && pdev->dev.of_node) simplefb_dt_disable(pdev);
This claims and enables clocks listed in the simple framebuffer dt node. This is needed so that the display engine, in case the required clocks are known by the kernel code and are described in the dt, will remain properly enabled. Signed-off-by: Luc Verhaegen <libv@skynet.be> --- drivers/video/fbdev/simplefb.c | 103 +++++++++++++++++++++++++++++++++++++++- 1 files changed, 101 insertions(+), 2 deletions(-)