| Message ID | 20230113171409.30470-25-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | dmaengine: dw-edma: Add RP/EP local DMA controllers support | expand |
On Fri, Jan 13, 2023 at 08:14:06PM +0300, Serge Semin wrote: > Since the DW PCIe RP/EP driver is about to be updated to register the DW > eDMA-based DMA-engine the drivers build modes must be synchronized. > Currently the DW PCIe RP/EP driver is always built as a builtin module. > Meanwhile the DW eDMA driver can be built as a loadable module. Thus in > the later case the kernel with DW PCIe controllers support will fail to be > linked due to lacking the DW eDMA probe/remove symbols. At the same time > forcibly selecting the DW eDMA driver from the DW PCIe RP/EP kconfig will > effectively eliminate the tristate type of the former driver fixing it to > just the builtin kernel module. > > Seeing the DW eDMA engine isn't that often met built into the DW PCIe > Root-ports and End-points let's convert the DW eDMA driver config to being > more flexible instead of just forcibly selecting the DW eDMA kconfig. In > order to do that first the DW eDMA PCIe driver config should be converted > to being depended from the DW eDMA core config instead of selecting the > one. Second the DW eDMA probe and remove symbols should be referenced only > if they are reachable by the caller. Thus the user will be able to build > the DW eDMA core driver with any type, meanwhile the dependent code will > be either restricted to the same build type (e.g. DW eDMA PCIe driver if > DW eDMA driver is built as a loadable module) or just won't be able to use > the eDMA engine registration/de-registration functionality (e.g. DW PCIe > RP/EP driver if DW eDMA driver is built as a loadable module). I'm trying to write the merge commit log, and I understand the linking issue, but I'm having a hard time figuring out what the user-visible scenarios are here. I assume there's something that works when CONFIG_PCIE_DW=y and CONFIG_DW_EDMA_PCIE=y but does *not* work when CONFIG_PCIE_DW=y and CONFIG_DW_EDMA_PCIE=m? If both scenarios worked the same, I would think the existing dw_edma_pcie_probe() would be enough, and you wouldn't need to call dw_pcie_edma_detect() from dw_pcie_host_init() and dw_pcie_ep_init(). > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > --- > > Changelog v8: > - This is a new patch added on v8 stage of the series in order to fix > the tbot-reported build issues. (@tbot) > --- > drivers/dma/dw-edma/Kconfig | 5 ++++- > include/linux/dma/edma.h | 2 +- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/dma/dw-edma/Kconfig b/drivers/dma/dw-edma/Kconfig > index 7ff17b2db6a1..2b6f2679508d 100644 > --- a/drivers/dma/dw-edma/Kconfig > +++ b/drivers/dma/dw-edma/Kconfig > @@ -9,11 +9,14 @@ config DW_EDMA > Support the Synopsys DesignWare eDMA controller, normally > implemented on endpoints SoCs. > > +if DW_EDMA > + > config DW_EDMA_PCIE > tristate "Synopsys DesignWare eDMA PCIe driver" > depends on PCI && PCI_MSI > - select DW_EDMA > help > Provides a glue-logic between the Synopsys DesignWare > eDMA controller and an endpoint PCIe device. This also serves > as a reference design to whom desires to use this IP. > + > +endif # DW_EDMA > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > index 08833f12b386..c062c8db472c 100644 > --- a/include/linux/dma/edma.h > +++ b/include/linux/dma/edma.h > @@ -101,7 +101,7 @@ struct dw_edma_chip { > }; > > /* Export to the platform drivers */ > -#if IS_ENABLED(CONFIG_DW_EDMA) > +#if IS_REACHABLE(CONFIG_DW_EDMA) > int dw_edma_probe(struct dw_edma_chip *chip); > int dw_edma_remove(struct dw_edma_chip *chip); > #else > -- > 2.39.0 > >
On Fri, Jan 20, 2023 at 04:50:36PM -0600, Bjorn Helgaas wrote: > On Fri, Jan 13, 2023 at 08:14:06PM +0300, Serge Semin wrote: > > Since the DW PCIe RP/EP driver is about to be updated to register the DW > > eDMA-based DMA-engine the drivers build modes must be synchronized. > > Currently the DW PCIe RP/EP driver is always built as a builtin module. > > Meanwhile the DW eDMA driver can be built as a loadable module. Thus in > > the later case the kernel with DW PCIe controllers support will fail to be > > linked due to lacking the DW eDMA probe/remove symbols. At the same time > > forcibly selecting the DW eDMA driver from the DW PCIe RP/EP kconfig will > > effectively eliminate the tristate type of the former driver fixing it to > > just the builtin kernel module. > > > > Seeing the DW eDMA engine isn't that often met built into the DW PCIe > > Root-ports and End-points let's convert the DW eDMA driver config to being > > more flexible instead of just forcibly selecting the DW eDMA kconfig. In > > order to do that first the DW eDMA PCIe driver config should be converted > > to being depended from the DW eDMA core config instead of selecting the > > one. Second the DW eDMA probe and remove symbols should be referenced only > > if they are reachable by the caller. Thus the user will be able to build > > the DW eDMA core driver with any type, meanwhile the dependent code will > > be either restricted to the same build type (e.g. DW eDMA PCIe driver if > > DW eDMA driver is built as a loadable module) or just won't be able to use > > the eDMA engine registration/de-registration functionality (e.g. DW PCIe > > RP/EP driver if DW eDMA driver is built as a loadable module). > > I'm trying to write the merge commit log, and I understand the linking > issue, but I'm having a hard time figuring out what the user-visible > scenarios are here. > > I assume there's something that works when CONFIG_PCIE_DW=y and > CONFIG_DW_EDMA_PCIE=y but does *not* work when CONFIG_PCIE_DW=y and > CONFIG_DW_EDMA_PCIE=m? No. The DW eDMA code availability (in other words the CONFIG_DW_EDMA config value) determines whether the corresponding driver (DW PCIe RP/EP or DW eDMA PCI) is capable to perform the eDMA engine probe procedure. Additionally both drivers has the opposite dependency from the DW eDMA code. | | DW PCIe RP/EP | DW eDMA PCIe | | CONFIG_DW_EDMA +----------------------+----------------------+ | | Probe eDMA | KConfig | Probe eDMA | Kconfig | +----------------+------------+---------+------------+---------+ | y | YES | y,n | YES | y,m,n | | m | NO | y,n | YES | m,n | | n | NO | y,n | NO | n | +--------------------------------------------------------------+ Basically it means the DW PCIe RP/EP driver will be able to probe the DW eDMA engine only if the corresponding driver is built into the kernel. At the same time the DW PCIe RP/EP driver doesn't depend on the DW eDMA core module config state. The DW eDMA PCIe driver in opposite depends on the DW eDMA code config state, but will always be able to probe the DW eDMA engine as long as the corresponding code is loaded as either a part of the kernel or as a loadable module. > > If both scenarios worked the same, I would think the existing > dw_edma_pcie_probe() would be enough, and you wouldn't need to call > dw_pcie_edma_detect() from dw_pcie_host_init() and dw_pcie_ep_init(). No. These methods have been implemented for the absolutely different drivers. dw_edma_pcie_probe() is called for an end-point PCIe-device found on a PCIe-bus. dw_pcie_host_init()/dw_pcie_ep_init() and dw_pcie_edma_detect() are called for a platform-device representing a DW PCIe RP/EP controller. In other words dw_pcie_edma_detect() and dw_edma_pcie_probe() are in no means interchangeable. -Serge(y) > > > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > --- > > > > Changelog v8: > > - This is a new patch added on v8 stage of the series in order to fix > > the tbot-reported build issues. (@tbot) > > --- > > drivers/dma/dw-edma/Kconfig | 5 ++++- > > include/linux/dma/edma.h | 2 +- > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/dma/dw-edma/Kconfig b/drivers/dma/dw-edma/Kconfig > > index 7ff17b2db6a1..2b6f2679508d 100644 > > --- a/drivers/dma/dw-edma/Kconfig > > +++ b/drivers/dma/dw-edma/Kconfig > > @@ -9,11 +9,14 @@ config DW_EDMA > > Support the Synopsys DesignWare eDMA controller, normally > > implemented on endpoints SoCs. > > > > +if DW_EDMA > > + > > config DW_EDMA_PCIE > > tristate "Synopsys DesignWare eDMA PCIe driver" > > depends on PCI && PCI_MSI > > - select DW_EDMA > > help > > Provides a glue-logic between the Synopsys DesignWare > > eDMA controller and an endpoint PCIe device. This also serves > > as a reference design to whom desires to use this IP. > > + > > +endif # DW_EDMA > > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h > > index 08833f12b386..c062c8db472c 100644 > > --- a/include/linux/dma/edma.h > > +++ b/include/linux/dma/edma.h > > @@ -101,7 +101,7 @@ struct dw_edma_chip { > > }; > > > > /* Export to the platform drivers */ > > -#if IS_ENABLED(CONFIG_DW_EDMA) > > +#if IS_REACHABLE(CONFIG_DW_EDMA) > > int dw_edma_probe(struct dw_edma_chip *chip); > > int dw_edma_remove(struct dw_edma_chip *chip); > > #else > > -- > > 2.39.0 > > > >
On Sun, Jan 22, 2023 at 03:11:16AM +0300, Serge Semin wrote: > On Fri, Jan 20, 2023 at 04:50:36PM -0600, Bjorn Helgaas wrote: > > On Fri, Jan 13, 2023 at 08:14:06PM +0300, Serge Semin wrote: > > > Since the DW PCIe RP/EP driver is about to be updated to register the DW > > > eDMA-based DMA-engine the drivers build modes must be synchronized. > > > Currently the DW PCIe RP/EP driver is always built as a builtin module. > > > Meanwhile the DW eDMA driver can be built as a loadable module. Thus in > > > the later case the kernel with DW PCIe controllers support will fail to be > > > linked due to lacking the DW eDMA probe/remove symbols. At the same time > > > forcibly selecting the DW eDMA driver from the DW PCIe RP/EP kconfig will > > > effectively eliminate the tristate type of the former driver fixing it to > > > just the builtin kernel module. > > > > > > Seeing the DW eDMA engine isn't that often met built into the DW PCIe > > > Root-ports and End-points let's convert the DW eDMA driver config to being > > > more flexible instead of just forcibly selecting the DW eDMA kconfig. In > > > order to do that first the DW eDMA PCIe driver config should be converted > > > to being depended from the DW eDMA core config instead of selecting the > > > one. Second the DW eDMA probe and remove symbols should be referenced only > > > if they are reachable by the caller. Thus the user will be able to build > > > the DW eDMA core driver with any type, meanwhile the dependent code will > > > be either restricted to the same build type (e.g. DW eDMA PCIe driver if > > > DW eDMA driver is built as a loadable module) or just won't be able to use > > > the eDMA engine registration/de-registration functionality (e.g. DW PCIe > > > RP/EP driver if DW eDMA driver is built as a loadable module). > > > > I'm trying to write the merge commit log, and I understand the linking > > issue, but I'm having a hard time figuring out what the user-visible > > scenarios are here. > > > > I assume there's something that works when CONFIG_PCIE_DW=y and > > CONFIG_DW_EDMA_PCIE=y but does *not* work when CONFIG_PCIE_DW=y and > > CONFIG_DW_EDMA_PCIE=m? > > No. The DW eDMA code availability (in other words the CONFIG_DW_EDMA > config value) determines whether the corresponding driver (DW PCIe > RP/EP or DW eDMA PCI) is capable to perform the eDMA engine probe > procedure. Additionally both drivers has the opposite dependency from > the DW eDMA code. > | | DW PCIe RP/EP | DW eDMA PCIe | > | CONFIG_DW_EDMA +----------------------+----------------------+ > | | Probe eDMA | KConfig | Probe eDMA | Kconfig | > +----------------+------------+---------+------------+---------+ > | y | YES | y,n | YES | y,m,n | > | m | NO | y,n | YES | m,n | > | n | NO | y,n | NO | n | > +--------------------------------------------------------------+ > > Basically it means the DW PCIe RP/EP driver will be able to probe the > DW eDMA engine only if the corresponding driver is built into the > kernel. At the same time the DW PCIe RP/EP driver doesn't depend on > the DW eDMA core module config state. The DW eDMA PCIe driver in > opposite depends on the DW eDMA code config state, but will always be > able to probe the DW eDMA engine as long as the corresponding code is > loaded as either a part of the kernel or as a loadable module. > > > If both scenarios worked the same, I would think the existing > > dw_edma_pcie_probe() would be enough, and you wouldn't need to call > > dw_pcie_edma_detect() from dw_pcie_host_init() and dw_pcie_ep_init(). > > No. These methods have been implemented for the absolutely different > drivers. > dw_edma_pcie_probe() is called for an end-point PCIe-device found on a > PCIe-bus. > dw_pcie_host_init()/dw_pcie_ep_init() and dw_pcie_edma_detect() are > called for a platform-device representing a DW PCIe RP/EP controller. > In other words dw_pcie_edma_detect() and dw_edma_pcie_probe() are in > no means interchangeable. The question is what the user-visible difference between CONFIG_DW_EDMA_PCIE=y and CONFIG_DW_EDMA_PCIE=m is. If there were no difference, dw_pcie_host_init() would not need to call dw_pcie_edma_detect(). Can you give me a one- or two-sentence merge commit comment that explains why we want to merge this? "Relax driver config settings" doesn't tell us that. Bjorn
On Mon, Jan 23, 2023 at 10:43:39AM -0600, Bjorn Helgaas wrote: > On Sun, Jan 22, 2023 at 03:11:16AM +0300, Serge Semin wrote: > > On Fri, Jan 20, 2023 at 04:50:36PM -0600, Bjorn Helgaas wrote: > > > On Fri, Jan 13, 2023 at 08:14:06PM +0300, Serge Semin wrote: > > > > Since the DW PCIe RP/EP driver is about to be updated to register the DW > > > > eDMA-based DMA-engine the drivers build modes must be synchronized. > > > > Currently the DW PCIe RP/EP driver is always built as a builtin module. > > > > Meanwhile the DW eDMA driver can be built as a loadable module. Thus in > > > > the later case the kernel with DW PCIe controllers support will fail to be > > > > linked due to lacking the DW eDMA probe/remove symbols. At the same time > > > > forcibly selecting the DW eDMA driver from the DW PCIe RP/EP kconfig will > > > > effectively eliminate the tristate type of the former driver fixing it to > > > > just the builtin kernel module. > > > > > > > > Seeing the DW eDMA engine isn't that often met built into the DW PCIe > > > > Root-ports and End-points let's convert the DW eDMA driver config to being > > > > more flexible instead of just forcibly selecting the DW eDMA kconfig. In > > > > order to do that first the DW eDMA PCIe driver config should be converted > > > > to being depended from the DW eDMA core config instead of selecting the > > > > one. Second the DW eDMA probe and remove symbols should be referenced only > > > > if they are reachable by the caller. Thus the user will be able to build > > > > the DW eDMA core driver with any type, meanwhile the dependent code will > > > > be either restricted to the same build type (e.g. DW eDMA PCIe driver if > > > > DW eDMA driver is built as a loadable module) or just won't be able to use > > > > the eDMA engine registration/de-registration functionality (e.g. DW PCIe > > > > RP/EP driver if DW eDMA driver is built as a loadable module). > > > > > > I'm trying to write the merge commit log, and I understand the linking > > > issue, but I'm having a hard time figuring out what the user-visible > > > scenarios are here. > > > > > > I assume there's something that works when CONFIG_PCIE_DW=y and > > > CONFIG_DW_EDMA_PCIE=y but does *not* work when CONFIG_PCIE_DW=y and > > > CONFIG_DW_EDMA_PCIE=m? > > > > No. The DW eDMA code availability (in other words the CONFIG_DW_EDMA > > config value) determines whether the corresponding driver (DW PCIe > > RP/EP or DW eDMA PCI) is capable to perform the eDMA engine probe > > procedure. Additionally both drivers has the opposite dependency from > > the DW eDMA code. > > | | DW PCIe RP/EP | DW eDMA PCIe | > > | CONFIG_DW_EDMA +----------------------+----------------------+ > > | | Probe eDMA | KConfig | Probe eDMA | Kconfig | > > +----------------+------------+---------+------------+---------+ > > | y | YES | y,n | YES | y,m,n | > > | m | NO | y,n | YES | m,n | > > | n | NO | y,n | NO | n | > > +--------------------------------------------------------------+ > > > > Basically it means the DW PCIe RP/EP driver will be able to probe the > > DW eDMA engine only if the corresponding driver is built into the > > kernel. At the same time the DW PCIe RP/EP driver doesn't depend on > > the DW eDMA core module config state. The DW eDMA PCIe driver in > > opposite depends on the DW eDMA code config state, but will always be > > able to probe the DW eDMA engine as long as the corresponding code is > > loaded as either a part of the kernel or as a loadable module. > > > > > If both scenarios worked the same, I would think the existing > > > dw_edma_pcie_probe() would be enough, and you wouldn't need to call > > > dw_pcie_edma_detect() from dw_pcie_host_init() and dw_pcie_ep_init(). > > > > No. These methods have been implemented for the absolutely different > > drivers. > > dw_edma_pcie_probe() is called for an end-point PCIe-device found on a > > PCIe-bus. > > dw_pcie_host_init()/dw_pcie_ep_init() and dw_pcie_edma_detect() are > > called for a platform-device representing a DW PCIe RP/EP controller. > > In other words dw_pcie_edma_detect() and dw_edma_pcie_probe() are in > > no means interchangeable. > > The question is what the user-visible difference between > CONFIG_DW_EDMA_PCIE=y and CONFIG_DW_EDMA_PCIE=m is. There will be no difference between them after this commit is applied from the DW eDMA core driver point of view. CONFIG_DW_EDMA_PCIE now depends on the CONFIG_DW_EDMA config state (see it's surrounded by if/endif in the Kconfig file). Without this patch the CONFIG_DW_EDMA_PCIE config determines the CONFIG_DW_EDMA config state by forcibly selecting the one. Using the similar approach for the CONFIG_PCIE_DW driver I found less attractive because it would have effectively converted the CONFIG_DW_EDMA config tristate to boolean. That's why instead I decided to convert the CONFIG_DW_EDMA config to being independent from any other config value. (See the table in the my previous email message.) > If there were no > difference, dw_pcie_host_init() would not need to call > dw_pcie_edma_detect(). Even if CONFIG_DW_EDMA (not CONFIG_DW_EDMA_PCIE) is set to m or n I would have still recommended to call dw_pcie_edma_detect() because the method performs the DW eDMA engine auto-detection independently from the DW eDMA driver availability. As a result the system log will have a number of eDMA detected channels if the engine was really found. It's up to the system administrator to make sure that the eDMA driver is properly built/loaded then for the engine to be actually available in the kernel/system. > > Can you give me a one- or two-sentence merge commit comment that > explains why we want to merge this? "Relax driver config settings" > doesn't tell us that. "Convert the DW eDMA kconfig to being independently selected by the user in order to preserve the module build options flexibility and fix the "undefined reference to" error on DW PCIe driver build." -Serge(y) > > Bjorn
On Tue, Jan 24, 2023 at 05:49:41PM +0300, Serge Semin wrote: > On Mon, Jan 23, 2023 at 10:43:39AM -0600, Bjorn Helgaas wrote: > > On Sun, Jan 22, 2023 at 03:11:16AM +0300, Serge Semin wrote: > > > On Fri, Jan 20, 2023 at 04:50:36PM -0600, Bjorn Helgaas wrote: > > > > On Fri, Jan 13, 2023 at 08:14:06PM +0300, Serge Semin wrote: > > > > > Since the DW PCIe RP/EP driver is about to be updated to register the DW > > > > > eDMA-based DMA-engine the drivers build modes must be synchronized. > > > > > Currently the DW PCIe RP/EP driver is always built as a builtin module. > > > > > Meanwhile the DW eDMA driver can be built as a loadable module. Thus in > > > > > the later case the kernel with DW PCIe controllers support will fail to be > > > > > linked due to lacking the DW eDMA probe/remove symbols. At the same time > > > > > forcibly selecting the DW eDMA driver from the DW PCIe RP/EP kconfig will > > > > > effectively eliminate the tristate type of the former driver fixing it to > > > > > just the builtin kernel module. > > > > > > > > > > Seeing the DW eDMA engine isn't that often met built into the DW PCIe > > > > > Root-ports and End-points let's convert the DW eDMA driver config to being > > > > > more flexible instead of just forcibly selecting the DW eDMA kconfig. In > > > > > order to do that first the DW eDMA PCIe driver config should be converted > > > > > to being depended from the DW eDMA core config instead of selecting the > > > > > one. Second the DW eDMA probe and remove symbols should be referenced only > > > > > if they are reachable by the caller. Thus the user will be able to build > > > > > the DW eDMA core driver with any type, meanwhile the dependent code will > > > > > be either restricted to the same build type (e.g. DW eDMA PCIe driver if > > > > > DW eDMA driver is built as a loadable module) or just won't be able to use > > > > > the eDMA engine registration/de-registration functionality (e.g. DW PCIe > > > > > RP/EP driver if DW eDMA driver is built as a loadable module). > > > > > > > > I'm trying to write the merge commit log, and I understand the linking > > > > issue, but I'm having a hard time figuring out what the user-visible > > > > scenarios are here. > > > > > > > > I assume there's something that works when CONFIG_PCIE_DW=y and > > > > CONFIG_DW_EDMA_PCIE=y but does *not* work when CONFIG_PCIE_DW=y and > > > > CONFIG_DW_EDMA_PCIE=m? > > > > > > No. The DW eDMA code availability (in other words the CONFIG_DW_EDMA > > > config value) determines whether the corresponding driver (DW PCIe > > > RP/EP or DW eDMA PCI) is capable to perform the eDMA engine probe > > > procedure. Additionally both drivers has the opposite dependency from > > > the DW eDMA code. > > > | | DW PCIe RP/EP | DW eDMA PCIe | > > > | CONFIG_DW_EDMA +----------------------+----------------------+ > > > | | Probe eDMA | KConfig | Probe eDMA | Kconfig | > > > +----------------+------------+---------+------------+---------+ > > > | y | YES | y,n | YES | y,m,n | > > > | m | NO | y,n | YES | m,n | > > > | n | NO | y,n | NO | n | > > > +--------------------------------------------------------------+ > > > > > > Basically it means the DW PCIe RP/EP driver will be able to probe the > > > DW eDMA engine only if the corresponding driver is built into the > > > kernel. At the same time the DW PCIe RP/EP driver doesn't depend on > > > the DW eDMA core module config state. The DW eDMA PCIe driver in > > > opposite depends on the DW eDMA code config state, but will always be > > > able to probe the DW eDMA engine as long as the corresponding code is > > > loaded as either a part of the kernel or as a loadable module. > > > > > > > If both scenarios worked the same, I would think the existing > > > > dw_edma_pcie_probe() would be enough, and you wouldn't need to call > > > > dw_pcie_edma_detect() from dw_pcie_host_init() and dw_pcie_ep_init(). > > > > > > No. These methods have been implemented for the absolutely different > > > drivers. > > > dw_edma_pcie_probe() is called for an end-point PCIe-device found on a > > > PCIe-bus. > > > dw_pcie_host_init()/dw_pcie_ep_init() and dw_pcie_edma_detect() are > > > called for a platform-device representing a DW PCIe RP/EP controller. > > > In other words dw_pcie_edma_detect() and dw_edma_pcie_probe() are in > > > no means interchangeable. > > > > The question is what the user-visible difference between > > CONFIG_DW_EDMA_PCIE=y and CONFIG_DW_EDMA_PCIE=m is. > > There will be no difference between them after this commit is applied > from the DW eDMA core driver point of view. CONFIG_DW_EDMA_PCIE now > depends on the CONFIG_DW_EDMA config state (see it's surrounded by > if/endif in the Kconfig file). Without this patch the > CONFIG_DW_EDMA_PCIE config determines the CONFIG_DW_EDMA config state > by forcibly selecting the one. Using the similar approach for the > CONFIG_PCIE_DW driver I found less attractive because it would have > effectively converted the CONFIG_DW_EDMA config tristate to boolean. > > That's why instead I decided to convert the CONFIG_DW_EDMA config to > being independent from any other config value. (See the table in the > my previous email message.) > > > If there were no > > difference, dw_pcie_host_init() would not need to call > > dw_pcie_edma_detect(). > > Even if CONFIG_DW_EDMA (not CONFIG_DW_EDMA_PCIE) is set to m or n I > would have still recommended to call dw_pcie_edma_detect() because the > method performs the DW eDMA engine auto-detection independently from the DW > eDMA driver availability. As a result the system log will have a > number of eDMA detected channels if the engine was really found. It's > up to the system administrator to make sure that the eDMA driver is > properly built/loaded then for the engine to be actually available in > the kernel/system. > > > Can you give me a one- or two-sentence merge commit comment that > > explains why we want to merge this? "Relax driver config settings" > > doesn't tell us that. > > "Convert the DW eDMA kconfig to being independently selected by the > user in order to preserve the module build options flexibility and fix > the "undefined reference to" error on DW PCIe driver build." In the commit log, I think "forcibly selecting the DW eDMA driver from the DW PCIe RP/EP kconfig" actually refers to just the "DW eDMA PCIe" driver" not the "DW PCIe RP/EP driver," right? The undefined reference to dw_edma_probe() doesn't actually happen unless we merge 27/27 without *this* patch, right? If so, I wouldn't call this a "fix" because nobody has ever seen the link failure. OK. I think this would be much simpler if it were split into two patches: 1) Prepare dw_edma_probe() for builtin callers When CONFIG_DW_EDMA=m, dw_edma_probe() is built as a module. Previously edma.h declared it as extern, which meant that builtin callers like dw_pcie_host_init() and dw_pcie_ep_init() caused link errors. Make it safe for builtin callers to call dw_edma_probe() by using IS_REACHABLE() to define a stub when CONFIG_DW_EDMA=m. Builtin callers will fail to detect and register eDMA devices when CONFIG_DW_EDMA=m but will otherwise work as before. 2) Make DW_EDMA_PCIE depend on DW_EDMA This seems like a good idea and is much nicer than "select DW_EDMA", but I think it should be a separate patch since it really only relates to dw-edma-pcie.c. I would use "depends on DW_EDMA" instead of adding if/endif around DW_EDMA_PCIE. What do you think? Am I still missing something? Bjorn
On Tue, Jan 24, 2023 at 05:47:44PM -0600, Bjorn Helgaas wrote: > On Tue, Jan 24, 2023 at 05:49:41PM +0300, Serge Semin wrote: > > On Mon, Jan 23, 2023 at 10:43:39AM -0600, Bjorn Helgaas wrote: > > > On Sun, Jan 22, 2023 at 03:11:16AM +0300, Serge Semin wrote: > > > > On Fri, Jan 20, 2023 at 04:50:36PM -0600, Bjorn Helgaas wrote: > > > > > On Fri, Jan 13, 2023 at 08:14:06PM +0300, Serge Semin wrote: > > > > > > Since the DW PCIe RP/EP driver is about to be updated to register the DW > > > > > > eDMA-based DMA-engine the drivers build modes must be synchronized. > > > > > > Currently the DW PCIe RP/EP driver is always built as a builtin module. > > > > > > Meanwhile the DW eDMA driver can be built as a loadable module. Thus in > > > > > > the later case the kernel with DW PCIe controllers support will fail to be > > > > > > linked due to lacking the DW eDMA probe/remove symbols. At the same time > > > > > > forcibly selecting the DW eDMA driver from the DW PCIe RP/EP kconfig will > > > > > > effectively eliminate the tristate type of the former driver fixing it to > > > > > > just the builtin kernel module. > > > > > > > > > > > > Seeing the DW eDMA engine isn't that often met built into the DW PCIe > > > > > > Root-ports and End-points let's convert the DW eDMA driver config to being > > > > > > more flexible instead of just forcibly selecting the DW eDMA kconfig. In > > > > > > order to do that first the DW eDMA PCIe driver config should be converted > > > > > > to being depended from the DW eDMA core config instead of selecting the > > > > > > one. Second the DW eDMA probe and remove symbols should be referenced only > > > > > > if they are reachable by the caller. Thus the user will be able to build > > > > > > the DW eDMA core driver with any type, meanwhile the dependent code will > > > > > > be either restricted to the same build type (e.g. DW eDMA PCIe driver if > > > > > > DW eDMA driver is built as a loadable module) or just won't be able to use > > > > > > the eDMA engine registration/de-registration functionality (e.g. DW PCIe > > > > > > RP/EP driver if DW eDMA driver is built as a loadable module). > > > > > > > > > > I'm trying to write the merge commit log, and I understand the linking > > > > > issue, but I'm having a hard time figuring out what the user-visible > > > > > scenarios are here. > > > > > > > > > > I assume there's something that works when CONFIG_PCIE_DW=y and > > > > > CONFIG_DW_EDMA_PCIE=y but does *not* work when CONFIG_PCIE_DW=y and > > > > > CONFIG_DW_EDMA_PCIE=m? > > > > > > > > No. The DW eDMA code availability (in other words the CONFIG_DW_EDMA > > > > config value) determines whether the corresponding driver (DW PCIe > > > > RP/EP or DW eDMA PCI) is capable to perform the eDMA engine probe > > > > procedure. Additionally both drivers has the opposite dependency from > > > > the DW eDMA code. > > > > | | DW PCIe RP/EP | DW eDMA PCIe | > > > > | CONFIG_DW_EDMA +----------------------+----------------------+ > > > > | | Probe eDMA | KConfig | Probe eDMA | Kconfig | > > > > +----------------+------------+---------+------------+---------+ > > > > | y | YES | y,n | YES | y,m,n | > > > > | m | NO | y,n | YES | m,n | > > > > | n | NO | y,n | NO | n | > > > > +--------------------------------------------------------------+ > > > > > > > > Basically it means the DW PCIe RP/EP driver will be able to probe the > > > > DW eDMA engine only if the corresponding driver is built into the > > > > kernel. At the same time the DW PCIe RP/EP driver doesn't depend on > > > > the DW eDMA core module config state. The DW eDMA PCIe driver in > > > > opposite depends on the DW eDMA code config state, but will always be > > > > able to probe the DW eDMA engine as long as the corresponding code is > > > > loaded as either a part of the kernel or as a loadable module. > > > > > > > > > If both scenarios worked the same, I would think the existing > > > > > dw_edma_pcie_probe() would be enough, and you wouldn't need to call > > > > > dw_pcie_edma_detect() from dw_pcie_host_init() and dw_pcie_ep_init(). > > > > > > > > No. These methods have been implemented for the absolutely different > > > > drivers. > > > > dw_edma_pcie_probe() is called for an end-point PCIe-device found on a > > > > PCIe-bus. > > > > dw_pcie_host_init()/dw_pcie_ep_init() and dw_pcie_edma_detect() are > > > > called for a platform-device representing a DW PCIe RP/EP controller. > > > > In other words dw_pcie_edma_detect() and dw_edma_pcie_probe() are in > > > > no means interchangeable. > > > > > > The question is what the user-visible difference between > > > CONFIG_DW_EDMA_PCIE=y and CONFIG_DW_EDMA_PCIE=m is. > > > > There will be no difference between them after this commit is applied > > from the DW eDMA core driver point of view. CONFIG_DW_EDMA_PCIE now > > depends on the CONFIG_DW_EDMA config state (see it's surrounded by > > if/endif in the Kconfig file). Without this patch the > > CONFIG_DW_EDMA_PCIE config determines the CONFIG_DW_EDMA config state > > by forcibly selecting the one. Using the similar approach for the > > CONFIG_PCIE_DW driver I found less attractive because it would have > > effectively converted the CONFIG_DW_EDMA config tristate to boolean. > > > > That's why instead I decided to convert the CONFIG_DW_EDMA config to > > being independent from any other config value. (See the table in the > > my previous email message.) > > > > > If there were no > > > difference, dw_pcie_host_init() would not need to call > > > dw_pcie_edma_detect(). > > > > Even if CONFIG_DW_EDMA (not CONFIG_DW_EDMA_PCIE) is set to m or n I > > would have still recommended to call dw_pcie_edma_detect() because the > > method performs the DW eDMA engine auto-detection independently from the DW > > eDMA driver availability. As a result the system log will have a > > number of eDMA detected channels if the engine was really found. It's > > up to the system administrator to make sure that the eDMA driver is > > properly built/loaded then for the engine to be actually available in > > the kernel/system. > > > > > Can you give me a one- or two-sentence merge commit comment that > > > explains why we want to merge this? "Relax driver config settings" > > > doesn't tell us that. > > > > "Convert the DW eDMA kconfig to being independently selected by the > > user in order to preserve the module build options flexibility and fix > > the "undefined reference to" error on DW PCIe driver build." > > In the commit log, I think "forcibly selecting the DW eDMA driver from > the DW PCIe RP/EP kconfig" actually refers to just the "DW eDMA PCIe" > driver" not the "DW PCIe RP/EP driver," right? Right. > > The undefined reference to dw_edma_probe() doesn't actually happen > unless we merge 27/27 without *this* patch, right? Right. > If so, I wouldn't > call this a "fix" because nobody has ever seen the link failure. > > OK. I think this would be much simpler if it were split into two > patches: > > 1) Prepare dw_edma_probe() for builtin callers > > When CONFIG_DW_EDMA=m, dw_edma_probe() is built as a module. > Previously edma.h declared it as extern, which meant that > builtin callers like dw_pcie_host_init() and dw_pcie_ep_init() > caused link errors. > > Make it safe for builtin callers to call dw_edma_probe() by using > IS_REACHABLE() to define a stub when CONFIG_DW_EDMA=m. > > Builtin callers will fail to detect and register eDMA devices > when CONFIG_DW_EDMA=m but will otherwise work as before. > > 2) Make DW_EDMA_PCIE depend on DW_EDMA > > This seems like a good idea and is much nicer than "select > DW_EDMA", but I think it should be a separate patch since it > really only relates to dw-edma-pcie.c. > I would use "depends on > DW_EDMA" instead of adding if/endif around DW_EDMA_PCIE. Could you explain why is the "depends on" operator more preferable than if/endif? In this case since we have a single core kconfig from which all the eDMA LLDD config(s) (except PCIE_DW for the reason previously described) will surely depend on, using if/endif would cause the possible new eDMA-capable LLDD(s) adding their kconfig entries within the if-endif clause without need to copy the same "depends on DW_EDMA" pattern over and over. That seems to look a bit more maintainable than the alternative you suggest. Do you think otherwise? > > Am I still missing something? No, you aren't. > What do you think? What you described was the second option I had in mind for the update to look like, but after all I decided to take a shorter path and combine the modifications into a single patch. If you think that splitting it up would make the update looking simpler then I'll do as you suggest. But in that case Lorenzo will need to re-merge the updated patchset v10. -Serge(y) > > Bjorn
On Wed, Jan 25, 2023 at 05:40:19PM +0300, Serge Semin wrote: > On Tue, Jan 24, 2023 at 05:47:44PM -0600, Bjorn Helgaas wrote: > > In the commit log, I think "forcibly selecting the DW eDMA driver from > > the DW PCIe RP/EP kconfig" actually refers to just the "DW eDMA PCIe" > > driver" not the "DW PCIe RP/EP driver," right? > > Right. Good. I think it's worth updating the commit log to clear this up because there are several things with very similar names, so it's confusing enough already ;) > > The undefined reference to dw_edma_probe() doesn't actually happen > > unless we merge 27/27 without *this* patch, right? > > Right. Thanks, I got unreasonably focused on the "fix 'undefined reference' error" comment, wondering if we needed to identify a Fixes: commit, so this clears that up, too. > > I would use "depends on > > DW_EDMA" instead of adding if/endif around DW_EDMA_PCIE. > > Could you explain why is the "depends on" operator more preferable > than if/endif? In this case since we have a single core kconfig from > which all the eDMA LLDD config(s) (except PCIE_DW for the reason > previously described) will surely depend on, using if/endif would > cause the possible new eDMA-capable LLDD(s) adding their kconfig > entries within the if-endif clause without need to copy the same > "depends on DW_EDMA" pattern over and over. That seems to look a bit > more maintainable than the alternative you suggest. Do you think > otherwise? Only that "depends on" is much more common and I always try to avoid unusual constructs. But I wasn't looking into the future and imagining several LLDDs with similar uses of "depends on DW_EDMA". Thanks for that perspective; with it, I think it's OK either way. > > What do you think? > > What you described was the second option I had in mind for the update > to look like, but after all I decided to take a shorter path and > combine the modifications into a single patch. If you think that > splitting it up would make the update looking simpler then I'll do as > you suggest. But in that case Lorenzo will need to re-merge the > updated patchset v10. It's a pretty trivial update, so I just did it myself. The result is at https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/ctrl/dwc&id=ecadcaed4ef7 I split this patch and tweaked some commit messages for consistency (including the "DW eDMA PCIe driver" change above). "git diff -b" with Lorenzo's current branch (95624672bb3e ("PCI: dwc: Add DW eDMA engine support")) is empty except for a minor comment change. Bjorn
On Wed, Jan 25, 2023 at 05:23:57PM -0600, Bjorn Helgaas wrote: > On Wed, Jan 25, 2023 at 05:40:19PM +0300, Serge Semin wrote: > > On Tue, Jan 24, 2023 at 05:47:44PM -0600, Bjorn Helgaas wrote: > > > > In the commit log, I think "forcibly selecting the DW eDMA driver from > > > the DW PCIe RP/EP kconfig" actually refers to just the "DW eDMA PCIe" > > > driver" not the "DW PCIe RP/EP driver," right? > > > > Right. > > Good. I think it's worth updating the commit log to clear this up > because there are several things with very similar names, so it's > confusing enough already ;) > > > > The undefined reference to dw_edma_probe() doesn't actually happen > > > unless we merge 27/27 without *this* patch, right? > > > > Right. > > Thanks, I got unreasonably focused on the "fix 'undefined reference' > error" comment, wondering if we needed to identify a Fixes: commit, so > this clears that up, too. > > > > I would use "depends on > > > DW_EDMA" instead of adding if/endif around DW_EDMA_PCIE. > > > > Could you explain why is the "depends on" operator more preferable > > than if/endif? In this case since we have a single core kconfig from > > which all the eDMA LLDD config(s) (except PCIE_DW for the reason > > previously described) will surely depend on, using if/endif would > > cause the possible new eDMA-capable LLDD(s) adding their kconfig > > entries within the if-endif clause without need to copy the same > > "depends on DW_EDMA" pattern over and over. That seems to look a bit > > more maintainable than the alternative you suggest. Do you think > > otherwise? > > Only that "depends on" is much more common and I always try to avoid > unusual constructs. But I wasn't looking into the future and > imagining several LLDDs with similar uses of "depends on DW_EDMA". > Thanks for that perspective; with it, I think it's OK either way. > > > > What do you think? > > > > What you described was the second option I had in mind for the update > > to look like, but after all I decided to take a shorter path and > > combine the modifications into a single patch. If you think that > > splitting it up would make the update looking simpler then I'll do as > > you suggest. But in that case Lorenzo will need to re-merge the > > updated patchset v10. > > It's a pretty trivial update, so I just did it myself. The result is > at https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/ctrl/dwc&id=ecadcaed4ef7 > > I split this patch and tweaked some commit messages for consistency > (including the "DW eDMA PCIe driver" change above). "git diff -b" > with Lorenzo's current branch (95624672bb3e ("PCI: dwc: Add DW eDMA > engine support")) is empty except for a minor comment change. Great! Thanks. Although I've already created v10 beforehand but didn't submitted it yet waiting for your response. The split up patches look exactly like yours. In addition to that since I was going to re-send v10 I also took into account your comments regarding the patch: [PATCH v9 19/27] dmaengine: dw-edma: Use non-atomic io-64 methods Link: https://lore.kernel.org/linux-pci/20230113171409.30470-20-Sergey.Semin@baikalelectronics.ru/ I've dropped unneeded modification and unpinned another fixes patch which turned out to be a part of those modifications. So if you re-based your pci/ctrl/dwc branch with that patch replaced with the patches attached to this email it would have been great. Otherwise it's ok to merge the series as is. Note in the attached "non-atomic io-64" patch I've already replaced the commit log with the your short version. -Sergey > > Bjorn
On Thu, Jan 26, 2023 at 07:37:50PM +0300, Serge Semin wrote: > On Wed, Jan 25, 2023 at 05:23:57PM -0600, Bjorn Helgaas wrote: > > It's a pretty trivial update, so I just did it myself. The result is > > at https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/ctrl/dwc&id=ecadcaed4ef7 > > > > I split this patch and tweaked some commit messages for consistency > > (including the "DW eDMA PCIe driver" change above). "git diff -b" > > with Lorenzo's current branch (95624672bb3e ("PCI: dwc: Add DW eDMA > > engine support")) is empty except for a minor comment change. > > Great! Thanks. Although I've already created v10 beforehand but didn't > submitted it yet waiting for your response. The split up patches look > exactly like yours. > > In addition to that since I was going to re-send v10 I also took into > account your comments regarding the patch: > [PATCH v9 19/27] dmaengine: dw-edma: Use non-atomic io-64 methods > Link: https://lore.kernel.org/linux-pci/20230113171409.30470-20-Sergey.Semin@baikalelectronics.ru/ > I've dropped unneeded modification and unpinned another fixes patch > which turned out to be a part of those modifications. So if you > re-based your pci/ctrl/dwc branch with that patch replaced with the > patches attached to this email it would have been great. Otherwise > it's ok to merge the series as is. > > Note in the attached "non-atomic io-64" patch I've already replaced > the commit log with the your short version. Awesome, thanks! I folded those updates in and updated my branch. And merged the whole thing into the PCI "next" branch. Thanks for all your work! Bjorn
On Thu, Jan 26, 2023 at 12:19:54PM -0600, Bjorn Helgaas wrote: > On Thu, Jan 26, 2023 at 07:37:50PM +0300, Serge Semin wrote: > > On Wed, Jan 25, 2023 at 05:23:57PM -0600, Bjorn Helgaas wrote: > > > > It's a pretty trivial update, so I just did it myself. The result is > > > at https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/ctrl/dwc&id=ecadcaed4ef7 > > > > > > I split this patch and tweaked some commit messages for consistency > > > (including the "DW eDMA PCIe driver" change above). "git diff -b" > > > with Lorenzo's current branch (95624672bb3e ("PCI: dwc: Add DW eDMA > > > engine support")) is empty except for a minor comment change. > > > > Great! Thanks. Although I've already created v10 beforehand but didn't > > submitted it yet waiting for your response. The split up patches look > > exactly like yours. > > > > In addition to that since I was going to re-send v10 I also took into > > account your comments regarding the patch: > > [PATCH v9 19/27] dmaengine: dw-edma: Use non-atomic io-64 methods > > Link: https://lore.kernel.org/linux-pci/20230113171409.30470-20-Sergey.Semin@baikalelectronics.ru/ > > I've dropped unneeded modification and unpinned another fixes patch > > which turned out to be a part of those modifications. So if you > > re-based your pci/ctrl/dwc branch with that patch replaced with the > > patches attached to this email it would have been great. Otherwise > > it's ok to merge the series as is. > > > > Note in the attached "non-atomic io-64" patch I've already replaced > > the commit log with the your short version. > > Awesome, thanks! I folded those updates in and updated my branch. > > And merged the whole thing into the PCI "next" branch. > > Thanks for all your work! Great! Thanks to you too with reviewing and helping to merge the bits. I've almost completed another series with a few more updates based on the notes made by you and Mani during the review. I'll submit the patchset soon. -Serge(y) > > Bjorn
diff --git a/drivers/dma/dw-edma/Kconfig b/drivers/dma/dw-edma/Kconfig index 7ff17b2db6a1..2b6f2679508d 100644 --- a/drivers/dma/dw-edma/Kconfig +++ b/drivers/dma/dw-edma/Kconfig @@ -9,11 +9,14 @@ config DW_EDMA Support the Synopsys DesignWare eDMA controller, normally implemented on endpoints SoCs. +if DW_EDMA + config DW_EDMA_PCIE tristate "Synopsys DesignWare eDMA PCIe driver" depends on PCI && PCI_MSI - select DW_EDMA help Provides a glue-logic between the Synopsys DesignWare eDMA controller and an endpoint PCIe device. This also serves as a reference design to whom desires to use this IP. + +endif # DW_EDMA diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h index 08833f12b386..c062c8db472c 100644 --- a/include/linux/dma/edma.h +++ b/include/linux/dma/edma.h @@ -101,7 +101,7 @@ struct dw_edma_chip { }; /* Export to the platform drivers */ -#if IS_ENABLED(CONFIG_DW_EDMA) +#if IS_REACHABLE(CONFIG_DW_EDMA) int dw_edma_probe(struct dw_edma_chip *chip); int dw_edma_remove(struct dw_edma_chip *chip); #else
Since the DW PCIe RP/EP driver is about to be updated to register the DW eDMA-based DMA-engine the drivers build modes must be synchronized. Currently the DW PCIe RP/EP driver is always built as a builtin module. Meanwhile the DW eDMA driver can be built as a loadable module. Thus in the later case the kernel with DW PCIe controllers support will fail to be linked due to lacking the DW eDMA probe/remove symbols. At the same time forcibly selecting the DW eDMA driver from the DW PCIe RP/EP kconfig will effectively eliminate the tristate type of the former driver fixing it to just the builtin kernel module. Seeing the DW eDMA engine isn't that often met built into the DW PCIe Root-ports and End-points let's convert the DW eDMA driver config to being more flexible instead of just forcibly selecting the DW eDMA kconfig. In order to do that first the DW eDMA PCIe driver config should be converted to being depended from the DW eDMA core config instead of selecting the one. Second the DW eDMA probe and remove symbols should be referenced only if they are reachable by the caller. Thus the user will be able to build the DW eDMA core driver with any type, meanwhile the dependent code will be either restricted to the same build type (e.g. DW eDMA PCIe driver if DW eDMA driver is built as a loadable module) or just won't be able to use the eDMA engine registration/de-registration functionality (e.g. DW PCIe RP/EP driver if DW eDMA driver is built as a loadable module). Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru> --- Changelog v8: - This is a new patch added on v8 stage of the series in order to fix the tbot-reported build issues. (@tbot) --- drivers/dma/dw-edma/Kconfig | 5 ++++- include/linux/dma/edma.h | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-)