[PATCH v1 0/2] display proper CPU frequency on hifive-unleashed

older
[PATCH] arm: imx6: configure NoC...

Sagar Shrikant Kadam

18 Feb 2020 18 Feb '20

5:13 p.m.

U-Boot cmd "cpu detail" shows wrong values, as the current cpu driver is fetching cpu_freq by reading the dt node property "clock-frequency". There should be a fallback mechanism to handle a case where this property if not present in DT it should be read from the prci driver.

This patch series uses the clk_get_rate method to fetch proper core clock frequency. The series is based on U-Boot commit f2a73d6867ef ("Merge tag 'u-boot-stm32-20200214' of https://gitlab.denx.de/u-boot/custodians/u-boot-stm")

Thanks to Vincent Chen vincent.chen@sifive.com for testing this patch.

=>-----------------------Log for reference------------------------ => cpu detail 0: cpu@1 rv64imafdc ID = 1, freq = 4.30 GHz <= Before patch 1: cpu@2 rv64imafdc ID = 2, freq = 4.30 GHz 2: cpu@3 rv64imafdc ID = 3, freq = 4.30 GHz 3: cpu@4 rv64imafdc ID = 4, freq = 4.30 GHz .................................................................

=> cpu detail 0: cpu@1 rv64imafdc ID = 1, freq = 999.100 MHz <= After patch 1: cpu@2 rv64imafdc ID = 2, freq = 999.100 MHz 2: cpu@3 rv64imafdc ID = 3, freq = 999.100 MHz 3: cpu@4 rv64imafdc ID = 4, freq = 999.100 MHz

Sagar Shrikant Kadam (2): fu540: prci: add request and free clock handlers cpu: clk: riscv: populate proper CPU core clk frequency

drivers/clk/sifive/fu540-prci.c | 24 ++++++++++++++++++++++++ drivers/cpu/riscv_cpu.c | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 62 insertions(+), 1 deletion(-)

-- 2.7.4

Show replies by date

Sagar Shrikant Kadam

18 Feb 18 Feb

5:13 p.m.

New subject: [PATCH v1 1/2] fu540: prci: add request and free clock handlers

Add handlers to check if a valid clock id is used to request clock by any driver using clk_request/clk_free API calls.

Signed-off-by: Sagar Shrikant Kadam sagar.kadam@sifive.com Tested-by: Vincent Chen vincent.chen@sifive.com --- drivers/clk/sifive/fu540-prci.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)

diff --git a/drivers/clk/sifive/fu540-prci.c b/drivers/clk/sifive/fu540-prci.c index 8847178..ecf29a0 100644 --- a/drivers/clk/sifive/fu540-prci.c +++ b/drivers/clk/sifive/fu540-prci.c @@ -580,6 +580,28 @@ static ulong sifive_fu540_prci_set_rate(struct clk *clk, ulong rate) return rate; }

+static int sifive_fu540_prci_clk_request(struct clk *clk) +{ + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, + clk->id); + + if (clk->id >= ARRAY_SIZE(__prci_init_clocks)) + return -EINVAL; + + return 0; +} + +static int sifive_fu540_prci_clk_free(struct clk *clk) +{ + debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev, + clk->id); + + if (clk->id >= ARRAY_SIZE(__prci_init_clocks)) + return -EINVAL; + + return 0; +} + static int sifive_fu540_prci_probe(struct udevice *dev) { int i, err; @@ -611,6 +633,8 @@ static int sifive_fu540_prci_probe(struct udevice *dev) static struct clk_ops sifive_fu540_prci_ops = { .set_rate = sifive_fu540_prci_set_rate, .get_rate = sifive_fu540_prci_get_rate, + .request = sifive_fu540_prci_clk_request, + .rfree = sifive_fu540_prci_clk_free, };

static const struct udevice_id sifive_fu540_prci_ids[] = {

-- 2.7.4

Sean Anderson

21 Feb 21 Feb

7:22 a.m.

New subject: [PATCH v1 1/2] fu540: prci: add request and free clock handlers

On 2/18/20 11:13 AM, Sagar Shrikant Kadam wrote:

...

+static int sifive_fu540_prci_clk_free(struct clk *clk) +{
debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev,
     clk->id);
if (clk->id >= ARRAY_SIZE(__prci_init_clocks))
return -EINVAL;
return 0;
+}

I don't think this function is necessary, since no struct clk should be passed to clk_free except one which was previously successfully requested.

...

static int sifive_fu540_prci_probe(struct udevice *dev) { int i, err; @@ -611,6 +633,8 @@ static int sifive_fu540_prci_probe(struct udevice *dev) static struct clk_ops sifive_fu540_prci_ops = { .set_rate = sifive_fu540_prci_set_rate, .get_rate = sifive_fu540_prci_get_rate,

.request = sifive_fu540_prci_clk_request,

.rfree = sifive_fu540_prci_clk_free,

};

static const struct udevice_id sifive_fu540_prci_ids[] = {

--Sean

Sagar Kadam

24 Feb 24 Feb

12:25 p.m.

New subject: [PATCH v1 1/2] fu540: prci: add request and free clock handlers

Hi Sean,

...

-----Original Message----- From: Sean Anderson seanga2@gmail.com Sent: Friday, February 21, 2020 11:53 AM To: Sagar Kadam sagar.kadam@sifive.com; u-boot@lists.denx.de Cc: lukma@denx.de; bmeng.cn@gmail.com; Anup.Patel@wdc.com; Paul Walmsley ( Sifive) paul.walmsley@sifive.com; Vincent Chen vincent.chen@sifive.com Subject: Re: [PATCH v1 1/2] fu540: prci: add request and free clock handlers

On 2/18/20 11:13 AM, Sagar Shrikant Kadam wrote:

...
+static int sifive_fu540_prci_clk_free(struct clk *clk) {
debug("%s(clk=%p) (dev=%p, id=%lu)\n", __func__, clk, clk->dev,
     clk->id);
if (clk->id >= ARRAY_SIZE(__prci_init_clocks))
return -EINVAL;
return 0;
+}
I don't think this function is necessary, since no struct clk should be passed to clk_free except one which was previously successfully requested.

Thanks for suggestion. I can drop this id check and keep the debug message as done in other similar drivers.

BR, Sagar Kadam

...

...
static int sifive_fu540_prci_probe(struct udevice *dev) { int i, err; @@ -611,6 +633,8 @@ static int sifive_fu540_prci_probe(struct udevice *dev) static struct clk_ops sifive_fu540_prci_ops = { .set_rate = sifive_fu540_prci_set_rate, .get_rate = sifive_fu540_prci_get_rate,

.request = sifive_fu540_prci_clk_request,

.rfree = sifive_fu540_prci_clk_free,

};

static const struct udevice_id sifive_fu540_prci_ids[] = {

--Sean

Sagar Shrikant Kadam

18 Feb 18 Feb

5:13 p.m.

New subject: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

Fetch core clock frequency from prci if clock-frequency for CPU nodes is missing in device tree, so that the cmd "#cpu detail" will show the correct CPU frequency.

U-Boot command "#cpu detail" is showing wrong frequency values. This patch fixes this issue by getting the core clock set in prci driver if clock-frequency is not added to CPU nodes in device tree. It is tested on HiFive Unleashed A00 board.

Signed-off-by: Sagar Shrikant Kadam sagar.kadam@sifive.com Tested-by: Vincent Chen vincent.chen@sifive.com --- drivers/cpu/riscv_cpu.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c index 28ad0aa..eb5491f 100644 --- a/drivers/cpu/riscv_cpu.c +++ b/drivers/cpu/riscv_cpu.c @@ -9,6 +9,8 @@ #include <errno.h> #include <dm/device-internal.h> #include <dm/lists.h> +#include <clk-uclass.h> +#include <dt-bindings/clock/sifive-fu540-prci.h>

DECLARE_GLOBAL_DATA_PTR;

@@ -25,11 +27,46 @@ static int riscv_cpu_get_desc(struct udevice *dev, char *buf, int size) return 0; }

+static ulong riscv_get_clkrate(int clk_index) +{ + int ret; + struct udevice *dev; + struct clk clk; + ulong rate; + + ret = uclass_get_device_by_driver(UCLASS_CLK, + DM_GET_DRIVER(sifive_fu540_prci), + &dev); + if (ret < 0) { + pr_err("%s: Could not get device driver\n", __func__); + return ret; + } + + clk.id = clk_index; + ret = clk_request(dev, &clk); + if (ret < 0) { + pr_err("%s: request to clock device failed...\n", __func__); + return ret; + } + + rate = clk_get_rate(&clk); + + clk_free(&clk); + + return rate; +} + static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info) { const char *mmu; + int ret;

- dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq); + ret = dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq); + if (ret) { + /* if clock-frequency is missing in DT, read it from prci */ + debug("Fetch core clk configured by prci\n"); + info->cpu_freq = riscv_get_clkrate(PRCI_CLK_COREPLL); + }

mmu = dev_read_string(dev, "mmu-type"); if (!mmu)

-- 2.7.4

Bin Meng

19 Feb 19 Feb

5:09 p.m.

New subject: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

+Sean Anderson

Hi Sagar,

On Wed, Feb 19, 2020 at 12:13 AM Sagar Shrikant Kadam sagar.kadam@sifive.com wrote:

...

Fetch core clock frequency from prci if clock-frequency for CPU nodes is missing in device tree, so that the cmd "#cpu detail" will show the correct CPU frequency.

U-Boot command "#cpu detail" is showing wrong frequency values. This patch fixes this issue by getting the core clock set in prci driver if clock-frequency is not added to CPU nodes in device tree. It is tested on HiFive Unleashed A00 board.

Signed-off-by: Sagar Shrikant Kadam sagar.kadam@sifive.com Tested-by: Vincent Chen vincent.chen@sifive.com

drivers/cpu/riscv_cpu.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c index 28ad0aa..eb5491f 100644 --- a/drivers/cpu/riscv_cpu.c +++ b/drivers/cpu/riscv_cpu.c @@ -9,6 +9,8 @@ #include <errno.h> #include <dm/device-internal.h> #include <dm/lists.h> +#include <clk-uclass.h> +#include <dt-bindings/clock/sifive-fu540-prci.h>

It's wrong to include a SoC specific header file in a generic driver.

...

DECLARE_GLOBAL_DATA_PTR;

@@ -25,11 +27,46 @@ static int riscv_cpu_get_desc(struct udevice *dev, char *buf, int size) return 0; }

+static ulong riscv_get_clkrate(int clk_index) +{

```
  int ret;
```
```
  struct udevice *dev;
```
```
  struct clk clk;
```
```
  ulong rate;
```

  ret = uclass_get_device_by_driver(UCLASS_CLK,

                                    DM_GET_DRIVER(sifive_fu540_prci),

                                    &dev);

```
  if (ret < 0) {
```

          pr_err("%s: Could not get device driver\n", __func__);

```
          return ret;
```
```
  }
```
```
  clk.id = clk_index;
```
```
  ret = clk_request(dev, &clk);
```
```
  if (ret < 0) {
```

          pr_err("%s: request to clock device failed...\n", __func__);

```
          return ret;
```
```
  }
```
```
  rate = clk_get_rate(&clk);
```
```
  clk_free(&clk);
```
```
  return rate;
```

static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info) { const char *mmu;

```
  int ret;
```

  dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);

  ret = dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);

```
  if (ret) {
```

          /* if clock-frequency is missing in DT, read it from prci */

          debug("Fetch core clk configured by prci\n");

          info->cpu_freq = riscv_get_clkrate(PRCI_CLK_COREPLL);

  }

  mmu = dev_read_string(dev, "mmu-type");
  if (!mmu)

What you were trying to do in this patch, I believe the following 2 patches already did it.

http://patchwork.ozlabs.org/patch/1236177/ http://patchwork.ozlabs.org/patch/1236180/

Regards, Bin

Sagar Kadam

21 Feb 21 Feb

6:59 a.m.

New subject: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

Hello Bin,

...

-----Original Message----- From: Bin Meng bmeng.cn@gmail.com Sent: Wednesday, February 19, 2020 9:40 PM To: Sagar Kadam sagar.kadam@sifive.com; Sean Anderson seanga2@gmail.com Cc: U-Boot Mailing List u-boot@lists.denx.de; Lukasz Majewski lukma@denx.de; Anup Patel Anup.Patel@wdc.com; Paul Walmsley ( Sifive) paul.walmsley@sifive.com; Vincent Chen vincent.chen@sifive.com Subject: Re: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

+Sean Anderson

Hi Sagar,

On Wed, Feb 19, 2020 at 12:13 AM Sagar Shrikant Kadam sagar.kadam@sifive.com wrote:

...
Fetch core clock frequency from prci if clock-frequency for CPU nodes is missing in device tree, so that the cmd "#cpu detail" will show the correct CPU frequency.

U-Boot command "#cpu detail" is showing wrong frequency values. This patch fixes this issue by getting the core clock set in prci driver if clock-frequency is not added to CPU nodes in device tree. It is tested on HiFive Unleashed A00 board.

Signed-off-by: Sagar Shrikant Kadam sagar.kadam@sifive.com Tested-by: Vincent Chen vincent.chen@sifive.com

drivers/cpu/riscv_cpu.c | 39

++++++++++++++++++++++++++++++++++++++-

...
1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c index 28ad0aa..eb5491f 100644 --- a/drivers/cpu/riscv_cpu.c +++ b/drivers/cpu/riscv_cpu.c @@ -9,6 +9,8 @@ #include <errno.h> #include <dm/device-internal.h> #include <dm/lists.h> +#include <clk-uclass.h> +#include <dt-bindings/clock/sifive-fu540-prci.h>

It's wrong to include a SoC specific header file in a generic driver.

Thanks for review. Ok. I will remove this.

...

DECLARE_GLOBAL_DATA_PTR;

@@ -25,11 +27,46 @@ static int riscv_cpu_get_desc(struct udevice *dev,

char *buf, int size)

...

    return 0;

}

+static ulong riscv_get_clkrate(int clk_index) +{

```
  int ret;
```
```
  struct udevice *dev;
```
```
  struct clk clk;
```
```
  ulong rate;
```

  ret = uclass_get_device_by_driver(UCLASS_CLK,

                                    DM_GET_DRIVER(sifive_fu540_prci),

                                    &dev);

```
  if (ret < 0) {
```

          pr_err("%s: Could not get device driver\n", __func__);

```
          return ret;
```
```
  }
```
```
  clk.id = clk_index;
```
```
  ret = clk_request(dev, &clk);
```
```
  if (ret < 0) {
```

          pr_err("%s: request to clock device failed...\n", __func__);

```
          return ret;
```
```
  }
```
```
  rate = clk_get_rate(&clk);
```
```
  clk_free(&clk);
```
```
  return rate;
```

static int riscv_cpu_get_info(struct udevice *dev, struct cpu_info *info) { const char *mmu;

```
  int ret;
```

  dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);

  ret = dev_read_u32(dev, "clock-frequency", (u32 *)&info->cpu_freq);

```
  if (ret) {
```

          /* if clock-frequency is missing in DT, read it from prci */

          debug("Fetch core clk configured by prci\n");

          info->cpu_freq = riscv_get_clkrate(PRCI_CLK_COREPLL);

  }

  mmu = dev_read_string(dev, "mmu-type");
  if (!mmu)

What you were trying to do in this patch, I believe the following 2 patches already did it.

http://patchwork.ozlabs.org/patch/1236177/ http://patchwork.ozlabs.org/patch/1236180/

Thanks for sharing the links. Unfortunately I didn’t come across it. I applied these two patches within my repo (assuming there are not extra dependencies) and would like to share my observation: The implementation in the patch links shared here read's clock dt property in clk_get_by_index. If the cpu dt node doesn't contain clock property it return's negative value and so the clk_get_rate here won't be be executed.

+ ret = clk_get_by_index(dev, 0, &clk); + if (!ret) { + ret = clk_get_rate(&clk);

Thus when I tested this on hifive unleashed the "cpu detail" showed frequency as 0 Hz.

Please correct me if I am wrong, but IMHO here we can check for negative return code [if (ret < 0)] instead of (!ret) and if "clocks" is missing in dt property then get the clock driver using uclass_get_device_by_driver->request the clock -> and get clock rate, just like below

- if (!ret) { + if (ret < 0) { + ret = uclass_get_device_by_driver(UCLASS_CLK, + DM_GET_DRIVER(sifive_fu540_prci), + &dev); + clk.id = 0; + ret = clk_request(dev, &clk); + if (ret < 0) { + pr_err("%s: request to clock device failed...\n", __func__); + return ret; + } +

Also there is missing "include linux/err.h" which is needed by IS_ERR_VALUE Please let me know if I can rebase and update my patches above the two patch's you pointed to.

...

Regards, Bin

Sean Anderson

7:17 a.m.

New subject: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

On 2/21/20 12:59 AM, Sagar Kadam wrote:

...

...
What you were trying to do in this patch, I believe the following 2 patches already did it.

http://patchwork.ozlabs.org/patch/1236177/ http://patchwork.ozlabs.org/patch/1236180/

Thanks for sharing the links. Unfortunately I didn’t come across it. I applied these two patches within my repo (assuming there are not extra dependencies) and would like to share my observation: The implementation in the patch links shared here read's clock dt property in clk_get_by_index. If the cpu dt node doesn't contain clock property it return's negative value and so the clk_get_rate here won't be be executed.
ret = clk_get_by_index(dev, 0, &clk);

if (!ret) {
ret = clk_get_rate(&clk);

This is working as designed. The idea is to use the clocks property if it exists and to fall back on clock-frequency otherwise.

...

Thus when I tested this on hifive unleashed the "cpu detail" showed frequency as 0 Hz.

This is because the cpu nodes in the hifive/fu540 device tree have neither a clock-frequency property or a clocks property.

...

Please correct me if I am wrong, but IMHO here we can check for negative return code [if (ret < 0)] instead of (!ret) and if "clocks" is missing in dt property then get the clock driver using uclass_get_device_by_driver->request the clock -> and get clock rate, just like below
  if (!ret) {
  if (ret < 0) {
          ret = uclass_get_device_by_driver(UCLASS_CLK,
                                            DM_GET_DRIVER(sifive_fu540_prci),
                                            &dev);

This is again adding board-specific code to a generic driver. Add a UCLASS_CLOCK driver if you want to support clocks. That way there is no need for code like this.

...

```
          clk.id = 0;
```

          ret = clk_request(dev, &clk);

```
          if (ret < 0) {
```

                  pr_err("%s: request to clock device failed...\n", __func__);

I belive pr_err is supported for linux compatibility. New code should use log_*. This is also probably debug-level and not err-level.

...

                  return ret;
          }
Also there is missing "include linux/err.h" which is needed by IS_ERR_VALUE

Yes, I noticed that when rebasing. It will be fixed in the next version of the series.

...

Please let me know if I can rebase and update my patches above the two patch's you pointed to.

...
Regards, Bin

--Sean

Sagar Kadam

24 Feb 24 Feb

12:31 p.m.

New subject: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

Hello Sean,

...

-----Original Message----- From: Sean Anderson seanga2@gmail.com Sent: Friday, February 21, 2020 11:48 AM To: Sagar Kadam sagar.kadam@sifive.com; Bin Meng bmeng.cn@gmail.com Cc: U-Boot Mailing List u-boot@lists.denx.de; Lukasz Majewski lukma@denx.de; Anup Patel Anup.Patel@wdc.com; Paul Walmsley ( Sifive) paul.walmsley@sifive.com; Vincent Chen vincent.chen@sifive.com Subject: Re: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

On 2/21/20 12:59 AM, Sagar Kadam wrote:

...
...
What you were trying to do in this patch, I believe the following 2 patches already did it.

http://patchwork.ozlabs.org/patch/1236177/ http://patchwork.ozlabs.org/patch/1236180/

Thanks for sharing the links. Unfortunately I didn’t come across it. I applied these two patches within my repo (assuming there are not extra dependencies) and would like to share my observation: The implementation in the patch links shared here read's clock dt property in clk_get_by_index. If the cpu dt node doesn't contain clock property it return's negative value and so the clk_get_rate here won't be be

executed.

...
ret = clk_get_by_index(dev, 0, &clk);

if (!ret) {
ret = clk_get_rate(&clk);
This is working as designed. The idea is to use the clocks property if it exists and to fall back on clock-frequency otherwise.

Thanks for clarifying.

...

...
Thus when I tested this on hifive unleashed the "cpu detail" showed

frequency as 0 Hz.

This is because the cpu nodes in the hifive/fu540 device tree have neither a clock-frequency property or a clocks property.

Yes, I will add clocks dt property.

...

...
Please correct me if I am wrong, but IMHO here we can check for negative return code [if (ret < 0)] instead of (!ret) and if "clocks" is missing in dt property then get the clock driver using uclass_get_device_by_driver->request the clock -> and get clock rate, just like below
  if (!ret) {
  if (ret < 0) {
          ret = uclass_get_device_by_driver(UCLASS_CLK,
                                            DM_GET_DRIVER(sifive_fu540_prci),
                                            &dev);
This is again adding board-specific code to a generic driver. Add a UCLASS_CLOCK driver if you want to support clocks. That way there is no need for code like this.

Thanks for suggestion. I will remove board-specific code from generic driver.

...

...
          clk.id = 0;
          ret = clk_request(dev, &clk);
          if (ret < 0) {
                  pr_err("%s: request to clock device
failed...\n", __func__);
I belive pr_err is supported for linux compatibility. New code should use log_*. This is also probably debug-level and not err-level.

Ok. I will replace pr_err with log_debug.

...

...
                  return ret;
          }
Also there is missing "include linux/err.h" which is needed by IS_ERR_VALUE
Yes, I noticed that when rebasing. It will be fixed in the next version of the series.

Thanks for updating.

BR, Sagar Kadam

...

...
Please let me know if I can rebase and update my patches above the two patch's you pointed to.

...
Regards, Bin

--Sean

1899

Age (days ago)

1905

Last active (days ago)

List overview

Download

8 comments

4 participants

tags (0)

participants (4)

Bin Meng
Sagar Kadam
Sagar Shrikant Kadam
Sean Anderson