Basic StarFive JH7110 RISC-V SoC support

From: Emil Renner Berthing <[email protected]>

Add device tree bindings for the StarFive JH7110 RISC-V SoC and the VisionFive2 board for it.

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

---
 Documentation/devicetree/bindings/riscv/starfive.yaml | 3 +++
 1 file changed, 3 insertions(+)

Documentation/devicetree/bindings/riscv/starfive.yaml +0 -0

index 5b36243fd674..543be573921d 100644

Quoted 1 line · Hal Feng

From: Emil Renner Berthing <[email protected]>

Drop last "bindings" from subject, it's redundant.

Quoted 11 lines

Add device tree bindings for the StarFive JH7110 RISC-V SoC and the VisionFive2 board for it.

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

---
 Documentation/devicetree/bindings/riscv/starfive.yaml | 3 +++
 1 file changed, 3 insertions(+)

Documentation/devicetree/bindings/riscv/starfive.yaml properties: · line 23

Quoted 8 lines

Documentation/devicetree/bindings/riscv/starfive.yaml +0 -0

index 5b36243fd674..543be573921d 100644
@@ -21,6 +21,9 @@ properties:
       - items:
           - const: beagle,beaglev-starlight-jh7100-r0
           - const: starfive,jh7100

Blank line.

From: Emil Renner Berthing <[email protected]>

Add compatible string for the StarFive JH7110 clint.

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

From: Emil Renner Berthing <[email protected]>

Add compatible string for StarFive JH7110 plic.

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

From: Emil Renner Berthing <[email protected]>

This cache controller is also used on the StarFive JH7100 and JH7110 SoCs.

From: Emil Renner Berthing <[email protected]>

This cache controller is also used on the StarFive JH7100 and JH7110 SoCs.

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

---
 Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml | 4 ++++
 1 file changed, 4 insertions(+)

Documentation/devicetree/bindings/riscv/sifive-l2-cache.yaml +4 -0

index ca3b9be58058..ba29ecfd3a92 100644
@@ -24,6 +24,8 @@ select:
         enum:
           - sifive,fu540-c000-ccache
           - sifive,fu740-c000-ccache
+          - starfive,jh7100-ccache
+          - starfive,jh7110-ccache
 
   required:
     - compatible

@@ -35,6 +37,8 @@ properties:
           - enum:
               - sifive,fu540-c000-ccache
               - sifive,fu740-c000-ccache
+              - starfive,jh7100-ccache
+              - starfive,jh7110-ccache
           - const: cache
       - items:
           - const: microchip,mpfs-ccache

Signature · 7 lines

2.17.1


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 4 lines · Hal Feng

From: Emil Renner Berthing <[email protected]>

This cache controller is also used on the StarFive JH7100 and JH7110 SoCs.

Ditto this patch, hopefully [0] will have landed as 6.1 material before you get around to an actual v2.

Thanks,
Conor

0 - https://lore.kernel.org/linux-riscv/[email protected]/

Quoted 9 lines · Conor Dooley

Quoted 4 lines · Hal Feng

From: Emil Renner Berthing <[email protected]>

This cache controller is also used on the StarFive JH7100 and JH7110 SoCs.

Ditto this patch, hopefully [0] will have landed as 6.1 material before you get around to an actual v2.

Quoted 4 lines

Thanks,
Conor

0 - https://lore.kernel.org/linux-riscv/[email protected]/

Also, the l2 cache is being proprely named the ccache (composable cache) as it is not necessarily an L2 cache.

From: Emil Renner Berthing <[email protected]>

This converts the driver to use the builtin_platform_driver_probe macro to initialize the driver. This macro ends up calling device_initcall as was used previously, but also allocates a platform device which gives us access to much nicer APIs such as platform_ioremap_resource, platform_get_irq and dev_err_probe.

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

---
 drivers/soc/sifive/sifive_l2_cache.c | 79 ++++++++++++++--------------
 1 file changed, 40 insertions(+), 39 deletions(-)

drivers/soc/sifive/sifive_l2_cache.c +40 -39

index 59640a1d0b28..010d612f7420 100644
@@ -7,9 +7,9 @@
  */
 #include <linux/debugfs.h>
 #include <linux/interrupt.h>
-#include <linux/of_irq.h>
-#include <linux/of_address.h>
-#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
 #include <asm/cacheinfo.h>
 #include <soc/sifive/sifive_l2_cache.h>
 

@@ -96,12 +96,6 @@ static void l2_config_read(void)
 	pr_info("L2CACHE: Index of the largest way enabled: %d\n", regval);
 }
 
-static const struct of_device_id sifive_l2_ids[] = {
-	{ .compatible = "sifive,fu540-c000-ccache" },
-	{ .compatible = "sifive,fu740-c000-ccache" },
-	{ /* end of table */ },
-};
-
 static ATOMIC_NOTIFIER_HEAD(l2_err_chain);
 
 int register_sifive_l2_error_notifier(struct notifier_block *nb)

@@ -192,36 +186,29 @@ static irqreturn_t l2_int_handler(int irq, void *device)
 	return IRQ_HANDLED;
 }
 
-static int __init sifive_l2_init(void)
+static int __init sifive_l2_probe(struct platform_device *pdev)
 {
-	struct device_node *np;
-	struct resource res;
-	int i, rc, intr_num;
-
-	np = of_find_matching_node(NULL, sifive_l2_ids);
-	if (!np)
-		return -ENODEV;
-
-	if (of_address_to_resource(np, 0, &res))
-		return -ENODEV;
-
-	l2_base = ioremap(res.start, resource_size(&res));
-	if (!l2_base)
-		return -ENOMEM;
-
-	intr_num = of_property_count_u32_elems(np, "interrupts");
-	if (!intr_num) {
-		pr_err("L2CACHE: no interrupts property\n");
-		return -ENODEV;
-	}
-
-	for (i = 0; i < intr_num; i++) {
-		g_irq[i] = irq_of_parse_and_map(np, i);
-		rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL);
-		if (rc) {
-			pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]);
-			return rc;
-		}
+	struct device *dev = &pdev->dev;
+	int nirqs;
+	int ret;
+	int i;
+
+	l2_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(l2_base))
+		return PTR_ERR(l2_base);
+
+	nirqs = platform_irq_count(pdev);
+	if (nirqs <= 0)
+		return dev_err_probe(dev, -ENODEV, "no interrupts\n");
+
+	for (i = 0; i < nirqs; i++) {
+		g_irq[i] = platform_get_irq(pdev, i);
+		if (g_irq[i] < 0)
+			return g_irq[i];
+
+		ret = devm_request_irq(dev, g_irq[i], l2_int_handler, 0, pdev->name, NULL);
+		if (ret)
+			return dev_err_probe(dev, ret, "Could not request IRQ %d\n", g_irq[i]);
 	}
 
 	l2_config_read();

@@ -234,4 +221,18 @@ static int __init sifive_l2_init(void)
 #endif
 	return 0;
 }
-device_initcall(sifive_l2_init);
+
+static const struct of_device_id sifive_l2_match[] = {
+	{ .compatible = "sifive,fu540-c000-ccache" },
+	{ .compatible = "sifive,fu740-c000-ccache" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver sifive_l2_driver = {
+	.driver = {
+		.name = "sifive_l2_cache",
+		.of_match_table = sifive_l2_match,
+		.suppress_bind_attrs = true,
+	},
+};
+builtin_platform_driver_probe(sifive_l2_driver, sifive_l2_probe);

Signature · 7 lines

2.17.1


_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

From: Emil Renner Berthing <[email protected]>

This converts the driver to use the builtin_platform_driver_probe macro to initialize the driver. This macro ends up calling device_initcall as was used previously, but also allocates a platform device which gives us access to much nicer APIs such as platform_ioremap_resource, platform_get_irq and dev_err_probe.

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

---
  drivers/soc/sifive/sifive_l2_cache.c | 79 ++++++++++++++--------------
  1 file changed, 40 insertions(+), 39 deletions(-)

drivers/soc/sifive/sifive_l2_cache.c +6 -38

index 59640a1d0b28..010d612f7420 100644
@@ -7,9 +7,9 @@
   */
  #include <linux/debugfs.h>
  #include <linux/interrupt.h>
-#include <linux/of_irq.h>
-#include <linux/of_address.h>
-#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
  #include <asm/cacheinfo.h>
  #include <soc/sifive/sifive_l2_cache.h>
 

@@ -96,12 +96,6 @@ static void l2_config_read(void)
  	pr_info("L2CACHE: Index of the largest way enabled: %d\n", regval);
  }
 
-static const struct of_device_id sifive_l2_ids[] = {
-	{ .compatible = "sifive,fu540-c000-ccache" },
-	{ .compatible = "sifive,fu740-c000-ccache" },
-	{ /* end of table */ },
-};
-
  static ATOMIC_NOTIFIER_HEAD(l2_err_chain);
 
  int register_sifive_l2_error_notifier(struct notifier_block *nb)

@@ -192,36 +186,29 @@ static irqreturn_t l2_int_handler(int irq, void *device)
  	return IRQ_HANDLED;
  }
 
-static int __init sifive_l2_init(void)
+static int __init sifive_l2_probe(struct platform_device *pdev)
  {
-	struct device_node *np;
-	struct resource res;
-	int i, rc, intr_num;
-
-	np = of_find_matching_node(NULL, sifive_l2_ids);
-	if (!np)
-		return -ENODEV;
-
-	if (of_address_to_resource(np, 0, &res))
-		return -ENODEV;
-
-	l2_base = ioremap(res.start, resource_size(&res));
-	if (!l2_base)
-		return -ENOMEM;
-
-	intr_num = of_property_count_u32_elems(np, "interrupts");
-	if (!intr_num) {
-		pr_err("L2CACHE: no interrupts property\n");
-		return -ENODEV;
-	}
-
-	for (i = 0; i < intr_num; i++) {
-		g_irq[i] = irq_of_parse_and_map(np, i);
-		rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL);
-		if (rc) {
-			pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]);
-			return rc;
-		}
+	struct device *dev = &pdev->dev;
+	int nirqs;

drivers/soc/sifive/sifive_l2_cache.c +21 -1

+		if (g_irq[i] < 0)
+			return g_irq[i];
+
+		ret = devm_request_irq(dev, g_irq[i], l2_int_handler, 0, pdev->name, NULL);
+		if (ret)
+			return dev_err_probe(dev, ret, "Could not request IRQ %d\n", g_irq[i]);
  	}
 
  	l2_config_read();

@@ -234,4 +221,18 @@ static int __init sifive_l2_init(void)
  #endif
  	return 0;
  }
-device_initcall(sifive_l2_init);
+
+static const struct of_device_id sifive_l2_match[] = {
+	{ .compatible = "sifive,fu540-c000-ccache" },
+	{ .compatible = "sifive,fu740-c000-ccache" },
+	{ /* sentinel */ }
+};
+
+static struct platform_driver sifive_l2_driver = {
+	.driver = {
+		.name = "sifive_l2_cache",
+		.of_match_table = sifive_l2_match,
+		.suppress_bind_attrs = true,
+	},
+};
+builtin_platform_driver_probe(sifive_l2_driver, sifive_l2_probe);

Quoted 7 lines · Hal Feng

From: Emil Renner Berthing <[email protected]>

This converts the driver to use the builtin_platform_driver_probe macro to initialize the driver. This macro ends up calling device_initcall as was used previously, but also allocates a platform device which gives us access to much nicer APIs such as platform_ioremap_resource, platform_get_irq and dev_err_probe.

This is useful, but also there are other changes currently being sorted out by Zong Li (cc'd into this message) which have already been reviewed and are hopefully queued for the next kernel release.

Quoted 2 lines

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

Quoted 70 lines

  drivers/soc/sifive/sifive_l2_cache.c | 79 ++++++++++++++--------------
  1 file changed, 40 insertions(+), 39 deletions(-)

drivers/soc/sifive/sifive_l2_cache.c +6 -38

index 59640a1d0b28..010d612f7420 100644
@@ -7,9 +7,9 @@
   */
  #include <linux/debugfs.h>
  #include <linux/interrupt.h>
-#include <linux/of_irq.h>
-#include <linux/of_address.h>
-#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
  #include <asm/cacheinfo.h>
  #include <soc/sifive/sifive_l2_cache.h>
 

@@ -96,12 +96,6 @@ static void l2_config_read(void)
      pr_info("L2CACHE: Index of the largest way enabled: %d\n", regval);
  }
 
-static const struct of_device_id sifive_l2_ids[] = {
-     { .compatible = "sifive,fu540-c000-ccache" },
-     { .compatible = "sifive,fu740-c000-ccache" },
-     { /* end of table */ },
-};
-
  static ATOMIC_NOTIFIER_HEAD(l2_err_chain);
 
  int register_sifive_l2_error_notifier(struct notifier_block *nb)

@@ -192,36 +186,29 @@ static irqreturn_t l2_int_handler(int irq, void *device)
      return IRQ_HANDLED;
  }
 
-static int __init sifive_l2_init(void)
+static int __init sifive_l2_probe(struct platform_device *pdev)
  {
-     struct device_node *np;
-     struct resource res;
-     int i, rc, intr_num;
-
-     np = of_find_matching_node(NULL, sifive_l2_ids);
-     if (!np)
-             return -ENODEV;
-
-     if (of_address_to_resource(np, 0, &res))
-             return -ENODEV;
-
-     l2_base = ioremap(res.start, resource_size(&res));
-     if (!l2_base)
-             return -ENOMEM;
-
-     intr_num = of_property_count_u32_elems(np, "interrupts");
-     if (!intr_num) {
-             pr_err("L2CACHE: no interrupts property\n");
-             return -ENODEV;
-     }
-
-     for (i = 0; i < intr_num; i++) {
-             g_irq[i] = irq_of_parse_and_map(np, i);
-             rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL);
-             if (rc) {
-                     pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]);
-                     return rc;
-             }
+     struct device *dev = &pdev->dev;
+     int nirqs;

drivers/soc/sifive/sifive_l2_cache.c static irqreturn_t l2_int_handler(int irq, void *device) · line 202

Quoted 10 lines

drivers/soc/sifive/sifive_l2_cache.c +10 -0

+     int ret;
+     int i;
+
+     l2_base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(l2_base))
+             return PTR_ERR(l2_base);
+
+     nirqs = platform_irq_count(pdev);
+     if (nirqs <= 0)
+             return dev_err_probe(dev, -ENODEV, "no interrupts\n");

I wonder if zero irqs is an actual issue here?

drivers/soc/sifive/sifive_l2_cache.c static irqreturn_t l2_int_handler(int irq, void *device) · line 205

Quoted 2 lines

drivers/soc/sifive/sifive_l2_cache.c +2 -0

+     for (i = 0; i < nirqs; i++) {
+             g_irq[i] = platform_get_irq(pdev, i);

I wonder if we need to keep g_irq[] around now? Is it going to be useful in the future?

Quoted 19 lines

drivers/soc/sifive/sifive_l2_cache.c +11 -1

+             if (g_irq[i] < 0)
+                     return g_irq[i];
+
+             ret = devm_request_irq(dev, g_irq[i], l2_int_handler, 0, pdev->name, NULL);
+             if (ret)
+                     return dev_err_probe(dev, ret, "Could not request IRQ %d\n", g_irq[i]);
      }
 
      l2_config_read();

@@ -234,4 +221,18 @@ static int __init sifive_l2_init(void)
  #endif
      return 0;
  }
-device_initcall(sifive_l2_init);
+
+static const struct of_device_id sifive_l2_match[] = {
+     { .compatible = "sifive,fu540-c000-ccache" },
+     { .compatible = "sifive,fu740-c000-ccache" },
+     { /* sentinel */ }

drivers/soc/sifive/sifive_l2_cache.c static int __init sifive_l2_init(void) · line 238

Quoted 10 lines

drivers/soc/sifive/sifive_l2_cache.c +10 -0

+};
+
+static struct platform_driver sifive_l2_driver = {
+     .driver = {
+             .name = "sifive_l2_cache",
+             .of_match_table = sifive_l2_match,
+             .suppress_bind_attrs = true,
+     },
+};
+builtin_platform_driver_probe(sifive_l2_driver, sifive_l2_probe);

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 10 lines · Ben Dooks

Quoted 7 lines · Hal Feng

From: Emil Renner Berthing <[email protected]>

This converts the driver to use the builtin_platform_driver_probe macro to initialize the driver. This macro ends up calling device_initcall as was used previously, but also allocates a platform device which gives us access to much nicer APIs such as platform_ioremap_resource, platform_get_irq and dev_err_probe.

Quoted 6 lines

This is useful, but also there are other changes currently being sorted out by Zong Li (cc'd into this message) which have already been reviewed and are hopefully queued for the next kernel release.

Quoted 2 lines

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

I'm ok with something like this being merged, but please note that if we ever want to support the JH7100 which uses registers in this peripheral to flush the cache for its non-coherent DMAs then this driver needs to be loaded before other peripherals or we will trigger the 2nd warning in arch/riscv/mm/dma-noncoherent.c. I'm not sure we can do that when it's a platform driver. See this patch for an alternative to support the JH71x0s:
https://github.com/esmil/linux/commit/9c5b29da56ae29159c9572c5bb195fe3a1b535c5

/Emil

Quoted 116 lines

Quoted 70 lines

   drivers/soc/sifive/sifive_l2_cache.c | 79 ++++++++++++++--------------
   1 file changed, 40 insertions(+), 39 deletions(-)

drivers/soc/sifive/sifive_l2_cache.c +6 -38

index 59640a1d0b28..010d612f7420 100644
@@ -7,9 +7,9 @@
    */
   #include <linux/debugfs.h>
   #include <linux/interrupt.h>
-#include <linux/of_irq.h>
-#include <linux/of_address.h>
-#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
   #include <asm/cacheinfo.h>
   #include <soc/sifive/sifive_l2_cache.h>
 

@@ -96,12 +96,6 @@ static void l2_config_read(void)
       pr_info("L2CACHE: Index of the largest way enabled: %d\n", regval);
   }
 
-static const struct of_device_id sifive_l2_ids[] = {
-     { .compatible = "sifive,fu540-c000-ccache" },
-     { .compatible = "sifive,fu740-c000-ccache" },
-     { /* end of table */ },
-};
-
   static ATOMIC_NOTIFIER_HEAD(l2_err_chain);
 
   int register_sifive_l2_error_notifier(struct notifier_block *nb)

@@ -192,36 +186,29 @@ static irqreturn_t l2_int_handler(int irq, void *device)
       return IRQ_HANDLED;
   }
 
-static int __init sifive_l2_init(void)
+static int __init sifive_l2_probe(struct platform_device *pdev)
   {
-     struct device_node *np;
-     struct resource res;
-     int i, rc, intr_num;
-
-     np = of_find_matching_node(NULL, sifive_l2_ids);
-     if (!np)
-             return -ENODEV;
-
-     if (of_address_to_resource(np, 0, &res))
-             return -ENODEV;
-
-     l2_base = ioremap(res.start, resource_size(&res));
-     if (!l2_base)
-             return -ENOMEM;
-
-     intr_num = of_property_count_u32_elems(np, "interrupts");
-     if (!intr_num) {
-             pr_err("L2CACHE: no interrupts property\n");
-             return -ENODEV;
-     }
-
-     for (i = 0; i < intr_num; i++) {
-             g_irq[i] = irq_of_parse_and_map(np, i);
-             rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL);
-             if (rc) {
-                     pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]);
-                     return rc;
-             }
+     struct device *dev = &pdev->dev;
+     int nirqs;

drivers/soc/sifive/sifive_l2_cache.c static irqreturn_t l2_int_handler(int irq, void *device) · line 202

Quoted 10 lines

drivers/soc/sifive/sifive_l2_cache.c +10 -0

+     int ret;
+     int i;
+
+     l2_base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(l2_base))
+             return PTR_ERR(l2_base);
+
+     nirqs = platform_irq_count(pdev);
+     if (nirqs <= 0)
+             return dev_err_probe(dev, -ENODEV, "no interrupts\n");

I wonder if zero irqs is an actual issue here?

drivers/soc/sifive/sifive_l2_cache.c static irqreturn_t l2_int_handler(int irq, void *device) · line 205

Quoted 2 lines

drivers/soc/sifive/sifive_l2_cache.c +2 -0

+     for (i = 0; i < nirqs; i++) {
+             g_irq[i] = platform_get_irq(pdev, i);

I wonder if we need to keep g_irq[] around now? Is it going to be useful in the future?

Quoted 27 lines

drivers/soc/sifive/sifive_l2_cache.c +19 -1

+             if (g_irq[i] < 0)
+                     return g_irq[i];
+
+             ret = devm_request_irq(dev, g_irq[i], l2_int_handler, 0, pdev->name, NULL);
+             if (ret)
+                     return dev_err_probe(dev, ret, "Could not request IRQ %d\n", g_irq[i]);
       }
 
       l2_config_read();

@@ -234,4 +221,18 @@ static int __init sifive_l2_init(void)
   #endif
       return 0;
   }
-device_initcall(sifive_l2_init);
+
+static const struct of_device_id sifive_l2_match[] = {
+     { .compatible = "sifive,fu540-c000-ccache" },
+     { .compatible = "sifive,fu740-c000-ccache" },
+     { /* sentinel */ }
+};
+
+static struct platform_driver sifive_l2_driver = {
+     .driver = {
+             .name = "sifive_l2_cache",
+             .of_match_table = sifive_l2_match,
+             .suppress_bind_attrs = true,
+     },

Quoted 8 lines

drivers/soc/sifive/sifive_l2_cache.c static int __init sifive_l2_init(void) · line 238

Quoted 2 lines

drivers/soc/sifive/sifive_l2_cache.c +2 -0

+};
+builtin_platform_driver_probe(sifive_l2_driver, sifive_l2_probe);

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 27 lines · Emil Renner Berthing

Quoted 10 lines · Ben Dooks

Quoted 7 lines · Hal Feng

From: Emil Renner Berthing <[email protected]>

This converts the driver to use the builtin_platform_driver_probe macro to initialize the driver. This macro ends up calling device_initcall as was used previously, but also allocates a platform device which gives us access to much nicer APIs such as platform_ioremap_resource, platform_get_irq and dev_err_probe.

Quoted 6 lines

This is useful, but also there are other changes currently being sorted out by Zong Li (cc'd into this message) which have already been reviewed and are hopefully queued for the next kernel release.

Quoted 2 lines

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

I'm ok with something like this being merged, but please note that if we ever want to support the JH7100 which uses registers in this peripheral to flush the cache for its non-coherent DMAs then this driver needs to be loaded before other peripherals or we will trigger the 2nd warning in arch/riscv/mm/dma-noncoherent.c. I'm not sure we can do that when it's a platform driver. See this patch for an alternative to support the JH71x0s:
https://github.com/esmil/linux/commit/9c5b29da56ae29159c9572c5bb195fe3a1b535c5

Quoted 1 line

/Emil

Are you replying to your own patch that does the conversion to platform driver and then saying that it could actually cause issues?

I'm all for dropping this for the moment and keeping the old early init for the ccache.

Quoted 125 lines

Quoted 116 lines

Quoted 70 lines

   drivers/soc/sifive/sifive_l2_cache.c | 79 ++++++++++++++--------------
   1 file changed, 40 insertions(+), 39 deletions(-)

drivers/soc/sifive/sifive_l2_cache.c +6 -38

index 59640a1d0b28..010d612f7420 100644
@@ -7,9 +7,9 @@
    */
   #include <linux/debugfs.h>
   #include <linux/interrupt.h>
-#include <linux/of_irq.h>
-#include <linux/of_address.h>
-#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
   #include <asm/cacheinfo.h>
   #include <soc/sifive/sifive_l2_cache.h>
 

@@ -96,12 +96,6 @@ static void l2_config_read(void)
       pr_info("L2CACHE: Index of the largest way enabled: %d\n", regval);
   }
 
-static const struct of_device_id sifive_l2_ids[] = {
-     { .compatible = "sifive,fu540-c000-ccache" },
-     { .compatible = "sifive,fu740-c000-ccache" },
-     { /* end of table */ },
-};
-
   static ATOMIC_NOTIFIER_HEAD(l2_err_chain);
 
   int register_sifive_l2_error_notifier(struct notifier_block *nb)

@@ -192,36 +186,29 @@ static irqreturn_t l2_int_handler(int irq, void *device)
       return IRQ_HANDLED;
   }
 
-static int __init sifive_l2_init(void)
+static int __init sifive_l2_probe(struct platform_device *pdev)
   {
-     struct device_node *np;
-     struct resource res;
-     int i, rc, intr_num;
-
-     np = of_find_matching_node(NULL, sifive_l2_ids);
-     if (!np)
-             return -ENODEV;
-
-     if (of_address_to_resource(np, 0, &res))
-             return -ENODEV;
-
-     l2_base = ioremap(res.start, resource_size(&res));
-     if (!l2_base)
-             return -ENOMEM;
-
-     intr_num = of_property_count_u32_elems(np, "interrupts");
-     if (!intr_num) {
-             pr_err("L2CACHE: no interrupts property\n");
-             return -ENODEV;
-     }
-
-     for (i = 0; i < intr_num; i++) {
-             g_irq[i] = irq_of_parse_and_map(np, i);
-             rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL);
-             if (rc) {
-                     pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]);
-                     return rc;
-             }
+     struct device *dev = &pdev->dev;
+     int nirqs;

drivers/soc/sifive/sifive_l2_cache.c static irqreturn_t l2_int_handler(int irq, void *device) · line 202

Quoted 10 lines

drivers/soc/sifive/sifive_l2_cache.c +10 -0

+     int ret;
+     int i;
+
+     l2_base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(l2_base))
+             return PTR_ERR(l2_base);
+
+     nirqs = platform_irq_count(pdev);
+     if (nirqs <= 0)
+             return dev_err_probe(dev, -ENODEV, "no interrupts\n");

I wonder if zero irqs is an actual issue here?

drivers/soc/sifive/sifive_l2_cache.c static irqreturn_t l2_int_handler(int irq, void *device) · line 205

Quoted 2 lines

drivers/soc/sifive/sifive_l2_cache.c +2 -0

+     for (i = 0; i < nirqs; i++) {
+             g_irq[i] = platform_get_irq(pdev, i);

I wonder if we need to keep g_irq[] around now? Is it going to be useful in the future?

Quoted 27 lines

drivers/soc/sifive/sifive_l2_cache.c +19 -1

+             if (g_irq[i] < 0)
+                     return g_irq[i];
+
+             ret = devm_request_irq(dev, g_irq[i], l2_int_handler, 0, pdev->name, NULL);
+             if (ret)
+                     return dev_err_probe(dev, ret, "Could not request IRQ %d\n", g_irq[i]);
       }
 
       l2_config_read();

@@ -234,4 +221,18 @@ static int __init sifive_l2_init(void)
   #endif
       return 0;
   }
-device_initcall(sifive_l2_init);
+
+static const struct of_device_id sifive_l2_match[] = {
+     { .compatible = "sifive,fu540-c000-ccache" },
+     { .compatible = "sifive,fu740-c000-ccache" },
+     { /* sentinel */ }
+};
+
+static struct platform_driver sifive_l2_driver = {
+     .driver = {
+             .name = "sifive_l2_cache",
+             .of_match_table = sifive_l2_match,
+             .suppress_bind_attrs = true,
+     },

Quoted 8 lines

drivers/soc/sifive/sifive_l2_cache.c static int __init sifive_l2_init(void) · line 238

Quoted 2 lines

drivers/soc/sifive/sifive_l2_cache.c +2 -0

+};
+builtin_platform_driver_probe(sifive_l2_driver, sifive_l2_probe);

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 5 lines

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Quoted 29 lines · Ben Dooks

Quoted 27 lines · Emil Renner Berthing

Quoted 10 lines · Ben Dooks

Quoted 7 lines · Hal Feng

From: Emil Renner Berthing <[email protected]>

This converts the driver to use the builtin_platform_driver_probe macro to initialize the driver. This macro ends up calling device_initcall as was used previously, but also allocates a platform device which gives us access to much nicer APIs such as platform_ioremap_resource, platform_get_irq and dev_err_probe.

Quoted 6 lines

This is useful, but also there are other changes currently being sorted out by Zong Li (cc'd into this message) which have already been reviewed and are hopefully queued for the next kernel release.

Quoted 2 lines

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

I'm ok with something like this being merged, but please note that if we ever want to support the JH7100 which uses registers in this peripheral to flush the cache for its non-coherent DMAs then this driver needs to be loaded before other peripherals or we will trigger the 2nd warning in arch/riscv/mm/dma-noncoherent.c. I'm not sure we can do that when it's a platform driver. See this patch for an alternative to support the JH71x0s:
https://github.com/esmil/linux/commit/9c5b29da56ae29159c9572c5bb195fe3a1b535c5

Quoted 5 lines

Quoted 1 line

/Emil

Are you replying to your own patch that does the conversion to platform driver and then saying that it could actually cause issues?

Yes, I can see it seems odd, but this patch lived for a while in the kernel repo for the JH7100 until I rebased on 6.0-rc1 and realized the above.
Hal Feng must have based his patches on a version of the code before that when preparing this series.

Quoted 2 lines

I'm all for dropping this for the moment and keeping the old early init for the ccache.

Cool.

/Emil

Quoted 137 lines

Quoted 125 lines

Quoted 116 lines

Quoted 70 lines

   drivers/soc/sifive/sifive_l2_cache.c | 79 ++++++++++++++--------------
   1 file changed, 40 insertions(+), 39 deletions(-)

drivers/soc/sifive/sifive_l2_cache.c +6 -38

index 59640a1d0b28..010d612f7420 100644
@@ -7,9 +7,9 @@
    */
   #include <linux/debugfs.h>
   #include <linux/interrupt.h>
-#include <linux/of_irq.h>
-#include <linux/of_address.h>
-#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
   #include <asm/cacheinfo.h>
   #include <soc/sifive/sifive_l2_cache.h>
 

@@ -96,12 +96,6 @@ static void l2_config_read(void)
       pr_info("L2CACHE: Index of the largest way enabled: %d\n", regval);
   }
 
-static const struct of_device_id sifive_l2_ids[] = {
-     { .compatible = "sifive,fu540-c000-ccache" },
-     { .compatible = "sifive,fu740-c000-ccache" },
-     { /* end of table */ },
-};
-
   static ATOMIC_NOTIFIER_HEAD(l2_err_chain);
 
   int register_sifive_l2_error_notifier(struct notifier_block *nb)

@@ -192,36 +186,29 @@ static irqreturn_t l2_int_handler(int irq, void *device)
       return IRQ_HANDLED;
   }
 
-static int __init sifive_l2_init(void)
+static int __init sifive_l2_probe(struct platform_device *pdev)
   {
-     struct device_node *np;
-     struct resource res;
-     int i, rc, intr_num;
-
-     np = of_find_matching_node(NULL, sifive_l2_ids);
-     if (!np)
-             return -ENODEV;
-
-     if (of_address_to_resource(np, 0, &res))
-             return -ENODEV;
-
-     l2_base = ioremap(res.start, resource_size(&res));
-     if (!l2_base)
-             return -ENOMEM;
-
-     intr_num = of_property_count_u32_elems(np, "interrupts");
-     if (!intr_num) {
-             pr_err("L2CACHE: no interrupts property\n");
-             return -ENODEV;
-     }
-
-     for (i = 0; i < intr_num; i++) {
-             g_irq[i] = irq_of_parse_and_map(np, i);
-             rc = request_irq(g_irq[i], l2_int_handler, 0, "l2_ecc", NULL);
-             if (rc) {
-                     pr_err("L2CACHE: Could not request IRQ %d\n", g_irq[i]);
-                     return rc;
-             }
+     struct device *dev = &pdev->dev;
+     int nirqs;

drivers/soc/sifive/sifive_l2_cache.c static irqreturn_t l2_int_handler(int irq, void *device) · line 202

Quoted 10 lines

drivers/soc/sifive/sifive_l2_cache.c +10 -0

+     int ret;
+     int i;
+
+     l2_base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(l2_base))
+             return PTR_ERR(l2_base);
+
+     nirqs = platform_irq_count(pdev);
+     if (nirqs <= 0)
+             return dev_err_probe(dev, -ENODEV, "no interrupts\n");

I wonder if zero irqs is an actual issue here?

drivers/soc/sifive/sifive_l2_cache.c static irqreturn_t l2_int_handler(int irq, void *device) · line 205

Quoted 2 lines

drivers/soc/sifive/sifive_l2_cache.c +2 -0

+     for (i = 0; i < nirqs; i++) {
+             g_irq[i] = platform_get_irq(pdev, i);

I wonder if we need to keep g_irq[] around now? Is it going to be useful in the future?

Quoted 27 lines

drivers/soc/sifive/sifive_l2_cache.c +19 -1

+             if (g_irq[i] < 0)
+                     return g_irq[i];
+
+             ret = devm_request_irq(dev, g_irq[i], l2_int_handler, 0, pdev->name, NULL);
+             if (ret)
+                     return dev_err_probe(dev, ret, "Could not request IRQ %d\n", g_irq[i]);
       }
 
       l2_config_read();

@@ -234,4 +221,18 @@ static int __init sifive_l2_init(void)
   #endif
       return 0;
   }
-device_initcall(sifive_l2_init);
+
+static const struct of_device_id sifive_l2_match[] = {
+     { .compatible = "sifive,fu540-c000-ccache" },
+     { .compatible = "sifive,fu740-c000-ccache" },
+     { /* sentinel */ }
+};
+
+static struct platform_driver sifive_l2_driver = {
+     .driver = {
+             .name = "sifive_l2_cache",
+             .of_match_table = sifive_l2_match,
+             .suppress_bind_attrs = true,
+     },

Quoted 8 lines

drivers/soc/sifive/sifive_l2_cache.c static int __init sifive_l2_init(void) · line 238

Quoted 2 lines

drivers/soc/sifive/sifive_l2_cache.c +2 -0

+};
+builtin_platform_driver_probe(sifive_l2_driver, sifive_l2_probe);

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 5 lines

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html

Quoted 42 lines · Emil Renner Berthing

Quoted 29 lines · Ben Dooks

Quoted 27 lines · Emil Renner Berthing

Quoted 10 lines · Ben Dooks

Quoted 7 lines · Hal Feng

From: Emil Renner Berthing <[email protected]>

This converts the driver to use the builtin_platform_driver_probe macro to initialize the driver. This macro ends up calling device_initcall as was used previously, but also allocates a platform device which gives us access to much nicer APIs such as platform_ioremap_resource, platform_get_irq and dev_err_probe.

Quoted 6 lines

This is useful, but also there are other changes currently being sorted out by Zong Li (cc'd into this message) which have already been reviewed and are hopefully queued for the next kernel release.

Quoted 2 lines

Signed-off-by Emil Renner Berthing <[email protected]>

Signed-off-by Hal Feng <[email protected]>

I'm ok with something like this being merged, but please note that if we ever want to support the JH7100 which uses registers in this peripheral to flush the cache for its non-coherent DMAs then this driver needs to be loaded before other peripherals or we will trigger the 2nd warning in arch/riscv/mm/dma-noncoherent.c. I'm not sure we can do that when it's a platform driver. See this patch for an alternative to support the JH71x0s:
https://github.com/esmil/linux/commit/9c5b29da56ae29159c9572c5bb195fe3a1b535c5

Quoted 5 lines

Quoted 1 line

/Emil

Are you replying to your own patch that does the conversion to platform driver and then saying that it could actually cause issues?

Yes, I can see it seems odd, but this patch lived for a while in the kernel repo for the JH7100 until I rebased on 6.0-rc1 and realized the above.
Hal Feng must have based his patches on a version of the code before that when preparing this series.

Quoted 4 lines

Quoted 2 lines

I'm all for dropping this for the moment and keeping the old early init for the ccache.

Cool.

FWIW, if converting to a platform driver will inhibit using the driver for doing non-coherent stuff I would like to NAK the patch :)

This series is also available at
https://github.com/hal-feng/linux/commits/visionfive2-minimal

[1] https://www.cnx-software.com/2022/08/23/starfive-visionfive-2-quad-core-risc-v-sbc-linux/
[2] https://wiki.rvspace.org/

Emil Renner Berthing (17):
dt-bindings: riscv: Add StarFive JH7110 bindings
dt-bindings: timer: Add StarFive JH7110 clint
dt-bindings: interrupt-controller: Add StarFive JH7110 plic
dt-bindings: sifive-l2-cache: Support StarFive JH71x0 SoCs
soc: sifive: l2 cache: Convert to platform driver
soc: sifive: l2 cache: Add StarFive JH71x0 support
reset: starfive: jh7100: Use 32bit I/O on 32bit registers
dt-bindings: reset: Add StarFive JH7110 reset definitions
clk: starfive: Factor out common clock driver code

dt-bindings: clock: Add StarFive JH7110 system clock definitions
dt-bindings: clock: Add starfive,jh7110-clkgen-sys bindings
clk: starfive: Add StarFive JH7110 system clock driver
dt-bindings: clock: Add StarFive JH7110 always-on definitions
dt-bindings: clock: Add starfive,jh7110-clkgen-aon bindings
clk: starfive: Add StarFive JH7110 always-on clock driver
RISC-V: Add initial StarFive JH7110 device tree
RISC-V: Add StarFive JH7110 VisionFive2 board device tree

Quoted 16 lines · Hal Feng

This series is also available at
https://github.com/hal-feng/linux/commits/visionfive2-minimal

[1] https://www.cnx-software.com/2022/08/23/starfive-visionfive-2-quad-core-risc-v-sbc-linux/
[2] https://wiki.rvspace.org/

Emil Renner Berthing (17):
dt-bindings: riscv: Add StarFive JH7110 bindings
dt-bindings: timer: Add StarFive JH7110 clint
dt-bindings: interrupt-controller: Add StarFive JH7110 plic
dt-bindings: sifive-l2-cache: Support StarFive JH71x0 SoCs
soc: sifive: l2 cache: Convert to platform driver
soc: sifive: l2 cache: Add StarFive JH71x0 support
reset: starfive: jh7100: Use 32bit I/O on 32bit registers
dt-bindings: reset: Add StarFive JH7110 reset definitions
clk: starfive: Factor out common clock driver code

Quoted 8 lines

dt-bindings: clock: Add StarFive JH7110 system clock definitions
dt-bindings: clock: Add starfive,jh7110-clkgen-sys bindings
clk: starfive: Add StarFive JH7110 system clock driver
dt-bindings: clock: Add StarFive JH7110 always-on definitions
dt-bindings: clock: Add starfive,jh7110-clkgen-aon bindings
clk: starfive: Add StarFive JH7110 always-on clock driver
RISC-V: Add initial StarFive JH7110 device tree
RISC-V: Add StarFive JH7110 VisionFive2 board device tree

Where is the rest of patches? Lists got only 5 of them. Anyway this is a bit too big patchset. Split per subsystem.

Quoted 29 lines · Krzysztof Kozlowski

Quoted 26 lines · Hal Feng

This series is also available at
https://github.com/hal-feng/linux/commits/visionfive2-minimal

[1]
https://www.cnx-software.com/2022/08/23/starfive-visionfive-2-quad-c
ore-risc-v-sbc-linux/
[2] https://wiki.rvspace.org/

Emil Renner Berthing (17):
dt-bindings: riscv: Add StarFive JH7110 bindings
dt-bindings: timer: Add StarFive JH7110 clint
dt-bindings: interrupt-controller: Add StarFive JH7110 plic
dt-bindings: sifive-l2-cache: Support StarFive JH71x0 SoCs
soc: sifive: l2 cache: Convert to platform driver
soc: sifive: l2 cache: Add StarFive JH71x0 support
reset: starfive: jh7100: Use 32bit I/O on 32bit registers
dt-bindings: reset: Add StarFive JH7110 reset definitions
clk: starfive: Factor out common clock driver code
dt-bindings: clock: Add StarFive JH7110 system clock definitions
dt-bindings: clock: Add starfive,jh7110-clkgen-sys bindings
clk: starfive: Add StarFive JH7110 system clock driver
dt-bindings: clock: Add StarFive JH7110 always-on definitions
dt-bindings: clock: Add starfive,jh7110-clkgen-aon bindings
clk: starfive: Add StarFive JH7110 always-on clock driver
RISC-V: Add initial StarFive JH7110 device tree
RISC-V: Add StarFive JH7110 VisionFive2 board device tree

Quoted 2 lines

Where is the rest of patches? Lists got only 5 of them. Anyway this is a bit too big patchset. Split per subsystem.

They seem to be coming in over time in dribs and drabs. I assume it is not

mailing list problem given how many lists are CCed on the mail and the

that they have different providers.

For v2 (or multiple v2s) please fix up your process so that this gets sent normally and not a couple of patches every hour.

Store the necessary properties in device tree instead of .c file, in order to apply this reset driver to other StarFive SoCs.

Signed-off-by Hal Feng <[email protected]>

---
 .../bindings/reset/starfive,jh7100-reset.yaml | 20 ++++++++
 arch/riscv/boot/dts/starfive/jh7100.dtsi      |  3 ++
 drivers/reset/reset-starfive-jh7100.c         | 50 +++++++++++++------
 3 files changed, 57 insertions(+), 16 deletions(-)

Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml +9 -0

index 300359a5e14b..3eff3f72a1ed 100644
@@ -20,19 +20,39 @@ properties:
   "#reset-cells":
     const: 1
 
+  starfive,assert-offset:
+    description: Offset of the first ASSERT register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,status-offset:
+    description: Offset of the first STATUS register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,nr-resets:

Quoted 35 lines · Hal Feng

Store the necessary properties in device tree instead of .c file, in order to apply this reset driver to other StarFive SoCs.

Signed-off-by Hal Feng <[email protected]>

---
 .../bindings/reset/starfive,jh7100-reset.yaml | 20 ++++++++
 arch/riscv/boot/dts/starfive/jh7100.dtsi      |  3 ++
 drivers/reset/reset-starfive-jh7100.c         | 50 +++++++++++++------
 3 files changed, 57 insertions(+), 16 deletions(-)

Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml +13 -0

index 300359a5e14b..3eff3f72a1ed 100644
@@ -20,19 +20,39 @@ properties:
   "#reset-cells":
     const: 1
 
+  starfive,assert-offset:
+    description: Offset of the first ASSERT register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,status-offset:
+    description: Offset of the first STATUS register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,nr-resets:
+    description: Number of reset signals
+    $ref: /schemas/types.yaml#/definitions/uint32
+
 required:
   - compatible
   - reg
   - "#reset-cells"
+  - starfive,assert-offset

Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml properties: · line 41

Quoted 2 lines

Documentation/devicetree/bindings/reset/starfive,jh7100-reset.yaml +2 -0

+  - starfive,status-offset
+  - starfive,nr-resets

Adding required properties is a red flag. You can't add required properties to an existing binding. That breaks the ABI unless the OS deals with the properties being absent. If the OS has to do that, then why add them in the first place? All this should be implied by the compatible string.

Add bindings for the reset controller on the JH7110 RISC-V SoC by StarFive Technology Ltd.

Signed-off-by Hal Feng <[email protected]>

---
 .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml

Quoted 9 lines · Hal Feng

Add bindings for the reset controller on the JH7110 RISC-V SoC by StarFive Technology Ltd.

Signed-off-by Hal Feng <[email protected]>

---
 .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/reset/starfive,jh7110-reset.example.dts:18:18: fatal error: dt-bindings/reset/starfive-jh7110.h: No such file or directory
18 | #include <dt-bindings/reset/starfive-jh7110.h>

      |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

compilation terminated.
make[1]: *** [scripts/Makefile.lib:384: Documentation/devicetree/bindings/reset/starfive,jh7110-reset.example.dtb] Error 1 make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1420: dt_binding_check] Error 2

Add bindings for the reset controller on the JH7110 RISC-V SoC by StarFive Technology Ltd.

Signed-off-by Hal Feng <[email protected]>

---
 .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +6 -0

new file mode 100644
index 000000000000..bb0010c200f9
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +23 -0

+
+required:
+  - compatible
+  - "#reset-cells"
+  - starfive,assert-offset
+  - starfive,status-offset
+  - starfive,nr-resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/starfive-jh7110.h>
+
+    syscrg_rst: reset-controller@13020000 {
+        compatible = "starfive,jh7110-reset";
+        #reset-cells = <1>;
+        starfive,assert-offset = <0x2F8>;
+        starfive,status-offset= <0x308>;
+        starfive,nr-resets = <JH7110_SYSRST_END>;
+    };
+
+...

Signature · 1 line

2.17.1

Quoted 25 lines · Hal Feng

Add bindings for the reset controller on the JH7110 RISC-V SoC by StarFive Technology Ltd.

Signed-off-by Hal Feng <[email protected]>

---
 .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +10 -0

new file mode 100644
index 000000000000..bb0010c200f9
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 SoC Reset Controller Device Tree Bindings
+
+maintainers:
+  - Emil Renner Berthing <[email protected]>

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 16

Quoted 6 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +6 -0

+  - Hal Feng <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - starfive,jh7110-reset

'reg' needed? Is this a sub-block of something else?

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 27

Quoted 12 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +11 -0

+
+  "#reset-cells":
+    const: 1
+
+  starfive,assert-offset:
+    description: Offset of the first ASSERT register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,status-offset:
+    description: Offset of the first STATUS register
+    $ref: /schemas/types.yaml#/definitions/uint32

These can't be implied from the compatible string?

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 31

Quoted 5 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +4 -0

+
+  starfive,nr-resets:
+    description: Number of reset signals
+    $ref: /schemas/types.yaml#/definitions/uint32

Why do you need this? Most bindings don't. If just to validate 'resets' args, then don't.

Quoted 29 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +23 -0

+
+required:
+  - compatible
+  - "#reset-cells"
+  - starfive,assert-offset
+  - starfive,status-offset
+  - starfive,nr-resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/starfive-jh7110.h>
+
+    syscrg_rst: reset-controller@13020000 {
+        compatible = "starfive,jh7110-reset";
+        #reset-cells = <1>;
+        starfive,assert-offset = <0x2F8>;
+        starfive,status-offset= <0x308>;
+        starfive,nr-resets = <JH7110_SYSRST_END>;
+    };
+
+...

Signature · 1 line

2.17.1

Quoted 26 lines · Rob Herring

Quoted 25 lines · Hal Feng

Add bindings for the reset controller on the JH7110 RISC-V SoC by StarFive Technology Ltd.

Signed-off-by Hal Feng <[email protected]>

---
 .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +10 -0

new file mode 100644
index 000000000000..bb0010c200f9
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 SoC Reset Controller Device Tree Bindings
+
+maintainers:
+  - Emil Renner Berthing <[email protected]>

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 16

Quoted 6 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +6 -0

+  - Hal Feng <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - starfive,jh7110-reset

'reg' needed? Is this a sub-block of something else?

Yes, the reset node is a child node of the syscon node, see patch 27 for detail. You might not see the complete patches at that time due to technical issue of our smtp email server. Again, I feel so sorry about that.

	syscrg: syscrg@13020000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x13020000 0x0 0x10000>;

		syscrg_clk: clock-controller@13020000 {
			compatible = "starfive,jh7110-clkgen-sys";
			clocks = <&osc>, <&gmac1_rmii_refin>,
				 <&gmac1_rgmii_rxin>,
				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
				 <&tdm_ext>, <&mclk_ext>;
			clock-names = "osc", "gmac1_rmii_refin",
				"gmac1_rgmii_rxin",
				"i2stx_bclk_ext", "i2stx_lrck_ext",

				"i2srx_bclk_ext", "i2srx_lrck_ext",
				"tdm_ext", "mclk_ext";
			#clock-cells = <1>;
		};

		syscrg_rst: reset-controller@13020000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;

			starfive,assert-offset = <0x2F8>;
			starfive,status-offset= <0x308>;
			starfive,nr-resets = <JH7110_SYSRST_END>;
		};
	};

In this case, we get the memory mapped space through the parent node with syscon APIs. You can see patch 13 for detail.

static int reset_starfive_register(struct platform_device *pdev, const u32 *asserted) {

Quoted 15 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 27

Quoted 13 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +11 -0

+
+  "#reset-cells":
+    const: 1
+
+  starfive,assert-offset:
+    description: Offset of the first ASSERT register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,status-offset:
+    description: Offset of the first STATUS register
+    $ref: /schemas/types.yaml#/definitions/uint32

These can't be implied from the compatible string?

These two properties are the key differences among different reset controllers.

There are five memory regions for clock and reset in StarFive JH7110 SoC. They are "syscrg", "aoncrg", "stgcrg", "ispcrg" and "voutcrg". Each memory region has different reset ASSERT/STATUS register offset and different number of reset signals.

After storing them in dt, the reset driver can register all reset controllers with the same compatible string.

All we expect is that all reset
controllers in a single SoC use the same compatible string for matching and the reset driver can be applied to all StarFive SoCs using different compatible strings.

Just like

arch/riscv/boot/dts/starfive/jh7100.dtsi:

	rstgen: reset-controller@11840000 {
		compatible = "starfive,jh7100-reset";
		reg = <0x0 0x11840000 0x0 0x10000>;
		#reset-cells = <1>;
		starfive,assert-offset = <0x0>;
		starfive,status-offset= <0x10>;
		starfive,nr-resets = <JH7100_RSTN_END>;
	};

arch/riscv/boot/dts/starfive/jh7110.dtsi:

	syscrg: syscrg@13020000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x13020000 0x0 0x10000>;

		syscrg_clk: clock-controller@13020000 {
			compatible = "starfive,jh7110-clkgen-sys";
			...
		};

		syscrg_rst: reset-controller@13020000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;
			starfive,assert-offset = <0x2F8>;
			starfive,status-offset= <0x308>;
			starfive,nr-resets = <JH7110_SYSRST_END>;
		};
	};

	aoncrg: aoncrg@17000000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x17000000 0x0 0x10000>;

		aoncrg_clk: clock-controller@17000000 {
			compatible = "starfive,jh7110-clkgen-aon";
			...
		};

		aoncrg_rst: reset-controller@17000000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;
			starfive,assert-offset = <0x38>;
			starfive,status-offset= <0x3C>;
			starfive,nr-resets = <JH7110_AONRST_END>;
		};
	};

	stgcrg: stgcrg@10230000 {	//Not submmited yet
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x10230000 0x0 0x10000>;

		stgcrg_clk: clock-controller@10230000 {
			compatible = "starfive,jh7110-clkgen-stg";
			...
		};

		stgcrg_rst: reset-controller@10230000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;
			starfive,assert-offset = <0x74>;
			starfive,status-offset= <0x78>;
			starfive,nr-resets = <JH7110_STGRST_END>;
		};
	};
	...

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 31

Quoted 5 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +4 -0

+
+  starfive,nr-resets:
+    description: Number of reset signals
+    $ref: /schemas/types.yaml#/definitions/uint32

Why do you need this? Most bindings don't. If just to validate 'resets' args, then don't.

Can be removed. Instead, the reset driver should includes some related binding headers or defines some macros for pointing out the number of reset signals of each reset controller.

Best regards,
Hal

Quoted 2 lines

Quoted 7 lines

Quoted 7 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +7 -0

+
+required:
+  - compatible
+  - "#reset-cells"
+  - starfive,assert-offset
+  - starfive,status-offset
+  - starfive,nr-resets

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 46

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +8 -0

+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/starfive-jh7110.h>
+
+    syscrg_rst: reset-controller@13020000 {

Quoted 59 lines · Hal Feng

Quoted 26 lines · Rob Herring

Quoted 25 lines · Hal Feng

Add bindings for the reset controller on the JH7110 RISC-V SoC by StarFive Technology Ltd.

Signed-off-by Hal Feng <[email protected]>

---
 .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +10 -0

new file mode 100644
index 000000000000..bb0010c200f9
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 SoC Reset Controller Device Tree Bindings
+
+maintainers:
+  - Emil Renner Berthing <[email protected]>

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 16

Quoted 6 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +6 -0

+  - Hal Feng <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - starfive,jh7110-reset

'reg' needed? Is this a sub-block of something else?

Yes, the reset node is a child node of the syscon node, see patch 27 for detail. You might not see the complete patches at that time due to technical issue of our smtp email server. Again, I feel so sorry about that.

	syscrg: syscrg@13020000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x13020000 0x0 0x10000>;

		syscrg_clk: clock-controller@13020000 {
			compatible = "starfive,jh7110-clkgen-sys";
			clocks = <&osc>, <&gmac1_rmii_refin>,
				 <&gmac1_rgmii_rxin>,
				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
				 <&tdm_ext>, <&mclk_ext>;
			clock-names = "osc", "gmac1_rmii_refin",
				"gmac1_rgmii_rxin",
				"i2stx_bclk_ext", "i2stx_lrck_ext",
				"i2srx_bclk_ext", "i2srx_lrck_ext",
				"tdm_ext", "mclk_ext";
			#clock-cells = <1>;
		};

Quoted 3 lines

		syscrg_rst: reset-controller@13020000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;

So the answer to the "reg needed?" is what? You have unit address but no reg, so this is not correct.

Quoted 12 lines

			starfive,assert-offset = <0x2F8>;
			starfive,status-offset= <0x308>;
			starfive,nr-resets = <JH7110_SYSRST_END>;
		};
	};

In this case, we get the memory mapped space through the parent node with syscon APIs. You can see patch 13 for detail.

static int reset_starfive_register(struct platform_device *pdev, const u32 *asserted) {

(...)

Quoted 2 lines

Quoted 2 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +5 -0

+
+  "#reset-cells":
+    const: 1
+
+  starfive,assert-offset:

Quoted 8 lines

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 27

Quoted 6 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +6 -0

+    description: Offset of the first ASSERT register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,status-offset:
+    description: Offset of the first STATUS register
+    $ref: /schemas/types.yaml#/definitions/uint32

These can't be implied from the compatible string?

Quoted 2 lines

These two properties are the key differences among different reset controllers.

Different as in different compatibles? Please answer the questions..>

Quoted 4 lines

There are five memory regions for clock and reset in StarFive JH7110 SoC. They are "syscrg", "aoncrg", "stgcrg", "ispcrg" and "voutcrg". Each memory region has different reset ASSERT/STATUS register offset and different number of reset signals.

Then these are not exactly the same devices, so using one compatible for them does not look correct.

Quoted 3 lines

After storing them in dt, the reset driver can register all reset controllers with the same compatible string.

Which is not how the compatible should be used...

Quoted 3 lines

All we expect is that all reset
controllers in a single SoC use the same compatible string for matching and the reset driver can be applied to all StarFive SoCs using different compatible strings.

Keep driver out of the talks.

Quoted 1 line

Just like

Existing bad pattern is not an argument to keep it going. Fix bad patterns instead.

Quoted 87 lines

arch/riscv/boot/dts/starfive/jh7100.dtsi:

	rstgen: reset-controller@11840000 {
		compatible = "starfive,jh7100-reset";
		reg = <0x0 0x11840000 0x0 0x10000>;
		#reset-cells = <1>;
		starfive,assert-offset = <0x0>;
		starfive,status-offset= <0x10>;
		starfive,nr-resets = <JH7100_RSTN_END>;
	};

arch/riscv/boot/dts/starfive/jh7110.dtsi:

	syscrg: syscrg@13020000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x13020000 0x0 0x10000>;

		syscrg_clk: clock-controller@13020000 {
			compatible = "starfive,jh7110-clkgen-sys";
			...
		};

		syscrg_rst: reset-controller@13020000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;
			starfive,assert-offset = <0x2F8>;
			starfive,status-offset= <0x308>;
			starfive,nr-resets = <JH7110_SYSRST_END>;
		};
	};

	aoncrg: aoncrg@17000000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x17000000 0x0 0x10000>;

		aoncrg_clk: clock-controller@17000000 {
			compatible = "starfive,jh7110-clkgen-aon";
			...
		};

		aoncrg_rst: reset-controller@17000000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;
			starfive,assert-offset = <0x38>;
			starfive,status-offset= <0x3C>;
			starfive,nr-resets = <JH7110_AONRST_END>;
		};
	};

	stgcrg: stgcrg@10230000 {	//Not submmited yet
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x10230000 0x0 0x10000>;

		stgcrg_clk: clock-controller@10230000 {
			compatible = "starfive,jh7110-clkgen-stg";
			...
		};

		stgcrg_rst: reset-controller@10230000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;
			starfive,assert-offset = <0x74>;
			starfive,status-offset= <0x78>;
			starfive,nr-resets = <JH7110_STGRST_END>;
		};
	};
	...

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 31

Quoted 5 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +4 -0

+
+  starfive,nr-resets:
+    description: Number of reset signals
+    $ref: /schemas/types.yaml#/definitions/uint32

Why do you need this? Most bindings don't. If just to validate 'resets' args, then don't.

Can be removed. Instead, the reset driver should includes some related binding headers or defines some macros for pointing out the number of reset signals of each reset controller.

Best regards,
Hal

Quoted 2 lines

Quoted 15 lines

Quoted 7 lines

Quoted 7 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +7 -0

+
+required:
+  - compatible
+  - "#reset-cells"
+  - starfive,assert-offset
+  - starfive,status-offset
+  - starfive,nr-resets

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 46

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +8 -0

+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/starfive-jh7110.h>
+
+    syscrg_rst: reset-controller@13020000 {

Please test your patches.

Best regards,
Krzysztof

Quoted 30 lines

Quoted 30 lines

Quoted 6 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 16

Quoted 4 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +4 -0

+properties:
+  compatible:
+    enum:
+      - starfive,jh7110-reset

'reg' needed? Is this a sub-block of something else?

Yes, the reset node is a child node of the syscon node, see patch 27 for detail. You might not see the complete patches at that time due to technical issue of our smtp email server. Again, I feel so sorry about that.

	syscrg: syscrg@13020000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x13020000 0x0 0x10000>;

		syscrg_clk: clock-controller@13020000 {
			compatible = "starfive,jh7110-clkgen-sys";
			clocks = <&osc>, <&gmac1_rmii_refin>,
				 <&gmac1_rgmii_rxin>,
				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
				 <&tdm_ext>, <&mclk_ext>;
			clock-names = "osc", "gmac1_rmii_refin",
				"gmac1_rgmii_rxin",
				"i2stx_bclk_ext", "i2stx_lrck_ext",
				"i2srx_bclk_ext", "i2srx_lrck_ext",
				"tdm_ext", "mclk_ext";
			#clock-cells = <1>;
		};

Quoted 6 lines

Quoted 3 lines

		syscrg_rst: reset-controller@13020000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;

So the answer to the "reg needed?" is what? You have unit address but no reg, so this is not correct.

Not needed in the reset-controller node, but needed in its parent node.

I am sorry
for missing description to point it out in the bindings. I will rewrite all bindings for the next version. Unit address here should be deleted.

Quoted 18 lines

Quoted 12 lines

			starfive,assert-offset = <0x2F8>;
			starfive,status-offset= <0x308>;
			starfive,nr-resets = <JH7110_SYSRST_END>;
		};
	};

In this case, we get the memory mapped space through the parent node with syscon APIs. You can see patch 13 for detail.

static int reset_starfive_register(struct platform_device *pdev, const u32 *asserted) {

(...)

Quoted 2 lines

Quoted 2 lines

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +5 -0

+
+  "#reset-cells":
+    const: 1
+
+  starfive,assert-offset:

Quoted 8 lines

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 27

Quoted 6 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +6 -0

+    description: Offset of the first ASSERT register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,status-offset:
+    description: Offset of the first STATUS register
+    $ref: /schemas/types.yaml#/definitions/uint32

These can't be implied from the compatible string?

Definitely can. We do this is for simplifying the reset driver.

Otherwise, we may need to define more compatibles because there are multiple reset blocks in JH7110. Another case can be found at
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/reset/altr,rst-mgr.yaml

Quoted 8 lines

Quoted 1 line

These two properties are the key differences among different reset controllers.

Different as in different compatibles? Please answer the questions..>

Quoted 4 lines

There are five memory regions for clock and reset in StarFive JH7110 SoC. They are "syscrg", "aoncrg", "stgcrg", "ispcrg" and "voutcrg". Each memory region has different reset ASSERT/STATUS register offset and different number of reset signals.

Quoted 2 lines

Then these are not exactly the same devices, so using one compatible for them does not look correct.

One compatible can just be matched by one device? I think this is what confuses me.

Best regards,
Hal

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

These two properties are the key differences among different reset controllers.

Different as in different compatibles? Please answer the questions..>

Quoted 4 lines

There are five memory regions for clock and reset in StarFive JH7110 SoC. They are "syscrg", "aoncrg", "stgcrg", "ispcrg" and "voutcrg". Each memory region has different reset ASSERT/STATUS register offset and different number of reset signals.

Quoted 5 lines

Quoted 2 lines

Then these are not exactly the same devices, so using one compatible for them does not look correct.

One compatible can just be matched by one device? I think this is what confuses me.

I don't understand the question.

Hey Hal Feng,

Quoted 8 lines · Krzysztof Kozlowski

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

These two properties are the key differences among different reset controllers.

Different as in different compatibles? Please answer the questions..>

Quoted 4 lines

There are five memory regions for clock and reset in StarFive JH7110 SoC. They are "syscrg", "aoncrg", "stgcrg", "ispcrg" and "voutcrg". Each memory region has different reset ASSERT/STATUS register offset and different number of reset signals.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Then these are not exactly the same devices, so using one compatible for them does not look correct.

One compatible can just be matched by one device? I think this is what confuses me.

I don't understand the question.

If two SoCs have exactly the same device/peripheral then they _can_ use the same compatible. If they share some common, viable feature-set then one can "fall back" to the other depending on what your Venn diagram of common features looks like. I've not been following this too closely, but I think what Krzysztof is suggesting is that you have a jh7100 and a jh7110 compatible. Then in your driver you just "know" that if you match against jh7110 which values to use for register offsets & vice versa for a match against the jh7100. There's many examples over the tree for how to handle this sort of thing rather than including it in the devicetree.

Maybe Rob and Krzysztof will scream at me for this description, but devicetree is about how periperhals etc are connected together in the system not about the internals of a given peripheral.

Following that logic, the devicetree should not contain register offsets etc that are a known quanitity once you've determined that you are running on vendor,soc-foo.

Hopefully that helps with your confusion somewhat? Conor.

Quoted 34 lines · Hal Feng

Quoted 34 lines

Quoted 33 lines

Quoted 6 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 16

Quoted 4 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +4 -0

+properties:
+  compatible:
+    enum:
+      - starfive,jh7110-reset

'reg' needed? Is this a sub-block of something else?

Yes, the reset node is a child node of the syscon node, see patch 27 for detail. You might not see the complete patches at that time due to technical issue of our smtp email server. Again, I feel so sorry about that.

	syscrg: syscrg@13020000 {
		compatible = "syscon", "simple-mfd";
		reg = <0x0 0x13020000 0x0 0x10000>;

		syscrg_clk: clock-controller@13020000 {
			compatible = "starfive,jh7110-clkgen-sys";
			clocks = <&osc>, <&gmac1_rmii_refin>,
				 <&gmac1_rgmii_rxin>,
				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
				 <&tdm_ext>, <&mclk_ext>;
			clock-names = "osc", "gmac1_rmii_refin",
				"gmac1_rgmii_rxin",
				"i2stx_bclk_ext", "i2stx_lrck_ext",
				"i2srx_bclk_ext", "i2srx_lrck_ext",
				"tdm_ext", "mclk_ext";
			#clock-cells = <1>;
		};

		syscrg_rst: reset-controller@13020000 {
			compatible = "starfive,jh7110-reset";
			#reset-cells = <1>;

Quoted 4 lines

Quoted 2 lines

So the answer to the "reg needed?" is what? You have unit address but no reg, so this is not correct.

Not needed in the reset-controller node, but needed in its parent node.

We do not talk about parent node. Rob's question was in this bindings. Is this document a binding for the parent node or for this node?

Quoted 37 lines

I am sorry
for missing description to point it out in the bindings. I will rewrite all bindings for the next version. Unit address here should be deleted.

Quoted 18 lines

Quoted 12 lines

			starfive,assert-offset = <0x2F8>;
			starfive,status-offset= <0x308>;
			starfive,nr-resets = <JH7110_SYSRST_END>;
		};
	};

In this case, we get the memory mapped space through the parent node with syscon APIs. You can see patch 13 for detail.

static int reset_starfive_register(struct platform_device *pdev, const u32 *asserted) {

(...)

Quoted 2 lines

Quoted 2 lines

Quoted 13 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +5 -0

+
+  "#reset-cells":
+    const: 1
+
+  starfive,assert-offset:

Quoted 8 lines

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 27

Quoted 6 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +6 -0

+    description: Offset of the first ASSERT register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,status-offset:
+    description: Offset of the first STATUS register
+    $ref: /schemas/types.yaml#/definitions/uint32

These can't be implied from the compatible string?

Quoted 1 line

Definitely can. We do this is for simplifying the reset driver.

The role of the bindings is not to simplify some specific driver in some specific OS...

Quoted 3 lines

Otherwise, we may need to define more compatibles because there are multiple reset blocks in JH7110. Another case can be found at
https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/reset/altr,rst-mgr.yaml

And why is this a problem? You have different hardware, so should have different compatibles. Otherwise we would have a compatible "all,everything" and use it in all possible devices.

Quoted 26 lines · Rob Herring

Quoted 25 lines · Hal Feng

Add bindings for the reset controller on the JH7110 RISC-V SoC by StarFive Technology Ltd.

Signed-off-by Hal Feng <[email protected]>

---
 .../bindings/reset/starfive,jh7110-reset.yaml | 54 +++++++++++++++++++
 1 file changed, 54 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +10 -0

new file mode 100644
index 000000000000..bb0010c200f9
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/starfive,jh7110-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: StarFive JH7110 SoC Reset Controller Device Tree Bindings
+
+maintainers:
+  - Emil Renner Berthing <[email protected]>

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 16

Quoted 6 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +6 -0

+  - Hal Feng <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - starfive,jh7110-reset

'reg' needed? Is this a sub-block of something else?

Yes, the reset node is a child node of the syscon node, see patch 27 for detail. You might not see the complete patches at that time due to technical issue of our smtp email server. Again, I feel so sorry about that.

        syscrg: syscrg@13020000 {
                compatible = "syscon", "simple-mfd";
                reg = <0x0 0x13020000 0x0 0x10000>;

                syscrg_clk: clock-controller@13020000 {
                        compatible = "starfive,jh7110-clkgen-sys";
                        clocks = <&osc>, <&gmac1_rmii_refin>,
                                 <&gmac1_rgmii_rxin>,
                                 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
                                 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
                                 <&tdm_ext>, <&mclk_ext>;
                        clock-names = "osc", "gmac1_rmii_refin",
                                "gmac1_rgmii_rxin",
                                "i2stx_bclk_ext", "i2stx_lrck_ext",
                                "i2srx_bclk_ext", "i2srx_lrck_ext",
                                "tdm_ext", "mclk_ext";
                        #clock-cells = <1>;
                };

                syscrg_rst: reset-controller@13020000 {
                        compatible = "starfive,jh7110-reset";
                        #reset-cells = <1>;
                        starfive,assert-offset = <0x2F8>;
                        starfive,status-offset= <0x308>;
                        starfive,nr-resets = <JH7110_SYSRST_END>;
                };
        };

In this case, we get the memory mapped space through the parent node with syscon APIs. You can see patch 13 for detail.

static int reset_starfive_register(struct platform_device *pdev, const u32 *asserted)

{
        struct starfive_reset *data;
        int ret;

        data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
        if (!data)
                return -ENOMEM;

        data->regmap = device_node_to_regmap(pdev->dev.of_node);                  //for JH7100
        if (IS_ERR(data->regmap)) {
                data->regmap = syscon_node_to_regmap(pdev->dev.of_node->parent);  //for JH7110
                if (IS_ERR(data->regmap)) {
                        dev_err(&pdev->dev, "failed to get regmap (error %ld)\n",
                                PTR_ERR(data->regmap));
                        return PTR_ERR(data->regmap);
                }
        }
        ...
}

We use this method to avoid errors when remapping the same address in two different drivers, because clock and reset of StarFive JH7110 share a common register address region. For similar implementation, refer to file [1] and [2].

[1] arch/riscv/boot/dts/canaan/k210.dtsi

        sysctl: syscon@50440000 {
                compatible = "canaan,k210-sysctl",
                             "syscon", "simple-mfd";
                reg = <0x50440000 0x100>;
                clocks = <&sysclk K210_CLK_APB1>;
                clock-names = "pclk";

                sysclk: clock-controller {
                        #clock-cells = <1>;
                        compatible = "canaan,k210-clk";
                        clocks = <&in0>;
                };

                sysrst: reset-controller {
                        compatible = "canaan,k210-rst";
                        #reset-cells = <1>;
                };

                reboot: syscon-reboot {
                        compatible = "syscon-reboot";
                        regmap = <&sysctl>;
                        offset = <48>;
                        mask = <1>;
                        value = <1>;
                };
        };

[2] drivers/reset/reset-k210.c

Quoted 14 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 27

Quoted 12 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +11 -0

+
+  "#reset-cells":
+    const: 1
+
+  starfive,assert-offset:
+    description: Offset of the first ASSERT register
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+  starfive,status-offset:
+    description: Offset of the first STATUS register
+    $ref: /schemas/types.yaml#/definitions/uint32

These can't be implied from the compatible string?

These two properties are the key differences among different reset controllers. There are five memory regions for clock and reset in StarFive JH7110 SoC. They are "syscrg", "aoncrg", "stgcrg", "ispcrg" and "voutcrg". Each memory region has different reset ASSERT/STATUS register offset and different number of reset signals. After storing them in dt, the reset driver can register all reset controllers with the same compatible string. All we expect is that all reset controllers in a single SoC use the same compatible string for matching and the reset driver can be applied to all StarFive SoCs using different compatible strings. Just like

arch/riscv/boot/dts/starfive/jh7100.dtsi:

        rstgen: reset-controller@11840000 {
                compatible = "starfive,jh7100-reset";
                reg = <0x0 0x11840000 0x0 0x10000>;
                #reset-cells = <1>;
                starfive,assert-offset = <0x0>;
                starfive,status-offset= <0x10>;
                starfive,nr-resets = <JH7100_RSTN_END>;
        };

arch/riscv/boot/dts/starfive/jh7110.dtsi:

        syscrg: syscrg@13020000 {
                compatible = "syscon", "simple-mfd";
                reg = <0x0 0x13020000 0x0 0x10000>;

                syscrg_clk: clock-controller@13020000 {
                        compatible = "starfive,jh7110-clkgen-sys";
                        ...
                };

                syscrg_rst: reset-controller@13020000 {
                        compatible = "starfive,jh7110-reset";
                        #reset-cells = <1>;
                        starfive,assert-offset = <0x2F8>;
                        starfive,status-offset= <0x308>;
                        starfive,nr-resets = <JH7110_SYSRST_END>;
                };
        };

        aoncrg: aoncrg@17000000 {
                compatible = "syscon", "simple-mfd";
                reg = <0x0 0x17000000 0x0 0x10000>;

                aoncrg_clk: clock-controller@17000000 {
                        compatible = "starfive,jh7110-clkgen-aon";
                        ...
                };

                aoncrg_rst: reset-controller@17000000 {
                        compatible = "starfive,jh7110-reset";
                        #reset-cells = <1>;
                        starfive,assert-offset = <0x38>;
                        starfive,status-offset= <0x3C>;
                        starfive,nr-resets = <JH7110_AONRST_END>;
                };
        };

        stgcrg: stgcrg@10230000 {       //Not submmited yet
                compatible = "syscon", "simple-mfd";
                reg = <0x0 0x10230000 0x0 0x10000>;

                stgcrg_clk: clock-controller@10230000 {
                        compatible = "starfive,jh7110-clkgen-stg";
                        ...
                };

                stgcrg_rst: reset-controller@10230000 {
                        compatible = "starfive,jh7110-reset";
                        #reset-cells = <1>;
                        starfive,assert-offset = <0x74>;
                        starfive,status-offset= <0x78>;
                        starfive,nr-resets = <JH7110_STGRST_END>;
                };
        };
        ...

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml line 31

Quoted 5 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +4 -0

+
+  starfive,nr-resets:
+    description: Number of reset signals
+    $ref: /schemas/types.yaml#/definitions/uint32

Why do you need this? Most bindings don't. If just to validate 'resets' args, then don't.

Can be removed. Instead, the reset driver should includes some related binding headers or defines some macros for pointing out the number of reset signals of each reset controller.

Best regards,
Hal

Quoted 21 lines

Quoted 21 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +19 -0

+
+required:
+  - compatible
+  - "#reset-cells"
+  - starfive,assert-offset
+  - starfive,status-offset
+  - starfive,nr-resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/reset/starfive-jh7110.h>
+
+    syscrg_rst: reset-controller@13020000 {
+        compatible = "starfive,jh7110-reset";
+        #reset-cells = <1>;
+        starfive,assert-offset = <0x2F8>;
+        starfive,status-offset= <0x308>;

Quoted 9 lines

Quoted 8 lines

Documentation/devicetree/bindings/reset/starfive,jh7110-reset.yaml new file +4 -1

+        starfive,nr-resets = <JH7110_SYSRST_END>;
+    };
+
+...
--

2.17.1

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

From: Emil Renner Berthing <[email protected]>

The clock control registers on the StarFive SoCs work identically, so factor out the code then drivers for different SoCs can share it without depending on each other. No functional change.

Clock registers address region is shared with reset controller on the new StarFive JH7110 SoC. Change to use regmap framework to allow base address sharing and preparation for JH7110 clock support.

Signed-off-by Hal Feng <[email protected]>

---

drivers/clk/starfive/clk-starfive-jh7100.c +1 -0

index 014e36f17595..410aa6e06842 100644
@@ -10,6 +10,7 @@
 #include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/init.h>
+#include <linux/mfd/syscon.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
 

drivers/clk/starfive/clk-starfive-jh7100.c +4 -0

+       if (IS_ERR(priv->regmap)) {
+               dev_err(priv->dev, "failed to get regmap (error %ld)\n",
+                       PTR_ERR(priv->regmap));
+               return PTR_ERR(priv->regmap);

Quoting Hal Feng (2022-09-29 10:56:02)

Quoted 4 lines

Clock registers address region is shared with reset controller on the new StarFive JH7110 SoC. Change to use regmap framework to allow base address sharing and preparation for JH7110 clock support.

Do the reset and clk parts share actual registers, where we would need to lock between rmw? Or is regmap just nice to have because it wraps up the register APIs with some extra features?

Quoted 3 lines

Signed-off-by Hal Feng <[email protected]>

---

[...]

Quoted 16 lines

drivers/clk/starfive/clk-starfive-jh7100.c +1 -0

index 014e36f17595..410aa6e06842 100644
@@ -10,6 +10,7 @@
 #include <linux/clk-provider.h>
 #include <linux/device.h>
 #include <linux/init.h>
+#include <linux/mfd/syscon.h>
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
 

@@ -295,11 +296,13 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
        if (!priv)
                return -ENOMEM;
 

drivers/clk/starfive/clk-starfive-jh7100.c static int __init clk_starfive_jh7100_probe(struct platform_device *pdev) · line 300

Quoted 6 lines

drivers/clk/starfive/clk-starfive-jh7100.c +1 -4

-       spin_lock_init(&priv->rmw_lock);
        priv->dev = &pdev->dev;
-       priv->base = devm_platform_ioremap_resource(pdev, 0);
-       if (IS_ERR(priv->base))
-               return PTR_ERR(priv->base);
+       priv->regmap = device_node_to_regmap(priv->dev->of_node);

This is sad. Why do we need to make a syscon? Can we instead use the auxiliary bus to make a reset device that either gets a regmap made here in this driver or uses a void __iomem * mapped with ioremap (priv->base)?

drivers/clk/starfive/clk-starfive-jh7100.c static int __init clk_starfive_jh7100_probe(struct platform_device *pdev) · line 304

Quoted 4 lines

drivers/clk/starfive/clk-starfive-jh7100.c +4 -0

+       if (IS_ERR(priv->regmap)) {
+               dev_err(priv->dev, "failed to get regmap (error %ld)\n",
+                       PTR_ERR(priv->regmap));
+               return PTR_ERR(priv->regmap);

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 4 lines

Quoted 4 lines

drivers/clk/starfive/clk-starfive-jh7100-audio.c +0 -0

@@ -295,11 +296,13 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
        if (!priv)
                return -ENOMEM;
 

Quoted 11 lines

drivers/clk/starfive/clk-starfive-jh7100-audio.c static int jh7100_audclk_probe(struct platform_device *pdev) · line 113

Quoted 6 lines

drivers/clk/starfive/clk-starfive-jh7100-audio.c +1 -4

-       spin_lock_init(&priv->rmw_lock);
        priv->dev = &pdev->dev;
-       priv->base = devm_platform_ioremap_resource(pdev, 0);
-       if (IS_ERR(priv->base))
-               return PTR_ERR(priv->base);
+       priv->regmap = device_node_to_regmap(priv->dev->of_node);

This is sad. Why do we need to make a syscon? Can we instead use the auxiliary bus to make a reset device that either gets a regmap made here in this driver or uses a void __iomem * mapped with ioremap (priv->base)?

In my original code the clock driver just registers the resets too similar to other combined clock and reset drivers. I wonder what you think about that approach:
https://github.com/esmil/linux/commit/36f15e1b827b02d7f493dc5fce31060b21976e68
and
https://github.com/esmil/linux/commit/4ccafadb72968480aa3dd28c227fcccae411c13b#diff-ffec81f902f810cb210012c25e8d88217ea5b4021419a4206d1fd4dd19edfce8R471

Quoting Emil Renner Berthing (2022-10-05 06:14:44)

Quoted 16 lines

Quoted 4 lines

Quoted 4 lines

drivers/clk/starfive/clk-starfive-jh7100-audio.c +0 -0

@@ -295,11 +296,13 @@ static int __init clk_starfive_jh7100_probe(struct platform_device *pdev)
        if (!priv)
                return -ENOMEM;
 

Quoted 11 lines

drivers/clk/starfive/clk-starfive-jh7100-audio.c static int jh7100_audclk_probe(struct platform_device *pdev) · line 113

Quoted 6 lines

drivers/clk/starfive/clk-starfive-jh7100-audio.c +1 -4

-       spin_lock_init(&priv->rmw_lock);
        priv->dev = &pdev->dev;
-       priv->base = devm_platform_ioremap_resource(pdev, 0);
-       if (IS_ERR(priv->base))
-               return PTR_ERR(priv->base);
+       priv->regmap = device_node_to_regmap(priv->dev->of_node);

This is sad. Why do we need to make a syscon? Can we instead use the auxiliary bus to make a reset device that either gets a regmap made here in this driver or uses a void __iomem * mapped with ioremap (priv->base)?

Quoted 6 lines

In my original code the clock driver just registers the resets too similar to other combined clock and reset drivers. I wonder what you think about that approach:
https://github.com/esmil/linux/commit/36f15e1b827b02d7f493dc5fce31060b21976e68
and
https://github.com/esmil/linux/commit/4ccafadb72968480aa3dd28c227fcccae411c13b#diff-ffec81f902f810cb210012c25e8d88217ea5b4021419a4206d1fd4dd19edfce8R471