Re: [PATCH v9 24/27] dmaengine: dw-edma: Relax driver config settings

From: Serge Semin <fancer.lancer@gmail.com>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Serge Semin" <Sergey.Semin@baikalelectronics.ru>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Vinod Koul" <vkoul@kernel.org>, "Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Cai Huoqing" <cai.huoqing@linux.dev>,
	"Robin Murphy" <robin.murphy@arm.com>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Frank Li" <Frank.Li@nxp.com>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
	"Alexey Malahov" <Alexey.Malahov@baikalelectronics.ru>,
	"Pavel Parkhomenko" <Pavel.Parkhomenko@baikalelectronics.ru>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	caihuoqing <caihuoqing@baidu.com>,
	"Yoshihiro Shimoda" <yoshihiro.shimoda.uh@renesas.com>,
	linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v9 24/27] dmaengine: dw-edma: Relax driver config settings
Date: Sun, 22 Jan 2023 03:11:16 +0300	[thread overview]
Message-ID: <20230122001116.jbhttuaed7zuls26@mobilestation> (raw)
In-Reply-To: <20230120225036.GA675763@bhelgaas>

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
> > 
> > 