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 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: add required items in i2c example
media: staging: dt-bindings: rkisp1: re-order properties
media: staging: dt-bindings: rkisp1: fix "no reg" error in parent node
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 | 64 +++++++++-------
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, 142 insertions(+), 31 deletions(-)
rename {drivers/staging/media/rkisp1/Documentation => Documentation}/devicetree/bindings/media/rockchip-isp1.yaml (83%)
--
2.26.0
Add missing required nodes in json-schema yaml file for
Rockchip ISP1 dt-bindings.
Signed-off-by: Helen Koike <[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.26.0
Add missing required items in Rockchip ISP1 dt-bindings example for
a complete i2c node.
Signed-off-by: Helen Koike <[email protected]>
---
Changes in v2:
- new patch in the series
---
.../devicetree/bindings/media/rockchip-isp1.yaml | 5 +++++
1 file changed, 5 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 a77b6ec500c95..8c7904845788d 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
@@ -169,6 +169,11 @@ examples:
};
i2c7: i2c@ff160000 {
+ compatible = "rockchip,rk3399-i2c";
+ reg = <0x0 0xff160000 0x0 0x1000>;
+ interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH 0>;
+ clocks = <&cru SCLK_I2C7>, <&cru PCLK_I2C7>;
+ clock-names = "i2c", "pclk";
clock-frequency = <400000>;
#address-cells = <1>;
#size-cells = <0>;
--
2.26.0
Organize properties order in dt-binbings 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]>
---
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 8c7904845788d..e5b9c0574e352 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.26.0
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]>
---
V3:
- this is a new patch in the series
---
.../Documentation/devicetree/bindings/media/rockchip-isp1.yaml | 1 +
1 file changed, 1 insertion(+)
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 e5b9c0574e352..4d111ef2e89c7 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
@@ -126,6 +126,7 @@ examples:
#include <dt-bindings/power/rk3399-power.h>
parent0: parent@0 {
+ reg = <0 0>;
#address-cells = <2>;
#size-cells = <2>;
--
2.26.0
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.
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 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 | 30 +++++++++----------
drivers/staging/media/rkisp1/rkisp1-dev.c | 8 ++---
2 files changed, 17 insertions(+), 21 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 4d111ef2e89c7..f10c53d008748 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,20 @@ 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
+ maxItems: 5
+ minItems: 3
+ description:
+ ISP clock
+ ISP AXI clock
+ ISP AHB clock
clock-names:
+ maxItems: 5
+ minItems: 3
items:
- - const: clk_isp
- - const: aclk_isp
- - const: aclk_isp_wrap
- - const: hclk_isp
- - const: hclk_isp_wrap
+ - const: isp
+ - const: aclk
+ - const: hclk
iommus:
maxItems: 1
@@ -135,11 +135,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 f38801fea10d9..175ac25fe99fa 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.26.0
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 634d2c3d621aa..d7141cf17072f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14679,7 +14679,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.26.0
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.26.0
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.26.0
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.26.0
On Thu, Jul 2, 2020 at 1:13 PM Helen Koike <[email protected]> wrote:
>
> Add missing required items in Rockchip ISP1 dt-bindings example for
> a complete i2c node.
>
> Signed-off-by: Helen Koike <[email protected]>
> ---
>
> Changes in v2:
> - new patch in the series
> ---
> .../devicetree/bindings/media/rockchip-isp1.yaml | 5 +++++
> 1 file changed, 5 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 a77b6ec500c95..8c7904845788d 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
> @@ -169,6 +169,11 @@ examples:
> };
>
> i2c7: i2c@ff160000 {
> + compatible = "rockchip,rk3399-i2c";
> + reg = <0x0 0xff160000 0x0 0x1000>;
This will give a warning now as the default cell sizes are 1.
> + interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH 0>;
> + clocks = <&cru SCLK_I2C7>, <&cru PCLK_I2C7>;
> + clock-names = "i2c", "pclk";
Just drop the unit address rather than adding all this.
> clock-frequency = <400000>;
> #address-cells = <1>;
> #size-cells = <0>;
> --
> 2.26.0
>
On Thu, Jul 2, 2020 at 1:13 PM 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]>
> ---
>
> Changes in v2:
> - New patch in the series
> ---
> .../devicetree/bindings/media/rockchip-isp1.yaml | 8 ++++++++
> 1 file changed, 8 insertions(+)
Acked-by: Rob Herring <[email protected]>
On Thu, Jul 2, 2020 at 1:13 PM Helen Koike <[email protected]> wrote:
>
> Organize properties order in dt-binbings to move it out of staging.
typo
>
> On top: compatible, reg and interrupts.
> Then alphabetical order, then properties starting with '#'.
>
> Signed-off-by: Helen Koike <[email protected]>
>
> ---
>
> V2:
> - this is a new patch in the series
> ---
> .../bindings/media/rockchip-isp1.yaml | 32 +++++++++----------
> 1 file changed, 16 insertions(+), 16 deletions(-)
Acked-by: Rob Herring <[email protected]>
On Thu, Jul 2, 2020 at 1:13 PM 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]>
> ---
>
> V3:
> - this is a new patch in the series
> ---
> .../Documentation/devicetree/bindings/media/rockchip-isp1.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> 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 e5b9c0574e352..4d111ef2e89c7 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
> @@ -126,6 +126,7 @@ examples:
> #include <dt-bindings/power/rk3399-power.h>
>
> parent0: parent@0 {
> + reg = <0 0>;
Just drop the unit-address.
> #address-cells = <2>;
> #size-cells = <2>;
>
> --
> 2.26.0
>
On Thu, Jul 2, 2020 at 1:13 PM 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.
But it looks like it is the wrap clocks you are removing.
>
> 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.
>
> 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 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 | 30 +++++++++----------
> drivers/staging/media/rkisp1/rkisp1-dev.c | 8 ++---
> 2 files changed, 17 insertions(+), 21 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 4d111ef2e89c7..f10c53d008748 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,20 @@ 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
This is the correct way to describe multiple clocks.
> + maxItems: 5
Now the 4th and 5th clock are undefined.
> + minItems: 3
> + description:
> + ISP clock
> + ISP AXI clock
> + ISP AHB clock
>
> clock-names:
> + maxItems: 5
This should not be more than the number of entries in 'items'.
> + minItems: 3
> items:
> - - const: clk_isp
> - - const: aclk_isp
> - - const: aclk_isp_wrap
> - - const: hclk_isp
> - - const: hclk_isp_wrap
> + - const: isp
> + - const: aclk
> + - const: hclk
>
> iommus:
> maxItems: 1
> @@ -135,11 +135,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 f38801fea10d9..175ac25fe99fa 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.26.0
>
Hi Rob,
Thanks for your review.
On 7/17/20 2:49 PM, Rob Herring wrote:
> On Thu, Jul 2, 2020 at 1:13 PM 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.
>
> But it looks like it is the wrap clocks you are removing.
From this binding yes, but the idea is to add in the dt wherever clock
responsible for the full ACLK path for instance.
In the example below, clock aclk_isp is ACLK_ISP0_WRAPPER.
Does this make sense?
>
>>
>> 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.
>>
>> 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 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 | 30 +++++++++----------
>> drivers/staging/media/rkisp1/rkisp1-dev.c | 8 ++---
>> 2 files changed, 17 insertions(+), 21 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 4d111ef2e89c7..f10c53d008748 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,20 @@ 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
>
> This is the correct way to describe multiple clocks.
The idea was to prepare for rk3288 and rk3399 isp1, as suggested here https://patchwork.kernel.org/patch/11475007/#23462085
Or should we do:
clocks:
oneOf:
# rk3288 clocks
- items:
- description: ISP clock
- description: ISP AXI clock
- description: ISP AHB clock
- description: ISP Pixel clock
- description: ISP JPEG source clock
# rk3399 isp0 clocks
- items:
- description: ISP clock
- description: ISP AXI clock
- description: ISP AHB clock
# rk3399 isp1 clocks
- items:
- description: ISP clock
- description: ISP AXI clock
- description: ISP AHB clock
- description: ISP Pixel clock
?
>
>> + maxItems: 5
>
> Now the 4th and 5th clock are undefined.
>
>> + minItems: 3
>> + description:
>> + ISP clock
>> + ISP AXI clock
>> + ISP AHB clock
>>
>> clock-names:
>> + maxItems: 5
>
> This should not be more than the number of entries in 'items'.
>
If we follow what I wrote above, should we have:
clock-names:
oneOf:
# rk3288 clocks
- items:
- const: clk_isp
- const: aclk_isp
- const: hclk_isp
- const: pclk_isp_in
- const: sclk_isp_jpe
# rk3399 isp0 clocks
- items:
- const: clk_isp
- const: aclk_isp
- const: hclk_isp
# rk3399 isp1 clocks
- items:
- const: clk_isp
- const: aclk_isp
- const: hclk_isp
- const: pclk_isp
?
Thanks
Helen
>> + minItems: 3
>> items:
>> - - const: clk_isp
>> - - const: aclk_isp
>> - - const: aclk_isp_wrap
>> - - const: hclk_isp
>> - - const: hclk_isp_wrap
>> + - const: isp
>> + - const: aclk
>> + - const: hclk
>>
>> iommus:
>> maxItems: 1
>> @@ -135,11 +135,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 f38801fea10d9..175ac25fe99fa 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.26.0
>>
On Fri, Jul 17, 2020 at 12:14 PM Helen Koike <[email protected]> wrote:
>
> Hi Rob,
>
> Thanks for your review.
>
> On 7/17/20 2:49 PM, Rob Herring wrote:
> > On Thu, Jul 2, 2020 at 1:13 PM 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.
> >
> > But it looks like it is the wrap clocks you are removing.
>
> From this binding yes, but the idea is to add in the dt wherever clock
> responsible for the full ACLK path for instance.
> In the example below, clock aclk_isp is ACLK_ISP0_WRAPPER.
> Does this make sense?
Just perhaps clarify the renaming.
>
> >
> >>
> >> 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.
> >>
> >> 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 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 | 30 +++++++++----------
> >> drivers/staging/media/rkisp1/rkisp1-dev.c | 8 ++---
> >> 2 files changed, 17 insertions(+), 21 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 4d111ef2e89c7..f10c53d008748 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,20 @@ 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
> >
> > This is the correct way to describe multiple clocks.
>
> The idea was to prepare for rk3288 and rk3399 isp1, as suggested here https://patchwork.kernel.org/patch/11475007/#23462085
>
> Or should we do:
>
> clocks:
> oneOf:
> # rk3288 clocks
> - items:
> - description: ISP clock
> - description: ISP AXI clock
> - description: ISP AHB clock
> - description: ISP Pixel clock
> - description: ISP JPEG source clock
The main section should have this and 'minItems: 3'. IOW, it's a
superset of what's valid. Then you can restrict specific compatibles
further with an if/then schema. For rk3288, you need one with
'minItems: 5'.
> # rk3399 isp0 clocks
> - items:
> - description: ISP clock
> - description: ISP AXI clock
> - description: ISP AHB clock
And this would be an if/then schema based on the compatible string and
defining 'maxItems: 3'.
> # rk3399 isp1 clocks
> - items:
> - description: ISP clock
> - description: ISP AXI clock
> - description: ISP AHB clock
> - description: ISP Pixel clock
And an if/then with { minItems: 4, maxItems: 4 }. Or really since
these are just different instances, just combine them into 1
conditional allowing 3 or 4 clocks.
There are lots of examples to follow in the tree.
Rob