| Message ID | 20190701100327.6425-2-antoine.tenart@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: mscc: PTP Hardware Clock (PHC) support | expand |
On Mon, Jul 01, 2019 at 12:03:20PM +0200, Antoine Tenart wrote: > One additional register range needs to be described within the Ocelot > device tree node: the PTP. This patch documents the binding needed to do > so. Hi Antoine Are there any more register banks? Maybe just add them all? Also, you should probably add a comment that despite it being in the Required part of the binding, it is actually optional. Andrew
Hi Andrew, On Mon, Jul 01, 2019 at 03:52:14PM +0200, Andrew Lunn wrote: > On Mon, Jul 01, 2019 at 12:03:20PM +0200, Antoine Tenart wrote: > > One additional register range needs to be described within the Ocelot > > device tree node: the PTP. This patch documents the binding needed to do > > so. > > Are there any more register banks? Maybe just add them all? I checked and there are (just a few) more. I also saw your other comment about interrupts, and it's also true there. Those definitions aren't related to the PHC so I'll prepare a patch for a following series to add all the missing parts. > Also, you should probably add a comment that despite it being in the > Required part of the binding, it is actually optional. I'm not sure about this: optional properties means some parts of the h/w can be missing or not wired. It's not the case here, it's "optional" in the driver only for dt compatibility (so that an older dt blob can work with a newer kernel image), but it's now mandatory in the binding. Thanks! Antoine
On Fri, Jul 05, 2019 at 03:30:16PM +0200, Antoine Tenart wrote: > Hi Andrew, > > On Mon, Jul 01, 2019 at 03:52:14PM +0200, Andrew Lunn wrote: > > On Mon, Jul 01, 2019 at 12:03:20PM +0200, Antoine Tenart wrote: > > > One additional register range needs to be described within the Ocelot > > > device tree node: the PTP. This patch documents the binding needed to do > > > so. > > > > Are there any more register banks? Maybe just add them all? > > I checked and there are (just a few) more. I also saw your other comment > about interrupts, and it's also true there. > > Those definitions aren't related to the PHC so I'll prepare a patch for > a following series to add all the missing parts. Thanks. > > Also, you should probably add a comment that despite it being in the > > Required part of the binding, it is actually optional. > > I'm not sure about this: optional properties means some parts of the h/w > can be missing or not wired. It's not the case here, it's "optional" in > the driver only for dt compatibility (so that an older dt blob can work > with a newer kernel image), but it's now mandatory in the binding. Hi Antoine If the driver can work without it, it is clearly optional. You just get reduced functionality. That is the thing with DT. You can never add more required properties after the first commit without breaking backwards compatibility. To make the documentation fit the driver, somewhere you need to state they are optional. Either by placing the new properties in the optional section of the binding, or add a comment. Andrew
Hi Andrew, On Fri, Jul 05, 2019 at 04:45:17PM +0200, Andrew Lunn wrote: > On Fri, Jul 05, 2019 at 03:30:16PM +0200, Antoine Tenart wrote: > > > > I'm not sure about this: optional properties means some parts of the h/w > > can be missing or not wired. It's not the case here, it's "optional" in > > the driver only for dt compatibility (so that an older dt blob can work > > with a newer kernel image), but it's now mandatory in the binding. > > If the driver can work without it, it is clearly optional. You just > get reduced functionality. That is the thing with DT. You can never > add more required properties after the first commit without breaking > backwards compatibility. To make the documentation fit the driver, > somewhere you need to state they are optional. Either by placing the > new properties in the optional section of the binding, or add a > comment. The documentation is unrelated to the driver. It's the documentation of the binding itself, which is only describing the h/w. But I discussed this with a someone and I got to the same conclusion as your statement, because there can be old dt blobs in the wild and the binding documentation can be used to make new code. That code should be aware of required/optional properties. I'll fix this in v2. Thanks! Antoine
diff --git a/Documentation/devicetree/bindings/net/mscc-ocelot.txt b/Documentation/devicetree/bindings/net/mscc-ocelot.txt index 9e5c17d426ce..0afadfaa33ee 100644 --- a/Documentation/devicetree/bindings/net/mscc-ocelot.txt +++ b/Documentation/devicetree/bindings/net/mscc-ocelot.txt @@ -12,6 +12,7 @@ Required properties: - "sys" - "rew" - "qs" + - "ptp" - "qsys" - "ana" - "portX" with X from 0 to the number of last port index available on that @@ -44,6 +45,7 @@ Example: reg = <0x1010000 0x10000>, <0x1030000 0x10000>, <0x1080000 0x100>, + <0x10e0000 0x10000>, <0x11e0000 0x100>, <0x11f0000 0x100>, <0x1200000 0x100>, @@ -57,9 +59,10 @@ Example: <0x1280000 0x100>, <0x1800000 0x80000>, <0x1880000 0x10000>; - reg-names = "sys", "rew", "qs", "port0", "port1", "port2", - "port3", "port4", "port5", "port6", "port7", - "port8", "port9", "port10", "qsys", "ana"; + reg-names = "sys", "rew", "qs", "ptp", "port0", "port1", + "port2", "port3", "port4", "port5", "port6", + "port7", "port8", "port9", "port10", "qsys", + "ana"; interrupts = <21 22>; interrupt-names = "xtr", "inj";
One additional register range needs to be described within the Ocelot device tree node: the PTP. This patch documents the binding needed to do so. Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com> --- Documentation/devicetree/bindings/net/mscc-ocelot.txt | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)