LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Geert Uytterhoeven <email@example.com>
To: M P <firstname.lastname@example.org>
Cc: Michel Pollet <email@example.com>,
Simon Horman <firstname.lastname@example.org>,
Phil Edworthy <email@example.com>,
Michel Pollet <firstname.lastname@example.org>,
Michael Turquette <email@example.com>,
Stephen Boyd <firstname.lastname@example.org>, Rob Herring <email@example.com>,
Mark Rutland <firstname.lastname@example.org>,
Geert Uytterhoeven <email@example.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
Linux Kernel Mailing List <firstname.lastname@example.org>
Subject: Re: [PATCH v7 1/5] dt-bindings: Add the r9a06g032-sysctrl.h file
Date: Fri, 1 Jun 2018 10:18:35 +0200 [thread overview]
Message-ID: <CAMuHMdUa4UFOsMb1z9aBdCOW1YmrnLOQ866TCOCfmvEu3H3Dxg@mail.gmail.com> (raw)
On Thu, May 31, 2018 at 12:01 PM, M P <email@example.com> wrote:
> On Thu, 31 May 2018 at 10:32, Geert Uytterhoeven <firstname.lastname@example.org> wrote:
>> On Thu, May 31, 2018 at 11:11 AM, M P <email@example.com> wrote:
>> > On Fri, 25 May 2018 at 11:31, Geert Uytterhoeven <firstname.lastname@example.org>
>> > wrote:
>> >> On Thu, May 24, 2018 at 11:28 AM, Michel Pollet
>> >> <email@example.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 <firstname.lastname@example.org>, <email@example.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.
> I understand -- the previous format was a lot more readable, here I had to
> work to make the table as small as I could, and quite a few bits of readability
> were lost in the process. It's already a huge file.
> I'm not sure if that would help, but here is the link to the old table
> format I used
> I had to throw that away to make up the new composite table that now contains
> what was in the device tree file before. However the names are the same.
> Perhaps I could change how I encode the register/bit pairs to use 8+8 bits to
> make the hex constants more readable?
Alternatively, you can use a macro to pack them, cfr. MOD_CLK_PACK()
>> >> 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.
> Well, I need these #define *somewhere* as I use them in my driver. So what
> do you want me to do?
> + Remove the header, move the constants into the driver?
> + Use hard coded numbers in the DT and remove the header entirely?
> + Duplicate the header, keep the full one with the driver, and only list
> the CA7+UART0 one in the dt-binding and duplicate the ones I need as
> I go along?
None of these. You can just move the #defines that will never be needed
from DT into the driver source file.
Cfr. "Internal Core Clocks" in drivers/clk/renesas/r8a7795-cpg-mssr.c.
>> > applicable.
>> You're 100% sure they're the same?
> Yes, the script logic hasn't changed in 2 years, and we've been using that
> hierarchy extensively since.
> Basically the logic is that if a clock doesn't have a gate and isn't a divisor
> of some sort, it's factored with it's parent clock... it used to be
> done with a DT
> alias of the same name.
>> > 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?
> Nope, as usual, people who wrote the documentation went their own way at
> some point, and didn't backport any changes they made.
Well, I guess we'll have to live with that...
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- firstname.lastname@example.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
next prev parent reply other threads:[~2018-06-01 8:18 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
2018-05-31 10:01 ` M P
2018-06-01 8:18 ` Geert Uytterhoeven [this message]
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
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='Re: [PATCH v7 1/5] dt-bindings: Add the r9a06g032-sysctrl.h file' \
* 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).