Re: [PATCH v5 7/8] clk: Add common clock driver for BM1880 SoC

From: Stephen Boyd
Date: Wed Sep 18 2019 - 00:51:29 EST

Next message: Stephen Boyd: "Re: [PATCH 1/2] dt-bindings: documentation: add clock bindings information for Agilex"
Previous message: Navid Emamdoost: "[PATCH] drivers/net: release skb on failure"
In reply to: Manivannan Sadhasivam: "[PATCH v5 7/8] clk: Add common clock driver for BM1880 SoC"
Next in thread: Manivannan Sadhasivam: "[PATCH v5 8/8] MAINTAINERS: Add entry for BM1880 SoC clock driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Quoting Manivannan Sadhasivam (2019-09-16 09:14:46)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index 801fa1cd0321..e70c64e43ff9 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -139,6 +139,13 @@ config COMMON_CLK_SI570
> This driver supports Silicon Labs 570/571/598/599 programmable
> clock generators.
>
> +config COMMON_CLK_BM1880
> + bool "Clock driver for Bitmain BM1880 SoC"

Can it be tristate?

> + depends on ARCH_BITMAIN || COMPILE_TEST
> + default ARCH_BITMAIN
> + help
> + This driver supports the clocks on Bitmain BM1880 SoC.
> +
> config COMMON_CLK_CDCE706
> tristate "Clock driver for TI CDCE706 clock synthesizer"
> depends on I2C
> diff --git a/drivers/clk/clk-bm1880.c b/drivers/clk/clk-bm1880.c
> new file mode 100644
> index 000000000000..3b10de929fd4
> --- /dev/null
> +++ b/drivers/clk/clk-bm1880.c
> @@ -0,0 +1,966 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Bitmain BM1880 SoC clock driver
> + *
> + * Copyright (c) 2019 Linaro Ltd.
> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <dt-bindings/clock/bm1880-clock.h>
> +
> +#define BM1880_CLK_MPLL_CTL 0x00
> +#define BM1880_CLK_SPLL_CTL 0x04
> +#define BM1880_CLK_FPLL_CTL 0x08
> +#define BM1880_CLK_DDRPLL_CTL 0x0c
> +
> +#define BM1880_CLK_ENABLE0 0x00
> +#define BM1880_CLK_ENABLE1 0x04
> +#define BM1880_CLK_SELECT 0x20
> +#define BM1880_CLK_DIV0 0x40
> +#define BM1880_CLK_DIV1 0x44
> +#define BM1880_CLK_DIV2 0x48
> +#define BM1880_CLK_DIV3 0x4c
> +#define BM1880_CLK_DIV4 0x50
> +#define BM1880_CLK_DIV5 0x54
> +#define BM1880_CLK_DIV6 0x58
> +#define BM1880_CLK_DIV7 0x5c
> +#define BM1880_CLK_DIV8 0x60
> +#define BM1880_CLK_DIV9 0x64
> +#define BM1880_CLK_DIV10 0x68
> +#define BM1880_CLK_DIV11 0x6c
> +#define BM1880_CLK_DIV12 0x70
> +#define BM1880_CLK_DIV13 0x74
> +#define BM1880_CLK_DIV14 0x78
> +#define BM1880_CLK_DIV15 0x7c
> +#define BM1880_CLK_DIV16 0x80
> +#define BM1880_CLK_DIV17 0x84
> +#define BM1880_CLK_DIV18 0x88
> +#define BM1880_CLK_DIV19 0x8c
> +#define BM1880_CLK_DIV20 0x90
> +#define BM1880_CLK_DIV21 0x94
> +#define BM1880_CLK_DIV22 0x98
> +#define BM1880_CLK_DIV23 0x9c
> +#define BM1880_CLK_DIV24 0xa0
> +#define BM1880_CLK_DIV25 0xa4
> +#define BM1880_CLK_DIV26 0xa8
> +#define BM1880_CLK_DIV27 0xac
> +#define BM1880_CLK_DIV28 0xb0
> +
> +#define to_bm1880_pll_clk(_hw) container_of(_hw, struct bm1880_pll_hw_clock, hw)
> +#define to_bm1880_div_clk(_hw) container_of(_hw, struct bm1880_div_hw_clock, hw)
> +
> +static DEFINE_SPINLOCK(bm1880_clk_lock);
> +
> +struct bm1880_clock_data {
> + void __iomem *pll_base;
> + void __iomem *sys_base;
> + struct clk_hw_onecell_data *clk_data;

Please call it hw_data or onecell_data instead so that the code doesn't
read as clk_data->clk_data. Also, probably can just make it part of the
same struct instead of a pointer inside so that it can all be allocated
in one big chunk instead of in two.

> +};
> +
> +struct bm1880_gate_clock {
> + unsigned int id;
> + const char *name;
> + const char *parent;
> + u32 gate_reg;
> + s8 gate_shift;
> + unsigned long flags;
> +};
> +
> +struct bm1880_mux_clock {
> + unsigned int id;
> + const char *name;
> + const char * const *parents;
> + s8 num_parents;
> + u32 reg;
> + s8 shift;
> + unsigned long flags;
> +};
> +
> +struct bm1880_div_clock {
> + unsigned int id;
> + const char *name;
> + u32 reg;
> + u8 shift;
> + u8 width;
> + u32 initval;
> + const struct clk_div_table *table;
> + unsigned long flags;
> +};
> +
> +struct bm1880_div_hw_clock {
> + struct bm1880_div_clock div;
> + void __iomem *base;
> + spinlock_t *lock;
> + struct clk_hw hw;
> + struct clk_init_data init;
> +};
> +
> +struct bm1880_composite_clock {
> + unsigned int id;
> + const char *name;
> + const char *parent;
> + const char * const *parents;
> + unsigned int num_parents;
> + unsigned long flags;
> +
> + u32 gate_reg;
> + u32 mux_reg;
> + u32 div_reg;
> +
> + s8 gate_shift;
> + s8 mux_shift;
> + s8 div_shift;
> + s8 div_width;
> + s16 div_initval;
> + const struct clk_div_table *table;
> +};
> +
> +struct bm1880_pll_clock {
> + unsigned int id;
> + const char *name;
> + u32 reg;
> + unsigned long flags;
> +};
> +
> +struct bm1880_pll_hw_clock {
> + struct bm1880_pll_clock pll;
> + void __iomem *base;
> + struct clk_hw hw;
> + struct clk_init_data init;
> +};
> +
> +static const struct clk_ops bm1880_pll_ops;
> +static const struct clk_ops bm1880_clk_div_ops;
> +
> +#define GATE_DIV(_id, _name, _parent, _gate_reg, _gate_shift, _div_reg, \
> + _div_shift, _div_width, _div_initval, _table, \
> + _flags) { \
> + .id = _id, \
> + .parent = _parent, \
> + .name = _name, \
> + .gate_reg = _gate_reg, \
> + .gate_shift = _gate_shift, \
> + .div_reg = _div_reg, \
> + .div_shift = _div_shift, \
> + .div_width = _div_width, \
> + .div_initval = _div_initval, \
> + .table = _table, \
> + .mux_shift = -1, \
> + .flags = _flags, \
> + }
> +
> +#define GATE_MUX(_id, _name, _parents, _gate_reg, _gate_shift, \
> + _mux_reg, _mux_shift, _flags) { \
> + .id = _id, \
> + .parents = _parents, \
> + .num_parents = ARRAY_SIZE(_parents), \
> + .name = _name, \
> + .gate_reg = _gate_reg, \
> + .gate_shift = _gate_shift, \
> + .div_shift = -1, \
> + .mux_reg = _mux_reg, \
> + .mux_shift = _mux_shift, \
> + .flags = _flags, \
> + }
> +
> +#define CLK_PLL(_id, _name, _parent, _reg, _flags) { \
> + .pll.id = _id, \
> + .pll.name = _name, \
> + .pll.reg = _reg, \
> + .hw.init = CLK_HW_INIT_PARENTS_DATA(_name, _parent, \
> + &bm1880_pll_ops, \
> + _flags), \
> + }
> +
> +#define CLK_DIV(_id, _name, _parent, _reg, _shift, _width, _initval, \
> + _table, _flags) { \
> + .div.id = _id, \
> + .div.name = _name, \
> + .div.reg = _reg, \
> + .div.shift = _shift, \
> + .div.width = _width, \
> + .div.initval = _initval, \
> + .div.table = _table, \
> + .hw.init = CLK_HW_INIT_HW(_name, _parent, \
> + &bm1880_clk_div_ops, \
> + _flags), \
> + }
> +
> +static struct clk_parent_data bm1880_pll_parent[] = {
> + { .fw_name = "osc", .name = "osc" },
> +};
> +
> +/*
> + * All PLL clocks are marked as CRITICAL, hence they are very crucial
> + * for the functioning of the SoC

Please add more information besides crucial to function of the clk.
Basically describe what the PLLs are clocking and why those child clks
aren't enabled or marked as critical themselves. The usage of
CLK_IS_CRITICAL is too liberal in this driver so this needs to be
cleaned up. For example, clk_mpll shouldn't be marked critical, just the
a53 CPU clk that can source from it should be marked critical because
it's for the CPU running code. It's also odd that we would register gate
clks or the a53 clks if we don't expect to ever turn those clks off. Can
that be avoided so that we don't need to mark anything critical for this
path?

> + */
> +static struct bm1880_pll_hw_clock bm1880_pll_clks[] = {
> + CLK_PLL(BM1880_CLK_MPLL, "clk_mpll", bm1880_pll_parent,
> + BM1880_CLK_MPLL_CTL, CLK_IS_CRITICAL),
> + CLK_PLL(BM1880_CLK_SPLL, "clk_spll", bm1880_pll_parent,
> + BM1880_CLK_SPLL_CTL, CLK_IS_CRITICAL),
> + CLK_PLL(BM1880_CLK_FPLL, "clk_fpll", bm1880_pll_parent,
> + BM1880_CLK_FPLL_CTL, CLK_IS_CRITICAL),
> + CLK_PLL(BM1880_CLK_DDRPLL, "clk_ddrpll", bm1880_pll_parent,
> + BM1880_CLK_DDRPLL_CTL, CLK_IS_CRITICAL),
> +};
> +
> +/*
> + * Clocks marked as CRITICAL are needed for the proper functioning
> + * of the SoC.
> + */
> +static const struct bm1880_gate_clock bm1880_gate_clks[] = {
> + { BM1880_CLK_AHB_ROM, "clk_ahb_rom", "clk_mux_axi6",
> + BM1880_CLK_ENABLE0, 2, CLK_IS_CRITICAL },
> + { BM1880_CLK_AXI_SRAM, "clk_axi_sram", "clk_axi1",
> + BM1880_CLK_ENABLE0, 3, CLK_IS_CRITICAL },
> + { BM1880_CLK_DDR_AXI, "clk_ddr_axi", "clk_mux_axi6",
> + BM1880_CLK_ENABLE0, 4, CLK_IS_CRITICAL },
> + { BM1880_CLK_APB_EFUSE, "clk_apb_efuse", "clk_mux_axi6",
> + BM1880_CLK_ENABLE0, 6, CLK_IS_CRITICAL },
> + { BM1880_CLK_AXI5_EMMC, "clk_axi5_emmc", "clk_axi5",
> + BM1880_CLK_ENABLE0, 7, 0 },
> + { BM1880_CLK_AXI5_SD, "clk_axi5_sd", "clk_axi5",
> + BM1880_CLK_ENABLE0, 10, 0 },
> + { BM1880_CLK_AXI4_ETH0, "clk_axi4_eth0", "clk_axi4",
> + BM1880_CLK_ENABLE0, 14, 0 },
> + { BM1880_CLK_AXI4_ETH1, "clk_axi4_eth1", "clk_axi4",
> + BM1880_CLK_ENABLE0, 16, 0 },
> + { BM1880_CLK_AXI1_GDMA, "clk_axi1_gdma", "clk_axi1",
> + BM1880_CLK_ENABLE0, 17, 0 },
> + /* Don't gate GPIO clocks as it is not owned by the GPIO driver */
> + { BM1880_CLK_APB_GPIO, "clk_apb_gpio", "clk_mux_axi6",
> + BM1880_CLK_ENABLE0, 18, CLK_IGNORE_UNUSED },
> + { BM1880_CLK_APB_GPIO_INTR, "clk_apb_gpio_intr", "clk_mux_axi6",
> + BM1880_CLK_ENABLE0, 19, CLK_IGNORE_UNUSED },
> + { BM1880_CLK_AXI1_MINER, "clk_axi1_miner", "clk_axi1",
> + BM1880_CLK_ENABLE0, 21, 0 },
> + { BM1880_CLK_AHB_SF, "clk_ahb_sf", "clk_mux_axi6",
> + BM1880_CLK_ENABLE0, 22, 0 },
> + { BM1880_CLK_SDMA_AXI, "clk_sdma_axi", "clk_axi5",
> + BM1880_CLK_ENABLE0, 23, 0 },
> + { BM1880_CLK_APB_I2C, "clk_apb_i2c", "clk_mux_axi6",
> + BM1880_CLK_ENABLE0, 25, 0 },
> + { BM1880_CLK_APB_WDT, "clk_apb_wdt", "clk_mux_axi6",
> + BM1880_CLK_ENABLE0, 26, 0 },
> + { BM1880_CLK_APB_JPEG, "clk_apb_jpeg", "clk_axi6",
> + BM1880_CLK_ENABLE0, 27, 0 },
> + { BM1880_CLK_AXI5_NF, "clk_axi5_nf", "clk_axi5",
> + BM1880_CLK_ENABLE0, 29, 0 },
> + { BM1880_CLK_APB_NF, "clk_apb_nf", "clk_axi6",
> + BM1880_CLK_ENABLE0, 30, 0 },
> + { BM1880_CLK_APB_PWM, "clk_apb_pwm", "clk_mux_axi6",
> + BM1880_CLK_ENABLE1, 0, 0 },
> + { BM1880_CLK_RV, "clk_rv", "clk_mux_rv",
> + BM1880_CLK_ENABLE1, 1, 0 },
> + { BM1880_CLK_APB_SPI, "clk_apb_spi", "clk_mux_axi6",
> + BM1880_CLK_ENABLE1, 2, 0 },
> + { BM1880_CLK_UART_500M, "clk_uart_500m", "clk_div_uart_500m",
> + BM1880_CLK_ENABLE1, 4, 0 },
> + { BM1880_CLK_APB_UART, "clk_apb_uart", "clk_axi6",
> + BM1880_CLK_ENABLE1, 5, 0 },
> + { BM1880_CLK_APB_I2S, "clk_apb_i2s", "clk_axi6",
> + BM1880_CLK_ENABLE1, 6, 0 },
> + { BM1880_CLK_AXI4_USB, "clk_axi4_usb", "clk_axi4",
> + BM1880_CLK_ENABLE1, 7, 0 },
> + { BM1880_CLK_APB_USB, "clk_apb_usb", "clk_axi6",
> + BM1880_CLK_ENABLE1, 8, 0 },
> + { BM1880_CLK_12M_USB, "clk_12m_usb", "clk_div_12m_usb",
> + BM1880_CLK_ENABLE1, 11, 0 },
> + { BM1880_CLK_APB_VIDEO, "clk_apb_video", "clk_axi6",
> + BM1880_CLK_ENABLE1, 12, 0 },
> + { BM1880_CLK_APB_VPP, "clk_apb_vpp", "clk_axi6",
> + BM1880_CLK_ENABLE1, 15, 0 },
> + { BM1880_CLK_AXI6, "clk_axi6", "clk_mux_axi6",
> + BM1880_CLK_ENABLE1, 21, CLK_IS_CRITICAL },
> +};
> +
> +static const char * const clk_a53_parents[] = { "clk_spll", "clk_mpll" };
> +static const char * const clk_rv_parents[] = { "clk_div_1_rv", "clk_div_0_rv" };
> +static const char * const clk_axi1_parents[] = { "clk_div_1_axi1", "clk_div_0_axi1" };
> +static const char * const clk_axi6_parents[] = { "clk_div_1_axi6", "clk_div_0_axi6" };

I sent some patches to make the basic clk types support the new way of
specifying parents. Can you use those patches instead of listing all
these strings?

> +
> +static const struct bm1880_mux_clock bm1880_mux_clks[] = {
> + { BM1880_CLK_MUX_RV, "clk_mux_rv", clk_rv_parents, 2,
> + BM1880_CLK_SELECT, 1, 0 },
> + { BM1880_CLK_MUX_AXI6, "clk_mux_axi6", clk_axi6_parents, 2,
> + BM1880_CLK_SELECT, 3, 0 },
> +};
> +
> +static const struct clk_div_table bm1880_div_table_0[] = {
> + { 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
> + { 4, 5 }, { 5, 6 }, { 6, 7 }, { 7, 8 },
> + { 8, 9 }, { 9, 10 }, { 10, 11 }, { 11, 12 },
> + { 12, 13 }, { 13, 14 }, { 14, 15 }, { 15, 16 },
> + { 16, 17 }, { 17, 18 }, { 18, 19 }, { 19, 20 },
> + { 20, 21 }, { 21, 22 }, { 22, 23 }, { 23, 24 },
> + { 24, 25 }, { 25, 26 }, { 26, 27 }, { 27, 28 },
> + { 28, 29 }, { 29, 30 }, { 30, 31 }, { 31, 32 },
> + { 0, 0 }
> +};
> +
> +static const struct clk_div_table bm1880_div_table_1[] = {
> + { 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
> + { 4, 5 }, { 5, 6 }, { 6, 7 }, { 7, 8 },
> + { 8, 9 }, { 9, 10 }, { 10, 11 }, { 11, 12 },
> + { 12, 13 }, { 13, 14 }, { 14, 15 }, { 15, 16 },
> + { 16, 17 }, { 17, 18 }, { 18, 19 }, { 19, 20 },
> + { 20, 21 }, { 21, 22 }, { 22, 23 }, { 23, 24 },
> + { 24, 25 }, { 25, 26 }, { 26, 27 }, { 27, 28 },
> + { 28, 29 }, { 29, 30 }, { 30, 31 }, { 31, 32 },
> + { 127, 128 }, { 0, 0 }
> +};
> +
> +static const struct clk_div_table bm1880_div_table_2[] = {
> + { 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
> + { 4, 5 }, { 5, 6 }, { 6, 7 }, { 7, 8 },
> + { 8, 9 }, { 9, 10 }, { 10, 11 }, { 11, 12 },
> + { 12, 13 }, { 13, 14 }, { 14, 15 }, { 15, 16 },
> + { 16, 17 }, { 17, 18 }, { 18, 19 }, { 19, 20 },
> + { 20, 21 }, { 21, 22 }, { 22, 23 }, { 23, 24 },
> + { 24, 25 }, { 25, 26 }, { 26, 27 }, { 27, 28 },
> + { 28, 29 }, { 29, 30 }, { 30, 31 }, { 31, 32 },
> + { 127, 128 }, { 255, 256 }, { 0, 0 }
> +};
> +
> +static const struct clk_div_table bm1880_div_table_3[] = {
> + { 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
> + { 4, 5 }, { 5, 6 }, { 6, 7 }, { 7, 8 },
> + { 8, 9 }, { 9, 10 }, { 10, 11 }, { 11, 12 },
> + { 12, 13 }, { 13, 14 }, { 14, 15 }, { 15, 16 },
> + { 16, 17 }, { 17, 18 }, { 18, 19 }, { 19, 20 },
> + { 20, 21 }, { 21, 22 }, { 22, 23 }, { 23, 24 },
> + { 24, 25 }, { 25, 26 }, { 26, 27 }, { 27, 28 },
> + { 28, 29 }, { 29, 30 }, { 30, 31 }, { 31, 32 },
> + { 127, 128 }, { 255, 256 }, { 511, 512 }, { 0, 0 }
> +};
> +
> +static const struct clk_div_table bm1880_div_table_4[] = {
> + { 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
> + { 4, 5 }, { 5, 6 }, { 6, 7 }, { 7, 8 },
> + { 8, 9 }, { 9, 10 }, { 10, 11 }, { 11, 12 },
> + { 12, 13 }, { 13, 14 }, { 14, 15 }, { 15, 16 },
> + { 16, 17 }, { 17, 18 }, { 18, 19 }, { 19, 20 },
> + { 20, 21 }, { 21, 22 }, { 22, 23 }, { 23, 24 },
> + { 24, 25 }, { 25, 26 }, { 26, 27 }, { 27, 28 },
> + { 28, 29 }, { 29, 30 }, { 30, 31 }, { 31, 32 },
> + { 127, 128 }, { 255, 256 }, { 511, 512 }, { 65535, 65536 },
> + { 0, 0 }
> +};
> +
> +/*
> + * Clocks marked as CRITICAL are needed for the proper functioning
> + * of the SoC.
> + */
> +static struct bm1880_div_hw_clock bm1880_div_clks[] = {
> + CLK_DIV(BM1880_CLK_DIV_0_RV, "clk_div_0_rv", &bm1880_pll_clks[1].hw,
> + BM1880_CLK_DIV12, 16, 5, 1, bm1880_div_table_0, 0),
> + CLK_DIV(BM1880_CLK_DIV_1_RV, "clk_div_1_rv", &bm1880_pll_clks[2].hw,
> + BM1880_CLK_DIV13, 16, 5, 1, bm1880_div_table_0, 0),
> + CLK_DIV(BM1880_CLK_DIV_UART_500M, "clk_div_uart_500m", &bm1880_pll_clks[2].hw,
> + BM1880_CLK_DIV15, 16, 7, 3, bm1880_div_table_1, 0),
> + CLK_DIV(BM1880_CLK_DIV_0_AXI1, "clk_div_0_axi1", &bm1880_pll_clks[0].hw,
> + BM1880_CLK_DIV21, 16, 5, 2, bm1880_div_table_0,
> + CLK_IS_CRITICAL),
> + CLK_DIV(BM1880_CLK_DIV_1_AXI1, "clk_div_1_axi1", &bm1880_pll_clks[2].hw,
> + BM1880_CLK_DIV22, 16, 5, 3, bm1880_div_table_0,
> + CLK_IS_CRITICAL),
> + CLK_DIV(BM1880_CLK_DIV_0_AXI6, "clk_div_0_axi6", &bm1880_pll_clks[2].hw,
> + BM1880_CLK_DIV27, 16, 5, 15, bm1880_div_table_0,
> + CLK_IS_CRITICAL),
> + CLK_DIV(BM1880_CLK_DIV_1_AXI6, "clk_div_1_axi6", &bm1880_pll_clks[0].hw,
> + BM1880_CLK_DIV28, 16, 5, 11, bm1880_div_table_0,
> + CLK_IS_CRITICAL),
> + CLK_DIV(BM1880_CLK_DIV_12M_USB, "clk_div_12m_usb", &bm1880_pll_clks[2].hw,
> + BM1880_CLK_DIV18, 16, 7, 125, bm1880_div_table_1, 0),
> +};
> +
> +/*
> + * Clocks marked as CRITICAL are all needed for the proper functioning
> + * of the SoC.
> + */
> +static struct bm1880_composite_clock bm1880_composite_clks[] = {
> + GATE_MUX(BM1880_CLK_A53, "clk_a53", clk_a53_parents,
> + BM1880_CLK_ENABLE0, 0, BM1880_CLK_SELECT, 0,
> + CLK_IS_CRITICAL),
> + GATE_DIV(BM1880_CLK_50M_A53, "clk_50m_a53", "clk_fpll",
> + BM1880_CLK_ENABLE0, 1, BM1880_CLK_DIV0, 16, 5, 30,
> + bm1880_div_table_0, CLK_IS_CRITICAL),
> + GATE_DIV(BM1880_CLK_EFUSE, "clk_efuse", "clk_fpll",
> + BM1880_CLK_ENABLE0, 5, BM1880_CLK_DIV1, 16, 7, 60,
> + bm1880_div_table_1, 0),
> + GATE_DIV(BM1880_CLK_EMMC, "clk_emmc", "clk_fpll",
> + BM1880_CLK_ENABLE0, 8, BM1880_CLK_DIV2, 16, 5, 15,
> + bm1880_div_table_0, 0),
> + GATE_DIV(BM1880_CLK_100K_EMMC, "clk_100k_emmc", "clk_div_12m_usb",
> + BM1880_CLK_ENABLE0, 9, BM1880_CLK_DIV3, 16, 8, 120,
> + bm1880_div_table_2, 0),
> + GATE_DIV(BM1880_CLK_SD, "clk_sd", "clk_fpll",
> + BM1880_CLK_ENABLE0, 11, BM1880_CLK_DIV4, 16, 5, 15,
> + bm1880_div_table_0, 0),
> + GATE_DIV(BM1880_CLK_100K_SD, "clk_100k_sd", "clk_div_12m_usb",
> + BM1880_CLK_ENABLE0, 12, BM1880_CLK_DIV5, 16, 8, 120,
> + bm1880_div_table_2, 0),
> + GATE_DIV(BM1880_CLK_500M_ETH0, "clk_500m_eth0", "clk_fpll",
> + BM1880_CLK_ENABLE0, 13, BM1880_CLK_DIV6, 16, 5, 3,
> + bm1880_div_table_0, 0),
> + GATE_DIV(BM1880_CLK_500M_ETH1, "clk_500m_eth1", "clk_fpll",
> + BM1880_CLK_ENABLE0, 15, BM1880_CLK_DIV7, 16, 5, 3,
> + bm1880_div_table_0, 0),
> + /* Don't gate GPIO clocks as it is not owned by the GPIO driver */
> + GATE_DIV(BM1880_CLK_GPIO_DB, "clk_gpio_db", "clk_div_12m_usb",
> + BM1880_CLK_ENABLE0, 20, BM1880_CLK_DIV8, 16, 16, 120,
> + bm1880_div_table_4, CLK_IGNORE_UNUSED),
> + GATE_DIV(BM1880_CLK_SDMA_AUD, "clk_sdma_aud", "clk_fpll",
> + BM1880_CLK_ENABLE0, 24, BM1880_CLK_DIV9, 16, 7, 61,
> + bm1880_div_table_1, 0),
> + GATE_DIV(BM1880_CLK_JPEG_AXI, "clk_jpeg_axi", "clk_fpll",
> + BM1880_CLK_ENABLE0, 28, BM1880_CLK_DIV10, 16, 5, 4,
> + bm1880_div_table_0, 0),
> + GATE_DIV(BM1880_CLK_NF, "clk_nf", "clk_fpll",
> + BM1880_CLK_ENABLE0, 31, BM1880_CLK_DIV11, 16, 5, 30,
> + bm1880_div_table_0, 0),
> + GATE_DIV(BM1880_CLK_TPU_AXI, "clk_tpu_axi", "clk_spll",
> + BM1880_CLK_ENABLE1, 3, BM1880_CLK_DIV14, 16, 5, 1,
> + bm1880_div_table_0, 0),
> + GATE_DIV(BM1880_CLK_125M_USB, "clk_125m_usb", "clk_fpll",
> + BM1880_CLK_ENABLE1, 9, BM1880_CLK_DIV16, 16, 5, 12,
> + bm1880_div_table_0, 0),
> + GATE_DIV(BM1880_CLK_33K_USB, "clk_33k_usb", "clk_div_12m_usb",
> + BM1880_CLK_ENABLE1, 10, BM1880_CLK_DIV17, 16, 9, 363,
> + bm1880_div_table_3, 0),
> + GATE_DIV(BM1880_CLK_VIDEO_AXI, "clk_video_axi", "clk_fpll",
> + BM1880_CLK_ENABLE1, 13, BM1880_CLK_DIV19, 16, 5, 4,
> + bm1880_div_table_0, 0),
> + GATE_DIV(BM1880_CLK_VPP_AXI, "clk_vpp_axi", "clk_fpll",
> + BM1880_CLK_ENABLE1, 14, BM1880_CLK_DIV20, 16, 5, 4,
> + bm1880_div_table_0, 0),
> + GATE_MUX(BM1880_CLK_AXI1, "clk_axi1", clk_axi1_parents,
> + BM1880_CLK_ENABLE1, 15, BM1880_CLK_SELECT, 2,
> + CLK_IS_CRITICAL),
> + GATE_DIV(BM1880_CLK_AXI2, "clk_axi2", "clk_fpll",
> + BM1880_CLK_ENABLE1, 17, BM1880_CLK_DIV23, 16, 5, 3,
> + bm1880_div_table_0, CLK_IS_CRITICAL),
> + GATE_DIV(BM1880_CLK_AXI3, "clk_axi3", "clk_mux_rv",
> + BM1880_CLK_ENABLE1, 18, BM1880_CLK_DIV24, 16, 5, 2,
> + bm1880_div_table_0, CLK_IS_CRITICAL),
> + GATE_DIV(BM1880_CLK_AXI4, "clk_axi4", "clk_fpll",
> + BM1880_CLK_ENABLE1, 19, BM1880_CLK_DIV25, 16, 5, 6,
> + bm1880_div_table_0, CLK_IS_CRITICAL),
> + GATE_DIV(BM1880_CLK_AXI5, "clk_axi5", "clk_fpll",
> + BM1880_CLK_ENABLE1, 20, BM1880_CLK_DIV26, 16, 5, 15,
> + bm1880_div_table_0, CLK_IS_CRITICAL),
> +};
> +
> +static unsigned long bm1880_pll_rate_calc(u32 regval, unsigned long parent_rate)
> +{
> + u32 fbdiv, fref, refdiv;
> + u32 postdiv1, postdiv2;
> + unsigned long rate, numerator, denominator;
> +
> + fbdiv = (regval >> 16) & 0xfff;
> + fref = parent_rate;
> + refdiv = regval & 0x1f;
> + postdiv1 = (regval >> 8) & 0x7;
> + postdiv2 = (regval >> 12) & 0x7;
> +
> + numerator = parent_rate * fbdiv;
> + denominator = refdiv * postdiv1 * postdiv2;
> + do_div(numerator, denominator);
> + rate = numerator;
> +
> + return rate;
> +}
> +
> +static unsigned long bm1880_pll_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct bm1880_pll_hw_clock *pll_hw = to_bm1880_pll_clk(hw);
> + unsigned long rate;
> + u32 regval;
> +
> + regval = readl(pll_hw->base + pll_hw->pll.reg);
> + rate = bm1880_pll_rate_calc(regval, parent_rate);
> +
> + return rate;
> +}
> +
> +static const struct clk_ops bm1880_pll_ops = {
> + .recalc_rate = bm1880_pll_recalc_rate,
> +};
> +
> +static struct clk_hw *bm1880_clk_register_pll(struct bm1880_pll_hw_clock *pll_clk,
> + void __iomem *sys_base)
> +{
> + struct clk_hw *hw;
> + int err;
> +
> + pll_clk->base = sys_base;
> + hw = &pll_clk->hw;
> +
> + err = clk_hw_register(NULL, hw);
> + if (err)
> + return ERR_PTR(err);
> +
> + return hw;
> +}
> +
> +static void bm1880_clk_unregister_pll(struct clk_hw *hw)
> +{
> + struct bm1880_pll_hw_clock *pll_hw = to_bm1880_pll_clk(hw);
> +
> + clk_hw_unregister(hw);
> + kfree(pll_hw);
> +}
> +
> +static int bm1880_clk_register_plls(struct bm1880_pll_hw_clock *clks,
> + int num_clks, struct bm1880_clock_data *data)
> +{
> + struct clk_hw *hw;
> + void __iomem *pll_base = data->pll_base;
> + int i;
> +
> + for (i = 0; i < num_clks; i++) {
> + struct bm1880_pll_hw_clock *bm1880_clk = &clks[i];
> +
> + hw = bm1880_clk_register_pll(bm1880_clk, pll_base);
> + if (IS_ERR(hw)) {
> + pr_err("%s: failed to register clock %s\n",
> + __func__, bm1880_clk->pll.name);
> + goto err_clk;
> + }
> +
> + data->clk_data->hws[clks[i].pll.id] = hw;
> + }
> +
> + return 0;
> +
> +err_clk:
> + while (i--)
> + bm1880_clk_unregister_pll(data->clk_data->hws[clks[i].pll.id]);
> +
> + return PTR_ERR(hw);
> +}
> +
> +static int bm1880_clk_register_mux(const struct bm1880_mux_clock *clks,
> + int num_clks, struct bm1880_clock_data *data)
> +{
> + struct clk_hw *hw;
> + void __iomem *sys_base = data->sys_base;
> + int i;
> +
> + for (i = 0; i < num_clks; i++) {
> + hw = clk_hw_register_mux(NULL, clks[i].name,
> + clks[i].parents,
> + clks[i].num_parents,
> + clks[i].flags,
> + sys_base + clks[i].reg,
> + clks[i].shift, 1, 0,
> + &bm1880_clk_lock);
> + if (IS_ERR(hw)) {
> + pr_err("%s: failed to register clock %s\n",
> + __func__, clks[i].name);
> + goto err_clk;
> + }
> +
> + data->clk_data->hws[clks[i].id] = hw;
> + }
> +
> + return 0;
> +
> +err_clk:
> + while (i--)
> + clk_hw_unregister_mux(data->clk_data->hws[clks[i].id]);
> +
> + return PTR_ERR(hw);
> +}
> +
> +static unsigned long bm1880_clk_div_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + struct bm1880_div_hw_clock *div_hw = to_bm1880_div_clk(hw);
> + struct bm1880_div_clock *div = &div_hw->div;
> + void __iomem *reg_addr = div_hw->base + div->reg;
> + unsigned int val;
> + unsigned long rate;
> +
> + if (!(readl(reg_addr) & BIT(3))) {
> + val = div->initval;
> + } else {
> + val = readl(reg_addr) >> div->shift;
> + val &= clk_div_mask(div->width);
> + }
> +
> + rate = divider_recalc_rate(hw, parent_rate, val, div->table,
> + div->flags, div->width);
> +
> + return rate;
> +}
> +
> +static long bm1880_clk_div_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *prate)
> +{
> + struct bm1880_div_hw_clock *div_hw = to_bm1880_div_clk(hw);
> + struct bm1880_div_clock *div = &div_hw->div;
> + void __iomem *reg_addr = div_hw->base + div->reg;
> +
> + if (div->flags & CLK_DIVIDER_READ_ONLY) {
> + u32 val;
> +
> + val = readl(reg_addr) >> div->shift;
> + val &= clk_div_mask(div->width);
> +
> + return divider_ro_round_rate(hw, rate, prate, div->table,
> + div->width, div->flags,
> + val);
> + }
> +
> + return divider_round_rate(hw, rate, prate, div->table,
> + div->width, div->flags);
> +}
> +
> +static int bm1880_clk_div_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + struct bm1880_div_hw_clock *div_hw = to_bm1880_div_clk(hw);
> + struct bm1880_div_clock *div = &div_hw->div;
> + void __iomem *reg_addr = div_hw->base + div->reg;
> + unsigned long flags = 0;
> + int value;
> + u32 val;
> +
> + value = divider_get_val(rate, parent_rate, div->table,
> + div->width, div_hw->div.flags);
> + if (value < 0)
> + return value;
> +
> + if (div_hw->lock)
> + spin_lock_irqsave(div_hw->lock, flags);
> + else
> + __acquire(div_hw->lock);
> +
> + if (div->flags & CLK_DIVIDER_HIWORD_MASK) {

Is this used by your driver? As far as I can recall it was a
rockchip/hisilicon specific thing that hasn't happened since.

> + val = clk_div_mask(div->width) << (div_hw->div.shift + 16);
> + } else {
> + val = readl(reg_addr);
> + val &= ~(clk_div_mask(div->width) << div_hw->div.shift);
> + }
> + val |= (u32)value << div->shift;
> + writel(val, reg_addr);
> +
> + if (div_hw->lock)
> + spin_unlock_irqrestore(div_hw->lock, flags);
> + else
> + __release(div_hw->lock);
> +
> + return 0;
> +}
> +
> +static const struct clk_ops bm1880_clk_div_ops = {
> + .recalc_rate = bm1880_clk_div_recalc_rate,
> + .round_rate = bm1880_clk_div_round_rate,
> + .set_rate = bm1880_clk_div_set_rate,
> +};
> +
> +static struct clk_hw *bm1880_clk_register_div(struct bm1880_div_hw_clock *div_clk,
> + void __iomem *sys_base)
> +{
> + struct clk_hw *hw;
> + int err;
> +
> + div_clk->div.flags = CLK_DIVIDER_ONE_BASED | CLK_DIVIDER_ALLOW_ZERO;
> + div_clk->base = sys_base;
> + div_clk->lock = &bm1880_clk_lock;
> +
> + hw = &div_clk->hw;
> + err = clk_hw_register(NULL, hw);
> + if (err)
> + return ERR_PTR(err);
> +
> + return hw;
> +}
> +
> +static void bm1880_clk_unregister_div(struct clk_hw *hw)
> +{
> + struct bm1880_div_hw_clock *div_hw = to_bm1880_div_clk(hw);
> +
> + clk_hw_unregister(hw);
> + kfree(div_hw);
> +}
> +
> +static int bm1880_clk_register_divs(struct bm1880_div_hw_clock *clks,
> + int num_clks, struct bm1880_clock_data *data)
> +{
> + struct clk_hw *hw;
> + void __iomem *sys_base = data->sys_base;
> + int i;
> +
> + for (i = 0; i < num_clks; i++) {
> + struct bm1880_div_hw_clock *bm1880_clk = &clks[i];
> +
> + hw = bm1880_clk_register_div(bm1880_clk, sys_base);
> + if (IS_ERR(hw)) {
> + pr_err("%s: failed to register clock %s\n",
> + __func__, bm1880_clk->div.name);
> + goto err_clk;
> + }
> +
> + data->clk_data->hws[clks[i].div.id] = hw;

This line is overly complicated. Please use local variables.

> + }
> +
> + return 0;
> +
> +err_clk:
> + while (i--)
> + bm1880_clk_unregister_div(data->clk_data->hws[clks[i].div.id]);
> +
> + return PTR_ERR(hw);
> +}
> +
> +static int bm1880_clk_register_gate(const struct bm1880_gate_clock *clks,
> + int num_clks, struct bm1880_clock_data *data)
> +{
> + struct clk_hw *hw;
> + void __iomem *sys_base = data->sys_base;
> + int i;
> +
> + for (i = 0; i < num_clks; i++) {
> + hw = clk_hw_register_gate(NULL, clks[i].name,
> + clks[i].parent,
> + clks[i].flags,
> + sys_base + clks[i].gate_reg,
> + clks[i].gate_shift,
> + 0,
> + &bm1880_clk_lock);

Weird tabs here.

> + if (IS_ERR(hw)) {
> + pr_err("%s: failed to register clock %s\n",
> + __func__, clks[i].name);
> + goto err_clk;
> + }
> +
> + data->clk_data->hws[clks[i].id] = hw;
> + }
> +
> + return 0;
> +
> +err_clk:
> + while (i--)
> + clk_hw_unregister_gate(data->clk_data->hws[clks[i].id]);
> +
> + return PTR_ERR(hw);
> +}
> +
> +static struct clk_hw *bm1880_clk_register_composite(struct bm1880_composite_clock *clks,
> + void __iomem *sys_base)
> +{
> + struct clk_hw *hw;
> + struct clk_mux *mux = NULL;
> + struct clk_gate *gate = NULL;
> + struct bm1880_div_hw_clock *div_hws = NULL;
> + struct clk_hw *mux_hw = NULL, *gate_hw = NULL, *div_hw = NULL;
> + const struct clk_ops *mux_ops = NULL, *gate_ops = NULL, *div_ops = NULL;
> + const char * const *parent_names;
> + const char *parent;
> + int num_parents;
> + int ret;
> +
> + if (clks->mux_shift >= 0) {
> + mux = kzalloc(sizeof(*mux), GFP_KERNEL);
> + if (!mux)
> + return ERR_PTR(-ENOMEM);
> +
> + mux->reg = sys_base + clks->mux_reg;
> + mux->mask = 1;
> + mux->shift = clks->mux_shift;
> + mux_hw = &mux->hw;
> + mux_ops = &clk_mux_ops;
> + mux->lock = &bm1880_clk_lock;
> +
> + parent_names = clks->parents;
> + num_parents = clks->num_parents;
> + } else {
> + parent = clks->parent;
> + parent_names = &parent;
> + num_parents = 1;
> + }
> +
> + if (clks->gate_shift >= 0) {
> + gate = kzalloc(sizeof(*gate), GFP_KERNEL);
> + if (!gate) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
> +
> + gate->reg = sys_base + clks->gate_reg;
> + gate->bit_idx = clks->gate_shift;
> + gate->lock = &bm1880_clk_lock;
> +
> + gate_hw = &gate->hw;
> + gate_ops = &clk_gate_ops;
> + }
> +
> + if (clks->div_shift >= 0) {
> + div_hws = kzalloc(sizeof(*div_hws), GFP_KERNEL);
> + if (!div_hws) {
> + ret = -ENOMEM;
> + goto err_out;
> + }
> +
> + div_hws->base = sys_base;
> + div_hws->div.reg = clks->div_reg;
> + div_hws->div.shift = clks->div_shift;
> + div_hws->div.width = clks->div_width;
> + div_hws->div.table = clks->table;
> + div_hws->div.initval = clks->div_initval;
> + div_hws->lock = &bm1880_clk_lock;
> + div_hws->div.flags = CLK_DIVIDER_ONE_BASED |
> + CLK_DIVIDER_ALLOW_ZERO;
> +
> + div_hw = &div_hws->hw;
> + div_ops = &bm1880_clk_div_ops;
> + }
> +
> + hw = clk_hw_register_composite(NULL, clks->name, parent_names,
> + num_parents, mux_hw, mux_ops, div_hw,
> + div_ops, gate_hw, gate_ops,
> + clks->flags);
> +

All of these need to be removed on probe failure. Any plans to do so?

> + if (IS_ERR(hw)) {
> + ret = PTR_ERR(hw);
> + goto err_out;
> + }
> +
> + return hw;
> +
> +err_out:
> + kfree(div_hws);
> + kfree(gate);
> + kfree(mux);
> +
> + return ERR_PTR(ret);
> +}
> +
> +static int bm1880_clk_register_composites(struct bm1880_composite_clock *clks,
> + int num_clks, struct bm1880_clock_data *data)
> +{
> + struct clk_hw *hw;
> + void __iomem *sys_base = data->sys_base;
> + int i;
> +
> + for (i = 0; i < num_clks; i++) {
> + struct bm1880_composite_clock *bm1880_clk = &clks[i];
> +
> + hw = bm1880_clk_register_composite(bm1880_clk, sys_base);
> + if (IS_ERR(hw)) {
> + pr_err("%s: failed to register clock %s\n",
> + __func__, bm1880_clk->name);
> + goto err_clk;
> + }
> +
> + data->clk_data->hws[clks[i].id] = hw;
> + }
> +
> + return 0;
> +
> +err_clk:
> + while (i--)
> + clk_hw_unregister_composite(data->clk_data->hws[clks[i].id]);
> +
> + return PTR_ERR(hw);
> +}
> +
> +static int bm1880_clk_probe(struct platform_device *pdev)
> +{
> + struct bm1880_clock_data *clk_data;
> + void __iomem *pll_base, *sys_base;
> + struct device_node *np = pdev->dev.of_node;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct clk_hw_onecell_data *clk_hw_data;
> + int num_clks, i;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + pll_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(pll_base))
> + return PTR_ERR(pll_base);
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + sys_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(sys_base))
> + return PTR_ERR(sys_base);
> +
> + clk_data = devm_kzalloc(dev, sizeof(*clk_data), GFP_KERNEL);
> + if (!clk_data)
> + return -ENOMEM;
> +
> + clk_data->pll_base = pll_base;
> + clk_data->sys_base = sys_base;
> +
> + num_clks = ARRAY_SIZE(bm1880_pll_clks) +
> + ARRAY_SIZE(bm1880_div_clks) +
> + ARRAY_SIZE(bm1880_mux_clks) +
> + ARRAY_SIZE(bm1880_composite_clks) +
> + ARRAY_SIZE(bm1880_gate_clks);
> +
> + clk_hw_data = devm_kzalloc(&pdev->dev, struct_size(clk_hw_data, hws,
> + num_clks), GFP_KERNEL);
> + if (!clk_hw_data)
> + return -ENOMEM;
> +
> + clk_data->clk_data = clk_hw_data;
> +
> + for (i = 0; i < num_clks; i++)
> + clk_data->clk_data->hws[i] = ERR_PTR(-ENOENT);
> +
> + clk_data->clk_data->num = num_clks;
> +
> + bm1880_clk_register_plls(bm1880_pll_clks,
> + ARRAY_SIZE(bm1880_pll_clks),
> + clk_data);
> +
> + bm1880_clk_register_divs(bm1880_div_clks,
> + ARRAY_SIZE(bm1880_div_clks),
> + clk_data);
> +
> + bm1880_clk_register_mux(bm1880_mux_clks,
> + ARRAY_SIZE(bm1880_mux_clks),
> + clk_data);
> +
> + bm1880_clk_register_composites(bm1880_composite_clks,
> + ARRAY_SIZE(bm1880_composite_clks),
> + clk_data);
> +
> + bm1880_clk_register_gate(bm1880_gate_clks,
> + ARRAY_SIZE(bm1880_gate_clks),
> + clk_data);
> +
> + return of_clk_add_hw_provider(np, of_clk_hw_onecell_get,

Can you use devm_of_clk_add_hw_provider()?

> + clk_data->clk_data);
> +}

Next message: Stephen Boyd: "Re: [PATCH 1/2] dt-bindings: documentation: add clock bindings information for Agilex"
Previous message: Navid Emamdoost: "[PATCH] drivers/net: release skb on failure"
In reply to: Manivannan Sadhasivam: "[PATCH v5 7/8] clk: Add common clock driver for BM1880 SoC"
Next in thread: Manivannan Sadhasivam: "[PATCH v5 8/8] MAINTAINERS: Add entry for BM1880 SoC clock driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]