| 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
| ||
|
Message-ID: <20250322161945-GYC11633@gentoo>
Date: Sat, 22 Mar 2025 16:19:45 +0000
From: Yixun Lan <dlan@...too.org>
To: Alex Elder <elder@...cstar.com>
Cc: p.zabel@...gutronix.de, mturquette@...libre.com, sboyd@...nel.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
heylenay@....org, guodong@...cstar.com, paul.walmsley@...ive.com,
palmer@...belt.com, aou@...s.berkeley.edu, spacemit@...ts.linux.dev,
devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND 3/7] clk: spacemit: add reset controller support
Hi Alex:
On 10:18 Fri 21 Mar , Alex Elder wrote:
> Define ccu_reset_data as a structure that contains the constant
> register offset and bitmasks used to assert and deassert a reset
> control on a SpacemiT K1 CCU. Define ccu_reset_controller_data as
> a structure that contains the address of an array of those structures
> and a count of the number of elements in the array.
>
> Add a pointer to a ccu_reset_controller_data structure to the
> k1_ccu_data structure. Reset support is optional for SpacemiT CCUs;
> the new pointer field will be null for CCUs without any resets.
>
> Finally, define a new ccu_reset_controller structure, which (for
> a CCU with resets) contains a pointer to the constant reset data,
> the regmap to be used for the controller, and an embedded a reset
> controller structure.
>
> Each reset control is asserted or deasserted by updating bits in
> a register. The bits used are defined by an assert mask and a
> deassert mask. In some cases, one (non-zero) mask asserts reset
> and a different (non-zero) mask deasserts it. Otherwise one mask
> is nonzero, and the other is zero. Either way, the bits in
> both masks are cleared, then either the assert mask or the deassert
> mask is set in a register to affect the state of a reset control.
>
> Signed-off-by: Alex Elder <elder@...cstar.com>
> ---
> drivers/clk/spacemit/ccu-k1.c | 93 +++++++++++++++++++++++++++++++++++
> 1 file changed, 93 insertions(+)
>
> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ccu-k1.c
> index f7367271396a0..6d879411c6c05 100644
> --- a/drivers/clk/spacemit/ccu-k1.c
> +++ b/drivers/clk/spacemit/ccu-k1.c
> @@ -10,6 +10,7 @@
> #include <linux/minmax.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
>
> #include "ccu_common.h"
> #include "ccu_pll.h"
> @@ -134,8 +135,26 @@ struct spacemit_ccu_clk {
> struct clk_hw *hw;
> };
>
> +struct ccu_reset_data {
> + u32 offset;
> + u32 assert_mask;
> + u32 deassert_mask;
> +};
> +
> +struct ccu_reset_controller_data {
> + u32 count;
> + const struct ccu_reset_data *data; /* array */
> +};
> +
> struct k1_ccu_data {
> struct spacemit_ccu_clk *clk; /* array with sentinel */
> + const struct ccu_reset_controller_data *rst_data;
> +};
> +
> +struct ccu_reset_controller {
> + struct regmap *regmap;
> + const struct ccu_reset_controller_data *data;
> + struct reset_controller_dev rcdev;
> };
>
> /* APBS clocks start */
> @@ -1630,6 +1649,48 @@ static const struct k1_ccu_data k1_ccu_apmu_data = {
> .clk = k1_ccu_apmu_clks,
> };
>
> +static struct ccu_reset_controller *
> +rcdev_to_controller(struct reset_controller_dev *rcdev)
I'd suggest to avoid the line break to make it slightly more readable, intuitive
as the 80 column limit isn't hard rule
there are maybe more place similar to this, I won't add more comments
https://github.com/torvalds/linux/commit/bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> +{
> + return container_of(rcdev, struct ccu_reset_controller, rcdev);
> +}
since this function is only used once, open-code it?
but I'd fine with either way if you prefer to keep it
> +
> +static int
> +k1_rst_update(struct reset_controller_dev *rcdev, unsigned long id, bool assert)
s/k1_rst_update/k1_reset_update/g
this is a taste change, but I found more people follow this when grep driver/reset
> +{
> + struct ccu_reset_controller *controller = rcdev_to_controller(rcdev);
> + struct regmap *regmap = controller->regmap;
> + const struct ccu_reset_data *data;
> + u32 val;
> + int ret;
> +
> + data = &controller->data->data[id];
> +
> + ret = regmap_read(regmap, data->offset, &val);
> + if (ret)
> + return ret;
> +
> + val &= ~(data->assert_mask | data->deassert_mask);
> + val |= assert ? data->assert_mask : data->deassert_mask;
> +
> + return regmap_write(regmap, data->offset, val);
> +}
> +
> +static int k1_rst_assert(struct reset_controller_dev *rcdev, unsigned long id)
same reason, rst -> reset, more below
> +{
> + return k1_rst_update(rcdev, id, true);
> +}
> +
> +static int k1_rst_deassert(struct reset_controller_dev *rcdev, unsigned long id)
> +{
> + return k1_rst_update(rcdev, id, false);
> +}
> +
> +static const struct reset_control_ops k1_reset_control_ops = {
> + .assert = k1_rst_assert,
> + .deassert = k1_rst_deassert,
> +};
> +
> static int k1_ccu_register(struct device *dev, struct regmap *regmap,
> struct regmap *lock_regmap,
> struct spacemit_ccu_clk *clks)
> @@ -1675,6 +1736,33 @@ static int k1_ccu_register(struct device *dev, struct regmap *regmap,
> return ret;
> }
>
> +static int
> +k1_reset_controller_register(struct device *dev, struct regmap *regmap,
> + const struct ccu_reset_controller_data *data)
> +{
> + struct ccu_reset_controller *controller;
> + struct reset_controller_dev *rcdev;
> +
> + /* Resets are optional */
> + if (!data)
> + return 0;
> +
> + controller = devm_kzalloc(dev, sizeof(*controller), GFP_KERNEL);
> + if (!controller)
> + return -ENOMEM;
> +
> + controller->regmap = regmap;
> + controller->data = data;
> +
> + rcdev = &controller->rcdev;
..
> + rcdev->owner = THIS_MODULE;
move to last?
> + rcdev->nr_resets = data->count;
> + rcdev->ops = &k1_reset_control_ops;
> + rcdev->of_node = dev->of_node;
> +
> + return devm_reset_controller_register(dev, rcdev);
> +}
> +
> static int k1_ccu_probe(struct platform_device *pdev)
> {
> struct regmap *base_regmap, *lock_regmap = NULL;
> @@ -1710,6 +1798,11 @@ static int k1_ccu_probe(struct platform_device *pdev)
> if (ret)
> return dev_err_probe(dev, ret, "failed to register clocks\n");
>
> + ret = k1_reset_controller_register(dev, base_regmap, data->rst_data);
..
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to register reset controller\n");
same 100 column reason
> +
> return 0;
> }
>
> --
> 2.43.0
>
--
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55
Powered by blists - more mailing lists