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

Mon Nov 27 10:26:45 UTC 2017

Philipp,

On 11/22/2017 06:52 AM, Philipp Tomsich wrote:
>
>
> 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.

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