Hi, On mer., févr. 28 2018, Gregory CLEMENT <gregory.clem...@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.clem...@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
