Re: [RFC PATCH v3 22/35] drivers/clocksource/sh_tmu: Add support CLOCKSOURCE.

On Thu, 19 Oct 2023 01:04:57 +0900,
Geert Uytterhoeven wrote:
> 
> Hi Sato-san,
> 
> Thanks for your patch!
> 
> On Sat, Oct 14, 2023 at 4:54 PM Yoshinori Sato
> <[email protected]> wrote:
> > Enables registration as a Clocksource in the case of OF.
> 
> I think this is not a good description.
> What this patch does, is to add support for early registration using
> TIMER_OF_DECLARE(), so the timer can be used as a clocksource
> on SoCs that do not have any other suitable timer.
> 
> Then I wondered: do you need this?  On R-Mobile A1, the TMU is also
> used as a clocksource, how come it works there?
> The trick is to set preset_lpj based on the CPU core clock frequency.
> 
> I see your v2 actually added that, but you dropped the code in v3.
> https://lore.kernel.org/linux-sh/236185b4a47f303332aafeacadd9c9652e650062.1694596125.git.ysato@users.sourceforge.jp
> Nevertheless, it doesn't work anymore, as you also removed the
> clock-frequency property from cpu@0 in DT...
> Adding that makes it work without this TMU patch.

Currently sh initializes the timer with a parameter called "earlytimer",
but this does not seem to work correctly with CONFIG_OF.
It will be initialized later when of_platform_depopulate is called,
so it can be started, but I think this is incorrect initialization
of the timer.
I think it's better to follow the OF procedure rather than the old
SH-specific framework.

> > Signed-off-by: Yoshinori Sato <[email protected]>
> 
> > --- a/drivers/clocksource/sh_tmu.c
> > +++ b/drivers/clocksource/sh_tmu.c
> 
> > @@ -403,7 +411,8 @@ static void sh_tmu_clock_event_resume(struct clock_event_device *ced)
> >  }
> >
> >  static void sh_tmu_register_clockevent(struct sh_tmu_channel *ch,
> > -                                      const char *name)
> > +                                      const char *name,
> > +                                      struct device_node *np)
> 
> "np" is unused in this function, hence this change is unneeded.
> 
> >  {
> >         struct clock_event_device *ced = &ch->ced;
> >         int ret;
> 
> >  static int sh_tmu_register(struct sh_tmu_channel *ch, const char *name,
> > +                          struct device_node *np,
> 
> This change is unneeded...
> 
> >                            bool clockevent, bool clocksource)
> >  {
> >         if (clockevent) {
> >                 ch->tmu->has_clockevent = true;
> > -               sh_tmu_register_clockevent(ch, name);
> > +               sh_tmu_register_clockevent(ch, name, np);
> 
> ... as sh_tmu_register_clockevent() doesn't use "np".
> 
> >         } else if (clocksource) {
> >                 ch->tmu->has_clocksource = true;
> >                 sh_tmu_register_clocksource(ch, name);
> 
> > @@ -465,53 +477,59 @@ static int sh_tmu_channel_setup(struct sh_tmu_channel *ch, unsigned int index,
> >         else
> >                 ch->base = tmu->mapbase + 8 + ch->index * 12;
> >
> > -       ch->irq = platform_get_irq(tmu->pdev, index);
> > +       if (tmu->pdev)
> > +               ch->irq = platform_get_irq(tmu->pdev, index);
> > +       else
> > +               ch->irq = of_irq_get(np, index);
> 
> You can use of_irq_get() uncondtionally.
> 
> >         if (ch->irq < 0)
> >                 return ch->irq;
> >
> >         ch->cs_enabled = false;
> >         ch->enable_count = 0;
> >
> > -       return sh_tmu_register(ch, dev_name(&tmu->pdev->dev),
> > +       return sh_tmu_register(ch, tmu->name, np,
> 
> No need to pass np.
> 
> >                                clockevent, clocksource);
> >  }
> >
> > -static int sh_tmu_map_memory(struct sh_tmu_device *tmu)
> > +static int sh_tmu_map_memory(struct sh_tmu_device *tmu, struct device_node *np)
> >  {
> >         struct resource *res;
> >
> > -       res = platform_get_resource(tmu->pdev, IORESOURCE_MEM, 0);
> > -       if (!res) {
> > -               dev_err(&tmu->pdev->dev, "failed to get I/O memory\n");
> > -               return -ENXIO;
> > -       }
> > +       if (tmu->pdev) {
> > +               res = platform_get_resource(tmu->pdev, IORESOURCE_MEM, 0);
> > +               if (!res) {
> > +                       pr_err("sh_tmu failed to get I/O memory\n");
> > +                       return -ENXIO;
> > +               }
> > +
> > +               tmu->mapbase = ioremap(res->start, resource_size(res));
> > +       } else
> > +               tmu->mapbase = of_iomap(np, 0);
> 
> You can use of_iomap() unconditionally.
> 
> >
> > -       tmu->mapbase = ioremap(res->start, resource_size(res));
> >         if (tmu->mapbase == NULL)
> >                 return -ENXIO;
> >
> >         return 0;
> >  }
> >
> > -static int sh_tmu_parse_dt(struct sh_tmu_device *tmu)
> > +static int sh_tmu_parse_dt(struct sh_tmu_device *tmu, struct device_node *np)
> >  {
> > -       struct device_node *np = tmu->pdev->dev.of_node;
> > -
> >         tmu->model = SH_TMU;
> >         tmu->num_channels = 3;
> >
> >         of_property_read_u32(np, "#renesas,channels", &tmu->num_channels);
> >
> >         if (tmu->num_channels != 2 && tmu->num_channels != 3) {
> > -               dev_err(&tmu->pdev->dev, "invalid number of channels %u\n",
> > -                       tmu->num_channels);
> > +               pr_err("%s: invalid number of channels %u\n",
> > +                      tmu->name, tmu->num_channels);
> >                 return -EINVAL;
> >         }
> >
> >         return 0;
> >  }
> >
> > -static int sh_tmu_setup(struct sh_tmu_device *tmu, struct platform_device *pdev)
> > +static int sh_tmu_setup(struct sh_tmu_device *tmu,
> > +                       struct platform_device *pdev, struct device_node *np)
> >  {
> >         unsigned int i;
> >         int ret;
> 
> > @@ -531,14 +554,17 @@ static int sh_tmu_setup(struct sh_tmu_device *tmu, struct platform_device *pdev)
> >                 tmu->model = id->driver_data;
> >                 tmu->num_channels = hweight8(cfg->channels_mask);
> >         } else {
> > -               dev_err(&tmu->pdev->dev, "missing platform data\n");
> > +               pr_err("%s missing platform data\n", tmu->name);
> >                 return -ENXIO;
> >         }
> >
> >         /* Get hold of clock. */
> > -       tmu->clk = clk_get(&tmu->pdev->dev, "fck");
> > +       if (pdev)
> > +               tmu->clk = clk_get(&tmu->pdev->dev, "fck");
> > +       else
> > +               tmu->clk = of_clk_get(np, 0);
> 
> You can use of_clk_get() unconditionally.
> 
> >         if (IS_ERR(tmu->clk)) {
> > -               dev_err(&tmu->pdev->dev, "cannot get clock\n");
> > +               pr_err("%s: cannot get clock\n", tmu->name);
> >                 return PTR_ERR(tmu->clk);
> >         }
> >
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> -- 
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
> 
> 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

-- 
Yosinori Sato