| Message ID | 20220522155046.260146-1-tmaimon77@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce Nuvoton Arbel NPCM8XX BMC SoC | expand |
On 5/22/22 08:50, Tomer Maimon wrote: > Add Nuvoton BMC NPCM845 watchdog support. > The NPCM845 uses the same watchdog as the NPCM750. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > --- > drivers/watchdog/npcm_wdt.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/watchdog/npcm_wdt.c b/drivers/watchdog/npcm_wdt.c > index 28a24caa2627..0b91a3fbec09 100644 > --- a/drivers/watchdog/npcm_wdt.c > +++ b/drivers/watchdog/npcm_wdt.c > @@ -231,6 +231,7 @@ static int npcm_wdt_probe(struct platform_device *pdev) > static const struct of_device_id npcm_wdt_match[] = { > {.compatible = "nuvoton,wpcm450-wdt"}, > {.compatible = "nuvoton,npcm750-wdt"}, > + {.compatible = "nuvoton,npcm845-wdt"}, > {}, > }; > MODULE_DEVICE_TABLE(of, npcm_wdt_match); Acked-by: Guenter Roeck <linux@roeck-us.net>
On Sun, 22 May 2022, Tomer Maimon wrote: > Nuvoton Arbel BMC NPCM7XX contains an integrated clock controller, which > generates and supplies clocks to all modules within the BMC. > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > +static struct clk_hw * > +npcm8xx_clk_register_pll(void __iomem *pllcon, const char *name, > + const char *parent_name, unsigned long flags) > +{ > + struct npcm8xx_clk_pll *pll; > + struct clk_init_data init; > + struct clk_hw *hw; > + int ret; > + > + pll = kzalloc(sizeof(*pll), GFP_KERNEL); > + if (!pll) > + return ERR_PTR(-ENOMEM); > + > + pr_debug("%s reg, name=%s, p=%s\n", __func__, name, parent_name); > + > + init.name = name; > + init.ops = &npcm8xx_clk_pll_ops; > + init.parent_names = &parent_name; > + init.num_parents = 1; > + init.flags = flags; > + > + pll->pllcon = pllcon; > + pll->hw.init = &init; > + > + hw = &pll->hw; > + > + ret = clk_hw_register(NULL, hw); > + if (ret) { > + kfree(pll); > + hw = ERR_PTR(ret); > + } > + > + return hw; > +} > + > +#define NPCM8XX_CLKEN1 (0x00) > +#define NPCM8XX_CLKEN2 (0x28) > +#define NPCM8XX_CLKEN3 (0x30) > +#define NPCM8XX_CLKEN4 (0x70) > +#define NPCM8XX_CLKSEL (0x04) > +#define NPCM8XX_CLKDIV1 (0x08) > +#define NPCM8XX_CLKDIV2 (0x2C) > +#define NPCM8XX_CLKDIV3 (0x58) > +#define NPCM8XX_CLKDIV4 (0x7C) > +#define NPCM8XX_PLLCON0 (0x0C) > +#define NPCM8XX_PLLCON1 (0x10) > +#define NPCM8XX_PLLCON2 (0x54) > +#define NPCM8XX_SWRSTR (0x14) > +#define NPCM8XX_IRQWAKECON (0x18) > +#define NPCM8XX_IRQWAKEFLAG (0x1C) > +#define NPCM8XX_IPSRST1 (0x20) > +#define NPCM8XX_IPSRST2 (0x24) > +#define NPCM8XX_IPSRST3 (0x34) > +#define NPCM8XX_WD0RCR (0x38) > +#define NPCM8XX_WD1RCR (0x3C) > +#define NPCM8XX_WD2RCR (0x40) > +#define NPCM8XX_SWRSTC1 (0x44) > +#define NPCM8XX_SWRSTC2 (0x48) > +#define NPCM8XX_SWRSTC3 (0x4C) > +#define NPCM8XX_SWRSTC4 (0x50) > +#define NPCM8XX_CORSTC (0x5C) > +#define NPCM8XX_PLLCONG (0x60) > +#define NPCM8XX_AHBCKFI (0x64) > +#define NPCM8XX_SECCNT (0x68) > +#define NPCM8XX_CNTR25M (0x6C) > +#define NPCM8XX_THRTL_CNT (0xC0) > + > +struct npcm8xx_clk_gate_data { > + u32 reg; > + u8 bit_idx; > + const char *name; > + const char *parent_name; > + unsigned long flags; > + /* > + * If this clock is exported via DT, set onecell_idx to constant > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h for > + * this specific clock. Otherwise, set to -1. > + */ > + int onecell_idx; > +}; > + > +struct npcm8xx_clk_mux_data { > + u8 shift; > + u8 mask; > + u32 *table; > + const char *name; > + const char * const *parent_names; > + u8 num_parents; > + unsigned long flags; > + /* > + * If this clock is exported via DT, set onecell_idx to constant > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h for > + * this specific clock. Otherwise, set to -1. > + */ > + int onecell_idx; > + > +}; > + > +struct npcm8xx_clk_div_fixed_data { > + u8 mult; > + u8 div; > + const char *name; > + const char *parent_name; > + u8 clk_divider_flags; > + /* > + * If this clock is exported via DT, set onecell_idx to constant > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h for > + * this specific clock. Otherwise, set to -1. > + */ > + int onecell_idx; > +}; > + > +struct npcm8xx_clk_div_data { > + u32 reg; > + u8 shift; > + u8 width; > + const char *name; > + const char *parent_name; > + u8 clk_divider_flags; > + unsigned long flags; > + /* > + * If this clock is exported via DT, set onecell_idx to constant > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h for > + * this specific clock. Otherwise, set to -1. > + */ > + int onecell_idx; > +}; > + > +struct npcm8xx_clk_pll_data { > + u32 reg; > + const char *name; > + const char *parent_name; > + unsigned long flags; > + /* > + * If this clock is exported via DT, set onecell_idx to constant > + * defined in include/dt-bindings/clock/nuvoton, NPCM8XX-clock.h for > + * this specific clock. Otherwise, set to -1. > + */ > + int onecell_idx; > +}; > + > +/* > + * Single copy of strings used to refer to clocks within this driver indexed by > + * above enum. > + */ > +#define NPCM8XX_CLK_S_REFCLK "refclk" > +#define NPCM8XX_CLK_S_SYSBYPCK "sysbypck" > +#define NPCM8XX_CLK_S_MCBYPCK "mcbypck" > +#define NPCM8XX_CLK_S_GFXBYPCK "gfxbypck" > +#define NPCM8XX_CLK_S_PLL0 "pll0" > +#define NPCM8XX_CLK_S_PLL1 "pll1" > +#define NPCM8XX_CLK_S_PLL1_DIV2 "pll1_div2" > +#define NPCM8XX_CLK_S_PLL2 "pll2" > +#define NPCM8XX_CLK_S_PLL_GFX "pll_gfx" > +#define NPCM8XX_CLK_S_PLL2_DIV2 "pll2_div2" > +#define NPCM8XX_CLK_S_PIX_MUX "gfx_pixel" > +#define NPCM8XX_CLK_S_GPRFSEL_MUX "gprfsel_mux" > +#define NPCM8XX_CLK_S_MC_MUX "mc_phy" > +#define NPCM8XX_CLK_S_CPU_MUX "cpu" /*AKA system clock.*/ Add spaces around comment. > +#define NPCM8XX_CLK_S_MC "mc" > +#define NPCM8XX_CLK_S_AXI "axi" /*AKA CLK2*/ > +#define NPCM8XX_CLK_S_AHB "ahb" /*AKA CLK4*/ Ditto. > +static void __init npcm8xx_clk_init(struct device_node *clk_np) > +{ > + struct clk_hw_onecell_data *npcm8xx_clk_data; > + void __iomem *clk_base; > + struct resource res; > + struct clk_hw *hw; > + int ret; > + int i; > + > + ret = of_address_to_resource(clk_np, 0, &res); > + if (ret) { > + pr_err("%pOFn: failed to get resource, ret %d\n", clk_np, ret); > + return; > + } > + > + clk_base = ioremap(res.start, resource_size(&res)); > + if (!clk_base) > + goto npcm8xx_init_error; > + > + npcm8xx_clk_data = kzalloc(struct_size(npcm8xx_clk_data, hws, > + NPCM8XX_NUM_CLOCKS), GFP_KERNEL); > + if (!npcm8xx_clk_data) > + goto npcm8xx_init_np_err; > + > + npcm8xx_clk_data->num = NPCM8XX_NUM_CLOCKS; > + > + for (i = 0; i < NPCM8XX_NUM_CLOCKS; i++) > + npcm8xx_clk_data->hws[i] = ERR_PTR(-EPROBE_DEFER); > + > + /* Register plls */ > + for (i = 0; i < ARRAY_SIZE(npcm8xx_plls); i++) { > + const struct npcm8xx_clk_pll_data *pll_data = &npcm8xx_plls[i]; > + > + hw = npcm8xx_clk_register_pll(clk_base + pll_data->reg, > + pll_data->name, > + pll_data->parent_name, > + pll_data->flags); > + if (IS_ERR(hw)) { Who deregisters the already registered plls on error paths? You might want to consider devm_ variants in npcm8xx_clk_register_pll() to make the cleanup simpler. Please check the other error path rollbacks from this point onward too. > + pr_err("npcm8xx_clk: Can't register pll\n"); > + goto npcm8xx_init_fail; > + } > + > + if (pll_data->onecell_idx >= 0) > + npcm8xx_clk_data->hws[pll_data->onecell_idx] = hw; > + } > + > + /* Register fixed dividers */ > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_PLL1_DIV2, > + NPCM8XX_CLK_S_PLL1, 0, 1, 2); > + if (IS_ERR(hw)) { > + pr_err("npcm8xx_clk: Can't register fixed div\n"); > + goto npcm8xx_init_fail; > + } > + > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_PLL2_DIV2, > + NPCM8XX_CLK_S_PLL2, 0, 1, 2); > + if (IS_ERR(hw)) { > + pr_err("npcm8xx_clk: Can't register pll div2\n"); > + goto npcm8xx_init_fail; > + } > + > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_PRE_CLK, > + NPCM8XX_CLK_S_CPU_MUX, 0, 1, 2); > + if (IS_ERR(hw)) { > + pr_err("npcm8xx_clk: Can't register ckclk div2\n"); > + goto npcm8xx_init_fail; > + } > + > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_AXI, > + NPCM8XX_CLK_S_TH, 0, 1, 2); > + if (IS_ERR(hw)) { > + pr_err("npcm8xx_clk: Can't register axi div2\n"); > + goto npcm8xx_init_fail; > + } > + > + hw = clk_hw_register_fixed_factor(NULL, NPCM8XX_CLK_S_ATB, > + NPCM8XX_CLK_S_AXI, 0, 1, 2); > + if (IS_ERR(hw)) { > + pr_err("npcm8xx_clk: Can't register atb div2\n"); > + goto npcm8xx_init_fail; > + } > + > + /* Register muxes */ > + for (i = 0; i < ARRAY_SIZE(npcm8xx_muxes); i++) { > + const struct npcm8xx_clk_mux_data *mux_data = &npcm8xx_muxes[i]; > + > + hw = clk_hw_register_mux_table(NULL, mux_data->name, > + mux_data->parent_names, > + mux_data->num_parents, > + mux_data->flags, > + clk_base + NPCM8XX_CLKSEL, > + mux_data->shift, > + mux_data->mask, 0, > + mux_data->table, > + &npcm8xx_clk_lock); > + > + if (IS_ERR(hw)) { > + pr_err("npcm8xx_clk: Can't register mux\n"); > + goto npcm8xx_init_fail; > + } > + > + if (mux_data->onecell_idx >= 0) > + npcm8xx_clk_data->hws[mux_data->onecell_idx] = hw; > + } > + > + /* Register clock dividers specified in npcm8xx_divs */ > + for (i = 0; i < ARRAY_SIZE(npcm8xx_divs); i++) { > + const struct npcm8xx_clk_div_data *div_data = &npcm8xx_divs[i]; > + > + hw = clk_hw_register_divider(NULL, div_data->name, > + div_data->parent_name, > + div_data->flags, > + clk_base + div_data->reg, > + div_data->shift, div_data->width, > + div_data->clk_divider_flags, > + &npcm8xx_clk_lock); > + if (IS_ERR(hw)) { > + pr_err("npcm8xx_clk: Can't register div table\n"); > + goto npcm8xx_init_fail; > + } > + > + if (div_data->onecell_idx >= 0) > + npcm8xx_clk_data->hws[div_data->onecell_idx] = hw; > + } > + > + ret = of_clk_add_hw_provider(clk_np, of_clk_hw_onecell_get, > + npcm8xx_clk_data); > + if (ret) > + pr_err("failed to add DT provider: %d\n", ret); > + > + of_node_put(clk_np); > + > + return; > + > +npcm8xx_init_fail: > + kfree(npcm8xx_clk_data->hws); > +npcm8xx_init_np_err: > + iounmap(clk_base); > +npcm8xx_init_error: > + of_node_put(clk_np); > +} > + > +CLK_OF_DECLARE(npcm8xx_clk_init, "nuvoton,npcm845-clk", npcm8xx_clk_init); >
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: > This adds initial device tree support for the > Nuvoton NPCM845 Board Management controller (BMC) SoC family. Thank you for your patch. There is something to discuss/improve. > > 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 | 77 +++++++ > 3 files changed, 275 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..19c672ecfee7 > --- /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> This is not a property of a SoC, but board. > + 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>; This is not a property of a SoC, but board. > + 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>; This is not a property of a SoC, but board. > + clock-output-names = "mcbypck"; > + }; > + > + soc { > + #address-cells = <2>; > + #size-cells = <2>; > + compatible = "simple-bus"; > + interrupt-parent = <&gic>; > + ranges; > + > + gcr: gcr@f0800000 { Generic node names. I guess it is system-controller? > + compatible = "nuvoton,npcm845-gcr", "syscon", > + "simple-mfd"; > + 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 { Generic node names. > + compatible = "nuvoton,npcm845-reset"; > + reg = <0x0 0xf0801000 0x0 0x78>; > + #reset-cells = <2>; > + syscon = <&gcr>; > + }; > + > + clk: clock-controller@f0801000 { > + compatible = "nuvoton,npcm845-clk"; > + #clock-cells = <1>; > + reg = <0x0 0xf0801000 0x0 0x1000>; > + clock-names = "refclk", "sysbypck", "mcbypck"; > + clocks = <&clk_refclk>, <&clk_sysbypck>, <&clk_mcbypck>; > + }; > + > + apb { > + #address-cells = <1>; > + #size-cells = <1>; > + compatible = "simple-bus"; > + interrupt-parent = <&gic>; > + ranges = <0x0 0x0 0xf0000000 0x00300000>, > + <0xfff00000 0x0 0xfff00000 0x00016000>; > + > + timer0: timer@8000 { > + compatible = "nuvoton,npcm845-timer"; > + interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0x8000 0x1C>; > + clocks = <&clk_refclk>; > + clock-names = "refclk"; > + }; > + > + serial0: serial@0 { > + compatible = "nuvoton,npcm845-uart"; > + reg = <0x0 0x1000>; > + clocks = <&clk NPCM8XX_CLK_UART>; > + interrupts = <GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>; > + reg-shift = <2>; > + status = "disabled"; > + }; > + > + serial1: serial@1000 { > + compatible = "nuvoton,npcm845-uart"; > + reg = <0x1000 0x1000>; > + clocks = <&clk NPCM8XX_CLK_UART>; > + interrupts = <GIC_SPI 193 IRQ_TYPE_LEVEL_HIGH>; > + reg-shift = <2>; > + status = "disabled"; > + }; > + > + serial2: serial@2000 { > + compatible = "nuvoton,npcm845-uart"; > + reg = <0x2000 0x1000>; > + clocks = <&clk NPCM8XX_CLK_UART>; > + interrupts = <GIC_SPI 194 IRQ_TYPE_LEVEL_HIGH>; > + reg-shift = <2>; > + status = "disabled"; > + }; > + > + serial3: serial@3000 { > + compatible = "nuvoton,npcm845-uart"; > + reg = <0x3000 0x1000>; > + clocks = <&clk NPCM8XX_CLK_UART>; > + interrupts = <GIC_SPI 195 IRQ_TYPE_LEVEL_HIGH>; > + reg-shift = <2>; > + status = "disabled"; > + }; > + > + serial4: serial@4000 { > + compatible = "nuvoton,npcm845-uart"; > + reg = <0x4000 0x1000>; > + clocks = <&clk NPCM8XX_CLK_UART>; > + interrupts = <GIC_SPI 196 IRQ_TYPE_LEVEL_HIGH>; > + reg-shift = <2>; > + status = "disabled"; > + }; > + > + serial5: serial@5000 { > + compatible = "nuvoton,npcm845-uart"; > + reg = <0x5000 0x1000>; > + clocks = <&clk NPCM8XX_CLK_UART>; > + interrupts = <GIC_SPI 197 IRQ_TYPE_LEVEL_HIGH>; > + reg-shift = <2>; > + status = "disabled"; > + }; > + > + serial6: serial@6000 { > + compatible = "nuvoton,npcm845-uart"; > + reg = <0x6000 0x1000>; > + clocks = <&clk NPCM8XX_CLK_UART>; > + interrupts = <GIC_SPI 198 IRQ_TYPE_LEVEL_HIGH>; > + reg-shift = <2>; > + status = "disabled"; > + }; > + > + watchdog0: watchdog@801c { > + compatible = "nuvoton,npcm845-wdt"; > + interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0x801c 0x4>; > + status = "disabled"; > + clocks = <&clk_refclk>; > + syscon = <&gcr>; > + }; > + > + watchdog1: watchdog@901c { > + compatible = "nuvoton,npcm845-wdt"; > + interrupts = <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0x901c 0x4>; > + status = "disabled"; > + clocks = <&clk_refclk>; > + syscon = <&gcr>; > + }; > + > + watchdog2: watchdog@a01c { > + compatible = "nuvoton,npcm845-wdt"; > + interrupts = <GIC_SPI 49 IRQ_TYPE_LEVEL_HIGH>; > + reg = <0xa01c 0x4>; > + status = "disabled"; > + clocks = <&clk_refclk>; > + syscon = <&gcr>; > + }; > + }; > + }; > +}; > diff --git a/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi > new file mode 100644 > index 000000000000..900cee112251 > --- /dev/null > +++ b/arch/arm64/boot/dts/nuvoton/nuvoton-npcm845.dtsi > @@ -0,0 +1,77 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2021 Nuvoton Technology tomer.maimon@nuvoton.com > + > +#include "nuvoton-common-npcm8xx.dtsi" > + > +/ { > + #address-cells = <2>; > + #size-cells = <2>; > + interrupt-parent = <&gic>; You do not have gic here, so it's not correct. Do not reference nodes outsides of the file. > + > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + 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. > + next-level-cache = <&l2>; > + enable-method = "psci"; > + }; > + > + cpu1: cpu@1 { > + device_type = "cpu"; > + compatible = "arm,cortex-a35"; > + clocks = <&clk NPCM8XX_CLK_CPU>; > + reg = <0x0 0x1>; > + next-level-cache = <&l2>; > + enable-method = "psci"; > + }; > + > + cpu2: cpu@2 { > + device_type = "cpu"; > + compatible = "arm,cortex-a35"; > + clocks = <&clk NPCM8XX_CLK_CPU>; > + reg = <0x0 0x2>; > + next-level-cache = <&l2>; > + enable-method = "psci"; > + }; > + > + cpu3: cpu@3 { > + device_type = "cpu"; > + compatible = "arm,cortex-a35"; > + clocks = <&clk NPCM8XX_CLK_CPU>; > + reg = <0x0 0x3>; > + next-level-cache = <&l2>; > + enable-method = "psci"; > + }; > + > + l2: l2-cache { > + compatible = "cache"; Is this a real compatible? What bindings are you using here? > + }; > + }; > + > + arm-pmu { > + compatible = "arm,cortex-a35-pmu"; > + interrupts = <GIC_SPI 242 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 243 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-affinity = <&cpu0>, <&cpu1>, <&cpu2>, <&cpu3>; > + }; > + > + psci { > + compatible = "arm,psci-1.0"; > + method = "smc"; Weird indentation. > + }; > + > + timer { > + compatible = "arm,armv8-timer"; > + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>, > + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>; > + }; > +}; 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: > +/ { > + model = "Nuvoton npcm845 Development Board (Device Tree)"; > + compatible = "nuvoton,npcm845"; > + > + aliases { > + serial0 = &serial0; > + serial1 = &serial1; > + serial2 = &serial2; > + serial3 = &serial3; > + }; > + apb { > + serial0: serial@0 { > + status = "okay"; > + }; > + > + serial1: serial@1000 { > + status = "disabled"; > + }; > + > + serial2: serial@2000 { > + status = "disabled"; > + }; > + > + serial3: serial@3000 { > + status = "disabled"; > + }; Please drop the aliases for disabled uarts. It probably also makes sense to have the status="disabled" properties in the .dtsi file and only override them when you explicitly want to enable a uart for a board. Arnd
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 Sun, May 22, 2022 at 5:50 PM Tomer Maimon <tmaimon77@gmail.com> wrote: > static const struct of_device_id npcm_rc_match[] = { > { .compatible = "nuvoton,npcm750-reset"}, > + { .compatible = "nuvoton,npcm845-reset"}, > { } > }; > +/* > + * The following procedure should be observed in USB PHY, USB device and > + * USB host initialization at BMC boot > + */ > +static int npcm_usb_reset(struct platform_device *pdev, struct npcm_rc_data *rc) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct device *dev = &pdev->dev; > + > + rc->gcr_regmap = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon"); > + if (IS_ERR(rc->gcr_regmap)) { > + dev_err(&pdev->dev, "Failed to find gcr syscon"); > + return PTR_ERR(rc->gcr_regmap); > + } > + > + if (of_device_is_compatible(np, "nuvoton,npcm750-reset")) > + npcm_usb_reset_npcm7xx(rc); > + else if (of_device_is_compatible(np, "nuvoton,npcm845-reset")) > + npcm_usb_reset_npcm8xx(rc); > + else > + return -ENODEV; > In place of the string comparison in of_device_is_compatible(), maybe just use the .data field of the of_device_id structure to point to the actual reset function. Alternatively, register two separate platform_driver instances here and use separate probe functions that do the soc specific bits and call into shared functions for the bits that are the same. Arnd
On 23/05/2022 14:58, Tomer Maimon wrote: > Hi Arnd, > > Thanks for your comment. > > On Mon, 23 May 2022 at 14:48, Arnd Bergmann <arnd@arndb.de > <mailto:arnd@arndb.de>> wrote: > > On Sun, May 22, 2022 at 5:50 PM Tomer Maimon <tmaimon77@gmail.com > <mailto: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. > > Just to make sure, Do you mean claim in the device tree? > like > > compatible = "nuvoton,npcm845-timer", "nuvoton,npcm-timer"; compatible = "nuvoton,npcm845-timer", "nuvoton,npcm750-timer""; Best regards, Krzysztof
Hi Krzysztof, On Mon, May 23, 2022 at 11:08 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 22/05/2022 17:50, Tomer Maimon wrote: > > This adds initial device tree support for the > > Nuvoton NPCM845 Board Management controller (BMC) SoC family. > > Thank you for your patch. There is something to discuss/improve. > > > The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and > > have various peripheral IPs. > > > > Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > > + l2: l2-cache { > > + compatible = "cache"; > > Is this a real compatible? What bindings are you using here? The compatible value and related properties are defined in the Devicetree Specification, v0.4-rc1, Section 3.9 ("Multi-level and Shared Cache Nodes (/cpus/cpu*/l?-cache)"). The properties are handled by dtschema/schemas/cache-controller.yaml, but the latter seems to lack any checking on the compatible value? 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 15:58, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Mon, May 23, 2022 at 11:08 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> On 22/05/2022 17:50, Tomer Maimon wrote: >>> This adds initial device tree support for the >>> Nuvoton NPCM845 Board Management controller (BMC) SoC family. >> >> Thank you for your patch. There is something to discuss/improve. >> >>> The NPCM845 based quad-core Cortex-A35 ARMv8 architecture and >>> have various peripheral IPs. >>> >>> Signed-off-by: Tomer Maimon <tmaimon77@gmail.com> > >>> + l2: l2-cache { >>> + compatible = "cache"; >> >> Is this a real compatible? What bindings are you using here? > > The compatible value and related properties are defined in the > Devicetree Specification, v0.4-rc1, Section 3.9 ("Multi-level and > Shared Cache Nodes (/cpus/cpu*/l?-cache)"). Indeed, thanks! > > The properties are handled by > dtschema/schemas/cache-controller.yaml, but the latter seems to lack > any checking on the compatible value? Best regards, Krzysztof
On 23/05/2022 16:17, Tomer Maimon wrote: > Hi, > > Thanks for your comments. > > the patch will modify according to your comments and will be sent in the > next kernel revision 5.19.rc1 > None of your emails reach lists because of using HTML. Please use appropriate messaging format. Best regards, Krzysztof
On Mon, May 23, 2022 at 1:59 PM Arnd Bergmann <arnd@arndb.de> wrote: > > 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 Something wrong with the script? > 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