| Message ID | 20220522155046.260146-1-tmaimon77@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand |
On 22/05/2022 17:50, Tomer Maimon wrote: > Add a compatible string for Nuvoton BMC NPCM845 UART. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 22/05/2022 17:50, Tomer Maimon wrote: > Using syscon device tree property instead of > device data to handle the NPCM GCR registers. https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > drivers/reset/reset-npcm.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/reset/reset-npcm.c b/drivers/reset/reset-npcm.c > index 2ea4d3136e15..0c963b21eddc 100644 > --- a/drivers/reset/reset-npcm.c > +++ b/drivers/reset/reset-npcm.c > @@ -138,8 +138,7 @@ static int npcm_reset_xlate(struct reset_controller_dev *rcdev, > } > > static const struct of_device_id npcm_rc_match[] = { > - { .compatible = "nuvoton,npcm750-reset", > - .data = (void *)"nuvoton,npcm750-gcr" }, > + { .compatible = "nuvoton,npcm750-reset"}, > { } > }; > > @@ -155,14 +154,10 @@ static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc) > u32 ipsrst1_bits = 0; > u32 ipsrst2_bits = NPCM_IPSRST2_USB_HOST; > u32 ipsrst3_bits = 0; > - const char *gcr_dt; > > - gcr_dt = (const char *) > - of_match_device(dev->driver->of_match_table, dev)->data; > - > - gcr_regmap = syscon_regmap_lookup_by_compatible(gcr_dt); > + gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon"); I think this just broke all existing boards... Best regards, Krzysztof
On 22/05/2022 17:50, Tomer Maimon wrote: > Add a compatible string for Nuvoton BMC NPCM845 SoC and a board > specific device tree for the NPCM845 (Arbel) evaluation board. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 22/05/2022 17:50, Tomer Maimon wrote: > Add initial Nuvoton NPCM845 evaluation board device tree. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > arch/arm64/boot/dts/nuvoton/Makefile | 2 + > .../boot/dts/nuvoton/nuvoton-npcm845-evb.dts | 50 +++++++++++++++++++ > 2 files changed, 52 insertions(+) > create mode 100644 arch/arm64/boot/dts/nuvoton/Makefile > create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > > diff --git a/arch/arm64/boot/dts/nuvoton/Makefile b/arch/arm64/boot/dts/nuvoton/Makefile > new file mode 100644 > index 000000000000..a99dab90472a > --- /dev/null > +++ b/arch/arm64/boot/dts/nuvoton/Makefile > @@ -0,0 +1,2 @@ > +# SPDX-License-Identifier: GPL-2.0 > +dtb-$(CONFIG_ARCH_NPCM) += nuvoton-npcm845-evb.dtb > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > new file mode 100644 > index 000000000000..d7a9a85f8075 > --- /dev/null > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts > @@ -0,0 +1,50 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2021 Nuvoton Technology tomer.maimon@nuvoton.com > + > +/dts-v1/; > +#include "nuvoton-npcm845.dtsi" > + > +/ { > + model = "Nuvoton npcm845 Development Board (Device Tree)"; s/ (Device Tree)// > + compatible = "nuvoton,npcm845"; This does not match your bindings. Please test your DTS with `make dtbs_check`. > + > + aliases { > + serial0 = &serial0; > + serial1 = &serial1; > + serial2 = &serial2; > + serial3 = &serial3; > + }; > + > + chosen { > + stdout-path = &serial0; > + }; > + > + memory { > + reg = <0x0 0x0 0x0 0x40000000>; > + }; > + > + ahb { > + No need for blank line. > + apb { > + serial0: serial@0 { > + status = "okay"; No, override by labels. Here and in places below. > + }; > + > + serial1: serial@1000 { > + status = "disabled"; > + }; > + > + serial2: serial@2000 { > + status = "disabled"; > + }; > + > + serial3: serial@3000 { > + status = "disabled"; > + }; > + > + watchdog1: watchdog@901c { > + status = "okay"; > + }; > + }; > + }; > +}; Best regards, Krzysztof
On Sun, May 22, 2022 at 5:50 PM Tomer Maimon <tmaimon77@gmail.com> wrote: > > This patchset adds initial support for the Nuvoton > Arbel NPCM8XX Board Management controller (BMC) SoC family. > > The Nuvoton Arbel NPCM8XX SoC is a fourth-generation BMC. > The NPCM8XX computing subsystem comprises a quadcore ARM > Cortex A35 ARM-V8 architecture. > > This patchset adds minimal architecture and drivers such as: > Clocksource, Clock, Reset, and WD. > > Some of the Arbel NPCM8XX peripherals are based on Poleg NPCM7XX. > > This patchset was tested on the Arbel NPCM8XX evaluation board. Thanks for your submission. Please note a few things about the process here: - The merge window is currently open, which means a lo Some of the Arbel NPCM8XX peripherals are based on Poleg NPCM7XX. This patchset was tested on the Arbel NPCM8XX evaluation board. Tomer Maimon (19): dt-bindings: timer: npcm: Add npcm845 compatible string clocksource: timer-npcm7xx: Add NPCM845 timer support dt-bindings: serial: 8250: Add npcm845 compatible string tty: serial: 8250: Add NPCM845 UART support dt-bindings: watchdog: npcm: Add npcm845 compatible string watchdog: npcm_wdt: Add NPCM845 watchdog support dt-binding: clk: npcm845: Add binding for Nuvoton NPCM8XX Clock clk: npcm8xx: add clock controller dt-bindings: reset: add syscon property reset: npcm: using syscon instead of device data dt-bindings: reset: npcm: Add support for NPCM8XX reset: npcm: Add NPCM8XX support dt-bindings: arm: npcm: Add maintainer dt-bindings: arm: npcm: Add nuvoton,npcm845 compatible string dt-bindings: arm: npcm: Add nuvoton,npcm845 GCR compatible string arm64: npcm: Add support for Nuvoton NPCM8XX BMC SoC arm64: dts: nuvoton: Add initial NPCM8XX device tree arm64: dts: nuvoton: Add initial NPCM845 EVB device tree arm64: defconfig: Add Nuvoton NPCM family supportt of maintainers won't be reviewing your patches at the moment. It may be better to wait for the -rc1 to be out before sending out v2 - don't send your patches to soc@kernel.org unless you want me to pick them up into the soc tree and they have been reviewed already. The series is clearly still under review at the moment, and I expect it to go through a few revisions first. - gmail marked your emails as possible spam for me. I don't know what happened here, but you may want to look into this to ensure that everybody receives it. Some of the Arbel NPCM8XX peripherals are based on Poleg NPCM7XX. This patchset was tested on the Arbel NPCM8XX evaluation board. Tomer Maimon (19): dt-bindings: timer: npcm: Add npcm845 compatible string clocksource: timer-npcm7xx: Add NPCM845 timer support dt-bindings: serial: 8250: Add npcm845 compatible string tty: serial: 8250: Add NPCM845 UART support dt-bindings: watchdog: npcm: Add npcm845 compatible string watchdog: npcm_wdt: Add NPCM845 watchdog support dt-binding: clk: npcm845: Add binding for Nuvoton NPCM8XX Clock clk: npcm8xx: add clock controller dt-bindings: reset: add syscon property reset: npcm: using syscon instead of device data dt-bindings: reset: npcm: Add support for NPCM8XX reset: npcm: Add NPCM8XX support dt-bindings: arm: npcm: Add maintainer dt-bindings: arm: npcm: Add nuvoton,npcm845 compatible string dt-bindings: arm: npcm: Add nuvoton,npcm845 GCR compatible string arm64: npcm: Add support for Nuvoton NPCM8XX BMC SoC arm64: dts: nuvoton: Add initial NPCM8XX device tree arm64: dts: nuvoton: Add initial NPCM845 EVB device tree arm64: defconfig: Add Nuvoton NPCM family support - For an initial platform submission, I can merge the clk/clocksource/serial/reset drivers along with the platform if they have an Ack from the subsystem maintainers. I would normally not include the watchdog patch in this as it's not essential, but I suppose that it's fine if you only do a oneline change and it has an Ack. If you have other nonessential drivers that need changes, best submit them separately though. Arnd
On Sun, May 22, 2022 at 5:50 PM Tomer Maimon <tmaimon77@gmail.com> wrote: > +++ b/drivers/tty/serial/8250/8250_of.c > @@ -333,6 +333,7 @@ static const struct of_device_id of_platform_serial_table[] = { > { .compatible = "ti,da830-uart", .data = (void *)PORT_DA830, }, > { .compatible = "nuvoton,wpcm450-uart", .data = (void *)PORT_NPCM, }, > { .compatible = "nuvoton,npcm750-uart", .data = (void *)PORT_NPCM, }, > + { .compatible = "nuvoton,npcm845-uart", .data = (void *)PORT_NPCM, }, > { /* end of list */ }, If these are compatible devices, it's usually easier to claim compatibility with both the specific chip and the older model as a fallback, to avoid driver changes. This seems to apply to the timer and watchdog devices as well. Arnd
On 23/05/2022 15:44, Tomer Maimon wrote: > On Mon, 23 May 2022 at 10:39, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org <mailto:krzysztof.kozlowski@linaro.org>> > wrote: > > On 22/05/2022 17:50, Tomer Maimon wrote: > > Describe syscon property that handles GCR registers > > in Nuvoton BMC NPCM reset driver. > > Please wrap according to Linux standards: > https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586 > <https://elixir.bootlin.com/linux/v5.18-rc4/source/Documentation/process/submitting-patches.rst#L586> > > Which problem do you see with the commit explanation body? It is wrapped not according to Linux standards. Best regards, Krzysztof
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
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: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