linux-kernel - Re: [PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

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]

Message-ID: <e21c00b0-a8ce-48f3-9ec9-72540701a78b@linaro.org>
Date: Mon, 28 Oct 2024 19:44:16 +0100
From: Daniel Lezcano <daniel.lezcano@...aro.org>
To: Sergio Paracuellos <sergio.paracuellos@...il.com>
Cc: linux-mips@...r.kernel.org, tglx@...utronix.de,
 tsbogend@...ha.franken.de, john@...ozen.org, linux-kernel@...r.kernel.org,
 yangshiji66@...look.com
Subject: Re: [PATCH 1/2] clocksource: Add Ralink System Tick Counter driver

On 28/10/2024 19:04, Sergio Paracuellos wrote:
> Hi Daniel,
> 
> Thanks for reviewing this.
> 
> On Mon, Oct 28, 2024 at 5:29 PM Daniel Lezcano
> <daniel.lezcano@...aro.org> wrote:
>>
>> On 20/09/2024 09:53, Sergio Paracuellos wrote:
>>> System Tick Counter is present on Ralink SoCs RT3352 and MT7620. This
>>> driver has been in 'arch/mips/ralink' directory since the beggining of
>>> Ralink architecture support. However, it can be moved into a more proper
>>> place in 'drivers/clocksource'. Hence add it here adding also support for
>>> compile test targets.
>>>
>>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@...il.com>
>>> ---
>>>    drivers/clocksource/Kconfig        |  10 ++
>>>    drivers/clocksource/Makefile       |   1 +
>>>    drivers/clocksource/timer-ralink.c | 150 +++++++++++++++++++++++++++++
>>>    3 files changed, 161 insertions(+)
>>>    create mode 100644 drivers/clocksource/timer-ralink.c
>>>
>>> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
>>> index 95dd4660b5b6..50339f4d3201 100644
>>> --- a/drivers/clocksource/Kconfig
>>> +++ b/drivers/clocksource/Kconfig
>>> @@ -753,4 +753,14 @@ config EP93XX_TIMER
>>>          Enables support for the Cirrus Logic timer block
>>>          EP93XX.
>>>
>>> +config CLKSRC_RALINK
>>
>> It is a timer
>>
>>          RALINK_TIMER
> 
> Sure, I will change to RALINK_TIMER instead.
> 
>>
>>> +     bool "Ralink System Tick Counter"
>>
>> Silent option please if possible.
>>
>> Let the platform Kconfig selects the driver
>>
>>> +     depends on SOC_RT305X || SOC_MT7620 || COMPILE_TEST
>>> +     default y if SOC_RT305X || SOC_MT7620
>>
>> You should have something similar the RISCV option, no default option
> 
> Sorry, I am not the best with Kconfig so I am not sure what you are
> exactly expecting here.MT7620
> Does the following work for you?
> 
> config RALINK_TIMER
>     bool "Ralink System Tick Counter" if COMPILE_TEST
>     depends on SOC_RT305X || SOC_MT7620
>     select CLKSRC_MMIO
>     select TIMER_OF
>     help
>       Enables support for system tick counter present on
>       Ralink SoCs RT3352 and MT7620.

Basically the idea is to have the platform's Kconfig selecting the 
RALINK_TIMER. If I'm not wrong the Kconfig in arch/riscv/ralink should 
select RALINK_TIMER under the "config SOC_RT305X" and "config 
SOC_MT7620". The block "config CLKEVT_RT3352" has to be removed.

Then this (clocksource) Kconfig option is a silent option. The user 
won't have to figure out which option to enable because that will be 
done directly when selecting RT305X or MT7620.

The only reason to not have it silent is if you really want to opt-out 
this timer because it is not present on a different version of RT305X or 
MT7620.

IOW, this option should be:

config RALINK_TIMER
      bool "Ralink System Tick Counter" if COMPILE_TEST
      select CLKSRC_MMIO
      select TIMER_OF
      help
        Enables support for system tick counter present on
        Ralink SoCs RT3352 and MT7620.

The option COMPILE_TEST is to compile on different platforms, thus 
increasing the compilation test coverage. At the first glance, the 
driver does not seem to pull arch dependent code except definitions 
which look compatible with other archs, so it should be fine.


>>> +     select CLKSRC_MMIO
>>> +     select TIMER_OF
>>> +     help
>>> +       Enables support for system tick counter present on
>>> +       Ralink SoCs RT3352 and MT7620.
>>> +
>>>    endmenu
>>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>>> index 22743785299e..c9214afcb712 100644
>>> --- a/drivers/clocksource/Makefile
>>
>> You should use git mv
>>
>> Otherwise the code is like submitting a new driver
> 
> Ok, i will squash two patches in one performing the git mv then.
> 
> Thanks,
>      Sergio Paracuellos
>>
>>
>>
>> --
>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog