On 09/05/2013 12:29 PM, Mike Turquette wrote: > On Wed, Sep 4, 2013 at 11:36 AM, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote: >> On 09/03/2013 05:22 PM, Mike Turquette wrote: >>> Quoting Stephen Warren (2013-08-30 14:37:46) >>>> On 08/30/2013 02:33 PM, Mike Turquette wrote: >> ... >>>>> The clock _data_ seems to always have some churn to it. Moving it out to >>>>> DT reduces that churn from Linux. My concern above is not about kernel >>>>> data size. >>>> >>>> That sounds like the opposite of what we should be doing. >>>> >>>> It's fine for kernel code/data to change; that's a natural part of >>>> development. Obviously, we should minimize churn, through thorough >>>> review, domain knowledge, etc. >>> >>> And with the "clock mapping" style bindings we'll end up changing both >>> the DT binding definition and the kernel. Not great. >> >> What's a "clock mapping" style binding? I guess that means the style >> where you have a single DT node that provides multiple clocks, rather >> than one DT node per clock? >> >> If the kernel driver changes its internal data, I don't see why that >> would have any impact at all on the DT binding definition. We should be >> able to use one DT binding definition with arbitrary drivers. > > Yes, I'm referring to a single node providing multiple clocks. As an > example see the Exynos 5420 binding: > Documentation/devicetree/bindings/clock/exynos5420-clock.txt > > The clock id's are stored as part of the binding definition resulting > in a mapping scheme that can be fragile. The mapping shouldn't be fragile if e.g. include/dt-bindings/clock/exynos5420.h were used to define the values. That way, both the Exynos clock driver and Exynos DT files could both include the header, and would always be in sync. > There have already been > patches to fix the id's assigned in the binding, which isn't supposed > to happen because it's a stable interface. That's definitely a real problem. The values should be stable. Preferably, the values should be derived from some aspect of the HW, and hence be stable. For example, many clock IDs on Tegra are derived from the clock's bit index within the peripheral clock enable registers. Although I must admit we have a bit of a mess in the Tegra clocks w.r.t. mis-using clock IDs for reset IDs and hence there are some peripheral clock IDS that don't map 1:1 with the register, and there are other clocks which aren't peripheral clocksthat we've assigned arbitrary IDs to rather than some HW-derived ID. Alternatively, perhaps a register address unique to the clock could be used. If new values are added, the additions should all happen in a single tree, and hence can be co-ordinated, thus avoiding any merge-conflicts. Even ignoring HW-derived clock IDs, people writing DT bindings simply need to get used to bindings being an ABI, and put extra effort into making sure the list of clocks is accurate and complete. Finally, while it's true that a DT binding definition is an ABI, and perhaps DT content isn't (so if there's a DT content bug it can simply be fixed), if DT is wrong because of insufficient thought about its content, it's still wrong, and the system doesn't work correctly. Whether we edit a kernel clock driver or a DT file to solve a problem, there was still a problem. Placing the data into DT doesn't make it any less likely there will be a problem if sufficient care isn't taken when thinking about the clock structure. > If clock phandles are > created by individual nodes in DT then the binding definition need > never be updated due to merge conflicts or renaming which plagues the > mapping scenario. That's true. But if we take that approach, shouldn't we just ban #clock-cells? The only case #clock-cells would still be legitimate would be an array of identical clocks represented by a single node, and even then the argument could be extended so say: just write out a node for each clock in the array, just like if the clocks weren't in an array or were different types. >>> And I'll respond to your points below but the whole "relocate the >>> problem to DT" argument is simply not my main point. What I want to do >>> is increase the usefulness of DT by allowing register-level details into >>> the binding which can >> >> Can you expand upon why a DT that encodes register-level details is more >> useful? I can't see why there would be any difference in usefulness. > > Sure. The usefulness comes out of the fact that we do not need to > maintain data synchronization across dts and clock provider drivers. Only the clock IDs. That's a very small amount of information. And synchronizing the two simply means including a header file that defines the IDs in both places. This is *exactly* why I created the include/dt-bindings/ directory, to house such header files. > The data lives in one place and only one place. We absolutely need a > phandle to a clock in DT link clock consumer devices to their input > clocks, so there is no question that should be in DT. Since we're > already doing that, why not do away with trying to keep dts and C > files in sync and just put all of the data in dts? It's a pure > separation of logic and data. The Linux clock provider driver is > purely logic and no data, which I imagine would appease the mind of > many a computer scientist. Separation of code and data is good. However, one can achieve that simply by putting data into C structs/arrays, without having to parse it out of DT. > Can you return the favor and tell me why putting register level > details into DT is inherently a bad idea? I'll drop my case if I can > be convinced that putting that level is detail into DT is The Wrong > Way, but I'll need more to go on than tradition and status quo. Here are a few reasons, in no particular order: 1) At least for large SoCs (rather than e.g. a simple clock generator chip/crystal with 1 or 2 outputs), clock drivers need a *lot* of data. We can either: a) Put that data into the clock driver, so it's "just there" for the clock driver to use. b) Represent that data in DT, and write code in the clock driver to parse DT when the driver probe()s. Option (a) is very simple. Option (b) entails writing (and executing) a whole bunch of DT parsing code. It's a lot of effort to define the DT binding for the data, convert the data into DT format, and write the parsing code. It wastes execution time at boot. In the end, the result of the parsing is exactly the same data structures that could have been embedded into DT in the first place. This seems like a futile effort. 2) If the clock driver knows it's running on e.g. Tegra20 vs. Tegra30, that information alone is enough to fully describe all details of the clock tree present within the SoC. That information is cast in stone in the SoC HW design. Philosophically, I believe DT should be used to describe what varies between different uses of a HW block, not the internals of a HW block itself. This means describing the environment around an IP block (e.g. which interrupts sinks an I2C controller is connected to, which GPIOs a board used for an SD card CD line), rather than the internals of a block, which are completely fixed. For clocks, this means that the routing of a clock module's inputs/outputs are useful to describe in DT, since they may be connected differently depending on which SoC a clock module is placed into, or which board an SoC is placed into. However, the HW within the block is fixed, so doesn't need to be represented by a data structure whose intent is to represent environmental differences. To be honest, I would rather not even put e.g. on-SoC I2C, SPI, SDHCI controllers into DT, since they are 100% defined by the top-level SoC node. However, in practice we must put those nodes into DT for a few reasons: a) They act as a container for the I2C/SPI devices that are connected to them, so at least something has to exist in DT. b) There's some board-specific parameterization of those controllers such as max bus clock rate for I2C/SPI, bus width for SDHCI, which GPIOs are used for CD/WP for SDHCI, or even whether the controller is enabled/disabled. c) Some of the resources those controllers use (IRQs, GPIOs) may also be used by board-specific entities (e.g. off-SoC IRQ source or GPIO sink). The on-SoC devices appear in DT in order to allow representation of the IRQs/GPIOs they use in a consistent manner with off-SoCs devices, for simplicity. As such, we end up treating them much like any other off-SoC device in terms of representing them as a DT node. 3) Why are clocks a special case? A "simple-gpio" controller binding and driver was proposed, and we had a very similar conversation to this one then. I believe simple-gpio was rejected for the reasons I presented above. pintctrl-simple was initially rejected because it would have ended up being essentially a very complex list of (register, value) writes that the kernel performed at bootup. I'm not sure how pinctrl-simple finally got accepted into the kernel; I think people were just busy and didn't notice it and hence didn't object:-( If we take the "DT should represent the register details" argument to extreme, why not have some hyperbole? :-) a) Do it for everything. For example, Tegra20 and Tegra30 I2S are semantically the same, but with register offsets moved around rather randomly. Perhaps we should have a mapping table of register field name to offset, bit position, and size in DT, and some automated abstraction layer in the kernel to parse this and re-direct all the register IO so we can use a single driver. To me this sounds a bit ridiculous, whereas putting all the clock register details in DT perhaps doesn't (at least depending on exactly what that ends up meaning). However, they're really exactly the same thing. b) Why have drivers at all? Shouldn't the kernel just manage the core ARM CPU, memory, MMU, and other standardized low-level tasks, yet all drivers should be written in Forth with the byte-code stored in DT and evaluated by the kernel instead? This even separates the driver code out of the kernel, and really reduces churn:-) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
