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

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 conor.dooley@microchip.com

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 <dm/device.h> #include <dm/devres.h> #include <dm/uclass.h> +#include <dt-bindings/clock/microchip-mpfs-clock.h> #include <linux/err.h>

#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.

Other than that: Reviewed-by: Padmarao Begari padmarao.begari@microchip.com

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;

On Tue, 2022-10-25 at 08:58 +0100, Conor Dooley wrote: The original devicetrees for PolarFire SoC messed up & defined the msspll's output as a fixed-frequency, 600 MHz clock & used that as the input for the clock controller node. The msspll is not a fixed frequency clock and later devicetrees handled this properly. Check the devicetree & if it is one of the fixed ones, register the msspll. Otherwise, skip registering it & pass the reference clock directly to the cfg clock registration function so that existing devicetrees are not broken by this change.

As the MSS PLL is not a "cfg" or a "periph" clock, add a new driver for it, based on the one in Linux.

Fixes: 2f27c9219e ("clk: Add Microchip PolarFire SoC clock driver") Signed-off-by: Conor Dooley conor.dooley@microchip.com

---

drivers/clk/microchip/mpfs_clk.c | 23 ++++- drivers/clk/microchip/mpfs_clk.h | 8 ++ drivers/clk/microchip/mpfs_clk_msspll.c | 119 ++++++++++++++++++++++++ 3 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 drivers/clk/microchip/mpfs_clk_msspll.c

diff --git a/drivers/clk/microchip/mpfs_clk.c b/drivers/clk/microchip/mpfs_clk.c index 7ba1218b56..f16f716f00 100644 --- a/drivers/clk/microchip/mpfs_clk.c +++ b/drivers/clk/microchip/mpfs_clk.c @@ -20,10 +20,12 @@ static int mpfs_clk_probe(struct udevice *dev) { struct clk *parent_clk = dev_get_priv(dev); struct clk clk_ahb = { .id = CLK_AHB };

• struct clk clk_msspll = { .id = CLK_MSSPLL }; void __iomem *base;
• void __iomem *msspll_base; int ret;

• base = dev_read_addr_ptr(dev);

• base = dev_read_addr_index_ptr(dev, 0); if (!base) return -EINVAL;

@@ -31,6 +33,25 @@ static int mpfs_clk_probe(struct udevice *dev) if (ret) return ret;

• /*

• * The original devicetrees for mpfs messed up & defined the
  

msspll's

• * output as a fixed-frequency, 600 MHz clock & used that as
  

the input

• * for the clock controller node. The msspll is however not a
  

fixed

• * frequency clock and later devicetrees handled this properly.
  

Check

• * the devicetree & if it is one of the fixed ones, register
  

the msspll.

• * Otherwise, skip registering it & pass the reference clock
  

directly

• * to the cfg clock registration function.
  

• */
  

• msspll_base = dev_read_addr_index_ptr(dev, 1);
• if (msspll_base) {

• ret = mpfs_clk_register_msspll(msspll_base,
  

parent_clk);

• if (ret)
  

• 	return ret;
  

• clk_request(dev, &clk_msspll);
  

• parent_clk = &clk_msspll;
  

• }
• ret = mpfs_clk_register_cfgs(base, parent_clk); if (ret) return ret;

diff --git a/drivers/clk/microchip/mpfs_clk.h b/drivers/clk/microchip/mpfs_clk.h index 35cfeac92e..cb7d303e67 100644 --- a/drivers/clk/microchip/mpfs_clk.h +++ b/drivers/clk/microchip/mpfs_clk.h @@ -15,6 +15,14 @@

• Return: zero on success, or a negative error code.

*/ int mpfs_clk_register_cfgs(void __iomem *base, struct clk *parent); +/**

• • mpfs_clk_register_msspll() - register the mss pll
• • @base: base address of the mpfs system register.
• • @parent: a pointer to parent clock.
• • Return: zero on success, or a negative error code.
• */

+int mpfs_clk_register_msspll(void __iomem *base, struct clk *parent); /**

• mpfs_clk_register_periphs() - register peripheral clocks

diff --git a/drivers/clk/microchip/mpfs_clk_msspll.c b/drivers/clk/microchip/mpfs_clk_msspll.c new file mode 100644 index 0000000000..f37c0d8604 --- /dev/null +++ b/drivers/clk/microchip/mpfs_clk_msspll.c @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright (C) 2022 Microchip Technology Inc.
• */

+#include <common.h> +#include <clk.h> +#include <clk-uclass.h> +#include <asm/io.h> +#include <dm/device.h> +#include <dm/devres.h> +#include <dm/uclass.h> +#include <dt-bindings/clock/microchip-mpfs-clock.h> +#include <linux/err.h>

+#include "mpfs_clk.h"

+#define MPFS_MSSPLL_CLOCK "mpfs_msspll_clock"

+/* address offset of control registers */ +#define REG_MSSPLL_REF_CR 0x08u +#define REG_MSSPLL_POSTDIV_CR 0x10u +#define REG_MSSPLL_SSCG_2_CR 0x2Cu

+#define MSSPLL_FBDIV_SHIFT 0x00u +#define MSSPLL_FBDIV_WIDTH 0x0Cu +#define MSSPLL_REFDIV_SHIFT 0x08u +#define MSSPLL_REFDIV_WIDTH 0x06u +#define MSSPLL_POSTDIV_SHIFT 0x08u +#define MSSPLL_POSTDIV_WIDTH 0x07u +#define MSSPLL_FIXED_DIV 4u

+/**

• • struct mpfs_msspll_hw_clock
• • @id: index of the msspll clock
• • @name: the msspll clocks name
• • @reg_offset: offset to the core complex's output of the msspll
• • @shift: shift to the divider bit field of a msspll clock output
• • @width: width of the divider bit field of the msspll clock output
• • @flags: common clock framework flags
• • @prate: the reference clock rate
• • @hw: clock instance
• */

+struct mpfs_msspll_hw_clock {

• void __iomem *base;
• unsigned int id;
• const char *name;
• u32 reg_offset;
• u32 shift;
• u32 width;
• u32 flags;
• u32 prate;
• struct clk hw;

+};

+#define to_mpfs_msspll_clk(_hw) container_of(_hw, struct mpfs_msspll_hw_clock, hw)

+static unsigned long mpfs_clk_msspll_recalc_rate(struct clk *hw) +{

• struct mpfs_msspll_hw_clock *msspll_hw =

to_mpfs_msspll_clk(hw);

• void __iomem *mult_addr = msspll_hw->base + msspll_hw-

...

reg_offset;

• void __iomem *ref_div_addr = msspll_hw->base +

REG_MSSPLL_REF_CR;

• void __iomem *postdiv_addr = msspll_hw->base +

REG_MSSPLL_POSTDIV_CR;

• u32 mult, ref_div, postdiv;
• unsigned long temp;
• mult = readl(mult_addr) >> MSSPLL_FBDIV_SHIFT;
• mult &= clk_div_mask(MSSPLL_FBDIV_WIDTH);
• ref_div = readl(ref_div_addr) >> MSSPLL_REFDIV_SHIFT;
• ref_div &= clk_div_mask(MSSPLL_REFDIV_WIDTH);
• postdiv = readl(postdiv_addr) >> MSSPLL_POSTDIV_SHIFT;
• postdiv &= clk_div_mask(MSSPLL_POSTDIV_WIDTH);
• temp = msspll_hw->prate / (ref_div * MSSPLL_FIXED_DIV *

postdiv);

• return temp * mult;

+}

+#define CLK_PLL(_id, _name, _shift, _width, _reg_offset, _flags) { \

• .id = _id,

\

• .name = _name,

\

• .shift = _shift, \
• .width = _width, \
• .reg_offset = _reg_offset,

\

• .flags = _flags, \

+}

+static struct mpfs_msspll_hw_clock mpfs_msspll_clks[] = {

• CLK_PLL(CLK_MSSPLL, "clk_msspll", MSSPLL_FBDIV_SHIFT,

• MSSPLL_FBDIV_WIDTH, REG_MSSPLL_SSCG_2_CR, 0),
  

+};

+int mpfs_clk_register_msspll(void __iomem *base, struct clk *parent) +{

• int id, ret;
• const char *name;
• struct clk *hw;
• hw = &mpfs_msspll_clks[0].hw;
• mpfs_msspll_clks[0].base = base;
• mpfs_msspll_clks[0].prate = clk_get_rate(parent);
• name = mpfs_msspll_clks[0].name;
• ret = clk_register(hw, MPFS_MSSPLL_CLOCK, name, parent->dev-

...

name);

• if (ret)

• ERR_PTR(ret);
  

• id = mpfs_msspll_clks[0].id;
• clk_dm(id, hw);
• return 0;

+}

+const struct clk_ops mpfs_msspll_clk_ops = {

• .get_rate = mpfs_clk_msspll_recalc_rate,

+};

+U_BOOT_DRIVER(mpfs_msspll_clock) = {

• .name = MPFS_MSSPLL_CLOCK,
• .id = UCLASS_CLK,
• .ops = &mpfs_msspll_clk_ops,

+};

Remove new blank line at EOF

Other than that: Reviewed-by: Padmarao Begari padmarao.begari@microchip.com Tested-by: Padmarao Begari padmarao.begari@microchip.com

On Tue, 2022-10-25 at 08:58 +0100, Conor Dooley wrote: Sync the critical clocks in the U-Boot driver with those marked as critical in Linux. The Linux driver has an explanation of why each clock is considered to be critical, so import that too.

Fixes: 2f27c9219e ("clk: Add Microchip PolarFire SoC clock driver") Signed-off-by: Conor Dooley conor.dooley@microchip.com

---

drivers/clk/microchip/mpfs_clk_periph.c | 28 ++++++++++++++++++-----

1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/clk/microchip/mpfs_clk_periph.c b/drivers/clk/microchip/mpfs_clk_periph.c index e23eb552c3..ddeccb9145 100644 --- a/drivers/clk/microchip/mpfs_clk_periph.c +++ b/drivers/clk/microchip/mpfs_clk_periph.c @@ -114,13 +114,27 @@ static ulong mpfs_periph_clk_recalc_rate(struct clk *hw) .periph.flags = _flags, \ }

+/*

• • Critical clocks:
• • • CLK_ENVM: reserved by hart software services (hss) superloop

monitor/m mode interrupt

• • trap handler
• • • CLK_MMUART0: reserved by the hss
• • • CLK_DDRC: provides clock to the ddr subsystem
• • • CLK_RTC: the onboard RTC's AHB bus clock must be kept running

as the rtc will stop

• • if the AHB interface clock is disabled
• • • CLK_FICx: these provide the processor side clocks to the "FIC"

(Fabric InterConnect)

• • clock domain crossers which provide the interface to the FPGA

fabric. Disabling them

• • causes the FPGA fabric to go into reset.
• • • CLK_ATHENA: The athena clock is FIC4, which is reserved for the

Athena TeraFire.

• */

static struct mpfs_periph_hw_clock mpfs_periph_clks[] = { CLK_PERIPH(CLK_ENVM, "clk_periph_envm", CLK_AHB, 0, CLK_IS_CRITICAL), CLK_PERIPH(CLK_MAC0, "clk_periph_mac0", CLK_AHB, 1, 0), CLK_PERIPH(CLK_MAC1, "clk_periph_mac1", CLK_AHB, 2, 0), CLK_PERIPH(CLK_MMC, "clk_periph_mmc", CLK_AHB, 3, 0), CLK_PERIPH(CLK_TIMER, "clk_periph_timer", CLK_RTCREF, 4, 0),

• CLK_PERIPH(CLK_MMUART0, "clk_periph_mmuart0", CLK_AHB, 5, 0),

• CLK_PERIPH(CLK_MMUART0, "clk_periph_mmuart0", CLK_AHB, 5,

CLK_IS_CRITICAL), CLK_PERIPH(CLK_MMUART1, "clk_periph_mmuart1", CLK_AHB, 6, 0), CLK_PERIPH(CLK_MMUART2, "clk_periph_mmuart2", CLK_AHB, 7, 0), CLK_PERIPH(CLK_MMUART3, "clk_periph_mmuart3", CLK_AHB, 8, 0), @@ -132,17 +146,17 @@ static struct mpfs_periph_hw_clock mpfs_periph_clks[] = { CLK_PERIPH(CLK_CAN0, "clk_periph_can0", CLK_AHB, 14, 0), CLK_PERIPH(CLK_CAN1, "clk_periph_can1", CLK_AHB, 15, 0), CLK_PERIPH(CLK_USB, "clk_periph_usb", CLK_AHB, 16, 0),

• CLK_PERIPH(CLK_RTC, "clk_periph_rtc", CLK_AHB, 18, 0),

• CLK_PERIPH(CLK_RTC, "clk_periph_rtc", CLK_AHB, 18,

CLK_IS_CRITICAL), CLK_PERIPH(CLK_QSPI, "clk_periph_qspi", CLK_AHB, 19, 0), CLK_PERIPH(CLK_GPIO0, "clk_periph_gpio0", CLK_AHB, 20, 0), CLK_PERIPH(CLK_GPIO1, "clk_periph_gpio1", CLK_AHB, 21, 0), CLK_PERIPH(CLK_GPIO2, "clk_periph_gpio2", CLK_AHB, 22, 0), CLK_PERIPH(CLK_DDRC, "clk_periph_ddrc", CLK_AHB, 23, CLK_IS_CRITICAL),

• CLK_PERIPH(CLK_FIC0, "clk_periph_fic0", CLK_AXI, 24, 0),
• CLK_PERIPH(CLK_FIC1, "clk_periph_fic1", CLK_AXI, 25, 0),
• CLK_PERIPH(CLK_FIC2, "clk_periph_fic2", CLK_AXI, 26, 0),
• CLK_PERIPH(CLK_FIC3, "clk_periph_fic3", CLK_AXI, 27, 0),
• CLK_PERIPH(CLK_ATHENA, "clk_periph_athena", CLK_AXI, 28, 0),

• CLK_PERIPH(CLK_FIC0, "clk_periph_fic0", CLK_AXI, 24,

CLK_IS_CRITICAL),

• CLK_PERIPH(CLK_FIC1, "clk_periph_fic1", CLK_AXI, 25,

CLK_IS_CRITICAL),

• CLK_PERIPH(CLK_FIC2, "clk_periph_fic2", CLK_AXI, 26,

CLK_IS_CRITICAL),

• CLK_PERIPH(CLK_FIC3, "clk_periph_fic3", CLK_AXI, 27,

CLK_IS_CRITICAL),

• CLK_PERIPH(CLK_ATHENA, "clk_periph_athena", CLK_AXI, 28,

CLK_IS_CRITICAL), CLK_PERIPH(CLK_CFM, "clk_periph_cfm", CLK_AHB, 29, 0), };

Reviewed-by: Padmarao Begari padmarao.begari@microchip.com