[PATCH v2 1/3] dt-bindings: media: sun6i: Add A64 CSI compatible (w/ H3 fallback)

[PATCH v2 1/3] dt-bindings: media: sun6i: Add A64 CSI compatible (w/ H3 fallback)

[Jagan Teki]

Allwinner A64 CSI has single channel time-multiplexed BT.656
CMOS sensor interface like H3.

Add a compatible string for it with H3 fallback compatible string,
in this case the H3 driver can be used.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v2:
- none

 Documentation/devicetree/bindings/media/sun6i-csi.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/media/sun6i-csi.txt b/Documentation/devicetree/bindings/media/sun6i-csi.txt
index eb5a14b828d9..5fb6fd4e2c7d 100644
--- a/Documentation/devicetree/bindings/media/sun6i-csi.txt
+++ b/Documentation/devicetree/bindings/media/sun6i-csi.txt
@@ -7,6 +7,7 @@ Required properties:
   - compatible: value must be one of:
     * "allwinner,sun6i-a31-csi"
     * "allwinner,sun8i-h3-csi"
+    * "allwinner,sun50i-a64-csi", "allwinner,sun8i-h3-csi"
     * "allwinner,sun8i-v3s-csi"
   - reg: base address and size of the memory-mapped region.
   - interrupts: interrupt associated to this IP
-- 
2.18.0.321.gffc6fa0e3

[PATCH v2 1/3] dt-bindings: media: sun6i: Add A64 CSI compatible (w/ H3 fallback)

[Jagan Teki]

Allwinner A64 CSI has single channel time-multiplexed BT.656
CMOS sensor interface like H3.

Add a compatible string for it with H3 fallback compatible string,
in this case the H3 driver can be used.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v2:
- none

 Documentation/devicetree/bindings/media/sun6i-csi.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/media/sun6i-csi.txt b/Documentation/devicetree/bindings/media/sun6i-csi.txt
index eb5a14b828d9..5fb6fd4e2c7d 100644
--- a/Documentation/devicetree/bindings/media/sun6i-csi.txt
+++ b/Documentation/devicetree/bindings/media/sun6i-csi.txt
@@ -7,6 +7,7 @@ Required properties:
   - compatible: value must be one of:
     * "allwinner,sun6i-a31-csi"
     * "allwinner,sun8i-h3-csi"
+    * "allwinner,sun50i-a64-csi", "allwinner,sun8i-h3-csi"
     * "allwinner,sun8i-v3s-csi"
   - reg: base address and size of the memory-mapped region.
   - interrupts: interrupt associated to this IP
-- 
2.18.0.321.gffc6fa0e3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Jagan Teki]

Allwinner A64 CSI controller has similar features as like in
H3, So add support for A64 via H3 fallback.

Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
the default clock 600MHz seems unable to drive the sensor(ov5640)
to capture the image.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v2:
- Use CSI_SCLK to 300MHz 

 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 384c417cb7a2..d7ab0006ebce 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -532,6 +532,12 @@
 			interrupt-controller;
 			#interrupt-cells = <3>;
 
+			csi_pins: csi-pins {
+				pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
+				       "PE7", "PE8", "PE9", "PE10", "PE11";
+				function = "csi0";
+			};
+
 			i2c0_pins: i2c0_pins {
 				pins = "PH0", "PH1";
 				function = "i2c0";
@@ -899,6 +905,23 @@
 			status = "disabled";
 		};
 
+		csi: csi@1cb0000 {
+			compatible = "allwinner,sun50i-a64-csi",
+				     "allwinner,sun8i-h3-csi";
+			reg = <0x01cb0000 0x1000>;
+			interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CSI>,
+				 <&ccu CLK_CSI_SCLK>,
+				 <&ccu CLK_DRAM_CSI>;
+			clock-names = "bus", "mod", "ram";
+			resets = <&ccu RST_BUS_CSI>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&csi_pins>;
+			assigned-clocks = <&ccu CLK_CSI_SCLK>;
+			assigned-clock-rates = <300000000>;
+			status = "disabled";
+		};
+
 		hdmi: hdmi@1ee0000 {
 			compatible = "allwinner,sun50i-a64-dw-hdmi",
 				     "allwinner,sun8i-a83t-dw-hdmi";
-- 
2.18.0.321.gffc6fa0e3

[PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Jagan Teki]

Allwinner A64 CSI controller has similar features as like in
H3, So add support for A64 via H3 fallback.

Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
the default clock 600MHz seems unable to drive the sensor(ov5640)
to capture the image.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v2:
- Use CSI_SCLK to 300MHz 

 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 384c417cb7a2..d7ab0006ebce 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -532,6 +532,12 @@
 			interrupt-controller;
 			#interrupt-cells = <3>;
 
+			csi_pins: csi-pins {
+				pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
+				       "PE7", "PE8", "PE9", "PE10", "PE11";
+				function = "csi0";
+			};
+
 			i2c0_pins: i2c0_pins {
 				pins = "PH0", "PH1";
 				function = "i2c0";
@@ -899,6 +905,23 @@
 			status = "disabled";
 		};
 
+		csi: csi@1cb0000 {
+			compatible = "allwinner,sun50i-a64-csi",
+				     "allwinner,sun8i-h3-csi";
+			reg = <0x01cb0000 0x1000>;
+			interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_CSI>,
+				 <&ccu CLK_CSI_SCLK>,
+				 <&ccu CLK_DRAM_CSI>;
+			clock-names = "bus", "mod", "ram";
+			resets = <&ccu RST_BUS_CSI>;
+			pinctrl-names = "default";
+			pinctrl-0 = <&csi_pins>;
+			assigned-clocks = <&ccu CLK_CSI_SCLK>;
+			assigned-clock-rates = <300000000>;
+			status = "disabled";
+		};
+
 		hdmi: hdmi@1ee0000 {
 			compatible = "allwinner,sun50i-a64-dw-hdmi",
 				     "allwinner,sun8i-a83t-dw-hdmi";
-- 
2.18.0.321.gffc6fa0e3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/3] arm64: dts: allwinner: a64: Add pinmux setting for CSI MCLK on PE1

[Jagan Teki]

Some camera modules have the SoC feeding a master clock to the sensor
instead of having a standalone crystal. This clock signal is generated
from the clock control unit and output from the CSI MCLK function of
pin PE1.

Add a pinmux setting for it for camera sensors to reference.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v2:
- new patch

 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index d7ab0006ebce..902b5238f1dd 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -538,6 +538,11 @@
 				function = "csi0";
 			};
 
+			csi_mclk_pin: csi-mclk {
+				pins = "PE1";
+				function = "csi0";
+			};
+
 			i2c0_pins: i2c0_pins {
 				pins = "PH0", "PH1";
 				function = "i2c0";
-- 
2.18.0.321.gffc6fa0e3

[PATCH v2 3/3] arm64: dts: allwinner: a64: Add pinmux setting for CSI MCLK on PE1

[Jagan Teki]

Some camera modules have the SoC feeding a master clock to the sensor
instead of having a standalone crystal. This clock signal is generated
from the clock control unit and output from the CSI MCLK function of
pin PE1.

Add a pinmux setting for it for camera sensors to reference.

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
Changes for v2:
- new patch

 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index d7ab0006ebce..902b5238f1dd 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -538,6 +538,11 @@
 				function = "csi0";
 			};
 
+			csi_mclk_pin: csi-mclk {
+				pins = "PE1";
+				function = "csi0";
+			};
+
 			i2c0_pins: i2c0_pins {
 				pins = "PH0", "PH1";
 				function = "i2c0";
-- 
2.18.0.321.gffc6fa0e3


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1928 bytes --]

On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> Allwinner A64 CSI controller has similar features as like in
> H3, So add support for A64 via H3 fallback.
> 
> Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> the default clock 600MHz seems unable to drive the sensor(ov5640)
> to capture the image.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v2:
> - Use CSI_SCLK to 300MHz 
> 
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 384c417cb7a2..d7ab0006ebce 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -532,6 +532,12 @@
>  			interrupt-controller;
>  			#interrupt-cells = <3>;
>  
> +			csi_pins: csi-pins {
> +				pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> +				       "PE7", "PE8", "PE9", "PE10", "PE11";
> +				function = "csi0";
> +			};
> +
>  			i2c0_pins: i2c0_pins {
>  				pins = "PH0", "PH1";
>  				function = "i2c0";
> @@ -899,6 +905,23 @@
>  			status = "disabled";
>  		};
>  
> +		csi: csi@1cb0000 {
> +			compatible = "allwinner,sun50i-a64-csi",
> +				     "allwinner,sun8i-h3-csi";
> +			reg = <0x01cb0000 0x1000>;
> +			interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ccu CLK_BUS_CSI>,
> +				 <&ccu CLK_CSI_SCLK>,
> +				 <&ccu CLK_DRAM_CSI>;
> +			clock-names = "bus", "mod", "ram";
> +			resets = <&ccu RST_BUS_CSI>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&csi_pins>;
> +			assigned-clocks = <&ccu CLK_CSI_SCLK>;
> +			assigned-clock-rates = <300000000>;

That should be enforced in the driver.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1928 bytes --]

On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> Allwinner A64 CSI controller has similar features as like in
> H3, So add support for A64 via H3 fallback.
> 
> Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> the default clock 600MHz seems unable to drive the sensor(ov5640)
> to capture the image.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v2:
> - Use CSI_SCLK to 300MHz 
> 
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 384c417cb7a2..d7ab0006ebce 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -532,6 +532,12 @@
>  			interrupt-controller;
>  			#interrupt-cells = <3>;
>  
> +			csi_pins: csi-pins {
> +				pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> +				       "PE7", "PE8", "PE9", "PE10", "PE11";
> +				function = "csi0";
> +			};
> +
>  			i2c0_pins: i2c0_pins {
>  				pins = "PH0", "PH1";
>  				function = "i2c0";
> @@ -899,6 +905,23 @@
>  			status = "disabled";
>  		};
>  
> +		csi: csi@1cb0000 {
> +			compatible = "allwinner,sun50i-a64-csi",
> +				     "allwinner,sun8i-h3-csi";
> +			reg = <0x01cb0000 0x1000>;
> +			interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&ccu CLK_BUS_CSI>,
> +				 <&ccu CLK_CSI_SCLK>,
> +				 <&ccu CLK_DRAM_CSI>;
> +			clock-names = "bus", "mod", "ram";
> +			resets = <&ccu RST_BUS_CSI>;
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&csi_pins>;
> +			assigned-clocks = <&ccu CLK_CSI_SCLK>;
> +			assigned-clock-rates = <300000000>;

That should be enforced in the driver.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] arm64: dts: allwinner: a64: Add pinmux setting for CSI MCLK on PE1

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

On Thu, Dec 06, 2018 at 06:53:06PM +0530, Jagan Teki wrote:
> Some camera modules have the SoC feeding a master clock to the sensor
> instead of having a standalone crystal. This clock signal is generated
> from the clock control unit and output from the CSI MCLK function of
> pin PE1.
> 
> Add a pinmux setting for it for camera sensors to reference.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v2:
> - new patch
> 
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index d7ab0006ebce..902b5238f1dd 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -538,6 +538,11 @@
>  				function = "csi0";
>  			};
>  
> +			csi_mclk_pin: csi-mclk {
> +				pins = "PE1";
> +				function = "csi0";
> +			};
> +

We're not merging nodes that have no users.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/3] arm64: dts: allwinner: a64: Add pinmux setting for CSI MCLK on PE1

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1140 bytes --]

On Thu, Dec 06, 2018 at 06:53:06PM +0530, Jagan Teki wrote:
> Some camera modules have the SoC feeding a master clock to the sensor
> instead of having a standalone crystal. This clock signal is generated
> from the clock control unit and output from the CSI MCLK function of
> pin PE1.
> 
> Add a pinmux setting for it for camera sensors to reference.
> 
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
> Changes for v2:
> - new patch
> 
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index d7ab0006ebce..902b5238f1dd 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -538,6 +538,11 @@
>  				function = "csi0";
>  			};
>  
> +			csi_mclk_pin: csi-mclk {
> +				pins = "PE1";
> +				function = "csi0";
> +			};
> +

We're not merging nodes that have no users.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] arm64: dts: allwinner: a64: Add pinmux setting for CSI MCLK on PE1

[Jagan Teki]

On Thu, Dec 6, 2018 at 9:05 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Dec 06, 2018 at 06:53:06PM +0530, Jagan Teki wrote:
> > Some camera modules have the SoC feeding a master clock to the sensor
> > instead of having a standalone crystal. This clock signal is generated
> > from the clock control unit and output from the CSI MCLK function of
> > pin PE1.
> >
> > Add a pinmux setting for it for camera sensors to reference.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Changes for v2:
> > - new patch
> >
> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > index d7ab0006ebce..902b5238f1dd 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > @@ -538,6 +538,11 @@
> >                               function = "csi0";
> >                       };
> >
> > +                     csi_mclk_pin: csi-mclk {
> > +                             pins = "PE1";
> > +                             function = "csi0";
> > +                     };
> > +
>
> We're not merging nodes that have no users.

Yes, v1 [1] has a consumer for this. Since it's under discussion about
PE group supply, opendrain I hold it. will send once the discussion
done, I even tested on top-of your ov5640 changes.

[1] https://patchwork.kernel.org/patch/10709077/

Re: [PATCH v2 3/3] arm64: dts: allwinner: a64: Add pinmux setting for CSI MCLK on PE1

[Jagan Teki]

On Thu, Dec 6, 2018 at 9:05 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Dec 06, 2018 at 06:53:06PM +0530, Jagan Teki wrote:
> > Some camera modules have the SoC feeding a master clock to the sensor
> > instead of having a standalone crystal. This clock signal is generated
> > from the clock control unit and output from the CSI MCLK function of
> > pin PE1.
> >
> > Add a pinmux setting for it for camera sensors to reference.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Changes for v2:
> > - new patch
> >
> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > index d7ab0006ebce..902b5238f1dd 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > @@ -538,6 +538,11 @@
> >                               function = "csi0";
> >                       };
> >
> > +                     csi_mclk_pin: csi-mclk {
> > +                             pins = "PE1";
> > +                             function = "csi0";
> > +                     };
> > +
>
> We're not merging nodes that have no users.

Yes, v1 [1] has a consumer for this. Since it's under discussion about
PE group supply, opendrain I hold it. will send once the discussion
done, I even tested on top-of your ov5640 changes.

[1] https://patchwork.kernel.org/patch/10709077/

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Michael Nazzareno Trimarchi]

Hi Maxime

On Thu, Dec 6, 2018 at 4:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> > Allwinner A64 CSI controller has similar features as like in
> > H3, So add support for A64 via H3 fallback.
> >
> > Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> > the default clock 600MHz seems unable to drive the sensor(ov5640)
> > to capture the image.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Changes for v2:
> > - Use CSI_SCLK to 300MHz
> >
> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > index 384c417cb7a2..d7ab0006ebce 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > @@ -532,6 +532,12 @@
> >                       interrupt-controller;
> >                       #interrupt-cells = <3>;
> >
> > +                     csi_pins: csi-pins {
> > +                             pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> > +                                    "PE7", "PE8", "PE9", "PE10", "PE11";
> > +                             function = "csi0";
> > +                     };
> > +
> >                       i2c0_pins: i2c0_pins {
> >                               pins = "PH0", "PH1";
> >                               function = "i2c0";
> > @@ -899,6 +905,23 @@
> >                       status = "disabled";
> >               };
> >
> > +             csi: csi@1cb0000 {
> > +                     compatible = "allwinner,sun50i-a64-csi",
> > +                                  "allwinner,sun8i-h3-csi";
> > +                     reg = <0x01cb0000 0x1000>;
> > +                     interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> > +                     clocks = <&ccu CLK_BUS_CSI>,
> > +                              <&ccu CLK_CSI_SCLK>,
> > +                              <&ccu CLK_DRAM_CSI>;
> > +                     clock-names = "bus", "mod", "ram";
> > +                     resets = <&ccu RST_BUS_CSI>;
> > +                     pinctrl-names = "default";
> > +                     pinctrl-0 = <&csi_pins>;
> > +                     assigned-clocks = <&ccu CLK_CSI_SCLK>;
> > +                     assigned-clock-rates = <300000000>;
>
> That should be enforced in the driver.
>

We are not really sure what is the best here. Our first idea was to
put in the board file and then Jagan
decide to put in dtsi. We don't have enough coverage of camera on this
CPU and I prefer to stay with this
minimal change that does not impact the driver.

Michael

> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Michael Nazzareno Trimarchi]

Hi Maxime

On Thu, Dec 6, 2018 at 4:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> > Allwinner A64 CSI controller has similar features as like in
> > H3, So add support for A64 via H3 fallback.
> >
> > Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> > the default clock 600MHz seems unable to drive the sensor(ov5640)
> > to capture the image.
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> > Changes for v2:
> > - Use CSI_SCLK to 300MHz
> >
> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > index 384c417cb7a2..d7ab0006ebce 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > @@ -532,6 +532,12 @@
> >                       interrupt-controller;
> >                       #interrupt-cells = <3>;
> >
> > +                     csi_pins: csi-pins {
> > +                             pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> > +                                    "PE7", "PE8", "PE9", "PE10", "PE11";
> > +                             function = "csi0";
> > +                     };
> > +
> >                       i2c0_pins: i2c0_pins {
> >                               pins = "PH0", "PH1";
> >                               function = "i2c0";
> > @@ -899,6 +905,23 @@
> >                       status = "disabled";
> >               };
> >
> > +             csi: csi@1cb0000 {
> > +                     compatible = "allwinner,sun50i-a64-csi",
> > +                                  "allwinner,sun8i-h3-csi";
> > +                     reg = <0x01cb0000 0x1000>;
> > +                     interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> > +                     clocks = <&ccu CLK_BUS_CSI>,
> > +                              <&ccu CLK_CSI_SCLK>,
> > +                              <&ccu CLK_DRAM_CSI>;
> > +                     clock-names = "bus", "mod", "ram";
> > +                     resets = <&ccu RST_BUS_CSI>;
> > +                     pinctrl-names = "default";
> > +                     pinctrl-0 = <&csi_pins>;
> > +                     assigned-clocks = <&ccu CLK_CSI_SCLK>;
> > +                     assigned-clock-rates = <300000000>;
>
> That should be enforced in the driver.
>

We are not really sure what is the best here. Our first idea was to
put in the board file and then Jagan
decide to put in dtsi. We don't have enough coverage of camera on this
CPU and I prefer to stay with this
minimal change that does not impact the driver.

Michael

> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3748 bytes --]

On Thu, Dec 06, 2018 at 06:07:59PM +0100, Michael Nazzareno Trimarchi wrote:
> On Thu, Dec 6, 2018 at 4:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> > > Allwinner A64 CSI controller has similar features as like in
> > > H3, So add support for A64 via H3 fallback.
> > >
> > > Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> > > the default clock 600MHz seems unable to drive the sensor(ov5640)
> > > to capture the image.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > > Changes for v2:
> > > - Use CSI_SCLK to 300MHz
> > >
> > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > index 384c417cb7a2..d7ab0006ebce 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > @@ -532,6 +532,12 @@
> > >                       interrupt-controller;
> > >                       #interrupt-cells = <3>;
> > >
> > > +                     csi_pins: csi-pins {
> > > +                             pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> > > +                                    "PE7", "PE8", "PE9", "PE10", "PE11";
> > > +                             function = "csi0";
> > > +                     };
> > > +
> > >                       i2c0_pins: i2c0_pins {
> > >                               pins = "PH0", "PH1";
> > >                               function = "i2c0";
> > > @@ -899,6 +905,23 @@
> > >                       status = "disabled";
> > >               };
> > >
> > > +             csi: csi@1cb0000 {
> > > +                     compatible = "allwinner,sun50i-a64-csi",
> > > +                                  "allwinner,sun8i-h3-csi";
> > > +                     reg = <0x01cb0000 0x1000>;
> > > +                     interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> > > +                     clocks = <&ccu CLK_BUS_CSI>,
> > > +                              <&ccu CLK_CSI_SCLK>,
> > > +                              <&ccu CLK_DRAM_CSI>;
> > > +                     clock-names = "bus", "mod", "ram";
> > > +                     resets = <&ccu RST_BUS_CSI>;
> > > +                     pinctrl-names = "default";
> > > +                     pinctrl-0 = <&csi_pins>;
> > > +                     assigned-clocks = <&ccu CLK_CSI_SCLK>;
> > > +                     assigned-clock-rates = <300000000>;
> >
> > That should be enforced in the driver.
> >
> 
> We are not really sure what is the best here. Our first idea was to
> put in the board file and then Jagan
> decide to put in dtsi. We don't have enough coverage of camera on this
> CPU and I prefer to stay with this
> minimal change that does not impact the driver.

The thing is that:
  - in this commit log, you're stating that it depends on the sensor,
    which indicates that this would be a board level addition
  - In another patch series, Jagan reported IIRC that it actually
    depends on the resolution, so it doesn't belong in the DT at all
  - And then, you don't even have any guarantee on the clock rate. The
    sole guarantee you have is that when your driver will probe, the
    rate will be close to those 300MHz. That's it. It might completely
    change after the driver has probed, or be rounded to something
    else entirely, who knows.

So really, putting it in the DT is nothing but a hack.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 3748 bytes --]

On Thu, Dec 06, 2018 at 06:07:59PM +0100, Michael Nazzareno Trimarchi wrote:
> On Thu, Dec 6, 2018 at 4:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> > > Allwinner A64 CSI controller has similar features as like in
> > > H3, So add support for A64 via H3 fallback.
> > >
> > > Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> > > the default clock 600MHz seems unable to drive the sensor(ov5640)
> > > to capture the image.
> > >
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > > Changes for v2:
> > > - Use CSI_SCLK to 300MHz
> > >
> > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > index 384c417cb7a2..d7ab0006ebce 100644
> > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > @@ -532,6 +532,12 @@
> > >                       interrupt-controller;
> > >                       #interrupt-cells = <3>;
> > >
> > > +                     csi_pins: csi-pins {
> > > +                             pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> > > +                                    "PE7", "PE8", "PE9", "PE10", "PE11";
> > > +                             function = "csi0";
> > > +                     };
> > > +
> > >                       i2c0_pins: i2c0_pins {
> > >                               pins = "PH0", "PH1";
> > >                               function = "i2c0";
> > > @@ -899,6 +905,23 @@
> > >                       status = "disabled";
> > >               };
> > >
> > > +             csi: csi@1cb0000 {
> > > +                     compatible = "allwinner,sun50i-a64-csi",
> > > +                                  "allwinner,sun8i-h3-csi";
> > > +                     reg = <0x01cb0000 0x1000>;
> > > +                     interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> > > +                     clocks = <&ccu CLK_BUS_CSI>,
> > > +                              <&ccu CLK_CSI_SCLK>,
> > > +                              <&ccu CLK_DRAM_CSI>;
> > > +                     clock-names = "bus", "mod", "ram";
> > > +                     resets = <&ccu RST_BUS_CSI>;
> > > +                     pinctrl-names = "default";
> > > +                     pinctrl-0 = <&csi_pins>;
> > > +                     assigned-clocks = <&ccu CLK_CSI_SCLK>;
> > > +                     assigned-clock-rates = <300000000>;
> >
> > That should be enforced in the driver.
> >
> 
> We are not really sure what is the best here. Our first idea was to
> put in the board file and then Jagan
> decide to put in dtsi. We don't have enough coverage of camera on this
> CPU and I prefer to stay with this
> minimal change that does not impact the driver.

The thing is that:
  - in this commit log, you're stating that it depends on the sensor,
    which indicates that this would be a board level addition
  - In another patch series, Jagan reported IIRC that it actually
    depends on the resolution, so it doesn't belong in the DT at all
  - And then, you don't even have any guarantee on the clock rate. The
    sole guarantee you have is that when your driver will probe, the
    rate will be close to those 300MHz. That's it. It might completely
    change after the driver has probed, or be rounded to something
    else entirely, who knows.

So really, putting it in the DT is nothing but a hack.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Michael Nazzareno Trimarchi]

Hi Maxime

On Fri, Dec 7, 2018 at 8:47 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Dec 06, 2018 at 06:07:59PM +0100, Michael Nazzareno Trimarchi wrote:
> > On Thu, Dec 6, 2018 at 4:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> > > > Allwinner A64 CSI controller has similar features as like in
> > > > H3, So add support for A64 via H3 fallback.
> > > >
> > > > Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> > > > the default clock 600MHz seems unable to drive the sensor(ov5640)
> > > > to capture the image.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > > Changes for v2:
> > > > - Use CSI_SCLK to 300MHz
> > > >
> > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > index 384c417cb7a2..d7ab0006ebce 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > @@ -532,6 +532,12 @@
> > > >                       interrupt-controller;
> > > >                       #interrupt-cells = <3>;
> > > >
> > > > +                     csi_pins: csi-pins {
> > > > +                             pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> > > > +                                    "PE7", "PE8", "PE9", "PE10", "PE11";
> > > > +                             function = "csi0";
> > > > +                     };
> > > > +
> > > >                       i2c0_pins: i2c0_pins {
> > > >                               pins = "PH0", "PH1";
> > > >                               function = "i2c0";
> > > > @@ -899,6 +905,23 @@
> > > >                       status = "disabled";
> > > >               };
> > > >
> > > > +             csi: csi@1cb0000 {
> > > > +                     compatible = "allwinner,sun50i-a64-csi",
> > > > +                                  "allwinner,sun8i-h3-csi";
> > > > +                     reg = <0x01cb0000 0x1000>;
> > > > +                     interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                     clocks = <&ccu CLK_BUS_CSI>,
> > > > +                              <&ccu CLK_CSI_SCLK>,
> > > > +                              <&ccu CLK_DRAM_CSI>;
> > > > +                     clock-names = "bus", "mod", "ram";
> > > > +                     resets = <&ccu RST_BUS_CSI>;
> > > > +                     pinctrl-names = "default";
> > > > +                     pinctrl-0 = <&csi_pins>;
> > > > +                     assigned-clocks = <&ccu CLK_CSI_SCLK>;
> > > > +                     assigned-clock-rates = <300000000>;
> > >
> > > That should be enforced in the driver.
> > >
> >
> > We are not really sure what is the best here. Our first idea was to
> > put in the board file and then Jagan
> > decide to put in dtsi. We don't have enough coverage of camera on this
> > CPU and I prefer to stay with this
> > minimal change that does not impact the driver.
>
> The thing is that:
>   - in this commit log, you're stating that it depends on the sensor,
>     which indicates that this would be a board level addition
>   - In another patch series, Jagan reported IIRC that it actually
>     depends on the resolution, so it doesn't belong in the DT at all
>   - And then, you don't even have any guarantee on the clock rate. The
>     sole guarantee you have is that when your driver will probe, the
>     rate will be close to those 300MHz. That's it. It might completely
>     change after the driver has probed, or be rounded to something
>     else entirely, who knows.

I'm not so interested in the story but it's clear what you ask but in
short having one
sensor up to 5M pixel we can be sure where the reason is but make it
more pratical.

We have a parent clock that is the peripheral clock on clock
tree that run at 600Mhz. With a clock divider of 0 the driver work but
the acquisition
as problem on quality. Now the same sensor seems to work when the
logic is clocked
as half of the speed. If I remind in the right way the divider can be
only possible so
you can get pclk / (n + 1) without reparent to a different input.
Now I don't find any nice documentation that state that 600Mhz
is not supported but anyway, you suggest to do the same that is done
in fimc driver so
set there the rate there and offcourse x platform change like:

#define A64_CSI_FREQUENCY 600

ret = clk_set_rate(csk_core_clk, A64_CSI_FREQUENCY);

>
> So really, putting it in the DT is nothing but a hack.
>

Michael


> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Michael Nazzareno Trimarchi]

Hi Maxime

On Fri, Dec 7, 2018 at 8:47 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Dec 06, 2018 at 06:07:59PM +0100, Michael Nazzareno Trimarchi wrote:
> > On Thu, Dec 6, 2018 at 4:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> > > > Allwinner A64 CSI controller has similar features as like in
> > > > H3, So add support for A64 via H3 fallback.
> > > >
> > > > Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> > > > the default clock 600MHz seems unable to drive the sensor(ov5640)
> > > > to capture the image.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > > Changes for v2:
> > > > - Use CSI_SCLK to 300MHz
> > > >
> > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > index 384c417cb7a2..d7ab0006ebce 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > @@ -532,6 +532,12 @@
> > > >                       interrupt-controller;
> > > >                       #interrupt-cells = <3>;
> > > >
> > > > +                     csi_pins: csi-pins {
> > > > +                             pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> > > > +                                    "PE7", "PE8", "PE9", "PE10", "PE11";
> > > > +                             function = "csi0";
> > > > +                     };
> > > > +
> > > >                       i2c0_pins: i2c0_pins {
> > > >                               pins = "PH0", "PH1";
> > > >                               function = "i2c0";
> > > > @@ -899,6 +905,23 @@
> > > >                       status = "disabled";
> > > >               };
> > > >
> > > > +             csi: csi@1cb0000 {
> > > > +                     compatible = "allwinner,sun50i-a64-csi",
> > > > +                                  "allwinner,sun8i-h3-csi";
> > > > +                     reg = <0x01cb0000 0x1000>;
> > > > +                     interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                     clocks = <&ccu CLK_BUS_CSI>,
> > > > +                              <&ccu CLK_CSI_SCLK>,
> > > > +                              <&ccu CLK_DRAM_CSI>;
> > > > +                     clock-names = "bus", "mod", "ram";
> > > > +                     resets = <&ccu RST_BUS_CSI>;
> > > > +                     pinctrl-names = "default";
> > > > +                     pinctrl-0 = <&csi_pins>;
> > > > +                     assigned-clocks = <&ccu CLK_CSI_SCLK>;
> > > > +                     assigned-clock-rates = <300000000>;
> > >
> > > That should be enforced in the driver.
> > >
> >
> > We are not really sure what is the best here. Our first idea was to
> > put in the board file and then Jagan
> > decide to put in dtsi. We don't have enough coverage of camera on this
> > CPU and I prefer to stay with this
> > minimal change that does not impact the driver.
>
> The thing is that:
>   - in this commit log, you're stating that it depends on the sensor,
>     which indicates that this would be a board level addition
>   - In another patch series, Jagan reported IIRC that it actually
>     depends on the resolution, so it doesn't belong in the DT at all
>   - And then, you don't even have any guarantee on the clock rate. The
>     sole guarantee you have is that when your driver will probe, the
>     rate will be close to those 300MHz. That's it. It might completely
>     change after the driver has probed, or be rounded to something
>     else entirely, who knows.

I'm not so interested in the story but it's clear what you ask but in
short having one
sensor up to 5M pixel we can be sure where the reason is but make it
more pratical.

We have a parent clock that is the peripheral clock on clock
tree that run at 600Mhz. With a clock divider of 0 the driver work but
the acquisition
as problem on quality. Now the same sensor seems to work when the
logic is clocked
as half of the speed. If I remind in the right way the divider can be
only possible so
you can get pclk / (n + 1) without reparent to a different input.
Now I don't find any nice documentation that state that 600Mhz
is not supported but anyway, you suggest to do the same that is done
in fimc driver so
set there the rate there and offcourse x platform change like:

#define A64_CSI_FREQUENCY 600

ret = clk_set_rate(csk_core_clk, A64_CSI_FREQUENCY);

>
> So really, putting it in the DT is nothing but a hack.
>

Michael


> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Jagan Teki]

On Fri, Dec 7, 2018 at 1:17 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Dec 06, 2018 at 06:07:59PM +0100, Michael Nazzareno Trimarchi wrote:
> > On Thu, Dec 6, 2018 at 4:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> > > > Allwinner A64 CSI controller has similar features as like in
> > > > H3, So add support for A64 via H3 fallback.
> > > >
> > > > Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> > > > the default clock 600MHz seems unable to drive the sensor(ov5640)
> > > > to capture the image.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > > Changes for v2:
> > > > - Use CSI_SCLK to 300MHz
> > > >
> > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > index 384c417cb7a2..d7ab0006ebce 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > @@ -532,6 +532,12 @@
> > > >                       interrupt-controller;
> > > >                       #interrupt-cells = <3>;
> > > >
> > > > +                     csi_pins: csi-pins {
> > > > +                             pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> > > > +                                    "PE7", "PE8", "PE9", "PE10", "PE11";
> > > > +                             function = "csi0";
> > > > +                     };
> > > > +
> > > >                       i2c0_pins: i2c0_pins {
> > > >                               pins = "PH0", "PH1";
> > > >                               function = "i2c0";
> > > > @@ -899,6 +905,23 @@
> > > >                       status = "disabled";
> > > >               };
> > > >
> > > > +             csi: csi@1cb0000 {
> > > > +                     compatible = "allwinner,sun50i-a64-csi",
> > > > +                                  "allwinner,sun8i-h3-csi";
> > > > +                     reg = <0x01cb0000 0x1000>;
> > > > +                     interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                     clocks = <&ccu CLK_BUS_CSI>,
> > > > +                              <&ccu CLK_CSI_SCLK>,
> > > > +                              <&ccu CLK_DRAM_CSI>;
> > > > +                     clock-names = "bus", "mod", "ram";
> > > > +                     resets = <&ccu RST_BUS_CSI>;
> > > > +                     pinctrl-names = "default";
> > > > +                     pinctrl-0 = <&csi_pins>;
> > > > +                     assigned-clocks = <&ccu CLK_CSI_SCLK>;
> > > > +                     assigned-clock-rates = <300000000>;
> > >
> > > That should be enforced in the driver.
> > >
> >
> > We are not really sure what is the best here. Our first idea was to
> > put in the board file and then Jagan
> > decide to put in dtsi. We don't have enough coverage of camera on this
> > CPU and I prefer to stay with this
> > minimal change that does not impact the driver.
>
> The thing is that:
>   - in this commit log, you're stating that it depends on the sensor,
>     which indicates that this would be a board level addition
>   - In another patch series, Jagan reported IIRC that it actually
>     depends on the resolution, so it doesn't belong in the DT at all
>   - And then, you don't even have any guarantee on the clock rate. The
>     sole guarantee you have is that when your driver will probe, the
>     rate will be close to those 300MHz. That's it. It might completely
>     change after the driver has probed, or be rounded to something
>     else entirely, who knows.

Let's to be clear.

- with default clock(600MHz) the sensor get probed but image capture
has an issue.
- with 300MHz the image capture working with 320x240@30, 640x480@30,
640x480@60, 1280x720@30 with UYVY8_2X8 and YUYV8_2X8 formats but
1080p@30 seems broken (the same I have mentioned in another mail)
- since all this is verified on ov5640, I have mentioned the same
thing on commit message for future reference.

Re: [PATCH v2 2/3] arm64: dts: allwinner: a64: Add A64 CSI controller

[Jagan Teki]

On Fri, Dec 7, 2018 at 1:17 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
>
> On Thu, Dec 06, 2018 at 06:07:59PM +0100, Michael Nazzareno Trimarchi wrote:
> > On Thu, Dec 6, 2018 at 4:34 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote:
> > > On Thu, Dec 06, 2018 at 06:53:05PM +0530, Jagan Teki wrote:
> > > > Allwinner A64 CSI controller has similar features as like in
> > > > H3, So add support for A64 via H3 fallback.
> > > >
> > > > Also updated CSI_SCLK to use 300MHz via assigned-clocks, since
> > > > the default clock 600MHz seems unable to drive the sensor(ov5640)
> > > > to capture the image.
> > > >
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > > Changes for v2:
> > > > - Use CSI_SCLK to 300MHz
> > > >
> > > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++++++++++++++++++
> > > >  1 file changed, 23 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > index 384c417cb7a2..d7ab0006ebce 100644
> > > > --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > > @@ -532,6 +532,12 @@
> > > >                       interrupt-controller;
> > > >                       #interrupt-cells = <3>;
> > > >
> > > > +                     csi_pins: csi-pins {
> > > > +                             pins = "PE0", "PE2", "PE3", "PE4", "PE5", "PE6",
> > > > +                                    "PE7", "PE8", "PE9", "PE10", "PE11";
> > > > +                             function = "csi0";
> > > > +                     };
> > > > +
> > > >                       i2c0_pins: i2c0_pins {
> > > >                               pins = "PH0", "PH1";
> > > >                               function = "i2c0";
> > > > @@ -899,6 +905,23 @@
> > > >                       status = "disabled";
> > > >               };
> > > >
> > > > +             csi: csi@1cb0000 {
> > > > +                     compatible = "allwinner,sun50i-a64-csi",
> > > > +                                  "allwinner,sun8i-h3-csi";
> > > > +                     reg = <0x01cb0000 0x1000>;
> > > > +                     interrupts = <GIC_SPI 84 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                     clocks = <&ccu CLK_BUS_CSI>,
> > > > +                              <&ccu CLK_CSI_SCLK>,
> > > > +                              <&ccu CLK_DRAM_CSI>;
> > > > +                     clock-names = "bus", "mod", "ram";
> > > > +                     resets = <&ccu RST_BUS_CSI>;
> > > > +                     pinctrl-names = "default";
> > > > +                     pinctrl-0 = <&csi_pins>;
> > > > +                     assigned-clocks = <&ccu CLK_CSI_SCLK>;
> > > > +                     assigned-clock-rates = <300000000>;
> > >
> > > That should be enforced in the driver.
> > >
> >
> > We are not really sure what is the best here. Our first idea was to
> > put in the board file and then Jagan
> > decide to put in dtsi. We don't have enough coverage of camera on this
> > CPU and I prefer to stay with this
> > minimal change that does not impact the driver.
>
> The thing is that:
>   - in this commit log, you're stating that it depends on the sensor,
>     which indicates that this would be a board level addition
>   - In another patch series, Jagan reported IIRC that it actually
>     depends on the resolution, so it doesn't belong in the DT at all
>   - And then, you don't even have any guarantee on the clock rate. The
>     sole guarantee you have is that when your driver will probe, the
>     rate will be close to those 300MHz. That's it. It might completely
>     change after the driver has probed, or be rounded to something
>     else entirely, who knows.

Let's to be clear.

- with default clock(600MHz) the sensor get probed but image capture
has an issue.
- with 300MHz the image capture working with 320x240@30, 640x480@30,
640x480@60, 1280x720@30 with UYVY8_2X8 and YUYV8_2X8 formats but
1080p@30 seems broken (the same I have mentioned in another mail)
- since all this is verified on ov5640, I have mentioned the same
thing on commit message for future reference.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel