From: Andy Yan <andy.yan@rock-chips.com> To: Alexey Klimov <klimov.linux@gmail.com> Cc: "Heiko Stübner" <heiko@sntech.de>, linux-rockchip@lists.infradead.org, linux@arm.linux.org.uk, "Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] ARM: rockchip: add reboot notifier Date: Thu, 10 Sep 2015 18:48:33 +0800 [thread overview] Message-ID: <55F16001.3000100@rock-chips.com> (raw) In-Reply-To: <CALW4P+JpP1EEaa3_H4uFzFcTnBW8XPNa7shCNLDQ=z7=3ZmnKA@mail.gmail.com> Hi Alexey: On 2015年09月09日 05:50, Alexey Klimov wrote: > Hi Andy, > > On Tue, Sep 8, 2015 at 3:43 PM, Andy Yan <andy.yan@rock-chips.com> wrote: >> rockchip platform have a protocol to pass the the kernel > Double 'the'? this is will be removed. > >> reboot mode to bootloader by some special registers when >> system reboot. By this way the bootloader can take different >> action according to the different kernel reboot mode, for >> example, command "reboot loader" will reboot the board to >> rockusb mode, this is a very convenient way to get the board >> to download mode. >> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> --- >> >> arch/arm/mach-rockchip/Makefile | 2 +- >> arch/arm/mach-rockchip/loader.h | 22 +++++++++ >> arch/arm/mach-rockchip/reboot.c | 103 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 126 insertions(+), 1 deletion(-) >> create mode 100644 arch/arm/mach-rockchip/loader.h >> create mode 100644 arch/arm/mach-rockchip/reboot.c >> >> diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile >> index 5c3a9b2..cd291e3 100644 >> --- a/arch/arm/mach-rockchip/Makefile >> +++ b/arch/arm/mach-rockchip/Makefile >> @@ -1,5 +1,5 @@ >> CFLAGS_platsmp.o := -march=armv7-a >> >> -obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o >> +obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o reboot.o >> obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o >> obj-$(CONFIG_SMP) += headsmp.o platsmp.o >> diff --git a/arch/arm/mach-rockchip/loader.h b/arch/arm/mach-rockchip/loader.h >> new file mode 100644 >> index 0000000..bf51baa >> --- /dev/null >> +++ b/arch/arm/mach-rockchip/loader.h >> @@ -0,0 +1,22 @@ >> +#ifndef __MACH_ROCKCHIP_LOADER_H >> +#define __MACH_ROCKCHIP_LOADER_H >> + >> +/*high 24 bits is tag, low 8 bits is type*/ >> +#define SYS_LOADER_REBOOT_FLAG 0x5242C300 >> + >> +enum { >> + BOOT_NORMAL = 0, /* normal boot */ >> + BOOT_LOADER, /* enter loader rockusb mode */ >> + BOOT_MASKROM, /* enter maskrom rockusb mode (not support now) */ >> + BOOT_RECOVER, /* enter recover */ >> + BOOT_NORECOVER, /* do not enter recover */ >> + BOOT_SECONDOS, /* boot second OS (not support now)*/ >> + BOOT_WIPEDATA, /* enter recover and wipe data. */ >> + BOOT_WIPEALL, /* enter recover and wipe all data. */ >> + BOOT_CHECKIMG, /* check firmware img with backup part*/ >> + BOOT_FASTBOOT, /* enter fast boot mode */ >> + BOOT_SECUREBOOT_DISABLE, >> + BOOT_CHARGING, /* enter charge mode */ >> + BOOT_MAX /* MAX VALID BOOT TYPE.*/ > Looks like you only implemented NORMAL, RECOVER, LOADER and CHARGING. > Are you keeping other entries for keeping right order and keep > consistency? > Or have plans for future? to keep the right order,some of them maybe implemented in the future. > >> +}; >> +#endif >> diff --git a/arch/arm/mach-rockchip/reboot.c b/arch/arm/mach-rockchip/reboot.c >> new file mode 100644 >> index 0000000..704bc16 >> --- /dev/null >> +++ b/arch/arm/mach-rockchip/reboot.c >> @@ -0,0 +1,103 @@ >> +#include <linux/init.h> > Usually people place in the beginning copyright and GPL license header info. > >> +#include <linux/module.h> >> +#include <linux/kernel.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/reboot.h> >> +#include <linux/regmap.h> >> +#include <linux/mfd/syscon.h> >> +#include "loader.h" >> + >> +#define RK3188_PMU_SYS_REG0 0x40 >> +#define RK3288_PMU_SYS_REG0 0x94 >> + >> +struct regmap *regmap; >> +int flag_reg; >> + >> +static int rockchip_get_pmu_regmap(void) >> +{ >> + struct device_node *node; >> + >> + node = of_find_node_by_path("/cpus"); > Is it critical not to check node for NULL here? ok, I will add a check here > >> + regmap = syscon_regmap_lookup_by_phandle(node, "rockchip,pmu"); >> + of_node_put(node); >> + if (!IS_ERR(regmap)) >> + return 0; >> + >> + regmap = syscon_regmap_lookup_by_compatible("rockchip,rk3066-pmu"); >> + of_node_put(node); >> + if (!IS_ERR(regmap)) >> + return 0; >> + >> + return -ENODEV; >> +} > This double of_node_put(node) confuses me. Could you please guide me over it? > > After I tried to re-create it by myself looking to code I think that > second of_node_put() is not needed. the second of_node_put is not needed, it will be removed. >> +static int rockchip_get_reboot_flag_regmap(void) >> +{ >> + int ret = rockchip_get_pmu_regmap(); >> + >> + if (ret < 0) >> + return ret; >> + >> + if (of_machine_is_compatible("rockchip,rk3288")) { >> + flag_reg = RK3288_PMU_SYS_REG0; >> + return 0; >> + } else if (of_machine_is_compatible("rockchip,rk3066a") || >> + of_machine_is_compatible("rockchip,rk3066b") || >> + of_machine_is_compatible("rockchip,rk3188")) { >> + flag_reg = RK3188_PMU_SYS_REG0; >> + return 0; >> + } >> + >> + return -ENODEV; > [..] > >> + >> +static int __init rockchip_reboot_init(void) >> +{ >> + int ret = 0; >> + >> + if (!rockchip_get_reboot_flag_regmap()) { >> + ret = register_restart_handler(&rockchip_reboot_handler); >> + if (ret) >> + pr_err("%s: cannot register reboot handler, %d\n", >> + __func__, ret); >> + } >> + >> +return ret; > Please align this correctly. OK, this will be aligned next version > > Thanks, > Alexey Klimov > > > Thanks for your review.
WARNING: multiple messages have this Message-ID (diff)
From: andy.yan@rock-chips.com (Andy Yan) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] ARM: rockchip: add reboot notifier Date: Thu, 10 Sep 2015 18:48:33 +0800 [thread overview] Message-ID: <55F16001.3000100@rock-chips.com> (raw) In-Reply-To: <CALW4P+JpP1EEaa3_H4uFzFcTnBW8XPNa7shCNLDQ=z7=3ZmnKA@mail.gmail.com> Hi Alexey: On 2015?09?09? 05:50, Alexey Klimov wrote: > Hi Andy, > > On Tue, Sep 8, 2015 at 3:43 PM, Andy Yan <andy.yan@rock-chips.com> wrote: >> rockchip platform have a protocol to pass the the kernel > Double 'the'? this is will be removed. > >> reboot mode to bootloader by some special registers when >> system reboot. By this way the bootloader can take different >> action according to the different kernel reboot mode, for >> example, command "reboot loader" will reboot the board to >> rockusb mode, this is a very convenient way to get the board >> to download mode. >> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> --- >> >> arch/arm/mach-rockchip/Makefile | 2 +- >> arch/arm/mach-rockchip/loader.h | 22 +++++++++ >> arch/arm/mach-rockchip/reboot.c | 103 ++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 126 insertions(+), 1 deletion(-) >> create mode 100644 arch/arm/mach-rockchip/loader.h >> create mode 100644 arch/arm/mach-rockchip/reboot.c >> >> diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile >> index 5c3a9b2..cd291e3 100644 >> --- a/arch/arm/mach-rockchip/Makefile >> +++ b/arch/arm/mach-rockchip/Makefile >> @@ -1,5 +1,5 @@ >> CFLAGS_platsmp.o := -march=armv7-a >> >> -obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o >> +obj-$(CONFIG_ARCH_ROCKCHIP) += rockchip.o reboot.o >> obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o >> obj-$(CONFIG_SMP) += headsmp.o platsmp.o >> diff --git a/arch/arm/mach-rockchip/loader.h b/arch/arm/mach-rockchip/loader.h >> new file mode 100644 >> index 0000000..bf51baa >> --- /dev/null >> +++ b/arch/arm/mach-rockchip/loader.h >> @@ -0,0 +1,22 @@ >> +#ifndef __MACH_ROCKCHIP_LOADER_H >> +#define __MACH_ROCKCHIP_LOADER_H >> + >> +/*high 24 bits is tag, low 8 bits is type*/ >> +#define SYS_LOADER_REBOOT_FLAG 0x5242C300 >> + >> +enum { >> + BOOT_NORMAL = 0, /* normal boot */ >> + BOOT_LOADER, /* enter loader rockusb mode */ >> + BOOT_MASKROM, /* enter maskrom rockusb mode (not support now) */ >> + BOOT_RECOVER, /* enter recover */ >> + BOOT_NORECOVER, /* do not enter recover */ >> + BOOT_SECONDOS, /* boot second OS (not support now)*/ >> + BOOT_WIPEDATA, /* enter recover and wipe data. */ >> + BOOT_WIPEALL, /* enter recover and wipe all data. */ >> + BOOT_CHECKIMG, /* check firmware img with backup part*/ >> + BOOT_FASTBOOT, /* enter fast boot mode */ >> + BOOT_SECUREBOOT_DISABLE, >> + BOOT_CHARGING, /* enter charge mode */ >> + BOOT_MAX /* MAX VALID BOOT TYPE.*/ > Looks like you only implemented NORMAL, RECOVER, LOADER and CHARGING. > Are you keeping other entries for keeping right order and keep > consistency? > Or have plans for future? to keep the right order,some of them maybe implemented in the future. > >> +}; >> +#endif >> diff --git a/arch/arm/mach-rockchip/reboot.c b/arch/arm/mach-rockchip/reboot.c >> new file mode 100644 >> index 0000000..704bc16 >> --- /dev/null >> +++ b/arch/arm/mach-rockchip/reboot.c >> @@ -0,0 +1,103 @@ >> +#include <linux/init.h> > Usually people place in the beginning copyright and GPL license header info. > >> +#include <linux/module.h> >> +#include <linux/kernel.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/reboot.h> >> +#include <linux/regmap.h> >> +#include <linux/mfd/syscon.h> >> +#include "loader.h" >> + >> +#define RK3188_PMU_SYS_REG0 0x40 >> +#define RK3288_PMU_SYS_REG0 0x94 >> + >> +struct regmap *regmap; >> +int flag_reg; >> + >> +static int rockchip_get_pmu_regmap(void) >> +{ >> + struct device_node *node; >> + >> + node = of_find_node_by_path("/cpus"); > Is it critical not to check node for NULL here? ok, I will add a check here > >> + regmap = syscon_regmap_lookup_by_phandle(node, "rockchip,pmu"); >> + of_node_put(node); >> + if (!IS_ERR(regmap)) >> + return 0; >> + >> + regmap = syscon_regmap_lookup_by_compatible("rockchip,rk3066-pmu"); >> + of_node_put(node); >> + if (!IS_ERR(regmap)) >> + return 0; >> + >> + return -ENODEV; >> +} > This double of_node_put(node) confuses me. Could you please guide me over it? > > After I tried to re-create it by myself looking to code I think that > second of_node_put() is not needed. the second of_node_put is not needed, it will be removed. >> +static int rockchip_get_reboot_flag_regmap(void) >> +{ >> + int ret = rockchip_get_pmu_regmap(); >> + >> + if (ret < 0) >> + return ret; >> + >> + if (of_machine_is_compatible("rockchip,rk3288")) { >> + flag_reg = RK3288_PMU_SYS_REG0; >> + return 0; >> + } else if (of_machine_is_compatible("rockchip,rk3066a") || >> + of_machine_is_compatible("rockchip,rk3066b") || >> + of_machine_is_compatible("rockchip,rk3188")) { >> + flag_reg = RK3188_PMU_SYS_REG0; >> + return 0; >> + } >> + >> + return -ENODEV; > [..] > >> + >> +static int __init rockchip_reboot_init(void) >> +{ >> + int ret = 0; >> + >> + if (!rockchip_get_reboot_flag_regmap()) { >> + ret = register_restart_handler(&rockchip_reboot_handler); >> + if (ret) >> + pr_err("%s: cannot register reboot handler, %d\n", >> + __func__, ret); >> + } >> + >> +return ret; > Please align this correctly. OK, this will be aligned next version > > Thanks, > Alexey Klimov > > > Thanks for your review.
next prev parent reply other threads:[~2015-09-10 10:48 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-09-08 12:43 [PATCH] ARM: rockchip: add reboot notifier Andy Yan 2015-09-08 12:43 ` Andy Yan 2015-09-08 21:50 ` Alexey Klimov 2015-09-08 21:50 ` Alexey Klimov 2015-09-10 10:48 ` Andy Yan [this message] 2015-09-10 10:48 ` Andy Yan 2015-09-08 22:46 ` set rockchip-specific uboot bootmode flags on reboot (was: [PATCH] ARM: rockchip: add reboot notifier) Heiko Stübner 2015-09-08 22:46 ` Heiko Stübner 2015-09-09 1:08 ` set rockchip-specific uboot bootmode flags on reboot Andy Yan 2015-09-09 1:08 ` Andy Yan 2015-09-09 18:05 ` set rockchip-specific uboot bootmode flags on reboot (was: [PATCH] ARM: rockchip: add reboot notifier) Simon Glass 2015-09-09 18:05 ` Simon Glass 2015-09-09 18:05 ` Simon Glass 2015-09-17 11:07 ` set rockchip-specific uboot bootmode flags on reboot Andy Yan 2015-09-17 11:07 ` Andy Yan 2015-09-17 11:07 ` Andy Yan 2015-09-22 23:16 ` Heiko Stübner 2015-09-22 23:16 ` Heiko Stübner 2015-09-22 23:16 ` Heiko Stübner 2015-09-28 9:56 ` Andy Yan 2015-09-28 9:56 ` Andy Yan
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=55F16001.3000100@rock-chips.com \ --to=andy.yan@rock-chips.com \ --cc=heiko@sntech.de \ --cc=klimov.linux@gmail.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=linux@arm.linux.org.uk \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.