| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d7b374670eb2f6d442f351106ab1221a.sboyd@kernel.org>
Date: Tue, 27 Aug 2024 16:50:03 -0700
From: Stephen Boyd <sboyd@...nel.org>
To: Bjorn Andersson <andersson@...nel.org>, Catalin Marinas <catalin.marinas@....com>, Conor Dooley <conor+dt@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Luo Jie <quic_luoj@...cinc.com>, Michael Turquette <mturquette@...libre.com>, Rob Herring <robh@...nel.org>, Will Deacon <will@...nel.org>
Cc: linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, quic_kkumarcs@...cinc.com, quic_suruchia@...cinc.com, quic_pavir@...cinc.com, quic_linchen@...cinc.com, quic_leiwei@...cinc.com, bartosz.golaszewski@...aro.org, srinivas.kandagatla@...aro.org, Luo Jie <quic_luoj@...cinc.com>
Subject: Re: [PATCH v3 2/4] clk: qcom: Add CMN PLL clock controller driver for IPQ SoC
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.
> 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.
> +#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.
> + 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()?
> +
> + 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.
> +{
> + 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");
Usually qcom clk drivers do this with pm_clk_add() and runtime PM. Why
can't that be done here?
> +
> + 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.
> +
> + /* 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?
Powered by blists - more mailing lists