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 46D54C433F5 for ; Thu, 25 Nov 2021 06:42:11 +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:References:In-Reply-To: Date:CC:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=qU7f5NjG5Io9wOPeQviWff3drxruPlUWkOuGN/9ABzA=; b=RfKabh9/Hx754e jCf0C5TTD3r1fBtFXpMsIdbnaxqCzpfIK/NTCzBA4luHhUmTN4nfntgF/0V/Of113DNgVzFQTjSoW wqLsVxk9iAdRwYLSQVzONAtWXa3GIE47jLyBujN++tSpyKoQC6w3Lj89izT8WDwTOXCVkU1pikfnC 5qRPSFPFttl16zu/E0Xo+6wDM3X5DbrQraywp46wMrzucacHea+lhDKX70lf8CxlnVjIoZG7toCDq FsPJsRCw53/AG6kdRTlWQZMpHkGxCvGQszKVkxGZA/Aj0TRcyoTJRUBKGlz5Kd+oiZq6kbkADQnWS bouFZSJRGqhr8vnuHkJA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mq8SH-006Whf-Bc; Thu, 25 Nov 2021 06:42:01 +0000 Received: from mailgw02.mediatek.com ([216.200.240.185]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mq8SC-006Weq-Mv; Thu, 25 Nov 2021 06:42:00 +0000 X-UUID: 3fda9e3b00bd4c59a1c673ebabc02d8a-20211124 X-UUID: 3fda9e3b00bd4c59a1c673ebabc02d8a-20211124 Received: from mtkcas66.mediatek.inc [(172.29.193.44)] by mailgw02.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLSv1.2 ECDHE-RSA-AES256-SHA384 256/256) with ESMTP id 818639336; Wed, 24 Nov 2021 23:41:45 -0700 Received: from mtkexhb02.mediatek.inc (172.21.101.103) by MTKMBS62N2.mediatek.inc (172.29.193.42) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Wed, 24 Nov 2021 22:41:43 -0800 Received: from mtkcas11.mediatek.inc (172.21.101.40) by mtkexhb02.mediatek.inc (172.21.101.103) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Thu, 25 Nov 2021 14:41:41 +0800 Received: from mtksdccf07 (172.21.84.99) by mtkcas11.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1497.2 via Frontend Transport; Thu, 25 Nov 2021 14:41:41 +0800 Message-ID: Subject: Re: [PATCH v3 5/7] soc: mediatek: apu: Add apusys and add apu power domain driver From: Flora.Fu To: AngeloGioacchino Del Regno , "Rob Herring" , Matthias Brugger , Michael Turquette , Stephen Boyd CC: Liam Girdwood , Mark Brown , Ikjoon Jang , Chun-Jie Chen , , , , , Date: Thu, 25 Nov 2021 14:41:41 +0800 In-Reply-To: References: <20211023101334.27686-1-flora.fu@mediatek.com> <20211023101334.27686-6-flora.fu@mediatek.com> X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.2 MIME-Version: 1.0 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211124_224156_819775_0CAC313F X-CRM114-Status: GOOD ( 49.21 ) X-BeenThere: linux-mediatek@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-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Tue, 2021-10-26 at 11:57 +0200, AngeloGioacchino Del Regno wrote: > Il 23/10/21 12:13, Flora Fu ha scritto: > > Add the apusys in soc. > > Add driver for apu power domains. > > APU power domain shall be enabled before accessing the > > internal sub modules. > > > > Signed-off-by: Flora Fu > > Hello, > thanks for the patch! However, there are a few things to improve... Hi, Thanks for the comments, I will update the driver into a tristate module and fix related coding defects in the next submission. > > > --- > > drivers/soc/mediatek/Kconfig | 19 + > > drivers/soc/mediatek/Makefile | 1 + > > drivers/soc/mediatek/apusys/Makefile | 2 + > > drivers/soc/mediatek/apusys/mtk-apu-pm.c | 633 > > +++++++++++++++++++++++ > > 4 files changed, 655 insertions(+) > > create mode 100644 drivers/soc/mediatek/apusys/Makefile > > create mode 100644 drivers/soc/mediatek/apusys/mtk-apu-pm.c > > > > diff --git a/drivers/soc/mediatek/Kconfig > > b/drivers/soc/mediatek/Kconfig > > index fdd8bc08569e..d9bac2710494 100644 > > --- a/drivers/soc/mediatek/Kconfig > > +++ b/drivers/soc/mediatek/Kconfig > > @@ -5,6 +5,25 @@ > > menu "MediaTek SoC drivers" > > depends on ARCH_MEDIATEK || COMPILE_TEST > > > > +config MTK_APUSYS > > + bool "MediaTek APUSYS Support" > > + select REGMAP > > + help > > + Say yes here to add support for the MediaTek AI Processing > > Unit > > + Subsystem (APUSYS). > > + The APUSYS is a proprietary hardware in SoC to support AI > > + operations. > > + > > If these config entries are for files in subfolder apusys/, then you > should move > > them to drivers/soc/mediatek/apusys/Kconfig and make sure that said > Kconfig will > > contain a "if MTK_APUSYS" as to include the subconfigs only if the > parent one is > > declared =y. > > > +config MTK_APU_PM > > + bool "MediaTek APU power management support" > > Can we use tristate here? This doesn't look like being a boot- > critical driver, > so it can as well be loaded after rootfs init. > Permitting to compile this as a module will also shrink the kernel > image size > and that's important when you build a generic kernel image, and > that's for both > the MediaTek SoCs case (older SoCs don't need this driver) and for > others. > > > + select REGMAP > > + select PM_GENERIC_DOMAINS if PM > > + help > > + Say yes here to add support for power management control > > + to Mediatek AI Processing Unit Subsystem (APUSYS). > > + APU power domain shall be enabled before accessing the > > + internal sub modules. > > + > > config MTK_CMDQ > > tristate "MediaTek CMDQ Support" > > depends on ARCH_MEDIATEK || COMPILE_TEST > > diff --git a/drivers/soc/mediatek/Makefile > > b/drivers/soc/mediatek/Makefile > > index 90270f8114ed..e46e7a3c21e7 100644 > > --- a/drivers/soc/mediatek/Makefile > > +++ b/drivers/soc/mediatek/Makefile > > @@ -1,4 +1,5 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > +obj-$(CONFIG_MTK_APUSYS) += apusys/ > > obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o > > obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o > > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > > diff --git a/drivers/soc/mediatek/apusys/Makefile > > b/drivers/soc/mediatek/apusys/Makefile > > new file mode 100644 > > index 000000000000..8821c0f0b7b7 > > --- /dev/null > > +++ b/drivers/soc/mediatek/apusys/Makefile > > @@ -0,0 +1,2 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +obj-$(CONFIG_MTK_APU_PM) += mtk-apu-pm.o > > diff --git a/drivers/soc/mediatek/apusys/mtk-apu-pm.c > > b/drivers/soc/mediatek/apusys/mtk-apu-pm.c > > new file mode 100644 > > index 000000000000..828aa9eb6b37 > > --- /dev/null > > +++ b/drivers/soc/mediatek/apusys/mtk-apu-pm.c > > @@ -0,0 +1,633 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2021 MediaTek Inc. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define APU_PD_IPUIF_HW_CG BIT(0) > > +#define APU_PD_RPC_AUTO_BUCK BIT(1) > > +#define APU_PD_CAPS(_pd, _x) ((_pd)->data->caps & > > (_x)) > > + > > +#define MTK_POLL_DELAY_US 10 > > +#define MTK_POLL_TIMEOUT USEC_PER_SEC > > + > > +/* rpc_top_con*/ > > +#define SLEEP_REQ BIT(0) > > +#define APU_BUCK_ELS_EN BIT(3) > > + > > +/*conn_clr, conn1_clr, vcore_clr */ > > +#define CG_CLR (0xFFFFFFFF) > > + > > +/* mt8192 rpc_sw_type */ > > +#define MT8192_RPC_SW_TYPE0 (0x200) > > +#define MT8192_RPC_SW_TYPE1 (0x210) > > +#define MT8192_RPC_SW_TYPE2 (0x220) > > +#define MT8192_RPC_SW_TYPE3 (0x230) > > +#define MT8192_RPC_SW_TYPE4 (0x240) > > +#define MT8192_RPC_SW_TYPE6 (0x260) > > +#define MT8192_RPC_SW_TYPE7 (0x270) > > What about... > > /* mt8192 rpc_sw_type (0..7) */ > #define MT8192_RPC_SW_TYPE(x) (0x200 + (x * 0x10)) > > > + > > +/* rpc_sw_type*/ > > +static const struct reg_sequence mt8192_rpc_sw_type[] = { > > + { MT8192_RPC_SW_TYPE0, 0xFF }, > > + { MT8192_RPC_SW_TYPE2, 0x7 }, > > + { MT8192_RPC_SW_TYPE3, 0x7 }, > > + { MT8192_RPC_SW_TYPE6, 0x3 }, > > +}; > > + > > +struct apu_top_domain { > > + u32 spm_ext_buck_iso; > > + u32 spm_ext_buck_iso_mask; > > + u32 spm_cross_wake_m01; > > + u32 wake_apu; > > + u32 spm_other_pwr; > > + u32 pwr_status; > > + u32 conn_clr; > > + u32 conn1_clr; > > + u32 vcore_clr; > > + u32 rpc_top_con; > > + u32 rpc_top_con_init_mask; > > + u32 rpc_top_sel; > > + u32 rpc_top_intf_pwr_rdy; > > + u32 pwr_rdy; > > + const struct reg_sequence *rpc_sw_type; > > + int rpc_sw_ntype; > > For the sake of clarity and readability, I would name this one > num_rpc_sw_type > or num_rpc_sw, which also keeps naming consistency across this file, > since later > in this file, you're doing the same with domains_data. > > > +}; > > + > > +static struct apu_top_domain mt8192_top_reg = { > > + .spm_ext_buck_iso = 0x39C, > > + .spm_ext_buck_iso_mask = 0x21, > > + .spm_cross_wake_m01 = 0x670, > > + .wake_apu = BIT(0), > > + .spm_other_pwr = 0x178, > > + .pwr_status = BIT(5), > > + .conn_clr = 0x008, > > Please drop leading zeroes. 0x8 here. > > > + .vcore_clr = 0x008, > > 0x8 > > > + .rpc_top_con = 0x000, > > 0 > > > + .rpc_top_con_init_mask = 0x49E, > > + .rpc_top_sel = 0x004, > > 0x4 > > > + .rpc_top_intf_pwr_rdy = 0x044, > > 0x44 > > > + .pwr_rdy = BIT(0), > > + .rpc_sw_type = mt8192_rpc_sw_type, > > + .rpc_sw_ntype = ARRAY_SIZE(mt8192_rpc_sw_type), > > +}; > > + > > +struct apusys { > > + struct device *dev; > > + struct regmap *scpsys; > > + struct regmap *conn; > > + struct regmap *conn1; > > + struct regmap *vcore; > > + struct regmap *rpc; > > + struct regulator *vsram_supply; > > + const struct apu_pm_data *data; > > + struct genpd_onecell_data pd_data; > > + struct generic_pm_domain *domains[]; > > +}; > > + > > +struct apu_domain { > > + struct generic_pm_domain genpd; > > + const struct apu_domain_data *data; > > + struct apusys *apusys; > > + struct regulator *domain_supply; > > + int num_clks; > > + struct clk_bulk_data *clks; > > + struct clk *clk_top_conn; > > + struct clk *clk_top_ipu_if; > > + struct clk *clk_off; > > + struct clk *clk_on_def; > > +}; > > + > > +struct apu_domain_data { > > + int domain_idx; > > + char *name; > > + struct apu_top_domain *topd; > > + u8 caps; > > +}; > > + > > +struct apu_pm_data { > > + const struct apu_domain_data *domains_data; > > + int num_domains; > > +}; > > + > > +#define to_apu_domain(gpd) container_of(gpd, struct apu_domain, > > genpd) > > + > > +static int apu_top_init_hw(struct apu_domain *pd) > > +{ > > + struct apusys *apusys = pd->apusys; > > + int ret; > > + > > + /* > > + * set memory type to PD or sleep group > > + * sw_type register for each memory group, set to PD mode > > default > > + */ > > + if (pd->data->topd->rpc_sw_ntype) { > > + ret = regmap_register_patch(apusys->rpc, > > + pd->data->topd- > > >rpc_sw_type, > > + pd->data->topd- > > >rpc_sw_ntype); > > + if (ret < 0) { > > + dev_err(apusys->dev, "Failed to rpc patch: > > %d\n", ret); > > + return ret; > > + } > > + } > > + > > + /* mask RPC IRQ and bypass WFI */ > > + regmap_set_bits(apusys->rpc, pd->data->topd->rpc_top_sel, > > + pd->data->topd->rpc_top_con_init_mask); > > This function may return a failure.. and this is not a performance- > critical case, > nor a case in which it's 100% granted that this won't fail: you're > calling this > at initialization phase (when you're adding the domain), so if > there's anything > that would make this fail, this is definitely the right place to > check for that. > > Please check the return value. > > > + > > + if (APU_PD_CAPS(pd, APU_PD_RPC_AUTO_BUCK)) > > + regmap_set_bits(apusys->rpc, > > + pd->data->topd->rpc_top_con, > > APU_BUCK_ELS_EN); > > + > > + return 0; > > +} > > + > > +static const struct apu_domain_data apu_domain_data_mt8192[] = { > > + { > > + .domain_idx = 0, > > + .name = "apu-top", > > + .caps = APU_PD_IPUIF_HW_CG, > > + .topd = &mt8192_top_reg, > > + } > > +}; > > + > > +static const struct apu_pm_data mt8192_apu_pm_data = { > > + .domains_data = apu_domain_data_mt8192, > > + .num_domains = ARRAY_SIZE(apu_domain_data_mt8192), > > +}; > > + > > +static int apu_top_power_on(struct generic_pm_domain *genpd) > > +{ > > + struct apu_domain *pd = to_apu_domain(genpd); > > + struct apusys *apusys = pd->apusys; > > + int ret, tmp; > > + > > + if (apusys->vsram_supply) { > > + ret = regulator_enable(apusys->vsram_supply); > > + if (ret < 0) > > + return ret; > > + } > > + > > + if (pd->domain_supply) { > > + ret = regulator_enable(pd->domain_supply); > > + if (ret < 0) > > + goto err_regulator; > > + } > > + > > + regmap_clear_bits(apusys->scpsys, pd->data->topd- > > >spm_ext_buck_iso, > > + pd->data->topd->spm_ext_buck_iso_mask); > > + > > + regmap_set_bits(apusys->rpc, pd->data->topd->rpc_top_sel, > > + pd->data->topd->rpc_top_con_init_mask); > > + > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + ret = clk_prepare_enable(pd->clk_top_conn); > > + if (ret) { > > + dev_err(apusys->dev, "Failed enable top_conn > > clk\n"); > > + goto err_clk; > > + } > > + > > + ret = clk_set_parent(pd->clk_top_ipu_if, pd- > > >clk_on_def); > > + if (ret) { > > + dev_err(apusys->dev, "Failed set ipu_if > > mux\n"); > > + goto err_clk; > > + } > > + > > + /* The ipu_if clock is gatting by HW. Only enable once. > > */ > > typo: gating > > > + if (!__clk_is_enabled(pd->clk_top_ipu_if)) { > > + ret = clk_prepare_enable(pd->clk_top_ipu_if); > > + if (ret) { > > + dev_err(apusys->dev, "Failed enable > > ipu_if\n"); > > + goto err_clk; > > + } > > + } > > + } else { > > + ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks); > > + if (ret) > > + goto err_clk; > > + } > > + > > + regmap_set_bits(apusys->scpsys, > > + pd->data->topd->spm_cross_wake_m01, > > + pd->data->topd->wake_apu); > > + > > + ret = regmap_read_poll_timeout(apusys->scpsys, > > + pd->data->topd->spm_other_pwr, > > + tmp, > > + (tmp & pd->data->topd- > > >pwr_status) == > > + pd->data->topd->pwr_status, > > + MTK_POLL_DELAY_US, > > MTK_POLL_TIMEOUT); > > + if (ret < 0) { > > + dev_err(apusys->dev, "apu top on wait SPM PWR_ACK != > > 0\n"); > > + goto err_clk; > > + } > > + > > + ret = regmap_read_poll_timeout(apusys->rpc, > > + pd->data->topd- > > >rpc_top_intf_pwr_rdy, > > + tmp, (tmp & pd->data->topd- > > >pwr_rdy) == > > + pd->data->topd->pwr_rdy, > > + MTK_POLL_DELAY_US, > > MTK_POLL_TIMEOUT); > > + if (ret < 0) { > > + dev_err(apusys->dev, "apu top on wait RPC PWR_RDY != > > 0\n"); > > + goto err_clk; > > + } > > + > > + if (apusys->vcore) > > + regmap_write(apusys->vcore, pd->data->topd->vcore_clr, > > CG_CLR); > > + > > + if (apusys->conn) > > + regmap_write(apusys->conn, pd->data->topd->conn_clr, > > CG_CLR); > > + > > + if (apusys->conn1) > > + regmap_write(apusys->conn1, pd->data->topd->conn1_clr, > > CG_CLR); > > + > > + return 0; > > + > > +err_clk: > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + clk_disable_unprepare(pd->clk_top_conn); > > + clk_disable_unprepare(pd->clk_top_ipu_if); > > + } else { > > + clk_bulk_disable_unprepare(pd->num_clks, pd->clks); > > + } > > + if (pd->domain_supply) > > + ret = regulator_disable(pd->domain_supply); > > +err_regulator: > > + if (apusys->vsram_supply) > > + ret = regulator_disable(apusys->vsram_supply); > > + > > + return ret; > > +} > > + > > +static int apu_top_power_off(struct generic_pm_domain *genpd) > > +{ > > + struct apu_domain *pd = to_apu_domain(genpd); > > + struct apusys *apusys = pd->apusys; > > + int ret, tmp; > > + > > + if (apusys->vcore) > > + regmap_write(apusys->vcore, pd->data->topd->vcore_clr, > > CG_CLR); > > + > > + if (apusys->conn) > > + regmap_write(apusys->conn, pd->data->topd->conn_clr, > > CG_CLR); > > + > > + if (apusys->conn1) > > + regmap_write(apusys->conn1, pd->data->topd->conn1_clr, > > CG_CLR); > > + > > + regmap_clear_bits(apusys->scpsys, > > + pd->data->topd->spm_cross_wake_m01, > > + pd->data->topd->wake_apu); > > + > > + regmap_set_bits(apusys->rpc, pd->data->topd->rpc_top_con, > > SLEEP_REQ); > > + > > + ret = regmap_read_poll_timeout(apusys->rpc, > > + pd->data->topd- > > >rpc_top_intf_pwr_rdy, > > + tmp, > > + (tmp & pd->data->topd->pwr_rdy) > > == 0x0, > > + MTK_POLL_DELAY_US, > > MTK_POLL_TIMEOUT); > > + if (ret < 0) { > > + dev_err(apusys->dev, "apu top off wait RPC PWR_RDY != > > 0\n"); > > + return ret; > > + } > > + > > + ret = regmap_read_poll_timeout(apusys->scpsys, > > + pd->data->topd->spm_other_pwr, > > tmp, > > + (tmp & pd->data->topd- > > >pwr_status) == 0x0, > > + MTK_POLL_DELAY_US, > > MTK_POLL_TIMEOUT); > > + if (ret < 0) { > > + dev_err(apusys->dev, "apu top off wait SPM PWR_ACK != > > 0\n"); > > + return ret; > > + } > > + > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + clk_disable_unprepare(pd->clk_top_conn); > > + ret = clk_set_parent(pd->clk_top_ipu_if, pd->clk_off); > > + if (ret) { > > + dev_err(apusys->dev, "Failed set ipu_if > > rate\n"); > > + return ret; > > + } > > + } else { > > + clk_bulk_disable_unprepare(pd->num_clks, pd->clks); > > + } > > + > > + regmap_set_bits(apusys->scpsys, > > + pd->data->topd->spm_ext_buck_iso, > > + pd->data->topd->spm_ext_buck_iso_mask); > > + > > + if (apusys->vsram_supply) { > > + ret = regulator_disable(apusys->vsram_supply); > > + if (ret < 0) > > + return ret; > > + } > > + > > + if (pd->domain_supply) { > > + ret = regulator_disable(pd->domain_supply); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static struct generic_pm_domain *apu_add_one_domain(struct apusys > > *apusys, > > + struct device_node > > *node) > > +{ > > + const struct apu_domain_data *domain_data; > > + struct apu_domain *pd; > > + int ret; > > + u32 id; > > + int i; > > + struct clk *clk; > > + > > + ret = of_property_read_u32(node, "reg", &id); > > + if (ret) { > > + dev_dbg(apusys->dev, "%pOF: invalid reg: %d\n", node, > > ret); > > + return ERR_PTR(-EINVAL); > > Just use the error value in 'ret', instead of forcing -EINVAL here. > > > + } > > + > > + if (id >= apusys->data->num_domains) { > > + dev_dbg(apusys->dev, "%pOF: invalid id %d\n", node, > > id); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + domain_data = &apusys->data->domains_data[id]; > > + > > + pd = devm_kzalloc(apusys->dev, sizeof(*pd), GFP_KERNEL); > > + if (!pd) > > + return ERR_PTR(-ENOMEM); > > + > > + pd->data = domain_data; > > + pd->apusys = apusys; > > + > > + pd->domain_supply = devm_regulator_get_optional(apusys->dev, > > "domain"); > > This may return -EPROBE_DEFER: in that case, you don't want to ignore > the supply, > but defer probing this driver until said supply becomes available so, > if that > happens, you should return. > > > + if (IS_ERR(pd->domain_supply)) > > + pd->domain_supply = NULL; > > + > > + /* > > + * For HW using ipu_if, the clock is switched to 26M > > + * when power down top domain and switch to default clock rate > > + * before power on. > > + */ > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + pd->clk_top_conn = of_clk_get_by_name(node, > > "clk_top_conn"); > > + if (IS_ERR(pd->clk_top_conn)) { > > + dev_err(apusys->dev, "Fail to get clk_top_conn > > clock\n"); > > + ret = PTR_ERR(pd->clk_top_conn); > > + goto err_put_clocks; > > + } > > + > > + pd->clk_top_ipu_if = of_clk_get_by_name(node, > > "clk_top_ipu_if"); > > + if (IS_ERR(pd->clk_top_ipu_if)) { > > + dev_err(apusys->dev, "Fail to get > > clk_top_ipu_if clock\n"); > > + ret = PTR_ERR(pd->clk_top_ipu_if); > > + goto err_put_clocks; > > + } > > + > > + pd->clk_off = of_clk_get_by_name(node, "clk_off"); > > + if (IS_ERR(pd->clk_off)) { > > + dev_err(apusys->dev, "Fail to get clk_off > > clock\n"); > > + ret = PTR_ERR(pd->clk_off); > > + goto err_put_clocks; > > + } > > + > > + pd->clk_on_def = of_clk_get_by_name(node, > > "clk_on_default"); > > + if (IS_ERR(pd->clk_on_def)) { > > + dev_err(apusys->dev, "Fail to get > > clk_on_default clock\n"); > > + ret = PTR_ERR(pd->clk_on_def); > > + goto err_put_clocks; > > + } > > + } else { > > + pd->num_clks = of_clk_get_parent_count(node); > > + if (pd->num_clks > 0) { > > + pd->clks = devm_kcalloc(apusys->dev, pd- > > >num_clks, > > + sizeof(*pd->clks), > > GFP_KERNEL); > > + if (!pd->clks) > > + return ERR_PTR(-ENOMEM); > > + } > > + > > + for (i = 0; i < pd->num_clks; i++) { > > + clk = of_clk_get(node, i); > > + if (IS_ERR(clk)) { > > + ret = PTR_ERR(clk); > > + dev_dbg(apusys->dev, > > + "%pOF: failed to get clk at > > index %d: %d\n", > > + node, i, ret); > > + goto err_put_clocks; > > + } > > + pd->clks[i].clk = clk; > > + } > > + } > > + > > + if (apusys->domains[id]) { > > + ret = -EINVAL; > > Error: X already exists -> -EEXIST > > > + dev_err(apusys->dev, "domain id %d already exists\n", > > id); > > + goto err_put_clocks; > > + } > > + > > + /* set rpc hw init status */ > > + ret = apu_top_init_hw(pd); > > + if (ret < 0) { > > + dev_dbg(apusys->dev, "top init fail ret = %d\n", ret); > > + goto err_put_clocks; > > + } > > + > > + if (!pd->data->name) > > + pd->genpd.name = node->name; > > + else > > + pd->genpd.name = pd->data->name; > > + pd->genpd.power_off = apu_top_power_off; > > + pd->genpd.power_on = apu_top_power_on; > > + > > + /* > > + * Initially turn on all domains to make the domains usable > > + * with !CONFIG_PM and to get the hardware in sync with the > > + * software. The unused domains will be switched off during > > + * late_init time. > > + */ > > + ret = pd->genpd.power_on(&pd->genpd); > > + if (ret < 0) { > > + dev_dbg(apusys->dev, "%pOF: power on domain fail: > > %d\n", > > + node, ret); > > + goto err_put_clocks; > > + } > > + > > + pm_genpd_init(&pd->genpd, NULL, false); > > This function may return failure: please check the return value. > > > + > > + apusys->domains[id] = &pd->genpd; > > + > > + return apusys->pd_data.domains[id]; > > + > > +err_put_clocks: > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + clk_put(pd->clk_top_conn); > > + clk_put(pd->clk_top_ipu_if); > > + clk_put(pd->clk_off); > > + clk_put(pd->clk_on_def); > > + } else { > > + clk_bulk_put(pd->num_clks, pd->clks); > > + } > > + return ERR_PTR(ret); > > +} > > + > > +static void apu_remove_one_domain(struct apu_domain *pd) > > +{ > > + int ret; > > + > > + if (pd->genpd.power_off) > > + pd->genpd.power_off(&pd->genpd); > > + > > + /* > > + * We're in the error cleanup already, so we only complain, > > + * but won't emit another error on top of the original one. > > + */ > > + ret = pm_genpd_remove(&pd->genpd); > > + if (ret < 0) > > + dev_dbg(pd->apusys->dev, > > + "Remove domain '%s' : %d - state may be > > inconsistent\n", > > + pd->genpd.name, ret); > > + > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + clk_put(pd->clk_top_conn); > > + clk_put(pd->clk_top_ipu_if); > > + clk_put(pd->clk_off); > > + clk_put(pd->clk_on_def); > > + } else { > > + clk_bulk_put(pd->num_clks, pd->clks); > > + } > > +} > > + > > +static void apu_domain_cleanup(struct apusys *apusys) > > +{ > > + struct generic_pm_domain *genpd; > > + struct apu_domain *pd; > > + int i; > > + > > + for (i = apusys->pd_data.num_domains - 1; i >= 0; i--) { > > + genpd = apusys->pd_data.domains[i]; > > + if (genpd) { > > + pd = to_apu_domain(genpd); > > + apu_remove_one_domain(pd); > > + } > > + } > > +} > > + > > +static const struct of_device_id apu_pm_of_match[] = { > > + { > > + .compatible = "mediatek,mt8192-apu-pm", > > + .data = &mt8192_apu_pm_data, > > + }, > > + { } > > +}; > > + > > +static int apu_pm_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + const struct apu_pm_data *data; > > + struct device_node *node; > > + struct apusys *apusys; > > + int ret; > > + > > + data = of_device_get_match_data(&pdev->dev); > > + if (!data) { > > + dev_dbg(dev, "no power domain data\n"); > > + return -EINVAL; > > + } > > + > > + apusys = devm_kzalloc(dev, > > + struct_size(apusys, domains, data- > > >num_domains), > > + GFP_KERNEL); > > + if (!apusys) > > + return -ENOMEM; > > + > > + apusys->dev = dev; > > + apusys->data = data; > > + apusys->pd_data.domains = apusys->domains; > > + apusys->pd_data.num_domains = data->num_domains; > > + > > + apusys->vsram_supply = devm_regulator_get_optional(dev, > > "vsram"); > > + if (IS_ERR(apusys->vsram_supply)) { > > + ret = PTR_ERR(apusys->vsram_supply); > > + if (ret != -EPROBE_DEFER) { > > + dev_err(dev, "vsram_supply fail, ret=%d", ret); > > + apusys->vsram_supply = NULL; > > + } > > + return ret; > > + } > > + > > + /* rpc */ > > + apusys->rpc = syscon_node_to_regmap(np); > > + if (IS_ERR(apusys->rpc)) { > > + dev_err(dev, "Unable to get rpc\n"); > > + return PTR_ERR(apusys->rpc); > > + } > > + > > + /* scpsys */ > > + apusys->scpsys = syscon_regmap_lookup_by_phandle(np, > > "mediatek,scpsys"); > > + if (IS_ERR(apusys->scpsys)) { > > + dev_err(dev, "Unable to get scpsys\n"); > > + return PTR_ERR(apusys->scpsys); > > + } > > + > > + /* apusys conn */ > > + apusys->conn = syscon_regmap_lookup_by_phandle_optional(np, > > "mediatek,apu-conn"); > > + if (IS_ERR(apusys->conn)) > > + dev_info(dev, "No optional phandle apu-conn\n"); > > I take it as if some platforms may be expected to not have any > optional phandle > for that (and for the others): in such case, this would result in > being a useless > message. Please use dev_dbg() here. > > > + > > + /* apusys conn1 */ > > + apusys->conn1 = syscon_regmap_lookup_by_phandle_optional(np, > > "mediatek,apu-conn1"); > > + if (IS_ERR(apusys->conn1)) > > + dev_info(dev, "No optional phandle apu-conn1\n"); > > + > > + /* apusys vcore */ > > + apusys->vcore = syscon_regmap_lookup_by_phandle_optional(np, > > "mediatek,apu-vcore"); > > + if (IS_ERR(apusys->vcore)) > > + dev_info(dev, "No optional phandle apu-vcore\n"); > > + > > + for_each_available_child_of_node(np, node) { > > + struct generic_pm_domain *domain; > > + > > + domain = apu_add_one_domain(apusys, node); > > + if (IS_ERR(domain)) { > > + ret = PTR_ERR(domain); > > + of_node_put(node); > > + goto err_cleanup_domains; > > + } > > + } > > + > > + ret = of_genpd_add_provider_onecell(np, &apusys->pd_data); > > + if (ret) { > > + dev_dbg(dev, "failed to add provider: %d\n", ret); > > + goto err_cleanup_domains; > > + } > > + > > + return 0; > > + > > +err_cleanup_domains: > > + apu_domain_cleanup(apusys); > > + return ret; > > +} > > + > > +static struct platform_driver apu_pm_driver = { > > + .probe = apu_pm_probe, > > In this driver, you already have the means to remove the domains and > cleanup the > state... it would probably be trivial to add a .remove handler. > > What do you think about that? > Also, is there be any reason for which removing this driver during > runtime > would be unsafe? > > In my opinion, it would be beneficial to have a remove handler also > in the > development usecase, as you'd be able to just recompile this module, > remove and > reinsert it to test new changes, without the need of copying an > entire new > kernel image and rebooting the device. > > > + .driver = { > > + .name = "mtk-apu-pm", > > + .suppress_bind_attrs = true, > > + .of_match_table = apu_pm_of_match, > > + }, > > +}; > > +builtin_platform_driver(apu_pm_driver); > > > > MODULE_LICENSE() is missing! Please add it. > Also, as said in the Kconfig comment, this driver can probably be a > module, in > which case... module_platform_driver(apu_pm_driver); > > Thanks, > - Angelo > _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek 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 BED8BC433EF for ; Thu, 25 Nov 2021 06:43:28 +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:References:In-Reply-To: Date:CC:To:From:Subject:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=AvMpL7yR3IKH7KAC+b7ViCEwHeL5X1BIfPwB//pOwhc=; b=wtVU9ezmYyqaye /QQj4CECneQpjq6Pjv47Ta2psmZPnZ3tFgyrVBfDizlnQuCLXF5sKIUpC2Urqmm1lFd+6tB13u37D 9K1iIpsebtwjo+BKYiDVO3UBn5JLc9vtTegqtkqACDO8puJafjRU7NVXTEuTlLN1Sl4wexTrp16o0 CvuBV35ZlEsIGezdlJZ30/bGS5MLQvaLPqbvMdrV6P5yDrO+xEorzV0AL+1VLa74lrnXlkxeBsw+h cOJb4vu918LQ0KPxKhIIsyny69ndWZ61SRbIH+GwkT61bGpM1CxpT15TTRdIjxVECuyKk3lwUdSW+ Gz/HAs3KNZJ49umxZVOg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mq8SL-006Wi9-5H; Thu, 25 Nov 2021 06:42:05 +0000 Received: from mailgw02.mediatek.com ([216.200.240.185]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mq8SC-006Weq-Mv; Thu, 25 Nov 2021 06:42:00 +0000 X-UUID: 3fda9e3b00bd4c59a1c673ebabc02d8a-20211124 X-UUID: 3fda9e3b00bd4c59a1c673ebabc02d8a-20211124 Received: from mtkcas66.mediatek.inc [(172.29.193.44)] by mailgw02.mediatek.com (envelope-from ) (musrelay.mediatek.com ESMTP with TLSv1.2 ECDHE-RSA-AES256-SHA384 256/256) with ESMTP id 818639336; Wed, 24 Nov 2021 23:41:45 -0700 Received: from mtkexhb02.mediatek.inc (172.21.101.103) by MTKMBS62N2.mediatek.inc (172.29.193.42) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Wed, 24 Nov 2021 22:41:43 -0800 Received: from mtkcas11.mediatek.inc (172.21.101.40) by mtkexhb02.mediatek.inc (172.21.101.103) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Thu, 25 Nov 2021 14:41:41 +0800 Received: from mtksdccf07 (172.21.84.99) by mtkcas11.mediatek.inc (172.21.101.73) with Microsoft SMTP Server id 15.0.1497.2 via Frontend Transport; Thu, 25 Nov 2021 14:41:41 +0800 Message-ID: Subject: Re: [PATCH v3 5/7] soc: mediatek: apu: Add apusys and add apu power domain driver From: Flora.Fu To: AngeloGioacchino Del Regno , "Rob Herring" , Matthias Brugger , Michael Turquette , Stephen Boyd CC: Liam Girdwood , Mark Brown , Ikjoon Jang , Chun-Jie Chen , , , , , Date: Thu, 25 Nov 2021 14:41:41 +0800 In-Reply-To: References: <20211023101334.27686-1-flora.fu@mediatek.com> <20211023101334.27686-6-flora.fu@mediatek.com> X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.2 MIME-Version: 1.0 X-MTK: N X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211124_224156_819775_0CAC313F X-CRM114-Status: GOOD ( 49.21 ) 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 On Tue, 2021-10-26 at 11:57 +0200, AngeloGioacchino Del Regno wrote: > Il 23/10/21 12:13, Flora Fu ha scritto: > > Add the apusys in soc. > > Add driver for apu power domains. > > APU power domain shall be enabled before accessing the > > internal sub modules. > > > > Signed-off-by: Flora Fu > > Hello, > thanks for the patch! However, there are a few things to improve... Hi, Thanks for the comments, I will update the driver into a tristate module and fix related coding defects in the next submission. > > > --- > > drivers/soc/mediatek/Kconfig | 19 + > > drivers/soc/mediatek/Makefile | 1 + > > drivers/soc/mediatek/apusys/Makefile | 2 + > > drivers/soc/mediatek/apusys/mtk-apu-pm.c | 633 > > +++++++++++++++++++++++ > > 4 files changed, 655 insertions(+) > > create mode 100644 drivers/soc/mediatek/apusys/Makefile > > create mode 100644 drivers/soc/mediatek/apusys/mtk-apu-pm.c > > > > diff --git a/drivers/soc/mediatek/Kconfig > > b/drivers/soc/mediatek/Kconfig > > index fdd8bc08569e..d9bac2710494 100644 > > --- a/drivers/soc/mediatek/Kconfig > > +++ b/drivers/soc/mediatek/Kconfig > > @@ -5,6 +5,25 @@ > > menu "MediaTek SoC drivers" > > depends on ARCH_MEDIATEK || COMPILE_TEST > > > > +config MTK_APUSYS > > + bool "MediaTek APUSYS Support" > > + select REGMAP > > + help > > + Say yes here to add support for the MediaTek AI Processing > > Unit > > + Subsystem (APUSYS). > > + The APUSYS is a proprietary hardware in SoC to support AI > > + operations. > > + > > If these config entries are for files in subfolder apusys/, then you > should move > > them to drivers/soc/mediatek/apusys/Kconfig and make sure that said > Kconfig will > > contain a "if MTK_APUSYS" as to include the subconfigs only if the > parent one is > > declared =y. > > > +config MTK_APU_PM > > + bool "MediaTek APU power management support" > > Can we use tristate here? This doesn't look like being a boot- > critical driver, > so it can as well be loaded after rootfs init. > Permitting to compile this as a module will also shrink the kernel > image size > and that's important when you build a generic kernel image, and > that's for both > the MediaTek SoCs case (older SoCs don't need this driver) and for > others. > > > + select REGMAP > > + select PM_GENERIC_DOMAINS if PM > > + help > > + Say yes here to add support for power management control > > + to Mediatek AI Processing Unit Subsystem (APUSYS). > > + APU power domain shall be enabled before accessing the > > + internal sub modules. > > + > > config MTK_CMDQ > > tristate "MediaTek CMDQ Support" > > depends on ARCH_MEDIATEK || COMPILE_TEST > > diff --git a/drivers/soc/mediatek/Makefile > > b/drivers/soc/mediatek/Makefile > > index 90270f8114ed..e46e7a3c21e7 100644 > > --- a/drivers/soc/mediatek/Makefile > > +++ b/drivers/soc/mediatek/Makefile > > @@ -1,4 +1,5 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > +obj-$(CONFIG_MTK_APUSYS) += apusys/ > > obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o > > obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o > > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > > diff --git a/drivers/soc/mediatek/apusys/Makefile > > b/drivers/soc/mediatek/apusys/Makefile > > new file mode 100644 > > index 000000000000..8821c0f0b7b7 > > --- /dev/null > > +++ b/drivers/soc/mediatek/apusys/Makefile > > @@ -0,0 +1,2 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > +obj-$(CONFIG_MTK_APU_PM) += mtk-apu-pm.o > > diff --git a/drivers/soc/mediatek/apusys/mtk-apu-pm.c > > b/drivers/soc/mediatek/apusys/mtk-apu-pm.c > > new file mode 100644 > > index 000000000000..828aa9eb6b37 > > --- /dev/null > > +++ b/drivers/soc/mediatek/apusys/mtk-apu-pm.c > > @@ -0,0 +1,633 @@ > > +// SPDX-License-Identifier: GPL-2.0-only > > +/* > > + * Copyright (c) 2021 MediaTek Inc. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define APU_PD_IPUIF_HW_CG BIT(0) > > +#define APU_PD_RPC_AUTO_BUCK BIT(1) > > +#define APU_PD_CAPS(_pd, _x) ((_pd)->data->caps & > > (_x)) > > + > > +#define MTK_POLL_DELAY_US 10 > > +#define MTK_POLL_TIMEOUT USEC_PER_SEC > > + > > +/* rpc_top_con*/ > > +#define SLEEP_REQ BIT(0) > > +#define APU_BUCK_ELS_EN BIT(3) > > + > > +/*conn_clr, conn1_clr, vcore_clr */ > > +#define CG_CLR (0xFFFFFFFF) > > + > > +/* mt8192 rpc_sw_type */ > > +#define MT8192_RPC_SW_TYPE0 (0x200) > > +#define MT8192_RPC_SW_TYPE1 (0x210) > > +#define MT8192_RPC_SW_TYPE2 (0x220) > > +#define MT8192_RPC_SW_TYPE3 (0x230) > > +#define MT8192_RPC_SW_TYPE4 (0x240) > > +#define MT8192_RPC_SW_TYPE6 (0x260) > > +#define MT8192_RPC_SW_TYPE7 (0x270) > > What about... > > /* mt8192 rpc_sw_type (0..7) */ > #define MT8192_RPC_SW_TYPE(x) (0x200 + (x * 0x10)) > > > + > > +/* rpc_sw_type*/ > > +static const struct reg_sequence mt8192_rpc_sw_type[] = { > > + { MT8192_RPC_SW_TYPE0, 0xFF }, > > + { MT8192_RPC_SW_TYPE2, 0x7 }, > > + { MT8192_RPC_SW_TYPE3, 0x7 }, > > + { MT8192_RPC_SW_TYPE6, 0x3 }, > > +}; > > + > > +struct apu_top_domain { > > + u32 spm_ext_buck_iso; > > + u32 spm_ext_buck_iso_mask; > > + u32 spm_cross_wake_m01; > > + u32 wake_apu; > > + u32 spm_other_pwr; > > + u32 pwr_status; > > + u32 conn_clr; > > + u32 conn1_clr; > > + u32 vcore_clr; > > + u32 rpc_top_con; > > + u32 rpc_top_con_init_mask; > > + u32 rpc_top_sel; > > + u32 rpc_top_intf_pwr_rdy; > > + u32 pwr_rdy; > > + const struct reg_sequence *rpc_sw_type; > > + int rpc_sw_ntype; > > For the sake of clarity and readability, I would name this one > num_rpc_sw_type > or num_rpc_sw, which also keeps naming consistency across this file, > since later > in this file, you're doing the same with domains_data. > > > +}; > > + > > +static struct apu_top_domain mt8192_top_reg = { > > + .spm_ext_buck_iso = 0x39C, > > + .spm_ext_buck_iso_mask = 0x21, > > + .spm_cross_wake_m01 = 0x670, > > + .wake_apu = BIT(0), > > + .spm_other_pwr = 0x178, > > + .pwr_status = BIT(5), > > + .conn_clr = 0x008, > > Please drop leading zeroes. 0x8 here. > > > + .vcore_clr = 0x008, > > 0x8 > > > + .rpc_top_con = 0x000, > > 0 > > > + .rpc_top_con_init_mask = 0x49E, > > + .rpc_top_sel = 0x004, > > 0x4 > > > + .rpc_top_intf_pwr_rdy = 0x044, > > 0x44 > > > + .pwr_rdy = BIT(0), > > + .rpc_sw_type = mt8192_rpc_sw_type, > > + .rpc_sw_ntype = ARRAY_SIZE(mt8192_rpc_sw_type), > > +}; > > + > > +struct apusys { > > + struct device *dev; > > + struct regmap *scpsys; > > + struct regmap *conn; > > + struct regmap *conn1; > > + struct regmap *vcore; > > + struct regmap *rpc; > > + struct regulator *vsram_supply; > > + const struct apu_pm_data *data; > > + struct genpd_onecell_data pd_data; > > + struct generic_pm_domain *domains[]; > > +}; > > + > > +struct apu_domain { > > + struct generic_pm_domain genpd; > > + const struct apu_domain_data *data; > > + struct apusys *apusys; > > + struct regulator *domain_supply; > > + int num_clks; > > + struct clk_bulk_data *clks; > > + struct clk *clk_top_conn; > > + struct clk *clk_top_ipu_if; > > + struct clk *clk_off; > > + struct clk *clk_on_def; > > +}; > > + > > +struct apu_domain_data { > > + int domain_idx; > > + char *name; > > + struct apu_top_domain *topd; > > + u8 caps; > > +}; > > + > > +struct apu_pm_data { > > + const struct apu_domain_data *domains_data; > > + int num_domains; > > +}; > > + > > +#define to_apu_domain(gpd) container_of(gpd, struct apu_domain, > > genpd) > > + > > +static int apu_top_init_hw(struct apu_domain *pd) > > +{ > > + struct apusys *apusys = pd->apusys; > > + int ret; > > + > > + /* > > + * set memory type to PD or sleep group > > + * sw_type register for each memory group, set to PD mode > > default > > + */ > > + if (pd->data->topd->rpc_sw_ntype) { > > + ret = regmap_register_patch(apusys->rpc, > > + pd->data->topd- > > >rpc_sw_type, > > + pd->data->topd- > > >rpc_sw_ntype); > > + if (ret < 0) { > > + dev_err(apusys->dev, "Failed to rpc patch: > > %d\n", ret); > > + return ret; > > + } > > + } > > + > > + /* mask RPC IRQ and bypass WFI */ > > + regmap_set_bits(apusys->rpc, pd->data->topd->rpc_top_sel, > > + pd->data->topd->rpc_top_con_init_mask); > > This function may return a failure.. and this is not a performance- > critical case, > nor a case in which it's 100% granted that this won't fail: you're > calling this > at initialization phase (when you're adding the domain), so if > there's anything > that would make this fail, this is definitely the right place to > check for that. > > Please check the return value. > > > + > > + if (APU_PD_CAPS(pd, APU_PD_RPC_AUTO_BUCK)) > > + regmap_set_bits(apusys->rpc, > > + pd->data->topd->rpc_top_con, > > APU_BUCK_ELS_EN); > > + > > + return 0; > > +} > > + > > +static const struct apu_domain_data apu_domain_data_mt8192[] = { > > + { > > + .domain_idx = 0, > > + .name = "apu-top", > > + .caps = APU_PD_IPUIF_HW_CG, > > + .topd = &mt8192_top_reg, > > + } > > +}; > > + > > +static const struct apu_pm_data mt8192_apu_pm_data = { > > + .domains_data = apu_domain_data_mt8192, > > + .num_domains = ARRAY_SIZE(apu_domain_data_mt8192), > > +}; > > + > > +static int apu_top_power_on(struct generic_pm_domain *genpd) > > +{ > > + struct apu_domain *pd = to_apu_domain(genpd); > > + struct apusys *apusys = pd->apusys; > > + int ret, tmp; > > + > > + if (apusys->vsram_supply) { > > + ret = regulator_enable(apusys->vsram_supply); > > + if (ret < 0) > > + return ret; > > + } > > + > > + if (pd->domain_supply) { > > + ret = regulator_enable(pd->domain_supply); > > + if (ret < 0) > > + goto err_regulator; > > + } > > + > > + regmap_clear_bits(apusys->scpsys, pd->data->topd- > > >spm_ext_buck_iso, > > + pd->data->topd->spm_ext_buck_iso_mask); > > + > > + regmap_set_bits(apusys->rpc, pd->data->topd->rpc_top_sel, > > + pd->data->topd->rpc_top_con_init_mask); > > + > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + ret = clk_prepare_enable(pd->clk_top_conn); > > + if (ret) { > > + dev_err(apusys->dev, "Failed enable top_conn > > clk\n"); > > + goto err_clk; > > + } > > + > > + ret = clk_set_parent(pd->clk_top_ipu_if, pd- > > >clk_on_def); > > + if (ret) { > > + dev_err(apusys->dev, "Failed set ipu_if > > mux\n"); > > + goto err_clk; > > + } > > + > > + /* The ipu_if clock is gatting by HW. Only enable once. > > */ > > typo: gating > > > + if (!__clk_is_enabled(pd->clk_top_ipu_if)) { > > + ret = clk_prepare_enable(pd->clk_top_ipu_if); > > + if (ret) { > > + dev_err(apusys->dev, "Failed enable > > ipu_if\n"); > > + goto err_clk; > > + } > > + } > > + } else { > > + ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks); > > + if (ret) > > + goto err_clk; > > + } > > + > > + regmap_set_bits(apusys->scpsys, > > + pd->data->topd->spm_cross_wake_m01, > > + pd->data->topd->wake_apu); > > + > > + ret = regmap_read_poll_timeout(apusys->scpsys, > > + pd->data->topd->spm_other_pwr, > > + tmp, > > + (tmp & pd->data->topd- > > >pwr_status) == > > + pd->data->topd->pwr_status, > > + MTK_POLL_DELAY_US, > > MTK_POLL_TIMEOUT); > > + if (ret < 0) { > > + dev_err(apusys->dev, "apu top on wait SPM PWR_ACK != > > 0\n"); > > + goto err_clk; > > + } > > + > > + ret = regmap_read_poll_timeout(apusys->rpc, > > + pd->data->topd- > > >rpc_top_intf_pwr_rdy, > > + tmp, (tmp & pd->data->topd- > > >pwr_rdy) == > > + pd->data->topd->pwr_rdy, > > + MTK_POLL_DELAY_US, > > MTK_POLL_TIMEOUT); > > + if (ret < 0) { > > + dev_err(apusys->dev, "apu top on wait RPC PWR_RDY != > > 0\n"); > > + goto err_clk; > > + } > > + > > + if (apusys->vcore) > > + regmap_write(apusys->vcore, pd->data->topd->vcore_clr, > > CG_CLR); > > + > > + if (apusys->conn) > > + regmap_write(apusys->conn, pd->data->topd->conn_clr, > > CG_CLR); > > + > > + if (apusys->conn1) > > + regmap_write(apusys->conn1, pd->data->topd->conn1_clr, > > CG_CLR); > > + > > + return 0; > > + > > +err_clk: > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + clk_disable_unprepare(pd->clk_top_conn); > > + clk_disable_unprepare(pd->clk_top_ipu_if); > > + } else { > > + clk_bulk_disable_unprepare(pd->num_clks, pd->clks); > > + } > > + if (pd->domain_supply) > > + ret = regulator_disable(pd->domain_supply); > > +err_regulator: > > + if (apusys->vsram_supply) > > + ret = regulator_disable(apusys->vsram_supply); > > + > > + return ret; > > +} > > + > > +static int apu_top_power_off(struct generic_pm_domain *genpd) > > +{ > > + struct apu_domain *pd = to_apu_domain(genpd); > > + struct apusys *apusys = pd->apusys; > > + int ret, tmp; > > + > > + if (apusys->vcore) > > + regmap_write(apusys->vcore, pd->data->topd->vcore_clr, > > CG_CLR); > > + > > + if (apusys->conn) > > + regmap_write(apusys->conn, pd->data->topd->conn_clr, > > CG_CLR); > > + > > + if (apusys->conn1) > > + regmap_write(apusys->conn1, pd->data->topd->conn1_clr, > > CG_CLR); > > + > > + regmap_clear_bits(apusys->scpsys, > > + pd->data->topd->spm_cross_wake_m01, > > + pd->data->topd->wake_apu); > > + > > + regmap_set_bits(apusys->rpc, pd->data->topd->rpc_top_con, > > SLEEP_REQ); > > + > > + ret = regmap_read_poll_timeout(apusys->rpc, > > + pd->data->topd- > > >rpc_top_intf_pwr_rdy, > > + tmp, > > + (tmp & pd->data->topd->pwr_rdy) > > == 0x0, > > + MTK_POLL_DELAY_US, > > MTK_POLL_TIMEOUT); > > + if (ret < 0) { > > + dev_err(apusys->dev, "apu top off wait RPC PWR_RDY != > > 0\n"); > > + return ret; > > + } > > + > > + ret = regmap_read_poll_timeout(apusys->scpsys, > > + pd->data->topd->spm_other_pwr, > > tmp, > > + (tmp & pd->data->topd- > > >pwr_status) == 0x0, > > + MTK_POLL_DELAY_US, > > MTK_POLL_TIMEOUT); > > + if (ret < 0) { > > + dev_err(apusys->dev, "apu top off wait SPM PWR_ACK != > > 0\n"); > > + return ret; > > + } > > + > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + clk_disable_unprepare(pd->clk_top_conn); > > + ret = clk_set_parent(pd->clk_top_ipu_if, pd->clk_off); > > + if (ret) { > > + dev_err(apusys->dev, "Failed set ipu_if > > rate\n"); > > + return ret; > > + } > > + } else { > > + clk_bulk_disable_unprepare(pd->num_clks, pd->clks); > > + } > > + > > + regmap_set_bits(apusys->scpsys, > > + pd->data->topd->spm_ext_buck_iso, > > + pd->data->topd->spm_ext_buck_iso_mask); > > + > > + if (apusys->vsram_supply) { > > + ret = regulator_disable(apusys->vsram_supply); > > + if (ret < 0) > > + return ret; > > + } > > + > > + if (pd->domain_supply) { > > + ret = regulator_disable(pd->domain_supply); > > + if (ret < 0) > > + return ret; > > + } > > + > > + return 0; > > +} > > + > > +static struct generic_pm_domain *apu_add_one_domain(struct apusys > > *apusys, > > + struct device_node > > *node) > > +{ > > + const struct apu_domain_data *domain_data; > > + struct apu_domain *pd; > > + int ret; > > + u32 id; > > + int i; > > + struct clk *clk; > > + > > + ret = of_property_read_u32(node, "reg", &id); > > + if (ret) { > > + dev_dbg(apusys->dev, "%pOF: invalid reg: %d\n", node, > > ret); > > + return ERR_PTR(-EINVAL); > > Just use the error value in 'ret', instead of forcing -EINVAL here. > > > + } > > + > > + if (id >= apusys->data->num_domains) { > > + dev_dbg(apusys->dev, "%pOF: invalid id %d\n", node, > > id); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + domain_data = &apusys->data->domains_data[id]; > > + > > + pd = devm_kzalloc(apusys->dev, sizeof(*pd), GFP_KERNEL); > > + if (!pd) > > + return ERR_PTR(-ENOMEM); > > + > > + pd->data = domain_data; > > + pd->apusys = apusys; > > + > > + pd->domain_supply = devm_regulator_get_optional(apusys->dev, > > "domain"); > > This may return -EPROBE_DEFER: in that case, you don't want to ignore > the supply, > but defer probing this driver until said supply becomes available so, > if that > happens, you should return. > > > + if (IS_ERR(pd->domain_supply)) > > + pd->domain_supply = NULL; > > + > > + /* > > + * For HW using ipu_if, the clock is switched to 26M > > + * when power down top domain and switch to default clock rate > > + * before power on. > > + */ > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + pd->clk_top_conn = of_clk_get_by_name(node, > > "clk_top_conn"); > > + if (IS_ERR(pd->clk_top_conn)) { > > + dev_err(apusys->dev, "Fail to get clk_top_conn > > clock\n"); > > + ret = PTR_ERR(pd->clk_top_conn); > > + goto err_put_clocks; > > + } > > + > > + pd->clk_top_ipu_if = of_clk_get_by_name(node, > > "clk_top_ipu_if"); > > + if (IS_ERR(pd->clk_top_ipu_if)) { > > + dev_err(apusys->dev, "Fail to get > > clk_top_ipu_if clock\n"); > > + ret = PTR_ERR(pd->clk_top_ipu_if); > > + goto err_put_clocks; > > + } > > + > > + pd->clk_off = of_clk_get_by_name(node, "clk_off"); > > + if (IS_ERR(pd->clk_off)) { > > + dev_err(apusys->dev, "Fail to get clk_off > > clock\n"); > > + ret = PTR_ERR(pd->clk_off); > > + goto err_put_clocks; > > + } > > + > > + pd->clk_on_def = of_clk_get_by_name(node, > > "clk_on_default"); > > + if (IS_ERR(pd->clk_on_def)) { > > + dev_err(apusys->dev, "Fail to get > > clk_on_default clock\n"); > > + ret = PTR_ERR(pd->clk_on_def); > > + goto err_put_clocks; > > + } > > + } else { > > + pd->num_clks = of_clk_get_parent_count(node); > > + if (pd->num_clks > 0) { > > + pd->clks = devm_kcalloc(apusys->dev, pd- > > >num_clks, > > + sizeof(*pd->clks), > > GFP_KERNEL); > > + if (!pd->clks) > > + return ERR_PTR(-ENOMEM); > > + } > > + > > + for (i = 0; i < pd->num_clks; i++) { > > + clk = of_clk_get(node, i); > > + if (IS_ERR(clk)) { > > + ret = PTR_ERR(clk); > > + dev_dbg(apusys->dev, > > + "%pOF: failed to get clk at > > index %d: %d\n", > > + node, i, ret); > > + goto err_put_clocks; > > + } > > + pd->clks[i].clk = clk; > > + } > > + } > > + > > + if (apusys->domains[id]) { > > + ret = -EINVAL; > > Error: X already exists -> -EEXIST > > > + dev_err(apusys->dev, "domain id %d already exists\n", > > id); > > + goto err_put_clocks; > > + } > > + > > + /* set rpc hw init status */ > > + ret = apu_top_init_hw(pd); > > + if (ret < 0) { > > + dev_dbg(apusys->dev, "top init fail ret = %d\n", ret); > > + goto err_put_clocks; > > + } > > + > > + if (!pd->data->name) > > + pd->genpd.name = node->name; > > + else > > + pd->genpd.name = pd->data->name; > > + pd->genpd.power_off = apu_top_power_off; > > + pd->genpd.power_on = apu_top_power_on; > > + > > + /* > > + * Initially turn on all domains to make the domains usable > > + * with !CONFIG_PM and to get the hardware in sync with the > > + * software. The unused domains will be switched off during > > + * late_init time. > > + */ > > + ret = pd->genpd.power_on(&pd->genpd); > > + if (ret < 0) { > > + dev_dbg(apusys->dev, "%pOF: power on domain fail: > > %d\n", > > + node, ret); > > + goto err_put_clocks; > > + } > > + > > + pm_genpd_init(&pd->genpd, NULL, false); > > This function may return failure: please check the return value. > > > + > > + apusys->domains[id] = &pd->genpd; > > + > > + return apusys->pd_data.domains[id]; > > + > > +err_put_clocks: > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + clk_put(pd->clk_top_conn); > > + clk_put(pd->clk_top_ipu_if); > > + clk_put(pd->clk_off); > > + clk_put(pd->clk_on_def); > > + } else { > > + clk_bulk_put(pd->num_clks, pd->clks); > > + } > > + return ERR_PTR(ret); > > +} > > + > > +static void apu_remove_one_domain(struct apu_domain *pd) > > +{ > > + int ret; > > + > > + if (pd->genpd.power_off) > > + pd->genpd.power_off(&pd->genpd); > > + > > + /* > > + * We're in the error cleanup already, so we only complain, > > + * but won't emit another error on top of the original one. > > + */ > > + ret = pm_genpd_remove(&pd->genpd); > > + if (ret < 0) > > + dev_dbg(pd->apusys->dev, > > + "Remove domain '%s' : %d - state may be > > inconsistent\n", > > + pd->genpd.name, ret); > > + > > + if (APU_PD_CAPS(pd, APU_PD_IPUIF_HW_CG)) { > > + clk_put(pd->clk_top_conn); > > + clk_put(pd->clk_top_ipu_if); > > + clk_put(pd->clk_off); > > + clk_put(pd->clk_on_def); > > + } else { > > + clk_bulk_put(pd->num_clks, pd->clks); > > + } > > +} > > + > > +static void apu_domain_cleanup(struct apusys *apusys) > > +{ > > + struct generic_pm_domain *genpd; > > + struct apu_domain *pd; > > + int i; > > + > > + for (i = apusys->pd_data.num_domains - 1; i >= 0; i--) { > > + genpd = apusys->pd_data.domains[i]; > > + if (genpd) { > > + pd = to_apu_domain(genpd); > > + apu_remove_one_domain(pd); > > + } > > + } > > +} > > + > > +static const struct of_device_id apu_pm_of_match[] = { > > + { > > + .compatible = "mediatek,mt8192-apu-pm", > > + .data = &mt8192_apu_pm_data, > > + }, > > + { } > > +}; > > + > > +static int apu_pm_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_node *np = dev->of_node; > > + const struct apu_pm_data *data; > > + struct device_node *node; > > + struct apusys *apusys; > > + int ret; > > + > > + data = of_device_get_match_data(&pdev->dev); > > + if (!data) { > > + dev_dbg(dev, "no power domain data\n"); > > + return -EINVAL; > > + } > > + > > + apusys = devm_kzalloc(dev, > > + struct_size(apusys, domains, data- > > >num_domains), > > + GFP_KERNEL); > > + if (!apusys) > > + return -ENOMEM; > > + > > + apusys->dev = dev; > > + apusys->data = data; > > + apusys->pd_data.domains = apusys->domains; > > + apusys->pd_data.num_domains = data->num_domains; > > + > > + apusys->vsram_supply = devm_regulator_get_optional(dev, > > "vsram"); > > + if (IS_ERR(apusys->vsram_supply)) { > > + ret = PTR_ERR(apusys->vsram_supply); > > + if (ret != -EPROBE_DEFER) { > > + dev_err(dev, "vsram_supply fail, ret=%d", ret); > > + apusys->vsram_supply = NULL; > > + } > > + return ret; > > + } > > + > > + /* rpc */ > > + apusys->rpc = syscon_node_to_regmap(np); > > + if (IS_ERR(apusys->rpc)) { > > + dev_err(dev, "Unable to get rpc\n"); > > + return PTR_ERR(apusys->rpc); > > + } > > + > > + /* scpsys */ > > + apusys->scpsys = syscon_regmap_lookup_by_phandle(np, > > "mediatek,scpsys"); > > + if (IS_ERR(apusys->scpsys)) { > > + dev_err(dev, "Unable to get scpsys\n"); > > + return PTR_ERR(apusys->scpsys); > > + } > > + > > + /* apusys conn */ > > + apusys->conn = syscon_regmap_lookup_by_phandle_optional(np, > > "mediatek,apu-conn"); > > + if (IS_ERR(apusys->conn)) > > + dev_info(dev, "No optional phandle apu-conn\n"); > > I take it as if some platforms may be expected to not have any > optional phandle > for that (and for the others): in such case, this would result in > being a useless > message. Please use dev_dbg() here. > > > + > > + /* apusys conn1 */ > > + apusys->conn1 = syscon_regmap_lookup_by_phandle_optional(np, > > "mediatek,apu-conn1"); > > + if (IS_ERR(apusys->conn1)) > > + dev_info(dev, "No optional phandle apu-conn1\n"); > > + > > + /* apusys vcore */ > > + apusys->vcore = syscon_regmap_lookup_by_phandle_optional(np, > > "mediatek,apu-vcore"); > > + if (IS_ERR(apusys->vcore)) > > + dev_info(dev, "No optional phandle apu-vcore\n"); > > + > > + for_each_available_child_of_node(np, node) { > > + struct generic_pm_domain *domain; > > + > > + domain = apu_add_one_domain(apusys, node); > > + if (IS_ERR(domain)) { > > + ret = PTR_ERR(domain); > > + of_node_put(node); > > + goto err_cleanup_domains; > > + } > > + } > > + > > + ret = of_genpd_add_provider_onecell(np, &apusys->pd_data); > > + if (ret) { > > + dev_dbg(dev, "failed to add provider: %d\n", ret); > > + goto err_cleanup_domains; > > + } > > + > > + return 0; > > + > > +err_cleanup_domains: > > + apu_domain_cleanup(apusys); > > + return ret; > > +} > > + > > +static struct platform_driver apu_pm_driver = { > > + .probe = apu_pm_probe, > > In this driver, you already have the means to remove the domains and > cleanup the > state... it would probably be trivial to add a .remove handler. > > What do you think about that? > Also, is there be any reason for which removing this driver during > runtime > would be unsafe? > > In my opinion, it would be beneficial to have a remove handler also > in the > development usecase, as you'd be able to just recompile this module, > remove and > reinsert it to test new changes, without the need of copying an > entire new > kernel image and rebooting the device. > > > + .driver = { > > + .name = "mtk-apu-pm", > > + .suppress_bind_attrs = true, > > + .of_match_table = apu_pm_of_match, > > + }, > > +}; > > +builtin_platform_driver(apu_pm_driver); > > > > MODULE_LICENSE() is missing! Please add it. > Also, as said in the Kconfig comment, this driver can probably be a > module, in > which case... module_platform_driver(apu_pm_driver); > > Thanks, > - Angelo > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel