| Message ID | 20240827-qcom_ipq_cmnpll-v3-2-8e009cece8b2@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add CMN PLL clock controller driver for IPQ9574 | expand |
Hi Stephen, Please find below a minor update to my earlier message on clk_ops usage. On 8/28/2024 1:44 PM, Jie Luo wrote: > > > On 8/28/2024 7:50 AM, Stephen Boyd wrote: >> Quoting Luo Jie (2024-08-27 05:46:00) >>> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile >>> index 8a6f0dabd02f..35f656146de7 100644 >>> --- a/drivers/clk/qcom/Makefile >>> +++ b/drivers/clk/qcom/Makefile >>> @@ -29,6 +29,7 @@ obj-$(CONFIG_CLK_X1E80100_TCSRCC) += tcsrcc-x1e80100.o >>> obj-$(CONFIG_CLK_QCM2290_GPUCC) += gpucc-qcm2290.o >>> obj-$(CONFIG_IPQ_APSS_PLL) += apss-ipq-pll.o >>> obj-$(CONFIG_IPQ_APSS_6018) += apss-ipq6018.o >>> +obj-$(CONFIG_IPQ_CMN_PLL) += clk-ipq-cmn-pll.o >> >> I don't see many other filenames with clk- prefix in this directory, so >> probably drop it. > > OK. > >> >>> obj-$(CONFIG_IPQ_GCC_4019) += gcc-ipq4019.o >>> obj-$(CONFIG_IPQ_GCC_5018) += gcc-ipq5018.o >>> obj-$(CONFIG_IPQ_GCC_5332) += gcc-ipq5332.o >>> diff --git a/drivers/clk/qcom/clk-ipq-cmn-pll.c b/drivers/clk/qcom/ >>> clk-ipq-cmn-pll.c >>> new file mode 100644 >>> index 000000000000..a9775c39b2f3 >>> --- /dev/null >>> +++ b/drivers/clk/qcom/clk-ipq-cmn-pll.c >>> @@ -0,0 +1,241 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/* >>> + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights >>> reserved. >>> + */ >>> + >>> +/* >>> + * CMN PLL block expects the reference clock from on-board Wi-Fi >>> block, and >>> + * supplies fixed rate clocks as output to the Ethernet hardware >>> blocks. >>> + * The Ethernet related blocks include PPE (packet process engine) >>> and the >>> + * external connected PHY (or switch) chip receiving clocks from the >>> CMN PLL. >>> + * >>> + * On the IPQ9574 SoC, There are three clocks with 50 MHZ, one clock >>> with >>> + * 25 MHZ which are output from the CMN PLL to Ethernet PHY (or >>> switch), >>> + * and one clock with 353 MHZ to PPE. >>> + * >>> + * +---------+ >>> + * | GCC | >>> + * +--+---+--+ >>> + * AHB CLK| |SYS CLK >>> + * V V >>> + * +-------+---+------+ >>> + * | +-------------> eth0-50mhz >>> + * REF CLK | IPQ9574 | >>> + * -------->+ +-------------> eth1-50mhz >>> + * | CMN PLL block | >>> + * | +-------------> eth2-50mhz >>> + * | | >>> + * +---------+--------+-------------> eth-25mhz >>> + * | >>> + * V >>> + * ppe-353mhz >>> + */ >>> + >>> +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h> >> >> Include dt-bindings after linux please. > > OK, will update. > >> >>> +#include <linux/bitfield.h> >>> +#include <linux/clk.h> >>> +#include <linux/clk-provider.h> >>> +#include <linux/delay.h> >>> +#include <linux/io.h> >>> +#include <linux/iopoll.h> >>> +#include <linux/of.h> >>> +#include <linux/of_address.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/slab.h> >>> + >>> +#define CMN_PLL_REFCLK_SRC_SELECTION 0x28 >>> +#define CMN_PLL_REFCLK_SRC_DIV GENMASK(9, 8) >>> + >>> +#define CMN_PLL_LOCKED 0x64 >>> +#define CMN_PLL_CLKS_LOCKED BIT(8) >>> + >>> +#define CMN_PLL_POWER_ON_AND_RESET 0x780 >>> +#define CMN_ANA_EN_SW_RSTN BIT(6) >>> + >>> +#define CMN_PLL_REFCLK_CONFIG 0x784 >>> +#define CMN_PLL_REFCLK_EXTERNAL BIT(9) >>> +#define CMN_PLL_REFCLK_DIV GENMASK(8, 4) >>> +#define CMN_PLL_REFCLK_INDEX GENMASK(3, 0) >>> + >>> +#define CMN_PLL_CTRL 0x78c >>> +#define CMN_PLL_CTRL_LOCK_DETECT_EN BIT(15) >>> + >>> +/** >>> + * struct cmn_pll_fixed_output_clk - CMN PLL output clocks information >>> + * @id: Clock specifier to be supplied >>> + * @name: Clock name to be registered >>> + * @rate: Clock rate >>> + */ >>> +struct cmn_pll_fixed_output_clk { >>> + unsigned int id; >>> + const char *name; >>> + const unsigned long rate; >>> +}; >>> + >>> +#define CLK_PLL_OUTPUT(_id, _name, _rate) { \ >>> + .id = _id, \ >>> + .name = _name, \ >>> + .rate = _rate, \ >>> +} >>> + >>> +static const struct cmn_pll_fixed_output_clk ipq9574_output_clks[] = { >>> + CLK_PLL_OUTPUT(PPE_353MHZ_CLK, "ppe-353mhz", 353000000UL), >>> + CLK_PLL_OUTPUT(ETH0_50MHZ_CLK, "eth0-50mhz", 50000000UL), >>> + CLK_PLL_OUTPUT(ETH1_50MHZ_CLK, "eth1-50mhz", 50000000UL), >>> + CLK_PLL_OUTPUT(ETH2_50MHZ_CLK, "eth2-50mhz", 50000000UL), >>> + CLK_PLL_OUTPUT(ETH_25MHZ_CLK, "eth-25mhz", 25000000UL), >>> +}; >>> + >>> +static int ipq_cmn_pll_config(struct device *dev, unsigned long >>> parent_rate) >>> +{ >>> + void __iomem *base; >>> + u32 val; >>> + >>> + base = devm_of_iomap(dev, dev->of_node, 0, NULL); >> >> Use platform_device APIs please. This is a platform driver. > > OK. Will update to use API devm_platform_ioremap_resource(). > >> >>> + if (IS_ERR(base)) >>> + return PTR_ERR(base); >>> + >>> + val = readl(base + CMN_PLL_REFCLK_CONFIG); >>> + val &= ~(CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_INDEX); >>> + >>> + /* >>> + * Configure the reference input clock selection as per the >>> given rate. >>> + * The output clock rates are always of fixed value. >>> + */ >>> + switch (parent_rate) { >>> + case 25000000: >>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3); >>> + break; >>> + case 31250000: >>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 4); >>> + break; >>> + case 40000000: >>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 6); >>> + break; >>> + case 48000000: >>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); >>> + break; >>> + case 50000000: >>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8); >>> + break; >>> + case 96000000: >>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); >>> + val &= ~CMN_PLL_REFCLK_DIV; >>> + val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2); >>> + break; >>> + default: >>> + return -EINVAL; >>> + } >> >> Why isn't this done with struct clk_ops::set_rate() or clk_ops::init()? > > OK, I will move this code into the clk_ops::init(). This code is expected to be executed once for initializing the CMN PLL to enable output clocks, and requires the parent clock rate to be available. However the parent clock rate is not available in the clk_ops::init(). Hence clk_ops::set_rate() seems to be the right option for this. Please let us know if this approach is fine. Thanks. > >> >>> + >>> + writel(val, base + CMN_PLL_REFCLK_CONFIG); >>> + >>> + /* Update the source clock rate selection. Only 96 MHZ uses >>> 0. */ >>> + val = readl(base + CMN_PLL_REFCLK_SRC_SELECTION); >>> + val &= ~CMN_PLL_REFCLK_SRC_DIV; >>> + if (parent_rate != 96000000) >>> + val |= FIELD_PREP(CMN_PLL_REFCLK_SRC_DIV, 1); >>> + >>> + writel(val, base + CMN_PLL_REFCLK_SRC_SELECTION); >>> + >>> + /* Enable PLL locked detect. */ >>> + val = readl(base + CMN_PLL_CTRL); >>> + val |= CMN_PLL_CTRL_LOCK_DETECT_EN; >>> + writel(val, base + CMN_PLL_CTRL); >>> + >>> + /* >>> + * Reset the CMN PLL block to ensure the updated configurations >>> + * take effect. >>> + */ >>> + val = readl(base + CMN_PLL_POWER_ON_AND_RESET); >>> + val &= ~CMN_ANA_EN_SW_RSTN; >>> + writel(val, base + CMN_PLL_POWER_ON_AND_RESET); >>> + usleep_range(1000, 1200); >>> + >>> + val |= CMN_ANA_EN_SW_RSTN; >>> + writel(val, base + CMN_PLL_POWER_ON_AND_RESET); >>> + >>> + /* Stability check of CMN PLL output clocks. */ >>> + return readl_poll_timeout(base + CMN_PLL_LOCKED, val, >>> + (val & CMN_PLL_CLKS_LOCKED), >>> + 100, 100000); >>> +} >>> + >>> +static int ipq_cmn_pll_clk_register(struct device *dev, const char >>> *parent) >> >> Please don't use string names to describe topology. > > > OK, I will update to use an instance of 'struct clk_parent_data' to > describe the parent clock. This will be part of clk_hw instance that we > will define now, to represent the PLL clock and its ops (such as .init) > along with the parent. > >> >>> +{ >>> + const struct cmn_pll_fixed_output_clk *fixed_clk; >>> + struct clk_hw_onecell_data *data; >>> + unsigned int num_clks; >>> + struct clk_hw *hw; >>> + + >>> + num_clks = ARRAY_SIZE(ipq9574_output_clks); >>> + fixed_clk = ipq9574_output_clks; >>> + >>> + data = devm_kzalloc(dev, struct_size(data, hws, num_clks), >>> GFP_KERNEL); >>> + if (!data) >>> + return -ENOMEM; >>> + >>> + for (i = 0; i < num_clks; i++) { >>> + hw = devm_clk_hw_register_fixed_rate(dev, >>> fixed_clk[i].name, >>> + parent, 0, >>> + fixed_clk[i].rate); >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + data->hws[fixed_clk[i].id] = hw; >>> + } >>> + data->num = num_clks; >>> + >>> + return devm_of_clk_add_hw_provider(dev, >>> of_clk_hw_onecell_get, data); >>> +} >>> + >>> +static int ipq_cmn_pll_clk_probe(struct platform_device *pdev) >>> +{ >>> + struct device *dev = &pdev->dev; >>> + struct clk *clk; >>> + int ret; >>> + >>> + /* >>> + * To access the CMN PLL registers, the GCC AHB & SYSY clocks >>> + * for CMN PLL block need to be enabled. >>> + */ >>> + clk = devm_clk_get_enabled(dev, "ahb"); >>> + if (IS_ERR(clk)) >>> + return dev_err_probe(dev, PTR_ERR(clk), >>> + "Enable AHB clock failed\n"); >>> + >>> + clk = devm_clk_get_enabled(dev, "sys"); >>> + if (IS_ERR(clk)) >>> + return dev_err_probe(dev, PTR_ERR(clk), >>> + "Enable SYS clock failed\n"); >> >> Usually qcom clk drivers do this with pm_clk_add() and runtime PM. Why >> can't that be done here? > > Yes, the pm_clk_add() can be used to manage clocks, I will udpate to use > the PM framework. Thanks for the suggestion. > >> >>> + >>> + clk = devm_clk_get(dev, "ref"); >>> + if (IS_ERR(clk)) >>> + return dev_err_probe(dev, PTR_ERR(clk), >>> + "Get reference clock failed\n"); >> >> We don't want clk providers to be clk consumers. Presumably this is the >> PLL's parent clk, and so the frequency should be passed to the clk_ops >> via the parent rate. > > Yes, this is the PLL's parent clock. OK, I will remove this code and > update to use clk_parent_data to describe this parent clock. > >> >>> + >>> + /* Configure CMN PLL to apply the reference clock. */ >>> + ret = ipq_cmn_pll_config(dev, clk_get_rate(clk)); >>> + if (ret) >>> + return dev_err_probe(dev, ret, "Configure CMN PLL >>> failed\n"); >>> + >>> + return ipq_cmn_pll_clk_register(dev, __clk_get_name(clk)); >>> +} >>> + >>> +static const struct of_device_id ipq_cmn_pll_clk_ids[] = { >>> + { .compatible = "qcom,ipq9574-cmn-pll", }, >>> + { } >>> +}; >> >> module device table? >> > I will add the MODULE_DEVICE_TABLE. > >
On 8/31/2024 6:24 AM, Stephen Boyd wrote: > Quoting Jie Luo (2024-08-30 09:14:28) >> Hi Stephen, >> Please find below a minor update to my earlier message on clk_ops usage. > > Ok. Next time you can trim the reply to save me time. OK. > >> On 8/28/2024 1:44 PM, Jie Luo wrote: >>> On 8/28/2024 7:50 AM, Stephen Boyd wrote: >>>> Quoting Luo Jie (2024-08-27 05:46:00) >>>>> + case 48000000: >>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); >>>>> + break; >>>>> + case 50000000: >>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8); >>>>> + break; >>>>> + case 96000000: >>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); >>>>> + val &= ~CMN_PLL_REFCLK_DIV; >>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2); >>>>> + break; >>>>> + default: >>>>> + return -EINVAL; >>>>> + } >>>> >>>> Why isn't this done with struct clk_ops::set_rate() or clk_ops::init()? >>> >>> OK, I will move this code into the clk_ops::init(). >> >> This code is expected to be executed once for initializing the CMN PLL >> to enable output clocks, and requires the parent clock rate to be >> available. However the parent clock rate is not available in the >> clk_ops::init(). Hence clk_ops::set_rate() seems to be the right option >> for this. Please let us know if this approach is fine. Thanks. > > Sure. It actually sounds like the PLL has a mux to select different > reference clks. Is that right? If so, it seems like there should be > multiple 'clocks' for the DT property and many parents possible. If > that's the case then it should be possible to have something like > > clocks = <0>, <&refclk>, <0>; > > in the DT node and then have clk_set_rate() from the consumer actually > set the parent index in hardware. If that's all static then it can be > done with assigned-clock-parents or assigned-clock-rates. Thanks Stephen. The CMN PLL block always uses a single input reference clock pin on any given IPQ SoC, however its rate may be different on different IPQ SoC. For example, its rate is 48MHZ on IPQ9574 and 96MHZ on IPQ5018. Your second suggestion seems more apt for this device. I can define the DT property 'assigned-clock-parents' to configure the clock parent of CMN PLL. The code for reference clock selection will be added in clk_ops::set_parent(). Please let us know if this approach is fine.
On 9/3/2024 2:39 AM, Dmitry Baryshkov wrote: > On Mon, Sep 02, 2024 at 11:33:57PM GMT, Jie Luo wrote: >> >> >> On 8/31/2024 6:24 AM, Stephen Boyd wrote: >>> Quoting Jie Luo (2024-08-30 09:14:28) >>>> Hi Stephen, >>>> Please find below a minor update to my earlier message on clk_ops usage. >>> >>> Ok. Next time you can trim the reply to save me time. >> >> OK. >> >>> >>>> On 8/28/2024 1:44 PM, Jie Luo wrote: >>>>> On 8/28/2024 7:50 AM, Stephen Boyd wrote: >>>>>> Quoting Luo Jie (2024-08-27 05:46:00) >>>>>>> + case 48000000: >>>>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); >>>>>>> + break; >>>>>>> + case 50000000: >>>>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8); >>>>>>> + break; >>>>>>> + case 96000000: >>>>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); >>>>>>> + val &= ~CMN_PLL_REFCLK_DIV; >>>>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2); >>>>>>> + break; >>>>>>> + default: >>>>>>> + return -EINVAL; >>>>>>> + } >>>>>> >>>>>> Why isn't this done with struct clk_ops::set_rate() or clk_ops::init()? >>>>> >>>>> OK, I will move this code into the clk_ops::init(). >>>> >>>> This code is expected to be executed once for initializing the CMN PLL >>>> to enable output clocks, and requires the parent clock rate to be >>>> available. However the parent clock rate is not available in the >>>> clk_ops::init(). Hence clk_ops::set_rate() seems to be the right option >>>> for this. Please let us know if this approach is fine. Thanks. >>> >>> Sure. It actually sounds like the PLL has a mux to select different >>> reference clks. Is that right? If so, it seems like there should be >>> multiple 'clocks' for the DT property and many parents possible. If >>> that's the case then it should be possible to have something like >>> >>> clocks = <0>, <&refclk>, <0>; >>> >>> in the DT node and then have clk_set_rate() from the consumer actually >>> set the parent index in hardware. If that's all static then it can be >>> done with assigned-clock-parents or assigned-clock-rates. >> >> Thanks Stephen. The CMN PLL block always uses a single input reference >> clock pin on any given IPQ SoC, however its rate may be different on >> different IPQ SoC. For example, its rate is 48MHZ on IPQ9574 and 96MHZ >> on IPQ5018. >> >> Your second suggestion seems more apt for this device. I can define the >> DT property 'assigned-clock-parents' to configure the clock parent of >> CMN PLL. The code for reference clock selection will be added in >> clk_ops::set_parent(). Please let us know if this approach is fine. > > What is the source of this clock? Can you call clk_get_rate() on this > input? > The source (parent clock) for CMN PLL is always from on-board Wi-Fi block for any given IPQ SoC. From the discussion so far, it seems there are two approaches possible which I would like to summarize below to be clear. Please let us know if this understanding or approach needs correction. Thanks. 1. clk_get_rate() requires the parent clock instance to be acquired by devm_clk_get(). Per our understanding from Stephen's previous comment, it is preferred that a clock provider driver (this) does not use the _get_ APIs on the parent clock to get the rate. Instead the parent rate should be passed to the clk_ops using parent data. So the parent clock should be specified in the DT using assigned-clock-parents property, and can be accessed from the clk_ops::set_parent(). This seems like a more reasonable method. 2. Alternatively, if it is architecturally acceptable to use devm_clk_get() and clk_get_rate() in this clock provider driver, we can save this parent clock rate into a local driver data structure and then access it from clk_ops::init() for configuring the PLL.
Quoting Dmitry Baryshkov (2024-09-03 07:08:07) > On Tue, 3 Sept 2024 at 17:00, Jie Luo <quic_luoj@quicinc.com> wrote: > > > > > > > > On 9/3/2024 2:39 AM, Dmitry Baryshkov wrote: > > > On Mon, Sep 02, 2024 at 11:33:57PM GMT, Jie Luo wrote: > > >> > > >> > > >> On 8/31/2024 6:24 AM, Stephen Boyd wrote: > > >>> Quoting Jie Luo (2024-08-30 09:14:28) > > >>>> Hi Stephen, > > >>>> Please find below a minor update to my earlier message on clk_ops usage. > > >>> > > >>> Ok. Next time you can trim the reply to save me time. > > >> > > >> OK. > > >> > > >>> > > >>>> On 8/28/2024 1:44 PM, Jie Luo wrote: > > >>>>> On 8/28/2024 7:50 AM, Stephen Boyd wrote: > > >>>>>> Quoting Luo Jie (2024-08-27 05:46:00) > > >>>>>>> + case 48000000: > > >>>>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); > > >>>>>>> + break; > > >>>>>>> + case 50000000: > > >>>>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8); > > >>>>>>> + break; > > >>>>>>> + case 96000000: > > >>>>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); > > >>>>>>> + val &= ~CMN_PLL_REFCLK_DIV; > > >>>>>>> + val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2); > > >>>>>>> + break; > > >>>>>>> + default: > > >>>>>>> + return -EINVAL; > > >>>>>>> + } > > >>>>>> > > >>>>>> Why isn't this done with struct clk_ops::set_rate() or clk_ops::init()? > > >>>>> > > >>>>> OK, I will move this code into the clk_ops::init(). > > >>>> > > >>>> This code is expected to be executed once for initializing the CMN PLL > > >>>> to enable output clocks, and requires the parent clock rate to be > > >>>> available. However the parent clock rate is not available in the > > >>>> clk_ops::init(). Hence clk_ops::set_rate() seems to be the right option > > >>>> for this. Please let us know if this approach is fine. Thanks. > > >>> > > >>> Sure. It actually sounds like the PLL has a mux to select different > > >>> reference clks. Is that right? If so, it seems like there should be > > >>> multiple 'clocks' for the DT property and many parents possible. If > > >>> that's the case then it should be possible to have something like > > >>> > > >>> clocks = <0>, <&refclk>, <0>; > > >>> > > >>> in the DT node and then have clk_set_rate() from the consumer actually > > >>> set the parent index in hardware. If that's all static then it can be > > >>> done with assigned-clock-parents or assigned-clock-rates. > > >> > > >> Thanks Stephen. The CMN PLL block always uses a single input reference > > >> clock pin on any given IPQ SoC, however its rate may be different on > > >> different IPQ SoC. For example, its rate is 48MHZ on IPQ9574 and 96MHZ > > >> on IPQ5018. How many input pins are there on the hardware block? It makes sense that only one pin would be used in practice, but I'm wondering if there are multiple pins in general. Why is the field called CMN_PLL_REFCLK_INDEX if it's not picking the reference clk desired (i.e. the pin that is actually connected)? > > >> > > >> Your second suggestion seems more apt for this device. I can define the > > >> DT property 'assigned-clock-parents' to configure the clock parent of > > >> CMN PLL. The code for reference clock selection will be added in > > >> clk_ops::set_parent(). Please let us know if this approach is fine. > > > > > > What is the source of this clock? Can you call clk_get_rate() on this > > > input? > > > > > > > The source (parent clock) for CMN PLL is always from on-board Wi-Fi > > block for any given IPQ SoC. > > > > From the discussion so far, it seems there are two approaches possible > > which I would like to summarize below to be clear. Please let us know > > if this understanding or approach needs correction. Thanks. > > > > 1. clk_get_rate() requires the parent clock instance to be acquired by > > devm_clk_get(). Per our understanding from Stephen's previous comment, > > it is preferred that a clock provider driver (this) does not use the > > _get_ APIs on the parent clock to get the rate. Instead the parent rate > > should be passed to the clk_ops using parent data. struct clk_parent_data doesn't pass parent rate information to the clk_ops. I'd like you to not use any clk consumer APIs (clk.h) if possible. > So the parent clock > > should be specified in the DT using assigned-clock-parents property, and > > can be accessed from the clk_ops::set_parent(). This seems like a more > > reasonable method. Yes, this makes sense if the clk actually has multiple possible parents. Don't read the rate of the clk in the clk_ops::set_parent() callback though. The callback should only program the hardware to select the parent based on the index passed to the clk_op. If the clk only has one possible parent then it's different. I'd do it through clk_ops::set_rate() and use assigned-clock-rates or just let the first child clk of the PLL set the rate and configure the PLL by having the PLL's determine_rate() callback figure out if the parent rate is valid. That register field with "index" makes me suspicious that this is a mux that we're trying to hide behind the parent rate. Quite possibly that's actually a hardware multiplier, i.e. l-val, and we need to set the index to pick which multiplier is used to achieve whatever frequency is desired for the PLL itself. I assume the 353MHz output clk is actually the one that is deciding what the index should be, and the other ones all fall out of the PLL somewhere else through a post-divider or something. What frequency does the PLL run at? > > assigned-clock-parents is necessary if there are multiple possible > parents. As you wrote that there is just one possible parent, then > there is no need to use it. > Stephen, your opinion? > > > 2. Alternatively, if it is architecturally acceptable to use > > devm_clk_get() and clk_get_rate() in this clock provider driver, we can > > save this parent clock rate into a local driver data structure and then > > access it from clk_ops::init() for configuring the PLL. > No, it isn't acceptable.
diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index cf6ad908327f..05bec64bf1dd 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -190,6 +190,16 @@ config IPQ_APSS_6018 Say Y if you want to support CPU frequency scaling on ipq based devices. +config IPQ_CMN_PLL + tristate "IPQ CMN PLL Clock Controller" + depends on IPQ_GCC_9574 + help + Support for CMN PLL clock controller on IPQ platform. The + CMN PLL feeds the reference clocks to the Ethernet devices + based on IPQ SoC. + Say Y or M if you want to support CMN PLL clock on the IPQ + based devices. + config IPQ_GCC_4019 tristate "IPQ4019 Global Clock Controller" help diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile index 8a6f0dabd02f..35f656146de7 100644 --- a/drivers/clk/qcom/Makefile +++ b/drivers/clk/qcom/Makefile @@ -29,6 +29,7 @@ obj-$(CONFIG_CLK_X1E80100_TCSRCC) += tcsrcc-x1e80100.o obj-$(CONFIG_CLK_QCM2290_GPUCC) += gpucc-qcm2290.o obj-$(CONFIG_IPQ_APSS_PLL) += apss-ipq-pll.o obj-$(CONFIG_IPQ_APSS_6018) += apss-ipq6018.o +obj-$(CONFIG_IPQ_CMN_PLL) += clk-ipq-cmn-pll.o obj-$(CONFIG_IPQ_GCC_4019) += gcc-ipq4019.o obj-$(CONFIG_IPQ_GCC_5018) += gcc-ipq5018.o obj-$(CONFIG_IPQ_GCC_5332) += gcc-ipq5332.o diff --git a/drivers/clk/qcom/clk-ipq-cmn-pll.c b/drivers/clk/qcom/clk-ipq-cmn-pll.c new file mode 100644 index 000000000000..a9775c39b2f3 --- /dev/null +++ b/drivers/clk/qcom/clk-ipq-cmn-pll.c @@ -0,0 +1,241 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. + */ + +/* + * CMN PLL block expects the reference clock from on-board Wi-Fi block, and + * supplies fixed rate clocks as output to the Ethernet hardware blocks. + * The Ethernet related blocks include PPE (packet process engine) and the + * external connected PHY (or switch) chip receiving clocks from the CMN PLL. + * + * On the IPQ9574 SoC, There are three clocks with 50 MHZ, one clock with + * 25 MHZ which are output from the CMN PLL to Ethernet PHY (or switch), + * and one clock with 353 MHZ to PPE. + * + * +---------+ + * | GCC | + * +--+---+--+ + * AHB CLK| |SYS CLK + * V V + * +-------+---+------+ + * | +-------------> eth0-50mhz + * REF CLK | IPQ9574 | + * -------->+ +-------------> eth1-50mhz + * | CMN PLL block | + * | +-------------> eth2-50mhz + * | | + * +---------+--------+-------------> eth-25mhz + * | + * V + * ppe-353mhz + */ + +#include <dt-bindings/clock/qcom,ipq-cmn-pll.h> +#include <linux/bitfield.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/delay.h> +#include <linux/io.h> +#include <linux/iopoll.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +#define CMN_PLL_REFCLK_SRC_SELECTION 0x28 +#define CMN_PLL_REFCLK_SRC_DIV GENMASK(9, 8) + +#define CMN_PLL_LOCKED 0x64 +#define CMN_PLL_CLKS_LOCKED BIT(8) + +#define CMN_PLL_POWER_ON_AND_RESET 0x780 +#define CMN_ANA_EN_SW_RSTN BIT(6) + +#define CMN_PLL_REFCLK_CONFIG 0x784 +#define CMN_PLL_REFCLK_EXTERNAL BIT(9) +#define CMN_PLL_REFCLK_DIV GENMASK(8, 4) +#define CMN_PLL_REFCLK_INDEX GENMASK(3, 0) + +#define CMN_PLL_CTRL 0x78c +#define CMN_PLL_CTRL_LOCK_DETECT_EN BIT(15) + +/** + * struct cmn_pll_fixed_output_clk - CMN PLL output clocks information + * @id: Clock specifier to be supplied + * @name: Clock name to be registered + * @rate: Clock rate + */ +struct cmn_pll_fixed_output_clk { + unsigned int id; + const char *name; + const unsigned long rate; +}; + +#define CLK_PLL_OUTPUT(_id, _name, _rate) { \ + .id = _id, \ + .name = _name, \ + .rate = _rate, \ +} + +static const struct cmn_pll_fixed_output_clk ipq9574_output_clks[] = { + CLK_PLL_OUTPUT(PPE_353MHZ_CLK, "ppe-353mhz", 353000000UL), + CLK_PLL_OUTPUT(ETH0_50MHZ_CLK, "eth0-50mhz", 50000000UL), + CLK_PLL_OUTPUT(ETH1_50MHZ_CLK, "eth1-50mhz", 50000000UL), + CLK_PLL_OUTPUT(ETH2_50MHZ_CLK, "eth2-50mhz", 50000000UL), + CLK_PLL_OUTPUT(ETH_25MHZ_CLK, "eth-25mhz", 25000000UL), +}; + +static int ipq_cmn_pll_config(struct device *dev, unsigned long parent_rate) +{ + void __iomem *base; + u32 val; + + base = devm_of_iomap(dev, dev->of_node, 0, NULL); + if (IS_ERR(base)) + return PTR_ERR(base); + + val = readl(base + CMN_PLL_REFCLK_CONFIG); + val &= ~(CMN_PLL_REFCLK_EXTERNAL | CMN_PLL_REFCLK_INDEX); + + /* + * Configure the reference input clock selection as per the given rate. + * The output clock rates are always of fixed value. + */ + switch (parent_rate) { + case 25000000: + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 3); + break; + case 31250000: + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 4); + break; + case 40000000: + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 6); + break; + case 48000000: + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); + break; + case 50000000: + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 8); + break; + case 96000000: + val |= FIELD_PREP(CMN_PLL_REFCLK_INDEX, 7); + val &= ~CMN_PLL_REFCLK_DIV; + val |= FIELD_PREP(CMN_PLL_REFCLK_DIV, 2); + break; + default: + return -EINVAL; + } + + writel(val, base + CMN_PLL_REFCLK_CONFIG); + + /* Update the source clock rate selection. Only 96 MHZ uses 0. */ + val = readl(base + CMN_PLL_REFCLK_SRC_SELECTION); + val &= ~CMN_PLL_REFCLK_SRC_DIV; + if (parent_rate != 96000000) + val |= FIELD_PREP(CMN_PLL_REFCLK_SRC_DIV, 1); + + writel(val, base + CMN_PLL_REFCLK_SRC_SELECTION); + + /* Enable PLL locked detect. */ + val = readl(base + CMN_PLL_CTRL); + val |= CMN_PLL_CTRL_LOCK_DETECT_EN; + writel(val, base + CMN_PLL_CTRL); + + /* + * Reset the CMN PLL block to ensure the updated configurations + * take effect. + */ + val = readl(base + CMN_PLL_POWER_ON_AND_RESET); + val &= ~CMN_ANA_EN_SW_RSTN; + writel(val, base + CMN_PLL_POWER_ON_AND_RESET); + usleep_range(1000, 1200); + + val |= CMN_ANA_EN_SW_RSTN; + writel(val, base + CMN_PLL_POWER_ON_AND_RESET); + + /* Stability check of CMN PLL output clocks. */ + return readl_poll_timeout(base + CMN_PLL_LOCKED, val, + (val & CMN_PLL_CLKS_LOCKED), + 100, 100000); +} + +static int ipq_cmn_pll_clk_register(struct device *dev, const char *parent) +{ + const struct cmn_pll_fixed_output_clk *fixed_clk; + struct clk_hw_onecell_data *data; + unsigned int num_clks; + struct clk_hw *hw; + int i; + + num_clks = ARRAY_SIZE(ipq9574_output_clks); + fixed_clk = ipq9574_output_clks; + + data = devm_kzalloc(dev, struct_size(data, hws, num_clks), GFP_KERNEL); + if (!data) + return -ENOMEM; + + for (i = 0; i < num_clks; i++) { + hw = devm_clk_hw_register_fixed_rate(dev, fixed_clk[i].name, + parent, 0, + fixed_clk[i].rate); + if (IS_ERR(hw)) + return PTR_ERR(hw); + + data->hws[fixed_clk[i].id] = hw; + } + data->num = num_clks; + + return devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, data); +} + +static int ipq_cmn_pll_clk_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct clk *clk; + int ret; + + /* + * To access the CMN PLL registers, the GCC AHB & SYSY clocks + * for CMN PLL block need to be enabled. + */ + clk = devm_clk_get_enabled(dev, "ahb"); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), + "Enable AHB clock failed\n"); + + clk = devm_clk_get_enabled(dev, "sys"); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), + "Enable SYS clock failed\n"); + + clk = devm_clk_get(dev, "ref"); + if (IS_ERR(clk)) + return dev_err_probe(dev, PTR_ERR(clk), + "Get reference clock failed\n"); + + /* Configure CMN PLL to apply the reference clock. */ + ret = ipq_cmn_pll_config(dev, clk_get_rate(clk)); + if (ret) + return dev_err_probe(dev, ret, "Configure CMN PLL failed\n"); + + return ipq_cmn_pll_clk_register(dev, __clk_get_name(clk)); +} + +static const struct of_device_id ipq_cmn_pll_clk_ids[] = { + { .compatible = "qcom,ipq9574-cmn-pll", }, + { } +}; + +static struct platform_driver ipq_cmn_pll_clk_driver = { + .probe = ipq_cmn_pll_clk_probe, + .driver = { + .name = "ipq_cmn_pll", + .of_match_table = ipq_cmn_pll_clk_ids, + }, +}; + +module_platform_driver(ipq_cmn_pll_clk_driver); + +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. IPQ CMN PLL Driver"); +MODULE_LICENSE("GPL");
The CMN PLL clock controller supplies clocks to the hardware blocks that together make up the Ethernet function on Qualcomm IPQ SoCs. The driver is initially supported for IPQ9574 SoC. The CMN PLL clock controller expects a reference input clock from the on-board Wi-Fi block acting as clock source. The input reference clock needs to be configured to one of the supported clock rates. The controller supplies a number of fixed-rate output clocks. For the IPQ9574, there is one output clock of 353 MHZ to PPE (Packet Process Engine) hardware block, three 50 MHZ output clocks and an additional 25 MHZ output clock supplied to the connected Ethernet devices. Signed-off-by: Luo Jie <quic_luoj@quicinc.com> --- drivers/clk/qcom/Kconfig | 10 ++ drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-ipq-cmn-pll.c | 241 +++++++++++++++++++++++++++++++++++++ 3 files changed, 252 insertions(+)