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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 56167C43217 for ; Mon, 28 Nov 2022 12:33:36 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230203AbiK1Mdf (ORCPT ); Mon, 28 Nov 2022 07:33:35 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:44740 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230132AbiK1Mdc (ORCPT ); Mon, 28 Nov 2022 07:33:32 -0500 Received: from mail-wm1-x330.google.com (mail-wm1-x330.google.com [IPv6:2a00:1450:4864:20::330]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 29A501A05F for ; Mon, 28 Nov 2022 04:33:31 -0800 (PST) Received: by mail-wm1-x330.google.com with SMTP id 83-20020a1c0256000000b003d03017c6efso10818343wmc.4 for ; Mon, 28 Nov 2022 04:33:31 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=mime-version:message-id:in-reply-to:date:subject:cc:to:from :user-agent:references:from:to:cc:subject:date:message-id:reply-to; bh=+gmo9nIirdbed4J/LDnU4BNvdJRcxPBpHK1YwQWHx3A=; b=Ov/KaEuWs1Y8eHtYdGfDUCbL84zNHUFESCRmPJbwiL4b4IkVYPUs0G1f90nHsfQ/zv ehKTVNWofEcOeyX9H++dsbFfqvqpLv4UI5SpMeltJX+7PhkxVUhqbOS5sWWAgao/afWy Vmu2IYeujw1lNxavecWXZtBV2Xt7zVVCw10XpuwEZq0dkxd13hpGwtmlvIqyQeE/KNoX rA3Xh3b7GEY59tYVfwF7IRQcBMpp4dMqeAK/0dBG+Bw03kgBT6s60v17KxK+pPdPro6G FLxZbP+uH3QEA3DqFca6aUkawOebjlWwlLD1jmOpKkTr40d27MyLfvvmN0ta9OMOThQY 4WLw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:message-id:in-reply-to:date:subject:cc:to:from :user-agent:references:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=+gmo9nIirdbed4J/LDnU4BNvdJRcxPBpHK1YwQWHx3A=; b=CNQiOnXRnzTOcUixHCbfQWG+ub0iKXssU9OhLNAXpTvdxxB8p/SyiWXUwBsAd/5Nat 84T+rEikChyeoC2rnI/60cejO6WeVr9KrhPXINGonsBLZbWomATNpg1dr+SiISqQQFUk Rpl/+MVXm4Qw/rVTAaLzZT39FXPdpSLMU8qF5b45NtB5XRrEgJfkIypaiUuTs0ykz2ID HnvSFcP6pRcBMniPdYCu+iYjCrm2tEe864xiO+jwIOxMJ5e10UWRyCkvWqTnoONORUHG mpwHmkEAIVIEo3n9Fx7K3XitmuPobDNF9PtN2jJr7hc1oqzOONIVgJdY+WmrMJSdFtH1 mY0A== X-Gm-Message-State: ANoB5pkLyXZ3hGZUtzY+YG1obUFE94liJVqMcOR4kfy45aqsYvsgjt96 Onxse5C3eKApjw3XZvz8JG0BmA== X-Google-Smtp-Source: AA0mqf4jVHezM2bL21HVhzKH1EZ/A3CxuShkRmJj0N14THKAhU4L/c00faHr4CoC1CPZ4bH3MFrAHA== X-Received: by 2002:a05:600c:35cf:b0:3cf:aa11:93a8 with SMTP id r15-20020a05600c35cf00b003cfaa1193a8mr41173747wmq.31.1669638809617; Mon, 28 Nov 2022 04:33:29 -0800 (PST) Received: from localhost (laubervilliers-658-1-213-31.w90-63.abo.wanadoo.fr. [90.63.244.31]) by smtp.gmail.com with ESMTPSA id bs30-20020a056000071e00b002364c77bcacsm10489898wrb.38.2022.11.28.04.33.28 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 28 Nov 2022 04:33:28 -0800 (PST) References: <20221123021346.18136-1-yu.tu@amlogic.com> <20221123021346.18136-4-yu.tu@amlogic.com> <1jbkov2vb9.fsf@starbuckisacylon.baylibre.com> <81d9a794-2920-64f1-1d80-50653113624c@amlogic.com> User-agent: mu4e 1.8.10; emacs 28.2 From: Jerome Brunet To: 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 Cc: kelvin.zhang@amlogic.com Subject: Re: [PATCH V5 3/4] clk: meson: s4: add s4 SoC peripheral clock controller driver and bindings Date: Mon, 28 Nov 2022 13:23:08 +0100 In-reply-to: <81d9a794-2920-64f1-1d80-50653113624c@amlogic.com> Message-ID: <1jilizp8bs.fsf@starbuckisacylon.baylibre.com> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-clk@vger.kernel.org On Mon 28 Nov 2022 at 16:08, Yu Tu wrote: >>> + >>> +/* >>> + * This RTC clock can be supplied by an external 32KHz crystal oscillator. >>> + * If it is used, it should be documented in using fw_name and documented in the >>> + * Bindings. Not currently in use on this board, so skip it. >>> + */ >>> +static u32 rtc_clk_sel[] = { 0, 1 }; >> No reason to do that > > I'm going to change it to static u32 rtc_clk_sel[] = { 0, 1, 2 };. > I don't know if that's okay with you? ... then there is no need to specify this table. > >> >>> +static const struct clk_parent_data rtc_clk_sel_parent_data[] = { >>> + { .hw = &s4_rtc_32k_by_oscin.hw }, >>> + { .hw = &s4_rtc_32k_by_oscin_div.hw }, >>> + { .fw_name = "ext_32k", } >>> +}; >>> + >>> +static struct clk_regmap s4_rtc_clk = { >>> + .data = &(struct clk_regmap_mux_data) { >>> + .offset = CLKCTRL_RTC_CTRL, >>> + .mask = 0x3, >>> + .shift = 0, >>> + .table = rtc_clk_sel, >>> + .flags = CLK_MUX_ROUND_CLOSEST, >>> + }, >>> + .hw.init = &(struct clk_init_data){ >>> + .name = "rtc_clk_sel", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = rtc_clk_sel_parent_data, >>> + .num_parents = 2, >>> + .flags = CLK_SET_RATE_PARENT, >>> + }, >>> +}; >>> + [...] >>> + >>> +/* Video Clocks */ >>> +static struct clk_regmap s4_vid_pll_div = { >>> + .data = &(struct meson_vid_pll_div_data){ >>> + .val = { >>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>> + .shift = 0, >>> + .width = 15, >>> + }, >>> + .sel = { >>> + .reg_off = CLKCTRL_VID_PLL_CLK_DIV, >>> + .shift = 16, >>> + .width = 2, >>> + }, >>> + }, >>> + .hw.init = &(struct clk_init_data) { >>> + .name = "vid_pll_div", >>> + /* Same to g12a */ >>> + .ops = &meson_vid_pll_div_ro_ops, >> Please add an helpful explanation. >> 'Same to g12a' is not helpful. >> > > "Because the vid_pll_div clock is a clock that does not need to change the > divisor, ops only provides meson_vid_pll_div_ro_ops." > I wonder if this description is ok for you? I understand this divider will not change with RO ops. I'm interrested why it does not change and how it is expected to be setup. > >>> + .parent_data = (const struct clk_parent_data []) { >>> + { .fw_name = "hdmi_pll", } >>> + }, >>> + .num_parents = 1, >>> + .flags = CLK_SET_RATE_PARENT, >>> + }, >>> +}; [...] >>> + >>> +static struct clk_regmap s4_vclk_sel = { >>> + .data = &(struct clk_regmap_mux_data){ >>> + .offset = CLKCTRL_VID_CLK_CTRL, >>> + .mask = 0x7, >>> + .shift = 16, >>> + }, >>> + .hw.init = &(struct clk_init_data){ >>> + .name = "vclk_sel", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = s4_vclk_parent_data, >>> + .num_parents = ARRAY_SIZE(s4_vclk_parent_data), >>> + }, >> You are stopping rate propagation here. >> It deserves an explanation. Same goes below. > > "When the driver uses this clock, needs to specify the patent clock he > wants in the dts." > Is ok for you? Then you still don't understand the clock flag usage. Preserving the parent selection (CLK_SET_RATE_NO_REPARENT) and rate propagation (CLK_SET_RATE_PARENT) is not the same thing. As it stands, your comment is not aliged with what you do. > >> >>> +};