Hi Laurent,

Thanks for your comments!

On Mon, Jun 22, 2015 at 10:36 PM, Laurent Pinchart
[off-list ref] wrote:

On Monday 22 June 2015 21:49:23 Geert Uytterhoeven wrote:

quoted

On Renesas SoCs (SH/R-Mobile, R-Car, RZ), the CPG (Clock Pulse Generator),
MSTP (Module Standby and Software Reset), and APMU (Advanced Power
Management Unit for AP-System Core) blocks are very intimately tied.

  - The CPG clock generates various clocks for LSI internal generation,
  - The MSTP block provides two functions:
      1. Module Standby: "Clock supply to specified modules is stopped by
         setting the module stop control register bits."
         However, the clock supply to a module is not stopped until all CPUs
         in the SoC agree.  Indeed, there are separate MSTP registers for
         application (Cortex-A) and real-time (SH and/or Cortex-R) cores.
      2. Reset Control. to perform a software reset of a specific module.
  - The APMU controls power and clock supply to the AP-system core (e.g.
  CA7, CA15, SCU).

MSTP and CPG registers are mixed in one register block.
The APMU has one or two separate register blocks.

The current DT bindings are split across 4 different binding documents:
  1. Renesas SoC-specific "core" CPG clocks that cannot be represented
easily in DT in another way (cfr. [1]),
  2. Renesas-specific MSTP "Module Standby" clocks (cfr. [2]),
  3. Renesas-specific "div6" variable factor clocks, also generated by the
CPG (cfr. [3]).
  3. Generic "fixed-factor-clock" clocks, generated by the CPG, but
represented in DT using the generic ""fixed-factor-clock" bindings (cfr.
[4]),

Note that currently the Reset Control function of the MSTP block is not
supported by DT nor Linux, and that there are no DT bindings for the APMU
block yet.


Issues with current bindings
----------------------------

General:
  - Tight coupling of CPG and MSTP is not represented in DT:
      - There's one CPG node, and separate MSTP nodes (one for each block of
        up to 32 possible MSTP clocks) next to it,
      - CPG Clock Domain requires "power-domain = <...>" properties linking
        individual device nodes to the "CPG/MSTP" clock domain provider.
        For now I use a link to the (single) CPG device node, while the
        clock provider (mostly) controls the (multiple) MSTP clocks (cfr.
        [6]).
      - On R-Car Gen3, any CPG/MSTP (and APMU) register write access
        requires writing a special value to a specific "Write Protect
        Register" inside the CPG first. Exact details and impact are still
        under investigation (cfr. [7]).

MSTP-specific:
  - The clock hierarchy (parent-child relationship) is represented in a flat
    structure,

What do you mean exactly here ?

If all clocks are structured in a tree topology, it would be nice to have a
tree topology in DT, too.
I agree this is impossible with clocks, as they can have multiple parents,
but it worked nicely with hierarchical SYSC power domains on R-Mobile.

quoted

  - Multiple arrays have to be kept in sync:
      - "clocks" (parents),
      - "clock-indices" (sparse bit index inside register),
      - "clock-output-names",
  - MSTP clocks are referred to using a reference to a device node and a bit
    index:
       - Bit index definitions in include/dt-bindings/clock/<soc>-clock.h
         are separated from the .dtsi file,
       - There's no protection against using a bit index definition for a
         different MSTP register, e.g.
           - clocks = <&mstp8_clks R8A7791_CLK_QSPI_MOD> (wrong),
           - clocks = <&mstp9_clks R8A7791_CLK_QSPI_MOD> (correct),

If this concerns you, we could add the MSTP number to the macro name, making
it easier to catch such issues.

That's one solution. But this doesn't solve the multiple arrays that have to be
kept in sync, which is where most bugs crept in in the past.

quoted

   - The "reg" properties contain only the SMSTPCRx (control register for
     System CPU) and MSTPSRx (optional status register) registers only.
     However, in reality there are many more registers:
       - Separate control registers for application (Cortex-A, supported)
         and real-time (SH and/or Cortex-R, not supported) cores,
       - Software reset registers (not supported).
     To make matters more complicated:
       - Many registers are optional.
         The current bindings just support one control register and one
         optional status register, but this doesn't scale to more register
         sets,

We could just use named registers for that.

Sure, up to 6 resources, for up to 6 out of 7 types of registers.
Sounds like overkill to me.

quoted

       - Register sets are not linear and not contiguous in the register
         space. I.e. it's not possible to derive the address of SMSTPCRx
         from the address of SMSTPCRy, or the address of MSTPSRx from the
         address of SMSTPCRx.

When the hardware is to be blamed we seem to all agree easily :-)

quoted

Proposal
--------

I'd like to propose the following, to resolve (most of) the issues with the
current bindings:

  - Get rid of the "clocks" node.
    IIRC grouping all clocks under a "clock" node was deprecated, but I
    can't find the email anymore?
      - "fixed-clock" clocks are now at the root node level,

I wonder whether we should move all clocks to a separate .dtsi, they take
quite a bit of space.

Perhaps.
Should we share more .dtsis for similar parts in the same SoC family,
cfr. arch/arm/boot/dts/am*?
Our SoC-specific compatible values complicate that, though.

quoted

      - The CPG node is now at the root node level,

The CPG node should actually be a child of a bus, not a child of the root
node. That's a separate issue though.

Yep. We don't have a bus topology in DT for on-SoC peripherals (yet).

quoted

  - CPG node:
      - Use e.g. "renesas,r8a7791-cpg" as compatible value,
  - MSTP clocks:
      - All MSTP clocks are now grouped inside the CPG node, under a new
        "mstp" subnode, without compatible values,

We've discussed this several times in the past and agreed, the MSTP nodes
should be subnodes of the CPG node.

OK.

quoted

      - Each individual MSTP clock has its own subnode, referring to the
        MSTP clock number, instead of using one node per register, and
        arrays of "clocks", "clock-output-names", and "clock-indices"
        properties.

That I don't really like. It would result in more than a hundred of clock
nodes. I have discussed this option with Mike when designing the clock DT
bindings and he didn't like it either.

I missed that discussion. Was it on a mailing list, with an archive?

If each clock node would have been a full-blown device node, with its own
compatible value, I can understand, as that would lead to one platform_device
+ resources per clock, i.e. quite some memory.

With my proposal, there would be only one platform device, for the CPG
device node (and possibly one for each div6 clock), i.e. much less memory
consumption.

quoted

      - The list of available registers is maintained as arrays inside the
        unified CPG/MSTP driver, e.g.

        struct cpg {
                void __iomem *base;
                unsigned int n_mstp;    /* Size of a register set */
                /*
                 * Optional arrays of register offsets for the various
                 * register sets:
                 *   - Array pointer may be NULL, if not supported */
                 *   - Array element may be ~0, if not supported */
                 */
                u16 *smstp;             /* System CPU control registers */
                u16 *rmstp;             /* Realtime CPU control registers */
                u16 *mstpsr;            /* Status registers */
                u16 *srcr;              /* Reset registers */
                ...

I'd create a struct to group smstp, rmstp, mstpsr, srcr & co and have a single
array of those structures.

I deliberately chose multiple arrays, as over the entire SoC range, 2-6 register
types (out of 7) are available. All other array pointers can be NULL.
In addition, not all MSTP blocks have the same number of register types
(e.g. R-Car Gen1 doesn't have status registers for all MSTP blocks).

You forgot the favourite topic of DT maintainers : how will this handle
backward compatibility ?

The new CPG node is compatible with e.g. "renesas,r8a7791-cpg", which is a
brand new value. Hence as long as you compile in the old driver, it'll be
compatible with old DTs.

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds