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 F0161C05027 for ; Fri, 20 Jan 2023 10:06:04 +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:MIME-Version:Message-ID:In-reply-to: Date:Subject:Cc:To:From:References:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=exyoamPxU40XHi7oG3zf8uPKYubGOpkDtEE9PQxD2WI=; b=d6aZInRGG2Mc3t +k6D/fl/wfui77lcuUQqSccUdOl9wHuPAa1yHeITcAyF6kyY8Kz0abnLvHkCkbUeSeA+TbtRq4PSc 05TOi5x3luoTvHZF9g8ylEGN9CQMMRFWwOW1XTItNK6UJImzKSqLaBzPKHaa8A1FzTf+JBYupFafV yaD6tqUxA49d0caxfXe+grpO8XI+rJLTnHlXuCIhWIHEYJZ/6uzSVmLnqUnYO0UAycDx7JqzIv5Cd MAtSrrPiPLMReSn6SNHxhdoK5pwqdcWV0uxCXkRgt3WSwoHTMGw/CrKKQqTjnYf17L1TV06niGiTp +qVqI+322Ntcz8PVZoqA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1pIoHR-009f6I-L3; Fri, 20 Jan 2023 10:05:53 +0000 Received: from mail-wm1-x32b.google.com ([2a00:1450:4864:20::32b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1pIo4r-009YXT-9q for linux-amlogic@lists.infradead.org; Fri, 20 Jan 2023 09:52:56 +0000 Received: by mail-wm1-x32b.google.com with SMTP id l8so3557323wms.3 for ; Fri, 20 Jan 2023 01:52:50 -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=FnjzLigWTWrmVYT7keQxDKaopp0We8SxlDZVkLkpEYc=; b=CkgI1BIZB/UVM6UII4xu7jq+VyvnfafGgTj6jesx5TVV9fBJwMW6oITwzQhWKtrHyk uy2yPxhzqMa5r4bm+UxhcH+fl6iTXEsXi6GI+HNlK/ao2CKpREvxU5RoR1Bh9t3AeOZX r+TqA9dF/GsFGNrBqh49qpoaqleQHh47gYaf9oAGojQn9bnthXK9GhnF0puo/5+z/HNr wJKhEq1s18AjKsEhqhQS/cuHm1+7IBtLM5pDCBJALqDMP+VPoWrI49Bs3yXwbvPoJd8G GWIYPcbVn1c2/TwM/B3PKALxRz7D7U5NClbZURDa2UXLeAJFG8+7ybjLNe81hJpcBDzC /gMg== 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=FnjzLigWTWrmVYT7keQxDKaopp0We8SxlDZVkLkpEYc=; b=4pv/58odJrz5XFWgwWgLjqK/xaD4BzHKauPL3yAnkrgN4p0YfFnF7CKkKPVVGrp2qq O+WXnUMAQ6IK3u2JqqEWf+74wrXIP8rqE7tTXBbyXSZh8D9gRK6y8i05Qd2EOZPzvvJK g72Hl4l38fya125AFK9XykPM+Uhks4Bs9lvaDLwzEYWk719B/23lnHGmDMVQrcBZ2nxQ LkTvYkF2x8SHSI+4lbS+r/XpszqyuKpUYc6EksObLLQi5gI9jWya67NKLPetbCdJJ3jP IEGUjcsodGyib1ZfynT+VEWu76lWwMs6uRpoe8xRQSwz0Kn+EW46IvD1mWwBC1zxgZqh omEw== X-Gm-Message-State: AFqh2kp/shG24Oj/hF187kYlyHepAn870wydYUv2cNs/zCdCc2DDIp4L B3Jy+RoZBVLdMhbDM4KdG5BShg== X-Google-Smtp-Source: AMrXdXup3WUxvn3YSSPPEY8ObOfbzJeR6OV0og1Ivv7BMGJZOj1s6tojl0jM8yq9LLbQWb2eu2d6fQ== X-Received: by 2002:a05:600c:3b05:b0:3d6:b691:b80d with SMTP id m5-20020a05600c3b0500b003d6b691b80dmr13511451wms.21.1674208369487; Fri, 20 Jan 2023 01:52:49 -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 m2-20020a05600c4f4200b003db0ad636d1sm1923545wmq.28.2023.01.20.01.52.48 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 20 Jan 2023 01:52:48 -0800 (PST) References: <20230116074214.2326-1-yu.tu@amlogic.com> <20230116074214.2326-4-yu.tu@amlogic.com> <1ja62eybrv.fsf@starbuckisacylon.baylibre.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" , "qi . duan" Subject: Re: [PATCH V6 3/3] clk: meson: s4: add support for Amlogic S4 SoC peripheral clock controller Date: Fri, 20 Jan 2023 10:47:31 +0100 In-reply-to: Message-ID: <1jwn5hwn0w.fsf@starbuckisacylon.baylibre.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230120_015253_379798_1587E40E X-CRM114-Status: GOOD ( 34.31 ) 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 Fri 20 Jan 2023 at 11:33, Yu Tu wrote: > Hi > On 2023/1/19 19:37, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> On Mon 16 Jan 2023 at 15:42, Yu Tu wrote: >> >>> Add the peripherals clock controller driver in the s4 SoC family. >>> >>> Signed-off-by: Yu Tu >> [...] >> >>> + >>> +/* 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", >>> + /* >>> + * The frequency division from the hdmi_pll clock to the vid_pll_div >>> + * clock is the default value of this register. When designing the >>> + * video module of the chip, a default value that can meet the >>> + * requirements of the video module will be solidified according >>> + * to the usage requirements of the chip, so as to facilitate chip >>> + * simulation. So this is ro_ops. >>> + * It is important to note that this clock is not used on this >>> + * chip and is described only for the integrity of the clock tree. >>> + */ >> If it is reset value and will be applicable to all the design, regarless >> of the use-case, then yes RO ops is OK >> >>>>From what I understand here, the value will depend on the use-case requirements. >> This is a typical case where the DT prop "assigned-rate" should be used, not RO ops. > > Check the previous chip history, the actual scene is not used at all, > basically is used in simulation. So the previous SOC was "ro_ops" without > any problems. This S4 SOC is not actually useful either. > > So when you were upstream, you had no problem making "ro_ops". I wonder if > I could delete this useless clock, so you don't have to worry about it. I don't know what to make of this. What is the point of adding a useless clock ? > >> >>> + .ops = &meson_vid_pll_div_ro_ops, >>> + .parent_data = (const struct clk_parent_data []) { >>> + { .fw_name = "hdmi_pll", } >>> + }, >>> + .num_parents = 1, >>> + .flags = CLK_SET_RATE_PARENT, >>> + }, >>> +}; >>> + >> >>> + >>> +/* VDEC clocks */ >>> +static const struct clk_parent_data s4_dec_parent_data[] = { >>> + { .fw_name = "fclk_div2p5", }, >>> + { .fw_name = "fclk_div3", }, >>> + { .fw_name = "fclk_div4", }, >>> + { .fw_name = "fclk_div5", }, >>> + { .fw_name = "fclk_div7", }, >>> + { .fw_name = "hifi_pll", }, >>> + { .fw_name = "gp0_pll", }, >>> + { .fw_name = "xtal", } >>> +}; >>> + >>> +static struct clk_regmap s4_vdec_p0_mux = { >>> + .data = &(struct clk_regmap_mux_data){ >>> + .offset = CLKCTRL_VDEC_CLK_CTRL, >>> + .mask = 0x7, >>> + .shift = 9, >>> + .flags = CLK_MUX_ROUND_CLOSEST, >>> + }, >>> + .hw.init = &(struct clk_init_data) { >>> + .name = "vdec_p0_mux", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = s4_dec_parent_data, >>> + .num_parents = ARRAY_SIZE(s4_dec_parent_data), >>> + /* >>> + * When the driver uses this clock, needs to specify the patent clock >>> + * he wants in the dts. >> s/patent/parent ? >> s/he wants/it requires ? > > Okay. > >> >>> + */ >>> + .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, >>> + }, >>> +}; >>> + >> [...] >> >>> +static const struct clk_parent_data s4_vpu_clkc_parent_data[] = { >>> + { .fw_name = "fclk_div4", }, >>> + { .fw_name = "fclk_div3", }, >>> + { .fw_name = "fclk_div5", }, >>> + { .fw_name = "fclk_div7", }, >>> + { .fw_name = "mpll1", }, >>> + { .hw = &s4_vid_pll.hw }, >>> + { .fw_name = "mpll2", }, >>> + { .fw_name = "gp0_pll", }, >>> +}; >>> + >>> +static struct clk_regmap s4_vpu_clkc_p0_mux = { >>> + .data = &(struct clk_regmap_mux_data){ >>> + .offset = CLKCTRL_VPU_CLKC_CTRL, >>> + .mask = 0x7, >>> + .shift = 9, >>> + }, >>> + .hw.init = &(struct clk_init_data) { >>> + .name = "vpu_clkc_p0_mux", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = s4_vpu_clkc_parent_data, >>> + .num_parents = ARRAY_SIZE(s4_vpu_clkc_parent_data), >>> + /* >>> + * When the driver uses this clock, needs to specify the patent clock >>> + * he wants in the dts. >>> + */ >> That's quite a lot of occurences of the same comment. >> At the same time, other clocks with the same flag have no comment. >> Please make general comment, before the Video/VPU section, explaining >> which clocks needs on a use-case basis (through DT) and possibly how it >> should be set, what should drive the choices. >> > > The owner of the corresponding driver module wants to have a fixed clock, > but I can't explain every specific reason. Why can't you ? > So I'm going to change it all > to.flags = CLK_SET_RATE_PARENT in the next version. Let CCF choose the > appropriate clock as you suggested. If there is a corresponding module you > want to change, ask him to give you a specific explanation. Do you think > that's all right? If the flag is actually required and there is a reason, no it is not. > > I will not reply to you below. Noted. > >>> + .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, >>> + }, >>> +}; >>> + >> >>> + >>> +/* EMMC/NAND clock */ >>> +static const struct clk_parent_data s4_sd_emmc_clk0_parent_data[] = { >>> + { .fw_name = "xtal", }, >>> + { .fw_name = "fclk_div2", }, >>> + { .fw_name = "fclk_div3", }, >>> + { .fw_name = "hifi_pll", }, >>> + { .fw_name = "fclk_div2p5", }, >>> + { .fw_name = "mpll2", }, >>> + { .fw_name = "mpll3", }, >>> + { .fw_name = "gp0_pll", }, >>> +}; >>> + >>> +static struct clk_regmap s4_sd_emmc_c_clk0_sel = { >>> + .data = &(struct clk_regmap_mux_data){ >>> + .offset = CLKCTRL_NAND_CLK_CTRL, >>> + .mask = 0x7, >>> + .shift = 9, >>> + }, >>> + .hw.init = &(struct clk_init_data) { >>> + .name = "sd_emmc_c_clk0_sel", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = s4_sd_emmc_clk0_parent_data, >>> + .num_parents = ARRAY_SIZE(s4_sd_emmc_clk0_parent_data), >>> + /* >>> + * When the driver uses this clock, needs to specify the patent clock >>> + * he wants in the dts. >>> + */ >> I'm getting a bit suspicious about the use (and abuse ...) of this flag. >> I don't quite get how selecting the base PLL for MMC should be done on >> use-case basis and should be up the board DT ... >> Other SoC have all used fdiv2 so far. Do you expect this setting to be >> part of the dtsi SoC file ? >> >>> + .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, >>> + }, >>> +}; >>> + >> >>> + >>> +/* SPICC Clock */ >>> +static const struct clk_parent_data s4_spicc_parent_data[] = { >>> + { .fw_name = "xtal", }, >>> + { .hw = &s4_sys_clk.hw }, >>> + { .fw_name = "fclk_div4", }, >>> + { .fw_name = "fclk_div3", }, >>> + { .fw_name = "fclk_div2", }, >>> + { .fw_name = "fclk_div5", }, >>> + { .fw_name = "fclk_div7", }, >>> +}; >>> + >>> +static struct clk_regmap s4_spicc0_mux = { >>> + .data = &(struct clk_regmap_mux_data){ >>> + .offset = CLKCTRL_SPICC_CLK_CTRL, >>> + .mask = 0x7, >>> + .shift = 7, >>> + }, >>> + .hw.init = &(struct clk_init_data) { >>> + .name = "spicc0_mux", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = s4_spicc_parent_data, >>> + .num_parents = ARRAY_SIZE(s4_spicc_parent_data), >>> + /* >>> + * When the driver uses this clock, needs to specify the patent clock >>> + * he wants in the dts. >>> + */ >> This is getting too far. All the parent clocks are fixed. >> Let CCF do the job of picking the most adequate clock for the job >> instead of manually settings things >> >>> + .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, >>> + }, >>> +}; >>> + >> >>> + >>> +/* PWM Clock */ >>> +static const struct clk_parent_data s4_pwm_parent_data[] = { >>> + { .fw_name = "xtal", }, >>> + { .hw = &s4_vid_pll.hw }, >>> + { .fw_name = "fclk_div4", }, >>> + { .fw_name = "fclk_div3", }, >>> +}; >>> + >>> +static struct clk_regmap s4_pwm_a_mux = { >>> + .data = &(struct clk_regmap_mux_data) { >>> + .offset = CLKCTRL_PWM_CLK_AB_CTRL, >>> + .mask = 0x3, >>> + .shift = 9, >>> + }, >>> + .hw.init = &(struct clk_init_data){ >>> + .name = "pwm_a_mux", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = s4_pwm_parent_data, >>> + .num_parents = ARRAY_SIZE(s4_pwm_parent_data), >>> + /* >>> + * When the driver uses this clock, needs to specify the patent clock >>> + * he wants in the dts. >>> + */ >> Same here ... this is really going to far. >> >>> + .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, >>> + }, >>> +}; >>> + >> >>> + >>> +static struct clk_regmap s4_saradc_mux = { >>> + .data = &(struct clk_regmap_mux_data) { >>> + .offset = CLKCTRL_SAR_CLK_CTRL, >>> + .mask = 0x3, >>> + .shift = 9, >>> + }, >>> + .hw.init = &(struct clk_init_data){ >>> + .name = "saradc_mux", >>> + .ops = &clk_regmap_mux_ops, >>> + .parent_data = (const struct clk_parent_data []) { >>> + { .fw_name = "xtal", }, >>> + { .hw = &s4_sys_clk.hw }, >>> + }, >>> + .num_parents = 2, >>> + /* >>> + * When the driver uses this clock, needs to specify the patent clock >>> + * he wants in the dts. >>> + */ >> For each clock type, if this flag is going to be used, I'd like a clear >> explanation about why it is use-case dependent and why you need manual >> control over this. Same applies to all the occurence. >> >>> + .flags = CLK_SET_RATE_NO_REPARENT | CLK_SET_RATE_PARENT, >>> + }, >>> +}; >> _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic