linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Robin Murphy <robin.murphy@arm.com>
To: Serge Semin <fancer.lancer@gmail.com>,
	Vinod Koul <vkoul@kernel.org>,
	Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: "Serge Semin" <Sergey.Semin@baikalelectronics.ru>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Cai Huoqing" <cai.huoqing@linux.dev>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Frank Li" <Frank.Li@nxp.com>,
	"Alexey Malahov" <Alexey.Malahov@baikalelectronics.ru>,
	"Pavel Parkhomenko" <Pavel.Parkhomenko@baikalelectronics.ru>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	caihuoqing <caihuoqing@baidu.com>,
	linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 22/24] dmaengine: dw-edma: Bypass dma-ranges mapping for the local setup
Date: Fri, 25 Nov 2022 15:32:23 +0000	[thread overview]
Message-ID: <8b7ce195-27b7-a27f-bf4e-fd5f20f2a83b@arm.com> (raw)
In-Reply-To: <20221107211134.wxaqi2sew6aejxne@mobilestation>

On 2022-11-07 21:11, Serge Semin wrote:
> On Tue, Nov 08, 2022 at 12:04:36AM +0300, Serge Semin wrote:
>> DW eDMA doesn't perform any translation of the traffic generated on the
>> CPU/Application side. It just generates read/write AXI-bus requests with
>> the specified addresses. But in case if the dma-ranges DT-property is
>> specified for a platform device node, Linux will use it to create a
>> mapping the PCIe-bus regions into the CPU memory ranges. This isn't what
>> we want for the eDMA embedded into the locally accessed DW PCIe Root Port
>> and End-point. In order to work that around let's set the chan_dma_dev
>> flag for each DW eDMA channel thus forcing the client drivers to getting a
>> custom dma-ranges-less parental device for the mappings.
>>
>> Note it will only work for the client drivers using the
>> dmaengine_get_dma_device() method to get the parental DMA device.
> 
> @Robin, we particularly need you opinion on this patch. I did as you
> said: call *_dma_configure() method to initialize the child device and
> set the DMA-mask here instead of the platform driver.

Apologies, I've been busy and this series got buried in my inbox before 
I'd clocked it as something I was supposed to be looking at.

> @Vinoud, @Manivannan I had to drop your tags from this patch since its
> content had been significantly changed.
> 
> -Sergey
> 
>>
>> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
>>
>> ---
>>
>> Changelog v2:
>> - Fix the comment a bit to being clearer. (@Manivannan)
>>
>> Changelog v3:
>> - Conditionally set dchan->dev->device.dma_coherent field since it can
>>    be missing on some platforms. (@Manivannan)
>> - Remove Manivannan' rb and tb tags since the patch content has been
>>    changed.
>>
>> Changelog v6:
>> - Directly call *_dma_configure() method on the child device used for
>>    the DMA buffers mapping. (@Robin)
>> - Explicitly set the DMA-mask of the child device in the channel
>>    allocation proecedure. (@Robin)
>> - Drop @Manivannan and @Vinod rb- and ab-tags due to significant patch
>>    content change.
>> ---
>>   drivers/dma/dw-edma/dw-edma-core.c | 44 ++++++++++++++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
>> index e3671bfbe186..846518509753 100644
>> --- a/drivers/dma/dw-edma/dw-edma-core.c
>> +++ b/drivers/dma/dw-edma/dw-edma-core.c
>> @@ -6,9 +6,11 @@
>>    * Author: Gustavo Pimentel <gustavo.pimentel@synopsys.com>
>>    */
>>   
>> +#include <linux/acpi.h>
>>   #include <linux/module.h>
>>   #include <linux/device.h>
>>   #include <linux/kernel.h>
>> +#include <linux/of_device.h>
>>   #include <linux/dmaengine.h>
>>   #include <linux/err.h>
>>   #include <linux/interrupt.h>
>> @@ -711,10 +713,52 @@ static irqreturn_t dw_edma_interrupt_common(int irq, void *data)
>>   static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
>>   {
>>   	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>> +	struct device *dev = chan->dw->chip->dev;
>> +	int ret;
>>   
>>   	if (chan->status != EDMA_ST_IDLE)
>>   		return -EBUSY;
>>   
>> +	/* Bypass the dma-ranges based memory regions mapping for the eDMA
>> +	 * controlled from the CPU/Application side since in that case
>> +	 * the local memory address is left untranslated.
>> +	 */
>> +	if (chan->dw->chip->flags & DW_EDMA_CHIP_LOCAL) {
>> +		ret = dma_coerce_mask_and_coherent(&dchan->dev->device,
>> +						   DMA_BIT_MASK(64));
>> +		if (ret) {

Setting a 64-bit mask should never fail, especially on any platform that 
will actually run this code.

>> +			ret = dma_coerce_mask_and_coherent(&dchan->dev->device,
>> +							   DMA_BIT_MASK(32));
>> +			if (ret)
>> +				return ret;
>> +		}
>> +
>> +		if (dev_of_node(dev)) {
>> +			struct device_node *node = dev_of_node(dev);
>> +
>> +			ret = of_dma_configure(&dchan->dev->device, node, true);
>> +		} else if (has_acpi_companion(dev)) {

Can this can ever happen? AFAICS there's no ACPI binding to match and 
probe the DWC driver, at best it could only probe as a standard PNP0A08 
host bridge which wouldn't know anything about eDMA anyway.

>> +			struct acpi_device *adev = to_acpi_device_node(dev->fwnode);
>> +
>> +			ret = acpi_dma_configure(&dchan->dev->device,
>> +						 acpi_get_dma_attr(adev));
>> +		} else {
>> +			ret = -EINVAL;
>> +		}
>> +
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (dchan->dev->device.dma_range_map) {
>> +			kfree(dchan->dev->device.dma_range_map);
>> +			dchan->dev->device.dma_range_map = NULL;
>> +		}

Ugh, I guess this is still here because now you're passing the channel 
device to of_dma_configure() such that it looks like a PCI child :(

Can we just set "chan->dev->device.of_node = dev->of_node;" beforehand 
so it works as expected (with f1ad5338a4d5 in place) and we don't need 
to be messing with the dma_range_map details at all? Note that that 
isn't as hacky as it might sound - it's a relatively well-established 
practice in places like I2C and SPI, and in this case it seems perfectly 
appropriate semantically as well.

(And there should be no need to bother with of_node refcounting, since 
the lifetime of the eDMA driver is bounded by the lifetime of the PCIe 
driver, thus the lifetime of the DMA channel devices is bounded by the 
lifetime of the PCIe platform device, which already holds a reference 
from of_device_alloc().)

Thanks,
Robin.

>> +
>> +		dchan->dev->chan_dma_dev = true;
>> +	} else {
>> +		dchan->dev->chan_dma_dev = false;
>> +	}
>> +
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.38.0
>>
>>

  reply	other threads:[~2022-11-25 15:32 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-07 21:04 [PATCH v6 00/24] dmaengine: dw-edma: Add RP/EP local DMA controllers support Serge Semin
2022-11-07 21:04 ` [PATCH v6 01/24] dmaengine: Fix dma_slave_config.dst_addr description Serge Semin
2022-11-07 21:04 ` [PATCH v6 02/24] dmaengine: dw-edma: Release requested IRQs on failure Serge Semin
2022-11-07 21:04 ` [PATCH v6 03/24] dmaengine: dw-edma: Convert ll/dt phys-address to PCIe bus/DMA address Serge Semin
2022-11-07 21:04 ` [PATCH v6 04/24] dmaengine: dw-edma: Fix missing src/dst address of the interleaved xfers Serge Semin
2022-11-07 21:04 ` [PATCH v6 05/24] dmaengine: dw-edma: Don't permit non-inc " Serge Semin
2022-11-07 21:04 ` [PATCH v6 06/24] dmaengine: dw-edma: Fix invalid interleaved xfers semantics Serge Semin
2022-11-07 21:04 ` [PATCH v6 07/24] dmaengine: dw-edma: Add CPU to PCIe bus address translation Serge Semin
2022-11-07 21:04 ` [PATCH v6 08/24] dmaengine: dw-edma: Add PCIe bus address getter to the remote EP glue-driver Serge Semin
2022-11-07 21:04 ` [PATCH v6 09/24] dmaengine: dw-edma: Drop chancnt initialization Serge Semin
2022-11-07 21:04 ` [PATCH v6 10/24] dmaengine: dw-edma: Fix DebugFS reg entry type Serge Semin
2022-11-07 21:04 ` [PATCH v6 11/24] dmaengine: dw-edma: Stop checking debugfs_create_*() return value Serge Semin
2022-11-07 21:04 ` [PATCH v6 12/24] dmaengine: dw-edma: Add dw_edma prefix to the DebugFS nodes descriptor Serge Semin
2022-11-07 21:04 ` [PATCH v6 13/24] dmaengine: dw-edma: Convert DebugFS descs to being kz-allocated Serge Semin
2022-11-07 21:04 ` [PATCH v6 14/24] dmaengine: dw-edma: Rename DebugFS dentry variables to 'dent' Serge Semin
2022-11-07 21:04 ` [PATCH v6 15/24] dmaengine: dw-edma: Simplify the DebugFS context CSRs init procedure Serge Semin
2022-11-07 21:04 ` [PATCH v6 16/24] dmaengine: dw-edma: Move eDMA data pointer to DebugFS node descriptor Serge Semin
2022-11-07 21:04 ` [PATCH v6 17/24] dmaengine: dw-edma: Join Write/Read channels into a single device Serge Semin
2022-11-07 21:04 ` [PATCH v6 18/24] dmaengine: dw-edma: Use DMA-engine device DebugFS subdirectory Serge Semin
2022-11-07 21:04 ` [PATCH v6 19/24] dmaengine: dw-edma: Use non-atomic io-64 methods Serge Semin
2022-11-07 21:04 ` [PATCH v6 20/24] dmaengine: dw-edma: Drop DT-region allocation Serge Semin
2022-11-07 21:04 ` [PATCH v6 21/24] dmaengine: dw-edma: Replace chip ID number with device name Serge Semin
2022-11-07 21:04 ` [PATCH v6 22/24] dmaengine: dw-edma: Bypass dma-ranges mapping for the local setup Serge Semin
2022-11-07 21:11   ` Serge Semin
2022-11-25 15:32     ` Robin Murphy [this message]
2022-11-26 23:45       ` Serge Semin
2022-11-30  0:15         ` Serge Semin
2022-12-01 11:52         ` Robin Murphy
2022-12-11 17:33           ` Serge Semin
2022-11-07 21:04 ` [PATCH v6 23/24] dmaengine: dw-edma: Skip cleanup procedure if no private data found Serge Semin
2022-11-07 21:04 ` [PATCH v6 24/24] PCI: dwc: Add DW eDMA engine support Serge Semin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=8b7ce195-27b7-a27f-bf4e-fd5f20f2a83b@arm.com \
    --to=robin.murphy@arm.com \
    --cc=Alexey.Malahov@baikalelectronics.ru \
    --cc=Frank.Li@nxp.com \
    --cc=Pavel.Parkhomenko@baikalelectronics.ru \
    --cc=Sergey.Semin@baikalelectronics.ru \
    --cc=bhelgaas@google.com \
    --cc=cai.huoqing@linux.dev \
    --cc=caihuoqing@baidu.com \
    --cc=dmaengine@vger.kernel.org \
    --cc=fancer.lancer@gmail.com \
    --cc=gustavo.pimentel@synopsys.com \
    --cc=jingoohan1@gmail.com \
    --cc=kw@linux.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=manivannan.sadhasivam@linaro.org \
    --cc=robh@kernel.org \
    --cc=vkoul@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).