From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EDAE9C433E2 for ; Fri, 17 Jul 2020 18:14:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id D6ABD20717 for ; Fri, 17 Jul 2020 18:14:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726381AbgGQSOl (ORCPT ); Fri, 17 Jul 2020 14:14:41 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:59888 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726205AbgGQSOl (ORCPT ); Fri, 17 Jul 2020 14:14:41 -0400 Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: koike) with ESMTPSA id E3F9C2A5DC6 Subject: Re: [PATCH v4 5/9] media: staging: rkisp1: remove unecessary clocks To: Rob Herring Cc: devicetree@vger.kernel.org, Linux Media Mailing List , "open list:ARM/Rockchip SoC..." , "linux-kernel@vger.kernel.org" , "open list:STAGING SUBSYSTEM" , "heiko@sntech.de" , Hans Verkuil , Collabora Kernel ML , Dafna Hirschfeld , Ezequiel Garcia , Mark Rutland , karthik.poduval@gmail.com, Johan Jonker , Tomasz Figa , Eddie Cai , Shunqian Zheng , Robin Murphy References: <20200702191322.2639681-1-helen.koike@collabora.com> <20200702191322.2639681-6-helen.koike@collabora.com> From: Helen Koike Message-ID: <822de449-8c01-d790-a9f9-e8b513bd7f87@collabora.com> Date: Fri, 17 Jul 2020 15:14:30 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org 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 0xff910000 0x0 0x4000>; >> interrupts = ; >> 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 >> From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 64B64C433E4 for ; Fri, 17 Jul 2020 18:14:46 +0000 (UTC) Received: from fraxinus.osuosl.org (smtp4.osuosl.org [140.211.166.137]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 489ED206F4 for ; Fri, 17 Jul 2020 18:14:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 489ED206F4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=collabora.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=driverdev-devel-bounces@linuxdriverproject.org Received: from localhost (localhost [127.0.0.1]) by fraxinus.osuosl.org (Postfix) with ESMTP id 20E0C868D1; Fri, 17 Jul 2020 18:14:46 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from fraxinus.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id R4HtBDVSCP3F; Fri, 17 Jul 2020 18:14:44 +0000 (UTC) Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by fraxinus.osuosl.org (Postfix) with ESMTP id AF9A7868CB; Fri, 17 Jul 2020 18:14:44 +0000 (UTC) Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by ash.osuosl.org (Postfix) with ESMTP id 9515D1BF83A for ; Fri, 17 Jul 2020 18:14:43 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 8A59C220EF for ; Fri, 17 Jul 2020 18:14:43 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id EAU1E3bH9hE5 for ; Fri, 17 Jul 2020 18:14:42 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227]) by silver.osuosl.org (Postfix) with ESMTPS id A826221513 for ; Fri, 17 Jul 2020 18:14:41 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) (Authenticated sender: koike) with ESMTPSA id E3F9C2A5DC6 Subject: Re: [PATCH v4 5/9] media: staging: rkisp1: remove unecessary clocks To: Rob Herring References: <20200702191322.2639681-1-helen.koike@collabora.com> <20200702191322.2639681-6-helen.koike@collabora.com> From: Helen Koike Message-ID: <822de449-8c01-d790-a9f9-e8b513bd7f87@collabora.com> Date: Fri, 17 Jul 2020 15:14:30 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-BeenThere: driverdev-devel@linuxdriverproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "open list:STAGING SUBSYSTEM" , devicetree@vger.kernel.org, Eddie Cai , Tomasz Figa , Dafna Hirschfeld , "heiko@sntech.de" , Shunqian Zheng , "linux-kernel@vger.kernel.org" , karthik.poduval@gmail.com, "open list:ARM/Rockchip SoC..." , Hans Verkuil , Robin Murphy , Mark Rutland , Collabora Kernel ML , Ezequiel Garcia , Johan Jonker , Linux Media Mailing List Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" 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 0xff910000 0x0 0x4000>; >> interrupts = ; >> 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 >> _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel