Re: [PATCH v2 1/4] dt-bindings: power: reset: add document for reboot-mode driver

[Andy Yan <andy.yan@rock-chips.com>]

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