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

[Jie Gan <jie.gan@oss.qualcomm.com>]

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.

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
>>>
>>