[1/5] dt-bindings: media: Add Allwinner A10 CSI binding

The Allwinner A10 CMOS Sensor Interface is a camera capture interface also
used in later (A10s, A13, A20, R8 and GR8) SoCs.

On some SoCs, like the A10, there's multiple instances of that controller,
with one instance supporting more channels and having an ISP.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 Documentation/devicetree/bindings/media/sun4i-csi.txt | 71 ++++++++++++-
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/media/sun4i-csi.txt

Hi Maxime,

On Tue, Nov 13, 2018 at 09:24:13AM +0100, Maxime Ripard wrote:
> The Allwinner A10 CMOS Sensor Interface is a camera capture interface also
> used in later (A10s, A13, A20, R8 and GR8) SoCs.
> 
> On some SoCs, like the A10, there's multiple instances of that controller,
> with one instance supporting more channels and having an ISP.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  Documentation/devicetree/bindings/media/sun4i-csi.txt | 71 ++++++++++++-
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/media/sun4i-csi.txt
> 
> diff --git a/Documentation/devicetree/bindings/media/sun4i-csi.txt b/Documentation/devicetree/bindings/media/sun4i-csi.txt
> new file mode 100644
> index 000000000000..3d96bcbef9d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/media/sun4i-csi.txt
> @@ -0,0 +1,71 @@
> +Allwinner A10 CMOS Sensor Interface
> +-------------------------------------
> +
> +The Allwinner A10 SoC features two camera capture interfaces, one
> +featuring an ISP and the other without. Later SoCs built upon that
> +design and used similar SoCs.
> +
> +Required properties:
> +  - compatible: value must be one of:
> +    * allwinner,sun4i-a10-csi
> +    * allwinner,sun5i-a13-csi, allwinner,sun4i-a10-csi
> +    * allwinner,sun7i-a20-csi, allwinner,sun4i-a10-csi
> +  - reg: base address and size of the memory-mapped region.
> +  - interrupts: interrupt associated to this IP
> +  - clocks: phandles to the clocks feeding the CSI
> +    * ahb: the CSI interface clock
> +    * mod: the CSI module clock
> +    * ram: the CSI DRAM clock
> +  - clock-names: the clock names mentioned above
> +  - resets: phandles to the reset line driving the CSI
> +
> +Optional properties:
> +  - allwinner,csi-channels: Number of channels available in the CSI
> +                            controller. If not present, the default
> +                            will be 1.
> +  - allwinner,has-isp: Whether the CSI controller has an ISP
> +                       associated to it or not

Is the ISP a part of the same device? It sounds like that this is actually
a different device if it contains an ISP as well, and that should be
apparent from the compatible string. What do you think?

> +
> +If allwinner,has-isp is set, an additional "isp" clock is needed,
> +being a phandle to the clock driving the ISP.
> +
> +The CSI node should contain one 'port' child node with one child
> +'endpoint' node, according to the bindings defined in
> +Documentation/devicetree/bindings/media/video-interfaces.txt. The
> +endpoint's bus type must be parallel or BT656.
> +
> +Endpoint node properties for CSI
> +---------------------------------
> +
> +- remote-endpoint	: (required) a phandle to the bus receiver's endpoint
> +			   node

Rob's opinion has been (AFAIU) that this is not needed as it's already a
part of the graph bindings. Unless you want to say that it's required, that
is --- the graph bindings document it as optional.

> +- bus-width:		: (required) must be 8

If this is the only value the hardware supports, I don't see why you should
specify it here.

> +- pclk-sample		: (optional) (default: sample on falling edge)
> +- hsync-active		: (only required for parallel)
> +- vsync-active		: (only required for parallel)
> +
> +Example:
> +
> +csi0: csi@1c09000 {
> +	compatible = "allwinner,sun7i-a20-csi",
> +		     "allwinner,sun4i-a10-csi";
> +	reg = <0x01c09000 0x1000>;
> +	interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
> +	clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
> +		 <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
> +	clock-names = "ahb", "mod", "isp", "ram";
> +	resets = <&ccu RST_CSI0>;
> +	allwinner,csi-channels = <4>;
> +	allwinner,has-isp;
> +	
> +	port {
> +		csi_from_ov5640: endpoint {
> +                        remote-endpoint = <&ov5640_to_csi>;
> +                        bus-width = <8>;
> +                        data-shift = <2>;

data-shift needs to be documented above if it's relevant for the device.

> +                        hsync-active = <1>; /* Active high */
> +                        vsync-active = <0>; /* Active low */
> +                        pclk-sample = <1>;  /* Rising */
> +                };
> +	};
> +};

Hi Maxime,

On Tue, Nov 13, 2018 at 09:24:13AM +0100, Maxime Ripard wrote:
> The Allwinner A10 CMOS Sensor Interface is a camera capture interface also
...
> +Optional properties:
> +  - allwinner,csi-channels: Number of channels available in the CSI
> +                            controller. If not present, the default
> +                            will be 1.

Is this virtual channels or something else btw.?

Hi Sakari,

On Tue, Nov 13, 2018 at 10:38:55AM +0200, Sakari Ailus wrote:
> > +  - allwinner,has-isp: Whether the CSI controller has an ISP
> > +                       associated to it or not
> 
> Is the ISP a part of the same device? It sounds like that this is actually
> a different device if it contains an ISP as well, and that should be
> apparent from the compatible string. What do you think?

I guess we can see it as both. It seems to be the exact same register
set, except for the fact that the first instance has that ISP in
addition, and several channels, as you pointed out in your other mail.

What these channels are is not exactly clear. It looks like it's
related to the BT656 interface where you could interleave channel
bytes over the bus. I haven't really looked into it, and it doesn't
look like we have any code (or hardware) able to do that though.

> > +
> > +If allwinner,has-isp is set, an additional "isp" clock is needed,
> > +being a phandle to the clock driving the ISP.
> > +
> > +The CSI node should contain one 'port' child node with one child
> > +'endpoint' node, according to the bindings defined in
> > +Documentation/devicetree/bindings/media/video-interfaces.txt. The
> > +endpoint's bus type must be parallel or BT656.
> > +
> > +Endpoint node properties for CSI
> > +---------------------------------
> > +
> > +- remote-endpoint	: (required) a phandle to the bus receiver's endpoint
> > +			   node
> 
> Rob's opinion has been (AFAIU) that this is not needed as it's already a
> part of the graph bindings. Unless you want to say that it's required, that
> is --- the graph bindings document it as optional.

Ok, I'll remove it.

> > +- bus-width:		: (required) must be 8
> 
> If this is the only value the hardware supports, I don't see why you should
> specify it here.

Ditto :)

> > +- pclk-sample		: (optional) (default: sample on falling edge)
> > +- hsync-active		: (only required for parallel)
> > +- vsync-active		: (only required for parallel)
> > +
> > +Example:
> > +
> > +csi0: csi@1c09000 {
> > +	compatible = "allwinner,sun7i-a20-csi",
> > +		     "allwinner,sun4i-a10-csi";
> > +	reg = <0x01c09000 0x1000>;
> > +	interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
> > +	clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
> > +		 <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
> > +	clock-names = "ahb", "mod", "isp", "ram";
> > +	resets = <&ccu RST_CSI0>;
> > +	allwinner,csi-channels = <4>;
> > +	allwinner,has-isp;
> > +	
> > +	port {
> > +		csi_from_ov5640: endpoint {
> > +                        remote-endpoint = <&ov5640_to_csi>;
> > +                        bus-width = <8>;
> > +                        data-shift = <2>;
> 
> data-shift needs to be documented above if it's relevant for the device.

It's not really related to the CSI device in that case but the sensor,
so I'll just leave it out.

Thanks!
Maxime

Hi Maxime,

On Thu, Nov 15, 2018 at 08:04:24PM +0100, Maxime Ripard wrote:
> Hi Sakari,
> 
> On Tue, Nov 13, 2018 at 10:38:55AM +0200, Sakari Ailus wrote:
> > > +  - allwinner,has-isp: Whether the CSI controller has an ISP
> > > +                       associated to it or not
> > 
> > Is the ISP a part of the same device? It sounds like that this is actually
> > a different device if it contains an ISP as well, and that should be
> > apparent from the compatible string. What do you think?
> 
> I guess we can see it as both. It seems to be the exact same register
> set, except for the fact that the first instance has that ISP in
> addition, and several channels, as you pointed out in your other mail.

I'm simply referring to existing practices as far as I know them. If it's a
different device, it should have a different compatible string, not a
vendor-specific property to tell it's somehow different.

Many SoCs also separate the DMA and the CSI-2 receivers, and thus they have
separate drivers. I don't know about your case, but the ISP requiring a
different clock is a hint.

> 
> What these channels are is not exactly clear. It looks like it's
> related to the BT656 interface where you could interleave channel
> bytes over the bus. I haven't really looked into it, and it doesn't
> look like we have any code (or hardware) able to do that though.
> 
> > > +
> > > +If allwinner,has-isp is set, an additional "isp" clock is needed,
> > > +being a phandle to the clock driving the ISP.
> > > +
> > > +The CSI node should contain one 'port' child node with one child
> > > +'endpoint' node, according to the bindings defined in
> > > +Documentation/devicetree/bindings/media/video-interfaces.txt. The
> > > +endpoint's bus type must be parallel or BT656.
> > > +
> > > +Endpoint node properties for CSI
> > > +---------------------------------
> > > +
> > > +- remote-endpoint	: (required) a phandle to the bus receiver's endpoint
> > > +			   node
> > 
> > Rob's opinion has been (AFAIU) that this is not needed as it's already a
> > part of the graph bindings. Unless you want to say that it's required, that
> > is --- the graph bindings document it as optional.
> 
> Ok, I'll remove it.
> 
> > > +- bus-width:		: (required) must be 8
> > 
> > If this is the only value the hardware supports, I don't see why you should
> > specify it here.
> 
> Ditto :)
> 
> > > +- pclk-sample		: (optional) (default: sample on falling edge)
> > > +- hsync-active		: (only required for parallel)
> > > +- vsync-active		: (only required for parallel)
> > > +
> > > +Example:
> > > +
> > > +csi0: csi@1c09000 {
> > > +	compatible = "allwinner,sun7i-a20-csi",
> > > +		     "allwinner,sun4i-a10-csi";
> > > +	reg = <0x01c09000 0x1000>;
> > > +	interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
> > > +	clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
> > > +		 <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
> > > +	clock-names = "ahb", "mod", "isp", "ram";
> > > +	resets = <&ccu RST_CSI0>;
> > > +	allwinner,csi-channels = <4>;
> > > +	allwinner,has-isp;
> > > +	
> > > +	port {
> > > +		csi_from_ov5640: endpoint {
> > > +                        remote-endpoint = <&ov5640_to_csi>;
> > > +                        bus-width = <8>;
> > > +                        data-shift = <2>;
> > 
> > data-shift needs to be documented above if it's relevant for the device.
> 
> It's not really related to the CSI device in that case but the sensor,
> so I'll just leave it out.

Hmm. data-shift should only be relevant for the receiver, shoudn't it?

Hi,

On Wed, Nov 21, 2018 at 11:56:50PM +0200, Sakari Ailus wrote:
> On Thu, Nov 15, 2018 at 08:04:24PM +0100, Maxime Ripard wrote:
> > On Tue, Nov 13, 2018 at 10:38:55AM +0200, Sakari Ailus wrote:
> > > > +  - allwinner,has-isp: Whether the CSI controller has an ISP
> > > > +                       associated to it or not
> > > 
> > > Is the ISP a part of the same device? It sounds like that this is actually
> > > a different device if it contains an ISP as well, and that should be
> > > apparent from the compatible string. What do you think?
> > 
> > I guess we can see it as both. It seems to be the exact same register
> > set, except for the fact that the first instance has that ISP in
> > addition, and several channels, as you pointed out in your other mail.
> 
> I'm simply referring to existing practices as far as I know them. If it's a
> different device, it should have a different compatible string, not a
> vendor-specific property to tell it's somehow different.

The line is blurrier than that. Different devices are indeed
represented using different compatibles, but different features set
can be represented using properties.

> Many SoCs also separate the DMA and the CSI-2 receivers, and thus they have
> separate drivers. I don't know about your case, but the ISP requiring a
> different clock is a hint.

In this particular case, the datasheet represents the ISP as part of
the DMA, so it looks like a feature. And since we don't have any
source code for this, we can only do guesswork.

> > What these channels are is not exactly clear. It looks like it's
> > related to the BT656 interface where you could interleave channel
> > bytes over the bus. I haven't really looked into it, and it doesn't
> > look like we have any code (or hardware) able to do that though.
> > 
> > > > +
> > > > +If allwinner,has-isp is set, an additional "isp" clock is needed,
> > > > +being a phandle to the clock driving the ISP.
> > > > +
> > > > +The CSI node should contain one 'port' child node with one child
> > > > +'endpoint' node, according to the bindings defined in
> > > > +Documentation/devicetree/bindings/media/video-interfaces.txt. The
> > > > +endpoint's bus type must be parallel or BT656.
> > > > +
> > > > +Endpoint node properties for CSI
> > > > +---------------------------------
> > > > +
> > > > +- remote-endpoint	: (required) a phandle to the bus receiver's endpoint
> > > > +			   node
> > > 
> > > Rob's opinion has been (AFAIU) that this is not needed as it's already a
> > > part of the graph bindings. Unless you want to say that it's required, that
> > > is --- the graph bindings document it as optional.
> > 
> > Ok, I'll remove it.
> > 
> > > > +- bus-width:		: (required) must be 8
> > > 
> > > If this is the only value the hardware supports, I don't see why you should
> > > specify it here.
> > 
> > Ditto :)
> > 
> > > > +- pclk-sample		: (optional) (default: sample on falling edge)
> > > > +- hsync-active		: (only required for parallel)
> > > > +- vsync-active		: (only required for parallel)
> > > > +
> > > > +Example:
> > > > +
> > > > +csi0: csi@1c09000 {
> > > > +	compatible = "allwinner,sun7i-a20-csi",
> > > > +		     "allwinner,sun4i-a10-csi";
> > > > +	reg = <0x01c09000 0x1000>;
> > > > +	interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
> > > > +	clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI0>,
> > > > +		 <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>;
> > > > +	clock-names = "ahb", "mod", "isp", "ram";
> > > > +	resets = <&ccu RST_CSI0>;
> > > > +	allwinner,csi-channels = <4>;
> > > > +	allwinner,has-isp;
> > > > +	
> > > > +	port {
> > > > +		csi_from_ov5640: endpoint {
> > > > +                        remote-endpoint = <&ov5640_to_csi>;
> > > > +                        bus-width = <8>;
> > > > +                        data-shift = <2>;
> > > 
> > > data-shift needs to be documented above if it's relevant for the device.
> > 
> > It's not really related to the CSI device in that case but the sensor,
> > so I'll just leave it out.
> 
> Hmm. data-shift should only be relevant for the receiver, shoudn't it?

Sorry, I mispoke. We're not using it anywhere in either drivers, so
it's totally useless.

Maxime