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=-16.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 36F6AC47088 for ; Wed, 26 May 2021 03:09:17 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 132FF61432 for ; Wed, 26 May 2021 03:09:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231444AbhEZDKq (ORCPT ); Tue, 25 May 2021 23:10:46 -0400 Received: from mail.kernel.org ([198.145.29.99]:46718 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229750AbhEZDKp (ORCPT ); Tue, 25 May 2021 23:10:45 -0400 Received: by mail.kernel.org (Postfix) with ESMTPSA id 8FCEA6128B; Wed, 26 May 2021 03:09:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1621998554; bh=wBPtpL04qpK8gH7qVHwO7jhI8IygzE0yr7WFFNC43vQ=; h=In-Reply-To:References:Subject:From:Cc:To:Date:From; b=E3royAXjOPJ38N54cFneV51m2i4IlRsDZUlyjN5R6nml3ui3qywPLe7phxAQpCGcE KwB05Y8bnHo8/n7s/tC/Hl3rMKFHrg+pXD1iy4KKUcAWN7S/3NruQ9ElEiDeBmIMUe CqhVz7b53aEfxGi/3MZ54csT93+gvxb0OvlyQvV8EkAdlG3grJ3sWiDj7LijDKJjK7 C3IBeST1wwtMhXWaxojh2G+48NH/NeRobOZ2Ozi6jJV8RYTv64Zq/GNALeNwNEdcnS IbIjnzbLDPYLqVHKwc09Pxc8tq7rS7KiVJWxsKKKfUrDQ7xQ6p/3ABoJ6ga5GHrXMF yvyTUDJWYOQKg== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable In-Reply-To: <20210524182745.22923-3-sven@svenpeter.dev> References: <20210524182745.22923-1-sven@svenpeter.dev> <20210524182745.22923-3-sven@svenpeter.dev> Subject: Re: [PATCH 2/3] clk: add support for gate clocks on Apple SoCs From: Stephen Boyd Cc: Sven Peter , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Hector Martin , Michael Turquette , Rob Herring , Mark Kettenis , Arnd Bergmann To: Sven Peter , devicetree@vger.kernel.org, linux-clk@vger.kernel.org Date: Tue, 25 May 2021 20:09:13 -0700 Message-ID: <162199855335.4130789.17766958356597249549@swboyd.mtv.corp.google.com> User-Agent: alot/0.9.1 Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Quoting Sven Peter (2021-05-24 11:27:44) > Add a simple driver for gate clocks found on Apple SoCs. These don't > have any frequency associated with them and are only used to enable > access to MMIO regions of various peripherals. Can we figure out what frequency they are clocking at? >=20 > Signed-off-by: Sven Peter > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index e80918be8e9c..ac987a8cf318 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -245,6 +245,18 @@ config CLK_TWL6040 > McPDM. McPDM module is using the external bit clock on the McPD= M bus > as functional clock. > =20 > +config COMMON_CLK_APPLE > + tristate "Clock driver for Apple platforms" > + depends on ARCH_APPLE && COMMON_CLK The COMMON_CLK depend is redundant. Please remove. > + default ARCH_APPLE > + help > + Support for clock gates on Apple SoCs such as the M1. > + > + These clock gates do not have a frequency associated with them = and > + are only used to power on/off various peripherals. Generally, a= clock > + gate needs to be enabled before the respective MMIO region can = be > + accessed. > + > config COMMON_CLK_AXI_CLKGEN > tristate "AXI clkgen driver" > depends on HAS_IOMEM || COMPILE_TEST > diff --git a/drivers/clk/clk-apple-gate.c b/drivers/clk/clk-apple-gate.c > new file mode 100644 > index 000000000000..799e9269758f > --- /dev/null > +++ b/drivers/clk/clk-apple-gate.c > @@ -0,0 +1,152 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Apple SoC clock/power gating driver > + * > + * Copyright The Asahi Linux Contributors > + */ > + > +#include Hopefully this can be droped. > +#include And this one too. clkdev is not modern. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define CLOCK_TARGET_MODE_MASK 0x0f APPLE_ prefix on all these? > +#define CLOCK_TARGET_MODE(m) (((m)&0xf)) > +#define CLOCK_ACTUAL_MODE_MASK 0xf0 > +#define CLOCK_ACTUAL_MODE(m) (((m)&0xf) << 4) > + > +#define CLOCK_MODE_ENABLE 0xf > +#define CLOCK_MODE_DISABLE 0 > + > +#define CLOCK_ENDISABLE_TIMEOUT 100 > + > +struct apple_clk_gate { > + struct clk_hw hw; > + void __iomem *reg; > +}; > + > +#define to_apple_clk_gate(_hw) container_of(_hw, struct apple_clk_gate, = hw) > + > +static int apple_clk_gate_endisable(struct clk_hw *hw, int enable) > +{ > + struct apple_clk_gate *gate =3D to_apple_clk_gate(hw); > + u32 reg; > + u32 mode; > + > + if (enable) > + mode =3D CLOCK_MODE_ENABLE; > + else > + mode =3D CLOCK_MODE_DISABLE; > + > + reg =3D readl(gate->reg); > + reg &=3D ~CLOCK_TARGET_MODE_MASK; > + reg |=3D CLOCK_TARGET_MODE(mode); > + writel(reg, gate->reg); > + > + return readl_poll_timeout_atomic(gate->reg, reg, > + (reg & CLOCK_ACTUAL_MODE_MASK) = =3D=3D > + CLOCK_ACTUAL_MODE(mode), > + 1, CLOCK_ENDISABLE_TIMEOUT); Maybe this return readl_poll_timeout_atomic(gate->reg, reg, (reg & CLOCK_ACTUAL_MODE_MASK) =3D=3D CLOCK_ACTUAL_MODE(mode), 1, CLOCK_ENDISABLE_TIMEOUT); at the least please don't break the =3D=3D across two lines. > +} > + > +static int apple_clk_gate_enable(struct clk_hw *hw) > +{ > + return apple_clk_gate_endisable(hw, 1); > +} > + > +static void apple_clk_gate_disable(struct clk_hw *hw) > +{ > + apple_clk_gate_endisable(hw, 0); > +} > + > +static int apple_clk_gate_is_enabled(struct clk_hw *hw) > +{ > + struct apple_clk_gate *gate =3D to_apple_clk_gate(hw); > + u32 reg; > + > + reg =3D readl(gate->reg); > + > + if ((reg & CLOCK_ACTUAL_MODE_MASK) =3D=3D CLOCK_ACTUAL_MODE(CLOCK= _MODE_ENABLE)) Any chance we can use FIELD_GET() and friends? Please don't do bit shifting stuff inside conditionals as it just makes life more complicated than it needs to be. > + return 1; > + return 0; How about just return ? > +} > + > +static const struct clk_ops apple_clk_gate_ops =3D { > + .enable =3D apple_clk_gate_enable, > + .disable =3D apple_clk_gate_disable, > + .is_enabled =3D apple_clk_gate_is_enabled, > +}; > + > +static int apple_gate_clk_probe(struct platform_device *pdev) > +{ > + struct device *dev =3D &pdev->dev; > + struct device_node *node =3D dev->of_node; > + const struct clk_parent_data parent_data[] =3D { > + { .index =3D 0 }, > + }; Yay thanks for not doing it the old way! > + struct apple_clk_gate *data; > + struct clk_hw *hw; > + struct clk_init_data init; > + struct resource *res; > + int num_parents; > + int ret; > + > + data =3D devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->reg =3D devm_ioremap_resource(dev, res); > + if (IS_ERR(data->reg)) > + return PTR_ERR(data->reg); > + > + num_parents =3D of_clk_get_parent_count(node); Oh no I spoke too soon. > + if (num_parents > 1) { > + dev_err(dev, "clock supports at most one parent\n"); > + return -EINVAL; > + } > + > + init.name =3D dev->of_node->name; > + init.ops =3D &apple_clk_gate_ops; > + init.flags =3D 0; > + init.parent_names =3D NULL; > + init.parent_data =3D parent_data; > + init.num_parents =3D num_parents; > + > + data->hw.init =3D &init; > + hw =3D &data->hw; > + > + ret =3D devm_clk_hw_register(dev, hw); > + if (ret) > + return ret; > + > + return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw); It looks like a one clk per DT node binding which is not how we do it. I see Rob has started that discussion on the binding so we can keep that there. > +} > + 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=-14.4 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=ham 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 A4CC6C2B9F7 for ; Wed, 26 May 2021 03:10:39 +0000 (UTC) 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 mail.kernel.org (Postfix) with ESMTPS id 65E9961432 for ; Wed, 26 May 2021 03:10:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 65E9961432 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org 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:Message-ID:Date:To:Cc:From:Subject: References:In-Reply-To:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=IBk5tKb94NSgVNIgHbxDTieHmWV+PpULeSwb6xOPJdM=; b=4uN0njYffWKWOX 6xQ2loJ+9cwNCdr7wTPn+rTwsSbNcpdGo9cH5R4eLzEHgYGjN0PzuGKSRIjRzA1l7wrrKuIhur7Rq Xs+iztlug92NOVQHQogfH5LXcKL/CskdhXKlEM+7leNnqs5+sL2opHuE4Y55Kb0cBRFClWgBLarwi uXnc50/y6DXJxV4/40I+yma07p2hP558v3WqjfbtAfmHpYEKdcvh/V7IYlFiHyAboCn0+yBSxQHK1 jqy7bRKCHNH1bOapr9ClGco5W8/QlCyn1sMw6KcdMnX7lHdDd2UOZJNjyMTDDN/haVsrZHBRvEWpD kXtnjn5vDjzuVbQHfItA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1lljv5-00ARMF-7U; Wed, 26 May 2021 03:09:19 +0000 Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1lljv1-00ARKX-N7 for linux-arm-kernel@lists.infradead.org; Wed, 26 May 2021 03:09:17 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id 8FCEA6128B; Wed, 26 May 2021 03:09:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1621998554; bh=wBPtpL04qpK8gH7qVHwO7jhI8IygzE0yr7WFFNC43vQ=; h=In-Reply-To:References:Subject:From:Cc:To:Date:From; b=E3royAXjOPJ38N54cFneV51m2i4IlRsDZUlyjN5R6nml3ui3qywPLe7phxAQpCGcE KwB05Y8bnHo8/n7s/tC/Hl3rMKFHrg+pXD1iy4KKUcAWN7S/3NruQ9ElEiDeBmIMUe CqhVz7b53aEfxGi/3MZ54csT93+gvxb0OvlyQvV8EkAdlG3grJ3sWiDj7LijDKJjK7 C3IBeST1wwtMhXWaxojh2G+48NH/NeRobOZ2Ozi6jJV8RYTv64Zq/GNALeNwNEdcnS IbIjnzbLDPYLqVHKwc09Pxc8tq7rS7KiVJWxsKKKfUrDQ7xQ6p/3ABoJ6ga5GHrXMF yvyTUDJWYOQKg== MIME-Version: 1.0 In-Reply-To: <20210524182745.22923-3-sven@svenpeter.dev> References: <20210524182745.22923-1-sven@svenpeter.dev> <20210524182745.22923-3-sven@svenpeter.dev> Subject: Re: [PATCH 2/3] clk: add support for gate clocks on Apple SoCs From: Stephen Boyd Cc: Sven Peter , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Hector Martin , Michael Turquette , Rob Herring , Mark Kettenis , Arnd Bergmann To: Sven Peter , devicetree@vger.kernel.org, linux-clk@vger.kernel.org Date: Tue, 25 May 2021 20:09:13 -0700 Message-ID: <162199855335.4130789.17766958356597249549@swboyd.mtv.corp.google.com> User-Agent: alot/0.9.1 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210525_200915_823624_8035C353 X-CRM114-Status: GOOD ( 31.58 ) 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 Quoting Sven Peter (2021-05-24 11:27:44) > Add a simple driver for gate clocks found on Apple SoCs. These don't > have any frequency associated with them and are only used to enable > access to MMIO regions of various peripherals. Can we figure out what frequency they are clocking at? > > Signed-off-by: Sven Peter > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index e80918be8e9c..ac987a8cf318 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -245,6 +245,18 @@ config CLK_TWL6040 > McPDM. McPDM module is using the external bit clock on the McPDM bus > as functional clock. > > +config COMMON_CLK_APPLE > + tristate "Clock driver for Apple platforms" > + depends on ARCH_APPLE && COMMON_CLK The COMMON_CLK depend is redundant. Please remove. > + default ARCH_APPLE > + help > + Support for clock gates on Apple SoCs such as the M1. > + > + These clock gates do not have a frequency associated with them and > + are only used to power on/off various peripherals. Generally, a clock > + gate needs to be enabled before the respective MMIO region can be > + accessed. > + > config COMMON_CLK_AXI_CLKGEN > tristate "AXI clkgen driver" > depends on HAS_IOMEM || COMPILE_TEST > diff --git a/drivers/clk/clk-apple-gate.c b/drivers/clk/clk-apple-gate.c > new file mode 100644 > index 000000000000..799e9269758f > --- /dev/null > +++ b/drivers/clk/clk-apple-gate.c > @@ -0,0 +1,152 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Apple SoC clock/power gating driver > + * > + * Copyright The Asahi Linux Contributors > + */ > + > +#include Hopefully this can be droped. > +#include And this one too. clkdev is not modern. > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define CLOCK_TARGET_MODE_MASK 0x0f APPLE_ prefix on all these? > +#define CLOCK_TARGET_MODE(m) (((m)&0xf)) > +#define CLOCK_ACTUAL_MODE_MASK 0xf0 > +#define CLOCK_ACTUAL_MODE(m) (((m)&0xf) << 4) > + > +#define CLOCK_MODE_ENABLE 0xf > +#define CLOCK_MODE_DISABLE 0 > + > +#define CLOCK_ENDISABLE_TIMEOUT 100 > + > +struct apple_clk_gate { > + struct clk_hw hw; > + void __iomem *reg; > +}; > + > +#define to_apple_clk_gate(_hw) container_of(_hw, struct apple_clk_gate, hw) > + > +static int apple_clk_gate_endisable(struct clk_hw *hw, int enable) > +{ > + struct apple_clk_gate *gate = to_apple_clk_gate(hw); > + u32 reg; > + u32 mode; > + > + if (enable) > + mode = CLOCK_MODE_ENABLE; > + else > + mode = CLOCK_MODE_DISABLE; > + > + reg = readl(gate->reg); > + reg &= ~CLOCK_TARGET_MODE_MASK; > + reg |= CLOCK_TARGET_MODE(mode); > + writel(reg, gate->reg); > + > + return readl_poll_timeout_atomic(gate->reg, reg, > + (reg & CLOCK_ACTUAL_MODE_MASK) == > + CLOCK_ACTUAL_MODE(mode), > + 1, CLOCK_ENDISABLE_TIMEOUT); Maybe this return readl_poll_timeout_atomic(gate->reg, reg, (reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(mode), 1, CLOCK_ENDISABLE_TIMEOUT); at the least please don't break the == across two lines. > +} > + > +static int apple_clk_gate_enable(struct clk_hw *hw) > +{ > + return apple_clk_gate_endisable(hw, 1); > +} > + > +static void apple_clk_gate_disable(struct clk_hw *hw) > +{ > + apple_clk_gate_endisable(hw, 0); > +} > + > +static int apple_clk_gate_is_enabled(struct clk_hw *hw) > +{ > + struct apple_clk_gate *gate = to_apple_clk_gate(hw); > + u32 reg; > + > + reg = readl(gate->reg); > + > + if ((reg & CLOCK_ACTUAL_MODE_MASK) == CLOCK_ACTUAL_MODE(CLOCK_MODE_ENABLE)) Any chance we can use FIELD_GET() and friends? Please don't do bit shifting stuff inside conditionals as it just makes life more complicated than it needs to be. > + return 1; > + return 0; How about just return ? > +} > + > +static const struct clk_ops apple_clk_gate_ops = { > + .enable = apple_clk_gate_enable, > + .disable = apple_clk_gate_disable, > + .is_enabled = apple_clk_gate_is_enabled, > +}; > + > +static int apple_gate_clk_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; > + const struct clk_parent_data parent_data[] = { > + { .index = 0 }, > + }; Yay thanks for not doing it the old way! > + struct apple_clk_gate *data; > + struct clk_hw *hw; > + struct clk_init_data init; > + struct resource *res; > + int num_parents; > + int ret; > + > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + data->reg = devm_ioremap_resource(dev, res); > + if (IS_ERR(data->reg)) > + return PTR_ERR(data->reg); > + > + num_parents = of_clk_get_parent_count(node); Oh no I spoke too soon. > + if (num_parents > 1) { > + dev_err(dev, "clock supports at most one parent\n"); > + return -EINVAL; > + } > + > + init.name = dev->of_node->name; > + init.ops = &apple_clk_gate_ops; > + init.flags = 0; > + init.parent_names = NULL; > + init.parent_data = parent_data; > + init.num_parents = num_parents; > + > + data->hw.init = &init; > + hw = &data->hw; > + > + ret = devm_clk_hw_register(dev, hw); > + if (ret) > + return ret; > + > + return devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw); It looks like a one clk per DT node binding which is not how we do it. I see Rob has started that discussion on the binding so we can keep that there. > +} > + _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel