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

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

On 25-02-24 09:06:23, Krzysztof Kozlowski wrote:
> >> Your "in fact" is not really related to the problem described. If you
> >> put it in the correct place, drivers will work just as fine.
> >
> > You also mentioned that in your comments. Yes, indeed the board dts file
> > could remap physical controller index as different board serial number,
> > but it is not what we would like to do (at least for CIX platforms).
> > In our both HW and SW documents, we have fixed our uart usage cases,
> > for example, UART2 as AP serial ports. UART0-UART1 as uart application,eg
> > bluetooth. Customer will do their design to follow above rules, and
> > it avoids each customer writing this alias at their board file.
> 
> Follow standard rules, you don't get an exception. That's not a property
> of the SoC.

Okay. I see below documents describes it:

Documentation/devicetree/bindings/serial/qcom,msm-uartdm.yaml:23:  serialN aliases should be
in a .dts file instead of in a .dtsi file.
Documentation/devicetree/usage-model.rst:327:at all.  The /chosen, /aliases, and /memory nodes are
informational

> >>>
> >>>>
> >>>>> +       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
upstream work based on workable console.

Which option you would like to accept?
- Option-1: use fixed clock in this initial version, and will be
replaced later. 
uart_apb: clock-200000000 {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <200000000>;
	clock-output-names = "apb_pclk";
};

uart_clk: clock-200000000 {
	compatible = "fixed-clock";
	#clock-cells = <0>;
	clock-frequency = <100000000>;
	clock-output-names = "uart_clk";
};

uart2: uart@040d0000 {
	compatible = "arm,pl011", "arm,primecell";
	reg = <0x0 0x040d0000 0x0 0x1000>;
	interrupts = <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>;
	clock-names = "uartclk", "apb_pclk";
	clocks = <&uart_clk>, <&uart_apb>;
	status = "disabled";
};

- Option-2: delete the console uart node and its fixed clock.
In that way, the user could boot the kernel at orion-o6 board,
but could not use its console.

-- 

Best regards,
Peter