| Message ID | 20220522155046.260146-12-tmaimon77@gmail.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand |
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> > --- > .../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 > +====================================================== No need to document consumers. Just mention the header. > +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. > +// 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. > + > +/* Reset lines on IP1 reset module (NPCM8XX_RESET_IPSRST1) */ > +#define NPCM8XX_RESET_GDMA0 3 IDs start from 0 and do not have holes. > +#define NPCM8XX_RESET_UDC1 5 > +#define NPCM8XX_RESET_GMAC3 6 > +#define NPCM8XX_RESET_UART_2_3 7 > +#define NPCM8XX_RESET_UDC2 8 > +#define NPCM8XX_RESET_PECI 9 > +#define NPCM8XX_RESET_AES 10 > +#define NPCM8XX_RESET_UART_0_1 11 > +#define NPCM8XX_RESET_MC 12 > +#define NPCM8XX_RESET_SMB2 13 > +#define NPCM8XX_RESET_SMB3 14 > +#define NPCM8XX_RESET_SMB4 15 > +#define NPCM8XX_RESET_SMB5 16 > +#define NPCM8XX_RESET_PWM_M0 18 > +#define NPCM8XX_RESET_TIMER_0_4 19 > +#define NPCM8XX_RESET_TIMER_5_9 20 > +#define NPCM8XX_RESET_GMAC4 21 > +#define NPCM8XX_RESET_UDC4 22 > +#define NPCM8XX_RESET_UDC5 23 > +#define NPCM8XX_RESET_UDC6 24 > +#define NPCM8XX_RESET_UDC3 25 > +#define NPCM8XX_RESET_ADC 27 > +#define NPCM8XX_RESET_SMB6 28 > +#define NPCM8XX_RESET_SMB7 29 > +#define NPCM8XX_RESET_SMB0 30 > +#define NPCM8XX_RESET_SMB1 31 > + > +/* Reset lines on IP2 reset module (NPCM8XX_RESET_IPSRST2) */ > +#define NPCM8XX_RESET_MFT0 0 > +#define NPCM8XX_RESET_MFT1 1 > +#define NPCM8XX_RESET_MFT2 2 > +#define NPCM8XX_RESET_MFT3 3 > +#define NPCM8XX_RESET_MFT4 4 > +#define NPCM8XX_RESET_MFT5 5 > +#define NPCM8XX_RESET_MFT6 6 > +#define NPCM8XX_RESET_MFT7 7 > +#define NPCM8XX_RESET_MMC 8 > +#define NPCM8XX_RESET_GFX_SYS 10 > +#define NPCM8XX_RESET_AHB_PCIBRG 11 > +#define NPCM8XX_RESET_VDMA 12 > +#define NPCM8XX_RESET_ECE 13 > +#define NPCM8XX_RESET_VCD 14 > +#define NPCM8XX_RESET_VIRUART1 16 > +#define NPCM8XX_RESET_VIRUART2 17 > +#define NPCM8XX_RESET_SIOX1 18 > +#define NPCM8XX_RESET_SIOX2 19 > +#define NPCM8XX_RESET_BT 20 > +#define NPCM8XX_RESET_3DES 21 > +#define NPCM8XX_RESET_PSPI2 23 > +#define NPCM8XX_RESET_GMAC2 25 > +#define NPCM8XX_RESET_USBH1 26 > +#define NPCM8XX_RESET_GMAC1 28 > +#define NPCM8XX_RESET_CP1 31 > + > +/* Reset lines on IP3 reset module (NPCM8XX_RESET_IPSRST3) */ > +#define NPCM8XX_RESET_PWM_M1 0 > +#define NPCM8XX_RESET_SMB12 1 > +#define NPCM8XX_RESET_SPIX 2 > +#define NPCM8XX_RESET_SMB13 3 > +#define NPCM8XX_RESET_UDC0 4 > +#define NPCM8XX_RESET_UDC7 5 > +#define NPCM8XX_RESET_UDC8 6 > +#define NPCM8XX_RESET_UDC9 7 > +#define NPCM8XX_RESET_USBHUB 8 > +#define NPCM8XX_RESET_PCI_MAILBOX 9 > +#define NPCM8XX_RESET_GDMA1 10 > +#define NPCM8XX_RESET_GDMA2 11 > +#define NPCM8XX_RESET_SMB14 12 > +#define NPCM8XX_RESET_SHA 13 > +#define NPCM8XX_RESET_SEC_ECC 14 > +#define NPCM8XX_RESET_PCIE_RC 15 > +#define NPCM8XX_RESET_TIMER_10_14 16 > +#define NPCM8XX_RESET_RNG 17 > +#define NPCM8XX_RESET_SMB15 18 > +#define NPCM8XX_RESET_SMB8 19 > +#define NPCM8XX_RESET_SMB9 20 > +#define NPCM8XX_RESET_SMB10 21 > +#define NPCM8XX_RESET_SMB11 22 > +#define NPCM8XX_RESET_ESPI 23 > +#define NPCM8XX_RESET_USB_PHY_1 24 > +#define NPCM8XX_RESET_USB_PHY_2 25 > + Best regards, Krzysztof
Hi Tomer, 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 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
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
On 23/05/2022 16:22, Geert Uytterhoeven wrote: > Hi Tomer, > > 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. 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. Best regards, Krzysztof
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
On 23/05/2022 17:11, Geert Uytterhoeven wrote: > 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. And such numbers (like GIC_SPI interrupt numbers) do not go to bindings. They go to DTS only. > 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. Correct. However this patch is some mixture of both approaches. The same pointed by Arnd: https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/ > 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)? Because you are embedding the device programming model into the bindings. It's the same as having properties: "vendor,value-for-register-xxx" We do not create bindings to describe programming model but hardware. Using the values from programming model is fragile and ties the bindings to that one programming model. Programming model can change, e.g. by mistake, but bindings should stay independent. > >> 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. Yes, fix the DTS. DTS are not the bindings. You can fix the DTS. You cannot fix the bindings because you affect both driver and DTS. > > BTW, are you aware of any driver that transforms interrupt numbers > obtained from DTS, because the DTS used the wrong number? Again, what do the DTS has here at all? The interrupt numbers are also not included in the bindings, so what does it prove? Best regards, Krzysztof
On 23/05/2022 17:22, Krzysztof Kozlowski wrote: >> 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. > > And such numbers (like GIC_SPI interrupt numbers) do not go to bindings. > They go to DTS only. > >> 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. > > Correct. > > However this patch is some mixture of both approaches. > > The same pointed by Arnd: > https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/ ...and one more from Arnd: https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/ Best regards, Krzysztof
Hi Krzysztof and Geert, Appreciate your comments! We are using the same binding method in the NPCM7XX that it is upstreamed a few years ago. https://elixir.bootlin.com/linux/v5.18/source/include/dt-bindings/reset/nuvoton,npcm7xx-reset.h In the Reset NPCM driver we check the reset spec arguments that are using the correct register offset and BIT. https://github.com/torvalds/linux/blob/master/drivers/reset/reset-npcm.c#L125 One more thing, Sorry about the mail format, I am using Gmail and now I am moving it to plain text mode. hope it will help. Any other suggestion to set the Gmail to work with the Linux community will help alot On Mon, 23 May 2022 at 18:23, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 23/05/2022 17:11, Geert Uytterhoeven wrote: > > 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. Will do it in the next patch. > >>> > >>>>>> + > >>>>>> +/* 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. > > And such numbers (like GIC_SPI interrupt numbers) do not go to bindings. > They go to DTS only. > > > 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. > > Correct. > > However this patch is some mixture of both approaches. > > The same pointed by Arnd: > https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/ > > > 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)? > > Because you are embedding the device programming model into the > bindings. It's the same as having properties: > "vendor,value-for-register-xxx" > > We do not create bindings to describe programming model but hardware. > Using the values from programming model is fragile and ties the bindings > to that one programming model. Programming model can change, e.g. by > mistake, but bindings should stay independent. > > > > >> 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. > > Yes, fix the DTS. DTS are not the bindings. You can fix the DTS. You > cannot fix the bindings because you affect both driver and DTS. > > > > > BTW, are you aware of any driver that transforms interrupt numbers > > obtained from DTS, because the DTS used the wrong number? > > Again, what do the DTS has here at all? The interrupt numbers are also > not included in the bindings, so what does it prove? > > Best regards, > Krzysztof Best regards, Tomer
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 +====================================================== +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 */ +// 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 + +/* Reset lines on IP1 reset module (NPCM8XX_RESET_IPSRST1) */ +#define NPCM8XX_RESET_GDMA0 3 +#define NPCM8XX_RESET_UDC1 5 +#define NPCM8XX_RESET_GMAC3 6 +#define NPCM8XX_RESET_UART_2_3 7 +#define NPCM8XX_RESET_UDC2 8 +#define NPCM8XX_RESET_PECI 9 +#define NPCM8XX_RESET_AES 10 +#define NPCM8XX_RESET_UART_0_1 11 +#define NPCM8XX_RESET_MC 12 +#define NPCM8XX_RESET_SMB2 13 +#define NPCM8XX_RESET_SMB3 14 +#define NPCM8XX_RESET_SMB4 15 +#define NPCM8XX_RESET_SMB5 16 +#define NPCM8XX_RESET_PWM_M0 18 +#define NPCM8XX_RESET_TIMER_0_4 19 +#define NPCM8XX_RESET_TIMER_5_9 20 +#define NPCM8XX_RESET_GMAC4 21 +#define NPCM8XX_RESET_UDC4 22 +#define NPCM8XX_RESET_UDC5 23 +#define NPCM8XX_RESET_UDC6 24 +#define NPCM8XX_RESET_UDC3 25 +#define NPCM8XX_RESET_ADC 27 +#define NPCM8XX_RESET_SMB6 28 +#define NPCM8XX_RESET_SMB7 29 +#define NPCM8XX_RESET_SMB0 30 +#define NPCM8XX_RESET_SMB1 31 + +/* Reset lines on IP2 reset module (NPCM8XX_RESET_IPSRST2) */ +#define NPCM8XX_RESET_MFT0 0 +#define NPCM8XX_RESET_MFT1 1 +#define NPCM8XX_RESET_MFT2 2 +#define NPCM8XX_RESET_MFT3 3 +#define NPCM8XX_RESET_MFT4 4 +#define NPCM8XX_RESET_MFT5 5 +#define NPCM8XX_RESET_MFT6 6 +#define NPCM8XX_RESET_MFT7 7 +#define NPCM8XX_RESET_MMC 8 +#define NPCM8XX_RESET_GFX_SYS 10 +#define NPCM8XX_RESET_AHB_PCIBRG 11 +#define NPCM8XX_RESET_VDMA 12 +#define NPCM8XX_RESET_ECE 13 +#define NPCM8XX_RESET_VCD 14 +#define NPCM8XX_RESET_VIRUART1 16 +#define NPCM8XX_RESET_VIRUART2 17 +#define NPCM8XX_RESET_SIOX1 18 +#define NPCM8XX_RESET_SIOX2 19 +#define NPCM8XX_RESET_BT 20 +#define NPCM8XX_RESET_3DES 21 +#define NPCM8XX_RESET_PSPI2 23 +#define NPCM8XX_RESET_GMAC2 25 +#define NPCM8XX_RESET_USBH1 26 +#define NPCM8XX_RESET_GMAC1 28 +#define NPCM8XX_RESET_CP1 31 + +/* Reset lines on IP3 reset module (NPCM8XX_RESET_IPSRST3) */ +#define NPCM8XX_RESET_PWM_M1 0 +#define NPCM8XX_RESET_SMB12 1 +#define NPCM8XX_RESET_SPIX 2 +#define NPCM8XX_RESET_SMB13 3 +#define NPCM8XX_RESET_UDC0 4 +#define NPCM8XX_RESET_UDC7 5 +#define NPCM8XX_RESET_UDC8 6 +#define NPCM8XX_RESET_UDC9 7 +#define NPCM8XX_RESET_USBHUB 8 +#define NPCM8XX_RESET_PCI_MAILBOX 9 +#define NPCM8XX_RESET_GDMA1 10 +#define NPCM8XX_RESET_GDMA2 11 +#define NPCM8XX_RESET_SMB14 12 +#define NPCM8XX_RESET_SHA 13 +#define NPCM8XX_RESET_SEC_ECC 14 +#define NPCM8XX_RESET_PCIE_RC 15 +#define NPCM8XX_RESET_TIMER_10_14 16 +#define NPCM8XX_RESET_RNG 17 +#define NPCM8XX_RESET_SMB15 18 +#define NPCM8XX_RESET_SMB8 19 +#define NPCM8XX_RESET_SMB9 20 +#define NPCM8XX_RESET_SMB10 21 +#define NPCM8XX_RESET_SMB11 22 +#define NPCM8XX_RESET_ESPI 23 +#define NPCM8XX_RESET_USB_PHY_1 24 +#define NPCM8XX_RESET_USB_PHY_2 25 + +/* Reset lines on IP4 reset module (NPCM8XX_RESET_IPSRST4) */ +#define NPCM8XX_RESET_SMB16 0 +#define NPCM8XX_RESET_SMB17 1 +#define NPCM8XX_RESET_SMB18 2 +#define NPCM8XX_RESET_SMB19 3 +#define NPCM8XX_RESET_SMB20 4 +#define NPCM8XX_RESET_SMB21 5 +#define NPCM8XX_RESET_SMB22 6 +#define NPCM8XX_RESET_SMB23 7 +#define NPCM8XX_RESET_I3C0 8 +#define NPCM8XX_RESET_I3C1 9 +#define NPCM8XX_RESET_I3C2 10 +#define NPCM8XX_RESET_I3C3 11 +#define NPCM8XX_RESET_I3C4 12 +#define NPCM8XX_RESET_I3C5 13 +#define NPCM8XX_RESET_UART4 16 +#define NPCM8XX_RESET_UART5 17 +#define NPCM8XX_RESET_UART6 18 +#define NPCM8XX_RESET_PCIMBX2 19 +#define NPCM8XX_RESET_SMB24 22 +#define NPCM8XX_RESET_SMB25 23 +#define NPCM8XX_RESET_SMB26 24 +#define NPCM8XX_RESET_USBPHY3 25 +#define NPCM8XX_RESET_PCIRCPHY 27 +#define NPCM8XX_RESET_PWM_M2 28 +#define NPCM8XX_RESET_JTM1 29 +#define NPCM8XX_RESET_JTM2 30 +#define NPCM8XX_RESET_USBH2 31 + +#endif
Add binding document and device tree binding constants for Nuvoton BMC NPCM8XX reset controller. Signed-off-by: Tomer Maimon <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