lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <55a896bcb94af629bb58c304268ac7ec.sboyd@kernel.org>
Date: Fri, 08 Mar 2024 21:21:34 -0800
From: Stephen Boyd <sboyd@...nel.org>
To: Albert Ou <aou@...s.berkeley.edu>, Chao Wei <chao.wei@...hgo.com>, Chen Wang <unicorn_wang@...look.com>, Conor Dooley <conor+dt@...nel.org>, Inochi Amaoto <inochiama@...look.com>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Michael Turquette <mturquette@...libre.com>, Palmer Dabbelt <palmer@...belt.com>, Paul Walmsley <paul.walmsley@...ive.com>, Rob Herring <robh+dt@...nel.org>
Cc: Jisheng Zhang <jszhang@...nel.org>, Liu Gui <kenneth.liu@...hgo.com>, Jingbao Qiu <qiujingbao.dlmu@...il.com>, dlan@...too.org, linux-clk@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-riscv@...ts.infradead.org
Subject: Re: [PATCH v8 2/8] clk: sophgo: Add CV1800/SG2000 series clock controller driver skeleton

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@...look.com>
> + */
> +
> +#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@...look.com>
> + */
> +
> +#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@...look.com>
> + */
> +
> +#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@...look.com>
> + */
> +
> +#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,

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ