Message ID | 20211110083152.31144-3-biju.das.jz@bp.renesas.com (mailing list archive) |
---|---|
State | Superseded |
Delegated to: | Geert Uytterhoeven |
Headers | show |
Series | Add RZ/G2L OSTM support | expand |
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
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
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
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
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
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
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
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