[version 2] amba uarts - arm type uarts fifo size provided by device tree

[version 2] amba uarts - arm type uarts fifo size provided by device tree

[Jorge Ramirez-Ortiz]

[PATCH 1/2] Documentation: bindings: add optional fifo size property
[PATCH 2/2] drivers/tty: pl011: read fifo size from OF if present [updated]

These patches aim at resolving an issue present on some of the amba pl011 uarts.

They were developed for the Hisilicon Hi6220 SoC where the fifo size on the
uarts is 64 despite the amba revision having been set to 1. 

This makes the current way of detecting the fifo size based on the value of the
amba_rev non usable.

A mechanism for those uarts to retrieve the fifo sizes using the optional
property "fifo-size" in the device tree declared in PATCH1/2 is implemented
under PATCH2/2.

This device tree setting shall take precedence over any other calculated values.

[PATCH 1/2] Documentation: bindings: add optional fifo size property to AMBA UARTs

[Jorge Ramirez-Ortiz]

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 Documentation/devicetree/bindings/serial/pl011.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/serial/pl011.txt b/Documentation/devicetree/bindings/serial/pl011.txt
index 5d2e840..05d1adb 100644
--- a/Documentation/devicetree/bindings/serial/pl011.txt
+++ b/Documentation/devicetree/bindings/serial/pl011.txt
@@ -13,5 +13,6 @@ Optional properties:
 - dmas:	   When present, may have one or two dma channels.
 	   The first one must be named "rx", the second one
 	   must be named "tx".
+- fifo-size:  When present, must have the fifo size.
 
 See also bindings/arm/primecell.txt
-- 
1.9.1

[PATCH 1/2] Documentation: bindings: add optional fifo size property to AMBA UARTs

[Rob Herring]

On Wed, Feb 18, 2015 at 2:07 PM, Jorge Ramirez-Ortiz
<jorge.ramirez-ortiz@linaro.org> wrote:
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>

Acked-by: Rob Herring <robh@kernel.org>

> ---
>  Documentation/devicetree/bindings/serial/pl011.txt | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/serial/pl011.txt b/Documentation/devicetree/bindings/serial/pl011.txt
> index 5d2e840..05d1adb 100644
> --- a/Documentation/devicetree/bindings/serial/pl011.txt
> +++ b/Documentation/devicetree/bindings/serial/pl011.txt
> @@ -13,5 +13,6 @@ Optional properties:
>  - dmas:           When present, may have one or two dma channels.
>            The first one must be named "rx", the second one
>            must be named "tx".
> +- fifo-size:  When present, must have the fifo size.
>
>  See also bindings/arm/primecell.txt
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] drivers/tty: pl011: read fifo size from OF if present

[Jorge Ramirez-Ortiz]

Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
---
 drivers/tty/serial/amba-pl011.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 02016fc..c6a2dd8 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -84,6 +84,11 @@ struct vendor_data {
 
 static unsigned int get_fifosize_arm(struct amba_device *dev)
 {
+	u32 val;
+
+	if (!of_property_read_u32(dev->dev.of_node, "fifo-size", &val))
+		return val;
+
 	return amba_rev(dev) < 3 ? 16 : 32;
 }
 
-- 
1.9.1

[PATCH 2/2] drivers/tty: pl011: read fifo size from OF if present

[Jorge Ramirez-Ortiz]

On 02/18/2015 03:07 PM, Jorge Ramirez-Ortiz wrote:
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  drivers/tty/serial/amba-pl011.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 02016fc..c6a2dd8 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -84,6 +84,11 @@ struct vendor_data {
>  
>  static unsigned int get_fifosize_arm(struct amba_device *dev)
>  {
> +	u32 val;
> +
> +	if (!of_property_read_u32(dev->dev.of_node, "fifo-size", &val))
> +		return val;
> +
>  	return amba_rev(dev) < 3 ? 16 : 32;
>  }
>  

just following up on this patch. is anything else required?

[version 2] amba uarts - arm type uarts fifo size provided by device tree

[Mark Rutland]

On Wed, Feb 18, 2015 at 08:07:47PM +0000, Jorge Ramirez-Ortiz wrote:
> [PATCH 1/2] Documentation: bindings: add optional fifo size property
> [PATCH 2/2] drivers/tty: pl011: read fifo size from OF if present [updated]
> 
> These patches aim at resolving an issue present on some of the amba pl011 uarts.
> 
> They were developed for the Hisilicon Hi6220 SoC where the fifo size on the
> uarts is 64 despite the amba revision having been set to 1. 

Are we certain that the FIFO length is the only difference between the
HI6220 PL011 and a regular PL011?

It may be worth adding a compatible string in addition, just in case.

Mark.

[version 2] amba uarts - arm type uarts fifo size provided by device tree

[Jorge Ramirez-Ortiz]

On 03/03/2015 11:55 AM, Mark Rutland wrote:
> On Wed, Feb 18, 2015 at 08:07:47PM +0000, Jorge Ramirez-Ortiz wrote:
>> [PATCH 1/2] Documentation: bindings: add optional fifo size property
>> [PATCH 2/2] drivers/tty: pl011: read fifo size from OF if present [updated]
>>
>> These patches aim at resolving an issue present on some of the amba pl011 uarts.
>>
>> They were developed for the Hisilicon Hi6220 SoC where the fifo size on the
>> uarts is 64 despite the amba revision having been set to 1. 
> Are we certain that the FIFO length is the only difference between the
> HI6220 PL011 and a regular PL011?

I am trying to get an answer from the hardware designers.

>
> It may be worth adding a compatible string in addition, just in case.
>
> Mark.

[version 2] amba uarts - arm type uarts fifo size provided by device tree

[Jorge Ramirez-Ortiz]

On 03/03/2015 11:55 AM, Mark Rutland wrote:
> On Wed, Feb 18, 2015 at 08:07:47PM +0000, Jorge Ramirez-Ortiz wrote:
>> [PATCH 1/2] Documentation: bindings: add optional fifo size property
>> [PATCH 2/2] drivers/tty: pl011: read fifo size from OF if present [updated]
>>
>> These patches aim at resolving an issue present on some of the amba pl011 uarts.
>>
>> They were developed for the Hisilicon Hi6220 SoC where the fifo size on the
>> uarts is 64 despite the amba revision having been set to 1. 
> Are we certain that the FIFO length is the only difference between the
> HI6220 PL011 and a regular PL011?

you were right.

I just confirmed that there are a number of differences on top of the fifo size
requiring many additional software changes.
This is not great news since the uart announces itself as vendor_arm version 1.

so please discard this patch.

>
> It may be worth adding a compatible string in addition, just in case.
>
> Mark.

[version 2] amba uarts - arm type uarts fifo size provided by device tree

[Russell King - ARM Linux]

On Mon, Mar 09, 2015 at 08:25:19AM -0400, Jorge Ramirez-Ortiz wrote:
> I just confirmed that there are a number of differences on top of the
> fifo size requiring many additional software changes.
> This is not great news since the uart announces itself as vendor_arm
> version 1.

And that is a _very_ bad thing.  The IP is claiming to be created by ARM,
but it isn't - that probably puts this PL011 in a sticky situation from a
legal point of view.  (We've had a CPU which identified itself as an ARM
Ltd ARM926, but it wasn't - the silicon vendor was forced to re-spin their
silicon to change the ID... I wouldn't be surprised if that happens here
too.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/2] drivers/tty: pl011: read fifo size from OF if present

[Mark Rutland]

On Wed, Feb 18, 2015 at 06:53:13PM +0000, Jorge Ramirez-Ortiz wrote:
> Signed-off-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
> ---
>  drivers/tty/serial/amba-pl011.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 02016fc..23fef63 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -84,6 +84,12 @@ struct vendor_data {
>  
>  static unsigned int get_fifosize_arm(struct amba_device *dev)
>  {
> +	const void *prop;
> +
> +	prop = of_get_property(dev->dev.of_node, "fifo-size", NULL);
> +	if (prop)
> +		return of_read_ulong(prop, 1);

Use of_property_read_u32.

You will need to sanity check the result, also.

What value do you need for your platform?

Mark.

> +
>  	return amba_rev(dev) < 3 ? 16 : 32;
>  }
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>