From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0A42A606AAC9 for ; Mon, 19 Oct 2026 15:56:38 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key, unprotected) header.d=lists.infradead.org header.i=@lists.infradead.org header.a=rsa-sha256 header.s=bombadil.20210309 header.b=RNb3mAex; dkim=pass (1024-bit key, unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.a=rsa-sha256 header.s=mail header.b=QeSUwDeh DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=522dSgSX8QDg4IeydtuZ1AFaoV8ZpxNqAXq0w4tKQaU=; b=RNb3mAexKXojJmA65WQlAeVr08 M0/LVj6y6UCuaH2n9PpSqjwn3DIou17pD+Bq3YnC2osZYb4Y3CD4Ry2RWo0+++grcl0J+GFGFW4q3 Utv++gleveIe6SZGUYz61RUVz9MlsHGKr+7YDh6DOqFOtBNwz57vIAqEg3HfYbucJDV4GKdEbZk6C Cckbt8QkH+ND0uFC7xUuy9TMWWT1SHoNDmrXhc+o9KB8Bvj1pctmuDv3K5rf7RCLy3Bmf91vhWuqb zP76/px4jrhKJL7X6wuxRvIa4uNkrhWx4WjxCaKWqKFMY812hL88aoahdYtFzUSVjcAMDbByRRF4W hYESY02g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.99.1 #2 (Red Hat Linux)) id 1xIpir-00000007u0W-3bJN; Mon, 19 Oct 2026 15:56:25 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.99.1 #2 (Red Hat Linux)) id 1xIpip-00000007tzt-36TW for linux-arm-kernel@lists.infradead.org; Mon, 19 Oct 2026 15:56:25 +0000 Received: from killaraus.ideasonboard.com (2001-14ba-70f3-e800--a06.rev.dnainternet.fi [IPv6:2001:14ba:70f3:e800::a06]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id E1E82D0; Mon, 19 Oct 2026 17:54:14 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1792425255; bh=9uCPKIsEr/8+KuBqoPLaVUoPiPXEjoFfr9QWURIr6aM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=QeSUwDehkhxrrH591BEFl/mwIa5Lg0XyL0OAF1o4NQmUFpzj6s6p6mfyZvFH7UhRe FTu9nWzUZmHVuOOIZq1K2/lSn3tqWcRrApQ4GOsGtVr5wMJw3LYO/MZC73GfWsnEwr rhjruCETvAoqwmGwKXbKHUFlmu2pKpNBuryrILH8= Date: Mon, 19 Oct 2026 18:56:19 +0300 From: Laurent Pinchart To: Geert Uytterhoeven Cc: Rob Herring , Krzysztof Kozlowski , Conor Dooley , Stephen Boyd , Brian Masney , Jerome Brunet , Ulf Hansson , Philipp Zabel , "Rafael J . Wysocki" , Marek Vasut , Bartosz Golaszewski , Konrad Dybcio , Kevin Hilman , Vinod Koul , Wolfram Sang , Kuninori Morimoto , =?utf-8?Q?Cl=C3=A9ment?= Le Goffic , Sudeep Holla , Cristian Marussi , arm-scmi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-clk@vger.kernel.org, linux-pm@vger.kernel.org, linux-renesas-soc@vger.kernel.org Subject: Re: [PATCH v4 2/8] clk: renesas: r8a78000: Add SCMI remapping support Message-ID: <20261019155619.GB3705@killaraus.ideasonboard.com> References: X-Archived-By: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.9.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20261019_085623_926775_9B155889 X-CRM114-Status: GOOD ( 48.15 ) X-Spam-Score: -2.1 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" X-Spam-Status: No, score=-16.2 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,RCVD_IN_DNSWL_BLOCKED,RCVD_IN_ZEN_BLOCKED_OPENDNS, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=4.0.1 X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-26) on aws-us-west-2-korg-listserv-alma10-1 Hi Geert, Thank you for the patch. On Fri, Oct 16, 2026 at 05:55:41PM +0200, Geert Uytterhoeven wrote: > Extend the Clock Pulse Generator driver for R-Car X5H with SCMI support, > to remap DT clock IDs to abstractions (clocks) offered by the SCMI > firmware when running on a system with SCMI, and SCMI support is > enabled. Take into account some peculiarities of the current SCMI > firmware implementations, and fall-back to fixed-rate clocks as > replacements for SCMI clocks that are not available. > Support for bus clocks used by serial, I2C, and SPI devices is included. > > This lets us avoid putting SCMI IDs in DT, and allows us to use the same > hardware description in SoC DTS for systems with and without SCMI > firmware. > > For now, it is assumed SCMI clock IDs are stable, and match SCP FW > SDKv4.31.0 and later. > > Suggested-by: Marek Vasut > Signed-off-by: Geert Uytterhoeven > --- > v4: > - Drop unneeded IS_ERR() check after __clk_get_hw(), > - Mark scmi_clk_np __free(device_node), > - Assume all SCMI clock IDs are stable, and drop support for handling > different ID spaces, > - Include initial mapping table for SCP FW SDKv4.31.0 and later, > - Try SCMI clocks first, and fall back to the default fixed-frequency > clocks when not available, as recent firmware versions no longer > restrict access to some bus clocks, > - Drop renesas,scmi-firmware support, just find an arm,scmi node > instead, > - Fall back to the non-SCMI defaults when SCMI clock support is > disabled, > > v3: > - Rebase, > - Consistently use /* sentinel */ for zero sentinels, > - s/firmware/renesas,scmi-firmware/, > - Reword, > > v2: > - Spin off from "clk: renesas: Add R-Car X5H CPG SCMI remapping > driver", > - Use reverse xmas ordering in r8a78000_cpg_probe(), > - Propagate error codes from scmi_get_base_info() now it can return > -EPROBE_DEFER, > - Improve error message on scmi_get_base_info() failure, > - Check if SCMI firmware node is available, > - Replace the scmi_clk_np member of struct r8a78000_cpg_priv by a > local variable in r8a78000_cpg_probe(), and pass it to > fill_clk_map(), > - Join split short lines, > - Fall back to the default support when SCMI support or firmware is > not available, or when the detected SCMI firmware version is not > supported by the driver, > - Drop mapping tables. > --- > drivers/clk/renesas/r8a78000-cpg.c | 149 ++++++++++++++++++++++++++--- > 1 file changed, 138 insertions(+), 11 deletions(-) > > diff --git a/drivers/clk/renesas/r8a78000-cpg.c b/drivers/clk/renesas/r8a78000-cpg.c > index 5ffd7b89cb48e41c..758cfe78ae58e0fb 100644 > --- a/drivers/clk/renesas/r8a78000-cpg.c > +++ b/drivers/clk/renesas/r8a78000-cpg.c > @@ -5,6 +5,7 @@ > * Copyright (C) 2026 Glider bv > */ > > +#include > #include > #include > #include > @@ -12,21 +13,27 @@ > #include > #include > #include > +#include > #include > > #include > > struct clk_map_in { > int dt_id; /* DT binding clock ID or -1 sentinel */ > - u32 fw_id; /* FIXED_CLK() ID */ > + u32 fw_id; /* SCMI firmware clock ID or FIXED_CLK() ID */ > }; > > struct clk_map { > int dt_id; /* DT binding clock ID or -1 sentinel */ > - u32 fw_id; /* FIXED_CLK() ID */ > + u32 fw_id; /* SCMI firmware clock ID or FIXED_CLK() ID */ > struct clk_hw *hw; > }; > > +struct cpg_data { > + const struct clk_map_in *default_map; > + const struct clk_map_in *fw_map; > +}; > + > enum fixed_clk { > FIXED_CLK_66M, > FIXED_CLK_133M, > @@ -39,13 +46,14 @@ static const unsigned long fixed_clk_rates[] = { > [FIXED_CLK_266M] = 266660000, > }; > > -#define FIXED_CLK(rate) FIXED_CLK_ ## rate > +#define FIXED_CLK_OFFSET 0x80000000 > +#define FIXED_CLK(rate) FIXED_CLK_OFFSET + FIXED_CLK_ ## rate #define FIXED_CLK(rate) (FIXED_CLK_OFFSET + FIXED_CLK_ ## rate) > > /** > * struct r8a78000_cpg_priv - Clock Pulse Generator Private Data > * > * @dev: CPG device > - * @map: Mapping from DT clock IDs to fixed-rate clocks > + * @map: Mapping from DT clock IDs to SCMI or fixed-rate clocks > */ > struct r8a78000_cpg_priv { > struct device *dev; > @@ -85,8 +93,19 @@ static struct clk_hw *r8a78000_clk_get(struct of_phandle_args *spec, > return ERR_PTR(-ENOENT); > } > > - dev_dbg(dev, "Mapping DT clock %u to fixed clock %u\n", id, map->fw_id); > + if (map->fw_id < FIXED_CLK_OFFSET) > + dev_dbg(dev, "Mapping DT clock %u to SCMI clock %u\n", id, > + map->fw_id); > + else > + dev_dbg(dev, "Mapping DT clock %u to fixed clock %u\n", id, > + map->fw_id - FIXED_CLK_OFFSET); > + > hw = map->hw; > + if (!hw) { > + /* CLOCK_ATTRIBUTES is not supported */ > + dev_err(dev, "Clock %u is not available\n", id); > + return ERR_PTR(-ENOENT); > + } > > dev_dbg(dev, "clock %u is %s at %lu Hz\n", id, clk_hw_get_name(hw), > clk_hw_get_rate(hw)); > @@ -94,6 +113,21 @@ static struct clk_hw *r8a78000_clk_get(struct of_phandle_args *spec, > return hw; > } > > +static struct device_node *scmi_find_proto(struct device_node *scmi, u32 proto) This is the kind of function name that looks prone to a namespace clash. I'd name it r8a78000_clk_scmi_find_proto(). > +{ > + for_each_available_child_of_node_scoped(scmi, child) { > + u32 reg; > + > + if (of_property_read_u32(child, "reg", ®)) > + continue; > + > + if (reg == proto) > + return_ptr(child); > + } > + > + return NULL; > +} > + > static struct clk_hw **register_fixed_clks(struct device *dev) > { > unsigned int n = ARRAY_SIZE(fixed_clk_rates); > @@ -122,11 +156,16 @@ static struct clk_hw **register_fixed_clks(struct device *dev) > } > > static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv, > - const struct clk_map_in *map_in) > + const struct clk_map_in *map_in, > + const struct clk_map_in *fallback, > + struct device_node *scmi_clk_np) > { > + struct of_phandle_args scmi_spec; > struct device *dev = priv->dev; > struct clk_hw **fixed_hws; > struct clk_map *map; > + struct clk_hw *hw; > + struct clk *clk; > unsigned int i; > > fixed_hws = register_fixed_clks(dev); > @@ -145,7 +184,46 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv, > break; > > map[i].fw_id = map_in[i].fw_id; > - map[i].hw = fixed_hws[map[i].fw_id]; > + if (map[i].fw_id >= FIXED_CLK_OFFSET) { > + enum fixed_clk idx = map[i].fw_id - FIXED_CLK_OFFSET; > + > + map[i].hw = fixed_hws[idx]; > + continue; > + } > + > + scmi_spec.np = scmi_clk_np; This seems to be safe because a NULL scmi_clk_np implies usage of the default map, which only contains fixed clock offsets, but that makes the code a bit too fragile for my taste. > + scmi_spec.args_count = 1; > + scmi_spec.args[0] = map[i].fw_id; > + > + clk = of_clk_get_from_provider(&scmi_spec); > + if (IS_ERR(clk)) > + return dev_err_cast_probe(dev, clk, > + "Failed to get SCMI clock %u\n", map[i].fw_id); > + > + hw = __clk_get_hw(clk); > + if (!hw && fallback != map_in) { And if the above is correct, fallback should always be different from map_in when fw_id corresponds to an SCMI clock. What issue does this catch ? Should the fw map contain a fixed clock entry for clocks not available in SCMI ? > + for (unsigned int j = 0; fallback[j].dt_id >= 0; j++) { > + if (fallback[j].dt_id == map_in[i].dt_id) { > + enum fixed_clk idx = fallback[j].fw_id - FIXED_CLK_OFFSET; > + > + map[i].hw = fixed_hws[idx]; > + break; > + } > + } > + if (map[i].hw) > + continue; > + } > + > + if (!hw) { > + /* CLOCK_ATTRIBUTES is not supported */ > + dev_warn(dev, "SCMI clock %u is NULL\n", map[i].fw_id); > + continue; > + } > + > + dev_dbg(priv->dev, "SCMI clock %u is %s at %lu Hz\n", > + map[i].fw_id, clk_hw_get_name(hw), clk_hw_get_rate(hw)); > + > + map[i].hw = hw; > } > > devm_kfree(dev, fixed_hws); > @@ -154,20 +232,55 @@ static const struct clk_map *fill_clk_map(struct r8a78000_cpg_priv *priv, > > static int r8a78000_cpg_probe(struct platform_device *pdev) > { > + struct device_node *scmi_clk_np __free(device_node) = NULL; > + struct device_node *scmi __free(device_node) = NULL; > struct device *dev = &pdev->dev; > + const struct cpg_data *cpg_data; > struct r8a78000_cpg_priv *priv; > const struct clk_map_in *map; > > - map = of_device_get_match_data(dev); > - if (!map) > + cpg_data = of_device_get_match_data(dev); > + if (!cpg_data) > return -ENODEV; > > + map = cpg_data->default_map; > + > priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > if (!priv) > return -ENOMEM; > > priv->dev = dev; > - priv->map = fill_clk_map(priv, map); > + > + if (!IS_ENABLED(CONFIG_COMMON_CLK_SCMI)) { > + dev_dbg(dev, "SCMI clock support is disabled\n"); > + goto fallback; > + } > + > + scmi = of_find_compatible_node(NULL, NULL, "arm,scmi"); > + if (!scmi) { > + dev_dbg(dev, "Cannot find SCMI firmware node\n"); > + goto fallback; > + } > + > + if (!of_device_is_available(scmi)) { > + dev_dbg(dev, "SCMI firmware node is not available\n"); > + goto fallback; > + } > + > + scmi_clk_np = scmi_find_proto(scmi, SCMI_PROTOCOL_CLOCK); > + if (!scmi_clk_np) { > + dev_dbg(dev, "Cannot find SCMI clock management protocol\n"); > + goto fallback; > + } > + > + map = cpg_data->fw_map; > + > +fallback: > + /* > + * We cannot do lazy look-up in r8a78000_clk_get(), as that function is > + * called with of_clk_mutex already held. > + */ > + priv->map = fill_clk_map(priv, map, cpg_data->default_map, scmi_clk_np); > if (IS_ERR(priv->map)) > return PTR_ERR(priv->map); > > @@ -181,10 +294,24 @@ static const struct clk_map_in r8a78000_cpg_default[] = { > { -1 } > }; > > +static const struct clk_map_in r8a78000_cpg_fw[] = { > + { R8A78000_CPG_SGASYNCD4_PERW_BUS, 1661 }, > + { R8A78000_CPG_SGASYNCD8_PERW_BUS, 1662 }, > + { R8A78000_CPG_SGASYNCD16_PERW_BUS, 1663 }, > + { R8A78000_CPG_MSO_SCP_MAIN, 1459 }, > + { R8A78000_CPG_MSOCK_PERW_BUS, 1667 }, > + { -1 } > +}; > + > +static const struct cpg_data r8a78000_cpg_data = { > + .default_map = r8a78000_cpg_default, > + .fw_map = r8a78000_cpg_fw, > +}; > + > static const struct of_device_id r8a78000_cpg_match[] = { > { > .compatible = "renesas,r8a78000-cpg", > - .data = &r8a78000_cpg_default, > + .data = &r8a78000_cpg_data, > }, > { /* sentinel */ } > }; -- Regards, Laurent Pinchart