Re: [PATCH 04/23] clk: samsung: fsd: Add cmu_peric block clock information

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 13/01/2022 13:11, Alim Akhtar wrote:
> This patch adds CMU_PERIC block clock information needed
> for various IPs functions found in this block.

Here and in all other commits, please do not use "This patch". Instead:
https://elixir.bootlin.com/linux/v5.13/source/Documentation/process/submitting-patches.rst#L89

> 
> Cc: linux-fsd@tesla.com
> Signed-off-by: Aswani Reddy <aswani.reddy@samsung.com>
> Signed-off-by: Niyas Ahmed S T <niyas.ahmed@samsung.com>
> Signed-off-by: Chandrasekar R <rcsekar@samsung.com>
> Signed-off-by: Jayati Sahu <jayati.sahu@samsung.com>
> Signed-off-by: Sriranjani P <sriranjani.p@samsung.com>
> Signed-off-by: Ajay Kumar <ajaykumar.rs@samsung.com>
> Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com>
> Signed-off-by: Alim Akhtar <alim.akhtar@samsung.com>
> ---
>  drivers/clk/samsung/clk-fsd.c | 464 +++++++++++++++++++++++++++++++++-
>  1 file changed, 463 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/samsung/clk-fsd.c b/drivers/clk/samsung/clk-fsd.c
> index e47523106d9e..6da20966ba99 100644
> --- a/drivers/clk/samsung/clk-fsd.c
> +++ b/drivers/clk/samsung/clk-fsd.c
> @@ -9,12 +9,59 @@
>   *
>   */
>  
> -#include <linux/clk-provider.h>
>  #include <linux/of.h>
> +#include <linux/clk.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/clk-provider.h>
> +#include <linux/platform_device.h>

Please order the includes alphabetically.

>  
>  #include "clk.h"
>  #include <dt-bindings/clock/fsd-clk.h>
>  
> +/* Gate register bits */
> +#define GATE_MANUAL		BIT(20)
> +#define GATE_ENABLE_HWACG	BIT(28)
> +
> +/* Gate register offsets range */
> +#define GATE_OFF_START		0x2000
> +#define GATE_OFF_END		0x2fff
> +
> +/**
> + * fsd_init_clocks - Set clocks initial configuration
> + * @np:			CMU device tree node with "reg" property (CMU addr)
> + * @reg_offs:		Register offsets array for clocks to init
> + * @reg_offs_len:	Number of register offsets in reg_offs array
> + *
> + * Set manual control mode for all gate clocks.
> + */
> +static void __init fsd_init_clocks(struct device_node *np,
> +		const unsigned long *reg_offs, size_t reg_offs_len)

The same as exynos_arm64_init_clocks - please re-use instead of duplicating.

> +{
> +	void __iomem *reg_base;
> +	size_t i;
> +
> +	reg_base = of_iomap(np, 0);
> +	if (!reg_base)
> +		panic("%s: failed to map registers\n", __func__);
> +
> +	for (i = 0; i < reg_offs_len; ++i) {
> +		void __iomem *reg = reg_base + reg_offs[i];
> +		u32 val;
> +
> +		/* Modify only gate clock registers */
> +		if (reg_offs[i] < GATE_OFF_START || reg_offs[i] > GATE_OFF_END)
> +			continue;
> +
> +		val = readl(reg);
> +		val |= GATE_MANUAL;
> +		val &= ~GATE_ENABLE_HWACG;
> +		writel(val, reg);
> +	}
> +
> +	iounmap(reg_base);
> +}
> +

(...)

> +/**
> + * fsd_cmu_probe - Probe function for FSD platform clocks
> + * @pdev: Pointer to platform device
> + *
> + * Configure clock hierarchy for clock domains of FSD platform
> + */
> +static int __init fsd_cmu_probe(struct platform_device *pdev)
> +{
> +	const struct samsung_cmu_info *info;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +
> +	info = of_device_get_match_data(dev);
> +	fsd_init_clocks(np, info->clk_regs, info->nr_clk_regs);
> +	samsung_cmu_register_one(np, info);
> +
> +	/* Keep bus clock running, so it's possible to access CMU registers */
> +	if (info->clk_name) {
> +		struct clk *bus_clk;
> +
> +		bus_clk = clk_get(dev, info->clk_name);
> +		if (IS_ERR(bus_clk)) {
> +			pr_err("%s: could not find bus clock %s; err = %ld\n",
> +			       __func__, info->clk_name, PTR_ERR(bus_clk));
> +		} else {
> +			clk_prepare_enable(bus_clk);
> +		}
> +	}
> +
> +	return 0;
> +}

Please re-use exynos_arm64_register_cmu(). This will also solve my
previous comment about exynos_arm64_init_clocks().

> +
> +/* CMUs which belong to Power Domains and need runtime PM to be implemented */
> +static const struct of_device_id fsd_cmu_of_match[] = {
> +	{
> +		.compatible = "tesla,fsd-clock-peric",
> +		.data = &peric_cmu_info,
> +	}, {
> +	},
> +};
> +
> +static struct platform_driver fsd_cmu_driver __refdata = {
> +	.driver	= {
> +		.name = "fsd-cmu",
> +		.of_match_table = fsd_cmu_of_match,
> +		.suppress_bind_attrs = true,
> +	},
> +	.probe = fsd_cmu_probe,
> +};
> +
> +static int __init fsd_cmu_init(void)
> +{
> +	return platform_driver_register(&fsd_cmu_driver);
> +}
> +core_initcall(fsd_cmu_init);
> 


Best regards,
Krzysztof