On Sun, Aug 03, 2014 at 05:17:40PM +0200, Luc Verhaegen wrote: > On Sun, Aug 03, 2014 at 01:53:56PM +0200, Maxime Ripard wrote: > > Hi, > > > > You forgot to Cc the linux-arm-kernel mailing-list and Emilio. > > This was deliberate, as the contents of the patch is still > controversial.
The content of your patch is not controversial, how you implement it is. > > That looks wrong. Why doesn't simplefb claim the clocks itself? > > The code up there uses clock names, and a set of three names which > requires the code to know which clocks are needed to make u-boot happy. I don't really know why you involve u-boot here. U-boot is long gone when that code runs. > This is fundamentally wrong, as it spreads the u-boot graphics driver > over both u-boot and the kernel, and this sort of detachment and lack > of segmentation is totally unacceptable. Hopefully, your KMS driver will fix that. > So, to have simplefb claim these clocks, as is, simplefb would need to > know the names of the relevant clocks. Adding platform and u-boot > version specific code to the simplefb driver directly is even more > fundamentally wrong. > > The solution for that is to have u-boot create the list of relevant > clocks in the dt node for simplefb. Which then runs straight into the > clocks being referenced as <&label register-bit-offset>, and the whole > discussion around that again. There's no discussion around that. It's just how it works, when the clock-cells property is set to one. It really is just that simple. > As stated, i will be working on writing up the code for that, and it > will nicely show how contrived this is. Time will further show how the > lack of abstraction with respect to clocks will come back to bite us. What lack of abstraction? We're talking about the DT here. You know, the thing that is supposed to describe how the hardware is laid out, and how different hardware block interact with each other. There's no abstraction because we are dealing with the lowest-level here. > The most obvious sign is that our clock code is spread over 3 places: > * once in the dts, to have other nodes claim the clock, by > <&label register-bit offset> Erm, what? What is that offset? > * once in the dts where the clock register is given a list of bit names > but no bit definitions. Still have no idea of what you are talking about. > * once in the clock driver where bitmasks exist (but no names) which > must match the list of names in the dts perfectly. Again, that has nothing to do with what you are talking about here. > If simplefb not claiming the clocks felt fundamentally wrong, then the > above should've made your hair stand on end. It is pure luck that that > works today, but that sort of luck will run out very quickly. I must be really dumb, but I really can't see the issue. Clocks will be enabled by u-boot, left enabled by Linux at boot, until after the drivers registrations, where the unused clocks will be disabled. I don't see any luck or anything wrong with this. -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
signature.asc
Description: Digital signature
