Re: [PATCH 0/2 v3] Add pl031 RTC support for Hi6220

[Arnd Bergmann]

On Thursday, July 7, 2016 7:10:30 PM CEST Michael Turquette wrote:
> Quoting Arnd Bergmann (2016-07-07 01:13:58)
> > On Wednesday, July 6, 2016 5:19:53 PM CEST Michael Turquette wrote:
> > > On Wed, Jul 6, 2016 at 12:38 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > > > On Wednesday, July 6, 2016 12:20:15 AM CEST John Stultz wrote:
> > > > > On Wed, Jul 6, 2016 at 12:04 AM, Olof Johansson <olof@xxxxxxxxx> wrote:
> > > > > > On Tue, Jul 5, 2016 at 11:55 PM, John Stultz <john.stultz@xxxxxxxxxx> wrote:
> > > > > >> On Tue, Jul 5, 2016 at 10:22 PM, Olof Johansson <olof@xxxxxxxxx> wrote:
> > > > > >>> On Wed, Jun 29, 2016 at 05:48:43PM -0700, John Stultz wrote:
> > > > The question this raises is why that clock was missed the first time
> > > > around. I'd suggest whoever owns the clock driver can go through the
> > > > documentation again and look for others that may have been missed,
> > > > then send a patch to the driver to add *all* the missing ones for the
> > > > merge window, and one release later we add the driver depending on
> > > > previously unknown clocks.
> > > 
> > > Well, I'm kicking the ant pile on this one, but sometimes the above
> > > suggestion is not possible. I'm currently hacking on a platform with
> > > very limited docs, so I cannot understand the whole clock tree, nor
> > > how all peripherals are wired up to it.
> > 
> > That's clearly not the case here though: the hi6220 clk driver
> > was contributed by hisilicon engineers that have all the documentation.
> > 
> > > Further complicating matters is that fact that any headers in the DT
> > > include chroot constitute an unbreakable ABI that shall stand for
> > > 1,000 years at least, so I'm very remiss to dump a bunch of constant
> > > values in there with names that might need to change at a later date.
> > 
> > Can you give an example why they might need to change?
> 
> Yes, the AmLogic GXBB clk driver that I just merged is a great example.
> The names of the clock signals enumerated in the header will surely
> change as our understanding of the hardware changes. These names are
> part of the ABI and yet we cannot possibly get them right on the first
> pass.

I see. Actually the names are not part of the ABI, just the numeric
values are. Changing the names is of course still a pain, but it's
not nearly as bad as adding further constants, which requires coordinating
between the dts files and the driver.

> > Usually the
> > hardware doesn't change. There is also a risk of having a driver
> 
> It's not about the hardware changing, but our incomplete understanding
> of the hardware will certainly change.
> 
> > binding that makes sense for the first 10 clocks that get added,
> > and then it later turns out that the chip actually has hundreds
> > of clocks that could really use a completely different binding
> > to allow a simpler driver.
> 
> Can you give an example of this scenario?

My impression here was that the include/dt-bindings/clock/hi6220-clock.h
was such a case: it started out with the clocks that were initially
needed, and then the RTC driver was added and it happened to need
additional clocks. My conclusion from this was that the initial
set of clocks did not even cover the essential devices and that there
would be further additions required once we add more drivers.

However, as 	Guodong Xu said, this was just a mistake and we won't
need to make any other changes to this file in the future, so my rant
was probably misplaced here.

A better example might be include/dt-bindings/clock/exynos5410.h:
In this case, we have already had four separate patches adding
additional clocks after the driver was first written (by folks
that have direct access to the documentation and to the hardware
developers), and they even left space for the new numbers in
the initial file, so it's not like they didn't know there was
something else.

	Arnd