Re: [PATCH 2/2] clocksource/drivers/nxp-timer: Add the System Timer Module for the s32g platform

[Daniel Lezcano]

On 25/03/2025 14:21, Ghennadi Procopciuc wrote:
> On 3/25/2025 2:51 PM, Daniel Lezcano wrote:
> [ ... ]
> > > > > > > > +static int __init nxp_stm_timer_probe(struct platform_device *pdev)
> > > > > > > > +{
> > > > > > > > +    struct device *dev = &pdev->dev;
> > > > > > > > +    struct device_node *np = dev->of_node;
> > > > > > > > +    struct stm_instances *stm_instances;
> > > > > > > > +    const char *name = of_node_full_name(np);
> > > > > > > > +    void __iomem *base;
> > > > > > > > +    int irq, ret;
> > > > > > > > +    struct clk *clk;
> > > > > > > > +
> > > > > > > > +    stm_instances =
> > > > > > > > (typeof(stm_instances))of_device_get_match_data(dev);
> > > > > > > > +    if (!stm_instances) {
> > > > > > > > +        dev_err(dev, "No STM instances associated with a cpu");
> > > > > > > > +        return -EINVAL;
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    base = devm_of_iomap(dev, np, 0, NULL);
> > > > > > > > +    if (IS_ERR(base)) {
> > > > > > > > +        dev_err(dev, "Failed to iomap %pOFn\n", np);
> > > > > > > > +        return PTR_ERR(base);
> > > > > > > > +    }
> > > > > > > > +
> > > > > > > > +    irq = irq_of_parse_and_map(np, 0);
> > > > > > > > +    if (irq <= 0) {
> > > > > > > > +        dev_err(dev, "Failed to parse and map IRQ: %d\n", irq);
> > > > > > > > +        return -EINVAL;
> > > > > > > > +    }
> > > > > > >
> > > > > > >     From commit description:
> > > > > > >
> > > > > > > > The first probed STM is used as a clocksource, the second will be
> > > > > > > > the
> > > > > > > > broadcast timer and the rest are used as a clockevent with the
> > > > > > > > affinity set to a CPU.
> > > > > > >
> > > > > > > Why is the interrupt mandatory when the node is probed as a
> > > > > > > clocksource?
> > > > > >
> > > > > > It relies on the DT description and it does not hurt to have a
> > > > > > consistent code path for clockevent / clocksource even if the
> > > > > > interrupt
> > > > > > is not requested for the clocksource later.
> > > > >
> > > > > If so, in my opinion, it would make sense to use the same STM instance
> > > > > for both the clocksource and broadcast clockevent, as both functions
> > > > > can
> > > > > be accommodated within a single STM instance, which will help reduce
> > > > > the
> > > > > number of STM instances used.
> > > >
> > > > The broadcast timer is stopped when it is unused which is the case for
> > > > the s32 because there are no idle state powering down the local timers.
> > > > They have to have be separated.
> > >
> > > This wouldn't be the case if the STM is kept running/counting during the
> > > clock event setup, with only the clock event interrupt being disabled
> > > (CCR.CEN).
> >
> > Are you asking to use two different channels for the same STM instance,
> > one for the clocksource and one for the clockevent ?
>
> I suggested using the CNT register to obtain the count for the clock
> source, while using one of the STM channels for the clock event.

Ah, ok.

I think it is preferable to keep them separated to keep the code 
modular. Given the number of STM on the platform, it does not hurt

--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog