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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id AF12FC433EF for ; Thu, 14 Oct 2021 22:09:28 +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 6AD976109E for ; Thu, 14 Oct 2021 22:09:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 6AD976109E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=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=BCAyxEpeeSBMv09Sgm3AvwntcRlON2N5zjFGLVHrk/s=; b=2FtHswK7Qj3Gis AxFxHP33ZozCyXl0/GwVAMv/3Ufae2fx50ljowuTwDKoqpNydWN0M4ckBAfZ4IpYvyi7xHOn+XoNV tGMxdE5zKhs3QUmHjH+sv6MrUfEZew4BApTCHYEqX9ZkY868V8BkDyvv5TU/bG0BsDn6GRmdyo2wq bQfzk5g5ksDYo1nerGIWdwwJ0Pgr4LoNf/GlPPccW56hBa9H2YRMY82ohhl6uNsngagsgDbRFXD0y kvK8mR+7C6S1K1v7AYt9naxj2bv5egFbtj/sgaE5/xlI5Xd6iE4dNoqu14cUnI0Mxg0sM3YqHmoHC Hlqokf0X7ENLxnTd8N7g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mb8t8-004TrZ-0M; Thu, 14 Oct 2021 22:07:46 +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 1mb8t3-004TrD-Ct for linux-arm-kernel@lists.infradead.org; Thu, 14 Oct 2021 22:07:43 +0000 Received: by mail.kernel.org (Postfix) with ESMTPSA id B1C9560F5D; Thu, 14 Oct 2021 22:07:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1634249260; bh=Q1YWLUn/G8kd4WV460nHtmbLmPKaJPqKkyEXOJc5qJE=; h=In-Reply-To:References:Subject:From:Cc:To:Date:From; b=dAp3M8j4JH+2u1qXq+pebG1Dp9Zex4XIh00Z83H61RE0Vgeyev2R8OG0dDbu56lxh PJFBEN3hGJI3ZnQQ6gowAowjIUp6WxcgOO0YTe3MwkDSI+G5e7zTPYJHaLBoB5zxO9 bK1CWVcvQXH7dFjGkK5FiO8/VJec2l9Lr47kEbLRhp9SgNmTsah2M6OJr00bbP1tq1 BnSSSbbrVrpuYjNMj4wCU8QLeB371m2J5g4p2C8YUJp48LGnehVbw/ixBnLMMM01YQ xsZ30h0mJzoEJcz3QIZl/7gMmgUXKVSZ8COuMV/28r3Vuz7SfLlNTWNX4jNIkvGT2X l3LLto1LJRtUw== MIME-Version: 1.0 In-Reply-To: <20211011165707.138157-8-marcan@marcan.st> References: <20211011165707.138157-1-marcan@marcan.st> <20211011165707.138157-8-marcan@marcan.st> Subject: Re: [RFC PATCH 7/9] clk: apple: Add clk-apple-cluster driver to manage CPU p-states From: Stephen Boyd Cc: Hector Martin , Alyssa Rosenzweig , Sven Peter , Marc Zyngier , Mark Kettenis , Michael Turquette , Rob Herring , Krzysztof Kozlowski , Viresh Kumar , Nishanth Menon , Catalin Marinas , Rafael J. Wysocki , Kevin Hilman , Ulf Hansson , linux-clk@vger.kernel.org, devicetree@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org To: Hector Martin , linux-arm-kernel@lists.infradead.org Date: Thu, 14 Oct 2021 15:07:39 -0700 Message-ID: <163424925931.1688384.9647104000291025081@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-20211014_150741_510042_F877ACFF X-CRM114-Status: GOOD ( 29.28 ) 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 Hector Martin (2021-10-11 09:57:05) > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig > index c5b3dc97396a..f3c8ad041f91 100644 > --- a/drivers/clk/Kconfig > +++ b/drivers/clk/Kconfig > @@ -390,6 +390,15 @@ config COMMON_CLK_K210 > help > Support for the Canaan Kendryte K210 RISC-V SoC clocks. > > +config COMMON_CLK_APPLE_CLUSTER Please place in alphabetical sort order of config name > + bool "Clock driver for Apple SoC CPU clusters" > + depends on ARCH_APPLE || COMPILE_TEST > + select CPUFREQ_DT > + default ARCH_APPLE > + help > + This driver supports CPU cluster frequency switching on Apple SoC > + platforms. > + > source "drivers/clk/actions/Kconfig" > source "drivers/clk/analogbits/Kconfig" > source "drivers/clk/baikal-t1/Kconfig" > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile > index e42312121e51..6dba8c2052c7 100644 > --- a/drivers/clk/Makefile > +++ b/drivers/clk/Makefile > @@ -31,6 +31,7 @@ obj-$(CONFIG_COMMON_CLK_FIXED_MMIO) += clk-fixed-mmio.o > obj-$(CONFIG_COMMON_CLK_FSL_FLEXSPI) += clk-fsl-flexspi.o > obj-$(CONFIG_COMMON_CLK_FSL_SAI) += clk-fsl-sai.o > obj-$(CONFIG_COMMON_CLK_GEMINI) += clk-gemini.o > +obj-$(CONFIG_COMMON_CLK_APPLE_CLUSTER) += clk-apple-cluster.o Please put this in sort order on file name. I don't know why aspeed is in the wrong place. Sigh. > obj-$(CONFIG_COMMON_CLK_ASPEED) += clk-aspeed.o > obj-$(CONFIG_MACH_ASPEED_G6) += clk-ast2600.o > obj-$(CONFIG_ARCH_HIGHBANK) += clk-highbank.o > diff --git a/drivers/clk/clk-apple-cluster.c b/drivers/clk/clk-apple-cluster.c > new file mode 100644 > index 000000000000..9e9be38f13b2 > --- /dev/null > +++ b/drivers/clk/clk-apple-cluster.c > @@ -0,0 +1,184 @@ > +// SPDX-License-Identifier: GPL-2.0-only OR MIT > +/* > + * Apple SoC CPU cluster performance state driver > + * > + * Copyright The Asahi Linux Contributors > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define APPLE_CLUSTER_PSTATE 0x20 > +#define APPLE_CLUSTER_PSTATE_BUSY BIT(31) > +#define APPLE_CLUSTER_PSTATE_SET BIT(25) > +#define APPLE_CLUSTER_PSTATE_DESIRED2 GENMASK(15, 12) > +#define APPLE_CLUSTER_PSTATE_DESIRED1 GENMASK(3, 0) > + > +struct apple_cluster_clk { > + struct clk_hw hw; > + struct device *dev; > + void __iomem *reg_base; > + bool has_pd; > +}; > + > +#define to_apple_cluster_clk(_hw) container_of(_hw, struct apple_cluster_clk, hw) > + > +#define APPLE_CLUSTER_SWITCH_TIMEOUT 100 > + > +static int apple_cluster_clk_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct apple_cluster_clk *cluster = to_apple_cluster_clk(hw); > + struct dev_pm_opp *opp; > + unsigned int level; > + u64 reg; > + > + opp = dev_pm_opp_find_freq_floor(cluster->dev, &rate); > + > + if (IS_ERR(opp)) > + return PTR_ERR(opp); > + > + level = dev_pm_opp_get_level(opp); > + > + dev_dbg(cluster->dev, "set_rate: %ld -> %d\n", rate, level); > + > + if (readq_poll_timeout(cluster->reg_base + APPLE_CLUSTER_PSTATE, reg, > + !(reg & APPLE_CLUSTER_PSTATE_BUSY), 2, > + APPLE_CLUSTER_SWITCH_TIMEOUT)) { > + dev_err(cluster->dev, "timed out waiting for busy flag\n"); > + return -EIO; > + } > + > + reg &= ~(APPLE_CLUSTER_PSTATE_DESIRED1 | APPLE_CLUSTER_PSTATE_DESIRED2); > + reg |= FIELD_PREP(APPLE_CLUSTER_PSTATE_DESIRED1, level); > + reg |= FIELD_PREP(APPLE_CLUSTER_PSTATE_DESIRED2, level); > + reg |= APPLE_CLUSTER_PSTATE_SET; > + > + writeq_relaxed(reg, cluster->reg_base + APPLE_CLUSTER_PSTATE); > + > + if (cluster->has_pd) > + dev_pm_genpd_set_performance_state(cluster->dev, > + dev_pm_opp_get_required_pstate(opp, 0)); This looks bad from a locking perspective. How is lockdep holding up with this driver? We're underneath the prepare lock here and we're setting a couple level registers which is all good but now we're calling into genpd code and who knows what's going to happen locking wise. I don't actually see anything in here that indicates this is supposed to be a clk provider. Is it being modeled as a clk so that it can use cpufreq-dt? If it was a clk provider I'd expect it to be looking at parent clk rates, and reading hardware to calculate frequencies based on dividers and multipliers, etc. None of that is happening here. Why not write a cpufreq driver, similar to qcom-cpufreq-hw.c that looks through the OPP table and then writes the value into the pstate registers? The registers in here look awfully similar to the qcom hardware. I don't know what the DESIRED1 and DESIRED2 registers are for though. Maybe they're so that one or the other frequency can be used if available? Like a min/max? Either way, writing this as a cpufreq driver avoids the clk framework entirely which is super great for me :) It also avoids locking headaches from the clk prepare lock, and it also lets you support lockless cpufreq transitions by implementing the fast_switch function. I don't see any downsides to the cpufreq driver approach. > + > + return 0; > +} > + > +static unsigned long apple_cluster_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) > +{ > + struct apple_cluster_clk *cluster = to_apple_cluster_clk(hw); > + struct dev_pm_opp *opp; > + u64 reg; > + > + reg = readq_relaxed(cluster->reg_base + APPLE_CLUSTER_PSTATE); > + > + opp = dev_pm_opp_find_level_exact(cluster->dev, > + FIELD_GET(APPLE_CLUSTER_PSTATE_DESIRED1, reg)); > + > + if (IS_ERR(opp)) { > + dev_err(cluster->dev, "failed to find level: 0x%llx (%ld)\n", reg, PTR_ERR(opp)); > + return 0; > + } > + > + return dev_pm_opp_get_freq(opp); > +} > + > +static long apple_cluster_clk_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + struct apple_cluster_clk *cluster = to_apple_cluster_clk(hw); > + struct dev_pm_opp *opp; > + > + opp = dev_pm_opp_find_freq_floor(cluster->dev, &rate); > + > + if (IS_ERR(opp)) { > + dev_err(cluster->dev, "failed to find rate: %ld (%ld)\n", rate, PTR_ERR(opp)); > + return PTR_ERR(opp); > + } > + > + return rate; > +} > + > +static const struct clk_ops apple_cluster_clk_ops = { > + .set_rate = apple_cluster_clk_set_rate, > + .recalc_rate = apple_cluster_clk_recalc_rate, > + .round_rate = apple_cluster_clk_round_rate, > +}; > + > +static int apple_cluster_clk_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *node = dev->of_node; > + struct apple_cluster_clk *cluster; > + struct clk_hw *hw; > + struct clk_init_data init; init = { }; is more kernel style. > + int ret; > + > + memset(&init, 0, sizeof(init)); And then memset is dropped. > + cluster = devm_kzalloc(dev, sizeof(*cluster), GFP_KERNEL); > + if (!cluster) > + return -ENOMEM; > + > + cluster->dev = dev; > + cluster->reg_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(cluster->reg_base)) > + return PTR_ERR(cluster->reg_base); > + > + hw = &cluster->hw; > + hw->init = &init; > + > + init.name = pdev->name; > + init.num_parents = 0; Drop? > + init.ops = &apple_cluster_clk_ops; > + init.flags = 0; Drop? _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel