Thread (8 messages) 8 messages, 3 authors, 2016-03-02

[PATCH v2 1/6] dt-bindings: arm/marvell: add DT bindings for AP806 DFX Server

From: robh@kernel.org (Rob Herring)
Date: 2016-03-02 17:09:35
Also in: linux-clk, linux-devicetree

On Fri, Feb 26, 2016 at 03:55:49PM -0800, Stephen Boyd wrote:
On 02/26, Thomas Petazzoni wrote:
quoted
I would entirely agree with you if this DFX Server was some sort of
"system control" IP block that provided clocks, resets, and all sort of
other features.

But this DFX server thing is just a bunch of registers with absolutely
no relation to clocks. Due to this, it would be completely awkward to
have clock references like:

	serial {
		clocks = <&dfxserver 42>;
	};

One will wonder "why the heck this UART controller is using a clock
from this really odd dfxserver thing" ? Currently we have:

	serial {
		clocks = <&coreclk 4>;
	};

which makes a *lot* more sense.
Really, this is your argument?
Sorry I don't see the difference and I don't agree with this
argument. dfxserver is just a phandle and possibly a poorly named
one at that. So is coreclk. The second example doesn't make a
*lot* more sense or really any more sense than the first example.
Maybe some #define should be used to make things readable:
&dfxserver CORE_CLK_X or something. Why someone would care what
the name of the phandle is for where the clk is coming from makes
no sense to me.

The miscellaneous register dumping ground, i.e. dfxserver, is a
total mess in hardware, agreed, but it doesn't mean we need to
pick it apart and describe the bits and pieces of it so that our
DT can be read as &coreclk 4 instead of &dfxserver 42.

Somebody delivered this dfxserver hardware block into the SoC.
They decided to put random clk control in there. In terms of
hardware blocks, I would guess that dfxserver has a couple clk
wires coming out and some SoC integration engineer had to wire
those up to the places like the uart that actually use them.
Embrace these hardware design decisions. Represent the hardware
in DT as it is represented in hardware by making one node for the
dfxserver because dfxserver is a hardware block.
Agreed.
quoted
Also, your idea of just hiding everything behind a MFD bothers me quite
a bit. If I push this idea further, then why shouldn't I replace my
entire DT with a single node, that covers the entire register space of
my SoC, and then handle *everything* as a huge MFD. In a way, it would
be quite useful for me, as it would resolve the on-going dispute over
DT binding stability with Rob and Mark.
That's a straw man fallacy. Nobody is asking for this. DT is
about describing relations between hardware blocks and the
resources they use. It is *not* about describing register level
details of hardware blocks and providing some data heavy format
so that drivers are nothing besides DT data driven husks of code.
Nor is it about grouping clk subtypes into different DT subnodes
to make writing drivers easier. That's what gets us into the mess
of DT backward compatibility when the data that should have been
in the driver has been put into DT.
quoted
For sure, I wouldn't have any DT backward compatibility issue, because
everything is hidden in my big MFD. But in terms of the DT as a
representation of the different HW blocks and the relations between
then, such a choice would be quite a failure.
What other functions do you have? You previously said the block was DFT 
registers which I would not expect s/w to ever touch.
I don't see any failure. The dfxserver is a hardware block and
that happens to be a clk provider, plain and simple. Consumers of
those clks are related to the dfxserver and we've properly
expressed the relations between them.
And you could have other types of consumers. Nothing requires nodes and 
providers to be 1-1.

You're being told to do it this way by multiple maintainers both because 
it is the preferred way to describe clocks and it gives a better chance 
for stable bindings. 

Rob
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help