LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Geert Uytterhoeven <geert@linux-m68k.org>
To: M P <buserror@gmail.com>
Cc: Michel Pollet <michel.pollet@bp.renesas.com>,
	Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
	Simon Horman <horms@verge.net.au>,
	Phil Edworthy <phil.edworthy@renesas.com>,
	Michel Pollet <buserror+upstream@gmail.com>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	linux-clk <linux-clk@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
	<devicetree@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v7 1/5] dt-bindings: Add the r9a06g032-sysctrl.h file
Date: Thu, 31 May 2018 11:32:52 +0200	[thread overview]
Message-ID: <CAMuHMdXQWSVh37HqrvFWeLNHZ5zNh0=9+hV0_nsQ9mi1HQSCaQ@mail.gmail.com> (raw)
In-Reply-To: <CAMMfpEwpiZnZEykzxQJMTwE+cDnOD_qA3nUzyB96DGTWPzSJrw@mail.gmail.com>

Hi Michel,

On Thu, May 31, 2018 at 11:11 AM, M P <buserror@gmail.com> wrote:
> On Fri, 25 May 2018 at 11:31, Geert Uytterhoeven <geert@linux-m68k.org>
> wrote:
>> On Thu, May 24, 2018 at 11:28 AM, Michel Pollet
>> <michel.pollet@bp.renesas.com> wrote:
>> > This adds the constants necessary to use the renesas,r9a06g032-sysctrl
> node.

>> > @@ -0,0 +1,187 @@
>> > +/* SPDX-License-Identifier: GPL-2.0 */
>> > +/*
>> > + * R9A06G032 sysctrl IDs
>> > + *
>> > + * Copyright (C) 2018 Renesas Electronics Europe Limited
>> > + *
>> > + * Michel Pollet <michel.pollet@bp.renesas.com>, <buserror@gmail.com>
>> > + * Derived from zx-reboot.c
>> > + */
>> > +
>> > +#ifndef __DT_BINDINGS_R9A06G032_SYSCTRL_H__
>> > +#define __DT_BINDINGS_R9A06G032_SYSCTRL_H__
>> > +
>> > +#define R9A06G032_CLKOUT                       0
>> > +#define R9A06G032_CLK_PLL_USB                  1
>> > +#define R9A06G032_CLK_48                       1 /* AKA CLK_PLL_USB */
>> > +#define R9A06G032_CLKOUT_D10                   2
>> > +#define R9A06G032_CLKOUT_D16                   3
>> > +#define R9A06G032_MSEBIS_CLK                   3 /* AKA CLKOUT_D16 */
>> > +#define R9A06G032_MSEBIM_CLK                   3 /* AKA CLKOUT_D16 */
>> > +#define R9A06G032_CLKOUT_D160                  4
>> > +#define R9A06G032_CLKOUT_D1OR2                 5
>> > +#define R9A06G032_CLK_DDRPHY_PLLCLK            5 /* AKA CLKOUT_D1OR2 */
>
>> [...]
>
>> I have 3 comments:
>
>>    1. I had expected this list to match (both name- and order-wise)
> Appendix
>>       C ("Clock Tree Structure") in the RZ/N1[DSL] User’s Manual: System
>>       Introduction, Multiplexing, Electrical and Mechanical Information.
>>       That would make it easier to review.
>
> Well, that document was made a *long* time after the internal documentation
> we used to generate the clock lists. There are a few things we had to do:
>
> * Renumber peripherals. We decided a long time ago that u-boot and linux
> would stick to zero based peripherals, and not one based numbers. It's next
> to impossible to keep mixing number based across software base, so we use
> UART0 while the hardware manual mentions UART1 -- It *is* documented
> extensively with out SDK, and makes life using linux a lot easier. It's
> across all our SDK, u-boot, webapps readmes, howtos etc.
>
> * Rename some peripherals. Plenty of peripherals names made little sense
> and had zero consistency, in fact, many names were different depending on
> the place they were used! -- "flashnand"+"nand_flash"+"FNAND" etc,
> "QUADSPI"+"QSPI" etc etc so we also re-normalized the names to match linux
> conventions.
>
> * Other internal reasons I can't document here
>
> Also, the value here are made up anyway -- so I've decided to sort them to
> make sure any clock that has a parent is numbered *after* the parent...

Well, all of that combines makes it very hard for us to review the list.

>>    2. These definitions seems to be a mix of:
>>         1. Internal core clocks,
>>         2. Other core clocks,
>>         3. Module clocks.
>
>>       The internal clocks do not need to be referenced from DT, so I would
>>       not make them part of the DT bindings.
>
> Why? I'm told that "Bindings aren't for linux" -- why can't I imagine
> 'something' needing them? Why would I decide NOT to include them,
> as they are there? I 'factored' a few of them to the same number when

Just general safety w.r.t. unchangeable DT definitions: anything that is
not listed here cannot be declared wrong later.

> applicable.

You're 100% sure they're the same?

> This is all done automatically BTW -- a script takes the original clocking
> spreadsheet, and converts it into a table, correcting 'human input' as it
> goes along.

So the internal spreadsheet doesn't match the public documentation neither?

>>    3. It looks like the module clocks can be referred to by register offset
>>       and bit position, which is similar to how this is handled on R-Car
>>       SoCs.
>>       Perhaps you can just drop the definitions for these from the header
>>       file, and refer to them by (combined) register offset and bit
> position
>>       instead?
>>       Or am I missing something?
>>       I believe this can also be done for the module resets (later).
>
> If you look in the .c file, you'll see that most gate have not just one
> register/bit pair associated with them -- there are several, spread
> across registers.. Also, there are clocks in there with two gates,
> or none. Given that, I've decided a separate numbering
> made sense.

OK, fair enough.

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

  reply	other threads:[~2018-05-31  9:32 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-24  9:28 [PATCH v7 0/5] arm: Base support for Renesas RZN1D-DB Board Michel Pollet
2018-05-24  9:28 ` [PATCH v7 1/5] dt-bindings: Add the r9a06g032-sysctrl.h file Michel Pollet
2018-05-25 10:31   ` Geert Uytterhoeven
2018-05-31  9:11     ` M P
2018-05-31  9:32       ` Geert Uytterhoeven [this message]
2018-05-31 10:01         ` M P
2018-06-01  8:18           ` Geert Uytterhoeven
2018-05-24  9:28 ` [PATCH v7 2/5] dt-bindings: clock: renesas,r9a06g032-sysctrl: documentation Michel Pollet
2018-05-25  9:23   ` Geert Uytterhoeven
2018-05-31 10:16     ` M P
2018-06-01  8:22       ` Geert Uytterhoeven
2018-05-24  9:28 ` [PATCH v7 3/5] ARM: dts: Renesas R9A06G032 base device tree file Michel Pollet
2018-05-25  9:27   ` Geert Uytterhoeven
2018-05-28  9:15     ` Simon Horman
2018-05-24  9:28 ` [PATCH v7 4/5] ARM: dts: Renesas RZN1D-DB Board base file Michel Pollet
2018-05-25  9:28   ` Geert Uytterhoeven
2018-05-24  9:28 ` [PATCH v7 5/5] clk: renesas: Renesas R9A06G032 clock driver Michel Pollet
2018-05-26 13:45   ` kbuild test robot
2018-05-30 19:49   ` Geert Uytterhoeven

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAMuHMdXQWSVh37HqrvFWeLNHZ5zNh0=9+hV0_nsQ9mi1HQSCaQ@mail.gmail.com' \
    --to=geert@linux-m68k.org \
    --cc=buserror+upstream@gmail.com \
    --cc=buserror@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=geert+renesas@glider.be \
    --cc=horms@verge.net.au \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=michel.pollet@bp.renesas.com \
    --cc=mturquette@baylibre.com \
    --cc=phil.edworthy@renesas.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --subject='Re: [PATCH v7 1/5] dt-bindings: Add the r9a06g032-sysctrl.h file' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).