Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932272AbcCXIDn (ORCPT ); Thu, 24 Mar 2016 04:03:43 -0400 Received: from regular1.263xmail.com ([211.150.99.132]:38801 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751593AbcCXID3 (ORCPT ); Thu, 24 Mar 2016 04:03:29 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-MAE-MAIL_UID: 0056f39f450b55da@maes@620a2bbedb260491 X-MAE-ID: 223.6.254.61 X-RL-SENDER: andy.yan@rock-chips.com X-LOGIN-NAME: andy.yan@rock-chips.com X-SENDER-IP: 121.15.173.1 Subject: Re: [PATCH v6 2/4] power: reset: add reboot mode driver To: Krzysztof Kozlowski References: <1458646525-491-1-git-send-email-andy.yan@rock-chips.com> <1458646642-591-1-git-send-email-andy.yan@rock-chips.com> Cc: robh+dt@kernel.org, sre@kernel.org, heiko@sntech.de, john.stultz@linaro.org, arnd@arndb.de, galak@codeaurora.org, ijc+devicetree@hellion.org.uk, catalin.marinas@arm.com, olof@lixom.net, alexandre.belloni@free-electrons.com, dbaryshkov@gmail.com, jun.nie@linaro.org, pawel.moll@arm.com, will.deacon@arm.com, linux-rockchip@lists.infradead.org, matthias.bgg@gmail.com, devicetree@vger.kernel.org, linux-pm@vger.kernel.org, f.fainelli@gmail.com, linux@arm.linux.org.uk, mbrugger@suse.com, linux-arm-kernel@lists.infradead.org, lorenzo.pieralisi@arm.com, moritz.fischer@ettus.com, linux-kernel@vger.kernel.org, wxt@rock-chips.com, dwmw2@infradead.org, mark.rutland@arm.com From: Andy Yan Message-ID: <56F39F36.5050702@rock-chips.com> Date: Thu, 24 Mar 2016 16:03:02 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10812 Lines: 318 Hi Krzystof: On 2016年03月24日 10:50, Krzysztof Kozlowski wrote: > Cool work! Few comments below. > > > On Tue, Mar 22, 2016 at 8:37 PM, Andy Yan wrote: >> This driver parse the reboot commands like "reboot loader" >> and "reboot recovery" to get a boot mode described in the >> device tree , then call the write interfae to store the boot >> mode in some place like special register or sram , which can >> be read by the bootloader after system reboot, then the bootloader >> can take different action according to the mode stored. >> >> This is commonly used on Android based devices, in order to >> reboot the device into fastboot or recovery mode. >> >> Reviewed-by: Matthias Brugger >> Reviewed-by: Moritz Fischer >> Tested-by: John Stultz >> Acked-by: John Stultz >> Signed-off-by: Andy Yan >> >> --- >> >> Changes in v6: None >> Changes in v5: >> - use two blank space under help in Kconfig >> - use unsigned int instead of int for member magic in struct mode_info >> >> Changes in v4: >> - make this driver depends on OF to avoid kbuild test error >> >> Changes in v3: >> - scan multi properities >> - add mask value for some platform which only use some bits of the register >> to store boot mode magic value >> >> Changes in v2: >> - move to dir drivers/power/reset/ >> - make syscon-reboot-mode a generic driver >> >> Changes in v1: >> - fix the embarrassed compile warning >> - correct the maskrom magic number >> - check for the normal reboot >> >> drivers/power/reset/Kconfig | 13 ++++ >> drivers/power/reset/Makefile | 2 + >> drivers/power/reset/reboot-mode.c | 106 +++++++++++++++++++++++++++++++ >> drivers/power/reset/reboot-mode.h | 6 ++ >> drivers/power/reset/syscon-reboot-mode.c | 65 +++++++++++++++++++ >> 5 files changed, 192 insertions(+) >> create mode 100644 drivers/power/reset/reboot-mode.c >> create mode 100644 drivers/power/reset/reboot-mode.h >> create mode 100644 drivers/power/reset/syscon-reboot-mode.c >> >> diff --git a/drivers/power/reset/Kconfig b/drivers/power/reset/Kconfig >> index 1131cf7..cf50630 100644 >> --- a/drivers/power/reset/Kconfig >> +++ b/drivers/power/reset/Kconfig >> @@ -173,5 +173,18 @@ config POWER_RESET_ZX >> help >> Reboot support for ZTE SoCs. >> >> +config REBOOT_MODE >> + tristate >> + >> +config SYSCON_REBOOT_MODE >> + bool "Generic SYSCON regmap reboot mode driver" >> + depends on OF >> + select REBOOT_MODE >> + help >> + Say y here will enable reboot mode driver. This will >> + get reboot mode arguments and store it in SYSCON mapped >> + register, then the bootloader can read it to take different >> + action according to the mode. >> + >> endif >> >> diff --git a/drivers/power/reset/Makefile b/drivers/power/reset/Makefile >> index 096fa67..a63865b 100644 >> --- a/drivers/power/reset/Makefile >> +++ b/drivers/power/reset/Makefile >> @@ -20,3 +20,5 @@ obj-$(CONFIG_POWER_RESET_SYSCON) += syscon-reboot.o >> obj-$(CONFIG_POWER_RESET_SYSCON_POWEROFF) += syscon-poweroff.o >> obj-$(CONFIG_POWER_RESET_RMOBILE) += rmobile-reset.o >> obj-$(CONFIG_POWER_RESET_ZX) += zx-reboot.o >> +obj-$(CONFIG_REBOOT_MODE) += reboot-mode.o >> +obj-$(CONFIG_SYSCON_REBOOT_MODE) += syscon-reboot-mode.o >> diff --git a/drivers/power/reset/reboot-mode.c b/drivers/power/reset/reboot-mode.c >> new file mode 100644 >> index 0000000..9aa7b80 >> --- /dev/null >> +++ b/drivers/power/reset/reboot-mode.c >> @@ -0,0 +1,106 @@ >> +/* >> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include "reboot-mode.h" >> + >> +#define PREFIX "mode-" >> + >> +struct mode_info { >> + char mode[32]; >> + unsigned int magic; >> + struct list_head list; >> +}; >> + >> +struct reboot_mode_driver { >> + struct list_head head; >> + int (*write)(int magic); >> + struct notifier_block reboot_notifier; >> +}; >> + >> +static int get_reboot_mode_magic(struct reboot_mode_driver *reboot, >> + const char *cmd) >> +{ >> + const char *normal = "normal"; >> + int magic = 0; >> + struct mode_info *info; >> + >> + if (!cmd) >> + cmd = normal; >> + >> + list_for_each_entry(info, &reboot->head, list) { >> + if (!strcmp(info->mode, cmd)) { >> + magic = info->magic; >> + break; >> + } >> + } >> + >> + return magic; > In absence of 'normal' mode (it is not described as required property) > the magic will be '0'. It would be nice to document that in bindings. > Imagine someone forgets about this and will wonder why 0x0 is written > to his precious register on normal reboot... If the magic value is '0', we won't touch the register, please see reboot_mode_notify bellow. > It would be nice to document that 'mode-normal' has a special > (hard-coded) meaning. > >> +} >> + >> +static int reboot_mode_notify(struct notifier_block *this, >> + unsigned long mode, void *cmd) >> +{ >> + struct reboot_mode_driver *reboot; >> + int magic; >> + >> + reboot = container_of(this, struct reboot_mode_driver, reboot_notifier); >> + magic = get_reboot_mode_magic(reboot, cmd); >> + if (magic) >> + reboot->write(magic); >> + >> + return NOTIFY_DONE; >> +} >> + >> +int reboot_mode_register(struct device *dev, int (*write)(int)) >> +{ >> + struct reboot_mode_driver *reboot; >> + struct mode_info *info; >> + struct property *prop; >> + size_t len = strlen(PREFIX); >> + int ret; >> + >> + reboot = devm_kzalloc(dev, sizeof(*reboot), GFP_KERNEL); >> + if (!reboot) >> + return -ENOMEM; >> + >> + reboot->write = write; >> + INIT_LIST_HEAD(&reboot->head); >> + for_each_property_of_node(dev->of_node, prop) { >> + if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len)) >> + continue; > New line please for readability. Okay > >> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); >> + if (!info) >> + return -ENOMEM; > Ditto. Okay > >> + strcpy(info->mode, prop->name + len); > Ehm, and how do you protect that name of mode is shorter than 32 characters? How about info->mode = prop->name + len ? > >> + if (of_property_read_u32(dev->of_node, prop->name, &info->magic)) { >> + dev_err(dev, "reboot mode %s without magic number\n", >> + info->mode); >> + devm_kfree(dev, info); >> + continue; >> + } >> + list_add_tail(&info->list, &reboot->head); >> + } >> + reboot->reboot_notifier.notifier_call = reboot_mode_notify; >> + ret = register_reboot_notifier(&reboot->reboot_notifier); >> + if (ret) >> + dev_err(dev, "can't register reboot notifier\n"); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(reboot_mode_register); >> + >> +MODULE_AUTHOR("Andy Yan > +MODULE_DESCRIPTION("System reboot mode driver"); >> +MODULE_LICENSE("GPL v2"); >> diff --git a/drivers/power/reset/reboot-mode.h b/drivers/power/reset/reboot-mode.h >> new file mode 100644 >> index 0000000..44ed34f >> --- /dev/null >> +++ b/drivers/power/reset/reboot-mode.h >> @@ -0,0 +1,6 @@ >> +#ifndef __REBOOT_MODE_H__ >> +#define __REBOOT_MODE_H__ >> + >> +int reboot_mode_register(struct device *dev, int (*write)(int)); > Documentation would be appreciated. Although it is local header but > you decoupled them and you are exporting the function. I think I should remove the EXPORT_SYMBOL_GPL. > >> + >> +#endif >> diff --git a/drivers/power/reset/syscon-reboot-mode.c b/drivers/power/reset/syscon-reboot-mode.c >> new file mode 100644 >> index 0000000..d1521d8 >> --- /dev/null >> +++ b/drivers/power/reset/syscon-reboot-mode.c >> @@ -0,0 +1,65 @@ >> +/* >> + * Copyright (c) 2016, Fuzhou Rockchip Electronics Co., Ltd >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include > I don't see the need of of_address or am I missing something? Yes, it useless, I will remove it. > >> +#include >> +#include >> +#include >> +#include >> +#include "reboot-mode.h" >> + >> +static struct regmap *map; >> +static u32 offset; >> +static u32 mask = 0xffffffff; > Why this cannot be part of state container? I suppose usually there > will be only one reboot mode handler, but: > 1. It is always nice to make things self-contained, so code could be > easily used in different contexts. Depending on static variables > prevents that. > 2. How do you protect against double binding (two reboot mode device > nodes in DT)? The second binding (put by mistake... or whatever > reason) will overwrite data used by previous device. Okay, I will make this self-contained. >> + >> +static int syscon_reboot_mode_write(int magic) >> +{ >> + regmap_update_bits(map, offset, mask, magic); >> + >> + return 0; > Why ignoring return value of regmap_update_bits? Okay, I will check it. > >> +} >> + >> +static int syscon_reboot_mode_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + >> + map = syscon_node_to_regmap(pdev->dev.parent->of_node); >> + if (IS_ERR(map)) >> + return PTR_ERR(map); > Empty line for readability. ok >> + if (of_property_read_u32(pdev->dev.of_node, "offset", &offset)) >> + return -EINVAL; > Empty line for readability. ok > > Best regards, > Krzysztof > > > >