All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jingbao Qiu <qiujingbao.dlmu@gmail.com>
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: a.zummo@towertech.it, robh+dt@kernel.org,
	 krzysztof.kozlowski+dt@linaro.org, conor@kernel.org,
	conor+dt@kernel.org,  chao.wei@sophgo.com,
	unicorn_wang@outlook.com, paul.walmsley@sifive.com,
	 palmer@dabbelt.com, aou@eecs.berkeley.edu,
	linux-rtc@vger.kernel.org,  devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, dlan@gentoo.org,
	 inochiama@outlook.com
Subject: Re: [PATCH v3 3/4] rtc: sophgo: add rtc support for Sophgo CV1800 SoC
Date: Thu, 28 Dec 2023 12:12:25 +0800	[thread overview]
Message-ID: <CAJRtX8Qehq-E+Z9T+fc6r36dtF8ZMNwyqxF3WGOQAmwHZHgLkw@mail.gmail.com> (raw)
In-Reply-To: <202312271350242a208426@mail.local>

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

  reply	other threads:[~2023-12-28  4:12 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-26 10:04 [PATCH v3 0/4] riscv: rtc: sophgo: add mfd and rtc support for CV1800 Jingbao Qiu
2023-12-26 10:04 ` [PATCH v3 1/4] dt-bindings: mfd: sophgo: add MFD subsys support for Sophgo CV1800 series SoC Jingbao Qiu
2023-12-26 11:38   ` Rob Herring
2023-12-27  7:05     ` Jingbao Qiu
2023-12-26 12:18   ` Krzysztof Kozlowski
2023-12-27  7:35     ` Jingbao Qiu
2023-12-27 11:37       ` Krzysztof Kozlowski
2023-12-27 11:41         ` Jingbao Qiu
2023-12-26 12:21   ` Krzysztof Kozlowski
2023-12-27  7:37     ` Jingbao Qiu
2023-12-26 10:04 ` [PATCH v3 2/4] dt-bindings: rtc: sophgo: add RTC " Jingbao Qiu
2023-12-26 11:38   ` Rob Herring
2023-12-26 10:04 ` [PATCH v3 3/4] rtc: sophgo: add rtc support for Sophgo CV1800 SoC Jingbao Qiu
2023-12-26 12:37   ` Alexandre Belloni
2023-12-27  8:03     ` Jingbao Qiu
2023-12-27 13:50       ` Alexandre Belloni
2023-12-28  4:12         ` Jingbao Qiu [this message]
2023-12-26 10:04 ` [PATCH v3 4/4] riscv: dts: sophgo: add rtc dt node for CV1800 Jingbao Qiu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJRtX8Qehq-E+Z9T+fc6r36dtF8ZMNwyqxF3WGOQAmwHZHgLkw@mail.gmail.com \
    --to=qiujingbao.dlmu@gmail.com \
    --cc=a.zummo@towertech.it \
    --cc=alexandre.belloni@bootlin.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=chao.wei@sophgo.com \
    --cc=conor+dt@kernel.org \
    --cc=conor@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dlan@gentoo.org \
    --cc=inochiama@outlook.com \
    --cc=krzysztof.kozlowski+dt@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rtc@vger.kernel.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=robh+dt@kernel.org \
    --cc=unicorn_wang@outlook.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.