Hi Rob:

On 2016?01?26? 01:11, Rob Herring wrote:

On Thu, Jan 21, 2016 at 02:27:57PM +0800, Andy Yan wrote:

quoted

Hi Rob:
    thanks for your review.
On 2016?01?21? 02:28, Rob Herring wrote:

quoted

On Tue, Jan 12, 2016 at 07:29:49PM +0800, Andy Yan wrote:

quoted

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

[...]

quoted

quoted

quoted

+		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.

quoted

quoted

quoted

+
+		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);
         }

quoted

quoted

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