Hi Geert,

On 09.06.2016 10:54, Geert Uytterhoeven wrote:

Hi Dirk,

On Thu, Jun 9, 2016 at 10:31 AM, Dirk Behme [off-list ref] wrote:

quoted

On 07.06.2016 10:17, Geert Uytterhoeven wrote:

quoted

On Tue, Jun 7, 2016 at 9:53 AM, Dirk Behme [off-list ref]
wrote:

quoted

I think I just want to discuss if we have a clever idea to further
improve
one detail. That is, if we have a clever idea to avoid the copy & paste
between the family members using anything like a hierarchical way of
defining the clocks in r8a779x-cpg-mssr.h.

quoted

Given the small amount of work needed to bootstrap r8a7796, I
think they still hold on their promises.

Well, regarding the r8a779x-cpg-mssr.h the small amount of work needed
isn't
a really good argument if you are good with cp & sed for the copy & paste
done ;)

They're not really created by cp & sed, as they must match the table in
the
datasheet (the latter may have been created by copy & paste though :-)

quoted

What I fear we end up the way we are doing the copy & paste
r8a779x-cpg-mssr.h is having 5 or 6 or even more of these files, all >
90%
identical. And once you have to change anything, you either have to
change
all these files. Or you miss anything, ending up with subtle bugs when
one
SoC does behave differently than an other one.

The point is these files are stable ABI: no single value can be changed.
No value can be reused. Only new values can be appended at the bottom
(if a newer revision of the datasheet documents more clocks than the old
 version, which happens from time to time).

IMHO a hierarchical way of defining the clocks has more opportunity of
accidentally referring to a clock that doesn't exist on a particular SoC.

Furthermore, r8a779x-cpg-mssr.h is not a good name to be part of a DT
binding,
due to the wildcard.
A future SoC may will match r8a779x and even be called (R-Car
<something>3?),
while using a completely different CPG block.

Just to give an example about what we are talking, I've done [1] below. This
should just be an *example*, though. I'm sure that we might need a more
clever way.

Thanks, it matches with what I had in mind what you wanted to do ;-)

quoted

--- a/include/dt-bindings/clock/r8a7796-cpg-mssr.h
+++ b/include/dt-bindings/clock/r8a7796-cpg-mssr.h

quoted

+#include <dt-bindings/clock/rcar3-cpg-mssr.h>

quoted

+#define R8A7796_CLK_ZB3D4              47
+#define R8A7796_CLK_S0D2               48
+#define R8A7796_CLK_S0D3               49
+#define R8A7796_CLK_S0D6               50
+#define R8A7796_CLK_S0D8               51
+#define R8A7796_CLK_S0D12              53

This means on M3-W, clocks may have different prefixes:
  - RCAR3_CLK_* if they are defined as common R-Car Gen3 clocks,
  - R8A7796_CLK_* if they are defined as M3-W-specific clocks.

Not such a big issue, it that can be worked around using

    #define R8A7796_CLK_Z RCAR3_CLK_Z
    [...]

quoted

+++ b/include/dt-bindings/clock/rcar3-cpg-mssr.h

@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) 2015 Renesas Electronics Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#ifndef __DT_BINDINGS_CLOCK_RCAR3_CPG_MSSR_H__
+#define __DT_BINDINGS_CLOCK_RCAR3_CPG_MSSR_H__
+
+#include <dt-bindings/clock/renesas-cpg-mssr.h>
+
+/* RCar3 CPG Core Clocks */
+#define RCAR3_CLK_Z            0
+#define RCAR3_CLK_Z2           1
+#define RCAR3_CLK_ZR           2
+#define RCAR3_CLK_ZG           3
+#define RCAR3_CLK_ZTR          4
+#define RCAR3_CLK_ZTRD2                5

While this approach allows to add SoC-specific clocks to the family-specific
base list, it does not allow to remove family-specific clocks that are not
present on unknown future species of the family.

If I answer with

#undef

that would be too easy? ;)


Best regards

Dirk