linux-kernel - Re: [RESEND PATCH v2 3/3] rtc: Add driver for Nuvoton ma35d1 rtc controller

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Aug 2023 16:26:16 +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 下午 03:30, Alexandre Belloni wrote:
> On 10/08/2023 15:21:47+0800, Jacky Huang wrote:
>>>>>> +	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)));
>
> Don't do that unless the regular alarm can't be set every second. Simply
> always use ALMIEN, then check rtctest is passing properly.

OK, I got it. I will drop the TICKINT and use ALMIEN only.

MA35D1 RTC has an alarm mask register which supports alarm mask for 
seconds, minutes, and hours.
We will use the alarm mask to have RTC generate an alarm interrupt per 
second, and make sure
the driver can pass rtctest.

>>          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;
>> }
>>


Best Regards,
Jacky Huang