Re: [PATCHv2 3/3] clk: socfpga: agilex: add clock driver for the Agilex platform

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Quoting Dinh Nguyen (2020-03-09 10:16:53)
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index f4169cc2fd31..d9ddc0bd91c0 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -18,6 +18,7 @@ endif
>  
>  # hardware specific clock types
>  # please keep this section sorted lexicographically by file path name
> +obj-$(CONFIG_ARCH_AGILEX)              += socfpga/

This is not sorted by file path name.

>  obj-$(CONFIG_MACH_ASM9260)             += clk-asm9260.o
>  obj-$(CONFIG_COMMON_CLK_AXI_CLKGEN)    += clk-axi-clkgen.o
>  obj-$(CONFIG_ARCH_AXXIA)               += clk-axm5516.o
> diff --git a/drivers/clk/socfpga/clk-agilex.c b/drivers/clk/socfpga/clk-agilex.c
> new file mode 100644
> index 000000000000..6789892085db
> --- /dev/null
> +++ b/drivers/clk/socfpga/clk-agilex.c
> @@ -0,0 +1,369 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019, Intel Corporation
> + */
> +#include <linux/slab.h>
> +#include <linux/clk-provider.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +
> +#include <dt-bindings/clock/agilex-clock.h>
> +
> +#include "stratix10-clk.h"
[...]
> +static const struct clk_parent_data emac_mux[] = {
> +       { .name = "emaca_free_clk", },
> +       { .name = "emacb_free_clk", },
> +};

Why no newlines between structures?

> +static const struct clk_parent_data noc_mux[] = {
> +       { .name = "noc_free_clk", },
> +       { .name = "boot_clk", },
> +};

As stated before, please use actual pointers for clks within the
controller and use .fw_name for clks outside of the controller.

> +
> +/* clocks in AO (always on) controller */
> +static const struct stratix10_pll_clock agilex_pll_clks[] = {
> +       { AGILEX_BOOT_CLK, "boot_clk", boot_mux, ARRAY_SIZE(boot_mux), 0,
> +         0x0},
> +       { AGILEX_MAIN_PLL_CLK, "main_pll", pll_mux, ARRAY_SIZE(pll_mux),
> +         0, 0x48},
> +       { AGILEX_PERIPH_PLL_CLK, "periph_pll", pll_mux, ARRAY_SIZE(pll_mux),
> +         0, 0x9c},
> +};
> +
> +static const struct stratix10_perip_c_clock agilex_main_perip_c_clks[] = {
> +       { AGILEX_MAIN_PLL_C0_CLK, "main_pll_c0", "main_pll", NULL, 1, 0, 0x58},
> +       { AGILEX_MAIN_PLL_C1_CLK, "main_pll_c1", "main_pll", NULL, 1, 0, 0x5C},
> +       { AGILEX_MAIN_PLL_C2_CLK, "main_pll_c2", "main_pll", NULL, 1, 0, 0x64},
> +       { AGILEX_MAIN_PLL_C3_CLK, "main_pll_c3", "main_pll", NULL, 1, 0, 0x68},
> +       { AGILEX_PERIPH_PLL_C0_CLK, "peri_pll_c0", "periph_pll", NULL, 1, 0, 0xAC},
> +       { AGILEX_PERIPH_PLL_C1_CLK, "peri_pll_c1", "periph_pll", NULL, 1, 0, 0xB0},
> +       { AGILEX_PERIPH_PLL_C2_CLK, "peri_pll_c2", "periph_pll", NULL, 1, 0, 0xB8},
> +       { AGILEX_PERIPH_PLL_C3_CLK, "peri_pll_c3", "periph_pll", NULL, 1, 0, 0xBC},
> +};
> +
> +static const struct stratix10_perip_cnt_clock agilex_main_perip_cnt_clks[] = {
> +       { AGILEX_MPU_FREE_CLK, "mpu_free_clk", NULL, mpu_free_mux, ARRAY_SIZE(mpu_free_mux),
> +          0, 0x3C, 0, 0, 0},
> +       { AGILEX_NOC_FREE_CLK, "noc_free_clk", NULL, noc_free_mux, ARRAY_SIZE(noc_free_mux),
> +         0, 0x40, 0, 0, 1},
> +       { AGILEX_L4_SYS_FREE_CLK, "l4_sys_free_clk", "noc_free_clk", NULL, 1, 0,
> +         0, 4, 0, 0},
> +       { AGILEX_NOC_CLK, "noc_clk", NULL, noc_mux, ARRAY_SIZE(noc_mux),
> +         0, 0, 0, 0x30, 1},
> +       { AGILEX_EMAC_A_FREE_CLK, "emaca_free_clk", NULL, emaca_free_mux, ARRAY_SIZE(emaca_free_mux),
> +         0, 0xD4, 0, 0x88, 0},
> +       { AGILEX_EMAC_B_FREE_CLK, "emacb_free_clk", NULL, emacb_free_mux, ARRAY_SIZE(emacb_free_mux),
> +         0, 0xD8, 0, 0x88, 1},
> +       { AGILEX_EMAC_PTP_FREE_CLK, "emac_ptp_free_clk", NULL, emac_ptp_free_mux,
> +         ARRAY_SIZE(emac_ptp_free_mux), 0, 0xDC, 0, 0x88, 2},
> +       { AGILEX_GPIO_DB_FREE_CLK, "gpio_db_free_clk", NULL, gpio_db_free_mux,
> +         ARRAY_SIZE(gpio_db_free_mux), 0, 0xE0, 0, 0x88, 3},
> +       { AGILEX_SDMMC_FREE_CLK, "sdmmc_free_clk", NULL, sdmmc_free_mux,
> +         ARRAY_SIZE(sdmmc_free_mux), 0, 0xE4, 0, 0x88, 4},
> +       { AGILEX_S2F_USER0_FREE_CLK, "s2f_user0_free_clk", NULL, s2f_usr0_free_mux,
> +         ARRAY_SIZE(s2f_usr0_free_mux), 0, 0xE8, 0, 0, 0},
> +       { AGILEX_S2F_USER1_FREE_CLK, "s2f_user1_free_clk", NULL, s2f_usr1_free_mux,
> +         ARRAY_SIZE(s2f_usr1_free_mux), 0, 0xEC, 0, 0x88, 5},
> +       { AGILEX_PSI_REF_FREE_CLK, "psi_ref_free_clk", NULL, psi_ref_free_mux,
> +         ARRAY_SIZE(psi_ref_free_mux), 0, 0xF0, 0, 0x88, 6},
> +};
> +
> +static const struct stratix10_gate_clock agilex_gate_clks[] = {
> +       { AGILEX_MPU_CLK, "mpu_clk", NULL, mpu_mux, ARRAY_SIZE(mpu_mux), 0, 0x24,
> +         0, 0, 0, 0, 0x30, 0, 0},
> +       { AGILEX_MPU_PERIPH_CLK, "mpu_periph_clk", "mpu_clk", NULL, 1, 0, 0x24,
> +         0, 0, 0, 0, 0, 0, 4},
> +       { AGILEX_MPU_L2RAM_CLK, "mpu_l2ram_clk", "mpu_clk", NULL, 1, 0, 0x24,
> +         0, 0, 0, 0, 0, 0, 2},
> +       { AGILEX_L4_MAIN_CLK, "l4_main_clk", "noc_clk", NULL, 1, 0, 0x24,
> +         1, 0x44, 0, 2, 0, 0, 0},
> +       { AGILEX_L4_MP_CLK, "l4_mp_clk", "noc_clk", NULL, 1, 0, 0x24,
> +         2, 0x44, 8, 2, 0, 0, 0},
> +       { AGILEX_L4_SP_CLK, "l4_sp_clk", "noc_clk", NULL, 1, CLK_IS_CRITICAL, 0x24,

Please comment in the code why CLK_IS_CRITICAL is used.

> +         3, 0x44, 16, 2, 0, 0, 0},
> +       { AGILEX_CS_AT_CLK, "cs_at_clk", "noc_clk", NULL, 1, 0, 0x24,
> +         4, 0x44, 24, 2, 0, 0, 0},
> +       { AGILEX_CS_TRACE_CLK, "cs_trace_clk", "noc_clk", NULL, 1, 0, 0x24,
> +         4, 0x44, 26, 2, 0, 0, 0},
> +       { AGILEX_CS_PDBG_CLK, "cs_pdbg_clk", "cs_at_clk", NULL, 1, 0, 0x24,
> +         4, 0x44, 28, 1, 0, 0, 0},
> +       { AGILEX_CS_TIMER_CLK, "cs_timer_clk", "noc_clk", NULL, 1, 0, 0x24,
> +         5, 0, 0, 0, 0, 0, 0},
> +       { AGILEX_S2F_USER0_CLK, "s2f_user0_clk", NULL, s2f_usr0_mux, ARRAY_SIZE(s2f_usr0_mux), 0, 0x24,
> +         6, 0, 0, 0, 0, 0, 0},
> +       { AGILEX_EMAC0_CLK, "emac0_clk", NULL, emac_mux, ARRAY_SIZE(emac_mux), 0, 0x7C,
> +         0, 0, 0, 0, 0x94, 26, 0},
> +       { AGILEX_EMAC1_CLK, "emac1_clk", NULL, emac_mux, ARRAY_SIZE(emac_mux), 0, 0x7C,
> +         1, 0, 0, 0, 0x94, 27, 0},
[..]
> +
> +static struct stratix10_clock_data *__socfpga_agilex_clk_init(struct platform_device *pdev,
> +                                                   int nr_clks)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct device *dev = &pdev->dev;
> +       struct stratix10_clock_data *clk_data;
> +       struct clk **clk_table;
> +       struct resource *res;
> +       void __iomem *base;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(base)) {
> +               pr_err("%s: failed to map clock registers\n", __func__);

ioremap fail usually prints an error message already. This is not
needed.

> +               return ERR_CAST(base);
> +       }
> +
> +       clk_data = devm_kzalloc(dev, sizeof(*clk_data), GFP_KERNEL);
> +       if (!clk_data)
> +               return ERR_PTR(-ENOMEM);
> +
> +       clk_data->base = base;
> +       clk_table = devm_kcalloc(dev, nr_clks, sizeof(*clk_table), GFP_KERNEL);
> +       if (!clk_table)
> +               return ERR_PTR(-ENOMEM);
> +
> +       clk_data->clk_data.clks = clk_table;
> +       clk_data->clk_data.clk_num = nr_clks;
> +       of_clk_add_provider(np, of_clk_src_onecell_get, &clk_data->clk_data);

What if this fails?

> +       return clk_data;
> +}
> +
> +static int agilex_clkmgr_init(struct platform_device *pdev)
> +{
> +       struct stratix10_clock_data *clk_data;
> +
> +       clk_data = __socfpga_agilex_clk_init(pdev, AGILEX_NUM_CLKS);
> +       if (IS_ERR(clk_data))
> +               return PTR_ERR(clk_data);
> +
> +       agilex_clk_register_pll(agilex_pll_clks, ARRAY_SIZE(agilex_pll_clks), clk_data);
> +
> +       agilex_clk_register_c_perip(agilex_main_perip_c_clks,
> +                                ARRAY_SIZE(agilex_main_perip_c_clks), clk_data);
> +
> +       agilex_clk_register_cnt_perip(agilex_main_perip_cnt_clks,
> +                                  ARRAY_SIZE(agilex_main_perip_cnt_clks),
> +                                  clk_data);
> +
> +       agilex_clk_register_gate(agilex_gate_clks, ARRAY_SIZE(agilex_gate_clks),
> +                             clk_data);
> +       return 0;
> +}
> +
> +static int agilex_clkmgr_probe(struct platform_device *pdev)
> +{
> +       return  agilex_clkmgr_init(pdev);

Why not just put the contents of that here in probe?

> +}
> +
> +static const struct of_device_id agilex_clkmgr_match_table[] = {
> +       { .compatible = "intel,agilex-clkmgr",
> +         .data = agilex_clkmgr_init },
> +       { }
> +};
> +
> +static struct platform_driver agilex_clkmgr_driver = {
> +       .probe          = agilex_clkmgr_probe,
> +       .driver         = {
> +               .name   = "agilex-clkmgr",
> +               .suppress_bind_attrs = true,
> +               .of_match_table = agilex_clkmgr_match_table,
> +       },
> +};
> +
> +static int __init agilex_clk_init(void)
> +{
> +       return platform_driver_register(&agilex_clkmgr_driver);
> +}
> +core_initcall(agilex_clk_init);
> diff --git a/drivers/clk/socfpga/clk-pll-s10.c b/drivers/clk/socfpga/clk-pll-s10.c
> index bcd3f14e9145..17fe5bd2c0e1 100644
> --- a/drivers/clk/socfpga/clk-pll-s10.c
> +++ b/drivers/clk/socfpga/clk-pll-s10.c
> @@ -27,6 +31,27 @@
>  
>  #define to_socfpga_clk(p) container_of(p, struct socfpga_pll, hw.hw)
>  
> +static unsigned long agilex_clk_pll_recalc_rate(struct clk_hw *hwclk,
> +                                               unsigned long parent_rate)
> +{
> +       struct socfpga_pll *socfpgaclk = to_socfpga_clk(hwclk);
> +       unsigned long arefdiv, reg, mdiv;
> +       unsigned long long vco_freq;
> +
> +       /* read VCO1 reg for numerator and denominator */
> +       reg = readl(socfpgaclk->hw.reg);
> +       arefdiv = (reg & SOCFPGA_PLL_AREFDIV_MASK) >> SOCFPGA_PLL_REFDIV_SHIFT;
> +
> +       vco_freq = (unsigned long long)parent_rate / arefdiv;
> +
> +       /* Read mdiv and fdiv from the fdbck register */
> +       reg = readl(socfpgaclk->hw.reg + 0x24);
> +       mdiv = (reg & SOCFPGA_AGILEX_PLL_MDIV_MASK);

Please remove useless parenthesis.

> +
> +       vco_freq = (unsigned long long)vco_freq * mdiv;
> +       return (unsigned long)vco_freq;

Drop the cast, it's implicit.

> +}
> +
>  static unsigned long clk_pll_recalc_rate(struct clk_hw *hwclk,
>                                          unsigned long parent_rate)
>  {
> @@ -98,6 +123,12 @@ static int clk_pll_prepare(struct clk_hw *hwclk)
>         return 0;
>  }
>  
> +static struct clk_ops agilex_clk_pll_ops = {

Can it be const?

> +       .recalc_rate = agilex_clk_pll_recalc_rate,
> +       .get_parent = clk_pll_get_parent,
> +       .prepare = clk_pll_prepare,
> +};
> +
>  static struct clk_ops clk_pll_ops = {

I guess this one could be const too, but in a different patch?

>         .recalc_rate = clk_pll_recalc_rate,
>         .get_parent = clk_pll_get_parent,




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux