Re: [PATCH v1 3/4] clk: qcom: add clock controller driver for qca8386/qca8084

[Jie Luo <quic_luoj@quicinc.com>]

On 8/9/2023 11:38 PM, Krzysztof Kozlowski wrote:
> On 09/08/2023 10:00, Luo Jie wrote:
>> Add clock & reset controller driver for qca8386/qca8084.
>>
>> Signed-off-by: Luo Jie <quic_luoj@quicinc.com>
>> ---
>>   drivers/clk/qcom/Kconfig       |    8 +
>>   drivers/clk/qcom/Makefile      |    1 +
>>   drivers/clk/qcom/nsscc-qca8k.c | 2195 ++++++++++++++++++++++++++++++++
>>   3 files changed, 2204 insertions(+)
>>   create mode 100644 drivers/clk/qcom/nsscc-qca8k.c
>>
>> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
>> index 263e55d75e3f..d84705ff920d 100644
>> --- a/drivers/clk/qcom/Kconfig
>> +++ b/drivers/clk/qcom/Kconfig
>> @@ -195,6 +195,14 @@ config IPQ_GCC_9574
>>   	  i2c, USB, SD/eMMC, etc. Select this for the root clock
>>   	  of ipq9574.
>>   
>> +config IPQ_NSSCC_QCA8K
>> +	tristate "QCA8K(QCA8386 or QCA8084) NSS Clock Controller"
> 
> Is it specific to some arch? We keep ARM or ARM64 for most of the entries.

Hi, Krzysztof,
It's not specific to the arch, which is configured by MDIO, not Soc, so 
it does not depend on the ARM.

> 
>> +	help
>> +	  Support for NSS(Network SubSystem) clock controller on
>> +	  qca8386/qca8084 chip.
>> +	  Say Y if you want to use network features of switch or PHY
>> +	  device. Select this for the root clock of qca8k.
>> +
>>   config MSM_GCC_8660
>>   	tristate "MSM8660 Global Clock Controller"
>>   	depends on ARM || COMPILE_TEST
> 
> ...
> 
>> +static int nss_cc_qca8k_probe(struct mdio_device *mdiodev)
>> +{
>> +	struct device *dev = &mdiodev->dev;
>> +	struct regmap *regmap;
>> +	struct qcom_reset_controller *reset;
>> +	struct qcom_cc_desc desc = nss_cc_qca8k_desc;
>> +	size_t num_clks = desc.num_clks;
>> +	struct clk_regmap **rclks = desc.clks;
>> +	struct qcom_cc *cc;
>> +	int ret, i;
>> +
>> +	cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
>> +	if (!cc)
>> +		return -ENOMEM;
>> +
>> +	cc->rclks = rclks;
>> +	cc->num_rclks = num_clks;
>> +	reset = &cc->reset;
>> +
>> +	regmap = devm_regmap_init(dev, NULL, mdiodev->bus, desc.config);
>> +
> 
> Drop blank line.

Okay.
> 
>> +	if (IS_ERR(regmap)) {
>> +		dev_err(dev, "Failed to init MDIO regmap\n");
> 
> All of error returns could be converted return dev_err_probe(), just to
> have smaller code. Not a requirement, though.
> 

will update this in the next patch set.

>> +		return PTR_ERR(regmap);
>> +	}
>> +
>> +	reset->rcdev.of_node = dev->of_node;
>> +	reset->rcdev.dev = dev;
>> +	reset->rcdev.ops = &qcom_reset_ops;
>> +	reset->rcdev.owner = dev->driver->owner;
>> +	reset->rcdev.nr_resets = desc.num_resets;
>> +	reset->regmap = regmap;
>> +	reset->reset_map = desc.resets;
>> +
>> +	ret = devm_reset_controller_register(dev, &reset->rcdev);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register QCA8K reset controller: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	for (i = 0; i < num_clks; i++) {
>> +		if (!rclks[i])
>> +			continue;
>> +
>> +		ret = devm_clk_register_regmap(dev, rclks[i]);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to regmap register for QCA8K clock: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	ret = devm_of_clk_add_hw_provider(dev, qcom_qca8k_clk_hw_get, cc);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register provider for QCA8K clock: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	dev_info(dev, "Registered NSSCC QCA8K clocks\n");
> 
> Drop the simple info for probe status. Kernel has other ways to do this.

will remove this in the next patch set.

> 
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id nss_cc_qca8k_match_table[] = {
>> +	{ .compatible = "qcom,qca8085-nsscc" },
>> +	{ .compatible = "qcom,qca8084-nsscc" },
>> +	{ .compatible = "qcom,qca8082-nsscc" },
>> +	{ .compatible = "qcom,qca8386-nsscc" },
>> +	{ .compatible = "qcom,qca8385-nsscc" },
>> +	{ .compatible = "qcom,qca8384-nsscc" },
> 
> You only need qca8084 here. Drop all other entries.
will remove these entries in the next patch, thanks for the review.

> 
>> +	{ }
>> +};
> 
> 
> 
> Best regards,
> Krzysztof
>