Re: [PATCH v3 1/9] doc: DT: vidc: binding document for Qualcomm video driver

2016-11-15 Thread Stanimir Varbanov
Hi Rob,

Thanks for the comments!

On 11/14/2016 07:04 PM, Rob Herring wrote:
> On Mon, Nov 07, 2016 at 07:33:55PM +0200, Stanimir Varbanov wrote:
>> Add binding document for Venus video encoder/decoder driver
>>
>> Cc: Rob Herring 
>> Cc: Mark Rutland 
>> Cc: devicet...@vger.kernel.org
>> Signed-off-by: Stanimir Varbanov 
>> ---
>>  .../devicetree/bindings/media/qcom,venus.txt   | 98 
>> ++
>>  1 file changed, 98 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt
>>
>> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt 
>> b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> new file mode 100644
>> index ..b2af347fbce4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
>> @@ -0,0 +1,98 @@
>> +* Qualcomm Venus video encode/decode accelerator
>> +
>> +- compatible:
>> +Usage: required
>> +Value type: 
>> +Definition: Value should contain one of:
>> +- "qcom,venus-msm8916"
>> +- "qcom,venus-msm8996"
> 
> The normal ordering is ,-

OK.

> 
>> +- reg:
>> +Usage: required
>> +Value type: 
>> +Definition: Register ranges as listed in the reg-names property.
>> +- reg-names:
>> +Usage: required
>> +Value type: 
>> +Definition: Should contain following entries:
>> +- "venus"   Venus register base
>> +- reg-names:
> 
> I'd prefer these grouped as one entry for reg-names.
> 
>> +Usage: optional for msm8996
> 
> Why optional?

The Venus hw block can work without internal video memory in which case
just performance will be impacted.

> 
>> +Value type: 
>> +Definition: Should contain following entries:
>> +- "vmem"Video memory register base
>> +- interrupts:
>> +Usage: required
>> +Value type: 
>> +Definition: Should contain interrupts as listed in the interrupt-names
>> +property.
>> +- interrupt-names:
>> +Usage: required
>> +Value type: 
>> +Definition: Should contain following entries:
>> +- "venus"   Venus interrupt line
>> +- interrupt-names:
>> +Usage: optional for msm8996
>> +Value type: 
>> +Definition: Should contain following entries:
>> +- "vmem"Video memory interrupt line
>> +- clocks:
>> +Usage: required
>> +Value type: 
>> +Definition: A List of phandle and clock specifier pairs as listed
>> +in clock-names property.
>> +- clock-names:
>> +Usage: required
>> +Value type: 
>> +Definition: Should contain the following entries:
>> +- "core"Core video accelerator clock
>> +- "iface"   Video accelerator AHB clock
>> +- "bus" Video accelerator AXI clock
>> +- clock-names:
>> +Usage: required for msm8996
> 
> Plus the 3 above?

Yes, 'required' without 'for xxx' means that the clocks are required for
all hw versions (SoCs) and msm8996 needs the extra clocks below.

> 
>> +Value type: 
>> +Definition: Should contain the following entries:
>> +- "subcore0"Subcore0 video accelerator clock
>> +- "subcore1"Subcore1 video accelerator clock
>> +- "mmssnoc_axi" Multimedia subsystem NOC AXI clock
>> +- "mmss_mmagic_iface"   Multimedia subsystem MMAGIC AHB clock
>> +- "mmss_mmagic_mbus"Multimedia subsystem MMAGIC MAXI clock
>> +- "mmagic_video_bus"MMAGIC video AXI clock
>> +- "video_mbus"  Video MAXI clock
>> +- clock-names:
>> +Usage: optional for msm8996
> 
> Clocks shouldn't be optional unless you failed to add in an initial 
> binding.

These clocks are needed by video memory block which I noted as
'optional'. There is another way to model this video memory hw block
i.e. by a child node of the venus node. Is that sounds better?



-- 
regards,
Stan
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 1/9] doc: DT: vidc: binding document for Qualcomm video driver

2016-11-14 Thread Rob Herring
On Mon, Nov 07, 2016 at 07:33:55PM +0200, Stanimir Varbanov wrote:
> Add binding document for Venus video encoder/decoder driver
> 
> Cc: Rob Herring 
> Cc: Mark Rutland 
> Cc: devicet...@vger.kernel.org
> Signed-off-by: Stanimir Varbanov 
> ---
>  .../devicetree/bindings/media/qcom,venus.txt   | 98 
> ++
>  1 file changed, 98 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/qcom,venus.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/qcom,venus.txt 
> b/Documentation/devicetree/bindings/media/qcom,venus.txt
> new file mode 100644
> index ..b2af347fbce4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/qcom,venus.txt
> @@ -0,0 +1,98 @@
> +* Qualcomm Venus video encode/decode accelerator
> +
> +- compatible:
> + Usage: required
> + Value type: 
> + Definition: Value should contain one of:
> + - "qcom,venus-msm8916"
> + - "qcom,venus-msm8996"

The normal ordering is ,-

> +- reg:
> + Usage: required
> + Value type: 
> + Definition: Register ranges as listed in the reg-names property.
> +- reg-names:
> + Usage: required
> + Value type: 
> + Definition: Should contain following entries:
> + - "venus"   Venus register base
> +- reg-names:

I'd prefer these grouped as one entry for reg-names.

> + Usage: optional for msm8996

Why optional?

> + Value type: 
> + Definition: Should contain following entries:
> + - "vmem"Video memory register base
> +- interrupts:
> + Usage: required
> + Value type: 
> + Definition: Should contain interrupts as listed in the interrupt-names
> + property.
> +- interrupt-names:
> + Usage: required
> + Value type: 
> + Definition: Should contain following entries:
> + - "venus"   Venus interrupt line
> +- interrupt-names:
> + Usage: optional for msm8996
> + Value type: 
> + Definition: Should contain following entries:
> + - "vmem"Video memory interrupt line
> +- clocks:
> + Usage: required
> + Value type: 
> + Definition: A List of phandle and clock specifier pairs as listed
> + in clock-names property.
> +- clock-names:
> + Usage: required
> + Value type: 
> + Definition: Should contain the following entries:
> + - "core"Core video accelerator clock
> + - "iface"   Video accelerator AHB clock
> + - "bus" Video accelerator AXI clock
> +- clock-names:
> + Usage: required for msm8996

Plus the 3 above?

> + Value type: 
> + Definition: Should contain the following entries:
> + - "subcore0"Subcore0 video accelerator clock
> + - "subcore1"Subcore1 video accelerator clock
> + - "mmssnoc_axi" Multimedia subsystem NOC AXI clock
> + - "mmss_mmagic_iface"   Multimedia subsystem MMAGIC AHB clock
> + - "mmss_mmagic_mbus"Multimedia subsystem MMAGIC MAXI clock
> + - "mmagic_video_bus"MMAGIC video AXI clock
> + - "video_mbus"  Video MAXI clock
> +- clock-names:
> + Usage: optional for msm8996

Clocks shouldn't be optional unless you failed to add in an initial 
binding.

> + Value type: 
> + Definition: Should contain the following entries:
> + - "vmem_bus"Video memory MAXI clock
> + - "vmem_iface"  Video memory AHB clock
> +- power-domains:
> + Usage: required
> + Value type: 
> + Definition: A phandle and power domain specifier pairs to the
> + power domain which is responsible for collapsing
> + and restoring power to the peripheral.
> +- rproc:
> + Usage: required
> + Value type: 
> + Definition: A phandle to remote processor responsible for
> + firmware loading and processor booting.
> +
> +- iommus:
> + Usage: required
> + Value type: 
> + Definition: A list of phandle and IOMMU specifier pairs.
> +
> +* An Example
> + video-codec@1d0 {
> + compatible = "qcom,venus-msm8916";
> + reg = <0x01d0 0xff000>;
> + reg-names = "venus";
> + interrupts = ;
> + interrupt-names = "venus";
> + clocks = <&gcc GCC_VENUS0_VCODEC0_CLK>,
> +  <&gcc GCC_VENUS0_AHB_CLK>,
> +  <&gcc GCC_VENUS0_AXI_CLK>;
> + clock-names = "core", "iface", "bus";
> + power-domains = <&gcc VENUS_GDSC>;
> + rproc = <&venus_rproc>;
> + iommus = <&apps_iommu 5>;
> + };
> -- 
> 2.7.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html