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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux