[PATCH] clk: imx: Add some delay before deassert the reset

[PATCH] clk: imx: Add some delay before deassert the reset

[Jacky Bai]

Some of the PCCs on i.MX8ULP have a sw_rst bit to control
the peripheral reset through SW method. For peripherals like GPU
that need sync reset, some delay is necessary befere & after release
the reset to make sure the HW is reset into a known status. So add
some delay before & after release reset.

Signed-off-by: Jacky Bai <ping.bai@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/clk/imx/clk-composite-7ulp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clk/imx/clk-composite-7ulp.c b/drivers/clk/imx/clk-composite-7ulp.c
index 8ed2e0ad2769..710fe4f84465 100644
--- a/drivers/clk/imx/clk-composite-7ulp.c
+++ b/drivers/clk/imx/clk-composite-7ulp.c
@@ -7,6 +7,7 @@
 
 #include <linux/bits.h>
 #include <linux/clk-provider.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/slab.h>
@@ -36,6 +37,9 @@ static int pcc_gate_enable(struct clk_hw *hw)
 	if (ret)
 		return ret;
 
+	/* wait before release reset */
+	udelay(1);
+
 	spin_lock_irqsave(gate->lock, flags);
 	/*
 	 * release the sw reset for peripherals associated with
@@ -47,6 +51,9 @@ static int pcc_gate_enable(struct clk_hw *hw)
 
 	spin_unlock_irqrestore(gate->lock, flags);
 
+	/* wait sync reset done */
+	udelay(1);
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH] clk: imx: Add some delay before deassert the reset

[Frank Li]

On Tue, Jul 29, 2025 at 11:38:17AM +0800, Jacky Bai wrote:
> Some of the PCCs on i.MX8ULP have a sw_rst bit to control
> the peripheral reset through SW method. For peripherals like GPU
> that need sync reset, some delay is necessary befere & after release
> the reset to make sure the HW is reset into a known status. So add
> some delay before & after release reset.

Nit: wrap at 75 char.

>
> Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/clk/imx/clk-composite-7ulp.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/clk/imx/clk-composite-7ulp.c b/drivers/clk/imx/clk-composite-7ulp.c
> index 8ed2e0ad2769..710fe4f84465 100644
> --- a/drivers/clk/imx/clk-composite-7ulp.c
> +++ b/drivers/clk/imx/clk-composite-7ulp.c
> @@ -7,6 +7,7 @@
>
>  #include <linux/bits.h>
>  #include <linux/clk-provider.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> @@ -36,6 +37,9 @@ static int pcc_gate_enable(struct clk_hw *hw)
>  	if (ret)
>  		return ret;
>
> +	/* wait before release reset */
> +	udelay(1);
> +

It is quite small value. udelay()'s implement is not necessary to access
MMIO register space. (arm64 use cp15)

writel(val, gate->reg);
udelay(1); // it may less than 1us as what you expect, because previous
write have not reach target place yet.

It needs a readl() before udelay(1) to make sure value actually reach to
target place.

Frank

>  	spin_lock_irqsave(gate->lock, flags);
>  	/*
>  	 * release the sw reset for peripherals associated with
> @@ -47,6 +51,9 @@ static int pcc_gate_enable(struct clk_hw *hw)
>
>  	spin_unlock_irqrestore(gate->lock, flags);
>
> +	/* wait sync reset done */
> +	udelay(1);
> +
>  	return 0;
>  }
>
> --
> 2.34.1
>

RE: [PATCH] clk: imx: Add some delay before deassert the reset

[Jacky Bai]

> Subject: Re: [PATCH] clk: imx: Add some delay before deassert the reset
> 
> On Tue, Jul 29, 2025 at 11:38:17AM +0800, Jacky Bai wrote:
> > Some of the PCCs on i.MX8ULP have a sw_rst bit to control the
> > peripheral reset through SW method. For peripherals like GPU that need
> > sync reset, some delay is necessary befere & after release the reset
> > to make sure the HW is reset into a known status. So add some delay
> > before & after release reset.
> 
> Nit: wrap at 75 char.

Thx.

> 
> >
> > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >  drivers/clk/imx/clk-composite-7ulp.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/clk/imx/clk-composite-7ulp.c
> > b/drivers/clk/imx/clk-composite-7ulp.c
> > index 8ed2e0ad2769..710fe4f84465 100644
> > --- a/drivers/clk/imx/clk-composite-7ulp.c
> > +++ b/drivers/clk/imx/clk-composite-7ulp.c
> > @@ -7,6 +7,7 @@
> >
> >  #include <linux/bits.h>
> >  #include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> >  #include <linux/slab.h>
> > @@ -36,6 +37,9 @@ static int pcc_gate_enable(struct clk_hw *hw)
> >  	if (ret)
> >  		return ret;
> >
> > +	/* wait before release reset */
> > +	udelay(1);
> > +
> 
> It is quite small value. udelay()'s implement is not necessary to access MMIO
> register space. (arm64 use cp15)
> 

1us should be enough as the GPU IP reset propagation only need 128 cycle
of its own clock(> 200MHz). the first udelay is to add some margin to make
sure the GPU clock is stable.

> writel(val, gate->reg);
> udelay(1); // it may less than 1us as what you expect, because previous write
> have not reach target place yet.
> 
> It needs a readl() before udelay(1) to make sure value actually reach to target
> place.
> 

Sure, will add this.

BR
Jacky Bai

> Frank
> 
> >  	spin_lock_irqsave(gate->lock, flags);
> >  	/*
> >  	 * release the sw reset for peripherals associated with @@ -47,6
> > +51,9 @@ static int pcc_gate_enable(struct clk_hw *hw)
> >
> >  	spin_unlock_irqrestore(gate->lock, flags);
> >
> > +	/* wait sync reset done */
> > +	udelay(1);
> > +
> >  	return 0;
> >  }
> >
> > --
> > 2.34.1
> >

Re: [PATCH] clk: imx: Add some delay before deassert the reset

[Daniel Baluta]

On Thu, Jul 31, 2025 at 3:39 AM Jacky Bai <ping.bai@nxp.com> wrote:
>
> > Subject: Re: [PATCH] clk: imx: Add some delay before deassert the reset
> >
> > On Tue, Jul 29, 2025 at 11:38:17AM +0800, Jacky Bai wrote:
> > > Some of the PCCs on i.MX8ULP have a sw_rst bit to control the
> > > peripheral reset through SW method. For peripherals like GPU that need
> > > sync reset, some delay is necessary befere & after release the reset
> > > to make sure the HW is reset into a known status. So add some delay
> > > before & after release reset.
> >
> > Nit: wrap at 75 char.
>
> Thx.
>
> >
> > >
> > > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > >  drivers/clk/imx/clk-composite-7ulp.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/clk/imx/clk-composite-7ulp.c
> > > b/drivers/clk/imx/clk-composite-7ulp.c
> > > index 8ed2e0ad2769..710fe4f84465 100644
> > > --- a/drivers/clk/imx/clk-composite-7ulp.c
> > > +++ b/drivers/clk/imx/clk-composite-7ulp.c
> > > @@ -7,6 +7,7 @@
> > >
> > >  #include <linux/bits.h>
> > >  #include <linux/clk-provider.h>
> > > +#include <linux/delay.h>
> > >  #include <linux/err.h>
> > >  #include <linux/io.h>
> > >  #include <linux/slab.h>
> > > @@ -36,6 +37,9 @@ static int pcc_gate_enable(struct clk_hw *hw)
> > >     if (ret)
> > >             return ret;
> > >
> > > +   /* wait before release reset */
> > > +   udelay(1);
> > > +
> >
> > It is quite small value. udelay()'s implement is not necessary to access MMIO
> > register space. (arm64 use cp15)
> >
>
> 1us should be enough as the GPU IP reset propagation only need 128 cycle
> of its own clock(> 200MHz). the first udelay is to add some margin to make
> sure the GPU clock is stable.
>
> > writel(val, gate->reg);
> > udelay(1); // it may less than 1us as what you expect, because previous write
> > have not reach target place yet.
> >
> > It needs a readl() before udelay(1) to make sure value actually reach to target
> > place.

I think we should capture this explanation in a comment above the udelay.

/* wait before release reset */ doesn't really tell much. We should
explain *why*
a code change is needed. The *what* is obvious from the code.

thanks,
Daniel.

RE: [PATCH] clk: imx: Add some delay before deassert the reset

[Jacky Bai]

> Subject: Re: [PATCH] clk: imx: Add some delay before deassert the reset
> 
> [You don't often get email from daniel.baluta@gmail.com. Learn why this is
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Thu, Jul 31, 2025 at 3:39 AM Jacky Bai <ping.bai@nxp.com> wrote:
> >
> > > Subject: Re: [PATCH] clk: imx: Add some delay before deassert the
> > > reset
> > >
> > > On Tue, Jul 29, 2025 at 11:38:17AM +0800, Jacky Bai wrote:
> > > > Some of the PCCs on i.MX8ULP have a sw_rst bit to control the
> > > > peripheral reset through SW method. For peripherals like GPU that
> > > > need sync reset, some delay is necessary befere & after release
> > > > the reset to make sure the HW is reset into a known status. So add
> > > > some delay before & after release reset.
> > >
> > > Nit: wrap at 75 char.
> >
> > Thx.
> >
> > >
> > > >
> > > > Signed-off-by: Jacky Bai <ping.bai@nxp.com>
> > > > Reviewed-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > >  drivers/clk/imx/clk-composite-7ulp.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/clk/imx/clk-composite-7ulp.c
> > > > b/drivers/clk/imx/clk-composite-7ulp.c
> > > > index 8ed2e0ad2769..710fe4f84465 100644
> > > > --- a/drivers/clk/imx/clk-composite-7ulp.c
> > > > +++ b/drivers/clk/imx/clk-composite-7ulp.c
> > > > @@ -7,6 +7,7 @@
> > > >
> > > >  #include <linux/bits.h>
> > > >  #include <linux/clk-provider.h>
> > > > +#include <linux/delay.h>
> > > >  #include <linux/err.h>
> > > >  #include <linux/io.h>
> > > >  #include <linux/slab.h>
> > > > @@ -36,6 +37,9 @@ static int pcc_gate_enable(struct clk_hw *hw)
> > > >     if (ret)
> > > >             return ret;
> > > >
> > > > +   /* wait before release reset */
> > > > +   udelay(1);
> > > > +
> > >
> > > It is quite small value. udelay()'s implement is not necessary to
> > > access MMIO register space. (arm64 use cp15)
> > >
> >
> > 1us should be enough as the GPU IP reset propagation only need 128
> > cycle of its own clock(> 200MHz). the first udelay is to add some
> > margin to make sure the GPU clock is stable.
> >
> > > writel(val, gate->reg);
> > > udelay(1); // it may less than 1us as what you expect, because
> > > previous write have not reach target place yet.
> > >
> > > It needs a readl() before udelay(1) to make sure value actually
> > > reach to target place.
> 
> I think we should capture this explanation in a comment above the udelay.
> 
> /* wait before release reset */ doesn't really tell much. We should explain
> *why* a code change is needed. The *what* is obvious from the code.
> 

Thx, will add some more comments for the details.

BR  
> thanks,
> Daniel.