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@xxxxxxxxxxxxxx>
> > > 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@xxxxxxxxxxxxxx>
> > > > > Reviewed-by: Lad Prabhakar
> > > > > <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > >
> > > > 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
