[ck]Re: 回复： Re: [PATCH openAnolis v1 2/4] PHYTIUM: rtc: new driver: rtc-phytium add phytium rtc device support

26 Mar 2021

这个patch是这次我们帮你们合入openAnolis中去，我们在合入的过程中发了了这些问题，我们是希望你们后期能够

修复或者回复一些疑惑的问题，目前its、rtc我们已经合入到内核里面去了，你发的这些疑问的地方是需要你们那边

解决的，如果你发现你review的需要修改，可以直接在最新的openAnolis代码中修改后，把patch发出，我们这边直接合入。

在 2021/3/26 上午12:45, 郭克 写道:
...

 ------------------------------------------------------------------
 发件人：行信 &lt;xingxin.hx(a)openanolis.org&gt;
 发送时间：2021年3月22日(星期一) 15:37
 收件人：cloud-kernel &lt;cloud-kernel(a)lists.openanolis.org&gt;
 主　题：[ck] Re: [PATCH openAnolis v1 2/4] PHYTIUM: rtc: new driver: 
 rtc-phytium add phytium rtc device support

 在 2021/3/22 上午10:28, Baolin Wang 写道:

 The subject line should be:
 PHYTIUM: rtc: Add Phytium rtc device support
  done

 在 2021/3/20 18:30, Xin Hao 写道:
  Add phytium ft25000 platform rtc driver.

 Signed-off-by: Xin Hao &lt;xingxin.hx(a)openanolis.org&gt;
 ---
   drivers/i2c/busses/i2c-designware-platdrv.c |   1 +
   drivers/rtc/Kconfig                         |  10 +
   drivers/rtc/Makefile                        |   1 +
   drivers/rtc/rtc-phytium.c                   | 316 ++++++++++++++++++++
   4 files changed, 328 insertions(+)
   create mode 100644 drivers/rtc/rtc-phytium.c

 diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
 b/drivers/i2c/busses/i2c-designware-platdrv.c
 index b5750fd85125..3818b7a0a847 100644
 --- a/drivers/i2c/busses/i2c-designware-platdrv.c
 +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
 @@ -151,6 +151,7 @@ static const struct acpi_device_id 
 dw_i2c_acpi_match[] = {
       { "APMC0D0F", 0 },
       { "HISI02A1", 0 },
       { "HISI02A2", 0 },
 +    { "PHYT0003", 0 },
       { }
   };
   MODULE_DEVICE_TABLE(acpi, dw_i2c_acpi_match);
 diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
 index 7d7be60a2413..3e4effbf31ef 100644
 --- a/drivers/rtc/Kconfig
 +++ b/drivers/rtc/Kconfig
 @@ -1793,6 +1793,16 @@ config RTC_DRV_RTD119X
         If you say yes here, you get support for the RTD1295 SoC
         Real Time Clock.
   +config RTC_DRV_PHYTIUM
 +    tristate "Phytium RTC"
 +    depends on ARCH_PHYTIUM
 +    default y if ARCH_PHYTIUM
 +    help
 +      Say yes here to support the Phytium SoC real time clock.
 +
 +      This driver can also be built as a module, if so, the module
 +      will be called "rtc-phytium".
 +
   comment "HID Sensor RTC drivers"
     config RTC_DRV_HID_SENSOR_TIME
 diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
 index 5ff2fc0c361a..289447233c6b 100644
 --- a/drivers/rtc/Makefile
 +++ b/drivers/rtc/Makefile
 @@ -120,6 +120,7 @@ obj-$(CONFIG_RTC_DRV_PCF85363)    += rtc-pcf85363.o
   obj-$(CONFIG_RTC_DRV_PCF8523)    += rtc-pcf8523.o
   obj-$(CONFIG_RTC_DRV_PCF8563)    += rtc-pcf8563.o
   obj-$(CONFIG_RTC_DRV_PCF8583)    += rtc-pcf8583.o
 +obj-$(CONFIG_RTC_DRV_PHYTIUM)    += rtc-phytium.o
   obj-$(CONFIG_RTC_DRV_PIC32)    += rtc-pic32.o
   obj-$(CONFIG_RTC_DRV_PL030)    += rtc-pl030.o
   obj-$(CONFIG_RTC_DRV_PL031)    += rtc-pl031.o
 diff --git a/drivers/rtc/rtc-phytium.c b/drivers/rtc/rtc-phytium.c
 new file mode 100644
 index 000000000000..a9b5f52a9e8f
 --- /dev/null
 +++ b/drivers/rtc/rtc-phytium.c
 @@ -0,0 +1,316 @@
 +/*
 + * Phytium Real Time Clock Driver
 + *
 + * Copyright (c) 2019, Phytium Technology Co., Ltd.
 + *
 + * Chen Baozi &lt;chenbaozi(a)phytium.com.cn&gt;
 + *
 + * This program is free software; you can redistribute  it and/or 
 modify it
 + * under  the terms of  the GNU General  Public License as published 
 by the
 + * Free Software Foundation;  either version 2 of the  License, or 
 (at your
 + * option) any later version.
 + *
 + */
 +
 +#include <linux/init.h>
 +#include <linux/module.h>
 +#include <linux/of.h>
 +#include <linux/platform_device.h>
 +#include <linux/io.h>
 +#include <linux/clk.h>
 +#include <linux/delay.h>

 Which API used this header file? Please remove it if unused.
  done

  +#include <linux/acpi.h>
 +/* registers and masks need comments  or No one knows what these are*/
 +#define RTC_CMR                 0x04
 +#define RTC_AES_SEL             0x08
 +#define RTC_AES_SEL_COUNTER     0x100
 +#define RTC_CCR                 0x0C
 +#define RTC_CCR_IE              BIT(0)
 +#define RTC_CCR_MASK            BIT(1)
 +#define RTC_CCR_EN              BIT(2)
 +#define RTC_CCR_WEN             BIT(3)
 +#define RTC_STAT                0x10
 +#define RTC_STAT_BIT            BIT(0)
 +#define RTC_RSTAT               0x14
 +#define RTC_EOI                 0x18
 +#define RTC_VER                 0x1C
 +#define RTC_CDR_LOW             0x20
 +#define RTC_CCVR                0x24
 +#define RTC_CLR_LOW             0x28
 +#define RTC_CLR                 0x2c
 +#define RTC_COUNTER_HB_OFFSET   15
 +#define RTC_COUNTER_LB_MASK     GENMASK(14, 0)
 +
 +struct phytium_rtc_dev {
 +    struct rtc_device *rtc;
 +    struct device *dev;
 +    unsigned long alarm_time;
 +    void __iomem *csr_base;
 +    struct clk *clk;
 +    unsigned int irq_wake;
 +    unsigned int irq_enabled;
 +};
 +
 +static int phytium_rtc_read_time(struct device *dev, struct rtc_time 
 *tm)
 +{
 +    struct phytium_rtc_dev *pdata = dev_get_drvdata(dev);
 +

 Remove redundant blank line.
  done

 +    unsigned long counter = 0;
 +    unsigned long tmp = 0;

 No need to initialize them.

  +
 +    writel(RTC_AES_SEL_COUNTER, pdata->csr_base + RTC_AES_SEL);
 +    counter = readl(pdata->csr_base + RTC_CCVR); 
 +    tmp = readl(pdata->csr_base + RTC_CDR_LOW); //why? I am confusing

 Can you add some comments to explain why we need read RTC_CDR_LOW 
 register, but did not use the return value?
  the PHYTIUM engineer can explain it ? thanks

  +
 +    rtc_time_to_tm(counter, tm);
 +    return rtc_valid_tm(tm);
 +}
 +
 +static int phytium_rtc_set_mmss(struct device *dev, unsigned long secs)
 +{
 +    struct phytium_rtc_dev *pdata = dev_get_drvdata(dev);
 +    unsigned long counter = 0;
 +    unsigned long tmp = 0;

 Ditto

> +
> +    writel(RTC_AES_SEL_COUNTER, pdata->csr_base + RTC_AES_SEL);
> +    writel(0x00000000, pdata->csr_base + RTC_CLR_LOW);
> +    writel((u32)secs, pdata->csr_base + RTC_CLR);
> +    writel(RTC_AES_SEL_COUNTER, pdata->csr_base + RTC_AES_SEL);
> +    counter = readl(pdata->csr_base + RTC_CLR); // what does these mean?
> +    tmp = readl(pdata->csr_base + RTC_CLR_LOW);
> +
> +    return 0;
> +}
> +
> +static int phytium_rtc_read_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
> +{
> +    struct phytium_rtc_dev *pdata = dev_get_drvdata(dev);
> +
> +    rtc_time_to_tm(pdata->alarm_time, &alrm->time);
> +    alrm->enabled = readl(pdata->csr_base + RTC_CCR) & RTC_CCR_IE;
> +
> +    return 0;
> +}
> +
> +static int phytium_rtc_alarm_irq_enable(struct device *dev, u32 
> enabled)
> +{
> +    struct phytium_rtc_dev *pdata = dev_get_drvdata(dev);
> +    u32 ccr;
> +
> +    ccr = readl(pdata->csr_base + RTC_CCR);
> +    if (enabled) {
> +        ccr &= ~RTC_CCR_MASK;
> +        ccr |= RTC_CCR_IE;
> +    } else {
> +        ccr &= ~RTC_CCR_IE;
> +        ccr |= RTC_CCR_MASK;
> +    }
> +    writel(ccr, pdata->csr_base + RTC_CCR);
> +
> +    return 0;
> +}
> + // it is very stange here. I think you made a mistake.
> +static int phytium_rtc_alarm_irq_enabled(struct device *dev)
> +{
> +    struct phytium_rtc_dev *pdata = dev_get_drvdata(dev);
> +
> +    return readl(pdata->csr_base + RTC_CCR) & RTC_CCR_IE ? 1 : 0;
> +}
> +
> +static int phytium_rtc_set_alarm(struct device *dev, struct 
> rtc_wkalrm *alrm)
> +{
> +    struct phytium_rtc_dev *pdata = dev_get_drvdata(dev);
> +    unsigned long alarm_time; //the variable is never read in the 
  other functions, why not to put it in private data instead of as a 
 local variable
   +
 +    rtc_tm_to_time(&alrm->time, &alarm_time);
 +
 +    pdata->alarm_time = alarm_time;
 +    writel((u32) pdata->alarm_time, pdata->csr_base + RTC_CMR);
 +
 +    phytium_rtc_alarm_irq_enable(dev, alrm->enabled);
 +
 +    return 0;
 +}
 +
 +static const struct rtc_class_ops phytium_rtc_ops = {
 +    .read_time    = phytium_rtc_read_time,
 +    .set_mmss    = phytium_rtc_set_mmss,

 I still think we should change to implement the 'set_time' ops, since 
 the set_mmss is deprecated in CK 5.10 and mainline kernel.
  no change

> +    .read_alarm    = phytium_rtc_read_alarm,
> +    .set_alarm    = phytium_rtc_set_alarm,
> +    .alarm_irq_enable = phytium_rtc_alarm_irq_enable,
> +};
> + /* the error handling is really ugly, you should handle these 
  error at the end of the function and you miss some resources 。I advise 
 u to learn to use goto to handle errors*/

> +static irqreturn_t phytium_rtc_interrupt(int irq, void *id)
> +{
> +    struct phytium_rtc_dev *pdata = (struct phytium_rtc_dev *) id;
> +
> +    /* Check if interrupt asserted */
> +    if (!(readl(pdata->csr_base + RTC_STAT) & RTC_STAT_BIT))
> +        return IRQ_NONE;
> +
> +    /* Clear interrupt */
> +    readl(pdata->csr_base + RTC_EOI);
> +
> +    rtc_update_irq(pdata->rtc, 1, RTC_IRQF | RTC_AF);
> +
> +    return IRQ_HANDLED;
> +}
> +
> +static int phytium_rtc_probe(struct platform_device *pdev)
> +{
> +    struct phytium_rtc_dev *pdata;
> +    struct resource *res;
> +    int ret;
> +    int irq;
> +
> +    pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +    if (!pdata)
> +        return -ENOMEM;
> +    platform_set_drvdata(pdev, pdata);
> +    pdata->dev = &pdev->dev;
> +
> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +    pdata->csr_base = devm_ioremap_resource(&pdev->dev, res);
> +    if (IS_ERR(pdata->csr_base))
> +        return PTR_ERR(pdata->csr_base);
> +
> +    irq = platform_get_irq(pdev, 0);
> +    if (irq < 0) {
> +        dev_err(&pdev->dev, "No IRQ resource\n");
> +        return irq;
> +    }
> +    ret = devm_request_irq(&pdev->dev, irq, phytium_rtc_interrupt, 0,
> +                   dev_name(&pdev->dev), pdata);
> +    if (ret) {
> +        dev_err(&pdev->dev, "Could not request IRQ\n");
> +        return ret;
> +    }
> +
> +#ifndef CONFIG_ACPI
> +    pdata->clk = devm_clk_get(&pdev->dev, NULL);
> +    if (IS_ERR(pdata->clk)) {
> +        dev_err(&pdev->dev, "Couldn't get the clock for RTC\n");
> +        return -ENODEV;
> +    }
> +
> +    ret = clk_prepare_enable(pdata->clk);
> +    if (ret)
> +        return ret;
> +#endif
> +
> +    /* Turn on the clock and the crystal */
> +    writel(RTC_CCR_EN, pdata->csr_base + RTC_CCR);
> +
> +    ret = device_init_wakeup(&pdev->dev, 1);
> +    if (ret) {
> +        clk_disable_unprepare(pdata->clk);
> +        return ret;
> +    }
> +
> +    pdata->rtc = devm_rtc_device_register(&pdev->dev, pdev->name,
> +                     &phytium_rtc_ops, THIS_MODULE);
> +    if (IS_ERR(pdata->rtc)) {
> +        clk_disable_unprepare(pdata->clk);
> +        return PTR_ERR(pdata->rtc);
> +    }
> +
> +    /* HW does not support update faster than 1 secondsthis variable is never used
*/
> +    pdata->rtc->uie_unsupported = 1;
> +
> +    return 0;
> +}
> +
> +static int phytium_rtc_remove(struct platform_device *pdev)
> +{
> +    struct phytium_rtc_dev *pdata = platform_get_drvdata(pdev);
> + //why enable this irq hers?
> +    phytium_rtc_alarm_irq_enable(&pdev->dev, 0);
> +    device_init_wakeup(&pdev->dev, 0);
> +    clk_disable_unprepare(pdata->clk);
> +    return 0;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int phytium_rtc_suspend(struct device *dev)
> +{ //the same as the phytium_rtc_resume, all the drive private 
  information should be got at probe time and stored for later use

> +    struct platform_device *pdev = to_platform_device(dev);
> +    struct phytium_rtc_dev *pdata = platform_get_drvdata(pdev);
> +    int irq;
> +
> +    /*
> +     * If this RTC alarm will be used for waking the system up,
> +     * don't disable it of course. Else we just disable the alarm
> +     * and await suspension.
> +     */
> +    irq = platform_get_irq(pdev, 0);
> +    if (device_may_wakeup(&pdev->dev)) {
> +        if (!enable_irq_wake(irq))
> +            pdata->irq_wake = 1;
> +    } else {
> +        pdata->irq_enabled = phytium_rtc_alarm_irq_enabled(dev);
> +        phytium_rtc_alarm_irq_enable(dev, 0);
> +        clk_disable_unprepare(pdata->clk);
> +    }
> +
> +    return 0;
> +}
> +
> +static int phytium_rtc_resume(struct device *dev)
> +{
> +    struct platform_device *pdev = to_platform_device(dev);
> +    struct phytium_rtc_dev *pdata = platform_get_drvdata(pdev);
> +    int irq;
> +    int rc;
> + //This irq should be got this irq in probe procedure and be 
  stored in your driver private data.
 > +    irq = platform_get_irq(pdev, 0);
> +    if (device_may_wakeup(&pdev->dev)) {
> +        if (pdata->irq_wake) {
> +            disable_irq_wake(irq);
> +            pdata->irq_wake = 0;
> +        }
> +    } else {
  //This clock is the same as above.

 +        rc = clk_prepare_enable(pdata->clk);
 +        if (rc) {
 +            dev_err(dev, "Unable to enable clock error %d\n", rc);
 +            return rc;
 +        }
 +        phytium_rtc_alarm_irq_enable(dev, pdata->irq_enabled);
 +    }
 +
 +    return 0;
 +}
 +#endif
 +
 +static SIMPLE_DEV_PM_OPS(phytium_rtc_pm_ops, phytium_rtc_suspend, 
 phytium_rtc_resume);
 +
 +#ifdef CONFIG_OF
 +static const struct of_device_id phytium_rtc_of_match[] = {
 +    { .compatible = "phytium,rtc" },
 +    { }
 +};
 +MODULE_DEVICE_TABLE(of, phytium_rtc_of_match);
 +#endif
 +
 +#ifdef CONFIG_ACPI
 +static const struct acpi_device_id phytium_rtc_acpi_match[] = {
 +    { "PHYT0002", 0 },
 +    { }
 +};
 +#endif
 +
 +static struct platform_driver phytium_rtc_driver = {
 +    .probe        = phytium_rtc_probe,
 +    .remove        = phytium_rtc_remove,
 +    .driver        = {
 +        .name    = "phytium-rtc",
 +        .pm = &phytium_rtc_pm_ops,
 +        .of_match_table    = of_match_ptr(phytium_rtc_of_match),
 +        .acpi_match_table = ACPI_PTR(phytium_rtc_acpi_match),
 +    },
 +};
 +
 +module_platform_driver(phytium_rtc_driver);
 +
 +MODULE_DESCRIPTION("Phytium RTC driver");
 +MODULE_AUTHOR("Chen Baozi &lt;chenbaozi(a)phytium.com.cn&gt;&quot;)>");
 +MODULE_LICENSE("GPL");

  _______________________________________________
 Cloud Kernel mailing list -- cloud-kernel(a)lists.openanolis.org
 To unsubscribe send an email to cloud-kernel-leave(a)lists.openanolis.org

 -- 
 Best Regards!

 _______________________________________________
 Cloud Kernel mailing list -- cloud-kernel(a)lists.openanolis.org
 To unsubscribe send an email to cloud-kernel-leave(a)lists.openanolis.org

 -- 
Best Regards!

2024

2023

2022

2021

[ck]Re: 回复： Re: [PATCH openAnolis v1 2/4] PHYTIUM: rtc: new driver: rtc-phytium add phytium rtc device support