* [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock @ 2019-10-03 15:48 Maxime Ripard 2019-10-03 15:48 ` [PATCH 2/2] ARM: dts: sun7i: Drop the module clock from the device tree Maxime Ripard 2019-10-03 15:51 ` [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock Chen-Yu Tsai 0 siblings, 2 replies; 8+ messages in thread From: Maxime Ripard @ 2019-10-03 15:48 UTC (permalink / raw) To: Mark Rutland, Rob Herring, Frank Rowand, sakari.ailus, mchehab Cc: devicetree, Chen-Yu Tsai, Maxime Ripard, linux-arm-kernel, linux-media It turns out that what was thought to be the module clock was actually the clock meant to be used by the sensor, and isn't playing any role with the CSI controller itself. Let's drop that clock from our binding. Fixes: c5e8f4ccd775 ("media: dt-bindings: media: Add Allwinner A10 CSI binding") Reported-by: Chen-Yu Tsai <wens@csie.org> Signed-off-by: Maxime Ripard <mripard@kernel.org> --- .../devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml index 5dd1cf490cd9..d3e423fcb6c2 100644 --- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml @@ -27,14 +27,12 @@ properties: clocks: items: - description: The CSI interface clock - - description: The CSI module clock - description: The CSI ISP clock - description: The CSI DRAM clock clock-names: items: - const: bus - - const: mod - const: isp - const: ram @@ -89,9 +87,8 @@ examples: compatible = "allwinner,sun7i-a20-csi0"; 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 = "bus", "mod", "isp", "ram"; + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>; + clock-names = "bus", "isp", "ram"; resets = <&ccu RST_CSI0>; port { -- 2.23.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] ARM: dts: sun7i: Drop the module clock from the device tree 2019-10-03 15:48 [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock Maxime Ripard @ 2019-10-03 15:48 ` Maxime Ripard 2019-10-03 15:51 ` [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock Chen-Yu Tsai 1 sibling, 0 replies; 8+ messages in thread From: Maxime Ripard @ 2019-10-03 15:48 UTC (permalink / raw) To: Mark Rutland, Rob Herring, Frank Rowand, sakari.ailus, mchehab Cc: devicetree, Chen-Yu Tsai, Maxime Ripard, linux-arm-kernel, linux-media What we thought would be the module clock is actually the clock meant to be used by the sensors, and play no role in the CSI controller. Now that the binding has been updated to reflect that, let's update the device tree too. Fixes: d2b9c6444301 ("ARM: dts: sun7i: Add CSI0 controller") Reported-by: Chen-Yu Tsai <wens@csie.org> Signed-off-by: Maxime Ripard <mripard@kernel.org> --- arch/arm/boot/dts/sun7i-a20.dtsi | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi index 874231be04e4..8aebefd6accf 100644 --- a/arch/arm/boot/dts/sun7i-a20.dtsi +++ b/arch/arm/boot/dts/sun7i-a20.dtsi @@ -380,9 +380,8 @@ compatible = "allwinner,sun7i-a20-csi0"; 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 = "bus", "mod", "isp", "ram"; + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>; + clock-names = "bus", "isp", "ram"; resets = <&ccu RST_CSI0>; status = "disabled"; }; -- 2.23.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock 2019-10-03 15:48 [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock Maxime Ripard 2019-10-03 15:48 ` [PATCH 2/2] ARM: dts: sun7i: Drop the module clock from the device tree Maxime Ripard @ 2019-10-03 15:51 ` Chen-Yu Tsai 2019-10-03 16:37 ` Maxime Ripard 1 sibling, 1 reply; 8+ messages in thread From: Chen-Yu Tsai @ 2019-10-03 15:51 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Rutland, devicetree, Rob Herring, Sakari Ailus, Mauro Carvalho Chehab, Frank Rowand, linux-arm-kernel, Linux Media Mailing List Hi, On Thu, Oct 3, 2019 at 11:48 PM Maxime Ripard <mripard@kernel.org> wrote: > > It turns out that what was thought to be the module clock was actually the > clock meant to be used by the sensor, and isn't playing any role with the > CSI controller itself. Let's drop that clock from our binding. > > Fixes: c5e8f4ccd775 ("media: dt-bindings: media: Add Allwinner A10 CSI binding") > Reported-by: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Maxime Ripard <mripard@kernel.org> > --- > .../devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > index 5dd1cf490cd9..d3e423fcb6c2 100644 > --- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > @@ -27,14 +27,12 @@ properties: > clocks: > items: > - description: The CSI interface clock > - - description: The CSI module clock > - description: The CSI ISP clock > - description: The CSI DRAM clock > > clock-names: > items: > - const: bus > - - const: mod > - const: isp > - const: ram > > @@ -89,9 +87,8 @@ examples: > compatible = "allwinner,sun7i-a20-csi0"; > 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 = "bus", "mod", "isp", "ram"; > + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>; > + clock-names = "bus", "isp", "ram"; I believe what we thought was the ISP clock is actually the module clock for the whole CSI subsystem. So we should still use the module clock entry, and remove the ISP entry. ChenYu > resets = <&ccu RST_CSI0>; > > port { > -- > 2.23.0 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock 2019-10-03 15:51 ` [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock Chen-Yu Tsai @ 2019-10-03 16:37 ` Maxime Ripard 2019-10-04 6:33 ` Chen-Yu Tsai 0 siblings, 1 reply; 8+ messages in thread From: Maxime Ripard @ 2019-10-03 16:37 UTC (permalink / raw) To: Chen-Yu Tsai Cc: Mark Rutland, devicetree, Rob Herring, Sakari Ailus, Mauro Carvalho Chehab, Frank Rowand, linux-arm-kernel, Linux Media Mailing List [-- Attachment #1.1: Type: text/plain, Size: 2959 bytes --] Hi, On Thu, Oct 03, 2019 at 11:51:05PM +0800, Chen-Yu Tsai wrote: > On Thu, Oct 3, 2019 at 11:48 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > It turns out that what was thought to be the module clock was actually the > > clock meant to be used by the sensor, and isn't playing any role with the > > CSI controller itself. Let's drop that clock from our binding. > > > > Fixes: c5e8f4ccd775 ("media: dt-bindings: media: Add Allwinner A10 CSI binding") > > Reported-by: Chen-Yu Tsai <wens@csie.org> > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > > --- > > .../devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > index 5dd1cf490cd9..d3e423fcb6c2 100644 > > --- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > @@ -27,14 +27,12 @@ properties: > > clocks: > > items: > > - description: The CSI interface clock > > - - description: The CSI module clock > > - description: The CSI ISP clock > > - description: The CSI DRAM clock > > > > clock-names: > > items: > > - const: bus > > - - const: mod > > - const: isp > > - const: ram > > > > @@ -89,9 +87,8 @@ examples: > > compatible = "allwinner,sun7i-a20-csi0"; > > 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 = "bus", "mod", "isp", "ram"; > > + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>; > > + clock-names = "bus", "isp", "ram"; > > I believe what we thought was the ISP clock is actually the module clock > for the whole CSI subsystem. So we should still use the module clock entry, > and remove the ISP entry. I'm really not sure it is. CSI1 on the A20 (possibly the A10 as well, I haven't checked), and the one on the A13 don't have any ISP embedded in the CSI controller. It makes some difference, because according to the BSP, you can see that the ISP clock is taken for CSI0: https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi0/sun4i_drv_csi.c#L389 While for CSI1, that block is commented out, and the ISP clock never used: https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi1/sun4i_drv_csi.c#L396 So I really believe that the ISP clock is here to feed the ISP block within the CSI controller if there's any. But it's far from always the case, as opposed to a module clock for the whole CSI controller that would be here in both cases. Maxime [-- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock 2019-10-03 16:37 ` Maxime Ripard @ 2019-10-04 6:33 ` Chen-Yu Tsai 2019-10-07 11:09 ` Maxime Ripard 0 siblings, 1 reply; 8+ messages in thread From: Chen-Yu Tsai @ 2019-10-04 6:33 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Rutland, devicetree, Rob Herring, Sakari Ailus, Mauro Carvalho Chehab, Frank Rowand, linux-arm-kernel, Linux Media Mailing List On Fri, Oct 4, 2019 at 12:37 AM Maxime Ripard <mripard@kernel.org> wrote: > > Hi, > > On Thu, Oct 03, 2019 at 11:51:05PM +0800, Chen-Yu Tsai wrote: > > On Thu, Oct 3, 2019 at 11:48 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > It turns out that what was thought to be the module clock was actually the > > > clock meant to be used by the sensor, and isn't playing any role with the > > > CSI controller itself. Let's drop that clock from our binding. > > > > > > Fixes: c5e8f4ccd775 ("media: dt-bindings: media: Add Allwinner A10 CSI binding") > > > Reported-by: Chen-Yu Tsai <wens@csie.org> > > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > > > --- > > > .../devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 7 ++----- > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > index 5dd1cf490cd9..d3e423fcb6c2 100644 > > > --- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > @@ -27,14 +27,12 @@ properties: > > > clocks: > > > items: > > > - description: The CSI interface clock > > > - - description: The CSI module clock > > > - description: The CSI ISP clock > > > - description: The CSI DRAM clock > > > > > > clock-names: > > > items: > > > - const: bus > > > - - const: mod > > > - const: isp > > > - const: ram > > > > > > @@ -89,9 +87,8 @@ examples: > > > compatible = "allwinner,sun7i-a20-csi0"; > > > 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 = "bus", "mod", "isp", "ram"; > > > + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>; > > > + clock-names = "bus", "isp", "ram"; > > > > I believe what we thought was the ISP clock is actually the module clock > > for the whole CSI subsystem. So we should still use the module clock entry, > > and remove the ISP entry. > > I'm really not sure it is. CSI1 on the A20 (possibly the A10 as well, > I haven't checked), and the one on the A13 don't have any ISP embedded > in the CSI controller. > > It makes some difference, because according to the BSP, you can see > that the ISP clock is taken for CSI0: > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi0/sun4i_drv_csi.c#L389 > > While for CSI1, that block is commented out, and the ISP clock never > used: > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi1/sun4i_drv_csi.c#L396 > > So I really believe that the ISP clock is here to feed the ISP block > within the CSI controller if there's any. But it's far from always the > case, as opposed to a module clock for the whole CSI controller that > would be here in both cases. I guess we should try to test this with CSI1 one, either on a Cubieboard or OlinuXino with a simple camera like the OV7670? Do you have any hardware on hand for this? I have the Cubieboard but I need to get some proper 2.0mm connector blocks. ChenYu _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock 2019-10-04 6:33 ` Chen-Yu Tsai @ 2019-10-07 11:09 ` Maxime Ripard 2019-10-07 11:11 ` Chen-Yu Tsai 0 siblings, 1 reply; 8+ messages in thread From: Maxime Ripard @ 2019-10-07 11:09 UTC (permalink / raw) To: Chen-Yu Tsai Cc: Mark Rutland, devicetree, Rob Herring, Sakari Ailus, Mauro Carvalho Chehab, Frank Rowand, linux-arm-kernel, Linux Media Mailing List [-- Attachment #1.1: Type: text/plain, Size: 3671 bytes --] Hi, On Fri, Oct 04, 2019 at 02:33:41PM +0800, Chen-Yu Tsai wrote: > On Fri, Oct 4, 2019 at 12:37 AM Maxime Ripard <mripard@kernel.org> wrote: > > On Thu, Oct 03, 2019 at 11:51:05PM +0800, Chen-Yu Tsai wrote: > > > On Thu, Oct 3, 2019 at 11:48 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > It turns out that what was thought to be the module clock was actually the > > > > clock meant to be used by the sensor, and isn't playing any role with the > > > > CSI controller itself. Let's drop that clock from our binding. > > > > > > > > Fixes: c5e8f4ccd775 ("media: dt-bindings: media: Add Allwinner A10 CSI binding") > > > > Reported-by: Chen-Yu Tsai <wens@csie.org> > > > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > > > > --- > > > > .../devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 7 ++----- > > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > > index 5dd1cf490cd9..d3e423fcb6c2 100644 > > > > --- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > > @@ -27,14 +27,12 @@ properties: > > > > clocks: > > > > items: > > > > - description: The CSI interface clock > > > > - - description: The CSI module clock > > > > - description: The CSI ISP clock > > > > - description: The CSI DRAM clock > > > > > > > > clock-names: > > > > items: > > > > - const: bus > > > > - - const: mod > > > > - const: isp > > > > - const: ram > > > > > > > > @@ -89,9 +87,8 @@ examples: > > > > compatible = "allwinner,sun7i-a20-csi0"; > > > > 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 = "bus", "mod", "isp", "ram"; > > > > + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>; > > > > + clock-names = "bus", "isp", "ram"; > > > > > > I believe what we thought was the ISP clock is actually the module clock > > > for the whole CSI subsystem. So we should still use the module clock entry, > > > and remove the ISP entry. > > > > I'm really not sure it is. CSI1 on the A20 (possibly the A10 as well, > > I haven't checked), and the one on the A13 don't have any ISP embedded > > in the CSI controller. > > > > It makes some difference, because according to the BSP, you can see > > that the ISP clock is taken for CSI0: > > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi0/sun4i_drv_csi.c#L389 > > > > While for CSI1, that block is commented out, and the ISP clock never > > used: > > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi1/sun4i_drv_csi.c#L396 > > > > So I really believe that the ISP clock is here to feed the ISP block > > within the CSI controller if there's any. But it's far from always the > > case, as opposed to a module clock for the whole CSI controller that > > would be here in both cases. > > I guess we should try to test this with CSI1 one, either on a Cubieboard > or OlinuXino with a simple camera like the OV7670? > > Do you have any hardware on hand for this? I have the Cubieboard but I > need to get some proper 2.0mm connector blocks. I've tested it with the A13 before, and it doesn't need the ISP clock Maxime [-- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock 2019-10-07 11:09 ` Maxime Ripard @ 2019-10-07 11:11 ` Chen-Yu Tsai 2019-10-07 12:12 ` Maxime Ripard 0 siblings, 1 reply; 8+ messages in thread From: Chen-Yu Tsai @ 2019-10-07 11:11 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Rutland, devicetree, Rob Herring, Sakari Ailus, Mauro Carvalho Chehab, Frank Rowand, linux-arm-kernel, Linux Media Mailing List On Mon, Oct 7, 2019 at 7:09 PM Maxime Ripard <mripard@kernel.org> wrote: > > Hi, > > On Fri, Oct 04, 2019 at 02:33:41PM +0800, Chen-Yu Tsai wrote: > > On Fri, Oct 4, 2019 at 12:37 AM Maxime Ripard <mripard@kernel.org> wrote: > > > On Thu, Oct 03, 2019 at 11:51:05PM +0800, Chen-Yu Tsai wrote: > > > > On Thu, Oct 3, 2019 at 11:48 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > > > It turns out that what was thought to be the module clock was actually the > > > > > clock meant to be used by the sensor, and isn't playing any role with the > > > > > CSI controller itself. Let's drop that clock from our binding. > > > > > > > > > > Fixes: c5e8f4ccd775 ("media: dt-bindings: media: Add Allwinner A10 CSI binding") > > > > > Reported-by: Chen-Yu Tsai <wens@csie.org> > > > > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > > > > > --- > > > > > .../devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 7 ++----- > > > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > > > index 5dd1cf490cd9..d3e423fcb6c2 100644 > > > > > --- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > > > @@ -27,14 +27,12 @@ properties: > > > > > clocks: > > > > > items: > > > > > - description: The CSI interface clock > > > > > - - description: The CSI module clock > > > > > - description: The CSI ISP clock > > > > > - description: The CSI DRAM clock > > > > > > > > > > clock-names: > > > > > items: > > > > > - const: bus > > > > > - - const: mod > > > > > - const: isp > > > > > - const: ram > > > > > > > > > > @@ -89,9 +87,8 @@ examples: > > > > > compatible = "allwinner,sun7i-a20-csi0"; > > > > > 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 = "bus", "mod", "isp", "ram"; > > > > > + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>; > > > > > + clock-names = "bus", "isp", "ram"; > > > > > > > > I believe what we thought was the ISP clock is actually the module clock > > > > for the whole CSI subsystem. So we should still use the module clock entry, > > > > and remove the ISP entry. > > > > > > I'm really not sure it is. CSI1 on the A20 (possibly the A10 as well, > > > I haven't checked), and the one on the A13 don't have any ISP embedded > > > in the CSI controller. > > > > > > It makes some difference, because according to the BSP, you can see > > > that the ISP clock is taken for CSI0: > > > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi0/sun4i_drv_csi.c#L389 > > > > > > While for CSI1, that block is commented out, and the ISP clock never > > > used: > > > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi1/sun4i_drv_csi.c#L396 > > > > > > So I really believe that the ISP clock is here to feed the ISP block > > > within the CSI controller if there's any. But it's far from always the > > > case, as opposed to a module clock for the whole CSI controller that > > > would be here in both cases. > > > > I guess we should try to test this with CSI1 one, either on a Cubieboard > > or OlinuXino with a simple camera like the OV7670? > > > > Do you have any hardware on hand for this? I have the Cubieboard but I > > need to get some proper 2.0mm connector blocks. > > I've tested it with the A13 before, and it doesn't need the ISP clock OK! That clears things up! ChenYu _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock 2019-10-07 11:11 ` Chen-Yu Tsai @ 2019-10-07 12:12 ` Maxime Ripard 0 siblings, 0 replies; 8+ messages in thread From: Maxime Ripard @ 2019-10-07 12:12 UTC (permalink / raw) To: Chen-Yu Tsai Cc: Mark Rutland, devicetree, Rob Herring, Sakari Ailus, Mauro Carvalho Chehab, Frank Rowand, linux-arm-kernel, Linux Media Mailing List [-- Attachment #1.1: Type: text/plain, Size: 4188 bytes --] On Mon, Oct 07, 2019 at 07:11:03PM +0800, Chen-Yu Tsai wrote: > On Mon, Oct 7, 2019 at 7:09 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > Hi, > > > > On Fri, Oct 04, 2019 at 02:33:41PM +0800, Chen-Yu Tsai wrote: > > > On Fri, Oct 4, 2019 at 12:37 AM Maxime Ripard <mripard@kernel.org> wrote: > > > > On Thu, Oct 03, 2019 at 11:51:05PM +0800, Chen-Yu Tsai wrote: > > > > > On Thu, Oct 3, 2019 at 11:48 PM Maxime Ripard <mripard@kernel.org> wrote: > > > > > > > > > > > > It turns out that what was thought to be the module clock was actually the > > > > > > clock meant to be used by the sensor, and isn't playing any role with the > > > > > > CSI controller itself. Let's drop that clock from our binding. > > > > > > > > > > > > Fixes: c5e8f4ccd775 ("media: dt-bindings: media: Add Allwinner A10 CSI binding") > > > > > > Reported-by: Chen-Yu Tsai <wens@csie.org> > > > > > > Signed-off-by: Maxime Ripard <mripard@kernel.org> > > > > > > --- > > > > > > .../devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml | 7 ++----- > > > > > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > > > > index 5dd1cf490cd9..d3e423fcb6c2 100644 > > > > > > --- a/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > > > > +++ b/Documentation/devicetree/bindings/media/allwinner,sun4i-a10-csi.yaml > > > > > > @@ -27,14 +27,12 @@ properties: > > > > > > clocks: > > > > > > items: > > > > > > - description: The CSI interface clock > > > > > > - - description: The CSI module clock > > > > > > - description: The CSI ISP clock > > > > > > - description: The CSI DRAM clock > > > > > > > > > > > > clock-names: > > > > > > items: > > > > > > - const: bus > > > > > > - - const: mod > > > > > > - const: isp > > > > > > - const: ram > > > > > > > > > > > > @@ -89,9 +87,8 @@ examples: > > > > > > compatible = "allwinner,sun7i-a20-csi0"; > > > > > > 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 = "bus", "mod", "isp", "ram"; > > > > > > + clocks = <&ccu CLK_AHB_CSI0>, <&ccu CLK_CSI_SCLK>, <&ccu CLK_DRAM_CSI0>; > > > > > > + clock-names = "bus", "isp", "ram"; > > > > > > > > > > I believe what we thought was the ISP clock is actually the module clock > > > > > for the whole CSI subsystem. So we should still use the module clock entry, > > > > > and remove the ISP entry. > > > > > > > > I'm really not sure it is. CSI1 on the A20 (possibly the A10 as well, > > > > I haven't checked), and the one on the A13 don't have any ISP embedded > > > > in the CSI controller. > > > > > > > > It makes some difference, because according to the BSP, you can see > > > > that the ISP clock is taken for CSI0: > > > > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi0/sun4i_drv_csi.c#L389 > > > > > > > > While for CSI1, that block is commented out, and the ISP clock never > > > > used: > > > > https://github.com/linux-sunxi/linux-sunxi/blob/sunxi-3.4/drivers/media/video/sun4i_csi/csi1/sun4i_drv_csi.c#L396 > > > > > > > > So I really believe that the ISP clock is here to feed the ISP block > > > > within the CSI controller if there's any. But it's far from always the > > > > case, as opposed to a module clock for the whole CSI controller that > > > > would be here in both cases. > > > > > > I guess we should try to test this with CSI1 one, either on a Cubieboard > > > or OlinuXino with a simple camera like the OV7670? > > > > > > Do you have any hardware on hand for this? I have the Cubieboard but I > > > need to get some proper 2.0mm connector blocks. > > > > I've tested it with the A13 before, and it doesn't need the ISP clock > > OK! That clears things up! I've added both patches as fixes then. Thanks! Maxime [-- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-10-07 12:12 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-03 15:48 [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock Maxime Ripard 2019-10-03 15:48 ` [PATCH 2/2] ARM: dts: sun7i: Drop the module clock from the device tree Maxime Ripard 2019-10-03 15:51 ` [PATCH 1/2] dt-bindings: media: sun4i-csi: Drop the module clock Chen-Yu Tsai 2019-10-03 16:37 ` Maxime Ripard 2019-10-04 6:33 ` Chen-Yu Tsai 2019-10-07 11:09 ` Maxime Ripard 2019-10-07 11:11 ` Chen-Yu Tsai 2019-10-07 12:12 ` Maxime Ripard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).