Hi Philipp,

On Mon, Jul 30, 2018 at 12:21:52PM +0200, Philipp Zabel wrote:

Hi Manivannan,

Thank you for the patch, a few small comments below:

On Sat, 2018-07-28 at 00:15 +0530, Manivannan Sadhasivam wrote:

quoted

Add Reset Management Unit (RMU) support for Actions Semi Owl SoCs.

Signed-off-by: Manivannan Sadhasivam <redacted>
---
 drivers/clk/actions/Kconfig      |  1 +
 drivers/clk/actions/Makefile     |  1 +
 drivers/clk/actions/owl-common.h |  2 +
 drivers/clk/actions/owl-reset.c  | 72 ++++++++++++++++++++++++++++++++
 drivers/clk/actions/owl-reset.h  | 32 ++++++++++++++
 5 files changed, 108 insertions(+)
 create mode 100644 drivers/clk/actions/owl-reset.c
 create mode 100644 drivers/clk/actions/owl-reset.h

diff --git a/drivers/clk/actions/Kconfig b/drivers/clk/actions/Kconfig
index dc38c85a4833..04f0a6355726 100644
--- a/drivers/clk/actions/Kconfig
+++ b/drivers/clk/actions/Kconfig

@@ -2,6 +2,7 @@ config CLK_ACTIONS
 	bool "Clock driver for Actions Semi SoCs"
 	depends on ARCH_ACTIONS || COMPILE_TEST
 	select REGMAP_MMIO
+	select RESET_CONTROLLER
 	default ARCH_ACTIONS
 
 if CLK_ACTIONS

diff --git a/drivers/clk/actions/Makefile b/drivers/clk/actions/Makefile
index 78c17d56f991..ccfdf9781cef 100644
--- a/drivers/clk/actions/Makefile
+++ b/drivers/clk/actions/Makefile

@@ -7,6 +7,7 @@ clk-owl-y			+= owl-divider.o
 clk-owl-y			+= owl-factor.o
 clk-owl-y			+= owl-composite.o
 clk-owl-y			+= owl-pll.o
+clk-owl-y			+= owl-reset.o
 
 # SoC support
 obj-$(CONFIG_CLK_OWL_S700)	+= owl-s700.o

diff --git a/drivers/clk/actions/owl-common.h b/drivers/clk/actions/owl-common.h
index 56f01f7774aa..4dc7f286831f 100644
--- a/drivers/clk/actions/owl-common.h
+++ b/drivers/clk/actions/owl-common.h

@@ -26,6 +26,8 @@ struct owl_clk_desc {
 	struct owl_clk_common		**clks;
 	unsigned long			num_clks;
 	struct clk_hw_onecell_data	*hw_clks;
+	struct owl_reset_map		*resets;

Could this be:
	const struct owl_reset_map	*resets;
?

Ack.

quoted

+	unsigned long			num_resets;
 	struct regmap			*regmap;
 };
 

diff --git a/drivers/clk/actions/owl-reset.c b/drivers/clk/actions/owl-reset.c
new file mode 100644
index 000000000000..91b161cc68de
--- /dev/null
+++ b/drivers/clk/actions/owl-reset.c

@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+//
+// Actions Semi Owl SoCs Reset Management Unit driver
+//
+// Copyright (c) 2018 Linaro Ltd.
+// Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+
+#include <linux/delay.h>
+#include <linux/io.h>

This seems unnecessary, since register access is done via regmap.

Will remove this header.

quoted

+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+#include "owl-reset.h"
+
+static int owl_reset_assert(struct reset_controller_dev *rcdev,
+			    unsigned long id)
+{
+	struct owl_reset *reset = to_owl_reset(rcdev);
+	const struct owl_reset_map *map = &reset->reset_map[id];
+	u32 reg;
+
+	regmap_read(reset->regmap, map->reg, &reg);
+	regmap_write(reset->regmap, map->reg, reg & ~map->bit);

This read-modify-write sequence needs locking against concurrent
register access. Better use regmap_update_bits():

	return regmap_update_bits(reset->regmap, map->reg, map->bit, 0);

Ack.

quoted

+
+	return 0;
+}
+
+static int owl_reset_deassert(struct reset_controller_dev *rcdev,
+			      unsigned long id)
+{
+	struct owl_reset *reset = to_owl_reset(rcdev);
+	const struct owl_reset_map *map = &reset->reset_map[id];
+	u32 reg;
+
+	regmap_read(reset->regmap, map->reg, &reg);
+	regmap_write(reset->regmap, map->reg, reg | map->bit);

Better:

	return regmap_update_bits(reset->regmap, map->reg, map->bit, map->bit);

Ack.

quoted

+
+	return 0;
+}
+
+static int owl_reset_reset(struct reset_controller_dev *rcdev,
+			   unsigned long id)
+{
+	owl_reset_assert(rcdev, id);
+	udelay(1);

Is the delay valid for all IP cores on all SoCs variants?

It is valid for S900 and S700 for now but I'm not sure about S500. Will
make sure while adding S500 support.

quoted

+	owl_reset_deassert(rcdev, id);
+
+	return 0;
+}
+
+static int owl_reset_status(struct reset_controller_dev *rcdev,
+			    unsigned long id)
+{
+	struct owl_reset *reset = to_owl_reset(rcdev);
+	const struct owl_reset_map *map = &reset->reset_map[id];
+	u32 reg;
+
+	regmap_read(reset->regmap, map->reg, &reg);

If this could return an error, better report it.

Ack.

quoted

+
+	/*
+	 * The reset control API expects 0 if reset is not asserted,
+	 * which is the opposite of what our hardware uses.
+	 */
+	return !(map->bit & reg);
+}
+
+const struct reset_control_ops owl_reset_ops = {
+	.assert		= owl_reset_assert,
+	.deassert	= owl_reset_deassert,
+	.reset		= owl_reset_reset,
+	.status		= owl_reset_status,
+};

diff --git a/drivers/clk/actions/owl-reset.h b/drivers/clk/actions/owl-reset.h
new file mode 100644
index 000000000000..1a5e987ba99b
--- /dev/null
+++ b/drivers/clk/actions/owl-reset.h

@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+//
+// Actions Semi Owl SoCs Reset Management Unit driver
+//
+// Copyright (c) 2018 Linaro Ltd.
+// Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
+
+#ifndef _OWL_RESET_H_
+#define _OWL_RESET_H_
+
+#include <linux/reset-controller.h>
+#include <linux/spinlock.h>

spinlock?

Oops. Not needed, will remove that.

quoted

+
+struct owl_reset_map {
+	u16	reg;

Note that this will be aligned to 32-bits. If the intention was to save
space, consider storing an u8 bit index instead of the mask.

Better to change it to u32, no need of memory saving here :)

quoted

+	u32	bit;
+};
+
+struct owl_reset {
+	struct reset_controller_dev	rcdev;
+	struct owl_reset_map		*reset_map;

Could this be
	const struct owl_reset_map	*reset_map;
?

Ack.

Thanks,
Mani

quoted

+	struct regmap			*regmap;
+};
+
+static inline struct owl_reset *to_owl_reset(struct reset_controller_dev *rcdev)
+{
+	return container_of(rcdev, struct owl_reset, rcdev);
+}
+
+extern const struct reset_control_ops owl_reset_ops;
+
+#endif /* _OWL_RESET_H_ */

regards
Philipp