Hi, On 08/14/2014 12:31 PM, Koen Kooi wrote: > > Op 14 aug. 2014, om 11:37 heeft Hans de Goede <hdegoede@xxxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
