From: John Stultz <john.stultz@linaro.org>
To: Rob Herring <robh@kernel.org>
Cc: "Andy Yan" <andy.yan@rock-chips.com>,
"Arnd Bergmann" <arnd@arndb.de>,
"Moritz Fischer" <moritz.fischer@ettus.com>,
"Matthias Brugger" <matthias.bgg@gmail.com>,
"Kumar Gala" <galak@codeaurora.org>,
"Ian Campbell" <ijc+devicetree@hellion.org.uk>,
"Catalin Marinas" <catalin.marinas@arm.com>,
"Heiko Stübner" <heiko@sntech.de>,
"Sebastian Reichel" <sre@kernel.org>,
"Dmitry Eremin-Solenikov" <dbaryshkov@gmail.com>,
"Alexandre Belloni" <alexandre.belloni@free-electrons.com>,
"Jun Nie" <jun.nie@linaro.org>, "Paweł Moll" <pawel.moll@arm.com>,
"Will Deacon" <will.deacon@arm.com>,
"open list:ARM/Rockchip SoC..."
<linux-rockchip@lists.infradead.org>,
"Caesar Wang" <wxt@rock-chips.com>,
devicetree@vger.kernel.org,
"Linux PM list" <linux-pm@vger.kernel.org>,
"Russell King - ARM Linux" <linux@arm.linux.org.uk>,
mbrugger@suse.com,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
lkml <linux-kernel@vger.kernel.org>,
"Richard Weinberger" <richard@nod.at>,
"David Woodhouse" <dwmw2@infradead.org>,
"Mark Rutland" <mark.rutland@arm.com>
Subject: Re: [PATCH v3 1/4] dt-bindings: power: reset: add document for reboot-mode driver
Date: Thu, 4 Feb 2016 15:46:15 -0800 [thread overview]
Message-ID: <CALAqxLVyznpLd2MLgGr28TDECpLZ=RpmhxH-SHqntXfBsctcmQ@mail.gmail.com> (raw)
In-Reply-To: <20160204230814.GA4847@rob-hp-laptop>
On Thu, Feb 4, 2016 at 3:08 PM, Rob Herring <robh@kernel.org> wrote:
> On Tue, Feb 02, 2016 at 05:59:11PM +0800, Andy Yan wrote:
>> add device tree bindings document for reboot-mode driver
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> Changes in v3:
>> - descirbe all reboot mode as properity instead of subnode
>>
>> Changes in v2: None
>> Changes in v1: None
>>
>> .../bindings/power/reset/reboot-mode.txt | 26 ++++++++++++++++
>> .../bindings/power/reset/syscon-reboot-mode.txt | 36 ++++++++++++++++++++++
>> 2 files changed, 62 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt
>>
>> diff --git a/Documentation/devicetree/bindings/power/reset/reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> new file mode 100644
>> index 0000000..517080f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> @@ -0,0 +1,26 @@
>> +Generic reboot mode core map driver
>> +
>> +This driver get reboot mode arguments and call the write
>> +interface to stores the magic value in special register
>> +or ram . Then the bootloader can read it and take different
>> +action according the argument stored.
>> +
>> +All mode properties are vendor specific, it is a indication to tell
>
> The values should be vendor specific. The property names should not. We
> can allow vendor specific ones, but we need to have a common set.
>
>> +the bootloder what to do when the system reboot, and should be named
>> +as mode-xxx = <magic> (xxx is mode name).
>> +
>> +- mode-normal: Normal reboot mode, system reboot with command "reboot".
>> +- mode-recovery: Android Recovery mode, it is a mode to format the device or update a new image.
>> +- mode-fastboot: Android fastboot mode, it's a mode to re-flash partitions on the device.
>> +- mode-loader: A bootloader mode, it's a mode used to download image on Rockchip platform,
>> + usually used in development.
>> +- mode-maskrom: It's a mode to download bootloader on Rockchip platform.
>> +
>> +Example:
>> + reboot-mode {
>> + mode-normal = <BOOT_NORMAL>;
>> + mode-recovery = <BOOT_RECOVERY>;
>> + mode-fastboot = <BOOT_FASTBOOT>;
>
> I tend to agree with John on calling this mode-bootloader.
>
> OTOH, fastboot is more specific about what the mode is. The name in DT
> and the userspace name don't necessarily have to be the same.
Wait. This is a bit confusing. The utility of adding a property name
and using that name be the reboot command parsed for made sense
(compared to earlier versions which had command strings) as it made
the dts more terse. But it sounds here like you're suggesting we
should have some logic in the driver that translates "reboot fastboot"
to mode-bootloader or vice versa.
>> + mode-loader = <BOOT_LOADER>;
>
> This one needs a better name. Maybe it should be 'rockchip,mode-loader'
> as it is vendor specific. Either way, loader is vague. Perhaps
> rockchip,mode-bl-download?
Hrm. So how what reboot command do you expect to trigger that?
Though I do like the vendor specific prefix here, as it clarifies how
universal the command is (and its easy add standard commands if they
become more established and remap things in the future).
>
>> + mode-maskrom = <BOOT_MASKROM>;
>
> I think this should be "mode-rom-download".
I think one of the difficult things here is that there's no real
standards for all bios/bootloader modes. So they are somewhat
firmware/bootloader/device specific, and thus we need something that
is flexible enough to allow lots of different modes to be easily
specified. That said, this does expose a userspace interface (though
one could argue kernel ABI doesn't cross reboots :) so we should try
to have some consistency so the same userspace can work on various
devices.
I do think the "bootloader" and "recovery" arguments are somewhat
defacto standards, well established on most android devices.
I think here the concern is rockchip probably has some userspace that
is already using "reboot maskrom" or "reboot loader" for their own
uses. And its a bit of a pain to ask that userspace to be reworked to
use "reboot rom-download" or "reboot rockchip,rom-download" depending
on how we try to deal with these. (Granted, non-upstream interfaces
aren't official, so that is their risk somewhat, but we avoid being
smug about that :)
Another part of the issue is there isn't really a way to probe for
reboot cmd capability here. As much as I'd rather not complicate
things, one couldn't easily extend existing userspace to work with
current kernels as well as future kernels, since the reboot with an
invalid command won't fail. The machine still resets. So you can't try
one and fallback to the other.
Maybe there needs to be a sysfs entry with the list of the supported commands?
thanks
-john
next prev parent reply other threads:[~2016-02-04 23:53 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-02 9:56 [PATCH v3 0/4] add reboot mode driver Andy Yan
2016-02-02 9:59 ` [PATCH v3 1/4] dt-bindings: power: reset: add document for reboot-mode driver Andy Yan
2016-02-02 18:29 ` John Stultz
2016-02-04 23:08 ` Rob Herring
2016-02-04 23:46 ` John Stultz [this message]
2016-02-05 4:35 ` Rob Herring
2016-02-05 5:03 ` John Stultz
2016-02-11 17:04 ` Rob Herring
2016-02-15 9:43 ` Andy Yan
2016-02-02 10:02 ` [PATCH v3 2/4] power: reset: add reboot mode driver Andy Yan
2016-02-02 18:16 ` Moritz Fischer
2016-02-03 11:54 ` kbuild test robot
2016-02-02 10:10 ` [PATCH v3 3/4] ARM: dts: rockchip: add syscon-reboot-mode DT node Andy Yan
2016-02-02 10:13 ` [PATCH v3 4/4] ARM64: " Andy Yan
2016-02-03 2:07 ` Shawn Lin
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='CALAqxLVyznpLd2MLgGr28TDECpLZ=RpmhxH-SHqntXfBsctcmQ@mail.gmail.com' \
--to=john.stultz@linaro.org \
--cc=alexandre.belloni@free-electrons.com \
--cc=andy.yan@rock-chips.com \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=dbaryshkov@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=dwmw2@infradead.org \
--cc=galak@codeaurora.org \
--cc=heiko@sntech.de \
--cc=ijc+devicetree@hellion.org.uk \
--cc=jun.nie@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux@arm.linux.org.uk \
--cc=lorenzo.pieralisi@arm.com \
--cc=mark.rutland@arm.com \
--cc=matthias.bgg@gmail.com \
--cc=mbrugger@suse.com \
--cc=moritz.fischer@ettus.com \
--cc=pawel.moll@arm.com \
--cc=richard@nod.at \
--cc=robh@kernel.org \
--cc=sre@kernel.org \
--cc=will.deacon@arm.com \
--cc=wxt@rock-chips.com \
/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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).