Re: [PATCH v2 2/4] clk: renesas: Add r8a7796 CPG Core Clock Definitions

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Dirk,

On Thu, Jun 9, 2016 at 10:31 AM, Dirk Behme <dirk.behme@de.bosch.com> wrote:
> On 07.06.2016 10:17, Geert Uytterhoeven wrote:
>> On Tue, Jun 7, 2016 at 9:53 AM, Dirk Behme <dirk.behme@de.bosch.com>
>> wrote:
>>>
>>> I think I just want to discuss if we have a clever idea to further
>>> improve
>>> one detail. That is, if we have a clever idea to avoid the copy & paste
>>> between the family members using anything like a hierarchical way of
>>> defining the clocks in r8a779x-cpg-mssr.h.
>>>
>>>> Given the small amount of work needed to bootstrap r8a7796, I
>>>> think they still hold on their promises.
>>>
>>> Well, regarding the r8a779x-cpg-mssr.h the small amount of work needed
>>> isn't
>>> a really good argument if you are good with cp & sed for the copy & paste
>>> done ;)
>>
>> They're not really created by cp & sed, as they must match the table in
>> the
>> datasheet (the latter may have been created by copy & paste though :-)
>>
>>> What I fear we end up the way we are doing the copy & paste
>>> r8a779x-cpg-mssr.h is having 5 or 6 or even more of these files, all >
>>> 90%
>>> identical. And once you have to change anything, you either have to
>>> change
>>> all these files. Or you miss anything, ending up with subtle bugs when
>>> one
>>> SoC does behave differently than an other one.
>>
>> The point is these files are stable ABI: no single value can be changed.
>> No value can be reused. Only new values can be appended at the bottom
>> (if a newer revision of the datasheet documents more clocks than the old
>>  version, which happens from time to time).
>>
>> IMHO a hierarchical way of defining the clocks has more opportunity of
>> accidentally referring to a clock that doesn't exist on a particular SoC.
>>
>> Furthermore, r8a779x-cpg-mssr.h is not a good name to be part of a DT
>> binding,
>> due to the wildcard.
>> A future SoC may will match r8a779x and even be called (R-Car
>> <something>3?),
>> while using a completely different CPG block.
>
> Just to give an example about what we are talking, I've done [1] below. This
> should just be an *example*, though. I'm sure that we might need a more
> clever way.

Thanks, it matches with what I had in mind what you wanted to do ;-)

> --- a/include/dt-bindings/clock/r8a7796-cpg-mssr.h
> +++ b/include/dt-bindings/clock/r8a7796-cpg-mssr.h

> +#include <dt-bindings/clock/rcar3-cpg-mssr.h>

> +#define R8A7796_CLK_ZB3D4              47
> +#define R8A7796_CLK_S0D2               48
> +#define R8A7796_CLK_S0D3               49
> +#define R8A7796_CLK_S0D6               50
> +#define R8A7796_CLK_S0D8               51
> +#define R8A7796_CLK_S0D12              53

This means on M3-W, clocks may have different prefixes:
  - RCAR3_CLK_* if they are defined as common R-Car Gen3 clocks,
  - R8A7796_CLK_* if they are defined as M3-W-specific clocks.

Not such a big issue, it that can be worked around using

    #define R8A7796_CLK_Z RCAR3_CLK_Z
    [...]

> +++ b/include/dt-bindings/clock/rcar3-cpg-mssr.h
> @@ -0,0 +1,63 @@
> +/*
> + * Copyright (C) 2015 Renesas Electronics Corp.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#ifndef __DT_BINDINGS_CLOCK_RCAR3_CPG_MSSR_H__
> +#define __DT_BINDINGS_CLOCK_RCAR3_CPG_MSSR_H__
> +
> +#include <dt-bindings/clock/renesas-cpg-mssr.h>
> +
> +/* RCar3 CPG Core Clocks */
> +#define RCAR3_CLK_Z            0
> +#define RCAR3_CLK_Z2           1
> +#define RCAR3_CLK_ZR           2
> +#define RCAR3_CLK_ZG           3
> +#define RCAR3_CLK_ZTR          4
> +#define RCAR3_CLK_ZTRD2                5

While this approach allows to add SoC-specific clocks to the family-specific
base list, it does not allow to remove family-specific clocks that are not
present on unknown future species of the family.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@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