> 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:
> > 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@xxxxxxx>
> > Signed-off-by: Jacky Bai <ping.bai@xxxxxxx>
> > ---
> > 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
> ++++++++++++++++++++++++++-
> > 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?
> [...]
> > 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@xxxxxxxxxxxxxx>
>
> regards
> Philipp
