On Sun, Aug 03, 2014 at 08:44:44PM +0200, Maxime Ripard wrote: > 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.
And the fundamental difference 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. s/u-boot/simplefb/ > > > 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. Err, no. The KMS will replace everything as soon as the kms driver actually starts. The way it is implemented in this patch, the actual bitpoking behind the simplefb driver is 95% in u-boot, 5% in kernel. And those 5% are hardcoded, and that is a split that is not acceptable. > > 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. "Describe" being the keyword there. "Referencing" clocks by directly "describing" a bit in a register, that's no longer describing. Why not move all register writes, with a bit of extra fluff for readability into one big humongous dt file to "describe" everything? > > > 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? It was written as <&label register-bit-offset> before. > > > * 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. clock-output-names. > > * 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. static const struct gates_data sun?i_ahb_gates_data in clk-sunxi.c: https://github.com/torvalds/linux/blob/master/drivers/clk/sunxi/clk-sunxi.c#L787 > > 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. Again, in the dt, those nodes depending on ahb gating, they reference the parent clock as <&label bit>. clock-output-names has no function inside the dt itself, you reference clocks by their register and bit directly. clock-output-names only gets a function in the kernel. But there both sides of the data are sitting in different places. clock-output-names lives in the dt and in dt alone. The actual bitmasks that map that list of names to the actual bits in the registers, that sits in the clk-sunxi.c ahb_gating structs. This separation of data is fundamentally wrong, and is a sure sign that something is being awkwardly worked around. Either you are fully name driven, or you are fully bit driven. Not both at the same time. You cannot reference clocks by direct register bits in the dt and use full names in the kernel and expect things to keep on matching up. Why, if referencing clocks by register/bit directly is the way to go, why does the kernel have to be different here? Luc Verhaegen. -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
