Re: [PATCH v8 08/13] drivers: memory: add DMC driver for Exynos5422

On 6/6/19 1:45 PM, Krzysztof Kozlowski wrote:

On Thu, 6 Jun 2019 at 12:38, Lukasz Luba [off-list ref] wrote:

quoted

Hi Krzysztof,

quoted

quoted

+/**
+ * exynos5_dmc_init_clks() - Initialize clocks needed for DMC operation.
+ * @dmc:       DMC structure containing needed fields
+ *
+ * Get the needed clocks defined in DT device, enable and set the right parents.
+ * Read current frequency and initialize the initial rate for governor.
+ */
+static int exynos5_dmc_init_clks(struct exynos5_dmc *dmc)
+{
+       int ret;
+       unsigned long target_volt = 0;
+       unsigned long target_rate = 0;
+
+       dmc->fout_spll = devm_clk_get(dmc->dev, "fout_spll");
+       if (IS_ERR(dmc->fout_spll))
+               return PTR_ERR(dmc->fout_spll);
+
+       dmc->fout_bpll = devm_clk_get(dmc->dev, "fout_bpll");
+       if (IS_ERR(dmc->fout_bpll))
+               return PTR_ERR(dmc->fout_bpll);
+
+       dmc->mout_mclk_cdrex = devm_clk_get(dmc->dev, "mout_mclk_cdrex");
+       if (IS_ERR(dmc->mout_mclk_cdrex))
+               return PTR_ERR(dmc->mout_mclk_cdrex);

You are not enabling this clock. It is divider so it is fine for him
but what about its parents? How can you guarantee that parents are
enabled?

It uses two parents in this configuration:
1. 'mout_bpll' which is set by the bootloader and is a default mode
2. 'mout_mx_mspll_ccore' which is used temporary as a 'bypass clock
source' only for the time when BPLL is changing it's settings

Do you suggest to put a call:

to make sure the parent is up and running?
OR just move the lines from the end of this function:
         clk_prepare_enable(dmc->fout_bpll);
         clk_prepare_enable(dmc->mout_bpll);
and add:
         ret = clk_set_parent(dmc->mout_mclk_cdrex, dmc->mout_bpll);
then call the clk_get_rate on 'mout_mclk_cdrex'

Ah, It's my mistake. I missed that later you enable its new parent. It's fine.

OK, so I will leave it as is and just fix the other stuff that you've
mentioned.

Regards,
Lukasz

Best regards,
Krzysztof