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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3E5D7EE14D7 for ; Thu, 7 Sep 2023 03:44:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230503AbjIGDo2 convert rfc822-to-8bit (ORCPT ); Wed, 6 Sep 2023 23:44:28 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52922 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229782AbjIGDo2 (ORCPT ); Wed, 6 Sep 2023 23:44:28 -0400 Received: from hsmtpd-def.xspmail.jp (hsmtpd-def.xspmail.jp [202.238.198.241]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0AD20CA for ; Wed, 6 Sep 2023 20:44:23 -0700 (PDT) X-Country-Code: JP Received: from sakura.ysato.name (ik1-413-38519.vs.sakura.ne.jp [153.127.30.23]) by hsmtpd-out-2.asahinet.cluster.xspmail.jp (Halon) with ESMTPA id 33c31bc9-d828-4a57-82e6-dcd9a59fbe48; Thu, 07 Sep 2023 12:44:21 +0900 (JST) Received: from SIOS1075.ysato.ml (al128006.dynamic.ppp.asahi-net.or.jp [111.234.128.6]) by sakura.ysato.name (Postfix) with ESMTPSA id CFE051C0079; Thu, 7 Sep 2023 12:44:20 +0900 (JST) Date: Thu, 07 Sep 2023 12:44:20 +0900 Message-ID: <87y1hitzor.wl-ysato@users.sourceforge.jp> From: Yoshinori Sato To: Geert Uytterhoeven Cc: linux-sh@vger.kernel.org, glaubitz@physik.fu-berlin.de Subject: Re: [RESEND RFC PATCH 04/12] clk: SH7750 / 7751 clk driver. In-Reply-To: References: <541eb279023563f17245deabc32b9f65dbf92b9a.1693444193.git.ysato@users.sourceforge.jp> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/28.2 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Precedence: bulk List-ID: X-Mailing-List: linux-sh@vger.kernel.org On Fri, 01 Sep 2023 21:26:47 +0900, Geert Uytterhoeven wrote: > > Hi Sato-san, > > On Thu, Aug 31, 2023 at 11:32 AM Yoshinori Sato > wrote: > > Use COMMON_CLK framework clock driver. > > > > Signed-off-by: Yoshinori Sato > > Thanks for your patch! > > > --- /dev/null > > +++ b/drivers/clk/sh/Kconfig > > @@ -0,0 +1,7 @@ > > +config COMMON_CLK_SH7750 > > + bool "Clcok driver for SH7750/SH7751" > > Clock > > > + depends on CPU_SUBTYPE_SH7750 || CPU_SUBTYPE_SH7750S || \ > > + CPU_SUBTYPE_SH7750R || \ > > + CPU_SUBTYPE_SH7751 || CPU_SUBTYPE_SH7751R > > || COMPILE_TEST? > > Anyway, I would do > > select COMMON_CLK_SH7750 if CPU_SUBTYPE_SH7750 || ... > > at the top, and > > bool "Clock driver for SH7750/SH7751" if COMPILE_TEST > > cfr. drivers/clk/renesas/Kconfig. > > BTW, do you plan to put all SuperH clock drivers under SH? > For a simple CPG like in SH7750/7751 that could make sense. > For the more complex ones like SH7724, you probably want to plug > into the existing drivers/clk/renesas/renesas-cpg-mssr.c instead, > as it is very similar to the CPG on later Renesas ARM SoCs. OK. move to under drivers/clk/renesas. > But even SH7751 has Standby Control registers with Module Stop > bits... It is possible to stop modules, but it seems that there are not many modules that can be stopped. I haven't used it yet, so I would like to consider it as a next step. > > + help > > + This driver supports the Renesas SH7750 and SH7751 CPG. > > diff --git a/drivers/clk/sh/Makefile b/drivers/clk/sh/Makefile > > new file mode 100644 > > index 000000000000..7122c37655aa > > --- /dev/null > > +++ b/drivers/clk/sh/Makefile > > @@ -0,0 +1,2 @@ > > +obj-$(CONFIG_COMMON_CLK_SH7750) += clk-sh7750.o > > +obj-$(CONFIG_COMMON_CLK_SH7750) += clk-shdiv.o > > These can be one line. > > > diff --git a/drivers/clk/sh/clk-sh7750.c b/drivers/clk/sh/clk-sh7750.c > > new file mode 100644 > > index 000000000000..f41712a9cf44 > > --- /dev/null > > +++ b/drivers/clk/sh/clk-sh7750.c > > @@ -0,0 +1,193 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Renesas SH7750/51 clock driver > > + * > > + * Copyright 2023 Yoshinori Sato > > + */ > > + > > +#include > > Do you need the consumer API? > > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +struct clk *sh_div_clk_register(struct device *dev, const char *name, > > + const char *parent_name, > > + void __iomem *reg, u8 shift, u8 width, > > + const struct clk_div_table *table, > > + spinlock_t *lock); > > Please move this to a (private) header file. Else builds with W=1 > will complain about "warning: no previous prototype". > > > + > > +static DEFINE_SPINLOCK(clklock); > > + > > +static struct clk_div_table pdiv_table[] = { > > const > > > + { .val = 0, .div = 2, }, > > + { .val = 1, .div = 3, }, > > + { .val = 2, .div = 4, }, > > + { .val = 3, .div = 6, }, > > + { .val = 4, .div = 8, }, > > + { .val = 0, .div = 0, }, > > +}; > > + > > +static struct clk_div_table div_table[] = { > > const > > > + { .val = 0, .div = 1, }, > > + { .val = 1, .div = 2, }, > > + { .val = 2, .div = 3, }, > > + { .val = 3, .div = 4, }, > > + { .val = 4, .div = 6, }, > > + { .val = 5, .div = 8, }, > > + { .val = 0, .div = 0, }, > > +}; > > + > > +struct pll_clock { > > + struct clk_hw hw; > > + void __iomem *frqcr; > > + void __iomem *wdt; > > + int md; > > u32 > > > + bool div1; > > +}; > > + > > +#define to_pll_clock(_hw) container_of(_hw, struct pll_clock, hw) > > + > > +static unsigned long pll_recalc_rate(struct clk_hw *hw, > > + unsigned long parent_rate) > > +{ > > + struct pll_clock *pll_clock = to_pll_clock(hw); > > + unsigned long rate = parent_rate; > > + uint16_t frqcr; > > + static const int pll1[] = { 12, 12, 6, 12, 6, 12, 1}; > > + > > + frqcr = ioread16(pll_clock->frqcr); > > + if (frqcr & (1 << 10)) { > > Please add a define for "1 << 10" (or "BIT(10)"). > > #define FRQCR_PLL1EN BIT(10) > > > + rate *= pll1[pll_clock->md]; > > + if (pll_clock->md < 6 && pll_clock->div1) > > + rate /= 2; > > + } > > + return rate; > > +} > > + > > +static const struct clk_ops pll_ops = { > > + .recalc_rate = pll_recalc_rate, > > +}; > > + > > +static void __init sh7750_pll_clk_setup(struct device_node *node) > > +{ > > + unsigned int num_parents; > > + struct clk *clk; > > + const char *clk_name = node->name; > > + const char *parent_name; > > + struct pll_clock *pll_clock; > > + struct clk_init_data init; > > + > > + num_parents = of_clk_get_parent_count(node); > > + if (num_parents < 1) { > > + pr_err("%s: no parent found", clk_name); > > + return; > > + } > > + > > + pll_clock = kzalloc(sizeof(struct pll_clock), GFP_KERNEL); > > + if (!pll_clock) > > + return; > > + > > + pll_clock->frqcr = of_iomap(node, 0); > > + if (pll_clock->frqcr == NULL) { > > + pr_err("%s: failed to map frequenct control register", > > frequency > > > + clk_name); > > + goto free_clock; > > + } > > + > > + pll_clock->wdt = of_iomap(node, 1); > > + if (pll_clock->wdt == NULL) { > > + pr_err("%s: failed to map watchdog register", clk_name); > > + goto unmap_frqcr; > > + } > > + > > + of_property_read_u32_index(node, "sh7750,md", 0, &pll_clock->md); > > R-Mobile A1 uses "renesas,mode" for this, cfr. > Documentation/devicetree/bindings/clock/renesas,cpg-clocks.yaml > > > + if (pll_clock->md >= 7) { > > + pr_err("%s: failed to clock mode setting (%d)\n", > > invalid clock mode setting? > %u > > > + clk_name, pll_clock->md); > > + goto unmap_wdt; > > + } > > + pll_clock->div1 = !of_property_read_bool(node, "sh7750,rtype"); > > Shouldn't this be derived from the compatible value instead? > > > +static void __init sh7750_div_clk_setup(struct device_node *node) > > +{ > > + unsigned int num_parents; > > + struct clk *clk; > > + const char *clk_name = node->name; > > + const char *parent_name; > > + void __iomem *freqcr = NULL; > > + int i; > > unsigned int > > > + int num_clks; > > + int offset; > > + > > + num_parents = of_clk_get_parent_count(node); > > + if (num_parents < 1) { > > + pr_err("%s: no parent found", clk_name); > > + return; > > + } > > + > > + num_clks = of_property_count_strings(node, "clock-output-names"); > > Please no more clock-output-names. > > > +CLK_OF_DECLARE(sh7750_div_clk, "renesas,sh7750-div-clock", > > + sh7750_div_clk_setup); > > +CLK_OF_DECLARE(sh7750_pll_clk, "renesas,sh7750-pll-clock", > > + sh7750_pll_clk_setup); > > I think this should be a unified clock driver generating all clocks. > As there are no bindings yet, I will comment on the DTS files instead. > > > diff --git a/drivers/clk/sh/clk-shdiv.c b/drivers/clk/sh/clk-shdiv.c > > new file mode 100644 > > index 000000000000..2c016c413dd6 > > --- /dev/null > > +++ b/drivers/clk/sh/clk-shdiv.c > > > +static const struct clk_ops sh_clk_divider_ops = { > > + .recalc_rate = sh_clk_divider_recalc_rate, > > + .round_rate = sh_clk_divider_round_rate, > > Please implement .determine_rate() instead of the deprecated > .round_rate(). > > > + .set_rate = sh_clk_divider_set_rate, > > +}; > > > --- a/drivers/sh/Makefile > > +++ b/drivers/sh/Makefile > > @@ -2,7 +2,9 @@ > > # > > # Makefile for the SuperH specific drivers. > > # > > +ifneq ($(CONFIG_RENESAS_SH_INTC),y) > > obj-$(CONFIG_SH_INTC) += intc/ > > +endif > > This change does not belong in this patch. > > > ifneq ($(CONFIG_COMMON_CLK),y) > > obj-$(CONFIG_HAVE_CLK) += clk/ > > endif > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds -- Yosinori Sato