| Message ID | 20240501121742.1215792-18-sunilvl@ventanamicro.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v5,01/17] arm64: PCI: Migrate ACPI related functions to pci-acpi.c | expand |
On Fri, May 03, 2024 at 07:29:35PM +0530, Sunil V L wrote: > On Thu, May 02, 2024 at 06:35:52PM +0300, Andy Shevchenko wrote: > > On Thu, May 02, 2024 at 04:50:57PM +0530, Sunil V L wrote: > > > On Thu, May 02, 2024 at 01:09:57PM +0300, Andy Shevchenko wrote: > > > > On Thu, May 02, 2024 at 03:20:08PM +0530, Sunil V L wrote: > > > > > On Thu, May 02, 2024 at 12:17:59PM +0300, Andy Shevchenko wrote: ... > > > > > This driver is not a duplicate of 8250_pnp. It just relies on UART > > > > > enumerated as platform device instead of using PNP interfaces. > > > > > Isn't it better and simple to have an option to enumerate as platform > > > > > device instead of PNP? > > > > > > > > Ah, then extract platform driver first from 8250_core.c. > > > > > > > Let me know if I understand your suggestion correctly. Do you mean call > > > something like serial8250_acpi_init() from serial8250_init() and > > > register the driver directly in serial8250_acpi_init()? > > > > Extract the code to be 8250_platform.c and update that file. > > I have locally the extraction of RSA code, I will see if I can help you > > with the rest. > > > Thanks!. That will be helpful. TBH, I don't understand what to do for > extracting the platform driver code. There are already several vendor > specific UART drivers (ex: 8250_fsl.c) which are enumerated as platform > devices. 8250_core.c looks cleanly supporting such drivers which can > register themselves with the core. For generic UART, DT has 8250_of.c > and ACPI has 8250_pnp.c. But 8250_pnp.c comes with baggage of PNP > contract. So, the driver in this patch is similar to vendor specific > drivers to support generic uart devices which are enumerated as platform > device. > I can rename 8250_acpi.c to 8250_platform.c if that is better. No, that's not what I meant. We _already_ have a generic platform driver, it's just inline in the 8250_core.c and needs to be extracted. When it's done, you may simply add an ACPI table to it. > Could you please help with a patch even if not compiled so that I can > understand your suggestion better?
On Wed, May 01, 2024 at 05:47:42PM +0530, Sunil V L wrote: > RISC-V has non-PNP generic 16550A compatible UART which needs to be > enumerated as ACPI platform device. Add driver support for such devices > similar to 8250_of. > > The driver is enabled when the CONFIG_SERIAL_ACPI_PLATFORM option is > enabled. Enable this option for RISC-V. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > --- > arch/riscv/configs/defconfig | 1 + > drivers/tty/serial/8250/8250_acpi.c | 96 +++++++++++++++++++++++++++++ > drivers/tty/serial/8250/Kconfig | 8 +++ > drivers/tty/serial/8250/Makefile | 1 + > 4 files changed, 106 insertions(+) > create mode 100644 drivers/tty/serial/8250/8250_acpi.c > > diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig > index 3cae018f9315..bea8241f52eb 100644 > --- a/arch/riscv/configs/defconfig > +++ b/arch/riscv/configs/defconfig > @@ -150,6 +150,7 @@ CONFIG_SERIAL_8250=y > CONFIG_SERIAL_8250_CONSOLE=y > CONFIG_SERIAL_8250_DW=y > CONFIG_SERIAL_OF_PLATFORM=y > +CONFIG_SERIAL_ACPI_PLATFORM=y > CONFIG_SERIAL_SH_SCI=y > CONFIG_SERIAL_EARLYCON_RISCV_SBI=y > CONFIG_VIRTIO_CONSOLE=y > diff --git a/drivers/tty/serial/8250/8250_acpi.c b/drivers/tty/serial/8250/8250_acpi.c > new file mode 100644 > index 000000000000..3682443bb69c > --- /dev/null > +++ b/drivers/tty/serial/8250/8250_acpi.c > @@ -0,0 +1,96 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Serial Port driver for ACPI platform devices > + * > + * This driver is for generic 16550 compatible UART enumerated via ACPI > + * platform bus instead of PNP bus like PNP0501. This is not a full > + * driver but mostly provides the ACPI wrapper and uses generic > + * 8250 framework for rest of the functionality. No copyright line? Odd, but ok, I'll take it, glad to see your company finally realizes the lack of needing them :) And as Andy said, please use the existing driver and extend what you need, don't write a new one, we really don't need a new one. > +static int acpi_platform_serial_probe(struct platform_device *pdev) > +{ > + struct acpi_serial_info *data; > + struct uart_8250_port port8250; > + struct device *dev = &pdev->dev; > + struct resource *regs; > + > + int ret, irq; > + > + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!regs) { > + dev_err(dev, "no registers defined\n"); > + return -EINVAL; > + } > + > + irq = platform_get_irq(pdev, 0); > + if (irq < 0) > + return irq; > + > + memset(&port8250, 0, sizeof(port8250)); > + > + spin_lock_init(&port8250.port.lock); Are you sure this works? thanks, greg k-h
On Fri, May 03, 2024 at 06:32:10PM +0300, Andy Shevchenko wrote: > On Fri, May 03, 2024 at 07:29:35PM +0530, Sunil V L wrote: > > On Thu, May 02, 2024 at 06:35:52PM +0300, Andy Shevchenko wrote: > > > On Thu, May 02, 2024 at 04:50:57PM +0530, Sunil V L wrote: > > > > On Thu, May 02, 2024 at 01:09:57PM +0300, Andy Shevchenko wrote: > > > > > On Thu, May 02, 2024 at 03:20:08PM +0530, Sunil V L wrote: > > > > > > On Thu, May 02, 2024 at 12:17:59PM +0300, Andy Shevchenko wrote: > > ... > > > > > > > This driver is not a duplicate of 8250_pnp. It just relies on UART > > > > > > enumerated as platform device instead of using PNP interfaces. > > > > > > Isn't it better and simple to have an option to enumerate as platform > > > > > > device instead of PNP? > > > > > > > > > > Ah, then extract platform driver first from 8250_core.c. > > > > > > > > > Let me know if I understand your suggestion correctly. Do you mean call > > > > something like serial8250_acpi_init() from serial8250_init() and > > > > register the driver directly in serial8250_acpi_init()? > > > > > > Extract the code to be 8250_platform.c and update that file. > > > I have locally the extraction of RSA code, I will see if I can help you > > > with the rest. > > > > > Thanks!. That will be helpful. TBH, I don't understand what to do for > > extracting the platform driver code. There are already several vendor > > specific UART drivers (ex: 8250_fsl.c) which are enumerated as platform > > devices. 8250_core.c looks cleanly supporting such drivers which can > > register themselves with the core. For generic UART, DT has 8250_of.c > > and ACPI has 8250_pnp.c. But 8250_pnp.c comes with baggage of PNP > > contract. So, the driver in this patch is similar to vendor specific > > drivers to support generic uart devices which are enumerated as platform > > device. > > > I can rename 8250_acpi.c to 8250_platform.c if that is better. > > No, that's not what I meant. We _already_ have a generic platform driver, > it's just inline in the 8250_core.c and needs to be extracted. When it's done, > you may simply add an ACPI table to it. > > > Could you please help with a patch even if not compiled so that I can > > understand your suggestion better? > Okay, Thanks! Andy. IIUC, you want to extract serial8250_isa_driver from 8250_core and then add ACPI support. I was not sure about that since I thought it was only for legacy ISA devices and they seem to be created by OS itself instead of discovery. ISA driver has some ordering dependency on PNP driver as well. Anyway, let me try if that is the acceptable way forward. Thanks, Sunil
diff --git a/arch/riscv/configs/defconfig b/arch/riscv/configs/defconfig index 3cae018f9315..bea8241f52eb 100644 --- a/arch/riscv/configs/defconfig +++ b/arch/riscv/configs/defconfig @@ -150,6 +150,7 @@ CONFIG_SERIAL_8250=y CONFIG_SERIAL_8250_CONSOLE=y CONFIG_SERIAL_8250_DW=y CONFIG_SERIAL_OF_PLATFORM=y +CONFIG_SERIAL_ACPI_PLATFORM=y CONFIG_SERIAL_SH_SCI=y CONFIG_SERIAL_EARLYCON_RISCV_SBI=y CONFIG_VIRTIO_CONSOLE=y diff --git a/drivers/tty/serial/8250/8250_acpi.c b/drivers/tty/serial/8250/8250_acpi.c new file mode 100644 index 000000000000..3682443bb69c --- /dev/null +++ b/drivers/tty/serial/8250/8250_acpi.c @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Serial Port driver for ACPI platform devices + * + * This driver is for generic 16550 compatible UART enumerated via ACPI + * platform bus instead of PNP bus like PNP0501. This is not a full + * driver but mostly provides the ACPI wrapper and uses generic + * 8250 framework for rest of the functionality. + */ + +#include <linux/acpi.h> +#include <linux/serial_reg.h> +#include <linux/serial_8250.h> + +#include "8250.h" + +struct acpi_serial_info { + int line; +}; + +static int acpi_platform_serial_probe(struct platform_device *pdev) +{ + struct acpi_serial_info *data; + struct uart_8250_port port8250; + struct device *dev = &pdev->dev; + struct resource *regs; + + int ret, irq; + + regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!regs) { + dev_err(dev, "no registers defined\n"); + return -EINVAL; + } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) + return irq; + + memset(&port8250, 0, sizeof(port8250)); + + spin_lock_init(&port8250.port.lock); + + port8250.port.mapbase = regs->start; + port8250.port.irq = irq; + port8250.port.type = PORT_16550A; + port8250.port.flags = UPF_SHARE_IRQ | UPF_BOOT_AUTOCONF | UPF_FIXED_PORT | + UPF_IOREMAP | UPF_FIXED_TYPE; + port8250.port.dev = dev; + port8250.port.mapsize = resource_size(regs); + port8250.port.iotype = UPIO_MEM; + port8250.port.irqflags = IRQF_SHARED; + + port8250.port.membase = devm_ioremap(dev, port8250.port.mapbase, port8250.port.mapsize); + if (!port8250.port.membase) + return -ENOMEM; + + ret = uart_read_and_validate_port_properties(&port8250.port); + if (ret) + return -EINVAL; + + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->line = serial8250_register_8250_port(&port8250); + if (data->line < 0) + return data->line; + + platform_set_drvdata(pdev, data); + return 0; +} + +static void acpi_platform_serial_remove(struct platform_device *pdev) +{ + struct acpi_serial_info *data = platform_get_drvdata(pdev); + + serial8250_unregister_port(data->line); +} + +static const struct acpi_device_id acpi_platform_serial_table[] = { + { "RSCV0003", 0 }, + { }, +}; +MODULE_DEVICE_TABLE(acpi, acpi_platform_serial_table); + +static struct platform_driver acpi_platform_serial_driver = { + .driver = { + .name = "acpi_serial", + .acpi_match_table = ACPI_PTR(acpi_platform_serial_table), + }, + .probe = acpi_platform_serial_probe, + .remove_new = acpi_platform_serial_remove, +}; + +builtin_platform_driver(acpi_platform_serial_driver); diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig index 47ff50763c04..fbfe4d3501b1 100644 --- a/drivers/tty/serial/8250/Kconfig +++ b/drivers/tty/serial/8250/Kconfig @@ -576,3 +576,11 @@ config SERIAL_OF_PLATFORM are probed through devicetree, including Open Firmware based PowerPC systems and embedded systems on architectures using the flattened device tree format. + +config SERIAL_ACPI_PLATFORM + tristate "ACPI platform bus based probing for 8250 ports" + depends on SERIAL_8250 && ACPI + default n + help + This option is used for generic 8250 compatible serial ports + that are enumerated through ACPI platform bus. diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile index ea2e81f58eac..8c0ef357fc4e 100644 --- a/drivers/tty/serial/8250/Makefile +++ b/drivers/tty/serial/8250/Makefile @@ -18,6 +18,7 @@ obj-$(CONFIG_SERIAL_8250_CONSOLE) += 8250_early.o obj-$(CONFIG_SERIAL_8250_ACCENT) += 8250_accent.o obj-$(CONFIG_SERIAL_8250_ACORN) += 8250_acorn.o +obj-$(CONFIG_SERIAL_ACPI_PLATFORM) += 8250_acpi.o obj-$(CONFIG_SERIAL_8250_ASPEED_VUART) += 8250_aspeed_vuart.o obj-$(CONFIG_SERIAL_8250_BCM2835AUX) += 8250_bcm2835aux.o obj-$(CONFIG_SERIAL_8250_BCM7271) += 8250_bcm7271.o
RISC-V has non-PNP generic 16550A compatible UART which needs to be enumerated as ACPI platform device. Add driver support for such devices similar to 8250_of. The driver is enabled when the CONFIG_SERIAL_ACPI_PLATFORM option is enabled. Enable this option for RISC-V. Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> --- arch/riscv/configs/defconfig | 1 + drivers/tty/serial/8250/8250_acpi.c | 96 +++++++++++++++++++++++++++++ drivers/tty/serial/8250/Kconfig | 8 +++ drivers/tty/serial/8250/Makefile | 1 + 4 files changed, 106 insertions(+) create mode 100644 drivers/tty/serial/8250/8250_acpi.c