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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 93384C54E94 for ; Wed, 25 Jan 2023 10:02:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To: Subject:MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=sW/UKNjtIu9C+llSG1SpDNGAqAjXiyecMR6KKfLbVEI=; b=SN8cg51y33TEsB 4DO+AvaXortzGjDfjxehPNjRT2qgiRESj2OhJxIPJOHHox/Uz2yduGe2EDYqx1LKUpa0SNGzqiKog x9hinIJdlPx09/2nzaDcfMY9UJL95mlvhSJmIACtSAfJXcx11g743q/sUfPAH4Z30Vuqw4PIAPYgp TlCmX8Lz9h6UO3wdT8RbDug2lXYghduEjuuTM8tkz5esLBaexN0/DOaomtOdGIa0zdNPYjKgl08TI UfguFt1PK2/vZU3OP9cukpltpdF9GyJFg3EYnbs7yhIOrFt6Jd6PF7zIYmIlW+8tXCN5zLXkMgTJE dtz2A2/8GZPRk+dmJE5g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pKcaa-006mvJ-MV; Wed, 25 Jan 2023 10:01:09 +0000 Received: from mail-wm1-x335.google.com ([2a00:1450:4864:20::335]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pKcVI-006klj-Em for linux-arm-kernel@lists.infradead.org; Wed, 25 Jan 2023 09:55:43 +0000 Received: by mail-wm1-x335.google.com with SMTP id o17-20020a05600c511100b003db021ef437so850990wms.4 for ; Wed, 25 Jan 2023 01:55:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=3TI3QogOYMMQNqPXE2b7du3WbjaJH52c2VaulOg3Bhg=; b=qug77F6kd2bvCnnzdGxs88Cb4zDm8xwdWaHtW/FM+aX+u6m/7F+m7t6l5ZxBN7FxzL PuisldXsfLPXNBsVX6qqQnVziYE3QVHDI/XRler2igoKlFYRilQpgt9OyXLfjAupLIM/ 4G3z7l08N0VnZlHQQYWMd4shRsqfr0GVLfJWF3ZKR3YB7Zh499c61/2DZRwAkd7UFv4d dVvS2UUNWMqOLFG2KqFUo3/tGeWQhdbNhpXsQ8RFmsjKLQ7APzRkskXAsAwBlNBNgt3R sm4iTGxNQ8zkvSRX1cZh+UnEE5Dy6qHzKK2eZsmndGqrEfbtxahEtSBHJY7yE5DTEsp8 qttA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=3TI3QogOYMMQNqPXE2b7du3WbjaJH52c2VaulOg3Bhg=; b=3Ssd0mrP4hEDzAdjLB/red5HkkSxIMGug3RsZfTj45p1YfOLmXzlkn2w1+ZmJEsZx0 hAWzH0c8NpKdY+y32ugRE39p8+mfjV9J7hZITC1l08nzcDzHozTBHC59Z3rNIwfjeDsF 5TIqfC7eAbw21e0dwg1elPd49Ww2robVhPJEfM4654N4nUKcDJoc/j2uDASv+ioSqQpi OCnYO4tjEkY4XvU5Ex32Q8T6kjj1f2zR5UqATg3Z64F674ncVVIG+t26pAiyRr9K1JpU 4pdjVU2kVj6+k/KAQyZNIVFdBgibP4lbk3nPjVXp8N7FYuDxH7e26uZvgH4zPpn5EQat GNvg== X-Gm-Message-State: AFqh2kq+/XpOoMWaFagbZyZDGmByZdgU0dXvxtIwjCYogdgXujQeo38z syACTRFmiuSL3AB2ABEzCZd2bU9GmQ5u5e+Y X-Google-Smtp-Source: AMrXdXu2KF4yvkw/TiJ9sngbPl4P5gsVXJvwzfHlUs9sLE0NttgDMQFlQjIghP75T8JZOp14TN7IMw== X-Received: by 2002:a05:600c:181a:b0:3d2:2043:9cbf with SMTP id n26-20020a05600c181a00b003d220439cbfmr31125729wmp.10.1674640536570; Wed, 25 Jan 2023 01:55:36 -0800 (PST) Received: from [192.168.1.109] ([178.197.216.144]) by smtp.gmail.com with ESMTPSA id j9-20020a5d6049000000b002be1dcb6efbsm4844333wrt.9.2023.01.25.01.55.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 25 Jan 2023 01:55:36 -0800 (PST) Message-ID: Date: Wed, 25 Jan 2023 10:55:34 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.7.0 Subject: Re: [PATCH 07/15] dt-bindings: clock: Add Ambarella clock bindings To: Li Chen Cc: Li Chen , Michael Turquette , Stephen Boyd , Rob Herring , Krzysztof Kozlowski , "moderated list:ARM/Ambarella SoC support" , "open list:COMMON CLK FRAMEWORK" , "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , open list References: <20230123073305.149940-1-lchen@ambarella.com> <20230123073305.149940-8-lchen@ambarella.com> <0c19efb4-3bca-f500-ca24-14b9d24369ef@linaro.org> <87y1prgdyu.wl-me@linux.beauty> Content-Language: en-US From: Krzysztof Kozlowski In-Reply-To: <87y1prgdyu.wl-me@linux.beauty> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230125_015540_565888_8CF87069 X-CRM114-Status: GOOD ( 34.62 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 25/01/2023 10:28, Li Chen wrote: > > Hi Krzysztof, > > Sorry for my late reply. > > On Mon, 23 Jan 2023 16:11:08 +0800, > Krzysztof Kozlowski wrote: >> >> On 23/01/2023 08:32, Li Chen wrote: >>> This patch introduce clock bindings for Ambarella. >>> >>> Signed-off-by: Li Chen >>> Change-Id: I29018a23ed3a5b79a1103e859a5c7ed7bb83a261 >> >> All the same problems plus new: >> >> Subject: drop second/last, redundant "bindings". The "dt-bindings" >> prefix is already stating that these are bindings. > > Well noted. > >>> --- >>> .../clock/ambarella,composite-clock.yaml | 52 ++++++++++++++++ >>> .../bindings/clock/ambarella,pll-clock.yaml | 59 +++++++++++++++++++ >>> MAINTAINERS | 2 + >>> 3 files changed, 113 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml >>> create mode 100644 Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml >>> >>> diff --git a/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml >>> new file mode 100644 >>> index 000000000000..fac1cb9379c4 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/clock/ambarella,composite-clock.yaml >>> @@ -0,0 +1,52 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/clock/ambarella,composite-clock.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Ambarella Composite Clock >>> + >>> +maintainers: >>> + - Li Chen >>> + >> >> Missing description. > > Thanks, description as below will be added in v2: > > "Ambarella SoCs integrates some composite clocks, like uart0, which aggrate the functionality > of the basic clock types, like mux and div." > >>> +properties: >>> + compatible: >>> + items: >> >> Drop items. > > Ok. > >>> + - const: ambarella,composite-clock >> >> Missing SoC specific compatible. This is anyway not really correct >> compatible... > > Most Ambarella's compatibles don't contain SoC name, because we prefer > to use syscon + offsets in dts to tell driver the correct register offsets, or > ues struct soc_device and SoC identity stores in a given physical address. That's not correct hardware description. Drop the syscon and offsets. > > So compatibles like "ambarella,composite-clock" and "ambarella,pinctrl" are > used widely in Ambarella kernels. What do you do downstream does not matter. You can invent any crazy idea and it is not an argument that it should be done like that. Usually downstream code is incorrect... > Feel free to correct me if you think this > is not a good idea. This is bad idea. Compatibles should be specific. Devices should not use syscons to poke other registers, unless strictly necessary, but have strictly defined MMIO address space and use it. > >>> + >>> + clocks: true >> >> No, needs constraints. > > Ok. I will list all clocks name > >>> + assigned-clocks: true >>> + assigned-clock-parents: true >>> + assigned-clock-rates: true >> >> Drop these three. > > Ok > >>> + clock-output-names: true >> >> Missing constraints. > > Ok, I will add "maxItems: 1" > >>> + amb,mux-regmap: true >> >> NAK. >> >> It's enough. The patches have very, very poor quality. >> >> Missing description, missing type/$ref, wrong prefix. > > Sorry, I forget to run dt_binding_check, I will spend some > time learning the binding and check, sorry for it. > >>> + amb,div-regmap: true >>> + amb,div-width: true >>> + amb,div-shift: true >> >> These two are arguments to phandle. > > I will add description and $ref to regmap and width/shift. Drop all these syscon properties. > >>> + >>> + '#clock-cells': >>> + const: 0 >>> + >>> +required: >>> + - compatible >>> + - reg >>> + - clocks >>> + - '#clock-cells' >>> + >>> +additionalProperties: false >> >> So why you decided to add it here and not in other places? > > I didn't understand it well. I will add it to other places in v2, > thanks for pointint out it. > >>> + >>> +examples: >>> + - | >>> + gclk_uart0: gclk-uart0 { >> >> Wrong indentation. > > Well noted. > >>> + #clock-cells = <0>; >>> + compatible = "ambarella,composite-clock"; >>> + clocks = <&osc>, <&gclk_core>, <&pll_out_enet>, <&pll_out_sd>; >>> + clock-output-names = "gclk_uart0"; >>> + assigned-clocks = <&gclk_uart0>; >>> + assigned-clock-parents = <&osc>; >>> + assigned-clock-rates = <24000000>; >>> + amb,mux-regmap = <&rct_syscon 0x1c8>; >>> + amb,div-regmap = <&rct_syscon 0x038>; >>> + amb,div-width = <24>; >>> + amb,div-shift = <0>; >>> + }; >>> diff --git a/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml >>> new file mode 100644 >>> index 000000000000..65c1feb60041 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/clock/ambarella,pll-clock.yaml >>> @@ -0,0 +1,59 @@ >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>> +%YAML 1.2 >>> +--- >>> +$id: http://devicetree.org/schemas/clock/ambarella,pll-clock.yaml# >>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>> + >>> +title: Ambarella PLL Clock >>> + >>> +maintainers: >>> + - Li Chen >>> + >>> +properties: >>> + compatible: >>> + enum: >>> + - ambarella,pll-clock >>> + - ambarella,clkpll-v0 >>> + >>> +if: >> >> No, this does not work like that. It sits under "allOf", located after >> "required:". > > Thanks, I will learn "allOf" and use it in v2. BTW, we use the two compatibles as below: > clocks { > compatible = "ambarella,clkpll-v0"; Nope. > ... > gclk_core: gclk-core { > #clock-cells = <0>; > compatible = "ambarella,pll-clock"; Also nope. > clocks = <&osc>; > clock-output-names = "gclk_core"; > amb,clk-regmap = <&rct_syscon 0x000 0x004 0x100 0x104 0x000 0x000>; Nope, nope, nope. You need proper clock-controller with its own MMIO address space. > }; > ... > } > > I'm not sure can I describe the two compatibles in this single yaml, can you give some advice? thanks! There are plenty of examples, including example-schema. Best regards, Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel