Re: [PATCH v1 3/6] PCI: dwc: Add HDMA support

[Serge Semin]

 On Fri, Jan 19, 2024 at 06:30:19PM +0530, Mrinmay Sarkar wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> 
> Hyper DMA (HDMA) is already supported by the dw-edma dmaengine driver.
> Unlike it's predecessor Embedded DMA (eDMA), HDMA supports only the
> unrolled mapping format. So the platform drivers need to provide a valid
> base address of the CSRs. Also, there is no standard way to auto detect
> the number of available read/write channels in a platform. So the platform
> drivers has to provide that information as well.
> 
> For adding HDMA support, the mapping format set by the platform drivers is
> used to detect whether eDMA or HDMA is being used, since we cannot auto
> detect it in a sane way.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Signed-off-by: Mrinmay Sarkar <quic_msarkar@xxxxxxxxxxx>
> ---
>  drivers/pci/controller/dwc/pcie-designware.c | 55 ++++++++++++++++++++++++----
>  1 file changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 96575b8..07a1f2d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -880,7 +880,29 @@ static struct dw_edma_plat_ops dw_pcie_edma_ops = {
>  	.irq_vector = dw_pcie_edma_irq_vector,
>  };
>  
> -static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> +static int dw_pcie_find_hdma(struct dw_pcie *pci)
> +{
> +	/*
> +	 * Since HDMA supports only unrolled mapping, platform drivers need to
> +	 * provide a valid base address.
> +	 */
> +	if (!pci->edma.reg_base)
> +		return -ENODEV;
> +
> +	/*
> +	 * Since there is no standard way to detect the number of read/write
> +	 * HDMA channels, platform drivers are expected to provide the channel
> +	 * count. Let's also do a sanity check of them to make sure that the
> +	 * counts are within the limit specified by the spec.
> +	 */
> +	if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > dw_edma_get_max_wr_ch(pci->edma.mf) ||
> +	    !pci->edma.ll_rd_cnt || pci->edma.ll_rd_cnt > dw_edma_get_max_rd_ch(pci->edma.mf))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int dw_pcie_find_edma(struct dw_pcie *pci)
>  {
>  	u32 val;
>  
> @@ -912,13 +934,6 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
>  		return -ENODEV;
>  	}
>  
> -	pci->edma.dev = pci->dev;
> -
> -	if (!pci->edma.ops)
> -		pci->edma.ops = &dw_pcie_edma_ops;
> -
> -	pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> -
>  	pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
>  	pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
>  
> @@ -930,6 +945,30 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
>  	return 0;
>  }
>  
> +static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> +{
> +	int ret;
> +

> +	if (pci->edma.mf == EDMA_MF_HDMA_NATIVE) {
> +		ret = dw_pcie_find_hdma(pci);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = dw_pcie_find_edma(pci);
> +		if (ret)
> +			return ret;
> +	}

Basically this change defines two versions of the eDMA info
initialization procedure:
1. use pre-defined CSRs mapping format and amount of channels,
2. auto-detect CSRs mapping and the amount of channels.
The second version also supports the optional CSRs mapping format
detection procedure by means of the DW_PCIE_CAP_EDMA_UNROLL flag
semantics. Thus should this patch is accepted there will be the
functionality duplication. I suggest to make things a bit more
flexible than that. Instead of creating the two types of the
init-methods selectable based on the mapping format, let's split up
the already available DW eDMA engine detection procedure into the next
three stages:
1. initialize DW eDMA data,
2. auto-detect the CSRs mapping format,
3. auto-detect the amount of channels.
and convert the later two to being optional. They will be skipped in case
if the mapping format or the amount of channels have been pre-defined
by the platform drivers. Thus we can keep the eDMA data init procedure
more linear thus easier to read, drop redundant DW_PCIE_CAP_EDMA_UNROLL flag
and use the new functionality for the Renesas R-Car S4-8's PCIe
controller (for which the auto-detection didn't work), for HDMA with compat
and _native_ CSRs mapping. See the attached patches for details:

0001-PCI-dwc-Fix-eDMA-mapping-info-string.patch
+- Just the eDMA log-message fix which will be useful in any case.

0002-PCI-dwc-Split-up-eDMA-parameters-auto-detection-proc.patch
+-> Split up the dw_pcie_edma_find_chip() method into three ones
described above.

0003-PCI-dwc-Convert-eDMA-mapping-detection-to-being-full.patch
+-> Skip the second stage the mapping format has been specified.

0004-PCI-dwc-Convert-eDMA-channels-detection-to-being-opt.patch
+-> Skip the amount of channels auto-detection if the amount has
already been specified.

0005-PCI-dwc-Drop-DW_PCIE_CAP_EDMA_UNROLL-flag.patch
+-> Drop the no longer needed DW_PCIE_CAP_EDMA_UNROLL flag.

After these patches are applied AFAICS the patches 5/6 and 6/6 of this
series shall work with no additional modification.

* Note I only build-tested the attached patches. So even though they
* aren't that much invasive please be read for a bit debugging.

-Serge(y)

> +
> +	pci->edma.dev = pci->dev;
> +
> +	if (!pci->edma.ops)
> +		pci->edma.ops = &dw_pcie_edma_ops;
> +
> +	pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> +
> +	return 0;
> +}
> +
>  static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
>  {
>  	struct platform_device *pdev = to_platform_device(pci->dev);
> -- 
> 2.7.4
>