[PATCH v2 0/2] serial: pl011: Add xilinx uart

[PATCH v2 0/2] serial: pl011: Add xilinx uart

[Shubhrajyoti Datta]

-Support uart peripheral in Xilinx Versal SOC.
Add the dt-binding for the same

v2: 
Update the commit message to reflect the AXI limitation.

Raviteja Narayanam (1):
  serial: pl011: Add support for Xilinx Uart

Shubhrajyoti Datta (1):
  dt-bindings: serial: pl011: Add 'arm,xlnx-uart'

 .../devicetree/bindings/serial/pl011.yaml     | 10 ++++--
 drivers/tty/serial/amba-pl011.c               | 33 +++++++++++++++++--
 2 files changed, 38 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] dt-bindings: serial: pl011: Add 'arm,xlnx-uart'

[Shubhrajyoti Datta]

The Xilinx Versal board uses the arm,pl011 ip. However the
axi port that it is connected to has a limitation that it allows
only 32-bit accesses. So to differentiate we add a compatible.

Add support for Uart used in Xilinx Versal SOCs as a platform
device.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
---
 Documentation/devicetree/bindings/serial/pl011.yaml | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/serial/pl011.yaml b/Documentation/devicetree/bindings/serial/pl011.yaml
index d8aed84abcd3..bf094ab93086 100644
--- a/Documentation/devicetree/bindings/serial/pl011.yaml
+++ b/Documentation/devicetree/bindings/serial/pl011.yaml
@@ -24,9 +24,13 @@ select:
 
 properties:
   compatible:
-    items:
-      - const: arm,pl011
-      - const: arm,primecell
+    oneOf:
+      - items:
+          - const: arm,pl011
+          - const: arm,primecell
+      - items:
+          - const: arm,pl011
+          - const: arm,xlnx-uart # xilinx uart as platform device
 
   reg:
     maxItems: 1
-- 
2.17.1

[PATCH v2 2/2] serial: pl011: Add support for Xilinx Uart

[Shubhrajyoti Datta]

From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>

The xilinx uart used in Versal SOC follows arm pl011 implementation
with just a minor change in data bus width. The minimum data
transaction width in Versal SOC is 32-bit as specified in the
TRM (Chapter 39: Transaction attributes). Pl011 defaults to 16-bit
in the driver.
So, add the xilinx uart as platform device with properties specified
in 'vendor_data' structure.

Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 drivers/tty/serial/amba-pl011.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index 97ef41cb2721..096a56f64d17 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -152,6 +152,21 @@ static const struct vendor_data vendor_sbsa = {
 	.fixed_options		= true,
 };
 
+static const struct vendor_data vendor_xlnx = {
+	.reg_offset             = pl011_std_offsets,
+	.ifls                   = UART011_IFLS_RX4_8 | UART011_IFLS_TX4_8,
+	.fr_busy                = UART01x_FR_BUSY,
+	.fr_dsr                 = UART01x_FR_DSR,
+	.fr_cts                 = UART01x_FR_CTS,
+	.fr_ri                  = UART011_FR_RI,
+	.access_32b             = true,
+	.oversampling           = false,
+	.dma_threshold          = false,
+	.cts_event_workaround   = false,
+	.always_enabled         = true,
+	.fixed_options          = false,
+};
+
 #ifdef CONFIG_ACPI_SPCR_TABLE
 static const struct vendor_data vendor_qdt_qdf2400_e44 = {
 	.reg_offset		= pl011_std_offsets,
@@ -2581,6 +2596,7 @@ static int __init pl011_early_console_setup(struct earlycon_device *device,
 }
 OF_EARLYCON_DECLARE(pl011, "arm,pl011", pl011_early_console_setup);
 OF_EARLYCON_DECLARE(pl011, "arm,sbsa-uart", pl011_early_console_setup);
+OF_EARLYCON_DECLARE(pl011, "arm,xlnx-uart", pl011_early_console_setup);
 
 /*
  * On Qualcomm Datacenter Technologies QDF2400 SOCs affected by
@@ -2824,6 +2840,7 @@ static int sbsa_uart_probe(struct platform_device *pdev)
 {
 	struct uart_amba_port *uap;
 	struct resource *r;
+	int xlnx_uart = 0;
 	int portnr, ret;
 	int baudrate;
 
@@ -2834,6 +2851,7 @@ static int sbsa_uart_probe(struct platform_device *pdev)
 	if (pdev->dev.of_node) {
 		struct device_node *np = pdev->dev.of_node;
 
+		xlnx_uart = of_device_is_compatible(np, "arm,xlnx-uart");
 		ret = of_property_read_u32(np, "current-speed", &baudrate);
 		if (ret)
 			return ret;
@@ -2863,13 +2881,23 @@ static int sbsa_uart_probe(struct platform_device *pdev)
 #endif
 		uap->vendor = &vendor_sbsa;
 
+	uap->port.ops   = &sbsa_uart_pops;
+
+	if (xlnx_uart) {
+		uap->vendor = &vendor_xlnx;
+		uap->clk = devm_clk_get(&pdev->dev, NULL);
+		if (IS_ERR(uap->clk))
+			return PTR_ERR(uap->clk);
+
+		uap->port.ops = &amba_pl011_pops;
+	}
+
 	uap->reg_offset	= uap->vendor->reg_offset;
 	uap->fifosize	= 32;
 	uap->port.iotype = uap->vendor->access_32b ? UPIO_MEM32 : UPIO_MEM;
-	uap->port.ops	= &sbsa_uart_pops;
 	uap->fixed_baud = baudrate;
 
-	snprintf(uap->type, sizeof(uap->type), "SBSA");
+	snprintf(uap->type, sizeof(uap->type), "%s\n", (xlnx_uart ? "xlnx_uart" : "SBSA"));
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
@@ -2893,6 +2921,7 @@ static int sbsa_uart_remove(struct platform_device *pdev)
 
 static const struct of_device_id sbsa_uart_of_match[] = {
 	{ .compatible = "arm,sbsa-uart", },
+	{ .compatible = "arm,xlnx-uart", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, sbsa_uart_of_match);
-- 
2.17.1

Re: [PATCH v2 1/2] dt-bindings: serial: pl011: Add 'arm,xlnx-uart'

[Rob Herring]

On Wed, Jul 20, 2022 at 8:26 AM Shubhrajyoti Datta
<shubhrajyoti.datta@xilinx.com> wrote:
>
> The Xilinx Versal board uses the arm,pl011 ip. However the

s/ip/IP/

> axi port that it is connected to has a limitation that it allows
> only 32-bit accesses. So to differentiate we add a compatible.

Why not just use the standard 'reg-io-width' property?

> Add support for Uart used in Xilinx Versal SOCs as a platform
> device.

What's a platform device? Don't include Linuxisms in your bindings.

>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> ---
>  Documentation/devicetree/bindings/serial/pl011.yaml | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/serial/pl011.yaml b/Documentation/devicetree/bindings/serial/pl011.yaml
> index d8aed84abcd3..bf094ab93086 100644
> --- a/Documentation/devicetree/bindings/serial/pl011.yaml
> +++ b/Documentation/devicetree/bindings/serial/pl011.yaml
> @@ -24,9 +24,13 @@ select:
>
>  properties:
>    compatible:
> -    items:
> -      - const: arm,pl011
> -      - const: arm,primecell
> +    oneOf:
> +      - items:
> +          - const: arm,pl011
> +          - const: arm,primecell
> +      - items:
> +          - const: arm,pl011
> +          - const: arm,xlnx-uart # xilinx uart as platform device

First, this is backwards. compatible is most specific to least
specific. In your case Arm is not the vendor and just 'xlnx-uart' is
not very specific. You said this is for Versal SoC, so something like
'xlnx,versal-pl011' would be more appropriate. But again, I think
reg-io-width is all you need here.

The IP is still a Primecell block, so it should have 'arm,primecell'
still as the definition of 'arm,primecell' is that it has the ID
registers. Yes, that means Linux will create an amba_device instead,
but you can't be designing your binding to work-around Linux.

Rob

Re: [PATCH v2 2/2] serial: pl011: Add support for Xilinx Uart

[Rob Herring]

On Wed, Jul 20, 2022 at 8:26 AM Shubhrajyoti Datta
<shubhrajyoti.datta@xilinx.com> wrote:
>
> From: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
>
> The xilinx uart used in Versal SOC follows arm pl011 implementation
> with just a minor change in data bus width. The minimum data
> transaction width in Versal SOC is 32-bit as specified in the
> TRM (Chapter 39: Transaction attributes). Pl011 defaults to 16-bit
> in the driver.
> So, add the xilinx uart as platform device with properties specified
> in 'vendor_data' structure.
>
> Signed-off-by: Raviteja Narayanam <raviteja.narayanam@xilinx.com>
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---
>  drivers/tty/serial/amba-pl011.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index 97ef41cb2721..096a56f64d17 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -152,6 +152,21 @@ static const struct vendor_data vendor_sbsa = {
>         .fixed_options          = true,
>  };
>
> +static const struct vendor_data vendor_xlnx = {
> +       .reg_offset             = pl011_std_offsets,
> +       .ifls                   = UART011_IFLS_RX4_8 | UART011_IFLS_TX4_8,
> +       .fr_busy                = UART01x_FR_BUSY,
> +       .fr_dsr                 = UART01x_FR_DSR,
> +       .fr_cts                 = UART01x_FR_CTS,
> +       .fr_ri                  = UART011_FR_RI,
> +       .access_32b             = true,
> +       .oversampling           = false,
> +       .dma_threshold          = false,
> +       .cts_event_workaround   = false,
> +       .always_enabled         = true,
> +       .fixed_options          = false,
> +};
> +
>  #ifdef CONFIG_ACPI_SPCR_TABLE
>  static const struct vendor_data vendor_qdt_qdf2400_e44 = {
>         .reg_offset             = pl011_std_offsets,
> @@ -2581,6 +2596,7 @@ static int __init pl011_early_console_setup(struct earlycon_device *device,
>  }
>  OF_EARLYCON_DECLARE(pl011, "arm,pl011", pl011_early_console_setup);
>  OF_EARLYCON_DECLARE(pl011, "arm,sbsa-uart", pl011_early_console_setup);
> +OF_EARLYCON_DECLARE(pl011, "arm,xlnx-uart", pl011_early_console_setup);
>
>  /*
>   * On Qualcomm Datacenter Technologies QDF2400 SOCs affected by
> @@ -2824,6 +2840,7 @@ static int sbsa_uart_probe(struct platform_device *pdev)
>  {
>         struct uart_amba_port *uap;
>         struct resource *r;
> +       int xlnx_uart = 0;
>         int portnr, ret;
>         int baudrate;
>
> @@ -2834,6 +2851,7 @@ static int sbsa_uart_probe(struct platform_device *pdev)
>         if (pdev->dev.of_node) {
>                 struct device_node *np = pdev->dev.of_node;
>
> +               xlnx_uart = of_device_is_compatible(np, "arm,xlnx-uart");
>                 ret = of_property_read_u32(np, "current-speed", &baudrate);
>                 if (ret)
>                         return ret;
> @@ -2863,13 +2881,23 @@ static int sbsa_uart_probe(struct platform_device *pdev)
>  #endif
>                 uap->vendor = &vendor_sbsa;
>
> +       uap->port.ops   = &sbsa_uart_pops;

SBSA is only a subset of PL011 functionality. I don't think you want
to use that. Is there some reason you can't set uap->port.iotype to
UPIO_MEM32 based on 'reg-io-width' in pl011_probe()? The console setup
would need something too, but it should be the same as 8250 setup
IIRC.

> +
> +       if (xlnx_uart) {
> +               uap->vendor = &vendor_xlnx;
> +               uap->clk = devm_clk_get(&pdev->dev, NULL);
> +               if (IS_ERR(uap->clk))
> +                       return PTR_ERR(uap->clk);
> +
> +               uap->port.ops = &amba_pl011_pops;

None of this would be needed either.

> +       }
> +
>         uap->reg_offset = uap->vendor->reg_offset;
>         uap->fifosize   = 32;
>         uap->port.iotype = uap->vendor->access_32b ? UPIO_MEM32 : UPIO_MEM;
> -       uap->port.ops   = &sbsa_uart_pops;
>         uap->fixed_baud = baudrate;
>
> -       snprintf(uap->type, sizeof(uap->type), "SBSA");
> +       snprintf(uap->type, sizeof(uap->type), "%s\n", (xlnx_uart ? "xlnx_uart" : "SBSA"));
>
>         r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> @@ -2893,6 +2921,7 @@ static int sbsa_uart_remove(struct platform_device *pdev)
>
>  static const struct of_device_id sbsa_uart_of_match[] = {
>         { .compatible = "arm,sbsa-uart", },
> +       { .compatible = "arm,xlnx-uart", },
>         {},
>  };
>  MODULE_DEVICE_TABLE(of, sbsa_uart_of_match);
> --
> 2.17.1
>