| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5d33f0b7-ab70-6992-fd30-b13d6f06e931@st.com>
Date: Wed, 7 Dec 2016 11:16:27 +0100
From: Amelie DELAUNAY <amelie.delaunay@...com>
To: Mathieu Poirier <mathieu.poirier@...aro.org>,
"a.zummo@...ertech.it" <a.zummo@...ertech.it>,
"alexandre.belloni@...e-electrons.com"
<alexandre.belloni@...e-electrons.com>
CC: "robh+dt@...nel.org" <robh+dt@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"mcoquelin.stm32@...il.com" <mcoquelin.stm32@...il.com>,
Alexandre TORGUE <alexandre.torgue@...com>,
"linux@...linux.org.uk" <linux@...linux.org.uk>,
"rtc-linux@...glegroups.com" <rtc-linux@...glegroups.com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Gabriel FERNANDEZ <gabriel.fernandez@...com>
Subject: Re: [PATCH 3/8] rtc: add STM32 RTC driver
On 12/05/2016 05:32 PM, Mathieu Poirier wrote:
> On Mon, Dec 05, 2016 at 10:43:14AM +0100, Amelie DELAUNAY wrote:
>> Hi Mathieu,
>>
>> Thanks for reviewing
>>
>> On 12/02/2016 06:56 PM, Mathieu Poirier wrote:
>>> On Fri, Dec 02, 2016 at 03:09:56PM +0100, Amelie Delaunay wrote:
>>>> This patch adds support for the STM32 RTC.
>>>
>>> Hello Amelie,
>>>
>>>>
>>>> Signed-off-by: Amelie Delaunay <amelie.delaunay@...com>
>>>> ---
>>>> drivers/rtc/Kconfig | 10 +
>>>> drivers/rtc/Makefile | 1 +
>>>> drivers/rtc/rtc-stm32.c | 777
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 788 insertions(+)
>>>> create mode 100644 drivers/rtc/rtc-stm32.c
>>>>
>>>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>>>> index e859d14..dd8b218 100644
>>>> --- a/drivers/rtc/Kconfig
>>>> +++ b/drivers/rtc/Kconfig
>>>> @@ -1706,6 +1706,16 @@ config RTC_DRV_PIC32
>>>> This driver can also be built as a module. If so, the module
>>>> will be called rtc-pic32
>>>>
>>>> +config RTC_DRV_STM32
>>>> + tristate "STM32 On-Chip RTC"
>>>> + depends on ARCH_STM32
>>>> + help
>>>> + If you say yes here you get support for the STM32 On-Chip
>>>> + Real Time Clock.
>>>> +
>>>> + This driver can also be built as a module, if so, the module
>>>> + will be called "rtc-stm32".
>>>> +
>>>> comment "HID Sensor RTC drivers"
>>>>
>>>> config RTC_DRV_HID_SENSOR_TIME
>>>> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
>>>> index 1ac694a..87bd9cc 100644
>>>> --- a/drivers/rtc/Makefile
>>>> +++ b/drivers/rtc/Makefile
>>>> @@ -144,6 +144,7 @@ obj-$(CONFIG_RTC_DRV_SNVS) += rtc-snvs.o
>>>> obj-$(CONFIG_RTC_DRV_SPEAR) += rtc-spear.o
>>>> obj-$(CONFIG_RTC_DRV_STARFIRE) += rtc-starfire.o
>>>> obj-$(CONFIG_RTC_DRV_STK17TA8) += rtc-stk17ta8.o
>>>> +obj-$(CONFIG_RTC_DRV_STM32) += rtc-stm32.o
>>>> obj-$(CONFIG_RTC_DRV_STMP) += rtc-stmp3xxx.o
>>>> obj-$(CONFIG_RTC_DRV_ST_LPC) += rtc-st-lpc.o
>>>> obj-$(CONFIG_RTC_DRV_SUN4V) += rtc-sun4v.o
>>>> diff --git a/drivers/rtc/rtc-stm32.c b/drivers/rtc/rtc-stm32.c
>>>> new file mode 100644
>>>> index 0000000..9e710ff
>>>> --- /dev/null
>>>> +++ b/drivers/rtc/rtc-stm32.c
>>>> @@ -0,0 +1,777 @@
>>>> +/*
>>>> + * Copyright (C) Amelie Delaunay 2015
>>>> + * Author: Amelie Delaunay <adelaunay.stm32@...il.com>
>>>> + * License terms: GNU General Public License (GPL), version 2
>>>> + */
>>>> +
>>>> +#include <linux/bcd.h>
>>>> +#include <linux/clk.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/ioport.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/mfd/syscon.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/rtc.h>
>>>> +#include <linux/spinlock.h>
>>>> +
>>>> +#define DRIVER_NAME "stm32_rtc"
>>>> +
>>>> +/* STM32 RTC registers */
>>>> +#define STM32_RTC_TR 0x00
>>>> +#define STM32_RTC_DR 0x04
>>>> +#define STM32_RTC_CR 0x08
>>>> +#define STM32_RTC_ISR 0x0C
>>>> +#define STM32_RTC_PRER 0x10
>>>> +#define STM32_RTC_ALRMAR 0x1C
>>>> +#define STM32_RTC_WPR 0x24
>>>> +
>>>> +/* STM32_RTC_TR bit fields */
>>>> +#define STM32_RTC_TR_SEC_SHIFT 0
>>>> +#define STM32_RTC_TR_SEC GENMASK(6, 0)
>>>> +#define STM32_RTC_TR_MIN_SHIFT 8
>>>> +#define STM32_RTC_TR_MIN GENMASK(14, 8)
>>>> +#define STM32_RTC_TR_HOUR_SHIFT 16
>>>> +#define STM32_RTC_TR_HOUR GENMASK(21, 16)
>>>> +
>>>> +/* STM32_RTC_DR bit fields */
>>>> +#define STM32_RTC_DR_DATE_SHIFT 0
>>>> +#define STM32_RTC_DR_DATE GENMASK(5, 0)
>>>> +#define STM32_RTC_DR_MONTH_SHIFT 8
>>>> +#define STM32_RTC_DR_MONTH GENMASK(11, 8)
>>>> +#define STM32_RTC_DR_WDAY_SHIFT 13
>>>> +#define STM32_RTC_DR_WDAY GENMASK(15, 13)
>>>> +#define STM32_RTC_DR_YEAR_SHIFT 16
>>>> +#define STM32_RTC_DR_YEAR GENMASK(23, 16)
>>>> +
>>>> +/* STM32_RTC_CR bit fields */
>>>> +#define STM32_RTC_CR_FMT BIT(6)
>>>> +#define STM32_RTC_CR_ALRAE BIT(8)
>>>> +#define STM32_RTC_CR_ALRAIE BIT(12)
>>>> +
>>>> +/* STM32_RTC_ISR bit fields */
>>>> +#define STM32_RTC_ISR_ALRAWF BIT(0)
>>>> +#define STM32_RTC_ISR_INITS BIT(4)
>>>> +#define STM32_RTC_ISR_RSF BIT(5)
>>>> +#define STM32_RTC_ISR_INITF BIT(6)
>>>> +#define STM32_RTC_ISR_INIT BIT(7)
>>>> +#define STM32_RTC_ISR_ALRAF BIT(8)
>>>> +
>>>> +/* STM32_RTC_PRER bit fields */
>>>> +#define STM32_RTC_PRER_PRED_S_SHIFT 0
>>>> +#define STM32_RTC_PRER_PRED_S GENMASK(14, 0)
>>>> +#define STM32_RTC_PRER_PRED_A_SHIFT 16
>>>> +#define STM32_RTC_PRER_PRED_A GENMASK(22, 16)
>>>> +
>>>> +/* STM32_RTC_ALRMAR and STM32_RTC_ALRMBR bit fields */
>>>> +#define STM32_RTC_ALRMXR_SEC_SHIFT 0
>>>> +#define STM32_RTC_ALRMXR_SEC GENMASK(6, 0)
>>>> +#define STM32_RTC_ALRMXR_SEC_MASK BIT(7)
>>>> +#define STM32_RTC_ALRMXR_MIN_SHIFT 8
>>>> +#define STM32_RTC_ALRMXR_MIN GENMASK(14, 8)
>>>> +#define STM32_RTC_ALRMXR_MIN_MASK BIT(15)
>>>> +#define STM32_RTC_ALRMXR_HOUR_SHIFT 16
>>>> +#define STM32_RTC_ALRMXR_HOUR GENMASK(21, 16)
>>>> +#define STM32_RTC_ALRMXR_PM BIT(22)
>>>> +#define STM32_RTC_ALRMXR_HOUR_MASK BIT(23)
>>>> +#define STM32_RTC_ALRMXR_DATE_SHIFT 24
>>>> +#define STM32_RTC_ALRMXR_DATE GENMASK(29, 24)
>>>> +#define STM32_RTC_ALRMXR_WDSEL BIT(30)
>>>> +#define STM32_RTC_ALRMXR_WDAY_SHIFT 24
>>>> +#define STM32_RTC_ALRMXR_WDAY GENMASK(27, 24)
>>>> +#define STM32_RTC_ALRMXR_DATE_MASK BIT(31)
>>>> +
>>>> +/* STM32_RTC_WPR key constants */
>>>> +#define RTC_WPR_1ST_KEY 0xCA
>>>> +#define RTC_WPR_2ND_KEY 0x53
>>>> +#define RTC_WPR_WRONG_KEY 0xFF
>>>> +
>>>> +/*
>>>> + * RTC registers are protected agains parasitic write access.
>>>> + * PWR_CR_DBP bit must be set to enable write access to RTC registers.
>>>> + */
>>>> +/* STM32_PWR_CR */
>>>> +#define PWR_CR 0x00
>>>> +/* STM32_PWR_CR bit field */
>>>> +#define PWR_CR_DBP BIT(8)
>>>> +
>>>> +static struct regmap *dbp;
>>>> +
>>>> +struct stm32_rtc {
>>>> + struct rtc_device *rtc_dev;
>>>> + void __iomem *base;
>>>> + struct clk *pclk;
>>>> + struct clk *ck_rtc;
>>>> + unsigned int clksrc;
>>>> + spinlock_t lock; /* Protects registers accesses */
>>>> + int irq_alarm;
>>>> + struct regmap *pwrcr;
>>>> +};
>>>> +
>>>> +static inline unsigned int stm32_rtc_readl(struct stm32_rtc *rtc,
>>>> + unsigned int offset)
>>>> +{
>>>> + return readl_relaxed(rtc->base + offset);
>>>> +}
>>>> +
>>>> +static inline void stm32_rtc_writel(struct stm32_rtc *rtc,
>>>> + unsigned int offset, unsigned int value)
>>>> +{
>>>> + writel_relaxed(value, rtc->base + offset);
>>>> +}
>>>
>>> I'm not sure wrapping the readl/writel_relaxed function does anything
>> special
>>> other than simply redirecting the reader to another section of the code.
>> During development phase, it is useful to add debug traces but you're right,
>> this can be remove.
>>>
>>>> +
>>>> +static void stm32_rtc_wpr_unlock(struct stm32_rtc *rtc)
>>>> +{
>>>> +// if (dbp)
>>>> +// regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, PWR_CR_DBP);
>>>
>>> Did checkpatch let you get away with this? What did you intend to do
>> here?
>> Hum, as surprising as it may seem, checkpatch didn't complained about these
>> comments! But anyway, this has to be removed, it was a tentative to
>> enable/disable backup domain write protection any time we have to write in a
>> protected RTC register, but it is not functionnal. I have commented this
>> just to keep it in mind and forget to remove it before sending.
>>>
>>>> +
>>>> + stm32_rtc_writel(rtc, STM32_RTC_WPR, RTC_WPR_1ST_KEY);
>>>> + stm32_rtc_writel(rtc, STM32_RTC_WPR, RTC_WPR_2ND_KEY);
>>>> +}
>>>> +
>>>> +static void stm32_rtc_wpr_lock(struct stm32_rtc *rtc)
>>>> +{
>>>> + stm32_rtc_writel(rtc, STM32_RTC_WPR, RTC_WPR_WRONG_KEY);
>>>> +
>>>> +// if (dbp)
>>>> +// regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, ~PWR_CR_DBP);
>>>> +}
>>>> +
>>>> +static int stm32_rtc_enter_init_mode(struct stm32_rtc *rtc)
>>>> +{
>>>> + unsigned int isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
>>>> +
>>>> + if (!(isr & STM32_RTC_ISR_INITF)) {
>>>> + isr |= STM32_RTC_ISR_INIT;
>>>> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
>>>> +
>>>> + return readl_relaxed_poll_timeout_atomic(
>>>> + rtc->base + STM32_RTC_ISR,
>>>> + isr, (isr & STM32_RTC_ISR_INITF),
>>>> + 10, 100000);
>>>
>>> When using hard coded numerics please add comments that explains the
>> reason
>>> behind the selected values.
>> Sure. It takes around 2 RTCCLK clock cycles to enter in initialization phase
>> mode. So it depends on the frequency of the ck_rtc parent clock.
>> Either I keep parent clock frequency and compute the exact timeout, or I use
>> the "best and worst cases": slowest RTCCLK frequency is 32kHz, so it can
>> take up to 62us, highest RTCCLK frequency should be 1MHz, so it can take
>> only 2us. Polling every 10us with a timeout of 100ms seemed reasonable and
>> be a good compromise.
>
> I think this is a resonnable approach - please add that explanation as a comment
> in the code.
Ok I'll do that.
>
>>>
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static void stm32_rtc_exit_init_mode(struct stm32_rtc *rtc)
>>>> +{
>>>> + unsigned int isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
>>>> +
>>>> + isr &= ~STM32_RTC_ISR_INIT;
>>>> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
>>>> +}
>>>> +
>>>> +static int stm32_rtc_wait_sync(struct stm32_rtc *rtc)
>>>> +{
>>>> + unsigned int isr;
>>>> +
>>>> + isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
>>>> +
>>>> + isr &= ~STM32_RTC_ISR_RSF;
>>>> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
>>>> +
>>>> + /* Wait the registers to be synchronised */
>>>> + return readl_relaxed_poll_timeout_atomic(rtc->base + STM32_RTC_ISR,
>>>> + isr,
>>>> + (isr & STM32_RTC_ISR_RSF),
>>>> + 10, 100000);
>>>
>>> Shouldn't the break condition be !((isr & STM32_RTC_ISR_RSF) ? If not
>> this
>>> probably deserve a better comment.
>> RSF bit is set by hardware each time the calendar registers are synchronized
>> (it takes up to 2 RTCCLK). So the break condition is correct: we poll until
>> RSF flag is set or timeout is reached.
>>>
>>>> +}
>>>> +
>>>> +static irqreturn_t stm32_rtc_alarm_irq(int irq, void *dev_id)
>>>> +{
>>>> + struct stm32_rtc *rtc = (struct stm32_rtc *)dev_id;
>>>> + unsigned long irqflags, events = 0;
>>>> + unsigned int isr, cr;
>>>> +
>>>> + spin_lock_irqsave(&rtc->lock, irqflags);
>>>> +
>>>> + isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
>>>> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
>>>> +
>>>> + if ((isr & STM32_RTC_ISR_ALRAF) &&
>>>> + (cr & STM32_RTC_CR_ALRAIE)) {
>>>> + /* Alarm A flag - Alarm interrupt */
>>>> + events |= RTC_IRQF | RTC_AF;
>>>> + isr &= ~STM32_RTC_ISR_ALRAF;
>>>> + }
>>>> +
>>>> + /* Clear event irqflags, otherwise new events won't be received */
>>>> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
>>>> +
>>>> + spin_unlock_irqrestore(&rtc->lock, irqflags);
>>>> +
>>>> + if (events) {
>>>> + dev_info(&rtc->rtc_dev->dev, "Alarm occurred\n");
>>>> +
>>>> + /* Pass event to the kernel */
>>>> + rtc_update_irq(rtc->rtc_dev, 1, events);
>>>> + return IRQ_HANDLED;
>>>> + } else {
>>>> + return IRQ_NONE;
>>>> + }
>>>> +}
>>>> +
>>>> +/* Convert rtc_time structure from bin to bcd format */
>>>> +static void tm2bcd(struct rtc_time *tm)
>>>> +{
>>>> + tm->tm_sec = bin2bcd(tm->tm_sec);
>>>> + tm->tm_min = bin2bcd(tm->tm_min);
>>>> + tm->tm_hour = bin2bcd(tm->tm_hour);
>>>> +
>>>> + tm->tm_mday = bin2bcd(tm->tm_mday);
>>>> + tm->tm_mon = bin2bcd(tm->tm_mon + 1);
>>>> + tm->tm_year = bin2bcd(tm->tm_year - 100);
>>>> + /*
>>>> + * Number of days since Sunday
>>>> + * - on kernel side, 0=Sunday...6=Saturday
>>>> + * - on rtc side, 0=invalid,1=Monday...7=Sunday
>>>> + */
>>>> + tm->tm_wday = (!tm->tm_wday) ? 7 : tm->tm_wday;
>>>> +}
>>>> +
>>>> +/* Convert rtc_time structure from bcd to bin format */
>>>> +static void bcd2tm(struct rtc_time *tm)
>>>> +{
>>>> + tm->tm_sec = bcd2bin(tm->tm_sec);
>>>> + tm->tm_min = bcd2bin(tm->tm_min);
>>>> + tm->tm_hour = bcd2bin(tm->tm_hour);
>>>> +
>>>> + tm->tm_mday = bcd2bin(tm->tm_mday);
>>>> + tm->tm_mon = bcd2bin(tm->tm_mon) - 1;
>>>> + tm->tm_year = bcd2bin(tm->tm_year) + 100;
>>>> + /*
>>>> + * Number of days since Sunday
>>>> + * - on kernel side, 0=Sunday...6=Saturday
>>>> + * - on rtc side, 0=invalid,1=Monday...7=Sunday
>>>> + */
>>>> + tm->tm_wday %= 7;
>>>> +}
>>>> +
>>>> +static int stm32_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>>> +{
>>>> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
>>>> + unsigned int tr, dr;
>>>> + unsigned long irqflags;
>>>> +
>>>> + spin_lock_irqsave(&rtc->lock, irqflags);
>>>> +
>>>> + /* Time and Date in BCD format */
>>>> + tr = stm32_rtc_readl(rtc, STM32_RTC_TR);
>>>> + dr = stm32_rtc_readl(rtc, STM32_RTC_DR);
>>>> +
>>>> + spin_unlock_irqrestore(&rtc->lock, irqflags);
>>>> +
>>>> + tm->tm_sec = (tr & STM32_RTC_TR_SEC) >> STM32_RTC_TR_SEC_SHIFT;
>>>> + tm->tm_min = (tr & STM32_RTC_TR_MIN) >> STM32_RTC_TR_MIN_SHIFT;
>>>> + tm->tm_hour = (tr & STM32_RTC_TR_HOUR) >> STM32_RTC_TR_HOUR_SHIFT;
>>>> +
>>>> + tm->tm_mday = (dr & STM32_RTC_DR_DATE) >> STM32_RTC_DR_DATE_SHIFT;
>>>> + tm->tm_mon = (dr & STM32_RTC_DR_MONTH) >> STM32_RTC_DR_MONTH_SHIFT;
>>>> + tm->tm_year = (dr & STM32_RTC_DR_YEAR) >> STM32_RTC_DR_YEAR_SHIFT;
>>>> + tm->tm_wday = (dr & STM32_RTC_DR_WDAY) >> STM32_RTC_DR_WDAY_SHIFT;
>>>> +
>>>> + /* We don't report tm_yday and tm_isdst */
>>>> +
>>>> + bcd2tm(tm);
>>>> +
>>>> + if (rtc_valid_tm(tm) < 0) {
>>>> + dev_err(dev, "%s: rtc_time is not valid.\n", __func__);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int stm32_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>>> +{
>>>> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
>>>> + unsigned int tr, dr;
>>>> + unsigned long irqflags;
>>>> + int ret = 0;
>>>> +
>>>> + if (rtc_valid_tm(tm) < 0) {
>>>> + dev_err(dev, "%s: rtc_time is not valid.\n", __func__);
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + tm2bcd(tm);
>>>> +
>>>> + /* Time in BCD format */
>>>> + tr = ((tm->tm_sec << STM32_RTC_TR_SEC_SHIFT) & STM32_RTC_TR_SEC) |
>>>> + ((tm->tm_min << STM32_RTC_TR_MIN_SHIFT) & STM32_RTC_TR_MIN) |
>>>> + ((tm->tm_hour << STM32_RTC_TR_HOUR_SHIFT) & STM32_RTC_TR_HOUR);
>>>> +
>>>> + /* Date in BCD format */
>>>> + dr = ((tm->tm_mday << STM32_RTC_DR_DATE_SHIFT) & STM32_RTC_DR_DATE)
>> |
>>>> + ((tm->tm_mon << STM32_RTC_DR_MONTH_SHIFT) & STM32_RTC_DR_MONTH)
>> |
>>>> + ((tm->tm_year << STM32_RTC_DR_YEAR_SHIFT) & STM32_RTC_DR_YEAR)
>> |
>>>> + ((tm->tm_wday << STM32_RTC_DR_WDAY_SHIFT) & STM32_RTC_DR_WDAY);
>>>> +
>>>> + spin_lock_irqsave(&rtc->lock, irqflags);
>>>> +
>>>> + stm32_rtc_wpr_unlock(rtc);
>>>> +
>>>> + ret = stm32_rtc_enter_init_mode(rtc);
>>>> + if (ret) {
>>>> + dev_err(dev, "Can't enter in init mode. Set time aborted.\n");
>>>> + goto end;
>>>> + }
>>>> +
>>>> + stm32_rtc_writel(rtc, STM32_RTC_TR, tr);
>>>> + stm32_rtc_writel(rtc, STM32_RTC_DR, dr);
>>>> +
>>>> + stm32_rtc_exit_init_mode(rtc);
>>>> +
>>>> + ret = stm32_rtc_wait_sync(rtc);
>>>> +end:
>>>> + stm32_rtc_wpr_lock(rtc);
>>>> +
>>>> + spin_unlock_irqrestore(&rtc->lock, irqflags);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int stm32_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
>> *alrm)
>>>> +{
>>>> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
>>>> + struct rtc_time *tm = &alrm->time;
>>>> + unsigned int alrmar, cr, isr;
>>>> + unsigned long irqflags;
>>>> +
>>>> + spin_lock_irqsave(&rtc->lock, irqflags);
>>>> +
>>>> + alrmar = stm32_rtc_readl(rtc, STM32_RTC_ALRMAR);
>>>> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
>>>> + isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
>>>> +
>>>> + spin_unlock_irqrestore(&rtc->lock, irqflags);
>>>> +
>>>> + if (alrmar & STM32_RTC_ALRMXR_DATE_MASK) {
>>>> + /*
>>>> + * Date/day don't care in Alarm comparison so alarm triggers
>>>> + * every day
>>>> + */
>>>> + tm->tm_mday = -1;
>>>> + tm->tm_wday = -1;
>>>> + } else {
>>>> + if (alrmar & STM32_RTC_ALRMXR_WDSEL) {
>>>> + /* Alarm is set to a day of week */
>>>> + tm->tm_mday = -1;
>>>> + tm->tm_wday = (alrmar & STM32_RTC_ALRMXR_WDAY) >>
>>>> + STM32_RTC_ALRMXR_WDAY_SHIFT;
>>>> + tm->tm_wday %= 7;
>>>> + } else {
>>>> + /* Alarm is set to a day of month */
>>>> + tm->tm_wday = -1;
>>>> + tm->tm_mday = (alrmar & STM32_RTC_ALRMXR_DATE) >>
>>>> + STM32_RTC_ALRMXR_DATE_SHIFT;
>>>> + }
>>>> + }
>>>> +
>>>> + if (alrmar & STM32_RTC_ALRMXR_HOUR_MASK) {
>>>> + /* Hours don't care in Alarm comparison */
>>>> + tm->tm_hour = -1;
>>>> + } else {
>>>> + tm->tm_hour = (alrmar & STM32_RTC_ALRMXR_HOUR) >>
>>>> + STM32_RTC_ALRMXR_HOUR_SHIFT;
>>>> + if (alrmar & STM32_RTC_ALRMXR_PM)
>>>> + tm->tm_hour += 12;
>>>> + }
>>>> +
>>>> + if (alrmar & STM32_RTC_ALRMXR_MIN_MASK) {
>>>> + /* Minutes don't care in Alarm comparison */
>>>> + tm->tm_min = -1;
>>>> + } else {
>>>> + tm->tm_min = (alrmar & STM32_RTC_ALRMXR_MIN) >>
>>>> + STM32_RTC_ALRMXR_MIN_SHIFT;
>>>> + }
>>>> +
>>>> + if (alrmar & STM32_RTC_ALRMXR_SEC_MASK) {
>>>> + /* Seconds don't care in Alarm comparison */
>>>> + tm->tm_sec = -1;
>>>> + } else {
>>>> + tm->tm_sec = (alrmar & STM32_RTC_ALRMXR_SEC) >>
>>>> + STM32_RTC_ALRMXR_SEC_SHIFT;
>>>> + }
>>>> +
>>>> + bcd2tm(tm);
>>>> +
>>>> + alrm->enabled = (cr & STM32_RTC_CR_ALRAE) ? 1 : 0;
>>>> + alrm->pending = (isr & STM32_RTC_ISR_ALRAF) ? 1 : 0;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int stm32_rtc_alarm_irq_enable(struct device *dev, unsigned int
>> enabled)
>>>> +{
>>>> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
>>>> + unsigned long irqflags;
>>>> + unsigned int isr, cr;
>>>> +
>>>> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
>>>
>>> Is the STM32_RTC_CR garanteed to be valid, i.e updated atomically? If not
>> this
>>> should probably be below the spinlock.
>> You're right.
>>>
>>>> +
>>>> + spin_lock_irqsave(&rtc->lock, irqflags);
>>>> +
>>>> + stm32_rtc_wpr_unlock(rtc);
>>>> +
>>>> + /* We expose Alarm A to the kernel */
>>>> + if (enabled)
>>>> + cr |= (STM32_RTC_CR_ALRAIE | STM32_RTC_CR_ALRAE);
>>>> + else
>>>> + cr &= ~(STM32_RTC_CR_ALRAIE | STM32_RTC_CR_ALRAE);
>>>> + stm32_rtc_writel(rtc, STM32_RTC_CR, cr);
>>>> +
>>>> + /* Clear event irqflags, otherwise new events won't be received */
>>>> + isr = stm32_rtc_readl(rtc, STM32_RTC_ISR);
>>>> + isr &= ~STM32_RTC_ISR_ALRAF;
>>>> + stm32_rtc_writel(rtc, STM32_RTC_ISR, isr);
>>>> +
>>>> + stm32_rtc_wpr_lock(rtc);
>>>> +
>>>> + spin_unlock_irqrestore(&rtc->lock, irqflags);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int stm32_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
>> *alrm)
>>>> +{
>>>> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
>>>> + struct rtc_time *tm = &alrm->time;
>>>> + unsigned long irqflags;
>>>> + unsigned int cr, isr, alrmar;
>>>> + int ret = 0;
>>>> +
>>>> + if (rtc_valid_tm(tm)) {
>>>> + dev_err(dev, "Alarm time not valid.\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + tm2bcd(tm);
>>>> +
>>>> + spin_lock_irqsave(&rtc->lock, irqflags);
>>>> +
>>>> + stm32_rtc_wpr_unlock(rtc);
>>>> +
>>>> + /* Disable Alarm */
>>>> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
>>>> + cr &= ~STM32_RTC_CR_ALRAE;
>>>> + stm32_rtc_writel(rtc, STM32_RTC_CR, cr);
>>>> +
>>>> + /* Poll Alarm write flag to be sure that Alarm update is allowed */
>>>> + ret = readl_relaxed_poll_timeout_atomic(rtc->base + STM32_RTC_ISR,
>>>> + isr,
>>>> + (isr & STM32_RTC_ISR_ALRAWF),
>>>> + 10, 100);
>>>> +
>>>> + if (ret) {
>>>> + dev_err(dev, "Alarm update not allowed\n");
>>>> + goto end;
>>>> + }
>>>> +
>>>> + alrmar = 0;
>>>> +
>>>> + if (tm->tm_mday < 0 && tm->tm_wday < 0) {
>>>> + /*
>>>> + * Date/day don't care in Alarm comparison so alarm triggers
>>>> + * every day
>>>> + */
>>>> + alrmar |= STM32_RTC_ALRMXR_DATE_MASK;
>>>> + } else {
>>>> + if (tm->tm_mday > 0) {
>>>> + /* Date is selected (ignoring wday) */
>>>> + alrmar |= (tm->tm_mday << STM32_RTC_ALRMXR_DATE_SHIFT) &
>>>> + STM32_RTC_ALRMXR_DATE;
>>>> + } else {
>>>> + /* Day of week is selected */
>>>> + int wday = (tm->tm_wday == 0) ? 7 : tm->tm_wday;
>>>> +
>>>> + alrmar |= STM32_RTC_ALRMXR_WDSEL;
>>>> + alrmar |= (wday << STM32_RTC_ALRMXR_WDAY_SHIFT) &
>>>> + STM32_RTC_ALRMXR_WDAY;
>>>> + }
>>>> + }
>>>> +
>>>> + if (tm->tm_hour < 0) {
>>>> + /* Hours don't care in Alarm comparison */
>>>> + alrmar |= STM32_RTC_ALRMXR_HOUR_MASK;
>>>> + } else {
>>>> + /* 24-hour format */
>>>> + alrmar &= ~STM32_RTC_ALRMXR_PM;
>>>> + alrmar |= (tm->tm_hour << STM32_RTC_ALRMXR_HOUR_SHIFT) &
>>>> + STM32_RTC_ALRMXR_HOUR;
>>>> + }
>>>> +
>>>> + if (tm->tm_min < 0) {
>>>> + /* Minutes don't care in Alarm comparison */
>>>> + alrmar |= STM32_RTC_ALRMXR_MIN_MASK;
>>>> + } else {
>>>> + alrmar |= (tm->tm_min << STM32_RTC_ALRMXR_MIN_SHIFT) &
>>>> + STM32_RTC_ALRMXR_MIN;
>>>> + }
>>>> +
>>>> + if (tm->tm_sec < 0) {
>>>> + /* Seconds don't care in Alarm comparison */
>>>> + alrmar |= STM32_RTC_ALRMXR_SEC_MASK;
>>>> + } else {
>>>> + alrmar |= (tm->tm_sec << STM32_RTC_ALRMXR_SEC_SHIFT) &
>>>> + STM32_RTC_ALRMXR_SEC;
>>>> + }
>>>> +
>>>> + /* Write to Alarm register */
>>>> + stm32_rtc_writel(rtc, STM32_RTC_ALRMAR, alrmar);
>>>> +
>>>> + if (alrm->enabled)
>>>> + stm32_rtc_alarm_irq_enable(dev, 1);
>>>> + else
>>>> + stm32_rtc_alarm_irq_enable(dev, 0);
>>>> +
>>>> +end:
>>>> + stm32_rtc_wpr_lock(rtc);
>>>> +
>>>> + spin_unlock_irqrestore(&rtc->lock, irqflags);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static const struct rtc_class_ops stm32_rtc_ops = {
>>>> + .read_time = stm32_rtc_read_time,
>>>> + .set_time = stm32_rtc_set_time,
>>>> + .read_alarm = stm32_rtc_read_alarm,
>>>> + .set_alarm = stm32_rtc_set_alarm,
>>>> + .alarm_irq_enable = stm32_rtc_alarm_irq_enable,
>>>> +};
>>>> +
>>>> +#ifdef CONFIG_OF
>>>> +static const struct of_device_id stm32_rtc_of_match[] = {
>>>> + { .compatible = "st,stm32-rtc" },
>>>> + {}
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, stm32_rtc_of_match);
>>>> +#endif
>>>> +
>>>> +static int stm32_rtc_init(struct platform_device *pdev,
>>>> + struct stm32_rtc *rtc)
>>>> +{
>>>> + unsigned int prer, pred_a, pred_s, pred_a_max, pred_s_max, cr;
>>>> + unsigned int rate;
>>>> + unsigned long irqflags;
>>>> + int ret = 0;
>>>> +
>>>> + rate = clk_get_rate(rtc->ck_rtc);
>>>> +
>>>> + /* Find prediv_a and prediv_s to obtain the 1Hz calendar clock */
>>>> + pred_a_max = STM32_RTC_PRER_PRED_A >> STM32_RTC_PRER_PRED_A_SHIFT;
>>>> + pred_s_max = STM32_RTC_PRER_PRED_S >> STM32_RTC_PRER_PRED_S_SHIFT;
>>>> +
>>>> + for (pred_a = pred_a_max; pred_a >= 0; pred_a--) {
>>>> + pred_s = (rate / (pred_a + 1)) - 1;
>>>> +
>>>> + if (((pred_s + 1) * (pred_a + 1)) == rate)
>>>> + break;
>>>> + }
>>>> +
>>>> + /*
>>>> + * Can't find a 1Hz, so give priority to RTC power consumption
>>>> + * by choosing the higher possible value for prediv_a
>>>> + */
>>>> + if ((pred_s > pred_s_max) || (pred_a > pred_a_max)) {
>>>> + pred_a = pred_a_max;
>>>> + pred_s = (rate / (pred_a + 1)) - 1;
>>>> +
>>>> + dev_warn(&pdev->dev, "ck_rtc is %s\n",
>>>> + (rate - ((pred_a + 1) * (pred_s + 1)) < 0) ?
>>>> + "fast" : "slow");
>>>> + }
>>>> +
>>>> + spin_lock_irqsave(&rtc->lock, irqflags);
>>>> +
>>>> + stm32_rtc_wpr_unlock(rtc);
>>>> +
>>>> + ret = stm32_rtc_enter_init_mode(rtc);
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev,
>>>> + "Can't enter in init mode. Prescaler config failed.\n");
>>>> + goto end;
>>>> + }
>>>> +
>>>> + prer = (pred_s << STM32_RTC_PRER_PRED_S_SHIFT) &
>> STM32_RTC_PRER_PRED_S;
>>>> + stm32_rtc_writel(rtc, STM32_RTC_PRER, prer);
>>>> + prer |= (pred_a << STM32_RTC_PRER_PRED_A_SHIFT) &
>> STM32_RTC_PRER_PRED_A;
>>>> + stm32_rtc_writel(rtc, STM32_RTC_PRER, prer);
>>>> +
>>>> + /* Force 24h time format */
>>>> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
>>>> + cr &= ~STM32_RTC_CR_FMT;
>>>> + stm32_rtc_writel(rtc, STM32_RTC_CR, cr);
>>>> +
>>>> + stm32_rtc_exit_init_mode(rtc);
>>>> +
>>>> + ret = stm32_rtc_wait_sync(rtc);
>>>> +
>>>> + if (stm32_rtc_readl(rtc, STM32_RTC_ISR) & STM32_RTC_ISR_INITS)
>>>> + dev_warn(&pdev->dev, "Date/Time must be initialized\n");
>>>> +end:
>>>> + stm32_rtc_wpr_lock(rtc);
>>>> +
>>>> + spin_unlock_irqrestore(&rtc->lock, irqflags);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int stm32_rtc_probe(struct platform_device *pdev)
>>>> +{
>>>> + struct stm32_rtc *rtc;
>>>> + struct resource *res;
>>>> + int ret;
>>>> +
>>>> + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
>>>> + if (!rtc)
>>>> + return -ENOMEM;
>>>> +
>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>
>>> The value of 'res' should be checked before using it.
>> res is checked in devm_ioremap_resource just below :
>> if (!res || resource_type(res) != IORESOURCE_MEM) {
>> dev_err(dev, "invalid resource\n");
>> return IOMEM_ERR_PTR(-EINVAL);
>> }
>> That's why it is not checked here.
>>>
>>>> + rtc->base = devm_ioremap_resource(&pdev->dev, res);
>>>> + if (IS_ERR(rtc->base))
>>>> + return PTR_ERR(rtc->base);
>>>> +
>>>> + dbp = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
>> "st,syscfg");
>>>> + if (IS_ERR(dbp)) {
>>>> + dev_err(&pdev->dev, "no st,syscfg\n");
>>>> + return PTR_ERR(dbp);
>>>> + }
>>>> +
>>>> + spin_lock_init(&rtc->lock);
>>>> +
>>>> + rtc->ck_rtc = devm_clk_get(&pdev->dev, "ck_rtc");
>>>> + if (IS_ERR(rtc->ck_rtc)) {
>>>> + dev_err(&pdev->dev, "no ck_rtc clock");
>>>> + return PTR_ERR(rtc->ck_rtc);
>>>> + }
>>>> +
>>>> + ret = clk_prepare_enable(rtc->ck_rtc);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (dbp)
>>>> + regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, PWR_CR_DBP);
>>>
>>> The code above exits if there is a problem with the dbp, there is no point
>> in
>>> checking again.
>> You're right.
>>>
>>>> +
>>>> + ret = stm32_rtc_init(pdev, rtc);
>>>> + if (ret)
>>>> + goto err;
>>>> +
>>>> + rtc->irq_alarm = platform_get_irq_byname(pdev, "alarm");
>>>> + if (rtc->irq_alarm <= 0) {
>>>> + dev_err(&pdev->dev, "no alarm irq\n");
>>>> + ret = -ENOENT;
>>>> + goto err;
>>>> + }
>>>> +
>>>> + platform_set_drvdata(pdev, rtc);
>>>> +
>>>> + device_init_wakeup(&pdev->dev, true);
>>>
>>> What happens if device_init_wakeup() returns an error?
>> It means that RTC won't be able to wake up the board with RTC alarm. I can
>> add a warning for the user in this case ?
>
> Not really sure - it really depends on the kind of system will use this.
> For some not being able to wake up the board might a minor problem while
> for others a reason to fail the probing.
>
> Do we need a new binging for this, i.e one that would indicate this RTC can (and
> should) be able to wake up the board and fail driver probing if this can't be
> done?
>
> I'll let Alessandro and Alexander be the judge of that.
>
> Thanks,
> Mathieu
>
Ok, I wait for Alessandro and Alexandre advice to send a V2.
Thanks,
Amelie
>>>
>>>> +
>>>> + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, pdev->name,
>>>> + &stm32_rtc_ops, THIS_MODULE);
>>>> + if (IS_ERR(rtc->rtc_dev)) {
>>>> + ret = PTR_ERR(rtc->rtc_dev);
>>>> + dev_err(&pdev->dev, "rtc device registration failed, err=%d\n",
>>>> + ret);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + /* Handle RTC alarm interrupts */
>>>> + ret = devm_request_irq(&pdev->dev, rtc->irq_alarm,
>>>> + stm32_rtc_alarm_irq, IRQF_TRIGGER_RISING,
>>>> + dev_name(&rtc->rtc_dev->dev), rtc);
>>>> + if (ret) {
>>>> + dev_err(&pdev->dev, "IRQ%d (alarm interrupt) already claimed\n",
>>>> + rtc->irq_alarm);
>>>> + goto err;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +err:
>>>> + clk_disable_unprepare(rtc->ck_rtc);
>>>> +
>>>> + if (dbp)
>>>> + regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, ~PWR_CR_DBP);
>>>
>>> Same comment as above.
>> OK.
>>>
>>>> +
>>>> + device_init_wakeup(&pdev->dev, false);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +static int __exit stm32_rtc_remove(struct platform_device *pdev)
>>>> +{
>>>> + struct stm32_rtc *rtc = platform_get_drvdata(pdev);
>>>> + unsigned int cr;
>>>> +
>>>> + /* Disable interrupts */
>>>> + stm32_rtc_wpr_unlock(rtc);
>>>> + cr = stm32_rtc_readl(rtc, STM32_RTC_CR);
>>>> + cr &= ~STM32_RTC_CR_ALRAIE;
>>>> + stm32_rtc_writel(rtc, STM32_RTC_CR, cr);
>>>> + stm32_rtc_wpr_lock(rtc);
>>>> +
>>>> + clk_disable_unprepare(rtc->ck_rtc);
>>>> +
>>>> + /* Enable backup domain write protection */
>>>> + if (dbp)
>>>> + regmap_update_bits(dbp, PWR_CR, PWR_CR_DBP, ~PWR_CR_DBP);
>>>> +
>>>> + device_init_wakeup(&pdev->dev, false);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int stm32_rtc_suspend(struct device *dev)
>>>> +{
>>>> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
>>>> +
>>>> + if (device_may_wakeup(dev))
>>>> + return enable_irq_wake(rtc->irq_alarm);
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int stm32_rtc_resume(struct device *dev)
>>>> +{
>>>> + struct stm32_rtc *rtc = dev_get_drvdata(dev);
>>>> + int ret = 0;
>>>> +
>>>> + ret = stm32_rtc_wait_sync(rtc);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + if (device_may_wakeup(dev))
>>>> + return disable_irq_wake(rtc->irq_alarm);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +#endif
>>>> +
>>>> +static SIMPLE_DEV_PM_OPS(stm32_rtc_pm_ops,
>>>> + stm32_rtc_suspend, stm32_rtc_resume);
>>>> +
>>>> +static struct platform_driver stm32_rtc_driver = {
>>>> + .probe = stm32_rtc_probe,
>>>> + .remove = stm32_rtc_remove,
>>>> + .driver = {
>>>> + .name = DRIVER_NAME,
>>>> + .pm = &stm32_rtc_pm_ops,
>>>> + .of_match_table = stm32_rtc_of_match,
>>>> + },
>>>> +};
>>>> +
>>>> +module_platform_driver(stm32_rtc_driver);
>>>> +
>>>> +MODULE_ALIAS("platform:" DRIVER_NAME);
>>>> +MODULE_AUTHOR("Amelie Delaunay <amelie.delaunay@...com>");
>>>> +MODULE_DESCRIPTION("STMicroelectronics STM32 Real Time Clock driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>> --
>>>> 1.9.1
>>>>
>>
>> Best regards,
>> Amelie
>>
Powered by blists - more mailing lists