devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Xuyang Dong" <dongxuyang@eswincomputing.com>
To: "Bo Gan" <ganboing@gmail.com>,
	mturquette@baylibre.com, sboyd@kernel.org, robh@kernel.org,
	krzk+dt@kernel.org, conor+dt@kernel.org,
	linux-clk@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org
Cc: ningyu@eswincomputing.com, linmin@eswincomputing.com,
	huangyifeng@eswincomputing.com, pinkesh.vaghela@einfochips.com
Subject: Re: Re: [PATCH v3 2/2] clock: eswin: Add eic7700 clock driver
Date: Tue, 8 Jul 2025 17:09:46 +0800 (GMT+08:00)	[thread overview]
Message-ID: <7a325b0b.2de1.197e94c605b.Coremail.dongxuyang@eswincomputing.com> (raw)
In-Reply-To: <0f3aff5b-ff54-48a2-ae95-b344d311c3a1@gmail.com>

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@eswincomputing.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

  reply	other threads:[~2025-07-08  9:09 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-24 10:32 [PATCH v3 0/2] Add driver support for ESWIN eic700 SoC clock controller dongxuyang
2025-06-24 10:32 ` [PATCH v3 1/2] dt-bindings: clock: eswin: Documentation for eic7700 SoC dongxuyang
2025-06-24 11:00   ` Krzysztof Kozlowski
2025-06-25  6:07     ` Krzysztof Kozlowski
2025-06-24 10:33 ` [PATCH v3 2/2] clock: eswin: Add eic7700 clock driver dongxuyang
2025-06-24 11:04   ` Krzysztof Kozlowski
2025-07-04  9:46     ` 董绪洋
2025-07-07  9:12   ` Bo Gan
2025-07-08  9:09     ` Xuyang Dong [this message]
2025-07-09 22:52       ` Bo Gan
2025-07-10  0:52         ` Xuyang Dong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7a325b0b.2de1.197e94c605b.Coremail.dongxuyang@eswincomputing.com \
    --to=dongxuyang@eswincomputing.com \
    --cc=conor+dt@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=ganboing@gmail.com \
    --cc=huangyifeng@eswincomputing.com \
    --cc=krzk+dt@kernel.org \
    --cc=linmin@eswincomputing.com \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=ningyu@eswincomputing.com \
    --cc=pinkesh.vaghela@einfochips.com \
    --cc=robh@kernel.org \
    --cc=sboyd@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).