Philipp,
On 11/22/2017 06:52 AM, Philipp Tomsich wrote:
On Fri, 3 Nov 2017, Kever Yang wrote:
From: Elaine Zhang <[email protected]>
Create driver to support all Rockchip SoCs soft reset.
Example of usage:
i2c driver:
ret = reset_get_by_name(dev, "i2c", &reset_ctl);
if (ret) {
error("reset_get_by_name() failed: %d\n", ret);
}
reset_assert(&reset_ctl);
udelay(50);
reset_deassert(&reset_ctl);
i2c dts node:
resets = <&cru SRST_P_I2C1>, <&cru SRST_I2C1>;
reset-names = "p_i2c", "i2c";
Signed-off-by: Elaine Zhang <[email protected]>
Signed-off-by: Kever Yang <[email protected]>
Acked-by: Philipp Tomsich <[email protected]>
Reviewed-by: Philipp Tomsich <[email protected]>
See below for requested changes.
---
drivers/reset/Kconfig | 8 ++++
drivers/reset/Makefile | 1 +
drivers/reset/reset-rockchip.c | 104
+++++++++++++++++++++++++++++++++++++++++
3 files changed, 113 insertions(+)
create mode 100644 drivers/reset/reset-rockchip.c
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index ce46e27..c0d6d75 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -74,4 +74,12 @@ config AST2500_RESET
resets that are supported by watchdog. The main limitation though
is that some reset signals, like I2C or MISC reset multiple
devices.
+config RESET_ROCKCHIP
+ bool "Reset controller driver for Rockchip SoCs"
+ depends on DM_RESET && CLK
+ default y
+ help
+ Support for reset controller on rockchip SoC. The main
limitation though
This line is too long.
Will fix.
+ is that some reset signals, like I2C or MISC reset multiple
devices.
+
endmenu
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 252cefe..7d7e080 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_TEGRA186_RESET) += tegra186-reset.o
obj-$(CONFIG_RESET_BCM6345) += reset-bcm6345.o
obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
obj-$(CONFIG_AST2500_RESET) += ast2500-reset.o
+obj-$(CONFIG_RESET_ROCKCHIP) += reset-rockchip.o
diff --git a/drivers/reset/reset-rockchip.c
b/drivers/reset/reset-rockchip.c
new file mode 100644
index 0000000..322ac27
--- /dev/null
+++ b/drivers/reset/reset-rockchip.c
@@ -0,0 +1,104 @@
+/*
+ * (C) Copyright 2017 Rockchip Electronics Co., Ltd
+ *
+ * SPDX-License-Identifier: GPL-2.0
+ */
+
+#include <common.h>
+#include <dm.h>
+#include <reset-uclass.h>
+#include <linux/io.h>
+
+struct rockchip_reset_priv {
+ void __iomem *base;
+ unsigned int sf_reset_offset;
+ unsigned int sf_reset_num;
+};
+
+static int rockchip_reset_request(struct reset_ctl *reset_ctl)
+{
+ struct rockchip_reset_priv *priv = dev_get_priv(reset_ctl->dev);
+
+ debug("%s(reset_ctl=%p) (dev=%p, id=%lu) (sf_reset_num=%d)\n",
__func__,
+ reset_ctl, reset_ctl->dev, reset_ctl->id,
priv->sf_reset_num);
+
+ if (reset_ctl->id / 16 >= priv->sf_reset_num)
Can you please write this in a more readable/maintainable way (and
possibly also add a comment)? E.g. in order to understand this, the
reader needs to know that there will be exactly 16 resets per register
and that these will be continuous (i.e. no holes), so this comparison
only makes sense for the final register...
Will add a comment, and a MAGIC MACRO will better than 16.
Also, I am not convinced that this code will not break in the future...
There is only one commit in kernel for softreset since 2014 and it used
for all rockchip SoCs.
drivers/clk/rockchip/softrst.c
+ return -EINVAL;
+
+ return 0;
+}
+
+static int rockchip_reset_free(struct reset_ctl *reset_ctl)
+{
+ debug("%s(reset_ctl=%p) (dev=%p, id=%lu)\n", __func__, reset_ctl,
+ reset_ctl->dev, reset_ctl->id);
+
+ return 0;
+}
+
+static int rockchip_reset_assert(struct reset_ctl *reset_ctl)
+{
+ struct rockchip_reset_priv *priv = dev_get_priv(reset_ctl->dev);
+ int bank = reset_ctl->id / 16;
+ int offset = reset_ctl->id % 16;
+
+ debug("%s(reset_ctl=%p) (dev=%p, id=%lu) (reg_addr=%p)\n",
__func__,
+ reset_ctl, reset_ctl->dev, reset_ctl->id,
+ priv->base + (bank * 4));
+
+ writel(BIT(offset) | (BIT(offset) << 16), priv->base + (bank * 4));
Again: it should be made clear to the reader why there's
1. (BIT | BIT << 16)
2. back is multiplied by 4
Also, if I am correct rk_setreg(...) should be used.
Yes, you are right, should use rk_setreg().
+
+ return 0;
+}
+
+static int rockchip_reset_deassert(struct reset_ctl *reset_ctl)
+{
+ struct rockchip_reset_priv *priv = dev_get_priv(reset_ctl->dev);
+ int bank = reset_ctl->id / 16;
+ int offset = reset_ctl->id % 16;
Again, these computations are magic without clearer code and/or
documentation.
+
+ debug("%s(reset_ctl=%p) (dev=%p, id=%lu) (reg_addr=%p)\n",
__func__,
+ reset_ctl, reset_ctl->dev, reset_ctl->id,
+ priv->base + (bank * 4));
+
+ writel((BIT(offset) << 16), priv->base + (bank * 4));
See above. Also, this looks like a case for rk_clrreg(...).
+
+ return 0;
+}
+
+struct reset_ops rockchip_reset_ops = {
+ .request = rockchip_reset_request,
+ .free = rockchip_reset_free,
+ .rst_assert = rockchip_reset_assert,
+ .rst_deassert = rockchip_reset_deassert,
+};
+
+static int rockchip_reset_probe(struct udevice *dev)
+{
+ struct rockchip_reset_priv *priv = dev_get_priv(dev);
+ fdt_addr_t addr;
+ fdt_size_t size;
+
+ addr = devfdt_get_addr_size_index(dev, 0, &size);
Please use the dev_...() function family.
Will fix.
Thanks,
- Kever
+ if (addr == FDT_ADDR_T_NONE)
+ return -EINVAL;
+
+ if ((priv->sf_reset_offset == 0) && (priv->sf_reset_num == 0))
+ return -EINVAL;
+
+ addr += priv->sf_reset_offset;
+ priv->base = ioremap(addr, size);
+
+ debug("%s(base=%p) (sf_reset_offset=%x, sf_reset_num=%d)\n",
__func__,
+ priv->base, priv->sf_reset_offset, priv->sf_reset_num);
+
+ return 0;
+}
+
+U_BOOT_DRIVER(rockchip_reset) = {
+ .name = "rockchip_reset",
+ .id = UCLASS_RESET,
+ .probe = rockchip_reset_probe,
+ .ops = &rockchip_reset_ops,
+ .priv_auto_alloc_size = sizeof(struct rockchip_reset_priv),
+};
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot
