Re: [PATCH v1 11/19] dt-bindings: reset: npcm: Add support for NPCM8XX

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

Hi Krzysztof,

On Mon, May 23, 2022 at 4:26 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> On 23/05/2022 16:22, Geert Uytterhoeven wrote:
> > On Mon, May 23, 2022 at 4:03 PM Tomer Maimon <tmaimon77@gmail.com> wrote:
> >> On Mon, 23 May 2022 at 12:01, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >>> On 22/05/2022 17:50, Tomer Maimon wrote:
> >>>> Add binding document and device tree binding
> >>>> constants for Nuvoton BMC NPCM8XX reset controller.
> >>>>
> >>>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com>
> >
> >>>> --- /dev/null
> >>>> +++ b/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h
> >>>> @@ -0,0 +1,124 @@
> >>>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>>> +// Copyright (c) 2022 Nuvoton Technology corporation.
> >>>> +
> >>>> +#ifndef _DT_BINDINGS_NPCM8XX_RESET_H
> >>>> +#define _DT_BINDINGS_NPCM8XX_RESET_H
> >>>> +
> >>>> +#define NPCM8XX_RESET_IPSRST1                0x20
> >>>> +#define NPCM8XX_RESET_IPSRST2                0x24
> >>>> +#define NPCM8XX_RESET_IPSRST3                0x34
> >>>> +#define NPCM8XX_RESET_IPSRST4                0x74
> >>>
> >>> What are these? All IDs should be incremental, decimal and start from 0.
> >>
> >> Register offset, we use the same method in NPCM7xx. please refer
> >> https://elixir.bootlin.com/linux/v5.18/source/include/dt-bindings/reset/nuvoton,npcm7xx-reset.h
> >>
> >> and the driver asserts the reset according to the reset include definitions
> >
> > So if they're easy to look up the values, you could do without the
> > definitions? Cfr. the interrupts properties in .dtsi files, where we
> > typically just use the hardcoded numbers.
> >
> > If you do decide to keep them, a comment explaining their origins
> > would be useful.
> >
> >>>> +
> >>>> +/* Reset lines on IP1 reset module (NPCM8XX_RESET_IPSRST1) */
> >>>> +#define NPCM8XX_RESET_GDMA0          3
> >>>
> >>> IDs start from 0 and do not have holes.
> >>
> >> This represents the reset BIT in the reset register.
> >
> > Likewise, I think it's a good idea to document that in a comment, cfr.
> > https://elixir.bootlin.com/linux/v5.18/source/include/dt-bindings/power/r8a7795-sysc.h#L8
>
> Renesas is also doing it not correct (just like many others). The
> bindings are not for register bits or offsets. Such data can be DTS but
> not part of bindings.

I think you are taking a too-extremist standpoint.
The two extremes are:
  1. Numbers correspond to hardware numbers, and are easy to look up
    in the hardware documentation (e.g. GIC SPI interrupt numbers).
     => Use the hardcoded numbers in DTS.
  2. Numbers do not correspond to hardware numbers, so we had to
     invent our own definitions and numbers, usually loosely
     based on some table in the hardware documentation.
     The driver will have to look up the numbers in a data
     structure, to know how to program the hardware.
     The numbers become part of the DT ABI, and cannot be changed
     (header file is append-only).
     => Use the binding definitions in DTS.

We are taking the middle ground: there is a one-to-one relation between
numbers and hardware numbers that can be looked up in or derived from
the hardware documentation, but the conversion is non-trivial (for the
casual human reviewer), or the documentation refers to names instead
of numbers in most sections (e.g. named power domains). Then why not
let the numbers match some feature in the hardware (e.g. register
offset or register bit)?

> Imagine now you made mistake in this register
> offset and hardware uses slightly different value. What now? Change
> bindings? No. Bindings hold here ID, the abstraction, and ID stays fixed.

I see no difference here with using the wrong interrupt number in an
interrupts property in DTS.  What do we do in that case? Fix the DTS.

BTW, are you aware of any driver that transforms interrupt numbers
obtained from DTS, because the DTS used the wrong number?

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