Re: [PATCH v4 5/9] media: staging: rkisp1: remove unecessary clocks
On Fri, Jul 17, 2020 at 12:14 PM Helen Koike 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
> > 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
> >>
> >> ---
> >>
> >> 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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4 5/9] media: staging: rkisp1: remove unecessary clocks
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 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 >> >> --- >> >> 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 0xff91 0x0 0x4000>; >> interrupts = ; >> clocks = < SCLK_ISP0>, >> - < ACLK_ISP0>, < ACLK_ISP0_WRAPPER>, >> - < HCLK_ISP0>, < HCLK_ISP0_WRAPPER>; >> -clock-names = "clk_isp", >> - "aclk_isp", "aclk_isp_wrap", >> - "hclk_isp", "hclk_isp_wrap"; >> + < ACLK_ISP0_WRAPPER>, >> +
Re: [PATCH v4 5/9] media: staging: rkisp1: remove unecessary clocks
On Thu, Jul 2, 2020 at 1:13 PM Helen Koike 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
>
> ---
>
> 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 0xff91 0x0 0x4000>;
> interrupts = ;
> clocks = < SCLK_ISP0>,
> - < ACLK_ISP0>, < ACLK_ISP0_WRAPPER>,
> - < HCLK_ISP0>, < HCLK_ISP0_WRAPPER>;
> -clock-names = "clk_isp",
> - "aclk_isp", "aclk_isp_wrap",
> - "hclk_isp", "hclk_isp_wrap";
> + < ACLK_ISP0_WRAPPER>,
> + < HCLK_ISP0_WRAPPER>;
> +clock-names = "isp", "aclk", "hclk";
> iommus = <_mmu>;
> phys = <>;
> 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
>
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
[PATCH v4 5/9] media: staging: rkisp1: remove unecessary clocks
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
---
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 0xff91 0x0 0x4000>;
interrupts = ;
clocks = < SCLK_ISP0>,
- < ACLK_ISP0>, < ACLK_ISP0_WRAPPER>,
- < HCLK_ISP0>, < HCLK_ISP0_WRAPPER>;
-clock-names = "clk_isp",
- "aclk_isp", "aclk_isp_wrap",
- "hclk_isp", "hclk_isp_wrap";
+ < ACLK_ISP0_WRAPPER>,
+ < HCLK_ISP0_WRAPPER>;
+clock-names = "isp", "aclk", "hclk";
iommus = <_mmu>;
phys = <>;
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
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
