linux-kernel - Re: [PATCH 2/3] rtc: support for the Amlogic on-chip RTC

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] [day] [month] [year] [list]

Message-ID: <d6187076-5fab-4804-a008-cfb5d85f8cd1@amlogic.com>
Date: Tue, 3 Sep 2024 10:48:27 +0800
From: Xianwei Zhao <xianwei.zhao@...ogic.com>
To: Alexandre Belloni <alexandre.belloni@...tlin.com>
Cc: Yiting Deng <yiting.deng@...ogic.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-amlogic@...ts.infradead.org,
 linux-rtc@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] rtc: support for the Amlogic on-chip RTC

Hi Alexandre,
     Thanks for your reply.

On 2024/9/3 04:19, Alexandre Belloni wrote:
> [ EXTERNAL EMAIL ]
> 
> On 02/09/2024 16:14:45+0800, Xianwei Zhao wrote:
>> Hi Alexandre,
>>      Thanks for your reply.
>>
>> On 2024/8/26 17:45, Alexandre Belloni wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> On 23/08/2024 17:19:45+0800, Xianwei Zhao via B4 Relay wrote:
>>>> From: Yiting Deng <yiting.deng@...ogic.com>
>>>>
>>>> Support for the on-chip RTC found in some of Amlogic's SoCs such as the
>>>> A113L2 and A113X2.
>>>>
>>>> Signed-off-by: Yiting Deng <yiting.deng@...ogic.com>
>>>> Signed-off-by: Xianwei Zhao <xianwei.zhao@...ogic.com>
>>>> ---
>>>>    drivers/rtc/Kconfig       |  12 +
>>>>    drivers/rtc/Makefile      |   1 +
>>>>    drivers/rtc/rtc-amlogic.c | 589 ++++++++++++++++++++++++++++++++++++++++++++++
>>>
>>> As pointed out, this is the third amlogic driver so the name of the file
>>> must be more specific.
>>>
>>
>> This RTC hardware includes a timing function and an alarm function.
>> But the existing has only timing function, alarm function is using the
>> system clock to implement a virtual alarm. And the relevant register access
>> method is also different.
>>
>> The "meson" string is meaningless, it just keeps going, and now the new
>> hardware uses the normal naming.
> 
> The proper naming is then definitively not just amlogic, because in 5
> year, you are going to say the exact same thing about this driver
> "register access is different, this is for old SoCs, etc"
> 
> amlogc-a4 would be more appropriate.
>

Will modify driver file name to rtc-amlogic-a4.c


>>>> +             /* Enable RTC */
>>>> +             regmap_write_bits(rtc->map, RTC_CTRL, RTC_ENABLE, RTC_ENABLE);
>>>
>>>                   This must not be done at probe time, else you loose the
>>>                   important information taht the time has never been set. Instead,
>>>                   it should only be enabled on the first .set_time invocation do
>>>                   you could now in .read_time that the time is currently invalid.
>>>
>> There are some doubts about this place.
>>
>> You mean that after the system is up, unless the time is set, it will fail
>> to read the time at any time, and the alarm clock will also fail.
>> In this case, the system must set a time.
> 
> Exactly, reading the time must not succeed if the time is known to be
> bad.
> 

Got it.

>>
>> When read time invlalid, system is will set time.
>> This part of the logic I see the kernel part has not been implemented, so
>> only the user application has been implemented. Whether this is reasonable,
>> if not set time, you will never use RTC module.
> 
> This is not going to be implemented in the kernel. The kernel can't know
> what is the proper time to set unless userspace tells it.
> 

OK will place enable action in the set_time process.

> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com