| 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: <2024082609451907fd19e2@mail.local>
Date: Mon, 26 Aug 2024 11:45:19 +0200
From: Alexandre Belloni <alexandre.belloni@...tlin.com>
To: xianwei.zhao@...ogic.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
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.
> +static void aml_set_time(struct aml_rtc_data *rtc, u32 time_sec)
Is indirection necessary, this function is used only once
> +{
> + 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
> +{
> + 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
> +{
> + 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
> +{
> + 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?
> + 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.
> + 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?
> +
> + 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
> +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.
> + 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