Message-ID: <e9ebbc1b-0d27-495b-9350-f635ed6ddfa3@amlogic.com>
Date: Mon, 2 Sep 2024 15:32:45 +0800
From: Xianwei Zhao <xianwei.zhao@...ogic.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 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

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@...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?
> 

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
> 

Powered by blists - more mailing lists