| Message ID | 20240501121742.1215792-18-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISC-V: ACPI: Add external interrupt controller support | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/vmtest-fixes-PR | fail | merge-conflict |
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. ... > + * This driver is for generic 16550 compatible UART enumerated via ACPI > + * platform bus instead of PNP bus like PNP0501. This is not a full This has to be told in the commit message. Anyway, we don't need a duplication code, please use 8250_pnp. ... > + { "RSCV0003", 0 }, Does it have _CID to be PNP0501? If not, add this ID to the 8250_pnp. ... P.S. The code you submitted has a lot of small style issues, I can comment on them if you want, but as I said this code is not needed at all.
On Thu, May 02, 2024 at 12:17:59PM +0300, Andy Shevchenko wrote: > 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. > > ... > > > + * This driver is for generic 16550 compatible UART enumerated via ACPI > > + * platform bus instead of PNP bus like PNP0501. This is not a full > > This has to be told in the commit message. Anyway, we don't need a duplication > code, please use 8250_pnp. > Hi Andy, Thank you for the review!. Major issue with PNP0501 is, it gets enumerated in a different way which causes issue to get _DEP to work. pnpacpi_init() creates PNP data structures which gets skipped if the UART puts _DEP on the GSI provider (interrupt controller). In that case, we need to somehow reinitialize such PNP devices after interrupt controller gets probed. I tried a solution [1] but it required several functions to be moved out of __init. 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? [1] - https://patchwork.kernel.org/project/linux-pci/patch/20240415170113.662318-14-sunilvl@ventanamicro.com/ Thanks, Sunil > ... > > > + { "RSCV0003", 0 }, > > Does it have _CID to be PNP0501? > If not, add this ID to the 8250_pnp. > > ... > > P.S. > The code you submitted has a lot of small style issues, I can comment on them > if you want, but as I said this code is not needed at all. > > -- > With Best Regards, > Andy Shevchenko > >
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: > > On Wed, May 01, 2024 at 05:47:42PM +0530, Sunil V L wrote: ... > > > + * This driver is for generic 16550 compatible UART enumerated via ACPI > > > + * platform bus instead of PNP bus like PNP0501. This is not a full > > > > This has to be told in the commit message. Anyway, we don't need a duplication > > code, please use 8250_pnp. > > Thank you for the review!. Major issue with PNP0501 is, it gets enumerated > in a different way which causes issue to get _DEP to work. > pnpacpi_init() creates PNP data structures which gets skipped if the > UART puts _DEP on the GSI provider (interrupt controller). In that case, > we need to somehow reinitialize such PNP devices after interrupt > controller gets probed. Then fix that code, we don't want a hack driver on top of the existing one for the same. What I might think out of head is that used IRQ core for your case should return a deferred probe error code when it's not ready, then 8250_pnp will get reprobed. > I tried a solution [1] but it required several > functions to be moved out of __init. > 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. > [1] - https://patchwork.kernel.org/project/linux-pci/patch/20240415170113.662318-14-sunilvl@ventanamicro.com/
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: > > > On Wed, May 01, 2024 at 05:47:42PM +0530, Sunil V L wrote: > > ... > > > > > + * This driver is for generic 16550 compatible UART enumerated via ACPI > > > > + * platform bus instead of PNP bus like PNP0501. This is not a full > > > > > > This has to be told in the commit message. Anyway, we don't need a duplication > > > code, please use 8250_pnp. > > > > Thank you for the review!. Major issue with PNP0501 is, it gets enumerated > > in a different way which causes issue to get _DEP to work. > > pnpacpi_init() creates PNP data structures which gets skipped if the > > UART puts _DEP on the GSI provider (interrupt controller). In that case, > > we need to somehow reinitialize such PNP devices after interrupt > > controller gets probed. > > Then fix that code, we don't want a hack driver on top of the existing one for > the same. > > What I might think out of head is that used IRQ core for your case should > return a deferred probe error code when it's not ready, then 8250_pnp will > get reprobed. > Deferred probe was ruled out in prior discussion. Also, deferred probe will not work with _DEP approach. The reason is, PNP devices itself are not getting created from the ACPI name space when they have _DEP. Hence, serial_pnp_probe() will not be called at all. > > I tried a solution [1] but it required several > > functions to be moved out of __init. > > > 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()? Thanks, Sunil
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.
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. Could you please help with a patch even if not compiled so that I can understand your suggestion better? Thanks for your help! Sunil
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