| Message ID | 20240501121742.1215792-1-sunilvl@ventanamicro.com |
|---|---|
| Headers | show |
| Series | RISC-V: ACPI: Add external interrupt controller support | expand |
On Wed, May 01, 2024 at 05:47:26PM +0530, Sunil V L wrote: > The functions defined in arm64 for ACPI support are required > for RISC-V also. To avoid duplication, move these functions > to common location. > > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com> > Acked-by: Bjorn Helgaas <bhelgaas@google.com> > --- > arch/arm64/kernel/pci.c | 191 ---------------------------------------- > drivers/pci/pci-acpi.c | 182 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 182 insertions(+), 191 deletions(-) Looks like a straight-forward move of the code, so: Acked-by: Will Deacon <will@kernel.org> Will
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 Wed, May 01, 2024 at 05:47:26PM +0530, Sunil V L wrote: > The functions defined in arm64 for ACPI support are required > for RISC-V also. To avoid duplication, move these functions > to common location. ... There are -M -C parameters to git format-patch. Use them in the next version of the series. (Note, you may add percentage numbers to each of those parameters to get prettier result.)
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 12:22:28PM +0300, Andy Shevchenko wrote: > On Wed, May 01, 2024 at 05:47:26PM +0530, Sunil V L wrote: > > The functions defined in arm64 for ACPI support are required > > for RISC-V also. To avoid duplication, move these functions > > to common location. > > ... > > There are -M -C parameters to git format-patch. Use them in the next version of > the series. (Note, you may add percentage numbers to each of those parameters > to get prettier result.) > Yeah, I tried different combination of percentage after you told me last time. I just didn't see any difference in the patch generated. Let me try again while sending next version. Thanks!
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 11:12:43AM +0100, Sudeep Holla wrote: > On Thu, May 02, 2024 at 03:32:25PM +0530, Sunil V L wrote: > > On Thu, May 02, 2024 at 12:24:14PM +0300, Andy Shevchenko wrote: > > > On Wed, May 01, 2024 at 05:47:28PM +0530, Sunil V L wrote: > > > > Add a new function for RISC-V to do any architecture specific > > > > initialization. This function will be used to create platform devices > > > > like APLIC, PLIC, RISC-V IOMMU etc. This is similar to acpi_arm_init(). > > > > > > What is the special about this architecture that it requires a separate > > > initialization that is _not_ going to be in other cases? > > > Please, elaborate. > > > > > This init function will be used to create GSI mapping structures and in > > future may be others like iommu. Like I mentioned, ARM already has > > similar function acpi_arm_init(). So, it is not new right? > > Just to add: > > This is to initialise everything around all the arch specific tables > which you will not have on any other architectures. We could execute > on all architectures but the tables will never be found. The main point > is why do we want to do that if we can optimise and skip on all other > archs. You need to carefully write the commit messages. Some kind of the above paragraphs has to be in there.
On Thu, May 02, 2024 at 01:19:55PM +0300, Andy Shevchenko wrote: > On Thu, May 02, 2024 at 11:12:43AM +0100, Sudeep Holla wrote: > > On Thu, May 02, 2024 at 03:32:25PM +0530, Sunil V L wrote: > > > On Thu, May 02, 2024 at 12:24:14PM +0300, Andy Shevchenko wrote: > > > > On Wed, May 01, 2024 at 05:47:28PM +0530, Sunil V L wrote: > > > > > Add a new function for RISC-V to do any architecture specific > > > > > initialization. This function will be used to create platform devices > > > > > like APLIC, PLIC, RISC-V IOMMU etc. This is similar to acpi_arm_init(). > > > > > > > > What is the special about this architecture that it requires a separate > > > > initialization that is _not_ going to be in other cases? > > > > Please, elaborate. > > > > > > > This init function will be used to create GSI mapping structures and in > > > future may be others like iommu. Like I mentioned, ARM already has > > > similar function acpi_arm_init(). So, it is not new right? > > > > Just to add: > > > > This is to initialise everything around all the arch specific tables > > which you will not have on any other architectures. We could execute > > on all architectures but the tables will never be found. The main point > > is why do we want to do that if we can optimise and skip on all other > > archs. > > You need to carefully write the commit messages. Some kind of the above > paragraphs has to be in there. > Sure, let me update the commit message on similar lines. Thanks, Sunil
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 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