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

[Peter Chen <peter.chen@cixtech.com>]

On 25-02-24 13:07:45, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> 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.
> 

Thanks for clarify, it is the second case, we have clock controller for
that, but now it is not ready to upstream due to some dependency, like
mailbox device driver, I will drop UART node at v2 patch set.

-- 

Best regards,
Peter