| Message ID | 20231226100431.331616-1-qiujingbao.dlmu@gmail.com |
|---|---|
| Headers | show |
| Series | riscv: rtc: sophgo: add mfd and rtc support for CV1800 | expand |
Hello, please run checkpatch.pl --strict, there are a few issues. On 26/12/2023 18:04:30+0800, Jingbao Qiu wrote: > +struct cv1800_rtc_priv { > + struct rtc_device *rtc_dev; > + struct device *dev; > + struct regmap *rtc_map; > + struct clk *clk; > + spinlock_t rtc_lock; This lock seems unnecessary, please check > + int irq; > +}; > + > +static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) > +{ > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > + > + if (enabled) > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN); > + else > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > + This could be: regmap_write(info->rtc_map, ALARM_ENABLE, enabled); > + return 0; > +} > + > +static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > + unsigned long alarm_time; > + > + alarm_time = rtc_tm_to_time64(&alrm->time); > + > + if (alarm_time > SEC_MAX_VAL) > + return -EINVAL; The core is already checking fr this. > + > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > + > + udelay(DEALY_TIME_PREPARE); Why is this needed? > + > + regmap_write(info->rtc_map, ALARM_TIME, alarm_time); > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN); You must follow alrm->enabled instead of unconditionally enabling the alarm. > + > + return 0; > +} > + > +static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info) Please explain those two calibration functions. I don't think you can achieve what you want to do. > +{ > + uint32_t calib_val = 0; > + uint32_t coarse_val = 0; > + uint32_t time_now = 0; > + uint32_t time_next = 0; > + uint32_t offset = CALIB_OFFSET_INIT; > + uint32_t coarse_timeout = REG_INIT_TIMEOUT; > + uint32_t get_val_timeout = 0; > + > + regmap_write(info->rtc_map, ANA_CALIB, CALIB_INIT_VAL); > + > + udelay(DEALY_TIME_PREPARE); > + > + /* Select 32K OSC tuning val source from rtc_sys */ > + regmap_update_bits(info->rtc_map, SEC_PULSE_GEN, > + (unsigned int)(~SEC_PULSE_GEN_SEL_MASK), > + (unsigned int)(~SEC_PULSE_SEL_INNER)); > + > + regmap_read(info->rtc_map, ANA_CALIB, &calib_val); > + > + regmap_write(info->rtc_map, FC_COARSE_EN, REG_ENABLE_FUN); > + > + while (--coarse_timeout) { > + regmap_read(info->rtc_map, FC_COARSE_CAL, &time_now); > + time_now >>= FC_COARSE_CAL_TIME_SHIFT; > + > + get_val_timeout = REG_INIT_TIMEOUT; > + > + while (time_next <= time_now && > + --get_val_timeout) { > + regmap_read(info->rtc_map, FC_COARSE_CAL, > + &time_next); > + time_next >>= FC_COARSE_CAL_TIME_SHIFT; > + udelay(DEALY_TIME_LOOP); > + } > + > + if (!get_val_timeout) > + return -1; > + > + udelay(DEALY_TIME_PREPARE); > + > + regmap_read(info->rtc_map, FC_COARSE_CAL, &coarse_val); > + coarse_val &= FC_COARSE_CAL_VAL_MASK; > + > + if (coarse_val > CALIB_FC_COARSE_PLUS_OFFSET) { > + calib_val += offset; > + offset >>= CALIB_OFFSET_SHIFT; > + regmap_write(info->rtc_map, ANA_CALIB, > + calib_val); > + } else if (coarse_val < CALIB_FC_COARSE_SUB_OFFSET) { > + calib_val -= offset; > + offset >>= CALIB_OFFSET_SHIFT; > + regmap_write(info->rtc_map, ANA_CALIB, > + calib_val); > + } else { > + regmap_write(info->rtc_map, FC_COARSE_EN, > + REG_DISABLE_FUN); > + break; > + } > + > + if (offset == 0) > + return -1; > + } > + > + return 0; > +} > + > +static int cv1800_rtc_32k_fine_val_calib(struct cv1800_rtc_priv *info) > +{ > + uint32_t val; > + uint64_t freq = CALIB_FREQ; > + uint32_t sec_cnt; > + uint32_t timeout = REG_INIT_TIMEOUT; > + uint32_t time_now = 0; > + uint32_t time_next = 0; > + > + regmap_write(info->rtc_map, FC_FINE_EN, REG_ENABLE_FUN); > + > + regmap_read(info->rtc_map, FC_FINE_CAL, &time_now); > + time_now >>= FC_FINE_CAL_TIME_SHIFT; > + > + while (time_next <= time_now && --timeout) { > + regmap_read(info->rtc_map, FC_FINE_CAL, &time_next); > + time_next >>= FC_FINE_CAL_TIME_SHIFT; > + udelay(DEALY_TIME_LOOP); > + } > + > + if (!timeout) > + return -1; > + > + regmap_read(info->rtc_map, FC_FINE_CAL, &val); > + val &= FC_FINE_CAL_VAL_MASK; > + > + do_div(freq, CALIB_FREQ_NS); > + freq = freq * CALIB_FRAC_EXT; > + do_div(freq, val); > + > + sec_cnt = ((do_div(freq, CALIB_FRAC_EXT) * CALIB_FREQ_MULT) / > + CALIB_FRAC_EXT & > + SEC_PULSE_GEN_INT_MASK) + > + (freq << SEC_PULSE_GEN_FRAC_SHIFT); > + > + regmap_write(info->rtc_map, SEC_PULSE_GEN, sec_cnt); > + regmap_write(info->rtc_map, FC_FINE_EN, REG_DISABLE_FUN); > + > + return 0; > +} > + > +static void rtc_enable_sec_counter(struct cv1800_rtc_priv *info) > +{ > + /* select inner sec pulse and select reg set calibration val */ > + regmap_update_bits(info->rtc_map, SEC_PULSE_GEN, > + (unsigned int)(~SEC_PULSE_GEN_SEL_MASK), > + (unsigned int)(~SEC_PULSE_SEL_INNER)); > + > + regmap_update_bits(info->rtc_map, ANA_CALIB, > + (unsigned int)(~CALIB_SEL_FTUNE_MASK), > + CALIB_SEL_FTUNE_INNER); > + > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); Don't disable alarms on probe. > +} > + > +static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > + unsigned int sec; > + unsigned int sec_ro_t; > + unsigned long flag; > + > + spin_lock_irqsave(&info->rtc_lock, flag); > + > + regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec); > + regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t); > + > + if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) { > + sec = sec_ro_t; > + regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec); > + regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN); What does this do? > + } > + > + spin_unlock_irqrestore(&info->rtc_lock, flag); > + > + rtc_time64_to_tm(sec, tm); > + > + return 0; > +} > + > +static int cv1800_rtc_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > + unsigned long sec; > + int ret; > + unsigned long flag; > + > + ret = rtc_valid_tm(tm); This is useless, tm will always be valid > + if (ret) > + return ret; > + > + sec = rtc_tm_to_time64(tm); > + > + spin_lock_irqsave(&info->rtc_lock, flag); > + > + regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec); > + regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN); > + > + regmap_write(info->rtc_map, MACRO_RG_SET_T, sec); > + > + spin_unlock_irqrestore(&info->rtc_lock, flag); > + > + return 0; > +} > + > +static irqreturn_t cv1800_rtc_irq_handler(int irq, void *dev_id) > +{ > + struct device *dev = dev_id; > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > + struct rtc_wkalrm alrm; > + > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > + > + rtc_read_alarm(info->rtc_dev, &alrm); > + alrm.enabled = 0; > + rtc_set_alarm(info->rtc_dev, &alrm); I don't get what you are doing here, you should call rtc_update_irq, not mess with alarms that have been set. > + > + return IRQ_HANDLED; > +} > + > +static const struct rtc_class_ops cv800b_rtc_ops = { > + .read_time = cv1800_rtc_read_time, > + .set_time = cv1800_rtc_set_time, > + .read_alarm = cv1800_rtc_read_alarm, > + .set_alarm = cv1800_rtc_set_alarm, > + .alarm_irq_enable = cv1800_rtc_alarm_irq_enable, > +}; > + > +static int cv1800_rtc_probe(struct platform_device *pdev) > +{ > + struct cv1800_rtc_priv *rtc; > + uint32_t ctrl_val; > + int ret; > + > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv), > + GFP_KERNEL); > + if (!rtc) > + return -ENOMEM; > + > + rtc->dev = &pdev->dev; > + > + rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent); > + if (IS_ERR(rtc->rtc_map)) > + return PTR_ERR(rtc->rtc_map); > + > + rtc->irq = platform_get_irq(pdev, 0); > + if (rtc->irq < 0) > + return rtc->irq; > + > + ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler, > + IRQF_TRIGGER_HIGH, "alarm", &pdev->dev); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "cannot register interrupt handler\n"); > + > + rtc->clk = devm_clk_get(rtc->dev, NULL); > + if (IS_ERR(rtc->clk)) > + return PTR_ERR(rtc->clk); > + You are going to leak rtc->clk after the next call. > + rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL); > + if (IS_ERR(rtc->clk)) > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk), > + "clk not found\n"); > + > + platform_set_drvdata(pdev, rtc); > + > + spin_lock_init(&rtc->rtc_lock); > + > + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, > + dev_name(&pdev->dev), > + &cv800b_rtc_ops, > + THIS_MODULE); > + if (IS_ERR(rtc->rtc_dev)) > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev), > + "can't register rtc device\n"); Please use devm_rtc_allocate_device and devm_rtc_register_device > + > + /* if use internal clk,so coarse calibrate rtc */ > + regmap_read(rtc->rtc_map, CTRL, &ctrl_val); > + ctrl_val &= CTRL_MODE_MASK; > + > + if (ctrl_val == CTRL_MODE_OSC32K) { > + ret = cv1800_rtc_32k_coarse_val_calib(rtc); > + if (ret) > + dev_err(&pdev->dev, "failed to coarse RTC val !\n"); > + > + ret = cv1800_rtc_32k_fine_val_calib(rtc); > + if (ret) > + dev_err(&pdev->dev, "failed to fine RTC val !\n"); > + } > + > + rtc_enable_sec_counter(rtc); I'm pretty sure you don't want to do that on every probe. > + > + return 0; > +} > + > +static const struct of_device_id cv1800_dt_ids[] = { > + { .compatible = "sophgo,cv1800b-rtc" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, cv1800_dt_ids); > + > +static struct platform_driver cv1800_rtc_driver = { > + .driver = { > + .name = "sophgo-cv800b-rtc", > + .of_match_table = cv1800_dt_ids, > + }, > + .probe = cv1800_rtc_probe, > +}; > + > +module_platform_driver(cv1800_rtc_driver); > +MODULE_AUTHOR("Jingbao Qiu"); > +MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver"); > +MODULE_LICENSE("GPL"); > -- > 2.25.1 >
On Tue, Dec 26, 2023 at 8:37 PM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > Hello, > > please run checkpatch.pl --strict, there are a few issues. > > On 26/12/2023 18:04:30+0800, Jingbao Qiu wrote: > > +struct cv1800_rtc_priv { > > + struct rtc_device *rtc_dev; > > + struct device *dev; > > + struct regmap *rtc_map; > > + struct clk *clk; > > + spinlock_t rtc_lock; > > This lock seems unnecessary, please check > you are right. I will fix it. > > + int irq; > > +}; > > + > > +static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) > > +{ > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > + > > + if (enabled) > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN); > > + else > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > > + > > This could be: > regmap_write(info->rtc_map, ALARM_ENABLE, enabled); you are right, i will fix it. > > > + return 0; > > +} > > + > > +static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > +{ > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > + unsigned long alarm_time; > > + > > + alarm_time = rtc_tm_to_time64(&alrm->time); > > + > > + if (alarm_time > SEC_MAX_VAL) > > + return -EINVAL; > > The core is already checking fr this. Thanks, I will remove it. > > > + > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > > + > > + udelay(DEALY_TIME_PREPARE); > > Why is this needed? This doesn't seem to require waiting, I will check it. > > > + > > + regmap_write(info->rtc_map, ALARM_TIME, alarm_time); > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN); > > You must follow alrm->enabled instead of unconditionally enabling the > alarm. ok,i will fix it. > > > + > > + return 0; > > +} > > + > > > > +static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info) > > Please explain those two calibration functions. I don't think you can > achieve what you want to do. The goal of these two calibration functions is to achieve calibration of RTC time. The code is written according to the data manual. The calibration circuit uses 25MHz crystal clock to sample 32KHz clock. In coarse tune mode, the 25MHz crystal clock samples one 32KHz clock cycle period and report the counting results. the datasheet link: Link: https://github.com/milkv-duo/duo-files/blob/main/duo/datasheet/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf page:195 > > > +{ > > + uint32_t calib_val = 0; > > + uint32_t coarse_val = 0; > > + uint32_t time_now = 0; > > + uint32_t time_next = 0; > > + uint32_t offset = CALIB_OFFSET_INIT; > > + uint32_t coarse_timeout = REG_INIT_TIMEOUT; > > + uint32_t get_val_timeout = 0; > > + > > + regmap_write(info->rtc_map, ANA_CALIB, CALIB_INIT_VAL); > > + > > + udelay(DEALY_TIME_PREPARE); > > + > > + /* Select 32K OSC tuning val source from rtc_sys */ > > + regmap_update_bits(info->rtc_map, SEC_PULSE_GEN, > > + (unsigned int)(~SEC_PULSE_GEN_SEL_MASK), > > + (unsigned int)(~SEC_PULSE_SEL_INNER)); > > + > > + regmap_read(info->rtc_map, ANA_CALIB, &calib_val); > > + > > + regmap_write(info->rtc_map, FC_COARSE_EN, REG_ENABLE_FUN); > > + > > + while (--coarse_timeout) { > > + regmap_read(info->rtc_map, FC_COARSE_CAL, &time_now); > > + time_now >>= FC_COARSE_CAL_TIME_SHIFT; > > + > > + get_val_timeout = REG_INIT_TIMEOUT; > > + > > + while (time_next <= time_now && > > + --get_val_timeout) { > > + regmap_read(info->rtc_map, FC_COARSE_CAL, > > + &time_next); > > + time_next >>= FC_COARSE_CAL_TIME_SHIFT; > > + udelay(DEALY_TIME_LOOP); > > + } > > + > > + if (!get_val_timeout) > > + return -1; > > + > > + udelay(DEALY_TIME_PREPARE); > > + > > + regmap_read(info->rtc_map, FC_COARSE_CAL, &coarse_val); > > + coarse_val &= FC_COARSE_CAL_VAL_MASK; > > + > > + if (coarse_val > CALIB_FC_COARSE_PLUS_OFFSET) { > > + calib_val += offset; > > + offset >>= CALIB_OFFSET_SHIFT; > > + regmap_write(info->rtc_map, ANA_CALIB, > > + calib_val); > > + } else if (coarse_val < CALIB_FC_COARSE_SUB_OFFSET) { > > + calib_val -= offset; > > + offset >>= CALIB_OFFSET_SHIFT; > > + regmap_write(info->rtc_map, ANA_CALIB, > > + calib_val); > > + } else { > > + regmap_write(info->rtc_map, FC_COARSE_EN, > > + REG_DISABLE_FUN); > > + break; > > + } > > + > > + if (offset == 0) > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > +static int cv1800_rtc_32k_fine_val_calib(struct cv1800_rtc_priv *info) > > +{ > > + uint32_t val; > > + uint64_t freq = CALIB_FREQ; > > + uint32_t sec_cnt; > > + uint32_t timeout = REG_INIT_TIMEOUT; > > + uint32_t time_now = 0; > > + uint32_t time_next = 0; > > + > > + regmap_write(info->rtc_map, FC_FINE_EN, REG_ENABLE_FUN); > > + > > + regmap_read(info->rtc_map, FC_FINE_CAL, &time_now); > > + time_now >>= FC_FINE_CAL_TIME_SHIFT; > > + > > + while (time_next <= time_now && --timeout) { > > + regmap_read(info->rtc_map, FC_FINE_CAL, &time_next); > > + time_next >>= FC_FINE_CAL_TIME_SHIFT; > > + udelay(DEALY_TIME_LOOP); > > + } > > + > > + if (!timeout) > > + return -1; > > + > > + regmap_read(info->rtc_map, FC_FINE_CAL, &val); > > + val &= FC_FINE_CAL_VAL_MASK; > > + > > + do_div(freq, CALIB_FREQ_NS); > > + freq = freq * CALIB_FRAC_EXT; > > + do_div(freq, val); > > + > > + sec_cnt = ((do_div(freq, CALIB_FRAC_EXT) * CALIB_FREQ_MULT) / > > + CALIB_FRAC_EXT & > > + SEC_PULSE_GEN_INT_MASK) + > > + (freq << SEC_PULSE_GEN_FRAC_SHIFT); > > + > > + regmap_write(info->rtc_map, SEC_PULSE_GEN, sec_cnt); > > + regmap_write(info->rtc_map, FC_FINE_EN, REG_DISABLE_FUN); > > + > > + return 0; > > +} > > + > > +static void rtc_enable_sec_counter(struct cv1800_rtc_priv *info) > > +{ > > + /* select inner sec pulse and select reg set calibration val */ > > + regmap_update_bits(info->rtc_map, SEC_PULSE_GEN, > > + (unsigned int)(~SEC_PULSE_GEN_SEL_MASK), > > + (unsigned int)(~SEC_PULSE_SEL_INNER)); > > + > > + regmap_update_bits(info->rtc_map, ANA_CALIB, > > + (unsigned int)(~CALIB_SEL_FTUNE_MASK), > > + CALIB_SEL_FTUNE_INNER); > > + > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > > Don't disable alarms on probe. thanks,I will fix it. > > > +} > > + > > +static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > + unsigned int sec; > > + unsigned int sec_ro_t; > > + unsigned long flag; > > + > > + spin_lock_irqsave(&info->rtc_lock, flag); > > + > > + regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec); > > + regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t); > > + > > + if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) { > > + sec = sec_ro_t; > > + regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec); > > + regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN); > > What does this do? the sec_ro_t be considered to have high accuracy after calibration. So every time read the time, update the RTC time. > > > + } > > + > > + spin_unlock_irqrestore(&info->rtc_lock, flag); > > + > > + rtc_time64_to_tm(sec, tm); > > + > > + return 0; > > +} > > + > > +static int cv1800_rtc_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > + unsigned long sec; > > + int ret; > > + unsigned long flag; > > + > > + ret = rtc_valid_tm(tm); > > This is useless, tm will always be valid ok,i will fix it. > > > + if (ret) > > + return ret; > > + > > + sec = rtc_tm_to_time64(tm); > > + > > + spin_lock_irqsave(&info->rtc_lock, flag); > > + > > + regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec); > > + regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN); > > + > > + regmap_write(info->rtc_map, MACRO_RG_SET_T, sec); > > + > > + spin_unlock_irqrestore(&info->rtc_lock, flag); > > + > > + return 0; > > +} > > + > > +static irqreturn_t cv1800_rtc_irq_handler(int irq, void *dev_id) > > +{ > > + struct device *dev = dev_id; > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > + struct rtc_wkalrm alrm; > > + > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > > + > > + rtc_read_alarm(info->rtc_dev, &alrm); > > + alrm.enabled = 0; > > + rtc_set_alarm(info->rtc_dev, &alrm); > > I don't get what you are doing here, you should call rtc_update_irq, not > mess with alarms that have been set. Thanks,I will fix it. > > > + > > + return IRQ_HANDLED; > > +} > > + > > +static const struct rtc_class_ops cv800b_rtc_ops = { > > + .read_time = cv1800_rtc_read_time, > > + .set_time = cv1800_rtc_set_time, > > + .read_alarm = cv1800_rtc_read_alarm, > > + .set_alarm = cv1800_rtc_set_alarm, > > + .alarm_irq_enable = cv1800_rtc_alarm_irq_enable, > > +}; > > + > > +static int cv1800_rtc_probe(struct platform_device *pdev) > > +{ > > + struct cv1800_rtc_priv *rtc; > > + uint32_t ctrl_val; > > + int ret; > > + > > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv), > > + GFP_KERNEL); > > + if (!rtc) > > + return -ENOMEM; > > + > > + rtc->dev = &pdev->dev; > > + > > + rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent); > > + if (IS_ERR(rtc->rtc_map)) > > + return PTR_ERR(rtc->rtc_map); > > + > > + rtc->irq = platform_get_irq(pdev, 0); > > + if (rtc->irq < 0) > > + return rtc->irq; > > + > > + ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler, > > + IRQF_TRIGGER_HIGH, "alarm", &pdev->dev); > > + if (ret) > > + return dev_err_probe(&pdev->dev, ret, > > + "cannot register interrupt handler\n"); > > + > > + rtc->clk = devm_clk_get(rtc->dev, NULL); > > + if (IS_ERR(rtc->clk)) > > + return PTR_ERR(rtc->clk); > > + > > You are going to leak rtc->clk after the next call. I will release him at the appropriate time. And add the remove function to release. > > > + rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL); > > + if (IS_ERR(rtc->clk)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk), > > + "clk not found\n"); > > + > > + platform_set_drvdata(pdev, rtc); > > + > > + spin_lock_init(&rtc->rtc_lock); > > + > > + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, > > + dev_name(&pdev->dev), > > + &cv800b_rtc_ops, > > + THIS_MODULE); > > + if (IS_ERR(rtc->rtc_dev)) > > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev), > > + "can't register rtc device\n"); > > Please use devm_rtc_allocate_device and devm_rtc_register_device ok,I will use it. > > > + > > + /* if use internal clk,so coarse calibrate rtc */ > > + regmap_read(rtc->rtc_map, CTRL, &ctrl_val); > > + ctrl_val &= CTRL_MODE_MASK; > > + > > + if (ctrl_val == CTRL_MODE_OSC32K) { > > + ret = cv1800_rtc_32k_coarse_val_calib(rtc); > > + if (ret) > > + dev_err(&pdev->dev, "failed to coarse RTC val !\n"); > > + > > + ret = cv1800_rtc_32k_fine_val_calib(rtc); > > + if (ret) > > + dev_err(&pdev->dev, "failed to fine RTC val !\n"); > > + } > > + > > + rtc_enable_sec_counter(rtc); > > I'm pretty sure you don't want to do that on every probe. you are right,i will fix it. > > > + > > + return 0; > > +} > > + > > +static const struct of_device_id cv1800_dt_ids[] = { > > + { .compatible = "sophgo,cv1800b-rtc" }, > > + { /* sentinel */ } > > +}; > > +MODULE_DEVICE_TABLE(of, cv1800_dt_ids); > > + > > +static struct platform_driver cv1800_rtc_driver = { > > + .driver = { > > + .name = "sophgo-cv800b-rtc", > > + .of_match_table = cv1800_dt_ids, > > + }, > > + .probe = cv1800_rtc_probe, > > +}; > > + > > +module_platform_driver(cv1800_rtc_driver); > > +MODULE_AUTHOR("Jingbao Qiu"); > > +MODULE_DESCRIPTION("Sophgo cv1800 RTC Driver"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.25.1 > > > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Best regards, Jingbao Qiu
On 27/12/2023 16:03:56+0800, Jingbao Qiu wrote: > On Tue, Dec 26, 2023 at 8:37 PM Alexandre Belloni > <alexandre.belloni@bootlin.com> wrote: > > > > Hello, > > > > please run checkpatch.pl --strict, there are a few issues. > > > > On 26/12/2023 18:04:30+0800, Jingbao Qiu wrote: > > > +struct cv1800_rtc_priv { > > > + struct rtc_device *rtc_dev; > > > + struct device *dev; > > > + struct regmap *rtc_map; > > > + struct clk *clk; > > > + spinlock_t rtc_lock; > > > > This lock seems unnecessary, please check > > > > you are right. I will fix it. > > > > + int irq; > > > +}; > > > + > > > +static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) > > > +{ > > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > > + > > > + if (enabled) > > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN); > > > + else > > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > > > + > > > > This could be: > > regmap_write(info->rtc_map, ALARM_ENABLE, enabled); > > you are right, i will fix it. > > > > > > + return 0; > > > +} > > > + > > > +static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > > +{ > > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > > + unsigned long alarm_time; > > > + > > > + alarm_time = rtc_tm_to_time64(&alrm->time); > > > + > > > + if (alarm_time > SEC_MAX_VAL) > > > + return -EINVAL; > > > > The core is already checking fr this. > > Thanks, I will remove it. > > > > > > + > > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > > > + > > > + udelay(DEALY_TIME_PREPARE); > > > > Why is this needed? > > This doesn't seem to require waiting, I will check it. > > > > > > + > > > + regmap_write(info->rtc_map, ALARM_TIME, alarm_time); > > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN); > > > > You must follow alrm->enabled instead of unconditionally enabling the > > alarm. > > ok,i will fix it. > > > > > > + > > > + return 0; > > > +} > > > + > > > > > > > +static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info) > > > > Please explain those two calibration functions. I don't think you can > > achieve what you want to do. > > The goal of these two calibration functions is to achieve calibration > of RTC time. > The code is written according to the data manual. > > The calibration circuit uses 25MHz crystal clock to sample 32KHz > clock. In coarse > tune mode, the 25MHz crystal clock samples one 32KHz clock cycle period and > report the counting results. > > the datasheet link: > Link: https://github.com/milkv-duo/duo-files/blob/main/duo/datasheet/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf > page:195 I'm really curious as to why this is calibrated using a 25MHz crystal as it may be as imprecise as the 32kHz one. I'm asking because we have an interface to get calibration done properly so you can use a precise clock like GPS, NTP or PTP. This is what you should probably implement instead or on top of it. > > > > > +} > > > + > > > +static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm) > > > +{ > > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > > + unsigned int sec; > > > + unsigned int sec_ro_t; > > > + unsigned long flag; > > > + > > > + spin_lock_irqsave(&info->rtc_lock, flag); > > > + > > > + regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec); > > > + regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t); > > > + > > > + if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) { > > > + sec = sec_ro_t; > > > + regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec); > > > + regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN); > > > > What does this do? > > the sec_ro_t be considered to have high accuracy after calibration. > So every time read the time, update the RTC time. So why don't you always use sec_ro_t instead of sec? Also, why is this done conditionally on a arbitrary value? As it stands, it will happen if the date is after 1995-07-09T16:12:48 for no good reason. This is awful because the alarm is matching SEC_CNTR_VAL with ALARM_TIME so if this means the calibration doesn't affect SEC_CNTR_VAL (which I seriously doubt), the alarm will end up being imprecise anyway > > > +static int cv1800_rtc_probe(struct platform_device *pdev) > > > +{ > > > + struct cv1800_rtc_priv *rtc; > > > + uint32_t ctrl_val; > > > + int ret; > > > + > > > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv), > > > + GFP_KERNEL); > > > + if (!rtc) > > > + return -ENOMEM; > > > + > > > + rtc->dev = &pdev->dev; > > > + > > > + rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent); > > > + if (IS_ERR(rtc->rtc_map)) > > > + return PTR_ERR(rtc->rtc_map); > > > + > > > + rtc->irq = platform_get_irq(pdev, 0); > > > + if (rtc->irq < 0) > > > + return rtc->irq; > > > + > > > + ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler, > > > + IRQF_TRIGGER_HIGH, "alarm", &pdev->dev); > > > + if (ret) > > > + return dev_err_probe(&pdev->dev, ret, > > > + "cannot register interrupt handler\n"); > > > + > > > + rtc->clk = devm_clk_get(rtc->dev, NULL); > > > + if (IS_ERR(rtc->clk)) > > > + return PTR_ERR(rtc->clk); > > > + > > > > You are going to leak rtc->clk after the next call. > > I will release him at the appropriate time. And add the remove > function to release. > > > > > > + rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL); > > > + if (IS_ERR(rtc->clk)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk), > > > + "clk not found\n"); > > > + > > > + platform_set_drvdata(pdev, rtc); > > > + > > > + spin_lock_init(&rtc->rtc_lock); > > > + > > > + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, > > > + dev_name(&pdev->dev), > > > + &cv800b_rtc_ops, > > > + THIS_MODULE); > > > + if (IS_ERR(rtc->rtc_dev)) > > > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev), > > > + "can't register rtc device\n"); > > > > Please use devm_rtc_allocate_device and devm_rtc_register_device > > ok,I will use it. Also you have to set the RTC range properly.
On Wed, Dec 27, 2023 at 9:50 PM Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > On 27/12/2023 16:03:56+0800, Jingbao Qiu wrote: > > On Tue, Dec 26, 2023 at 8:37 PM Alexandre Belloni > > <alexandre.belloni@bootlin.com> wrote: > > > > > > Hello, > > > > > > please run checkpatch.pl --strict, there are a few issues. > > > > > > On 26/12/2023 18:04:30+0800, Jingbao Qiu wrote: > > > > +struct cv1800_rtc_priv { > > > > + struct rtc_device *rtc_dev; > > > > + struct device *dev; > > > > + struct regmap *rtc_map; > > > > + struct clk *clk; > > > > + spinlock_t rtc_lock; > > > > > > This lock seems unnecessary, please check > > > > > > > you are right. I will fix it. > > > > > > + int irq; > > > > +}; > > > > + > > > > +static int cv1800_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) > > > > +{ > > > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > > > + > > > > + if (enabled) > > > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN); > > > > + else > > > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > > > > + > > > > > > This could be: > > > regmap_write(info->rtc_map, ALARM_ENABLE, enabled); > > > > you are right, i will fix it. > > > > > > > > > + return 0; > > > > +} > > > > + > > > > +static int cv1800_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > > > +{ > > > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > > > + unsigned long alarm_time; > > > > + > > > > + alarm_time = rtc_tm_to_time64(&alrm->time); > > > > + > > > > + if (alarm_time > SEC_MAX_VAL) > > > > + return -EINVAL; > > > > > > The core is already checking fr this. > > > > Thanks, I will remove it. > > > > > > > > > + > > > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_DISABLE_FUN); > > > > + > > > > + udelay(DEALY_TIME_PREPARE); > > > > > > Why is this needed? > > > > This doesn't seem to require waiting, I will check it. > > > > > > > > > + > > > > + regmap_write(info->rtc_map, ALARM_TIME, alarm_time); > > > > + regmap_write(info->rtc_map, ALARM_ENABLE, REG_ENABLE_FUN); > > > > > > You must follow alrm->enabled instead of unconditionally enabling the > > > alarm. > > > > ok,i will fix it. > > > > > > > > > + > > > > + return 0; > > > > +} > > > > + > > > > > > > > > > +static int cv1800_rtc_32k_coarse_val_calib(struct cv1800_rtc_priv *info) > > > > > > Please explain those two calibration functions. I don't think you can > > > achieve what you want to do. > > > > The goal of these two calibration functions is to achieve calibration > > of RTC time. > > The code is written according to the data manual. > > > > The calibration circuit uses 25MHz crystal clock to sample 32KHz > > clock. In coarse > > tune mode, the 25MHz crystal clock samples one 32KHz clock cycle period and > > report the counting results. > > > > the datasheet link: > > Link: https://github.com/milkv-duo/duo-files/blob/main/duo/datasheet/CV1800B-CV1801B-Preliminary-Datasheet-full-en.pdf > > page:195 > > > I'm really curious as to why this is calibrated using a 25MHz crystal as > it may be as imprecise as the 32kHz one. I'm asking because we have an > interface to get calibration done properly so you can use a precise clock > like GPS, NTP or PTP. This is what you should probably implement > instead or on top of it. > Thanks, I will communicate with the IC designers later. > > > > > > > +} > > > > + > > > > +static int cv1800_rtc_read_time(struct device *dev, struct rtc_time *tm) > > > > +{ > > > > + struct cv1800_rtc_priv *info = dev_get_drvdata(dev); > > > > + unsigned int sec; > > > > + unsigned int sec_ro_t; > > > > + unsigned long flag; > > > > + > > > > + spin_lock_irqsave(&info->rtc_lock, flag); > > > > + > > > > + regmap_read(info->rtc_map, SEC_CNTR_VAL, &sec); > > > > + regmap_read(info->rtc_map, MACRO_RO_T, &sec_ro_t); > > > > + > > > > + if (sec_ro_t > (SET_SEC_CNTR_VAL_UPDATE)) { > > > > + sec = sec_ro_t; > > > > + regmap_write(info->rtc_map, SET_SEC_CNTR_VAL, sec); > > > > + regmap_write(info->rtc_map, SET_SEC_CNTR_TRIG, REG_ENABLE_FUN); > > > > > > What does this do? > > > > the sec_ro_t be considered to have high accuracy after calibration. > > So every time read the time, update the RTC time. > > So why don't you always use sec_ro_t instead of sec? > Also, why is this done conditionally on a arbitrary value? As it stands, > it will happen if the date is after 1995-07-09T16:12:48 for no good > reason. > This is awful because the alarm is matching SEC_CNTR_VAL with ALARM_TIME > so if this means the calibration doesn't affect SEC_CNTR_VAL (which I > seriously doubt), the alarm will end up being imprecise anyway > Thanks, I will communicate with the IC designers later. > > > > +static int cv1800_rtc_probe(struct platform_device *pdev) > > > > +{ > > > > + struct cv1800_rtc_priv *rtc; > > > > + uint32_t ctrl_val; > > > > + int ret; > > > > + > > > > + rtc = devm_kzalloc(&pdev->dev, sizeof(struct cv1800_rtc_priv), > > > > + GFP_KERNEL); > > > > + if (!rtc) > > > > + return -ENOMEM; > > > > + > > > > + rtc->dev = &pdev->dev; > > > > + > > > > + rtc->rtc_map = syscon_node_to_regmap(rtc->dev->of_node->parent); > > > > + if (IS_ERR(rtc->rtc_map)) > > > > + return PTR_ERR(rtc->rtc_map); > > > > + > > > > + rtc->irq = platform_get_irq(pdev, 0); > > > > + if (rtc->irq < 0) > > > > + return rtc->irq; > > > > + > > > > + ret = devm_request_irq(&pdev->dev, rtc->irq, cv1800_rtc_irq_handler, > > > > + IRQF_TRIGGER_HIGH, "alarm", &pdev->dev); > > > > + if (ret) > > > > + return dev_err_probe(&pdev->dev, ret, > > > > + "cannot register interrupt handler\n"); > > > > + > > > > + rtc->clk = devm_clk_get(rtc->dev, NULL); > > > > + if (IS_ERR(rtc->clk)) > > > > + return PTR_ERR(rtc->clk); > > > > + > > > > > > You are going to leak rtc->clk after the next call. > > > > I will release him at the appropriate time. And add the remove > > function to release. > > > > > > > > > + rtc->clk = devm_clk_get_enabled(&pdev->dev, NULL); > > > > + if (IS_ERR(rtc->clk)) > > > > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk), > > > > + "clk not found\n"); > > > > + > > > > + platform_set_drvdata(pdev, rtc); > > > > + > > > > + spin_lock_init(&rtc->rtc_lock); > > > > + > > > > + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, > > > > + dev_name(&pdev->dev), > > > > + &cv800b_rtc_ops, > > > > + THIS_MODULE); > > > > + if (IS_ERR(rtc->rtc_dev)) > > > > + return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtc_dev), > > > > + "can't register rtc device\n"); > > > > > > Please use devm_rtc_allocate_device and devm_rtc_register_device > > > > ok,I will use it. > > Also you have to set the RTC range properly. Thanks,I will fix it. Best regards, Jingbao Qiu > > > -- > Alexandre Belloni, co-owner and COO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
This patch serises are to add rtc driver and providers by writing and reading syscon registers for the CV1800 RISC-V SoC. And add documentation and nodes to describe System Controller(syscon). This patch series depends on the clk patch support for Sophgo CV1800 SoC patch series which can be found at below link: https://lore.kernel.org/all/IA1PR20MB495354167CE560FC18E28DC5BB90A@IA1PR20MB4953.namprd20.prod.outlook.com/ Changes since v2: - add mfd support for CV1800 - add rtc to mfd - using regmap replace iomap - merge register address in dts v2: https://lore.kernel.org/lkml/20231217110952.78784-1-qiujingbao.dlmu@gmail.com/ Changes since v1 - fix duplicate names in subject - using RTC replace RTC controller - improve the properties of dt-bindings - using `unevaluatedProperties` replace `additionalProperties` - dt-bindings passed the test - using `devm_platform_ioremap_resource()` replace `platform_get_resource()` and `devm_ioremap_resource()` - fix random order of the code - fix wrong wrapping of the `devm_request_irq()` and map the flag with dts - using devm_clk_get_enabled replace `devm_clk_get()` and `clk_prepare_enable()` - fix return style - add rtc clock calibration function - use spinlock when write register on read/set time v1: https://lore.kernel.org/lkml/20231121094642.2973795-1-qiujingbao.dlmu@gmail.com/ Jingbao Qiu (4): dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC dt-bindings: rtc: sophgo: add RTC support for Sophgo CV1800 series SoC rtc: sophgo: add rtc support for Sophgo CV1800 SoC riscv: dts: sophgo: add rtc dt node for CV1800 .../bindings/mfd/sophgo,cv1800-subsys.yaml | 51 +++ .../bindings/rtc/sophgo,cv1800-rtc.yaml | 45 ++ arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 11 + drivers/rtc/Kconfig | 6 + drivers/rtc/Makefile | 1 + drivers/rtc/rtc-cv1800.c | 417 ++++++++++++++++++ 6 files changed, 531 insertions(+) create mode 100644 Documentation/devicetree/bindings/mfd/sophgo,cv1800-subsys.yaml create mode 100644 Documentation/devicetree/bindings/rtc/sophgo,cv1800-rtc.yaml create mode 100644 drivers/rtc/rtc-cv1800.c base-commit: dc0684adf3b6be6b20fec9295027980021d30353