Hi Laurent,
Thanks for your comments!
On Mon, Jun 22, 2015 at 10:36 PM, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> On Monday 22 June 2015 21:49:23 Geert Uytterhoeven wrote:
>> 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.
>> - 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.
>> - 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.
>> - 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 :-)
>
>> 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.
>> - 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).
>> - 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.
>> - 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.
>> - 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@xxxxxxxxxxxxxx
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
