| 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: <7f4d41a0-b1b9-4b63-8590-63f4fcf1a359@quicinc.com>
Date: Wed, 28 Aug 2024 13:44:12 +0800
From: Jie Luo <quic_luoj@...cinc.com>
To: Stephen Boyd <sboyd@...nel.org>, 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>,
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>
Subject: Re: [PATCH v3 2/4] clk: qcom: Add CMN PLL clock controller driver for
IPQ SoC
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().
>
>> +
>> + 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.
Powered by blists - more mailing lists