| Message ID | 20220608095623.22327-1-tmaimon77@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand |
On 08/06/2022 11:56, Tomer Maimon wrote: > Modify nuvoton,npcm750-reset specific name to > general NPCM file name, nuvoton,npcm-reset. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > .../{nuvoton,npcm750-reset.yaml => nuvoton,npcm-reset.yaml} | 2 +- No. Name from the first compatible should be used. Best regards, Krzysztof
Hi Arnd, Sorry but Just to clarify, This patch should not be applied and the NPCM8XX UART should use nuvoton,npcm750-uart compatible in the device tree? Because I thought that in your comment a few weeks ago https://www.spinics.net/lists/linux-serial/msg48179.html We need only to modify the compatible string in the device tree as we did in V2 patchset https://www.spinics.net/lists/arm-kernel/msg986480.html On Wed, 8 Jun 2022 at 15:01, Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Jun 8, 2022 at 11:56 AM Tomer Maimon <tmaimon77@gmail.com> wrote: > > > > Add Nuvoton BMC NPCM845 UART support. > > The NPCM845 uses the same UART as the NPCM750. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > This one should no longer be needed if the timers are compatible with the > old ones and correctly described in the DT. > > Arnd Thanks, Tomer
On Wed, Jun 8, 2022 at 3:40 PM Tomer Maimon <tmaimon77@gmail.com> wrote: > > Sorry but Just to clarify, This patch should not be applied and the > NPCM8XX UART should use nuvoton,npcm750-uart compatible in the device > tree? > > Because I thought that in your comment a few weeks ago > https://www.spinics.net/lists/linux-serial/msg48179.html > > We need only to modify the compatible string in the device tree as we > did in V2 patchset > https://www.spinics.net/lists/arm-kernel/msg986480.html Yes, this is correct: with the DT file from v2, the driver no longer needs to be changed, it will just match the fallback value. Arnd
On Wed, 08 Jun 2022 12:56:12 +0300, Tomer Maimon wrote: > Describe syscon property that handles general > control registers(GCR) in Nuvoton BMC NPCM > reset driver. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > .../devicetree/bindings/reset/nuvoton,npcm-reset.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > Running 'make dtbs_check' with the schema in this patch gives the following warnings. Consider if they are expected or the schema is incorrect. These may not be new warnings. Note that it is not yet a requirement to have 0 warnings for dtbs_check. This will change in the future. Full log is available here: https://patchwork.ozlabs.org/patch/ rstc@f0801000: 'nuvoton,sysgcr' is a required property arch/arm/boot/dts/nuvoton-npcm730-gbs.dtb arch/arm/boot/dts/nuvoton-npcm730-gsj.dtb arch/arm/boot/dts/nuvoton-npcm730-kudo.dtb arch/arm/boot/dts/nuvoton-npcm750-evb.dtb arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dtb
Hi Krzysztof, Will revert the change in V3 On Wed, 8 Jun 2022 at 13:03, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 08/06/2022 11:56, Tomer Maimon wrote: > > Modify nuvoton,npcm750-reset specific name to > > general NPCM file name, nuvoton,npcm-reset. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > .../{nuvoton,npcm750-reset.yaml => nuvoton,npcm-reset.yaml} | 2 +- > > > No. Name from the first compatible should be used. > > Best regards, > Krzysztof Thanks, Tomer
Hi Krzysztof, Sorry, probably I missed your comments (too many patches to handle at one time :-))... On Wed, 8 Jun 2022 at 13:21, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 08/06/2022 11:56, Tomer Maimon wrote: > > This adds initial device tree support for the > > Nuvoton NPCM845 Board Management controller (BMC) SoC family. > > > > The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and > > have various peripheral IPs. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > --- > > arch/arm64/boot/dts/Makefile | 1 + > > .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 197 ++++++++++++++++++ > > .../boot/dts/nuvoton/nuvoton-npcm845.dtsi | 76 +++++++ > > 3 files changed, 274 insertions(+) > > create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi > > > > diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile > > index 1ba04e31a438..7b107fa7414b 100644 > > --- a/arch/arm64/boot/dts/Makefile > > +++ b/arch/arm64/boot/dts/Makefile > > @@ -19,6 +19,7 @@ subdir-y += lg > > subdir-y += marvell > > subdir-y += mediatek > > subdir-y += microchip > > +subdir-y += nuvoton > > subdir-y += nvidia > > subdir-y += qcom > > subdir-y += realtek > > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > new file mode 100644 > > index 000000000000..97e108c50760 > > --- /dev/null > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > @@ -0,0 +1,197 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +// Copyright (c) 2021 Nuvoton Technology tomer.maimon@nuvoton.com > > + > > +#include <dt-bindings/clock/nuvoton,npcm8xx-clock.h> > > +#include <dt-bindings/interrupt-controller/arm-gic.h> > > +#include <dt-bindings/interrupt-controller/irq.h> > > + > > +/ { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + interrupt-parent = <&gic>; > > + > > + /* external reference clock */ > > + clk_refclk: clk-refclk { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <25000000>; > > Ignored comment. Could we use it as a default clock-frequency? > > > + clock-output-names = "refclk"; > > + }; > > + > > + /* external reference clock for cpu. float in normal operation */ > > + clk_sysbypck: clk-sysbypck { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <1000000000>; > > Ignored comment. same as above > > > + clock-output-names = "sysbypck"; > > + }; > > + > > + /* external reference clock for MC. float in normal operation */ > > + clk_mcbypck: clk-mcbypck { > > + compatible = "fixed-clock"; > > + #clock-cells = <0>; > > + clock-frequency = <1050000000>; same as above > > + clock-output-names = "mcbypck"; > > + }; > > + > > + soc { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + compatible = "simple-bus"; > > + interrupt-parent = <&gic>; > > + ranges; > > + > > + gcr: gcr@f0800000 { I understand it sounds generic but I try to be as much compatible with NPCM7XX https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L91 > > Ignored comment. > > > + compatible = "nuvoton,npcm845-gcr", "syscon", > > + "simple-mfd"; > > This is not a simple-mfd... I see original bindings defined it that way, > but why? I think they should be corrected - remove simple-mfd from the > bindings and DTS. will remove in both places in V3 > > > > + reg = <0x0 0xf0800000 0x0 0x1000>; > > + }; > > + > > + gic: interrupt-controller@dfff9000 { > > + compatible = "arm,gic-400"; > > + reg = <0x0 0xdfff9000 0x0 0x1000>, > > + <0x0 0xdfffa000 0x0 0x2000>, > > + <0x0 0xdfffc000 0x0 0x2000>, > > + <0x0 0xdfffe000 0x0 0x2000>; > > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>; > > + #interrupt-cells = <3>; > > + interrupt-controller; > > + #address-cells = <0>; > > + ppi-partitions { > > + ppi_cluster0: interrupt-partition-0 { > > + affinity = <&cpu0 &cpu1 &cpu2 &cpu3>; > > + }; > > + }; > > + }; > > + }; > > + > > + ahb { > > + #address-cells = <2>; > > + #size-cells = <2>; > > + compatible = "simple-bus"; > > + interrupt-parent = <&gic>; > > + ranges; > > + > > + rstc: rstc@f0801000 { > > Ignored comment. > I understand it sounds generic but I try to be as much compatible with NPCM7XX https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L109 > Four comments from v1 ignored in this patch alone. > one more comment in V1 "+ cpu0: cpu@0 { + device_type = "cpu"; + compatible = "arm,cortex-a35"; + clocks = <&clk NPCM8XX_CLK_CPU>; + reg = <0x0 0x0>; Why do you have two address cells? A bit more complicated and not necessary, I think." the arm,cortex-a35 is 64 Bit this is why we use #address-cells = <2>; and therefore reg = <0x0 0x0>; > I'll stop reviewing, it is a waste of my time. > > NAK for this change. > > Best regards, > Krzysztof Again sorry to miss these comments in V1. Appreciate your time. Best regards, Tomer
Hi Tomer, On Fri, Jun 10, 2022 at 12:30 AM Tomer Maimon <tmaimon77@gmail.com> wrote: > On Wed, 8 Jun 2022 at 13:21, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: > > On 08/06/2022 11:56, Tomer Maimon wrote: > > > This adds initial device tree support for the > > > Nuvoton NPCM845 Board Management controller (BMC) SoC family. > > > > > > The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and > > > have various peripheral IPs. > > > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > > --- /dev/null > > > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi > > > @@ -0,0 +1,197 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +// Copyright (c) 2021 Nuvoton Technology tomer.maimon@nuvoton.com > > > + > > > +#include <dt-bindings/clock/nuvoton,npcm8xx-clock.h> > > > +#include <dt-bindings/interrupt-controller/arm-gic.h> > > > +#include <dt-bindings/interrupt-controller/irq.h> > > > + > > > +/ { > > > + #address-cells = <2>; > > > + #size-cells = <2>; > > > + interrupt-parent = <&gic>; > > > + > > > + /* external reference clock */ > > > + clk_refclk: clk-refclk { > > > + compatible = "fixed-clock"; > > > + #clock-cells = <0>; > > > + clock-frequency = <25000000>; > > > > Ignored comment. > Could we use it as a default clock-frequency? If the oscillator is present on the board, and not an SoC builtin, its clock frequency should be described in the board DTS. Some clocks may be optional, and left unpopulated. Others clocks may be fed with different frequencies than the default. > > > > > + clock-output-names = "refclk"; > > > + }; > > > + > > > + /* external reference clock for cpu. float in normal operation */ > > > + clk_sysbypck: clk-sysbypck { > > > + compatible = "fixed-clock"; > > > + #clock-cells = <0>; > > > + clock-frequency = <1000000000>; > > > > Ignored comment. > same as above > > > > > + clock-output-names = "sysbypck"; > > > + }; > > > + > > > + /* external reference clock for MC. float in normal operation */ > > > + clk_mcbypck: clk-mcbypck { > > > + compatible = "fixed-clock"; > > > + #clock-cells = <0>; > > > + clock-frequency = <1050000000>; > same as above > > > + clock-output-names = "mcbypck"; > > > + }; > "+ cpu0: cpu@0 { > + device_type = "cpu"; > + compatible = "arm,cortex-a35"; > + clocks = <&clk NPCM8XX_CLK_CPU>; > + reg = <0x0 0x0>; > Why do you have two address cells? A bit more complicated and not > necessary, I think." > the arm,cortex-a35 is 64 Bit this is why we use #address-cells = <2>; > and therefore reg = <0x0 0x0>; These addresses are not addresses on the main memory bus (which is indeed 64-bit), but on the logical CPU bus. Now, Documentation/devicetree/bindings/arm/cpus.yaml says you can have #address-cells = <2> if you have non-zero MPIDR_EL1 high bits. 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 10/06/2022 00:29, Tomer Maimon wrote: >>> + clk_refclk: clk-refclk { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-frequency = <25000000>; >> >> Ignored comment. > Could we use it as a default clock-frequency? In DTS? If my assumption, that this clock is not on SoC itself, is correct, then the answer is no, you cannot. The clock physically sits on the board, so it is defined by board DTS. Feel free to embed in SoC DTSI most of the clock properties, but the core property - frequency - must be outside. >> >>> + clock-output-names = "refclk"; >>> + }; >>> + >>> + /* external reference clock for cpu. float in normal operation */ >>> + clk_sysbypck: clk-sysbypck { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-frequency = <1000000000>; >> >> Ignored comment. > same as above >> >>> + clock-output-names = "sysbypck"; >>> + }; >>> + >>> + /* external reference clock for MC. float in normal operation */ >>> + clk_mcbypck: clk-mcbypck { >>> + compatible = "fixed-clock"; >>> + #clock-cells = <0>; >>> + clock-frequency = <1050000000>; > same as above >>> + clock-output-names = "mcbypck"; >>> + }; >>> + >>> + soc { >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + compatible = "simple-bus"; >>> + interrupt-parent = <&gic>; >>> + ranges; >>> + >>> + gcr: gcr@f0800000 { > I understand it sounds generic but I try to be as much compatible with NPCM7XX > https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L91 Then fix NPCM7XX. Please do not multiple bad choices because it looks similar. Fix the other wrong one. >> >> Ignored comment. >> >>> + compatible = "nuvoton,npcm845-gcr", "syscon", >>> + "simple-mfd"; >> >> This is not a simple-mfd... I see original bindings defined it that way, >> but why? I think they should be corrected - remove simple-mfd from the >> bindings and DTS. > will remove in both places in V3 >> >> >>> + reg = <0x0 0xf0800000 0x0 0x1000>; >>> + }; >>> + >>> + gic: interrupt-controller@dfff9000 { >>> + compatible = "arm,gic-400"; >>> + reg = <0x0 0xdfff9000 0x0 0x1000>, >>> + <0x0 0xdfffa000 0x0 0x2000>, >>> + <0x0 0xdfffc000 0x0 0x2000>, >>> + <0x0 0xdfffe000 0x0 0x2000>; >>> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>; >>> + #interrupt-cells = <3>; >>> + interrupt-controller; >>> + #address-cells = <0>; >>> + ppi-partitions { >>> + ppi_cluster0: interrupt-partition-0 { >>> + affinity = <&cpu0 &cpu1 &cpu2 &cpu3>; >>> + }; >>> + }; >>> + }; >>> + }; >>> + >>> + ahb { >>> + #address-cells = <2>; >>> + #size-cells = <2>; >>> + compatible = "simple-bus"; >>> + interrupt-parent = <&gic>; > >>> + ranges; >>> + >>> + rstc: rstc@f0801000 { >> >> Ignored comment. >> > I understand it sounds generic but I try to be as much compatible with NPCM7XX > https://elixir.bootlin.com/linux/v5.19-rc1/source/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi#L109 Fix 7xx. Best regards, Krzysztof
On 10/06/2022 09:57, Geert Uytterhoeven wrote: >> "+ cpu0: cpu@0 { >> + device_type = "cpu"; >> + compatible = "arm,cortex-a35"; >> + clocks = <&clk NPCM8XX_CLK_CPU>; >> + reg = <0x0 0x0>; >> Why do you have two address cells? A bit more complicated and not >> necessary, I think." >> the arm,cortex-a35 is 64 Bit this is why we use #address-cells = <2>; >> and therefore reg = <0x0 0x0>; > > These addresses are not addresses on the main memory bus (which > is indeed 64-bit), but on the logical CPU bus. > Now, Documentation/devicetree/bindings/arm/cpus.yaml says you can > have #address-cells = <2> if you have non-zero MPIDR_EL1 high bits. > Thanks Tomer and Geert for explanation. OK for me. Best regards, Krzysztof
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. Addressed comments from: - Arnd Bergmann : https://www.spinics.net/lists/arm-kernel/msg982728.html https://www.spinics.net/lists/linux-serial/msg48179.html - Stephen Boyd: https://www.spinics.net/lists/arm-kernel/msg983594.html - Krzysztof Kozlowski: https://www.spinics.net/lists/kernel/msg4368955.html https://www.spinics.net/lists/arm-kernel/msg982427.html https://lore.kernel.org/all/973d75b8-0eb6-ff5b-6cd2-9b7d7c5cbcaa@linaro.org/ https://www.spinics.net/lists/arm-kernel/msg982778.html https://www.spinics.net/lists/arm-kernel/msg982588.html - Ilpo Järvinen: https://www.spinics.net/lists/kernel/msg4368936.html - Stephen Boyd: https://www.spinics.net/lists/arm-kernel/msg983596.html - Geert Uytterhoeven : https://www.spinics.net/lists/kernel/msg4369514.html Changes since version 1: - NPCM8XX clock driver - Modify dt-binding. - Remove unsed definition and include. - Include alphabetically. - Use clock devm. - NPCM reset driver - Modify dt-binding. - Modify syscon name. - Add syscon support to NPCM7XX dts reset node. - use data structure. - NPCM8XX device tree: - Modify evb compatible name. - Add NPCM7xx compatible. - Remove disable nodes from the EVB DTS. Tomer Maimon (20): clocksource: timer-npcm7xx: Add NPCM845 timer 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: modify to general NPCM name dt-bindings: reset: npcm: add GCR syscon property ARM: dts: nuvoton: add reset 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 .../devicetree/bindings/arm/npcm/npcm.yaml | 7 + .../bindings/arm/npcm/nuvoton,gcr.yaml | 2 + .../bindings/clock/nuvoton,npcm845-clk.yaml | 63 ++ ...750-reset.yaml => nuvoton,npcm-reset.yaml} | 21 +- .../devicetree/bindings/serial/8250.yaml | 1 + .../bindings/watchdog/nuvoton,npcm-wdt.txt | 3 +- MAINTAINERS | 3 + arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi | 1 + arch/arm64/Kconfig.platforms | 11 + arch/arm64/boot/dts/Makefile | 1 + arch/arm64/boot/dts/nuvoton/Makefile | 2 + .../dts/nuvoton/nuvoton-common-npcm8xx.dtsi | 197 +++++ .../boot/dts/nuvoton/nuvoton-npcm845-evb.dts | 30 + .../boot/dts/nuvoton/nuvoton-npcm845.dtsi | 76 ++ arch/arm64/configs/defconfig | 3 + drivers/clk/Kconfig | 6 + drivers/clk/Makefile | 1 + drivers/clk/clk-npcm8xx.c | 756 ++++++++++++++++++ drivers/clocksource/timer-npcm7xx.c | 1 + drivers/reset/reset-npcm.c | 202 ++++- drivers/tty/serial/8250/8250_of.c | 1 + drivers/watchdog/npcm_wdt.c | 1 + .../dt-bindings/clock/nuvoton,npcm8xx-clock.h | 50 ++ .../dt-bindings/reset/nuvoton,npcm8xx-reset.h | 128 +++ 24 files changed, 1530 insertions(+), 37 deletions(-) create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,npcm845-clk.yaml rename Documentation/devicetree/bindings/reset/{nuvoton,npcm750-reset.yaml => nuvoton,npcm-reset.yaml} (58%) create mode 100644 arch/arm64/boot/dts/nuvoton/Makefile create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-common-npcm8xx.dtsi create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845-evb.dts create mode 100644 arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi create mode 100644 drivers/clk/clk-npcm8xx.c create mode 100644 include/dt-bindings/clock/nuvoton,npcm8xx-clock.h create mode 100644 include/dt-bindings/reset/nuvoton,npcm8xx-reset.h