* Re: [PATCH] rtc: s3c-rtc: Avoid using broken ALMYEAR register
2018-11-13 11:32 ` [PATCH] rtc: s3c-rtc: Avoid using broken ALMYEAR register Marek Szyprowski
@ 2018-11-13 11:39 ` Alexandre Belloni
2018-11-13 11:52 ` Marek Szyprowski
2018-11-13 11:50 ` Krzysztof Kozlowski
2018-11-14 9:59 ` Alexandre Belloni
2 siblings, 1 reply; 7+ messages in thread
From: Alexandre Belloni @ 2018-11-13 11:39 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-rtc, linux-samsung-soc, Alessandro Zummo,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz
Hello Marek,
On 13/11/2018 12:32:50+0100, Marek Szyprowski wrote:
> (RTC,ALM)YEAR registers of Exynos built-in RTC device contains 3 BCD
> characters. s3c-rtc driver uses only 2 lower of them and supports years
> from 2000..2099 range. The third BCD value is typically set to 0, but it
> looks that handling of it is broken in the hardware. It sometimes
> defaults to a random (even non-BCD) value. This is not an issue
> for handling RTCYEAR register, because bcd2bin() properly handles only
> 8bit values (2 BCD characters, the third one is skipped). The problem
> is however with ALMYEAR register and proper RTC alarm operation. When
> YEAREN bit is set for the configured alarm, RTC hardware triggers alarm
> only when ALMYEAR and RTCYEAR matches. This usually doesn't happen
> because of the random noise on the third BCD character.
>
I'm probably missing something but can't you set that third BCD value to
0 in ALMYEAR?
> Fix this by simply skipping setting ALMYEAR register in alarm
> configuration. This workaround fixes broken alarm operation on Exynos
> built-in rtc device. My tests revealed that the issue happens on the
> following Exynos series: 3250, 4210, 4412, 5250 and 5410.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/rtc/rtc-s3c.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> index 75c8c5033e08..58e03ac3578b 100644
> --- a/drivers/rtc/rtc-s3c.c
> +++ b/drivers/rtc/rtc-s3c.c
> @@ -327,7 +327,6 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
> struct rtc_time *tm = &alrm->time;
> unsigned int alrm_en;
> int ret;
> - int year = tm->tm_year - 100;
>
> dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
> alrm->enabled,
> @@ -356,11 +355,6 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
> writeb(bin2bcd(tm->tm_hour), info->base + S3C2410_ALMHOUR);
> }
>
> - if (year < 100 && year >= 0) {
> - alrm_en |= S3C2410_RTCALM_YEAREN;
> - writeb(bin2bcd(year), info->base + S3C2410_ALMYEAR);
> - }
> -
> if (tm->tm_mon < 12 && tm->tm_mon >= 0) {
> alrm_en |= S3C2410_RTCALM_MONEN;
> writeb(bin2bcd(tm->tm_mon + 1), info->base + S3C2410_ALMMON);
> --
> 2.17.1
>
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] rtc: s3c-rtc: Avoid using broken ALMYEAR register
2018-11-13 11:39 ` Alexandre Belloni
@ 2018-11-13 11:52 ` Marek Szyprowski
0 siblings, 0 replies; 7+ messages in thread
From: Marek Szyprowski @ 2018-11-13 11:52 UTC (permalink / raw)
To: Alexandre Belloni
Cc: linux-rtc, linux-samsung-soc, Alessandro Zummo,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz
Hi Alexandre,
On 2018-11-13 12:39, Alexandre Belloni wrote:
> On 13/11/2018 12:32:50+0100, Marek Szyprowski wrote:
>> (RTC,ALM)YEAR registers of Exynos built-in RTC device contains 3 BCD
>> characters. s3c-rtc driver uses only 2 lower of them and supports years
>> from 2000..2099 range. The third BCD value is typically set to 0, but it
>> looks that handling of it is broken in the hardware. It sometimes
>> defaults to a random (even non-BCD) value. This is not an issue
>> for handling RTCYEAR register, because bcd2bin() properly handles only
>> 8bit values (2 BCD characters, the third one is skipped). The problem
>> is however with ALMYEAR register and proper RTC alarm operation. When
>> YEAREN bit is set for the configured alarm, RTC hardware triggers alarm
>> only when ALMYEAR and RTCYEAR matches. This usually doesn't happen
>> because of the random noise on the third BCD character.
>>
> I'm probably missing something but can't you set that third BCD value to
> 0 in ALMYEAR?
s3c-rtc code sets it to 0 already, both for ALMYEAR and RTCYEAR registers
(bin2bcd returns 2 BCD characters, which are written to the register as u32,
with leading zeros). However I've observed that both mentioned registers
after such operation can still contain random noise on the third BCD
character, what make the alarm completely unreliable.
> ...
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rtc: s3c-rtc: Avoid using broken ALMYEAR register
2018-11-13 11:32 ` [PATCH] rtc: s3c-rtc: Avoid using broken ALMYEAR register Marek Szyprowski
2018-11-13 11:39 ` Alexandre Belloni
@ 2018-11-13 11:50 ` Krzysztof Kozlowski
2018-11-13 12:15 ` Marek Szyprowski
2018-11-14 9:59 ` Alexandre Belloni
2 siblings, 1 reply; 7+ messages in thread
From: Krzysztof Kozlowski @ 2018-11-13 11:50 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-rtc, linux-samsung-soc@vger.kernel.org, alexandre.belloni,
a.zummo, Bartłomiej Żołnierkiewicz
On Tue, 13 Nov 2018 at 12:33, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> (RTC,ALM)YEAR registers of Exynos built-in RTC device contains 3 BCD
> characters. s3c-rtc driver uses only 2 lower of them and supports years
> from 2000..2099 range. The third BCD value is typically set to 0, but it
> looks that handling of it is broken in the hardware. It sometimes
> defaults to a random (even non-BCD) value. This is not an issue
> for handling RTCYEAR register, because bcd2bin() properly handles only
> 8bit values (2 BCD characters, the third one is skipped). The problem
> is however with ALMYEAR register and proper RTC alarm operation. When
> YEAREN bit is set for the configured alarm, RTC hardware triggers alarm
> only when ALMYEAR and RTCYEAR matches. This usually doesn't happen
> because of the random noise on the third BCD character.
>
> Fix this by simply skipping setting ALMYEAR register in alarm
> configuration. This workaround fixes broken alarm operation on Exynos
> built-in rtc device. My tests revealed that the issue happens on the
> following Exynos series: 3250, 4210, 4412, 5250 and 5410.
Does it mean that alarm set for 20.11.2019 will be triggered also this
year on 20.11.2018 (and any other year as well)?
Best regards,
Krzysztof
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/rtc/rtc-s3c.c | 6 ------
> 1 file changed, 6 deletions(-)
>
> diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c
> index 75c8c5033e08..58e03ac3578b 100644
> --- a/drivers/rtc/rtc-s3c.c
> +++ b/drivers/rtc/rtc-s3c.c
> @@ -327,7 +327,6 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
> struct rtc_time *tm = &alrm->time;
> unsigned int alrm_en;
> int ret;
> - int year = tm->tm_year - 100;
>
> dev_dbg(dev, "s3c_rtc_setalarm: %d, %04d.%02d.%02d %02d:%02d:%02d\n",
> alrm->enabled,
> @@ -356,11 +355,6 @@ static int s3c_rtc_setalarm(struct device *dev, struct rtc_wkalrm *alrm)
> writeb(bin2bcd(tm->tm_hour), info->base + S3C2410_ALMHOUR);
> }
>
> - if (year < 100 && year >= 0) {
> - alrm_en |= S3C2410_RTCALM_YEAREN;
> - writeb(bin2bcd(year), info->base + S3C2410_ALMYEAR);
> - }
> -
> if (tm->tm_mon < 12 && tm->tm_mon >= 0) {
> alrm_en |= S3C2410_RTCALM_MONEN;
> writeb(bin2bcd(tm->tm_mon + 1), info->base + S3C2410_ALMMON);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] rtc: s3c-rtc: Avoid using broken ALMYEAR register
2018-11-13 11:50 ` Krzysztof Kozlowski
@ 2018-11-13 12:15 ` Marek Szyprowski
2018-11-13 12:33 ` Alexandre Belloni
0 siblings, 1 reply; 7+ messages in thread
From: Marek Szyprowski @ 2018-11-13 12:15 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: linux-rtc, linux-samsung-soc@vger.kernel.org, alexandre.belloni,
a.zummo, Bartłomiej Żołnierkiewicz
Hi Krzysztof,
On 2018-11-13 12:50, Krzysztof Kozlowski wrote:
> On Tue, 13 Nov 2018 at 12:33, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>> (RTC,ALM)YEAR registers of Exynos built-in RTC device contains 3 BCD
>> characters. s3c-rtc driver uses only 2 lower of them and supports years
>> from 2000..2099 range. The third BCD value is typically set to 0, but it
>> looks that handling of it is broken in the hardware. It sometimes
>> defaults to a random (even non-BCD) value. This is not an issue
>> for handling RTCYEAR register, because bcd2bin() properly handles only
>> 8bit values (2 BCD characters, the third one is skipped). The problem
>> is however with ALMYEAR register and proper RTC alarm operation. When
>> YEAREN bit is set for the configured alarm, RTC hardware triggers alarm
>> only when ALMYEAR and RTCYEAR matches. This usually doesn't happen
>> because of the random noise on the third BCD character.
>>
>> Fix this by simply skipping setting ALMYEAR register in alarm
>> configuration. This workaround fixes broken alarm operation on Exynos
>> built-in rtc device. My tests revealed that the issue happens on the
>> following Exynos series: 3250, 4210, 4412, 5250 and 5410.
> Does it mean that alarm set for 20.11.2019 will be triggered also this
> year on 20.11.2018 (and any other year as well)?
Yes, this will trigger one year earlier.
The question is if this is really and issue in typical use cases...
The only other workaround I see is to store the requested alarm time in
the driver context and check each time when it triggers interrupt if the
year matches requested value.
> ...
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rtc: s3c-rtc: Avoid using broken ALMYEAR register
2018-11-13 12:15 ` Marek Szyprowski
@ 2018-11-13 12:33 ` Alexandre Belloni
0 siblings, 0 replies; 7+ messages in thread
From: Alexandre Belloni @ 2018-11-13 12:33 UTC (permalink / raw)
To: Marek Szyprowski
Cc: Krzysztof Kozlowski, linux-rtc, linux-samsung-soc@vger.kernel.org,
a.zummo, Bartłomiej Żołnierkiewicz
On 13/11/2018 13:15:33+0100, Marek Szyprowski wrote:
> Hi Krzysztof,
>
> On 2018-11-13 12:50, Krzysztof Kozlowski wrote:
> > On Tue, 13 Nov 2018 at 12:33, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >> (RTC,ALM)YEAR registers of Exynos built-in RTC device contains 3 BCD
> >> characters. s3c-rtc driver uses only 2 lower of them and supports years
> >> from 2000..2099 range. The third BCD value is typically set to 0, but it
> >> looks that handling of it is broken in the hardware. It sometimes
> >> defaults to a random (even non-BCD) value. This is not an issue
> >> for handling RTCYEAR register, because bcd2bin() properly handles only
> >> 8bit values (2 BCD characters, the third one is skipped). The problem
> >> is however with ALMYEAR register and proper RTC alarm operation. When
> >> YEAREN bit is set for the configured alarm, RTC hardware triggers alarm
> >> only when ALMYEAR and RTCYEAR matches. This usually doesn't happen
> >> because of the random noise on the third BCD character.
> >>
> >> Fix this by simply skipping setting ALMYEAR register in alarm
> >> configuration. This workaround fixes broken alarm operation on Exynos
> >> built-in rtc device. My tests revealed that the issue happens on the
> >> following Exynos series: 3250, 4210, 4412, 5250 and 5410.
> > Does it mean that alarm set for 20.11.2019 will be triggered also this
> > year on 20.11.2018 (and any other year as well)?
> Yes, this will trigger one year earlier.
> The question is if this is really and issue in typical use cases...
>
Note that the core will already set the alarm again if it fired too
early. So this is way better than not firing at all.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rtc: s3c-rtc: Avoid using broken ALMYEAR register
2018-11-13 11:32 ` [PATCH] rtc: s3c-rtc: Avoid using broken ALMYEAR register Marek Szyprowski
2018-11-13 11:39 ` Alexandre Belloni
2018-11-13 11:50 ` Krzysztof Kozlowski
@ 2018-11-14 9:59 ` Alexandre Belloni
2 siblings, 0 replies; 7+ messages in thread
From: Alexandre Belloni @ 2018-11-14 9:59 UTC (permalink / raw)
To: Marek Szyprowski
Cc: linux-rtc, linux-samsung-soc, Alessandro Zummo,
Krzysztof Kozlowski, Bartlomiej Zolnierkiewicz
On 13/11/2018 12:32:50+0100, Marek Szyprowski wrote:
> (RTC,ALM)YEAR registers of Exynos built-in RTC device contains 3 BCD
> characters. s3c-rtc driver uses only 2 lower of them and supports years
> from 2000..2099 range. The third BCD value is typically set to 0, but it
> looks that handling of it is broken in the hardware. It sometimes
> defaults to a random (even non-BCD) value. This is not an issue
> for handling RTCYEAR register, because bcd2bin() properly handles only
> 8bit values (2 BCD characters, the third one is skipped). The problem
> is however with ALMYEAR register and proper RTC alarm operation. When
> YEAREN bit is set for the configured alarm, RTC hardware triggers alarm
> only when ALMYEAR and RTCYEAR matches. This usually doesn't happen
> because of the random noise on the third BCD character.
>
> Fix this by simply skipping setting ALMYEAR register in alarm
> configuration. This workaround fixes broken alarm operation on Exynos
> built-in rtc device. My tests revealed that the issue happens on the
> following Exynos series: 3250, 4210, 4412, 5250 and 5410.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
> drivers/rtc/rtc-s3c.c | 6 ------
> 1 file changed, 6 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply [flat|nested] 7+ messages in thread