| Message ID | 20171205140646.30367-6-linux-kernel-dev@beckhoff.com |
|---|---|
| State | New |
| Headers | show |
| Series | add mxc driver for i.MX53 SRTC | expand |
On Tue, Dec 05, 2017 at 03:06:46PM +0100, linux-kernel-dev@beckhoff.com wrote: > From: Patrick Bruenn <p.bruenn@beckhoff.com> > > Neither rtc-imxdi, rtc-mxc nor rtc-snvs are compatible with i.MX53. > > This is driver enables support for the low power domain SRTC features: > - 32-bit MSB of non-rollover time counter > - 32-bit alarm register > > Based on: > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/rtc/rtc-mxc_v2.c?h=imx_2.6.35_11.09.01 > > Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com> > > --- > > v2: > - have seperate patches for dt-binding, CONFIG option, imx53.dtsi and driver > - add SPDX-License-Identifier and cleanup copyright notice > - replace __raw_readl/writel() with readl/writel() > - fix PM_SLEEP callbacks > - add CONFIG_RTC_DRV_MXC_V2 to build rtc-mxc_v2.c > - remove misleading or obvious comments and fix style of the remaining > - avoid endless loop while waiting for hw > - implement consistent locking; make spinlock a member of dev struct > - enable clk only for register accesses > - remove all udelay() calls since they are obsolete or redundant > (we are already waiting for register flags to change) > - init platform_data before registering irq callback > - let set_time() fail, when 32 bit rtc counter exceeded > - make names more consistent > - cleanup and reorder includes > - cleanup and remove unused defines > > To: Alessandro Zummo <a.zummo@towertech.it> > To: Alexandre Belloni <alexandre.belloni@free-electrons.com> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Cc: linux-rtc@vger.kernel.org (open list:REAL TIME CLOCK (RTC) SUBSYSTEM) > Cc: devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Cc: linux-kernel@vger.kernel.org (open list) > Cc: Fabio Estevam <fabio.estevam@nxp.com> > Cc: Juergen Borleis <jbe@pengutronix.de> > Cc: Noel Vellemans <Noel.Vellemans@visionbms.com> > Cc: Shawn Guo <shawnguo@kernel.org> > Cc: Sascha Hauer <kernel@pengutronix.de> (maintainer:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE) > Cc: Russell King <linux@armlinux.org.uk> (maintainer:ARM PORT) > Cc: linux-arm-kernel@lists.infradead.org (moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE) > > Cc: Philippe Ombredanne <pombredanne@nexb.com> > Cc: Lothar Waßmann <LW@KARO-electronics.de> > --- > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-mxc_v2.c | 433 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 434 insertions(+) > create mode 100644 drivers/rtc/rtc-mxc_v2.c > > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index f2f50c11dc38..dcf60e61ae5c 100644 > --- a/drivers/rtc/Makefile > +++ b/drivers/rtc/Makefile > @@ -106,6 +106,7 @@ obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o > obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o > obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o > obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o > +obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o > obj-$(CONFIG_RTC_DRV_NUC900) += rtc-nuc900.o > obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o > obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o > diff --git a/drivers/rtc/rtc-mxc_v2.c b/drivers/rtc/rtc-mxc_v2.c > new file mode 100644 > index 000000000000..c5a6d2c293bb > --- /dev/null > +++ b/drivers/rtc/rtc-mxc_v2.c > @@ -0,0 +1,433 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Real Time Clock (RTC) Driver for i.MX53 > + * Copyright (c) 2004-2011 Freescale Semiconductor, Inc. > + * Copyright (c) 2017 Beckhoff Automation GmbH & Co. KG > + */ > + > +#include <linux/clk.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/rtc.h> > + > +#define SRTC_LPPDR_INIT 0x41736166 /* init for glitch detect */ > + > +#define SRTC_LPCR_EN_LP BIT(3) /* lp enable */ > +#define SRTC_LPCR_WAE BIT(4) /* lp wakeup alarm enable */ > +#define SRTC_LPCR_ALP BIT(7) /* lp alarm flag */ > +#define SRTC_LPCR_NSA BIT(11) /* lp non secure access */ > +#define SRTC_LPCR_NVE BIT(14) /* lp non valid state exit bit */ > +#define SRTC_LPCR_IE BIT(15) /* lp init state exit bit */ > + > +#define SRTC_LPSR_ALP BIT(3) /* lp alarm flag */ > +#define SRTC_LPSR_NVES BIT(14) /* lp non-valid state exit status */ > +#define SRTC_LPSR_IES BIT(15) /* lp init state exit status */ > + > +#define SRTC_LPSCMR 0x00 /* LP Secure Counter MSB Reg */ > +#define SRTC_LPSCLR 0x04 /* LP Secure Counter LSB Reg */ > +#define SRTC_LPSAR 0x08 /* LP Secure Alarm Reg */ > +#define SRTC_LPCR 0x10 /* LP Control Reg */ > +#define SRTC_LPSR 0x14 /* LP Status Reg */ > +#define SRTC_LPPDR 0x18 /* LP Power Supply Glitch Detector Reg */ > + > +/* max. number of retries to read registers, 120 was max during test */ > +#define REG_READ_TIMEOUT 2000 > + > +struct mxc_rtc_data { > + struct rtc_device *rtc; > + void __iomem *ioaddr; > + struct clk *clk; > + spinlock_t lock; /* protects register access */ > + int irq; > +}; > + > +/* > + * This function does write synchronization for writes to the lp srtc block. > + * To take care of the asynchronous CKIL clock, all writes from the IP domain > + * will be synchronized to the CKIL domain. > + * The caller should hold the pdata->lock > + */ > +static inline void mxc_rtc_sync_lp_locked(void __iomem *ioaddr) > +{ > + unsigned int i; > + > + /* Wait for 3 CKIL cycles */ > + for (i = 0; i < 3; i++) { > + const u32 count = readl(ioaddr + SRTC_LPSCLR); > + unsigned int timeout = REG_READ_TIMEOUT; > + > + while ((readl(ioaddr + SRTC_LPSCLR)) == count) { > + if (!--timeout) { > + pr_err("SRTC_LPSCLR stuck! Check your hw.\n"); > + return; > + } > + } > + } > +} > + > +/* > + * This function updates the RTC alarm registers and then clears all the > + * interrupt status bits. > + * The caller should hold the pdata->lock > + * > + * @param alrm the new alarm value to be updated in the RTC > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata, > + struct rtc_time *alarm_tm) > +{ > + void __iomem *const ioaddr = pdata->ioaddr; > + unsigned long time; > + > + rtc_tm_to_time(alarm_tm, &time); > + > + if (time > U32_MAX) { > + pr_err("Hopefully I am out of service by then :-(\n"); > + return -EINVAL; > + } This will never happen as on your target hardware unsigned long is a 32bit type. Not sure what is best to do here. Maybe you should test the return value of rtc_tm_to_time. ATM it returns 0 unconditionally, but rtc_tm_to_time could detect when the input time doesn't fit into its return type and return an error in this case. Also I just realized that it's unsigned and only overflows in the year 2106. I'm most likely dead then so I don't care that much ;) > + > + writel((u32)time, ioaddr + SRTC_LPSAR); > + > + /* clear alarm interrupt status bit */ > + writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR); > + > + mxc_rtc_sync_lp_locked(ioaddr); > + return 0; > +} > + > +/* This function is the RTC interrupt service routine. */ > +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) > +{ > + struct platform_device *pdev = dev_id; > + struct mxc_rtc_data *pdata = platform_get_drvdata(pdev); > + void __iomem *ioaddr = pdata->ioaddr; > + unsigned long flags; > + u32 events = 0; > + u32 lp_status; > + u32 lp_cr; > + > + spin_lock_irqsave(&pdata->lock, flags); > + if (clk_prepare_enable(pdata->clk)) { > + spin_unlock_irqrestore(&pdata->lock, flags); > + return IRQ_NONE; > + } You are not allowed to do a clk_prepare under a spinlock. That was the original reason to split enabling a clk into clk_prepare and clk_enable. Everything that can block is done in clk_prepare and only non blocking things are done in clk_enable. If you want to enable/disable the clock on demand you can clk_prepare() in probe and clk_enable when you actually need it. > + > + lp_status = readl(ioaddr + SRTC_LPSR); > + lp_cr = readl(ioaddr + SRTC_LPCR); > + > + /* update irq data & counter */ > + if (lp_status & SRTC_LPSR_ALP) { > + if (lp_cr & SRTC_LPCR_ALP) > + events = (RTC_AF | RTC_IRQF); > + > + /* disable further lp alarm interrupts */ > + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + } > + > + /* Update interrupt enables */ > + writel(lp_cr, ioaddr + SRTC_LPCR); > + > + /* clear interrupt status */ > + writel(lp_status, ioaddr + SRTC_LPSR); > + > + mxc_rtc_sync_lp_locked(ioaddr); > + rtc_update_irq(pdata->rtc, 1, events); > + clk_disable_unprepare(pdata->clk); > + spin_unlock_irqrestore(&pdata->lock, flags); > + return IRQ_HANDLED; > +} > + > +/* > + * Enable clk and aquire spinlock > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_lock(struct mxc_rtc_data *const pdata) > +{ > + int ret; > + > + spin_lock_irq(&pdata->lock); > + ret = clk_prepare_enable(pdata->clk); > + if (ret) { > + spin_unlock_irq(&pdata->lock); > + return ret; > + } > + return 0; > +} > + > +static int mxc_rtc_unlock(struct mxc_rtc_data *const pdata) > +{ > + clk_disable_unprepare(pdata->clk); > + spin_unlock_irq(&pdata->lock); > + return 0; > +} > + > +/* > + * This function reads the current RTC time into tm in Gregorian date. > + * > + * @param tm contains the RTC time value upon return > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); > + time_t now; > + int ret = mxc_rtc_lock(pdata); > + > + if (ret) > + return ret; > + > + now = readl(pdata->ioaddr + SRTC_LPSCMR); > + rtc_time_to_tm(now, tm); > + ret = rtc_valid_tm(tm); > + mxc_rtc_unlock(pdata); I don't think this needs to be locked. > + return ret; > +} > + > +/* > + * This function sets the internal RTC time based on tm in Gregorian date. > + * > + * @param tm the time value to be set in the RTC > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); > + time64_t time = rtc_tm_to_time64(tm); > + int ret; > + > + if (time > U32_MAX) { > + dev_err(dev, "RTC exceeded by %llus\n", time - U32_MAX); > + return -EINVAL; > + } > + > + ret = mxc_rtc_lock(pdata); > + if (ret) > + return ret; > + > + writel(time, pdata->ioaddr + SRTC_LPSCMR); > + mxc_rtc_sync_lp_locked(pdata->ioaddr); > + return mxc_rtc_unlock(pdata); > +} > + > +/* > + * This function reads the current alarm value into the passed in \b alrm > + * argument. It updates the \b alrm's pending field value based on the whether > + * an alarm interrupt occurs or not. > + * > + * @param alrm contains the RTC alarm value upon return > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + int ret; > + > + ret = mxc_rtc_lock(pdata); > + if (ret) > + return ret; > + > + rtc_time_to_tm(readl(ioaddr + SRTC_LPSAR), &alrm->time); > + alrm->pending = !!(readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP); > + return mxc_rtc_unlock(pdata); > +} > + > +/* > + * Enable/Disable alarm interrupt > + * The caller should hold the pdata->lock > + */ > +static void mxc_rtc_alarm_irq_enable_locked(struct mxc_rtc_data *pdata, > + unsigned int enable) > +{ > + u32 lp_cr = readl(pdata->ioaddr + SRTC_LPCR); > + > + if (enable) > + lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + else > + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + > + writel(lp_cr, pdata->ioaddr + SRTC_LPCR); > +} > + > +static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable) > +{ > + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); > + int ret = mxc_rtc_lock(pdata); > + > + if (ret) > + return ret; > + > + mxc_rtc_alarm_irq_enable_locked(pdata, enable); > + return mxc_rtc_unlock(pdata); > +} > + > +/* > + * This function sets the RTC alarm based on passed in alrm. > + * > + * @param alrm the alarm value to be set in the RTC > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); > + int ret = mxc_rtc_lock(pdata); > + > + if (ret) > + return ret; > + > + ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time); Is it worth it to make this a separate function? > + if (!ret) { > + mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled); > + mxc_rtc_sync_lp_locked(pdata->ioaddr); > + } > + mxc_rtc_unlock(pdata); > + return ret; > +} > + > +static const struct rtc_class_ops mxc_rtc_ops = { > + .read_time = mxc_rtc_read_time, > + .set_time = mxc_rtc_set_time, > + .read_alarm = mxc_rtc_read_alarm, > + .set_alarm = mxc_rtc_set_alarm, > + .alarm_irq_enable = mxc_rtc_alarm_irq_enable, > +}; > + > +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag) > +{ > + unsigned int timeout = REG_READ_TIMEOUT; > + > + while (!(readl(ioaddr) & flag)) { > + if (!--timeout) { > + pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr); Please use dev_* functions for printing. In this case the message should probably be printed from the caller. > + return -EBUSY; > + } > + } > + return 0; > +} > + > +static int mxc_rtc_probe(struct platform_device *pdev) > +{ > + struct mxc_rtc_data *pdata; > + struct resource *res; > + void __iomem *ioaddr; > + int ret = 0; > + > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > + if (!pdata) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; > + > + pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res); > + if (IS_ERR(pdata->ioaddr)) > + return PTR_ERR(pdata->ioaddr); > + > + ioaddr = pdata->ioaddr; > + > + pdata->clk = devm_clk_get(&pdev->dev, NULL); > + if (IS_ERR(pdata->clk)) { > + dev_err(&pdev->dev, "unable to get rtc clock!\n"); > + return PTR_ERR(pdata->clk); > + } > + > + spin_lock_init(&pdata->lock); > + pdata->irq = platform_get_irq(pdev, 0); > + if (pdata->irq < 0) > + return pdata->irq; > + > + device_init_wakeup(&pdev->dev, 1); > + > + ret = clk_prepare_enable(pdata->clk); > + if (ret) > + return ret; > + /* initialize glitch detect */ > + writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR); > + > + /* clear lp interrupt status */ > + writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); > + > + /* move out of init state */ > + writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR); > + xc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES); If this can fail, shouldn't you test for an error? > + > + /* move out of non-valid state */ > + writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA | > + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR); > + mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES); dito Sascha
On 06/12/2017 at 09:36:18 +0100, Sascha Hauer wrote: > > +/* > > + * This function updates the RTC alarm registers and then clears all the > > + * interrupt status bits. > > + * The caller should hold the pdata->lock > > + * > > + * @param alrm the new alarm value to be updated in the RTC > > + * > > + * @return 0 if successful; non-zero otherwise. > > + */ > > +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata, > > + struct rtc_time *alarm_tm) > > +{ > > + void __iomem *const ioaddr = pdata->ioaddr; > > + unsigned long time; > > + > > + rtc_tm_to_time(alarm_tm, &time); > > + > > + if (time > U32_MAX) { > > + pr_err("Hopefully I am out of service by then :-(\n"); > > + return -EINVAL; > > + } > > This will never happen as on your target hardware unsigned long is a > 32bit type. Not sure what is best to do here. Maybe you should test > the return value of rtc_tm_to_time. ATM it returns 0 unconditionally, > but rtc_tm_to_time could detect when the input time doesn't fit into > its return type and return an error in this case. > Also I just realized that it's unsigned and only overflows in the year > 2106. I'm most likely dead then so I don't care that much ;) > One solution is to use the 64bit version instead so it doesn't overflow. This makes the time > U32_MAX work. Also, I'll send (hopefully soon) a series adding proper range checking for the whole RTC subsystem. And yes, it not urgent as I don't think I will care so much in 2106 too ;) > > +/* > > + * This function reads the current RTC time into tm in Gregorian date. > > + * > > + * @param tm contains the RTC time value upon return > > + * > > + * @return 0 if successful; non-zero otherwise. > > + */ > > +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); > > + time_t now; > > + int ret = mxc_rtc_lock(pdata); > > + > > + if (ret) > > + return ret; > > + > > + now = readl(pdata->ioaddr + SRTC_LPSCMR); > > + rtc_time_to_tm(now, tm); > > + ret = rtc_valid_tm(tm); This check is useless for two reasons: you know that rtc_time_to_tm will generate a valid tm and the core always checks the tm anyway.
>From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com] >Sent: Mittwoch, 6. Dezember 2017 09:58 >On 06/12/2017 at 09:36:18 +0100, Sascha Hauer wrote: >> > +/* >> > + * This function updates the RTC alarm registers and then clears all the >> > + * interrupt status bits. >> > + * The caller should hold the pdata->lock >> > + * >> > + * @param alrm the new alarm value to be updated in the RTC >> > + * >> > + * @return 0 if successful; non-zero otherwise. >> > + */ >> > +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata, >> > + struct rtc_time *alarm_tm) >> > +{ >> > + void __iomem *const ioaddr = pdata->ioaddr; >> > + unsigned long time; >> > + >> > + rtc_tm_to_time(alarm_tm, &time); >> > + >> > + if (time > U32_MAX) { >> > + pr_err("Hopefully I am out of service by then :-(\n"); >> > + return -EINVAL; >> > + } >> >> This will never happen as on your target hardware unsigned long is a >> 32bit type. Not sure what is best to do here. Maybe you should test >> the return value of rtc_tm_to_time. ATM it returns 0 unconditionally, >> but rtc_tm_to_time could detect when the input time doesn't fit into >> its return type and return an error in this case. >> Also I just realized that it's unsigned and only overflows in the year >> 2106. I'm most likely dead then so I don't care that much ;) >> > >One solution is to use the 64bit version instead so it doesn't overflow. >This makes the time > U32_MAX work. >Also, I'll send (hopefully soon) a series adding proper range checking >for the whole RTC subsystem. And yes, it not urgent as I don't think I >will care so much in 2106 too ;) > I just noticed that in mxc_rtc_set_time() I am using the 64bit version. After thinking a while about this issue, I think the 64bit version is better suited for my use case. It makes explicitly clear that I need to push the time into a 32bit hw register and "unsigned long" is just by accident the correct size for me. >> > +/* >> > + * This function reads the current RTC time into tm in Gregorian date. >> > + * >> > + * @param tm contains the RTC time value upon return >> > + * >> > + * @return 0 if successful; non-zero otherwise. >> > + */ >> > +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm) >> > +{ >> > + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); >> > + time_t now; >> > + int ret = mxc_rtc_lock(pdata); >> > + >> > + if (ret) >> > + return ret; >> > + >> > + now = readl(pdata->ioaddr + SRTC_LPSCMR); >> > + rtc_time_to_tm(now, tm); >> > + ret = rtc_valid_tm(tm); > >This check is useless for two reasons: you know that rtc_time_to_tm will >generate a valid tm and the core always checks the tm anyway. I will remove this with the next version Thanks for your time and help, Patrick Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
>From: Sascha Hauer [mailto:s.hauer@pengutronix.de] >Sent: Mittwoch, 6. Dezember 2017 09:36 >On Tue, Dec 05, 2017 at 03:06:46PM +0100, linux-kernel-dev@beckhoff.com >wrote: >> +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata, >> + struct rtc_time *alarm_tm) >> +{ >> + void __iomem *const ioaddr = pdata->ioaddr; >> + unsigned long time; >> + >> + rtc_tm_to_time(alarm_tm, &time); >> + >> + if (time > U32_MAX) { >> + pr_err("Hopefully I am out of service by then :-(\n"); >> + return -EINVAL; >> + } > >This will never happen as on your target hardware unsigned long is a >32bit type. Not sure what is best to do here. Maybe you should test >the return value of rtc_tm_to_time. ATM it returns 0 unconditionally, >but rtc_tm_to_time could detect when the input time doesn't fit into >its return type and return an error in this case. >Also I just realized that it's unsigned and only overflows in the year >2106. I'm most likely dead then so I don't care that much ;) > please see my response to Alexandre's follow up >> +/* This function is the RTC interrupt service routine. */ >> +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) >> +{ >> + struct platform_device *pdev = dev_id; >> + struct mxc_rtc_data *pdata = platform_get_drvdata(pdev); >> + void __iomem *ioaddr = pdata->ioaddr; >> + unsigned long flags; >> + u32 events = 0; >> + u32 lp_status; >> + u32 lp_cr; >> + >> + spin_lock_irqsave(&pdata->lock, flags); >> + if (clk_prepare_enable(pdata->clk)) { >> + spin_unlock_irqrestore(&pdata->lock, flags); >> + return IRQ_NONE; >> + } > >You are not allowed to do a clk_prepare under a spinlock. That was the >original reason to split enabling a clk into clk_prepare and clk_enable. >Everything that can block is done in clk_prepare and only non blocking >things are done in clk_enable. >If you want to enable/disable the clock on demand you can clk_prepare() >in probe and clk_enable when you actually need it. > Thanks for clarification. To be honest when I read Lothar's suggestion it was the first time I thought about the idea of keeping the clk disabled most of the time. I am not very experienced with this. But a rtctest loop run for hours so I assume it would be okay to keep the clk disabled until hw access. If there is no objection from somebody who knows the i.MX53 SRTC HW better, I will stick to the clock on demand model and make sure I avoid blocking. >> + >> +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm) >> +{ >> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); >> + time_t now; >> + int ret = mxc_rtc_lock(pdata); >> + >> + if (ret) >> + return ret; >> + >> + now = readl(pdata->ioaddr + SRTC_LPSCMR); >> + rtc_time_to_tm(now, tm); >> + ret = rtc_valid_tm(tm); >> + mxc_rtc_unlock(pdata); > >I don't think this needs to be locked. I will change this to only enable the clock for the readl() >> +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) >> +{ >> + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); >> + int ret = mxc_rtc_lock(pdata); >> + >> + if (ret) >> + return ret; >> + >> + ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time); > >Is it worth it to make this a separate function? Maybe not, I think it is an artifact from a refactoring. I will reconsider this for the next version. > >> + if (!ret) { >> + mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled); >> + mxc_rtc_sync_lp_locked(pdata->ioaddr); >> + } >> + mxc_rtc_unlock(pdata); >> + return ret; >> +} >> + >> +static const struct rtc_class_ops mxc_rtc_ops = { >> + .read_time = mxc_rtc_read_time, >> + .set_time = mxc_rtc_set_time, >> + .read_alarm = mxc_rtc_read_alarm, >> + .set_alarm = mxc_rtc_set_alarm, >> + .alarm_irq_enable = mxc_rtc_alarm_irq_enable, >> +}; >> + >> +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag) >> +{ >> + unsigned int timeout = REG_READ_TIMEOUT; >> + >> + while (!(readl(ioaddr) & flag)) { >> + if (!--timeout) { >> + pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr); > >Please use dev_* functions for printing. In this case the message should >probably be printed from the caller. Do you have a link at hand about dev_* vs. pr_*? I just choose pr_err here, because I would have to change the functions signature to get a device. However, I will drop the message and move it to the caller. >> + /* clear lp interrupt status */ >> + writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); >> + >> + /* move out of init state */ >> + writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR); >> + xc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES); > >If this can fail, shouldn't you test for an error? very probably >> + >> + /* move out of non-valid state */ >> + writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA | >> + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR); >> + mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES); > >dito sure Thanks, Patrick --- Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
On 05/12/2017 at 15:06:46 +0100, linux-kernel-dev@beckhoff.com wrote: > +/* This function is the RTC interrupt service routine. */ > +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) > +{ > + struct platform_device *pdev = dev_id; > + struct mxc_rtc_data *pdata = platform_get_drvdata(pdev); > + void __iomem *ioaddr = pdata->ioaddr; > + unsigned long flags; > + u32 events = 0; > + u32 lp_status; > + u32 lp_cr; > + > + spin_lock_irqsave(&pdata->lock, flags); > + if (clk_prepare_enable(pdata->clk)) { > + spin_unlock_irqrestore(&pdata->lock, flags); > + return IRQ_NONE; > + } > + > + lp_status = readl(ioaddr + SRTC_LPSR); > + lp_cr = readl(ioaddr + SRTC_LPCR); > + > + /* update irq data & counter */ > + if (lp_status & SRTC_LPSR_ALP) { > + if (lp_cr & SRTC_LPCR_ALP) > + events = (RTC_AF | RTC_IRQF); I would just call rtc_update_irq here... > + > + /* disable further lp alarm interrupts */ > + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + } > + > + /* Update interrupt enables */ > + writel(lp_cr, ioaddr + SRTC_LPCR); > + > + /* clear interrupt status */ > + writel(lp_status, ioaddr + SRTC_LPSR); > + > + mxc_rtc_sync_lp_locked(ioaddr); > + rtc_update_irq(pdata->rtc, 1, events); ... because calling it here with events == 0 will result in a lot of work for nothing (the core will walk through the timers and set the alarm again).
On Wed, Dec 06, 2017 at 10:17:06AM +0000, Patrick Brünn wrote: > >From: Sascha Hauer [mailto:s.hauer@pengutronix.de] > >Sent: Mittwoch, 6. Dezember 2017 09:36 > >On Tue, Dec 05, 2017 at 03:06:46PM +0100, linux-kernel-dev@beckhoff.com > >wrote: > >> +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag) > >> +{ > >> + unsigned int timeout = REG_READ_TIMEOUT; > >> + > >> + while (!(readl(ioaddr) & flag)) { > >> + if (!--timeout) { > >> + pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr); > > > >Please use dev_* functions for printing. In this case the message should > >probably be printed from the caller. > Do you have a link at hand about dev_* vs. pr_*? I just choose pr_err here, > because I would have to change the functions signature to get a device. > However, I will drop the message and move it to the caller. No, I don't have a link. However, a message printed with pr_err comes with absolutely no context, so without grepping the source you do not get a clue which device has a problem. You could add more context using some prefix either to each message or by using #define pr_fmt(fmt) "mydriver: " fmt but then you still not know which instance the message throws. Using dev_* just does the right thing without much thinking. Sascha
diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index f2f50c11dc38..dcf60e61ae5c 100644 --- a/drivers/rtc/Makefile +++ b/drivers/rtc/Makefile @@ -106,6 +106,7 @@ obj-$(CONFIG_RTC_DRV_MT6397) += rtc-mt6397.o obj-$(CONFIG_RTC_DRV_MT7622) += rtc-mt7622.o obj-$(CONFIG_RTC_DRV_MV) += rtc-mv.o obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o +obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o obj-$(CONFIG_RTC_DRV_NUC900) += rtc-nuc900.o obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o diff --git a/drivers/rtc/rtc-mxc_v2.c b/drivers/rtc/rtc-mxc_v2.c new file mode 100644 index 000000000000..c5a6d2c293bb --- /dev/null +++ b/drivers/rtc/rtc-mxc_v2.c @@ -0,0 +1,433 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Real Time Clock (RTC) Driver for i.MX53 + * Copyright (c) 2004-2011 Freescale Semiconductor, Inc. + * Copyright (c) 2017 Beckhoff Automation GmbH & Co. KG + */ + +#include <linux/clk.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/rtc.h> + +#define SRTC_LPPDR_INIT 0x41736166 /* init for glitch detect */ + +#define SRTC_LPCR_EN_LP BIT(3) /* lp enable */ +#define SRTC_LPCR_WAE BIT(4) /* lp wakeup alarm enable */ +#define SRTC_LPCR_ALP BIT(7) /* lp alarm flag */ +#define SRTC_LPCR_NSA BIT(11) /* lp non secure access */ +#define SRTC_LPCR_NVE BIT(14) /* lp non valid state exit bit */ +#define SRTC_LPCR_IE BIT(15) /* lp init state exit bit */ + +#define SRTC_LPSR_ALP BIT(3) /* lp alarm flag */ +#define SRTC_LPSR_NVES BIT(14) /* lp non-valid state exit status */ +#define SRTC_LPSR_IES BIT(15) /* lp init state exit status */ + +#define SRTC_LPSCMR 0x00 /* LP Secure Counter MSB Reg */ +#define SRTC_LPSCLR 0x04 /* LP Secure Counter LSB Reg */ +#define SRTC_LPSAR 0x08 /* LP Secure Alarm Reg */ +#define SRTC_LPCR 0x10 /* LP Control Reg */ +#define SRTC_LPSR 0x14 /* LP Status Reg */ +#define SRTC_LPPDR 0x18 /* LP Power Supply Glitch Detector Reg */ + +/* max. number of retries to read registers, 120 was max during test */ +#define REG_READ_TIMEOUT 2000 + +struct mxc_rtc_data { + struct rtc_device *rtc; + void __iomem *ioaddr; + struct clk *clk; + spinlock_t lock; /* protects register access */ + int irq; +}; + +/* + * This function does write synchronization for writes to the lp srtc block. + * To take care of the asynchronous CKIL clock, all writes from the IP domain + * will be synchronized to the CKIL domain. + * The caller should hold the pdata->lock + */ +static inline void mxc_rtc_sync_lp_locked(void __iomem *ioaddr) +{ + unsigned int i; + + /* Wait for 3 CKIL cycles */ + for (i = 0; i < 3; i++) { + const u32 count = readl(ioaddr + SRTC_LPSCLR); + unsigned int timeout = REG_READ_TIMEOUT; + + while ((readl(ioaddr + SRTC_LPSCLR)) == count) { + if (!--timeout) { + pr_err("SRTC_LPSCLR stuck! Check your hw.\n"); + return; + } + } + } +} + +/* + * This function updates the RTC alarm registers and then clears all the + * interrupt status bits. + * The caller should hold the pdata->lock + * + * @param alrm the new alarm value to be updated in the RTC + * + * @return 0 if successful; non-zero otherwise. + */ +static int mxc_rtc_write_alarm_locked(struct mxc_rtc_data *const pdata, + struct rtc_time *alarm_tm) +{ + void __iomem *const ioaddr = pdata->ioaddr; + unsigned long time; + + rtc_tm_to_time(alarm_tm, &time); + + if (time > U32_MAX) { + pr_err("Hopefully I am out of service by then :-(\n"); + return -EINVAL; + } + + writel((u32)time, ioaddr + SRTC_LPSAR); + + /* clear alarm interrupt status bit */ + writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR); + + mxc_rtc_sync_lp_locked(ioaddr); + return 0; +} + +/* This function is the RTC interrupt service routine. */ +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) +{ + struct platform_device *pdev = dev_id; + struct mxc_rtc_data *pdata = platform_get_drvdata(pdev); + void __iomem *ioaddr = pdata->ioaddr; + unsigned long flags; + u32 events = 0; + u32 lp_status; + u32 lp_cr; + + spin_lock_irqsave(&pdata->lock, flags); + if (clk_prepare_enable(pdata->clk)) { + spin_unlock_irqrestore(&pdata->lock, flags); + return IRQ_NONE; + } + + lp_status = readl(ioaddr + SRTC_LPSR); + lp_cr = readl(ioaddr + SRTC_LPCR); + + /* update irq data & counter */ + if (lp_status & SRTC_LPSR_ALP) { + if (lp_cr & SRTC_LPCR_ALP) + events = (RTC_AF | RTC_IRQF); + + /* disable further lp alarm interrupts */ + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); + } + + /* Update interrupt enables */ + writel(lp_cr, ioaddr + SRTC_LPCR); + + /* clear interrupt status */ + writel(lp_status, ioaddr + SRTC_LPSR); + + mxc_rtc_sync_lp_locked(ioaddr); + rtc_update_irq(pdata->rtc, 1, events); + clk_disable_unprepare(pdata->clk); + spin_unlock_irqrestore(&pdata->lock, flags); + return IRQ_HANDLED; +} + +/* + * Enable clk and aquire spinlock + * @return 0 if successful; non-zero otherwise. + */ +static int mxc_rtc_lock(struct mxc_rtc_data *const pdata) +{ + int ret; + + spin_lock_irq(&pdata->lock); + ret = clk_prepare_enable(pdata->clk); + if (ret) { + spin_unlock_irq(&pdata->lock); + return ret; + } + return 0; +} + +static int mxc_rtc_unlock(struct mxc_rtc_data *const pdata) +{ + clk_disable_unprepare(pdata->clk); + spin_unlock_irq(&pdata->lock); + return 0; +} + +/* + * This function reads the current RTC time into tm in Gregorian date. + * + * @param tm contains the RTC time value upon return + * + * @return 0 if successful; non-zero otherwise. + */ +static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm) +{ + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); + time_t now; + int ret = mxc_rtc_lock(pdata); + + if (ret) + return ret; + + now = readl(pdata->ioaddr + SRTC_LPSCMR); + rtc_time_to_tm(now, tm); + ret = rtc_valid_tm(tm); + mxc_rtc_unlock(pdata); + return ret; +} + +/* + * This function sets the internal RTC time based on tm in Gregorian date. + * + * @param tm the time value to be set in the RTC + * + * @return 0 if successful; non-zero otherwise. + */ +static int mxc_rtc_set_time(struct device *dev, struct rtc_time *tm) +{ + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); + time64_t time = rtc_tm_to_time64(tm); + int ret; + + if (time > U32_MAX) { + dev_err(dev, "RTC exceeded by %llus\n", time - U32_MAX); + return -EINVAL; + } + + ret = mxc_rtc_lock(pdata); + if (ret) + return ret; + + writel(time, pdata->ioaddr + SRTC_LPSCMR); + mxc_rtc_sync_lp_locked(pdata->ioaddr); + return mxc_rtc_unlock(pdata); +} + +/* + * This function reads the current alarm value into the passed in \b alrm + * argument. It updates the \b alrm's pending field value based on the whether + * an alarm interrupt occurs or not. + * + * @param alrm contains the RTC alarm value upon return + * + * @return 0 if successful; non-zero otherwise. + */ +static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) +{ + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); + void __iomem *ioaddr = pdata->ioaddr; + int ret; + + ret = mxc_rtc_lock(pdata); + if (ret) + return ret; + + rtc_time_to_tm(readl(ioaddr + SRTC_LPSAR), &alrm->time); + alrm->pending = !!(readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP); + return mxc_rtc_unlock(pdata); +} + +/* + * Enable/Disable alarm interrupt + * The caller should hold the pdata->lock + */ +static void mxc_rtc_alarm_irq_enable_locked(struct mxc_rtc_data *pdata, + unsigned int enable) +{ + u32 lp_cr = readl(pdata->ioaddr + SRTC_LPCR); + + if (enable) + lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE); + else + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); + + writel(lp_cr, pdata->ioaddr + SRTC_LPCR); +} + +static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable) +{ + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); + int ret = mxc_rtc_lock(pdata); + + if (ret) + return ret; + + mxc_rtc_alarm_irq_enable_locked(pdata, enable); + return mxc_rtc_unlock(pdata); +} + +/* + * This function sets the RTC alarm based on passed in alrm. + * + * @param alrm the alarm value to be set in the RTC + * + * @return 0 if successful; non-zero otherwise. + */ +static int mxc_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) +{ + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); + int ret = mxc_rtc_lock(pdata); + + if (ret) + return ret; + + ret = mxc_rtc_write_alarm_locked(pdata, &alrm->time); + if (!ret) { + mxc_rtc_alarm_irq_enable_locked(pdata, alrm->enabled); + mxc_rtc_sync_lp_locked(pdata->ioaddr); + } + mxc_rtc_unlock(pdata); + return ret; +} + +static const struct rtc_class_ops mxc_rtc_ops = { + .read_time = mxc_rtc_read_time, + .set_time = mxc_rtc_set_time, + .read_alarm = mxc_rtc_read_alarm, + .set_alarm = mxc_rtc_set_alarm, + .alarm_irq_enable = mxc_rtc_alarm_irq_enable, +}; + +static int mxc_rtc_wait_for_flag(void *__iomem ioaddr, int flag) +{ + unsigned int timeout = REG_READ_TIMEOUT; + + while (!(readl(ioaddr) & flag)) { + if (!--timeout) { + pr_err("Wait timeout for 0x%x@%p!\n", flag, ioaddr); + return -EBUSY; + } + } + return 0; +} + +static int mxc_rtc_probe(struct platform_device *pdev) +{ + struct mxc_rtc_data *pdata; + struct resource *res; + void __iomem *ioaddr; + int ret = 0; + + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); + if (!pdata) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENODEV; + + pdata->ioaddr = devm_ioremap_resource(&pdev->dev, res); + if (IS_ERR(pdata->ioaddr)) + return PTR_ERR(pdata->ioaddr); + + ioaddr = pdata->ioaddr; + + pdata->clk = devm_clk_get(&pdev->dev, NULL); + if (IS_ERR(pdata->clk)) { + dev_err(&pdev->dev, "unable to get rtc clock!\n"); + return PTR_ERR(pdata->clk); + } + + spin_lock_init(&pdata->lock); + pdata->irq = platform_get_irq(pdev, 0); + if (pdata->irq < 0) + return pdata->irq; + + device_init_wakeup(&pdev->dev, 1); + + ret = clk_prepare_enable(pdata->clk); + if (ret) + return ret; + /* initialize glitch detect */ + writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR); + + /* clear lp interrupt status */ + writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); + + /* move out of init state */ + writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR); + mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_IES); + + /* move out of non-valid state */ + writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA | + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR); + mxc_rtc_wait_for_flag(ioaddr + SRTC_LPSR, SRTC_LPSR_NVES); + + clk_disable_unprepare(pdata->clk); + platform_set_drvdata(pdev, pdata); + ret = + devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt, 0, + pdev->name, pdev); + if (ret < 0) { + dev_err(&pdev->dev, "interrupt not available.\n"); + return ret; + } + + pdata->rtc = + devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops, + THIS_MODULE); + if (IS_ERR(pdata->rtc)) + return PTR_ERR(pdata->rtc); + + return 0; +} + +static int __exit mxc_rtc_remove(struct platform_device *pdev) +{ + return 0; +} + +#ifdef CONFIG_PM_SLEEP +static int mxc_rtc_suspend(struct device *dev) +{ + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); + + if (device_may_wakeup(dev)) + enable_irq_wake(pdata->irq); + + return 0; +} + +static int mxc_rtc_resume(struct device *dev) +{ + struct mxc_rtc_data *pdata = dev_get_drvdata(dev); + + if (device_may_wakeup(dev)) + disable_irq_wake(pdata->irq); + + return 0; +} +#endif + +static SIMPLE_DEV_PM_OPS(mxc_rtc_pm_ops, mxc_rtc_suspend, mxc_rtc_resume); + +static const struct of_device_id mxc_ids[] = { + { .compatible = "fsl,imx53-rtc", }, + {} +}; + +static struct platform_driver mxc_rtc_driver = { + .driver = { + .name = "mxc_rtc_v2", + .of_match_table = mxc_ids, + .pm = &mxc_rtc_pm_ops, + }, + .probe = mxc_rtc_probe, + .remove = mxc_rtc_remove, +}; + +module_platform_driver(mxc_rtc_driver); + +MODULE_AUTHOR("Freescale Semiconductor, Inc."); +MODULE_DESCRIPTION("Real Time Clock (RTC) Driver for i.MX53"); +MODULE_LICENSE("GPL");