Re: [PATCH v4 2/7] clk: qcom: Add generic clkref_en support

[Qiang Yu]

On Thu, May 28, 2026 at 09:46:51PM +0800, Jie Gan wrote:
> 
> 
> On 5/28/2026 9:06 PM, Qiang Yu wrote:
> > On Thu, May 28, 2026 at 11:03:45AM +0800, Jie Gan wrote:
> > > 
> > > 
> > > On 5/28/2026 10:29 AM, Qiang Yu wrote:
> > > > Before XO refclk is distributed to PCIe/USB/eDP PHYs, it passes through
> > > > a QREF block. QREF is powered by dedicated LDO rails, and the clkref_en
> > > > register controls whether refclk is gated through to the PHY side.
> > > > 
> > > > These clkref controls are different from typical GCC branch clocks:
> > > > - only a single enable bit is present, without branch-style config bits
> > > > - regulators must be voted before enable and unvoted after disable
> > > > 
> > > > Model this as a dedicated clk_ref clock type with custom clk_ops instead
> > > > of reusing struct clk_branch semantics.
> > > > 
> > > > Also provide a common registration/probe API so the same clkref model
> > > > can be reused regardless of where clkref_en registers are placed, e.g.
> > > > TCSR on glymur and TLMM on SM8750.
> > > > 
> 
> [...]
> 
> > > > +
> > > > +static int qcom_clk_ref_is_enabled(struct clk_hw *hw)
> > > > +{
> > > > +	struct qcom_clk_ref *rclk = to_qcom_clk_ref(hw);
> > > > +	u32 val;
> > > > +	int ret;
> > > > +
> > > > +	ret = regmap_read(rclk->regmap, rclk->desc.offset, &val);
> > > > +	if (ret)
> > > > +		return ret;
> > > 
> > > regmap_read returns a negative error code on failure, but the
> > > clk_ops.is_enabled() treats the non-zero value as enabled.
> > > 
> > 
> > A regmap_read failure doesn't mean the clock is disabled.
> 
> Do we have special reason to treat the error number as "true"? Its worthy to
> add a comment to explain why.
>
I'm not sure. In some clk drivers, they return fail. eg.clk-branch.c

- Qiang Yu
> Thanks,
> Jie
> 
> > 
> > - Qiang Yu
> > > Thanks,
> > > Jie
> > > 
> > > > +
> > > > +	return !!(val & QCOM_CLK_REF_EN_MASK);
> > > > +}
> > > > +
> > > > +static const struct clk_ops qcom_clk_ref_ops = {
> > > > +	.prepare = qcom_clk_ref_prepare,
> > > > +	.unprepare = qcom_clk_ref_unprepare,
> > > > +	.enable = qcom_clk_ref_enable,
> > > > +	.disable = qcom_clk_ref_disable,
> > > > +	.is_enabled = qcom_clk_ref_is_enabled,
> > > > +};
> > > > +
> > > > +static int qcom_clk_ref_register(struct device *dev, struct regmap *regmap,
> > > > +				 struct qcom_clk_ref *clk_refs,
> > > > +				 const struct qcom_clk_ref_desc *descs,
> > > > +				 size_t num_clk_refs)
> > > > +{
> > > > +	const struct qcom_clk_ref_desc *desc;
> > > > +	struct qcom_clk_ref *clk_ref;
> > > > +	size_t clk_idx;
> > > > +	unsigned int i;
> > > > +	int ret;
> > > > +
> > > > +	for (clk_idx = 0; clk_idx < num_clk_refs; clk_idx++) {
> > > > +		clk_ref = &clk_refs[clk_idx];
> > > > +		desc = &descs[clk_idx];
> > > > +
> > > > +		if (!desc->name)
> > > > +			continue;
> > > > +
> > > > +		clk_ref->regmap = regmap;
> > > > +		clk_ref->desc = *desc;
> > > > +
> > > > +		if (clk_ref->desc.num_regulators) {
> > > > +			clk_ref->regulators = devm_kcalloc(dev, clk_ref->desc.num_regulators,
> > > > +							   sizeof(*clk_ref->regulators),
> > > > +							   GFP_KERNEL);
> > > > +			if (!clk_ref->regulators)
> > > > +				return -ENOMEM;
> > > > +
> > > > +			for (i = 0; i < clk_ref->desc.num_regulators; i++)
> > > > +				clk_ref->regulators[i].supply =
> > > > +					clk_ref->desc.regulator_names[i];
> > > > +
> > > > +			ret = devm_regulator_bulk_get(dev, clk_ref->desc.num_regulators,
> > > > +						      clk_ref->regulators);
> > > > +			if (ret)
> > > > +				return dev_err_probe(dev, ret,
> > > > +						     "Failed to get regulators for %s\n",
> > > > +						     clk_ref->desc.name);
> > > > +		}
> > > > +
> > > > +		clk_ref->init_data.name = clk_ref->desc.name;
> > > > +		clk_ref->init_data.parent_data = &qcom_clk_ref_parent_data;
> > > > +		clk_ref->init_data.num_parents = 1;
> > > > +		clk_ref->init_data.ops = &qcom_clk_ref_ops;
> > > > +		clk_ref->hw.init = &clk_ref->init_data;
> > > > +
> > > > +		ret = devm_clk_hw_register(dev, &clk_ref->hw);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static struct clk_hw *qcom_clk_ref_provider_get(struct of_phandle_args *clkspec, void *data)
> > > > +{
> > > > +	struct qcom_clk_ref_provider *provider = data;
> > > > +	unsigned int idx = clkspec->args[0];
> > > > +
> > > > +	if (idx >= provider->num_refs)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	if (!provider->refs[idx].regmap)
> > > > +		return ERR_PTR(-ENOENT);
> > > > +
> > > > +	return &provider->refs[idx].hw;
> > > > +}
> > > > +
> > > > +int qcom_clk_ref_probe(struct platform_device *pdev,
> > > > +		       const struct regmap_config *config,
> > > > +		       const struct qcom_clk_ref_desc *descs,
> > > > +		       size_t num_clk_refs)
> > > > +{
> > > > +	struct qcom_clk_ref_provider *provider;
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct regmap *regmap;
> > > > +	void __iomem *base;
> > > > +	int ret;
> > > > +
> > > > +	base = devm_platform_ioremap_resource(pdev, 0);
> > > > +	if (IS_ERR(base))
> > > > +		return PTR_ERR(base);
> > > > +
> > > > +	regmap = devm_regmap_init_mmio(dev, base, config);
> > > > +	if (IS_ERR(regmap))
> > > > +		return PTR_ERR(regmap);
> > > > +
> > > > +	provider = devm_kzalloc(dev, sizeof(*provider), GFP_KERNEL);
> > > > +	if (!provider)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	provider->refs = devm_kcalloc(dev, num_clk_refs, sizeof(*provider->refs),
> > > > +				      GFP_KERNEL);
> > > > +	if (!provider->refs)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	provider->num_refs = num_clk_refs;
> > > > +
> > > > +	ret = qcom_clk_ref_register(dev, regmap, provider->refs, descs,
> > > > +				    provider->num_refs);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return devm_of_clk_add_hw_provider(dev, qcom_clk_ref_provider_get, provider);
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(qcom_clk_ref_probe);
> > > > diff --git a/include/linux/clk/qcom.h b/include/linux/clk/qcom.h
> > > > new file mode 100644
> > > > index 000000000000..09e2e3178cfb
> > > > --- /dev/null
> > > > +++ b/include/linux/clk/qcom.h
> > > > @@ -0,0 +1,69 @@
> > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > +/*
> > > > + * Copyright (c) 2026, Qualcomm Technologies, Inc. and/or its subsidiaries.
> > > > + */
> > > > +
> > > > +#ifndef __LINUX_CLK_QCOM_H
> > > > +#define __LINUX_CLK_QCOM_H
> > > > +
> > > > +#include <linux/clk-provider.h>
> > > > +#include <linux/errno.h>
> > > > +#include <linux/kconfig.h>
> > > > +#include <linux/regmap.h>
> > > > +#include <linux/types.h>
> > > > +
> > > > +struct device;
> > > > +struct platform_device;
> > > > +struct regulator_bulk_data;
> > > > +
> > > > +/**
> > > > + * struct qcom_clk_ref_desc - descriptor for a clkref_en gate clock
> > > > + * @name: clock name exposed to the common clock framework
> > > > + * @offset: clkref_en register offset from the block base
> > > > + * @regulator_names: optional supply names enabled while preparing the clock
> > > > + * @num_regulators: number of entries in @regulator_names
> > > > + */
> > > > +struct qcom_clk_ref_desc {
> > > > +	const char *name;
> > > > +	u32 offset;
> > > > +	const char * const *regulator_names;
> > > > +	unsigned int num_regulators;
> > > > +};
> > > > +
> > > > +/**
> > > > + * struct qcom_clk_ref - per-clock data for a clkref_en gate clock
> > > > + * @hw: common clock framework hardware clock handle
> > > > + * @init_data: common clock framework registration data
> > > > + * @regmap: register map backing the clkref_en register
> > > > + * @desc: clock descriptor copied at registration time
> > > > + * @regulators: optional bulk regulator handles for @desc.regulator_names
> > > > + */
> > > > +struct qcom_clk_ref {
> > > > +	struct clk_hw hw;
> > > > +	struct clk_init_data init_data;
> > > > +	struct regmap *regmap;
> > > > +	struct qcom_clk_ref_desc desc;
> > > > +	struct regulator_bulk_data *regulators;
> > > > +};
> > > > +
> > > > +#if IS_ENABLED(CONFIG_COMMON_CLK_QCOM)
> > > > +
> > > > +int qcom_clk_ref_probe(struct platform_device *pdev,
> > > > +		       const struct regmap_config *config,
> > > > +		       const struct qcom_clk_ref_desc *descs,
> > > > +		       size_t num_clk_refs);
> > > > +
> > > > +#else
> > > > +
> > > > +static inline int
> > > > +qcom_clk_ref_probe(struct platform_device *pdev,
> > > > +		   const struct regmap_config *config,
> > > > +		   const struct qcom_clk_ref_desc *descs,
> > > > +		   size_t num_clk_refs)
> > > > +{
> > > > +	return -EOPNOTSUPP;
> > > > +}
> > > > +
> > > > +#endif
> > > > +
> > > > +#endif
> > > > 
> > > 
>