On Sun, Aug 03, 2014 at 09:36:55PM +0200, Luc Verhaegen wrote: > 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?
I want simplefb to run, which means having the clocks enabled, but that code isn't going to be merged as is. > > > 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. We do agree on that then. > > > 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? How would you represent a single register controling 32 gates then? with 32 nodes? Be reasonable. We have to have an ID. We chose for that ID to have the one described in the datasheet. What fools we make.... > > > 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. And I'm supposed to catch all your typos then? > > > > > * 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. Again, nothing to do with the clocks property. Could we *please* get to *a* point? Any point would do, but really, get your facts together. > > > * 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 Still nothing to do with the clocks property. > > > 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. Yes, and? > 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. We are neither name driver nor bit driven, we are phandle driven. Like I said above, exposing a register controlling 32 gates would then require to create 32 nodes. For sanity reasons, we chose to add an extra argument. Just like it's already used for interrupts, reset lines, gpios, pretty much every thing. The bitmask here is to make sure we don't get an invalid clock ID, and be able to deal with "holes" in our register. -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
signature.asc
Description: Digital signature
