Re: [PATCH v1 03/10] clk: add Microchip LAN966X / LAN969X generic clock controller driver

["Sascha Hauer" <s.hauer@pengutronix.de>]

On 2026-06-12 14:58, Marco Felsch wrote:
> Hi Oleksij,
> 
> thanks for your patch, please see below.
> 
> On 26-06-12, Oleksij Rempel wrote:
> > Port the Microchip LAN966X Generic Clock Controller (GCK) driver from
> > the Linux kernel. GCK generates and supplies clocks to peripherals on
> > the LAN966X and LAN969X switch SoCs (UART, SDHCI, QSPI, SGPIO, ...).
> > 
> > Ported from Linux drivers/clk/clk-lan966x.c at tag v7.1-rc7
> > Barebox-specific deltas:
> > - probe() takes `struct device *` instead of `platform_device`
> > - no devm_* - explicit kfree on probe failure
> > - clk_parent_data carries both `.fw_name` (for Linux source-compat) and
> >   `.name` (used by barebox at register time, since barebox's clk
> >   framework does not resolve `fw_name` via clock-names).
> > - Linux's `.determine_rate` is implemented here as `.round_rate`.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/clk/Kconfig       |   9 ++
> >  drivers/clk/Makefile      |   1 +
> >  drivers/clk/clk-lan966x.c | 325 ++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 335 insertions(+)
> >  create mode 100644 drivers/clk/clk-lan966x.c
> > 
> > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> > index d2a61329e125..fe7056f8971b 100644
> > --- a/drivers/clk/Kconfig
> > +++ b/drivers/clk/Kconfig
> > @@ -84,4 +84,13 @@ config COMMON_CLK_GPIO
> >  
> >  source "drivers/clk/sifive/Kconfig"
> >  
> > +config COMMON_CLK_LAN966X
> > +	bool "Generic Clock Controller driver for LAN966X SoC"
> > +	depends on OFDEVICE
> > +	depends on COMMON_CLK
> > +	help
> > +	  Microchip LAN966X and LAN969X SoC Generic Clock Controller (GCK).
> > +	  GCK generates and supplies clocks to various peripherals within the
> > +	  SoC.
> > +
> >  endif
> > diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> > index 4fda2c1e0dd3..d7e06de04230 100644
> > --- a/drivers/clk/Makefile
> > +++ b/drivers/clk/Makefile
> > @@ -33,3 +33,4 @@ obj-$(CONFIG_COMMON_CLK_SCMI)	+= clk-scmi.o
> >  obj-$(CONFIG_COMMON_CLK_GPIO)   += clk-gpio.o
> >  obj-$(CONFIG_TI_SCI_CLK)	+= ti-sci-clk.o
> >  obj-$(CONFIG_ARCH_K3)		+= k3/
> > +obj-$(CONFIG_COMMON_CLK_LAN966X)	+= clk-lan966x.o
> > diff --git a/drivers/clk/clk-lan966x.c b/drivers/clk/clk-lan966x.c
> > new file mode 100644
> > index 000000000000..a2f43b2f8dce
> > --- /dev/null
> > +++ b/drivers/clk/clk-lan966x.c
> > @@ -0,0 +1,325 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Microchip LAN966x SoC Clock driver.
> > + *
> > + * Copyright (C) 2021 Microchip Technology, Inc. and its subsidiaries
> > + *
> > + * Author: Kavyasree Kotagiri <kavyasree.kotagiri@microchip.com>
> > + *
> > + * Ported from Linux drivers/clk/clk-lan966x.c. Structure and identifiers
> > + * are kept aligned with Linux so future fixes can be backported with
> > + * minimal context churn.
> > + */
> > +
> > +#include <common.h>
> > +#include <init.h>
> > +#include <driver.h>
> > +#include <io.h>
> > +#include <of.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/spinlock.h>
> > +
> > +#define GCK_ENA         BIT(0)
> > +#define GCK_SRC_SEL     GENMASK(9, 8)
> > +#define GCK_PRESCALER   GENMASK(23, 16)
> > +
> > +#define DIV_MAX		255
> > +
> > +static const char * const lan966x_clk_names[] = {
> > +	"qspi0", "qspi1", "qspi2", "sdmmc0",
> > +	"pi", "mcan0", "mcan1", "flexcom0",
> > +	"flexcom1", "flexcom2", "flexcom3",
> > +	"flexcom4", "timer1", "usb_refclk",
> > +};
> > +
> > +static const char * const lan969x_clk_names[] = {
> > +	"qspi0", "qspi2", "sdmmc0", "sdmmc1",
> > +	"mcan0", "mcan1", "flexcom0",
> > +	"flexcom1", "flexcom2", "flexcom3",
> > +	"timer1", "usb_refclk",
> > +};
> > +
> > +struct lan966x_gck {
> > +	struct clk_hw hw;
> > +	void __iomem *reg;
> > +};
> > +#define to_lan966x_gck(hw) container_of(hw, struct lan966x_gck, hw)
> > +
> > +static const struct clk_parent_data lan966x_gck_pdata[] = {
> > +	{ .fw_name = "cpu", .name = "cpu-clk", },
> > +	{ .fw_name = "ddr", .name = "ddr-clk", },
> > +	{ .fw_name = "sys", .name = "fx100-clk", },
> > +};
> > +
> > +static struct clk_init_data init = {
> > +	.parent_data = lan966x_gck_pdata,
> > +	.num_parents = ARRAY_SIZE(lan966x_gck_pdata),
> > +};
> > +
> > +struct clk_gate_soc_desc {
> > +	const char *name;
> > +	int bit_idx;
> > +};
> > +
> > +static const struct clk_gate_soc_desc lan966x_clk_gate_desc[] = {
> > +	{ "uhphs", 11 },
> > +	{ "udphs", 10 },
> > +	{ "mcramc", 9 },
> > +	{ "hmatrix", 8 },
> > +	{ }
> > +};
> > +
> > +static const struct clk_gate_soc_desc lan969x_clk_gate_desc[] = {
> > +	{ "usb_drd", 10 },
> > +	{ "mcramc", 9 },
> > +	{ "hmatrix", 8 },
> > +	{ }
> > +};
> > +
> > +struct lan966x_match_data {
> > +	char *name;
> > +	const char * const *clk_name;
> > +	const struct clk_gate_soc_desc *clk_gate_desc;
> > +	u8 num_generic_clks;
> > +	u8 num_total_clks;
> > +};
> > +
> > +static struct lan966x_match_data lan966x_desc = {
> > +	.name = "lan966x",
> > +	.clk_name = lan966x_clk_names,
> > +	.clk_gate_desc = lan966x_clk_gate_desc,
> > +	.num_total_clks = 18,
> > +	.num_generic_clks = 14,
> > +};
> > +
> > +static struct lan966x_match_data lan969x_desc = {
> > +	.name = "lan969x",
> > +	.clk_name = lan969x_clk_names,
> > +	.clk_gate_desc = lan969x_clk_gate_desc,
> > +	.num_total_clks = 15,
> > +	.num_generic_clks = 12,
> > +};
> > +
> > +static DEFINE_SPINLOCK(clk_gate_lock);
> > +static void __iomem *base;
> 
> This seems odd, is this also part of the Linux kernel? Furthermore it's
> only used during lan966x_gck_clk_register().
> 
> > +static int lan966x_gck_enable(struct clk_hw *hw)
> > +{
> > +	struct lan966x_gck *gck = to_lan966x_gck(hw);
> > +	u32 val = readl(gck->reg);
> > +
> > +	val |= GCK_ENA;
> > +	writel(val, gck->reg);
> > +
> > +	return 0;
> > +}
> > +
> > +static void lan966x_gck_disable(struct clk_hw *hw)
> > +{
> > +	struct lan966x_gck *gck = to_lan966x_gck(hw);
> > +	u32 val = readl(gck->reg);
> > +
> > +	val &= ~GCK_ENA;
> > +	writel(val, gck->reg);
> > +}
> > +
> > +static int lan966x_gck_set_rate(struct clk_hw *hw,
> > +				unsigned long rate,
> > +				unsigned long parent_rate)
> > +{
> > +	struct lan966x_gck *gck = to_lan966x_gck(hw);
> > +	u32 div, val = readl(gck->reg);
> > +
> > +	if (rate == 0 || parent_rate == 0)
> > +		return -EINVAL;
> > +
> > +	/* Set Prescalar */
> > +	div = parent_rate / rate;
> > +	val &= ~GCK_PRESCALER;
> > +	val |= FIELD_PREP(GCK_PRESCALER, (div - 1));
> > +	writel(val, gck->reg);
> > +
> > +	return 0;
> > +}
> > +
> > +static unsigned long lan966x_gck_recalc_rate(struct clk_hw *hw,
> > +					     unsigned long parent_rate)
> > +{
> > +	struct lan966x_gck *gck = to_lan966x_gck(hw);
> > +	u32 div, val = readl(gck->reg);
> > +
> > +	div = FIELD_GET(GCK_PRESCALER, val);
> > +
> > +	return parent_rate / (div + 1);
> > +}
> > +
> > +/*
> > + * Linux uses .determine_rate, which barebox does not have. round_rate is
> > + * called against the already-selected parent, so we just clamp the divider.
> > + * Source selection happens via .set_parent / .get_parent.
> > + */
> > +static long lan966x_gck_round_rate(struct clk_hw *hw, unsigned long rate,
> > +				   unsigned long *parent_rate)
> > +{
> > +	unsigned long div;
> > +
> > +	if (!rate || !*parent_rate)
> > +		return 0;
> > +
> > +	div = DIV_ROUND_CLOSEST(*parent_rate, rate);
> > +	if (div > DIV_MAX + 1)
> > +		div = DIV_MAX + 1;
> > +	if (div < 1)
> > +		div = 1;
> > +
> > +	return *parent_rate / div;
> > +}
> > +
> > +static int lan966x_gck_get_parent(struct clk_hw *hw)
> > +{
> > +	struct lan966x_gck *gck = to_lan966x_gck(hw);
> > +	u32 val = readl(gck->reg);
> > +
> > +	return FIELD_GET(GCK_SRC_SEL, val);
> > +}
> > +
> > +static int lan966x_gck_set_parent(struct clk_hw *hw, u8 index)
> > +{
> > +	struct lan966x_gck *gck = to_lan966x_gck(hw);
> > +	u32 val = readl(gck->reg);
> > +
> > +	val &= ~GCK_SRC_SEL;
> > +	val |= FIELD_PREP(GCK_SRC_SEL, index);
> > +	writel(val, gck->reg);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct clk_ops lan966x_gck_ops = {
> > +	.enable		= lan966x_gck_enable,
> > +	.disable	= lan966x_gck_disable,
> > +	.set_rate	= lan966x_gck_set_rate,
> > +	.recalc_rate	= lan966x_gck_recalc_rate,
> > +	.round_rate	= lan966x_gck_round_rate,
> > +	.set_parent	= lan966x_gck_set_parent,
> > +	.get_parent	= lan966x_gck_get_parent,
> > +};
> > +
> > +static struct clk_hw *lan966x_gck_clk_register(struct device *dev, int i)
> > +{
> > +	struct lan966x_gck *priv;
> > +	int ret;
> > +
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	priv->reg = base + (i * 4);
> > +	priv->hw.init = &init;
> > +	ret = clk_hw_register(dev, &priv->hw);
> > +	if (ret) {
> > +		kfree(priv);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	return &priv->hw;
> > +};
> > +
> > +static int lan966x_gate_clk_register(struct device *dev,
> > +				     const struct lan966x_match_data *data,
> > +				     struct clk_onecell_data *clk_data,
> > +				     void __iomem *gate_base)
> > +{
> > +	struct clk_hw *hw;
> > +	int i;
> > +
> > +	for (i = data->num_generic_clks; i < data->num_total_clks; ++i) {
> > +		int idx = i - data->num_generic_clks;
> > +		const struct clk_gate_soc_desc *desc;
> > +
> > +		desc = &data->clk_gate_desc[idx];
> > +
> > +		hw = clk_hw_register_gate(dev, desc->name,
> > +					  data->name, 0, gate_base,
> > +					  desc->bit_idx,
> > +					  0, &clk_gate_lock);
> > +		if (IS_ERR(hw)) {
> > +			dev_err(dev, "failed to register %s clock\n",
> > +				desc->name);
> > +			return PTR_ERR(hw);
> > +		}
> > +		clk_data->clks[i] = clk_hw_to_clk(hw);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int lan966x_clk_probe(struct device *dev)
> > +{
> > +	const struct lan966x_match_data *data;
> > +	struct clk_onecell_data *clk_data;
> > +	struct resource *iores;
> > +	int i, ret;
> > +
> > +	data = device_get_match_data(dev);
> > +	if (!data)
> > +		return -EINVAL;
> > +
> > +	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> > +	if (!clk_data)
> > +		return -ENOMEM;
> > +
> > +	clk_data->clks = kcalloc(data->num_total_clks,
> > +				 sizeof(*clk_data->clks), GFP_KERNEL);
> > +	if (!clk_data->clks)
> > +		return -ENOMEM;
> 		^
> This leaks the clk_data memory.

Usually we ignore the memory leaks in the probe paths as there's no real
gain in freeing a few bytes from a functions error path that is executed
only once.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |