Message-ID: <347cf148-bda8-852b-768c-fa2b57ce5bcb@gmail.com>
Date:   Thu, 10 Aug 2023 15:21:47 +0800
From:   Jacky Huang <ychuang570808@...il.com>
To:     Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc:     a.zummo@...ertech.it, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
        linux-rtc@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        soc@...nel.org, mjchen@...oton.com, schung@...oton.com,
        Jacky Huang <ychuang3@...oton.com>
Subject: Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc
 controller



On 2023/8/10 上午 06:51, Alexandre Belloni wrote:
> On 09/08/2023 16:12:54+0800, Jacky Huang wrote:
>>
>> On 2023/8/9 上午 10:10, Alexandre Belloni wrote:
>>> Hello,
>>>
>>> On 09/08/2023 01:15:42+0000, Jacky Huang wrote:
>>>> +
>>>> +struct ma35_bcd_time {
>>>> +	int bcd_sec;
>>>> +	int bcd_min;
>>>> +	int bcd_hour;
>>>> +	int bcd_mday;
>>>> +	int bcd_mon;
>>>> +	int bcd_year;
>>>> +};
>>> I don't get why this struct is useful.
>> I will remove this and modify code.
>>
>>
>>>> +
>>>> +static irqreturn_t ma35d1_rtc_interrupt(int irq, void *data)
>>>> +{
>>>> +	struct ma35_rtc *rtc = (struct ma35_rtc *)data;
>>>> +	unsigned long events = 0, rtc_irq;
>>>> +
>>>> +	rtc_irq = rtc_reg_read(rtc, MA35_REG_RTC_INTSTS);
>>>> +
>>>> +	if (rtc_irq & RTC_INTSTS_ALMIF) {
>>>> +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_ALMIF);
>>>> +		events |= RTC_AF | RTC_IRQF;
>>>> +	}
>>>> +
>>>> +	if (rtc_irq & RTC_INTSTS_TICKIF) {
>>>> +		rtc_reg_write(rtc, MA35_REG_RTC_INTSTS, RTC_INTSTS_TICKIF);
>>>> +		events |= RTC_UF | RTC_IRQF;
>>> How did you test this path?
>> We use BusyBox 'date' command to observe the time changed.
>> In addition, we wrote a simple code to test it.
>> (https://github.com/OpenNuvoton/MA35D1_Linux_Applications/tree/master/examples/rtc)
>>
> You should probably run rtctest:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/rtc/rtctest.c

Thank you for providing this information.
We will run this test before submitting the next version.

>
>>>> +	if (settm->tm_year < 100) {
>>>> +		dev_warn(dev, "The year will be between 1970-1999, right?\n");
>>> No, don't do that, properly set the rtc hardware range and let the user
>>> choose their time offset/window.
>> We will modify the code as:
>>
>> #define MA35_BASE_YEAR 2000 /* assume 20YY not 19YY */
>>
>> int year;
>>
>> year = tm->tm_year + 1900 – MA35_BASE_YEAR;
>> if (year < 0) {
>>      dev_err(dev, "invalid year: %d\n", year);
>>      return -EINVAL;
>> }
> This is not needed as the rtc core is going to check the value is in the
> range once you set it.

OK, I will drop the "if (year < 0)" check.

>>>> +	time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
>>>> +	cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
>>>> +	wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
>>> Are the registers properly latched when reading? How do you ensure that
>>> MA35_REG_RTC_TIME didn't change before reading MA35_REG_RTC_CAL ?
>> We will update the code as
>>
>> do {
>>      time = rtc_reg_read(rtc, MA35_REG_RTC_TIME);
>>      cal  = rtc_reg_read(rtc, MA35_REG_RTC_CAL);
>>      wday = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY);
>> } while ((time != rtc_reg_read(rtc, MA35_REG_RTC_TIME)) ||
>>           (cal != rtc_reg_read(rtc, MA35_REG_RTC_CAL)) ||
>>           (wday != = rtc_reg_read(rtc, MA35_REG_RTC_WEEKDAY)) );
> Ok, so this mean your hardware doesn't do latching. I don't think it is
> necessary to check MA35_REG_RTC_WEEKDAY.

OK, I will remove the MA35_REG_RTC_WEEKDAY check.
>>
>>>> +
>>>> +	return ma35d1_rtc_bcd2bin(time, cal, wday, tm);
>>>> +}
>>>> +
>>>> +static int ma35d1_rtc_set_time(struct device *dev, struct rtc_time *tm)
>>>> +{
>>>> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
>>>> +	struct ma35_bcd_time gettm;
>>>> +	u32 val;
>>>> +
>>>> +	ma35d1_rtc_bin2bcd(dev, tm, &gettm);
>>>> +
>>>> +	val = gettm.bcd_mday | gettm.bcd_mon | gettm.bcd_year;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_CAL, val);
>>>> +
>>>> +	val = gettm.bcd_sec | gettm.bcd_min | gettm.bcd_hour;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_TIME, val);
>>>> +
>>> Same question about latching, shouldn't you stop the rtc while doing
>>> this?
>> In fact, once enabled, the RTC hardware of MA35D1 cannot be stopped; this is
>> how the hardware is designed.
>> Inside the MA35D1 RTC, there's an internal counter that advances by 128
>> counts per second.
>> When the time or date register is updated, the internal counter of the RTC
>> hardware will automatically reset to 0,
>> so there is no need to worry about memory latch issues.
> Ok, great
>
>>
>>>> +	val = tm->tm_wday;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_WEEKDAY, val);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int ma35d1_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
>>>> +{
>>>> +	struct ma35_rtc *rtc = dev_get_drvdata(dev);
>>>> +	struct ma35_bcd_time tm;
>>>> +	unsigned long val;
>>>> +
>>>> +	ma35d1_rtc_bin2bcd(dev, &alrm->time, &tm);
>>>> +
>>>> +	val = tm.bcd_mday | tm.bcd_mon | tm.bcd_year;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_CALM, val);
>>>> +
>>>> +	val = tm.bcd_sec | tm.bcd_min | tm.bcd_hour;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_TALM, val);
>>>> +
>>> What about handling alrm.enabled here?
>> The MA35D1 RTC hardware design does not have an alarm enable/disable bit.
>> The decision to utilize the alarm can only be made through enabling or
>> disabling the alarm interrupt.
> Exactly, you should use alrm.enabled to enable or disable the alarm
> interrupt. You can simply call ma35d1_alarm_irq_enable, many drivers are
> doing this.

OK, we will use irq enable/disable to support alrm.enabled.

>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct rtc_class_ops ma35d1_rtc_ops = {
>>>> +	.read_time = ma35d1_rtc_read_time,
>>>> +	.set_time = ma35d1_rtc_set_time,
>>>> +	.read_alarm = ma35d1_rtc_read_alarm,
>>>> +	.set_alarm = ma35d1_rtc_set_alarm,
>>>> +	.alarm_irq_enable = ma35d1_alarm_irq_enable,
>>>> +};
>>>> +
>>>> +static int ma35d1_rtc_probe(struct platform_device *pdev)
>>>> +{
>>>> +	struct ma35_rtc *rtc;
>>>> +	struct clk *clk;
>>>> +	u32 regval;
>>>> +	int err;
>>>> +
>>>> +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
>>>> +	if (!rtc)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	rtc->rtc_reg = devm_platform_ioremap_resource(pdev, 0);
>>>> +	if (IS_ERR(rtc->rtc_reg))
>>>> +		return PTR_ERR(rtc->rtc_reg);
>>>> +
>>>> +	clk = of_clk_get(pdev->dev.of_node, 0);
>>>> +	if (IS_ERR(clk))
>>>> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "failed to find rtc clock\n");
>>>> +
>>>> +	err = clk_prepare_enable(clk);
>>>> +	if (err)
>>>> +		return -ENOENT;
>>>> +
>>>> +	platform_set_drvdata(pdev, rtc);
>>>> +
>>>> +	rtc->rtcdev = devm_rtc_device_register(&pdev->dev, pdev->name,
>>>> +					       &ma35d1_rtc_ops, THIS_MODULE);
>>>> +	if (IS_ERR(rtc->rtcdev))
>>>> +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->rtcdev),
>>>> +				     "failed to register rtc device\n");
>>> This MUST be done last in probe, else you open a race with userspace.
>>>
>> Yes, I will moved it to last in probe.
>>
>>
>>>> +
>>>> +	err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
>>>> +	if (err)
>>>> +		return err;
>>>> +
>>> I don't believe you should do this on every probe but only when this
>>> hasn't been done yet.
>>>
>>>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
>>>> +	regval |= RTC_CLKFMT_24HEN;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);
>>>> +
>>> ditto
>> I will modify them as:
>>
>> If (!(rtc_reg_read(rtc, MA35_REG_RTC_INIT) & RTC_INIT_ACK)) {
>>      err = ma35d1_rtc_init(rtc, RTC_INIT_TIMEOUT);
>>      if (err)
>>          return err;
>>      regval = rtc_reg_read(rtc, MA35_REG_RTC_CLKFMT);
>>      regval |= RTC_CLKFMT_24HEN;
>>      rtc_reg_write(rtc, MA35_REG_RTC_CLKFMT, regval);}
>> }
>>
>>
>>>> +	rtc->irq_num = platform_get_irq(pdev, 0);
>>>> +
>>>> +	err = devm_request_irq(&pdev->dev, rtc->irq_num, ma35d1_rtc_interrupt,
>>>> +			       IRQF_NO_SUSPEND, "ma35d1rtc", rtc);
>>>> +	if (err)
>>>> +		return dev_err_probe(&pdev->dev, err, "Failed to request rtc irq\n");
>>>> +
>>>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>>>> +	regval |= RTC_INTEN_TICKIEN;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
>>>> +
>>>> +	device_init_wakeup(&pdev->dev, true);
>>>> +
>>> You must set the rtc range here.
>> I will add:
>> ma35d1_rtc->rtcdev->range_min = RTC_TIMESTAMP_BEGIN_2000;
>> ma35d1_rtc->rtcdev->range_max = RTC_TIMESTAMP_END_2099;
>>
> Perfect. Maybe you should run rtc-range to check for proper operation in
> the range:
> https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c

We will run this test.
Thanks for providing this information.

>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int ma35d1_rtc_suspend(struct platform_device *pdev, pm_message_t state)
>>>> +{
>>>> +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
>>>> +	u32 regval;
>>>> +
>>>> +	if (device_may_wakeup(&pdev->dev))
>>>> +		enable_irq_wake(rtc->irq_num);
>>>> +
>>>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>>>> +	regval &= ~RTC_INTEN_TICKIEN;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
>>> This is not what the user is asking, don't do this. Also, how was this
>>> tested?
>> Sure, I will remove these three lines of code.
>>
>> We test it with "echo mem > /sys/power/state".
>>
> Yes, my point is that if UIE is enabled, then the user wants to be woken
> up every second. If this is not what is wanted, then UIE has to be
> disabled before going to suspend.
>
> My question is why are you enabling RTC_INTEN_TICKIEN in probe? I don't
> expect anyone to use an actual hardware tick interrupt, unless the alarm
> is broken and can't be set every second. This is why I questioned the
> RTC_UF path because I don't expect it to be taken at all.

Yes, we will remove TICKIEN from probe and modify ma35d1_alarm_irq_enable().
TICKIEN will be enabled only if UIE is enabled.

static int ma35d1_alarm_irq_enable(struct device *dev, unsigned int enabled)
{
     struct ma35d1_rtc *rtc = dev_get_drvdata(dev);

     if (enabled) {
         if (rtc->rtc->uie_rtctimer.enabled)
             rtc_reg_write(rtc, NVT_RTC_INTEN,
                       (rtc_reg_read(rtc, 
NVT_RTC_INTEN)|(RTC_INTEN_TICKIEN)));
         if (rtc->rtc->aie_timer.enabled)
             rtc_reg_write(rtc, NVT_RTC_INTEN,
                       (rtc_reg_read(rtc, 
NVT_RTC_INTEN)|(RTC_INTEN_ALMIEN)));
     } else {
         if (rtc->rtc->uie_rtctimer.enabled)
             rtc_reg_write(rtc, NVT_RTC_INTEN,
                       (rtc_reg_read(rtc, NVT_RTC_INTEN) & 
(~RTC_INTEN_TICKIEN)));
         if (rtc->rtc->aie_timer.enabled)
             rtc_reg_write(rtc, NVT_RTC_INTEN,
                       (rtc_reg_read(rtc, NVT_RTC_INTEN) & 
(~RTC_INTEN_ALMIEN)));
     }
     return 0;
}



>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int ma35d1_rtc_resume(struct platform_device *pdev)
>>>> +{
>>>> +	struct ma35_rtc *rtc = platform_get_drvdata(pdev);
>>>> +	u32 regval;
>>>> +
>>>> +	if (device_may_wakeup(&pdev->dev))
>>>> +		disable_irq_wake(rtc->irq_num);
>>>> +
>>>> +	regval = rtc_reg_read(rtc, MA35_REG_RTC_INTEN);
>>>> +	regval |= RTC_INTEN_TICKIEN;
>>>> +	rtc_reg_write(rtc, MA35_REG_RTC_INTEN, regval);
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id ma35d1_rtc_of_match[] = {
>>>> +	{ .compatible = "nuvoton,ma35d1-rtc", },
>>>> +	{},
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, ma35d1_rtc_of_match);
>>>> +
>>>> +static struct platform_driver ma35d1_rtc_driver = {
>>>> +	.suspend    = ma35d1_rtc_suspend,
>>>> +	.resume     = ma35d1_rtc_resume,
>>>> +	.probe      = ma35d1_rtc_probe,
>>>> +	.driver		= {
>>>> +		.name	= "rtc-ma35d1",
>>>> +		.of_match_table = ma35d1_rtc_of_match,
>>>> +	},
>>>> +};
>>>> +
>>>> +module_platform_driver(ma35d1_rtc_driver);
>>>> +
>>>> +MODULE_AUTHOR("Min-Jen Chen <mjchen@...oton.com>");
>>>> +MODULE_DESCRIPTION("MA35D1 RTC driver");
>>>> +MODULE_LICENSE("GPL");
>>>> -- 
>>>> 2.34.1
>>>>
>>
>> Best Regards,
>> Jacky Huang
>>

Best Regards,
Jacky Huang

Powered by blists - more mailing lists