[PATCH v3] serial: 8250_bcm2835aux: Add ACPI support

[PATCH v3] serial: 8250_bcm2835aux: Add ACPI support

[Adrien Thierry]

Add ACPI support to 8250_bcm2835aux driver. This makes it possible to
use the miniuart on the Raspberry Pi with the tianocore/edk2 UEFI
firmware.

Signed-off-by: Adrien Thierry <athierry@redhat.com>
---
V1 -> V2: Refactored code to remove unnecessary conditional paths and
intermediate variables
V2 -> V3: Cleaned up coding style and addressed review comments

 drivers/tty/serial/8250/8250_bcm2835aux.c | 52 ++++++++++++++++++++---
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
index fd95860cd661..2a1226a78a0c 100644
--- a/drivers/tty/serial/8250/8250_bcm2835aux.c
+++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
@@ -17,6 +17,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 
 #include "8250.h"
 
@@ -44,6 +45,10 @@ struct bcm2835aux_data {
 	u32 cntl;
 };
 
+struct bcm2835_aux_serial_driver_data {
+	resource_size_t offset;
+};
+
 static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
 {
 	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
@@ -80,9 +85,12 @@ static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up)
 
 static int bcm2835aux_serial_probe(struct platform_device *pdev)
 {
+	const struct bcm2835_aux_serial_driver_data *bcm_data;
 	struct uart_8250_port up = { };
 	struct bcm2835aux_data *data;
+	resource_size_t offset = 0;
 	struct resource *res;
+	unsigned int uartclk;
 	int ret;
 
 	/* allocate the custom structure */
@@ -109,9 +117,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, data);
 
 	/* get the clock - this also enables the HW */
-	data->clk = devm_clk_get(&pdev->dev, NULL);
-	if (IS_ERR(data->clk))
-		return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), "could not get clk\n");
+	data->clk = devm_clk_get_optional(&pdev->dev, NULL);
 
 	/* get the interrupt */
 	ret = platform_get_irq(pdev, 0);
@@ -125,8 +131,24 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "memory resource not found");
 		return -EINVAL;
 	}
-	up.port.mapbase = res->start;
-	up.port.mapsize = resource_size(res);
+
+	bcm_data = device_get_match_data(&pdev->dev);
+
+	/* Some UEFI implementations (e.g. tianocore/edk2 for the Raspberry Pi)
+	 * describe the miniuart with a base address that encompasses the auxiliary
+	 * registers shared between the miniuart and spi.
+	 *
+	 * This is due to historical reasons, see discussion here :
+	 * https://edk2.groups.io/g/devel/topic/87501357#84349
+	 *
+	 * We need to add the offset between the miniuart and auxiliary
+	 * registers to get the real miniuart base address.
+	 */
+	if (bcm_data)
+		offset = bcm_data->offset;
+
+	up.port.mapbase = res->start + offset;
+	up.port.mapsize = resource_size(res) - offset;
 
 	/* Check for a fixed line number */
 	ret = of_alias_get_id(pdev->dev.of_node, "serial");
@@ -141,12 +163,19 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	uartclk = clk_get_rate(data->clk);
+	if (!uartclk) {
+		ret = device_property_read_u32(&pdev->dev, "clock-frequency", &uartclk);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret, "could not get clk rate\n");
+	}
+
 	/* the HW-clock divider for bcm2835aux is 8,
 	 * but 8250 expects a divider of 16,
 	 * so we have to multiply the actual clock by 2
 	 * to get identical baudrates.
 	 */
-	up.port.uartclk = clk_get_rate(data->clk) * 2;
+	up.port.uartclk = uartclk * 2;
 
 	/* register the port */
 	ret = serial8250_register_8250_port(&up);
@@ -173,16 +202,27 @@ static int bcm2835aux_serial_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct bcm2835_aux_serial_driver_data bcm2835_acpi_data = {
+	.offset = 0x40,
+};
+
 static const struct of_device_id bcm2835aux_serial_match[] = {
 	{ .compatible = "brcm,bcm2835-aux-uart" },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bcm2835aux_serial_match);
 
+static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = {
+	{ "BCM2836", (kernel_ulong_t)&bcm2835_acpi_data },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, bcm2835aux_serial_acpi_match);
+
 static struct platform_driver bcm2835aux_serial_driver = {
 	.driver = {
 		.name = "bcm2835-aux-uart",
 		.of_match_table = bcm2835aux_serial_match,
+		.acpi_match_table = bcm2835aux_serial_acpi_match,
 	},
 	.probe  = bcm2835aux_serial_probe,
 	.remove = bcm2835aux_serial_remove,

base-commit: 2ade8eef993c37a2a43e51a9b1f6c25509a2acce
-- 
2.34.1

Re: [PATCH v3] serial: 8250_bcm2835aux: Add ACPI support

[Florian Fainelli]

On 2/7/22 3:21 PM, Adrien Thierry wrote:
> Add ACPI support to 8250_bcm2835aux driver. This makes it possible to
> use the miniuart on the Raspberry Pi with the tianocore/edk2 UEFI
> firmware.
> 
> Signed-off-by: Adrien Thierry <athierry@redhat.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>

Looks a lot better, thanks!
-- 
Florian

Re: [PATCH v3] serial: 8250_bcm2835aux: Add ACPI support

[Jeremy Linton]

Hi,

On 2/7/22 17:21, Adrien Thierry wrote:
> Add ACPI support to 8250_bcm2835aux driver. This makes it possible to
> use the miniuart on the Raspberry Pi with the tianocore/edk2 UEFI
> firmware.

I merged this to 5.17rc3, switched the console to this device and it now 
works in linux! Thanks for doing this!

Tested-by: Jeremy Linton <jeremy.linton@arm.com>

> 
> Signed-off-by: Adrien Thierry <athierry@redhat.com>
> ---
> V1 -> V2: Refactored code to remove unnecessary conditional paths and
> intermediate variables
> V2 -> V3: Cleaned up coding style and addressed review comments
> 
>   drivers/tty/serial/8250/8250_bcm2835aux.c | 52 ++++++++++++++++++++---
>   1 file changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_bcm2835aux.c b/drivers/tty/serial/8250/8250_bcm2835aux.c
> index fd95860cd661..2a1226a78a0c 100644
> --- a/drivers/tty/serial/8250/8250_bcm2835aux.c
> +++ b/drivers/tty/serial/8250/8250_bcm2835aux.c
> @@ -17,6 +17,7 @@
>   #include <linux/module.h>
>   #include <linux/of.h>
>   #include <linux/platform_device.h>
> +#include <linux/property.h>
>   
>   #include "8250.h"
>   
> @@ -44,6 +45,10 @@ struct bcm2835aux_data {
>   	u32 cntl;
>   };
>   
> +struct bcm2835_aux_serial_driver_data {
> +	resource_size_t offset;
> +};
> +
>   static void bcm2835aux_rs485_start_tx(struct uart_8250_port *up)
>   {
>   	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> @@ -80,9 +85,12 @@ static void bcm2835aux_rs485_stop_tx(struct uart_8250_port *up)
>   
>   static int bcm2835aux_serial_probe(struct platform_device *pdev)
>   {
> +	const struct bcm2835_aux_serial_driver_data *bcm_data;
>   	struct uart_8250_port up = { };
>   	struct bcm2835aux_data *data;
> +	resource_size_t offset = 0;
>   	struct resource *res;
> +	unsigned int uartclk;
>   	int ret;
>   
>   	/* allocate the custom structure */
> @@ -109,9 +117,7 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
>   	platform_set_drvdata(pdev, data);
>   
>   	/* get the clock - this also enables the HW */
> -	data->clk = devm_clk_get(&pdev->dev, NULL);
> -	if (IS_ERR(data->clk))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), "could not get clk\n");
> +	data->clk = devm_clk_get_optional(&pdev->dev, NULL);
>   
>   	/* get the interrupt */
>   	ret = platform_get_irq(pdev, 0);
> @@ -125,8 +131,24 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
>   		dev_err(&pdev->dev, "memory resource not found");
>   		return -EINVAL;
>   	}
> -	up.port.mapbase = res->start;
> -	up.port.mapsize = resource_size(res);
> +
> +	bcm_data = device_get_match_data(&pdev->dev);
> +
> +	/* Some UEFI implementations (e.g. tianocore/edk2 for the Raspberry Pi)
> +	 * describe the miniuart with a base address that encompasses the auxiliary
> +	 * registers shared between the miniuart and spi.
> +	 *
> +	 * This is due to historical reasons, see discussion here :
> +	 * https://edk2.groups.io/g/devel/topic/87501357#84349
> +	 *
> +	 * We need to add the offset between the miniuart and auxiliary
> +	 * registers to get the real miniuart base address.
> +	 */
> +	if (bcm_data)
> +		offset = bcm_data->offset;
> +
> +	up.port.mapbase = res->start + offset;
> +	up.port.mapsize = resource_size(res) - offset;
>   
>   	/* Check for a fixed line number */
>   	ret = of_alias_get_id(pdev->dev.of_node, "serial");
> @@ -141,12 +163,19 @@ static int bcm2835aux_serial_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> +	uartclk = clk_get_rate(data->clk);
> +	if (!uartclk) {
> +		ret = device_property_read_u32(&pdev->dev, "clock-frequency", &uartclk);
> +		if (ret)
> +			return dev_err_probe(&pdev->dev, ret, "could not get clk rate\n");
> +	}
> +
>   	/* the HW-clock divider for bcm2835aux is 8,
>   	 * but 8250 expects a divider of 16,
>   	 * so we have to multiply the actual clock by 2
>   	 * to get identical baudrates.
>   	 */
> -	up.port.uartclk = clk_get_rate(data->clk) * 2;
> +	up.port.uartclk = uartclk * 2;
>   
>   	/* register the port */
>   	ret = serial8250_register_8250_port(&up);
> @@ -173,16 +202,27 @@ static int bcm2835aux_serial_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static const struct bcm2835_aux_serial_driver_data bcm2835_acpi_data = {
> +	.offset = 0x40,
> +};
> +
>   static const struct of_device_id bcm2835aux_serial_match[] = {
>   	{ .compatible = "brcm,bcm2835-aux-uart" },
>   	{ },
>   };
>   MODULE_DEVICE_TABLE(of, bcm2835aux_serial_match);
>   
> +static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = {
> +	{ "BCM2836", (kernel_ulong_t)&bcm2835_acpi_data },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, bcm2835aux_serial_acpi_match);
> +
>   static struct platform_driver bcm2835aux_serial_driver = {
>   	.driver = {
>   		.name = "bcm2835-aux-uart",
>   		.of_match_table = bcm2835aux_serial_match,
> +		.acpi_match_table = bcm2835aux_serial_acpi_match,
>   	},
>   	.probe  = bcm2835aux_serial_probe,
>   	.remove = bcm2835aux_serial_remove,
> 
> base-commit: 2ade8eef993c37a2a43e51a9b1f6c25509a2acce
> 

Re: [PATCH v3] serial: 8250_bcm2835aux: Add ACPI support

[Andy Shevchenko]

On Tue, Feb 8, 2022 at 11:13 AM Adrien Thierry <athierry@redhat.com> wrote:
>
> Add ACPI support to 8250_bcm2835aux driver. This makes it possible to
> use the miniuart on the Raspberry Pi with the tianocore/edk2 UEFI

TianoCore EDK II

> firmware.

...

>         /* get the clock - this also enables the HW */
> -       data->clk = devm_clk_get(&pdev->dev, NULL);

> -       if (IS_ERR(data->clk))
> -               return dev_err_probe(&pdev->dev, PTR_ERR(data->clk), "could not get clk\n");

You shouldn't remove the error handling. Even if optional there may be
other types of errors that need to be reported.

> +       data->clk = devm_clk_get_optional(&pdev->dev, NULL);

...


> +       bcm_data = device_get_match_data(&pdev->dev);

Move this closer to the condition where it's used the first time.

> +       /* Some UEFI implementations (e.g. tianocore/edk2 for the Raspberry Pi)
.
If there are some not doing that, can we end up in the situation when
for the same ID we have different offset?
Also, Why not go and fix that implementation?
Can you provide a DSDT excerpt to show how it looks there?

TianoCore EDK II

/*
 * Multi-line comments should look
 * like this.
 */

> +        * describe the miniuart with a base address that encompasses the auxiliary
> +        * registers shared between the miniuart and spi.

SPI

> +        *
> +        * This is due to historical reasons, see discussion here :
> +        * https://edk2.groups.io/g/devel/topic/87501357#84349
> +        *
> +        * We need to add the offset between the miniuart and auxiliary
> +        * registers to get the real miniuart base address.
> +        */
> +       if (bcm_data)
> +               offset = bcm_data->offset;

...

> +static const struct acpi_device_id bcm2835aux_serial_acpi_match[] = {
> +       { "BCM2836", (kernel_ulong_t)&bcm2835_acpi_data },
> +       { }
> +};

I believe this ID is allocated by Broadcom. Can we have a confirmation, please?

-- 
With Best Regards,
Andy Shevchenko