[PATCH 0/2] Add RZ/G2L OSTM support

[PATCH 0/2] Add RZ/G2L OSTM support

[Biju Das]

This patch series aims to add Generic Timer Module (a.k.a OSTM) support
for RZ/G2L series.

OSTM modules needs to deassert the reset line before accessing
any registers.

Biju Das (2):
  dt-bindings: timer: renesas: ostm: Document Renesas RZ/G2L OSTM
  clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

 .../bindings/timer/renesas,ostm.yaml          | 29 +++++++++++---
 drivers/clocksource/renesas-ostm.c            | 38 +++++++++++++++++++
 2 files changed, 62 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH 0/2] Add RZ/G2L OSTM support

[Biju Das]

This patch series aims to add Generic Timer Module (a.k.a OSTM) support
for RZ/G2L series.

OSTM modules needs to deassert the reset line before accessing
any registers.

Biju Das (2):
  dt-bindings: timer: renesas: ostm: Document Renesas RZ/G2L OSTM
  clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

 .../bindings/timer/renesas,ostm.yaml          | 29 +++++++++++---
 drivers/clocksource/renesas-ostm.c            | 38 +++++++++++++++++++
 2 files changed, 62 insertions(+), 5 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] dt-bindings: timer: renesas: ostm: Document Renesas RZ/G2L OSTM

[Biju Das]

Document the General Timer Module(a.k.a OSTM) found on the RZ/G2L SoC.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../bindings/timer/renesas,ostm.yaml          | 29 +++++++++++++++----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/renesas,ostm.yaml b/Documentation/devicetree/bindings/timer/renesas,ostm.yaml
index 600d47ab7d58..5a8ffdc8b1cb 100644
--- a/Documentation/devicetree/bindings/timer/renesas,ostm.yaml
+++ b/Documentation/devicetree/bindings/timer/renesas,ostm.yaml
@@ -19,11 +19,17 @@ description:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - renesas,r7s72100-ostm # RZ/A1H
-          - renesas,r7s9210-ostm  # RZ/A2M
-      - const: renesas,ostm       # Generic
+    oneOf:
+      - items:
+          - enum:
+              - renesas,r7s72100-ostm  # RZ/A1H
+              - renesas,r7s9210-ostm   # RZ/A2M
+          - const: renesas,ostm        # Generic
+
+      - items:
+          - enum:
+              - renesas,r9a07g044-ostm # RZ/G2{L,LC}
+          - const: renesas,rzg2l-ostm  # RZ/G2L
 
   reg:
     maxItems: 1
@@ -37,6 +43,9 @@ properties:
   power-domains:
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -44,6 +53,16 @@ required:
   - clocks
   - power-domains
 
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - renesas,rzg2l-ostm
+then:
+  required:
+    - resets
+
 additionalProperties: false
 
 examples:
-- 
2.17.1

[PATCH 1/2] dt-bindings: timer: renesas: ostm: Document Renesas RZ/G2L OSTM

[Biju Das]

Document the General Timer Module(a.k.a OSTM) found on the RZ/G2L SoC.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../bindings/timer/renesas,ostm.yaml          | 29 +++++++++++++++----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/renesas,ostm.yaml b/Documentation/devicetree/bindings/timer/renesas,ostm.yaml
index 600d47ab7d58..5a8ffdc8b1cb 100644
--- a/Documentation/devicetree/bindings/timer/renesas,ostm.yaml
+++ b/Documentation/devicetree/bindings/timer/renesas,ostm.yaml
@@ -19,11 +19,17 @@ description:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - renesas,r7s72100-ostm # RZ/A1H
-          - renesas,r7s9210-ostm  # RZ/A2M
-      - const: renesas,ostm       # Generic
+    oneOf:
+      - items:
+          - enum:
+              - renesas,r7s72100-ostm  # RZ/A1H
+              - renesas,r7s9210-ostm   # RZ/A2M
+          - const: renesas,ostm        # Generic
+
+      - items:
+          - enum:
+              - renesas,r9a07g044-ostm # RZ/G2{L,LC}
+          - const: renesas,rzg2l-ostm  # RZ/G2L
 
   reg:
     maxItems: 1
@@ -37,6 +43,9 @@ properties:
   power-domains:
     maxItems: 1
 
+  resets:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -44,6 +53,16 @@ required:
   - clocks
   - power-domains
 
+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - renesas,rzg2l-ostm
+then:
+  required:
+    - resets
+
 additionalProperties: false
 
 examples:
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Biju Das]

RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
deassert the reset line before accessing any registers.

This patch adds an entry point for RZ/G2L so that we can deassert
the reset line in probe callback.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/clocksource/renesas-ostm.c | 38 ++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/clocksource/renesas-ostm.c b/drivers/clocksource/renesas-ostm.c
index 3d06ba66008c..6b28c45c86f7 100644
--- a/drivers/clocksource/renesas-ostm.c
+++ b/drivers/clocksource/renesas-ostm.c
@@ -9,6 +9,8 @@
 #include <linux/clk.h>
 #include <linux/clockchips.h>
 #include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/sched_clock.h>
 #include <linux/slab.h>
 
@@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node *np)
 }
 
 TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
+
+#ifdef CONFIG_ARCH_R9A07G044
+static int __init ostm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct reset_control *rstc;
+	int ret;
+
+	rstc = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(rstc))
+		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");
+
+	reset_control_deassert(rstc);
+
+	ret = ostm_init(dev->of_node);
+	if (ret) {
+		reset_control_assert(rstc);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id ostm_of_table[] = {
+	{ .compatible = "renesas,rzg2l-ostm", },
+	{ }
+};
+
+static struct platform_driver ostm_device_driver = {
+	.driver		= {
+		.name	= "rzg2l_ostm",
+		.of_match_table = of_match_ptr(ostm_of_table),
+	},
+};
+builtin_platform_driver_probe(ostm_device_driver, ostm_probe);
+#endif
-- 
2.17.1

[PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Biju Das]

RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
deassert the reset line before accessing any registers.

This patch adds an entry point for RZ/G2L so that we can deassert
the reset line in probe callback.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/clocksource/renesas-ostm.c | 38 ++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/clocksource/renesas-ostm.c b/drivers/clocksource/renesas-ostm.c
index 3d06ba66008c..6b28c45c86f7 100644
--- a/drivers/clocksource/renesas-ostm.c
+++ b/drivers/clocksource/renesas-ostm.c
@@ -9,6 +9,8 @@
 #include <linux/clk.h>
 #include <linux/clockchips.h>
 #include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/sched_clock.h>
 #include <linux/slab.h>
 
@@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node *np)
 }
 
 TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
+
+#ifdef CONFIG_ARCH_R9A07G044
+static int __init ostm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct reset_control *rstc;
+	int ret;
+
+	rstc = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(rstc))
+		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");
+
+	reset_control_deassert(rstc);
+
+	ret = ostm_init(dev->of_node);
+	if (ret) {
+		reset_control_assert(rstc);
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id ostm_of_table[] = {
+	{ .compatible = "renesas,rzg2l-ostm", },
+	{ }
+};
+
+static struct platform_driver ostm_device_driver = {
+	.driver		= {
+		.name	= "rzg2l_ostm",
+		.of_match_table = of_match_ptr(ostm_of_table),
+	},
+};
+builtin_platform_driver_probe(ostm_device_driver, ostm_probe);
+#endif
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Philipp Zabel]

On Wed, 2021-11-10 at 08:31 +0000, Biju Das wrote:
[...]
> +#ifdef CONFIG_ARCH_R9A07G044
> +static int __init ostm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int ret;
> +
> +	rstc = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(rstc))
> +		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");
> +
> +	reset_control_deassert(rstc);
> +
> +	ret = ostm_init(dev->of_node);
> +	if (ret) {
> +		reset_control_assert(rstc);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ostm_of_table[] = {
> +	{ .compatible = "renesas,rzg2l-ostm", },
> +	{ }
> +};
> +
> +static struct platform_driver ostm_device_driver = {
> +	.driver		= {
> +		.name	= "rzg2l_ostm",
> +		.of_match_table = of_match_ptr(ostm_of_table),
> +	},
> +};
> +builtin_platform_driver_probe(ostm_device_driver, ostm_probe);
> +#endif

I assuming the corresponding reset controller driver is builtin as well.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Philipp Zabel]

On Wed, 2021-11-10 at 08:31 +0000, Biju Das wrote:
[...]
> +#ifdef CONFIG_ARCH_R9A07G044
> +static int __init ostm_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct reset_control *rstc;
> +	int ret;
> +
> +	rstc = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(rstc))
> +		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");
> +
> +	reset_control_deassert(rstc);
> +
> +	ret = ostm_init(dev->of_node);
> +	if (ret) {
> +		reset_control_assert(rstc);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ostm_of_table[] = {
> +	{ .compatible = "renesas,rzg2l-ostm", },
> +	{ }
> +};
> +
> +static struct platform_driver ostm_device_driver = {
> +	.driver		= {
> +		.name	= "rzg2l_ostm",
> +		.of_match_table = of_match_ptr(ostm_of_table),
> +	},
> +};
> +builtin_platform_driver_probe(ostm_device_driver, ostm_probe);
> +#endif

I assuming the corresponding reset controller driver is builtin as well.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Biju Das]

Hi Philipp Zabel,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM
> support
> 
> On Wed, 2021-11-10 at 08:31 +0000, Biju Das wrote:
> [...]
> > +#ifdef CONFIG_ARCH_R9A07G044
> > +static int __init ostm_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct reset_control *rstc;
> > +	int ret;
> > +
> > +	rstc = devm_reset_control_get_exclusive(dev, NULL);
> > +	if (IS_ERR(rstc))
> > +		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get
> reset");
> > +
> > +	reset_control_deassert(rstc);
> > +
> > +	ret = ostm_init(dev->of_node);
> > +	if (ret) {
> > +		reset_control_assert(rstc);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ostm_of_table[] = {
> > +	{ .compatible = "renesas,rzg2l-ostm", },
> > +	{ }
> > +};
> > +
> > +static struct platform_driver ostm_device_driver = {
> > +	.driver		= {
> > +		.name	= "rzg2l_ostm",
> > +		.of_match_table = of_match_ptr(ostm_of_table),
> > +	},
> > +};
> > +builtin_platform_driver_probe(ostm_device_driver, ostm_probe); #endif
> 
> I assuming the corresponding reset controller driver is builtin as well.

Indeed.

Cheers,
Biju

> 
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> regards
> Philipp

RE: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Biju Das]

Hi Philipp Zabel,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM
> support
> 
> On Wed, 2021-11-10 at 08:31 +0000, Biju Das wrote:
> [...]
> > +#ifdef CONFIG_ARCH_R9A07G044
> > +static int __init ostm_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct reset_control *rstc;
> > +	int ret;
> > +
> > +	rstc = devm_reset_control_get_exclusive(dev, NULL);
> > +	if (IS_ERR(rstc))
> > +		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get
> reset");
> > +
> > +	reset_control_deassert(rstc);
> > +
> > +	ret = ostm_init(dev->of_node);
> > +	if (ret) {
> > +		reset_control_assert(rstc);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id ostm_of_table[] = {
> > +	{ .compatible = "renesas,rzg2l-ostm", },
> > +	{ }
> > +};
> > +
> > +static struct platform_driver ostm_device_driver = {
> > +	.driver		= {
> > +		.name	= "rzg2l_ostm",
> > +		.of_match_table = of_match_ptr(ostm_of_table),
> > +	},
> > +};
> > +builtin_platform_driver_probe(ostm_device_driver, ostm_probe); #endif
> 
> I assuming the corresponding reset controller driver is builtin as well.

Indeed.

Cheers,
Biju

> 
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> regards
> Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Geert Uytterhoeven]

Hi Biju,

On Wed, Nov 10, 2021 at 9:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> deassert the reset line before accessing any registers.
>
> This patch adds an entry point for RZ/G2L so that we can deassert
> the reset line in probe callback.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clocksource/renesas-ostm.c
> +++ b/drivers/clocksource/renesas-ostm.c
> @@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node *np)
>  }
>
>  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);

Background: this driver uses TIMER_OF_DECLARE() because the OSTM
is the system timer on RZ/A SoCs, which do not have the ARM architectured
timer.  RZ/G2L does have the ARM architectured timer.

> +
> +#ifdef CONFIG_ARCH_R9A07G044
> +static int __init ostm_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct reset_control *rstc;
> +       int ret;
> +
> +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> +       if (IS_ERR(rstc))
> +               return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");
> +
> +       reset_control_deassert(rstc);
> +
> +       ret = ostm_init(dev->of_node);
> +       if (ret) {
> +               reset_control_assert(rstc);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id ostm_of_table[] = {
> +       { .compatible = "renesas,rzg2l-ostm", },

I believe the OSTM block on RZ/G2L is identical to the one on RZ/A,
and the requirement to deassert its module reset is an SoC integration
feature on RZ/G2L.  Hence the driver should match on "renesas,ostm"
for both?

So my suggestion would be to include the reset handling in ostm_init()
instead, but make it optional, and error out in case of -EPROBE_DEFER.
In case initialization from TIMER_OF_DECLARE() failed, the platform
driver can kick in later and retry.

However, it seems __of_reset_control_get() ignores all errors,
including -EPROBE_DEFER, if optional is true, so this won't work?
Philipp: is that correct? If yes, ostm_init() has to check the presence
of a resets property to see if the reset is optional or required.

> +       { }
> +};
> +
> +static struct platform_driver ostm_device_driver = {
> +       .driver         = {
> +               .name   = "rzg2l_ostm",
> +               .of_match_table = of_match_ptr(ostm_of_table),
> +       },
> +};
> +builtin_platform_driver_probe(ostm_device_driver, ostm_probe);
> +#endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Geert Uytterhoeven]

Hi Biju,

On Wed, Nov 10, 2021 at 9:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> deassert the reset line before accessing any registers.
>
> This patch adds an entry point for RZ/G2L so that we can deassert
> the reset line in probe callback.
>
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/clocksource/renesas-ostm.c
> +++ b/drivers/clocksource/renesas-ostm.c
> @@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node *np)
>  }
>
>  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);

Background: this driver uses TIMER_OF_DECLARE() because the OSTM
is the system timer on RZ/A SoCs, which do not have the ARM architectured
timer.  RZ/G2L does have the ARM architectured timer.

> +
> +#ifdef CONFIG_ARCH_R9A07G044
> +static int __init ostm_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct reset_control *rstc;
> +       int ret;
> +
> +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> +       if (IS_ERR(rstc))
> +               return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");
> +
> +       reset_control_deassert(rstc);
> +
> +       ret = ostm_init(dev->of_node);
> +       if (ret) {
> +               reset_control_assert(rstc);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id ostm_of_table[] = {
> +       { .compatible = "renesas,rzg2l-ostm", },

I believe the OSTM block on RZ/G2L is identical to the one on RZ/A,
and the requirement to deassert its module reset is an SoC integration
feature on RZ/G2L.  Hence the driver should match on "renesas,ostm"
for both?

So my suggestion would be to include the reset handling in ostm_init()
instead, but make it optional, and error out in case of -EPROBE_DEFER.
In case initialization from TIMER_OF_DECLARE() failed, the platform
driver can kick in later and retry.

However, it seems __of_reset_control_get() ignores all errors,
including -EPROBE_DEFER, if optional is true, so this won't work?
Philipp: is that correct? If yes, ostm_init() has to check the presence
of a resets property to see if the reset is optional or required.

> +       { }
> +};
> +
> +static struct platform_driver ostm_device_driver = {
> +       .driver         = {
> +               .name   = "rzg2l_ostm",
> +               .of_match_table = of_match_ptr(ostm_of_table),
> +       },
> +};
> +builtin_platform_driver_probe(ostm_device_driver, ostm_probe);
> +#endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Philipp Zabel]

Hi Geert, Biju,

On Wed, 2021-11-10 at 11:27 +0100, Geert Uytterhoeven wrote:
> Hi Biju,
> 
> On Wed, Nov 10, 2021 at 9:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> > deassert the reset line before accessing any registers.
> > 
> > This patch adds an entry point for RZ/G2L so that we can deassert
> > the reset line in probe callback.
> > 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clocksource/renesas-ostm.c
> > +++ b/drivers/clocksource/renesas-ostm.c
> > @@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node *np)
> >  }
> > 
> >  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> 
> Background: this driver uses TIMER_OF_DECLARE() because the OSTM
> is the system timer on RZ/A SoCs, which do not have the ARM architectured
> timer.  RZ/G2L does have the ARM architectured timer.

Thanks.

> > +
> > +#ifdef CONFIG_ARCH_R9A07G044
> > +static int __init ostm_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct reset_control *rstc;
> > +       int ret;
> > +
> > +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> > +       if (IS_ERR(rstc))
> > +               return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");
> > +
> > +       reset_control_deassert(rstc);
> > +
> > +       ret = ostm_init(dev->of_node);
> > +       if (ret) {
> > +               reset_control_assert(rstc);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id ostm_of_table[] = {
> > +       { .compatible = "renesas,rzg2l-ostm", },
> 
> I believe the OSTM block on RZ/G2L is identical to the one on RZ/A,
> and the requirement to deassert its module reset is an SoC integration
> feature on RZ/G2L.  Hence the driver should match on "renesas,ostm"
> for both?

If that is the case, the reset could be made required for
  compatible = "renesas,r9a07g044-ostm", "renesas,ostm";
in the .yaml file.

> So my suggestion would be to include the reset handling in ostm_init()
> instead, but make it optional, and error out in case of -EPROBE_DEFER.
>
> In case initialization from TIMER_OF_DECLARE() failed, the platform
> driver can kick in later and retry.
> 
> However, it seems __of_reset_control_get() ignores all errors,
> including -EPROBE_DEFER, if optional is true, so this won't work?
>
> Philipp: is that correct? If yes, ostm_init() has to check the presence
> of a resets property to see if the reset is optional or required.

No, __of_reset_control_get() should only replace its -ENOENT return
value due to errors from of_property_match_string() and
of_parse_phandle_with_args() with NULL. Anything else I'd consider a
bug.

Specifically, -EPROBE_DEFER is still returned if no existing rcdev is
found matching the successful "resets" phandle lookup. So

	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
	if (IS_ERR(rstc))
		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");

	reset_control_deassert(rstc);

added to ostm_init() should work. Note that platform_probe() will throw
an additional warning if -EPROBE_DEFER is returned from non-hotpluggable
drivers (and turn it into -ENXIO).

regards
Philipp

Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Philipp Zabel]

Hi Geert, Biju,

On Wed, 2021-11-10 at 11:27 +0100, Geert Uytterhoeven wrote:
> Hi Biju,
> 
> On Wed, Nov 10, 2021 at 9:32 AM Biju Das <biju.das.jz@bp.renesas.com> wrote:
> > RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> > deassert the reset line before accessing any registers.
> > 
> > This patch adds an entry point for RZ/G2L so that we can deassert
> > the reset line in probe callback.
> > 
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/clocksource/renesas-ostm.c
> > +++ b/drivers/clocksource/renesas-ostm.c
> > @@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node *np)
> >  }
> > 
> >  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> 
> Background: this driver uses TIMER_OF_DECLARE() because the OSTM
> is the system timer on RZ/A SoCs, which do not have the ARM architectured
> timer.  RZ/G2L does have the ARM architectured timer.

Thanks.

> > +
> > +#ifdef CONFIG_ARCH_R9A07G044
> > +static int __init ostm_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       struct reset_control *rstc;
> > +       int ret;
> > +
> > +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> > +       if (IS_ERR(rstc))
> > +               return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");
> > +
> > +       reset_control_deassert(rstc);
> > +
> > +       ret = ostm_init(dev->of_node);
> > +       if (ret) {
> > +               reset_control_assert(rstc);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static const struct of_device_id ostm_of_table[] = {
> > +       { .compatible = "renesas,rzg2l-ostm", },
> 
> I believe the OSTM block on RZ/G2L is identical to the one on RZ/A,
> and the requirement to deassert its module reset is an SoC integration
> feature on RZ/G2L.  Hence the driver should match on "renesas,ostm"
> for both?

If that is the case, the reset could be made required for
  compatible = "renesas,r9a07g044-ostm", "renesas,ostm";
in the .yaml file.

> So my suggestion would be to include the reset handling in ostm_init()
> instead, but make it optional, and error out in case of -EPROBE_DEFER.
>
> In case initialization from TIMER_OF_DECLARE() failed, the platform
> driver can kick in later and retry.
> 
> However, it seems __of_reset_control_get() ignores all errors,
> including -EPROBE_DEFER, if optional is true, so this won't work?
>
> Philipp: is that correct? If yes, ostm_init() has to check the presence
> of a resets property to see if the reset is optional or required.

No, __of_reset_control_get() should only replace its -ENOENT return
value due to errors from of_property_match_string() and
of_parse_phandle_with_args() with NULL. Anything else I'd consider a
bug.

Specifically, -EPROBE_DEFER is still returned if no existing rcdev is
found matching the successful "resets" phandle lookup. So

	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
	if (IS_ERR(rstc))
		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get reset");

	reset_control_deassert(rstc);

added to ostm_init() should work. Note that platform_probe() will throw
an additional warning if -EPROBE_DEFER is returned from non-hotpluggable
drivers (and turn it into -ENXIO).

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Biju Das]

Hi Philipp and Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM
> support
> 
> Hi Geert, Biju,
> 
> On Wed, 2021-11-10 at 11:27 +0100, Geert Uytterhoeven wrote:
> > Hi Biju,
> >
> > On Wed, Nov 10, 2021 at 9:32 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> > > deassert the reset line before accessing any registers.
> > >
> > > This patch adds an entry point for RZ/G2L so that we can deassert
> > > the reset line in probe callback.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/clocksource/renesas-ostm.c
> > > +++ b/drivers/clocksource/renesas-ostm.c
> > > @@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node
> > > *np)
> > >  }
> > >
> > >  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> >
> > Background: this driver uses TIMER_OF_DECLARE() because the OSTM is
> > the system timer on RZ/A SoCs, which do not have the ARM architectured
> > timer.  RZ/G2L does have the ARM architectured timer.
> 
> Thanks.
> 
> > > +
> > > +#ifdef CONFIG_ARCH_R9A07G044
> > > +static int __init ostm_probe(struct platform_device *pdev) {
> > > +       struct device *dev = &pdev->dev;
> > > +       struct reset_control *rstc;
> > > +       int ret;
> > > +
> > > +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> > > +       if (IS_ERR(rstc))
> > > +               return dev_err_probe(dev, PTR_ERR(rstc), "failed to
> > > + get reset");
> > > +
> > > +       reset_control_deassert(rstc);
> > > +
> > > +       ret = ostm_init(dev->of_node);
> > > +       if (ret) {
> > > +               reset_control_assert(rstc);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct of_device_id ostm_of_table[] = {
> > > +       { .compatible = "renesas,rzg2l-ostm", },
> >
> > I believe the OSTM block on RZ/G2L is identical to the one on RZ/A,
> > and the requirement to deassert its module reset is an SoC integration
> > feature on RZ/G2L.  Hence the driver should match on "renesas,ostm"
> > for both?
> 
> If that is the case, the reset could be made required for
>   compatible = "renesas,r9a07g044-ostm", "renesas,ostm"; in the .yaml
> file.
> 
> > So my suggestion would be to include the reset handling in ostm_init()
> > instead, but make it optional, and error out in case of -EPROBE_DEFER.
> >
> > In case initialization from TIMER_OF_DECLARE() failed, the platform
> > driver can kick in later and retry.
> >
> > However, it seems __of_reset_control_get() ignores all errors,
> > including -EPROBE_DEFER, if optional is true, so this won't work?
> >
> > Philipp: is that correct? If yes, ostm_init() has to check the
> > presence of a resets property to see if the reset is optional or
> required.
> 
> No, __of_reset_control_get() should only replace its -ENOENT return value
> due to errors from of_property_match_string() and
> of_parse_phandle_with_args() with NULL. Anything else I'd consider a bug.
> 
> Specifically, -EPROBE_DEFER is still returned if no existing rcdev is
> found matching the successful "resets" phandle lookup. So
> 
> 	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);

In this case, How do we get dev here from device_node, as device is not available at this point?

Regards,
Biju

> 	if (IS_ERR(rstc))
> 		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get
> reset");
> 
> 	reset_control_deassert(rstc);
> 
> added to ostm_init() should work. Note that platform_probe() will throw an
> additional warning if -EPROBE_DEFER is returned from non-hotpluggable
> drivers (and turn it into -ENXIO).
> 
> regards
> Philipp

RE: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Biju Das]

Hi Philipp and Geert,

Thanks for the feedback.

> Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM
> support
> 
> Hi Geert, Biju,
> 
> On Wed, 2021-11-10 at 11:27 +0100, Geert Uytterhoeven wrote:
> > Hi Biju,
> >
> > On Wed, Nov 10, 2021 at 9:32 AM Biju Das <biju.das.jz@bp.renesas.com>
> wrote:
> > > RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> > > deassert the reset line before accessing any registers.
> > >
> > > This patch adds an entry point for RZ/G2L so that we can deassert
> > > the reset line in probe callback.
> > >
> > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Thanks for your patch!
> >
> > > --- a/drivers/clocksource/renesas-ostm.c
> > > +++ b/drivers/clocksource/renesas-ostm.c
> > > @@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node
> > > *np)
> > >  }
> > >
> > >  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> >
> > Background: this driver uses TIMER_OF_DECLARE() because the OSTM is
> > the system timer on RZ/A SoCs, which do not have the ARM architectured
> > timer.  RZ/G2L does have the ARM architectured timer.
> 
> Thanks.
> 
> > > +
> > > +#ifdef CONFIG_ARCH_R9A07G044
> > > +static int __init ostm_probe(struct platform_device *pdev) {
> > > +       struct device *dev = &pdev->dev;
> > > +       struct reset_control *rstc;
> > > +       int ret;
> > > +
> > > +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> > > +       if (IS_ERR(rstc))
> > > +               return dev_err_probe(dev, PTR_ERR(rstc), "failed to
> > > + get reset");
> > > +
> > > +       reset_control_deassert(rstc);
> > > +
> > > +       ret = ostm_init(dev->of_node);
> > > +       if (ret) {
> > > +               reset_control_assert(rstc);
> > > +               return ret;
> > > +       }
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct of_device_id ostm_of_table[] = {
> > > +       { .compatible = "renesas,rzg2l-ostm", },
> >
> > I believe the OSTM block on RZ/G2L is identical to the one on RZ/A,
> > and the requirement to deassert its module reset is an SoC integration
> > feature on RZ/G2L.  Hence the driver should match on "renesas,ostm"
> > for both?
> 
> If that is the case, the reset could be made required for
>   compatible = "renesas,r9a07g044-ostm", "renesas,ostm"; in the .yaml
> file.
> 
> > So my suggestion would be to include the reset handling in ostm_init()
> > instead, but make it optional, and error out in case of -EPROBE_DEFER.
> >
> > In case initialization from TIMER_OF_DECLARE() failed, the platform
> > driver can kick in later and retry.
> >
> > However, it seems __of_reset_control_get() ignores all errors,
> > including -EPROBE_DEFER, if optional is true, so this won't work?
> >
> > Philipp: is that correct? If yes, ostm_init() has to check the
> > presence of a resets property to see if the reset is optional or
> required.
> 
> No, __of_reset_control_get() should only replace its -ENOENT return value
> due to errors from of_property_match_string() and
> of_parse_phandle_with_args() with NULL. Anything else I'd consider a bug.
> 
> Specifically, -EPROBE_DEFER is still returned if no existing rcdev is
> found matching the successful "resets" phandle lookup. So
> 
> 	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);

In this case, How do we get dev here from device_node, as device is not available at this point?

Regards,
Biju

> 	if (IS_ERR(rstc))
> 		return dev_err_probe(dev, PTR_ERR(rstc), "failed to get
> reset");
> 
> 	reset_control_deassert(rstc);
> 
> added to ostm_init() should work. Note that platform_probe() will throw an
> additional warning if -EPROBE_DEFER is returned from non-hotpluggable
> drivers (and turn it into -ENXIO).
> 
> regards
> Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Philipp Zabel]

On Wed, 2021-11-10 at 11:37 +0000, Biju Das wrote:
> Hi Philipp and Geert,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM
> > support
> > 
> > Hi Geert, Biju,
> > 
> > On Wed, 2021-11-10 at 11:27 +0100, Geert Uytterhoeven wrote:
> > > Hi Biju,
> > > 
> > > On Wed, Nov 10, 2021 at 9:32 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> > > > deassert the reset line before accessing any registers.
> > > > 
> > > > This patch adds an entry point for RZ/G2L so that we can deassert
> > > > the reset line in probe callback.
> > > > 
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > 
> > > Thanks for your patch!
> > > 
> > > > --- a/drivers/clocksource/renesas-ostm.c
> > > > +++ b/drivers/clocksource/renesas-ostm.c
> > > > @@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node
> > > > *np)
> > > >  }
> > > > 
> > > >  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> > > 
> > > Background: this driver uses TIMER_OF_DECLARE() because the OSTM is
> > > the system timer on RZ/A SoCs, which do not have the ARM architectured
> > > timer.  RZ/G2L does have the ARM architectured timer.
> > 
> > Thanks.
> > 
> > > > +
> > > > +#ifdef CONFIG_ARCH_R9A07G044
> > > > +static int __init ostm_probe(struct platform_device *pdev) {
> > > > +       struct device *dev = &pdev->dev;
> > > > +       struct reset_control *rstc;
> > > > +       int ret;
> > > > +
> > > > +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> > > > +       if (IS_ERR(rstc))
> > > > +               return dev_err_probe(dev, PTR_ERR(rstc), "failed to
> > > > + get reset");
> > > > +
> > > > +       reset_control_deassert(rstc);
> > > > +
> > > > +       ret = ostm_init(dev->of_node);
> > > > +       if (ret) {
> > > > +               reset_control_assert(rstc);
> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static const struct of_device_id ostm_of_table[] = {
> > > > +       { .compatible = "renesas,rzg2l-ostm", },
> > > 
> > > I believe the OSTM block on RZ/G2L is identical to the one on RZ/A,
> > > and the requirement to deassert its module reset is an SoC integration
> > > feature on RZ/G2L.  Hence the driver should match on "renesas,ostm"
> > > for both?
> > 
> > If that is the case, the reset could be made required for
> >   compatible = "renesas,r9a07g044-ostm", "renesas,ostm"; in the .yaml
> > file.
> > 
> > > So my suggestion would be to include the reset handling in ostm_init()
> > > instead, but make it optional, and error out in case of -EPROBE_DEFER.
> > > 
> > > In case initialization from TIMER_OF_DECLARE() failed, the platform
> > > driver can kick in later and retry.
> > > 
> > > However, it seems __of_reset_control_get() ignores all errors,
> > > including -EPROBE_DEFER, if optional is true, so this won't work?
> > > 
> > > Philipp: is that correct? If yes, ostm_init() has to check the
> > > presence of a resets property to see if the reset is optional or
> > required.
> > 
> > No, __of_reset_control_get() should only replace its -ENOENT return value
> > due to errors from of_property_match_string() and
> > of_parse_phandle_with_args() with NULL. Anything else I'd consider a bug.
> > 
> > Specifically, -EPROBE_DEFER is still returned if no existing rcdev is
> > found matching the successful "resets" phandle lookup. So
> > 
> > 	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
> 
> In this case, How do we get dev here from device_node, as device is not available at this point?

Oh, right.
We are missing an of_reset_control_get_optional_exclusive() for this:

static inline struct reset_control *of_reset_control_get_optional_exclusive(
		struct device_node *node, const char *id)
{
	return __of_reset_control_get(node, id, 0, false, true, true);
}

regards
Philipp

Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Philipp Zabel]

On Wed, 2021-11-10 at 11:37 +0000, Biju Das wrote:
> Hi Philipp and Geert,
> 
> Thanks for the feedback.
> 
> > Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM
> > support
> > 
> > Hi Geert, Biju,
> > 
> > On Wed, 2021-11-10 at 11:27 +0100, Geert Uytterhoeven wrote:
> > > Hi Biju,
> > > 
> > > On Wed, Nov 10, 2021 at 9:32 AM Biju Das <biju.das.jz@bp.renesas.com>
> > wrote:
> > > > RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> > > > deassert the reset line before accessing any registers.
> > > > 
> > > > This patch adds an entry point for RZ/G2L so that we can deassert
> > > > the reset line in probe callback.
> > > > 
> > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > 
> > > Thanks for your patch!
> > > 
> > > > --- a/drivers/clocksource/renesas-ostm.c
> > > > +++ b/drivers/clocksource/renesas-ostm.c
> > > > @@ -209,3 +211,39 @@ static int __init ostm_init(struct device_node
> > > > *np)
> > > >  }
> > > > 
> > > >  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> > > 
> > > Background: this driver uses TIMER_OF_DECLARE() because the OSTM is
> > > the system timer on RZ/A SoCs, which do not have the ARM architectured
> > > timer.  RZ/G2L does have the ARM architectured timer.
> > 
> > Thanks.
> > 
> > > > +
> > > > +#ifdef CONFIG_ARCH_R9A07G044
> > > > +static int __init ostm_probe(struct platform_device *pdev) {
> > > > +       struct device *dev = &pdev->dev;
> > > > +       struct reset_control *rstc;
> > > > +       int ret;
> > > > +
> > > > +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> > > > +       if (IS_ERR(rstc))
> > > > +               return dev_err_probe(dev, PTR_ERR(rstc), "failed to
> > > > + get reset");
> > > > +
> > > > +       reset_control_deassert(rstc);
> > > > +
> > > > +       ret = ostm_init(dev->of_node);
> > > > +       if (ret) {
> > > > +               reset_control_assert(rstc);
> > > > +               return ret;
> > > > +       }
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +
> > > > +static const struct of_device_id ostm_of_table[] = {
> > > > +       { .compatible = "renesas,rzg2l-ostm", },
> > > 
> > > I believe the OSTM block on RZ/G2L is identical to the one on RZ/A,
> > > and the requirement to deassert its module reset is an SoC integration
> > > feature on RZ/G2L.  Hence the driver should match on "renesas,ostm"
> > > for both?
> > 
> > If that is the case, the reset could be made required for
> >   compatible = "renesas,r9a07g044-ostm", "renesas,ostm"; in the .yaml
> > file.
> > 
> > > So my suggestion would be to include the reset handling in ostm_init()
> > > instead, but make it optional, and error out in case of -EPROBE_DEFER.
> > > 
> > > In case initialization from TIMER_OF_DECLARE() failed, the platform
> > > driver can kick in later and retry.
> > > 
> > > However, it seems __of_reset_control_get() ignores all errors,
> > > including -EPROBE_DEFER, if optional is true, so this won't work?
> > > 
> > > Philipp: is that correct? If yes, ostm_init() has to check the
> > > presence of a resets property to see if the reset is optional or
> > required.
> > 
> > No, __of_reset_control_get() should only replace its -ENOENT return value
> > due to errors from of_property_match_string() and
> > of_parse_phandle_with_args() with NULL. Anything else I'd consider a bug.
> > 
> > Specifically, -EPROBE_DEFER is still returned if no existing rcdev is
> > found matching the successful "resets" phandle lookup. So
> > 
> > 	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
> 
> In this case, How do we get dev here from device_node, as device is not available at this point?

Oh, right.
We are missing an of_reset_control_get_optional_exclusive() for this:

static inline struct reset_control *of_reset_control_get_optional_exclusive(
		struct device_node *node, const char *id)
{
	return __of_reset_control_get(node, id, 0, false, true, true);
}

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Biju Das]

Hi Philipp,

> Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM
> support
> 
> On Wed, 2021-11-10 at 11:37 +0000, Biju Das wrote:
> > Hi Philipp and Geert,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add
> > > RZ/G2L OSTM support
> > >
> > > Hi Geert, Biju,
> > >
> > > On Wed, 2021-11-10 at 11:27 +0100, Geert Uytterhoeven wrote:
> > > > Hi Biju,
> > > >
> > > > On Wed, Nov 10, 2021 at 9:32 AM Biju Das
> > > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> > > > > deassert the reset line before accessing any registers.
> > > > >
> > > > > This patch adds an entry point for RZ/G2L so that we can
> > > > > deassert the reset line in probe callback.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > Reviewed-by: Lad Prabhakar
> > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/clocksource/renesas-ostm.c
> > > > > +++ b/drivers/clocksource/renesas-ostm.c
> > > > > @@ -209,3 +211,39 @@ static int __init ostm_init(struct
> > > > > device_node
> > > > > *np)
> > > > >  }
> > > > >
> > > > >  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> > > >
> > > > Background: this driver uses TIMER_OF_DECLARE() because the OSTM
> > > > is the system timer on RZ/A SoCs, which do not have the ARM
> > > > architectured timer.  RZ/G2L does have the ARM architectured timer.
> > >
> > > Thanks.
> > >
> > > > > +
> > > > > +#ifdef CONFIG_ARCH_R9A07G044
> > > > > +static int __init ostm_probe(struct platform_device *pdev) {
> > > > > +       struct device *dev = &pdev->dev;
> > > > > +       struct reset_control *rstc;
> > > > > +       int ret;
> > > > > +
> > > > > +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> > > > > +       if (IS_ERR(rstc))
> > > > > +               return dev_err_probe(dev, PTR_ERR(rstc), "failed
> > > > > + to get reset");
> > > > > +
> > > > > +       reset_control_deassert(rstc);
> > > > > +
> > > > > +       ret = ostm_init(dev->of_node);
> > > > > +       if (ret) {
> > > > > +               reset_control_assert(rstc);
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct of_device_id ostm_of_table[] = {
> > > > > +       { .compatible = "renesas,rzg2l-ostm", },
> > > >
> > > > I believe the OSTM block on RZ/G2L is identical to the one on
> > > > RZ/A, and the requirement to deassert its module reset is an SoC
> > > > integration feature on RZ/G2L.  Hence the driver should match on
> "renesas,ostm"
> > > > for both?
> > >
> > > If that is the case, the reset could be made required for
> > >   compatible = "renesas,r9a07g044-ostm", "renesas,ostm"; in the
> > > .yaml file.
> > >
> > > > So my suggestion would be to include the reset handling in
> > > > ostm_init() instead, but make it optional, and error out in case of
> -EPROBE_DEFER.
> > > >
> > > > In case initialization from TIMER_OF_DECLARE() failed, the
> > > > platform driver can kick in later and retry.
> > > >
> > > > However, it seems __of_reset_control_get() ignores all errors,
> > > > including -EPROBE_DEFER, if optional is true, so this won't work?
> > > >
> > > > Philipp: is that correct? If yes, ostm_init() has to check the
> > > > presence of a resets property to see if the reset is optional or
> > > required.
> > >
> > > No, __of_reset_control_get() should only replace its -ENOENT return
> > > value due to errors from of_property_match_string() and
> > > of_parse_phandle_with_args() with NULL. Anything else I'd consider a
> bug.
> > >
> > > Specifically, -EPROBE_DEFER is still returned if no existing rcdev
> > > is found matching the successful "resets" phandle lookup. So
> > >
> > > 	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
> >
> > In this case, How do we get dev here from device_node, as device is not
> available at this point?
> 
> Oh, right.
> We are missing an of_reset_control_get_optional_exclusive() for this:
> 
> static inline struct reset_control
> *of_reset_control_get_optional_exclusive(
> 		struct device_node *node, const char *id) {
> 	return __of_reset_control_get(node, id, 0, false, true, true); }

I have tested OSTM driver with this new API and works.

How do we proceed? Will you submit this patch separate in reset tree ? or Do you want me to send this patch 
as v2 for this series?

Please let me know.

Regards,
Biju 
> 
> regards
> Philipp

RE: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM support

[Biju Das]

Hi Philipp,

> Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add RZ/G2L OSTM
> support
> 
> On Wed, 2021-11-10 at 11:37 +0000, Biju Das wrote:
> > Hi Philipp and Geert,
> >
> > Thanks for the feedback.
> >
> > > Subject: Re: [PATCH 2/2] clocksource/drivers/renesas-ostm: Add
> > > RZ/G2L OSTM support
> > >
> > > Hi Geert, Biju,
> > >
> > > On Wed, 2021-11-10 at 11:27 +0100, Geert Uytterhoeven wrote:
> > > > Hi Biju,
> > > >
> > > > On Wed, Nov 10, 2021 at 9:32 AM Biju Das
> > > > <biju.das.jz@bp.renesas.com>
> > > wrote:
> > > > > RZ/G2L SoC has Generic Timer Module(a.k.a OSTM) which needs to
> > > > > deassert the reset line before accessing any registers.
> > > > >
> > > > > This patch adds an entry point for RZ/G2L so that we can
> > > > > deassert the reset line in probe callback.
> > > > >
> > > > > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > > > Reviewed-by: Lad Prabhakar
> > > > > <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > > --- a/drivers/clocksource/renesas-ostm.c
> > > > > +++ b/drivers/clocksource/renesas-ostm.c
> > > > > @@ -209,3 +211,39 @@ static int __init ostm_init(struct
> > > > > device_node
> > > > > *np)
> > > > >  }
> > > > >
> > > > >  TIMER_OF_DECLARE(ostm, "renesas,ostm", ostm_init);
> > > >
> > > > Background: this driver uses TIMER_OF_DECLARE() because the OSTM
> > > > is the system timer on RZ/A SoCs, which do not have the ARM
> > > > architectured timer.  RZ/G2L does have the ARM architectured timer.
> > >
> > > Thanks.
> > >
> > > > > +
> > > > > +#ifdef CONFIG_ARCH_R9A07G044
> > > > > +static int __init ostm_probe(struct platform_device *pdev) {
> > > > > +       struct device *dev = &pdev->dev;
> > > > > +       struct reset_control *rstc;
> > > > > +       int ret;
> > > > > +
> > > > > +       rstc = devm_reset_control_get_exclusive(dev, NULL);
> > > > > +       if (IS_ERR(rstc))
> > > > > +               return dev_err_probe(dev, PTR_ERR(rstc), "failed
> > > > > + to get reset");
> > > > > +
> > > > > +       reset_control_deassert(rstc);
> > > > > +
> > > > > +       ret = ostm_init(dev->of_node);
> > > > > +       if (ret) {
> > > > > +               reset_control_assert(rstc);
> > > > > +               return ret;
> > > > > +       }
> > > > > +
> > > > > +       return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct of_device_id ostm_of_table[] = {
> > > > > +       { .compatible = "renesas,rzg2l-ostm", },
> > > >
> > > > I believe the OSTM block on RZ/G2L is identical to the one on
> > > > RZ/A, and the requirement to deassert its module reset is an SoC
> > > > integration feature on RZ/G2L.  Hence the driver should match on
> "renesas,ostm"
> > > > for both?
> > >
> > > If that is the case, the reset could be made required for
> > >   compatible = "renesas,r9a07g044-ostm", "renesas,ostm"; in the
> > > .yaml file.
> > >
> > > > So my suggestion would be to include the reset handling in
> > > > ostm_init() instead, but make it optional, and error out in case of
> -EPROBE_DEFER.
> > > >
> > > > In case initialization from TIMER_OF_DECLARE() failed, the
> > > > platform driver can kick in later and retry.
> > > >
> > > > However, it seems __of_reset_control_get() ignores all errors,
> > > > including -EPROBE_DEFER, if optional is true, so this won't work?
> > > >
> > > > Philipp: is that correct? If yes, ostm_init() has to check the
> > > > presence of a resets property to see if the reset is optional or
> > > required.
> > >
> > > No, __of_reset_control_get() should only replace its -ENOENT return
> > > value due to errors from of_property_match_string() and
> > > of_parse_phandle_with_args() with NULL. Anything else I'd consider a
> bug.
> > >
> > > Specifically, -EPROBE_DEFER is still returned if no existing rcdev
> > > is found matching the successful "resets" phandle lookup. So
> > >
> > > 	rstc = devm_reset_control_get_optional_exclusive(dev, NULL);
> >
> > In this case, How do we get dev here from device_node, as device is not
> available at this point?
> 
> Oh, right.
> We are missing an of_reset_control_get_optional_exclusive() for this:
> 
> static inline struct reset_control
> *of_reset_control_get_optional_exclusive(
> 		struct device_node *node, const char *id) {
> 	return __of_reset_control_get(node, id, 0, false, true, true); }

I have tested OSTM driver with this new API and works.

How do we proceed? Will you submit this patch separate in reset tree ? or Do you want me to send this patch 
as v2 for this series?

Please let me know.

Regards,
Biju 
> 
> regards
> Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel