linux-kernel - Re: Re: [PATCH v3 2/2] clock: eswin: Add eic7700 clock 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: <7a325b0b.2de1.197e94c605b.Coremail.dongxuyang@eswincomputing.com>
Date: Tue, 8 Jul 2025 17:09:46 +0800 (GMT+08:00)
From: "Xuyang Dong" <dongxuyang@...incomputing.com>
To: "Bo Gan" <ganboing@...il.com>, mturquette@...libre.com, sboyd@...nel.org,
	robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
	linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org
Cc: ningyu@...incomputing.com, linmin@...incomputing.com,
	huangyifeng@...incomputing.com, pinkesh.vaghela@...fochips.com
Subject: Re: Re: [PATCH v3 2/2] clock: eswin: Add eic7700 clock driver

Hi Bo,

Thank you for your suggestion, it improves our driver development efforts.
Per your recommendations, we will optimize the driver program.

> On 6/24/25 03:33, dongxuyang@...incomputing.com wrote:
> This is totally wrong I think. Why does the clock driver have to care about
> CPU voltage? This functionality belongs to cpufreq. You can take JH7110 as
> reference and see how it's done: https://lore.kernel.org/all/20230606105656.124355-4-mason.huo@starfivetech.com/
> Looking at eswin vendor u-boot, it seems you have some SoC that can operate
> at 1.6Ghz without bumping the voltage. Why not do it via operating-points-v2,
> like the other SoCs? It can then be overridden by board device-tree and u-boot
> Also the logic of switching clock before changing PLL should be done using
> notifier: https://lore.kernel.org/r/20240826080430.179788-2-xingyu.wu@starfivetech.com
> Remove undocumented parameters such as "cpu_no_boost_1_6ghz" and
> "cpu-default-frequency".

When higher cpu frequency is applied, the higher voltage must be 
configured accordingly. So, from my perspective, it's better to
implement the clk, regulator and cpu frequency separately.
clk.c and clk-eic7700.c are responsible for setting clk only.
regulator-eic7700.c is for voltage configuration.
cpufreq-eic7700.c is for cpu frequency configuration, and it will call
the APIs of clk and regulator.

Is this the right approach?

> Overall I think you better do some real cleanup and refactor of this patch
> before sending it out again. The driver is quite long, and I suggest you should
> consider optimizing/condensing the logic. I guess you probably carried over the
> same code and hacks you made for the vendor tree (eswincomputing/linux-stable)
> There's no way they can be accepted by upstream. Take a look at other clk tree
> implementations and spend some real effort fixing the code. Don't let the
> reviewers grow impatient by only changing something superficially.

We'll improve the quality of our responses.

Best regards,
Xuyang