Subject: Re: [PATCH v2 9/9] clk: imx: Add the pcc reset controller support on
imx8ulp

Hi Jacky,

On Tue, 2021-08-10 at 14:28 +0800, Jacky Bai wrote:

quoted

On i.MX8ULP, for some of the PCCs, it has a peripheral SW RST bit
resides in the same registers as the clock controller. So add this SW
RST controller support alongs with the pcc clock initialization.

the reset and clock shared the same register, to avoid  accessing the
same register by reset control and clock control concurrently, locking
is necessary, so reuse the imx_ccm_lock spinlock to simplify the code.

Suggested-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Jacky Bai <ping.bai@nxp.com>
---
  v2 changes:
    - add 'Suggested-by' as suggested by Victor Liu
---
 drivers/clk/imx/Kconfig              |   1 +
 drivers/clk/imx/clk-composite-7ulp.c |  10 +++
 drivers/clk/imx/clk-imx8ulp.c        | 115

++++++++++++++++++++++++++-

quoted

 3 files changed, 123 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index

b81d6437ed95..0d1e3a6ac32a 100644

--- a/drivers/clk/imx/Kconfig
+++ b/drivers/clk/imx/Kconfig

@@ -102,5 +102,6 @@ config CLK_IMX8QXP  config CLK_IMX8ULP
 	tristate "IMX8ULP CCM Clock Driver"
 	depends on ARCH_MXC || COMPILE_TEST
+	select RESET_CONTROLLER

This shouldn't be required anymore, devm_reset_controller_register() has a
stub since commit 48a74b1147f7 ("reset: Add compile-test stubs").

So we don't need to select 'RESET_CONTROLLER' explicitly, right?

[...]

quoted

diff --git a/drivers/clk/imx/clk-imx8ulp.c

b/drivers/clk/imx/clk-imx8ulp.c index 6aad04114658..ea596cd6855a
100644

--- a/drivers/clk/imx/clk-imx8ulp.c
+++ b/drivers/clk/imx/clk-imx8ulp.c

@@ -9,6 +9,7 @@
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
+#include <linux/reset-controller.h>
 #include <linux/slab.h>

 #include "clk.h"

@@ -48,6 +49,98 @@ static const char * const nic_per_divplat[] = {

"nic_per_divplat" };  static const char * const lpav_axi_div[] = {
"lpav_axi_div" };  static const char * const lpav_bus_div[] = {
"lpav_bus_div" };

+struct pcc_reset_dev {
+	void __iomem *base;
+	struct reset_controller_dev rcdev;
+	const u32 *resets;
+	spinlock_t *lock;

I'd add a comment to this lock, stating that it is set to imx_ccm_lock and
protects access to registers shared with clock control.

Ok, will add some comments, thx.

BR
Jacky Bai

With these addressed,

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp