| Message ID | 1601270140-4306-1-git-send-email-varada@codeaurora.org |
|---|---|
| Headers | show |
| Series | Add minimal boot support for IPQ5018 | expand |
On Mon, Sep 28, 2020 at 10:45:35AM +0530, Varadarajan Narayanan wrote: > This patch adds support for the global clock controller found on > the IPQ5018 based devices. > > Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> > --- > .../devicetree/bindings/clock/qcom,gcc.yaml | 3 + > include/dt-bindings/clock/qcom,gcc-ipq5018.h | 183 +++++++++++++++++++++ > include/dt-bindings/reset/qcom,gcc-ipq5018.h | 119 ++++++++++++++ > 3 files changed, 305 insertions(+) > create mode 100644 include/dt-bindings/clock/qcom,gcc-ipq5018.h > create mode 100644 include/dt-bindings/reset/qcom,gcc-ipq5018.h > > diff --git a/Documentation/devicetree/bindings/clock/qcom,gcc.yaml b/Documentation/devicetree/bindings/clock/qcom,gcc.yaml > index ee0467f..74d67fc 100644 > --- a/Documentation/devicetree/bindings/clock/qcom,gcc.yaml > +++ b/Documentation/devicetree/bindings/clock/qcom,gcc.yaml > @@ -18,6 +18,8 @@ description: | > - dt-bindings/clock/qcom,gcc-apq8084.h > - dt-bindings/reset/qcom,gcc-apq8084.h > - dt-bindings/clock/qcom,gcc-ipq4019.h > + - dt-bindings/clock/qcom,gcc-ipq5018.h > + - dt-bindings/reset/qcom,gcc-ipq5018.h > - dt-bindings/clock/qcom,gcc-ipq6018.h > - dt-bindings/reset/qcom,gcc-ipq6018.h > - dt-bindings/clock/qcom,gcc-ipq806x.h (qcom,gcc-ipq8064) > @@ -39,6 +41,7 @@ properties: > enum: > - qcom,gcc-apq8084 > - qcom,gcc-ipq4019 > + - qcom,gcc-ipq5018 > - qcom,gcc-ipq6018 > - qcom,gcc-ipq8064 > - qcom,gcc-msm8660 > diff --git a/include/dt-bindings/clock/qcom,gcc-ipq5018.h b/include/dt-bindings/clock/qcom,gcc-ipq5018.h > new file mode 100644 > index 00000000..069165f > --- /dev/null > +++ b/include/dt-bindings/clock/qcom,gcc-ipq5018.h > @@ -0,0 +1,183 @@ > +/* SPDX-License-Identifier: GPL-2.0+ */ Only care about Linux and GPL OSs? And your employer is okay with GPL3 (and GPL4, ...)? IOW, dual license please.
Quoting Varadarajan Narayanan (2020-09-27 22:15:36) > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig > index 0583273..d1a2504 100644 > --- a/drivers/clk/qcom/Kconfig > +++ b/drivers/clk/qcom/Kconfig > @@ -155,6 +155,14 @@ config IPQ_GCC_8074 > i2c, USB, SD/eMMC, etc. Select this for the root clock > of ipq8074. > > +config IPQ_GCC_5018 > + tristate "IPQ5018 Global Clock Controller" > + help > + Support for global clock controller on ipq5018 devices. > + Say Y if you want to use peripheral devices such as UART, SPI, > + i2c, USB, SD/eMMC, etc. Select this for the root clock > + of ipq5018. What is the root clock of ipq5018? Please drop that last sentence. > + > config MSM_GCC_8660 > tristate "MSM8660 Global Clock Controller" > help > diff --git a/drivers/clk/qcom/gcc-ipq5018.c b/drivers/clk/qcom/gcc-ipq5018.c > new file mode 100644 > index 00000000..9056386 > --- /dev/null > +++ b/drivers/clk/qcom/gcc-ipq5018.c > @@ -0,0 +1,3833 @@ > +// SPDX-License-Identifier: GPL-2.0+ > + > +#include <linux/kernel.h> > +#include <linux/err.h> > +#include <linux/platform_device.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/clk-provider.h> > +#include <linux/regmap.h> > + > +#include <linux/reset-controller.h> Why is this attached to dt-bindings? Please remove that newline above and move this away from dt-bindings below. > +#include <dt-bindings/clock/qcom,gcc-ipq5018.h> > +#include <dt-bindings/reset/qcom,gcc-ipq5018.h> > + > +#include "common.h" > +#include "clk-regmap.h" > +#include "clk-pll.h" > +#include "clk-rcg.h" > +#include "clk-branch.h" > +#include "clk-alpha-pll.h" > +#include "clk-regmap-divider.h" > +#include "clk-regmap-mux.h" > +#include "reset.h" > + > +#define F(f, s, h, m, n) { (f), (s), (2 * (h) - 1), (m), (n) } This is in clk-rcg.h already. > + > +static const char * const gcc_usb3phy_0_cc_pipe_clk_xo[] = { > + "usb3phy_0_cc_pipe_clk", > + "xo", > +}; All these names structures need to change, see next comment. > + > +static struct clk_rcg2 apss_ahb_clk_src = { > + .cmd_rcgr = 0x46000, > + .mnd_width = 0, > + .hid_width = 5, > + .freq_tbl = ftbl_apss_ahb_clk_src, > + .parent_map = gcc_xo_gpll0_gpll0_out_main_div2_map, > + .clkr.hw.init = &(struct clk_init_data){ > + .name = "apss_ahb_clk_src", > + .parent_names = gcc_xo_gpll0_gpll0_out_main_div2, > + .num_parents = 3, Please migrate to the new way of specifying clks with clk_init_data::clk_parent_data > + .ops = &clk_rcg2_ops, > + .flags = CLK_IS_CRITICAL | CLK_IGNORE_UNUSED, Why is it critical and ignore unused? Do you need this clk to be here at all? Can we just enable it when this driver probes with a register write and then ignore it from there on out? > + }, > +}; > + > +static struct clk_regmap_div apss_ahb_postdiv_clk_src = { > + .reg = 0x46018, > + .shift = 4, > + .width = 4, > + .clkr = { > + .hw.init = &(struct clk_init_data){ > + .name = "apss_ahb_postdiv_clk_src", > + .parent_names = (const char *[]){ > + "apss_ahb_clk_src" > + }, > + .num_parents = 1, > + .ops = &clk_regmap_div_ops, > + }, > + }, > +}; > + [...] > + > +static struct clk_branch gcc_qdss_dap_clk = { > + .halt_reg = 0x29084, > + .clkr = { > + .enable_reg = 0x29084, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_qdss_dap_clk", > + .parent_names = (const char *[]){ > + "qdss_tsctr_clk_src" > + }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL, Whenever CLK_IS_CRITICAL is there please document why it is needed. And if possible remove the clk structure and hit the clk on in driver probe so we don't waste memory modeling something that never matters. Typically that can only be done if nothing references this clk as a parent or if we're willing to break the clk tree and ignore describing parents. In this case it's a branch so probably nothing else is under it so we can just turn it on during probe and stop caring. > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_qdss_cfg_ahb_clk = { > + .halt_reg = 0x29008, > + .clkr = { > + .enable_reg = 0x29008, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_qdss_cfg_ahb_clk", > + .parent_names = (const char *[]){ > + "pcnoc_clk_src" > + }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT, > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + [...] > + > +static struct clk_branch gcc_qdss_stm_clk = { > + .halt_reg = 0x29044, > + .clkr = { > + .enable_reg = 0x29044, > + .enable_mask = BIT(0), > + .hw.init = &(struct clk_init_data){ > + .name = "gcc_qdss_stm_clk", > + .parent_names = (const char *[]){ > + "qdss_stm_clk_src" > + }, > + .num_parents = 1, > + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED, Why ignore unused? Probably should just be turned on somewhere else? > + .ops = &clk_branch2_ops, > + }, > + }, > +}; > + > +static struct clk_branch gcc_qdss_traceclkin_clk = { > + .halt_reg = 0x29060, > + .clkr = { > + .enable_reg = 0x29060, [...] > + > +static int gcc_ipq5018_probe(struct platform_device *pdev) > +{ > + int i, ret; > + struct regmap *regmap; > + struct clk *clk; > + struct qcom_cc_desc ipq5018_desc = gcc_ipq5018_desc; > + > + regmap = qcom_cc_map(pdev, &ipq5018_desc); > + if (IS_ERR(regmap)) > + return PTR_ERR(regmap); > + > + for (i = 0; i < ARRAY_SIZE(gcc_ipq5018_hws); i++) { > + clk = devm_clk_register(&pdev->dev, gcc_ipq5018_hws[i]); > + if (IS_ERR(clk)) > + return PTR_ERR(clk); > + } We really need to move this into the qcom_cc_desc so it is part of qcom_cc_really_probe() > + /*Gen2 PHY*/ > + clk_register_fixed_rate(&pdev->dev, "pcie20_phy0_pipe_clk", NULL, > + CLK_IS_ROOT, 125000000); > + clk_register_fixed_rate(&pdev->dev, "pcie20_phy1_pipe_clk", NULL, > + CLK_IS_ROOT, 125000000); These should be coming from some pcie phy and part of the DT binding as a 'clocks' element that this device consumes. > + > + clk_alpha_pll_configure(&ubi32_pll_main, regmap, &ubi32_pll_config); > + > + ret = qcom_cc_really_probe(pdev, &ipq5018_desc, regmap); > + if (ret) { > + dev_err(&pdev->dev, "Failed to register ipq5018 GCC clocks\n"); > + return ret; > + } > + > + dev_info(&pdev->dev, "Registered ipq5018 GCC clocks provider"); Please drop this noise. > + > + return ret; > +} > + > +static int gcc_ipq5018_remove(struct platform_device *pdev) > +{ > + return 0; > +} > + If there isn't anything in the remove function it can be omitted. > +static struct platform_driver gcc_ipq5018_driver = { > + .probe = gcc_ipq5018_probe, > + .remove = gcc_ipq5018_remove, > + .driver = { > + .name = "qcom,gcc-ipq5018", > + .owner = THIS_MODULE, > + .of_match_table = gcc_ipq5018_match_table, > + }, > +}; > + > +static int __init gcc_ipq5018_init(void) > +{ > + return platform_driver_register(&gcc_ipq5018_driver); > +} > +core_initcall(gcc_ipq5018_init); > + > +static void __exit gcc_ipq5018_exit(void) > +{ > + platform_driver_unregister(&gcc_ipq5018_driver); > +} > +module_exit(gcc_ipq5018_exit); > + > +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. GCC IPQ5018 Driver"); > +MODULE_LICENSE("GPL v2"); > +MODULE_ALIAS("platform:gcc-ipq5018"); I think alias isn't needed anymore. > diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h > index 03a5de5..31fde45 100644 > --- a/include/linux/clk-provider.h > +++ b/include/linux/clk-provider.h > @@ -20,8 +20,8 @@ > #define CLK_SET_PARENT_GATE BIT(1) /* must be gated across re-parent */ > #define CLK_SET_RATE_PARENT BIT(2) /* propagate rate change up one level */ > #define CLK_IGNORE_UNUSED BIT(3) /* do not gate even if unused */ > - /* unused */ > - /* unused */ > +#define CLK_IS_ROOT BIT(4) /* root clk, has no parent */ > +#define CLK_IS_BASIC BIT(5) /* Basic clk, can't do a to_clk_foo() */ Please no. Drop this hunk. > #define CLK_GET_RATE_NOCACHE BIT(6) /* do not use the cached clk rate */ > #define CLK_SET_RATE_NO_REPARENT BIT(7) /* don't re-parent on rate change */ > #define CLK_GET_ACCURACY_NOCACHE BIT(8) /* do not use the cached clk accuracy */
Can you check your get_maintainers script invocation? Not sure why arm64 maintainers are Cced on a clk patch. Quoting Varadarajan Narayanan (2020-09-27 22:15:34) > Add programming sequence support for managing the Stromer > PLLs. > > Signed-off-by: Varadarajan Narayanan <varada@codeaurora.org> > --- > drivers/clk/qcom/clk-alpha-pll.c | 156 ++++++++++++++++++++++++++++++++++++++- > drivers/clk/qcom/clk-alpha-pll.h | 5 ++ > 2 files changed, 160 insertions(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/clk-alpha-pll.c b/drivers/clk/qcom/clk-alpha-pll.c > index 26139ef..ce3257f 100644 > --- a/drivers/clk/qcom/clk-alpha-pll.c > +++ b/drivers/clk/qcom/clk-alpha-pll.c > @@ -116,6 +116,19 @@ const u8 clk_alpha_pll_regs[][PLL_OFF_MAX_REGS] = { > [PLL_OFF_OPMODE] = 0x38, > [PLL_OFF_ALPHA_VAL] = 0x40, > }, > + Nitpick: Drop this newline. > + [CLK_ALPHA_PLL_TYPE_STROMER] = { > + [PLL_OFF_L_VAL] = 0x08, > + [PLL_OFF_ALPHA_VAL] = 0x10, > + [PLL_OFF_ALPHA_VAL_U] = 0x14, > + [PLL_OFF_USER_CTL] = 0x18, > + [PLL_OFF_USER_CTL_U] = 0x1c, > + [PLL_OFF_CONFIG_CTL] = 0x20, > + [PLL_OFF_CONFIG_CTL_U] = 0xff, > + [PLL_OFF_TEST_CTL] = 0x30, > + [PLL_OFF_TEST_CTL_U] = 0x34, > + [PLL_OFF_STATUS] = 0x28, > + }, > }; > EXPORT_SYMBOL_GPL(clk_alpha_pll_regs); > > @@ -127,6 +140,8 @@ EXPORT_SYMBOL_GPL(clk_alpha_pll_regs); > #define ALPHA_BITWIDTH 32U > #define ALPHA_SHIFT(w) min(w, ALPHA_BITWIDTH) > > +#define PLL_STATUS_REG_SHIFT 8 This should have an ALPHA_ prefix. > + > #define PLL_HUAYRA_M_WIDTH 8 > #define PLL_HUAYRA_M_SHIFT 8 > #define PLL_HUAYRA_M_MASK 0xff > @@ -240,14 +255,143 @@ void clk_alpha_pll_configure(struct clk_alpha_pll *pll, struct regmap *regmap, > mask |= config->pre_div_mask; > mask |= config->post_div_mask; > mask |= config->vco_mask; > + mask |= config->alpha_en_mask; > + mask |= config->alpha_mode_mask; > > regmap_update_bits(regmap, PLL_USER_CTL(pll), mask, val); > > + /* Stromer APSS PLL does not enable LOCK_DET by default, so enable it */ > + val_u = config->status_reg_val << PLL_STATUS_REG_SHIFT; > + val_u |= config->lock_det; > + > + mask_u = config->status_reg_mask; > + mask_u |= config->lock_det; > + > + if (val_u != 0) if (val_u) is more canonical. > + regmap_update_bits(regmap, PLL_USER_CTL_U(pll), mask_u, val_u); > + > + if (config->test_ctl_val != 0) Same comment > + regmap_write(regmap, PLL_TEST_CTL(pll), config->test_ctl_val); > + > + if (config->test_ctl_hi_val != 0) Same comment > + regmap_write(regmap, PLL_TEST_CTL_U(pll), config->test_ctl_hi_val); > + > if (pll->flags & SUPPORTS_FSM_MODE) > qcom_pll_set_fsm_mode(regmap, PLL_MODE(pll), 6, 0); > } > EXPORT_SYMBOL_GPL(clk_alpha_pll_configure); > > +static unsigned long > +alpha_pll_stromer_calc_rate(u64 prate, u32 l, u64 a) > +{ > + return (prate * l) + ((prate * a) >> ALPHA_REG_BITWIDTH); Is this not already in this file? Why can't we use alpha_pll_calc_rate()? > +} > + > +static unsigned long > +alpha_pll_stromer_round_rate(unsigned long rate, unsigned long prate, u32 *l, u64 *a) > +{ > + u64 remainder; > + u64 quotient; > + > + quotient = rate; > + remainder = do_div(quotient, prate); > + *l = quotient; > + > + if (!remainder) { > + *a = 0; > + return rate; > + } > + > + quotient = remainder << ALPHA_REG_BITWIDTH; > + > + remainder = do_div(quotient, prate); > + > + if (remainder) > + quotient++; > + > + *a = quotient; > + return alpha_pll_stromer_calc_rate(prate, *l, *a); > +} > + > +static unsigned long > +clk_alpha_pll_stromer_recalc_rate(struct clk_hw *hw, unsigned long parent_rate) > +{ > + u32 l, low, high, ctl; > + u64 a = 0, prate = parent_rate; > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > + > + regmap_read(pll->clkr.regmap, PLL_L_VAL(pll), &l); > + > + regmap_read(pll->clkr.regmap, PLL_USER_CTL(pll), &ctl); > + if (ctl & PLL_ALPHA_EN) { > + regmap_read(pll->clkr.regmap, PLL_ALPHA_VAL(pll), &low); > + regmap_read(pll->clkr.regmap, PLL_ALPHA_VAL_U(pll), > + &high); > + a = (u64)high << ALPHA_BITWIDTH | low; > + } > + > + return alpha_pll_stromer_calc_rate(prate, l, a); > +} > + > +static int clk_alpha_pll_stromer_determine_rate(struct clk_hw *hw, > + struct clk_rate_request *req) > +{ > + unsigned long rate = req->rate; > + u32 l; > + u64 a; > + > + rate = alpha_pll_stromer_round_rate(rate, req->best_parent_rate, &l, &a); Why assign to rate if nobody is going to look at it? Should probably be set to req->rate instead? > + > + return 0; > +} > + > +static int clk_alpha_pll_stromer_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long prate) > +{ > + struct clk_alpha_pll *pll = to_clk_alpha_pll(hw); > + u32 l; > + int ret; > + u64 a; > + > + rate = alpha_pll_stromer_round_rate(rate, prate, &l, &a); > + > + /* Write desired values to registers */ Please drop this useless comment. > + regmap_write(pll->clkr.regmap, PLL_L_VAL(pll), l); > + regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL(pll), a); > + regmap_write(pll->clkr.regmap, PLL_ALPHA_VAL_U(pll), > + a >> ALPHA_BITWIDTH); > + > + regmap_update_bits(pll->clkr.regmap, PLL_USER_CTL(pll), > + PLL_ALPHA_EN, PLL_ALPHA_EN); > + > + if (!clk_hw_is_enabled(hw)) > + return 0; > + > + /* Stromer PLL supports Dynamic programming. The /* goes on a line by itself. > + * It allows the PLL frequency to be changed on-the-fly without first > + * execution of a shutdown procedure followed by a bring up procedure. > + */ Cool feature. Maybe that can go into the header file though? > + > + regmap_update_bits(pll->clkr.regmap, PLL_MODE(pll), PLL_UPDATE, > + PLL_UPDATE); > + /* Make sure PLL_UPDATE request goes through */ > + mb(); regmap APIs already have memory barriers so this isn't needed? > + > + /* Wait for PLL_UPDATE to be cleared */ I think the code already says this so we can just drop this comment. > + ret = wait_for_pll_update(pll); > + if (ret) > + return ret; > + > + /* Wait 11or more PLL clk_ref ticks[to be explored more on wait] */ > + Is this a TODO? > + /* Poll LOCK_DET for one */ I think the code already says this so we can just drop this comment. > + ret = wait_for_pll_enable_lock(pll); > + if (ret) > + return ret; > + > + return 0; > +} > + > static int clk_alpha_pll_hwfsm_enable(struct clk_hw *hw) > { > int ret;
The IPQ5018 is Qualcomm's 802.11ax SoC for Routers, Gateways and Access Points. This series adds minimal board boot support for ipq5018-mp03.1-c2 board. Varadarajan Narayanan (7): clk: qcom: clk-alpha-pll: Add support for Stromer PLLs dt-bindings: arm64: ipq5018: Add binding descriptions for clock and reset clk: qcom: Add Global Clock controller (GCC) driver for IPQ5018 dt-bindings: pinctrl: qcom: Add ipq5018 pinctrl bindings pinctrl: qcom: Add IPQ5018 pinctrl driver arm64: dts: Add ipq5018 SoC and MP03 board support arm64: defconfig: Enable IPQ5018 SoC base configs Documentation/devicetree/bindings/arm/qcom.yaml | 7 + .../devicetree/bindings/clock/qcom,gcc.yaml | 3 + .../bindings/pinctrl/qcom,ipq5018-pinctrl.yaml | 143 + arch/arm64/boot/dts/qcom/Makefile | 1 + arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts | 30 + arch/arm64/boot/dts/qcom/ipq5018.dtsi | 201 + arch/arm64/configs/defconfig | 3 + drivers/clk/qcom/Kconfig | 8 + drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-alpha-pll.c | 156 +- drivers/clk/qcom/clk-alpha-pll.h | 5 + drivers/clk/qcom/gcc-ipq5018.c | 3833 ++++++++++++++++++++ drivers/pinctrl/qcom/Kconfig | 10 + drivers/pinctrl/qcom/Makefile | 1 + drivers/pinctrl/qcom/pinctrl-ipq5018.c | 903 +++++ include/dt-bindings/clock/qcom,gcc-ipq5018.h | 183 + include/dt-bindings/reset/qcom,gcc-ipq5018.h | 119 + include/linux/clk-provider.h | 4 +- 18 files changed, 5608 insertions(+), 3 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,ipq5018-pinctrl.yaml create mode 100644 arch/arm64/boot/dts/qcom/ipq5018-mp03.1-c2.dts create mode 100644 arch/arm64/boot/dts/qcom/ipq5018.dtsi create mode 100644 drivers/clk/qcom/gcc-ipq5018.c create mode 100644 drivers/pinctrl/qcom/pinctrl-ipq5018.c create mode 100644 include/dt-bindings/clock/qcom,gcc-ipq5018.h create mode 100644 include/dt-bindings/reset/qcom,gcc-ipq5018.h