Re: [PATCH v4 5/5] arm64: dts: Add dts files for Hisilicon Hi6220 SoC

[Mark Rutland <mark.rutland@arm.com>]

On Wed, May 06, 2015 at 05:03:29PM +0100, Brent Wang wrote:
> Hi Mark,
> 
> 2015-05-06 23:44 GMT+08:00, Mark Rutland <mark.rutland@arm.com>:
> > Hi,
> >
> >> How about add the following binding rule to my 2/5 patch:
> >> ---------------------------
> >> *Hisilicon Enhanced ARM AMBA Primecell PL011 serial UART
> >>
> >> Required properties:
> >> - compatible: must be "hisilicon,hi6220-uart", "arm,primecell",
> >> "arm,pl011"
> >
> > "arm,primecell" should come last.
> >
> >> - reg: exactly one register range with length 0x1000
> >> - interrupts: exactly one interrupt specifier
> >>
> >> See also bindings/serial/pl011.txt
> >>
> >> Example:
> >>
> >> uart0: uart@f8015000 {
> >>         compatible = "hisilicon,hi6220-uart", "arm,pl011",
> >> "arm,primecell";
> >>         reg = <0x0 0xf8015000 0x0 0x1000>;
> >>         interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> >>         clocks = <&ao_ctrl HI6220_UART0_PCLK>, <&ao_ctrl
> >> HI6220_UART0_PCLK>;
> >>         clock-names = "uartclk", "apb_pclk";
> >> };
> >
> > How about for the moment we just modify the existing pl011 binding
> > document to allow for the hi6220-specific string in addition to its
> > existing strings? We can always split it out later if the hi6220 UART
> > binding needs to be significantly divergent.
> How about adding the following rule to pl011.txt?
> ----------
> diff --git a/Documentation/devicetree/bindings/serial/pl011.txt
> b/Documentation/devicetree/bindings/serial/pl011.txt
> index ba3ecb8..1221ccb 100644
> --- a/Documentation/devicetree/bindings/serial/pl011.txt
> +++ b/Documentation/devicetree/bindings/serial/pl011.txt
> @@ -35,6 +35,10 @@ Optional properties:
>  - poll-timeout-ms:
>            Poll timeout when auto-poll is set, default
>            3000ms.
> +- compatible: "hisilicon,hi6220-uart":
> +          Hisilicon does some enhancements (e.g. larger FIFO length)
> +           based on PL011, so when using these UART hosts, this compatible
> +           string should be added.

Up at the top of the document there's already a description of the
compatible property. Just change it into a list something like:

- compatible: should contain one of the following sequences:
  * "arm,pl011", "arm,primecell"
  * "hisilicon,hi6220-pl011", "arm,pl011", "arm,primecell"

>  See also bindings/arm/primecell.txt
> ------------
> >
> >> > Is UART 0 different from UART1 and UART2?
> >> Yes,  but my patch just includes UART0, we do some changements for
> >> UART1/2
> >> to improve performance.
> >
> > Sure, but we need to make sure we can choose a sane compatible string
> > for them, that won't clash with whatever we choose for UART0.
> OK, do I also need to add compatible ""hisilicon,hi6220-uart"," to UART0 node?

This depends on a number of factors. Questions below.

> > Are they the same IP as UART0, or fundamentally different?
> The same IP.
> >
> > Are they also PL011 derived?
> Yes, just do some enhancements to improve performance.

What exactly is different between UART0 and UART{1,2}, given they are
the same IP?

Can UART{1,2} be used as if they are standard PL011 instances?

Is the difference internal, or do they just have different
clocks/regulators/etc?

Do they have feature control pins tied off differently?

Are they simply programmed differently at reset by firmware?

Thanks,
Mark.