[U-Boot,1/2] rockchip: clk: rv1108: remove duplicate reset init
diff mbox series

Message ID 20191023174515.8892-1-heiko@sntech.de
State Superseded
Delegated to: Kever Yang
Headers show
Series
  • [U-Boot,1/2] rockchip: clk: rv1108: remove duplicate reset init
Related show

Commit Message

Heiko Stuebner Oct. 23, 2019, 5:45 p.m. UTC
rockchip_reset_bind() already does the needed init for the reset
registers, only referenced the wrong cru structure.

So we can get rid of the open-coded reset init and just fix
the correct cru reference.

Signed-off-by: Heiko Stuebner <heiko@sntech.de>
---
 drivers/clk/rockchip/clk_rv1108.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

Comments

Kever Yang Nov. 7, 2019, 9:03 a.m. UTC | #1
Heiko,

On 2019/10/24 上午1:45, Heiko Stuebner wrote:
> rockchip_reset_bind() already does the needed init for the reset
> registers, only referenced the wrong cru structure.
>
> So we can get rid of the open-coded reset init and just fix
> the correct cru reference.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
>   drivers/clk/rockchip/clk_rv1108.c | 13 +------------
>   1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/drivers/clk/rockchip/clk_rv1108.c b/drivers/clk/rockchip/clk_rv1108.c
> index 3ebb007fab..5dc31e1eb0 100644
> --- a/drivers/clk/rockchip/clk_rv1108.c
> +++ b/drivers/clk/rockchip/clk_rv1108.c
> @@ -698,22 +698,11 @@ static int rv1108_clk_bind(struct udevice *dev)
>   	}
>   
>   #if CONFIG_IS_ENABLED(CONFIG_RESET_ROCKCHIP)
> -	ret = offsetof(struct rk3368_cru, softrst_con[0]);
> +	ret = offsetof(struct rv1108_cru, softrst_con[0]);
>   	ret = rockchip_reset_bind(dev, ret, 13);
>   	if (ret)
>   		debug("Warning: software reset driver bind faile\n");
>   #endif
> -	ret = device_bind_driver_to_node(dev, "rockchip_reset", "reset",
> -					 dev_ofnode(dev), &sf_child);

You can't just remove this blob of code, for there is a 'sysreset' 
driver and a 'reset' driver, they are

different, so you should fix this part to be available 'sysreset' driver 
so that the software can reset the SoC.


Thanks,

- Kever

> -	if (ret) {
> -		debug("Warning: No rockchip reset driver: ret=%d\n", ret);
> -	} else {
> -		sf_priv = malloc(sizeof(struct softreset_reg));
> -		sf_priv->sf_reset_offset = offsetof(struct rv1108_cru,
> -						    softrst_con[0]);
> -		sf_priv->sf_reset_num = 13;
> -		sf_child->priv = sf_priv;
> -	}
>   
>   	return 0;
>   }
Heiko Stuebner Nov. 7, 2019, 9:13 a.m. UTC | #2
Hi Kever,

Am Donnerstag, 7. November 2019, 10:03:19 CET schrieb Kever Yang:
> On 2019/10/24 上午1:45, Heiko Stuebner wrote:
> > rockchip_reset_bind() already does the needed init for the reset
> > registers, only referenced the wrong cru structure.
> >
> > So we can get rid of the open-coded reset init and just fix
> > the correct cru reference.
> >
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >   drivers/clk/rockchip/clk_rv1108.c | 13 +------------
> >   1 file changed, 1 insertion(+), 12 deletions(-)
> >
> > diff --git a/drivers/clk/rockchip/clk_rv1108.c b/drivers/clk/rockchip/clk_rv1108.c
> > index 3ebb007fab..5dc31e1eb0 100644
> > --- a/drivers/clk/rockchip/clk_rv1108.c
> > +++ b/drivers/clk/rockchip/clk_rv1108.c
> > @@ -698,22 +698,11 @@ static int rv1108_clk_bind(struct udevice *dev)
> >   	}
> >   
> >   #if CONFIG_IS_ENABLED(CONFIG_RESET_ROCKCHIP)
> > -	ret = offsetof(struct rk3368_cru, softrst_con[0]);
> > +	ret = offsetof(struct rv1108_cru, softrst_con[0]);
> >   	ret = rockchip_reset_bind(dev, ret, 13);
> >   	if (ret)
> >   		debug("Warning: software reset driver bind faile\n");
> >   #endif
> > -	ret = device_bind_driver_to_node(dev, "rockchip_reset", "reset",
> > -					 dev_ofnode(dev), &sf_child);
> 
> You can't just remove this blob of code, for there is a 'sysreset' 
> driver and a 'reset' driver, they are
> 
> different, so you should fix this part to be available 'sysreset' driver 
> so that the software can reset the SoC.

While there is a sysreset driver, it seems the rv1108 only did init
the softrst code here and not the sysreset at all.

So rockchip_reset_bind really only does the same as the remove
part of code. What I did see was that struct softreset_reg
still is defined in the rockchip clock.h and after this patch no-one
is using at all anymore. The rockchip_reset driver uses a different
struct altogether it seems.

Adding an appropriate sysreset driver for rv1108
should likely be a separate patch though.

Heiko


> > -	if (ret) {
> > -		debug("Warning: No rockchip reset driver: ret=%d\n", ret);
> > -	} else {
> > -		sf_priv = malloc(sizeof(struct softreset_reg));
> > -		sf_priv->sf_reset_offset = offsetof(struct rv1108_cru,
> > -						    softrst_con[0]);
> > -		sf_priv->sf_reset_num = 13;
> > -		sf_child->priv = sf_priv;
> > -	}
> >   
> >   	return 0;
> >   }
> 
> 
>
Heiko Stuebner Nov. 8, 2019, 11:11 p.m. UTC | #3
Hi Kever,

Am Donnerstag, 7. November 2019, 10:13:02 CET schrieb Heiko Stübner:
> Am Donnerstag, 7. November 2019, 10:03:19 CET schrieb Kever Yang:
> > On 2019/10/24 上午1:45, Heiko Stuebner wrote:
> > > rockchip_reset_bind() already does the needed init for the reset
> > > registers, only referenced the wrong cru structure.
> > >
> > > So we can get rid of the open-coded reset init and just fix
> > > the correct cru reference.
> > >
> > > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > > ---
> > >   drivers/clk/rockchip/clk_rv1108.c | 13 +------------
> > >   1 file changed, 1 insertion(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/clk/rockchip/clk_rv1108.c b/drivers/clk/rockchip/clk_rv1108.c
> > > index 3ebb007fab..5dc31e1eb0 100644
> > > --- a/drivers/clk/rockchip/clk_rv1108.c
> > > +++ b/drivers/clk/rockchip/clk_rv1108.c
> > > @@ -698,22 +698,11 @@ static int rv1108_clk_bind(struct udevice *dev)
> > >   	}
> > >   
> > >   #if CONFIG_IS_ENABLED(CONFIG_RESET_ROCKCHIP)
> > > -	ret = offsetof(struct rk3368_cru, softrst_con[0]);
> > > +	ret = offsetof(struct rv1108_cru, softrst_con[0]);
> > >   	ret = rockchip_reset_bind(dev, ret, 13);
> > >   	if (ret)
> > >   		debug("Warning: software reset driver bind faile\n");
> > >   #endif
> > > -	ret = device_bind_driver_to_node(dev, "rockchip_reset", "reset",
> > > -					 dev_ofnode(dev), &sf_child);
> > 
> > You can't just remove this blob of code, for there is a 'sysreset' 
> > driver and a 'reset' driver, they are
> > 
> > different, so you should fix this part to be available 'sysreset' driver 
> > so that the software can reset the SoC.
> 
> While there is a sysreset driver, it seems the rv1108 only did init
> the softrst code here and not the sysreset at all.
> 
> So rockchip_reset_bind really only does the same as the remove
> part of code. What I did see was that struct softreset_reg
> still is defined in the rockchip clock.h and after this patch no-one
> is using at all anymore. The rockchip_reset driver uses a different
> struct altogether it seems.
> 
> Adding an appropriate sysreset driver for rv1108
> should likely be a separate patch though.

I just sent a v2, dropping the unused softreset_reg struct as well.

When reviewing please take into account, the rv1108 had the following
situation before:

-registration of sysreset
- registration of old softreset - broken because that driver code is gone
- registration of new softreset - broken because of the CONFIG_* from patch2
			and also because it was using the wrong struct (rk3368 instead of rv1108)

now after the patch (in v2), we have the expected
- registration of sysreset
- registration of new softreset with correct cru struct

Heiko

Patch
diff mbox series

diff --git a/drivers/clk/rockchip/clk_rv1108.c b/drivers/clk/rockchip/clk_rv1108.c
index 3ebb007fab..5dc31e1eb0 100644
--- a/drivers/clk/rockchip/clk_rv1108.c
+++ b/drivers/clk/rockchip/clk_rv1108.c
@@ -698,22 +698,11 @@  static int rv1108_clk_bind(struct udevice *dev)
 	}
 
 #if CONFIG_IS_ENABLED(CONFIG_RESET_ROCKCHIP)
-	ret = offsetof(struct rk3368_cru, softrst_con[0]);
+	ret = offsetof(struct rv1108_cru, softrst_con[0]);
 	ret = rockchip_reset_bind(dev, ret, 13);
 	if (ret)
 		debug("Warning: software reset driver bind faile\n");
 #endif
-	ret = device_bind_driver_to_node(dev, "rockchip_reset", "reset",
-					 dev_ofnode(dev), &sf_child);
-	if (ret) {
-		debug("Warning: No rockchip reset driver: ret=%d\n", ret);
-	} else {
-		sf_priv = malloc(sizeof(struct softreset_reg));
-		sf_priv->sf_reset_offset = offsetof(struct rv1108_cru,
-						    softrst_con[0]);
-		sf_priv->sf_reset_num = 13;
-		sf_child->priv = sf_priv;
-	}
 
 	return 0;
 }