| 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: <dcc28bf5-0b3e-4133-80c4-e677ecb38388@kernel.org>
Date: Mon, 26 Aug 2024 10:27:06 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: xianwei.zhao@...ogic.com, Yiting Deng <yiting.deng@...ogic.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>
Cc: 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 11:19, 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 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 602 insertions(+)
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 2a95b05982ad..0dd042701c3b 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -2043,4 +2043,16 @@ config RTC_DRV_SSD202D
> This driver can also be built as a module, if so, the module
> will be called "rtc-ssd20xd".
>
> +config RTC_DRV_AMLOGIC
> + tristate "Amlogic RTC"
> + depends on ARCH_MESON || COMPILE_TEST
So the third driver? What is wrong with existing ones? And why this one
is named so differently?
> + select REGMAP_MMIO
> + default y
> + help
> + If you say yes here you get support for the RTC block on the
> + Amlogic A113L2(A4) and A113X2(A5) SoCs.
> +
> + This driver can also be built as a module. If so, the module
> + will be called "rtc-amlogic".
> +
> endif # RTC_CLASS
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index 3004e372f25f..d4a56ddb4246 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_RTC_DRV_ABB5ZES3) += rtc-ab-b5ze-s3.o
> obj-$(CONFIG_RTC_DRV_ABEOZ9) += rtc-ab-eoz9.o
> obj-$(CONFIG_RTC_DRV_ABX80X) += rtc-abx80x.o
> obj-$(CONFIG_RTC_DRV_AC100) += rtc-ac100.o
> +obj-$(CONFIG_RTC_DRV_AMLOGIC) += rtc-amlogic.o
> obj-$(CONFIG_RTC_DRV_ARMADA38X) += rtc-armada38x.o
> obj-$(CONFIG_RTC_DRV_AS3722) += rtc-as3722.o
> obj-$(CONFIG_RTC_DRV_ASM9260) += rtc-asm9260.o
> +
> +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__);
No, do not use pr_, but driver ones.
> + 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.
Messed kerneldoc. You have warning shere. Fix it.
> + */
> +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");
Where is the ABI documented?
> + 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);
Document the ABI.
> +
> +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
Again incorrect kerneldoc.
> + */
> +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,
> +};
> +
> +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);
> + 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);
> +}
> +
> +static int aml_rtc_probe(struct platform_device *pdev)
> +{
> + struct aml_rtc_data *rtc;
> + void __iomem *base;
> + struct clk *core_clk;
> + int ret = 0;
> +
> + rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> + if (!rtc)
> + return -ENOMEM;
> +
> + rtc->config = of_device_get_match_data(&pdev->dev);
> + if (!rtc->config)
> + return -ENODEV;
> +
> + base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(base)) {
> + dev_err(&pdev->dev, "resource ioremap failed\n");
> + return PTR_ERR(base);
> + }
> +
> + rtc->map = devm_regmap_init_mmio(&pdev->dev, base, &aml_rtc_regmap_config);
> + if (IS_ERR(rtc->map)) {
> + dev_err(&pdev->dev, "regmap init failed\n");
> + return PTR_ERR(rtc->map);
> + }
> +
> + rtc->irq = platform_get_irq(pdev, 0);
> + if (rtc->irq < 0)
> + return rtc->irq;
> +
> + rtc->sclk = devm_clk_get(&pdev->dev, "rtc_osc");
Clock name should be: "osc"
> + if (IS_ERR(rtc->sclk))
> + return PTR_ERR(rtc->sclk);
> + if (clk_get_rate(rtc->sclk) != OSC_32K && clk_get_rate(rtc->sclk) != OSC_24M) {
> + dev_err(&pdev->dev, "Invalid clock configuration\n");
> + return -EINVAL;
> + }
> +
> + core_clk = devm_clk_get(&pdev->dev, "rtc_sys_clk");
Clock name: "sys"
> + if (IS_ERR(core_clk)) {
> + dev_err(&pdev->dev, "failed to get rtc sys clk\n");
Syntax is return dev_err_probe.
> + return PTR_ERR(core_clk);
> + }
> + clk_prepare_enable(core_clk);
> +
> + aml_rtc_init(&pdev->dev, rtc);
> +
> + device_init_wakeup(&pdev->dev, 1);
> + platform_set_drvdata(pdev, rtc);
> +
> + rtc->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
> + if (IS_ERR(rtc->rtc_dev))
> + return PTR_ERR(rtc->rtc_dev);
> +
> + ret = devm_request_irq(&pdev->dev, rtc->irq, aml_rtc_handler,
> + IRQF_ONESHOT, "aml-rtc alarm", rtc);
> + if (ret) {
> + dev_err(&pdev->dev, "IRQ%d request failed, ret = %d\n",
> + rtc->irq, ret);
Your code is buggy. You leave with prepared clock.
Use devm_clk_get_enabled where applicable.
> + return ret;
> + }
> +
> + rtc->rtc_dev->ops = &aml_rtc_ops;
> + rtc->rtc_dev->range_min = 0;
> + rtc->rtc_dev->range_max = U32_MAX;
> +
> + ret = rtc_add_group(rtc->rtc_dev, &aml_rtc_sysfs_files);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed to create sysfs group: %d\n", ret);
> + return ret;
> + }
> +
> + return devm_rtc_register_device(rtc->rtc_dev);
> +}
> +
> +static int aml_rtc_suspend(struct device *dev)
> +{
> + struct aml_rtc_data *rtc = dev_get_drvdata(dev);
> +
> + if (device_may_wakeup(dev))
> + enable_irq_wake(rtc->irq);
> +
> + return 0;
> +}
> +
> +static int aml_rtc_resume(struct device *dev)
> +{
> + struct aml_rtc_data *rtc = dev_get_drvdata(dev);
> +
> + if (device_may_wakeup(dev))
> + disable_irq_wake(rtc->irq);
> +
> + return 0;
> +}
> +
Where is the remove to cleanup?
> +static SIMPLE_DEV_PM_OPS(aml_rtc_pm_ops,
> + aml_rtc_suspend, aml_rtc_resume);
> +
> +static const struct aml_rtc_config a5_rtc_config = {
> +};
> +
> +static const struct aml_rtc_config a4_rtc_config = {
> + .gray_stored = true,
> +};
> +
> +static const struct of_device_id aml_rtc_device_id[] = {
> + {
> + .compatible = "amlogic,a4-rtc",
> + .data = &a4_rtc_config,
> + },
> + {
> + .compatible = "amlogic,a5-rtc",
> + .data = &a5_rtc_config,
> + },
> +};
> +MODULE_DEVICE_TABLE(of, aml_rtc_device_id);
> +
> +static struct platform_driver aml_rtc_driver = {
> + .probe = aml_rtc_probe,
> + .driver = {
> + .name = "aml-rtc",
> + .of_match_table = of_match_ptr(aml_rtc_device_id),
Drop of_match_ptr. You have a warning here.
> + .pm = &aml_rtc_pm_ops,
> + },
Best regards,
Krzysztof
Powered by blists - more mailing lists