| 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: <20ffd260-3c24-460f-bdbc-965573e110e3@amlogic.com>
Date: Mon, 2 Sep 2024 16:14:45 +0800
From: Xianwei Zhao <xianwei.zhao@...ogic.com>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: Yiting Deng <yiting.deng@...ogic.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, linux-amlogic@...ts.infradead.org,
linux-rtc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rtc: support for the Amlogic on-chip RTC
Hi Alexandre,
Thanks for your reply.
On 2024/8/26 17:45, Alexandre Belloni wrote:
> [ EXTERNAL EMAIL ]
>
> On 23/08/2024 17:19:45+0800, Xianwei Zhao via B4 Relay wrote:
>> From: Yiting Deng <yiting.deng@...ogic.com>
>>
>> Support for the on-chip RTC found in some of Amlogic's SoCs such as the
>> A113L2 and A113X2.
>>
>> Signed-off-by: Yiting Deng <yiting.deng@...ogic.com>
>> Signed-off-by: Xianwei Zhao <xianwei.zhao@...ogic.com>
>> ---
>> drivers/rtc/Kconfig | 12 +
>> drivers/rtc/Makefile | 1 +
>> drivers/rtc/rtc-amlogic.c | 589 ++++++++++++++++++++++++++++++++++++++++++++++
>
> As pointed out, this is the third amlogic driver so the name of the file
> must be more specific.
>
This RTC hardware includes a timing function and an alarm function.
But the existing has only timing function, alarm function is using the
system clock to implement a virtual alarm. And the relevant register
access method is also different.
The "meson" string is meaningless, it just keeps going, and now the new
hardware uses the normal naming.
>> +static void aml_set_time(struct aml_rtc_data *rtc, u32 time_sec)
>
> Is indirection necessary, this function is used only once
>
I will delete this function.
>> +{
>> + if (rtc->config->gray_stored)
>> + time_sec = binary_to_gray(time_sec);
>> + regmap_write(rtc->map, RTC_COUNTER_REG, time_sec);
>> +}
>> +
>> +static u32 aml_read_time(struct aml_rtc_data *rtc)
> Ditto
>
I will delete this function.
>> +{
>> + u32 time_sec;
>> +
>> + regmap_read(rtc->map, RTC_REAL_TIME, &time_sec);
>> + if (rtc->config->gray_stored)
>> + time_sec = gray_to_binary(time_sec);
>> + return time_sec;
>> +}
>> +
>> +static u32 aml_read_alarm(struct aml_rtc_data *rtc)
> Ditto
>
I will delete this function.
>> +{
>> + u32 alarm_sec;
>> +
>> + regmap_read(rtc->map, RTC_ALARM0_REG, &alarm_sec);
>> + if (rtc->config->gray_stored)
>> + alarm_sec = gray_to_binary(alarm_sec);
>> + return alarm_sec;
>> +}
>> +
>> +static void aml_set_alarm(struct aml_rtc_data *rtc, u32 alarm_sec)
> Ditto
>
I will delete this function.
>> +{
>> + if (rtc->config->gray_stored)
>> + alarm_sec = binary_to_gray(alarm_sec);
>> + regmap_write(rtc->map, RTC_ALARM0_REG, alarm_sec);
>> +}
>> +
>> +static int aml_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>> +{
>> + struct aml_rtc_data *rtc = dev_get_drvdata(dev);
>> + time64_t alarm_sec;
>> +
>> + if (alarm->enabled) {
>
> Why aren't you setting the alarm when it is not enabled?
>
Will delete this judgment.
>> + regmap_update_bits(rtc->map, RTC_CTRL,
>> + RTC_ALRM0_EN, RTC_ALRM0_EN);
>> + regmap_update_bits(rtc->map, RTC_INT_MASK,
>> + RTC_ALRM0_IRQ_MSK, 0);
>> +
>> + alarm_sec = rtc_tm_to_time64(&alarm->time);
>> + if (alarm_sec > U32_MAX) {
>
> This is never going to happen, the test and error message are not
> necessary.
>
Indeed, will delete this part of the processing.
>> + dev_err(dev, "alarm value invalid!\n");
>> + return -EINVAL;
>> + }
>> + aml_set_alarm(rtc, alarm_sec);
>> + }
>> + dev_dbg(dev, "%s: alarm->enabled=%d alarm_set=%llds\n", __func__,
>> + alarm->enabled, alarm_sec);
>> +
>> + return 0;
>> +}
>> +
>> +static int aml_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>> +{
>> + struct aml_rtc_data *rtc = dev_get_drvdata(dev);
>> + u32 alarm_sec;
>> + u32 reg_val;
>> + int alarm_enable, alarm_mask;
>> +
>> + alarm_sec = aml_read_alarm(rtc);
>> + rtc_time64_to_tm(alarm_sec, &alarm->time);
>> +
>> + regmap_read(rtc->map, RTC_CTRL, ®_val);
>> + alarm_enable = FIELD_GET(RTC_ALRM0_EN, reg_val);
>> +
>> + regmap_read(rtc->map, RTC_INT_MASK, ®_val);
>> + alarm_mask = FIELD_GET(RTC_ALRM0_IRQ_MSK, reg_val);
>> +
>> + alarm->enabled = (alarm_enable && !alarm_mask) ? 1 : 0;
>> + dev_dbg(dev, "%s: alarm->enabled=%d alarm=%us\n", __func__,
>> + alarm->enabled, alarm_sec);
>> +
>> + return 0;
>> +}
>> +
>> +static irqreturn_t aml_rtc_handler(int irq, void *data)
>> +{
>> + struct aml_rtc_data *rtc = (struct aml_rtc_data *)data;
>> +
>> + regmap_write(rtc->map, RTC_ALARM0_REG, 0);
>> + regmap_update_bits(rtc->map, RTC_INT_CLR,
>> + RTC_ALRM0_IRQ_STATUS, RTC_ALRM0_IRQ_STATUS);
>
> Are you sure regmap_update_bits is necessary here?
>
There is only write RTC_ALRM0_IRQ_STATUS, to clear it. will fix it.
>> +
>> + rtc_update_irq(rtc->rtc_dev, 1, RTC_AF | RTC_IRQF);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>
>
>> +static int aml_rtc_adjust_sec(struct device *dev, u32 match_counter,
>> + int ops, int enable)
>> +{
>> + struct aml_rtc_data *rtc = dev_get_drvdata(dev->parent);
>> + u32 reg_val;
>> +
>> + if (!FIELD_FIT(RTC_MATCH_COUNTER, match_counter)) {
>> + pr_err("%s: invalid match_counter\n", __func__);
>> + return -EINVAL;
>> + }
>> +
>> + reg_val = FIELD_PREP(RTC_SEC_ADJUST_CTRL, ops)
>> + | FIELD_PREP(RTC_MATCH_COUNTER, match_counter)
>> + | FIELD_PREP(RTC_ADJ_VALID, enable);
>> + /* Set sec_adjust_ctrl, match_counter and Valid adjust */
>> + regmap_write(rtc->map, RTC_SEC_ADJUST_REG, reg_val);
>> +
>> + return 0;
>> +}
>> +
>> +static int aml_rtc_set_calibration(struct device *dev, u32 calibration)
>> +{
>> + int cal_ops, enable, match_counter;
>> + int ret;
>> +
>> + match_counter = FIELD_GET(RTC_MATCH_COUNTER, calibration);
>> + cal_ops = FIELD_GET(RTC_SEC_ADJUST_CTRL, calibration);
>> + enable = FIELD_GET(RTC_ADJ_VALID, calibration);
>> +
>> + ret = aml_rtc_adjust_sec(dev, match_counter, cal_ops, enable);
>> + dev_dbg(dev, "%s: Success to store RTC calibration attribute\n",
>> + __func__);
>> +
>> + return ret;
>> +}
>> +
>> +static int aml_rtc_get_calibration(struct device *dev, u32 *calibration)
>> +{
>> + struct aml_rtc_data *rtc = dev_get_drvdata(dev->parent);
>> + u32 reg_val;
>> +
>> + regmap_read(rtc->map, RTC_SEC_ADJUST_REG, ®_val);
>> + *calibration = FIELD_GET(RTC_SEC_ADJUST_CTRL | RTC_MATCH_COUNTER, reg_val);
>> + /* BIT is only UL defined，and GENMASK has no type, its' donot used together */
>> + *calibration |= FIELD_PREP(RTC_ADJ_VALID, FIELD_GET(RTC_ADJ_VALID, reg_val));
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * The calibration value transferred from buf takes bit[18:0] to represent
>> + * match_counter, while takes bit[20:19] to represent sec_adjust_ctrl, bit[23]
>> + * to represent adj_valid. enable/disable sec_adjust_ctrl and match_counter.
>> + * @buf: Separate buf to match_counter, sec_adjust_ctrl and adj_valid
>> + * match_counter: bit[18:0], value is 0 ~ 0x7fff
>> + * sec_adjust_ctrl: bit[20:19], value is 0 ~ 2. 3 to insert a second once every
>> + * match_counter+1 seconds, 2 to swallow a second once every match_counter+1 seconds
>> + * 0 or 1 to no adjustment
>> + * adj_valid: bit[23], value is 0 or 1, 0 to disable sec_adjust_ctrl and
>> + * match_counter, and 1 to enable them.
>> + */
>> +static ssize_t rtc_calibration_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int retval;
>> + int calibration = 0;
>> +
>> + if (sscanf(buf, " %i ", &calibration) != 1) {
>> + dev_err(dev, "Failed to store RTC calibration attribute\n");
>> + return -EINVAL;
>> + }
>> + retval = aml_rtc_set_calibration(dev, calibration);
>> +
>> + return retval ? retval : count;
>> +}
>> +
>> +static ssize_t rtc_calibration_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + int retval = 0;
>> + u32 calibration = 0;
>> +
>> + retval = aml_rtc_get_calibration(dev, &calibration);
>> + if (retval < 0) {
>> + dev_err(dev, "Failed to read RTC calibration attribute\n");
>> + sprintf(buf, "0\n");
>> + return retval;
>> + }
>> +
>> + return sprintf(buf, "0x%x\n", calibration);
>> +}
>> +static DEVICE_ATTR_RW(rtc_calibration);
>> +
>> +static int rtc_set_div256_adjust(struct device *dev, u32 *value)
>> +{
>> + struct aml_rtc_data *rtc = dev_get_drvdata(dev->parent);
>> + u32 div256_adj;
>> +
>> + div256_adj = FIELD_PREP(RTC_DIV256_ADJ_DSR | RTC_DIV256_ADJ_VAL, *value);
>> + /*
>> + * AO_RTC_SEC_ADJUST_REG bit 24 insert/remove(1/0) a div256 cycle,
>> + * bit 25 valid/invalid(1/0) div256_adj_val
>> + */
>> + regmap_write_bits(rtc->map, RTC_SEC_ADJUST_REG,
>> + RTC_DIV256_ADJ_DSR | RTC_DIV256_ADJ_VAL, div256_adj);
>> + /* rtc need about 30ms to adjust its time after written */
>> + mdelay(30);
>> +
>> + return 0;
>> +}
>> +
>> +static int rtc_get_div256_adjust(struct device *dev, u32 *value)
>> +{
>> + struct aml_rtc_data *rtc = dev_get_drvdata(dev->parent);
>> + u32 reg_val;
>> +
>> + regmap_read(rtc->map, RTC_SEC_ADJUST_REG, ®_val);
>> + *value = FIELD_GET(RTC_DIV256_ADJ_DSR | RTC_DIV256_ADJ_VAL, reg_val);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * div256 adjust function is controlled using bit[24] and bit[25].
>> + * transferred buf takes bit[0] to represent div256 adj val, bit[1]
>> + * to represent div256 adj enable/disable. div256 cycle means that adjust
>> + * 1/32768/256 once by written once, it's val is equal to 1/128s
>> + * @buf: 3: enable div256 adjust and insert a div256 cycle
>> + * 2: enable div256 adjust and remove a div256 cycle
>> + * 1 or 0: disable div256 adjust
>> + */
>> +static ssize_t rtc_div256_adjust_store(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + int retval;
>> + u32 value = 0;
>> +
>> + if (sscanf(buf, " %i ", &value) != 1) {
>> + dev_err(dev, "Failed to store RTC div256 adjust attribute\n");
>> + return -EINVAL;
>> + }
>> + retval = rtc_set_div256_adjust(dev, &value);
>> +
>> + return retval ? retval : count;
>> +}
>> +
>> +static ssize_t rtc_div256_adjust_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + int retval = 0;
>> + u32 value = 0;
>> +
>> + retval = rtc_get_div256_adjust(dev, &value);
>> + if (retval < 0) {
>> + dev_err(dev, "Failed to read RTC div256 adjust attribute\n");
>> + sprintf(buf, "0\n");
>> + return retval;
>> + }
>> +
>> + return sprintf(buf, "0x%x\n", value);
>> +}
>> +static DEVICE_ATTR_RW(rtc_div256_adjust);
>> +
>> +static struct attribute *aml_rtc_attrs[] = {
>> + &dev_attr_rtc_calibration.attr,
>> + &dev_attr_rtc_div256_adjust.attr,
>> + NULL,
>> +};
>> +
>> +static const struct attribute_group aml_rtc_sysfs_files = {
>> + .attrs = aml_rtc_attrs,
>> +};
>> +
>
> You must use the standard RTC API to handle calibration, see
> .read_offset and .set_offset
>
Will do.
>> +static void aml_rtc_init(struct device *dev, struct aml_rtc_data *rtc)
>> +{
>> + u32 reg_val;
>> + u32 rtc_enable;
>> +
>> + regmap_read(rtc->map, RTC_CTRL, ®_val);
>> + rtc_enable = FIELD_GET(RTC_ENABLE, reg_val);
>> + if (!rtc_enable) {
>> + if (clk_get_rate(rtc->sclk) == OSC_24M) {
>> + /* select 24M oscillator */
>> + regmap_update_bits(rtc->map, RTC_CTRL, RTC_OSC_SEL, RTC_OSC_SEL);
>> +
>> + /*
>> + * Set RTC oscillator to freq_out to freq_in/((N0*M0+N1*M1)/(M0+M1))
>> + * Enable clock_in gate of oscillator 24MHz
>> + * Set N0 to 733, N1 to 732
>> + */
>> + reg_val = FIELD_PREP(RTC_OSCIN_IN_EN, 1)
>> + | FIELD_PREP(RTC_OSCIN_OUT_CFG, 1)
>> + | FIELD_PREP(RTC_OSCIN_OUT_N0M0, RTC_OSCIN_OUT_32K_N0)
>> + | FIELD_PREP(RTC_OSCIN_OUT_N1M1, RTC_OSCIN_OUT_32K_N1);
>> + regmap_write_bits(rtc->map, RTC_OSCIN_CTRL0, RTC_OSCIN_IN_EN
>> + | RTC_OSCIN_OUT_CFG | RTC_OSCIN_OUT_N0M0
>> + | RTC_OSCIN_OUT_N1M1, reg_val);
>> +
>> + /* Set M0 to 2, M1 to 3, so freq_out = 32768 Hz*/
>> + reg_val = FIELD_PREP(RTC_OSCIN_OUT_N0M0, RTC_OSCIN_OUT_32K_M0)
>> + | FIELD_PREP(RTC_OSCIN_OUT_N1M1, RTC_OSCIN_OUT_32K_M1);
>> + regmap_write_bits(rtc->map, RTC_OSCIN_CTRL1, RTC_OSCIN_OUT_N0M0
>> + | RTC_OSCIN_OUT_N1M1, reg_val);
>> + } else {
>> + /* select 32K oscillator */
>> + regmap_write_bits(rtc->map, RTC_CTRL, RTC_OSC_SEL, 0);
>> + }
>> + /* Enable RTC */
>> + regmap_write_bits(rtc->map, RTC_CTRL, RTC_ENABLE, RTC_ENABLE);
>
> This must not be done at probe time, else you loose the
> important information taht the time has never been set. Instead,
> it should only be enabled on the first .set_time invocation do
> you could now in .read_time that the time is currently invalid.
>
There are some doubts about this place.
You mean that after the system is up, unless the time is set, it will
fail to read the time at any time, and the alarm clock will also fail.
In this case, the system must set a time.
When read time invlalid, system is will set time.
This part of the logic I see the kernel part has not been implemented,
so only the user application has been implemented. Whether this is
reasonable, if not set time, you will never use RTC module.
>> + usleep_range(100, 200);
>> + }
>> + regmap_write_bits(rtc->map, RTC_INT_MASK,
>> + RTC_ALRM0_IRQ_MSK, RTC_ALRM0_IRQ_MSK);
>> + regmap_write_bits(rtc->map, RTC_CTRL, RTC_ALRM0_EN, 0);
>> +}
>> +
>
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Powered by blists - more mailing lists