Re: [PATCH v2 09/10] ARM: dts: r8a779x: Add reset module support

[Dirk Behme <dirk.behme@de.bosch.com>]

Hi Geert,

On 11.05.2016 14:13, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Wed, May 11, 2016 at 1:56 PM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> On 11.05.2016 10:06, Geert Uytterhoeven wrote:
>>> On Wed, May 11, 2016 at 7:29 AM, Dirk Behme <dirk.behme@de.bosch.com>
>>> wrote:
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/misc/renesas,rcar-rst.txt
>>>> @@ -0,0 +1,15 @@
>>>> +Renesas RCar r8a779x reset module
>>>> +-----------------------------------------------------
>>>> +This binding defines the reset module found on Renesas RCar r8a779x
>>>> +SoCs. The reset module contains several reset related registers,
>>>> +the meaning of them is implementation dependent.
>>>> +
>>>> +Required properties:
>>>> +- compatible : "renesas,rcar-rst"
>>>> +- reg : Location and size of the reset module
>>>> +
>>>> +Example:
>>>> +       reset-controller@e6160000 {
>>>> +               compatible = "renesas,rcar-rst";
>>>> +               reg = <0 0xe6160000 0 0x200>;
>>>> +       };
>>>
>>>
>>> While I understand you want to match on a single comptible value, the RST
>>> module itself highly depends on the actual SoC.
>>> Furthermore, R-Car Gen1 doesn't have RST.
>>>
>>> Hence I'd go for requiring 2 compatible values:
>>>   - An SoC-specific one, e.g. "renesas,r8a7795-rst",
>>>   - A family-specific one, e.g. "renesas,rcar-gen3-rst".
>>>
>>> Your driver code can match against the two family-specific compatible
>>> values
>>> using of_find_matching_node().
>>
>> This way? See below [1].
>>
>> Best regards
>>
>> Dirk
>>
>> [1]
>>
>> --- a/drivers/misc/boot-mode-reg/rcar.c
>> +++ b/drivers/misc/boot-mode-reg/rcar.c
>> @@ -16,24 +16,43 @@
>>  #include <linux/io.h>
>>  #include <linux/module.h>
>>  #include <linux/of.h>
>> +#include <linux/of_address.h>
>>
>>  #include <misc/boot-mode-reg.h>
>>
>> -#define MODEMR 0xe6160060
>> +#define RCAR_RST_BASE 0xe6160000
>> +#define RCAR_RST_SIZE 0x200
>> +#define MODEMR 0x60
>> +
>> +static struct of_device_id rcar_rst_ids[] __initdata = {
>
> const
>
>> +       { .compatible = "renesas,rcar-gen2-rst" },
>> +       { .compatible = "renesas,rcar-gen3-rst" },
>> +       {}
>> +};
>>
>>  static int __init rcar_read_mode_pins(void)
>>  {
>> -       void __iomem *modemr;
>> +       void __iomem *reset;
>> +       struct device_node *np;
>>         int err = -ENOMEM;
>>         static u32 mode;
>>
>> -       modemr = ioremap_nocache(MODEMR, 4);
>> -       if (!modemr) {
>> -               pr_err("failed to map boot mode register");
>> +       np = of_find_matching_node(NULL, rcar_rst_ids);
>
> Yep, like this.
>
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/misc/renesas,rcar-rst.txt
>> @@ -0,0 +1,24 @@
>> +Renesas RCar r8a779x reset module
>
> R-Car Gen2 and Gen3
>
>> +-----------------------------------------------------
>> +This binding defines the reset module found on Renesas RCar r8a779x
>
> R-Car Gen2 and Gen3
>
>> +SoCs. The reset module contains several reset related registers,
>> +the meaning of them is implementation dependent.
>
> You may want to add more functionality from
> http://www.spinics.net/lists/linux-sh/msg44754.html


Just an understanding question:

Why do you prefer the solution with the drivers/misc/boot-mode-reg/ 
solution done in this patch series over the solution you proposed in

http://www.spinics.net/lists/linux-sh/msg44755.html

and the reset of that series?

Best regards

Dirk


>> +Required properties:
>> +- compatible : "renesas,rcar-gen2-rst" for RCar Gen2 or
>
> R-Car
>
>> +               "renesas,rcar-gen3-rst" for RCar Gen3
>
> R-Car
>
>> +              and additionally a SoC specific property like
>> +              "renesas,r8a7790-rst" or
>> +              "renesas,r8a7791-rst" or
>> +              "renesas,r8a7792-rst" or
>> +              "renesas,r8a7793-rst" or
>> +              "renesas,r8a7794-rst" or
>> +              "renesas,r8a7795-rst" or
>> +              "renesas,r8a7796-rst"
>
> The SoC-specific compatible values should be listed first.
>
>> +- reg : Location and size of the reset module
>> +
>> +Example:
>> +       reset-controller@e6160000 {
>> +               compatible = "renesas,rcar-gen3-rst", "renesas,r8a7795-rst";
>
> The SoC-specific compatible value should be listed first.
> The same is true for the actual dtsi files.
>
>> +               reg = <0 0xe6160000 0 0x200>;
>> +       };
>