linux-kernel - Re: [PATCH v3 3/5] clk: qcom: Add QDU1000 and QRU1000 GCC support

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: <20221107173237.xkeigtihoes3vsux@builder.lan>
Date:   Mon, 7 Nov 2022 11:32:37 -0600
From:   Bjorn Andersson <andersson@...nel.org>
To:     Melody Olvera <quic_molvera@...cinc.com>
Cc:     Andy Gross <agross@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Taniya Das <quic_tdas@...cinc.com>,
        linux-arm-msm@...r.kernel.org, linux-clk@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/5] clk: qcom: Add QDU1000 and QRU1000 GCC support

On Wed, Oct 26, 2022 at 12:04:39PM -0700, Melody Olvera wrote:
> diff --git a/drivers/clk/qcom/gcc-qdu1000.c b/drivers/clk/qcom/gcc-qdu1000.c
> new file mode 100644
> index 000000000000..7bd8ebf0ddb5
> --- /dev/null
> +++ b/drivers/clk/qcom/gcc-qdu1000.c
> @@ -0,0 +1,2645 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +#include <dt-bindings/clock/qcom,gcc-qdu1000.h>
> +
> +#include "clk-alpha-pll.h"
> +#include "clk-branch.h"
> +#include "clk-rcg.h"
> +#include "clk-regmap.h"
> +#include "clk-regmap-divider.h"
> +#include "clk-regmap-mux.h"
> +#include "clk-regmap-phy-mux.h"
> +#include "reset.h"
> +
> +enum {
> +	P_BI_TCXO,
> +	P_GCC_GPLL0_OUT_EVEN,
> +	P_GCC_GPLL0_OUT_MAIN,
> +	P_GCC_GPLL1_OUT_MAIN,
> +	P_GCC_GPLL2_OUT_MAIN,
> +	P_GCC_GPLL3_OUT_MAIN,
> +	P_GCC_GPLL4_OUT_MAIN,
> +	P_GCC_GPLL5_OUT_MAIN,
> +	P_GCC_GPLL6_OUT_MAIN,
> +	P_GCC_GPLL7_OUT_MAIN,
> +	P_GCC_GPLL8_OUT_MAIN,
> +	P_PCIE_0_PHY_AUX_CLK,
> +	P_PCIE_0_PIPE_CLK,
> +	P_SLEEP_CLK,
> +	P_USB3_PHY_WRAPPER_GCC_USB30_PIPE_CLK,
> +};
[..]
> +static const struct clk_parent_data gcc_parent_data_1[] = {
> +	{ .index = P_BI_TCXO },
> +	{ .hw = &gcc_gpll0.clkr.hw },
> +	{ .index = P_SLEEP_CLK },

.index here refers to the index in the clocks property in DT.

I think it's okay to reuse the parent-enum, but the entries within must
then match the order defined in the DT binding. So you need to ensure
that the first N entires in the enum matches the binding.

Perhaps it's cleaner to just carry a separate enum for the clocks order,
as we've done in the other drivers?

If nothing else it makes it clear that one number space is arbitrary and
internal to the driver and the other is ABI.

> +	{ .hw = &gcc_gpll0_out_even.clkr.hw },
> +};
> +
[..]
> +static struct clk_regmap_mux gcc_pcie_0_phy_aux_clk_src = {
> +	.reg = 0x9d080,
> +	.shift = 0,
> +	.width = 2,
> +	.parent_map = gcc_parent_map_6,
> +	.clkr = {
> +		.hw.init = &(const struct clk_init_data){

Sorry for being picky, but I do like when there's a space between the
')' and '{' in these lines...

> +			.name = "gcc_pcie_0_phy_aux_clk_src",
> +			.parent_data = gcc_parent_data_6,
> +			.num_parents = ARRAY_SIZE(gcc_parent_data_6),
> +			.ops = &clk_regmap_mux_closest_ops,
> +		},
> +	},
> +};

Regards,
Bjorn