| Message ID | 20171128073927.12035-1-linux-kernel-dev@beckhoff.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | rtc: add mxc driver for i.MX53 | expand |
Hi, A really quick review: On 28/11/2017 at 08:39:27 +0100, linux-kernel-dev@beckhoff.com wrote: > From: Patrick Bruenn <p.bruenn@beckhoff.com> > > Neither rtc-imxdi nor rtc-mxc are compatible with i.MX53. > Add a modernized version of mxc_v2 from here: > 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 > > Changes to that version: > - updated to v4.15-rc1 > - removed ioctl() > - removed proc() > - removed manual(redundant) enable_irq flag > I'd say this doesn't add much information as this link is probably bound to die at some point (especially seeing the NXP/Qualcomm/Broadcom story). > Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com> > > --- > > Open issues: > - driver naming, should it be merged with rtc-mxc.c ? This seems different enough to be a separate driver. > - document DT binding "fsl,imx53-rtc" accordingly Yes, this has to be documented in a previous patch > - Should unused defines be removed or kept for someone else to be > useful? I don't care much, either are fine but the less lines, the easier the reviews. > - Is the use of __raw_readl/writel() correct? Should it be replaced with > readl/writel()? The __raw version will not work if someone is running a BE kernel so they should be moved to the _relaxed version or the usual readl/writel. > - suspend/resume() seems different to existing rtc-mxc.c, should I apply > the pattern from rtc-mxc.c? If you mean using SIMPLE_DEV_PM_OPS, yes, you should do that. > - On Shawns tree imx53.dtsi has been reverted already[1][2]. Should I split > the imx53.dtsi change into a separate patch based on his tree? Or can > we still stop the full revert and just remove the imx25-rtc compatible? > I am not in a hurry, so we could just wait until the revert landed in > Linus tree. Whatever you think is best. > I'm not taking DT changes through the RTC tree so it should be in a separate patch that will go through Shawn's tree You should also use checkpatch --strict, most of the warnings are correct. > [1] https://www.spinics.net/lists/arm-kernel/msg617113.html > [2] commit ee76f7729babd2700afd6f3874449d8084dd85ea > > 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) > --- > arch/arm/boot/dts/imx53.dtsi | 2 +- > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-mxc_v2.c | 531 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 533 insertions(+), 1 deletion(-) > create mode 100644 drivers/rtc/rtc-mxc_v2.c > > diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi > index 589a67c5f796..3d1a55e11ea8 100644 > --- a/arch/arm/boot/dts/imx53.dtsi > +++ b/arch/arm/boot/dts/imx53.dtsi > @@ -434,7 +434,7 @@ > }; > > srtc: srtc@53fa4000 { > - compatible = "fsl,imx53-rtc", "fsl,imx25-rtc"; > + compatible = "fsl,imx53-rtc"; > reg = <0x53fa4000 0x4000>; > interrupts = <24>; > interrupt-parent = <&tzic>; > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index f2f50c11dc38..fb3dc458c185 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) += rtc-mxc_v2.o This definitively needs a different Kconfig symbol. > 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..5049b521b38e > --- /dev/null > +++ b/drivers/rtc/rtc-mxc_v2.c > @@ -0,0 +1,531 @@ I think SPDX identifier will soon be required on new files, please add one. > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights Reserved. > + */ > + > +/* > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > +/* > + * Implementation based on rtc-ds1553.c > + */ > + > +/*! > + * @defgroup RTC Real Time Clock (RTC) Driver for i.MX53 > + */ > +/*! > + * @file rtc-mxc_v2.c > + * @brief Real Time Clock interface > + * > + * This file contains Real Time Clock interface for Linux. > + * > + * @ingroup RTC > + */ > + Is that comment really useful? > +#include <linux/slab.h> > +#include <linux/delay.h> > +#include <linux/rtc.h> > +#include <linux/module.h> > +#include <linux/fs.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/uaccess.h> > +#include <linux/io.h> This should be in alphabetical order > +//#include <linux/mxc_srtc.h> This is not useful > +#define RTC_READ_TIME_47BIT _IOR('p', 0x20, unsigned long long) > +/* blocks until LPSCMR is set, returns difference */ > +#define RTC_WAIT_TIME_SET _IOR('p', 0x21, int64_t) > + Those ioctls are not used. > +#define SRTC_LPSCLR_LLPSC_LSH 17 /* start bit for LSB time value */ > + > +#define SRTC_LPPDR_INIT 0x41736166 /* init for glitch detect */ > + > +#define SRTC_LPCR_SWR_LP (1 << 0) /* lp software reset */ > +#define SRTC_LPCR_EN_LP (1 << 3) /* lp enable */ > +#define SRTC_LPCR_WAE (1 << 4) /* lp wakeup alarm enable */ > +#define SRTC_LPCR_SAE (1 << 5) /* lp security alarm enable */ > +#define SRTC_LPCR_SI (1 << 6) /* lp security interrupt enable */ > +#define SRTC_LPCR_ALP (1 << 7) /* lp alarm flag */ > +#define SRTC_LPCR_LTC (1 << 8) /* lp lock time counter */ > +#define SRTC_LPCR_LMC (1 << 9) /* lp lock monotonic counter */ > +#define SRTC_LPCR_SV (1 << 10) /* lp security violation */ > +#define SRTC_LPCR_NSA (1 << 11) /* lp non secure access */ > +#define SRTC_LPCR_NVEIE (1 << 12) /* lp non valid state exit int en */ > +#define SRTC_LPCR_IEIE (1 << 13) /* lp init state exit int enable */ > +#define SRTC_LPCR_NVE (1 << 14) /* lp non valid state exit bit */ > +#define SRTC_LPCR_IE (1 << 15) /* lp init state exit bit */ > + > +#define SRTC_LPCR_ALL_INT_EN (SRTC_LPCR_WAE | SRTC_LPCR_SAE | \ > + SRTC_LPCR_SI | SRTC_LPCR_ALP | \ > + SRTC_LPCR_NVEIE | SRTC_LPCR_IEIE) > + > +#define SRTC_LPSR_TRI (1 << 0) /* lp time read invalidate */ > +#define SRTC_LPSR_PGD (1 << 1) /* lp power supply glitc detected */ > +#define SRTC_LPSR_CTD (1 << 2) /* lp clock tampering detected */ > +#define SRTC_LPSR_ALP (1 << 3) /* lp alarm flag */ > +#define SRTC_LPSR_MR (1 << 4) /* lp monotonic counter rollover */ > +#define SRTC_LPSR_TR (1 << 5) /* lp time rollover */ > +#define SRTC_LPSR_EAD (1 << 6) /* lp external alarm detected */ > +#define SRTC_LPSR_IT0 (1 << 7) /* lp IIM throttle */ > +#define SRTC_LPSR_IT1 (1 << 8) > +#define SRTC_LPSR_IT2 (1 << 9) > +#define SRTC_LPSR_SM0 (1 << 10) /* lp security mode */ > +#define SRTC_LPSR_SM1 (1 << 11) > +#define SRTC_LPSR_STATE_LP0 (1 << 12) /* lp state */ > +#define SRTC_LPSR_STATE_LP1 (1 << 13) > +#define SRTC_LPSR_NVES (1 << 14) /* lp non-valid state exit status */ > +#define SRTC_LPSR_IES (1 << 15) /* lp init state exit status */ > + > +#define MAX_PIE_NUM 15 > +#define MAX_PIE_FREQ 32768 > +#define MIN_PIE_FREQ 1 > + > +#define SRTC_PI0 (1 << 0) > +#define SRTC_PI1 (1 << 1) > +#define SRTC_PI2 (1 << 2) > +#define SRTC_PI3 (1 << 3) > +#define SRTC_PI4 (1 << 4) > +#define SRTC_PI5 (1 << 5) > +#define SRTC_PI6 (1 << 6) > +#define SRTC_PI7 (1 << 7) > +#define SRTC_PI8 (1 << 8) > +#define SRTC_PI9 (1 << 9) > +#define SRTC_PI10 (1 << 10) > +#define SRTC_PI11 (1 << 11) > +#define SRTC_PI12 (1 << 12) > +#define SRTC_PI13 (1 << 13) > +#define SRTC_PI14 (1 << 14) > +#define SRTC_PI15 (1 << 15) > + > +#define PIT_ALL_ON (SRTC_PI1 | SRTC_PI2 | SRTC_PI3 | \ > + SRTC_PI4 | SRTC_PI5 | SRTC_PI6 | SRTC_PI7 | \ > + SRTC_PI8 | SRTC_PI9 | SRTC_PI10 | SRTC_PI11 | \ > + SRTC_PI12 | SRTC_PI13 | SRTC_PI14 | SRTC_PI15) > + > +#define SRTC_SWR_HP (1 << 0) /* hp software reset */ > +#define SRTC_EN_HP (1 << 3) /* hp enable */ > +#define SRTC_TS (1 << 4) /* time synchronize hp with lp */ > + > +#define SRTC_IE_AHP (1 << 16) /* Alarm HP Interrupt Enable bit */ > +#define SRTC_IE_WDHP (1 << 18) /* Write Done HP Interrupt Enable bit */ > +#define SRTC_IE_WDLP (1 << 19) /* Write Done LP Interrupt Enable bit */ > + > +#define SRTC_ISR_AHP (1 << 16) /* interrupt status: alarm hp */ > +#define SRTC_ISR_WDHP (1 << 18) /* interrupt status: write done hp */ > +#define SRTC_ISR_WDLP (1 << 19) /* interrupt status: write done lp */ > +#define SRTC_ISR_WPHP (1 << 20) /* interrupt status: write pending hp */ > +#define SRTC_ISR_WPLP (1 << 21) /* interrupt status: write pending lp */ > + > +#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_LPSMCR 0x0C /* LP Secure Monotonic Counter 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 */ > +#define SRTC_LPGR 0x1C /* LP General Purpose Reg */ > +#define SRTC_HPCMR 0x20 /* HP Counter MSB Reg */ > +#define SRTC_HPCLR 0x24 /* HP Counter LSB Reg */ > +#define SRTC_HPAMR 0x28 /* HP Alarm MSB Reg */ > +#define SRTC_HPALR 0x2C /* HP Alarm LSB Reg */ > +#define SRTC_HPCR 0x30 /* HP Control Reg */ > +#define SRTC_HPISR 0x34 /* HP Interrupt Status Reg */ > +#define SRTC_HPIENR 0x38 /* HP Interrupt Enable Reg */ > + > +#define SRTC_SECMODE_MASK 0x3 /* the mask of SRTC security mode */ > +#define SRTC_SECMODE_LOW 0x0 /* Low Security */ > +#define SRTC_SECMODE_MED 0x1 /* Medium Security */ > +#define SRTC_SECMODE_HIGH 0x2 /* High Security */ > +#define SRTC_SECMODE_RESERVED 0x3 /* Reserved */ > + > +struct rtc_drv_data { > + struct rtc_device *rtc; > + void __iomem *ioaddr; > + int irq; > + struct clk *clk; > +}; > + > +static DEFINE_SPINLOCK(rtc_lock); > + Shouldn't that lock be part of rtc_drv_data? > +/*! > + * 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. > + */ > +static inline void rtc_write_sync_lp(void __iomem *ioaddr) > +{ > + unsigned int i, count; > + /* Wait for 3 CKIL cycles */ > + for (i = 0; i < 3; i++) { > + count = readl(ioaddr + SRTC_LPSCLR); > + while ((readl(ioaddr + SRTC_LPSCLR)) == count) > + ; > + } > +} > + > +/*! > + * This function updates the RTC alarm registers and then clears all the > + * interrupt status bits. > + * > + * @param alrm the new alarm value to be updated in the RTC > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm) > +{ > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + struct rtc_time alarm_tm, now_tm; > + unsigned long now, time; > + int ret; > + > + now = __raw_readl(ioaddr + SRTC_LPSCMR); > + rtc_time_to_tm(now, &now_tm); > + > + alarm_tm.tm_year = now_tm.tm_year; > + alarm_tm.tm_mon = now_tm.tm_mon; > + alarm_tm.tm_mday = now_tm.tm_mday; > + > + alarm_tm.tm_hour = alrm->tm_hour; > + alarm_tm.tm_min = alrm->tm_min; > + alarm_tm.tm_sec = alrm->tm_sec; > + > + rtc_tm_to_time(&now_tm, &now); > + rtc_tm_to_time(&alarm_tm, &time); > + > + if (time < now) { > + time += 60 * 60 * 24; > + rtc_time_to_tm(time, &alarm_tm); > + } > + ret = rtc_tm_to_time(&alarm_tm, &time); > + > + __raw_writel(time, ioaddr + SRTC_LPSAR); > + > + /* clear alarm interrupt status bit */ > + __raw_writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR); > + > + return ret; > +} > + > +/*! > + * This function is the RTC interrupt service routine. > + * > + * @param irq RTC IRQ number > + * @param dev_id device ID which is not used > + * > + * @return IRQ_HANDLED as defined in the include/linux/interrupt.h file. > + */ > +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) > +{ > + struct platform_device *pdev = dev_id; > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + void __iomem *ioaddr = pdata->ioaddr; > + u32 lp_status, lp_cr; > + u32 events = 0; > + > + lp_status = __raw_readl(ioaddr + SRTC_LPSR); > + lp_cr = __raw_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 */ > + __raw_writel(lp_cr, ioaddr + SRTC_LPCR); > + > + /* clear interrupt status */ > + __raw_writel(lp_status, ioaddr + SRTC_LPSR); > + > + rtc_write_sync_lp(ioaddr); > + rtc_update_irq(pdata->rtc, 1, events); > + return IRQ_HANDLED; > +} > + > +/*! > + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); > + time_t now = readl(pdata->ioaddr + SRTC_LPSCMR); > + > + rtc_time_to_tm(now, tm); > + return rtc_valid_tm(tm); > +} > + > +/*! > + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); > + time64_t time = rtc_tm_to_time64(tm); > + > + if (time > UINT_MAX) { > + dev_warn(dev, "time_t has overflow\n"); > + } > + > + writel(time, pdata->ioaddr + SRTC_LPSCMR); > + rtc_write_sync_lp(pdata->ioaddr); > + return 0; > +} > + > +/*! > + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + > + rtc_time_to_tm(__raw_readl(ioaddr + SRTC_LPSAR), &alrm->time); > + alrm->pending = > + ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP) != 0) ? 1 : 0; > + > + return 0; > +} > + > +static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable) > +{ > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + unsigned long lock_flags = 0; > + u32 lp_cr; > + > + spin_lock_irqsave(&rtc_lock, lock_flags); > + lp_cr = __raw_readl(pdata->ioaddr + SRTC_LPCR); > + > + if (enable) > + lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + else > + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + > + __raw_writel(lp_cr, pdata->ioaddr + SRTC_LPCR); > + spin_unlock_irqrestore(&rtc_lock, lock_flags); > + return 0; > +} > + > +/*! > + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + int ret; > + > + ret = rtc_update_alarm(dev, &alrm->time); > + if (!ret) > + mxc_rtc_alarm_irq_enable(dev, alrm->enabled); > + > + rtc_write_sync_lp(ioaddr); > + return ret; > +} > + > +/*! > + * The RTC driver structure > + */ > +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, > +}; > + > +/*! MXC RTC Power management control */ This comment is probably wrong ;) > +static int mxc_rtc_probe(struct platform_device *pdev) > +{ > + struct timespec tv; > + struct resource *res; > + struct rtc_drv_data *pdata = NULL; > + 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); > + } > + ret = clk_prepare_enable(pdata->clk); > + if (ret) > + return ret; > + > + /* Configure and enable the RTC */ > + pdata->irq = platform_get_irq(pdev, 0); > + if (pdata->irq >= 0 > + && devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt, > + IRQF_SHARED, pdev->name, pdev) < 0) { > + dev_warn(&pdev->dev, "interrupt not available.\n"); > + pdata->irq = -1; > + } > + > + if (pdata->irq >= 0) > + device_init_wakeup(&pdev->dev, 1); > + > + /* initialize glitch detect */ > + __raw_writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR); > + udelay(100); > + > + /* clear lp interrupt status */ > + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); > + udelay(100); > + > + /* move out of init state */ > + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR); > + > + udelay(100); > + > + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_IES) == 0) > + ; > + > + /* move out of non-valid state */ > + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA | > + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR); > + > + udelay(100); > + > + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_NVES) == 0) > + ; > + > + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); > + udelay(100); > + > + platform_set_drvdata(pdev, pdata); > + pdata->rtc = > + devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops, > + THIS_MODULE); > + if (IS_ERR(pdata->rtc)) { > + ret = PTR_ERR(pdata->rtc); > + goto exit_put_clk; > + } > + > + tv.tv_nsec = 0; > + tv.tv_sec = __raw_readl(ioaddr + SRTC_LPSCMR); > + > + /* By default, devices should wakeup if they can */ > + /* So srtc is set as "should wakeup" as it can */ That comment is not formatted correctly. > + device_init_wakeup(&pdev->dev, 1); > + > + return ret; For clarity, this has to return 0. > + > +exit_put_clk: > + clk_disable_unprepare(pdata->clk); > + return ret; > +} > + > +static int __exit mxc_rtc_remove(struct platform_device *pdev) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(pdata->clk); > + return 0; > +} > + > +/*! > + * This function is called to save the system time delta relative to > + * the MXC RTC when enterring a low power state. This time delta is > + * then used on resume to adjust the system time to account for time > + * loss while suspended. > + * > + * @param pdev not used > + * @param state Power state to enter. > + * > + * @return The function always returns 0. > + */ > +static int mxc_rtc_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(&pdev->dev)) > + enable_irq_wake(pdata->irq); > + > + return 0; > +} > + > +/*! > + * This function is called to correct the system time based on the > + * current MXC RTC time relative to the time delta saved during > + * suspend. > + * > + * @param pdev not used > + * > + * @return The function always returns 0. > + */ > +static int mxc_rtc_resume(struct platform_device *pdev) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(&pdev->dev)) > + disable_irq_wake(pdata->irq); > + > + return 0; > +} > + > +static const struct of_device_id mxc_ids[] = { > + {.compatible = "fsl,imx53-rtc",}, > + {} > +}; > + > +/*! > + * Contains pointers to the power management callback functions. > + */ > +static struct platform_driver mxc_rtc_driver = { > + .driver = { > + .name = "mxc_rtc_v8", Isn't it weird to have v8 here when the file is named v2? > + .of_match_table = mxc_ids, > + }, > + .probe = mxc_rtc_probe, > + .remove = mxc_rtc_remove, > + .suspend = mxc_rtc_suspend, > + .resume = mxc_rtc_resume, > +}; > + > +module_platform_driver(mxc_rtc_driver); > + > +MODULE_AUTHOR("Freescale Semiconductor, Inc."); > +MODULE_DESCRIPTION("Realtime Clock Driver (RTC)"); > +MODULE_LICENSE("GPL"); > -- > 2.11.0 > >
On Wed, Nov 29, 2017 at 11:11 PM, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote: > On 28/11/2017 at 08:39:27 +0100, linux-kernel-dev@beckhoff.com wrote: >> From: Patrick Bruenn <p.bruenn@beckhoff.com> [] >> diff --git a/drivers/rtc/rtc-mxc_v2.c b/drivers/rtc/rtc-mxc_v2.c >> new file mode 100644 >> index 000000000000..5049b521b38e >> --- /dev/null >> +++ b/drivers/rtc/rtc-mxc_v2.c >> @@ -0,0 +1,531 @@ > > I think SPDX identifier will soon be required on new files, please add > one. > >> +/* >> + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights Reserved. >> + */ >> + >> +/* >> + * The code contained herein is licensed under the GNU General Public >> + * License. You may obtain a copy of the GNU General Public License >> + * Version 2 or later at the following locations: >> + * >> + * http://www.opensource.org/licenses/gpl-license.html >> + * http://www.gnu.org/copyleft/gpl.html >> + */ Exactly! And while you are it , you could replace the boilerplate license text with the SPDX id.
On Tue, Nov 28, 2017 at 08:39:27AM +0100, linux-kernel-dev@beckhoff.com wrote: > From: Patrick Bruenn <p.bruenn@beckhoff.com> > > Neither rtc-imxdi nor rtc-mxc are compatible with i.MX53. > Add a modernized version of mxc_v2 from here: > 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 > > Changes to that version: > - updated to v4.15-rc1 > - removed ioctl() > - removed proc() > - removed manual(redundant) enable_irq flag > > Signed-off-by: Patrick Bruenn <p.bruenn@beckhoff.com> > > --- > > Open issues: > - driver naming, should it be merged with rtc-mxc.c ? > - document DT binding "fsl,imx53-rtc" accordingly > - Should unused defines be removed or kept for someone else to be > useful? > - Is the use of __raw_readl/writel() correct? Should it be replaced with > readl/writel()? > - suspend/resume() seems different to existing rtc-mxc.c, should I apply > the pattern from rtc-mxc.c? > - On Shawns tree imx53.dtsi has been reverted already[1][2]. Should I split > the imx53.dtsi change into a separate patch based on his tree? Or can > we still stop the full revert and just remove the imx25-rtc compatible? > I am not in a hurry, so we could just wait until the revert landed in > Linus tree. Whatever you think is best. > > [1] https://www.spinics.net/lists/arm-kernel/msg617113.html > [2] commit ee76f7729babd2700afd6f3874449d8084dd85ea > > 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) > --- > arch/arm/boot/dts/imx53.dtsi | 2 +- > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-mxc_v2.c | 531 +++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 533 insertions(+), 1 deletion(-) > create mode 100644 drivers/rtc/rtc-mxc_v2.c > > diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi > index 589a67c5f796..3d1a55e11ea8 100644 > --- a/arch/arm/boot/dts/imx53.dtsi > +++ b/arch/arm/boot/dts/imx53.dtsi > @@ -434,7 +434,7 @@ > }; > > srtc: srtc@53fa4000 { > - compatible = "fsl,imx53-rtc", "fsl,imx25-rtc"; > + compatible = "fsl,imx53-rtc"; > reg = <0x53fa4000 0x4000>; > interrupts = <24>; > interrupt-parent = <&tzic>; > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index f2f50c11dc38..fb3dc458c185 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) += 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..5049b521b38e > --- /dev/null > +++ b/drivers/rtc/rtc-mxc_v2.c > @@ -0,0 +1,531 @@ > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights Reserved. > + */ > + > +/* > + * The code contained herein is licensed under the GNU General Public > + * License. You may obtain a copy of the GNU General Public License > + * Version 2 or later at the following locations: > + * > + * http://www.opensource.org/licenses/gpl-license.html > + * http://www.gnu.org/copyleft/gpl.html > + */ > +/* > + * Implementation based on rtc-ds1553.c > + */ > + > +/*! > + * @defgroup RTC Real Time Clock (RTC) Driver for i.MX53 > + */ > +/*! > + * @file rtc-mxc_v2.c > + * @brief Real Time Clock interface > + * > + * This file contains Real Time Clock interface for Linux. > + * > + * @ingroup RTC > + */ > + > +#include <linux/slab.h> > +#include <linux/delay.h> > +#include <linux/rtc.h> > +#include <linux/module.h> > +#include <linux/fs.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/platform_device.h> > +#include <linux/clk.h> > +#include <linux/device.h> > +#include <linux/uaccess.h> > +#include <linux/io.h> > +//#include <linux/mxc_srtc.h> > +#define RTC_READ_TIME_47BIT _IOR('p', 0x20, unsigned long long) > +/* blocks until LPSCMR is set, returns difference */ > +#define RTC_WAIT_TIME_SET _IOR('p', 0x21, int64_t) > + > +#define SRTC_LPSCLR_LLPSC_LSH 17 /* start bit for LSB time value */ > + > +#define SRTC_LPPDR_INIT 0x41736166 /* init for glitch detect */ > + > +#define SRTC_LPCR_SWR_LP (1 << 0) /* lp software reset */ > +#define SRTC_LPCR_EN_LP (1 << 3) /* lp enable */ > +#define SRTC_LPCR_WAE (1 << 4) /* lp wakeup alarm enable */ > +#define SRTC_LPCR_SAE (1 << 5) /* lp security alarm enable */ > +#define SRTC_LPCR_SI (1 << 6) /* lp security interrupt enable */ > +#define SRTC_LPCR_ALP (1 << 7) /* lp alarm flag */ > +#define SRTC_LPCR_LTC (1 << 8) /* lp lock time counter */ > +#define SRTC_LPCR_LMC (1 << 9) /* lp lock monotonic counter */ > +#define SRTC_LPCR_SV (1 << 10) /* lp security violation */ > +#define SRTC_LPCR_NSA (1 << 11) /* lp non secure access */ > +#define SRTC_LPCR_NVEIE (1 << 12) /* lp non valid state exit int en */ > +#define SRTC_LPCR_IEIE (1 << 13) /* lp init state exit int enable */ > +#define SRTC_LPCR_NVE (1 << 14) /* lp non valid state exit bit */ > +#define SRTC_LPCR_IE (1 << 15) /* lp init state exit bit */ > + > +#define SRTC_LPCR_ALL_INT_EN (SRTC_LPCR_WAE | SRTC_LPCR_SAE | \ > + SRTC_LPCR_SI | SRTC_LPCR_ALP | \ > + SRTC_LPCR_NVEIE | SRTC_LPCR_IEIE) > + > +#define SRTC_LPSR_TRI (1 << 0) /* lp time read invalidate */ > +#define SRTC_LPSR_PGD (1 << 1) /* lp power supply glitc detected */ > +#define SRTC_LPSR_CTD (1 << 2) /* lp clock tampering detected */ > +#define SRTC_LPSR_ALP (1 << 3) /* lp alarm flag */ > +#define SRTC_LPSR_MR (1 << 4) /* lp monotonic counter rollover */ > +#define SRTC_LPSR_TR (1 << 5) /* lp time rollover */ > +#define SRTC_LPSR_EAD (1 << 6) /* lp external alarm detected */ > +#define SRTC_LPSR_IT0 (1 << 7) /* lp IIM throttle */ > +#define SRTC_LPSR_IT1 (1 << 8) > +#define SRTC_LPSR_IT2 (1 << 9) > +#define SRTC_LPSR_SM0 (1 << 10) /* lp security mode */ > +#define SRTC_LPSR_SM1 (1 << 11) > +#define SRTC_LPSR_STATE_LP0 (1 << 12) /* lp state */ > +#define SRTC_LPSR_STATE_LP1 (1 << 13) > +#define SRTC_LPSR_NVES (1 << 14) /* lp non-valid state exit status */ > +#define SRTC_LPSR_IES (1 << 15) /* lp init state exit status */ > + > +#define MAX_PIE_NUM 15 > +#define MAX_PIE_FREQ 32768 > +#define MIN_PIE_FREQ 1 > + > +#define SRTC_PI0 (1 << 0) > +#define SRTC_PI1 (1 << 1) > +#define SRTC_PI2 (1 << 2) > +#define SRTC_PI3 (1 << 3) > +#define SRTC_PI4 (1 << 4) > +#define SRTC_PI5 (1 << 5) > +#define SRTC_PI6 (1 << 6) > +#define SRTC_PI7 (1 << 7) > +#define SRTC_PI8 (1 << 8) > +#define SRTC_PI9 (1 << 9) > +#define SRTC_PI10 (1 << 10) > +#define SRTC_PI11 (1 << 11) > +#define SRTC_PI12 (1 << 12) > +#define SRTC_PI13 (1 << 13) > +#define SRTC_PI14 (1 << 14) > +#define SRTC_PI15 (1 << 15) > + > +#define PIT_ALL_ON (SRTC_PI1 | SRTC_PI2 | SRTC_PI3 | \ > + SRTC_PI4 | SRTC_PI5 | SRTC_PI6 | SRTC_PI7 | \ > + SRTC_PI8 | SRTC_PI9 | SRTC_PI10 | SRTC_PI11 | \ > + SRTC_PI12 | SRTC_PI13 | SRTC_PI14 | SRTC_PI15) > + > +#define SRTC_SWR_HP (1 << 0) /* hp software reset */ > +#define SRTC_EN_HP (1 << 3) /* hp enable */ > +#define SRTC_TS (1 << 4) /* time synchronize hp with lp */ > + > +#define SRTC_IE_AHP (1 << 16) /* Alarm HP Interrupt Enable bit */ > +#define SRTC_IE_WDHP (1 << 18) /* Write Done HP Interrupt Enable bit */ > +#define SRTC_IE_WDLP (1 << 19) /* Write Done LP Interrupt Enable bit */ > + > +#define SRTC_ISR_AHP (1 << 16) /* interrupt status: alarm hp */ > +#define SRTC_ISR_WDHP (1 << 18) /* interrupt status: write done hp */ > +#define SRTC_ISR_WDLP (1 << 19) /* interrupt status: write done lp */ > +#define SRTC_ISR_WPHP (1 << 20) /* interrupt status: write pending hp */ > +#define SRTC_ISR_WPLP (1 << 21) /* interrupt status: write pending lp */ > + > +#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_LPSMCR 0x0C /* LP Secure Monotonic Counter 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 */ > +#define SRTC_LPGR 0x1C /* LP General Purpose Reg */ > +#define SRTC_HPCMR 0x20 /* HP Counter MSB Reg */ > +#define SRTC_HPCLR 0x24 /* HP Counter LSB Reg */ > +#define SRTC_HPAMR 0x28 /* HP Alarm MSB Reg */ > +#define SRTC_HPALR 0x2C /* HP Alarm LSB Reg */ > +#define SRTC_HPCR 0x30 /* HP Control Reg */ > +#define SRTC_HPISR 0x34 /* HP Interrupt Status Reg */ > +#define SRTC_HPIENR 0x38 /* HP Interrupt Enable Reg */ > + > +#define SRTC_SECMODE_MASK 0x3 /* the mask of SRTC security mode */ > +#define SRTC_SECMODE_LOW 0x0 /* Low Security */ > +#define SRTC_SECMODE_MED 0x1 /* Medium Security */ > +#define SRTC_SECMODE_HIGH 0x2 /* High Security */ > +#define SRTC_SECMODE_RESERVED 0x3 /* Reserved */ > + > +struct rtc_drv_data { > + struct rtc_device *rtc; > + void __iomem *ioaddr; > + int irq; > + struct clk *clk; > +}; > + > +static DEFINE_SPINLOCK(rtc_lock); The lock should be a member of your driver data struct. > + > +/*! > + * 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. > + */ Please drop this exclamation mark at the beginning of comments. If anything, use kerneldoc style for the function descriptions. > +static inline void rtc_write_sync_lp(void __iomem *ioaddr) > +{ > + unsigned int i, count; > + /* Wait for 3 CKIL cycles */ > + for (i = 0; i < 3; i++) { > + count = readl(ioaddr + SRTC_LPSCLR); > + while ((readl(ioaddr + SRTC_LPSCLR)) == count) > + ; This becomes an infinite loop when the hardware is not working as expected. You should avoid that. > + } > +} > + > +/*! > + * This function updates the RTC alarm registers and then clears all the > + * interrupt status bits. > + * > + * @param alrm the new alarm value to be updated in the RTC > + * > + * @return 0 if successful; non-zero otherwise. > + */ > +static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm) > +{ mxc_rtc_update_alarm please like the other device specific functions please. Same for rtc_write_sync_lp(). > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + struct rtc_time alarm_tm, now_tm; > + unsigned long now, time; > + int ret; > + > + now = __raw_readl(ioaddr + SRTC_LPSCMR); > + rtc_time_to_tm(now, &now_tm); > + > + alarm_tm.tm_year = now_tm.tm_year; > + alarm_tm.tm_mon = now_tm.tm_mon; > + alarm_tm.tm_mday = now_tm.tm_mday; > + > + alarm_tm.tm_hour = alrm->tm_hour; > + alarm_tm.tm_min = alrm->tm_min; > + alarm_tm.tm_sec = alrm->tm_sec; > + > + rtc_tm_to_time(&now_tm, &now); > + rtc_tm_to_time(&alarm_tm, &time); > + > + if (time < now) { > + time += 60 * 60 * 24; > + rtc_time_to_tm(time, &alarm_tm); > + } > + ret = rtc_tm_to_time(&alarm_tm, &time); > + > + __raw_writel(time, ioaddr + SRTC_LPSAR); > + > + /* clear alarm interrupt status bit */ > + __raw_writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR); > + > + return ret; > +} > + > +/*! > + * This function is the RTC interrupt service routine. > + * > + * @param irq RTC IRQ number > + * @param dev_id device ID which is not used > + * > + * @return IRQ_HANDLED as defined in the include/linux/interrupt.h file. > + */ > +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) > +{ > + struct platform_device *pdev = dev_id; > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + void __iomem *ioaddr = pdata->ioaddr; > + u32 lp_status, lp_cr; > + u32 events = 0; > + > + lp_status = __raw_readl(ioaddr + SRTC_LPSR); > + lp_cr = __raw_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 */ > + __raw_writel(lp_cr, ioaddr + SRTC_LPCR); > + > + /* clear interrupt status */ > + __raw_writel(lp_status, ioaddr + SRTC_LPSR); > + > + rtc_write_sync_lp(ioaddr); > + rtc_update_irq(pdata->rtc, 1, events); > + return IRQ_HANDLED; > +} What about locking? You lock the read/modify/write operation of the LPCR register below, and so you should do here. > + > +/*! > + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); > + time_t now = readl(pdata->ioaddr + SRTC_LPSCMR); > + > + rtc_time_to_tm(now, tm); > + return rtc_valid_tm(tm); > +} > + > +/*! > + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); > + time64_t time = rtc_tm_to_time64(tm); > + > + if (time > UINT_MAX) { > + dev_warn(dev, "time_t has overflow\n"); > + } SIGH. Who builds such a RTC in the 21st century? Shouldn't this return an error? > + > + writel(time, pdata->ioaddr + SRTC_LPSCMR); > + rtc_write_sync_lp(pdata->ioaddr); > + return 0; > +} > + > +/*! > + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + > + rtc_time_to_tm(__raw_readl(ioaddr + SRTC_LPSAR), &alrm->time); > + alrm->pending = > + ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP) != 0) ? 1 : 0; if/else would be much easier to read here. > + > + return 0; > +} > + > +static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable) > +{ > + struct rtc_drv_data *pdata = dev_get_drvdata(dev); > + unsigned long lock_flags = 0; > + u32 lp_cr; > + > + spin_lock_irqsave(&rtc_lock, lock_flags); > + lp_cr = __raw_readl(pdata->ioaddr + SRTC_LPCR); > + > + if (enable) > + lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + else > + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); > + > + __raw_writel(lp_cr, pdata->ioaddr + SRTC_LPCR); > + spin_unlock_irqrestore(&rtc_lock, lock_flags); > + return 0; > +} > + > +/*! > + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); > + void __iomem *ioaddr = pdata->ioaddr; > + int ret; > + > + ret = rtc_update_alarm(dev, &alrm->time); > + if (!ret) > + mxc_rtc_alarm_irq_enable(dev, alrm->enabled); > + > + rtc_write_sync_lp(ioaddr); > + return ret; > +} > + > +/*! > + * The RTC driver structure > + */ > +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, > +}; > + > +/*! MXC RTC Power management control */ > +static int mxc_rtc_probe(struct platform_device *pdev) > +{ > + struct timespec tv; > + struct resource *res; > + struct rtc_drv_data *pdata = NULL; No need to initialize. > + 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); > + } > + ret = clk_prepare_enable(pdata->clk); > + if (ret) > + return ret; > + > + /* Configure and enable the RTC */ > + pdata->irq = platform_get_irq(pdev, 0); > + if (pdata->irq >= 0 > + && devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt, > + IRQF_SHARED, pdev->name, pdev) < 0) { > + dev_warn(&pdev->dev, "interrupt not available.\n"); > + pdata->irq = -1; > + } I would just make the interrupt mandatory. I see no valid reason why it shouldn't be available and it makes your driver logic simpler when you can just assume it's available. > + > + if (pdata->irq >= 0) > + device_init_wakeup(&pdev->dev, 1); > + > + /* initialize glitch detect */ > + __raw_writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR); > + udelay(100); > + > + /* clear lp interrupt status */ > + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); > + udelay(100); > + > + /* move out of init state */ > + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR); > + > + udelay(100); > + > + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_IES) == 0) > + ; > + > + /* move out of non-valid state */ > + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA | > + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR); > + > + udelay(100); Where do all these udelay() come from? Are they really needed? > + > + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_NVES) == 0) > + ; > + > + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); > + udelay(100); > + > + platform_set_drvdata(pdev, pdata); > + pdata->rtc = > + devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops, > + THIS_MODULE); > + if (IS_ERR(pdata->rtc)) { > + ret = PTR_ERR(pdata->rtc); > + goto exit_put_clk; > + } > + > + tv.tv_nsec = 0; > + tv.tv_sec = __raw_readl(ioaddr + SRTC_LPSCMR); Why this? tv is set but unused. > + > + /* By default, devices should wakeup if they can */ > + /* So srtc is set as "should wakeup" as it can */ > + device_init_wakeup(&pdev->dev, 1); Why is this done twice, one time only when we have a valid irq? This looks fishy. > + > + return ret; > + > +exit_put_clk: > + clk_disable_unprepare(pdata->clk); > + return ret; > +} > + > +static int __exit mxc_rtc_remove(struct platform_device *pdev) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(pdata->clk); > + return 0; > +} > + > +/*! > + * This function is called to save the system time delta relative to > + * the MXC RTC when enterring a low power state. This time delta is > + * then used on resume to adjust the system time to account for time > + * loss while suspended. > + * > + * @param pdev not used Not true. > + * @param state Power state to enter. > + * > + * @return The function always returns 0. > + */ > +static int mxc_rtc_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(&pdev->dev)) > + enable_irq_wake(pdata->irq); > + > + return 0; > +} > + > +/*! > + * This function is called to correct the system time based on the > + * current MXC RTC time relative to the time delta saved during > + * suspend. > + * > + * @param pdev not used Not true. > + * > + * @return The function always returns 0. > + */ > +static int mxc_rtc_resume(struct platform_device *pdev) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(&pdev->dev)) > + disable_irq_wake(pdata->irq); > + > + return 0; > +} > + > +static const struct of_device_id mxc_ids[] = { > + {.compatible = "fsl,imx53-rtc",}, > + {} > +}; > + > +/*! > + * Contains pointers to the power management callback functions. True, but doesn't contain any useful information. > + */ > +static struct platform_driver mxc_rtc_driver = { > + .driver = { > + .name = "mxc_rtc_v8", > + .of_match_table = mxc_ids, > + }, Indentation is broken here. > + .probe = mxc_rtc_probe, > + .remove = mxc_rtc_remove, > + .suspend = mxc_rtc_suspend, > + .resume = mxc_rtc_resume, > +}; > + > +module_platform_driver(mxc_rtc_driver); > + > +MODULE_AUTHOR("Freescale Semiconductor, Inc."); > +MODULE_DESCRIPTION("Realtime Clock Driver (RTC)"); > +MODULE_LICENSE("GPL"); Sascha
Hi, On Thu, 30 Nov 2017 10:50:47 +0100 Sascha Hauer wrote: > On Tue, Nov 28, 2017 at 08:39:27AM +0100, linux-kernel-dev@beckhoff.com wrote: [...] > > +/*! MXC RTC Power management control */ > > +static int mxc_rtc_probe(struct platform_device *pdev) > > +{ > > + struct timespec tv; > > + struct resource *res; > > + struct rtc_drv_data *pdata = NULL; > > No need to initialize. > > > + void __iomem *ioaddr; > > + int ret = 0; > Same here. Lothar Waßmann
Hi, On Tue, 28 Nov 2017 08:39:27 +0100 linux-kernel-dev@beckhoff.com wrote: > From: Patrick Bruenn <p.bruenn@beckhoff.com> [...] > +/*! > + * This function is the RTC interrupt service routine. > + * > + * @param irq RTC IRQ number > + * @param dev_id device ID which is not used > + * > + * @return IRQ_HANDLED as defined in the include/linux/interrupt.h file. > + */ > +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) > +{ > + struct platform_device *pdev = dev_id; > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + void __iomem *ioaddr = pdata->ioaddr; > + u32 lp_status, lp_cr; > + u32 events = 0; > + > + lp_status = __raw_readl(ioaddr + SRTC_LPSR); > + lp_cr = __raw_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 */ > + __raw_writel(lp_cr, ioaddr + SRTC_LPCR); > + > + /* clear interrupt status */ > + __raw_writel(lp_status, ioaddr + SRTC_LPSR); > + > + rtc_write_sync_lp(ioaddr); > + rtc_update_irq(pdata->rtc, 1, events); > + return IRQ_HANDLED; > +} > + see comment below... > +/*! MXC RTC Power management control */ > +static int mxc_rtc_probe(struct platform_device *pdev) > +{ > + struct timespec tv; > + struct resource *res; > + struct rtc_drv_data *pdata = NULL; > + 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); > + } > + ret = clk_prepare_enable(pdata->clk); > + if (ret) > + return ret; > Is it really necessary to have the clock enabled all the time, or should it be enabled/disabled for the register accesses only? > + /* Configure and enable the RTC */ > + pdata->irq = platform_get_irq(pdev, 0); > + if (pdata->irq >= 0 > + && devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt, > + IRQF_SHARED, pdev->name, pdev) < 0) { > If requesting an IRQ with the IRQF_SHARED flag, the interrupt handler must check whether it was responsible for the IRQ and return IRQ_NONE if that is not the case to allow some other interrupt handler to jump in. But AFAICS the RTC IRQ is not shared with any other device, so requesting the IRQ with IRQF_SHARED is invalid here! > + dev_warn(&pdev->dev, "interrupt not available.\n"); > + pdata->irq = -1; > + } > + > + if (pdata->irq >= 0) > + device_init_wakeup(&pdev->dev, 1); > + > + /* initialize glitch detect */ > + __raw_writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR); > + udelay(100); > + > + /* clear lp interrupt status */ > + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); > + udelay(100); > + > + /* move out of init state */ > + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR); > + > + udelay(100); > + > + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_IES) == 0) > + ; > Loops polling for the change of HW controlled bits should have a timeout, to prevent locking up when the hardware misbehaves. > + /* move out of non-valid state */ > + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA | > + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR); > + > + udelay(100); > + > + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_NVES) == 0) > + ; > dto. > + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); > + udelay(100); > + > + platform_set_drvdata(pdev, pdata); > The IRQ handler uses platform_get_drvdata(), so this has to be done BEFORE registering the interrupt handler. > + pdata->rtc = > + devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops, > + THIS_MODULE); > + if (IS_ERR(pdata->rtc)) { > + ret = PTR_ERR(pdata->rtc); > + goto exit_put_clk; > + } > + > + tv.tv_nsec = 0; > + tv.tv_sec = __raw_readl(ioaddr + SRTC_LPSCMR); > + > + /* By default, devices should wakeup if they can */ > + /* So srtc is set as "should wakeup" as it can */ > multi line comment style? > + device_init_wakeup(&pdev->dev, 1); > + > + return ret; > + > +exit_put_clk: > + clk_disable_unprepare(pdata->clk); > + return ret; > +} > + > +static int __exit mxc_rtc_remove(struct platform_device *pdev) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(pdata->clk); > + return 0; > +} > + > +/*! > + * This function is called to save the system time delta relative to > + * the MXC RTC when enterring a low power state. This time delta is > + * then used on resume to adjust the system time to account for time > + * loss while suspended. > + * > + * @param pdev not used > + * @param state Power state to enter. > + * > + * @return The function always returns 0. > + */ > +static int mxc_rtc_suspend(struct platform_device *pdev, pm_message_t state) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(&pdev->dev)) > + enable_irq_wake(pdata->irq); > + > + return 0; > +} > + > +/*! > + * This function is called to correct the system time based on the > + * current MXC RTC time relative to the time delta saved during > + * suspend. > + * > + * @param pdev not used > + * > + * @return The function always returns 0. > + */ > +static int mxc_rtc_resume(struct platform_device *pdev) > +{ > + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); > + > + if (device_may_wakeup(&pdev->dev)) > + disable_irq_wake(pdata->irq); > + > + return 0; > +} > + > +static const struct of_device_id mxc_ids[] = { > + {.compatible = "fsl,imx53-rtc",}, > missing spaces after '{' and before '}' Lothar Waßmann
>From: Philippe Ombredanne [mailto:pombredanne@nexb.com] >Sent: Donnerstag, 30. November 2017 09:18 >> >>> +/* >>> + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights >Reserved. >>> + */ >>> + >>> +/* >>> + * The code contained herein is licensed under the GNU General Public >>> + * License. You may obtain a copy of the GNU General Public License >>> + * Version 2 or later at the following locations: >>> + * >>> + * http://www.opensource.org/licenses/gpl-license.html >>> + * http://www.gnu.org/copyleft/gpl.html >>> + */ > >Exactly! >And while you are it , you could replace the boilerplate license text >with the SPDX id. >-- How would a perfect header look like? Looking at : git show b24413180f5600bcb3bb70fbed5cf186b60864bd -- drivers/ and: git grep SPDX -- Documentation/ it could be: +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights + */ or: +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights + */ Personally I would prefer: +/* + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights + * + * SPDX-License-Identifier: GPL-2.0 + */ So, is there any guideline? To be clear: I don't want to waste anyone's time on this. But as SPDX was intended for automation, I think it might be good to try having some common pattern here. I don't want to start a discussion, so in case there is no guideline I will just take the /* SPDX */ in firstline version. Regards, Patrick Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
Hi, On 30/11/2017 at 13:36:22 +0000, Patrick Brünn wrote: > >From: Philippe Ombredanne [mailto:pombredanne@nexb.com] > >Sent: Donnerstag, 30. November 2017 09:18 > >> > >>> +/* > >>> + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > >Reserved. > >>> + */ > >>> + > >>> +/* > >>> + * The code contained herein is licensed under the GNU General Public > >>> + * License. You may obtain a copy of the GNU General Public License > >>> + * Version 2 or later at the following locations: > >>> + * > >>> + * http://www.opensource.org/licenses/gpl-license.html > >>> + * http://www.gnu.org/copyleft/gpl.html > >>> + */ > > > >Exactly! > >And while you are it , you could replace the boilerplate license text > >with the SPDX id. > >-- > > How would a perfect header look like? > Looking at : > git show b24413180f5600bcb3bb70fbed5cf186b60864bd -- drivers/ > and: > git grep SPDX -- Documentation/ > > it could be: > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > + */ > or: > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > + */ > > Personally I would prefer: > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ > > So, is there any guideline? > I'm quoting LWN here: "For normal C source files, the string will be a comment using the "//" syntax; header files, instead, use traditional (/* */) comments for reasons related to tooling." > To be clear: I don't want to waste anyone's time on this. But as SPDX > was intended for automation, I think it might be good to try having > some common pattern here. > I don't want to start a discussion, so in case there is no guideline I will > just take the /* SPDX */ in firstline version.
On Thu, Nov 30, 2017 at 2:36 PM, Patrick Brünn <P.Bruenn@beckhoff.com> wrote: >>From: Philippe Ombredanne [mailto:pombredanne@nexb.com] >>Sent: Donnerstag, 30. November 2017 09:18 >>> >>>> +/* >>>> + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights >>Reserved. >>>> + */ >>>> + >>>> +/* >>>> + * The code contained herein is licensed under the GNU General Public >>>> + * License. You may obtain a copy of the GNU General Public License >>>> + * Version 2 or later at the following locations: >>>> + * >>>> + * http://www.opensource.org/licenses/gpl-license.html >>>> + * http://www.gnu.org/copyleft/gpl.html >>>> + */ >> >>Exactly! >>And while you are it , you could replace the boilerplate license text >>with the SPDX id. >>-- > > How would a perfect header look like? > Looking at : > git show b24413180f5600bcb3bb70fbed5cf186b60864bd -- drivers/ > and: > git grep SPDX -- Documentation/ > > it could be: > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > + */ > or: > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > + */ > > Personally I would prefer: > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ > > So, is there any guideline? > > To be clear: I don't want to waste anyone's time on this. But as SPDX > was intended for automation, I think it might be good to try having > some common pattern here. > I don't want to start a discussion, so in case there is no guideline I will > just take the /* SPDX */ in firstline version. Patrick: There are several threads on the topic explaining why: Linus made it clear that he wants this, and on the first line: > +// SPDX-License-Identifier: GPL-2.0 Thomas: I tend to prefer as Linus pointed the // style throughout when there is only a license and copyright involved. Here this would mean: > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2004-2011 Freescale Semiconductor, Inc. All Rights Reserved. What's you take since you wrote the doc for this? And is there still some updates needed for this since your last patch set? Patrick: Side note a trailing "All Rights Reserved." in a copyright statement is never needed, even though customarily cargo culted by everyone, including me. Started in 1910, it become fully obsolete in 2000 thanks to Nicaragua [1] signing the Berne convention. [1] https://en.wikipedia.org/wiki/All_rights_reserved
>From: Philippe Ombredanne [mailto:pombredanne@nexb.com] >Sent: Donnerstag, 30. November 2017 15:00 >On Thu, Nov 30, 2017 at 2:36 PM, Patrick Brünn <P.Bruenn@beckhoff.com> >wrote: > >Thomas: >I tend to prefer as Linus pointed the // style throughout when there >is only a license and copyright involved. Here this would mean: > >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2004-2011 Freescale Semiconductor, Inc. All Rights >Reserved. > >What's you take since you wrote the doc for this? And is there still >some updates needed for this since your last patch set? > While looking closer into different files I think it would become multiline again: +// 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 + */ > >Patrick: >Side note a trailing "All Rights Reserved." in a copyright statement >is never needed, even though customarily cargo culted by everyone, >including me. >Started in 1910, it become fully obsolete in 2000 thanks to Nicaragua >[1] signing the Berne convention. > >[1] https://en.wikipedia.org/wiki/All_rights_reserved Interessting. I just kept it because it was in the original file from freescale. But with this background, I think it's fine to drop it, now. Thanks, Patrick Beckhoff Automation GmbH & Co. KG | Managing Director: Dipl. Phys. Hans Beckhoff Registered office: Verl, Germany | Register court: Guetersloh HRA 7075
On Thu, Nov 30, 2017 at 01:36:22PM +0000, Patrick Brünn wrote: > >From: Philippe Ombredanne [mailto:pombredanne@nexb.com] > >Sent: Donnerstag, 30. November 2017 09:18 > >> > >>> +/* > >>> + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > >Reserved. > >>> + */ > >>> + > >>> +/* > >>> + * The code contained herein is licensed under the GNU General Public > >>> + * License. You may obtain a copy of the GNU General Public License > >>> + * Version 2 or later at the following locations: > >>> + * > >>> + * http://www.opensource.org/licenses/gpl-license.html > >>> + * http://www.gnu.org/copyleft/gpl.html > >>> + */ > > > >Exactly! > >And while you are it , you could replace the boilerplate license text > >with the SPDX id. > >-- > > How would a perfect header look like? > Looking at : > git show b24413180f5600bcb3bb70fbed5cf186b60864bd -- drivers/ > and: > git grep SPDX -- Documentation/ > > it could be: > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > + */ > or: > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > + */ > > Personally I would prefer: > +/* > + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ > > So, is there any guideline? There's been lots of discussion about this, but Linus T has set the requirements. From Thomas' patches adding documentation for this: +1. Placement: + + The SPDX license identifier in kernel files shall be added at the first + possible line in a file which can contain a comment. For the majority + or files this is the first line, except for scripts which require the + '#!PATH_TO_INTERPRETER' in the first line. For those scripts the SPDX + identifier goes into the second line. + +| + +2. Style: + + The SPDX license identifier is added in form of a comment. The comment + style depends on the file type: + + :: + + C source: // SPDX-License-Identifier: <SPDX License Expression> + C header: /* SPDX-License-Identifier: <SPDX License Expression> */ + ASM: /* SPDX-License-Identifier: <SPDX License Expression> */ + scripts: # SPDX-License-Identifier: <SPDX License Expression> + .rst: .. SPDX-License-Identifier: <SPDX License Expression> + + If a specific tool cannot handle the standard comment style, then the + appropriate comment mechanism which the tool accepts shall be used. This + is the reason for having the "/\* \*/" style comment in C header + files. There was build breakage observed with generated .lds files where + 'ld' failed to parse the C++ comment. This has been fixed by now, but + there are still older assembler tools which cannot handle C++ style + comments. There is additional information in Thomas' patch about the SPDX identifiers, how to deal with multi-licensed files, etc. It's well worth reading before throwing a SPDX tag on a file to make sure you get the correct tag(s) and expressions. It's a fixed format. Tabs and additional spaces that are not in the format are bad news. The idea here is (as detailed by Linus) to be able to grep for the SPDX identifiers using standard Unix tools, sort then and then eliminate duplicates. If someone decides to use a tab after the SPDX-Identifier: rather than a space, then it messes up "uniq"'s ability to get rid of the duplicates. As I say, the documentation is there to help people get this right, so it's well worth reading. I don't think it's in mainline yet.
diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi index 589a67c5f796..3d1a55e11ea8 100644 --- a/arch/arm/boot/dts/imx53.dtsi +++ b/arch/arm/boot/dts/imx53.dtsi @@ -434,7 +434,7 @@ }; srtc: srtc@53fa4000 { - compatible = "fsl,imx53-rtc", "fsl,imx25-rtc"; + compatible = "fsl,imx53-rtc"; reg = <0x53fa4000 0x4000>; interrupts = <24>; interrupt-parent = <&tzic>; diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile index f2f50c11dc38..fb3dc458c185 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) += 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..5049b521b38e --- /dev/null +++ b/drivers/rtc/rtc-mxc_v2.c @@ -0,0 +1,531 @@ +/* + * Copyright (C) 2004-2011 Freescale Semiconductor, Inc. All Rights Reserved. + */ + +/* + * The code contained herein is licensed under the GNU General Public + * License. You may obtain a copy of the GNU General Public License + * Version 2 or later at the following locations: + * + * http://www.opensource.org/licenses/gpl-license.html + * http://www.gnu.org/copyleft/gpl.html + */ +/* + * Implementation based on rtc-ds1553.c + */ + +/*! + * @defgroup RTC Real Time Clock (RTC) Driver for i.MX53 + */ +/*! + * @file rtc-mxc_v2.c + * @brief Real Time Clock interface + * + * This file contains Real Time Clock interface for Linux. + * + * @ingroup RTC + */ + +#include <linux/slab.h> +#include <linux/delay.h> +#include <linux/rtc.h> +#include <linux/module.h> +#include <linux/fs.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/clk.h> +#include <linux/device.h> +#include <linux/uaccess.h> +#include <linux/io.h> +//#include <linux/mxc_srtc.h> +#define RTC_READ_TIME_47BIT _IOR('p', 0x20, unsigned long long) +/* blocks until LPSCMR is set, returns difference */ +#define RTC_WAIT_TIME_SET _IOR('p', 0x21, int64_t) + +#define SRTC_LPSCLR_LLPSC_LSH 17 /* start bit for LSB time value */ + +#define SRTC_LPPDR_INIT 0x41736166 /* init for glitch detect */ + +#define SRTC_LPCR_SWR_LP (1 << 0) /* lp software reset */ +#define SRTC_LPCR_EN_LP (1 << 3) /* lp enable */ +#define SRTC_LPCR_WAE (1 << 4) /* lp wakeup alarm enable */ +#define SRTC_LPCR_SAE (1 << 5) /* lp security alarm enable */ +#define SRTC_LPCR_SI (1 << 6) /* lp security interrupt enable */ +#define SRTC_LPCR_ALP (1 << 7) /* lp alarm flag */ +#define SRTC_LPCR_LTC (1 << 8) /* lp lock time counter */ +#define SRTC_LPCR_LMC (1 << 9) /* lp lock monotonic counter */ +#define SRTC_LPCR_SV (1 << 10) /* lp security violation */ +#define SRTC_LPCR_NSA (1 << 11) /* lp non secure access */ +#define SRTC_LPCR_NVEIE (1 << 12) /* lp non valid state exit int en */ +#define SRTC_LPCR_IEIE (1 << 13) /* lp init state exit int enable */ +#define SRTC_LPCR_NVE (1 << 14) /* lp non valid state exit bit */ +#define SRTC_LPCR_IE (1 << 15) /* lp init state exit bit */ + +#define SRTC_LPCR_ALL_INT_EN (SRTC_LPCR_WAE | SRTC_LPCR_SAE | \ + SRTC_LPCR_SI | SRTC_LPCR_ALP | \ + SRTC_LPCR_NVEIE | SRTC_LPCR_IEIE) + +#define SRTC_LPSR_TRI (1 << 0) /* lp time read invalidate */ +#define SRTC_LPSR_PGD (1 << 1) /* lp power supply glitc detected */ +#define SRTC_LPSR_CTD (1 << 2) /* lp clock tampering detected */ +#define SRTC_LPSR_ALP (1 << 3) /* lp alarm flag */ +#define SRTC_LPSR_MR (1 << 4) /* lp monotonic counter rollover */ +#define SRTC_LPSR_TR (1 << 5) /* lp time rollover */ +#define SRTC_LPSR_EAD (1 << 6) /* lp external alarm detected */ +#define SRTC_LPSR_IT0 (1 << 7) /* lp IIM throttle */ +#define SRTC_LPSR_IT1 (1 << 8) +#define SRTC_LPSR_IT2 (1 << 9) +#define SRTC_LPSR_SM0 (1 << 10) /* lp security mode */ +#define SRTC_LPSR_SM1 (1 << 11) +#define SRTC_LPSR_STATE_LP0 (1 << 12) /* lp state */ +#define SRTC_LPSR_STATE_LP1 (1 << 13) +#define SRTC_LPSR_NVES (1 << 14) /* lp non-valid state exit status */ +#define SRTC_LPSR_IES (1 << 15) /* lp init state exit status */ + +#define MAX_PIE_NUM 15 +#define MAX_PIE_FREQ 32768 +#define MIN_PIE_FREQ 1 + +#define SRTC_PI0 (1 << 0) +#define SRTC_PI1 (1 << 1) +#define SRTC_PI2 (1 << 2) +#define SRTC_PI3 (1 << 3) +#define SRTC_PI4 (1 << 4) +#define SRTC_PI5 (1 << 5) +#define SRTC_PI6 (1 << 6) +#define SRTC_PI7 (1 << 7) +#define SRTC_PI8 (1 << 8) +#define SRTC_PI9 (1 << 9) +#define SRTC_PI10 (1 << 10) +#define SRTC_PI11 (1 << 11) +#define SRTC_PI12 (1 << 12) +#define SRTC_PI13 (1 << 13) +#define SRTC_PI14 (1 << 14) +#define SRTC_PI15 (1 << 15) + +#define PIT_ALL_ON (SRTC_PI1 | SRTC_PI2 | SRTC_PI3 | \ + SRTC_PI4 | SRTC_PI5 | SRTC_PI6 | SRTC_PI7 | \ + SRTC_PI8 | SRTC_PI9 | SRTC_PI10 | SRTC_PI11 | \ + SRTC_PI12 | SRTC_PI13 | SRTC_PI14 | SRTC_PI15) + +#define SRTC_SWR_HP (1 << 0) /* hp software reset */ +#define SRTC_EN_HP (1 << 3) /* hp enable */ +#define SRTC_TS (1 << 4) /* time synchronize hp with lp */ + +#define SRTC_IE_AHP (1 << 16) /* Alarm HP Interrupt Enable bit */ +#define SRTC_IE_WDHP (1 << 18) /* Write Done HP Interrupt Enable bit */ +#define SRTC_IE_WDLP (1 << 19) /* Write Done LP Interrupt Enable bit */ + +#define SRTC_ISR_AHP (1 << 16) /* interrupt status: alarm hp */ +#define SRTC_ISR_WDHP (1 << 18) /* interrupt status: write done hp */ +#define SRTC_ISR_WDLP (1 << 19) /* interrupt status: write done lp */ +#define SRTC_ISR_WPHP (1 << 20) /* interrupt status: write pending hp */ +#define SRTC_ISR_WPLP (1 << 21) /* interrupt status: write pending lp */ + +#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_LPSMCR 0x0C /* LP Secure Monotonic Counter 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 */ +#define SRTC_LPGR 0x1C /* LP General Purpose Reg */ +#define SRTC_HPCMR 0x20 /* HP Counter MSB Reg */ +#define SRTC_HPCLR 0x24 /* HP Counter LSB Reg */ +#define SRTC_HPAMR 0x28 /* HP Alarm MSB Reg */ +#define SRTC_HPALR 0x2C /* HP Alarm LSB Reg */ +#define SRTC_HPCR 0x30 /* HP Control Reg */ +#define SRTC_HPISR 0x34 /* HP Interrupt Status Reg */ +#define SRTC_HPIENR 0x38 /* HP Interrupt Enable Reg */ + +#define SRTC_SECMODE_MASK 0x3 /* the mask of SRTC security mode */ +#define SRTC_SECMODE_LOW 0x0 /* Low Security */ +#define SRTC_SECMODE_MED 0x1 /* Medium Security */ +#define SRTC_SECMODE_HIGH 0x2 /* High Security */ +#define SRTC_SECMODE_RESERVED 0x3 /* Reserved */ + +struct rtc_drv_data { + struct rtc_device *rtc; + void __iomem *ioaddr; + int irq; + struct clk *clk; +}; + +static DEFINE_SPINLOCK(rtc_lock); + +/*! + * 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. + */ +static inline void rtc_write_sync_lp(void __iomem *ioaddr) +{ + unsigned int i, count; + /* Wait for 3 CKIL cycles */ + for (i = 0; i < 3; i++) { + count = readl(ioaddr + SRTC_LPSCLR); + while ((readl(ioaddr + SRTC_LPSCLR)) == count) + ; + } +} + +/*! + * This function updates the RTC alarm registers and then clears all the + * interrupt status bits. + * + * @param alrm the new alarm value to be updated in the RTC + * + * @return 0 if successful; non-zero otherwise. + */ +static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm) +{ + struct rtc_drv_data *pdata = dev_get_drvdata(dev); + void __iomem *ioaddr = pdata->ioaddr; + struct rtc_time alarm_tm, now_tm; + unsigned long now, time; + int ret; + + now = __raw_readl(ioaddr + SRTC_LPSCMR); + rtc_time_to_tm(now, &now_tm); + + alarm_tm.tm_year = now_tm.tm_year; + alarm_tm.tm_mon = now_tm.tm_mon; + alarm_tm.tm_mday = now_tm.tm_mday; + + alarm_tm.tm_hour = alrm->tm_hour; + alarm_tm.tm_min = alrm->tm_min; + alarm_tm.tm_sec = alrm->tm_sec; + + rtc_tm_to_time(&now_tm, &now); + rtc_tm_to_time(&alarm_tm, &time); + + if (time < now) { + time += 60 * 60 * 24; + rtc_time_to_tm(time, &alarm_tm); + } + ret = rtc_tm_to_time(&alarm_tm, &time); + + __raw_writel(time, ioaddr + SRTC_LPSAR); + + /* clear alarm interrupt status bit */ + __raw_writel(SRTC_LPSR_ALP, ioaddr + SRTC_LPSR); + + return ret; +} + +/*! + * This function is the RTC interrupt service routine. + * + * @param irq RTC IRQ number + * @param dev_id device ID which is not used + * + * @return IRQ_HANDLED as defined in the include/linux/interrupt.h file. + */ +static irqreturn_t mxc_rtc_interrupt(int irq, void *dev_id) +{ + struct platform_device *pdev = dev_id; + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); + void __iomem *ioaddr = pdata->ioaddr; + u32 lp_status, lp_cr; + u32 events = 0; + + lp_status = __raw_readl(ioaddr + SRTC_LPSR); + lp_cr = __raw_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 */ + __raw_writel(lp_cr, ioaddr + SRTC_LPCR); + + /* clear interrupt status */ + __raw_writel(lp_status, ioaddr + SRTC_LPSR); + + rtc_write_sync_lp(ioaddr); + rtc_update_irq(pdata->rtc, 1, events); + return IRQ_HANDLED; +} + +/*! + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); + time_t now = readl(pdata->ioaddr + SRTC_LPSCMR); + + rtc_time_to_tm(now, tm); + return rtc_valid_tm(tm); +} + +/*! + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); + time64_t time = rtc_tm_to_time64(tm); + + if (time > UINT_MAX) { + dev_warn(dev, "time_t has overflow\n"); + } + + writel(time, pdata->ioaddr + SRTC_LPSCMR); + rtc_write_sync_lp(pdata->ioaddr); + return 0; +} + +/*! + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); + void __iomem *ioaddr = pdata->ioaddr; + + rtc_time_to_tm(__raw_readl(ioaddr + SRTC_LPSAR), &alrm->time); + alrm->pending = + ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_ALP) != 0) ? 1 : 0; + + return 0; +} + +static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enable) +{ + struct rtc_drv_data *pdata = dev_get_drvdata(dev); + unsigned long lock_flags = 0; + u32 lp_cr; + + spin_lock_irqsave(&rtc_lock, lock_flags); + lp_cr = __raw_readl(pdata->ioaddr + SRTC_LPCR); + + if (enable) + lp_cr |= (SRTC_LPCR_ALP | SRTC_LPCR_WAE); + else + lp_cr &= ~(SRTC_LPCR_ALP | SRTC_LPCR_WAE); + + __raw_writel(lp_cr, pdata->ioaddr + SRTC_LPCR); + spin_unlock_irqrestore(&rtc_lock, lock_flags); + return 0; +} + +/*! + * 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 rtc_drv_data *pdata = dev_get_drvdata(dev); + void __iomem *ioaddr = pdata->ioaddr; + int ret; + + ret = rtc_update_alarm(dev, &alrm->time); + if (!ret) + mxc_rtc_alarm_irq_enable(dev, alrm->enabled); + + rtc_write_sync_lp(ioaddr); + return ret; +} + +/*! + * The RTC driver structure + */ +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, +}; + +/*! MXC RTC Power management control */ +static int mxc_rtc_probe(struct platform_device *pdev) +{ + struct timespec tv; + struct resource *res; + struct rtc_drv_data *pdata = NULL; + 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); + } + ret = clk_prepare_enable(pdata->clk); + if (ret) + return ret; + + /* Configure and enable the RTC */ + pdata->irq = platform_get_irq(pdev, 0); + if (pdata->irq >= 0 + && devm_request_irq(&pdev->dev, pdata->irq, mxc_rtc_interrupt, + IRQF_SHARED, pdev->name, pdev) < 0) { + dev_warn(&pdev->dev, "interrupt not available.\n"); + pdata->irq = -1; + } + + if (pdata->irq >= 0) + device_init_wakeup(&pdev->dev, 1); + + /* initialize glitch detect */ + __raw_writel(SRTC_LPPDR_INIT, ioaddr + SRTC_LPPDR); + udelay(100); + + /* clear lp interrupt status */ + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); + udelay(100); + + /* move out of init state */ + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NSA), ioaddr + SRTC_LPCR); + + udelay(100); + + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_IES) == 0) + ; + + /* move out of non-valid state */ + __raw_writel((SRTC_LPCR_IE | SRTC_LPCR_NVE | SRTC_LPCR_NSA | + SRTC_LPCR_EN_LP), ioaddr + SRTC_LPCR); + + udelay(100); + + while ((__raw_readl(ioaddr + SRTC_LPSR) & SRTC_LPSR_NVES) == 0) + ; + + __raw_writel(0xFFFFFFFF, ioaddr + SRTC_LPSR); + udelay(100); + + platform_set_drvdata(pdev, pdata); + pdata->rtc = + devm_rtc_device_register(&pdev->dev, pdev->name, &mxc_rtc_ops, + THIS_MODULE); + if (IS_ERR(pdata->rtc)) { + ret = PTR_ERR(pdata->rtc); + goto exit_put_clk; + } + + tv.tv_nsec = 0; + tv.tv_sec = __raw_readl(ioaddr + SRTC_LPSCMR); + + /* By default, devices should wakeup if they can */ + /* So srtc is set as "should wakeup" as it can */ + device_init_wakeup(&pdev->dev, 1); + + return ret; + +exit_put_clk: + clk_disable_unprepare(pdata->clk); + return ret; +} + +static int __exit mxc_rtc_remove(struct platform_device *pdev) +{ + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); + + clk_disable_unprepare(pdata->clk); + return 0; +} + +/*! + * This function is called to save the system time delta relative to + * the MXC RTC when enterring a low power state. This time delta is + * then used on resume to adjust the system time to account for time + * loss while suspended. + * + * @param pdev not used + * @param state Power state to enter. + * + * @return The function always returns 0. + */ +static int mxc_rtc_suspend(struct platform_device *pdev, pm_message_t state) +{ + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); + + if (device_may_wakeup(&pdev->dev)) + enable_irq_wake(pdata->irq); + + return 0; +} + +/*! + * This function is called to correct the system time based on the + * current MXC RTC time relative to the time delta saved during + * suspend. + * + * @param pdev not used + * + * @return The function always returns 0. + */ +static int mxc_rtc_resume(struct platform_device *pdev) +{ + struct rtc_drv_data *pdata = platform_get_drvdata(pdev); + + if (device_may_wakeup(&pdev->dev)) + disable_irq_wake(pdata->irq); + + return 0; +} + +static const struct of_device_id mxc_ids[] = { + {.compatible = "fsl,imx53-rtc",}, + {} +}; + +/*! + * Contains pointers to the power management callback functions. + */ +static struct platform_driver mxc_rtc_driver = { + .driver = { + .name = "mxc_rtc_v8", + .of_match_table = mxc_ids, + }, + .probe = mxc_rtc_probe, + .remove = mxc_rtc_remove, + .suspend = mxc_rtc_suspend, + .resume = mxc_rtc_resume, +}; + +module_platform_driver(mxc_rtc_driver); + +MODULE_AUTHOR("Freescale Semiconductor, Inc."); +MODULE_DESCRIPTION("Realtime Clock Driver (RTC)"); +MODULE_LICENSE("GPL");