Re: [PATCH v4 3/5] clk: dt: binding for basic multiplexer clock
From: Tero Kristo
Date: Fri Sep 06 2013 - 02:54:16 EST
Hi,
Chirping in my thoughts below.
On 09/05/2013 11:30 PM, Stephen Warren wrote:
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.
How can you claim option (a) to be very simple compared to (b)? I think
both are as easy / or hard to implement.
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.
Not really, consider a new SoC where you don't have any kind of data at
all. You need to write the data tables anyway, whether they are under DT
or some kernel data struct. The execution time remain in place for
parsing DT data though, but I wouldn't think this to be a problem. Also,
you should consider multiarch ARM kernel, where same kernel binary
should support multiple SoCs, this would entail having clock data for
all built in to the kernel, which can be a problem also. It just boils
down to balancing things between execution time and memory cost, which
IMO, kernel should not decide, but should be decided by people who use
the kernel for whatever purpose it may be.
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.
You can just as easily claim that anything internal to SoC should be
left out from DT, as this is cast in stone (or rather, silicon) also. We
should only use it to describe board layout. Everything else, the kernel
can 'know' by compile time.
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:-)
I can turn this around, as you went to this road. Why have DT at all?
Personally I hate the whole idea of a devicetree, however am forced to
use it as somebody decided it is a good idea to have one. It doesn't
really solve any problems, it just creates new ones in a way of
political BS where everybody claims they know how DT should be used, and
this just prevents people from actually using it at all. Also, it
creates just one new unnecessary dependency for boot, previously we had
bootloader and kernel images which had to be in sync, now we have
bootloader + DT + kernel. What next? Maybe we should move the clock data
into a firmware file of its own?
Why do you care so much what actually goes into the devicetree?
Shouldn't people be let use it how they see fit? For the clock bindings
business this is the same, even if the bindings are there, you are free
to use them if you like, and if you don't like them, you can do things
differently.
Personally, I have large amount of code which depends on these basic
clock bindings right now, and would like to see them go forward. I can
of course go back and convert the code to such format that everything is
as static tables under kernel, but in that case I don't think I need DT
for anything. Also, then people start to complain again that you should
move stuff to DT. Grrrr.... :)
-Tero
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/