diff mbox series

serial: 8250_mtk: Add ACPI support

Message ID 20250114033324.3533292-1-yenchia.chen@mediatek.com (mailing list archive)
State New
Headers show
Series serial: 8250_mtk: Add ACPI support | expand

Commit Message

Yenchia Chen Jan. 14, 2025, 3:33 a.m. UTC
Add ACPI support to 8250_mtk driver. This makes it possible to
use uart with edk2 UEFI firmware.

Signed-off-by: Yenchia Chen <yenchia.chen@mediatek.com>
---
 drivers/tty/serial/8250/8250_mtk.c | 31 ++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

Comments

Jiri Slaby Jan. 14, 2025, 7:31 a.m. UTC | #1
On 14. 01. 25, 4:33, Yenchia Chen wrote:
> Add ACPI support to 8250_mtk driver. This makes it possible to
> use uart with edk2 UEFI firmware.

Could you mention what hardware this is in particular?

> Signed-off-by: Yenchia Chen <yenchia.chen@mediatek.com>
> ---
>   drivers/tty/serial/8250/8250_mtk.c | 31 ++++++++++++++++++++++++------
>   1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
> index b44de2ed7413..a4f1c30f77b0 100644
> --- a/drivers/tty/serial/8250/8250_mtk.c
> +++ b/drivers/tty/serial/8250/8250_mtk.c
> @@ -19,6 +19,7 @@
>   #include <linux/dma-mapping.h>
>   #include <linux/tty.h>
>   #include <linux/tty_flip.h>
> +#include <linux/acpi.h>

Sort this properly.

>   #include "8250.h"
>   
> @@ -519,6 +520,7 @@ static int mtk8250_probe(struct platform_device *pdev)
>   	struct mtk8250_data *data;
>   	struct resource *regs;
>   	int irq, err;
> +	acpi_handle hdl = ACPI_HANDLE(&pdev->dev);

'hdl' sounds very weird and is not used in the tree for acpi_handles. 
I'd use 'acpi_handle' or 'acpi_dev_handle' in this case.

>   	irq = platform_get_irq(pdev, 0);
>   	if (irq < 0)
> @@ -545,8 +547,12 @@ static int mtk8250_probe(struct platform_device *pdev)
>   		err = mtk8250_probe_of(pdev, &uart.port, data);
>   		if (err)
>   			return err;
> -	} else
> -		return -ENODEV;
> +	} else {
> +		if (!hdl) {

so this should be:
   } else if () {

> +			dev_err(&pdev->dev, "no device\n");

Why this?

> +			return -ENODEV;

As this is self explanatory already, right?

> +		}
> +	}
>   
>   	spin_lock_init(&uart.port.lock);
>   	uart.port.mapbase = regs->start;
> @@ -560,16 +566,22 @@ static int mtk8250_probe(struct platform_device *pdev)
>   	uart.port.private_data = data;
>   	uart.port.shutdown = mtk8250_shutdown;
>   	uart.port.startup = mtk8250_startup;
> -	uart.port.set_termios = mtk8250_set_termios;
> -	uart.port.uartclk = clk_get_rate(data->uart_clk);
> +	if (hdl) {
> +		uart.port.uartclk = 26000000;

This is a magic constant. Define a macro for this. Hint: 26 * HZ_PER_MHZ.

Is it not/cannot it be part of the acpi table? What does MTKI0511 look like?

> +	} else {
> +		uart.port.set_termios = mtk8250_set_termios;
> +		uart.port.uartclk = clk_get_rate(data->uart_clk);
> +	}
>   #ifdef CONFIG_SERIAL_8250_DMA
>   	if (data->dma)
>   		uart.dma = data->dma;
>   #endif
>   
> -	/* Disable Rate Fix function */
> -	writel(0x0, uart.port.membase +
> +	if (!hdl) {
> +		/* Disable Rate Fix function */

Why is this only for non-ACPI devices?

> +		writel(0x0, uart.port.membase +
>   			(MTK_UART_RATE_FIX << uart.port.regshift));
> +	}
>   
>   	platform_set_drvdata(pdev, data);
>   
> @@ -647,11 +659,18 @@ static const struct of_device_id mtk8250_of_match[] = {
>   };
>   MODULE_DEVICE_TABLE(of, mtk8250_of_match);
>   
> +static const struct acpi_device_id mtk8250_acpi_match[] = {
> +	{ "MTKI0511", 0 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(acpi, mtk8250_acpi_match);
> +
>   static struct platform_driver mtk8250_platform_driver = {
>   	.driver = {
>   		.name		= "mt6577-uart",
>   		.pm		= &mtk8250_pm_ops,
>   		.of_match_table	= mtk8250_of_match,
> +		.acpi_match_table = ACPI_PTR(mtk8250_acpi_match),
>   	},
>   	.probe			= mtk8250_probe,
>   	.remove			= mtk8250_remove,

thanks,
Jiri Slaby Jan. 14, 2025, 7:35 a.m. UTC | #2
On 14. 01. 25, 8:31, Jiri Slaby wrote:
>> @@ -560,16 +566,22 @@ static int mtk8250_probe(struct platform_device 
>> *pdev)
>>       uart.port.private_data = data;
>>       uart.port.shutdown = mtk8250_shutdown;
>>       uart.port.startup = mtk8250_startup;
>> -    uart.port.set_termios = mtk8250_set_termios;

Wait, ::set_termios() is NOT optional.
kernel test robot Jan. 15, 2025, 7:45 p.m. UTC | #3
Hi Yenchia,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.13-rc7 next-20250115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yenchia-Chen/serial-8250_mtk-Add-ACPI-support/20250114-113715
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20250114033324.3533292-1-yenchia.chen%40mediatek.com
patch subject: [PATCH] serial: 8250_mtk: Add ACPI support
config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20250116/202501160328.DUMWkTOc-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250116/202501160328.DUMWkTOc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501160328.DUMWkTOc-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/tty/serial/8250/8250_mtk.c:662:36: warning: 'mtk8250_acpi_match' defined but not used [-Wunused-const-variable=]
     662 | static const struct acpi_device_id mtk8250_acpi_match[] = {
         |                                    ^~~~~~~~~~~~~~~~~~


vim +/mtk8250_acpi_match +662 drivers/tty/serial/8250/8250_mtk.c

   661	
 > 662	static const struct acpi_device_id mtk8250_acpi_match[] = {
   663		{ "MTKI0511", 0 },
   664		{},
   665	};
   666	MODULE_DEVICE_TABLE(acpi, mtk8250_acpi_match);
   667
kernel test robot Jan. 15, 2025, 8:48 p.m. UTC | #4
Hi Yenchia,

kernel test robot noticed the following build warnings:

[auto build test WARNING on tty/tty-testing]
[also build test WARNING on tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.13-rc7 next-20250115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Yenchia-Chen/serial-8250_mtk-Add-ACPI-support/20250114-113715
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20250114033324.3533292-1-yenchia.chen%40mediatek.com
patch subject: [PATCH] serial: 8250_mtk: Add ACPI support
config: powerpc-randconfig-001-20250116 (https://download.01.org/0day-ci/archive/20250116/202501160444.iXv8byKW-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project f5cd181ffbb7cb61d582fe130d46580d5969d47a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250116/202501160444.iXv8byKW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202501160444.iXv8byKW-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/tty/serial/8250/8250_mtk.c:662:36: warning: unused variable 'mtk8250_acpi_match' [-Wunused-const-variable]
     662 | static const struct acpi_device_id mtk8250_acpi_match[] = {
         |                                    ^~~~~~~~~~~~~~~~~~
   1 warning generated.


vim +/mtk8250_acpi_match +662 drivers/tty/serial/8250/8250_mtk.c

   661	
 > 662	static const struct acpi_device_id mtk8250_acpi_match[] = {
   663		{ "MTKI0511", 0 },
   664		{},
   665	};
   666	MODULE_DEVICE_TABLE(acpi, mtk8250_acpi_match);
   667
Yenchia Chen Jan. 16, 2025, 3:53 a.m. UTC | #5
> Wait, ::set_termios() is NOT optional.

Got it, will put it back in next version.

> This is a magic constant. Define a macro for this. Hint: 26 * HZ_PER_MHZ.
> Is it not/cannot it be part of the acpi table? What does MTKI0511 look like?

Currently clock settings are not included in ACPI table,  so using fixed clock here.

We'd like to change the flow as shown below to remove the dependency on the ACPI device:

	uart.port.set_termios = mtk8250_set_termios;
	uart.port.uartclk = clk_get_rate(data->uart_clk);
+	if (!uart.port.uartclk)
+		uart.port.uartclk = 26 * HZ_PER_MZ;

> Why is this only for non-ACPI devices?

In ACPI devices, these registers are inaccessible due to some related settings not being configured.
Jonathan Cameron Jan. 16, 2025, 12:16 p.m. UTC | #6
On Thu, 16 Jan 2025 03:45:33 +0800
kernel test robot <lkp@intel.com> wrote:

> Hi Yenchia,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on tty/tty-testing]
> [also build test WARNING on tty/tty-next tty/tty-linus usb/usb-testing usb/usb-next usb/usb-linus linus/master v6.13-rc7 next-20250115]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Yenchia-Chen/serial-8250_mtk-Add-ACPI-support/20250114-113715
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
> patch link:    https://lore.kernel.org/r/20250114033324.3533292-1-yenchia.chen%40mediatek.com
> patch subject: [PATCH] serial: 8250_mtk: Add ACPI support
> config: sh-allyesconfig (https://download.01.org/0day-ci/archive/20250116/202501160328.DUMWkTOc-lkp@intel.com/config)
> compiler: sh4-linux-gcc (GCC) 14.2.0
> reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250116/202501160328.DUMWkTOc-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202501160328.DUMWkTOc-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
> >> drivers/tty/serial/8250/8250_mtk.c:662:36: warning: 'mtk8250_acpi_match' defined but not used [-Wunused-const-variable=]  
>      662 | static const struct acpi_device_id mtk8250_acpi_match[] = {
>          |                                    ^~~~~~~~~~~~~~~~~~
> 
> 
> vim +/mtk8250_acpi_match +662 drivers/tty/serial/8250/8250_mtk.c
> 
>    661	
>  > 662	static const struct acpi_device_id mtk8250_acpi_match[] = {  
>    663		{ "MTKI0511", 0 },
>    664		{},
>    665	};
I'd drop the ACPI_PTR() use and just set it unconditionally.  This table
is tiny so not worth complexity of doing anything else.

>    666	MODULE_DEVICE_TABLE(acpi, mtk8250_acpi_match);
>    667	
>
diff mbox series

Patch

diff --git a/drivers/tty/serial/8250/8250_mtk.c b/drivers/tty/serial/8250/8250_mtk.c
index b44de2ed7413..a4f1c30f77b0 100644
--- a/drivers/tty/serial/8250/8250_mtk.c
+++ b/drivers/tty/serial/8250/8250_mtk.c
@@ -19,6 +19,7 @@ 
 #include <linux/dma-mapping.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
+#include <linux/acpi.h>
 
 #include "8250.h"
 
@@ -519,6 +520,7 @@  static int mtk8250_probe(struct platform_device *pdev)
 	struct mtk8250_data *data;
 	struct resource *regs;
 	int irq, err;
+	acpi_handle hdl = ACPI_HANDLE(&pdev->dev);
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
@@ -545,8 +547,12 @@  static int mtk8250_probe(struct platform_device *pdev)
 		err = mtk8250_probe_of(pdev, &uart.port, data);
 		if (err)
 			return err;
-	} else
-		return -ENODEV;
+	} else {
+		if (!hdl) {
+			dev_err(&pdev->dev, "no device\n");
+			return -ENODEV;
+		}
+	}
 
 	spin_lock_init(&uart.port.lock);
 	uart.port.mapbase = regs->start;
@@ -560,16 +566,22 @@  static int mtk8250_probe(struct platform_device *pdev)
 	uart.port.private_data = data;
 	uart.port.shutdown = mtk8250_shutdown;
 	uart.port.startup = mtk8250_startup;
-	uart.port.set_termios = mtk8250_set_termios;
-	uart.port.uartclk = clk_get_rate(data->uart_clk);
+	if (hdl) {
+		uart.port.uartclk = 26000000;
+	} else {
+		uart.port.set_termios = mtk8250_set_termios;
+		uart.port.uartclk = clk_get_rate(data->uart_clk);
+	}
 #ifdef CONFIG_SERIAL_8250_DMA
 	if (data->dma)
 		uart.dma = data->dma;
 #endif
 
-	/* Disable Rate Fix function */
-	writel(0x0, uart.port.membase +
+	if (!hdl) {
+		/* Disable Rate Fix function */
+		writel(0x0, uart.port.membase +
 			(MTK_UART_RATE_FIX << uart.port.regshift));
+	}
 
 	platform_set_drvdata(pdev, data);
 
@@ -647,11 +659,18 @@  static const struct of_device_id mtk8250_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, mtk8250_of_match);
 
+static const struct acpi_device_id mtk8250_acpi_match[] = {
+	{ "MTKI0511", 0 },
+	{},
+};
+MODULE_DEVICE_TABLE(acpi, mtk8250_acpi_match);
+
 static struct platform_driver mtk8250_platform_driver = {
 	.driver = {
 		.name		= "mt6577-uart",
 		.pm		= &mtk8250_pm_ops,
 		.of_match_table	= mtk8250_of_match,
+		.acpi_match_table = ACPI_PTR(mtk8250_acpi_match),
 	},
 	.probe			= mtk8250_probe,
 	.remove			= mtk8250_remove,