| Message ID | 20240516090031.2373-2-anand@edgeble.ai |
|---|---|
| State | New |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | [v1,1/9] rockchip: RK3328: Read the reset cause from clock reset unit for RK3328 SoC | expand |
Hi Anand, On 5/16/24 10:59 AM, Anand Moon wrote: > Read the reset cause from clock reset unit for RK3328 SoC. > > Cc: Jagan Teki <jagan@edgeble.ai> > Signed-off-by: Anand Moon <anand@edgeble.ai> > --- > arch/arm/mach-rockchip/cpu-info.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-rockchip/cpu-info.c b/arch/arm/mach-rockchip/cpu-info.c > index 14c7331e1a..fce4bd7541 100644 > --- a/arch/arm/mach-rockchip/cpu-info.c > +++ b/arch/arm/mach-rockchip/cpu-info.c > @@ -8,12 +8,17 @@ > #include <init.h> > #include <asm/arch-rockchip/clock.h> > #include <asm/arch-rockchip/cru.h> > +#if IS_ENABLED(CONFIG_ROCKCHIP_RK3328) > +#include <asm/arch-rockchip/cru_rk3328.h> > +#endif > #include <asm/arch-rockchip/hardware.h> > #include <linux/err.h> > > char *get_reset_cause(void) > { > - struct rockchip_cru *cru = rockchip_get_cru(); > +#if IS_ENABLED(CONFIG_ROCKCHIP_RK3328) > + struct rk3328_cru *cru = rockchip_get_cru(); > +#endif NACK. If I only apply this patch, it breaks support for rk3288 and rk3399 (the ones which have a rockchip_cru struct). Do: """ #if IS_ENABLED(CONFIG_ROCKCHIP_RK3328) struct rk3328_cru *cru = rockchip_get_cru(); #else struct rockchip_cru *cru = rockchip_get_cru(); #endif """ instead to keep the current support and add stuff. However, I very much don't like this. There's no need for cpuinfo.c to become SoC-specific. I would recommend to move get_reset_cause() in some file that is SoC specific, and out of the DISPLAY_CPUINFO ifdef since it may be useful outside of this very specific print_cpuinfo function (c.f. the rk3399 firefly board). Yes there's a bit of code duplication then, BUT, nothing guarantees us the reset reason will always be stored in that glb_rst_st register and moreover, RK3588 already has more reasons that currently supported, and this prevents us from expanding it. You can have a """ __weak char *get_reset_cause(void) { return "could not get reset cause"; } """ in replacement and let SoC-specific files expand it by overriding it. And.... to think even further than that, maybe we should actually expand drivers/sysreset/sysreset_rockchip.c to support more SoCs. Then we could get the status via get_status callback from sysreset_ops in https://elixir.bootlin.com/u-boot/latest/source/common/board_f.c#L904 and remove this cpu-info.c file from the arch/arm/mach-rockchip. Reporting the SoC could be done with a CPU UCLASS (c.f. what's done for IMX8 already), from https://elixir.bootlin.com/u-boot/latest/source/common/board_f.c#L176. This honestly makes more sense to me. Cheers, Quentin
diff --git a/arch/arm/mach-rockchip/cpu-info.c b/arch/arm/mach-rockchip/cpu-info.c index 14c7331e1a..fce4bd7541 100644 --- a/arch/arm/mach-rockchip/cpu-info.c +++ b/arch/arm/mach-rockchip/cpu-info.c @@ -8,12 +8,17 @@ #include <init.h> #include <asm/arch-rockchip/clock.h> #include <asm/arch-rockchip/cru.h> +#if IS_ENABLED(CONFIG_ROCKCHIP_RK3328) +#include <asm/arch-rockchip/cru_rk3328.h> +#endif #include <asm/arch-rockchip/hardware.h> #include <linux/err.h> char *get_reset_cause(void) { - struct rockchip_cru *cru = rockchip_get_cru(); +#if IS_ENABLED(CONFIG_ROCKCHIP_RK3328) + struct rk3328_cru *cru = rockchip_get_cru(); +#endif char *cause = NULL; if (IS_ERR(cru))
Read the reset cause from clock reset unit for RK3328 SoC. Cc: Jagan Teki <jagan@edgeble.ai> Signed-off-by: Anand Moon <anand@edgeble.ai> --- arch/arm/mach-rockchip/cpu-info.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)