linux-rockchip.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399
@ 2020-07-22 15:55 Helen Koike
  2020-07-22 15:55 ` [PATCH v5 2/9] media: staging: dt-bindings: rkisp1: drop i2c unit address Helen Koike
                   ` (8 more replies)
  0 siblings, 9 replies; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree, linux-media, linux-rockchip
  Cc: linux-kernel, devel, robh+dt, heiko, hverkuil-cisco, kernel,
	dafna.hirschfeld, ezequiel, mark.rutland, karthik.poduval,
	jbx6244, tfiga, eddie.cai.linux, zhengsq, robin.murphy

Move the bindings out of drivers/staging and place them in
Documentation/devicetree/bindings instead.

Also, add DT nodes for RK3399 and verify with make ARCH=arm64 dtbs_check
and make ARCH=arm64 dt_binding_check.

Tested by verifying images streamed from Scarlet Chromebook

Changes in v5:
- Drop unit addresses in dt-bindings example for simplification and fix
errors as suggested by Rob Herring in previous version
- Fix typos
- Re-write clock organization with if/then schema

Changes in v4:
- simplify clocks with "isp", "aclk" and "hclk" as suggested by
Robin Murphy on https://patchwork.kernel.org/patch/11475007/

Changes in v3:
- dropped accepted patches
- cleanup clocks
- fix "no reg" error in dt-bindings parent@0 example
- add linux-rockchip list in MAINTAINERS and reorder items
- add Scarlet sensors dt nodes to the series

Changes in v2:
Add patches modifying bindings, as sugested by Johan Jonker in v1,
before moving them out of staging.

Eddie Cai (1):
  arm64: dts: rockchip: add isp and sensors for Scarlet

Helen Koike (7):
  media: staging: dt-bindings: rkisp1: add missing required nodes
  media: staging: dt-bindings: rkisp1: drop i2c unit address
  media: staging: dt-bindings: rkisp1: re-order properties
  media: staging: dt-bindings: rkisp1: drop parent unit address
  media: staging: rkisp1: remove unecessary clocks
  dt-bindings: media: rkisp1: move rockchip-isp1 bindings out of staging
  media: MAINTAINERS: rkisp1: add path to dt-bindings

Shunqian Zheng (1):
  arm64: dts: rockchip: add isp0 node for rk3399

 .../bindings/media/rockchip-isp1.yaml         | 81 ++++++++++++-------
 MAINTAINERS                                   |  2 +
 .../boot/dts/rockchip/rk3399-gru-scarlet.dtsi | 74 +++++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi      | 25 ++++++
 drivers/staging/media/rkisp1/rkisp1-dev.c     |  8 +-
 5 files changed, 156 insertions(+), 34 deletions(-)
 rename {drivers/staging/media/rkisp1/Documentation => Documentation}/devicetree/bindings/media/rockchip-isp1.yaml (80%)

-- 
2.28.0.rc1

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v5 1/9] media: staging: dt-bindings: rkisp1: add missing required nodes
       [not found] ` <20200722155533.252844-1-helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
@ 2020-07-22 15:55   ` Helen Koike
  2020-09-18 14:02     ` Rob Herring
  2020-07-22 15:55   ` [PATCH v5 4/9] media: staging: dt-bindings: rkisp1: drop parent unit address Helen Koike
  1 sibling, 1 reply; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-media-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, heiko-4mtYJXux2i+zQB+pC5nmwQ,
	hverkuil-cisco-qWit8jRvyhVmR6Xm/wNWPw,
	kernel-ZGY8ohtN/8qB+jHODAdFcQ,
	dafna.hirschfeld-ZGY8ohtN/8qB+jHODAdFcQ,
	ezequiel-ZGY8ohtN/8qB+jHODAdFcQ, mark.rutland-5wv7dgnIgG8,
	karthik.poduval-Re5JQEeQqe8AvxtiuMwx3w,
	jbx6244-Re5JQEeQqe8AvxtiuMwx3w, tfiga-F7+t8E8rja9g9hUCZPvPmw,
	eddie.cai.linux-Re5JQEeQqe8AvxtiuMwx3w,
	zhengsq-TNX95d0MmH7DzftRWevZcw, robin.murphy-5wv7dgnIgG8

Add missing required nodes in json-schema yaml file for
Rockchip ISP1 dt-bindings.

Signed-off-by: Helen Koike <helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---

Changes in v2:
- New patch in the series
---
 .../devicetree/bindings/media/rockchip-isp1.yaml          | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
index af246b71eac6b..a77b6ec500c95 100644
--- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
+++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
@@ -94,11 +94,19 @@ properties:
 
               remote-endpoint: true
 
+        required:
+          - reg
+          - "#address-cells"
+          - "#size-cells"
+
     required:
+      - "#address-cells"
+      - "#size-cells"
       - port@0
 
 required:
   - compatible
+  - reg
   - interrupts
   - clocks
   - clock-names
-- 
2.28.0.rc1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v5 2/9] media: staging: dt-bindings: rkisp1: drop i2c unit address
  2020-07-22 15:55 [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Helen Koike
@ 2020-07-22 15:55 ` Helen Koike
  2020-09-18 14:03   ` Rob Herring
  2020-07-22 15:55 ` [PATCH v5 3/9] media: staging: dt-bindings: rkisp1: re-order properties Helen Koike
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree, linux-media, linux-rockchip
  Cc: linux-kernel, devel, robh+dt, heiko, hverkuil-cisco, kernel,
	dafna.hirschfeld, ezequiel, mark.rutland, karthik.poduval,
	jbx6244, tfiga, eddie.cai.linux, zhengsq, robin.murphy

Add missing required items in Rockchip ISP1 dt-bindings example for
a complete i2c node.
Drop unit address to Fix error:
/example-0/parent/i2c@ff160000: node has a unit name, but no reg or ranges property
Remove unecessary fields for the example.

Signed-off-by: Helen Koike <helen.koike@collabora.com>
---

Changes in v5:
- Patch re-written to drop unity address instead of completing i2c node

Changes in v2:
- new patch in the series

tmp: i2c drop fields
---
 .../Documentation/devicetree/bindings/media/rockchip-isp1.yaml | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
index a77b6ec500c95..4c31cfaee2709 100644
--- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
+++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
@@ -168,8 +168,7 @@ examples:
             };
         };
 
-        i2c7: i2c@ff160000 {
-            clock-frequency = <400000>;
+        i2c7: i2c {
             #address-cells = <1>;
             #size-cells = <0>;
 
-- 
2.28.0.rc1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v5 3/9] media: staging: dt-bindings: rkisp1: re-order properties
  2020-07-22 15:55 [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Helen Koike
  2020-07-22 15:55 ` [PATCH v5 2/9] media: staging: dt-bindings: rkisp1: drop i2c unit address Helen Koike
@ 2020-07-22 15:55 ` Helen Koike
       [not found] ` <20200722155533.252844-1-helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree, linux-media, linux-rockchip
  Cc: linux-kernel, devel, robh+dt, heiko, hverkuil-cisco, kernel,
	dafna.hirschfeld, ezequiel, mark.rutland, karthik.poduval,
	jbx6244, tfiga, eddie.cai.linux, zhengsq, robin.murphy

Organize properties order in dt-bindings to move it out of staging.

On top: compatible, reg and interrupts.
Then alphabetical order, then properties starting with '#'.

Signed-off-by: Helen Koike <helen.koike@collabora.com>
Acked-by: Rob Herring <robh@kernel.org>
---
V5:
- s/binbings/bindings

V2:
- this is a new patch in the series
---
 .../bindings/media/rockchip-isp1.yaml         | 32 +++++++++----------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
index 4c31cfaee2709..79ebacab83cf3 100644
--- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
+++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
@@ -23,19 +23,6 @@ properties:
   interrupts:
     maxItems: 1
 
-  iommus:
-    maxItems: 1
-
-  power-domains:
-    maxItems: 1
-
-  phys:
-    maxItems: 1
-    description: phandle for the PHY port
-
-  phy-names:
-    const: dphy
-
   clocks:
     items:
       - description: ISP clock
@@ -52,6 +39,19 @@ properties:
       - const: hclk_isp
       - const: hclk_isp_wrap
 
+  iommus:
+    maxItems: 1
+
+  phys:
+    maxItems: 1
+    description: phandle for the PHY port
+
+  phy-names:
+    const: dphy
+
+  power-domains:
+    maxItems: 1
+
   # See ./video-interfaces.txt for details
   ports:
     type: object
@@ -110,10 +110,10 @@ required:
   - interrupts
   - clocks
   - clock-names
-  - power-domains
   - iommus
   - phys
   - phy-names
+  - power-domains
   - ports
 
 additionalProperties: false
@@ -139,19 +139,19 @@ examples:
             clock-names = "clk_isp",
                           "aclk_isp", "aclk_isp_wrap",
                           "hclk_isp", "hclk_isp_wrap";
-            power-domains = <&power RK3399_PD_ISP0>;
             iommus = <&isp0_mmu>;
             phys = <&dphy>;
             phy-names = "dphy";
+            power-domains = <&power RK3399_PD_ISP0>;
 
             ports {
                 #address-cells = <1>;
                 #size-cells = <0>;
 
                 port@0 {
+                    reg = <0>;
                     #address-cells = <1>;
                     #size-cells = <0>;
-                    reg = <0>;
 
                     mipi_in_wcam: endpoint@0 {
                         reg = <0>;
-- 
2.28.0.rc1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v5 4/9] media: staging: dt-bindings: rkisp1: drop parent unit address
       [not found] ` <20200722155533.252844-1-helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
  2020-07-22 15:55   ` [PATCH v5 1/9] media: staging: dt-bindings: rkisp1: add missing required nodes Helen Koike
@ 2020-07-22 15:55   ` Helen Koike
  2020-09-18 14:04     ` Rob Herring
  1 sibling, 1 reply; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-media-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	devel-gWbeCf7V1WCQmaza687I9mD2FQJk+8+b,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, heiko-4mtYJXux2i+zQB+pC5nmwQ,
	hverkuil-cisco-qWit8jRvyhVmR6Xm/wNWPw,
	kernel-ZGY8ohtN/8qB+jHODAdFcQ,
	dafna.hirschfeld-ZGY8ohtN/8qB+jHODAdFcQ,
	ezequiel-ZGY8ohtN/8qB+jHODAdFcQ, mark.rutland-5wv7dgnIgG8,
	karthik.poduval-Re5JQEeQqe8AvxtiuMwx3w,
	jbx6244-Re5JQEeQqe8AvxtiuMwx3w, tfiga-F7+t8E8rja9g9hUCZPvPmw,
	eddie.cai.linux-Re5JQEeQqe8AvxtiuMwx3w,
	zhengsq-TNX95d0MmH7DzftRWevZcw, robin.murphy-5wv7dgnIgG8

Fix the following error found with make ARCH=arm64 dt_binding_check:

Documentation/devicetree/bindings/media/rockchip-isp1.example.dts:24.27-101.11:
Warning (unit_address_vs_reg): /example-0/parent@0: node has a unit name, but no reg or ranges property

Reported-by: Johan Jonker <jbx6244-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Helen Koike <helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
---
V5:
- Patch re-written to drop the unit address instead of adding reg

V3:
- this is a new patch in the series
---
 .../Documentation/devicetree/bindings/media/rockchip-isp1.yaml  | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
index 79ebacab83cf3..62a6b9c959498 100644
--- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
+++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
@@ -125,7 +125,7 @@ examples:
     #include <dt-bindings/interrupt-controller/arm-gic.h>
     #include <dt-bindings/power/rk3399-power.h>
 
-    parent0: parent@0 {
+    parent0: parent {
         #address-cells = <2>;
         #size-cells = <2>;
 
-- 
2.28.0.rc1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks
  2020-07-22 15:55 [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Helen Koike
                   ` (2 preceding siblings ...)
       [not found] ` <20200722155533.252844-1-helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
@ 2020-07-22 15:55 ` Helen Koike
  2020-09-22 14:24   ` Rob Herring
  2020-07-22 15:55 ` [PATCH v5 6/9] dt-bindings: media: rkisp1: move rockchip-isp1 bindings out of staging Helen Koike
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree, linux-media, linux-rockchip
  Cc: linux-kernel, devel, robh+dt, heiko, hverkuil-cisco, kernel,
	dafna.hirschfeld, ezequiel, mark.rutland, karthik.poduval,
	jbx6244, tfiga, eddie.cai.linux, zhengsq, robin.murphy

aclk_isp_wrap is a child of aclk_isp, and hclk_isp_wrap is a child of
hclk_isp, thus we can remove parents from the list.

Also, for the isp0, we only need the ISP clock, ACLK and HCLK.
In the future we'll need a pixel clock for RK3288 and RK3399, and a JPEG
clock for RK3288.

So with the goal to cleanup the dt-bindings and remove it from staging,
simplify clock names to isp, aclk and hclk.

Assigned clocks are meant to refer to the full path in the clock tree,
i.e. the leaf in the tree.
For instance, in RK3399, the clock responsible for ACLK (ISP AXI CLOCK)
is aclk_isp0_wrapper.

For reference, this is the isp clock topology on RK3399:

 xin24m
    pll_npll
       npll
          clk_isp1
          clk_isp0
    pll_cpll
       cpll
          aclk_isp1
             aclk_isp1_noc
             hclk_isp1
                aclk_isp1_wrapper
                hclk_isp1_noc
          aclk_isp0
             hclk_isp1_wrapper
             aclk_isp0_wrapper
             aclk_isp0_noc
             hclk_isp0
                hclk_isp0_wrapper
                hclk_isp0_noc
 pclkin_isp1_wrapper

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---
Changes in V5:
- Use if/then schema as suggested by Rob Herring on
https://patchwork.linuxtv.org/project/linux-media/patch/20200702191322.2639681-6-helen.koike@collabora.com/#119729

Changes in V4:
- update binding according to suggestion by Robin Murphy
on https://patchwork.kernel.org/patch/11475007/

Changes in V3:
- this is a new patch in the series
---
 .../bindings/media/rockchip-isp1.yaml         | 50 ++++++++++++-------
 drivers/staging/media/rkisp1/rkisp1-dev.c     |  8 ++-
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
index 62a6b9c959498..23c677d15037a 100644
--- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
+++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
@@ -24,20 +24,10 @@ properties:
     maxItems: 1
 
   clocks:
-    items:
-      - description: ISP clock
-      - description: ISP AXI clock clock
-      - description: ISP AXI clock  wrapper clock
-      - description: ISP AHB clock clock
-      - description: ISP AHB wrapper clock
+    minItems: 3
 
   clock-names:
-    items:
-      - const: clk_isp
-      - const: aclk_isp
-      - const: aclk_isp_wrap
-      - const: hclk_isp
-      - const: hclk_isp_wrap
+    minItems: 3
 
   iommus:
     maxItems: 1
@@ -116,6 +106,34 @@ required:
   - power-domains
   - ports
 
+if:
+  properties:
+    compatible:
+      contains:
+        const: rockchip,rk3399-cif-isp
+then:
+  properties:
+    clocks:
+      maxItems: 4
+      minItems: 3
+      items:
+        # isp0 and isp1
+        - description: ISP clock
+        - description: ISP AXI clock
+        - description: ISP AHB clock
+        # only for isp1
+        - description: ISP Pixel clock
+    clock-names:
+      maxItems: 4
+      minItems: 3
+      items:
+        # isp0 and isp1
+        - const: isp
+        - const: aclk
+        - const: hclk
+        # only for isp1
+        - const: pclk_isp
+
 additionalProperties: false
 
 examples:
@@ -134,11 +152,9 @@ examples:
             reg = <0x0 0xff910000 0x0 0x4000>;
             interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>;
             clocks = <&cru SCLK_ISP0>,
-                     <&cru ACLK_ISP0>, <&cru ACLK_ISP0_WRAPPER>,
-                     <&cru HCLK_ISP0>, <&cru HCLK_ISP0_WRAPPER>;
-            clock-names = "clk_isp",
-                          "aclk_isp", "aclk_isp_wrap",
-                          "hclk_isp", "hclk_isp_wrap";
+                     <&cru ACLK_ISP0_WRAPPER>,
+                     <&cru HCLK_ISP0_WRAPPER>;
+            clock-names = "isp", "aclk", "hclk";
             iommus = <&isp0_mmu>;
             phys = <&dphy>;
             phy-names = "dphy";
diff --git a/drivers/staging/media/rkisp1/rkisp1-dev.c b/drivers/staging/media/rkisp1/rkisp1-dev.c
index a0eb8f08708b9..1ebf10a6f188c 100644
--- a/drivers/staging/media/rkisp1/rkisp1-dev.c
+++ b/drivers/staging/media/rkisp1/rkisp1-dev.c
@@ -406,11 +406,9 @@ static irqreturn_t rkisp1_isr(int irq, void *ctx)
 }
 
 static const char * const rk3399_isp_clks[] = {
-	"clk_isp",
-	"aclk_isp",
-	"hclk_isp",
-	"aclk_isp_wrap",
-	"hclk_isp_wrap",
+	"isp",
+	"aclk",
+	"hclk",
 };
 
 static const struct rkisp1_match_data rk3399_isp_clk_data = {
-- 
2.28.0.rc1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v5 6/9] dt-bindings: media: rkisp1: move rockchip-isp1 bindings out of staging
  2020-07-22 15:55 [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Helen Koike
                   ` (3 preceding siblings ...)
  2020-07-22 15:55 ` [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks Helen Koike
@ 2020-07-22 15:55 ` Helen Koike
  2020-07-22 15:55 ` [PATCH v5 7/9] media: MAINTAINERS: rkisp1: add path to dt-bindings Helen Koike
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree, linux-media, linux-rockchip
  Cc: linux-kernel, devel, robh+dt, heiko, hverkuil-cisco, kernel,
	dafna.hirschfeld, ezequiel, mark.rutland, karthik.poduval,
	jbx6244, tfiga, eddie.cai.linux, zhengsq, robin.murphy

Move rkisp1 bindings to Documentation/devicetree/bindings/media

Verified with:
make ARCH=arm64 dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml

Signed-off-by: Helen Koike <helen.koike@collabora.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/media/rockchip-isp1.yaml                  | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename {drivers/staging/media/rkisp1/Documentation => Documentation}/devicetree/bindings/media/rockchip-isp1.yaml (100%)

diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
similarity index 100%
rename from drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
rename to Documentation/devicetree/bindings/media/rockchip-isp1.yaml
-- 
2.28.0.rc1

^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v5 7/9] media: MAINTAINERS: rkisp1: add path to dt-bindings
  2020-07-22 15:55 [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Helen Koike
                   ` (4 preceding siblings ...)
  2020-07-22 15:55 ` [PATCH v5 6/9] dt-bindings: media: rkisp1: move rockchip-isp1 bindings out of staging Helen Koike
@ 2020-07-22 15:55 ` Helen Koike
  2020-07-22 15:55 ` [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399 Helen Koike
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree, linux-media, linux-rockchip
  Cc: linux-kernel, devel, robh+dt, heiko, hverkuil-cisco, kernel,
	dafna.hirschfeld, ezequiel, mark.rutland, karthik.poduval,
	jbx6244, tfiga, eddie.cai.linux, zhengsq, robin.murphy

The Rockchip ISP bindings was moved out of staging.
Update MAINTAINERS file with the new path.

Fields sorted according to output of
./scripts/parse-maintainers.pl --input=MAINTAINERS --output=MAINTAINERS
--order

Signed-off-by: Helen Koike <helen.koike@collabora.com>

---

V3:
- Add line:
L:     linux-rockchip@lists.infradead.org
- Re-order:
F:     drivers/staging/media/rkisp1/

V2:
- This is a new patch in the series
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5392f00cec46d..bfd947ea5c920 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14717,7 +14717,9 @@ F:	include/linux/hid-roccat*
 ROCKCHIP ISP V1 DRIVER
 M:	Helen Koike <helen.koike@collabora.com>
 L:	linux-media@vger.kernel.org
+L:	linux-rockchip@lists.infradead.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/media/rockchip-isp1.yaml
 F:	drivers/staging/media/rkisp1/
 
 ROCKCHIP RASTER 2D GRAPHIC ACCELERATION UNIT DRIVER
-- 
2.28.0.rc1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399
  2020-07-22 15:55 [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Helen Koike
                   ` (5 preceding siblings ...)
  2020-07-22 15:55 ` [PATCH v5 7/9] media: MAINTAINERS: rkisp1: add path to dt-bindings Helen Koike
@ 2020-07-22 15:55 ` Helen Koike
  2020-09-26 13:00   ` Tomasz Figa
  2020-07-22 15:55 ` [PATCH v5 9/9] arm64: dts: rockchip: add isp and sensors for Scarlet Helen Koike
  2020-09-26 13:03 ` [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Tomasz Figa
  8 siblings, 1 reply; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree, linux-media, linux-rockchip
  Cc: linux-kernel, devel, robh+dt, heiko, hverkuil-cisco, kernel,
	dafna.hirschfeld, ezequiel, mark.rutland, karthik.poduval,
	jbx6244, tfiga, eddie.cai.linux, zhengsq, robin.murphy

From: Shunqian Zheng <zhengsq@rock-chips.com>

RK3399 has two ISPs, but only isp0 was tested.
Add isp0 node in rk3399 dtsi

Verified with:
make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml

Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
Signed-off-by: Helen Koike <helen.koike@collabora.com>

---

V4:
- update clock names

V3:
- clean up clocks

V2:
- re-order power-domains property

V1:
This patch was originally part of this patchset:

    https://patchwork.kernel.org/patch/10267431/

The only difference is:
- add phy properties
- add ports
---
 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index dba9641947a3a..ed8ba75dbbce8 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 {
 		status = "disabled";
 	};
 
+	isp0: isp0@ff910000 {
+		compatible = "rockchip,rk3399-cif-isp";
+		reg = <0x0 0xff910000 0x0 0x4000>;
+		interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru SCLK_ISP0>,
+			 <&cru ACLK_ISP0_WRAPPER>,
+			 <&cru HCLK_ISP0_WRAPPER>;
+		clock-names = "isp", "aclk", "hclk";
+		iommus = <&isp0_mmu>;
+		phys = <&mipi_dphy_rx0>;
+		phy-names = "dphy";
+		power-domains = <&power RK3399_PD_ISP0>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+			};
+		};
+	};
+
 	isp0_mmu: iommu@ff914000 {
 		compatible = "rockchip,iommu";
 		reg = <0x0 0xff914000 0x0 0x100>, <0x0 0xff915000 0x0 0x100>;
-- 
2.28.0.rc1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v5 9/9] arm64: dts: rockchip: add isp and sensors for Scarlet
  2020-07-22 15:55 [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Helen Koike
                   ` (6 preceding siblings ...)
  2020-07-22 15:55 ` [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399 Helen Koike
@ 2020-07-22 15:55 ` Helen Koike
  2020-09-26 13:03 ` [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Tomasz Figa
  8 siblings, 0 replies; 23+ messages in thread
From: Helen Koike @ 2020-07-22 15:55 UTC (permalink / raw)
  To: devicetree, linux-media, linux-rockchip
  Cc: linux-kernel, devel, robh+dt, heiko, hverkuil-cisco, kernel,
	dafna.hirschfeld, ezequiel, mark.rutland, karthik.poduval,
	jbx6244, tfiga, eddie.cai.linux, zhengsq, robin.murphy

From: Eddie Cai <eddie.cai.linux@gmail.com>

Enable ISP and camera sensor ov2685 and ov5695 for Scarlet Chromebook

Verified with:
    make ARCH=arm64 dtbs_check

Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
Signed-off-by: Eddie Cai <eddie.cai.linux@gmail.com>
Signed-off-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Helen Koike <helen.koike@collabora.com>
---

This patch is based on:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/527854

Changes in V3:
- This patch was first submitted stand alone at
https://lore.kernel.org/patchwork/patch/1223736/
I'm including in this patch series without changes
---
 .../boot/dts/rockchip/rk3399-gru-scarlet.dtsi | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399-gru-scarlet.dtsi b/arch/arm64/boot/dts/rockchip/rk3399-gru-scarlet.dtsi
index 4373ed732af76..ae08205aa8e24 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399-gru-scarlet.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399-gru-scarlet.dtsi
@@ -296,6 +296,52 @@ camera: &i2c7 {
 
 	/* 24M mclk is shared between world and user cameras */
 	pinctrl-0 = <&i2c7_xfer &test_clkout1>;
+
+	/* Rear-facing camera */
+	wcam: camera@36 {
+		compatible = "ovti,ov5695";
+		reg = <0x36>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&wcam_rst>;
+
+		clocks = <&cru SCLK_TESTCLKOUT1>;
+		clock-names = "xvclk";
+
+		avdd-supply = <&pp2800_cam>;
+		dvdd-supply = <&pp1250_cam>;
+		dovdd-supply = <&pp1800_s0>;
+		reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
+
+		port {
+			wcam_out: endpoint {
+				remote-endpoint = <&mipi_in_wcam>;
+				data-lanes = <1 2>;
+			};
+		};
+	};
+
+	/* Front-facing camera */
+	ucam: camera@3c {
+		compatible = "ovti,ov2685";
+		reg = <0x3c>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&ucam_rst>;
+
+		clocks = <&cru SCLK_TESTCLKOUT1>;
+		clock-names = "xvclk";
+
+		avdd-supply = <&pp2800_cam>;
+		dovdd-supply = <&pp1800_s0>;
+		dvdd-supply = <&pp1800_s0>;
+		reset-gpios = <&gpio2 3 GPIO_ACTIVE_LOW>;
+
+		port {
+			ucam_out: endpoint {
+				remote-endpoint = <&mipi_in_ucam>;
+				data-lanes = <1>;
+			};
+		};
+	};
 };
 
 &cdn_dp {
@@ -353,10 +399,38 @@ &io_domains {
 	gpio1830-supply = <&pp1800_s0>;		/* APIO4_VDD;  4c 4d */
 };
 
+&isp0 {
+	status = "okay";
+
+	ports {
+		port@0 {
+			mipi_in_wcam: endpoint@0 {
+				reg = <0>;
+				remote-endpoint = <&wcam_out>;
+				data-lanes = <1 2>;
+			};
+
+			mipi_in_ucam: endpoint@1 {
+				reg = <1>;
+				remote-endpoint = <&ucam_out>;
+				data-lanes = <1>;
+			};
+		};
+	};
+};
+
+&isp0_mmu {
+	status = "okay";
+};
+
 &max98357a {
 	sdmode-gpios = <&gpio0 2 GPIO_ACTIVE_HIGH>;
 };
 
+&mipi_dphy_rx0 {
+	status = "okay";
+};
+
 &mipi_dsi {
 	status = "okay";
 	clock-master;
-- 
2.28.0.rc1

^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 1/9] media: staging: dt-bindings: rkisp1: add missing required nodes
  2020-07-22 15:55   ` [PATCH v5 1/9] media: staging: dt-bindings: rkisp1: add missing required nodes Helen Koike
@ 2020-09-18 14:02     ` Rob Herring
  0 siblings, 0 replies; 23+ messages in thread
From: Rob Herring @ 2020-09-18 14:02 UTC (permalink / raw)
  To: Helen Koike
  Cc: open list:STAGING SUBSYSTEM, devicetree, Eddie Cai, Tomasz Figa,
	Dafna Hirschfeld, heiko, Shunqian Zheng, linux-kernel,
	karthik.poduval, open list:ARM/Rockchip SoC...,
	Hans Verkuil, Robin Murphy, Mark Rutland, Collabora Kernel ML,
	Ezequiel Garcia, Johan Jonker, Linux Media Mailing List

On Wed, Jul 22, 2020 at 9:55 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> Add missing required nodes in json-schema yaml file for
> Rockchip ISP1 dt-bindings.
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
>
> Changes in v2:
> - New patch in the series
> ---
>  .../devicetree/bindings/media/rockchip-isp1.yaml          | 8 ++++++++
>  1 file changed, 8 insertions(+)

Sorry, PW filters patches and this isn't under
Documentation/devicetree/bindings/.

Acked-by: Rob Herring <robh@kernel.org>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 2/9] media: staging: dt-bindings: rkisp1: drop i2c unit address
  2020-07-22 15:55 ` [PATCH v5 2/9] media: staging: dt-bindings: rkisp1: drop i2c unit address Helen Koike
@ 2020-09-18 14:03   ` Rob Herring
  0 siblings, 0 replies; 23+ messages in thread
From: Rob Herring @ 2020-09-18 14:03 UTC (permalink / raw)
  To: Helen Koike
  Cc: open list:STAGING SUBSYSTEM, devicetree, Eddie Cai, Tomasz Figa,
	Dafna Hirschfeld, heiko, Shunqian Zheng, linux-kernel,
	karthik.poduval, open list:ARM/Rockchip SoC...,
	Hans Verkuil, Robin Murphy, Mark Rutland, Collabora Kernel ML,
	Ezequiel Garcia, Johan Jonker, Linux Media Mailing List

On Wed, Jul 22, 2020 at 9:55 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> Add missing required items in Rockchip ISP1 dt-bindings example for
> a complete i2c node.
> Drop unit address to Fix error:
> /example-0/parent/i2c@ff160000: node has a unit name, but no reg or ranges property
> Remove unecessary fields for the example.
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
>
> Changes in v5:
> - Patch re-written to drop unity address instead of completing i2c node
>
> Changes in v2:
> - new patch in the series
>
> tmp: i2c drop fields
> ---
>  .../Documentation/devicetree/bindings/media/rockchip-isp1.yaml | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Acked-by: Rob Herring <robh@kernel.org>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 4/9] media: staging: dt-bindings: rkisp1: drop parent unit address
  2020-07-22 15:55   ` [PATCH v5 4/9] media: staging: dt-bindings: rkisp1: drop parent unit address Helen Koike
@ 2020-09-18 14:04     ` Rob Herring
  0 siblings, 0 replies; 23+ messages in thread
From: Rob Herring @ 2020-09-18 14:04 UTC (permalink / raw)
  To: Helen Koike
  Cc: open list:STAGING SUBSYSTEM, devicetree, Eddie Cai, Tomasz Figa,
	Dafna Hirschfeld, heiko, Shunqian Zheng, linux-kernel,
	karthik.poduval, open list:ARM/Rockchip SoC...,
	Hans Verkuil, Robin Murphy, Mark Rutland, Collabora Kernel ML,
	Ezequiel Garcia, Johan Jonker, Linux Media Mailing List

On Wed, Jul 22, 2020 at 9:56 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> Fix the following error found with make ARCH=arm64 dt_binding_check:
>
> Documentation/devicetree/bindings/media/rockchip-isp1.example.dts:24.27-101.11:
> Warning (unit_address_vs_reg): /example-0/parent@0: node has a unit name, but no reg or ranges property
>
> Reported-by: Johan Jonker <jbx6244@gmail.com>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> ---
> V5:
> - Patch re-written to drop the unit address instead of adding reg
>
> V3:
> - this is a new patch in the series
> ---
>  .../Documentation/devicetree/bindings/media/rockchip-isp1.yaml  | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Rob Herring <robh@kernel.org>

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks
  2020-07-22 15:55 ` [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks Helen Koike
@ 2020-09-22 14:24   ` Rob Herring
  2020-10-14 16:46     ` Helen Koike
  0 siblings, 1 reply; 23+ messages in thread
From: Rob Herring @ 2020-09-22 14:24 UTC (permalink / raw)
  To: Helen Koike
  Cc: open list:STAGING SUBSYSTEM, devicetree, Eddie Cai, Tomasz Figa,
	Dafna Hirschfeld, heiko, Shunqian Zheng, linux-kernel,
	karthik.poduval, open list:ARM/Rockchip SoC...,
	Hans Verkuil, Robin Murphy, Mark Rutland, Collabora Kernel ML,
	Ezequiel Garcia, Johan Jonker, Linux Media Mailing List

On Wed, Jul 22, 2020 at 9:56 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> aclk_isp_wrap is a child of aclk_isp, and hclk_isp_wrap is a child of
> hclk_isp, thus we can remove parents from the list.
>
> Also, for the isp0, we only need the ISP clock, ACLK and HCLK.
> In the future we'll need a pixel clock for RK3288 and RK3399, and a JPEG
> clock for RK3288.
>
> So with the goal to cleanup the dt-bindings and remove it from staging,
> simplify clock names to isp, aclk and hclk.
>
> Assigned clocks are meant to refer to the full path in the clock tree,
> i.e. the leaf in the tree.
> For instance, in RK3399, the clock responsible for ACLK (ISP AXI CLOCK)
> is aclk_isp0_wrapper.
>
> For reference, this is the isp clock topology on RK3399:
>
>  xin24m
>     pll_npll
>        npll
>           clk_isp1
>           clk_isp0
>     pll_cpll
>        cpll
>           aclk_isp1
>              aclk_isp1_noc
>              hclk_isp1
>                 aclk_isp1_wrapper
>                 hclk_isp1_noc
>           aclk_isp0
>              hclk_isp1_wrapper
>              aclk_isp0_wrapper
>              aclk_isp0_noc
>              hclk_isp0
>                 hclk_isp0_wrapper
>                 hclk_isp0_noc
>  pclkin_isp1_wrapper
>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>
> ---
> Changes in V5:
> - Use if/then schema as suggested by Rob Herring on
> https://patchwork.linuxtv.org/project/linux-media/patch/20200702191322.2639681-6-helen.koike@collabora.com/#119729
>
> Changes in V4:
> - update binding according to suggestion by Robin Murphy
> on https://patchwork.kernel.org/patch/11475007/
>
> Changes in V3:
> - this is a new patch in the series
> ---
>  .../bindings/media/rockchip-isp1.yaml         | 50 ++++++++++++-------
>  drivers/staging/media/rkisp1/rkisp1-dev.c     |  8 ++-
>  2 files changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> index 62a6b9c959498..23c677d15037a 100644
> --- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> +++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> @@ -24,20 +24,10 @@ properties:
>      maxItems: 1
>
>    clocks:
> -    items:
> -      - description: ISP clock
> -      - description: ISP AXI clock clock
> -      - description: ISP AXI clock  wrapper clock
> -      - description: ISP AHB clock clock
> -      - description: ISP AHB wrapper clock
> +    minItems: 3

You need maxItems here too or it will always be 3.

>
>    clock-names:
> -    items:
> -      - const: clk_isp
> -      - const: aclk_isp
> -      - const: aclk_isp_wrap
> -      - const: hclk_isp
> -      - const: hclk_isp_wrap
> +    minItems: 3
>
>    iommus:
>      maxItems: 1
> @@ -116,6 +106,34 @@ required:
>    - power-domains
>    - ports
>
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        const: rockchip,rk3399-cif-isp
> +then:
> +  properties:
> +    clocks:
> +      maxItems: 4
> +      minItems: 3

For a single compatible you shouldn't really have a variable number of clocks.

> +      items:
> +        # isp0 and isp1
> +        - description: ISP clock
> +        - description: ISP AXI clock
> +        - description: ISP AHB clock
> +        # only for isp1
> +        - description: ISP Pixel clock
> +    clock-names:
> +      maxItems: 4
> +      minItems: 3
> +      items:
> +        # isp0 and isp1
> +        - const: isp
> +        - const: aclk
> +        - const: hclk
> +        # only for isp1
> +        - const: pclk_isp

Don't you need an 'else' clause. For not rockchip,rk3399-cif-isp,
there's no definition of what clocks there are.

Rob

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399
  2020-07-22 15:55 ` [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399 Helen Koike
@ 2020-09-26 13:00   ` Tomasz Figa
  2020-10-14 16:27     ` Helen Koike
  0 siblings, 1 reply; 23+ messages in thread
From: Tomasz Figa @ 2020-09-26 13:00 UTC (permalink / raw)
  To: Helen Koike
  Cc: devel, devicetree, eddie.cai.linux, dafna.hirschfeld, heiko,
	zhengsq, linux-kernel, karthik.poduval, linux-rockchip, robh+dt,
	hverkuil-cisco, robin.murphy, mark.rutland, kernel, ezequiel,
	jbx6244, linux-media

Hi Helen,

On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote:
> From: Shunqian Zheng <zhengsq@rock-chips.com>
> 
> RK3399 has two ISPs, but only isp0 was tested.
> Add isp0 node in rk3399 dtsi
> 
> Verified with:
> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> 
> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> 
> ---
> 
> V4:
> - update clock names
> 
> V3:
> - clean up clocks
> 
> V2:
> - re-order power-domains property
> 
> V1:
> This patch was originally part of this patchset:
> 
>     https://patchwork.kernel.org/patch/10267431/
> 
> The only difference is:
> - add phy properties
> - add ports
> ---
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> index dba9641947a3a..ed8ba75dbbce8 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 {
>  		status = "disabled";
>  	};
>  
> +	isp0: isp0@ff910000 {
> +		compatible = "rockchip,rk3399-cif-isp";
> +		reg = <0x0 0xff910000 0x0 0x4000>;
> +		interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>;
> +		clocks = <&cru SCLK_ISP0>,
> +			 <&cru ACLK_ISP0_WRAPPER>,
> +			 <&cru HCLK_ISP0_WRAPPER>;
> +		clock-names = "isp", "aclk", "hclk";
> +		iommus = <&isp0_mmu>;
> +		phys = <&mipi_dphy_rx0>;
> +		phy-names = "dphy";
> +		power-domains = <&power RK3399_PD_ISP0>;

Should this have status = "disabled" too? The mipi_dphy_rx0 node is
disabled by default too, so in the default configuration the driver
would always fail to probe.

Best regards,
Tomasz

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399
  2020-07-22 15:55 [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Helen Koike
                   ` (7 preceding siblings ...)
  2020-07-22 15:55 ` [PATCH v5 9/9] arm64: dts: rockchip: add isp and sensors for Scarlet Helen Koike
@ 2020-09-26 13:03 ` Tomasz Figa
  8 siblings, 0 replies; 23+ messages in thread
From: Tomasz Figa @ 2020-09-26 13:03 UTC (permalink / raw)
  To: Helen Koike
  Cc: devel, devicetree, eddie.cai.linux, dafna.hirschfeld, heiko,
	zhengsq, linux-kernel, karthik.poduval, linux-rockchip, robh+dt,
	hverkuil-cisco, robin.murphy, mark.rutland, kernel, ezequiel,
	jbx6244, linux-media

Hi Helen,

On Wed, Jul 22, 2020 at 12:55:24PM -0300, Helen Koike wrote:
> Move the bindings out of drivers/staging and place them in
> Documentation/devicetree/bindings instead.
> 
> Also, add DT nodes for RK3399 and verify with make ARCH=arm64 dtbs_check
> and make ARCH=arm64 dt_binding_check.
> 
> Tested by verifying images streamed from Scarlet Chromebook
> 
> Changes in v5:
> - Drop unit addresses in dt-bindings example for simplification and fix
> errors as suggested by Rob Herring in previous version
> - Fix typos
> - Re-write clock organization with if/then schema
>

Besides one comment to patch 8/9,

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399
  2020-09-26 13:00   ` Tomasz Figa
@ 2020-10-14 16:27     ` Helen Koike
  2020-10-14 16:42       ` Robin Murphy
  2020-10-14 16:43       ` Tomasz Figa
  0 siblings, 2 replies; 23+ messages in thread
From: Helen Koike @ 2020-10-14 16:27 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: devel, devicetree, eddie.cai.linux, dafna.hirschfeld, heiko,
	zhengsq, linux-kernel, karthik.poduval, linux-rockchip, robh+dt,
	hverkuil-cisco, robin.murphy, mark.rutland, kernel, ezequiel,
	jbx6244, linux-media

Hi Tomasz,

On 9/26/20 10:00 AM, Tomasz Figa wrote:
> Hi Helen,
> 
> On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote:
>> From: Shunqian Zheng <zhengsq@rock-chips.com>
>>
>> RK3399 has two ISPs, but only isp0 was tested.
>> Add isp0 node in rk3399 dtsi
>>
>> Verified with:
>> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>>
>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
>> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>>
>> V4:
>> - update clock names
>>
>> V3:
>> - clean up clocks
>>
>> V2:
>> - re-order power-domains property
>>
>> V1:
>> This patch was originally part of this patchset:
>>
>>     https://patchwork.kernel.org/patch/10267431/
>>
>> The only difference is:
>> - add phy properties
>> - add ports
>> ---
>>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
>>  1 file changed, 25 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>> index dba9641947a3a..ed8ba75dbbce8 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 {
>>  		status = "disabled";
>>  	};
>>  
>> +	isp0: isp0@ff910000 {
>> +		compatible = "rockchip,rk3399-cif-isp";
>> +		reg = <0x0 0xff910000 0x0 0x4000>;
>> +		interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>;
>> +		clocks = <&cru SCLK_ISP0>,
>> +			 <&cru ACLK_ISP0_WRAPPER>,
>> +			 <&cru HCLK_ISP0_WRAPPER>;
>> +		clock-names = "isp", "aclk", "hclk";
>> +		iommus = <&isp0_mmu>;
>> +		phys = <&mipi_dphy_rx0>;
>> +		phy-names = "dphy";
>> +		power-domains = <&power RK3399_PD_ISP0>;
> 
> Should this have status = "disabled" too? The mipi_dphy_rx0 node is
> disabled by default too, so in the default configuration the driver
> would always fail to probe.

I'm thinking what is the overall guideline here.
Since isp and mipi_dphy are always present in the rk3399, shouldn't they always be enabled?
Or since they are only useful if a sensor is present, we should let the dts of the board to
enable it?

Thanks
Helen

> 
> Best regards,
> Tomasz
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399
  2020-10-14 16:27     ` Helen Koike
@ 2020-10-14 16:42       ` Robin Murphy
  2020-10-14 16:43       ` Tomasz Figa
  1 sibling, 0 replies; 23+ messages in thread
From: Robin Murphy @ 2020-10-14 16:42 UTC (permalink / raw)
  To: Helen Koike, Tomasz Figa
  Cc: devel, devicetree, eddie.cai.linux, dafna.hirschfeld, heiko,
	zhengsq, linux-kernel, karthik.poduval, linux-rockchip, robh+dt,
	hverkuil-cisco, mark.rutland, kernel, ezequiel, jbx6244,
	linux-media

On 2020-10-14 17:27, Helen Koike wrote:
> Hi Tomasz,
> 
> On 9/26/20 10:00 AM, Tomasz Figa wrote:
>> Hi Helen,
>>
>> On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote:
>>> From: Shunqian Zheng <zhengsq@rock-chips.com>
>>>
>>> RK3399 has two ISPs, but only isp0 was tested.
>>> Add isp0 node in rk3399 dtsi
>>>
>>> Verified with:
>>> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>>>
>>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
>>> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>
>>> ---
>>>
>>> V4:
>>> - update clock names
>>>
>>> V3:
>>> - clean up clocks
>>>
>>> V2:
>>> - re-order power-domains property
>>>
>>> V1:
>>> This patch was originally part of this patchset:
>>>
>>>      https://patchwork.kernel.org/patch/10267431/
>>>
>>> The only difference is:
>>> - add phy properties
>>> - add ports
>>> ---
>>>   arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
>>>   1 file changed, 25 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>>> index dba9641947a3a..ed8ba75dbbce8 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>>> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 {
>>>   		status = "disabled";
>>>   	};
>>>   
>>> +	isp0: isp0@ff910000 {
>>> +		compatible = "rockchip,rk3399-cif-isp";
>>> +		reg = <0x0 0xff910000 0x0 0x4000>;
>>> +		interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>;
>>> +		clocks = <&cru SCLK_ISP0>,
>>> +			 <&cru ACLK_ISP0_WRAPPER>,
>>> +			 <&cru HCLK_ISP0_WRAPPER>;
>>> +		clock-names = "isp", "aclk", "hclk";
>>> +		iommus = <&isp0_mmu>;
>>> +		phys = <&mipi_dphy_rx0>;
>>> +		phy-names = "dphy";
>>> +		power-domains = <&power RK3399_PD_ISP0>;
>>
>> Should this have status = "disabled" too? The mipi_dphy_rx0 node is
>> disabled by default too, so in the default configuration the driver
>> would always fail to probe.
> 
> I'm thinking what is the overall guideline here.
> Since isp and mipi_dphy are always present in the rk3399, shouldn't they always be enabled?
> Or since they are only useful if a sensor is present, we should let the dts of the board to
> enable it?

Yes, the usual pattern is that anything which needs additional hardware 
outside the SoC to be useful is disabled by default in the SoC DTSI, and 
enabled in individual board DTSs as appropriate. See USB, HDMI, etc. for 
instance.

There's probably a further debate about how much the board itself should 
enable if it only breaks out a connector for the user to add their own 
camera module, but hey, one step at a time ;)

Robin.

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399
  2020-10-14 16:27     ` Helen Koike
  2020-10-14 16:42       ` Robin Murphy
@ 2020-10-14 16:43       ` Tomasz Figa
  2020-10-14 16:52         ` Helen Koike
  1 sibling, 1 reply; 23+ messages in thread
From: Tomasz Figa @ 2020-10-14 16:43 UTC (permalink / raw)
  To: Helen Koike
  Cc: devel, linux-devicetree, Eddie Cai, Dafna Hirschfeld,
	Heiko Stübner, Shunqian Zheng, Linux Kernel Mailing List,
	karthik.poduval, open list:ARM/Rockchip SoC...,
	Rob Herring, Hans Verkuil, Robin Murphy, Mark Rutland, kernel,
	Ezequiel Garcia, jbx6244, Linux Media Mailing List

On Wed, Oct 14, 2020 at 6:27 PM Helen Koike <helen.koike@collabora.com> wrote:
>
> Hi Tomasz,
>
> On 9/26/20 10:00 AM, Tomasz Figa wrote:
> > Hi Helen,
> >
> > On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote:
> >> From: Shunqian Zheng <zhengsq@rock-chips.com>
> >>
> >> RK3399 has two ISPs, but only isp0 was tested.
> >> Add isp0 node in rk3399 dtsi
> >>
> >> Verified with:
> >> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> >>
> >> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
> >> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >>
> >> V4:
> >> - update clock names
> >>
> >> V3:
> >> - clean up clocks
> >>
> >> V2:
> >> - re-order power-domains property
> >>
> >> V1:
> >> This patch was originally part of this patchset:
> >>
> >>     https://patchwork.kernel.org/patch/10267431/
> >>
> >> The only difference is:
> >> - add phy properties
> >> - add ports
> >> ---
> >>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
> >>  1 file changed, 25 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> >> index dba9641947a3a..ed8ba75dbbce8 100644
> >> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> >> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
> >> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 {
> >>              status = "disabled";
> >>      };
> >>
> >> +    isp0: isp0@ff910000 {
> >> +            compatible = "rockchip,rk3399-cif-isp";
> >> +            reg = <0x0 0xff910000 0x0 0x4000>;
> >> +            interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>;
> >> +            clocks = <&cru SCLK_ISP0>,
> >> +                     <&cru ACLK_ISP0_WRAPPER>,
> >> +                     <&cru HCLK_ISP0_WRAPPER>;
> >> +            clock-names = "isp", "aclk", "hclk";
> >> +            iommus = <&isp0_mmu>;
> >> +            phys = <&mipi_dphy_rx0>;
> >> +            phy-names = "dphy";
> >> +            power-domains = <&power RK3399_PD_ISP0>;
> >
> > Should this have status = "disabled" too? The mipi_dphy_rx0 node is
> > disabled by default too, so in the default configuration the driver
> > would always fail to probe.
>
> I'm thinking what is the overall guideline here.
> Since isp and mipi_dphy are always present in the rk3399, shouldn't they always be enabled?
> Or since they are only useful if a sensor is present, we should let the dts of the board to
> enable it?

I don't have a strong opinion. I'm fine with enabling both by default
as well, as it shouldn't hurt.

That said, I recall some alternative CIF IP block being present on
this SoC as well (and patches posted recently), which AFAIR can't be
activated at the same time as the ISP, so perhaps both of the
alternatives should be disabled by default?

Best regards,
Tomasz

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks
  2020-09-22 14:24   ` Rob Herring
@ 2020-10-14 16:46     ` Helen Koike
  2020-10-20 15:14       ` Rob Herring
  0 siblings, 1 reply; 23+ messages in thread
From: Helen Koike @ 2020-10-14 16:46 UTC (permalink / raw)
  To: Rob Herring
  Cc: open list:STAGING SUBSYSTEM, devicetree, Eddie Cai, Tomasz Figa,
	Dafna Hirschfeld, heiko, Shunqian Zheng, linux-kernel,
	karthik.poduval, open list:ARM/Rockchip SoC...,
	Hans Verkuil, Robin Murphy, Mark Rutland, Collabora Kernel ML,
	Ezequiel Garcia, Johan Jonker, Linux Media Mailing List

Hi Rob,

Thnaks for your reply.

On 9/22/20 11:24 AM, Rob Herring wrote:
> On Wed, Jul 22, 2020 at 9:56 AM Helen Koike <helen.koike@collabora.com> wrote:
>>
>> aclk_isp_wrap is a child of aclk_isp, and hclk_isp_wrap is a child of
>> hclk_isp, thus we can remove parents from the list.
>>
>> Also, for the isp0, we only need the ISP clock, ACLK and HCLK.
>> In the future we'll need a pixel clock for RK3288 and RK3399, and a JPEG
>> clock for RK3288.
>>
>> So with the goal to cleanup the dt-bindings and remove it from staging,
>> simplify clock names to isp, aclk and hclk.
>>
>> Assigned clocks are meant to refer to the full path in the clock tree,
>> i.e. the leaf in the tree.
>> For instance, in RK3399, the clock responsible for ACLK (ISP AXI CLOCK)
>> is aclk_isp0_wrapper.
>>
>> For reference, this is the isp clock topology on RK3399:
>>
>>  xin24m
>>     pll_npll
>>        npll
>>           clk_isp1
>>           clk_isp0
>>     pll_cpll
>>        cpll
>>           aclk_isp1
>>              aclk_isp1_noc
>>              hclk_isp1
>>                 aclk_isp1_wrapper
>>                 hclk_isp1_noc
>>           aclk_isp0
>>              hclk_isp1_wrapper
>>              aclk_isp0_wrapper
>>              aclk_isp0_noc
>>              hclk_isp0
>>                 hclk_isp0_wrapper
>>                 hclk_isp0_noc
>>  pclkin_isp1_wrapper
>>
>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>
>> ---
>> Changes in V5:
>> - Use if/then schema as suggested by Rob Herring on
>> https://patchwork.linuxtv.org/project/linux-media/patch/20200702191322.2639681-6-helen.koike@collabora.com/#119729
>>
>> Changes in V4:
>> - update binding according to suggestion by Robin Murphy
>> on https://patchwork.kernel.org/patch/11475007/
>>
>> Changes in V3:
>> - this is a new patch in the series
>> ---
>>  .../bindings/media/rockchip-isp1.yaml         | 50 ++++++++++++-------
>>  drivers/staging/media/rkisp1/rkisp1-dev.c     |  8 ++-
>>  2 files changed, 36 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>> index 62a6b9c959498..23c677d15037a 100644
>> --- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>> +++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>> @@ -24,20 +24,10 @@ properties:
>>      maxItems: 1
>>
>>    clocks:
>> -    items:
>> -      - description: ISP clock
>> -      - description: ISP AXI clock clock
>> -      - description: ISP AXI clock  wrapper clock
>> -      - description: ISP AHB clock clock
>> -      - description: ISP AHB wrapper clock
>> +    minItems: 3
> 
> You need maxItems here too or it will always be 3.
> 
>>
>>    clock-names:
>> -    items:
>> -      - const: clk_isp
>> -      - const: aclk_isp
>> -      - const: aclk_isp_wrap
>> -      - const: hclk_isp
>> -      - const: hclk_isp_wrap
>> +    minItems: 3
>>
>>    iommus:
>>      maxItems: 1
>> @@ -116,6 +106,34 @@ required:
>>    - power-domains
>>    - ports
>>
>> +if:
>> +  properties:
>> +    compatible:
>> +      contains:
>> +        const: rockchip,rk3399-cif-isp
>> +then:
>> +  properties:
>> +    clocks:
>> +      maxItems: 4
>> +      minItems: 3
> 
> For a single compatible you shouldn't really have a variable number of clocks.

I'm not entirely sure how to make this separation, since isp0 and isp1 (not yet supported)
would use the same compatible.
Unless if we separate in two compatibles, but maybe this is an overhead just for an extra clock.
What do you think?

> 
>> +      items:
>> +        # isp0 and isp1
>> +        - description: ISP clock
>> +        - description: ISP AXI clock
>> +        - description: ISP AHB clock
>> +        # only for isp1
>> +        - description: ISP Pixel clock
>> +    clock-names:
>> +      maxItems: 4
>> +      minItems: 3
>> +      items:
>> +        # isp0 and isp1
>> +        - const: isp
>> +        - const: aclk
>> +        - const: hclk
>> +        # only for isp1
>> +        - const: pclk_isp
> 
> Don't you need an 'else' clause. For not rockchip,rk3399-cif-isp,
> there's no definition of what clocks there are.

There is only one compatible defined for now, rk3288 will be added later.
The idea to add if/then is to make it easier to add rk3288:

https://patchwork.kernel.org/project/linux-media/patch/20200406073017.19462-4-karthik.poduval@gmail.com/

Regards,
Helen

> 
> Rob
> 

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399
  2020-10-14 16:43       ` Tomasz Figa
@ 2020-10-14 16:52         ` Helen Koike
  0 siblings, 0 replies; 23+ messages in thread
From: Helen Koike @ 2020-10-14 16:52 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: devel, linux-devicetree, Eddie Cai, Dafna Hirschfeld,
	Heiko Stübner, Shunqian Zheng, Linux Kernel Mailing List,
	karthik.poduval, open list:ARM/Rockchip SoC...,
	Rob Herring, Hans Verkuil, Robin Murphy, Mark Rutland, kernel,
	Ezequiel Garcia, jbx6244, Linux Media Mailing List

Thank you Tomasz and Robin for your comments,

On 10/14/20 1:43 PM, Tomasz Figa wrote:
> On Wed, Oct 14, 2020 at 6:27 PM Helen Koike <helen.koike@collabora.com> wrote:
>>
>> Hi Tomasz,
>>
>> On 9/26/20 10:00 AM, Tomasz Figa wrote:
>>> Hi Helen,
>>>
>>> On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote:
>>>> From: Shunqian Zheng <zhengsq@rock-chips.com>
>>>>
>>>> RK3399 has two ISPs, but only isp0 was tested.
>>>> Add isp0 node in rk3399 dtsi
>>>>
>>>> Verified with:
>>>> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>>>>
>>>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com>
>>>> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com>
>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>
>>>> ---
>>>>
>>>> V4:
>>>> - update clock names
>>>>
>>>> V3:
>>>> - clean up clocks
>>>>
>>>> V2:
>>>> - re-order power-domains property
>>>>
>>>> V1:
>>>> This patch was originally part of this patchset:
>>>>
>>>>     https://patchwork.kernel.org/patch/10267431/
>>>>
>>>> The only difference is:
>>>> - add phy properties
>>>> - add ports
>>>> ---
>>>>  arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++
>>>>  1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>>>> index dba9641947a3a..ed8ba75dbbce8 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
>>>> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 {
>>>>              status = "disabled";
>>>>      };
>>>>
>>>> +    isp0: isp0@ff910000 {
>>>> +            compatible = "rockchip,rk3399-cif-isp";
>>>> +            reg = <0x0 0xff910000 0x0 0x4000>;
>>>> +            interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>;
>>>> +            clocks = <&cru SCLK_ISP0>,
>>>> +                     <&cru ACLK_ISP0_WRAPPER>,
>>>> +                     <&cru HCLK_ISP0_WRAPPER>;
>>>> +            clock-names = "isp", "aclk", "hclk";
>>>> +            iommus = <&isp0_mmu>;
>>>> +            phys = <&mipi_dphy_rx0>;
>>>> +            phy-names = "dphy";
>>>> +            power-domains = <&power RK3399_PD_ISP0>;
>>>
>>> Should this have status = "disabled" too? The mipi_dphy_rx0 node is
>>> disabled by default too, so in the default configuration the driver
>>> would always fail to probe.
>>
>> I'm thinking what is the overall guideline here.
>> Since isp and mipi_dphy are always present in the rk3399, shouldn't they always be enabled?
>> Or since they are only useful if a sensor is present, we should let the dts of the board to
>> enable it?
> 
> I don't have a strong opinion. I'm fine with enabling both by default
> as well, as it shouldn't hurt.
> 
> That said, I recall some alternative CIF IP block being present on
> this SoC as well (and patches posted recently), which AFAIR can't be
> activated at the same time as the ISP, so perhaps both of the
> alternatives should be disabled by default?
> 
> Best regards,
> Tomasz
> 

Based on these two last emails, I think it make sense to disable them by default.

I'll just wait for feedback on patch 5/9 to submit an updated version.

Thanks
Helen

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks
  2020-10-14 16:46     ` Helen Koike
@ 2020-10-20 15:14       ` Rob Herring
  2020-10-21 13:54         ` Helen Koike
  0 siblings, 1 reply; 23+ messages in thread
From: Rob Herring @ 2020-10-20 15:14 UTC (permalink / raw)
  To: Helen Koike
  Cc: open list:STAGING SUBSYSTEM, devicetree, Eddie Cai, Tomasz Figa,
	Dafna Hirschfeld, heiko, Shunqian Zheng, linux-kernel,
	karthik.poduval, open list:ARM/Rockchip SoC...,
	Hans Verkuil, Robin Murphy, Mark Rutland, Collabora Kernel ML,
	Ezequiel Garcia, Johan Jonker, Linux Media Mailing List

On Wed, Oct 14, 2020 at 11:46 AM Helen Koike <helen.koike@collabora.com> wrote:
>
> Hi Rob,
>
> Thnaks for your reply.
>
> On 9/22/20 11:24 AM, Rob Herring wrote:
> > On Wed, Jul 22, 2020 at 9:56 AM Helen Koike <helen.koike@collabora.com> wrote:
> >>
> >> aclk_isp_wrap is a child of aclk_isp, and hclk_isp_wrap is a child of
> >> hclk_isp, thus we can remove parents from the list.
> >>
> >> Also, for the isp0, we only need the ISP clock, ACLK and HCLK.
> >> In the future we'll need a pixel clock for RK3288 and RK3399, and a JPEG
> >> clock for RK3288.
> >>
> >> So with the goal to cleanup the dt-bindings and remove it from staging,
> >> simplify clock names to isp, aclk and hclk.
> >>
> >> Assigned clocks are meant to refer to the full path in the clock tree,
> >> i.e. the leaf in the tree.
> >> For instance, in RK3399, the clock responsible for ACLK (ISP AXI CLOCK)
> >> is aclk_isp0_wrapper.
> >>
> >> For reference, this is the isp clock topology on RK3399:
> >>
> >>  xin24m
> >>     pll_npll
> >>        npll
> >>           clk_isp1
> >>           clk_isp0
> >>     pll_cpll
> >>        cpll
> >>           aclk_isp1
> >>              aclk_isp1_noc
> >>              hclk_isp1
> >>                 aclk_isp1_wrapper
> >>                 hclk_isp1_noc
> >>           aclk_isp0
> >>              hclk_isp1_wrapper
> >>              aclk_isp0_wrapper
> >>              aclk_isp0_noc
> >>              hclk_isp0
> >>                 hclk_isp0_wrapper
> >>                 hclk_isp0_noc
> >>  pclkin_isp1_wrapper
> >>
> >> Signed-off-by: Helen Koike <helen.koike@collabora.com>
> >>
> >> ---
> >> Changes in V5:
> >> - Use if/then schema as suggested by Rob Herring on
> >> https://patchwork.linuxtv.org/project/linux-media/patch/20200702191322.2639681-6-helen.koike@collabora.com/#119729
> >>
> >> Changes in V4:
> >> - update binding according to suggestion by Robin Murphy
> >> on https://patchwork.kernel.org/patch/11475007/
> >>
> >> Changes in V3:
> >> - this is a new patch in the series
> >> ---
> >>  .../bindings/media/rockchip-isp1.yaml         | 50 ++++++++++++-------
> >>  drivers/staging/media/rkisp1/rkisp1-dev.c     |  8 ++-
> >>  2 files changed, 36 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> >> index 62a6b9c959498..23c677d15037a 100644
> >> --- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> >> +++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> >> @@ -24,20 +24,10 @@ properties:
> >>      maxItems: 1
> >>
> >>    clocks:
> >> -    items:
> >> -      - description: ISP clock
> >> -      - description: ISP AXI clock clock
> >> -      - description: ISP AXI clock  wrapper clock
> >> -      - description: ISP AHB clock clock
> >> -      - description: ISP AHB wrapper clock
> >> +    minItems: 3
> >
> > You need maxItems here too or it will always be 3.
> >
> >>
> >>    clock-names:
> >> -    items:
> >> -      - const: clk_isp
> >> -      - const: aclk_isp
> >> -      - const: aclk_isp_wrap
> >> -      - const: hclk_isp
> >> -      - const: hclk_isp_wrap
> >> +    minItems: 3
> >>
> >>    iommus:
> >>      maxItems: 1
> >> @@ -116,6 +106,34 @@ required:
> >>    - power-domains
> >>    - ports
> >>
> >> +if:
> >> +  properties:
> >> +    compatible:
> >> +      contains:
> >> +        const: rockchip,rk3399-cif-isp
> >> +then:
> >> +  properties:
> >> +    clocks:
> >> +      maxItems: 4
> >> +      minItems: 3
> >
> > For a single compatible you shouldn't really have a variable number of clocks.
>
> I'm not entirely sure how to make this separation, since isp0 and isp1 (not yet supported)
> would use the same compatible.
> Unless if we separate in two compatibles, but maybe this is an overhead just for an extra clock.
> What do you think?

In that case, it's fine.

>
> >
> >> +      items:
> >> +        # isp0 and isp1
> >> +        - description: ISP clock
> >> +        - description: ISP AXI clock
> >> +        - description: ISP AHB clock
> >> +        # only for isp1
> >> +        - description: ISP Pixel clock
> >> +    clock-names:
> >> +      maxItems: 4
> >> +      minItems: 3
> >> +      items:
> >> +        # isp0 and isp1
> >> +        - const: isp
> >> +        - const: aclk
> >> +        - const: hclk
> >> +        # only for isp1
> >> +        - const: pclk_isp
> >
> > Don't you need an 'else' clause. For not rockchip,rk3399-cif-isp,
> > there's no definition of what clocks there are.
>
> There is only one compatible defined for now, rk3288 will be added later.
> The idea to add if/then is to make it easier to add rk3288:
>
> https://patchwork.kernel.org/project/linux-media/patch/20200406073017.19462-4-karthik.poduval@gmail.com/

Hopefully, the clock names will be aligned? Looks like they are the
same with just 1 additional clock. Ideally, you define them all at the
top level and the if/then schema just defines how many clocks for each
compatible.

Rob

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks
  2020-10-20 15:14       ` Rob Herring
@ 2020-10-21 13:54         ` Helen Koike
  0 siblings, 0 replies; 23+ messages in thread
From: Helen Koike @ 2020-10-21 13:54 UTC (permalink / raw)
  To: Rob Herring
  Cc: open list:STAGING SUBSYSTEM, devicetree, Eddie Cai, Tomasz Figa,
	Dafna Hirschfeld, heiko, Shunqian Zheng, linux-kernel,
	karthik.poduval, open list:ARM/Rockchip SoC...,
	Hans Verkuil, Robin Murphy, Mark Rutland, Collabora Kernel ML,
	Ezequiel Garcia, Johan Jonker, Linux Media Mailing List

Hi Rob,

On 10/20/20 12:14 PM, Rob Herring wrote:
> On Wed, Oct 14, 2020 at 11:46 AM Helen Koike <helen.koike@collabora.com> wrote:
>>
>> Hi Rob,
>>
>> Thnaks for your reply.
>>
>> On 9/22/20 11:24 AM, Rob Herring wrote:
>>> On Wed, Jul 22, 2020 at 9:56 AM Helen Koike <helen.koike@collabora.com> wrote:
>>>>
>>>> aclk_isp_wrap is a child of aclk_isp, and hclk_isp_wrap is a child of
>>>> hclk_isp, thus we can remove parents from the list.
>>>>
>>>> Also, for the isp0, we only need the ISP clock, ACLK and HCLK.
>>>> In the future we'll need a pixel clock for RK3288 and RK3399, and a JPEG
>>>> clock for RK3288.
>>>>
>>>> So with the goal to cleanup the dt-bindings and remove it from staging,
>>>> simplify clock names to isp, aclk and hclk.
>>>>
>>>> Assigned clocks are meant to refer to the full path in the clock tree,
>>>> i.e. the leaf in the tree.
>>>> For instance, in RK3399, the clock responsible for ACLK (ISP AXI CLOCK)
>>>> is aclk_isp0_wrapper.
>>>>
>>>> For reference, this is the isp clock topology on RK3399:
>>>>
>>>>  xin24m
>>>>     pll_npll
>>>>        npll
>>>>           clk_isp1
>>>>           clk_isp0
>>>>     pll_cpll
>>>>        cpll
>>>>           aclk_isp1
>>>>              aclk_isp1_noc
>>>>              hclk_isp1
>>>>                 aclk_isp1_wrapper
>>>>                 hclk_isp1_noc
>>>>           aclk_isp0
>>>>              hclk_isp1_wrapper
>>>>              aclk_isp0_wrapper
>>>>              aclk_isp0_noc
>>>>              hclk_isp0
>>>>                 hclk_isp0_wrapper
>>>>                 hclk_isp0_noc
>>>>  pclkin_isp1_wrapper
>>>>
>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com>
>>>>
>>>> ---
>>>> Changes in V5:
>>>> - Use if/then schema as suggested by Rob Herring on
>>>> https://patchwork.linuxtv.org/project/linux-media/patch/20200702191322.2639681-6-helen.koike@collabora.com/#119729
>>>>
>>>> Changes in V4:
>>>> - update binding according to suggestion by Robin Murphy
>>>> on https://patchwork.kernel.org/patch/11475007/
>>>>
>>>> Changes in V3:
>>>> - this is a new patch in the series
>>>> ---
>>>>  .../bindings/media/rockchip-isp1.yaml         | 50 ++++++++++++-------
>>>>  drivers/staging/media/rkisp1/rkisp1-dev.c     |  8 ++-
>>>>  2 files changed, 36 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>>>> index 62a6b9c959498..23c677d15037a 100644
>>>> --- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>>>> +++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
>>>> @@ -24,20 +24,10 @@ properties:
>>>>      maxItems: 1
>>>>
>>>>    clocks:
>>>> -    items:
>>>> -      - description: ISP clock
>>>> -      - description: ISP AXI clock clock
>>>> -      - description: ISP AXI clock  wrapper clock
>>>> -      - description: ISP AHB clock clock
>>>> -      - description: ISP AHB wrapper clock
>>>> +    minItems: 3
>>>
>>> You need maxItems here too or it will always be 3.
>>>
>>>>
>>>>    clock-names:
>>>> -    items:
>>>> -      - const: clk_isp
>>>> -      - const: aclk_isp
>>>> -      - const: aclk_isp_wrap
>>>> -      - const: hclk_isp
>>>> -      - const: hclk_isp_wrap
>>>> +    minItems: 3
>>>>
>>>>    iommus:
>>>>      maxItems: 1
>>>> @@ -116,6 +106,34 @@ required:
>>>>    - power-domains
>>>>    - ports
>>>>
>>>> +if:
>>>> +  properties:
>>>> +    compatible:
>>>> +      contains:
>>>> +        const: rockchip,rk3399-cif-isp
>>>> +then:
>>>> +  properties:
>>>> +    clocks:
>>>> +      maxItems: 4
>>>> +      minItems: 3
>>>
>>> For a single compatible you shouldn't really have a variable number of clocks.
>>
>> I'm not entirely sure how to make this separation, since isp0 and isp1 (not yet supported)
>> would use the same compatible.
>> Unless if we separate in two compatibles, but maybe this is an overhead just for an extra clock.
>> What do you think?
> 
> In that case, it's fine.
> 
>>
>>>
>>>> +      items:
>>>> +        # isp0 and isp1
>>>> +        - description: ISP clock
>>>> +        - description: ISP AXI clock
>>>> +        - description: ISP AHB clock
>>>> +        # only for isp1
>>>> +        - description: ISP Pixel clock
>>>> +    clock-names:
>>>> +      maxItems: 4
>>>> +      minItems: 3
>>>> +      items:
>>>> +        # isp0 and isp1
>>>> +        - const: isp
>>>> +        - const: aclk
>>>> +        - const: hclk
>>>> +        # only for isp1
>>>> +        - const: pclk_isp
>>>
>>> Don't you need an 'else' clause. For not rockchip,rk3399-cif-isp,
>>> there's no definition of what clocks there are.
>>
>> There is only one compatible defined for now, rk3288 will be added later.
>> The idea to add if/then is to make it easier to add rk3288:
>>
>> https://patchwork.kernel.org/project/linux-media/patch/20200406073017.19462-4-karthik.poduval@gmail.com/
> 
> Hopefully, the clock names will be aligned? Looks like they are the
> same with just 1 additional clock. Ideally, you define them all at the
> top level and the if/then schema just defines how many clocks for each
> compatible.

I submitted another version, where I try to capture what you suggested here,
please check if I got it right this time (or not).
Maybe I misunderstood which kind of alignment you are expecting for the clock names,
should they be each in a different line?

https://patchwork.linuxtv.org/project/linux-media/patch/20201020193850.1460644-6-helen.koike@collabora.com/

Thanks
Helen

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2020-10-21 13:54 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-22 15:55 [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Helen Koike
2020-07-22 15:55 ` [PATCH v5 2/9] media: staging: dt-bindings: rkisp1: drop i2c unit address Helen Koike
2020-09-18 14:03   ` Rob Herring
2020-07-22 15:55 ` [PATCH v5 3/9] media: staging: dt-bindings: rkisp1: re-order properties Helen Koike
     [not found] ` <20200722155533.252844-1-helen.koike-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
2020-07-22 15:55   ` [PATCH v5 1/9] media: staging: dt-bindings: rkisp1: add missing required nodes Helen Koike
2020-09-18 14:02     ` Rob Herring
2020-07-22 15:55   ` [PATCH v5 4/9] media: staging: dt-bindings: rkisp1: drop parent unit address Helen Koike
2020-09-18 14:04     ` Rob Herring
2020-07-22 15:55 ` [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks Helen Koike
2020-09-22 14:24   ` Rob Herring
2020-10-14 16:46     ` Helen Koike
2020-10-20 15:14       ` Rob Herring
2020-10-21 13:54         ` Helen Koike
2020-07-22 15:55 ` [PATCH v5 6/9] dt-bindings: media: rkisp1: move rockchip-isp1 bindings out of staging Helen Koike
2020-07-22 15:55 ` [PATCH v5 7/9] media: MAINTAINERS: rkisp1: add path to dt-bindings Helen Koike
2020-07-22 15:55 ` [PATCH v5 8/9] arm64: dts: rockchip: add isp0 node for rk3399 Helen Koike
2020-09-26 13:00   ` Tomasz Figa
2020-10-14 16:27     ` Helen Koike
2020-10-14 16:42       ` Robin Murphy
2020-10-14 16:43       ` Tomasz Figa
2020-10-14 16:52         ` Helen Koike
2020-07-22 15:55 ` [PATCH v5 9/9] arm64: dts: rockchip: add isp and sensors for Scarlet Helen Koike
2020-09-26 13:03 ` [PATCH v5 0/9] move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 Tomasz Figa

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