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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ