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 phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D3102C433FE for ; Wed, 2 Nov 2022 16:42:13 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 78FCA84EEA; Wed, 2 Nov 2022 17:42:11 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=kernel.org header.i=@kernel.org header.b="L1FpqBSI"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 68C1184EEB; Wed, 2 Nov 2022 17:42:09 +0100 (CET) Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 39FA284D36 for ; Wed, 2 Nov 2022 17:42:06 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=kernel.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=conor@kernel.org Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 474E661A91; Wed, 2 Nov 2022 16:42:04 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 2CF66C433C1; Wed, 2 Nov 2022 16:42:02 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1667407323; bh=j2ebhjS3LRgoXe09zr66VFoVAbwobbeDx1Wvufew1l8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=L1FpqBSIvktkGnzyivQybP8uQl7cJv1puDcoYkk17i5sK5PNaqq1qgdSTlxugUoyv ezqHss8fvu8QZjS0rOqhGKkGAJVLAjhfZGbP4hw6o6Gpl9dRjx3NcscSW1NxKL6bl0 fYntmOoFriTJMfLbahxMyIg5O7DXpJ+Uo7fLnqkoPyLl705cm4ojHgQ3ijafK9qaht hEd22n9GflsuTwOL/nbnIDV+nTDC0oLqla9fdL1YP+bt4ShWpLGlnxF33KCLay1DA9 0qBl5/EF6oHZy7Bkag++N2+gcJASXL/zkR4nBg8p1dPQYx6MWSrfnN7MAp2Vy6A7ul 6mnqlgfhe3R4Q== Date: Wed, 2 Nov 2022 16:41:59 +0000 From: Conor Dooley To: Padmarao.Begari@microchip.com Cc: rick@andestech.com, Conor.Dooley@microchip.com, ycliang@andestech.com, lukma@denx.de, seanga2@gmail.com, u-boot@lists.denx.de Subject: Re: [PATCH v1 2/6] clk: microchip: mpfs: convert parent rate acquistion to get_get_rate() Message-ID: References: <20221025075848.110754-1-conor.dooley@microchip.com> <20221025075848.110754-3-conor.dooley@microchip.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Wed, Nov 02, 2022 at 01:20:33PM +0000, Padmarao.Begari@microchip.com wrote: > Hi Conor, > > > On Tue, 2022-10-25 at 08:58 +0100, Conor Dooley wrote: > > Currently the clock driver for PolarFire SoC takes a very naive > > approach > > to the relationship between clocks. It reads the dt to get an input > > clock, assumes that that is fixed frequency, reads the "clock- > s/that that/that Nope, not a typo ;) > > > frequency" > > property & uses that to set up both the "cfg" and "periph" clocks. > > > > Simplifying for the sake of incremental fixes, the "correct" > > parentage for > > the clocks currently supported in U-Boot is that the "cfg" clocks > > should > > be children of the fixed frequency clock in the dt. The AHB clock is > > one > > of these "cfg" clocks and is the parent of the "periph" clocks. > > > > Instead of passing the clock rate of the fixed-frequency clock to the > > "cfg" and "periph" registration functions and the name of the > > parents, > > pass their actual parents & use clk_get_rate() to determine their > > parents > > rates. > > > > The "periph" clocks are purely gate clocks and should not be reading > > the > > AHB clocks registers to determine their rates, as they can simply > > report > > the output of clk_get_rate() on their parent. > > > > Signed-off-by: Conor Dooley > > --- > > drivers/clk/microchip/Makefile | 2 +- > > drivers/clk/microchip/mpfs_clk.c | 18 ++++++++---------- > > drivers/clk/microchip/mpfs_clk.h | 12 ++++-------- > > drivers/clk/microchip/mpfs_clk_cfg.c | 7 +++---- > > drivers/clk/microchip/mpfs_clk_periph.c | 16 ++++------------ > > 5 files changed, 20 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/clk/microchip/Makefile > > b/drivers/clk/microchip/Makefile > > index 904b345d75..329b2c0c93 100644 > > --- a/drivers/clk/microchip/Makefile > > +++ b/drivers/clk/microchip/Makefile > > @@ -1 +1 @@ > > -obj-y += mpfs_clk.o mpfs_clk_cfg.o mpfs_clk_periph.o > > +obj-y += mpfs_clk.o mpfs_clk_cfg.o mpfs_clk_periph.o > > mpfs_clk_msspll.o > > diff --git a/drivers/clk/microchip/mpfs_clk.c > > b/drivers/clk/microchip/mpfs_clk.c > > index 67828c9bf4..7ba1218b56 100644 > > --- a/drivers/clk/microchip/mpfs_clk.c > > +++ b/drivers/clk/microchip/mpfs_clk.c > > @@ -11,34 +11,32 @@ > > #include > > #include > > #include > > +#include > > #include > > > > #include "mpfs_clk.h" > > > > static int mpfs_clk_probe(struct udevice *dev) > > { > > - int ret; > > + struct clk *parent_clk = dev_get_priv(dev); > > + struct clk clk_ahb = { .id = CLK_AHB }; > The peripheral clock updated code added in this patch but removed it in > the patch 4, you can update only related code in this patch instead of > removing it later. Right. I did that intentionally so that each patch did the minimum required to fix individual issues. This patch does some reorganisation so that the follow up patches were only as minimal fixes as possible. I'll reorganise things if you think that's the better way to go about it though. > > Other than that: > Reviewed-by: Padmarao Begari > > > void __iomem *base; > > - u32 clk_rate; > > - const char *parent_clk_name; > > - struct clk *clk = dev_get_priv(dev); > > + int ret; > > > > base = dev_read_addr_ptr(dev); > > if (!base) > > return -EINVAL; > > > > - ret = clk_get_by_index(dev, 0, clk); > > + ret = clk_get_by_index(dev, 0, parent_clk); > > if (ret) > > return ret; > > > > - dev_read_u32(clk->dev, "clock-frequency", &clk_rate); > > - parent_clk_name = clk->dev->name; > > - > > - ret = mpfs_clk_register_cfgs(base, clk_rate, parent_clk_name); > > + ret = mpfs_clk_register_cfgs(base, parent_clk); > > if (ret) > > return ret; > > > > - ret = mpfs_clk_register_periphs(base, clk_rate, "clk_ahb"); > > + clk_request(dev, &clk_ahb); > > + ret = mpfs_clk_register_periphs(base, &clk_ahb); > > > > return ret; > > } > > diff --git a/drivers/clk/microchip/mpfs_clk.h > > b/drivers/clk/microchip/mpfs_clk.h > > index 442562a5e7..35cfeac92e 100644 > > --- a/drivers/clk/microchip/mpfs_clk.h > > +++ b/drivers/clk/microchip/mpfs_clk.h > > @@ -11,22 +11,18 @@ > > * mpfs_clk_register_cfgs() - register configuration clocks > > * > > * @base: base address of the mpfs system register. > > - * @clk_rate: the mpfs pll clock rate. > > - * @parent_name: a pointer to parent clock name. > > + * @parent: a pointer to parent clock. > > * Return: zero on success, or a negative error code. > > */ > > -int mpfs_clk_register_cfgs(void __iomem *base, u32 clk_rate, > > - const char *parent_name); > > +int mpfs_clk_register_cfgs(void __iomem *base, struct clk *parent); > > /** > > * mpfs_clk_register_periphs() - register peripheral clocks > > * > > * @base: base address of the mpfs system register. > > - * @clk_rate: the mpfs pll clock rate. > > - * @parent_name: a pointer to parent clock name. > > + * @parent: a pointer to parent clock. > > * Return: zero on success, or a negative error code. > > */ > > -int mpfs_clk_register_periphs(void __iomem *base, u32 clk_rate, > > - const char *parent_name); > > +int mpfs_clk_register_periphs(void __iomem *base, struct clk > > *parent); > > /** > > * divider_get_val() - get the clock divider value > > * > > diff --git a/drivers/clk/microchip/mpfs_clk_cfg.c > > b/drivers/clk/microchip/mpfs_clk_cfg.c > > index fefddd1413..5739fd66e8 100644 > > --- a/drivers/clk/microchip/mpfs_clk_cfg.c > > +++ b/drivers/clk/microchip/mpfs_clk_cfg.c > > @@ -117,8 +117,7 @@ static struct mpfs_cfg_hw_clock mpfs_cfg_clks[] = > > { > > CLK_CFG(CLK_AHB, "clk_ahb", 4, 2, mpfs_div_ahb_table, 0), > > }; > > > > -int mpfs_clk_register_cfgs(void __iomem *base, u32 clk_rate, > > - const char *parent_name) > > +int mpfs_clk_register_cfgs(void __iomem *base, struct clk *parent) > > { > > int ret; > > int i, id, num_clks; > > @@ -129,9 +128,9 @@ int mpfs_clk_register_cfgs(void __iomem *base, > > u32 clk_rate, > > for (i = 0; i < num_clks; i++) { > > hw = &mpfs_cfg_clks[i].hw; > > mpfs_cfg_clks[i].sys_base = base; > > - mpfs_cfg_clks[i].prate = clk_rate; > > + mpfs_cfg_clks[i].prate = clk_get_rate(parent); > > name = mpfs_cfg_clks[i].cfg.name; > > - ret = clk_register(hw, MPFS_CFG_CLOCK, name, > > parent_name); > > + ret = clk_register(hw, MPFS_CFG_CLOCK, name, parent- > > >dev->name); > > if (ret) > > ERR_PTR(ret); > > id = mpfs_cfg_clks[i].cfg.id; > > diff --git a/drivers/clk/microchip/mpfs_clk_periph.c > > b/drivers/clk/microchip/mpfs_clk_periph.c > > index 61d90eb4a8..1488ef503e 100644 > > --- a/drivers/clk/microchip/mpfs_clk_periph.c > > +++ b/drivers/clk/microchip/mpfs_clk_periph.c > > @@ -99,16 +99,9 @@ static int mpfs_periph_clk_disable(struct clk *hw) > > static ulong mpfs_periph_clk_recalc_rate(struct clk *hw) > > { > > struct mpfs_periph_hw_clock *periph_hw = > > to_mpfs_periph_clk(hw); > > - void __iomem *base_addr = periph_hw->sys_base; > > - unsigned long rate; > > - u32 val; > > > > - val = readl(base_addr + REG_CLOCK_CONFIG_CR) >> CFG_AHB_SHIFT; > > - val &= clk_div_mask(CFG_WIDTH); > > - rate = periph_hw->prate / (1u << val); > > - hw->rate = rate; > > + return periph_hw->prate; > > > > - return rate; > > } > > > > #define CLK_PERIPH(_id, _name, _shift, _flags) { \ > > @@ -150,8 +143,7 @@ static struct mpfs_periph_hw_clock > > mpfs_periph_clks[] = { > > CLK_PERIPH(CLK_CFM, "clk_periph_cfm", 29, 0), > > }; > > > > -int mpfs_clk_register_periphs(void __iomem *base, u32 clk_rate, > > - const char *parent_name) > > +int mpfs_clk_register_periphs(void __iomem *base, struct clk > > *parent) > > { > > int ret; > > int i, id, num_clks; > > @@ -162,9 +154,9 @@ int mpfs_clk_register_periphs(void __iomem *base, > > u32 clk_rate, > > for (i = 0; i < num_clks; i++) { > > hw = &mpfs_periph_clks[i].hw; > > mpfs_periph_clks[i].sys_base = base; > > - mpfs_periph_clks[i].prate = clk_rate; > > + mpfs_periph_clks[i].prate = clk_get_rate(parent); > > name = mpfs_periph_clks[i].periph.name; > > - ret = clk_register(hw, MPFS_PERIPH_CLOCK, name, > > parent_name); > > + ret = clk_register(hw, MPFS_PERIPH_CLOCK, name, parent- > > >dev->name); > > if (ret) > > ERR_PTR(ret); > > id = mpfs_periph_clks[i].periph.id;