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
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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
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
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 <[email protected]>
---
V3:
- Add line:
L: [email protected]
- 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 <[email protected]>
L: [email protected]
+L: [email protected]
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
From: Shunqian Zheng <[email protected]>
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 <[email protected]>
Signed-off-by: Jacob Chen <[email protected]>
Signed-off-by: Helen Koike <[email protected]>
---
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
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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
.../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
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 <[email protected]>
---
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
Add missing required nodes in json-schema yaml file for
Rockchip ISP1 dt-bindings.
Signed-off-by: Helen Koike <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Helen Koike <[email protected]>
---
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
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 <[email protected]>
---
Changes in V5:
- Use if/then schema as suggested by Rob Herring on
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/#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
From: Eddie Cai <[email protected]>
Enable ISP and camera sensor ov2685 and ov5695 for Scarlet Chromebook
Verified with:
make ARCH=arm64 dtbs_check
Signed-off-by: Shunqian Zheng <[email protected]>
Signed-off-by: Eddie Cai <[email protected]>
Signed-off-by: Tomasz Figa <[email protected]>
Signed-off-by: Helen Koike <[email protected]>
---
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
On Wed, Jul 22, 2020 at 9:55 AM Helen Koike <[email protected]> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
On Wed, Jul 22, 2020 at 9:55 AM Helen Koike <[email protected]> wrote:
>
> Add missing required nodes in json-schema yaml file for
> Rockchip ISP1 dt-bindings.
>
> Signed-off-by: Helen Koike <[email protected]>
> Acked-by: Rob Herring <[email protected]>
> ---
>
> 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 <[email protected]>
On Wed, Jul 22, 2020 at 9:56 AM Helen Koike <[email protected]> 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 <[email protected]>
> Signed-off-by: Helen Koike <[email protected]>
> ---
> 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 <[email protected]>
On Wed, Jul 22, 2020 at 9:56 AM Helen Koike <[email protected]> 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 <[email protected]>
>
> ---
> Changes in V5:
> - Use if/then schema as suggested by Rob Herring on
> https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/#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
Hi Helen,
On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote:
> From: Shunqian Zheng <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jacob Chen <[email protected]>
> Signed-off-by: Helen Koike <[email protected]>
>
> ---
>
> 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
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 <[email protected]>
Best regards,
Tomasz
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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Jacob Chen <[email protected]>
>> Signed-off-by: Helen Koike <[email protected]>
>>
>> ---
>>
>> 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
>
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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Jacob Chen <[email protected]>
>>> Signed-off-by: Helen Koike <[email protected]>
>>>
>>> ---
>>>
>>> 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.
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 <[email protected]> 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 <[email protected]>
>>
>> ---
>> Changes in V5:
>> - Use if/then schema as suggested by Rob Herring on
>> https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/#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/[email protected]/
Regards,
Helen
>
> Rob
>
On Wed, Oct 14, 2020 at 6:27 PM Helen Koike <[email protected]> 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 <[email protected]>
> >>
> >> 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 <[email protected]>
> >> Signed-off-by: Jacob Chen <[email protected]>
> >> Signed-off-by: Helen Koike <[email protected]>
> >>
> >> ---
> >>
> >> 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
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 <[email protected]> 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 <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Signed-off-by: Jacob Chen <[email protected]>
>>>> Signed-off-by: Helen Koike <[email protected]>
>>>>
>>>> ---
>>>>
>>>> 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
On Wed, Oct 14, 2020 at 11:46 AM Helen Koike <[email protected]> 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 <[email protected]> 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 <[email protected]>
> >>
> >> ---
> >> Changes in V5:
> >> - Use if/then schema as suggested by Rob Herring on
> >> https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/#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/[email protected]/
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
Hi Rob,
On 10/20/20 12:14 PM, Rob Herring wrote:
> On Wed, Oct 14, 2020 at 11:46 AM Helen Koike <[email protected]> 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 <[email protected]> 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 <[email protected]>
>>>>
>>>> ---
>>>> Changes in V5:
>>>> - Use if/then schema as suggested by Rob Herring on
>>>> https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/#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/[email protected]/
>
> 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/[email protected]/
Thanks
Helen