by Rob Herring

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq

On Wed, Oct 12, 2016 at 03:12:54PM -0700, Markus Mayer wrote:
> Add the binding document for the new brcmstb-avs-cpufreq driver.
>
> Signed-off-by: Markus Mayer <[email protected]>
> Acked-by: Viresh Kumar <[email protected]>
> ---
> .../bindings/cpufreq/brcm,stb-avs-cpu-freq.txt | 76 ++++++++++++++++++++++
> MAINTAINERS | 7 ++
> 2 files changed, 83 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt b/Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
> new file mode 100644
> index 0000000..23a2c3d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
> @@ -0,0 +1,76 @@
> +Broadcom STB AVS CPUfreq driver bindings

Bindings describe h/w, not drivers or Linux subsystems.

> +========================================
> +
> +A total of three DT nodes are required. One node (brcm,avs-cpu-data-mem)
> +references the mailbox register used to communicate with the AVS CPU[1]. The
> +second node (brcm,avs-cpu-l2-intr) is required to trigger an interrupt on
> +the AVS CPU. The interrupt tells the AVS CPU that it needs to process a
> +command sent to it by a driver. Interrupting the AVS CPU is mandatory for
> +commands to be processed.
> +
> +The interface also requires a reference to the AVS host interrupt controller,
> +so a driver can react to interrupts generated by the AVS CPU whenever a command
> +has been processed. See [2] for more information on the brcm,l2-intc node.
> +
> +[1] The AVS CPU is an independent co-processor that runs proprietary
> +firmware. On some SoCs, this firmware supports DFS and DVFS in addition to
> +Adaptive Voltage Scaling.
> +
> +[2] Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt
> +
> +
> +Node brcm,avs-cpu-data-mem
> +--------------------------
> +
> +Required properties:
> +- compatible: Sould be one of: brcm,avs-cpu-data-mem,
> + brcm,bcm7271-avs-cpu-data-mem or brcm,bcm7268-avs-cpu-data-mem

This does not match the example. Need to clarify valid combinations and
ordering.

> +- reg: Specifies base physical address and size of the registers.
> +- interrupts: The interrupt that the AVS CPU will use to interrupt the host
> + when a command completed.
> +- interrupt-parent: The interrupt controller the above interrupt is routed
> + through.
> +- interrupt-names: The name of the interrupt used to interrupt the host.
> +
> +Optional properties:
> +- None
> +
> +Node brcm,avs-cpu-l2-intr
> +-------------------------
> +
> +Required properties:
> +- compatible: Sould be one of: brcm,avs-cpu-l2-intr,
> + brcm,bcm7271-avs-cpu-l2-intr or brcm,bcm7268-avs-cpu-l2-intr

ditto.

> +- reg: Specifies base physical address and size of the registers.
> +
> +Optional properties:
> +- None
> +
> +
> +Example
> +=======
> +
> + avs_host_l2_intc: interrupt-controller@f04d1200 {
> + #interrupt-cells = <1>;
> + compatible = "brcm,l2-intc";
> + interrupt-parent = <&intc>;
> + reg = <0xf04d1200 0x48>;
> + interrupt-controller;
> + interrupts = <0x0 0x19 0x0>;
> + interrupt-names = "avs";
> + };
> +
> + avs-cpu-data-mem@f04c4000 {
> + compatible = "brcm,bcm7271-avs-cpu-data-mem",
> + "brcm,avs-cpu-data-mem";
> + reg = <0xf04c4000 0x60>;
> + interrupts = <0x1a>;
> + interrupt-parent = <&avs_host_l2_intc>;
> + interrupt-names = "sw_intr";
> + };
> +
> + avs-cpu-l2-intr@f04d1100 {
> + compatible = "brcm,bcm7271-avs-cpu-l2-intr",
> + "brcm,avs-cpu-l2-intr";
> + reg = <0xf04d1100 0x10>;
> + };

I'm wondering if this would be better described as just 1 node for the
AVS cpu? Are there any other functions or resources tied to the cpu
beyond your current usage?

Rob

2016-10-19 17:32:52

by Markus Mayer

[permalink] [raw]

Subject: Re: [PATCH v4 1/3] dt: cpufreq: brcm: New binding document for brcmstb-avs-cpufreq

On 18 October 2016 at 06:38, Rob Herring <[email protected]> wrote:
> On Wed, Oct 12, 2016 at 03:12:54PM -0700, Markus Mayer wrote:
>> Add the binding document for the new brcmstb-avs-cpufreq driver.
>>
>> Signed-off-by: Markus Mayer <[email protected]>
>> Acked-by: Viresh Kumar <[email protected]>
>> ---
>> .../bindings/cpufreq/brcm,stb-avs-cpu-freq.txt | 76 ++++++++++++++++++++++
>> MAINTAINERS | 7 ++
>> 2 files changed, 83 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
>>
>> diff --git a/Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt b/Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
>> new file mode 100644
>> index 0000000..23a2c3d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/cpufreq/brcm,stb-avs-cpu-freq.txt
>> @@ -0,0 +1,76 @@
>> +Broadcom STB AVS CPUfreq driver bindings
>
> Bindings describe h/w, not drivers or Linux subsystems.

I can call this "AVS mail box and interrupt register bindings". Would that work?

>> +========================================
>> +
>> +A total of three DT nodes are required. One node (brcm,avs-cpu-data-mem)
>> +references the mailbox register used to communicate with the AVS CPU[1]. The
>> +second node (brcm,avs-cpu-l2-intr) is required to trigger an interrupt on
>> +the AVS CPU. The interrupt tells the AVS CPU that it needs to process a
>> +command sent to it by a driver. Interrupting the AVS CPU is mandatory for
>> +commands to be processed.
>> +
>> +The interface also requires a reference to the AVS host interrupt controller,
>> +so a driver can react to interrupts generated by the AVS CPU whenever a command
>> +has been processed. See [2] for more information on the brcm,l2-intc node.
>> +
>> +[1] The AVS CPU is an independent co-processor that runs proprietary
>> +firmware. On some SoCs, this firmware supports DFS and DVFS in addition to
>> +Adaptive Voltage Scaling.
>> +
>> +[2] Documentation/devicetree/bindings/interrupt-controller/brcm,l2-intc.txt
>> +
>> +
>> +Node brcm,avs-cpu-data-mem
>> +--------------------------
>> +
>> +Required properties:
>> +- compatible: Sould be one of: brcm,avs-cpu-data-mem,
>> + brcm,bcm7271-avs-cpu-data-mem or brcm,bcm7268-avs-cpu-data-mem
>
> This does not match the example. Need to clarify valid combinations and
> ordering.

If I say "must include brcm,avs-cpu-data-mem and should also include
either brcm,bcm7271-avs-cpu-data-mem or
brcm,bcm7268-avs-cpu-data-mem", would that be acceptable? Same for the
other binding below.

>> +- reg: Specifies base physical address and size of the registers.
>> +- interrupts: The interrupt that the AVS CPU will use to interrupt the host
>> + when a command completed.
>> +- interrupt-parent: The interrupt controller the above interrupt is routed
>> + through.
>> +- interrupt-names: The name of the interrupt used to interrupt the host.
>> +
>> +Optional properties:
>> +- None
>> +
>> +Node brcm,avs-cpu-l2-intr
>> +-------------------------
>> +
>> +Required properties:
>> +- compatible: Sould be one of: brcm,avs-cpu-l2-intr,
>> + brcm,bcm7271-avs-cpu-l2-intr or brcm,bcm7268-avs-cpu-l2-intr
>
> ditto.
>
>> +- reg: Specifies base physical address and size of the registers.
>> +
>> +Optional properties:
>> +- None
>> +
>> +
>> +Example
>> +=======
>> +
>> + avs_host_l2_intc: interrupt-controller@f04d1200 {
>> + #interrupt-cells = <1>;
>> + compatible = "brcm,l2-intc";
>> + interrupt-parent = <&intc>;
>> + reg = <0xf04d1200 0x48>;
>> + interrupt-controller;
>> + interrupts = <0x0 0x19 0x0>;
>> + interrupt-names = "avs";
>> + };
>> +
>> + avs-cpu-data-mem@f04c4000 {
>> + compatible = "brcm,bcm7271-avs-cpu-data-mem",
>> + "brcm,avs-cpu-data-mem";
>> + reg = <0xf04c4000 0x60>;
>> + interrupts = <0x1a>;
>> + interrupt-parent = <&avs_host_l2_intc>;
>> + interrupt-names = "sw_intr";
>> + };
>> +
>> + avs-cpu-l2-intr@f04d1100 {
>> + compatible = "brcm,bcm7271-avs-cpu-l2-intr",
>> + "brcm,avs-cpu-l2-intr";
>> + reg = <0xf04d1100 0x10>;
>> + };
>
> I'm wondering if this would be better described as just 1 node for the
> AVS cpu? Are there any other functions or resources tied to the cpu
> beyond your current usage?

Hm. I thought about this one a bit. brcm,l2-intc is also used
elsewhere. And all three nodes have different, non-adjacent memory
addresses (not hugely so, but still). Mapping the entire area covering
all three registers in one go doesn't seem like a good idea and would
lead to very odd offsets. Also, while it is not likely for some of the
registers to get moved around in newer SoCs, I suppose it is possible
(provided the HW guys have a compelling reason for it). I am by no
means a DT expert, so if there is a way to cleanly represent this as
one node, by all means, please let me know.

> Rob