| Message ID | 1456326866-30854-2-git-send-email-thomas.petazzoni@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/24, Thomas Petazzoni wrote: > +Marvell AP806 DFX Server > +------------------------ > + > +The Marvell AP806 HW block (which is a core component of the Marvell > +Armada 7K and 8K SOCs) has a set of registers called "DFX > +Server". This set of registers contains miscellaneous registers, most > +of them being used for silicon fine-tuning and manufacturing testing, > +and as such are not publicly documented. However, this DFX server > +register range also contains a few documented and useful registers, > +for example for clock control. > + > +This Device Tree binding allows to represent the entire DFX server > +register space as one single DT node. > + > +Required properties: > +- compatible: the first and second values must be: > + "simple-mfd", "syscon" > +- reg: address and length of following register sets for the DFX > + server Example? I would think the binding would be done in a way that we don't have to describe every little register in this misc register block. Instead, dfx server has a compatible: node@f00 { reg = <0xf00 0x100>; compatible = "marvell,dfx-ap806"; #clock-cells = <1>; }; And then a driver that probes this compatible string and registers a handful of clks. If we ever get into a situation where we want to expose non-clk functionality from this device node, we would make some sort of mfd driver that probes based on the same compatible string and creates platform devices in software to register the clk device component and whatever other device type is used. In that situation, we could assign a regmap to the mfd device and the clk device would be a child of the mfd and get a regmap through the dev->parent pointer. TL;DR I'm still lost why we have to describe each clk in DT.
Stephen, Thanks for your feedback! On Thu, 25 Feb 2016 15:37:10 -0800, Stephen Boyd wrote: > I would think the binding would be done in a way that we don't > have to describe every little register in this misc register > block. Instead, dfx server has a compatible: > > node@f00 { > reg = <0xf00 0x100>; > compatible = "marvell,dfx-ap806"; > #clock-cells = <1>; > }; > > And then a driver that probes this compatible string and > registers a handful of clks. If we ever get into a situation > where we want to expose non-clk functionality from this device > node, we would make some sort of mfd driver that probes based on > the same compatible string and creates platform devices in > software to register the clk device component and whatever other > device type is used. In that situation, we could assign a regmap > to the mfd device and the clk device would be a child of the mfd > and get a regmap through the dev->parent pointer. > > TL;DR I'm still lost why we have to describe each clk in DT. 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. 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. 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. Best regards, Thomas
On 02/26, Thomas Petazzoni wrote: > 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. 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. > > 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. > > 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. > 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.
On Fri, Feb 26, 2016 at 03:55:49PM -0800, Stephen Boyd wrote: > On 02/26, Thomas Petazzoni wrote: > > 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. > > 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. > > > > > 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
diff --git a/Documentation/devicetree/bindings/arm/marvell/marvell,ap806-dfx-server.txt b/Documentation/devicetree/bindings/arm/marvell/marvell,ap806-dfx-server.txt new file mode 100644 index 0000000..44eb3f0 --- /dev/null +++ b/Documentation/devicetree/bindings/arm/marvell/marvell,ap806-dfx-server.txt @@ -0,0 +1,19 @@ +Marvell AP806 DFX Server +------------------------ + +The Marvell AP806 HW block (which is a core component of the Marvell +Armada 7K and 8K SOCs) has a set of registers called "DFX +Server". This set of registers contains miscellaneous registers, most +of them being used for silicon fine-tuning and manufacturing testing, +and as such are not publicly documented. However, this DFX server +register range also contains a few documented and useful registers, +for example for clock control. + +This Device Tree binding allows to represent the entire DFX server +register space as one single DT node. + +Required properties: +- compatible: the first and second values must be: + "simple-mfd", "syscon" +- reg: address and length of following register sets for the DFX + server
The following patch adds a Device Tree binding documentation for the AP806 DFX Server register area found in Marvell Armada 7K/8K SOCs. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- .../bindings/arm/marvell/marvell,ap806-dfx-server.txt | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/marvell/marvell,ap806-dfx-server.txt