| 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: <6dd9ed2750abe1f5174805673411ccb919ee5461.camel@pengutronix.de>
Date: Fri, 14 Feb 2025 12:54:17 +0100
From: Philipp Zabel <p.zabel@...gutronix.de>
To: Wilson Ding <dingwei@...vell.com>, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Cc: andrew@...n.ch, gregory.clement@...tlin.com,
sebastian.hesselbarth@...il.com, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, salee@...vell.com, gakula@...vell.com
Subject: Re: [PATCH 1/4] [PATCH 1/4] reset: simple: Add syscon device
compatible
On Do, 2025-02-13 at 22:58 -0800, Wilson Ding wrote:
> Introduce the new ops for updating reset line and getting status. Thus,
> the reset controller can be accessed through either direct I/O or regmap
> interfaces.
Please don't add a new layer of function pointer indirection, just add
a new struct reset_control_ops for the regmap variant.
> It enables the support of the syscon devices with the simple
> reset code. To adapt the DT binding of the syscon device, the number of
> reset lines must be specified in device data.
If the DT node had a reg property, number of reset lines could be
determined from its size, like for MMIO.
> Signed-off-by: Wilson Ding <dingwei@...vell.com>
> ---
> drivers/reset/reset-simple.c | 117 +++++++++++++++++++++++------
> include/linux/reset/reset-simple.h | 11 +++
> 2 files changed, 107 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
> index 276067839830..e4e777d40a79 100644
> --- a/drivers/reset/reset-simple.c
> +++ b/drivers/reset/reset-simple.c
> @@ -15,8 +15,10 @@
> #include <linux/device.h>
> #include <linux/err.h>
> #include <linux/io.h>
> +#include <linux/mfd/syscon.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> +#include <linux/regmap.h>
> #include <linux/reset-controller.h>
> #include <linux/reset/reset-simple.h>
> #include <linux/spinlock.h>
> @@ -27,10 +29,9 @@ to_reset_simple_data(struct reset_controller_dev *rcdev)
> return container_of(rcdev, struct reset_simple_data, rcdev);
> }
>
> -static int reset_simple_update(struct reset_controller_dev *rcdev,
> +static int reset_simple_update_mmio(struct reset_simple_data *data,
> unsigned long id, bool assert)
No need to rename or change the function prototype.
> {
> - struct reset_simple_data *data = to_reset_simple_data(rcdev);
> int reg_width = sizeof(u32);
> int bank = id / (reg_width * BITS_PER_BYTE);
> int offset = id % (reg_width * BITS_PER_BYTE);
> @@ -51,16 +52,40 @@ static int reset_simple_update(struct reset_controller_dev *rcdev,
> return 0;
> }
>
> +static int reset_simple_update_regmap(struct reset_simple_data *data,
> + unsigned long id, bool assert)
I'd call this reset_simple_regmap_update().
> +{
> + int reg_width = sizeof(u32);
> + int bank = id / (reg_width * BITS_PER_BYTE);
> + int offset = id % (reg_width * BITS_PER_BYTE);
> + u32 mask, val;
> +
> + mask = BIT(offset);
> +
> + if (assert ^ data->active_low)
> + val = mask;
> + else
> + val = 0;
> +
> + return regmap_write_bits(data->regmap,
> + data->reg_offset + (bank * reg_width),
> + mask, val);
> +}
> +
> static int reset_simple_assert(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> - return reset_simple_update(rcdev, id, true);
> + struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +
> + return data->ops.update(data, id, true);
> }
>
> static int reset_simple_deassert(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> - return reset_simple_update(rcdev, id, false);
> + struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +
> + return data->ops.update(data, id, false);
> }
No need for indirection. Better to just add separate
reset_simple_regmap_assert/deassert() functions.
> static int reset_simple_reset(struct reset_controller_dev *rcdev,
> @@ -81,10 +106,9 @@ static int reset_simple_reset(struct reset_controller_dev *rcdev,
> return reset_simple_deassert(rcdev, id);
> }
>
> -static int reset_simple_status(struct reset_controller_dev *rcdev,
> - unsigned long id)
> +static int reset_simple_status_mmio(struct reset_simple_data *data,
> + unsigned long id)
> {
> - struct reset_simple_data *data = to_reset_simple_data(rcdev);
> int reg_width = sizeof(u32);
> int bank = id / (reg_width * BITS_PER_BYTE);
> int offset = id % (reg_width * BITS_PER_BYTE);
> @@ -95,6 +119,31 @@ static int reset_simple_status(struct reset_controller_dev *rcdev,
> return !(reg & BIT(offset)) ^ !data->status_active_low;
> }
>
> +static int reset_simple_status_regmap(struct reset_simple_data *data,
> + unsigned long id)
> +{
> + int reg_width = sizeof(u32);
> + int bank = id / (reg_width * BITS_PER_BYTE);
> + int offset = id % (reg_width * BITS_PER_BYTE);
> + u32 reg;
> + int ret;
> +
> + ret = regmap_read(data->regmap, data->reg_offset + (bank * reg_width),
> + ®);
> + if (ret)
> + return ret;
> +
> + return !(reg & BIT(offset)) ^ !data->status_active_low;
> +}
> +
> +static int reset_simple_status(struct reset_controller_dev *rcdev,
> + unsigned long id)
> +{
> + struct reset_simple_data *data = to_reset_simple_data(rcdev);
> +
> + return data->ops.status(data, id);
> +}
Same as above, no need for indirection.
Just add separate reset_simple_regmap_assert/deassert() functions ...
> +
> const struct reset_control_ops reset_simple_ops = {
> .assert = reset_simple_assert,
> .deassert = reset_simple_deassert,
... and add a const struct reset_control_ops reset_simple_regmap_ops.
regards
Philipp
Powered by blists - more mailing lists