Message-ID: <5bbfd825.2ea1.197f1d1c88a.Coremail.dongxuyang@eswincomputing.com>
Date: Thu, 10 Jul 2025 08:52:26 +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,

Thanks for the great input-we’ll work on these changes!

> Hi Xuyang
> 
> On 7/8/25 02:09, Xuyang Dong wrote:
> > 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?
> > 
> 
> Some context for people not familiar with this SoC/Board. The regulator is not
> part of the SoC, but on the board. The GPIO pin is controlling the ratio of a
> DC/DC converter to select between 0.8V and 0.9V. I think there's no need for
> regulator-eic7700.c, and it actually would be wrong if you do it this way,
> because per your datasheet, CPU voltage can be any value within a supported
> range, and it's up to the board vendor to determine the voltage. Thus, better
> to model it with a "regulator-gpio" in the device-tree. No code change needed.
> (Assuming you have GPIO/pinctrl merged, which think you already did?)
> 
> For cpufreq, I don't see why it can't be just modeled by "operating-points-v2"
> just like other SoC/boards. Once complication is the 0.8/0.9 voltage selection
> I see two potential ways to solve it (assuming using opp):
> 
> 1. Extend the opp to dynamically choose 0.8/0.9 based on your OTP settings
> 2. Isolate this logic in u-boot to patch the opp-table in device-tree before
>     boot, or in grub boot scenario, also hook the EFI_DT_FIXUP protocol in
>     u-boot to patch device-tree before grub hands off to Linux
> 
> For 1, you probably need to have a stable OTP layout, which doesn't vary from
> chip to chip and board to board. It also requires you to have a OTP driver in
> Linux kernel to read from OTP.
> 
> 2 is probably simpler and a lot easier to implement. There's also very minimal
> or virtually no code change to Linux. It's perhaps easier to do board specific
> stuff in u-boot. You can use 0.9V by default in opp-table in device-tree and
> u-boot can do the work of adjusting it down to 0.8 based on some OTP settings.
> There's also no harm if something went wrong, e.g., OTP is empty or u-boot
> doesn't implement the patching logic. In that case, you just waste some power.
> It's also possible to remove some frequencies in u-boot if that freq can't be
> achieved no matter how high the voltage.
> 
> >> 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
> Bo

Best regards,
Xuyang

Powered by blists - more mailing lists