Re: [PATCH 6/6] arm64: dts: cix: add initial CIX P1(SKY1) dts support

[Krzysztof Kozlowski <krzk@kernel.org>]

On 24/02/2025 11:39, Peter Chen wrote:
>>>>>>
>>>>>>> +       sky1_fixed_clocks: fixed-clocks {
>>>>>>> +               uartclk: uartclk {
>>>>>>> +                       compatible = "fixed-clock";
>>>>>>> +                       #clock-cells = <0>;
>>>>>>> +                       clock-frequency = <100000000>;
>>>>>>> +                       clock-output-names = "uartclk";
>>>>>>
>>>>>>> +               uart_apb_pclk: uart_apb_pclk {
>>>>>>> +                       compatible = "fixed-clock";
>>>>>>> +                       #clock-cells = <0>;
>>>>>>> +                       clock-frequency = <200000000>;
>>>>>>> +                       clock-output-names = "apb_pclk";
>>>>>>
>>>>>>
>>>>>> Clock names don't need "clk" in them, and there should
>>>>>> be no underscore -- use '-' instead of '_' when separating
>>>>>> strings in DT.
>>>>>
>>>>> Will change to:
>>>>> uart_apb: clock-uart-apb {
>>>>
>>>> No, instead explain why this is part of SoC - or what are you missing
>>>> here - and use preferred naming.
>>>
>>> It is in SoC part, APB clock uses to visit register, and the function
>>> amba_get_enable_pclk at file drivers/amba/bus.c needs it during uart
>>> device probes. It uses common Arm uart pl011 IP, the binding doc
>>> described at: Documentation/devicetree/bindings/serial/pl011.yaml
>>
>> So you added fake clock? Everything you wrote is not the reason to add
>> such clock.
> 
> Not a fake clock, it is the real clocks, but depends on firmware open
> their parents and configure their rate. It could let others do their

In one place you speak about UART, which is the consumer and not
relevant. Here you mention it is real clock. That's all confusing, so to
clarify:

We talk about clock which is generated/output by something. Something
which controls way it is generated is clock controller. Either you have
here crystal or have here clock controller. If first, fixed clock is for
that. If second, you need proper clock controller binding. You can add
stubs for missing pieces, but this requires explanation and TODO/FIXME
comment.




> upstream work based on workable console.
> 
> Which option you would like to accept?

You did not describe the hardware, so I have no clue what is there.

Best regards,
Krzysztof