On Mon, 2019-12-02 at 22:45 +0800, Jun Nie wrote:
> Extend reset operations to support combination of three type flags:
> ASSERT/DEASSERT SET/CLEAR POLL.
>
> Signed-off-by: Jun Nie <jun.nie@xxxxxxxxxx>
> ---
> drivers/clk/hisilicon/reset.c | 58 ++++++++++++++++++++++++++++++++---
> 1 file changed, 53 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/hisilicon/reset.c b/drivers/clk/hisilicon/reset.c
> index 93cee17db8b1..de7d186b0894 100644
> --- a/drivers/clk/hisilicon/reset.c
> +++ b/drivers/clk/hisilicon/reset.c
> @@ -2,20 +2,25 @@
> /*
> * Hisilicon Reset Controller Driver
> *
> - * Copyright (c) 2015-2016 HiSilicon Technologies Co., Ltd.
> + * Copyright (c) 2015-2019 HiSilicon Technologies Co., Ltd.
> */
>
> #include <linux/io.h>
> +#include <linux/iopoll.h>
> #include <linux/of_address.h>
> #include <linux/platform_device.h>
> #include <linux/reset-controller.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> +
> +#include <dt-bindings/reset/hisilicon-resets.h>
> #include "reset.h"
>
> #define HISI_RESET_BIT_MASK 0x1f
> #define HISI_RESET_OFFSET_SHIFT 8
> #define HISI_RESET_OFFSET_MASK 0xffff00
> +#define HISI_RESET_FLAG_SHIFT 24
> +#define HISI_RESET_FLAG_MASK 0xff000000
>
> struct hisi_reset_controller {
> spinlock_t lock;
> @@ -30,14 +35,17 @@ struct hisi_reset_controller {
> static int hisi_reset_of_xlate(struct reset_controller_dev *rcdev,
> const struct of_phandle_args *reset_spec)
> {
> + unsigned long flags;
> u32 offset;
> u8 bit;
>
> + flags = (reset_spec->args[2] << HISI_RESET_FLAG_SHIFT)
> + & HISI_RESET_FLAG_MASK;
> offset = (reset_spec->args[0] << HISI_RESET_OFFSET_SHIFT)
> & HISI_RESET_OFFSET_MASK;
> bit = reset_spec->args[1] & HISI_RESET_BIT_MASK;
>
> - return (offset | bit);
> + return (flags | offset | bit);
> }
>
> static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> @@ -48,13 +56,33 @@ static int hisi_reset_assert(struct reset_controller_dev *rcdev,
> u32 offset, reg;
> u8 bit;
>
> + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> + if (flags & HISI_ASSERT_NONE)
> + return -ENOTSUPP; /* assert not supported for this reset */
As long as .assert and .deassert are the only implemented operations for
this reset controller, this does not make any sense to me. Are there
resets that can only be deasserted?
> +
> offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
> bit = id & HISI_RESET_BIT_MASK;
>
> + pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
> + flags & HISI_ASSERT_POLL ? "poll" : "",
> + flags & HISI_ASSERT_SET ? "set":"clear", offset, bit);
> +
> + if (flags & HISI_ASSERT_POLL) {
Since HISI_ASSERT_POLL is 0, this is always false.
> + if (flags & HISI_ASSERT_SET)
> + return readl_poll_timeout(rstc->membase + offset,
> + reg, reg & BIT(bit), 0, 5000);
> + else
> + return readl_poll_timeout(rstc->membase + offset,
> + reg, !(reg & BIT(bit)),
> + 0, 5000);
And this is effectively dead code. Do you really have hardware that
asserts or asserts a reset line in reaction to a read access?
Should HISI_ASSERT_POLL and HISI_DEASSERT_POLL be mutually exclusive?
> + }
> +
> spin_lock_irqsave(&rstc->lock, flags);
>
> reg = readl(rstc->membase + offset);
> - writel(reg | BIT(bit), rstc->membase + offset);
> + /* Default is setting to assert for no flag case. */
> + reg = (flags & HISI_ASSERT_CLEAR) ? reg & ~BIT(bit) : reg | BIT(bit);
Consider moving this into a helper function with a boolean set/clear
parameter.
As long as HISI_ASSERT_CLEAR is 0, the first branch is dead code.
> + writel(reg, rstc->membase + offset);
>
> spin_unlock_irqrestore(&rstc->lock, flags);
>
> @@ -69,13 +97,33 @@ static int hisi_reset_deassert(struct reset_controller_dev *rcdev,
> u32 offset, reg;
> u8 bit;
>
> + flags = (id & HISI_RESET_FLAG_MASK) >> HISI_RESET_FLAG_SHIFT;
> + if (flags & HISI_DEASSERT_NONE)
> + return -ENOTSUPP; /* deassert not supported for this reset */
> +
Are there resets that can only ever be asserted?
> offset = (id & HISI_RESET_OFFSET_MASK) >> HISI_RESET_OFFSET_SHIFT;
> bit = id & HISI_RESET_BIT_MASK;
>
> + pr_devel("%s %s to %s 0x%x:bit[%d]\n", __func__,
> + flags & HISI_DEASSERT_POLL ? "poll" : "",
> + flags & HISI_DEASSERT_SET ? "clear":"set", offset, bit);
> +
> + if (flags & HISI_DEASSERT_POLL) {
> + if (flags & HISI_DEASSERT_SET)
> + return readl_poll_timeout(rstc->membase + offset,
> + reg, reg & BIT(bit), 0, 5000);
> + else
> + return readl_poll_timeout(rstc->membase + offset,
> + reg, !(reg & BIT(bit)),
> + 0, 5000);
See above, this code currently can never be reached.
> + }
> +
> spin_lock_irqsave(&rstc->lock, flags);
>
> reg = readl(rstc->membase + offset);
> - writel(reg & ~BIT(bit), rstc->membase + offset);
> + /* Default is clearing to deasseart for no flag case. */
> + reg = (flags & HISI_DEASSERT_SET) ? reg | BIT(bit) : reg & ~BIT(bit);
Same as above, the read-modify-write for set/clear could be split out
into a helper.
> + writel(reg, rstc->membase + offset);
>
> spin_unlock_irqrestore(&rstc->lock, flags);
>
> @@ -103,7 +151,7 @@ struct hisi_reset_controller *hisi_reset_init(struct platform_device *pdev)
> rstc->rcdev.owner = THIS_MODULE;
> rstc->rcdev.ops = &hisi_reset_ops;
> rstc->rcdev.of_node = pdev->dev.of_node;
> - rstc->rcdev.of_reset_n_cells = 2;
> + rstc->rcdev.of_reset_n_cells = 3;
This breaks current device trees (before patch 3). You can make sure
device trees with #reset-cells = <2> keep working by parsing the #reset-
cells and setting this value to 2 for old DTs.
> rstc->rcdev.of_xlate = hisi_reset_of_xlate;
> reset_controller_register(&rstc->rcdev);
regards
Philipp
