Hi Arnaud,
[...]
>> +struct armada38x_rtc {
>> + struct rtc_device *rtc_dev;
>> + void __iomem *regs;
>> + void __iomem *regs_soc;
>> + spinlock_t lock;
>
> out of curiosity, why use a spinlock and not a mutex?
To be able to use it in the interrupt context.
>
>
>> + int irq;
>> +};
>> +
>> +/*
>> + * According to the datasheet, we need to wait for 5us between each
>> + * write
>> + */
>> +static void rtc_delayed_write(u32 val, struct armada38x_rtc *rtc, int offset)
>> +{
>> + writel(val, rtc->regs + offset);
>> + udelay(5);
>> +}
>
> In the remaining of the driver, you have various direct calls to writel()
> w/o that 5µs protection, to update/modifiy rtc registers. Is that 5µs
> trick specific to a given subpart of the registers? In that case, I
> guess it would help to update the comment.
This direct call were done because there was not a call to write just after.
As explained in the comment the 5us is needed only _between_ two consecutive
write. I can emphasize it needed.
>
>
>> +
>> +static int armada38x_rtc_read_time(struct device *dev, struct rtc_time *tm)
>> +{
>> + struct armada38x_rtc *rtc = dev_get_drvdata(dev);
>> + unsigned long time, time_check, flags;
>> +
>> + spin_lock_irqsave(&rtc->lock, flags);
>> +
>> + time = readl(rtc->regs + RTC_TIME);
>> + /*
>> + * WA for failing time set attempts. As stated in HW ERRATA if
>> + * more than one second between two time reads is detected
>> + * then read once again.
>> + */
>> + time_check = readl(rtc->regs + RTC_TIME);
>> + if ((time_check - time) > 1)
>> + time_check = readl(rtc->regs + RTC_TIME);
>
> Bear with me; I don't have access to HW ERRATA.
>
> Are you guaranteed to get a valid value at third read if you got a bad
> one before, i.e. no need to put a while loop around that workaround?
I don't have enough information to answer you, maybe the Marvell designer can
answer it. I will ask.
>
> Additionally, the workaround seems to be related to time
> setting. Looking at time setting code below, it seems the issue is also
> created by the fact the writel() call is not under the protection of the
> spinlock.
>
>> +
>> + spin_unlock_irqrestore(&rtc->lock, flags);
>> +
>> + rtc_time_to_tm(time_check, tm);
>> +
>> + return 0;
>
> Does the block provide anyway to detect the oscillator was not running,
> i.e. the value we are reading is not valid?
I didn't see anything related to this, but here again I will ask.
>
>
>> +}
>> +
>> +static int armada38x_rtc_set_time(struct device *dev, struct rtc_time *tm)
>> +{
>> + struct armada38x_rtc *rtc = dev_get_drvdata(dev);
>> + int ret = 0;
>> + unsigned long time;
>> +
>> + ret = rtc_tm_to_time(tm, &time);
>
>
>> +
>> + if (ret)
>> + goto out;
>> + /*
>> + * Setting the RTC time not always succeeds. According to the
>> + * errata we need to first write on the status register and
>> + * then wait for 1s before writing to the time register to be
>> + * sure that the data will be taken into account.
>> + */
>> + writel(0, rtc->regs + RTC_STATUS);
>> + ssleep(1);
>> +
>> + writel(time, rtc->regs + RTC_TIME);
>
> I think writel(time + 1, ...) would correct the one second shift you
> introduce using you ssleep() above.
I will just move the rtc_tm_to_time-) function here, before the second write.
>
> Additionally, as discussed above, I do not see why you're not protecting
> the write using you spinlock.
Initially the spinlock was around the 2 writel, but you can't have sleep
in a critical section hold by spinlock so I removed it. However it would be
possible to use them around each writel, but I am not sure that it makes sens.
>
>
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +static int armada38x_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>> +{
>> + struct armada38x_rtc *rtc = dev_get_drvdata(dev);
>> + unsigned long time, flags;
>> + u32 val;
>> +
>> + spin_lock_irqsave(&rtc->lock, flags);
>> +
>> + time = readl(rtc->regs + RTC_ALARM1);
>> + val = readl(rtc->regs + RTC_IRQ1_CONF) & RTC_IRQ1_AL_EN;
>> +
>> + spin_unlock_irqrestore(&rtc->lock, flags);
>> +
>> + alrm->enabled = val ? 1 : 0;
>> + rtc_time_to_tm(time, &alrm->time);
>> +
>> + return 0;
>> +}
>> +
>
> In the two functions below, regarding RTC interrupt activation, do you
> need a check that a valid IRQ was passed in the .dts and that
> devm_request_irq() went ok in probe(), i.e. that rtc->irq > 0?
You're right is the irq is not valid, I can even removed these 2 functions
from armada38x_rtc_ops.
>
>> +static int armada38x_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>> +{
>> + struct armada38x_rtc *rtc = dev_get_drvdata(dev);
>> + unsigned long time, flags;
>> + int ret = 0;
>> + u32 val;
>> +
>> + ret = rtc_tm_to_time(&alrm->time, &time);
>> +
>> + if (ret)
>> + goto out;
>> +
>> + spin_lock_irqsave(&rtc->lock, flags);
>> +
>> + rtc_delayed_write(time, rtc, RTC_ALARM1);
>> +
>> + if (alrm->enabled) {
>> + rtc_delayed_write(RTC_IRQ1_AL_EN , rtc, RTC_IRQ1_CONF);
>> + val = readl(rtc->regs_soc + SOC_RTC_INTERRUPT);
>> + writel(val | SOC_RTC_ALARM1_MASK,
>> + rtc->regs_soc + SOC_RTC_INTERRUPT);
>> + }
>> +
>> + spin_unlock_irqrestore(&rtc->lock, flags);
>> +
>> +out:
>> + return ret;
>> +}
>> +
>> +static int armada38x_rtc_alarm_irq_enable(struct device *dev,
>> + unsigned int enabled)
>> +{
>> + struct armada38x_rtc *rtc = dev_get_drvdata(dev);
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&rtc->lock, flags);
>> +
>> + if (enabled)
>> + writel(RTC_IRQ1_AL_EN, rtc->regs + RTC_IRQ1_CONF);
>> + else
>> + writel(0, rtc->regs + RTC_IRQ1_CONF);
>
> I find the following more readable, but it is subjective:
>
> writel(enabled ? RTC_IRQ1_AL_EN : 0, rtc->regs + RTC_IRQ1_CONF);
>
>
>> +
>> + spin_unlock_irqrestore(&rtc->lock, flags);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t armada38x_rtc_alarm_irq(int irq, void *data)
>> +{
>> + struct armada38x_rtc *rtc = data;
>> + u32 val;
>> + int event = RTC_IRQF | RTC_AF;
>> +
>> + dev_dbg(&rtc->rtc_dev->dev, "%s:irq(%d)\n", __func__, irq);
>> + val = readl(rtc->regs_soc + SOC_RTC_INTERRUPT);
>> +
>> + spin_lock(&rtc->lock);
>> +
>> + writel(val & ~SOC_RTC_ALARM1, rtc->regs_soc + SOC_RTC_INTERRUPT);
>
> Why not putting the readl() above under protection of the spinlock to
> protect a change that could occur between the readl() and the writel()?
Indeed it would be better.
>
>
>> + val = readl(rtc->regs + RTC_IRQ1_CONF);
>> + /* disable all the interrupts for alarm 1 */
>> + rtc_delayed_write(0, rtc, RTC_IRQ1_CONF);
>> + /* Ack the event */
>> + writel(RTC_STATUS_ALARM1, rtc->regs + RTC_STATUS);
>> +
>> + spin_unlock(&rtc->lock);
>> +
>> + if (val & RTC_IRQ1_FREQ_EN) {
>> + if (val & RTC_IRQ1_FREQ_1HZ)
>> + event |= RTC_UF;
>> + else
>> + event |= RTC_PF;
>> + }
>> +
>> + rtc_update_irq(rtc->rtc_dev, 1, event);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static const struct rtc_class_ops armada38x_rtc_ops = {
>> + .read_time = armada38x_rtc_read_time,
>> + .set_time = armada38x_rtc_set_time,
>> + .read_alarm = armada38x_rtc_read_alarm,
>> + .set_alarm = armada38x_rtc_set_alarm,
>> + .alarm_irq_enable = armada38x_rtc_alarm_irq_enable,
>> +};
>> +
>> +static __init int armada38x_rtc_probe(struct platform_device *pdev)
>> +{
>> + struct resource *res;
>> + struct armada38x_rtc *rtc;
>> + int ret;
>> +
>> + rtc = devm_kzalloc(&pdev->dev, sizeof(struct armada38x_rtc),
>> + GFP_KERNEL);
>> + if (!rtc)
>> + return -ENOMEM;
>> +
>> + spin_lock_init(&rtc->lock);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + rtc->regs = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(rtc->regs))
>> + return PTR_ERR(rtc->regs);
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + rtc->regs_soc = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(rtc->regs_soc))
>> + return PTR_ERR(rtc->regs_soc);
>> +
>> + rtc->irq = platform_get_irq(pdev, 0);
>> +
>> + if (rtc->irq < 0) {
>> + dev_err(&pdev->dev, "no irq\n");
>> + return rtc->irq;
>> + }
>> + if (devm_request_irq(&pdev->dev, rtc->irq, armada38x_rtc_alarm_irq,
>> + 0, pdev->name, rtc) < 0) {
>> + dev_warn(&pdev->dev, "Interrupt not available.\n");
>> + rtc->irq = -1;
>> + }
>> + platform_set_drvdata(pdev, rtc);
>> +
>> + device_init_wakeup(&pdev->dev, 1);
>
> if devm_request_irq() returns an error, should device_init_wakeup() be
> called? See comment below under armada38x_rtc_suspend().
We should not called it and also set the set_alarm and alarm_irq_enable operation
to NULL.
>
>> + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
>> + &armada38x_rtc_ops, THIS_MODULE);
>> + if (IS_ERR(rtc->rtc_dev)) {
>> + ret = PTR_ERR(rtc->rtc_dev);
>> + dev_err(&pdev->dev, "Failed to register RTC device: %d\n", ret);
>> + if (ret == 0)
>> + ret = -EINVAL;
>
> I may be missing something but I do not see how ret can be 0 above
> because the initial check uses IS_ERR() and not IS_ERR_OR_NULL().
Indeed I can remove this. The devm_rtc_device_register won't return NULL.
>
>
>> + return ret;
>> + }
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM_SLEEP
>> +static int armada38x_rtc_suspend(struct device *dev)
>> +{
>> + if (device_may_wakeup(dev)) {
>> + struct armada38x_rtc *rtc = dev_get_drvdata(dev);
>> +
>> + return enable_irq_wake(rtc->irq);
>> + }
>
> AFAICT, rtc->irq maybe -1 in the call above. You need to either call
> device_init_wakeup() when devm_request_irq() succeed or check rtc->irq
> is valid in the "if" above.
I won't call device_init_wakeup() if rtc->irq is -1.
Thanks,
Gregory
>
>> +
>> + return 0;
>> +}
>> +
>> +static int armada38x_rtc_resume(struct device *dev)
>> +{
>> + if (device_may_wakeup(dev)) {
>> + struct armada38x_rtc *rtc = dev_get_drvdata(dev);
>> +
>> + return disable_irq_wake(rtc->irq);
>> + }
>> +
>> + return 0;
>> +}
>> +#endif
>> +
>> +static SIMPLE_DEV_PM_OPS(armada38x_rtc_pm_ops,
>> + armada38x_rtc_suspend, armada38x_rtc_resume);
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id armada38x_rtc_of_match_table[] = {
>> + { .compatible = "marvell,armada-380-rtc", },
>> + {}
>> +};
>> +#endif
>> +
>> +static struct platform_driver armada38x_rtc_driver = {
>> + .driver = {
>> + .name = "armada38x-rtc",
>> + .pm = &armada38x_rtc_pm_ops,
>> + .of_match_table = of_match_ptr(armada38x_rtc_of_match_table),
>> + },
>> +};
>> +
>> +module_platform_driver_probe(armada38x_rtc_driver, armada38x_rtc_probe);
>> +
>> +MODULE_DESCRIPTION("Marvell Armada 38x RTC driver");
>> +MODULE_AUTHOR("Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>");
>> +MODULE_LICENSE("GPL");
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
