[PATCH] clk: mvebu: cp110: Fix clock tree representation
Gregory CLEMENT
gregory.clement at bootlin.com
Tue Mar 13 04:32:47 PDT 2018
Hi,
On mer., févr. 28 2018, Gregory CLEMENT <gregory.clement at bootlin.com> wrote:
> Thanks to new documentation, we have a better view of the clock tree.
> There were few mistakes in the first version of this driver, the main one
> being the parental link between the clocks. Actually the tree is more
> flat that we though. Most of the IP blocks require two clocks: one for
> the IP itself and one for accessing the registers, and unlike what we
> wrote there is no link between these two clocks.
>
> The other mistakes were about the name of the clocks: the root clock is
> not the Audio PLL but the PLL0, and what we called the EIP clock is named
> the x2 Core clock and is used by other IP block than the EIP ones.
Do you have any feedback on this patch?
I would like to have time to address them if you have any remark.
Else do you want a Pull Request or could you apply it directly?
The only other patch around the mvebu clocks was set a few days ago [1]
and seems to be eventually a fix patch. That means that there would be
only one patch in the Pull Request for 4.17, but I can do it if you
prefer.
Thanks,
Gregory
[1]: "[PATCH] clk: mvebu: armada-38x: add support for missing clocks"
https://lkml.org/lkml/2018/3/8/812
>
> Signed-off-by: Gregory CLEMENT <gregory.clement at bootlin.com>
> ---
> drivers/clk/mvebu/cp110-system-controller.c | 94 ++++++++++++-----------------
> 1 file changed, 39 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/clk/mvebu/cp110-system-controller.c b/drivers/clk/mvebu/cp110-system-controller.c
> index ca9a0a536174..75bf7b8f282f 100644
> --- a/drivers/clk/mvebu/cp110-system-controller.c
> +++ b/drivers/clk/mvebu/cp110-system-controller.c
> @@ -13,18 +13,17 @@
> /*
> * CP110 has 6 core clocks:
> *
> - * - APLL (1 Ghz)
> - * - PPv2 core (1/3 APLL)
> - * - EIP (1/2 APLL)
> - * - Core (1/2 EIP)
> - * - SDIO (2/5 APLL)
> + * - PLL0 (1 Ghz)
> + * - PPv2 core (1/3 PLL0)
> + * - x2 Core (1/2 PLL0)
> + * - Core (1/2 x2 Core)
> + * - SDIO (2/5 PLL0)
> *
> * - NAND clock, which is either:
> * - Equal to SDIO clock
> - * - 2/5 APLL
> + * - 2/5 PLL0
> *
> - * CP110 has 32 gatable clocks, for the various peripherals in the
> - * IP. They have fairly complicated parent/child relationships.
> + * CP110 has 32 gatable clocks, for the various peripherals in the IP.
> */
>
> #define pr_fmt(fmt) "cp110-system-controller: " fmt
> @@ -53,9 +52,9 @@ enum {
> #define CP110_CLK_NUM \
> (CP110_MAX_CORE_CLOCKS + CP110_MAX_GATABLE_CLOCKS)
>
> -#define CP110_CORE_APLL 0
> +#define CP110_CORE_PLL0 0
> #define CP110_CORE_PPV2 1
> -#define CP110_CORE_EIP 2
> +#define CP110_CORE_X2CORE 2
> #define CP110_CORE_CORE 3
> #define CP110_CORE_NAND 4
> #define CP110_CORE_SDIO 5
> @@ -237,7 +236,7 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
> struct regmap *regmap;
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
> - const char *ppv2_name, *apll_name, *core_name, *eip_name, *nand_name,
> + const char *ppv2_name, *pll0_name, *core_name, *x2core_name, *nand_name,
> *sdio_name;
> struct clk_hw_onecell_data *cp110_clk_data;
> struct clk_hw *hw, **cp110_clks;
> @@ -263,20 +262,20 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
> cp110_clks = cp110_clk_data->hws;
> cp110_clk_data->num = CP110_CLK_NUM;
>
> - /* Register the APLL which is the root of the hw tree */
> - apll_name = cp110_unique_name(dev, syscon_node, "apll");
> - hw = clk_hw_register_fixed_rate(NULL, apll_name, NULL, 0,
> + /* Register the PLL0 which is the root of the hw tree */
> + pll0_name = cp110_unique_name(dev, syscon_node, "pll0");
> + hw = clk_hw_register_fixed_rate(NULL, pll0_name, NULL, 0,
> 1000 * 1000 * 1000);
> if (IS_ERR(hw)) {
> ret = PTR_ERR(hw);
> - goto fail_apll;
> + goto fail_pll0;
> }
>
> - cp110_clks[CP110_CORE_APLL] = hw;
> + cp110_clks[CP110_CORE_PLL0] = hw;
>
> - /* PPv2 is APLL/3 */
> + /* PPv2 is PLL0/3 */
> ppv2_name = cp110_unique_name(dev, syscon_node, "ppv2-core");
> - hw = clk_hw_register_fixed_factor(NULL, ppv2_name, apll_name, 0, 1, 3);
> + hw = clk_hw_register_fixed_factor(NULL, ppv2_name, pll0_name, 0, 1, 3);
> if (IS_ERR(hw)) {
> ret = PTR_ERR(hw);
> goto fail_ppv2;
> @@ -284,30 +283,32 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
>
> cp110_clks[CP110_CORE_PPV2] = hw;
>
> - /* EIP clock is APLL/2 */
> - eip_name = cp110_unique_name(dev, syscon_node, "eip");
> - hw = clk_hw_register_fixed_factor(NULL, eip_name, apll_name, 0, 1, 2);
> + /* X2CORE clock is PLL0/2 */
> + x2core_name = cp110_unique_name(dev, syscon_node, "x2core");
> + hw = clk_hw_register_fixed_factor(NULL, x2core_name, pll0_name,
> + 0, 1, 2);
> if (IS_ERR(hw)) {
> ret = PTR_ERR(hw);
> goto fail_eip;
> }
>
> - cp110_clks[CP110_CORE_EIP] = hw;
> + cp110_clks[CP110_CORE_X2CORE] = hw;
>
> - /* Core clock is EIP/2 */
> + /* Core clock is X2CORE/2 */
> core_name = cp110_unique_name(dev, syscon_node, "core");
> - hw = clk_hw_register_fixed_factor(NULL, core_name, eip_name, 0, 1, 2);
> + hw = clk_hw_register_fixed_factor(NULL, core_name, x2core_name,
> + 0, 1, 2);
> if (IS_ERR(hw)) {
> ret = PTR_ERR(hw);
> goto fail_core;
> }
>
> cp110_clks[CP110_CORE_CORE] = hw;
> - /* NAND can be either APLL/2.5 or core clock */
> + /* NAND can be either PLL0/2.5 or core clock */
> nand_name = cp110_unique_name(dev, syscon_node, "nand-core");
> if (nand_clk_ctrl & NF_CLOCK_SEL_400_MASK)
> hw = clk_hw_register_fixed_factor(NULL, nand_name,
> - apll_name, 0, 2, 5);
> + pll0_name, 0, 2, 5);
> else
> hw = clk_hw_register_fixed_factor(NULL, nand_name,
> core_name, 0, 1, 1);
> @@ -318,10 +319,10 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
>
> cp110_clks[CP110_CORE_NAND] = hw;
>
> - /* SDIO clock is APLL/2.5 */
> + /* SDIO clock is PLL0/2.5 */
> sdio_name = cp110_unique_name(dev, syscon_node, "sdio-core");
> hw = clk_hw_register_fixed_factor(NULL, sdio_name,
> - apll_name, 0, 2, 5);
> + pll0_name, 0, 2, 5);
> if (IS_ERR(hw)) {
> ret = PTR_ERR(hw);
> goto fail_sdio;
> @@ -341,40 +342,23 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
> continue;
>
> switch (i) {
> - case CP110_GATE_AUDIO:
> - case CP110_GATE_COMM_UNIT:
> - case CP110_GATE_EIP150:
> - case CP110_GATE_EIP197:
> - case CP110_GATE_SLOW_IO:
> - parent = gate_name[CP110_GATE_MAIN];
> - break;
> - case CP110_GATE_MG:
> - parent = gate_name[CP110_GATE_MG_CORE];
> - break;
> case CP110_GATE_NAND:
> parent = nand_name;
> break;
> + case CP110_GATE_MG:
> + case CP110_GATE_GOP_DP:
> case CP110_GATE_PPV2:
> parent = ppv2_name;
> break;
> case CP110_GATE_SDIO:
> parent = sdio_name;
> break;
> - case CP110_GATE_GOP_DP:
> - parent = gate_name[CP110_GATE_SDMMC_GOP];
> - break;
> - case CP110_GATE_XOR1:
> - case CP110_GATE_XOR0:
> - case CP110_GATE_PCIE_X1_0:
> - case CP110_GATE_PCIE_X1_1:
> + case CP110_GATE_MAIN:
> + case CP110_GATE_PCIE_XOR:
> case CP110_GATE_PCIE_X4:
> - parent = gate_name[CP110_GATE_PCIE_XOR];
> - break;
> - case CP110_GATE_SATA:
> - case CP110_GATE_USB3H0:
> - case CP110_GATE_USB3H1:
> - case CP110_GATE_USB3DEV:
> - parent = gate_name[CP110_GATE_SATA_USB];
> + case CP110_GATE_EIP150:
> + case CP110_GATE_EIP197:
> + parent = x2core_name;
> break;
> default:
> parent = core_name;
> @@ -413,12 +397,12 @@ static int cp110_syscon_common_probe(struct platform_device *pdev,
> fail_nand:
> clk_hw_unregister_fixed_factor(cp110_clks[CP110_CORE_CORE]);
> fail_core:
> - clk_hw_unregister_fixed_factor(cp110_clks[CP110_CORE_EIP]);
> + clk_hw_unregister_fixed_factor(cp110_clks[CP110_CORE_X2CORE]);
> fail_eip:
> clk_hw_unregister_fixed_factor(cp110_clks[CP110_CORE_PPV2]);
> fail_ppv2:
> - clk_hw_unregister_fixed_rate(cp110_clks[CP110_CORE_APLL]);
> -fail_apll:
> + clk_hw_unregister_fixed_rate(cp110_clks[CP110_CORE_PLL0]);
> +fail_pll0:
> return ret;
> }
>
> --
> 2.16.1
>
--
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com
More information about the linux-arm-kernel
mailing list