Re: [PATCH v2 01/16] dt-bindings: mvebu-uart: update documentation with extended UART

[Miquel RAYNAL <miquel.raynal@free-electrons.com>]

Hi Rob,

On Tue, 17 Oct 2017 17:00:22 -0500
Rob Herring <robh@kernel.org> wrote:

> On Fri, Oct 13, 2017 at 11:01:45AM +0200, Miquel Raynal wrote:
> > Update the Device Tree binding documentation for the Marvell EBU
> > UART, in order to allow describing the extended UART IP block, in
> > addition to the already supported standard UART IP. This requires
> > adding a new compatible string, the introduction of a clocks
> > property, and extensions to the interrupts property.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> > Reviewed-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
> > ---
> >  .../devicetree/bindings/serial/mvebu-uart.txt      | 49
> > +++++++++++++++++++--- 1 file changed, 44 insertions(+), 5
> > deletions(-)
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/serial/mvebu-uart.txt
> > b/Documentation/devicetree/bindings/serial/mvebu-uart.txt index
> > d37fabe17bd1..3df3a3fab4bb 100644 ---
> > a/Documentation/devicetree/bindings/serial/mvebu-uart.txt +++
> > b/Documentation/devicetree/bindings/serial/mvebu-uart.txt @@ -1,13
> > +1,52 @@ -* Marvell UART : Non standard UART used in some of
> > Marvell EBU SoCs (e.g., Armada-3700) +* Marvell UART : Non standard
> > UART used in some of Marvell EBU SoCs
> > +                 e.g., Armada-3700.
> >  
> >  Required properties:
> > -- compatible: "marvell,armada-3700-uart"
> > +- compatible:
> > +    - "marvell,armada-3700-uart" for the standard variant of the
> > UART
> > +      (32 bytes FIFO, no DMA, level interrupts, 8-bit access to the
> > +      FIFO, baudrate limited to 230400).
> > +    - "marvell,armada-3700-uart-ext" for the extended variant of
> > the
> > +      UART (128 bytes FIFO, DMA, front interrupts, 8-bit or 32-bit
> > +      accesses to the FIFO, baudrate unlimited by the dividers).  
> 
> What do you call the next extended version?
> marvell,armada-3700-uart-ext-ext?

I don't know what you mean by "next extended version"?

> 
> This is different versions of UART on the same chip?

Today in mainline there is support for the A3700 UART IP.
This series add support for another IP, based on the A3700, but with
extended features (explaining the -ext suffix).

Can you precise what is bothering you?

> 
> >  - reg: offset and length of the register set for the device.
> > -- interrupts: device interrupt
> > +- clocks: UART reference clock used to derive the baudrate (only
> > +      mandatory with "marvell,armada-3700-uart-ext" compatible).  
> 
> How is this optional? The freq is fixed if not present? If so, what 
> frequency?

The "clocks" property should not be optional at all but that is how the
bindings were handled before this series, so I can't tell now that
this property is mandatory as it would break compatibility with older
versions of the driver.

When no clock is provided, the frequency is fixed by the bootloader
and cannot be changed. There is no standard frequency for it but the
one chosen by the bootloader often is 115200 as the UART is usually
used as the serial console.

Because the bootloader does only initialize the UART in use for the
serial console, the clock is mandatory when using another port or it
will not work at all.

> 
> > +- interrupts:
> > +    - Must contain three elements for the standard variant of the
> > IP
> > +      (marvell,armada-3700-uart): "uart-sum", "uart-tx" and
> > "uart-rx",
> > +      respectively the UART sum interrupt, the UART TX interrupt
> > and
> > +      UART RX interrupt. A corresponding interrupt-names property
> > must
> > +      be defined.
> > +    - Must contain two elements for the extended variant of the IP
> > +      (marvell,armada-3700-uart-ext): "uart-tx" and "uart-rx",
> > +      respectively the UART TX interrupt and the UART RX
> > interrupt. A
> > +      corresponding interrupts-names property must be defined.
> > +    - For backward compatibility reasons, a single element
> > interrupts
> > +      property is also supported for the standard variant of the
> > IP,
> > +      containing only the UART sum interrupt. This form is
> > deprecated
> > +      and should no longer be used.
> >  
> >  Example:
> > -	serial@12000 {
> > +	uart0: serial@12000 {
> >  		compatible = "marvell,armada-3700-uart";
> >  		reg = <0x12000 0x200>;
> > -		interrupts = <43>;
> > +		clocks = <&xtalclk>;
> > +		interrupts =
> > +		<GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
> > +		<GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> > +		<GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>;
> > +		interrupt-names = "uart-sum", "uart-tx", "uart-rx";
> > +		status = "disabled";  
> 
> Don't show status in examples.

Ok.

> 
> > +	};
> > +
> > +	uart1: serial@12200 {
> > +		compatible = "marvell,armada-3700-uart-ext";
> > +		reg = <0x12200 0x30>;
> > +		clocks = <&xtalclk>;
> > +		interrupts =
> > +		<GIC_SPI 30 IRQ_TYPE_EDGE_RISING>,
> > +		<GIC_SPI 31 IRQ_TYPE_EDGE_RISING>;
> > +		interrupt-names = "uart-tx", "uart-rx";
> > +		status = "disabled";
> >  	};
> > -- 
> > 2.11.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > devicetree" in the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  

Thanks,
Miquèl

-- 
Miquel Raynal, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com