Re: [PATCH v8 2/8] clk: sophgo: Add CV1800/SG2000 series clock controller driver skeleton

From: Stephen Boyd
Date: Sat Mar 09 2024 - 00:21:45 EST

Next message: Jakub Kicinski: "Re: [PATCH net-next v10 13/13] Documentation: networking: document phy_link_topology"
Previous message: syzbot: "Re: [syzbot] [netfilter?] KASAN: slab-use-after-free Read in ip_skb_dst_mtu"
Next in thread: Inochi Amaoto: "Re: [PATCH v8 2/8] clk: sophgo: Add CV1800/SG2000 series clock controller driver skeleton"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Quoting Inochi Amaoto (2024-02-13 00:22:34)
> diff --git a/drivers/clk/sophgo/Kconfig b/drivers/clk/sophgo/Kconfig
> new file mode 100644
> index 000000000000..d67009fa749f
> --- /dev/null
> +++ b/drivers/clk/sophgo/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# common clock support for SOPHGO SoC family.
> +
> +config CLK_SOPHGO_CV1800
> + tristate "Support for the Sophgo CV1800 series SoCs clock controller"
> + default m

Please remove any default and set it in the defconfig instead.

> + depends on ARCH_SOPHGO || COMPILE_TEST
> + help
> + This driver supports clock controller of Sophgo CV18XX series SoC.
> + The driver require a 25MHz Oscillator to function generate clock.
> + It includes PLLs, common clock function and some vendor clock for
> + IPs of CV18XX series SoC
> diff --git a/drivers/clk/sophgo/clk-cv1800.c b/drivers/clk/sophgo/clk-cv1800.c
> new file mode 100644
> index 000000000000..7183e67f20bf
> --- /dev/null
> +++ b/drivers/clk/sophgo/clk-cv1800.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Inochi Amaoto <inochiama@xxxxxxxxxxx>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/spinlock.h>
> +
> +#include "clk-cv18xx-common.h"
> +
> +struct cv1800_clk_ctrl;
> +
> +struct cv1800_clk_desc {
> + struct clk_hw_onecell_data *clks_data;
> +
> + int (*pre_init)(struct device *dev, void __iomem *base,
> + struct cv1800_clk_ctrl *ctrl,
> + const struct cv1800_clk_desc *desc);
> +};
> +
> +struct cv1800_clk_ctrl {
> + const struct cv1800_clk_desc *desc;
> + spinlock_t lock;
> +};
> +
> +static int cv1800_clk_init_ctrl(struct device *dev, void __iomem *reg,
> + struct cv1800_clk_ctrl *ctrl,
> + const struct cv1800_clk_desc *desc)
> +{
> + int i, ret;
> +
> + ctrl->desc = desc;
> + spin_lock_init(&ctrl->lock);
> +
> + for (i = 0; i < desc->clks_data->num; i++) {
> + struct clk_hw *hw = desc->clks_data->hws[i];
> + struct cv1800_clk_common *common;
> + const char *name;
> +
> + if (!hw)
> + continue;
> +
> + name = hw->init->name;
> +
> + common = hw_to_cv1800_clk_common(hw);
> + common->base = reg;
> + common->lock = &ctrl->lock;
> +
> + ret = devm_clk_hw_register(dev, hw);
> + if (ret) {
> + dev_err(dev, "Couldn't register clock %d - %s\n",
> + i, name);
> + return ret;
> + }
> + }
> +
> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
> + desc->clks_data);
> +
> + return ret;

Just return devm...

> +}
> +
> +static int cv1800_clk_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + void __iomem *reg;
> + int ret;
> + const struct cv1800_clk_desc *desc;
> + struct cv1800_clk_ctrl *ctrl;
> +
> + reg = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(reg))
> + return PTR_ERR(reg);
> +
> + desc = device_get_match_data(dev);
> + if (!desc) {
> + dev_err(dev, "no match data for platform\n");
> + return -EINVAL;
> + }
> +
> + ctrl = devm_kmalloc(dev, sizeof(*ctrl), GFP_KERNEL);

Why not devm_kzalloc?

> + if (!ctrl)
> + return -ENOMEM;
> +
> + if (desc->pre_init) {
> + ret = desc->pre_init(dev, reg, ctrl, desc);
> + if (ret)
> + return ret;
> + }
> +
> + ret = cv1800_clk_init_ctrl(dev, reg, ctrl, desc);
> +
> + return ret;

This is return cv1800_clk_init_ctrl(...

> +}
> +
> +static const struct of_device_id cv1800_clk_ids[] = {
> + { }

Don't do this. Just send the whole driver as one patch.

> +};
> +MODULE_DEVICE_TABLE(of, cv1800_clk_ids);
> +
> +static struct platform_driver cv1800_clk_driver = {
> + .probe = cv1800_clk_probe,
> + .driver = {
> + .name = "cv1800-clk",
> + .suppress_bind_attrs = true,
> + .of_match_table = cv1800_clk_ids,
> + },
> +};
> +module_platform_driver(cv1800_clk_driver);
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/clk/sophgo/clk-cv18xx-common.c b/drivers/clk/sophgo/clk-cv18xx-common.c
> new file mode 100644
> index 000000000000..cbcdd88f0e23
> --- /dev/null
> +++ b/drivers/clk/sophgo/clk-cv18xx-common.c
> @@ -0,0 +1,66 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Inochi Amaoto <inochiama@xxxxxxxxxxx>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/spinlock.h>
> +#include <linux/bug.h>
> +
> +#include "clk-cv18xx-common.h"
> +
> +int cv1800_clk_setbit(struct cv1800_clk_common *common,
> + struct cv1800_clk_regbit *field)
> +{
> + u32 mask = BIT(field->shift);
> + u32 value;
> + unsigned long flags;
> +
> + spin_lock_irqsave(common->lock, flags);
> +
> + value = readl(common->base + field->reg);
> + writel(value | mask, common->base + field->reg);
> +
> + spin_unlock_irqrestore(common->lock, flags);
> +
> + return 0;
> +}
> +
> +int cv1800_clk_clearbit(struct cv1800_clk_common *common,
> + struct cv1800_clk_regbit *field)
> +{
> + u32 mask = BIT(field->shift);
> + u32 value;
> + unsigned long flags;
> +
> + spin_lock_irqsave(common->lock, flags);
> +
> + value = readl(common->base + field->reg);
> + writel(value & ~mask, common->base + field->reg);
> +
> + spin_unlock_irqrestore(common->lock, flags);
> +
> + return 0;
> +}
> +
> +int cv1800_clk_checkbit(struct cv1800_clk_common *common,
> + struct cv1800_clk_regbit *field)
> +{
> + return readl(common->base + field->reg) & BIT(field->shift);
> +}
> +
> +#define PLL_LOCK_TIMEOUT_US (200 * 1000)
> +
> +void cv1800_clk_wait_for_lock(struct cv1800_clk_common *common,
> + u32 reg, u32 lock)
> +{
> + void __iomem *addr = common->base + reg;
> + u32 regval;
> +
> + if (!lock)
> + return;
> +
> + WARN_ON(readl_relaxed_poll_timeout(addr, regval, regval & lock,
> + 100, PLL_LOCK_TIMEOUT_US));
> +}
> diff --git a/drivers/clk/sophgo/clk-cv18xx-common.h b/drivers/clk/sophgo/clk-cv18xx-common.h
> new file mode 100644
> index 000000000000..2bfda02b2064
> --- /dev/null
> +++ b/drivers/clk/sophgo/clk-cv18xx-common.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023 Inochi Amaoto <inochiama@xxxxxxxxxxx>
> + */
> +
> +#ifndef _CLK_SOPHGO_CV18XX_IP_H_
> +#define _CLK_SOPHGO_CV18XX_IP_H_
> +
> +#include <linux/compiler.h>
> +#include <linux/clk-provider.h>
> +#include <linux/bitfield.h>
> +
> +struct cv1800_clk_common {
> + void __iomem *base;
> + spinlock_t *lock;
> + struct clk_hw hw;
> + unsigned long features;
> +};
> +
> +#define CV1800_CLK_COMMON(_name, _parents, _op, _flags) \
> + { \
> + .hw.init = CLK_HW_INIT_PARENTS_DATA(_name, _parents, \
> + _op, _flags), \
> + }
> +
> +static inline struct cv1800_clk_common *
> +hw_to_cv1800_clk_common(struct clk_hw *hw)
> +{
> + return container_of(hw, struct cv1800_clk_common, hw);
> +}
> +
> +struct cv1800_clk_regbit {
> + u16 reg;
> + s8 shift;
> +};
> +
> +struct cv1800_clk_regfield {
> + u16 reg;
> + u8 shift;
> + u8 width;
> + s16 initval;
> + unsigned long flags;
> +};
> +
> +#define CV1800_CLK_BIT(_reg, _shift) \
> + { \
> + .reg = _reg, \
> + .shift = _shift, \
> + }
> +
> +#define CV1800_CLK_REG(_reg, _shift, _width, _initval, _flags) \
> + { \
> + .reg = _reg, \
> + .shift = _shift, \
> + .width = _width, \
> + .initval = _initval, \
> + .flags = _flags, \
> + }
> +
> +#define cv1800_clk_regfield_genmask(_reg) \
> + GENMASK((_reg)->shift + (_reg)->width - 1, (_reg)->shift)
> +#define cv1800_clk_regfield_get(_val, _reg) \
> + (((_val) >> (_reg)->shift) & GENMASK((_reg)->width - 1, 0))
> +#define cv1800_clk_regfield_set(_val, _new, _reg) \
> + (((_val) & ~cv1800_clk_regfield_genmask((_reg))) | \
> + (((_new) & GENMASK((_reg)->width - 1, 0)) << (_reg)->shift))
> +
> +#define _CV1800_SET_FIELD(_reg, _val, _field) \
> + (((_reg) & ~(_field)) | FIELD_PREP((_field), (_val)))
> +
> +int cv1800_clk_setbit(struct cv1800_clk_common *common,
> + struct cv1800_clk_regbit *field);
> +int cv1800_clk_clearbit(struct cv1800_clk_common *common,
> + struct cv1800_clk_regbit *field);
> +int cv1800_clk_checkbit(struct cv1800_clk_common *common,
> + struct cv1800_clk_regbit *field);
> +
> +void cv1800_clk_wait_for_lock(struct cv1800_clk_common *common,
> + u32 reg, u32 lock);
> +
> +#endif // _CLK_SOPHGO_CV18XX_IP_H_
> diff --git a/drivers/clk/sophgo/clk-cv18xx-ip.c b/drivers/clk/sophgo/clk-cv18xx-ip.c
> new file mode 100644
> index 000000000000..cd397d102442
> --- /dev/null
> +++ b/drivers/clk/sophgo/clk-cv18xx-ip.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 Inochi Amaoto <inochiama@xxxxxxxxxxx>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/io.h>
> +#include <linux/gcd.h>
> +#include <linux/spinlock.h>
> +
> +#include "clk-cv18xx-ip.h"
> +
> +/* GATE */
> +const struct clk_ops cv1800_clk_gate_ops = {
> + .disable = NULL,
> + .enable = NULL,
> + .is_enabled = NULL,
> +
> + .recalc_rate = NULL,
> + .round_rate = NULL,
> + .set_rate = NULL,
> +};

Everything is NULL. What are you trying to do? Point out what will come
later? Please squash patches.

> +
> +/* DIV */
> +const struct clk_ops cv1800_clk_div_ops = {
> + .disable = NULL,
> + .enable = NULL,
> + .is_enabled = NULL,
> +
> + .determine_rate = NULL,
> + .recalc_rate = NULL,
> + .set_rate = NULL,
> +};
> +
> +const struct clk_ops cv1800_clk_bypass_div_ops = {
> + .disable = NULL,
> + .enable = NULL,
> + .is_enabled = NULL,
> +
> + .determine_rate = NULL,

Next message: Jakub Kicinski: "Re: [PATCH net-next v10 13/13] Documentation: networking: document phy_link_topology"
Previous message: syzbot: "Re: [syzbot] [netfilter?] KASAN: slab-use-after-free Read in ip_skb_dst_mtu"
Next in thread: Inochi Amaoto: "Re: [PATCH v8 2/8] clk: sophgo: Add CV1800/SG2000 series clock controller driver skeleton"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]