| Message ID | 20210801160921.233081-1-romain.perier@gmail.com |
|---|---|
| Headers | show |
| Series | Add RTC for MStar SoCs | expand |
Hi Romain, On Mon, 2 Aug 2021 at 01:09, Romain Perier <romain.perier@gmail.com> wrote: > +++ b/arch/arm/boot/dts/mstar-v7.dtsi > @@ -116,6 +116,13 @@ watchdog@6000 { > clocks = <&xtal_div2>; > }; > > + rtc@2400 { I think the rtc should be before the watchdog as the address is lower. I think maybe this got flipped around during cherry-picking. I can flip it around when I pull this into an mstar dts for 5.15 branch so you don't need to fix it and send a v3 assuming everything else is ok. Thanks, Daniel
Hello, On 01/08/2021 18:09:20+0200, Romain Perier wrote: > +static int msc313_rtc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct msc313_rtc *priv; > + int ret; > + int irq; > + unsigned long rate; > + u16 reg; > + > + priv = devm_kzalloc(&pdev->dev, sizeof(struct msc313_rtc), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + priv->rtc_base = devm_platform_ioremap_resource(pdev, 0); > + if (IS_ERR(priv->rtc_base)) > + return PTR_ERR(priv->rtc_base); > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return -EINVAL; > + > + priv->rtc_dev = devm_rtc_allocate_device(dev); > + if (IS_ERR(priv->rtc_dev)) > + return PTR_ERR(priv->rtc_dev); > + > + priv->rtc_dev->ops = &msc313_rtc_ops; > + priv->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_0000; I'm pretty sure this doesn't fit in this RTC registers, you should probably leave range_min to 0 (i.e. not set it at all). > + priv->rtc_dev->range_max = U32_MAX - 1; /* 2106-02-07 06:28:14 */ I guess this one should be U32_MAX > + > + ret = devm_request_irq(dev, irq, msc313_rtc_interrupt, IRQF_SHARED, > + dev_name(&pdev->dev), &pdev->dev); > + if (ret) { > + dev_err(dev, "Could not request IRQ\n"); > + return ret; > + } > + > + priv->clk = devm_clk_get(dev, NULL); > + if (IS_ERR(priv->clk)) { > + dev_err(dev, "No input reference clock\n"); > + return PTR_ERR(priv->clk); > + } > + > + ret = clk_prepare_enable(priv->clk); > + if (ret) { > + dev_err(dev, "Failed to enable the reference clock, %d\n", ret); > + return ret; > + } > + > + ret = devm_add_action_or_reset(dev, (void (*) (void *))clk_disable_unprepare, priv->clk); > + if (ret) > + return ret; > + > + rate = clk_get_rate(priv->clk); > + writew(rate & 0xFFFF, priv->rtc_base + REG_RTC_FREQ_CW_L); > + writew((rate >> 16) & 0xFFFF, priv->rtc_base + REG_RTC_FREQ_CW_H); > + > + reg = readw(priv->rtc_base + REG_RTC_CTRL); > + reg |= CNT_EN_BIT; > + writew(reg, priv->rtc_base + REG_RTC_CTRL); > + If on POR, CNT_EN_BIT is not set, then it would be nice to use that to know whether the RTC is properly set. You can then check CNT_EN_BIT in .read_time and return -EINVAL if it is not set. Then you can set the bit in .set_time. It is anyway useless to let the RTC running if it is not set. > + platform_set_drvdata(pdev, priv); > + > + return devm_rtc_register_device(priv->rtc_dev); > +} > + > +static const struct of_device_id msc313_rtc_of_match_table[] = { > + { .compatible = "mstar,msc313-rtc" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, ms_rtc_of_match_table); > + > +static struct platform_driver msc313_rtc_driver = { > + .probe = msc313_rtc_probe, > + .driver = { > + .name = "msc313-rtc", > + .of_match_table = msc313_rtc_of_match_table, > + }, > +}; > + > +module_platform_driver(msc313_rtc_driver); > + > +MODULE_AUTHOR("Daniel Palmer <daniel@thingy.jp>"); > +MODULE_AUTHOR("Romain Perier <romain.perier@gmail.com>"); > +MODULE_DESCRIPTION("MStar RTC Driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.30.2 >
Hi, As I need to send a v3, I can fix it, no worries. Cheers, Romain Le lun. 2 août 2021 à 12:40, Daniel Palmer <daniel@0x0f.com> a écrit : > > Hi Romain, > > On Mon, 2 Aug 2021 at 01:09, Romain Perier <romain.perier@gmail.com> wrote: > > +++ b/arch/arm/boot/dts/mstar-v7.dtsi > > @@ -116,6 +116,13 @@ watchdog@6000 { > > clocks = <&xtal_div2>; > > }; > > > > + rtc@2400 { > > I think the rtc should be before the watchdog as the address is lower. > I think maybe this got flipped around during cherry-picking. > I can flip it around when I pull this into an mstar dts for 5.15 > branch so you don't need to fix it and send a v3 assuming everything > else is ok. > > Thanks, > > Daniel
Hello, Le ven. 6 août 2021 à 21:38, Alexandre Belloni <alexandre.belloni@bootlin.com> a écrit : > > Hello, > > On 01/08/2021 18:09:20+0200, Romain Perier wrote: > > +static int msc313_rtc_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct msc313_rtc *priv; > > + int ret; > > + int irq; > > + unsigned long rate; > > + u16 reg; > > + > > + priv = devm_kzalloc(&pdev->dev, sizeof(struct msc313_rtc), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + priv->rtc_base = devm_platform_ioremap_resource(pdev, 0); > > + if (IS_ERR(priv->rtc_base)) > > + return PTR_ERR(priv->rtc_base); > > + > > + irq = platform_get_irq(pdev, 0); > > + if (irq < 0) > > + return -EINVAL; > > + > > + priv->rtc_dev = devm_rtc_allocate_device(dev); > > + if (IS_ERR(priv->rtc_dev)) > > + return PTR_ERR(priv->rtc_dev); > > + > > + priv->rtc_dev->ops = &msc313_rtc_ops; > > + priv->rtc_dev->range_min = RTC_TIMESTAMP_BEGIN_0000; > > I'm pretty sure this doesn't fit in this RTC registers, you should > probably leave range_min to 0 (i.e. not set it at all). ack > > > + priv->rtc_dev->range_max = U32_MAX - 1; /* 2106-02-07 06:28:14 */ > > I guess this one should be U32_MAX ack > > + > > + ret = devm_request_irq(dev, irq, msc313_rtc_interrupt, IRQF_SHARED, > > + dev_name(&pdev->dev), &pdev->dev); > > + if (ret) { > > + dev_err(dev, "Could not request IRQ\n"); > > + return ret; > > + } > > + > > + priv->clk = devm_clk_get(dev, NULL); > > + if (IS_ERR(priv->clk)) { > > + dev_err(dev, "No input reference clock\n"); > > + return PTR_ERR(priv->clk); > > + } > > + > > + ret = clk_prepare_enable(priv->clk); > > + if (ret) { > > + dev_err(dev, "Failed to enable the reference clock, %d\n", ret); > > + return ret; > > + } > > + > > + ret = devm_add_action_or_reset(dev, (void (*) (void *))clk_disable_unprepare, priv->clk); > > + if (ret) > > + return ret; > > + > > + rate = clk_get_rate(priv->clk); > > + writew(rate & 0xFFFF, priv->rtc_base + REG_RTC_FREQ_CW_L); > > + writew((rate >> 16) & 0xFFFF, priv->rtc_base + REG_RTC_FREQ_CW_H); > > + > > + reg = readw(priv->rtc_base + REG_RTC_CTRL); > > + reg |= CNT_EN_BIT; > > + writew(reg, priv->rtc_base + REG_RTC_CTRL); > > + > > If on POR, CNT_EN_BIT is not set, then it would be nice to use that to > know whether the RTC is properly set. You can then check CNT_EN_BIT in > .read_time and return -EINVAL if it is not set. Then you can set the bit > in .set_time. It is anyway useless to let the RTC running if it is not > set. Yeah, this is to be sure that the RTC is alive with a valid value (which makes sense). Ok I will fix everything in v3, then. Romain
This patches series adds a new driver for the RTC found in the Mstar MSC313e SoCs and newer. It adds a basic rtc driver, the corresponding devicetree bindings and its documentation. The rtctest (from selftests) has been passed on this driver, with the following output: $ rtctest TAP version 13 1..7 # Starting 7 tests from 2 test cases. # RUN rtc.date_read ... # rtctest.c:47:date_read:Current RTC date/time is 01/01/1970 00:02:03. # OK rtc.date_read ok 1 rtc.date_read # RUN rtc.uie_read ... # OK rtc.uie_read ok 2 rtc.uie_read # RUN rtc.uie_select ... # OK rtc.uie_select ok 3 rtc.uie_select # RUN rtc.alarm_alm_set ... # rtctest.c:136:alarm_alm_set:Alarm time now set to 00:02:12. # rtctest.c:156:alarm_alm_set:data: 1a0 # OK rtc.alarm_alm_set ok 4 rtc.alarm_alm_set # RUN rtc.alarm_wkalm_set ... # rtctest.c:192:alarm_wkalm_set:Alarm time now set to 01/01/1970 00:02:15. # OK rtc.alarm_wkalm_set ok 5 rtc.alarm_wkalm_set # RUN rtc.alarm_alm_set_minute ... # rtctest.c:238:alarm_alm_set_minute:Alarm time now set to 00:03:00. # rtctest.c:258:alarm_alm_set_minute:data: 1a0 # OK rtc.alarm_alm_set_minute ok 6 [ 180.545015] random: fast init done rtc.alarm_alm_set_minute # RUN rtc.alarm_wkalm_set_minute ... # rtctest.c:294:alarm_wkalm_set_minute:Alarm time now set to 01/01/1970 00:04:00. # OK rtc.alarm_wkalm_set_minute ok 7 rtc.alarm_wkalm_set_minute # PASSED: 7 / 7 tests passed. # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0 Changes since v1: - Fixed the DT bindings documentation and fixed dt_binding_check (an include was missing) - Added || COMPILE_TEST to kconfig - Removed rtc_valid_tm from msc313_rtc_read_time() - Removed the last write of the msc313_rtc_set_time() function (not required) and improved comments - Replaced the relaxed io by normal io in msc313_rtc_interrupt() - Added checks to be sure that the alarm fired in msc313_rtc_interrupt() - Removed msc313_rtc_remove() (replaced by devm_add_action_or_reset) - Removed unnecessary software reset of the IP in the probe function (the soft reset is never executed, it is a mistake from the initial refactoring) - Switched to devm_rtc_allocate_device() and devm_rtc_register_device(), and dropped the error message related to the rtc device allocation - Added an RTC range by setting .range_min and .range_max - Added the "start-year" property to the DT bindings documentation Daniel Palmer (1): rtc: Add support for the MSTAR MSC313 RTC Romain Perier (2): dt-bindings: rtc: Add Mstar MSC313e RTC devicetree bindings documentation ARM: dts: mstar: Add rtc device node .../bindings/rtc/mstar,msc313-rtc.yaml | 49 ++++ MAINTAINERS | 1 + arch/arm/boot/dts/mstar-v7.dtsi | 7 + drivers/rtc/Kconfig | 10 + drivers/rtc/Makefile | 1 + drivers/rtc/rtc-msc313.c | 246 ++++++++++++++++++ 6 files changed, 314 insertions(+) create mode 100644 Documentation/devicetree/bindings/rtc/mstar,msc313-rtc.yaml create mode 100644 drivers/rtc/rtc-msc313.c