[U-Boot] [U-Boot, RESEND, 1/2] drivers/reset: support rockchip reset drivers

[Philipp Tomsich]

On Fri, 3 Nov 2017, Kever Yang wrote:

> From: Elaine Zhang <zhangqing at rock-chips.com>
>
> 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 <zhangqing at rock-chips.com>
> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
> Acked-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>

Reviewed-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>

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.

> +	  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...

Also, I am not convinced that this code will not break in the future...

> +		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.

> +
> +	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.

> +	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),
> +};
>