Re: [PATCH] ARM: vexpress: bindings: Add generic PD awareness to the spc controller

[Mathieu Poirier]

On 7 January 2015 at 03:54, Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx> wrote:
> On Tue, Jan 06, 2015 at 10:01:03PM +0000, Rob Herring wrote:
>> On Tue, Jan 6, 2015 at 12:36 PM, Mathieu Poirier
>> <mathieu.poirier@xxxxxxxxxx> wrote:
>> > On 6 January 2015 at 10:02, Rob Herring <robherring2@xxxxxxxxx> wrote:
>> >> On Tue, Jan 6, 2015 at 10:45 AM,  <mathieu.poirier@xxxxxxxxxx> wrote:
>> >>> From: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>> >>>
>> >>> Among other things, the serial power controller (SPC) controls power to
>> >>> the A7 and A15 clusters.  Theses clusters also happen to contains the
>
> "These clusters also happen to contain"
>
>> >>> coresight tracers used for HW assisted tracing.
>> >>>
>> >>> By modellling these to power domains in a way that is comprehensible to
>                ^ too many l
>
>> >>> the generic power domain sub-system and using the runtime PM API in the
>> >>> coresight drivers, we can prevent power to the domains from being
>> >>> turned off while tracing related operations are still pending.
>> >>>
>> >>> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>> >>> ---
>> >>>  .../bindings/arm/vexpress-power-controller.txt     | 54 ++++++++++++++++++++++
>> >>>  1 file changed, 54 insertions(+)
>> >>>  create mode 100644 Documentation/devicetree/bindings/arm/vexpress-power-controller.txt
>> >>>
>> >>> diff --git a/Documentation/devicetree/bindings/arm/vexpress-power-controller.txt b/Documentation/devicetree/bindings/arm/vexpress-power-controller.txt
>> >>> new file mode 100644
>> >>> index 000000000000..3af5624dc5cb
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/arm/vexpress-power-controller.txt
>> >>> @@ -0,0 +1,54 @@
>> >>> +ARM Versatile Express Power Controller
>> >>> +--------------------------------------
>> >>> +
>> >>> +This binding models the serial power controller (SPC) in a way that is
>> >>> +intelligible to the generic power domain subsystem and in accordance
>> >>> +with the guidelines from:
>> >>> +
>> >>> +Documentation/devicetree/bindings/power/power_domain.txt
>> >>> +
>> >>> +The binding doesn't have a '<reg>' property as the base address for HW
>> >>> +access is provided by the vexpress-scc sub-system.
>> >>> +
>> >>> +Required node properties:
>> >>> +- compatible value : = "arm,vexpress-power-controller";
>> >>> +- #power-domain-cells : = Number of cells in a PM domain specifier, as
>> >>> +  specified in "power_domain.txt" referenced above.
>> >>> +
>> >>> +Example:
>> >>> +       A7_A15_cluster_pd: A7-A15-cluster-pd {
>> >>
>> >> This is more a description of the power domain than the power
>> >> controller IP block.
>> >
>> > This one is a little tricky - the controller itself (SPC) is accessed
>> > via registers mapped by another driver (SCC) and have two completely
>> > separate files.  The SPC driver itself is not DT'ed, simply because it
>> > doesn't have too,  hence writing things the way I did.
>> >
>> > I will ask the vexpress maintainers where they want to see this code
>> > going.  Would you accept this binding as part of the existing
>> > "arm,vexpress-sysreg" [1] or even "arm,vexpress-scc" [2]?
>
> We are not CC'ed :). [2] is my answer, see below.

Thanks for the review - does it also mean you'd like to see the
creation of the generic power domains moved to tc2_pm.c ?

>
>> > [1]. Documentation/devicetree/bindings/arm/vexpress-sysreg.txt
>> > [2]. Documentation/devicetree/bindings/arm/vexpress-scc.txt
>>
>> Yes, that makes sense. I think either you could have sub node for the
>> SPC or just use the sysreg node phandle directly. Given the
>> simplicity, I'd probably go for the latter, but I haven't looked
>> closely at these drivers and will defer to VExpress maintainers.
>
> There is no such a thing as "vexpress-power-controller", SCC (and SPC)
> are testchip specific, adding the bindings to [2] is what I would like to
> see, with a compatible string that is testchip specific, please refer
> to the bindings for details (ie arm,vexpress-scc,v2p-ca15_a7).
>
>>
>> >
>> >>
>> >>> +               compatible = "arm,vexpress-power-controller";
>> >>> +               #power-domain-cells = <1>;
>> >>
>> >> You need to define what goes in the cell. That is specific to the
>> >> power controller. It could be generic in that N corresponds to power
>> >> domain N in the controller if the controller is generic in that way
>> >> (i.e. all the register accesses are just indexed).
>> >
>> > That is exactly how things are - one controller and two power domains.
>> > Based on what the generic power domain code does #power-domain-cells
>> > is the argument indicating what domain a device should be added to.
>> > From what I read this is also how things are (sparsely) explained in
>> > "power_domain.txt".  As such I'm not exactly sure of what you'd like
>> > to see modified - enlightenment would be appreciated.
>>
>> I wasn't sure if there are more domains you didn't add. If what 0 and
>> 1 correspond to varies by board, then it's probably fine as it. If
>> they are always cluster 0 and cluster 1, then just say that.
>
> It is testchip specific (see above), and it should be documented as
> such. You should define what the cell number corresponds to (eg cell == 0 A15
> cluster power domain, cell == 1 A7 cluster power domain).
>
> Lorenzo
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/