[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 at 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 at bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj at 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