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 7EC09C04A68 for ; Thu, 28 Jul 2022 09:48:52 +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:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Cc:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=KDXt+USAju3n6Ir3js2QdFIKDckhSMdhh0lRZ9dHeeM=; b=m2vxTCLrIVfRiJ 68lRDg88wYic9k2wA3ERA25LG/5i1EwHtrrAysVjkeM5n6YkLSUvph/mqH2MoIxmwgLWzHYw7NW+i xMkYL+fH0LceQYte+n9GMN+YKIy0mt4Nq1JS8hwWOJO0rxlXnM2so4gFnmNgR29BZCXI+L7lcOIrh Ge5zhV6/CmE+Jxuan2d0k/pWuESXSrDYLjDlBaxhCRoxP3wlZV0slrhRfNxJdzsqG3+cAQU73THf7 QVXJcb2/2L8j6GKj2iJ4avaY5rVzvP2sn1I/1YfYAh0GgW8ETFfg81hu518+ek8E9RnoPat9vEkRo Akxw1RmHfAyVuojVW5MA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oH08C-007b98-HY; Thu, 28 Jul 2022 09:48:36 +0000 Received: from mail-lf1-x12a.google.com ([2a00:1450:4864:20::12a]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oH07w-007afB-7E for linux-amlogic@lists.infradead.org; Thu, 28 Jul 2022 09:48:25 +0000 Received: by mail-lf1-x12a.google.com with SMTP id w15so2022242lft.11 for ; Thu, 28 Jul 2022 02:48:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=message-id:date:mime-version:user-agent:subject:content-language:to :references:from:in-reply-to:content-transfer-encoding; bh=DPAyrSv5Oh9I0JWv/cbh3M8iNlmpiJ+L3KUAI07k8nM=; b=khcBZtlfNFfGUhTWR1nODMkOMfMe1YD6Zeye1trFQiFlQ5lXNAXloTmbqF82kXyCwm TrO2qrn8Brdqcl2r7R+MlapuXQxHBbeQXqGrHr07QRxVfajM2YYEM0m0LQxCfNu5Hujp 0mGjHb+OzofD5rCCEzIPDqwPeQWAJXYFmaB7Z5L0YaN3+gXbBYl3+PpT9+e2g1NycHZs OoV8LlvWkKoJr6SBIbjgcLJdfcXB+eqR9gsFX2bD/DBpTj/PoZPlqItVgfdmIPH7u6Je zt+pKZsdjOz22679/7lEkBbqf6u2RgsRwuKHjkfRQIUuqXRadPPI0ST1SzBme1ezmfTw vKIw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:references:from:in-reply-to :content-transfer-encoding; bh=DPAyrSv5Oh9I0JWv/cbh3M8iNlmpiJ+L3KUAI07k8nM=; b=0RRwwNlLgX1PNJzi033n4mUFH3IfDJ6rIS7xQVPPrpeDqQKffHifO9t5JBSjyZiqKt KMgnt9gGT2tUiQBOb2ZnAfGI2AMX39anocGNTWX2UV8qleICPSuYmycJAmdK7/QAuCZy WJlLHIaDF9je9iLABOaQfL/xMbVEKhfr+N/txS090uj9+DhzYw0MTW68QlTlIMxdc1bc 9/LaJlY8Uj23KLMJeWMJlmTYQvUbCe+u+pT2I/Gifwij89RiK6rXh/3YzLv8XiSmS12a f4EaiKUiuWIk4rQrvohb7emSjfPMqf49W4Kh/v//LbJQ2F07f8VWaUdTNwZ4uLNqFNl0 E39Q== X-Gm-Message-State: AJIora+HzEPSWqgE2eqHDgnWaiBSiZbJX6tXSwMBSm5P9yb8ZSGILOND 8LFoQeFzTEaQqUhnBwJS+BixmA== X-Google-Smtp-Source: AGRyM1tcCDD/G+nBlrNp6d4l4wQUepusSGXteoykxFWtMJnaYlcJzDCpBuQD0aCBwcGy4p3FFgt/9w== X-Received: by 2002:a19:7606:0:b0:48a:74b4:8479 with SMTP id c6-20020a197606000000b0048a74b48479mr9159783lff.441.1659001694241; Thu, 28 Jul 2022 02:48:14 -0700 (PDT) Received: from [192.168.3.197] (78-26-46-173.network.trollfjord.no. [78.26.46.173]) by smtp.gmail.com with ESMTPSA id p11-20020a2eb98b000000b0025e2e1d7c52sm55711ljp.44.2022.07.28.02.48.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 28 Jul 2022 02:48:13 -0700 (PDT) Message-ID: Date: Thu, 28 Jul 2022 11:48:12 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH V2 1/3] dt-bindings: clk: meson: add S4 SoC clock controller bindings Content-Language: en-US To: Jerome Brunet , Yu Tu , linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, Rob Herring , Neil Armstrong , Kevin Hilman , Michael Turquette , Stephen Boyd , Krzysztof Kozlowski , Martin Blumenstingl References: <20220728054202.6981-1-yu.tu@amlogic.com> <20220728054202.6981-2-yu.tu@amlogic.com> <82e3fd36-df96-a555-4cea-47fabd26502b@linaro.org> <1jv8rhfw8h.fsf@starbuckisacylon.baylibre.com> <367cf98b-ef06-8f44-76c8-9099a1ec13dc@linaro.org> <1jmtctfuli.fsf@starbuckisacylon.baylibre.com> From: Krzysztof Kozlowski In-Reply-To: <1jmtctfuli.fsf@starbuckisacylon.baylibre.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220728_024820_300044_08C072B2 X-CRM114-Status: GOOD ( 30.09 ) X-BeenThere: linux-amlogic@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-amlogic" Errors-To: linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org On 28/07/2022 11:09, Jerome Brunet wrote: > > On Thu 28 Jul 2022 at 11:02, Krzysztof Kozlowski wrote: > >> On 28/07/2022 10:50, Jerome Brunet wrote: >>> >>> On Thu 28 Jul 2022 at 10:41, Krzysztof Kozlowski wrote: >>> >>>> On 28/07/2022 07:42, Yu Tu wrote: > [...] >>>>> +/* >>>>> + * CLKID index values >>>>> + */ >>>>> + >>>>> +#define CLKID_FIXED_PLL 1 >>>>> +#define CLKID_FCLK_DIV2 3 >>>>> +#define CLKID_FCLK_DIV3 5 >>>>> +#define CLKID_FCLK_DIV4 7 >>>>> +#define CLKID_FCLK_DIV5 9 >>>>> +#define CLKID_FCLK_DIV7 11 >>>> >>>> Why these aren't continuous? IDs are expected to be incremented by 1. >>>> >>> >>> All clocks have IDs, it is one big table in the driver, but we are not exposing them all. >>> For example, with composite 'mux / div / gate' assembly, we usually need >>> only the leaf. >> >> I understand you do not expose them all, but that is not the reason to >> increment ID by 2 or 3... Otherwise these are not IDs and you are not >> expected to put register offsets into the bindings (you do not bindings >> in such case). > > Why is it not an IDs if it not continuous in the bindings ? > > If there is technical reason, we'll probably end up exposing everything. It > would not be a dramatic change. I asked for this over v1 because we have > done that is the past and I think it makes sense. > > I'm happy to be convinced to do things differently. Just looking for the > technical reason that require contiuous exposed IDs. > > The other IDs exists, but we do not expose them as bindings. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/meson/gxbb.h#n125 https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/ https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/ https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/ The IDs are abstract numbers, where the number does not matter because it is not tied to driver implementation or device programming model. The driver maps ID to respective clock. Using some meaningful numbers as these IDs, means you tied bindings to your implementation and any change in implementation requires change in the bindings. This contradicts the idea of bindings. > >> >> >>> Same has been done for the other AML controllers: >>> For ex: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/clock/gxbb-clkc.h >> >> This cannot be fixed now, but it is very poor argument. Like saying "we >> had a bug in other driver, so we implemented the bug here as well". > > I agree, "done before" is not a good argument. I was trying to provide a > better picutre. I'm just surprised to have this new requirement that IDs > have to be incremented by 1 (in the bindings) and I'd like to understand > why what we had done could be considered a bug now. It was always, just no one ever enforced it. And almost all clock and reset providers follow it. There are just literally few exceptions. > For example the simple-reset driver compute the reset offset from the IDs: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/reset/reset-simple.c This is one of the exceptions where it actually made sense, but I would argue it still contradicts the bindings. You have now binding which is tied to both Linux implementation and to device programming model. However fixing it would require creating huge mapping tables for each SoC, so obviously this exception is quite reasonable. Clock drivers require tables and translation anyway. Almost all clock drivers did it, so such exception is not justified. > There might be holes in the IDs if not all bits have reset maps. > I don't think that would be a bug either. Bug was of course highly exaggerated example. :) Best regards, Krzysztof _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic