| Message ID | 20230328021912.177301-1-ychuang570808@gmail.com |
|---|---|
| Headers | show |
| Series | Introduce Nuvoton ma35d1 SoC | expand |
On 28. 03. 23, 4:19, Jacky Huang wrote: > +static void transmit_chars(struct uart_ma35d1_port *up) > +{ > + struct circ_buf *xmit = &up->port.state->xmit; > + int count; > + u8 ch; > + > + if (uart_tx_stopped(&up->port)) { > + ma35d1serial_stop_tx(&up->port); > + return; > + } > + if (uart_circ_empty(xmit)) { > + __stop_tx(up); > + return; > + } Why is this necessary? > + count = UART_FIFO_DEPTH - ((serial_in(up, UART_REG_FSR) & FSR_TXPTR_MSK) >> 16); > + > + uart_port_tx_limited(&up->port, ch, count, > + !(serial_in(up, UART_REG_FSR) & FSR_TX_FULL), > + serial_out(up, UART_REG_THR, ch), > + ({})); > + > + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) > + uart_write_wakeup(&up->port); > + > + if (uart_circ_empty(xmit)) > + __stop_tx(up); uart_port_tx_limited() should take care about the above and this too, right? > +} ... > +static void receive_chars(struct uart_ma35d1_port *up) > +{ > + u8 flag; > + u32 fsr; > + unsigned int ch; Shouldn't ch be u8 too? > + int max_count = 256; > + > + fsr = serial_in(up, UART_REG_FSR); > + do { > + flag = TTY_NORMAL; > + up->port.icount.rx++; > + > + if (unlikely(fsr & (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF))) { > + if (fsr & FSR_BIF) { > + up->port.icount.brk++; > + if (uart_handle_break(&up->port)) > + continue; > + } > + if (fsr & FSR_FEF) > + up->port.icount.frame++; > + if (fsr & FSR_PEF) > + up->port.icount.parity++; > + if (fsr & FSR_RX_OVER_IF) > + up->port.icount.overrun++; > + > + serial_out(up, UART_REG_FSR, fsr & > + (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF)); > + > + if (fsr & FSR_BIF) > + flag = TTY_BREAK; > + else if (fsr & FSR_PEF) > + flag = TTY_PARITY; > + else if (fsr & FSR_FEF) > + flag = TTY_FRAME; > + } > + > + ch = serial_in(up, UART_REG_RBR); > + if (uart_handle_sysrq_char(&up->port, ch)) > + continue; > + > + spin_lock(&up->port.lock); > + uart_insert_char(&up->port, fsr, FSR_RX_OVER_IF, ch, flag); > + spin_unlock(&up->port.lock); > + > + fsr = serial_in(up, UART_REG_FSR); > + } while (!(fsr & FSR_RX_EMPTY) && (max_count-- > 0)); > + > + spin_lock(&up->port.lock); > + tty_flip_buffer_push(&up->port.state->port); > + spin_unlock(&up->port.lock); > +} ... > +static int ma35d1serial_remove(struct platform_device *dev) > +{ > + struct uart_port *port = platform_get_drvdata(dev); > + > + if (port) { Can this ever be NULL? > + uart_remove_one_port(&ma35d1serial_reg, port); > + free_irq(port->irq, port); > + } > + return 0; > +} > + > +static int ma35d1serial_suspend(struct platform_device *dev, pm_message_t state) > +{ > + int i; > + struct uart_ma35d1_port *up; > + > + if (dev->dev.of_node) > + i = of_alias_get_id(dev->dev.of_node, "serial"); > + if (i < 0) { > + dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", i); > + return i; > + } > + up = &ma35d1serial_ports[i]; platform_get_drvdata(dev) ? > + if (i == 0) { > + up->console_baud_rate = serial_in(up, UART_REG_BAUD); > + up->console_line = serial_in(up, UART_REG_LCR); > + up->console_int = serial_in(up, UART_REG_IER); > + } > + return 0; > +} > + > +static int ma35d1serial_resume(struct platform_device *dev) > +{ > + int i; > + struct uart_ma35d1_port *up; > + > + if (dev->dev.of_node) > + i = of_alias_get_id(dev->dev.of_node, "serial"); > + if (i < 0) { > + dev_err(&dev->dev, "failed to get alias/pdev id, errno %d\n", i); > + return i; > + } > + up = &ma35d1serial_ports[i]; > + if (i == 0) { > + serial_out(up, UART_REG_BAUD, up->console_baud_rate); > + serial_out(up, UART_REG_LCR, up->console_line); > + serial_out(up, UART_REG_IER, up->console_int); > + } > + return 0; > +} No uart_suspend_port()/uart_resume_port()? You don't wait for transmitter to be empty in suspend. You don't stop tx etc. > +static struct platform_driver ma35d1serial_driver = { > + .probe = ma35d1serial_probe, > + .remove = ma35d1serial_remove, > + .suspend = ma35d1serial_suspend, > + .resume = ma35d1serial_resume, > + .driver = { > + .name = "ma35d1-uart", > + .owner = THIS_MODULE, > + .of_match_table = of_match_ptr(ma35d1_serial_of_match), > + }, > +}; > + > +static int __init ma35d1serial_init(void) > +{ > + int ret; > + > + ret = uart_register_driver(&ma35d1serial_reg); > + if (ret) > + return ret; > + ret = platform_driver_register(&ma35d1serial_driver); > + if (ret) > + uart_unregister_driver(&ma35d1serial_reg); > + return ret; > +} > + > +static void __exit ma35d1serial_exit(void) > +{ > + platform_driver_unregister(&ma35d1serial_driver); > + uart_unregister_driver(&ma35d1serial_reg); > +} > + > +module_init(ma35d1serial_init); > +module_exit(ma35d1serial_exit); > + > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("MA35D1 serial driver"); > +MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR); Why is this needed? How are other platform drivers autoloaded? thanks,
Quoting Jacky Huang (2023-03-27 19:19:08) > diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi > new file mode 100644 > index 000000000000..0740b0b218a7 > --- /dev/null > +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi > @@ -0,0 +1,231 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2023 Nuvoton Technology Corp. > + * Author: Shan-Chun Hung <schung@nuvoton.com> > + * Jacky huang <ychuang3@nuvoton.com> > + */ > + > +#include <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/input/input.h> > +#include <dt-bindings/gpio/gpio.h> > +#include <dt-bindings/clock/nuvoton,ma35d1-clk.h> > +#include <dt-bindings/reset/nuvoton,ma35d1-reset.h> > + > +/ { > + compatible = "nuvoton,ma35d1"; > + interrupt-parent = <&gic>; > + #address-cells = <2>; > + #size-cells = <2>; > + > + cpus { > + #address-cells = <2>; > + #size-cells = <0>; > + > + cpu0: cpu@0 { > + device_type = "cpu"; > + compatible = "arm,cortex-a35"; > + reg = <0x0 0x0>; > + enable-method = "psci"; > + next-level-cache = <&L2_0>; > + }; > + > + cpu1: cpu@1 { > + device_type = "cpu"; > + compatible = "arm,cortex-a35"; > + reg = <0x0 0x1>; > + enable-method = "psci"; > + next-level-cache = <&L2_0>; > + }; > + > + L2_0: l2-cache0 { Just l2-cache for the node name. Doesn't it go under the cpu0 node as well? > + compatible = "cache"; > + cache-level = <2>; > + }; > + }; > + > + psci { > + compatible = "arm,psci-0.2"; > + method = "smc"; > + }; > + > + timer { > + compatible = "arm,armv8-timer"; > + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | > + IRQ_TYPE_LEVEL_LOW)>, /* Physical Secure */ > + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | > + IRQ_TYPE_LEVEL_LOW)>, /* Physical Non-Secure */ > + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | > + IRQ_TYPE_LEVEL_LOW)>, /* Virtual */ > + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | > + IRQ_TYPE_LEVEL_LOW)>; /* Hypervisor */ > + clock-frequency = <12000000>; Remove this property. The frequency should be read by the driver. > + interrupt-parent = <&gic>; > + }; Please create an 'soc' node for the SoC to hold all the nodes that have a reg property. > + > + sys: system-management@40460000 { > + compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd"; > + reg = <0x0 0x40460000 0x0 0x200>; > + > + reset: reset-controller { > + compatible = "nuvoton,ma35d1-reset"; > + #reset-cells = <1>; > + }; > + }; > + > + clk: clock-controller@40460200 { > + compatible = "nuvoton,ma35d1-clk", "syscon"; > + reg = <0x00000000 0x40460200 0x0 0x100>; > + #clock-cells = <1>; > + clocks = <&clk_hxt>; > + nuvoton,sys = <&sys>; > + }; It looks like the device at 40460000 is a reset and clock controller. Just make it one node and register the clk or reset device as an auxiliary device. > + > + gic: interrupt-controller@50801000 { > + compatible = "arm,gic-400"; > + reg = <0x0 0x50801000 0 0x1000>, /* GICD */ > + <0x0 0x50802000 0 0x2000>, /* GICC */ > + <0x0 0x50804000 0 0x2000>, /* GICH */ > + <0x0 0x50806000 0 0x2000>; /* GICV */ > + #interrupt-cells = <3>; > + interrupt-parent = <&gic>; > + interrupt-controller; > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) | > + IRQ_TYPE_LEVEL_HIGH)>; > + }; > + > + uart0:serial@40700000 { Add a space after : > + compatible = "nuvoton,ma35d1-uart"; > + reg = <0x0 0x40700000 0x0 0x100>; > + interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk UART0_GATE>; > + status = "disabled"; > + };
Quoting Jacky Huang (2023-03-27 19:19:09) > diff --git a/drivers/clk/nuvoton/Kconfig b/drivers/clk/nuvoton/Kconfig > new file mode 100644 > index 000000000000..c1324efedcb9 > --- /dev/null > +++ b/drivers/clk/nuvoton/Kconfig > @@ -0,0 +1,19 @@ > +# SPDX-License-Identifier: GPL-2.0 > +# common clock support for Nuvoton SoC family. > + > +config COMMON_CLK_NUVOTON > + bool "Nuvoton clock controller common support" > + depends on ARCH_NUVOTON Add || COMPILE_TEST > + default ARCH_NUVOTON > + help > + Say y here to enable common clock controller for Nuvoton platforms. > + > +if COMMON_CLK_NUVOTON Add a newline here. > +config CLK_MA35D1 > + bool "Nuvoton MA35D1 clock controller support" > + depends on ARM64 || COMPILE_TEST Drop this > + default y And this. > + help > + Build the driver for MA35D1 Clock Driver. This says driver twice. "Build the clk driver for MA35D1 SoCs"? > + > +endif > diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c > new file mode 100644 > index 000000000000..340a889a1417 > --- /dev/null > +++ b/drivers/clk/nuvoton/clk-ma35d1-divider.c > @@ -0,0 +1,140 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2023 Nuvoton Technology Corp. > + * Author: Chi-Fang Li <cfli0@nuvoton.com> > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/device.h> > +#include <linux/regmap.h> > +#include <linux/spinlock.h> > + > +#include "clk-ma35d1.h" > + > +#define clk_div_mask(width) ((1 << (width)) - 1) clk-provider.h defines this already, use that? > + > +struct ma35d1_adc_clk_div { > + struct clk_hw hw; > + void __iomem *reg; > + u8 shift; > + u8 width; > + u32 mask; > + const struct clk_div_table *table; > + /* protects concurrent access to clock divider registers */ > + spinlock_t *lock; > +}; > + > +static inline struct ma35d1_adc_clk_div *to_ma35d1_adc_clk_div(struct clk_hw *_hw) > +{ > + return container_of(_hw, struct ma35d1_adc_clk_div, hw); > +} > + > +static inline unsigned long ma35d1_clkdiv_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + unsigned int val; > + struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw); > + > + val = readl_relaxed(dclk->reg) >> dclk->shift; > + val &= clk_div_mask(dclk->width); > + val += 1; > + return divider_recalc_rate(hw, parent_rate, val, dclk->table, > + CLK_DIVIDER_ROUND_CLOSEST, dclk->width); > +} > + > +static inline long ma35d1_clkdiv_round_rate(struct clk_hw *hw, > + unsigned long rate, > + unsigned long *prate) > +{ > + struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw); > + > + return divider_round_rate(hw, rate, prate, dclk->table, > + dclk->width, CLK_DIVIDER_ROUND_CLOSEST); > +} > + > +static inline int ma35d1_clkdiv_set_rate(struct clk_hw *hw, > + unsigned long rate, > + unsigned long parent_rate) > +{ > + int value; > + unsigned long flags = 0; > + u32 data; > + struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw); > + > + value = divider_get_val(rate, parent_rate, dclk->table, > + dclk->width, CLK_DIVIDER_ROUND_CLOSEST); > + > + if (dclk->lock) > + spin_lock_irqsave(dclk->lock, flags); Can you just always lock? That makes the conditional locking go away. > + > + data = readl_relaxed(dclk->reg); > + data &= ~(clk_div_mask(dclk->width) << dclk->shift); > + data |= (value - 1) << dclk->shift; > + data |= dclk->mask; > + > + writel_relaxed(data, dclk->reg); > + > + if (dclk->lock) > + spin_unlock_irqrestore(dclk->lock, flags); > + > + return 0; > +} > + > +static const struct clk_ops ma35d1_adc_clkdiv_ops = { > + .recalc_rate = ma35d1_clkdiv_recalc_rate, > + .round_rate = ma35d1_clkdiv_round_rate, > + .set_rate = ma35d1_clkdiv_set_rate, > +}; > + > +struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name, > + const char *parent_name, > + spinlock_t *lock, > + unsigned long flags, void __iomem *reg, > + u8 shift, u8 width, u32 mask_bit) > +{ > + struct ma35d1_adc_clk_div *div; > + struct clk_init_data init; > + struct clk_div_table *table; > + u32 max_div, min_div; > + struct clk_hw *hw; > + int ret; > + int i; > + > + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); > + if (!div) > + return ERR_PTR(-ENOMEM); > + > + max_div = clk_div_mask(width) + 1; > + min_div = 1; > + > + table = devm_kcalloc(dev, max_div + 1, sizeof(*table), GFP_KERNEL); > + if (!table) > + return ERR_PTR(-ENOMEM); > + > + for (i = 0; i < max_div; i++) { > + table[i].val = (min_div + i); Drop useless parenthesis. > + table[i].div = 2 * table[i].val; > + } > + table[max_div].val = 0; > + table[max_div].div = 0; > + > + init.name = name; > + init.ops = &ma35d1_adc_clkdiv_ops; > + init.flags |= flags; > + init.parent_names = parent_name ? &parent_name : NULL; > + init.num_parents = parent_name ? 1 : 0; > + > + div->reg = reg; > + div->shift = shift; > + div->width = width; > + div->mask = mask_bit ? BIT(mask_bit) : 0; > + div->lock = lock; > + div->hw.init = &init; > + div->table = table; > + > + hw = &div->hw; > + ret = devm_clk_hw_register(dev, hw); > + if (ret < 0) Use if (ret) instead. > + return ERR_PTR(ret); > + return hw; > +} > diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c b/drivers/clk/nuvoton/clk-ma35d1-pll.c > new file mode 100644 > index 000000000000..b36fbda4fa0a > --- /dev/null > +++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c > @@ -0,0 +1,350 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2023 Nuvoton Technology Corp. > + * Author: Chi-Fang Li <cfli0@nuvoton.com> > + */ > + > +#include <linux/bitfield.h> > +#include <linux/clk-provider.h> > +#include <linux/slab.h> > +#include <linux/regmap.h> > + > +#include "clk-ma35d1.h" > + > +#define REG_SYS_RLKTZNS 0x1a4 /* Register Lock Control */ > + > +struct ma35d1_clk_pll { > + struct clk_hw hw; > + u8 type; > + u8 mode; > + unsigned long rate; > + void __iomem *ctl0_base; This needs an include for __iomem define. Just include kernel.h for now. > + void __iomem *ctl1_base; > + void __iomem *ctl2_base; > + struct regmap *regmap; > +}; > + > +static inline struct ma35d1_clk_pll *to_ma35d1_clk_pll(struct clk_hw *_hw) > +{ > + return container_of(_hw, struct ma35d1_clk_pll, hw); Missing include as well. > +} > + > +static void ma35d1_unlock_regs(struct ma35d1_clk_pll *pll) > +{ > + int ret; > + > + do { > + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x59); > + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x16); > + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x88); > + regmap_read(pll->regmap, REG_SYS_RLKTZNS, &ret); > + } while (ret == 0); Any sort of timeout? > +} > + > +static void ma35d1_lock_regs(struct ma35d1_clk_pll *pll) > +{ > + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x0); > +} > + > +static unsigned long ma35d1_calc_smic_pll_freq(u32 pll0_ctl0, > + unsigned long parent_rate) > +{ > + u32 m, n, p, outdiv; > + u64 pll_freq; > + u32 clk_div_table[] = { 1, 2, 4, 8 }; const > + > + if (pll0_ctl0 & SPLL0_CTL0_BP) > + return parent_rate; > + > + n = FIELD_GET(SPLL0_CTL0_FBDIV, pll0_ctl0); > + m = FIELD_GET(SPLL0_CTL0_INDIV, pll0_ctl0); > + p = FIELD_GET(SPLL0_CTL0_OUTDIV, pll0_ctl0); > + outdiv = clk_div_table[p]; > + pll_freq = (u64)parent_rate * n; > + do_div(pll_freq, m * outdiv); > + return (unsigned long)pll_freq; Remove useless cast. > +} > + > +static unsigned long ma35d1_calc_pll_freq(u8 mode, u32 *reg_ctl, > + unsigned long parent_rate) > +{ > + u32 m, n, p; > + u64 pll_freq, x; > + > + if (reg_ctl[1] & PLL_CTL1_BP) > + return parent_rate; > + > + if (mode == PLL_MODE_INT) { > + n = FIELD_GET(PLL_CTL0_FBDIV, reg_ctl[0]); > + m = FIELD_GET(PLL_CTL0_INDIV, reg_ctl[0]); > + p = FIELD_GET(PLL_CTL1_OUTDIV, reg_ctl[1]); > + pll_freq = (u64)parent_rate * n; > + do_div(pll_freq, m * p); > + } else { > + n = FIELD_GET(PLL_CTL0_FBDIV, reg_ctl[0]); > + m = FIELD_GET(PLL_CTL0_INDIV, reg_ctl[0]); > + p = FIELD_GET(PLL_CTL1_OUTDIV, reg_ctl[1]); The n, m, p are the same, so pull them out of the if-else to deduplicate. > + x = FIELD_GET(PLL_CTL1_FRAC, reg_ctl[1]); > + /* 2 decimal places floating to integer (ex. 1.23 to 123) */ > + n = n * 100 + ((x * 100) / FIELD_MAX(PLL_CTL1_FRAC)); Is this mult_frac()? > + pll_freq = ((u64)parent_rate * n) / 100 / m / p; > + } Add a newline > + return (unsigned long)pll_freq; Remove useless cast. > +} > + > +static int ma35d1_pll_find_closest(struct ma35d1_clk_pll *pll, > + unsigned long rate, > + unsigned long parent_rate, > + u32 *reg_ctl, unsigned long *freq) > +{ > + int p, m, n; > + int fbdiv_min, fbdiv_max; > + unsigned long diff = 0xffffffff; > + > + *freq = 0; > + if ((rate < PLL_FCLKO_MIN_FREQ) || (rate > PLL_FCLKO_MAX_FREQ)) > + return -EINVAL; > + > + if (pll->mode == PLL_MODE_INT) { > + fbdiv_min = FBDIV_MIN; > + fbdiv_max = FBDIV_MAX; > + } else { > + fbdiv_min = FBDIV_FRAC_MIN; > + fbdiv_max = FBDIV_FRAC_MAX; > + } > + > + for (m = INDIV_MIN; m <= INDIV_MAX; m++) { > + for (n = fbdiv_min; n <= fbdiv_max; n++) { > + for (p = OUTDIV_MIN; p <= OUTDIV_MAX; p++) { > + unsigned long tmp, fout; > + u64 fclk; > + > + tmp = parent_rate / m; > + if (tmp < PLL_FREF_M_MIN_FREQ || > + tmp > PLL_FREF_M_MAX_FREQ) > + continue; /* constrain */ > + > + fclk = (u64)parent_rate * n / m; > + /* for 2 decimal places */ > + if (pll->mode != PLL_MODE_INT) > + fclk /= 100; > + > + if (fclk < PLL_FCLK_MIN_FREQ || > + fclk > PLL_FCLK_MAX_FREQ) > + continue; /* constrain */ > + > + fout = (unsigned long)(fclk / p); > + if (fout < PLL_FCLKO_MIN_FREQ || > + fout > PLL_FCLKO_MAX_FREQ) > + continue; /* constrain */ > + > + if (abs(rate - fout) < diff) { > + reg_ctl[0] = FIELD_PREP(PLL_CTL0_INDIV, m) | > + FIELD_PREP(PLL_CTL0_FBDIV, n); > + reg_ctl[1] = FIELD_PREP(PLL_CTL1_OUTDIV, p); > + *freq = fout; > + diff = abs(rate - fout); > + if (diff == 0) > + break; > + } > + } > + } > + } > + if (*freq == 0) > + return -EINVAL; /* cannot find even one valid setting */ > + return 0; > +} > + > +static int ma35d1_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); > + u32 reg_ctl[3] = { 0 }; > + unsigned long pll_freq; > + int ret; > + > + if ((parent_rate < PLL_FREF_MIN_FREQ) || > + (parent_rate > PLL_FREF_MAX_FREQ)) Remove useless parenthesis. > + return -EINVAL; > + > + if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) { Remove useless parenthesis. > + pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n"); > + return -EACCES; > + } > + > + ret = ma35d1_pll_find_closest(pll, rate, parent_rate, reg_ctl, &pll_freq); > + if (ret != 0) > + return ret; > + pll->rate = pll_freq; > + > + switch (pll->mode) { > + case PLL_MODE_INT: > + reg_ctl[0] |= FIELD_PREP(PLL_CTL0_MODE, PLL_MODE_INT); > + break; > + case PLL_MODE_FRAC: > + reg_ctl[0] |= FIELD_PREP(PLL_CTL0_MODE, PLL_MODE_FRAC); > + break; > + case PLL_MODE_SS: > + reg_ctl[0] |= FIELD_PREP(PLL_CTL0_MODE, PLL_MODE_SS) | > + FIELD_PREP(PLL_CTL0_SSRATE, PLL_SS_RATE); > + reg_ctl[2] = FIELD_PREP(PLL_CTL2_SLOPE, PLL_SLOPE); > + break; > + } > + reg_ctl[1] |= PLL_CTL1_PD; > + > + ma35d1_unlock_regs(pll); > + writel_relaxed(reg_ctl[0], pll->ctl0_base); > + writel_relaxed(reg_ctl[1], pll->ctl1_base); > + writel_relaxed(reg_ctl[2], pll->ctl2_base); > + ma35d1_lock_regs(pll); > + return 0; > +} > + > +static unsigned long ma35d1_clk_pll_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); > + u32 reg_ctl[3]; > + unsigned long pll_freq; > + > + if ((parent_rate < PLL_FREF_MIN_FREQ) || (parent_rate > PLL_FREF_MAX_FREQ)) Remove useless parenthesis. > + return 0; > + > + switch (pll->type) { > + case MA35D1_CAPLL: > + reg_ctl[0] = readl_relaxed(pll->ctl0_base); > + pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], parent_rate); return? > + break; > + case MA35D1_DDRPLL: > + case MA35D1_APLL: > + case MA35D1_EPLL: > + case MA35D1_VPLL: > + reg_ctl[0] = readl_relaxed(pll->ctl0_base); > + reg_ctl[1] = readl_relaxed(pll->ctl1_base); > + pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, parent_rate); return? > + break; > + } > + return pll_freq; return 0? > +} > + > +static long ma35d1_clk_pll_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); > + u32 reg_ctl[3] = { 0 }; > + unsigned long pll_freq; > + long ret; > + > + if (*parent_rate < PLL_FREF_MIN_FREQ || *parent_rate > PLL_FREF_MAX_FREQ) > + return -EINVAL; > + > + ret = ma35d1_pll_find_closest(pll, rate, *parent_rate, reg_ctl, &pll_freq); > + if (ret != 0) > + return ret; > + > + switch (pll->type) { > + case MA35D1_CAPLL: > + reg_ctl[0] = readl_relaxed(pll->ctl0_base); > + pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], *parent_rate); return? > + break; > + case MA35D1_DDRPLL: > + case MA35D1_APLL: > + case MA35D1_EPLL: > + case MA35D1_VPLL: > + reg_ctl[0] = readl_relaxed(pll->ctl0_base); > + reg_ctl[1] = readl_relaxed(pll->ctl1_base); > + pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, *parent_rate); return? > + break; > + } > + return pll_freq; return 0? > +} > + > +static int ma35d1_clk_pll_is_prepared(struct clk_hw *hw) > +{ > + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); > + u32 val = readl_relaxed(pll->ctl1_base); > + > + return val & PLL_CTL1_PD ? 0 : 1; > +} > + > +static int ma35d1_clk_pll_prepare(struct clk_hw *hw) > +{ > + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); > + u32 val; > + > + if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) { Drop useless parenthesis. > + pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n"); > + return -EACCES; > + } > + > + ma35d1_unlock_regs(pll); > + val = readl_relaxed(pll->ctl1_base); > + val &= ~PLL_CTL1_PD; > + writel_relaxed(val, pll->ctl1_base); > + ma35d1_lock_regs(pll); > + return 0; > +} > + > +static void ma35d1_clk_pll_unprepare(struct clk_hw *hw) > +{ > + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); > + u32 val; > + > + if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) { Drop useless parenthesis. > + pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n"); These read-only checks should be removed and different clk_ops should be assigned for the read-only type of clks. > + } else { > + val = readl_relaxed(pll->ctl1_base); > + val |= PLL_CTL1_PD; > + writel_relaxed(val, pll->ctl1_base); > + } > +} > + > +static const struct clk_ops ma35d1_clk_pll_ops = { > + .is_prepared = ma35d1_clk_pll_is_prepared, > + .prepare = ma35d1_clk_pll_prepare, > + .unprepare = ma35d1_clk_pll_unprepare, > + .set_rate = ma35d1_clk_pll_set_rate, > + .recalc_rate = ma35d1_clk_pll_recalc_rate, > + .round_rate = ma35d1_clk_pll_round_rate, > +}; > + > +struct clk_hw *ma35d1_reg_clk_pll(enum ma35d1_pll_type type, Pass a device here. > + u8 u8mode, const char *name, > + const char *parent, > + unsigned long targetFreq, > + void __iomem *base, > + struct regmap *regmap) > +{ > + struct ma35d1_clk_pll *pll; > + struct clk_hw *hw; > + struct clk_init_data init; init = {}; > + int ret; > + > + pll = kmalloc(sizeof(*pll), GFP_KERNEL); Just use kzalloc(). This isn't a hot path. > + if (!pll) > + return ERR_PTR(-ENOMEM); > + > + pll->type = type; > + pll->mode = u8mode; > + pll->rate = targetFreq; > + pll->ctl0_base = base + REG_PLL_CTL0_OFFSET; > + pll->ctl1_base = base + REG_PLL_CTL1_OFFSET; > + pll->ctl2_base = base + REG_PLL_CTL2_OFFSET; > + pll->regmap = regmap; > + > + init.name = name; > + init.flags = 0; > + init.parent_names = &parent; > + init.num_parents = 1; > + init.ops = &ma35d1_clk_pll_ops; > + pll->hw.init = &init; > + hw = &pll->hw; > + > + ret = clk_hw_register(NULL, hw); Use devm_ > + if (ret) { > + pr_err("failed to register vsi-pll clock!!!\n"); I'm going to put a dev_err_probe() into clk registration code. Remove this printk. > + kfree(pll); > + return ERR_PTR(ret); > + } > + return hw; > +} > diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c > new file mode 100644 > index 000000000000..e4d3ced396a3 > --- /dev/null > +++ b/drivers/clk/nuvoton/clk-ma35d1.c > @@ -0,0 +1,963 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2023 Nuvoton Technology Corp. > + * Author: Chi-Fang Li <cfli0@nuvoton.com> > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/mfd/syscon.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/platform_device.h> > +#include <linux/spinlock.h> > +#include <dt-bindings/clock/nuvoton,ma35d1-clk.h> > + > +#include "clk-ma35d1.h" > + > +static DEFINE_SPINLOCK(ma35d1_lock); > + > +static const char *const ca35clk_sel_clks[] = { Use clk_parent_data instead. > + "hxt", "capll", "ddrpll", "dummy" > +}; > + [...] > + > +static inline struct clk_hw *ma35d1_clk_fixed(const char *name, int rate) > +{ > + return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate); > +} > + > +static inline struct clk_hw *ma35d1_clk_mux(const char *name, > + void __iomem *reg, > + u8 shift, u8 width, > + const char *const *parents, > + int num_parents) > +{ > + return clk_hw_register_mux(NULL, name, parents, num_parents, > + CLK_SET_RATE_NO_REPARENT, reg, shift, > + width, 0, &ma35d1_lock); > +} > + > +static inline struct clk_hw *ma35d1_clk_divider(const char *name, > + const char *parent, > + void __iomem *reg, u8 shift, > + u8 width) > +{ > + return clk_hw_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT, > + reg, shift, width, 0, &ma35d1_lock); > +} > + > +static inline struct clk_hw *ma35d1_clk_divider_pow2(const char *name, > + const char *parent, > + void __iomem *reg, > + u8 shift, u8 width) > +{ > + return clk_hw_register_divider(NULL, name, parent, > + CLK_DIVIDER_POWER_OF_TWO, reg, shift, > + width, 0, &ma35d1_lock); > +} > + > +static inline struct clk_hw *ma35d1_clk_divider_table(const char *name, > + const char *parent, > + void __iomem *reg, > + u8 shift, u8 width, > + const struct clk_div_table *table) > +{ > + return clk_hw_register_divider_table(NULL, name, parent, 0, > + reg, shift, width, 0, table, > + &ma35d1_lock); > +} > + > +static inline struct clk_hw *ma35d1_clk_fixed_factor(const char *name, > + const char *parent, > + unsigned int mult, > + unsigned int div) > +{ > + return clk_hw_register_fixed_factor(NULL, name, parent, > + CLK_SET_RATE_PARENT, mult, div); > +} > + > +static inline struct clk_hw *ma35d1_clk_gate(const char *name, > + const char *parent, > + void __iomem *reg, u8 shift) > +{ > + return clk_hw_register_gate(NULL, name, parent, CLK_SET_RATE_PARENT, > + reg, shift, 0, &ma35d1_lock); > +} These need to use devm_ to unregister on failure. > + > +static int ma35d1_get_pll_setting(struct device_node *clk_node, > + u32 *pllmode, u32 *pllfreq) > +{ > + const char *of_str; > + int i; > + > + for (i = 0; i < PLL_MAX_NUM; i++) { > + if (of_property_read_string_index(clk_node, "nuvoton,pll-mode", i, &of_str)) > + return -EINVAL; > + if (!strcmp(of_str, "integer")) > + pllmode[i] = PLL_MODE_INT; > + else if (!strcmp(of_str, "fractional")) > + pllmode[i] = PLL_MODE_FRAC; > + else if (!strcmp(of_str, "spread-spectrum")) > + pllmode[i] = PLL_MODE_SS; > + else > + return -EINVAL; > + } > + return of_property_read_u32_array(clk_node, "assigned-clock-rates", The clk framework looks at this property. Why is the driver looking at it too? > + pllfreq, PLL_MAX_NUM); > +} > +
Dear Stephen, Thanks for your advice. On 2023/3/29 上午 01:57, Stephen Boyd wrote: > Quoting Jacky Huang (2023-03-27 19:19:08) >> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi >> new file mode 100644 >> index 000000000000..0740b0b218a7 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi >> @@ -0,0 +1,231 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (C) 2023 Nuvoton Technology Corp. >> + * Author: Shan-Chun Hung <schung@nuvoton.com> >> + * Jacky huang <ychuang3@nuvoton.com> >> + */ >> + >> +#include <dt-bindings/interrupt-controller/arm-gic.h> >> +#include <dt-bindings/input/input.h> >> +#include <dt-bindings/gpio/gpio.h> >> +#include <dt-bindings/clock/nuvoton,ma35d1-clk.h> >> +#include <dt-bindings/reset/nuvoton,ma35d1-reset.h> >> + >> +/ { >> + compatible = "nuvoton,ma35d1"; >> + interrupt-parent = <&gic>; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + cpus { >> + #address-cells = <2>; >> + #size-cells = <0>; >> + >> + cpu0: cpu@0 { >> + device_type = "cpu"; >> + compatible = "arm,cortex-a35"; >> + reg = <0x0 0x0>; >> + enable-method = "psci"; >> + next-level-cache = <&L2_0>; >> + }; >> + >> + cpu1: cpu@1 { >> + device_type = "cpu"; >> + compatible = "arm,cortex-a35"; >> + reg = <0x0 0x1>; >> + enable-method = "psci"; >> + next-level-cache = <&L2_0>; >> + }; >> + >> + L2_0: l2-cache0 { > Just l2-cache for the node name. Doesn't it go under the cpu0 node as > well? This describes the level-2 cache which is external to and shared by cpu0 & cpu1. And only level-1 cache is inside of CPU core. L2_0 is must, because both cpu0 and cpu1 has a next-level-cache = <&L2_0> property. Many identical example of l2-cache node can be found in arm64 dts, such as k3-arm642.dtsi, rk3328.dtsi, mt8195.dtsi, etc. Here is just a copy of similar arm64 multi-core SoCs. So we would like to keep this unchanged. Is it OK for you? Thanks. >> + compatible = "cache"; >> + cache-level = <2>; >> + }; >> + }; >> + >> + psci { >> + compatible = "arm,psci-0.2"; >> + method = "smc"; >> + }; >> + >> + timer { >> + compatible = "arm,armv8-timer"; >> + interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(4) | >> + IRQ_TYPE_LEVEL_LOW)>, /* Physical Secure */ >> + <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(4) | >> + IRQ_TYPE_LEVEL_LOW)>, /* Physical Non-Secure */ >> + <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(4) | >> + IRQ_TYPE_LEVEL_LOW)>, /* Virtual */ >> + <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(4) | >> + IRQ_TYPE_LEVEL_LOW)>; /* Hypervisor */ >> + clock-frequency = <12000000>; > Remove this property. The frequency should be read by the driver. I will remove it. >> + interrupt-parent = <&gic>; >> + }; > Please create an 'soc' node for the SoC to hold all the nodes that have > a reg property. OK, we will use soc node in the next version. >> + >> + sys: system-management@40460000 { >> + compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd"; >> + reg = <0x0 0x40460000 0x0 0x200>; >> + >> + reset: reset-controller { >> + compatible = "nuvoton,ma35d1-reset"; >> + #reset-cells = <1>; >> + }; >> + }; >> + >> + clk: clock-controller@40460200 { >> + compatible = "nuvoton,ma35d1-clk", "syscon"; >> + reg = <0x00000000 0x40460200 0x0 0x100>; >> + #clock-cells = <1>; >> + clocks = <&clk_hxt>; >> + nuvoton,sys = <&sys>; >> + }; > It looks like the device at 40460000 is a reset and clock controller. > Just make it one node and register the clk or reset device as an > auxiliary device. 40460000 is for system control registers, including power contrl, multifunction pin control, usb phy control, IP reset control, power-on setting information, and many other miscellaneous controls. The registers of reset controller is only a subset of system control registers. 40460200 is for clock controller which is independent of the system control integration The register base of clock controller is very close to system controller, but in fact the two are independent. >> + >> + gic: interrupt-controller@50801000 { >> + compatible = "arm,gic-400"; >> + reg = <0x0 0x50801000 0 0x1000>, /* GICD */ >> + <0x0 0x50802000 0 0x2000>, /* GICC */ >> + <0x0 0x50804000 0 0x2000>, /* GICH */ >> + <0x0 0x50806000 0 0x2000>; /* GICV */ >> + #interrupt-cells = <3>; >> + interrupt-parent = <&gic>; >> + interrupt-controller; >> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) | >> + IRQ_TYPE_LEVEL_HIGH)>; >> + }; >> + >> + uart0:serial@40700000 { > Add a space after : I will fix it. Thank you. >> + compatible = "nuvoton,ma35d1-uart"; >> + reg = <0x0 0x40700000 0x0 0x100>; >> + interrupts = <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&clk UART0_GATE>; >> + status = "disabled"; >> + }; Best regards, Jacky Huang
Quoting Jacky Huang (2023-03-28 19:03:24) > On 2023/3/29 上午 01:57, Stephen Boyd wrote: > > Quoting Jacky Huang (2023-03-27 19:19:08) > >> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi > >> new file mode 100644 > >> index 000000000000..0740b0b218a7 > >> --- /dev/null > >> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi > >> @@ -0,0 +1,231 @@ [...] > >> + > >> + L2_0: l2-cache0 { > > Just l2-cache for the node name. Doesn't it go under the cpu0 node as > > well? > > This describes the level-2 cache which is external to and shared by cpu0 > & cpu1. > And only level-1 cache is inside of CPU core. > L2_0 is must, because both cpu0 and cpu1 has a next-level-cache = > <&L2_0> property. Ok. The name should just be l2-cache then, not l2-cache0. > > Many identical example of l2-cache node can be found in arm64 dts, such > as k3-arm642.dtsi, > rk3328.dtsi, mt8195.dtsi, etc. Here is just a copy of similar arm64 > multi-core SoCs. > > So we would like to keep this unchanged. Is it OK for you? Thanks. > Mostly ok, yes. > > >> + > >> + sys: system-management@40460000 { > >> + compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd"; > >> + reg = <0x0 0x40460000 0x0 0x200>; > >> + > >> + reset: reset-controller { > >> + compatible = "nuvoton,ma35d1-reset"; > >> + #reset-cells = <1>; > >> + }; > >> + }; > >> + > >> + clk: clock-controller@40460200 { > >> + compatible = "nuvoton,ma35d1-clk", "syscon"; > >> + reg = <0x00000000 0x40460200 0x0 0x100>; > >> + #clock-cells = <1>; > >> + clocks = <&clk_hxt>; > >> + nuvoton,sys = <&sys>; > >> + }; > > It looks like the device at 40460000 is a reset and clock controller. > > Just make it one node and register the clk or reset device as an > > auxiliary device. > > 40460000 is for system control registers, including power contrl, > multifunction pin control, > usb phy control, IP reset control, power-on setting information, and > many other miscellaneous controls. > The registers of reset controller is only a subset of system control > registers. > > 40460200 is for clock controller which is independent of the system > control integration > The register base of clock controller is very close to system > controller, but in fact the two are independent. What do you use the syscon for then? The clock driver must want to use the syscon for something, implying that they are the same device.
Dear Stephen, On 2023/3/29 上午 10:19, Stephen Boyd wrote: > Quoting Jacky Huang (2023-03-28 19:03:24) >> On 2023/3/29 上午 01:57, Stephen Boyd wrote: >>> Quoting Jacky Huang (2023-03-27 19:19:08) >>>> diff --git a/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi >>>> new file mode 100644 >>>> index 000000000000..0740b0b218a7 >>>> --- /dev/null >>>> +++ b/arch/arm64/boot/dts/nuvoton/ma35d1.dtsi >>>> @@ -0,0 +1,231 @@ > [...] >>>> + >>>> + L2_0: l2-cache0 { >>> Just l2-cache for the node name. Doesn't it go under the cpu0 node as >>> well? >> This describes the level-2 cache which is external to and shared by cpu0 >> & cpu1. >> And only level-1 cache is inside of CPU core. >> L2_0 is must, because both cpu0 and cpu1 has a next-level-cache = >> <&L2_0> property. > Ok. The name should just be l2-cache then, not l2-cache0. OK, I will fix it. >> Many identical example of l2-cache node can be found in arm64 dts, such >> as k3-arm642.dtsi, >> rk3328.dtsi, mt8195.dtsi, etc. Here is just a copy of similar arm64 >> multi-core SoCs. >> >> So we would like to keep this unchanged. Is it OK for you? Thanks. >> > Mostly ok, yes. Thank you for the agreement. > >>>> + >>>> + sys: system-management@40460000 { >>>> + compatible = "nuvoton,ma35d1-sys", "syscon", "simple-mfd"; >>>> + reg = <0x0 0x40460000 0x0 0x200>; >>>> + >>>> + reset: reset-controller { >>>> + compatible = "nuvoton,ma35d1-reset"; >>>> + #reset-cells = <1>; >>>> + }; >>>> + }; >>>> + >>>> + clk: clock-controller@40460200 { >>>> + compatible = "nuvoton,ma35d1-clk", "syscon"; >>>> + reg = <0x00000000 0x40460200 0x0 0x100>; >>>> + #clock-cells = <1>; >>>> + clocks = <&clk_hxt>; >>>> + nuvoton,sys = <&sys>; >>>> + }; >>> It looks like the device at 40460000 is a reset and clock controller. >>> Just make it one node and register the clk or reset device as an >>> auxiliary device. >> 40460000 is for system control registers, including power contrl, >> multifunction pin control, >> usb phy control, IP reset control, power-on setting information, and >> many other miscellaneous controls. >> The registers of reset controller is only a subset of system control >> registers. >> >> 40460200 is for clock controller which is independent of the system >> control integration >> The register base of clock controller is very close to system >> controller, but in fact the two are independent. > What do you use the syscon for then? The clock driver must want to use > the syscon for something, implying that they are the same device. The register lock mechanism is applied to protect many critical registers from false written. The register lock control register is one register in system controller. Some registers of the clock controller are lock protected. Not only clock controller, but other IP such as RTC, PWM, ADC, etc, also have lock protected registers. All these IP requires syscon to access the lock/unlock control register in the system controller. That's why we add a <&sys> to the clock controller. Should we implement a ma35d1-sysctl driver to protect register_lock() and register_unlock() and export to those drivers? If yes, we can remove the <&sys> from clock controller. Best regards, Jacky Huang
Quoting Jacky Huang (2023-03-28 19:39:36) > On 2023/3/29 上午 10:19, Stephen Boyd wrote: > > What do you use the syscon for then? The clock driver must want to use > > the syscon for something, implying that they are the same device. > > The register lock mechanism is applied to protect many critical > registers from false written. > The register lock control register is one register in system controller. > Some registers of the clock controller are lock protected. Not only > clock controller, but other > IP such as RTC, PWM, ADC, etc, also have lock protected registers. All > these IP requires > syscon to access the lock/unlock control register in the system controller. > That's why we add a <&sys> to the clock controller. > > Should we implement a ma35d1-sysctl driver to protect register_lock() > and register_unlock() > and export to those drivers? If yes, we can remove the <&sys> from > clock controller. > You can implement the lock and unlock in the hwspinlock framework. See drivers/hwspinlock.
Dear Stephen, On 2023/3/29 上午 10:46, Stephen Boyd wrote: > Quoting Jacky Huang (2023-03-28 19:39:36) >> On 2023/3/29 上午 10:19, Stephen Boyd wrote: >>> What do you use the syscon for then? The clock driver must want to use >>> the syscon for something, implying that they are the same device. >> The register lock mechanism is applied to protect many critical >> registers from false written. >> The register lock control register is one register in system controller. >> Some registers of the clock controller are lock protected. Not only >> clock controller, but other >> IP such as RTC, PWM, ADC, etc, also have lock protected registers. All >> these IP requires >> syscon to access the lock/unlock control register in the system controller. >> That's why we add a <&sys> to the clock controller. >> >> Should we implement a ma35d1-sysctl driver to protect register_lock() >> and register_unlock() >> and export to those drivers? If yes, we can remove the <&sys> from >> clock controller. >> > You can implement the lock and unlock in the hwspinlock framework. See > drivers/hwspinlock. I may not explain clearly enough. The lock/unlock register of system controller is more like a kind of write protection for specific registers, rather than preventing hetero-core CPU access. In many different IP of ma35d1 contain write protected registers. In fact, ma35d1 has a "hardware semaphore" IP, and we have implemented the driver in drivers/hwspinlock. Even the control register of "hardware semaphore" is also write protected. So, should we implement a system controller driver to provide register_unlock() function? Is it OK to have such a driver in drivers/mfd? Or, just use syscon in device tree for those devices that have write protect registers. Best regards, Jacky Huang
Quoting Jacky Huang (2023-03-28 20:13:11) > Dear Stephen, > > > On 2023/3/29 上午 10:46, Stephen Boyd wrote: > > Quoting Jacky Huang (2023-03-28 19:39:36) > >> On 2023/3/29 上午 10:19, Stephen Boyd wrote: > >>> What do you use the syscon for then? The clock driver must want to use > >>> the syscon for something, implying that they are the same device. > >> The register lock mechanism is applied to protect many critical > >> registers from false written. > >> The register lock control register is one register in system controller. > >> Some registers of the clock controller are lock protected. Not only > >> clock controller, but other > >> IP such as RTC, PWM, ADC, etc, also have lock protected registers. All > >> these IP requires > >> syscon to access the lock/unlock control register in the system controller. > >> That's why we add a <&sys> to the clock controller. > >> > >> Should we implement a ma35d1-sysctl driver to protect register_lock() > >> and register_unlock() > >> and export to those drivers? If yes, we can remove the <&sys> from > >> clock controller. > >> > > You can implement the lock and unlock in the hwspinlock framework. See > > drivers/hwspinlock. > > I may not explain clearly enough. The lock/unlock register of system > controller is more like > a kind of write protection for specific registers, rather than > preventing hetero-core CPU access. > In many different IP of ma35d1 contain write protected registers. > In fact, ma35d1 has a "hardware semaphore" IP, and we have implemented > the driver in drivers/hwspinlock. > Even the control register of "hardware semaphore" is also write protected. What's the need to lock and unlock the registers? Is some other processor also writing to the registers that we need to synchronize against? Or is Linux the only entity reading and writing the registers? I'm wondering if we should simply unlock the registers and never lock them. > > So, should we implement a system controller driver to provide > register_unlock() function? > Is it OK to have such a driver in drivers/mfd? > Or, just use syscon in device tree for those devices that have write > protect registers. > The hwspinlock framework doesn't require there to be another entity accessing some resource. It's there to implement hardware locks. I don't see why it can't be used here.
Dear Stephen, On 2023/3/29 上午 11:25, Stephen Boyd wrote: > Quoting Jacky Huang (2023-03-28 20:13:11) >> Dear Stephen, >> >> >> On 2023/3/29 上午 10:46, Stephen Boyd wrote: >>> Quoting Jacky Huang (2023-03-28 19:39:36) >>>> On 2023/3/29 上午 10:19, Stephen Boyd wrote: >>>>> What do you use the syscon for then? The clock driver must want to use >>>>> the syscon for something, implying that they are the same device. >>>> The register lock mechanism is applied to protect many critical >>>> registers from false written. >>>> The register lock control register is one register in system controller. >>>> Some registers of the clock controller are lock protected. Not only >>>> clock controller, but other >>>> IP such as RTC, PWM, ADC, etc, also have lock protected registers. All >>>> these IP requires >>>> syscon to access the lock/unlock control register in the system controller. >>>> That's why we add a <&sys> to the clock controller. >>>> >>>> Should we implement a ma35d1-sysctl driver to protect register_lock() >>>> and register_unlock() >>>> and export to those drivers? If yes, we can remove the <&sys> from >>>> clock controller. >>>> >>> You can implement the lock and unlock in the hwspinlock framework. See >>> drivers/hwspinlock. >> I may not explain clearly enough. The lock/unlock register of system >> controller is more like >> a kind of write protection for specific registers, rather than >> preventing hetero-core CPU access. >> In many different IP of ma35d1 contain write protected registers. >> In fact, ma35d1 has a "hardware semaphore" IP, and we have implemented >> the driver in drivers/hwspinlock. >> Even the control register of "hardware semaphore" is also write protected. > What's the need to lock and unlock the registers? Is some other > processor also writing to the registers that we need to synchronize > against? Or is Linux the only entity reading and writing the registers? > I'm wondering if we should simply unlock the registers and never lock > them. > >> So, should we implement a system controller driver to provide >> register_unlock() function? >> Is it OK to have such a driver in drivers/mfd? >> Or, just use syscon in device tree for those devices that have write >> protect registers. >> > The hwspinlock framework doesn't require there to be another entity > accessing some resource. It's there to implement hardware locks. I don't > see why it can't be used here. The current usage of register lock/unlock protect is as the following code: static void ma35d1_unlock_regs(struct ma35d1_clk_pll *pll) { int ret; do { regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x59); regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x16); regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x88); regmap_read(pll->regmap, REG_SYS_RLKTZNS, &ret); } while (ret == 0); } static void ma35d1_lock_regs(struct ma35d1_clk_pll *pll) { regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x0); } And the following code is to unlock registers for write and then lock again. ma35d1_unlock_regs(pll); writel_relaxed(reg_ctl[0], pll->ctl0_base); writel_relaxed(reg_ctl[1], pll->ctl1_base); writel_relaxed(reg_ctl[2], pll->ctl2_base); ma35d1_lock_regs(pll); The above code is from the clk-ma35d1-pll.c from this patchset. We just employ regmap mechansim for the access to REG_SYS_RLKTZNS register. Is this implementation OK for you? Thank you. Best regards, Jacky Huang
Quoting Jacky Huang (2023-03-28 20:43:23) > On 2023/3/29 上午 11:25, Stephen Boyd wrote: > > Quoting Jacky Huang (2023-03-28 20:13:11) > >> I may not explain clearly enough. The lock/unlock register of system > >> controller is more like > >> a kind of write protection for specific registers, rather than > >> preventing hetero-core CPU access. > >> In many different IP of ma35d1 contain write protected registers. > >> In fact, ma35d1 has a "hardware semaphore" IP, and we have implemented > >> the driver in drivers/hwspinlock. > >> Even the control register of "hardware semaphore" is also write protected. > > What's the need to lock and unlock the registers? Is some other > > processor also writing to the registers that we need to synchronize > > against? Or is Linux the only entity reading and writing the registers? > > I'm wondering if we should simply unlock the registers and never lock > > them. Can you answer this question? > > > >> So, should we implement a system controller driver to provide > >> register_unlock() function? > >> Is it OK to have such a driver in drivers/mfd? > >> Or, just use syscon in device tree for those devices that have write > >> protect registers. > >> > > The hwspinlock framework doesn't require there to be another entity > > accessing some resource. It's there to implement hardware locks. I don't > > see why it can't be used here. > > The current usage of register lock/unlock protect is as the following code: > > static void ma35d1_unlock_regs(struct ma35d1_clk_pll *pll) > { > int ret; > > do { > regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x59); > regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x16); > regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x88); > regmap_read(pll->regmap, REG_SYS_RLKTZNS, &ret); > } while (ret == 0); > } > > static void ma35d1_lock_regs(struct ma35d1_clk_pll *pll) > { > regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x0); > } > > And the following code is to unlock registers for write and then lock again. > > ma35d1_unlock_regs(pll); > writel_relaxed(reg_ctl[0], pll->ctl0_base); > writel_relaxed(reg_ctl[1], pll->ctl1_base); > writel_relaxed(reg_ctl[2], pll->ctl2_base); > ma35d1_lock_regs(pll); > > The above code is from the clk-ma35d1-pll.c from this patchset. Yeah I understand that you write some registers in the syscon to lock the registers. > > We just employ regmap mechansim for the access to REG_SYS_RLKTZNS register. > Is this implementation OK for you? Thank you. > No. Why can't that be a hwspinlock? Or why can't it be unlocked all the time and rely on software spinlocks in the kernel to prevent concurrent access to the registers accessed by a driver, like a lock for the clk registers and a lock for the reset registers, etc. Or if no two clks or resets exist within one 32-bit word then no lock is necessary.
Dear Stephen, On 2023/3/29 上午 11:54, Stephen Boyd wrote: > Quoting Jacky Huang (2023-03-28 20:43:23) >> On 2023/3/29 上午 11:25, Stephen Boyd wrote: >>> Quoting Jacky Huang (2023-03-28 20:13:11) >>>> I may not explain clearly enough. The lock/unlock register of system >>>> controller is more like >>>> a kind of write protection for specific registers, rather than >>>> preventing hetero-core CPU access. >>>> In many different IP of ma35d1 contain write protected registers. >>>> In fact, ma35d1 has a "hardware semaphore" IP, and we have implemented >>>> the driver in drivers/hwspinlock. >>>> Even the control register of "hardware semaphore" is also write protected. >>> What's the need to lock and unlock the registers? Is some other >>> processor also writing to the registers that we need to synchronize >>> against? Or is Linux the only entity reading and writing the registers? >>> I'm wondering if we should simply unlock the registers and never lock >>> them. > Can you answer this question? Sorry, I miss this. The lock and unlock register mechanism is a hardware design of ma35d1 SoC. The purpose is to prevent from unexpected write to some registers. However, I think this is a redundant design if s/w is done properly. Even though I think it's a redundant design, it's out there and we have to deal with it. And of course we have unlock and lock pair, I just lost to write in the above. >>>> So, should we implement a system controller driver to provide >>>> register_unlock() function? >>>> Is it OK to have such a driver in drivers/mfd? >>>> Or, just use syscon in device tree for those devices that have write >>>> protect registers. >>>> >>> The hwspinlock framework doesn't require there to be another entity >>> accessing some resource. It's there to implement hardware locks. I don't >>> see why it can't be used here. >> The current usage of register lock/unlock protect is as the following code: >> >> static void ma35d1_unlock_regs(struct ma35d1_clk_pll *pll) >> { >> int ret; >> >> do { >> regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x59); >> regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x16); >> regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x88); >> regmap_read(pll->regmap, REG_SYS_RLKTZNS, &ret); >> } while (ret == 0); >> } >> >> static void ma35d1_lock_regs(struct ma35d1_clk_pll *pll) >> { >> regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x0); >> } >> >> And the following code is to unlock registers for write and then lock again. >> >> ma35d1_unlock_regs(pll); >> writel_relaxed(reg_ctl[0], pll->ctl0_base); >> writel_relaxed(reg_ctl[1], pll->ctl1_base); >> writel_relaxed(reg_ctl[2], pll->ctl2_base); >> ma35d1_lock_regs(pll); >> >> The above code is from the clk-ma35d1-pll.c from this patchset. > Yeah I understand that you write some registers in the syscon to lock > the registers. > >> We just employ regmap mechansim for the access to REG_SYS_RLKTZNS register. >> Is this implementation OK for you? Thank you. >> > No. Why can't that be a hwspinlock? Or why can't it be unlocked all the > time and rely on software spinlocks in the kernel to prevent concurrent > access to the registers accessed by a driver, like a lock for the clk > registers and a lock for the reset registers, etc. Or if no two clks or > resets exist within one 32-bit word then no lock is necessary. You gave a good suggestion here. Yes, of course we can let it be unlocked all the time. We can unlock the "register lock" before entering Linux. As a result, the unlonk and lock register related code is not required. Thus, we can remove syscon from the clock controller node. If you agree with this, we will modify it in the next version. Best regards, Jacky Huang
Dear Jiri, Thanks for your advice. On 2023/3/28 下午 05:23, Jiri Slaby wrote: > On 28. 03. 23, 4:19, Jacky Huang wrote: >> +static void transmit_chars(struct uart_ma35d1_port *up) >> +{ >> + struct circ_buf *xmit = &up->port.state->xmit; >> + int count; >> + u8 ch; >> + >> + if (uart_tx_stopped(&up->port)) { >> + ma35d1serial_stop_tx(&up->port); >> + return; >> + } >> + if (uart_circ_empty(xmit)) { >> + __stop_tx(up); >> + return; >> + } > > Why is this necessary? > I will remove this reduntant uart_circ_empty() check. >> + count = UART_FIFO_DEPTH - ((serial_in(up, UART_REG_FSR) & >> FSR_TXPTR_MSK) >> 16); >> + >> + uart_port_tx_limited(&up->port, ch, count, >> + !(serial_in(up, UART_REG_FSR) & FSR_TX_FULL), >> + serial_out(up, UART_REG_THR, ch), >> + ({})); >> + >> + if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS) >> + uart_write_wakeup(&up->port); >> + >> + if (uart_circ_empty(xmit)) >> + __stop_tx(up); > > uart_port_tx_limited() should take care about the above and this too, > right? Yes, you are right. These two are also redundant. I will remove them. Thank you. > >> +} > ... >> +static void receive_chars(struct uart_ma35d1_port *up) >> +{ >> + u8 flag; >> + u32 fsr; >> + unsigned int ch; > > Shouldn't ch be u8 too? OK, I will fix it as u8. > >> + int max_count = 256; >> + >> + fsr = serial_in(up, UART_REG_FSR); >> + do { >> + flag = TTY_NORMAL; >> + up->port.icount.rx++; >> + >> + if (unlikely(fsr & (FSR_BIF | FSR_FEF | FSR_PEF | >> FSR_RX_OVER_IF))) { >> + if (fsr & FSR_BIF) { >> + up->port.icount.brk++; >> + if (uart_handle_break(&up->port)) >> + continue; >> + } >> + if (fsr & FSR_FEF) >> + up->port.icount.frame++; >> + if (fsr & FSR_PEF) >> + up->port.icount.parity++; >> + if (fsr & FSR_RX_OVER_IF) >> + up->port.icount.overrun++; >> + >> + serial_out(up, UART_REG_FSR, fsr & >> + (FSR_BIF | FSR_FEF | FSR_PEF | FSR_RX_OVER_IF)); >> + >> + if (fsr & FSR_BIF) >> + flag = TTY_BREAK; >> + else if (fsr & FSR_PEF) >> + flag = TTY_PARITY; >> + else if (fsr & FSR_FEF) >> + flag = TTY_FRAME; >> + } >> + >> + ch = serial_in(up, UART_REG_RBR); >> + if (uart_handle_sysrq_char(&up->port, ch)) >> + continue; >> + >> + spin_lock(&up->port.lock); >> + uart_insert_char(&up->port, fsr, FSR_RX_OVER_IF, ch, flag); >> + spin_unlock(&up->port.lock); >> + >> + fsr = serial_in(up, UART_REG_FSR); >> + } while (!(fsr & FSR_RX_EMPTY) && (max_count-- > 0)); >> + >> + spin_lock(&up->port.lock); >> + tty_flip_buffer_push(&up->port.state->port); >> + spin_unlock(&up->port.lock); >> +} > ... >> +static int ma35d1serial_remove(struct platform_device *dev) >> +{ >> + struct uart_port *port = platform_get_drvdata(dev); >> + >> + if (port) { > > Can this ever be NULL? OK, I will remove this NULL check. > >> + uart_remove_one_port(&ma35d1serial_reg, port); >> + free_irq(port->irq, port); >> + } >> + return 0; >> +} >> + >> +static int ma35d1serial_suspend(struct platform_device *dev, >> pm_message_t state) >> +{ >> + int i; >> + struct uart_ma35d1_port *up; >> + >> + if (dev->dev.of_node) >> + i = of_alias_get_id(dev->dev.of_node, "serial"); >> + if (i < 0) { >> + dev_err(&dev->dev, "failed to get alias/pdev id, errno >> %d\n", i); >> + return i; >> + } >> + up = &ma35d1serial_ports[i]; > > platform_get_drvdata(dev) ? We will fix it. > >> + if (i == 0) { >> + up->console_baud_rate = serial_in(up, UART_REG_BAUD); >> + up->console_line = serial_in(up, UART_REG_LCR); >> + up->console_int = serial_in(up, UART_REG_IER); >> + } >> + return 0; >> +} >> + >> +static int ma35d1serial_resume(struct platform_device *dev) >> +{ >> + int i; >> + struct uart_ma35d1_port *up; >> + >> + if (dev->dev.of_node) >> + i = of_alias_get_id(dev->dev.of_node, "serial"); >> + if (i < 0) { >> + dev_err(&dev->dev, "failed to get alias/pdev id, errno >> %d\n", i); >> + return i; >> + } >> + up = &ma35d1serial_ports[i]; >> + if (i == 0) { >> + serial_out(up, UART_REG_BAUD, up->console_baud_rate); >> + serial_out(up, UART_REG_LCR, up->console_line); >> + serial_out(up, UART_REG_IER, up->console_int); >> + } >> + return 0; >> +} > > No uart_suspend_port()/uart_resume_port()? You don't wait for > transmitter to be empty in suspend. You don't stop tx etc. > We will fix it in the next version. >> +static struct platform_driver ma35d1serial_driver = { >> + .probe = ma35d1serial_probe, >> + .remove = ma35d1serial_remove, >> + .suspend = ma35d1serial_suspend, >> + .resume = ma35d1serial_resume, >> + .driver = { >> + .name = "ma35d1-uart", >> + .owner = THIS_MODULE, >> + .of_match_table = of_match_ptr(ma35d1_serial_of_match), >> + }, >> +}; >> + >> +static int __init ma35d1serial_init(void) >> +{ >> + int ret; >> + >> + ret = uart_register_driver(&ma35d1serial_reg); >> + if (ret) >> + return ret; >> + ret = platform_driver_register(&ma35d1serial_driver); >> + if (ret) >> + uart_unregister_driver(&ma35d1serial_reg); >> + return ret; >> +} >> + >> +static void __exit ma35d1serial_exit(void) >> +{ >> + platform_driver_unregister(&ma35d1serial_driver); >> + uart_unregister_driver(&ma35d1serial_reg); >> +} >> + >> +module_init(ma35d1serial_init); >> +module_exit(ma35d1serial_exit); >> + >> +MODULE_LICENSE("GPL"); >> +MODULE_DESCRIPTION("MA35D1 serial driver"); > > >> +MODULE_ALIAS_CHARDEV_MAJOR(TTY_MAJOR); > > Why is this needed? How are other platform drivers autoloaded? > > thanks, Yes, it's not needed. I will remove it. Best regards, Jacky Huang
On 28/03/2023 04:19, Jacky Huang wrote: > From: Jacky Huang <ychuang3@nuvoton.com> > > Add initial device tree support for Nuvoton ma35d1 SoC, including > cpu, clock, reset, and serial controllers. > Add reference boards som-256m and iot-512m. > > Signed-off-by: Jacky Huang <ychuang3@nuvoton.com> > + gic: interrupt-controller@50801000 { > + compatible = "arm,gic-400"; > + reg = <0x0 0x50801000 0 0x1000>, /* GICD */ > + <0x0 0x50802000 0 0x2000>, /* GICC */ > + <0x0 0x50804000 0 0x2000>, /* GICH */ > + <0x0 0x50806000 0 0x2000>; /* GICV */ > + #interrupt-cells = <3>; > + interrupt-parent = <&gic>; > + interrupt-controller; > + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) | > + IRQ_TYPE_LEVEL_HIGH)>; > + }; > + > + uart0:serial@40700000 { There is always space after label:. Best regards, Krzysztof
Dear Krzysztof, On 2023/3/29 下午 04:21, Krzysztof Kozlowski wrote: > On 28/03/2023 04:19, Jacky Huang wrote: >> From: Jacky Huang <ychuang3@nuvoton.com> >> >> Add initial device tree support for Nuvoton ma35d1 SoC, including >> cpu, clock, reset, and serial controllers. >> Add reference boards som-256m and iot-512m. >> >> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com> > > >> + gic: interrupt-controller@50801000 { >> + compatible = "arm,gic-400"; >> + reg = <0x0 0x50801000 0 0x1000>, /* GICD */ >> + <0x0 0x50802000 0 0x2000>, /* GICC */ >> + <0x0 0x50804000 0 0x2000>, /* GICH */ >> + <0x0 0x50806000 0 0x2000>; /* GICV */ >> + #interrupt-cells = <3>; >> + interrupt-parent = <&gic>; >> + interrupt-controller; >> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_RAW(0x13) | >> + IRQ_TYPE_LEVEL_HIGH)>; >> + }; >> + >> + uart0:serial@40700000 { > There is always space after label:. > > > Best regards, > Krzysztof > I will fix them all. Best regards, Jacky Huang
Quoting Jacky Huang (2023-03-28 23:02:31) > > On 2023/3/29 上午 11:54, Stephen Boyd wrote: > > Quoting Jacky Huang (2023-03-28 20:43:23) > > > >> We just employ regmap mechansim for the access to REG_SYS_RLKTZNS register. > >> Is this implementation OK for you? Thank you. > >> > > No. Why can't that be a hwspinlock? Or why can't it be unlocked all the > > time and rely on software spinlocks in the kernel to prevent concurrent > > access to the registers accessed by a driver, like a lock for the clk > > registers and a lock for the reset registers, etc. Or if no two clks or > > resets exist within one 32-bit word then no lock is necessary. > > You gave a good suggestion here. Yes, of course we can let it be > unlocked all the time. > We can unlock the "register lock" before entering Linux. > As a result, the unlonk and lock register related code is not required. > Thus, we can remove syscon from the clock controller node. > > If you agree with this, we will modify it in the next version. > Sure, that works.
Dear Stephen, Thanks for your advice. On 2023/3/29 上午 02:18, Stephen Boyd wrote: > Quoting Jacky Huang (2023-03-27 19:19:09) >> diff --git a/drivers/clk/nuvoton/Kconfig b/drivers/clk/nuvoton/Kconfig >> new file mode 100644 >> index 000000000000..c1324efedcb9 >> --- /dev/null >> +++ b/drivers/clk/nuvoton/Kconfig >> @@ -0,0 +1,19 @@ >> +# SPDX-License-Identifier: GPL-2.0 >> +# common clock support for Nuvoton SoC family. >> + >> +config COMMON_CLK_NUVOTON >> + bool "Nuvoton clock controller common support" >> + depends on ARCH_NUVOTON > Add || COMPILE_TEST > >> + default ARCH_NUVOTON >> + help >> + Say y here to enable common clock controller for Nuvoton platforms. >> + >> +if COMMON_CLK_NUVOTON > Add a newline here. > >> +config CLK_MA35D1 >> + bool "Nuvoton MA35D1 clock controller support" >> + depends on ARM64 || COMPILE_TEST > Drop this > >> + default y > And this. > >> + help >> + Build the driver for MA35D1 Clock Driver. > This says driver twice. "Build the clk driver for MA35D1 SoCs"? I will fix all above of Kconfig file. Thank you. >> + >> +endif >> diff --git a/drivers/clk/nuvoton/clk-ma35d1-divider.c b/drivers/clk/nuvoton/clk-ma35d1-divider.c >> new file mode 100644 >> index 000000000000..340a889a1417 >> --- /dev/null >> +++ b/drivers/clk/nuvoton/clk-ma35d1-divider.c >> @@ -0,0 +1,140 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (C) 2023 Nuvoton Technology Corp. >> + * Author: Chi-Fang Li <cfli0@nuvoton.com> >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/device.h> >> +#include <linux/regmap.h> >> +#include <linux/spinlock.h> >> + >> +#include "clk-ma35d1.h" >> + >> +#define clk_div_mask(width) ((1 << (width)) - 1) > clk-provider.h defines this already, use that? Yes, I will remove this. >> + >> +struct ma35d1_adc_clk_div { >> + struct clk_hw hw; >> + void __iomem *reg; >> + u8 shift; >> + u8 width; >> + u32 mask; >> + const struct clk_div_table *table; >> + /* protects concurrent access to clock divider registers */ >> + spinlock_t *lock; >> +}; >> + >> +static inline struct ma35d1_adc_clk_div *to_ma35d1_adc_clk_div(struct clk_hw *_hw) >> +{ >> + return container_of(_hw, struct ma35d1_adc_clk_div, hw); >> +} >> + >> +static inline unsigned long ma35d1_clkdiv_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + unsigned int val; >> + struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw); >> + >> + val = readl_relaxed(dclk->reg) >> dclk->shift; >> + val &= clk_div_mask(dclk->width); >> + val += 1; >> + return divider_recalc_rate(hw, parent_rate, val, dclk->table, >> + CLK_DIVIDER_ROUND_CLOSEST, dclk->width); >> +} >> + >> +static inline long ma35d1_clkdiv_round_rate(struct clk_hw *hw, >> + unsigned long rate, >> + unsigned long *prate) >> +{ >> + struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw); >> + >> + return divider_round_rate(hw, rate, prate, dclk->table, >> + dclk->width, CLK_DIVIDER_ROUND_CLOSEST); >> +} >> + >> +static inline int ma35d1_clkdiv_set_rate(struct clk_hw *hw, >> + unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + int value; >> + unsigned long flags = 0; >> + u32 data; >> + struct ma35d1_adc_clk_div *dclk = to_ma35d1_adc_clk_div(hw); >> + >> + value = divider_get_val(rate, parent_rate, dclk->table, >> + dclk->width, CLK_DIVIDER_ROUND_CLOSEST); >> + >> + if (dclk->lock) >> + spin_lock_irqsave(dclk->lock, flags); > Can you just always lock? That makes the conditional locking go away. OK, I will remove the "if (dclk->lock)" check. > >> + >> + data = readl_relaxed(dclk->reg); >> + data &= ~(clk_div_mask(dclk->width) << dclk->shift); >> + data |= (value - 1) << dclk->shift; >> + data |= dclk->mask; >> + >> + writel_relaxed(data, dclk->reg); >> + >> + if (dclk->lock) >> + spin_unlock_irqrestore(dclk->lock, flags); >> + >> + return 0; >> +} >> + >> +static const struct clk_ops ma35d1_adc_clkdiv_ops = { >> + .recalc_rate = ma35d1_clkdiv_recalc_rate, >> + .round_rate = ma35d1_clkdiv_round_rate, >> + .set_rate = ma35d1_clkdiv_set_rate, >> +}; >> + >> +struct clk_hw *ma35d1_reg_adc_clkdiv(struct device *dev, const char *name, >> + const char *parent_name, >> + spinlock_t *lock, >> + unsigned long flags, void __iomem *reg, >> + u8 shift, u8 width, u32 mask_bit) >> +{ >> + struct ma35d1_adc_clk_div *div; >> + struct clk_init_data init; >> + struct clk_div_table *table; >> + u32 max_div, min_div; >> + struct clk_hw *hw; >> + int ret; >> + int i; >> + >> + div = devm_kzalloc(dev, sizeof(*div), GFP_KERNEL); >> + if (!div) >> + return ERR_PTR(-ENOMEM); >> + >> + max_div = clk_div_mask(width) + 1; >> + min_div = 1; >> + >> + table = devm_kcalloc(dev, max_div + 1, sizeof(*table), GFP_KERNEL); >> + if (!table) >> + return ERR_PTR(-ENOMEM); >> + >> + for (i = 0; i < max_div; i++) { >> + table[i].val = (min_div + i); > Drop useless parenthesis. I will fix it. > >> + table[i].div = 2 * table[i].val; >> + } >> + table[max_div].val = 0; >> + table[max_div].div = 0; >> + >> + init.name = name; >> + init.ops = &ma35d1_adc_clkdiv_ops; >> + init.flags |= flags; >> + init.parent_names = parent_name ? &parent_name : NULL; >> + init.num_parents = parent_name ? 1 : 0; >> + >> + div->reg = reg; >> + div->shift = shift; >> + div->width = width; >> + div->mask = mask_bit ? BIT(mask_bit) : 0; >> + div->lock = lock; >> + div->hw.init = &init; >> + div->table = table; >> + >> + hw = &div->hw; >> + ret = devm_clk_hw_register(dev, hw); >> + if (ret < 0) > Use if (ret) instead. I will fix it. > >> + return ERR_PTR(ret); >> + return hw; >> +} >> diff --git a/drivers/clk/nuvoton/clk-ma35d1-pll.c b/drivers/clk/nuvoton/clk-ma35d1-pll.c >> new file mode 100644 >> index 000000000000..b36fbda4fa0a >> --- /dev/null >> +++ b/drivers/clk/nuvoton/clk-ma35d1-pll.c >> @@ -0,0 +1,350 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (C) 2023 Nuvoton Technology Corp. >> + * Author: Chi-Fang Li <cfli0@nuvoton.com> >> + */ >> + >> +#include <linux/bitfield.h> >> +#include <linux/clk-provider.h> >> +#include <linux/slab.h> >> +#include <linux/regmap.h> >> + >> +#include "clk-ma35d1.h" >> + >> +#define REG_SYS_RLKTZNS 0x1a4 /* Register Lock Control */ >> + >> +struct ma35d1_clk_pll { >> + struct clk_hw hw; >> + u8 type; >> + u8 mode; >> + unsigned long rate; >> + void __iomem *ctl0_base; > This needs an include for __iomem define. Just include kernel.h for now. OK, I will add it. >> + void __iomem *ctl1_base; >> + void __iomem *ctl2_base; >> + struct regmap *regmap; >> +}; >> + >> +static inline struct ma35d1_clk_pll *to_ma35d1_clk_pll(struct clk_hw *_hw) >> +{ >> + return container_of(_hw, struct ma35d1_clk_pll, hw); > Missing include as well. I will add the #include <linux/container_of.h> > >> +} >> + >> +static void ma35d1_unlock_regs(struct ma35d1_clk_pll *pll) >> +{ >> + int ret; >> + >> + do { >> + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x59); >> + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x16); >> + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x88); >> + regmap_read(pll->regmap, REG_SYS_RLKTZNS, &ret); >> + } while (ret == 0); > Any sort of timeout? As we have discussed in [PATCH v6 08/12] of this patchset, we will remove the register lock. So, this function will be removed in the next version. >> +} >> + >> +static void ma35d1_lock_regs(struct ma35d1_clk_pll *pll) >> +{ >> + regmap_write(pll->regmap, REG_SYS_RLKTZNS, 0x0); >> +} >> + >> +static unsigned long ma35d1_calc_smic_pll_freq(u32 pll0_ctl0, >> + unsigned long parent_rate) >> +{ >> + u32 m, n, p, outdiv; >> + u64 pll_freq; >> + u32 clk_div_table[] = { 1, 2, 4, 8 }; > const OK, I will add const to the clk_div_table[]. > >> + >> + if (pll0_ctl0 & SPLL0_CTL0_BP) >> + return parent_rate; >> + >> + n = FIELD_GET(SPLL0_CTL0_FBDIV, pll0_ctl0); >> + m = FIELD_GET(SPLL0_CTL0_INDIV, pll0_ctl0); >> + p = FIELD_GET(SPLL0_CTL0_OUTDIV, pll0_ctl0); >> + outdiv = clk_div_table[p]; >> + pll_freq = (u64)parent_rate * n; >> + do_div(pll_freq, m * outdiv); >> + return (unsigned long)pll_freq; > Remove useless cast. I will fix it. >> +} >> + >> +static unsigned long ma35d1_calc_pll_freq(u8 mode, u32 *reg_ctl, >> + unsigned long parent_rate) >> +{ >> + u32 m, n, p; >> + u64 pll_freq, x; >> + >> + if (reg_ctl[1] & PLL_CTL1_BP) >> + return parent_rate; >> + >> + if (mode == PLL_MODE_INT) { >> + n = FIELD_GET(PLL_CTL0_FBDIV, reg_ctl[0]); >> + m = FIELD_GET(PLL_CTL0_INDIV, reg_ctl[0]); >> + p = FIELD_GET(PLL_CTL1_OUTDIV, reg_ctl[1]); >> + pll_freq = (u64)parent_rate * n; >> + do_div(pll_freq, m * p); >> + } else { >> + n = FIELD_GET(PLL_CTL0_FBDIV, reg_ctl[0]); >> + m = FIELD_GET(PLL_CTL0_INDIV, reg_ctl[0]); >> + p = FIELD_GET(PLL_CTL1_OUTDIV, reg_ctl[1]); > The n, m, p are the same, so pull them out of the if-else to deduplicate. Ok, I will modify it. > >> + x = FIELD_GET(PLL_CTL1_FRAC, reg_ctl[1]); >> + /* 2 decimal places floating to integer (ex. 1.23 to 123) */ >> + n = n * 100 + ((x * 100) / FIELD_MAX(PLL_CTL1_FRAC)); > Is this mult_frac()? Not exactly. The number is represented as n.x. Suppose the number is 123.45, then n is 123 and x is 45. We have it be multiplied with 100 to be integer 12345, which is n. In the following pll_freq = ((u64)parent_rate * n) / 100 / m / p; We can keep precision of n. It seems not applicable to mult_frac(). > >> + pll_freq = ((u64)parent_rate * n) / 100 / m / p; >> + } > Add a newline > >> + return (unsigned long)pll_freq; > Remove useless cast. I will fix them. >> +} >> + >> +static int ma35d1_pll_find_closest(struct ma35d1_clk_pll *pll, >> + unsigned long rate, >> + unsigned long parent_rate, >> + u32 *reg_ctl, unsigned long *freq) >> +{ >> + int p, m, n; >> + int fbdiv_min, fbdiv_max; >> + unsigned long diff = 0xffffffff; >> + >> + *freq = 0; >> + if ((rate < PLL_FCLKO_MIN_FREQ) || (rate > PLL_FCLKO_MAX_FREQ)) >> + return -EINVAL; >> + >> + if (pll->mode == PLL_MODE_INT) { >> + fbdiv_min = FBDIV_MIN; >> + fbdiv_max = FBDIV_MAX; >> + } else { >> + fbdiv_min = FBDIV_FRAC_MIN; >> + fbdiv_max = FBDIV_FRAC_MAX; >> + } >> + >> + for (m = INDIV_MIN; m <= INDIV_MAX; m++) { >> + for (n = fbdiv_min; n <= fbdiv_max; n++) { >> + for (p = OUTDIV_MIN; p <= OUTDIV_MAX; p++) { >> + unsigned long tmp, fout; >> + u64 fclk; >> + >> + tmp = parent_rate / m; >> + if (tmp < PLL_FREF_M_MIN_FREQ || >> + tmp > PLL_FREF_M_MAX_FREQ) >> + continue; /* constrain */ >> + >> + fclk = (u64)parent_rate * n / m; >> + /* for 2 decimal places */ >> + if (pll->mode != PLL_MODE_INT) >> + fclk /= 100; >> + >> + if (fclk < PLL_FCLK_MIN_FREQ || >> + fclk > PLL_FCLK_MAX_FREQ) >> + continue; /* constrain */ >> + >> + fout = (unsigned long)(fclk / p); >> + if (fout < PLL_FCLKO_MIN_FREQ || >> + fout > PLL_FCLKO_MAX_FREQ) >> + continue; /* constrain */ >> + >> + if (abs(rate - fout) < diff) { >> + reg_ctl[0] = FIELD_PREP(PLL_CTL0_INDIV, m) | >> + FIELD_PREP(PLL_CTL0_FBDIV, n); >> + reg_ctl[1] = FIELD_PREP(PLL_CTL1_OUTDIV, p); >> + *freq = fout; >> + diff = abs(rate - fout); >> + if (diff == 0) >> + break; >> + } >> + } >> + } >> + } >> + if (*freq == 0) >> + return -EINVAL; /* cannot find even one valid setting */ >> + return 0; >> +} >> + >> +static int ma35d1_clk_pll_set_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); >> + u32 reg_ctl[3] = { 0 }; >> + unsigned long pll_freq; >> + int ret; >> + >> + if ((parent_rate < PLL_FREF_MIN_FREQ) || >> + (parent_rate > PLL_FREF_MAX_FREQ)) > Remove useless parenthesis. > >> + return -EINVAL; >> + >> + if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) { > Remove useless parenthesis. I will fix them. >> + pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n"); >> + return -EACCES; >> + } >> + >> + ret = ma35d1_pll_find_closest(pll, rate, parent_rate, reg_ctl, &pll_freq); >> + if (ret != 0) >> + return ret; >> + pll->rate = pll_freq; >> + >> + switch (pll->mode) { >> + case PLL_MODE_INT: >> + reg_ctl[0] |= FIELD_PREP(PLL_CTL0_MODE, PLL_MODE_INT); >> + break; >> + case PLL_MODE_FRAC: >> + reg_ctl[0] |= FIELD_PREP(PLL_CTL0_MODE, PLL_MODE_FRAC); >> + break; >> + case PLL_MODE_SS: >> + reg_ctl[0] |= FIELD_PREP(PLL_CTL0_MODE, PLL_MODE_SS) | >> + FIELD_PREP(PLL_CTL0_SSRATE, PLL_SS_RATE); >> + reg_ctl[2] = FIELD_PREP(PLL_CTL2_SLOPE, PLL_SLOPE); >> + break; >> + } >> + reg_ctl[1] |= PLL_CTL1_PD; >> + >> + ma35d1_unlock_regs(pll); >> + writel_relaxed(reg_ctl[0], pll->ctl0_base); >> + writel_relaxed(reg_ctl[1], pll->ctl1_base); >> + writel_relaxed(reg_ctl[2], pll->ctl2_base); >> + ma35d1_lock_regs(pll); >> + return 0; >> +} >> + >> +static unsigned long ma35d1_clk_pll_recalc_rate(struct clk_hw *hw, >> + unsigned long parent_rate) >> +{ >> + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); >> + u32 reg_ctl[3]; >> + unsigned long pll_freq; >> + >> + if ((parent_rate < PLL_FREF_MIN_FREQ) || (parent_rate > PLL_FREF_MAX_FREQ)) > Remove useless parenthesis. I will fix it. > >> + return 0; >> + >> + switch (pll->type) { >> + case MA35D1_CAPLL: >> + reg_ctl[0] = readl_relaxed(pll->ctl0_base); >> + pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], parent_rate); > return? Yes, I will have it return here. >> + break; >> + case MA35D1_DDRPLL: >> + case MA35D1_APLL: >> + case MA35D1_EPLL: >> + case MA35D1_VPLL: >> + reg_ctl[0] = readl_relaxed(pll->ctl0_base); >> + reg_ctl[1] = readl_relaxed(pll->ctl1_base); >> + pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, parent_rate); > return? > >> + break; >> + } >> + return pll_freq; > return 0? Yes, I will fix them. > >> +} >> + >> +static long ma35d1_clk_pll_round_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long *parent_rate) >> +{ >> + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); >> + u32 reg_ctl[3] = { 0 }; >> + unsigned long pll_freq; >> + long ret; >> + >> + if (*parent_rate < PLL_FREF_MIN_FREQ || *parent_rate > PLL_FREF_MAX_FREQ) >> + return -EINVAL; >> + >> + ret = ma35d1_pll_find_closest(pll, rate, *parent_rate, reg_ctl, &pll_freq); >> + if (ret != 0) >> + return ret; >> + >> + switch (pll->type) { >> + case MA35D1_CAPLL: >> + reg_ctl[0] = readl_relaxed(pll->ctl0_base); >> + pll_freq = ma35d1_calc_smic_pll_freq(reg_ctl[0], *parent_rate); > return? > >> + break; >> + case MA35D1_DDRPLL: >> + case MA35D1_APLL: >> + case MA35D1_EPLL: >> + case MA35D1_VPLL: >> + reg_ctl[0] = readl_relaxed(pll->ctl0_base); >> + reg_ctl[1] = readl_relaxed(pll->ctl1_base); >> + pll_freq = ma35d1_calc_pll_freq(pll->mode, reg_ctl, *parent_rate); > return? > >> + break; >> + } >> + return pll_freq; > return 0? Yes, I will fix them. >> +} >> + >> +static int ma35d1_clk_pll_is_prepared(struct clk_hw *hw) >> +{ >> + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); >> + u32 val = readl_relaxed(pll->ctl1_base); >> + >> + return val & PLL_CTL1_PD ? 0 : 1; >> +} >> + >> +static int ma35d1_clk_pll_prepare(struct clk_hw *hw) >> +{ >> + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); >> + u32 val; >> + >> + if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) { > Drop useless parenthesis. I will fix it. > >> + pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n"); >> + return -EACCES; >> + } >> + >> + ma35d1_unlock_regs(pll); >> + val = readl_relaxed(pll->ctl1_base); >> + val &= ~PLL_CTL1_PD; >> + writel_relaxed(val, pll->ctl1_base); >> + ma35d1_lock_regs(pll); >> + return 0; >> +} >> + >> +static void ma35d1_clk_pll_unprepare(struct clk_hw *hw) >> +{ >> + struct ma35d1_clk_pll *pll = to_ma35d1_clk_pll(hw); >> + u32 val; >> + >> + if ((pll->type == MA35D1_CAPLL) || (pll->type == MA35D1_DDRPLL)) { > Drop useless parenthesis. I will fix it. > >> + pr_warn("Nuvoton MA35D1 CAPLL/DDRPLL is read only.\n"); > These read-only checks should be removed and different clk_ops should be > assigned for the read-only type of clks. OK, I will fix it. >> + } else { >> + val = readl_relaxed(pll->ctl1_base); >> + val |= PLL_CTL1_PD; >> + writel_relaxed(val, pll->ctl1_base); >> + } >> +} >> + >> +static const struct clk_ops ma35d1_clk_pll_ops = { >> + .is_prepared = ma35d1_clk_pll_is_prepared, >> + .prepare = ma35d1_clk_pll_prepare, >> + .unprepare = ma35d1_clk_pll_unprepare, >> + .set_rate = ma35d1_clk_pll_set_rate, >> + .recalc_rate = ma35d1_clk_pll_recalc_rate, >> + .round_rate = ma35d1_clk_pll_round_rate, >> +}; >> + >> +struct clk_hw *ma35d1_reg_clk_pll(enum ma35d1_pll_type type, > Pass a device here. OK, I will add it. >> + u8 u8mode, const char *name, >> + const char *parent, >> + unsigned long targetFreq, >> + void __iomem *base, >> + struct regmap *regmap) >> +{ >> + struct ma35d1_clk_pll *pll; >> + struct clk_hw *hw; >> + struct clk_init_data init; > init = {}; > >> + int ret; >> + >> + pll = kmalloc(sizeof(*pll), GFP_KERNEL); > Just use kzalloc(). This isn't a hot path. I will use devm_kzalloc(). >> + if (!pll) >> + return ERR_PTR(-ENOMEM); >> + >> + pll->type = type; >> + pll->mode = u8mode; >> + pll->rate = targetFreq; >> + pll->ctl0_base = base + REG_PLL_CTL0_OFFSET; >> + pll->ctl1_base = base + REG_PLL_CTL1_OFFSET; >> + pll->ctl2_base = base + REG_PLL_CTL2_OFFSET; >> + pll->regmap = regmap; >> + >> + init.name = name; >> + init.flags = 0; >> + init.parent_names = &parent; >> + init.num_parents = 1; >> + init.ops = &ma35d1_clk_pll_ops; >> + pll->hw.init = &init; >> + hw = &pll->hw; >> + >> + ret = clk_hw_register(NULL, hw); > Use devm_ OK, I will fix it. > >> + if (ret) { >> + pr_err("failed to register vsi-pll clock!!!\n"); > I'm going to put a dev_err_probe() into clk registration code. Remove > this printk. OK, I will remove it. >> + kfree(pll); >> + return ERR_PTR(ret); >> + } >> + return hw; >> +} >> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c >> new file mode 100644 >> index 000000000000..e4d3ced396a3 >> --- /dev/null >> +++ b/drivers/clk/nuvoton/clk-ma35d1.c >> @@ -0,0 +1,963 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (C) 2023 Nuvoton Technology Corp. >> + * Author: Chi-Fang Li <cfli0@nuvoton.com> >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/mfd/syscon.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_address.h> >> +#include <linux/platform_device.h> >> +#include <linux/spinlock.h> >> +#include <dt-bindings/clock/nuvoton,ma35d1-clk.h> >> + >> +#include "clk-ma35d1.h" >> + >> +static DEFINE_SPINLOCK(ma35d1_lock); >> + >> +static const char *const ca35clk_sel_clks[] = { > Use clk_parent_data instead. I will fix it. > >> + "hxt", "capll", "ddrpll", "dummy" >> +}; >> + > [...] >> + >> +static inline struct clk_hw *ma35d1_clk_fixed(const char *name, int rate) >> +{ >> + return clk_hw_register_fixed_rate(NULL, name, NULL, 0, rate); >> +} >> + >> +static inline struct clk_hw *ma35d1_clk_mux(const char *name, >> + void __iomem *reg, >> + u8 shift, u8 width, >> + const char *const *parents, >> + int num_parents) >> +{ >> + return clk_hw_register_mux(NULL, name, parents, num_parents, >> + CLK_SET_RATE_NO_REPARENT, reg, shift, >> + width, 0, &ma35d1_lock); >> +} >> + >> +static inline struct clk_hw *ma35d1_clk_divider(const char *name, >> + const char *parent, >> + void __iomem *reg, u8 shift, >> + u8 width) >> +{ >> + return clk_hw_register_divider(NULL, name, parent, CLK_SET_RATE_PARENT, >> + reg, shift, width, 0, &ma35d1_lock); >> +} >> + >> +static inline struct clk_hw *ma35d1_clk_divider_pow2(const char *name, >> + const char *parent, >> + void __iomem *reg, >> + u8 shift, u8 width) >> +{ >> + return clk_hw_register_divider(NULL, name, parent, >> + CLK_DIVIDER_POWER_OF_TWO, reg, shift, >> + width, 0, &ma35d1_lock); >> +} >> + >> +static inline struct clk_hw *ma35d1_clk_divider_table(const char *name, >> + const char *parent, >> + void __iomem *reg, >> + u8 shift, u8 width, >> + const struct clk_div_table *table) >> +{ >> + return clk_hw_register_divider_table(NULL, name, parent, 0, >> + reg, shift, width, 0, table, >> + &ma35d1_lock); >> +} >> + >> +static inline struct clk_hw *ma35d1_clk_fixed_factor(const char *name, >> + const char *parent, >> + unsigned int mult, >> + unsigned int div) >> +{ >> + return clk_hw_register_fixed_factor(NULL, name, parent, >> + CLK_SET_RATE_PARENT, mult, div); >> +} >> + >> +static inline struct clk_hw *ma35d1_clk_gate(const char *name, >> + const char *parent, >> + void __iomem *reg, u8 shift) >> +{ >> + return clk_hw_register_gate(NULL, name, parent, CLK_SET_RATE_PARENT, >> + reg, shift, 0, &ma35d1_lock); >> +} > These need to use devm_ to unregister on failure. OK, I will fix them all. >> + >> +static int ma35d1_get_pll_setting(struct device_node *clk_node, >> + u32 *pllmode, u32 *pllfreq) >> +{ >> + const char *of_str; >> + int i; >> + >> + for (i = 0; i < PLL_MAX_NUM; i++) { >> + if (of_property_read_string_index(clk_node, "nuvoton,pll-mode", i, &of_str)) >> + return -EINVAL; >> + if (!strcmp(of_str, "integer")) >> + pllmode[i] = PLL_MODE_INT; >> + else if (!strcmp(of_str, "fractional")) >> + pllmode[i] = PLL_MODE_FRAC; >> + else if (!strcmp(of_str, "spread-spectrum")) >> + pllmode[i] = PLL_MODE_SS; >> + else >> + return -EINVAL; >> + } >> + return of_property_read_u32_array(clk_node, "assigned-clock-rates", > The clk framework looks at this property. Why is the driver looking at > it too? We should not use "assigned-clock-rates", instead we should create a vendor-specific property to describe the pll frequency array. I will fix it in the next version. >> + pllfreq, PLL_MAX_NUM); >> +} >> + Best regards, Jacky Huang
Hi Jacky, On Tue, Mar 28, 2023 at 02:19:10AM +0000, Jacky Huang wrote: > From: Jacky Huang <ychuang3@nuvoton.com> > > This driver supports individual IP reset for ma35d1. The reset > control registers is a subset of system control registers. > > Signed-off-by: Jacky Huang <ychuang3@nuvoton.com> > --- > drivers/reset/Kconfig | 6 ++ > drivers/reset/Makefile | 1 + > drivers/reset/reset-ma35d1.c | 152 +++++++++++++++++++++++++++++++++++ > 3 files changed, 159 insertions(+) > create mode 100644 drivers/reset/reset-ma35d1.c > [...] > diff --git a/drivers/reset/reset-ma35d1.c b/drivers/reset/reset-ma35d1.c > new file mode 100644 > index 000000000000..221299e7b873 > --- /dev/null > +++ b/drivers/reset/reset-ma35d1.c > @@ -0,0 +1,152 @@ [...] > +static int ma35d1_reset_update(struct reset_controller_dev *rcdev, > + unsigned long id, bool assert) > +{ > + unsigned int reg; > + int ret; > + int offset = (id / RST_PRE_REG) * 4; > + struct ma35d1_reset_data *data = > + container_of(rcdev, struct ma35d1_reset_data, rcdev); > + > + ret = regmap_read(data->regmap, REG_SYS_IPRST0 + offset, ®); > + if (ret < 0) > + return ret; > + if (assert) > + reg |= 1 << (id % RST_PRE_REG); > + else > + reg &= ~(1 << (id % RST_PRE_REG)); > + > + return regmap_write(data->regmap, REG_SYS_IPRST0 + offset, reg); This should use regmap_update_bits(). [...] > +static int ma35d1_reset_status(struct reset_controller_dev *rcdev, > + unsigned long id) > +{ > + int reg, ret; > + int offset = id / RST_PRE_REG; Should this be int offset = (id / RST_PRE_REG) * 4; ? > +static int ma35d1_reset_probe(struct platform_device *pdev) > +{ > + int err; > + struct device *dev = &pdev->dev; > + struct device_node *parent; > + struct ma35d1_reset_data *reset_data; > + struct ma35d1_reboot_data *reboot_data; > + > + if (!pdev->dev.of_node) { > + dev_err(&pdev->dev, "Device tree node not found\n"); > + return -EINVAL; > + } > + > + reset_data = devm_kzalloc(dev, sizeof(*reset_data), GFP_KERNEL); > + if (!reset_data) > + return -ENOMEM; > + > + reboot_data = devm_kzalloc(dev, sizeof(*reboot_data), GFP_KERNEL); > + if (!reboot_data) > + return -ENOMEM; These structures could be combined into one. regards Philipp
Dear Philipp, On 2023/4/25 上午 04:02, Philipp Zabel wrote: > Hi Jacky, > > On Tue, Mar 28, 2023 at 02:19:10AM +0000, Jacky Huang wrote: >> From: Jacky Huang <ychuang3@nuvoton.com> >> >> This driver supports individual IP reset for ma35d1. The reset >> control registers is a subset of system control registers. >> >> Signed-off-by: Jacky Huang <ychuang3@nuvoton.com> >> --- >> drivers/reset/Kconfig | 6 ++ >> drivers/reset/Makefile | 1 + >> drivers/reset/reset-ma35d1.c | 152 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 159 insertions(+) >> create mode 100644 drivers/reset/reset-ma35d1.c >> > [...] >> diff --git a/drivers/reset/reset-ma35d1.c b/drivers/reset/reset-ma35d1.c >> new file mode 100644 >> index 000000000000..221299e7b873 >> --- /dev/null >> +++ b/drivers/reset/reset-ma35d1.c >> @@ -0,0 +1,152 @@ > [...] >> +static int ma35d1_reset_update(struct reset_controller_dev *rcdev, >> + unsigned long id, bool assert) >> +{ >> + unsigned int reg; >> + int ret; >> + int offset = (id / RST_PRE_REG) * 4; >> + struct ma35d1_reset_data *data = >> + container_of(rcdev, struct ma35d1_reset_data, rcdev); >> + >> + ret = regmap_read(data->regmap, REG_SYS_IPRST0 + offset, ®); >> + if (ret < 0) >> + return ret; >> + if (assert) >> + reg |= 1 << (id % RST_PRE_REG); >> + else >> + reg &= ~(1 << (id % RST_PRE_REG)); >> + >> + return regmap_write(data->regmap, REG_SYS_IPRST0 + offset, reg); > This should use regmap_update_bits(). > > [...] >> +static int ma35d1_reset_status(struct reset_controller_dev *rcdev, >> + unsigned long id) >> +{ >> + int reg, ret; >> + int offset = id / RST_PRE_REG; > Should this be > > int offset = (id / RST_PRE_REG) * 4; > > ? Yes, here is a coding mistake. As the register offset was modified to be indexed by lookup table in v7, this code was obsoleted. Thank you for pointing out this. >> +static int ma35d1_reset_probe(struct platform_device *pdev) >> +{ >> + int err; >> + struct device *dev = &pdev->dev; >> + struct device_node *parent; >> + struct ma35d1_reset_data *reset_data; >> + struct ma35d1_reboot_data *reboot_data; >> + >> + if (!pdev->dev.of_node) { >> + dev_err(&pdev->dev, "Device tree node not found\n"); >> + return -EINVAL; >> + } >> + >> + reset_data = devm_kzalloc(dev, sizeof(*reset_data), GFP_KERNEL); >> + if (!reset_data) >> + return -ENOMEM; >> + >> + reboot_data = devm_kzalloc(dev, sizeof(*reboot_data), GFP_KERNEL); >> + if (!reboot_data) >> + return -ENOMEM; > These structures could be combined into one. OK, we will combine them into one. > regards > Philipp Best regards, Jacky Huang
From: Jacky Huang <ychuang3@nuvoton.com> This patchset adds initial support for the Nuvoton ma35d1 SoC, including initial device tree, clock driver, reset driver, and serial driver. This patchset cover letter is based from the initial support for Nuvoton ma35d1 to keep tracking the version history. This patchset had been applied to Linux kernel 6.3-rc3 and tested on the Nuvoton ma35d1 SOM evaluation board. (ma35d1 information: https://www.nuvoton.com/products/microprocessors/arm-cortex-a35-mpus/) MA35D1 porting on linux-5.10.y can be found at: https://github.com/OpenNuvoton/MPU-Family v6: - Combine nuvoton,ma35d1-clk.yaml and nuvoton,ma35d1-clk.h into one patch - Combine nuvoton,ma35d1-reset.yaml and nuvoton,ma35d1-reset.h into one patch - rename Documentation/devicetree/bindings/arm/npcm directory as nuvoton - Remove patch for adding include/linux/mfd/ma35d1-sys.h as it's not required - Update dtsi & dts files and move board-specific nodes to dts - Modify reset driver - Modify serial driver, fix coding style issues - Modify clock driver, rewrite the PLL calculation functions v5: - Add ARCH_NUVOTON to arm64 Kconfig - Add ARCH_NUVOTON to defconfig - Add the clock driver - Add the reset driver - Add the serial driver - Add us to the maintainer v4: - patch 4/5 is a resend - Fixed dt_binding_check errors of nuvoton,ma35d1-clk.yaml - Modify ma35d1.dtsi 1. Add a node hxt_24m 2. Fixed the base address of gic node 3. Add clocks and clock-names to clock node - Fixed borad binding mistakes of nuvoton.yaml v3: - added patch 4/5 and 5/5 - introduce CONFIG_ARCH_NUVOTON option - add initial bindings for Nuvoton Platform boards - fixed coding style problem of nuvoton,ma35d1-clk.h - added CAPLL to clock-controller node - modify the chosen node of ma35d1-evb.dts - modify clock yaml "clk-pll-mode" to "nuvoton,clk-pll-mode" v2: - fixed dt_binding_check failed of nuvoton,ma35d1-clk.yaml Jacky Huang (12): arm64: Kconfig.platforms: Add config for Nuvoton MA35 platform arm64: defconfig: Add support for Nuvoton MA35 family SoCs dt-bindings: clock: nuvoton: add binding for ma35d1 clock controller dt-bindings: reset: nuvoton: add binding for ma35d1 IP reset control dt-bindings: mfd: syscon: Add nuvoton,ma35d1-sys compatible dt-bindings: arm: Add initial bindings for Nuvoton platform dt-bindings: serial: Document ma35d1 uart controller arm64: dts: nuvoton: Add initial ma35d1 device tree clk: nuvoton: Add clock driver for ma35d1 clock controller reset: Add Nuvoton ma35d1 reset driver support tty: serial: Add Nuvoton ma35d1 serial driver support MAINTAINERS: Add entry for NUVOTON MA35 .../bindings/arm/nuvoton/nuvoton,ma35d1.yaml | 30 + .../nuvoton,npcm-gcr.yaml} | 2 +- .../npcm.yaml => nuvoton/nuvoton,npcm.yaml} | 2 +- .../bindings/clock/nuvoton,ma35d1-clk.yaml | 72 ++ .../devicetree/bindings/mfd/syscon.yaml | 1 + .../bindings/reset/nuvoton,ma35d1-reset.yaml | 44 + .../serial/nuvoton,ma35d1-serial.yaml | 48 + MAINTAINERS | 9 + arch/arm64/Kconfig.platforms | 9 + arch/arm64/boot/dts/nuvoton/Makefile | 2 + .../boot/dts/nuvoton/ma35d1-iot-512m.dts | 56 + .../boot/dts/nuvoton/ma35d1-som-256m.dts | 56 + arch/arm64/boot/dts/nuvoton/ma35d1.dtsi | 231 +++++ arch/arm64/configs/defconfig | 1 + drivers/clk/Makefile | 1 + drivers/clk/nuvoton/Kconfig | 19 + drivers/clk/nuvoton/Makefile | 4 + drivers/clk/nuvoton/clk-ma35d1-divider.c | 140 +++ drivers/clk/nuvoton/clk-ma35d1-pll.c | 350 +++++++ drivers/clk/nuvoton/clk-ma35d1.c | 963 ++++++++++++++++++ drivers/clk/nuvoton/clk-ma35d1.h | 123 +++ drivers/reset/Kconfig | 6 + drivers/reset/Makefile | 1 + drivers/reset/reset-ma35d1.c | 152 +++ drivers/tty/serial/Kconfig | 18 + drivers/tty/serial/Makefile | 1 + drivers/tty/serial/ma35d1_serial.c | 802 +++++++++++++++ .../dt-bindings/clock/nuvoton,ma35d1-clk.h | 253 +++++ .../dt-bindings/reset/nuvoton,ma35d1-reset.h | 108 ++ 29 files changed, 3502 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/arm/nuvoton/nuvoton,ma35d1.yaml rename Documentation/devicetree/bindings/arm/{npcm/nuvoton,gcr.yaml => nuvoton/nuvoton,npcm-gcr.yaml} (93%) rename Documentation/devicetree/bindings/arm/{npcm/npcm.yaml => nuvoton/nuvoton,npcm.yaml} (93%) create mode 100644 Documentation/devicetree/bindings/clock/nuvoton,ma35d1-clk.yaml create mode 100644 Documentation/devicetree/bindings/reset/nuvoton,ma35d1-reset.yaml create mode 100644 Documentation/devicetree/bindings/serial/nuvoton,ma35d1-serial.yaml create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-iot-512m.dts create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1-som-256m.dts create mode 100644 arch/arm64/boot/dts/nuvoton/ma35d1.dtsi create mode 100644 drivers/clk/nuvoton/Kconfig create mode 100644 drivers/clk/nuvoton/Makefile create mode 100644 drivers/clk/nuvoton/clk-ma35d1-divider.c create mode 100644 drivers/clk/nuvoton/clk-ma35d1-pll.c create mode 100644 drivers/clk/nuvoton/clk-ma35d1.c create mode 100644 drivers/clk/nuvoton/clk-ma35d1.h create mode 100644 drivers/reset/reset-ma35d1.c create mode 100644 drivers/tty/serial/ma35d1_serial.c create mode 100644 include/dt-bindings/clock/nuvoton,ma35d1-clk.h create mode 100644 include/dt-bindings/reset/nuvoton,ma35d1-reset.h