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

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 23/05/2022 16:03, Tomer Maimon wrote:
> Hi Krzysztof,
> 
> Thanks for your comments.

Please stop replying in HTML. It's not the format of emails used in the
Linux. It makes very difficult to read your replies.

> 
> 
> On Mon, 23 May 2022 at 12:01, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org <mailto: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
>     <mailto:tmaimon77@gmail.com>>
>     > ---
>     >  .../bindings/reset/nuvoton,npcm-reset.txt     |  17 ++-
>     >  .../dt-bindings/reset/nuvoton,npcm8xx-reset.h | 124
>     ++++++++++++++++++
>     >  2 files changed, 139 insertions(+), 2 deletions(-)
>     >  create mode 100644 include/dt-bindings/reset/nuvoton,npcm8xx-reset.h
>     >
>     > diff --git
>     a/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.txt
>     b/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.txt
>     > index cb1613092ee7..b7eb8615b68b 100644
>     > --- a/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.txt
>     > +++ b/Documentation/devicetree/bindings/reset/nuvoton,npcm-reset.txt
>     > @@ -1,14 +1,15 @@
>     >  Nuvoton NPCM Reset controller
>     > 
>     >  Required properties:
>     > -- compatible : "nuvoton,npcm750-reset" for NPCM7XX BMC
>     > +- compatible : "nuvoton,npcm750-reset" for Poleg NPCM7XX BMC.
>     > +               "nuvoton,npcm845-reset" for Arbel NPCM8XX BMC.
>     >  - reg : specifies physical base address and size of the register.
>     >  - #reset-cells: must be set to 2
>     >  - syscon: a phandle to access GCR registers.
>     > 
>     >  Optional property:
>     >  - nuvoton,sw-reset-number - Contains the software reset number to
>     restart the SoC.
>     > -  NPCM7xx contain four software reset that represent numbers 1 to 4.
>     > +  NPCM7xx and NPCM8xx contain four software reset that represent
>     numbers 1 to 4.
>     > 
>     >    If 'nuvoton,sw-reset-number' is not specified software reset is
>     disabled.
>     > 
>     > @@ -32,3 +33,15 @@ example:
>     >          };
>     > 
>     >  The index could be found in
>     <dt-bindings/reset/nuvoton,npcm7xx-reset.h>.
>     > +
>     > +Specifying reset lines connected to IP NPCM8XX modules
>     > +======================================================
> 
> we prefer to use the same explanation as the NPCM7XX reset explanation
> in the reset binding document.

??

> 
>     No need to document consumers. Just mention the header.

What explanation? Consumers are trivial. Once you convert it to DT
schema there should be no such code at all.

> 
>     > +example:
>     > +
>     > +        spi0: spi@..... {
>     > +                ...
>     > +                resets = <&rstc NPCM8XX_RESET_IPSRST2
>     NPCM8XX_RESET_PSPI1>;
>     > +                ...
>     > +        };
>     > +
>     > +The index could be found in
>     <dt-bindings/reset/nuvoton,npcm8xx-reset.h>.
>     > diff --git a/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h
>     b/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h
>     > new file mode 100644
>     > index 000000000000..4b832a0fd1dd
>     > --- /dev/null
>     > +++ b/include/dt-bindings/reset/nuvoton,npcm8xx-reset.h
>     > @@ -0,0 +1,124 @@
>     > +/* SPDX-License-Identifier: GPL-2.0 */
> 
>     Dual license.
> 
> O.K. 
> 
> 
>     > +// 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
> <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 

Register offsets, a device programming model, are not part of bindings.
 Bindings should be independent of programming model, so only IDs are
allowed.

Why did you add register offsets to bindings at the first place?

> 
> 
>     > +
>     > +/* 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. 

Again, not programming model in the bindings. No bits, not register
values, no register offsets.


Best regards,
Krzysztof