Re: [PATCH v3 1/2] rtc: Add driver for Microchip PolarFire SoC

[<Conor.Dooley@microchip.com>]

On 17/05/2022 22:09, Alexandre Belloni wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello,
> 
> On 16/05/2022 09:28:38+0100, Conor Dooley wrote:
>> +struct mpfs_rtc_dev {
>> +     struct rtc_device *rtc;
>> +     void __iomem *base;
>> +     int wakeup_irq;
> 
> I believe this is only used in .probe so you make it local to this
> function.> 
>> +};
>> +
> 
> 
>> +static int mpfs_rtc_readtime(struct device *dev, struct rtc_time *tm)
>> +{
>> +     struct mpfs_rtc_dev *rtcdev = dev_get_drvdata(dev);
>> +     u64 time;
>> +
>> +     time = ((u64)readl(rtcdev->base + DATETIME_UPPER_REG) & DATETIME_UPPER_MASK) << 32;
>> +     time |= readl(rtcdev->base + DATETIME_LOWER_REG);
> 
> Are the registers properly latched on a DATETIME_UPPER_REG read?

It's a good thing you asked - I went to double check and these reads are
backwards. /facepalm
A read from the upper register will always be aligned with the last read
of the lower register. Stupid oversight, sorry.

> 
>> +     rtc_time64_to_tm(time + rtcdev->rtc->range_min, tm);
> 
> range_min is never set so it will end up being 0. I guess you can avoid
> a bunch of arithmetic in all the driver. Offsetting will happen in the
> core which will probably never happen anyway because the max year is
> 141338. I guess we will all be gone by then ;)

SGTM, including not being around in year 141338...

Thanks,
Conor.

> 
>> +
>> +     return 0;
>> +}
>> +
> 
>> +static int mpfs_rtc_probe(struct platform_device *pdev)
>> +{
>> +     struct mpfs_rtc_dev *rtcdev;
>> +     struct clk *clk;
>> +     u32 prescaler;
>> +     int ret;
>> +
>> +     rtcdev = devm_kzalloc(&pdev->dev, sizeof(struct mpfs_rtc_dev), GFP_KERNEL);
>> +     if (!rtcdev)
>> +             return -ENOMEM;
>> +
>> +     platform_set_drvdata(pdev, rtcdev);
>> +
>> +     rtcdev->rtc = devm_rtc_allocate_device(&pdev->dev);
>> +     if (IS_ERR(rtcdev->rtc))
>> +             return PTR_ERR(rtcdev->rtc);
>> +
>> +     rtcdev->rtc->ops = &mpfs_rtc_ops;
>> +
>> +     /* range is capped by alarm max, lower reg is 31:0 & upper is 10:0 */
>> +     rtcdev->rtc->range_max = GENMASK_ULL(42, 0);
>> +
>> +     clk = mpfs_rtc_init_clk(&pdev->dev);
>> +     if (IS_ERR(clk))
>> +             return PTR_ERR(clk);
>> +
>> +     rtcdev->base = devm_platform_ioremap_resource(pdev, 0);
>> +     if (IS_ERR(rtcdev->base)) {
>> +             dev_dbg(&pdev->dev, "invalid ioremap resources\n");
>> +             return PTR_ERR(rtcdev->base);
>> +     }
>> +
>> +     rtcdev->wakeup_irq = platform_get_irq(pdev, 0);
>> +     if (rtcdev->wakeup_irq <= 0) {
>> +             dev_dbg(&pdev->dev, "could not get wakeup irq\n");
>> +             return rtcdev->wakeup_irq;
>> +     }
>> +     ret = devm_request_irq(&pdev->dev, rtcdev->wakeup_irq, mpfs_rtc_wakeup_irq_handler, 0,
>> +                            dev_name(&pdev->dev), rtcdev);
>> +     if (ret) {
>> +             dev_dbg(&pdev->dev, "could not request wakeup irq\n");
>> +             return ret;
>> +     }
>> +
>> +     /* prescaler hardware adds 1 to reg value */
>> +     prescaler = clk_get_rate(devm_clk_get(&pdev->dev, "rtcref")) - 1;
>> +
>> +     if (prescaler > MAX_PRESCALER_COUNT) {
>> +             dev_dbg(&pdev->dev, "invalid prescaler %d\n", prescaler);
>> +             return -EINVAL;
>> +     }
>> +
>> +     writel(prescaler, rtcdev->base + PRESCALER_REG);
>> +     dev_info(&pdev->dev, "prescaler set to: 0x%X \r\n", prescaler);
>> +
>> +     device_init_wakeup(&pdev->dev, true);
>> +     ret = dev_pm_set_wake_irq(&pdev->dev, rtcdev->wakeup_irq);
>> +     if (ret)
>> +             dev_err(&pdev->dev, "failed to enable irq wake\n");
>> +
>> +     return devm_rtc_register_device(rtcdev->rtc);
>> +}
>> +
>> +static int mpfs_rtc_remove(struct platform_device *pdev)
>> +{
>> +     mpfs_rtc_alarm_irq_enable(&pdev->dev, 0);
> 
> This is not something you want to do if you want to wake up from
> hibernate or any similar sleep state.
> 
>> +     device_init_wakeup(&pdev->dev, 0);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id mpfs_rtc_of_match[] = {
>> +     { .compatible = "microchip,mpfs-rtc" },
>> +     { }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, mpfs_rtc_of_match);
>> +
>> +static struct platform_driver mpfs_rtc_driver = {
>> +     .probe = mpfs_rtc_probe,
>> +     .remove = mpfs_rtc_remove,
>> +     .driver = {
>> +             .name = "mpfs_rtc",
>> +             .of_match_table = mpfs_rtc_of_match,
>> +     },
>> +};
>> +
>> +module_platform_driver(mpfs_rtc_driver);
>> +
>> +MODULE_DESCRIPTION("Real time clock for Microchip Polarfire SoC");
>> +MODULE_AUTHOR("Daire McNamara <daire.mcnamara@microchip.com>");
>> +MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
>> +MODULE_LICENSE("GPL");
>> --
>> 2.36.1
>>
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com