| 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
| ||
|
Date: Wed, 30 Dec 2015 12:41:36 +0900
From: Krzysztof Kozlowski <k.kozlowski@...sung.com>
To: Yadwinder Singh Brar <yadi.brar01@...il.com>
Cc: Sangbeom Kim <sbkim73@...sung.com>,
Alessandro Zummo <a.zummo@...ertech.it>,
Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
Lee Jones <lee.jones@...aro.org>,
linux-kernel <linux-kernel@...r.kernel.org>,
linux-samsung-soc <linux-samsung-soc@...r.kernel.org>,
rtc-linux@...glegroups.com, Alim Akhtar <alim.akhtar@...sung.com>
Subject: Re: [PATCH 3/3] rtc: s5m: Make register configuration per S2MPS device
to remove exceptions
On 30.12.2015 12:38, Krzysztof Kozlowski wrote:
> On 30.12.2015 11:53, Yadwinder Singh Brar wrote:
>> Hi Krysztof,
>>
>> On Tue, Dec 29, 2015 at 5:53 PM, Krzysztof Kozlowski
>> <k.kozlowski@...sung.com> wrote:
>>> Before updating time and alarm the driver must set appropriate mask in
>>> UDR register. For that purpose the driver uses common register
>>> configuration and a lot of exceptions per device in the code. The
>>> exceptions are not obvious, for example except the change in the logic
>>> sometimes the fields are swapped (WUDR and AUDR between S2MPS14 and
>>> S2MPS15). This leads to quite complicated code.
>>>
>>> Try to make it more obvious by:
>>> 1. Documenting the UDR masks for devices and operations.
>>> 2. Adding fields in register configuration structure for each operation
>>> (read time, write time and alarm).
>>> 3. Splitting the configuration per S2MPS13, S2MPS14 and S2MPS15 thus
>>> removing exceptions for them.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@...sung.com>
>>>
>>> ---
>>>
>>> Tested only on S2MPS11 (Odroid XU4).
>>> ---
>>> drivers/rtc/rtc-s5m.c | 110 +++++++++++++++++++++++++++-------------
>>> include/linux/mfd/samsung/rtc.h | 2 +
>>> 2 files changed, 77 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
>>> index 559db8f72117..7407d7394bb4 100644
>>> --- a/drivers/rtc/rtc-s5m.c
>>> +++ b/drivers/rtc/rtc-s5m.c
>>> @@ -38,7 +38,22 @@
>>> */
>>> #define UDR_READ_RETRY_CNT 5
>>>
>>> -/* Registers used by the driver which are different between chipsets. */
>>> +/*
>>> + * Registers used by the driver which are different between chipsets.
>>> + *
>>> + * Operations like read time and write alarm/time require updating
>>> + * specific fields in UDR register. These fields usually are auto-cleared
>>> + * (with some exceptions).
>>> + *
>>> + * Table of operations per device:
>>> + *
>>> + * Device | Write time | Read time | Write alarm
>>> + * =================================================
>>> + * S5M8767 | UDR + TIME | | UDR
>>> + * S2MPS11/14 | WUDR | RUDR | WUDR + RUDR
>>> + * S2MPS13 | WUDR | RUDR | WUDR + AUDR
>>> + * S2MPS15 | WUDR | RUDR | AUDR
>>> + */
>>> struct s5m_rtc_reg_config {
>>> /* Number of registers used for setting time/alarm0/alarm1 */
>>> unsigned int regs_count;
>>> @@ -58,8 +73,13 @@ struct s5m_rtc_reg_config {
>>> unsigned int udr_update;
>>> /* Auto-cleared mask in UDR field for writing time and alarm */
>>> unsigned int autoclear_udr_mask;
>>> - /* Mask for UDR field in 'udr_update' register */
>>> - unsigned int udr_mask;
>>> + /*
>>> + * Masks in UDR field for time and alarm operations.
>>> + * The read time mask can be 0. Rest should not.
>>> + */
>>> + unsigned int read_time_udr_mask;
>>> + unsigned int write_time_udr_mask;
>>> + unsigned int write_alarm_udr_mask;
>>> };
>>>
>>> /* Register map for S5M8763 and S5M8767 */
>>> @@ -71,14 +91,44 @@ static const struct s5m_rtc_reg_config s5m_rtc_regs = {
>>> .alarm1 = S5M_ALARM1_SEC,
>>> .udr_update = S5M_RTC_UDR_CON,
>>> .autoclear_udr_mask = S5M_RTC_UDR_MASK,
>>> - .udr_mask = S5M_RTC_UDR_MASK,
>>> + .read_time_udr_mask = 0, /* Not needed */
>>> + .write_time_udr_mask = S5M_RTC_UDR_MASK | S5M_RTC_TIME_EN_MASK,
>>> + .write_alarm_udr_mask = S5M_RTC_UDR_MASK,
>>> +};
>>> +
>>> +/* Register map for S2MPS13 */
>>> +static const struct s5m_rtc_reg_config s2mps13_rtc_regs = {
>>> + .regs_count = 7,
>>> + .time = S2MPS_RTC_SEC,
>>> + .ctrl = S2MPS_RTC_CTRL,
>>> + .alarm0 = S2MPS_ALARM0_SEC,
>>> + .alarm1 = S2MPS_ALARM1_SEC,
>>> + .udr_update = S2MPS_RTC_UDR_CON,
>>> + .autoclear_udr_mask = S2MPS_RTC_WUDR_MASK,
>>> + .read_time_udr_mask = S2MPS_RTC_RUDR_MASK,
>>> + .write_time_udr_mask = S2MPS_RTC_WUDR_MASK,
>>> + .write_alarm_udr_mask = S2MPS_RTC_WUDR_MASK | S2MPS13_RTC_AUDR_MASK,
>>> +};
>>> +
>>> +/* Register map for S2MPS11/14 */
>>> +static const struct s5m_rtc_reg_config s2mps14_rtc_regs = {
>>> + .regs_count = 7,
>>> + .time = S2MPS_RTC_SEC,
>>> + .ctrl = S2MPS_RTC_CTRL,
>>> + .alarm0 = S2MPS_ALARM0_SEC,
>>> + .alarm1 = S2MPS_ALARM1_SEC,
>>> + .udr_update = S2MPS_RTC_UDR_CON,
>>> + .autoclear_udr_mask = S2MPS_RTC_WUDR_MASK,
>>> + .read_time_udr_mask = S2MPS_RTC_RUDR_MASK,
>>> + .write_time_udr_mask = S2MPS_RTC_WUDR_MASK,
>>> + .write_alarm_udr_mask = S2MPS_RTC_WUDR_MASK | S2MPS_RTC_RUDR_MASK,
>>> };
>>>
>>> /*
>>> - * Register map for S2MPS14.
>>> - * It may be also suitable for S2MPS11 but this was not tested.
>>> + * Register map for S2MPS15 - in comparison to S2MPS14 the WUDR and AUDR bits
>>> + * are swapped.
>>> */
>>> -static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>>> +static const struct s5m_rtc_reg_config s2mps15_rtc_regs = {
>>> .regs_count = 7,
>>> .time = S2MPS_RTC_SEC,
>>> .ctrl = S2MPS_RTC_CTRL,
>>> @@ -86,7 +136,9 @@ static const struct s5m_rtc_reg_config s2mps_rtc_regs = {
>>> .alarm1 = S2MPS_ALARM1_SEC,
>>> .udr_update = S2MPS_RTC_UDR_CON,
>>> .autoclear_udr_mask = S2MPS_RTC_WUDR_MASK,
>>> - .udr_mask = S2MPS_RTC_WUDR_MASK,
>>> + .read_time_udr_mask = S2MPS_RTC_RUDR_MASK,
>>> + .write_time_udr_mask = S2MPS15_RTC_WUDR_MASK,
>>> + .write_alarm_udr_mask = S2MPS15_RTC_AUDR_MASK,
>>> };
>>>
>>> struct s5m_rtc_info {
>>> @@ -223,21 +275,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
>>> return ret;
>>> }
>>>
>>> - switch (info->device_type) {
>>> - case S5M8763X:
>>> - case S5M8767X:
>>> - data |= info->regs->udr_mask | S5M_RTC_TIME_EN_MASK;
>>> - case S2MPS15X:
>>> - /* As per UM, for write time register, set WUDR bit to high */
>>> - data |= S2MPS15_RTC_WUDR_MASK;
>>> - break;
>>> - case S2MPS14X:
>>> - case S2MPS13X:
>>> - data |= info->regs->udr_mask;
>>> - break;
>>> - default:
>>> - return -EINVAL;
>>> - }
>>> + data |= info->regs->write_time_udr_mask;
>>>
>>> ret = regmap_write(info->regmap, info->regs->udr_update, data);
>>> if (ret < 0) {
>>> @@ -262,22 +300,16 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
>>> return ret;
>>> }
>>>
>>> - data |= info->regs->udr_mask;
>>> + data |= info->regs->write_alarm_udr_mask;
>>> switch (info->device_type) {
>>> case S5M8763X:
>>> case S5M8767X:
>>> data &= ~S5M_RTC_TIME_EN_MASK;
>>> break;
>>> case S2MPS15X:
>>> - /* As per UM, for write alarm, set A_UDR(bit[4]) to high
>>> - * udr_mask above sets bit[4]
>>> - */
>>> - break;
>>> case S2MPS14X:
>>> - data |= S2MPS_RTC_RUDR_MASK;
>>> - break;
>>> case S2MPS13X:
>>> - data |= S2MPS13_RTC_AUDR_MASK;
>>> + /* No exceptions needed */
>>> break;
>>> default:
>>> return -EINVAL;
>>> @@ -338,11 +370,11 @@ static int s5m_rtc_read_time(struct device *dev, struct rtc_time *tm)
>>> u8 data[info->regs->regs_count];
>>> int ret;
>>>
>>> - if (info->device_type == S2MPS15X || info->device_type == S2MPS14X ||
>>> - info->device_type == S2MPS13X) {
>>> + if (info->regs->read_time_udr_mask) {
>>> ret = regmap_update_bits(info->regmap,
>>> info->regs->udr_update,
>>> - S2MPS_RTC_RUDR_MASK, S2MPS_RTC_RUDR_MASK);
>>> + info->regs->read_time_udr_mask,
>>> + info->regs->read_time_udr_mask);
>>> if (ret) {
>>> dev_err(dev,
>>> "Failed to prepare registers for time reading: %d\n",
>>> @@ -709,10 +741,18 @@ static int s5m_rtc_probe(struct platform_device *pdev)
>>>
>>> switch (platform_get_device_id(pdev)->driver_data) {
>>> case S2MPS15X:
>>> + regmap_cfg = &s2mps14_rtc_regmap_config;
>>> + info->regs = &s2mps15_rtc_regs;
>>> + alarm_irq = S2MPS14_IRQ_RTCA0;
>>> + break;
>>> case S2MPS14X:
>>> + regmap_cfg = &s2mps14_rtc_regmap_config;
>>> + info->regs = &s2mps14_rtc_regs;
>>> + alarm_irq = S2MPS14_IRQ_RTCA0;
>>> + break;
>>> case S2MPS13X:
>>> regmap_cfg = &s2mps14_rtc_regmap_config;
>>> - info->regs = &s2mps_rtc_regs;
>>> + info->regs = &s2mps13_rtc_regs;
>>> alarm_irq = S2MPS14_IRQ_RTCA0;
>>> break;
>>> case S5M8763X:
>>> diff --git a/include/linux/mfd/samsung/rtc.h b/include/linux/mfd/samsung/rtc.h
>>> index a65e4655d470..af0df5f3f651 100644
>>> --- a/include/linux/mfd/samsung/rtc.h
>>> +++ b/include/linux/mfd/samsung/rtc.h
>>> @@ -105,6 +105,8 @@ enum s2mps_rtc_reg {
>>> #define S5M_RTC_UDR_MASK (1 << S5M_RTC_UDR_SHIFT)
>>> #define S2MPS_RTC_WUDR_SHIFT 4
>>> #define S2MPS_RTC_WUDR_MASK (1 << S2MPS_RTC_WUDR_SHIFT)
>>> +#define S2MPS15_RTC_AUDR_SHIFT 4
>>> +#define S2MPS15_RTC_AUDR_MASK (4 << S2MPS15_RTC_AUDR_SHIFT)
>>
>> Shouldn't be ?
>> +#define S2MPS15_RTC_AUDR_MASK (1 << S2MPS15_RTC_AUDR_SHIFT)
>>
>> because "As per UM, for write alarm, set A_UDR(bit[4]) to high"
>>
>
> Nope. :) You wrote set bit[4], not bit[1].
Oh damn, you are right. The value should be '1'. Shame on me...
Thanks for pointing this out.
BR
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists