Re: [PATCH v2 2/7] clocksource: Add Tegra186 timers support

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5601 bytes --]

On Fri, Apr 03, 2020 at 07:14:33PM +0300, Dmitry Osipenko wrote:
> 01.04.2020 01:19, Thierry Reding пишет:
> ...
> > +static void tmr_writel(struct tegra186_tmr *tmr, u32 value, unsigned int offset)
> > +{
> > +	writel(value, tmr->regs + offset);
> 
> relaxed?
> 
> > +}
> > +
> > +static void wdt_writel(struct tegra186_wdt *wdt, u32 value, unsigned int offset)
> > +{
> > +	writel(value, wdt->regs + offset);
> 
> relaxed?
> 
> > +}
> > +
> > +static u32 wdt_readl(struct tegra186_wdt *wdt, unsigned int offset)
> > +{
> > +	return readl(wdt->regs + offset);
> 
> relaxed?

Done.

> 
> > +}
> 
> ...
> > +static irqreturn_t tegra186_timer_irq(int irq, void *data)
> > +{
> > +	struct tegra186_timer *tegra = data;
> > +
> > +	if (tegra->wdt) {
> 
> Why this check is needed? Please see more below in regards to
> devm_request_irq().

We don't need it. However, I've changed all of these instances to
watchdog_active() calls instead to make sure we only ping the WDT
when we need to.

> 
> > +		tegra186_wdt_disable(tegra->wdt);
> > +		tegra186_wdt_enable(tegra->wdt);
> > +	}
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int tegra186_timer_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct tegra186_timer *tegra;
> > +	int err;
> > +
> > +	tegra = devm_kzalloc(dev, sizeof(*tegra), GFP_KERNEL);
> > +	if (!tegra)
> > +		return -ENOMEM;
> > +
> > +	tegra->soc = of_device_get_match_data(dev);
> > +	dev_set_drvdata(dev, tegra);
> > +	tegra->dev = dev;
> > +
> > +	tegra->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(tegra->regs))
> > +		return PTR_ERR(tegra->regs);
> > +
> > +	err = platform_get_irq(pdev, 0);
> > +	if (err < 0) {
> > +		dev_err(dev, "failed to get interrupt #0: %d\n", err);
> 
> Duplicated error message isn't needed for platform_get_irq().

Dropped.

> > +		return err;
> > +	}
> > +
> > +	tegra->irq = err;
> > +
> > +	err = devm_request_irq(dev, tegra->irq, tegra186_timer_irq,
> > +			       IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> 
> Why IRQF_ONESHOT?
> 
> And IRQF_TRIGGER_HIGH?.. the interrupt-level should come from the
> device-tree.

Yeah, I don't know how I came up with these. Probably copied them from
somewhere else. I've dropped both of these since they aren't needed.

> > +			       "tegra186-timer", tegra);
> > +	if (err < 0) {
> > +		dev_err(dev, "failed to request IRQ#%u: %d\n", tegra->irq, err);
> > +		return err;
> > +	}
> 
> Interrupt should be requested at the end of tegra186_timer_probe(),
> otherwise probe order isn't correct, leading to a potential race conditions.

I don't think there's an actual issue here because the watchdog that is
initialized below is disabled by default and won't be enabled until the
userspace explicitly asks it to. Since the watchdog is the only one to
currently generate an interrupt, this should be fine.

That said, you're right and it's safer to initialize the interrupt as
late as possible, so I've moved this to the end of the function.

> 
> > +	/* create a watchdog using a preconfigured timer */
> > +	tegra->wdt = tegra186_wdt_create(tegra, 0);
> > +	if (IS_ERR(tegra->wdt)) {
> > +		err = PTR_ERR(tegra->wdt);
> > +		dev_err(dev, "failed to create WDT: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	err = tegra186_timer_tsc_init(tegra);
> > +	if (err < 0) {
> > +		dev_err(dev, "failed to register TSC counter: %d\n", err);
> > +		return err;
> > +	}
> > +
> > +	err = tegra186_timer_osc_init(tegra);
> > +	if (err < 0) {
> > +		dev_err(dev, "failed to register OSC counter: %d\n", err);
> > +		goto unregister_tsc;
> > +	}
> > +
> > +	err = tegra186_timer_usec_init(tegra);
> > +	if (err < 0) {
> > +		dev_err(dev, "failed to register USEC counter: %d\n", err);
> > +		goto unregister_osc;
> > +	}
> > +
> > +	return 0;
> > +

And added an unregister_usec: label here to clean up the USEC
clocksource if we fail to request the IRQ.

> > +unregister_osc:
> > +	clocksource_unregister(&tegra->osc);
> > +unregister_tsc:
> > +	clocksource_unregister(&tegra->tsc);
> 
> Looks like there is an opportunity for devm_clocksource_register_hz().

Yeah, I guess I could follow up with a patch to do that. I suspect that
there's no such implementation because very few drivers actually end up
unregistering their clocksources. A quick grep shows that only about
one fifth of the users unregister the clocksource.

If Daniel and Thomas think this is a good idea I can look at adding that
and converting some of the users.

> > +	return err;
> > +}
> > +
> > +static int tegra186_timer_remove(struct platform_device *pdev)
> > +{
> > +	struct tegra186_timer *tegra = platform_get_drvdata(pdev);
> > +
> > +	clocksource_unregister(&tegra->usec);
> > +	clocksource_unregister(&tegra->osc);
> > +	clocksource_unregister(&tegra->tsc);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused tegra186_timer_suspend(struct device *dev)
> > +{
> > +	struct tegra186_timer *tegra = dev_get_drvdata(dev);
> > +
> > +	if (tegra->wdt)
> > +		tegra186_wdt_disable(tegra->wdt);
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused tegra186_timer_resume(struct device *dev)
> > +{
> > +	struct tegra186_timer *tegra = dev_get_drvdata(dev);
> > +
> > +	if (tegra->wdt)
> 
> Could tegra->wdt ever be NULL?

No, it can't. But as above, I've used watchdog_active() here to make
sure we only enable the watchdog when we should.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]