| Message ID | 1452598189-8272-1-git-send-email-andy.yan@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 12, 2016 at 3:29 AM, Andy Yan <andy.yan@rock-chips.com> wrote: > add device tree binding document for reboot-mode driver > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > --- > > Changes in v2: None > Changes in v1: None > > .../bindings/power/reset/reboot-mode.txt | 41 +++++++++++++++++ > .../bindings/power/reset/syscon-reboot-mode.txt | 52 ++++++++++++++++++++++ > 2 files changed, 93 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..81d9f66 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt > @@ -0,0 +1,41 @@ > +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 to the argument stored. > + > +Required properties: > +- compatible: only support "syscon-reboot-mode" now. > + > +Each mode is represented as a sub-node of reboot_mode: > + > +Subnode required properties: > +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot". > +- loader,magic: magic number for the mode, this is vendor specific. > + > +Example: > + reboot_mode { > + compatible = "syscon-reboot-mode"; > + offset = <0x40>; > + > + loader { > + linux,mode = "loader"; > + loader,magic = <BOOT_LOADER>; > + }; > + > + maskrom { > + linux,mode = "maskrom"; > + loader,magic = <BOOT_MASKROM>; > + }; > + > + recovery { > + linux,mode = "recovery"; > + loader,magic = <BOOT_RECOVERY>; > + }; > + > + fastboot { > + linux,mode = "fastboot"; > + loader,magic = <BOOT_FASTBOOT>; > + }; > + }; So one minor thought here. While the commands are somewhat vendor specific, would it be a good idea for the example commands to match the common commands on Android devices? For example, usually "bootloader" is what gets you into fastboot mode on nexus devices. thanks -john
Hi John: On 2016?01?16? 06:41, John Stultz wrote: > On Tue, Jan 12, 2016 at 3:29 AM, Andy Yan <andy.yan@rock-chips.com> wrote: >> add device tree binding document for reboot-mode driver >> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> >> --- >> >> Changes in v2: None >> Changes in v1: None >> >> .../bindings/power/reset/reboot-mode.txt | 41 +++++++++++++++++ >> .../bindings/power/reset/syscon-reboot-mode.txt | 52 ++++++++++++++++++++++ >> 2 files changed, 93 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..81d9f66 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt >> @@ -0,0 +1,41 @@ >> +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 to the argument stored. >> + >> +Required properties: >> +- compatible: only support "syscon-reboot-mode" now. >> + >> +Each mode is represented as a sub-node of reboot_mode: >> + >> +Subnode required properties: >> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot". >> +- loader,magic: magic number for the mode, this is vendor specific. >> + >> +Example: >> + reboot_mode { >> + compatible = "syscon-reboot-mode"; >> + offset = <0x40>; >> + >> + loader { >> + linux,mode = "loader"; >> + loader,magic = <BOOT_LOADER>; >> + }; >> + >> + maskrom { >> + linux,mode = "maskrom"; >> + loader,magic = <BOOT_MASKROM>; >> + }; >> + >> + recovery { >> + linux,mode = "recovery"; >> + loader,magic = <BOOT_RECOVERY>; >> + }; >> + >> + fastboot { >> + linux,mode = "fastboot"; >> + loader,magic = <BOOT_FASTBOOT>; >> + }; >> + }; > > So one minor thought here. While the commands are somewhat vendor > specific, would it be a good idea for the example commands to match > the common commands on Android devices? For example, usually > "bootloader" is what gets you into fastboot mode on nexus devices. > > thanks > -john > > > > When I run the "adb help" , I got the following message: adb reboot [bootloader|recovery] - reboots the device, optionally into the bootloader or recovery program adb reboot-bootloader - reboots the device into the bootloader It only says "adb reboot-bootloader" will reboot the device into bootloader, not specific to fastboot. I add @Simon from Google here, maybe he can give some suggestions.
On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote: > add device tree binding document for reboot-mode driver > > Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > > --- > > Changes in v2: None > Changes in v1: None > > .../bindings/power/reset/reboot-mode.txt | 41 +++++++++++++++++ > .../bindings/power/reset/syscon-reboot-mode.txt | 52 ++++++++++++++++++++++ > 2 files changed, 93 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..81d9f66 > --- /dev/null > +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt > @@ -0,0 +1,41 @@ > +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 to the argument stored. > + > +Required properties: > +- compatible: only support "syscon-reboot-mode" now. > + > +Each mode is represented as a sub-node of reboot_mode: > + > +Subnode required properties: > +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot". > +- loader,magic: magic number for the mode, this is vendor specific. > + > +Example: > + reboot_mode { reboot-mode instead please. > + compatible = "syscon-reboot-mode"; > + offset = <0x40>; This doc by itself is a little confusing. For example, is a child of the syscon node? I would remove offset (and perhaps compatible) from this example. > + > + loader { > + linux,mode = "loader"; > + loader,magic = <BOOT_LOADER>; > + }; Sorry, my previous suggestion was not clear. I'm suggesting get rid of the subnodes and just do properties like this: loader = <BOOT_LOADER>; maskrom = <BOOT_MASKROM>; That's the same amount of information unless node names and linux,mode values are going to diverge. Do they need to? I can't see a reason. We need to be clear what loader means. More specifically, it is boot into bootloader shell. > + > + maskrom { In theory, the bootrom could have multiple modes. This typically means a USB download mode. So perhaps a more precise name would be "rom-download". In chips I'm familiar with the bootrom mode is selected via a different mechanism than the secondary bootloader modes, but I suppose the same mechanism could be used. > + linux,mode = "maskrom"; > + loader,magic = <BOOT_MASKROM>; > + }; > + > + recovery { > + linux,mode = "recovery"; > + loader,magic = <BOOT_RECOVERY>; > + }; > + > + fastboot { > + linux,mode = "fastboot"; > + loader,magic = <BOOT_FASTBOOT>; > + };
On Wed, Jan 20, 2016 at 10:28 AM, Rob Herring <robh@kernel.org> wrote: > On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote: >> add device tree binding document for reboot-mode driver >> >> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >> >> --- >> >> Changes in v2: None >> Changes in v1: None >> >> .../bindings/power/reset/reboot-mode.txt | 41 +++++++++++++++++ >> .../bindings/power/reset/syscon-reboot-mode.txt | 52 ++++++++++++++++++++++ >> 2 files changed, 93 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..81d9f66 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt >> @@ -0,0 +1,41 @@ >> +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 to the argument stored. >> + >> +Required properties: >> +- compatible: only support "syscon-reboot-mode" now. >> + >> +Each mode is represented as a sub-node of reboot_mode: >> + >> +Subnode required properties: >> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot". >> +- loader,magic: magic number for the mode, this is vendor specific. >> + >> +Example: >> + reboot_mode { > > reboot-mode instead please. > >> + compatible = "syscon-reboot-mode"; >> + offset = <0x40>; > > This doc by itself is a little confusing. For example, is a child of the > syscon node? I would remove offset (and perhaps compatible) from this > example. > >> + >> + loader { >> + linux,mode = "loader"; >> + loader,magic = <BOOT_LOADER>; >> + }; > > Sorry, my previous suggestion was not clear. I'm suggesting get rid of > the subnodes and just do properties like this: > > loader = <BOOT_LOADER>; > maskrom = <BOOT_MASKROM>; > > That's the same amount of information unless node names and linux,mode > values are going to diverge. Do they need to? I can't see a reason. It seems like devices already have a number of various different vendor specific commands. So this sort of flexibility helps us adapt the driver to different hardware/system environments (which may use different conventions then what Android commonly uses). Unless I'm misunderstanding you and you're instead suggesting we can dynamically parse the command mode from the node name? That said, I agree we should try to push vendors to using the same conventions when they are providing the same functionality. But I'm not sure we should enforce that by making vendors introduce a new dt binding for every new reboot command they want to implement. thanks -john
On Wed, Jan 20, 2016 at 12:47 PM, John Stultz <john.stultz@linaro.org> wrote: > On Wed, Jan 20, 2016 at 10:28 AM, Rob Herring <robh@kernel.org> wrote: >> On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote: >>> add device tree binding document for reboot-mode driver >>> >>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >>> >>> --- >>> >>> Changes in v2: None >>> Changes in v1: None >>> >>> .../bindings/power/reset/reboot-mode.txt | 41 +++++++++++++++++ >>> .../bindings/power/reset/syscon-reboot-mode.txt | 52 ++++++++++++++++++++++ >>> 2 files changed, 93 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..81d9f66 >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt >>> @@ -0,0 +1,41 @@ >>> +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 to the argument stored. >>> + >>> +Required properties: >>> +- compatible: only support "syscon-reboot-mode" now. >>> + >>> +Each mode is represented as a sub-node of reboot_mode: >>> + >>> +Subnode required properties: >>> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot". >>> +- loader,magic: magic number for the mode, this is vendor specific. >>> + >>> +Example: >>> + reboot_mode { >> >> reboot-mode instead please. >> >>> + compatible = "syscon-reboot-mode"; >>> + offset = <0x40>; >> >> This doc by itself is a little confusing. For example, is a child of the >> syscon node? I would remove offset (and perhaps compatible) from this >> example. >> >>> + >>> + loader { >>> + linux,mode = "loader"; >>> + loader,magic = <BOOT_LOADER>; >>> + }; >> >> Sorry, my previous suggestion was not clear. I'm suggesting get rid of >> the subnodes and just do properties like this: >> >> loader = <BOOT_LOADER>; >> maskrom = <BOOT_MASKROM>; >> >> That's the same amount of information unless node names and linux,mode >> values are going to diverge. Do they need to? I can't see a reason. > > > It seems like devices already have a number of various different > vendor specific commands. So this sort of flexibility helps us adapt > the driver to different hardware/system environments (which may use > different conventions then what Android commonly uses). > > Unless I'm misunderstanding you and you're instead suggesting we can > dynamically parse the command mode from the node name? Yes, except it would be the property name. The driver could either iterate over the properties of the reboot-mode node or lookup the property when it has the reboot reason string. We generally like to parse everything up front at probe, but the latter would be somewhat easier to implement. Perhaps we would want to prefix nodes with "mode-" or something. Vendors can certainly still add their own (e.g. vendor,halt-and-catch-fire = <0xdead>). The question here really is will we ever need additional data for each mode (in the DT) besides "magic". We can do some amount of extending the property like allowing more that single 32-bit value. We can't mix strings and integers though. If we don't need that, then lets go with the more concise approach. This isn't really an evolving area. Selecting boot mode/device has existed for a long time on PCs (it is fixed list in the IPMI spec for example). So I can't imagine that we'd have to extend this, but I'd like to hear thoughts on it. You had mentioned the Galaxy Nexus having a value and a string, but I'd find it surprising if the bootloader actually uses the string. > That said, I agree we should try to push vendors to using the same > conventions when they are providing the same functionality. But I'm > not sure we should enforce that by making vendors introduce a new dt > binding for every new reboot command they want to implement. Either way they need to document it. It's either a new linux,mode value or a new property name. Vendors are free to add whatever they want, but for upstream it has to go thru review. Rob
On Wed, Jan 20, 2016 at 11:53 AM, Rob Herring <robh@kernel.org> wrote: > On Wed, Jan 20, 2016 at 12:47 PM, John Stultz <john.stultz@linaro.org> wrote: > The question here really is will we ever need additional data for each > mode (in the DT) besides "magic". We can do some amount of extending > the property like allowing more that single 32-bit value. We can't mix > strings and integers though. If we don't need that, then lets go with > the more concise approach. This isn't really an evolving area. > Selecting boot mode/device has existed for a long time on PCs (it is > fixed list in the IPMI spec for example). So I can't imagine that we'd > have to extend this, but I'd like to hear thoughts on it. You had > mentioned the Galaxy Nexus having a value and a string, but I'd find > it surprising if the bootloader actually uses the string. So I've been told the galaxy nexus and possibly other older TI hardware used only a string. And the HTC implementations I've seen have code for both a string and command to be placed, but usually don't actually copy anything to the string address (which suggest its not used by the bootloader, as you note). Either way, we'd have to extend or implement a new driver, but that's a bridge I think we can cross later when we see such hardware support upstream (rather then try to create a super driver that handles everything a creative firmware developer could come up with). thanks -john
Hi Rob:
thanks for your review.
On 2016?01?21? 02:28, Rob Herring wrote:
> On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote:
>> add device tree binding document for reboot-mode driver
>>
>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com>
>>
>> ---
>>
>> Changes in v2: None
>> Changes in v1: None
>>
>> .../bindings/power/reset/reboot-mode.txt | 41 +++++++++++++++++
>> .../bindings/power/reset/syscon-reboot-mode.txt | 52 ++++++++++++++++++++++
>> 2 files changed, 93 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..81d9f66
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt
>> @@ -0,0 +1,41 @@
>> +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 to the argument stored.
>> +
>> +Required properties:
>> +- compatible: only support "syscon-reboot-mode" now.
>> +
>> +Each mode is represented as a sub-node of reboot_mode:
>> +
>> +Subnode required properties:
>> +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot".
>> +- loader,magic: magic number for the mode, this is vendor specific.
>> +
>> +Example:
>> + reboot_mode {
> reboot-mode instead please.
>
Sorry, I have already correct it in DT file, forget it here. It
will be changed in next version.
>> + compatible = "syscon-reboot-mode";
>> + offset = <0x40>;
> This doc by itself is a little confusing. For example, is a child of the
> syscon node? I would remove offset (and perhaps compatible) from this
> example.
Yes, is a child of a syscon mapped node. For example, Rockchip
platform use a register of PMU(rk3066/rk3288) or GRF(rk3036), PMU and
GRF are aleady mapped by syscon.
offset and compatible are used by write interface driver like
syscon-reboot-mode.c. If you don't like it appear in the core map doc, I
will move it to the syscon-reboot-mode.txt?
>> +
>> + loader {
>> + linux,mode = "loader";
>> + loader,magic = <BOOT_LOADER>;
>> + };
> Sorry, my previous suggestion was not clear. I'm suggesting get rid of
> the subnodes and just do properties like this:
>
> loader = <BOOT_LOADER>;
> maskrom = <BOOT_MASKROM>;
>
> That's the same amount of information unless node names and linux,mode
> values are going to diverge. Do they need to? I can't see a reason.
Because the command"linux,mode" and value"loader,magic" is vendor
specific. I don't know what commands and how many mode other platform
will use. So as John says in his reply, this sort of flexibility help us
adapt the driver to different hardware/system environments.
>
> We need to be clear what loader means. More specifically, it is boot
> into bootloader shell.
Actually, Rockchip platform will reboot into a bootloader download
mode with this command. This mode can download faster than maskrom
download mode.
>> +
>> + maskrom {
> In theory, the bootrom could have multiple modes. This typically means a
> USB download mode. So perhaps a more precise name would be
> "rom-download".
>
> In chips I'm familiar with the bootrom mode is selected via a different
> mechanism than the secondary bootloader modes, but I suppose the same
> mechanism could be used.
Yes , they use the same mechanism.
>> + linux,mode = "maskrom";
>> + loader,magic = <BOOT_MASKROM>;
>> + };
>> +
>> + recovery {
>> + linux,mode = "recovery";
>> + loader,magic = <BOOT_RECOVERY>;
>> + };
>> +
>> + fastboot {
>> + linux,mode = "fastboot";
>> + loader,magic = <BOOT_FASTBOOT>;
>> + };
>
>
>
On Thu, Jan 21, 2016 at 02:27:57PM +0800, Andy Yan wrote: > Hi Rob: > thanks for your review. > On 2016?01?21? 02:28, Rob Herring wrote: > >On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote: > >>add device tree binding document for reboot-mode driver > >> > >>Signed-off-by: Andy Yan <andy.yan@rock-chips.com> > >> > >>--- > >> > >>Changes in v2: None > >>Changes in v1: None > >> > >> .../bindings/power/reset/reboot-mode.txt | 41 +++++++++++++++++ > >> .../bindings/power/reset/syscon-reboot-mode.txt | 52 ++++++++++++++++++++++ > >> 2 files changed, 93 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..81d9f66 > >>--- /dev/null > >>+++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt > >>@@ -0,0 +1,41 @@ > >>+Generic reboot mode core map driver [...] > >>+ compatible = "syscon-reboot-mode"; > >>+ offset = <0x40>; > >This doc by itself is a little confusing. For example, is a child of the > >syscon node? I would remove offset (and perhaps compatible) from this > >example. > > Yes, is a child of a syscon mapped node. For example, Rockchip platform > use a register of PMU(rk3066/rk3288) or GRF(rk3036), PMU and GRF are aleady > mapped by syscon. > offset and compatible are used by write interface driver like > syscon-reboot-mode.c. If you don't like it appear in the core map doc, I > will move it to the syscon-reboot-mode.txt? Yes, try to make this doc stand on its own. It will obviously be incomplete lacking information on where in the DT it goes. So perhaps a note stating reboot-mode node location is defined in platform specific binding docs. > >>+ > >>+ loader { > >>+ linux,mode = "loader"; > >>+ loader,magic = <BOOT_LOADER>; > >>+ }; > >Sorry, my previous suggestion was not clear. I'm suggesting get rid of > >the subnodes and just do properties like this: > > > >loader = <BOOT_LOADER>; > >maskrom = <BOOT_MASKROM>; > > > >That's the same amount of information unless node names and linux,mode > >values are going to diverge. Do they need to? I can't see a reason. > > Because the command"linux,mode" and value"loader,magic" is vendor > specific. I don't know what commands and how many mode other platform will > use. So as John says in his reply, this sort of flexibility help us adapt > the driver to different hardware/system environments. The only part of "reboot to fastboot" that is vendor specific would be the magic value. While we can have custom modes, we should standardize the common ones as much as possible. As I pointed out in my reply to John, we can still support vendor specific modes with just a property. > > > >We need to be clear what loader means. More specifically, it is boot > >into bootloader shell. > Actually, Rockchip platform will reboot into a bootloader download mode > with this command. This mode can download faster than maskrom download mode. My point is proven. I assumed one thing and you meant something else. Doesn't matter what the mode is, just needs to be clear. Rob
Hi Rob: On 2016?01?26? 01:11, Rob Herring wrote: > On Thu, Jan 21, 2016 at 02:27:57PM +0800, Andy Yan wrote: >> Hi Rob: >> thanks for your review. >> On 2016?01?21? 02:28, Rob Herring wrote: >>> On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote: >>>> add device tree binding document for reboot-mode driver >>>> >>>> Signed-off-by: Andy Yan <andy.yan@rock-chips.com> >>>> >>>> --- >>>> >>>> Changes in v2: None >>>> Changes in v1: None >>>> >>>> .../bindings/power/reset/reboot-mode.txt | 41 +++++++++++++++++ >>>> .../bindings/power/reset/syscon-reboot-mode.txt | 52 ++++++++++++++++++++++ >>>> 2 files changed, 93 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..81d9f66 >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt >>>> @@ -0,0 +1,41 @@ >>>> +Generic reboot mode core map driver > [...] > >>>> + compatible = "syscon-reboot-mode"; >>>> + offset = <0x40>; >>> This doc by itself is a little confusing. For example, is a child of the >>> syscon node? I would remove offset (and perhaps compatible) from this >>> example. >> Yes, is a child of a syscon mapped node. For example, Rockchip platform >> use a register of PMU(rk3066/rk3288) or GRF(rk3036), PMU and GRF are aleady >> mapped by syscon. >> offset and compatible are used by write interface driver like >> syscon-reboot-mode.c. If you don't like it appear in the core map doc, I >> will move it to the syscon-reboot-mode.txt? > Yes, try to make this doc stand on its own. It will obviously be > incomplete lacking information on where in the DT it goes. So perhaps a > note stating reboot-mode node location is defined in platform specific > binding docs. > >>>> + >>>> + loader { >>>> + linux,mode = "loader"; >>>> + loader,magic = <BOOT_LOADER>; >>>> + }; >>> Sorry, my previous suggestion was not clear. I'm suggesting get rid of >>> the subnodes and just do properties like this: >>> >>> loader = <BOOT_LOADER>; >>> maskrom = <BOOT_MASKROM>; >>> >>> That's the same amount of information unless node names and linux,mode >>> values are going to diverge. Do they need to? I can't see a reason. >> Because the command"linux,mode" and value"loader,magic" is vendor >> specific. I don't know what commands and how many mode other platform will >> use. So as John says in his reply, this sort of flexibility help us adapt >> the driver to different hardware/system environments. > The only part of "reboot to fastboot" that is vendor specific would be > the magic value. While we can have custom modes, we should standardize > the common ones as much as possible. As I pointed out in my reply to > John, we can still support vendor specific modes with just a property. Based your reply to John, I rebuild the code like bellow, I hope this is what you mean. DTS file: reboot-mode { compatible = "syscon-reboot-mode"; offset = <0x94>; mode-normal = <BOOT_NORMAL>; mode-recovery = <BOOT_RECOVERY>; mode-fastboot = <BOOT_FASTBOOT>; mode-loader = <BOOT_LOADER>; mode-maskrom = <BOOT_MASKROM>; }; driver: #define PREFIX "mode-" struct property *prop; size_t len = strlen(PREFIX); for_each_property_of_node(dev->of_node, prop) { if (len > strlen(prop->name) || strncmp(prop->name, PREFIX, len)) continue; info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL); if (!info) return -ENOMEM; strcpy(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); } >>> We need to be clear what loader means. More specifically, it is boot >>> into bootloader shell. >> Actually, Rockchip platform will reboot into a bootloader download mode >> with this command. This mode can download faster than maskrom download mode. > My point is proven. I assumed one thing and you meant something else. > Doesn't matter what the mode is, just needs to be clear. > > Rob > > >
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..81d9f66 --- /dev/null +++ b/Documentation/devicetree/bindings/power/reset/reboot-mode.txt @@ -0,0 +1,41 @@ +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 to the argument stored. + +Required properties: +- compatible: only support "syscon-reboot-mode" now. + +Each mode is represented as a sub-node of reboot_mode: + +Subnode required properties: +- linux,mode: reboot mode command,such as "loader", "recovery", "fastboot". +- loader,magic: magic number for the mode, this is vendor specific. + +Example: + reboot_mode { + compatible = "syscon-reboot-mode"; + offset = <0x40>; + + loader { + linux,mode = "loader"; + loader,magic = <BOOT_LOADER>; + }; + + maskrom { + linux,mode = "maskrom"; + loader,magic = <BOOT_MASKROM>; + }; + + recovery { + linux,mode = "recovery"; + loader,magic = <BOOT_RECOVERY>; + }; + + fastboot { + linux,mode = "fastboot"; + loader,magic = <BOOT_FASTBOOT>; + }; + }; diff --git a/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt new file mode 100644 index 0000000..6bce7dd --- /dev/null +++ b/Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt @@ -0,0 +1,52 @@ +SYSCON reboot mode driver + +This driver get reboot mode magic value form reboot-mode driver +and stores it in a SYSCON mapped register. Then the bootloader +can read it and take different action according to the magic +value stored. + +This DT node should be represented as a sub-node of a "syscon", "simple-mfd" +node. + +Required properties: +- compatible: should be "syscon-reboot-mode" +- offset: offset in the register map for the storage register (in bytes) + +The rest of the properties should follow the generic reboot-mode discription +found in reboot-mode.txt + +Example: + pmu: pmu@20004000 { + compatible = "rockchip,rk3066-pmu", "syscon", "simple-mfd"; + reg = <0x20004000 0x100>; + + reboot-mode { + compatible = "syscon-reboot-mode"; + offset = <0x40>; + + normal { + linux,mode = "normal"; + loader,magic = <BOOT_NORMAL>; + }; + + loader { + linux,mode = "loader"; + loader,magic = <BOOT_LOADER>; + }; + + maskrom { + linux,mode = "maskrom"; + loader,magic = <BOOT_MASKROM>; + }; + + recovery { + linux,mode = "recovery"; + loader,magic = <BOOT_RECOVERY>; + }; + + fastboot { + linux,mode = "fastboot"; + loader,magic = <BOOT_FASTBOOT>; + }; + }; + };
add device tree binding document for reboot-mode driver Signed-off-by: Andy Yan <andy.yan@rock-chips.com> --- Changes in v2: None Changes in v1: None .../bindings/power/reset/reboot-mode.txt | 41 +++++++++++++++++ .../bindings/power/reset/syscon-reboot-mode.txt | 52 ++++++++++++++++++++++ 2 files changed, 93 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/reset/reboot-mode.txt create mode 100644 Documentation/devicetree/bindings/power/reset/syscon-reboot-mode.txt