[v4,5/9] media: staging: rkisp1: remove unecessary clocks
diff mbox series

Message ID 20200702191322.2639681-6-helen.koike@collabora.com
State New
Headers show
Series
  • move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399
Related show

Commit Message

Helen Koike July 2, 2020, 7:13 p.m. UTC
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 <helen.koike@collabora.com>

---

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

Comments

Rob Herring July 17, 2020, 5:49 p.m. UTC | #1
On Thu, Jul 2, 2020 at 1:13 PM Helen Koike <helen.koike@collabora.com> wrote:
>
> aclk_isp_wrap is a child of aclk_isp, and hclk_isp_wrap is a child of
> hclk_isp, thus we can remove parents from the list.

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 <helen.koike@collabora.com>
>
> ---
>
> 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
>
Helen Koike July 17, 2020, 6:14 p.m. UTC | #2
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 <helen.koike@collabora.com> wrote:
>>
>> aclk_isp_wrap is a child of aclk_isp, and hclk_isp_wrap is a child of
>> hclk_isp, thus we can remove parents from the list.
> 
> 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 <helen.koike@collabora.com>
>>
>> ---
>>
>> 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
>>
Rob Herring July 17, 2020, 6:44 p.m. UTC | #3
On Fri, Jul 17, 2020 at 12:14 PM Helen Koike <helen.koike@collabora.com> 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 <helen.koike@collabora.com> wrote:
> >>
> >> aclk_isp_wrap is a child of aclk_isp, and hclk_isp_wrap is a child of
> >> hclk_isp, thus we can remove parents from the list.
> >
> > 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 <helen.koike@collabora.com>
> >>
> >> ---
> >>
> >> 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

Patch
diff mbox series

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 = {