RE: [RFC 3/5] media: platform: rcar_drif: Add DRIF support

Hi Geert,

Thank you for the review comments.

On Wed, Oct 12, 2016 at 4:10 PM, Ramesh Shanmugasundaram
[off-list ref] wrote:

quoted

This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3

SoCs.

quoted

The driver exposes each instance of DRIF as a V4L2 SDR device. A DRIF
device represents a channel and each channel can have one or two
sub-channels respectively depending on the target board.

DRIF supports only Rx functionality. It receives samples from a RF
frontend tuner chip it is interfaced with. The combination of DRIF and
the tuner device, which is registered as a sub-device, determines the
receive sample rate and format.

In order to be compliant as a V4L2 SDR device, DRIF needs to bind with
the tuner device, which can be provided by a third party vendor. DRIF
acts as a slave device and the tuner device acts as a master
transmitting the samples. The driver allows asynchronous binding of a
tuner device that is registered as a v4l2 sub-device. The driver can
learn about the tuner it is interfaced with based on port endpoint
properties of the device in device tree. The V4L2 SDR device inherits
the controls exposed by the tuner device.

The device can also be configured to use either one or both of the
data pins at runtime based on the master (tuner) configuration.

Thanks for your patch!

quoted

--- /dev/null
+++ b/Documentation/devicetree/bindings/media/renesas,drif.txt

@@ -0,0 +1,109 @@
+Renesas R-Car Gen3 DRIF controller (DRIF)
+-----------------------------------------
+
+Required properties:
+--------------------
+- compatible: "renesas,drif-r8a7795" if DRIF controller is a part of

R8A7795 SoC.

"renesas,r8a7795-drif", as Rob already pointed out.

Agreed.

quoted

+             "renesas,rcar-gen3-drif" for a generic R-Car Gen3

compatible device.

quoted

+             When compatible with the generic version, nodes must list

the

quoted

+             SoC-specific version corresponding to the platform first
+             followed by the generic version.
+
+- reg: offset and length of each sub-channel.
+- interrupts: associated with each sub-channel.
+- clocks: phandles and clock specifiers for each sub-channel.
+- clock-names: clock input name strings: "fck0", "fck1".
+- pinctrl-0: pin control group to be used for this controller.
+- pinctrl-names: must be "default".
+- dmas: phandles to the DMA channels for each sub-channel.
+- dma-names: names for the DMA channels: "rx0", "rx1".
+
+Required child nodes:
+---------------------
+- Each DRIF channel can have one or both of the sub-channels enabled
+in a
+  setup. The sub-channels are represented as a child node. The name
+of the
+  child nodes are "sub-channel0" and "sub-channel1" respectively.
+Each child
+  node supports the "status" property only, which is used to
+enable/disable
+  the respective sub-channel.

quoted

+Example
+--------
+
+SoC common dtsi file
+
+drif0: rif@e6f40000 {
+       compatible = "renesas,drif-r8a7795",
+                  "renesas,rcar-gen3-drif";
+       reg = <0 0xe6f40000 0 0x64>, <0 0xe6f50000 0 0x64>;
+       interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
+                  <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
+       clocks = <&cpg CPG_MOD 515>, <&cpg CPG_MOD 514>;
+       clock-names = "fck0", "fck1";
+       dmas = <&dmac1 0x20>, <&dmac1 0x22>;
+       dma-names = "rx0", "rx1";

I could not find the DMAC channels in the datasheet?

It is mentioned only in v0.5 h/w manual. v0.52 manual introduced DRIF chapter but then some of the old references were missing :-(. There are few more doc anomalies, which I shall document in the next version of the patch.

Most modules are either tied to dmac0, or two both dmac1 and dmac2.
In the latter case, you want to list two sets of dmas, one for each DMAC.

You are right. I have added both dmac1 & 2 now.

quoted

+       power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
+       status = "disabled";
+
+       sub-channel0 {
+               status = "disabled";
+       };
+
+       sub-channel1 {
+               status = "disabled";
+       };
+
+};

As you're modelling this in DT under a single device node, this means you
cannot use runtime PM to manage the module clocks of the individual
channels.

An alternative could be to have two separate nodes for each channel, and
tie them together using a phandle.

I agree & thanks for the suggestion. Is the below model looks anything closer? Appreciate your inputs.

dtsi
-------

                drif00: rif@e6f40000 {
                        compatible = "renesas,r8a7795-drif",
									 "renesas,rcar-gen3-drif";
                        reg = <0 0xe6f40000 0 0x64>;
                        interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
                        clocks = <&cpg CPG_MOD 515>;
                        clock-names = "fck";
                        dmas = <&dmac1 0x20>, <&dmac2 0x20>;
                        dma-names = "rx";
                        power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
                        status = "disabled";
                };   

                drif01: rif@e6f50000 {
                        compatible = "renesas,r8a7795-drif",
									 "renesas,rcar-gen3-drif";
                        reg = <0 0xe6f50000 0 0x64>;
                        interrupts = <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
                        clocks = <&cpg CPG_MOD 514>;
                        clock-names = "fck";
                        dmas = <&dmac1 0x22>, <&dmac2 0x22>;
                        dma-names = "rx";
                        power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
                        status = "disabled";
                };
				
                drif0: rif@0 {
                        compatible = "renesas,r8a7795-drif",
                                     "renesas,rcar-gen3-drif";
						sub-channels = <&drif00>, <&drif01>;
                        status = "disabled";
						
                };   

-------
board dts file would have something like this

&drif00 {
        status = "okay";
};      
        
&drif01 {
        status = "okay";
};      
        
&drif0 {
        pinctrl-0 = <&drif0_pins>;
        pinctrl-names = "default";
        status = "okay";

        port {
                drif0_ep: endpoint {
                     remote-endpoint = <&max2175_0_ep>;
                };
        };
};
-------
The drif0X driver instance will help only in registering with genpd.
The parent drif0 instance will parse "sub-channels" phandles and use the resources of respective enabled sub-channels using it's pdev. 

Thanks,
Ramesh

��칻
�&�~�&���+-��ݶ��w��˛���m�^�'
����{ay�
ʇڙ�,j��f���h�����/oSc��ڳ9�u�����&jw��(�階�ݢj"���m�����z�ޖ���f���h���~�m�