diff mbox series

[v2,4/7] riscv: Rework Sifive CLINT as UCLASS_TIMER driver

Message ID 20200729095636.1077054-5-seanga2@gmail.com
State Superseded
Delegated to: Andes
Headers show
Series riscv: Clean up timer drivers | expand

Commit Message

Sean Anderson July 29, 2020, 9:56 a.m. UTC
This converts the clint driver from the riscv-specific interface to be a
DM-based UCLASS_TIMER driver. We also need to re-add the initialization for
IPI back into the SPL code. This was previously implicitly done when the
timer was initialized. In addition, the SiFive DDR driver previously
implicitly depended on the CLINT to select REGMAP.

Unlike Andes's PLMT/PLIC (which AFAIK never have anything pass it a dtb),
the SiFive CLINT is part of the device tree passed in by qemu. This device
tree doesn't have a clocks or clock-frequency property on clint, so we need
to fall back on the timebase-frequency property. Perhaps in the future we
can get a clock-frequency property added to the qemu dtb.

Signed-off-by: Sean Anderson <seanga2@gmail.com>
---
This patch builds but has only been tested on the K210 and QEMU. It has NOT
been tested on a HiFive.

(no changes since v1)

 arch/riscv/Kconfig            |  4 --
 arch/riscv/lib/sifive_clint.c | 87 +++++++++++++++++++++++------------
 common/spl/spl_opensbi.c      |  5 ++
 drivers/ram/sifive/Kconfig    |  2 +
 4 files changed, 64 insertions(+), 34 deletions(-)

Comments

Bin Meng Aug. 20, 2020, 7:43 a.m. UTC | #1
On Wed, Jul 29, 2020 at 5:56 PM Sean Anderson <seanga2@gmail.com> wrote:
>
> This converts the clint driver from the riscv-specific interface to be a
> DM-based UCLASS_TIMER driver. We also need to re-add the initialization for
> IPI back into the SPL code. This was previously implicitly done when the
> timer was initialized. In addition, the SiFive DDR driver previously
> implicitly depended on the CLINT to select REGMAP.
>
> Unlike Andes's PLMT/PLIC (which AFAIK never have anything pass it a dtb),
> the SiFive CLINT is part of the device tree passed in by qemu. This device
> tree doesn't have a clocks or clock-frequency property on clint, so we need
> to fall back on the timebase-frequency property. Perhaps in the future we
> can get a clock-frequency property added to the qemu dtb.
>
> Signed-off-by: Sean Anderson <seanga2@gmail.com>
> ---
> This patch builds but has only been tested on the K210 and QEMU. It has NOT
> been tested on a HiFive.
>
> (no changes since v1)
>
>  arch/riscv/Kconfig            |  4 --
>  arch/riscv/lib/sifive_clint.c | 87 +++++++++++++++++++++++------------
>  common/spl/spl_opensbi.c      |  5 ++
>  drivers/ram/sifive/Kconfig    |  2 +
>  4 files changed, 64 insertions(+), 34 deletions(-)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index d9155b9bab..aaa3b833a5 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -155,10 +155,6 @@ config 64BIT
>  config SIFIVE_CLINT
>         bool
>         depends on RISCV_MMODE || SPL_RISCV_MMODE
> -       select REGMAP
> -       select SYSCON
> -       select SPL_REGMAP if SPL
> -       select SPL_SYSCON if SPL
>         help
>           The SiFive CLINT block holds memory-mapped control and status registers
>           associated with software and timer interrupts.
> diff --git a/arch/riscv/lib/sifive_clint.c b/arch/riscv/lib/sifive_clint.c
> index b9a2c649cc..3345a17ad2 100644
> --- a/arch/riscv/lib/sifive_clint.c
> +++ b/arch/riscv/lib/sifive_clint.c
> @@ -8,9 +8,9 @@
>   */
>
>  #include <common.h>
> +#include <clk.h>
>  #include <dm.h>
> -#include <regmap.h>
> -#include <syscon.h>
> +#include <timer.h>
>  #include <asm/io.h>
>  #include <asm/syscon.h>
>  #include <linux/err.h>
> @@ -24,35 +24,19 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> -int riscv_get_time(u64 *time)
> -{
> -       /* ensure timer register base has a sane value */
> -       riscv_init_ipi();
> -
> -       *time = readq((void __iomem *)MTIME_REG(gd->arch.clint));
> -
> -       return 0;
> -}
> -
> -int riscv_set_timecmp(int hart, u64 cmp)
> -{
> -       /* ensure timer register base has a sane value */
> -       riscv_init_ipi();
> -
> -       writeq(cmp, (void __iomem *)MTIMECMP_REG(gd->arch.clint, hart));
> -
> -       return 0;
> -}
> -
>  int riscv_init_ipi(void)
>  {
> -       if (!gd->arch.clint) {
> -               long *ret = syscon_get_first_range(RISCV_SYSCON_CLINT);
> +       int ret;
> +       struct udevice *dev;
>
> -               if (IS_ERR(ret))
> -                       return PTR_ERR(ret);
> -               gd->arch.clint = ret;
> -       }
> +       ret = uclass_get_device_by_driver(UCLASS_TIMER,
> +                                         DM_GET_DRIVER(sifive_clint), &dev);
> +       if (ret)
> +               return ret;
> +
> +       gd->arch.clint = dev_read_addr_ptr(dev);
> +       if (!gd->arch.clint)
> +               return -EINVAL;
>
>         return 0;
>  }
> @@ -78,14 +62,57 @@ int riscv_get_ipi(int hart, int *pending)
>         return 0;
>  }
>
> +static int sifive_clint_get_count(struct udevice *dev, u64 *count)
> +{
> +       *count = readq((void __iomem *)MTIME_REG(dev->priv));
> +
> +       return 0;
> +}
> +
> +static const struct timer_ops sifive_clint_ops = {
> +       .get_count = sifive_clint_get_count,
> +};
> +
> +static int sifive_clint_probe(struct udevice *dev)
> +{
> +       int ret;
> +       ofnode cpu;
> +       struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> +       u32 rate;
> +
> +       dev->priv = dev_read_addr_ptr(dev);
> +       if (!dev->priv)
> +               return -EINVAL;
> +
> +       /* Did we get our clock rate from the device tree? */
> +       if (uc_priv->clock_rate)
> +               return 0;
> +
> +       /* Fall back to timebase-frequency */
> +       cpu = ofnode_path("/cpus");
> +       if (!ofnode_valid(cpu))
> +               return -EINVAL;
> +
> +       ret = ofnode_read_u32(cpu, "timebase-frequency", &rate);
> +       if (ret)
> +               return ret;
> +
> +       log_warning("missing clocks or clock-frequency property, falling back on timebase-frequency\n");
> +       uc_priv->clock_rate = rate;
> +
> +       return 0;
> +}
> +
>  static const struct udevice_id sifive_clint_ids[] = {
> -       { .compatible = "riscv,clint0", .data = RISCV_SYSCON_CLINT },

RISCV_SYSCON_CLINT should be removed from syscon.h

> +       { .compatible = "riscv,clint0" },
>         { }
>  };
>
>  U_BOOT_DRIVER(sifive_clint) = {
>         .name           = "sifive_clint",
> -       .id             = UCLASS_SYSCON,
> +       .id             = UCLASS_TIMER,
>         .of_match       = sifive_clint_ids,
> +       .probe          = sifive_clint_probe,
> +       .ops            = &sifive_clint_ops,
>         .flags          = DM_FLAG_PRE_RELOC,
>  };
> diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
> index 14f335f75f..3440bc0294 100644
> --- a/common/spl/spl_opensbi.c
> +++ b/common/spl/spl_opensbi.c
> @@ -79,6 +79,11 @@ void spl_invoke_opensbi(struct spl_image_info *spl_image)
>         invalidate_icache_all();
>
>  #ifdef CONFIG_SPL_SMP
> +       /* Initialize the IPI before we use it */
> +       ret = riscv_init_ipi();
> +       if (ret)
> +               hang();

Why above is needed? This is already called in arch_cpu_init_dm() in
arch/riscv/cpu/cpu.c

> +
>         /*
>          * Start OpenSBI on all secondary harts and wait for acknowledgment.
>          *
> diff --git a/drivers/ram/sifive/Kconfig b/drivers/ram/sifive/Kconfig
> index 6aca22ab2a..31ad0a7e45 100644
> --- a/drivers/ram/sifive/Kconfig
> +++ b/drivers/ram/sifive/Kconfig
> @@ -9,5 +9,7 @@ config SIFIVE_FU540_DDR
>         bool "SiFive FU540 DDR driver"
>         depends on RAM_SIFIVE
>         default y if TARGET_SIFIVE_FU540
> +       select REGMAP
> +       select SPL_REGMAP if SPL

I don't understand why this driver depends on REGMAP?

>         help
>           This enables DDR support for the platforms based on SiFive FU540 SoC.
> --

Regards,
Bin
Sean Anderson Aug. 20, 2020, 10:23 a.m. UTC | #2
On 8/20/20 3:43 AM, Bin Meng wrote:
> On Wed, Jul 29, 2020 at 5:56 PM Sean Anderson <seanga2@gmail.com> wrote:
>>
>> This converts the clint driver from the riscv-specific interface to be a
>> DM-based UCLASS_TIMER driver. We also need to re-add the initialization for
>> IPI back into the SPL code. This was previously implicitly done when the
>> timer was initialized. In addition, the SiFive DDR driver previously
>> implicitly depended on the CLINT to select REGMAP.
>>
>> Unlike Andes's PLMT/PLIC (which AFAIK never have anything pass it a dtb),
>> the SiFive CLINT is part of the device tree passed in by qemu. This device
>> tree doesn't have a clocks or clock-frequency property on clint, so we need
>> to fall back on the timebase-frequency property. Perhaps in the future we
>> can get a clock-frequency property added to the qemu dtb.
>>
>> Signed-off-by: Sean Anderson <seanga2@gmail.com>
>> ---
>> This patch builds but has only been tested on the K210 and QEMU. It has NOT
>> been tested on a HiFive.
>>
>> (no changes since v1)
>>
>>  arch/riscv/Kconfig            |  4 --
>>  arch/riscv/lib/sifive_clint.c | 87 +++++++++++++++++++++++------------
>>  common/spl/spl_opensbi.c      |  5 ++
>>  drivers/ram/sifive/Kconfig    |  2 +
>>  4 files changed, 64 insertions(+), 34 deletions(-)
>>
>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>> index d9155b9bab..aaa3b833a5 100644
>> --- a/arch/riscv/Kconfig
>> +++ b/arch/riscv/Kconfig
>> @@ -155,10 +155,6 @@ config 64BIT
>>  config SIFIVE_CLINT
>>         bool
>>         depends on RISCV_MMODE || SPL_RISCV_MMODE
>> -       select REGMAP
>> -       select SYSCON
>> -       select SPL_REGMAP if SPL
>> -       select SPL_SYSCON if SPL
>>         help
>>           The SiFive CLINT block holds memory-mapped control and status registers
>>           associated with software and timer interrupts.
>> diff --git a/arch/riscv/lib/sifive_clint.c b/arch/riscv/lib/sifive_clint.c
>> index b9a2c649cc..3345a17ad2 100644
>> --- a/arch/riscv/lib/sifive_clint.c
>> +++ b/arch/riscv/lib/sifive_clint.c
>> @@ -8,9 +8,9 @@
>>   */
>>
>>  #include <common.h>
>> +#include <clk.h>
>>  #include <dm.h>
>> -#include <regmap.h>
>> -#include <syscon.h>
>> +#include <timer.h>
>>  #include <asm/io.h>
>>  #include <asm/syscon.h>
>>  #include <linux/err.h>
>> @@ -24,35 +24,19 @@
>>
>>  DECLARE_GLOBAL_DATA_PTR;
>>
>> -int riscv_get_time(u64 *time)
>> -{
>> -       /* ensure timer register base has a sane value */
>> -       riscv_init_ipi();
>> -
>> -       *time = readq((void __iomem *)MTIME_REG(gd->arch.clint));
>> -
>> -       return 0;
>> -}
>> -
>> -int riscv_set_timecmp(int hart, u64 cmp)
>> -{
>> -       /* ensure timer register base has a sane value */
>> -       riscv_init_ipi();
>> -
>> -       writeq(cmp, (void __iomem *)MTIMECMP_REG(gd->arch.clint, hart));
>> -
>> -       return 0;
>> -}
>> -
>>  int riscv_init_ipi(void)
>>  {
>> -       if (!gd->arch.clint) {
>> -               long *ret = syscon_get_first_range(RISCV_SYSCON_CLINT);
>> +       int ret;
>> +       struct udevice *dev;
>>
>> -               if (IS_ERR(ret))
>> -                       return PTR_ERR(ret);
>> -               gd->arch.clint = ret;
>> -       }
>> +       ret = uclass_get_device_by_driver(UCLASS_TIMER,
>> +                                         DM_GET_DRIVER(sifive_clint), &dev);
>> +       if (ret)
>> +               return ret;
>> +
>> +       gd->arch.clint = dev_read_addr_ptr(dev);
>> +       if (!gd->arch.clint)
>> +               return -EINVAL;
>>
>>         return 0;
>>  }
>> @@ -78,14 +62,57 @@ int riscv_get_ipi(int hart, int *pending)
>>         return 0;
>>  }
>>
>> +static int sifive_clint_get_count(struct udevice *dev, u64 *count)
>> +{
>> +       *count = readq((void __iomem *)MTIME_REG(dev->priv));
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct timer_ops sifive_clint_ops = {
>> +       .get_count = sifive_clint_get_count,
>> +};
>> +
>> +static int sifive_clint_probe(struct udevice *dev)
>> +{
>> +       int ret;
>> +       ofnode cpu;
>> +       struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
>> +       u32 rate;
>> +
>> +       dev->priv = dev_read_addr_ptr(dev);
>> +       if (!dev->priv)
>> +               return -EINVAL;
>> +
>> +       /* Did we get our clock rate from the device tree? */
>> +       if (uc_priv->clock_rate)
>> +               return 0;
>> +
>> +       /* Fall back to timebase-frequency */
>> +       cpu = ofnode_path("/cpus");
>> +       if (!ofnode_valid(cpu))
>> +               return -EINVAL;
>> +
>> +       ret = ofnode_read_u32(cpu, "timebase-frequency", &rate);
>> +       if (ret)
>> +               return ret;
>> +
>> +       log_warning("missing clocks or clock-frequency property, falling back on timebase-frequency\n");
>> +       uc_priv->clock_rate = rate;
>> +
>> +       return 0;
>> +}
>> +
>>  static const struct udevice_id sifive_clint_ids[] = {
>> -       { .compatible = "riscv,clint0", .data = RISCV_SYSCON_CLINT },
> 
> RISCV_SYSCON_CLINT should be removed from syscon.h

This can't be removed because the IPI driver still depends on it.

>> +       { .compatible = "riscv,clint0" },
>>         { }
>>  };
>>
>>  U_BOOT_DRIVER(sifive_clint) = {
>>         .name           = "sifive_clint",
>> -       .id             = UCLASS_SYSCON,
>> +       .id             = UCLASS_TIMER,
>>         .of_match       = sifive_clint_ids,
>> +       .probe          = sifive_clint_probe,
>> +       .ops            = &sifive_clint_ops,
>>         .flags          = DM_FLAG_PRE_RELOC,
>>  };
>> diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
>> index 14f335f75f..3440bc0294 100644
>> --- a/common/spl/spl_opensbi.c
>> +++ b/common/spl/spl_opensbi.c
>> @@ -79,6 +79,11 @@ void spl_invoke_opensbi(struct spl_image_info *spl_image)
>>         invalidate_icache_all();
>>
>>  #ifdef CONFIG_SPL_SMP
>> +       /* Initialize the IPI before we use it */
>> +       ret = riscv_init_ipi();
>> +       if (ret)
>> +               hang();
> 
> Why above is needed? This is already called in arch_cpu_init_dm() in
> arch/riscv/cpu/cpu.c

I believe I ran into problems with it not getting called in SPL when
testing. I can look into whether this is still needed.

> 
>> +
>>         /*
>>          * Start OpenSBI on all secondary harts and wait for acknowledgment.
>>          *
>> diff --git a/drivers/ram/sifive/Kconfig b/drivers/ram/sifive/Kconfig
>> index 6aca22ab2a..31ad0a7e45 100644
>> --- a/drivers/ram/sifive/Kconfig
>> +++ b/drivers/ram/sifive/Kconfig
>> @@ -9,5 +9,7 @@ config SIFIVE_FU540_DDR
>>         bool "SiFive FU540 DDR driver"
>>         depends on RAM_SIFIVE
>>         default y if TARGET_SIFIVE_FU540
>> +       select REGMAP
>> +       select SPL_REGMAP if SPL
> 
> I don't understand why this driver depends on REGMAP?

I don't either, but it fails to build without these two lines.
Previously, regmap was always enabled by the sifive clint. So now that
it no longer enables regmap, this dependency became apparent.

> 
>>         help
>>           This enables DDR support for the platforms based on SiFive FU540 SoC.
>> --
> 
> Regards,
> Bin
>
diff mbox series

Patch

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index d9155b9bab..aaa3b833a5 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -155,10 +155,6 @@  config 64BIT
 config SIFIVE_CLINT
 	bool
 	depends on RISCV_MMODE || SPL_RISCV_MMODE
-	select REGMAP
-	select SYSCON
-	select SPL_REGMAP if SPL
-	select SPL_SYSCON if SPL
 	help
 	  The SiFive CLINT block holds memory-mapped control and status registers
 	  associated with software and timer interrupts.
diff --git a/arch/riscv/lib/sifive_clint.c b/arch/riscv/lib/sifive_clint.c
index b9a2c649cc..3345a17ad2 100644
--- a/arch/riscv/lib/sifive_clint.c
+++ b/arch/riscv/lib/sifive_clint.c
@@ -8,9 +8,9 @@ 
  */
 
 #include <common.h>
+#include <clk.h>
 #include <dm.h>
-#include <regmap.h>
-#include <syscon.h>
+#include <timer.h>
 #include <asm/io.h>
 #include <asm/syscon.h>
 #include <linux/err.h>
@@ -24,35 +24,19 @@ 
 
 DECLARE_GLOBAL_DATA_PTR;
 
-int riscv_get_time(u64 *time)
-{
-	/* ensure timer register base has a sane value */
-	riscv_init_ipi();
-
-	*time = readq((void __iomem *)MTIME_REG(gd->arch.clint));
-
-	return 0;
-}
-
-int riscv_set_timecmp(int hart, u64 cmp)
-{
-	/* ensure timer register base has a sane value */
-	riscv_init_ipi();
-
-	writeq(cmp, (void __iomem *)MTIMECMP_REG(gd->arch.clint, hart));
-
-	return 0;
-}
-
 int riscv_init_ipi(void)
 {
-	if (!gd->arch.clint) {
-		long *ret = syscon_get_first_range(RISCV_SYSCON_CLINT);
+	int ret;
+	struct udevice *dev;
 
-		if (IS_ERR(ret))
-			return PTR_ERR(ret);
-		gd->arch.clint = ret;
-	}
+	ret = uclass_get_device_by_driver(UCLASS_TIMER,
+					  DM_GET_DRIVER(sifive_clint), &dev);
+	if (ret)
+		return ret;
+
+	gd->arch.clint = dev_read_addr_ptr(dev);
+	if (!gd->arch.clint)
+		return -EINVAL;
 
 	return 0;
 }
@@ -78,14 +62,57 @@  int riscv_get_ipi(int hart, int *pending)
 	return 0;
 }
 
+static int sifive_clint_get_count(struct udevice *dev, u64 *count)
+{
+	*count = readq((void __iomem *)MTIME_REG(dev->priv));
+
+	return 0;
+}
+
+static const struct timer_ops sifive_clint_ops = {
+	.get_count = sifive_clint_get_count,
+};
+
+static int sifive_clint_probe(struct udevice *dev)
+{
+	int ret;
+	ofnode cpu;
+	struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+	u32 rate;
+
+	dev->priv = dev_read_addr_ptr(dev);
+	if (!dev->priv)
+		return -EINVAL;
+
+	/* Did we get our clock rate from the device tree? */
+	if (uc_priv->clock_rate)
+		return 0;
+
+	/* Fall back to timebase-frequency */
+	cpu = ofnode_path("/cpus");
+	if (!ofnode_valid(cpu))
+		return -EINVAL;
+
+	ret = ofnode_read_u32(cpu, "timebase-frequency", &rate);
+	if (ret)
+		return ret;
+
+	log_warning("missing clocks or clock-frequency property, falling back on timebase-frequency\n");
+	uc_priv->clock_rate = rate;
+
+	return 0;
+}
+
 static const struct udevice_id sifive_clint_ids[] = {
-	{ .compatible = "riscv,clint0", .data = RISCV_SYSCON_CLINT },
+	{ .compatible = "riscv,clint0" },
 	{ }
 };
 
 U_BOOT_DRIVER(sifive_clint) = {
 	.name		= "sifive_clint",
-	.id		= UCLASS_SYSCON,
+	.id		= UCLASS_TIMER,
 	.of_match	= sifive_clint_ids,
+	.probe		= sifive_clint_probe,
+	.ops		= &sifive_clint_ops,
 	.flags		= DM_FLAG_PRE_RELOC,
 };
diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
index 14f335f75f..3440bc0294 100644
--- a/common/spl/spl_opensbi.c
+++ b/common/spl/spl_opensbi.c
@@ -79,6 +79,11 @@  void spl_invoke_opensbi(struct spl_image_info *spl_image)
 	invalidate_icache_all();
 
 #ifdef CONFIG_SPL_SMP
+	/* Initialize the IPI before we use it */
+	ret = riscv_init_ipi();
+	if (ret)
+		hang();
+
 	/*
 	 * Start OpenSBI on all secondary harts and wait for acknowledgment.
 	 *
diff --git a/drivers/ram/sifive/Kconfig b/drivers/ram/sifive/Kconfig
index 6aca22ab2a..31ad0a7e45 100644
--- a/drivers/ram/sifive/Kconfig
+++ b/drivers/ram/sifive/Kconfig
@@ -9,5 +9,7 @@  config SIFIVE_FU540_DDR
 	bool "SiFive FU540 DDR driver"
 	depends on RAM_SIFIVE
 	default y if TARGET_SIFIVE_FU540
+	select REGMAP
+	select SPL_REGMAP if SPL
 	help
 	  This enables DDR support for the platforms based on SiFive FU540 SoC.