Re: [PATCH v3 2/2] clock: eswin: Add eic7700 clock driver

[Bo Gan <ganboing@gmail.com>]

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@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?
> 

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