diff mbox series

[regression,4.20-rc1,bisected] cpufreq fails to register on db820c

Message ID 20181106012928.GA32044@centauri.lan
State New
Headers show
Series [regression,4.20-rc1,bisected] cpufreq fails to register on db820c | expand

Commit Message

Niklas Cassel Nov. 6, 2018, 1:29 a.m. UTC 
Hello Bartosz,

I've run into a regression on v4.20-rc1, where cpufreq on dragonboard 820c
fails to register.

I've bisected the issue to:
e888d445ac33 ("nvmem: resolve cells from DT at registration time")

If I revert that patch, cpufreq starts working on dragonboard 820c again.


Before your patch:
[    5.052985] qcom_cpufreq_kryo_probe:110 speedbin_nvmem: ffff8000d4d51e00
[    5.053133] qcom_cpufreq_kryo_probe:120 speedbin: ffff8000d4d51d00
[    5.058895] qcom_cpufreq_kryo_probe:121 *speedbin: 0


After your patch:
[    4.791664] qcom_cpufreq_kryo_probe:110 speedbin_nvmem: ffff8000d472ed00
[    4.791815] qcom_cpufreq_kryo_probe:120 speedbin: ffff8000d451b300
[    4.797575] qcom_cpufreq_kryo_probe:121 *speedbin: 12


The prints are debug prints that I've added locally:



So it appears that the read from nvmem_cell_read()
gets another value after your commit.

This value is then given to dev_pm_opp_set_supported_hw(),
however, since this new value (12) from nvmem_cell_read()
doesn't have any matching opp-supported-hw in device tree,
cpufreq fails to register.

This error only happens when we read this new incorrect value
(12 instead of 0).
Any idea why nvmem_cell_read() returns a new value for the
same efuse?


Kind regards,
Niklas

Comments

Srinivas Kandagatla Nov. 6, 2018, 2:37 p.m. UTC | #1 
Hi Niklas,

thanks for reporting this..

I found the issue, DT support seems to be totally broken in NVMEM.
Will send this fix once you can confirm it works for you!

Can you try this patch!
---------------------->cut<--------------------------

Author: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date:   Tue Nov 6 14:32:30 2018 +0000

     nvmem: core: fix of_nvmem_cell_get()

     NVMEM DT support seems to be totally broken after
     e888d445ac33 ("nvmem: resolve cells from DT at registration time")
     Fix this!

     Fixes: e888d445ac33 ("nvmem: resolve cells from DT at registration 
time")
     Reported-by: Niklas Cassel <niklas.cassel@linaro.org>
     Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>


diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 9b18ce90f907..7ae8072f56ae 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -44,6 +44,7 @@ struct nvmem_cell {
         int                     bytes;
         int                     bit_offset;
         int                     nbits;
+       struct device_node      *np;
         struct nvmem_device     *nvmem;
         struct list_head        node;
  };
@@ -530,6 +531,7 @@ static int nvmem_add_cells_from_of(struct 
nvmem_device *nvmem)
                         return -ENOMEM;

                 cell->nvmem = nvmem;
+               cell->np = child;
                 cell->offset = be32_to_cpup(addr++);
                 cell->bytes = be32_to_cpup(addr);
                 cell->name = kasprintf(GFP_KERNEL, "%pOFn", child);
@@ -960,14 +962,13 @@ nvmem_cell_get_from_lookup(struct device *dev, 
const char *con_id)

  #if IS_ENABLED(CONFIG_OF)
  static struct nvmem_cell *
-nvmem_find_cell_by_index(struct nvmem_device *nvmem, int index)
+nvmem_find_cell_by_node(struct nvmem_device *nvmem, struct device_node *np)
  {
         struct nvmem_cell *cell = NULL;
-       int i = 0;

         mutex_lock(&nvmem_mutex);
         list_for_each_entry(cell, &nvmem->cells, node) {
-               if (index == i++)
+               if (np == cell->np)
                         break;
         }
         mutex_unlock(&nvmem_mutex);
@@ -1001,7 +1002,6 @@ struct nvmem_cell *of_nvmem_cell_get(struct 
device_node *np, const char *id)
         cell_np = of_parse_phandle(np, "nvmem-cells", index);
         if (!cell_np)
                 return ERR_PTR(-EINVAL);
-
         nvmem_np = of_get_next_parent(cell_np);
         if (!nvmem_np)
                 return ERR_PTR(-EINVAL);
@@ -1011,7 +1011,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct 
device_node *np, const char *id)
         if (IS_ERR(nvmem))
                 return ERR_CAST(nvmem);

-       cell = nvmem_find_cell_by_index(nvmem, index);
+       cell = nvmem_find_cell_by_node(nvmem, cell_np);
         if (!cell) {
                 __nvmem_device_put(nvmem);
                 return ERR_PTR(-ENOENT);

---------------------->cut<--------------------------
On 06/11/18 01:29, Niklas Cassel wrote:
> Hello Bartosz,

> 

> I've run into a regression on v4.20-rc1, where cpufreq on dragonboard 820c

> fails to register.

> 

> I've bisected the issue to:

> e888d445ac33 ("nvmem: resolve cells from DT at registration time")

> 

> If I revert that patch, cpufreq starts working on dragonboard 820c again.

> 

> 

> Before your patch:

> [    5.052985] qcom_cpufreq_kryo_probe:110 speedbin_nvmem: ffff8000d4d51e00

> [    5.053133] qcom_cpufreq_kryo_probe:120 speedbin: ffff8000d4d51d00

> [    5.058895] qcom_cpufreq_kryo_probe:121 *speedbin: 0

> 

> 

> After your patch:

> [    4.791664] qcom_cpufreq_kryo_probe:110 speedbin_nvmem: ffff8000d472ed00

> [    4.791815] qcom_cpufreq_kryo_probe:120 speedbin: ffff8000d451b300

> [    4.797575] qcom_cpufreq_kryo_probe:121 *speedbin: 12

> 

> 

> The prints are debug prints that I've added locally:

> 

> --- a/drivers/cpufreq/qcom-cpufreq-kryo.c

> +++ b/drivers/cpufreq/qcom-cpufreq-kryo.c

> @@ -107,6 +107,7 @@ static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)

>          }

>   

>          speedbin_nvmem = of_nvmem_cell_get(np, NULL);

> +       pr_err("%s:%d speedbin_nvmem: %lx\n", __func__, __LINE__, speedbin_nvmem);

>          of_node_put(np);

>          if (IS_ERR(speedbin_nvmem)) {

>                  if (PTR_ERR(speedbin_nvmem) != -EPROBE_DEFER)

> @@ -116,6 +117,8 @@ static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)

>          }

>   

>          speedbin = nvmem_cell_read(speedbin_nvmem, &len);

> +       pr_err("%s:%d speedbin: %lx\n", __func__, __LINE__, speedbin);

> +       pr_err("%s:%d *speedbin: %u\n", __func__, __LINE__, (unsigned int)(*speedbin));

>          nvmem_cell_put(speedbin_nvmem);

>          if (IS_ERR(speedbin))

>                  return PTR_ERR(speedbin);

> 

> 

> So it appears that the read from nvmem_cell_read()

> gets another value after your commit.

> 

> This value is then given to dev_pm_opp_set_supported_hw(),

> however, since this new value (12) from nvmem_cell_read()

> doesn't have any matching opp-supported-hw in device tree,

> cpufreq fails to register.

> 

> This error only happens when we read this new incorrect value

> (12 instead of 0).

> Any idea why nvmem_cell_read() returns a new value for the

> same efuse?

> 

> 

> Kind regards,

> Niklas

>
Niklas Cassel Nov. 6, 2018, 3:26 p.m. UTC | #2 
On Tue, Nov 06, 2018 at 02:37:57PM +0000, Srinivas Kandagatla wrote:
> Hi Niklas,

> 

> thanks for reporting this..

> 

> I found the issue, DT support seems to be totally broken in NVMEM.

> Will send this fix once you can confirm it works for you!

> 

> Can you try this patch!

> ---------------------->cut<--------------------------

> 

> Author: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> Date:   Tue Nov 6 14:32:30 2018 +0000

> 

>     nvmem: core: fix of_nvmem_cell_get()

> 

>     NVMEM DT support seems to be totally broken after

>     e888d445ac33 ("nvmem: resolve cells from DT at registration time")

>     Fix this!

> 

>     Fixes: e888d445ac33 ("nvmem: resolve cells from DT at registration

> time")

>     Reported-by: Niklas Cassel <niklas.cassel@linaro.org>

>     Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

> 

> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c

> index 9b18ce90f907..7ae8072f56ae 100644

> --- a/drivers/nvmem/core.c

> +++ b/drivers/nvmem/core.c

> @@ -44,6 +44,7 @@ struct nvmem_cell {

>         int                     bytes;

>         int                     bit_offset;

>         int                     nbits;

> +       struct device_node      *np;

>         struct nvmem_device     *nvmem;

>         struct list_head        node;

>  };

> @@ -530,6 +531,7 @@ static int nvmem_add_cells_from_of(struct nvmem_device

> *nvmem)

>                         return -ENOMEM;

> 

>                 cell->nvmem = nvmem;

> +               cell->np = child;

>                 cell->offset = be32_to_cpup(addr++);

>                 cell->bytes = be32_to_cpup(addr);

>                 cell->name = kasprintf(GFP_KERNEL, "%pOFn", child);

> @@ -960,14 +962,13 @@ nvmem_cell_get_from_lookup(struct device *dev, const

> char *con_id)

> 

>  #if IS_ENABLED(CONFIG_OF)

>  static struct nvmem_cell *

> -nvmem_find_cell_by_index(struct nvmem_device *nvmem, int index)

> +nvmem_find_cell_by_node(struct nvmem_device *nvmem, struct device_node *np)

>  {

>         struct nvmem_cell *cell = NULL;

> -       int i = 0;

> 

>         mutex_lock(&nvmem_mutex);

>         list_for_each_entry(cell, &nvmem->cells, node) {

> -               if (index == i++)

> +               if (np == cell->np)

>                         break;

>         }

>         mutex_unlock(&nvmem_mutex);

> @@ -1001,7 +1002,6 @@ struct nvmem_cell *of_nvmem_cell_get(struct

> device_node *np, const char *id)

>         cell_np = of_parse_phandle(np, "nvmem-cells", index);

>         if (!cell_np)

>                 return ERR_PTR(-EINVAL);

> -

>         nvmem_np = of_get_next_parent(cell_np);

>         if (!nvmem_np)

>                 return ERR_PTR(-EINVAL);

> @@ -1011,7 +1011,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct

> device_node *np, const char *id)

>         if (IS_ERR(nvmem))

>                 return ERR_CAST(nvmem);

> 

> -       cell = nvmem_find_cell_by_index(nvmem, index);

> +       cell = nvmem_find_cell_by_node(nvmem, cell_np);

>         if (!cell) {

>                 __nvmem_device_put(nvmem);

>                 return ERR_PTR(-ENOENT);

> 


Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
diff mbox series

Patch

--- a/drivers/cpufreq/qcom-cpufreq-kryo.c
+++ b/drivers/cpufreq/qcom-cpufreq-kryo.c
@@ -107,6 +107,7 @@  static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)
        }
 
        speedbin_nvmem = of_nvmem_cell_get(np, NULL);
+       pr_err("%s:%d speedbin_nvmem: %lx\n", __func__, __LINE__, speedbin_nvmem);
        of_node_put(np);
        if (IS_ERR(speedbin_nvmem)) {
                if (PTR_ERR(speedbin_nvmem) != -EPROBE_DEFER)
@@ -116,6 +117,8 @@  static int qcom_cpufreq_kryo_probe(struct platform_device *pdev)
        }
 
        speedbin = nvmem_cell_read(speedbin_nvmem, &len);
+       pr_err("%s:%d speedbin: %lx\n", __func__, __LINE__, speedbin);
+       pr_err("%s:%d *speedbin: %u\n", __func__, __LINE__, (unsigned int)(*speedbin));
        nvmem_cell_put(speedbin_nvmem);
        if (IS_ERR(speedbin))
                return PTR_ERR(speedbin);