Re: [PATCH 2/3] rtc: support for the Amlogic on-chip RTC

From: Xianwei Zhao
Date: Mon Sep 02 2024 - 03:33:09 EST

Next message: Manivannan Sadhasivam: "Re: [PATCH 1/2] PCI: dra7xx: Fix threaded IRQ request for "dra7xx-pcie-main" IRQ"
Previous message: Chao Yu: "Re: [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL"
Next in thread: Xianwei Zhao: "Re: [PATCH 2/3] rtc: support for the Amlogic on-chip RTC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Krzysztof,
Thanks for your reply.

On 2024/8/26 16:27, Krzysztof Kozlowski wrote:

[ EXTERNAL EMAIL ]

On 23/08/2024 11:19, Xianwei Zhao via B4 Relay wrote:

From: Yiting Deng <yiting.deng@xxxxxxxxxxx>

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@xxxxxxxxxxx>
Signed-off-by: Xianwei Zhao <xianwei.zhao@xxxxxxxxxxx>
---
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?

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.
The "meson" string is meaningless, it just keeps going, and now the new hardware uses the normal naming.

+ 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.

Will do.

+ 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, &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.

I will delete 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?

I will move it to the standard RTC API to handle calibration.
So here will delete it.

+ 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.

I will move it to the standard RTC API to handle calibration.
So here will delete it.

+
+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, &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.

This is not used functions, I will delete it.

+ */
+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, &reg_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"

Will do.

+ 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"

Will do.

+ if (IS_ERR(core_clk)) {
+ dev_err(&pdev->dev, "failed to get rtc sys clk\n");

Syntax is return dev_err_probe.

Will do.

+ 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.

Will fix it.

+ 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?

Will add remove function.

+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.

Will do.

+ .pm = &aml_rtc_pm_ops,
+ },

Best regards,
Krzysztof

Next message: Manivannan Sadhasivam: "Re: [PATCH 1/2] PCI: dra7xx: Fix threaded IRQ request for "dra7xx-pcie-main" IRQ"
Previous message: Chao Yu: "Re: [RFC PATCH] f2fs: don't set SBI_QUOTA_NEED_REPAIR flag if receive SIGKILL"
Next in thread: Xianwei Zhao: "Re: [PATCH 2/3] rtc: support for the Amlogic on-chip RTC"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]