[PATCH/RFC v3 20/22] clk: renesas: r8a7778: Remove obsolete r8a7778_clocks_init()

[PATCH/RFC v3 20/22] clk: renesas: r8a7778: Remove obsolete r8a7778_clocks_init()

[Dirk Behme]

On 01.06.2016 21:21, Geert Uytterhoeven wrote:
> The R-Car M1A board code no longer calls r8a7778_clocks_init().
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  drivers/clk/renesas/clk-r8a7778.c | 13 -------------
>  include/linux/clk/renesas.h       |  1 -
>  2 files changed, 14 deletions(-)
>
> diff --git a/drivers/clk/renesas/clk-r8a7778.c b/drivers/clk/renesas/clk-r8a7778.c
> index 07ea411098a75ad1..886a8380e91247a1 100644
> --- a/drivers/clk/renesas/clk-r8a7778.c
> +++ b/drivers/clk/renesas/clk-r8a7778.c
> @@ -143,16 +143,3 @@ static void __init r8a7778_cpg_clocks_init(struct device_node *np)
>
>  CLK_OF_DECLARE(r8a7778_cpg_clks, "renesas,r8a7778-cpg-clocks",
>  	       r8a7778_cpg_clocks_init);
> -
> -void __init r8a7778_clocks_init(u32 mode)
> -{
> -	BUG_ON(!(mode & BIT(19)));
> -
> -	cpg_mode_rates = (!!(mode & BIT(18)) << 2) |
> -			 (!!(mode & BIT(12)) << 1) |
> -			 (!!(mode & BIT(11)));
> -	cpg_mode_divs = (!!(mode & BIT(2)) << 1) |
> -			(!!(mode & BIT(1)));
> -
> -	of_clk_init(NULL);
> -}


Just a question on how you structured the patches: Is there a special 
reason why you do the adding of new code and the removal of dead code in 
two patches?

Having both in one patch normally makes it more obvious which old code 
is replaced by which new code.

An example: In patch

[PATCH/RFC v3 11/22] clk: renesas: r8a7778: Obtain mode pin values using 
R-Car RST driver

I wondered where the section

+	BUG_ON(!(mode & BIT(19)));
+
+	cpg_mode_rates = (!!(mode & BIT(18)) << 2) |
+			 (!!(mode & BIT(12)) << 1) |
+			 (!!(mode & BIT(11)));
+	cpg_mode_divs = (!!(mode & BIT(2)) << 1) |
+			(!!(mode & BIT(1)));

comes from. This becomes obvious with this patch 20/22. But it's 9 
patches later ;) So why don't make the whole replacement visible in one 
patch?

Best regards

Dirk

[PATCH/RFC v3 20/22] clk: renesas: r8a7778: Remove obsolete r8a7778_clocks_init()

[Geert Uytterhoeven]

Hi Dirk,

On Thu, Jun 2, 2016 at 7:54 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 01.06.2016 21:21, Geert Uytterhoeven wrote:
>> The R-Car M1A board code no longer calls r8a7778_clocks_init().
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> v3:
>>   - New.
>> ---
>>  drivers/clk/renesas/clk-r8a7778.c | 13 -------------
>>  include/linux/clk/renesas.h       |  1 -
>>  2 files changed, 14 deletions(-)
>>
>> diff --git a/drivers/clk/renesas/clk-r8a7778.c
>> b/drivers/clk/renesas/clk-r8a7778.c
>> index 07ea411098a75ad1..886a8380e91247a1 100644
>> --- a/drivers/clk/renesas/clk-r8a7778.c
>> +++ b/drivers/clk/renesas/clk-r8a7778.c
>> @@ -143,16 +143,3 @@ static void __init r8a7778_cpg_clocks_init(struct
>> device_node *np)
>>
>>  CLK_OF_DECLARE(r8a7778_cpg_clks, "renesas,r8a7778-cpg-clocks",
>>                r8a7778_cpg_clocks_init);
>> -
>> -void __init r8a7778_clocks_init(u32 mode)
>> -{
>> -       BUG_ON(!(mode & BIT(19)));
>> -
>> -       cpg_mode_rates = (!!(mode & BIT(18)) << 2) |
>> -                        (!!(mode & BIT(12)) << 1) |
>> -                        (!!(mode & BIT(11)));
>> -       cpg_mode_divs = (!!(mode & BIT(2)) << 1) |
>> -                       (!!(mode & BIT(1)));
>> -
>> -       of_clk_init(NULL);
>> -}
>
> Just a question on how you structured the patches: Is there a special reason
> why you do the adding of new code and the removal of dead code in two
> patches?
>
> Having both in one patch normally makes it more obvious which old code is
> replaced by which new code.
>
> An example: In patch
>
> [PATCH/RFC v3 11/22] clk: renesas: r8a7778: Obtain mode pin values using
> R-Car RST driver
>
> I wondered where the section
>
> +       BUG_ON(!(mode & BIT(19)));
> +
> +       cpg_mode_rates = (!!(mode & BIT(18)) << 2) |
> +                        (!!(mode & BIT(12)) << 1) |
> +                        (!!(mode & BIT(11)));
> +       cpg_mode_divs = (!!(mode & BIT(2)) << 1) |
> +                       (!!(mode & BIT(1)));
>
> comes from. This becomes obvious with this patch 20/22. But it's 9 patches
> later ;) So why don't make the whole replacement visible in one patch?

Because that would break bisection.
Path 20 depends on patch 17, which depends on patch 11.
These three can't be a single patch because they touch multiple subsystems.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for obtaining mode pin state

[Dirk Behme]

On 01.06.2016 21:20, Geert Uytterhoeven wrote:
> 	Hi all,
>
> Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the
> state of the mode pins either by a call from the board code, or directly
> by using a hardcoded register access. This is a bit messy, and creates a
> dependency between driver and platform code.
>
> This RFC patch series converts the various Renesas R-Car clock drivers
> and support code from reading the mode pin states using a hardcoded
> register access to using a new R-Car RST driver.
>
> This series consists of 5 parts:
>   A. Patches 1 and 2 add DT bindings and driver code for the R-Car RST
>      driver,
>   B. Patches 3-10 add device nodes for the RST modules to the R-Car DTS
>      files,
>   C. Patches 11-16 convert the clock drivers to call into the new R-Car
>      RST driver,
>   D. Patches 17-19 remove passing mode pin state to the clock drivers
>      from the board code,
>   E. Patches 20-22 remove dead code from the clock drivers.
>
> I've kept all parts together as this is an RFC, and to avoid losing the
> bigger picture. I will split it in series per maintainer, when we have
> decided how to handle the dependencies (see below).
>
> After this, there are still a few users of rcar_gen2_read_mode_pins()
> left in board code, which should be converted in one way or another.
> However, all R-Car clock drivers will rely on the presence in DT of a
> device node for the RST module.  Backwards compatibility with old DTBs
> is retained only for R-Car Gen2, which has fallback code calling into
> rcar_gen2_read_mode_pins().
>
> As is usually the case with moving functionality from board code to DT,
> there are lots of hard dependencies:
>   - The DT updates in Part B can be merged as soon as the DT bindings in
>     Part A have been approved,
>   - The clock driver updates in Part C depend functionally on the driver
>     code in Part A,
>   - The board code cleanups in Part D depend on the clock driver updates
>     in Part C,
>   - The block driver cleanups in part E depend on the board code
>     cleanups in part D.
>
> This series has evolved over time, cfr.
>   - "[PATCH/RFC 00/11] ARM: shmobile: Let CPG use syscon for MD pin
>     values" (http://www.spinics.net/lists/linux-clk/msg01478.html),
>   - "[PATCH 00/10] arm64: renesas: Obtain MD pin values using
>     syscon/regmap".
>     (http://www.spinics.net/lists/linux-sh/msg44757.html)
>
> Changes compared to v2:
>   - Use "renesas,<soctype>-rst" instead of "renesas,rst-<soctype>",
>   - Drop "syscon" compatible value and "renesas,modemr" property, use a
>     real driver instead,
>   - Add support for R-Car M1A, H1, and M3-W.
>
> Changes compared to v1:
>   - Add support for R-Car H3.
>
> An alternative and broader solution was proposed by Simon Horman as
> "[PATCH/RFC 0/6] boot-mode-reg: Add core"
> (http://www.spinics.net/lists/linux-sh/msg45969.html).
>
> This patch series is against renesas-drivers.
> It has been tested on r8a7778/bockw, r8a7779/marzen, r8a7791/koelsch,
> and r8a7795/salvator-x.
>
> Thanks for your comments!
>
> Geert Uytterhoeven (22):
>   reset: Add renesas,rst DT bindings
>   soc: renesas: Add R-Car RST driver
>   ARM: dts: r8a7778: Add device node for RESET/WDT module
>   ARM: dts: r8a7779: Add device node for RESET/WDT module
>   ARM: dts: r8a7790: Add device node for RST module
>   ARM: dts: r8a7791: Add device node for RST module
>   ARM: dts: r8a7793: Add device node for RST module
>   ARM: dts: r8a7794: Add device node for RST module
>   arm64: renesas: r8a7795 dtsi: Add device node for RST module
>   arm64: renesas: r8a7796 dtsi: Add device node for RST module
>   clk: renesas: r8a7778: Obtain mode pin values using R-Car RST driver
>   clk: renesas: r8a7779: Obtain mode pin values from R-Car RST driver
>   clk: renesas: rcar-gen2: Obtain mode pin values using RST driver
>   clk: renesas: r8a7795: Obtain mode pin values from R-Car RST driver
>   clk: renesas: r8a7796: Obtain mode pin values from R-Car RST driver
>   clk: renesas: rcar-gen3-cpg: Remove obsolete
>     rcar_gen3_read_mode_pins()
>   ARM: shmobile: r8a7778: Stop passing mode pins state to clock driver
>   ARM: shmobile: r8a7779: Stop passing mode pins state to clock driver
>   ARM: shmobile: rcar-gen2: Stop passing mode pins state to clock driver
>   clk: renesas: r8a7778: Remove obsolete r8a7778_clocks_init()
>   clk: renesas: r8a7779: Remove obsolete r8a7779_clocks_init()
>   clk: renesas: rcar-gen2: Remove obsolete rcar_gen2_clocks_init()
>
>  .../devicetree/bindings/reset/renesas,rst.txt      | 35 ++++++++
>  arch/arm/boot/dts/r8a7778.dtsi                     |  5 ++
>  arch/arm/boot/dts/r8a7779.dtsi                     |  5 ++
>  arch/arm/boot/dts/r8a7790.dtsi                     |  5 ++
>  arch/arm/boot/dts/r8a7791.dtsi                     |  5 ++
>  arch/arm/boot/dts/r8a7793.dtsi                     |  5 ++
>  arch/arm/boot/dts/r8a7794.dtsi                     |  5 ++
>  arch/arm/mach-shmobile/setup-r8a7778.c             | 15 ----
>  arch/arm/mach-shmobile/setup-r8a7779.c             | 27 -------
>  arch/arm/mach-shmobile/setup-rcar-gen2.c           |  6 +-
>  arch/arm64/boot/dts/renesas/r8a7795.dtsi           |  5 ++
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi           |  5 ++
>  drivers/clk/renesas/clk-r8a7778.c                  | 26 +++---
>  drivers/clk/renesas/clk-r8a7779.c                  | 18 ++---
>  drivers/clk/renesas/clk-rcar-gen2.c                | 17 ++--
>  drivers/clk/renesas/r8a7795-cpg-mssr.c             |  8 +-
>  drivers/clk/renesas/r8a7796-cpg-mssr.c             |  8 +-
>  drivers/clk/renesas/rcar-gen3-cpg.c                | 17 ----
>  drivers/clk/renesas/rcar-gen3-cpg.h                |  1 -
>  drivers/soc/renesas/Makefile                       |  5 ++
>  drivers/soc/renesas/rcar-rst.c                     | 94 ++++++++++++++++++++++
>  include/linux/clk/renesas.h                        |  4 -
>  include/linux/soc/renesas/rcar-rst.h               |  6 ++
>  23 files changed, 227 insertions(+), 100 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/reset/renesas,rst.txt
>  create mode 100644 drivers/soc/renesas/rcar-rst.c
>  create mode 100644 include/linux/soc/renesas/rcar-rst.h


Whole series:

Acked-by: Dirk Behme <dirk.behme@de.bosch.com>

Thanks!

Dirk

[PATCH/RFC v3 01/22] reset: Add renesas,rst DT bindings

[Dirk Behme]

On 01.06.2016 21:20, Geert Uytterhoeven wrote:
> Add DT bindings for the Renesas R-Car Reset Controller (R-Car Gen1
> RESET/WDT and R-Car Gen2/Gen3 RST).
>
> As the features provided by the hardware module differ a lot across the
> various SoC families and members, only SoC-specific compatible values
> are defined.
>
> For now we use the RST only for providing access to the state of the
> mode pins.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> v3:
>   - Clarify current usage,
>   - Use "renesas,<soctype>-rst" instead of "renesas,rst-<soctype>",
>   - Drop "syscon" compatible value,
>   - Add R-Car M3-W,
>   - Add R-Car Gen1,
>
> v2:
>   - Add Acked-by.
> ---
>  .../devicetree/bindings/reset/renesas,rst.txt      | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/renesas,rst.txt
>
> diff --git a/Documentation/devicetree/bindings/reset/renesas,rst.txt b/Documentation/devicetree/bindings/reset/renesas,rst.txt
> new file mode 100644
> index 0000000000000000..488c72e1ee849cd3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/renesas,rst.txt
> @@ -0,0 +1,35 @@
> +DT bindings for the Renesas R-Car Reset Controller
> +
> +The R-Car Reset Controller provides reset control, and implements the following
> +functions:
> +  - Latching of the levels on mode pins when PRESET# is negated,
> +  - Mode monitoring register,
> +  - Reset control of peripheral devices (on R-Car Gen1),
> +  - Watchdog timer (on R-Car Gen1).


Quite minor nit: s/./,/

Or drop all the ',' completely?


> +  - Register-based reset control and boot address registers for the various CPU
> +    cores (on R-Car Gen2/Gen3),

Best regards

Dirk

[PATCH/RFC v3 01/22] reset: Add renesas,rst DT bindings

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wednesday 01 Jun 2016 21:20:59 Geert Uytterhoeven wrote:
> Add DT bindings for the Renesas R-Car Reset Controller (R-Car Gen1
> RESET/WDT and R-Car Gen2/Gen3 RST).
> 
> As the features provided by the hardware module differ a lot across the
> various SoC families and members, only SoC-specific compatible values
> are defined.
> 
> For now we use the RST only for providing access to the state of the
> mode pins.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> v3:
>   - Clarify current usage,
>   - Use "renesas,<soctype>-rst" instead of "renesas,rst-<soctype>",
>   - Drop "syscon" compatible value,
>   - Add R-Car M3-W,
>   - Add R-Car Gen1,
> 
> v2:
>   - Add Acked-by.
> ---
>  .../devicetree/bindings/reset/renesas,rst.txt      | 35 +++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/renesas,rst.txt
> 
> diff --git a/Documentation/devicetree/bindings/reset/renesas,rst.txt
> b/Documentation/devicetree/bindings/reset/renesas,rst.txt new file mode
> 100644
> index 0000000000000000..488c72e1ee849cd3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/renesas,rst.txt
> @@ -0,0 +1,35 @@
> +DT bindings for the Renesas R-Car Reset Controller
> +
> +The R-Car Reset Controller provides reset control, and implements the
> following
> +functions:
> +  - Latching of the levels on mode pins when PRESET# is negated,
> +  - Mode monitoring register,
> +  - Reset control of peripheral devices (on R-Car Gen1),
> +  - Watchdog timer (on R-Car Gen1).
> +  - Register-based reset control and boot address registers for the various
> CPU
> +    cores (on R-Car Gen2/Gen3),
> +
> +
> +Required properties:
> +  - compatible: Should be
> +		  - "renesas,<soctype>-reset-wdt" for R-Car Gen1,
> +		  - "renesas,<soctype>-rst" for R-Car Gen2/Gen3.
> +		Examples with soctypes are:
> +		  - "renesas,r8a7778-reset-wdt" (R-Car M1A)
> +		  - "renesas,r8a7779-reset-wdt" (R-Car H1)
> +		  - "renesas,r8a7790-rst" (R-Car H2)
> +		  - "renesas,r8a7791-rst" (R-Car M2-W)
> +		  - "renesas,r8a7792-rst" (R-Car V2H
> +		  - "renesas,r8a7793-rst" (R-Car M2-N)
> +		  - "renesas,r8a7794-rst" (R-Car E2)
> +		  - "renesas,r8a7795-rst" (R-Car H3)
> +		  - "renesas,r8a7796-rst" (R-Car M3-W)

Any specific reason for such a large indentation ? (I know this is really 
nitpicking)

Apart from that, it's just a bit of a shame we can't have generic compatible 
strings, but that would require additional DT properties to describe the reset 
controller features, and I assume that's not a path we want to take.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +  - reg: Address start and address range for the device.
> +
> +
> +Example:
> +
> +	rst: reset-controller at e6160000 {
> +		compatible = "renesas,r8a7795-rst";
> +		reg = <0 0xe6160000 0 0x0200>;
> +	};

-- 
Regards,

Laurent Pinchart

[PATCH/RFC v3 01/22] reset: Add renesas,rst DT bindings

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Jun 2, 2016 at 11:47 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/reset/renesas,rst.txt
>> @@ -0,0 +1,35 @@
>> +DT bindings for the Renesas R-Car Reset Controller
>> +
>> +The R-Car Reset Controller provides reset control, and implements the
>> following
>> +functions:
>> +  - Latching of the levels on mode pins when PRESET# is negated,
>> +  - Mode monitoring register,
>> +  - Reset control of peripheral devices (on R-Car Gen1),
>> +  - Watchdog timer (on R-Car Gen1).
>> +  - Register-based reset control and boot address registers for the various
>> CPU
>> +    cores (on R-Car Gen2/Gen3),
>> +
>> +
>> +Required properties:
>> +  - compatible: Should be
>> +               - "renesas,<soctype>-reset-wdt" for R-Car Gen1,
>> +               - "renesas,<soctype>-rst" for R-Car Gen2/Gen3.
>> +             Examples with soctypes are:
>> +               - "renesas,r8a7778-reset-wdt" (R-Car M1A)
>> +               - "renesas,r8a7779-reset-wdt" (R-Car H1)
>> +               - "renesas,r8a7790-rst" (R-Car H2)
>> +               - "renesas,r8a7791-rst" (R-Car M2-W)
>> +               - "renesas,r8a7792-rst" (R-Car V2H
>> +               - "renesas,r8a7793-rst" (R-Car M2-N)
>> +               - "renesas,r8a7794-rst" (R-Car E2)
>> +               - "renesas,r8a7795-rst" (R-Car H3)
>> +               - "renesas,r8a7796-rst" (R-Car M3-W)
>
> Any specific reason for such a large indentation ? (I know this is really
> nitpicking)

To align the two paragraphs and lists.

> Apart from that, it's just a bit of a shame we can't have generic compatible
> strings, but that would require additional DT properties to describe the reset
> controller features, and I assume that's not a path we want to take.

The reset controller is one of the few blocks that really differs
between members
of the same family, due to the relationship with the various CPU cores.

> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH/RFC v3 02/22] soc: renesas: Add R-Car RST driver

[Dirk Behme]

On 01.06.2016 21:21, Geert Uytterhoeven wrote:
> Add a driver for the Renesas R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3
> RST module.
>
> For now this driver just provides an API to obtain the state of the mode
> pins, as latched at reset time.  As this is typically called from the
> probe function of a clock driver, which can run much earlier than any
> initcall, calling rcar_rst_read_mode_pins() may force an early
> initialization of the driver.
>
> Despite the current simple and almost identical handling for all
> supported SoCs, the driver matches against SoC-specific compatible
> values only, as the features provided by the hardware module differ a
> lot across the various SoC families and members.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  drivers/soc/renesas/Makefile         |  5 ++
>  drivers/soc/renesas/rcar-rst.c       | 94 ++++++++++++++++++++++++++++++++++++
>  include/linux/soc/renesas/rcar-rst.h |  6 +++
>  3 files changed, 105 insertions(+)
>  create mode 100644 drivers/soc/renesas/rcar-rst.c
>  create mode 100644 include/linux/soc/renesas/rcar-rst.h
>
> diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
> index f32a001c65c3ab44..0d732ff893dd8fba 100644
> --- a/drivers/soc/renesas/Makefile
> +++ b/drivers/soc/renesas/Makefile
> @@ -1,3 +1,8 @@
> +obj-$(CONFIG_ARCH_RCAR_GEN1)	+= rcar-rst.o
> +obj-$(CONFIG_ARCH_RCAR_GEN2)	+= rcar-rst.o
> +obj-$(CONFIG_ARCH_R8A7795)	+= rcar-rst.o
> +obj-$(CONFIG_ARCH_R8A7796)	+= rcar-rst.o
> +
>  obj-$(CONFIG_ARCH_R8A7779)	+= rcar-sysc.o r8a7779-sysc.o
>  obj-$(CONFIG_ARCH_R8A7790)	+= rcar-sysc.o r8a7790-sysc.o
>  obj-$(CONFIG_ARCH_R8A7791)	+= rcar-sysc.o r8a7791-sysc.o
> diff --git a/drivers/soc/renesas/rcar-rst.c b/drivers/soc/renesas/rcar-rst.c
> new file mode 100644
> index 0000000000000000..1997c348c0853254
> --- /dev/null
> +++ b/drivers/soc/renesas/rcar-rst.c
> @@ -0,0 +1,94 @@
> +/*
> + * R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3 RST Driver
> + *
> + * Copyright (C) 2016 Glider bvba
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/soc/renesas/rcar-rst.h>
> +
> +struct rst_config {
> +	unsigned int modemr;	/* Mode Monitoring Register Offset */
> +};
> +
> +static const struct rst_config rcar_rst_gen1 __initconst = {
> +	.modemr = 0x20,
> +};
> +
> +static const struct rst_config rcar_rst_gen2 __initconst = {
> +	.modemr = 0x60,
> +};
> +
> +static const struct rst_config rcar_rst_gen3 __initconst = {
> +	.modemr = 0x60,
> +};
> +
> +static const struct of_device_id rcar_rst_matches[] __initconst = {
> +	{ .compatible = "renesas,r8a7778-reset-wdt", .data = &rcar_rst_gen1 },
> +	{ .compatible = "renesas,r8a7779-reset-wdt", .data = &rcar_rst_gen1 },
> +	{ .compatible = "renesas,r8a7790-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7791-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7792-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7793-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7794-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7795-rst", .data = &rcar_rst_gen3 },
> +	{ .compatible = "renesas,r8a7796-rst", .data = &rcar_rst_gen3 },
> +	{ /* sentinel */ }
> +};
> +
> +static void __iomem *rcar_rst_base __initdata;
> +static u32 saved_mode __initdata;
> +
> +static int __init rcar_rst_init(void)
> +{
> +	const struct of_device_id *match;
> +	const struct rst_config *cfg;
> +	struct device_node *np;
> +	void __iomem *base;
> +	int error = 0;
> +
> +	if (rcar_rst_base)
> +		return 0;
> +
> +	np = of_find_matching_node_and_match(NULL, rcar_rst_matches, &match);
> +	if (!np)
> +		return -ENODEV;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_warn("%s: Cannot map regs\n", np->full_name);
> +		error = -ENOMEM;
> +		goto out_put;
> +	}
> +
> +	rcar_rst_base = base;
> +	cfg = match->data;
> +	saved_mode = ioread32(base + cfg->modemr);
> +
> +	pr_debug("%s: MODE = 0x%08x\n", np->full_name, saved_mode);
> +
> +out_put:
> +	of_node_put(np);
> +	return error;
> +}
> +arch_initcall(rcar_rst_init);
> +
> +int __init rcar_rst_read_mode_pins(u32 *mode)


Just a style issue: Is the string 'pins' in the function name still 
relevant? I.e. what's about just 'rcar_rst_read_mode()'?


Best regards

Dirk

[PATCH/RFC v3 02/22] soc: renesas: Add R-Car RST driver

[Geert Uytterhoeven]

Hi Dirk,

On Thu, Jun 2, 2016 at 7:42 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>> +int __init rcar_rst_read_mode_pins(u32 *mode)
>
> Just a style issue: Is the string 'pins' in the function name still
> relevant? I.e. what's about just 'rcar_rst_read_mode()'?

I feel "mode" is a too generic word for a public API.
It's used a several contexts inside the RST module (secure mode, 64-bit
addressing mode, free-running mode, step-up mode).

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH/RFC v3 02/22] soc: renesas: Add R-Car RST driver

[Dirk Behme]

On 10.06.2016 09:58, Geert Uytterhoeven wrote:
> Hi Dirk,
>
> On Thu, Jun 2, 2016 at 7:42 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
>>> +int __init rcar_rst_read_mode_pins(u32 *mode)
>>
>> Just a style issue: Is the string 'pins' in the function name still
>> relevant? I.e. what's about just 'rcar_rst_read_mode()'?
>
> I feel "mode" is a too generic word for a public API.
> It's used a several contexts inside the RST module (secure mode, 64-bit
> addressing mode, free-running mode, step-up mode).

What's about

rcar_rst_read_mode_monitor()

then?

Taken from the manual ;)


Best regards

Dirk

[PATCH/RFC v3 02/22] soc: renesas: Add R-Car RST driver

[Laurent Pinchart]

On Friday 10 Jun 2016 09:58:28 Geert Uytterhoeven wrote:
> Hi Dirk,
> 
> On Thu, Jun 2, 2016 at 7:42 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> >> +int __init rcar_rst_read_mode_pins(u32 *mode)
> > 
> > Just a style issue: Is the string 'pins' in the function name still
> > relevant? I.e. what's about just 'rcar_rst_read_mode()'?
> 
> I feel "mode" is a too generic word for a public API.
> It's used a several contexts inside the RST module (secure mode, 64-bit
> addressing mode, free-running mode, step-up mode).

<bikeshedding>

If it's "pins" that bothers Dirk, how about rcar_rst_read_boot_mode() ? Or 
maybe rcar_rst_boot_mode(), given that the function caches the value, it 
doesn't read it every time.

</bikeshedding>

-- 
Regards,

Laurent Pinchart

[PATCH/RFC v3 02/22] soc: renesas: Add R-Car RST driver

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wednesday 01 Jun 2016 21:21:00 Geert Uytterhoeven wrote:
> Add a driver for the Renesas R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3
> RST module.
> 
> For now this driver just provides an API to obtain the state of the mode
> pins, as latched at reset time.  As this is typically called from the
> probe function of a clock driver, which can run much earlier than any
> initcall, calling rcar_rst_read_mode_pins() may force an early
> initialization of the driver.
> 
> Despite the current simple and almost identical handling for all
> supported SoCs, the driver matches against SoC-specific compatible
> values only, as the features provided by the hardware module differ a
> lot across the various SoC families and members.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - New.
> ---
>  drivers/soc/renesas/Makefile         |  5 ++
>  drivers/soc/renesas/rcar-rst.c       | 94 +++++++++++++++++++++++++++++++++
>  include/linux/soc/renesas/rcar-rst.h |  6 +++
>  3 files changed, 105 insertions(+)
>  create mode 100644 drivers/soc/renesas/rcar-rst.c
>  create mode 100644 include/linux/soc/renesas/rcar-rst.h
> 
> diff --git a/drivers/soc/renesas/Makefile b/drivers/soc/renesas/Makefile
> index f32a001c65c3ab44..0d732ff893dd8fba 100644
> --- a/drivers/soc/renesas/Makefile
> +++ b/drivers/soc/renesas/Makefile
> @@ -1,3 +1,8 @@
> +obj-$(CONFIG_ARCH_RCAR_GEN1)	+= rcar-rst.o
> +obj-$(CONFIG_ARCH_RCAR_GEN2)	+= rcar-rst.o
> +obj-$(CONFIG_ARCH_R8A7795)	+= rcar-rst.o
> +obj-$(CONFIG_ARCH_R8A7796)	+= rcar-rst.o
> +
>  obj-$(CONFIG_ARCH_R8A7779)	+= rcar-sysc.o r8a7779-sysc.o
>  obj-$(CONFIG_ARCH_R8A7790)	+= rcar-sysc.o r8a7790-sysc.o
>  obj-$(CONFIG_ARCH_R8A7791)	+= rcar-sysc.o r8a7791-sysc.o
> diff --git a/drivers/soc/renesas/rcar-rst.c b/drivers/soc/renesas/rcar-rst.c
> new file mode 100644
> index 0000000000000000..1997c348c0853254
> --- /dev/null
> +++ b/drivers/soc/renesas/rcar-rst.c
> @@ -0,0 +1,94 @@
> +/*
> + * R-Car Gen1 RESET/WDT and R-Car Gen2/Gen3 RST Driver
> + *
> + * Copyright (C) 2016 Glider bvba
> + *
> + * This file is subject to the terms and conditions of the GNU General
> Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/of_address.h>
> +#include <linux/soc/renesas/rcar-rst.h>
> +
> +struct rst_config {
> +	unsigned int modemr;	/* Mode Monitoring Register Offset */
> +};
> +
> +static const struct rst_config rcar_rst_gen1 __initconst = {
> +	.modemr = 0x20,
> +};
> +
> +static const struct rst_config rcar_rst_gen2 __initconst = {
> +	.modemr = 0x60,
> +};
> +
> +static const struct rst_config rcar_rst_gen3 __initconst = {
> +	.modemr = 0x60,
> +};
> +
> +static const struct of_device_id rcar_rst_matches[] __initconst = {
> +	{ .compatible = "renesas,r8a7778-reset-wdt", .data = &rcar_rst_gen1 },
> +	{ .compatible = "renesas,r8a7779-reset-wdt", .data = &rcar_rst_gen1 },
> +	{ .compatible = "renesas,r8a7790-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7791-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7792-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7793-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7794-rst", .data = &rcar_rst_gen2 },
> +	{ .compatible = "renesas,r8a7795-rst", .data = &rcar_rst_gen3 },
> +	{ .compatible = "renesas,r8a7796-rst", .data = &rcar_rst_gen3 },
> +	{ /* sentinel */ }
> +};
> +
> +static void __iomem *rcar_rst_base __initdata;
> +static u32 saved_mode __initdata;
> +
> +static int __init rcar_rst_init(void)
> +{
> +	const struct of_device_id *match;
> +	const struct rst_config *cfg;
> +	struct device_node *np;
> +	void __iomem *base;
> +	int error = 0;
> +
> +	if (rcar_rst_base)
> +		return 0;
> +
> +	np = of_find_matching_node_and_match(NULL, rcar_rst_matches, &match);
> +	if (!np)
> +		return -ENODEV;
> +
> +	base = of_iomap(np, 0);
> +	if (!base) {
> +		pr_warn("%s: Cannot map regs\n", np->full_name);
> +		error = -ENOMEM;
> +		goto out_put;
> +	}
> +
> +	rcar_rst_base = base;
> +	cfg = match->data;
> +	saved_mode = ioread32(base + cfg->modemr);
> +
> +	pr_debug("%s: MODE = 0x%08x\n", np->full_name, saved_mode);
> +
> +out_put:
> +	of_node_put(np);
> +	return error;
> +}
> +arch_initcall(rcar_rst_init);

Given that rcar_rst_init() is only used as a support function for 
rcar_rst_read_mode_pins(), how about removing the initcall ?

> +int __init rcar_rst_read_mode_pins(u32 *mode)
> +{
> +	int error;
> +
> +	if (!rcar_rst_base) {
> +		error = rcar_rst_init();
> +		if (error)
> +			return error;
> +	}
> +
> +	*mode = saved_mode;
> +	return 0;
> +}
> diff --git a/include/linux/soc/renesas/rcar-rst.h
> b/include/linux/soc/renesas/rcar-rst.h new file mode 100644
> index 0000000000000000..a18e0783946b66ec
> --- /dev/null
> +++ b/include/linux/soc/renesas/rcar-rst.h
> @@ -0,0 +1,6 @@
> +#ifndef __LINUX_SOC_RENESAS_RCAR_RST_H__
> +#define __LINUX_SOC_RENESAS_RCAR_RST_H__
> +
> +int rcar_rst_read_mode_pins(u32 *mode);
> +
> +#endif /* __LINUX_SOC_RENESAS_RCAR_RST_H__ */

-- 
Regards,

Laurent Pinchart

[PATCH/RFC v3 02/22] soc: renesas: Add R-Car RST driver

[Geert Uytterhoeven]

Hi Laurent,

On Thu, Jun 2, 2016 at 11:58 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> --- /dev/null
>> +++ b/drivers/soc/renesas/rcar-rst.c

>> +static int __init rcar_rst_init(void)
>> +{

[...]

>> +}
>> +arch_initcall(rcar_rst_init);
>
> Given that rcar_rst_init() is only used as a support function for
> rcar_rst_read_mode_pins(), how about removing the initcall ?

Thanks, good idea!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH/RFC v3 14/22] clk: renesas: r8a7795: Obtain mode pin values from R-Car RST driver

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wednesday 01 Jun 2016 21:21:12 Geert Uytterhoeven wrote:
> Obtain the values of the mode pins from the R-Car RST driver, which
> relies on the presence in DT of a device node for the RST module.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3:
>   - New.
> ---
>  drivers/clk/renesas/r8a7795-cpg-mssr.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/renesas/r8a7795-cpg-mssr.c
> b/drivers/clk/renesas/r8a7795-cpg-mssr.c index
> e53aff5b23ad4967..e1f2359a59feee7e 100644
> --- a/drivers/clk/renesas/r8a7795-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a7795-cpg-mssr.c
> @@ -15,6 +15,7 @@
>  #include <linux/device.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/soc/renesas/rcar-rst.h>
> 
>  #include <dt-bindings/clock/r8a7795-cpg-mssr.h>
> 
> @@ -294,7 +295,12 @@ static const struct rcar_gen3_cpg_pll_config
> cpg_pll_configs[16] __initconst = { static int __init
> r8a7795_cpg_mssr_init(struct device *dev)
>  {
>  	const struct rcar_gen3_cpg_pll_config *cpg_pll_config;
> -	u32 cpg_mode = rcar_gen3_read_mode_pins();
> +	u32 cpg_mode;
> +	int error;
> +
> +	error = rcar_rst_read_mode_pins(&cpg_mode);
> +	if (error)
> +		return error;
> 
>  	cpg_pll_config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
>  	if (!cpg_pll_config->extal_div) {

-- 
Regards,

Laurent Pinchart

[PATCH/RFC v3 15/22] clk: renesas: r8a7796: Obtain mode pin values from R-Car RST driver

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wednesday 01 Jun 2016 21:21:13 Geert Uytterhoeven wrote:
> Obtain the values of the mode pins from the R-Car RST driver, which
> relies on the presence in DT of a device node for the RST module.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> v3:
>   - New.
> ---
>  drivers/clk/renesas/r8a7796-cpg-mssr.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/renesas/r8a7796-cpg-mssr.c
> b/drivers/clk/renesas/r8a7796-cpg-mssr.c index
> c84b549c14d2e57d..3db2d1cfc8cd788e 100644
> --- a/drivers/clk/renesas/r8a7796-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a7796-cpg-mssr.c
> @@ -16,6 +16,7 @@
>  #include <linux/device.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
> +#include <linux/soc/renesas/rcar-rst.h>
> 
>  #include <dt-bindings/clock/r8a7796-cpg-mssr.h>
> 
> @@ -159,7 +160,12 @@ static const struct rcar_gen3_cpg_pll_config
> cpg_pll_configs[16] __initconst = { static int __init
> r8a7796_cpg_mssr_init(struct device *dev)
>  {
>  	const struct rcar_gen3_cpg_pll_config *cpg_pll_config;
> -	u32 cpg_mode = rcar_gen3_read_mode_pins();
> +	u32 cpg_mode;
> +	int error;
> +
> +	error = rcar_rst_read_mode_pins(&cpg_mode);
> +	if (error)
> +		return error;
> 
>  	cpg_pll_config = &cpg_pll_configs[CPG_PLL_CONFIG_INDEX(cpg_mode)];
>  	if (!cpg_pll_config->extal_div) {

-- 
Regards,

Laurent Pinchart

[PATCH/RFC v3 16/22] clk: renesas: rcar-gen3-cpg: Remove obsolete rcar_gen3_read_mode_pins()

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Wednesday 01 Jun 2016 21:21:14 Geert Uytterhoeven wrote:
> All R-Car Gen3 clock drivers now obtain the values of the mode pins from
> the R-Car RST driver.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/clk/renesas/rcar-gen3-cpg.c | 17 -----------------
>  drivers/clk/renesas/rcar-gen3-cpg.h |  1 -
>  2 files changed, 18 deletions(-)
> 
> diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c
> b/drivers/clk/renesas/rcar-gen3-cpg.c index
> bb4f2f9a8c2f5ba8..9d76076da4948fc4 100644
> --- a/drivers/clk/renesas/rcar-gen3-cpg.c
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.c
> @@ -333,23 +333,6 @@ struct clk * __init rcar_gen3_cpg_clk_register(struct
> device *dev, __clk_get_name(parent), 0, mult, div);
>  }
> 
> -/*
> - * Reset register definitions.
> - */
> -#define MODEMR	0xe6160060
> -
> -u32 __init rcar_gen3_read_mode_pins(void)
> -{
> -	void __iomem *modemr = ioremap_nocache(MODEMR, 4);
> -	u32 mode;
> -
> -	BUG_ON(!modemr);
> -	mode = ioread32(modemr);
> -	iounmap(modemr);
> -
> -	return mode;
> -}
> -
>  int __init rcar_gen3_cpg_init(const struct rcar_gen3_cpg_pll_config
> *config, unsigned int clk_extalr)
>  {
> diff --git a/drivers/clk/renesas/rcar-gen3-cpg.h
> b/drivers/clk/renesas/rcar-gen3-cpg.h index
> f699085147d1aece..f788f481dd42cdf6 100644
> --- a/drivers/clk/renesas/rcar-gen3-cpg.h
> +++ b/drivers/clk/renesas/rcar-gen3-cpg.h
> @@ -33,7 +33,6 @@ struct rcar_gen3_cpg_pll_config {
> 
>  #define CPG_RCKCR	0x240
> 
> -u32 rcar_gen3_read_mode_pins(void);
>  struct clk *rcar_gen3_cpg_clk_register(struct device *dev,
>  	const struct cpg_core_clk *core, const struct cpg_mssr_info *info,
>  	struct clk **clks, void __iomem *base);

-- 
Regards,

Laurent Pinchart

[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for obtaining mode pin state

[Stephen Boyd]

On 06/01, Geert Uytterhoeven wrote:
> 	Hi all,
> 
> Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the
> state of the mode pins either by a call from the board code, or directly
> by using a hardcoded register access. This is a bit messy, and creates a
> dependency between driver and platform code.
> 
> This RFC patch series converts the various Renesas R-Car clock drivers
> and support code from reading the mode pin states using a hardcoded
> register access to using a new R-Car RST driver.

Dumb question, can we use the nvmem reading APIs instead of an
SoC specific function to read the modes?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for obtaining mode pin state

[Geert Uytterhoeven]

Hi Stephen,

On Thu, Jun 30, 2016 at 10:14 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/01, Geert Uytterhoeven wrote:
>> Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the
>> state of the mode pins either by a call from the board code, or directly
>> by using a hardcoded register access. This is a bit messy, and creates a
>> dependency between driver and platform code.
>>
>> This RFC patch series converts the various Renesas R-Car clock drivers
>> and support code from reading the mode pin states using a hardcoded
>> register access to using a new R-Car RST driver.
>
> Dumb question, can we use the nvmem reading APIs instead of an
> SoC specific function to read the modes?

Thanks for your suggestion, the nvmem API indeed looks like a suitable API,
as it does support read-only nvmems.

Unfortunately I also see a few disadvantages:
  1. nvmem_init() is a subsys_initcall(), while most of our users (except for
     the recent renesas-cpg-mssr driver) are clock drivers using
     CLK_OF_DECLARE(), and are thus initialized from of_clk_init() at much
     earlier time_init() time.
     Of course the mvmem subsystem and/or the clock drivers can be changed, if
     deemed useful.
  2. Using the nvmem DT bindings means we have to add more DT glue from the
     nvmem consumer(s) to the nvmem provider. As we need to provide backwards
     compatibility with old DTSes, this means we need more C code or DT fixup
     code to handle that.
  3. The nvmem subsystem may be overkill to provide access to a simple 32-bit
     read-only register that never changes value after boot.

Thoughts?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for obtaining mode pin state

[Stephen Boyd]

On 09/01, Geert Uytterhoeven wrote:
> Hi Stephen,
> 
> On Thu, Jun 30, 2016 at 10:14 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> > On 06/01, Geert Uytterhoeven wrote:
> >> Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the
> >> state of the mode pins either by a call from the board code, or directly
> >> by using a hardcoded register access. This is a bit messy, and creates a
> >> dependency between driver and platform code.
> >>
> >> This RFC patch series converts the various Renesas R-Car clock drivers
> >> and support code from reading the mode pin states using a hardcoded
> >> register access to using a new R-Car RST driver.
> >
> > Dumb question, can we use the nvmem reading APIs instead of an
> > SoC specific function to read the modes?
> 
> Thanks for your suggestion, the nvmem API indeed looks like a suitable API,
> as it does support read-only nvmems.
> 
> Unfortunately I also see a few disadvantages:
>   1. nvmem_init() is a subsys_initcall(), while most of our users (except for
>      the recent renesas-cpg-mssr driver) are clock drivers using
>      CLK_OF_DECLARE(), and are thus initialized from of_clk_init() at much
>      earlier time_init() time.
>      Of course the mvmem subsystem and/or the clock drivers can be changed, if
>      deemed useful.

Sounds like this is solvable.

>   2. Using the nvmem DT bindings means we have to add more DT glue from the
>      nvmem consumer(s) to the nvmem provider. As we need to provide backwards
>      compatibility with old DTSes, this means we need more C code or DT fixup
>      code to handle that.

Ah I wasn't aware we were keeping backwards compatibility around.


>   3. The nvmem subsystem may be overkill to provide access to a simple 32-bit
>      read-only register that never changes value after boot.

The nvmem subsystem is designed to read values from things that
mostly never change. Overkill may be true, but the nice thing
about using nvmem APIs is that the driver doesn't have to use
some platform specific function that duplicates similar
functionality. It's unfortunate that backwards incompatibility
limits our ability to move to common linux frameworks when they
are created after the binding is used.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH/RFC v3 00/22] soc: renesas: Add R-Car RST driver for obtaining mode pin state

[Geert Uytterhoeven]

Hi Stephen,

On Tue, Sep 13, 2016 at 12:16 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 09/01, Geert Uytterhoeven wrote:
>> On Thu, Jun 30, 2016 at 10:14 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>> > On 06/01, Geert Uytterhoeven wrote:
>> >> Currently the R-Car Clock Pulse Generator (CPG) drivers obtains the
>> >> state of the mode pins either by a call from the board code, or directly
>> >> by using a hardcoded register access. This is a bit messy, and creates a
>> >> dependency between driver and platform code.
>> >>
>> >> This RFC patch series converts the various Renesas R-Car clock drivers
>> >> and support code from reading the mode pin states using a hardcoded
>> >> register access to using a new R-Car RST driver.
>> >
>> > Dumb question, can we use the nvmem reading APIs instead of an
>> > SoC specific function to read the modes?
>>
>> Thanks for your suggestion, the nvmem API indeed looks like a suitable API,
>> as it does support read-only nvmems.
>>
>> Unfortunately I also see a few disadvantages:
>>   1. nvmem_init() is a subsys_initcall(), while most of our users (except for
>>      the recent renesas-cpg-mssr driver) are clock drivers using
>>      CLK_OF_DECLARE(), and are thus initialized from of_clk_init() at much
>>      earlier time_init() time.
>>      Of course the mvmem subsystem and/or the clock drivers can be changed, if
>>      deemed useful.
>
> Sounds like this is solvable.

Sure.

>>   2. Using the nvmem DT bindings means we have to add more DT glue from the
>>      nvmem consumer(s) to the nvmem provider. As we need to provide backwards
>>      compatibility with old DTSes, this means we need more C code or DT fixup
>>      code to handle that.
>
> Ah I wasn't aware we were keeping backwards compatibility around.
>
>>   3. The nvmem subsystem may be overkill to provide access to a simple 32-bit
>>      read-only register that never changes value after boot.
>
> The nvmem subsystem is designed to read values from things that
> mostly never change. Overkill may be true, but the nice thing
> about using nvmem APIs is that the driver doesn't have to use
> some platform specific function that duplicates similar
> functionality. It's unfortunate that backwards incompatibility
> limits our ability to move to common linux frameworks when they
> are created after the binding is used.

This is continuing work to support old, current, and new SoCs properly.
When the first support for R-Car Gen2 SoCs was added, many frameworks
didn't have DT support, or didn't even exist.

Unlike in the mobile market, development and life span is much larger than
6 months here...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds