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

[Ramesh Shanmugasundaram]

Hi Laurent,

Thank you for the review comments.

> On Tuesday 18 Oct 2016 16:29:24 Geert Uytterhoeven wrote:
> > On Wed, Oct 12, 2016 at 4:10 PM, Ramesh Shanmugasundaram wrote:
> > > This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3
> SoCs.
> > > 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!
> >
> > > --- /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.
> >
> > > + "renesas,rcar-gen3-drif" for a generic R-Car Gen3
> > > + compatible
> > > device.
> > > + When compatible with the generic version, nodes must
> > > + list
> > > the
> > > + 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.
> > >
> > > +Example
> > > +
> > > +
> > > +SoC common dtsi file
> > > +
> > > +drif0: rif@e6f4 {
> > > +   compatible = "renesas,drif-r8a7795",
> > > +  "renesas,rcar-gen3-drif";
> > > +   reg = <0 0xe6f4 0 0x64>, <0 0xe6f5 0 0x64>;
> > > +   interrupts = ,
> > > +  ;
> > > +   clocks = < CPG_MOD 515>, < CPG_MOD 514>;
> > > +   clock-names = "fck0", "fck1";
> > > +   dmas = < 0x20>, < 0x22>;
> > > +   dma-names = "rx0", "rx1";
> >
> > I could not find the DMAC channels in the datasheet?
> > 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.
> >
> > > +   power-domains = < 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.
> 
> A quick glance at the documentation shows no dependency between the two
> channels at a software level. They both share the same clock and
> synchronization input pins, but from a hardware point of view that seems
> to be it. It thus looks like we could indeed model the two channels as
> separate nodes, without tying them together.

Thanks & I agree with your suggestion to keep each DRIF sub-channels as 
separate node. However, I would 

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

[Ramesh Shanmugasundaram]

Hi Geert,

Thank you for the review comments.

> On Wed, Oct 12, 2016 at 4:10 PM, Ramesh Shanmugasundaram
>  wrote:
> > This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3
> SoCs.
> > 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!
> 
> > --- /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.

> 
> > + "renesas,rcar-gen3-drif" for a generic R-Car Gen3
> compatible device.
> > + When compatible with the generic version, nodes must list
> the
> > + 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.
> 
> > +Example
> > +
> > +
> > +SoC common dtsi file
> > +
> > +drif0: rif@e6f4 {
> > +   compatible = "renesas,drif-r8a7795",
> > +  "renesas,rcar-gen3-drif";
> > +   reg = <0 0xe6f4 0 0x64>, <0 0xe6f5 0 0x64>;
> > +   interrupts = ,
> > +  ;
> > +   clocks = < CPG_MOD 515>, < CPG_MOD 514>;
> > +   clock-names = "fck0", "fck1";
> > +   dmas = < 0x20>, < 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.

> 
> > +   power-domains = < 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@e6f4 {
compatible = "renesas,r8a7795-drif",
 
"renesas,rcar-gen3-drif";
reg = <0 0xe6f4 0 0x64>;
interrupts = ;
   

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

[Ramesh Shanmugasundaram]

Hi Rob,

Thank you for the review comments.

> Subject: Re: [RFC 3/5] media: platform: rcar_drif: Add DRIF support
> 
> On Wed, Oct 12, 2016 at 03:10:27PM +0100, Ramesh Shanmugasundaram wrote:
> > This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3
> SoCs.
> > 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.
> >
> > Signed-off-by: Ramesh Shanmugasundaram
> > <ramesh.shanmugasunda...@bp.renesas.com>
> > ---
> >  .../devicetree/bindings/media/renesas,drif.txt |  109 ++
> >  drivers/media/platform/Kconfig |   25 +
> >  drivers/media/platform/Makefile|1 +
> >  drivers/media/platform/rcar_drif.c | 1534
> 
> >  4 files changed, 1669 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/media/renesas,drif.txt
> >  create mode 100644 drivers/media/platform/rcar_drif.c
> >
> > diff --git a/Documentation/devicetree/bindings/media/renesas,drif.txt
> > b/Documentation/devicetree/bindings/media/renesas,drif.txt
> > new file mode 100644
> > index 000..24239d9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt
> > @@ -0,0 +1,109 @@
> > +Renesas R-Car Gen3 DRIF controller (DRIF)
> 
> Define what is DRIF here, not just in the commit text.

Agreed.

> 
> > +-
> > +
> > +Required properties:
> > +
> > +- compatible: "renesas,drif-r8a7795" if DRIF controller is a part of
> R8A7795 SoC.
> 
> renesas,r8a7795-drif would be the normal ordering.

Agreed.

> 
> > + "renesas,rcar-gen3-drif" for a generic R-Car Gen3 compatible
> device.
> > + When compatible with the generic version, nodes must list the
> > + 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.
> > +
> > +Optional properties:
> > +
> > +- port: video interface child port node of a channel that defines the
> > +local
> 
> This is an audio device, why does it have a video port?

Apologies for the wording. I intend to refer a regular port node like mentioned 
here - https://www.kernel.org/doc/Documentation/devicetree/bindings/graph.txt

> 
> > +  and remote endpoints. The remote endpoint is assumed to a tuner
> > +subdevice
> > +  endpoint.
> &

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

[Geert Uytterhoeven]

Hi Ramesh,

On Wed, Oct 12, 2016 at 4:10 PM, Ramesh Shanmugasundaram
 wrote:
> This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3 SoCs.
> 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!

> --- /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.

> + "renesas,rcar-gen3-drif" for a generic R-Car Gen3 compatible 
> device.
> + When compatible with the generic version, nodes must list the
> + 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.

> +Example
> +
> +
> +SoC common dtsi file
> +
> +drif0: rif@e6f4 {
> +   compatible = "renesas,drif-r8a7795",
> +  "renesas,rcar-gen3-drif";
> +   reg = <0 0xe6f4 0 0x64>, <0 0xe6f5 0 0x64>;
> +   interrupts = ,
> +  ;
> +   clocks = < CPG_MOD 515>, < CPG_MOD 514>;
> +   clock-names = "fck0", "fck1";
> +   dmas = < 0x20>, < 0x22>;
> +   dma-names = "rx0", "rx1";

I could not find the DMAC channels in the datasheet?
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.

> +   power-domains = < 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.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

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

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

[Rob Herring]

On Wed, Oct 12, 2016 at 03:10:27PM +0100, Ramesh Shanmugasundaram wrote:
> This patch adds Digital Radio Interface (DRIF) support to R-Car Gen3 SoCs.
> 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.
> 
> Signed-off-by: Ramesh Shanmugasundaram 
> 
> ---
>  .../devicetree/bindings/media/renesas,drif.txt |  109 ++
>  drivers/media/platform/Kconfig |   25 +
>  drivers/media/platform/Makefile|1 +
>  drivers/media/platform/rcar_drif.c | 1534 
> 
>  4 files changed, 1669 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/renesas,drif.txt
>  create mode 100644 drivers/media/platform/rcar_drif.c
> 
> diff --git a/Documentation/devicetree/bindings/media/renesas,drif.txt 
> b/Documentation/devicetree/bindings/media/renesas,drif.txt
> new file mode 100644
> index 000..24239d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/renesas,drif.txt
> @@ -0,0 +1,109 @@
> +Renesas R-Car Gen3 DRIF controller (DRIF)

Define what is DRIF here, not just in the commit text.

> +-
> +
> +Required properties:
> +
> +- compatible: "renesas,drif-r8a7795" if DRIF controller is a part of R8A7795 
> SoC.

renesas,r8a7795-drif would be the normal ordering.

> +   "renesas,rcar-gen3-drif" for a generic R-Car Gen3 compatible 
> device.
> +   When compatible with the generic version, nodes must list the
> +   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.
> +
> +Optional properties:
> +
> +- port: video interface child port node of a channel that defines the local

This is an audio device, why does it have a video port?

> +  and remote endpoints. The remote endpoint is assumed to a tuner subdevice
> +  endpoint.
> +- power-domains: phandle to respective power domain.
> +- renesas,syncmd   : sync mode
> +  0 (Frame start sync pulse mode. 1-bit width pulse
> + indicates start of a frame)
> +  1 (L/R sync or I2S mode) (default)
> +- renesas,lsb-first: empty property indicates lsb bit is received first.
> +  When not defined msb bit is received first (default)
> +- renesas,syncac-pol-high  : empty property indicates sync signal polarity.
> +  When defined, active high or high->low sync signal.
> +  When not defined, active low or low->high sync signal
> +  (default)
> +- renesas,dtdl : delay between sync signal and start of reception.
> +  Must contain one of the following values:
> +  0   (no bit delay)
> +  50  (0.5-clock-cycle delay)
> +  100 (1-clock-cycle delay) (default)
> +  150 (1.5-clock-cycle delay)
> +  200 (2-clock-cycle delay)
> +-