On 1/28/2025 5:29 PM, Konrad Dybcio wrote:
On 27.01.2025 10:31 AM, Sricharan R wrote:
From: Sricharan Ramabadhran <quic_srichara@xxxxxxxxxxx>
CPU on Qualcomm ipq5424 is clocked by huayra PLL with RCG support.
Add support for the APSS PLL, RCG and clock enable for ipq5424.
The PLL, RCG register space are clubbed. Hence adding new APSS driver
for both PLL and RCG/CBC control. Also the L3 cache has a separate pll
and needs to be scaled along with the CPU.
Co-developed-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>
Signed-off-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>
Signed-off-by: Sricharan Ramabadhran <quic_srichara@xxxxxxxxxxx>
---
[...]
+#define GPLL0_CLK_RATE 800000000
+#define CPU_NOM_CLK_RATE 1416000000
+#define CPU_TURBO_CLK_RATE 1800000000
+#define L3_NOM_CLK_RATE 984000000
+#define L3_TURBO_CLK_RATE 1272000000
Please inline these values
ok.
+
+enum {
+ P_XO,
+ P_GPLL0,
+ P_APSS_PLL_EARLY,
+ P_L3_PLL,
+};
+
+struct apss_clk {
+ struct notifier_block cpu_clk_notifier;
+ struct clk_hw *hw;
+ struct device *dev;
+ struct clk *l3_clk;
+};
+
+static struct clk_branch l3_core_clk = {
+ .halt_reg = 0x1008c,
+ .clkr = {
+ .enable_reg = 0x1008c,
+ .enable_mask = BIT(0),
+ .hw.init = &(struct clk_init_data){
+ .name = "l3_clk",
+ .parent_hws = (const struct clk_hw *[]){
+ &l3_clk_src.clkr.hw },
&l3_clk_src.clkr.hw
},
+static unsigned long get_l3_clk_from_tbl(unsigned long rate)
+{
+ struct clk_rcg2 *l3_rcg2 = container_of(&l3_clk_src.clkr, struct clk_rcg2, clkr);
+ u8 max_clk = sizeof(ftbl_apss_clk_src) / sizeof(struct freq_tbl);
+ u8 loop;
+
+ for (loop = 0; loop < max_clk; loop++)
+ if (ftbl_apss_clk_src[loop].freq == rate)
+ return l3_rcg2->freq_tbl[loop].freq;
This looks extremely explosive if anyone makes changes to the driver..
Use an OPP table in the devicetree instead
ok, already using OPPtable for cpu. To understand better, since L3 clk
is separate that needs to be scaled along with cpu, are you suggesting
to use dev_pm_opp_find_freq here for indexing ?
And please add a newline before the return statement
ok
+ return 0;
+}
+
+static int cpu_clk_notifier_fn(struct notifier_block *nb, unsigned long action,
+ void *data)
+{
+ struct apss_clk *apss_ipq5424_cfg = container_of(nb, struct apss_clk, cpu_clk_notifier);
+ struct clk_notifier_data *cnd = (struct clk_notifier_data *)data;
+ struct device *dev = apss_ipq5424_cfg->dev;
+ unsigned long rate = 0, l3_rate;
+ int err = 0;
Please use 'ret'
ok
+
+ dev_dbg(dev, "action:%ld old_rate:%ld new_rate:%ld\n", action,
+ cnd->old_rate, cnd->new_rate);
+
+ switch (action) {
+ case PRE_RATE_CHANGE:
+ if (cnd->old_rate < cnd->new_rate)
+ rate = cnd->new_rate;
+ break;
Why are the breaks indented like this?
ok, will fix
+ case POST_RATE_CHANGE:
+ if (cnd->old_rate > cnd->new_rate)
+ rate = cnd->new_rate;
+ break;
+ };
+
+ if (!rate)
+ goto notif_ret;
In cases like these, just return directly instead of jumping
ok
+
+ l3_rate = get_l3_clk_from_tbl(rate);
+ if (!l3_rate) {
+ dev_err(dev, "Failed to get l3 clock rate from l3_tbl\n");
+ return NOTIFY_BAD;
+ }
+
+ err = clk_set_rate(apss_ipq5424_cfg->l3_clk, l3_rate);
+ if (err) {
+ dev_err(dev, "Failed to set l3 clock rate(%ld) err(%d)\n", l3_rate, err);
+ return NOTIFY_BAD;
+ }
+
+notif_ret:
+ return NOTIFY_OK;
+}
+
+static int apss_ipq5424_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct apss_clk *apss_ipq5424_cfg;
+ struct regmap *regmap;
+ void __iomem *base;
+ int ret;
+
+ apss_ipq5424_cfg = devm_kzalloc(&pdev->dev, sizeof(struct apss_clk), GFP_KERNEL);
Since there is no "config" in there, something like "ipq5424_apsscc" would be
more fitting
ok
+ if (IS_ERR_OR_NULL(apss_ipq5424_cfg))
+ return PTR_ERR(apss_ipq5424_cfg);
https://elixir.bootlin.com/linux/v6.13/source/include/linux/device.h#L326-L329
|_
> elixir.bootlin.com/linux/v6.13/source/drivers/base/devres.c#L819-L820
It can never throw an errno, just check for if (!apss...)
ok
+
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ regmap = devm_regmap_init_mmio(dev, base, &apss_ipq5424_regmap_config);
+ if (!regmap)
+ return PTR_ERR(regmap);
devm_platform_get_and_ioremap_resource()
ok
+
+ clk_alpha_pll_configure(&ipq5424_l3_pll, regmap, &l3_pll_config);
+
+ clk_alpha_pll_configure(&ipq5424_apss_pll, regmap, &apss_pll_config);
+
+ ret = qcom_cc_really_probe(dev, &apss_ipq5424_desc, regmap);
+ if (ret)
+ return ret;
+
+ dev_dbg(&pdev->dev, "Registered APSS & L3 clock provider\n");
+
+ apss_ipq5424_cfg->dev = dev;
+ apss_ipq5424_cfg->hw = &apss_silver_clk_src.clkr.hw;
+ apss_ipq5424_cfg->cpu_clk_notifier.notifier_call = cpu_clk_notifier_fn;
+
+ apss_ipq5424_cfg->l3_clk = clk_hw_get_clk(&l3_core_clk.clkr.hw, "l3_clk");
+ if (IS_ERR(apss_ipq5424_cfg->l3_clk)) {
+ dev_err(&pdev->dev, "Failed to get L3 clk, %ld\n",
+ PTR_ERR(apss_ipq5424_cfg->l3_clk));
+ return PTR_ERR(apss_ipq5424_cfg->l3_clk);
+ }
Now that you'll use OPP, you can drop all this getting.. maybe even the
apss_ipq5424_cfg struct could be let go
ok, is the suggestion here to use devm_pm_opp_set_config ?
+
+ ret = devm_clk_notifier_register(&pdev->dev, apss_ipq5424_cfg->hw->clk,
+ &apss_ipq5424_cfg->cpu_clk_notifier);
+ if (ret)
+ return ret;
+
+ return 0;
Just return ret instead
ok
+}
+
+static const struct of_device_id apss_ipq5424_match_table[] = {
+ { .compatible = "qcom,ipq5424-apss-clk" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, apss_ipq5424_match_table);
+
+static struct platform_driver apss_ipq5424_driver = {
+ .probe = apss_ipq5424_probe,
+ .driver = {
+ .name = "apss-ipq5424-clk",
+ .of_match_table = apss_ipq5424_match_table,
+ },
+};
+
+module_platform_driver(apss_ipq5424_driver);
+
+MODULE_DESCRIPTION("QCOM APSS IPQ5424 CLK Driver");
+MODULE_LICENSE("GPL v2");
Please don't skip running 'checkpatch'.
Infact ran it with --strict as well. But thought "GPL V2" was correct
and let it. Anyways will change.
Regards,
Sricharan