Since the DW eDMA driver now supports eDMA controllers embedded into the
locally accessible DW PCIe Root Ports and Endpoints, we can use the
updated interface to register DW eDMA as DMA engine device if it's
available. In order to successfully do that the DW PCIe core driver need
to perform some preparations first. First of all it needs to find out the
eDMA controller CSRs base address, whether they are accessible over the
Port Logic or iATU unrolled space. Afterwards it can try to auto-detect
the eDMA controller availability and number of it's read/write channels.
If none was found the procedure will just silently halt with no error
returned. Secondly the platform is supposed to provide either combined or
per-channel IRQ signals. If no valid IRQs set is found the procedure will
also halt with no error returned so to be backward compatible with the
platforms where DW PCIe controllers have eDMA embedded but lack of the
IRQs defined for them. Finally before actually probing the eDMA device we
need to allocate LLP items buffers. After that the DW eDMA can be
registered. If registration is successful the info-message regarding the
number of detected Read/Write eDMA channels will be printed to the system
log in the similar way as it's done for the iATU settings.
Signed-off-by: Serge Semin <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
Acked-By: Vinod Koul <[email protected]>
---
Changelog v2:
- Don't fail eDMA detection procedure if the DW eDMA driver couldn't probe
device. That happens if the driver is disabled. (@Manivannan)
- Add "dma" registers resource mapping procedure. (@Manivannan)
- Move the eDMA CSRs space detection into the dw_pcie_map_detect() method.
- Remove eDMA on the dw_pcie_ep_init() internal errors. (@Manivannan)
- Remove eDMA in the dw_pcie_ep_exit() method.
- Move the dw_pcie_edma_detect() method execution to the tail of the
dw_pcie_ep_init() function.
Changelog v3:
- Add more comprehensive and less regression prune eDMA block detection
procedure.
- Remove Manivannan tb tag since the patch content has been changed.
---
.../pci/controller/dwc/pcie-designware-ep.c | 12 +-
.../pci/controller/dwc/pcie-designware-host.c | 13 +-
drivers/pci/controller/dwc/pcie-designware.c | 186 ++++++++++++++++++
drivers/pci/controller/dwc/pcie-designware.h | 20 ++
4 files changed, 228 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 80a64b63c055..0fe83f08e0d6 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -612,8 +612,11 @@ int dw_pcie_ep_raise_msix_irq(struct dw_pcie_ep *ep, u8 func_no,
void dw_pcie_ep_exit(struct dw_pcie_ep *ep)
{
+ struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
struct pci_epc *epc = ep->epc;
+ dw_pcie_edma_remove(pci);
+
pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
epc->mem->window.page_size);
@@ -767,6 +770,10 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
goto err_exit_epc_mem;
}
+ ret = dw_pcie_edma_detect(pci);
+ if (ret)
+ goto err_free_epc_mem;
+
if (ep->ops->get_features) {
epc_features = ep->ops->get_features(ep);
if (epc_features->core_init_notifier)
@@ -775,10 +782,13 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep)
ret = dw_pcie_ep_init_complete(ep);
if (ret)
- goto err_free_epc_mem;
+ goto err_remove_edma;
return 0;
+err_remove_edma:
+ dw_pcie_edma_remove(pci);
+
err_free_epc_mem:
pci_epc_mem_free_addr(epc, ep->msi_mem_phys, ep->msi_mem,
epc->mem->window.page_size);
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 35da6ec41405..70dab96b1f66 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -481,14 +481,18 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
dw_pcie_iatu_detect(pci);
- ret = dw_pcie_setup_rc(pp);
+ ret = dw_pcie_edma_detect(pci);
if (ret)
goto err_free_msi;
+ ret = dw_pcie_setup_rc(pp);
+ if (ret)
+ goto err_remove_edma;
+
if (!dw_pcie_link_up(pci)) {
ret = dw_pcie_start_link(pci);
if (ret)
- goto err_free_msi;
+ goto err_remove_edma;
}
/* Ignore errors, the link may come up later */
@@ -505,6 +509,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
err_stop_link:
dw_pcie_stop_link(pci);
+err_remove_edma:
+ dw_pcie_edma_remove(pci);
+
err_free_msi:
if (pp->has_msi_ctrl)
dw_pcie_free_msi(pp);
@@ -526,6 +533,8 @@ void dw_pcie_host_deinit(struct dw_pcie_rp *pp)
dw_pcie_stop_link(pci);
+ dw_pcie_edma_remove(pci);
+
if (pp->has_msi_ctrl)
dw_pcie_free_msi(pp);
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 1e06ccf2dc9e..3dcbdaf980e4 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -12,6 +12,7 @@
#include <linux/bitops.h>
#include <linux/clk.h>
#include <linux/delay.h>
+#include <linux/dma/edma.h>
#include <linux/gpio/consumer.h>
#include <linux/ioport.h>
#include <linux/of.h>
@@ -142,6 +143,18 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
if (!pci->atu_size)
pci->atu_size = SZ_4K;
+ /* eDMA region can be mapped to a custom base address */
+ if (!pci->edma.reg_base) {
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma");
+ if (res) {
+ pci->edma.reg_base = devm_ioremap_resource(pci->dev, res);
+ if (IS_ERR(pci->edma.reg_base))
+ return PTR_ERR(pci->edma.reg_base);
+ } else if (pci->atu_size >= 2 * DEFAULT_DBI_DMA_OFFSET) {
+ pci->edma.reg_base = pci->atu_base + DEFAULT_DBI_DMA_OFFSET;
+ }
+ }
+
/* LLDD is supposed to manually switch the clocks and resets state */
if (dw_pcie_cap_is(pci, REQ_RES)) {
ret = dw_pcie_get_clocks(pci);
@@ -782,6 +795,179 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
pci->region_align / SZ_1K, (pci->region_limit + 1) / SZ_1G);
}
+static u32 dw_pcie_readl_dma(struct dw_pcie *pci, u32 reg)
+{
+ u32 val = 0;
+ int ret;
+
+ if (pci->ops && pci->ops->read_dbi)
+ return pci->ops->read_dbi(pci, pci->edma.reg_base, reg, 4);
+
+ ret = dw_pcie_read(pci->edma.reg_base + reg, 4, &val);
+ if (ret)
+ dev_err(pci->dev, "Read DMA address failed\n");
+
+ return val;
+}
+
+static int dw_pcie_edma_irq_vector(struct device *dev, unsigned int nr)
+{
+ struct platform_device *pdev = to_platform_device(dev);
+ char name[6];
+ int ret;
+
+ if (nr >= EDMA_MAX_WR_CH + EDMA_MAX_RD_CH)
+ return -EINVAL;
+
+ ret = platform_get_irq_byname_optional(pdev, "dma");
+ if (ret > 0)
+ return ret;
+
+ snprintf(name, sizeof(name), "dma%u", nr);
+
+ return platform_get_irq_byname_optional(pdev, name);
+}
+
+static struct dw_edma_core_ops dw_pcie_edma_ops = {
+ .irq_vector = dw_pcie_edma_irq_vector,
+};
+
+static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
+{
+ u32 val;
+
+ val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
+ if (val == 0xFFFFFFFF && pci->edma.reg_base) {
+ pci->edma.mf = EDMA_MF_EDMA_UNROLL;
+
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
+ } else if (val != 0xFFFFFFFF) {
+ pci->edma.mf = EDMA_MF_EDMA_LEGACY;
+
+ pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
+ } else {
+ 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);
+
+ /* Sanity check the channels count if the mapping was incorrect */
+ if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
+ !pci->edma.ll_rd_cnt || pci->edma.ll_rd_cnt > EDMA_MAX_RD_CH)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
+{
+ struct platform_device *pdev = to_platform_device(pci->dev);
+ u16 ch_cnt = pci->edma.ll_wr_cnt + pci->edma.ll_rd_cnt;
+ char name[6];
+ int ret;
+
+ if (pci->edma.nr_irqs == 1)
+ return 0;
+ else if (pci->edma.nr_irqs > 1)
+ return pci->edma.nr_irqs != ch_cnt ? -EINVAL : 0;
+
+ ret = platform_get_irq_byname_optional(pdev, "dma");
+ if (ret > 0) {
+ pci->edma.nr_irqs = 1;
+ return 0;
+ }
+
+ for (; pci->edma.nr_irqs < ch_cnt; pci->edma.nr_irqs++) {
+ snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs);
+
+ ret = platform_get_irq_byname_optional(pdev, name);
+ if (ret <= 0)
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int dw_pcie_edma_ll_alloc(struct dw_pcie *pci)
+{
+ struct dw_edma_region *ll;
+ dma_addr_t paddr;
+ int i;
+
+ for (i = 0; i < pci->edma.ll_wr_cnt; i++) {
+ ll = &pci->edma.ll_region_wr[i];
+ ll->sz = DMA_LLP_MEM_SIZE;
+ ll->vaddr = dmam_alloc_coherent(pci->dev, ll->sz,
+ &paddr, GFP_KERNEL);
+ if (!ll->vaddr)
+ return -ENOMEM;
+
+ ll->paddr = paddr;
+ }
+
+ for (i = 0; i < pci->edma.ll_rd_cnt; i++) {
+ ll = &pci->edma.ll_region_rd[i];
+ ll->sz = DMA_LLP_MEM_SIZE;
+ ll->vaddr = dmam_alloc_coherent(pci->dev, ll->sz,
+ &paddr, GFP_KERNEL);
+ if (!ll->vaddr)
+ return -ENOMEM;
+
+ ll->paddr = paddr;
+ }
+
+ return 0;
+}
+
+int dw_pcie_edma_detect(struct dw_pcie *pci)
+{
+ int ret;
+
+ /* Don't fail if no eDMA was found (for the backward compatibility) */
+ ret = dw_pcie_edma_find_chip(pci);
+ if (ret)
+ return 0;
+
+ /* Don't fail on the IRQs verification (for the backward compatibility) */
+ ret = dw_pcie_edma_irq_verify(pci);
+ if (ret) {
+ dev_err(pci->dev, "Invalid eDMA IRQs found\n");
+ return 0;
+ }
+
+ ret = dw_pcie_edma_ll_alloc(pci);
+ if (ret) {
+ dev_err(pci->dev, "Couldn't allocate LLP memory\n");
+ return ret;
+ }
+
+ /* Don't fail if the DW eDMA driver can't find the device */
+ ret = dw_edma_probe(&pci->edma);
+ if (ret && ret != -ENODEV) {
+ dev_err(pci->dev, "Couldn't register eDMA device\n");
+ return ret;
+ }
+
+ dev_info(pci->dev, "eDMA: unroll %s, %hu wr, %hu rd\n",
+ pci->edma.mf == EDMA_MF_EDMA_UNROLL ? "T" : "F",
+ pci->edma.ll_wr_cnt, pci->edma.ll_rd_cnt);
+
+ return 0;
+}
+
+void dw_pcie_edma_remove(struct dw_pcie *pci)
+{
+ dw_edma_remove(&pci->edma);
+}
+
void dw_pcie_setup(struct dw_pcie *pci)
{
u32 val;
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index f5530069a204..d11c246b9bc1 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -15,6 +15,7 @@
#include <linux/bitops.h>
#include <linux/clk.h>
#include <linux/dma-mapping.h>
+#include <linux/dma/edma.h>
#include <linux/gpio/consumer.h>
#include <linux/irq.h>
#include <linux/msi.h>
@@ -167,6 +168,18 @@
#define PCIE_MSIX_DOORBELL 0x948
#define PCIE_MSIX_DOORBELL_PF_SHIFT 24
+/*
+ * eDMA CSRs. DW PCIe IP-core v4.70a and older had the eDMA registers accessible
+ * over the Port Logic registers space. Afterwords the unrolled mapping was
+ * introduced so eDMA and iATU could be accessed via a dedicated registers
+ * space.
+ */
+#define PCIE_DMA_VIEWPORT_BASE 0x970
+#define PCIE_DMA_UNROLL_BASE 0x80000
+#define PCIE_DMA_CTRL 0x008
+#define PCIE_DMA_NUM_WR_CHAN GENMASK(3, 0)
+#define PCIE_DMA_NUM_RD_CHAN GENMASK(19, 16)
+
#define PCIE_PL_CHK_REG_CONTROL_STATUS 0xB20
#define PCIE_PL_CHK_REG_CHK_REG_START BIT(0)
#define PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS BIT(1)
@@ -215,6 +228,7 @@
* this offset, if atu_base not set.
*/
#define DEFAULT_DBI_ATU_OFFSET (0x3 << 20)
+#define DEFAULT_DBI_DMA_OFFSET PCIE_DMA_UNROLL_BASE
#define MAX_MSI_IRQS 256
#define MAX_MSI_IRQS_PER_CTRL 32
@@ -226,6 +240,9 @@
#define MAX_IATU_IN 256
#define MAX_IATU_OUT 256
+/* Default eDMA LLP memory size */
+#define DMA_LLP_MEM_SIZE PAGE_SIZE
+
struct dw_pcie;
struct dw_pcie_rp;
struct dw_pcie_ep;
@@ -370,6 +387,7 @@ struct dw_pcie {
int num_lanes;
int link_gen;
u8 n_fts[2];
+ struct dw_edma_chip edma;
struct clk_bulk_data app_clks[DW_PCIE_NUM_APP_CLKS];
struct clk_bulk_data core_clks[DW_PCIE_NUM_CORE_CLKS];
struct reset_control_bulk_data app_rsts[DW_PCIE_NUM_APP_RSTS];
@@ -409,6 +427,8 @@ int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
void dw_pcie_setup(struct dw_pcie *pci);
void dw_pcie_iatu_detect(struct dw_pcie *pci);
+int dw_pcie_edma_detect(struct dw_pcie *pci);
+void dw_pcie_edma_remove(struct dw_pcie *pci);
static inline void dw_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
{
--
2.35.1
On Mon, Aug 22, 2022 at 09:53:32PM +0300, Serge Semin wrote:
> Since the DW eDMA driver now supports eDMA controllers embedded into the
> locally accessible DW PCIe Root Ports and Endpoints, we can use the
> updated interface to register DW eDMA as DMA engine device if it's
> available. In order to successfully do that the DW PCIe core driver need
> to perform some preparations first. First of all it needs to find out the
> eDMA controller CSRs base address, whether they are accessible over the
> Port Logic or iATU unrolled space. Afterwards it can try to auto-detect
> the eDMA controller availability and number of it's read/write channels.
> If none was found the procedure will just silently halt with no error
> returned. Secondly the platform is supposed to provide either combined or
> per-channel IRQ signals. If no valid IRQs set is found the procedure will
> also halt with no error returned so to be backward compatible with the
> platforms where DW PCIe controllers have eDMA embedded but lack of the
> IRQs defined for them. Finally before actually probing the eDMA device we
> need to allocate LLP items buffers. After that the DW eDMA can be
> registered. If registration is successful the info-message regarding the
> number of detected Read/Write eDMA channels will be printed to the system
> log in the similar way as it's done for the iATU settings.
>
> Signed-off-by: Serge Semin <[email protected]>
> Reviewed-by: Manivannan Sadhasivam <[email protected]>
> Acked-By: Vinod Koul <[email protected]>
>
> ---
>
> Changelog v2:
> - Don't fail eDMA detection procedure if the DW eDMA driver couldn't probe
> device. That happens if the driver is disabled. (@Manivannan)
> - Add "dma" registers resource mapping procedure. (@Manivannan)
> - Move the eDMA CSRs space detection into the dw_pcie_map_detect() method.
> - Remove eDMA on the dw_pcie_ep_init() internal errors. (@Manivannan)
> - Remove eDMA in the dw_pcie_ep_exit() method.
> - Move the dw_pcie_edma_detect() method execution to the tail of the
> dw_pcie_ep_init() function.
>
> Changelog v3:
> - Add more comprehensive and less regression prune eDMA block detection
> procedure.
> - Remove Manivannan tb tag since the patch content has been changed.
> ---
> .../pci/controller/dwc/pcie-designware-ep.c | 12 +-
> .../pci/controller/dwc/pcie-designware-host.c | 13 +-
> drivers/pci/controller/dwc/pcie-designware.c | 186 ++++++++++++++++++
> drivers/pci/controller/dwc/pcie-designware.h | 20 ++
> 4 files changed, 228 insertions(+), 3 deletions(-)
>
[...]
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 1e06ccf2dc9e..3dcbdaf980e4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -12,6 +12,7 @@
> #include <linux/bitops.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> +#include <linux/dma/edma.h>
> #include <linux/gpio/consumer.h>
> #include <linux/ioport.h>
> #include <linux/of.h>
> @@ -142,6 +143,18 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> if (!pci->atu_size)
> pci->atu_size = SZ_4K;
>
> + /* eDMA region can be mapped to a custom base address */
> + if (!pci->edma.reg_base) {
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma");
> + if (res) {
> + pci->edma.reg_base = devm_ioremap_resource(pci->dev, res);
> + if (IS_ERR(pci->edma.reg_base))
> + return PTR_ERR(pci->edma.reg_base);
> + } else if (pci->atu_size >= 2 * DEFAULT_DBI_DMA_OFFSET) {
> + pci->edma.reg_base = pci->atu_base + DEFAULT_DBI_DMA_OFFSET;
> + }
> + }
> +
> /* LLDD is supposed to manually switch the clocks and resets state */
> if (dw_pcie_cap_is(pci, REQ_RES)) {
> ret = dw_pcie_get_clocks(pci);
> @@ -782,6 +795,179 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
> pci->region_align / SZ_1K, (pci->region_limit + 1) / SZ_1G);
> }
>
> +static u32 dw_pcie_readl_dma(struct dw_pcie *pci, u32 reg)
> +{
> + u32 val = 0;
> + int ret;
> +
> + if (pci->ops && pci->ops->read_dbi)
> + return pci->ops->read_dbi(pci, pci->edma.reg_base, reg, 4);
> +
> + ret = dw_pcie_read(pci->edma.reg_base + reg, 4, &val);
> + if (ret)
> + dev_err(pci->dev, "Read DMA address failed\n");
> +
> + return val;
> +}
> +
> +static int dw_pcie_edma_irq_vector(struct device *dev, unsigned int nr)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + char name[6];
> + int ret;
> +
> + if (nr >= EDMA_MAX_WR_CH + EDMA_MAX_RD_CH)
> + return -EINVAL;
> +
> + ret = platform_get_irq_byname_optional(pdev, "dma");
> + if (ret > 0)
> + return ret;
> +
> + snprintf(name, sizeof(name), "dma%u", nr);
> +
> + return platform_get_irq_byname_optional(pdev, name);
> +}
> +
> +static struct dw_edma_core_ops dw_pcie_edma_ops = {
> + .irq_vector = dw_pcie_edma_irq_vector,
> +};
> +
> +static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> +{
> + u32 val;
> +
> + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> + if (val == 0xFFFFFFFF && pci->edma.reg_base) {
Nit: Lower case for hex here and below?
Thanks,
Mani
> + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> +
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> + } else if (val != 0xFFFFFFFF) {
> + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
> +
> + pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> + } else {
> + 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);
> +
> + /* Sanity check the channels count if the mapping was incorrect */
> + if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
> + !pci->edma.ll_rd_cnt || pci->edma.ll_rd_cnt > EDMA_MAX_RD_CH)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
> +{
> + struct platform_device *pdev = to_platform_device(pci->dev);
> + u16 ch_cnt = pci->edma.ll_wr_cnt + pci->edma.ll_rd_cnt;
> + char name[6];
> + int ret;
> +
> + if (pci->edma.nr_irqs == 1)
> + return 0;
> + else if (pci->edma.nr_irqs > 1)
> + return pci->edma.nr_irqs != ch_cnt ? -EINVAL : 0;
> +
> + ret = platform_get_irq_byname_optional(pdev, "dma");
> + if (ret > 0) {
> + pci->edma.nr_irqs = 1;
> + return 0;
> + }
> +
> + for (; pci->edma.nr_irqs < ch_cnt; pci->edma.nr_irqs++) {
> + snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs);
> +
> + ret = platform_get_irq_byname_optional(pdev, name);
> + if (ret <= 0)
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int dw_pcie_edma_ll_alloc(struct dw_pcie *pci)
> +{
> + struct dw_edma_region *ll;
> + dma_addr_t paddr;
> + int i;
> +
> + for (i = 0; i < pci->edma.ll_wr_cnt; i++) {
> + ll = &pci->edma.ll_region_wr[i];
> + ll->sz = DMA_LLP_MEM_SIZE;
> + ll->vaddr = dmam_alloc_coherent(pci->dev, ll->sz,
> + &paddr, GFP_KERNEL);
> + if (!ll->vaddr)
> + return -ENOMEM;
> +
> + ll->paddr = paddr;
> + }
> +
> + for (i = 0; i < pci->edma.ll_rd_cnt; i++) {
> + ll = &pci->edma.ll_region_rd[i];
> + ll->sz = DMA_LLP_MEM_SIZE;
> + ll->vaddr = dmam_alloc_coherent(pci->dev, ll->sz,
> + &paddr, GFP_KERNEL);
> + if (!ll->vaddr)
> + return -ENOMEM;
> +
> + ll->paddr = paddr;
> + }
> +
> + return 0;
> +}
> +
> +int dw_pcie_edma_detect(struct dw_pcie *pci)
> +{
> + int ret;
> +
> + /* Don't fail if no eDMA was found (for the backward compatibility) */
> + ret = dw_pcie_edma_find_chip(pci);
> + if (ret)
> + return 0;
> +
> + /* Don't fail on the IRQs verification (for the backward compatibility) */
> + ret = dw_pcie_edma_irq_verify(pci);
> + if (ret) {
> + dev_err(pci->dev, "Invalid eDMA IRQs found\n");
> + return 0;
> + }
> +
> + ret = dw_pcie_edma_ll_alloc(pci);
> + if (ret) {
> + dev_err(pci->dev, "Couldn't allocate LLP memory\n");
> + return ret;
> + }
> +
> + /* Don't fail if the DW eDMA driver can't find the device */
> + ret = dw_edma_probe(&pci->edma);
> + if (ret && ret != -ENODEV) {
> + dev_err(pci->dev, "Couldn't register eDMA device\n");
> + return ret;
> + }
> +
> + dev_info(pci->dev, "eDMA: unroll %s, %hu wr, %hu rd\n",
> + pci->edma.mf == EDMA_MF_EDMA_UNROLL ? "T" : "F",
> + pci->edma.ll_wr_cnt, pci->edma.ll_rd_cnt);
> +
> + return 0;
> +}
> +
> +void dw_pcie_edma_remove(struct dw_pcie *pci)
> +{
> + dw_edma_remove(&pci->edma);
> +}
> +
> void dw_pcie_setup(struct dw_pcie *pci)
> {
> u32 val;
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index f5530069a204..d11c246b9bc1 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -15,6 +15,7 @@
> #include <linux/bitops.h>
> #include <linux/clk.h>
> #include <linux/dma-mapping.h>
> +#include <linux/dma/edma.h>
> #include <linux/gpio/consumer.h>
> #include <linux/irq.h>
> #include <linux/msi.h>
> @@ -167,6 +168,18 @@
> #define PCIE_MSIX_DOORBELL 0x948
> #define PCIE_MSIX_DOORBELL_PF_SHIFT 24
>
> +/*
> + * eDMA CSRs. DW PCIe IP-core v4.70a and older had the eDMA registers accessible
> + * over the Port Logic registers space. Afterwords the unrolled mapping was
> + * introduced so eDMA and iATU could be accessed via a dedicated registers
> + * space.
> + */
> +#define PCIE_DMA_VIEWPORT_BASE 0x970
> +#define PCIE_DMA_UNROLL_BASE 0x80000
> +#define PCIE_DMA_CTRL 0x008
> +#define PCIE_DMA_NUM_WR_CHAN GENMASK(3, 0)
> +#define PCIE_DMA_NUM_RD_CHAN GENMASK(19, 16)
> +
> #define PCIE_PL_CHK_REG_CONTROL_STATUS 0xB20
> #define PCIE_PL_CHK_REG_CHK_REG_START BIT(0)
> #define PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS BIT(1)
> @@ -215,6 +228,7 @@
> * this offset, if atu_base not set.
> */
> #define DEFAULT_DBI_ATU_OFFSET (0x3 << 20)
> +#define DEFAULT_DBI_DMA_OFFSET PCIE_DMA_UNROLL_BASE
>
> #define MAX_MSI_IRQS 256
> #define MAX_MSI_IRQS_PER_CTRL 32
> @@ -226,6 +240,9 @@
> #define MAX_IATU_IN 256
> #define MAX_IATU_OUT 256
>
> +/* Default eDMA LLP memory size */
> +#define DMA_LLP_MEM_SIZE PAGE_SIZE
> +
> struct dw_pcie;
> struct dw_pcie_rp;
> struct dw_pcie_ep;
> @@ -370,6 +387,7 @@ struct dw_pcie {
> int num_lanes;
> int link_gen;
> u8 n_fts[2];
> + struct dw_edma_chip edma;
> struct clk_bulk_data app_clks[DW_PCIE_NUM_APP_CLKS];
> struct clk_bulk_data core_clks[DW_PCIE_NUM_CORE_CLKS];
> struct reset_control_bulk_data app_rsts[DW_PCIE_NUM_APP_RSTS];
> @@ -409,6 +427,8 @@ int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
> void dw_pcie_setup(struct dw_pcie *pci);
> void dw_pcie_iatu_detect(struct dw_pcie *pci);
> +int dw_pcie_edma_detect(struct dw_pcie *pci);
> +void dw_pcie_edma_remove(struct dw_pcie *pci);
>
> static inline void dw_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
> {
> --
> 2.35.1
>
--
மணிவண்ணன் சதாசிவம்
On Tue, Aug 23, 2022 at 09:19:37PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Aug 22, 2022 at 09:53:32PM +0300, Serge Semin wrote:
> > Since the DW eDMA driver now supports eDMA controllers embedded into the
> > locally accessible DW PCIe Root Ports and Endpoints, we can use the
> > updated interface to register DW eDMA as DMA engine device if it's
> > available. In order to successfully do that the DW PCIe core driver need
> > to perform some preparations first. First of all it needs to find out the
> > eDMA controller CSRs base address, whether they are accessible over the
> > Port Logic or iATU unrolled space. Afterwards it can try to auto-detect
> > the eDMA controller availability and number of it's read/write channels.
> > If none was found the procedure will just silently halt with no error
> > returned. Secondly the platform is supposed to provide either combined or
> > per-channel IRQ signals. If no valid IRQs set is found the procedure will
> > also halt with no error returned so to be backward compatible with the
> > platforms where DW PCIe controllers have eDMA embedded but lack of the
> > IRQs defined for them. Finally before actually probing the eDMA device we
> > need to allocate LLP items buffers. After that the DW eDMA can be
> > registered. If registration is successful the info-message regarding the
> > number of detected Read/Write eDMA channels will be printed to the system
> > log in the similar way as it's done for the iATU settings.
> >
> > Signed-off-by: Serge Semin <[email protected]>
> > Reviewed-by: Manivannan Sadhasivam <[email protected]>
> > Acked-By: Vinod Koul <[email protected]>
> >
> > ---
> >
> > Changelog v2:
> > - Don't fail eDMA detection procedure if the DW eDMA driver couldn't probe
> > device. That happens if the driver is disabled. (@Manivannan)
> > - Add "dma" registers resource mapping procedure. (@Manivannan)
> > - Move the eDMA CSRs space detection into the dw_pcie_map_detect() method.
> > - Remove eDMA on the dw_pcie_ep_init() internal errors. (@Manivannan)
> > - Remove eDMA in the dw_pcie_ep_exit() method.
> > - Move the dw_pcie_edma_detect() method execution to the tail of the
> > dw_pcie_ep_init() function.
> >
> > Changelog v3:
> > - Add more comprehensive and less regression prune eDMA block detection
> > procedure.
> > - Remove Manivannan tb tag since the patch content has been changed.
> > ---
> > .../pci/controller/dwc/pcie-designware-ep.c | 12 +-
> > .../pci/controller/dwc/pcie-designware-host.c | 13 +-
> > drivers/pci/controller/dwc/pcie-designware.c | 186 ++++++++++++++++++
> > drivers/pci/controller/dwc/pcie-designware.h | 20 ++
> > 4 files changed, 228 insertions(+), 3 deletions(-)
> >
>
> [...]
>
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 1e06ccf2dc9e..3dcbdaf980e4 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -12,6 +12,7 @@
> > #include <linux/bitops.h>
> > #include <linux/clk.h>
> > #include <linux/delay.h>
> > +#include <linux/dma/edma.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/ioport.h>
> > #include <linux/of.h>
> > @@ -142,6 +143,18 @@ int dw_pcie_get_resources(struct dw_pcie *pci)
> > if (!pci->atu_size)
> > pci->atu_size = SZ_4K;
> >
> > + /* eDMA region can be mapped to a custom base address */
> > + if (!pci->edma.reg_base) {
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dma");
> > + if (res) {
> > + pci->edma.reg_base = devm_ioremap_resource(pci->dev, res);
> > + if (IS_ERR(pci->edma.reg_base))
> > + return PTR_ERR(pci->edma.reg_base);
> > + } else if (pci->atu_size >= 2 * DEFAULT_DBI_DMA_OFFSET) {
> > + pci->edma.reg_base = pci->atu_base + DEFAULT_DBI_DMA_OFFSET;
> > + }
> > + }
> > +
> > /* LLDD is supposed to manually switch the clocks and resets state */
> > if (dw_pcie_cap_is(pci, REQ_RES)) {
> > ret = dw_pcie_get_clocks(pci);
> > @@ -782,6 +795,179 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
> > pci->region_align / SZ_1K, (pci->region_limit + 1) / SZ_1G);
> > }
> >
> > +static u32 dw_pcie_readl_dma(struct dw_pcie *pci, u32 reg)
> > +{
> > + u32 val = 0;
> > + int ret;
> > +
> > + if (pci->ops && pci->ops->read_dbi)
> > + return pci->ops->read_dbi(pci, pci->edma.reg_base, reg, 4);
> > +
> > + ret = dw_pcie_read(pci->edma.reg_base + reg, 4, &val);
> > + if (ret)
> > + dev_err(pci->dev, "Read DMA address failed\n");
> > +
> > + return val;
> > +}
> > +
> > +static int dw_pcie_edma_irq_vector(struct device *dev, unsigned int nr)
> > +{
> > + struct platform_device *pdev = to_platform_device(dev);
> > + char name[6];
> > + int ret;
> > +
> > + if (nr >= EDMA_MAX_WR_CH + EDMA_MAX_RD_CH)
> > + return -EINVAL;
> > +
> > + ret = platform_get_irq_byname_optional(pdev, "dma");
> > + if (ret > 0)
> > + return ret;
> > +
> > + snprintf(name, sizeof(name), "dma%u", nr);
> > +
> > + return platform_get_irq_byname_optional(pdev, name);
> > +}
> > +
> > +static struct dw_edma_core_ops dw_pcie_edma_ops = {
> > + .irq_vector = dw_pcie_edma_irq_vector,
> > +};
> > +
> > +static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> > +{
> > + u32 val;
> > +
> > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > + if (val == 0xFFFFFFFF && pci->edma.reg_base) {
>
> Nit: Lower case for hex here and below?
I would have fully agreed with you if the rest of the file had the
lower-cased literals. But in this case it uses the upper-case hexes.
So IMO we'd better stick to the convention specific to the file for
now. If it gets to be that much required somebody will perform the
conversion at once in the framework of another cleanup patch(set).
-Sergey
>
> Thanks,
> Mani
>
> > + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > +
> > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > + } else if (val != 0xFFFFFFFF) {
> > + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
> > +
> > + pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> > + } else {
> > + 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);
> > +
> > + /* Sanity check the channels count if the mapping was incorrect */
> > + if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > EDMA_MAX_WR_CH ||
> > + !pci->edma.ll_rd_cnt || pci->edma.ll_rd_cnt > EDMA_MAX_RD_CH)
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
> > +{
> > + struct platform_device *pdev = to_platform_device(pci->dev);
> > + u16 ch_cnt = pci->edma.ll_wr_cnt + pci->edma.ll_rd_cnt;
> > + char name[6];
> > + int ret;
> > +
> > + if (pci->edma.nr_irqs == 1)
> > + return 0;
> > + else if (pci->edma.nr_irqs > 1)
> > + return pci->edma.nr_irqs != ch_cnt ? -EINVAL : 0;
> > +
> > + ret = platform_get_irq_byname_optional(pdev, "dma");
> > + if (ret > 0) {
> > + pci->edma.nr_irqs = 1;
> > + return 0;
> > + }
> > +
> > + for (; pci->edma.nr_irqs < ch_cnt; pci->edma.nr_irqs++) {
> > + snprintf(name, sizeof(name), "dma%d", pci->edma.nr_irqs);
> > +
> > + ret = platform_get_irq_byname_optional(pdev, name);
> > + if (ret <= 0)
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int dw_pcie_edma_ll_alloc(struct dw_pcie *pci)
> > +{
> > + struct dw_edma_region *ll;
> > + dma_addr_t paddr;
> > + int i;
> > +
> > + for (i = 0; i < pci->edma.ll_wr_cnt; i++) {
> > + ll = &pci->edma.ll_region_wr[i];
> > + ll->sz = DMA_LLP_MEM_SIZE;
> > + ll->vaddr = dmam_alloc_coherent(pci->dev, ll->sz,
> > + &paddr, GFP_KERNEL);
> > + if (!ll->vaddr)
> > + return -ENOMEM;
> > +
> > + ll->paddr = paddr;
> > + }
> > +
> > + for (i = 0; i < pci->edma.ll_rd_cnt; i++) {
> > + ll = &pci->edma.ll_region_rd[i];
> > + ll->sz = DMA_LLP_MEM_SIZE;
> > + ll->vaddr = dmam_alloc_coherent(pci->dev, ll->sz,
> > + &paddr, GFP_KERNEL);
> > + if (!ll->vaddr)
> > + return -ENOMEM;
> > +
> > + ll->paddr = paddr;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int dw_pcie_edma_detect(struct dw_pcie *pci)
> > +{
> > + int ret;
> > +
> > + /* Don't fail if no eDMA was found (for the backward compatibility) */
> > + ret = dw_pcie_edma_find_chip(pci);
> > + if (ret)
> > + return 0;
> > +
> > + /* Don't fail on the IRQs verification (for the backward compatibility) */
> > + ret = dw_pcie_edma_irq_verify(pci);
> > + if (ret) {
> > + dev_err(pci->dev, "Invalid eDMA IRQs found\n");
> > + return 0;
> > + }
> > +
> > + ret = dw_pcie_edma_ll_alloc(pci);
> > + if (ret) {
> > + dev_err(pci->dev, "Couldn't allocate LLP memory\n");
> > + return ret;
> > + }
> > +
> > + /* Don't fail if the DW eDMA driver can't find the device */
> > + ret = dw_edma_probe(&pci->edma);
> > + if (ret && ret != -ENODEV) {
> > + dev_err(pci->dev, "Couldn't register eDMA device\n");
> > + return ret;
> > + }
> > +
> > + dev_info(pci->dev, "eDMA: unroll %s, %hu wr, %hu rd\n",
> > + pci->edma.mf == EDMA_MF_EDMA_UNROLL ? "T" : "F",
> > + pci->edma.ll_wr_cnt, pci->edma.ll_rd_cnt);
> > +
> > + return 0;
> > +}
> > +
> > +void dw_pcie_edma_remove(struct dw_pcie *pci)
> > +{
> > + dw_edma_remove(&pci->edma);
> > +}
> > +
> > void dw_pcie_setup(struct dw_pcie *pci)
> > {
> > u32 val;
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index f5530069a204..d11c246b9bc1 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -15,6 +15,7 @@
> > #include <linux/bitops.h>
> > #include <linux/clk.h>
> > #include <linux/dma-mapping.h>
> > +#include <linux/dma/edma.h>
> > #include <linux/gpio/consumer.h>
> > #include <linux/irq.h>
> > #include <linux/msi.h>
> > @@ -167,6 +168,18 @@
> > #define PCIE_MSIX_DOORBELL 0x948
> > #define PCIE_MSIX_DOORBELL_PF_SHIFT 24
> >
> > +/*
> > + * eDMA CSRs. DW PCIe IP-core v4.70a and older had the eDMA registers accessible
> > + * over the Port Logic registers space. Afterwords the unrolled mapping was
> > + * introduced so eDMA and iATU could be accessed via a dedicated registers
> > + * space.
> > + */
> > +#define PCIE_DMA_VIEWPORT_BASE 0x970
> > +#define PCIE_DMA_UNROLL_BASE 0x80000
> > +#define PCIE_DMA_CTRL 0x008
> > +#define PCIE_DMA_NUM_WR_CHAN GENMASK(3, 0)
> > +#define PCIE_DMA_NUM_RD_CHAN GENMASK(19, 16)
> > +
> > #define PCIE_PL_CHK_REG_CONTROL_STATUS 0xB20
> > #define PCIE_PL_CHK_REG_CHK_REG_START BIT(0)
> > #define PCIE_PL_CHK_REG_CHK_REG_CONTINUOUS BIT(1)
> > @@ -215,6 +228,7 @@
> > * this offset, if atu_base not set.
> > */
> > #define DEFAULT_DBI_ATU_OFFSET (0x3 << 20)
> > +#define DEFAULT_DBI_DMA_OFFSET PCIE_DMA_UNROLL_BASE
> >
> > #define MAX_MSI_IRQS 256
> > #define MAX_MSI_IRQS_PER_CTRL 32
> > @@ -226,6 +240,9 @@
> > #define MAX_IATU_IN 256
> > #define MAX_IATU_OUT 256
> >
> > +/* Default eDMA LLP memory size */
> > +#define DMA_LLP_MEM_SIZE PAGE_SIZE
> > +
> > struct dw_pcie;
> > struct dw_pcie_rp;
> > struct dw_pcie_ep;
> > @@ -370,6 +387,7 @@ struct dw_pcie {
> > int num_lanes;
> > int link_gen;
> > u8 n_fts[2];
> > + struct dw_edma_chip edma;
> > struct clk_bulk_data app_clks[DW_PCIE_NUM_APP_CLKS];
> > struct clk_bulk_data core_clks[DW_PCIE_NUM_CORE_CLKS];
> > struct reset_control_bulk_data app_rsts[DW_PCIE_NUM_APP_RSTS];
> > @@ -409,6 +427,8 @@ int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
> > void dw_pcie_setup(struct dw_pcie *pci);
> > void dw_pcie_iatu_detect(struct dw_pcie *pci);
> > +int dw_pcie_edma_detect(struct dw_pcie *pci);
> > +void dw_pcie_edma_remove(struct dw_pcie *pci);
> >
> > static inline void dw_pcie_writel_dbi(struct dw_pcie *pci, u32 reg, u32 val)
> > {
> > --
> > 2.35.1
> >
>
> --
> மணிவண்ணன் சதாசிவம்
On Mon, Aug 22, 2022 at 09:53:32PM +0300, Serge Semin wrote:
> Since the DW eDMA driver now supports eDMA controllers embedded into the
> locally accessible DW PCIe Root Ports and Endpoints, we can use the
> updated interface to register DW eDMA as DMA engine device if it's
> available. In order to successfully do that the DW PCIe core driver need
> to perform some preparations first. First of all it needs to find out the
> eDMA controller CSRs base address, whether they are accessible over the
> Port Logic or iATU unrolled space. Afterwards it can try to auto-detect
> the eDMA controller availability and number of it's read/write channels.
s/it's//
> If none was found the procedure will just silently halt with no error
> returned. Secondly the platform is supposed to provide either combined or
> per-channel IRQ signals. If no valid IRQs set is found the procedure will
> also halt with no error returned so to be backward compatible with the
> platforms where DW PCIe controllers have eDMA embedded but lack of the
> IRQs defined for them. Finally before actually probing the eDMA device we
> need to allocate LLP items buffers. After that the DW eDMA can be
> registered. If registration is successful the info-message regarding the
> number of detected Read/Write eDMA channels will be printed to the system
> log in the similar way as it's done for the iATU settings.
s/in the similar way as it's done/as is done/
> +static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> +{
> + u32 val;
> +
> + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> + if (val == 0xFFFFFFFF && pci->edma.reg_base) {
> + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> +
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> + } else if (val != 0xFFFFFFFF) {
Consider PCI_POSSIBLE_ERROR() as an annotation about the meaning of
0xFFFFFFFF and something to grep for.
> + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
> +
> + pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> + } else {
> + return -ENODEV;
> + }
> + * eDMA CSRs. DW PCIe IP-core v4.70a and older had the eDMA registers accessible
> + * over the Port Logic registers space. Afterwords the unrolled mapping was
s/Afterwords/Afterwards/
> + * introduced so eDMA and iATU could be accessed via a dedicated registers
> + * space.
On Wed, Aug 24, 2022 at 09:13:19PM +0300, Serge Semin wrote:
> On Wed, Aug 24, 2022 at 11:51:18AM -0500, Bjorn Helgaas wrote:
> > On Mon, Aug 22, 2022 at 09:53:32PM +0300, Serge Semin wrote:
> > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > + if (val == 0xFFFFFFFF && pci->edma.reg_base) {
> > > + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > > +
> > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > + } else if (val != 0xFFFFFFFF) {
> >
>
> > Consider PCI_POSSIBLE_ERROR() as an annotation about the meaning of
> > 0xFFFFFFFF and something to grep for.
>
> In this case FFs don't mean an error but a special value, which
> indicates that the eDMA is mapped via the unrolled CSRs space. The
> similar approach has been implemented for the iATU legacy/unroll setup
> auto-detection. So I don't see much reasons to have it grepped, so as
> to have a macro-based parametrization since the special value will
> unluckily change while having the explicit literal utilized gives a
> better understanding of the way the algorithm works.
If 0xFFFFFFFF is the result of a successful PCIe Memory Read, and not
something synthesized by the host bridge when it handles an
Unsupported Request completion, I'm fine with keeping it as is.
On Wed, Aug 24, 2022 at 11:51:18AM -0500, Bjorn Helgaas wrote:
> On Mon, Aug 22, 2022 at 09:53:32PM +0300, Serge Semin wrote:
> > Since the DW eDMA driver now supports eDMA controllers embedded into the
> > locally accessible DW PCIe Root Ports and Endpoints, we can use the
> > updated interface to register DW eDMA as DMA engine device if it's
> > available. In order to successfully do that the DW PCIe core driver need
> > to perform some preparations first. First of all it needs to find out the
> > eDMA controller CSRs base address, whether they are accessible over the
> > Port Logic or iATU unrolled space. Afterwards it can try to auto-detect
> > the eDMA controller availability and number of it's read/write channels.
>
> s/it's//
Ok.
>
> > If none was found the procedure will just silently halt with no error
> > returned. Secondly the platform is supposed to provide either combined or
> > per-channel IRQ signals. If no valid IRQs set is found the procedure will
> > also halt with no error returned so to be backward compatible with the
> > platforms where DW PCIe controllers have eDMA embedded but lack of the
> > IRQs defined for them. Finally before actually probing the eDMA device we
> > need to allocate LLP items buffers. After that the DW eDMA can be
> > registered. If registration is successful the info-message regarding the
> > number of detected Read/Write eDMA channels will be printed to the system
> > log in the similar way as it's done for the iATU settings.
>
> s/in the similar way as it's done/as is done/
Ok
>
> > +static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> > +{
> > + u32 val;
> > +
> > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > + if (val == 0xFFFFFFFF && pci->edma.reg_base) {
> > + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > +
> > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > + } else if (val != 0xFFFFFFFF) {
>
> Consider PCI_POSSIBLE_ERROR() as an annotation about the meaning of
> 0xFFFFFFFF and something to grep for.
In this case FFs don't mean an error but a special value, which
indicates that the eDMA is mapped via the unrolled CSRs space. The
similar approach has been implemented for the iATU legacy/unroll setup
auto-detection. So I don't see much reasons to have it grepped, so as
to have a macro-based parametrization since the special value will
unluckily change while having the explicit literal utilized gives a
better understanding of the way the algorithm works.
>
> > + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
> > +
> > + pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> > + } else {
> > + return -ENODEV;
> > + }
>
> > + * eDMA CSRs. DW PCIe IP-core v4.70a and older had the eDMA registers accessible
> > + * over the Port Logic registers space. Afterwords the unrolled mapping was
>
> s/Afterwords/Afterwards/
Ok.
-Sergey
>
> > + * introduced so eDMA and iATU could be accessed via a dedicated registers
> > + * space.
On Wed, Aug 24, 2022 at 01:17:54PM -0500, Bjorn Helgaas wrote:
> On Wed, Aug 24, 2022 at 09:13:19PM +0300, Serge Semin wrote:
> > On Wed, Aug 24, 2022 at 11:51:18AM -0500, Bjorn Helgaas wrote:
> > > On Mon, Aug 22, 2022 at 09:53:32PM +0300, Serge Semin wrote:
>
> > > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > > + if (val == 0xFFFFFFFF && pci->edma.reg_base) {
> > > > + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > > > +
> > > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > > + } else if (val != 0xFFFFFFFF) {
> > >
> >
> > > Consider PCI_POSSIBLE_ERROR() as an annotation about the meaning of
> > > 0xFFFFFFFF and something to grep for.
> >
> > In this case FFs don't mean an error but a special value, which
> > indicates that the eDMA is mapped via the unrolled CSRs space. The
> > similar approach has been implemented for the iATU legacy/unroll setup
> > auto-detection. So I don't see much reasons to have it grepped, so as
> > to have a macro-based parametrization since the special value will
> > unluckily change while having the explicit literal utilized gives a
> > better understanding of the way the algorithm works.
>
> If 0xFFFFFFFF is the result of a successful PCIe Memory Read,
Right. It is.
> and not
> something synthesized by the host bridge when it handles an
> Unsupported Request completion,
No it isn't. To be clear 0xFFs don't indicate some PCIe bus/controller
malfunction, but they are a result of reading the
DMA_CTRL_VIEWPORT_OFF register which doesn't exist. The manual
explicitly says: "Note - When register does not exist, value is fixed
to 32'hFFFF_FFFF". The register doesn't exist if either eDMA is
unavailable or the eDMA CSRs are mapped via the unrolled state. That
logic is used to auto-detect the eDMA availability and the way of it's
CSRs mapping.
> I'm fine with keeping it as is.
Ok. Thanks.
-Sergey
On Thu, Aug 25, 2022 at 08:16:14AM +0300, Serge Semin wrote:
> On Wed, Aug 24, 2022 at 01:17:54PM -0500, Bjorn Helgaas wrote:
> > On Wed, Aug 24, 2022 at 09:13:19PM +0300, Serge Semin wrote:
> > > On Wed, Aug 24, 2022 at 11:51:18AM -0500, Bjorn Helgaas wrote:
> > > > On Mon, Aug 22, 2022 at 09:53:32PM +0300, Serge Semin wrote:
> >
> > > > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > > > + if (val == 0xFFFFFFFF && pci->edma.reg_base) {
> > > > > + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > > > > +
> > > > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > > > + } else if (val != 0xFFFFFFFF) {
> > > >
> > >
> > > > Consider PCI_POSSIBLE_ERROR() as an annotation about the meaning of
> > > > 0xFFFFFFFF and something to grep for.
> > >
> > > In this case FFs don't mean an error but a special value, which
> > > indicates that the eDMA is mapped via the unrolled CSRs space. The
> > > similar approach has been implemented for the iATU legacy/unroll setup
> > > auto-detection. So I don't see much reasons to have it grepped, so as
> > > to have a macro-based parametrization since the special value will
> > > unluckily change while having the explicit literal utilized gives a
> > > better understanding of the way the algorithm works.
>
> > If 0xFFFFFFFF is the result of a successful PCIe Memory Read,
>
> Right. It is.
>
> > and not
> > something synthesized by the host bridge when it handles an
> > Unsupported Request completion,
>
> No it isn't. To be clear 0xFFs don't indicate some PCIe bus/controller
> malfunction, but they are a result of reading the
> DMA_CTRL_VIEWPORT_OFF register which doesn't exist. The manual
> explicitly says: "Note - When register does not exist, value is fixed
> to 32'hFFFF_FFFF". The register doesn't exist if either eDMA is
> unavailable or the eDMA CSRs are mapped via the unrolled state.
OK. I don't think that's worded very well in the manual. A register
that does not exist does not have a value, and attempts to read it
should fail. If they want to say the register always exists and
contains 0xFFFFFFFF for versions earlier than X, that would make
sense. Wouldn't be the first time a manual is ambiguous ;)
If the device itself, i.e., not the Root Complex, is fabricating this
0xFFFFFFFF value, reading it should not cause any AER or other error
status bits to be set.
If the Root Complex fabricates 0xFFFFFFFF upon receipt of a Completion
with Unsupported Request status, I would expect bits like Received
Master Abort to be set in the Root Port's Secondary Status register.
On Thu, Aug 25, 2022 at 11:04:43AM -0500, Bjorn Helgaas wrote:
> On Thu, Aug 25, 2022 at 08:16:14AM +0300, Serge Semin wrote:
> > On Wed, Aug 24, 2022 at 01:17:54PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Aug 24, 2022 at 09:13:19PM +0300, Serge Semin wrote:
> > > > On Wed, Aug 24, 2022 at 11:51:18AM -0500, Bjorn Helgaas wrote:
> > > > > On Mon, Aug 22, 2022 at 09:53:32PM +0300, Serge Semin wrote:
> > >
> > > > > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > > > > + if (val == 0xFFFFFFFF && pci->edma.reg_base) {
> > > > > > + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > > > > > +
> > > > > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > > > > + } else if (val != 0xFFFFFFFF) {
> > > > >
> > > >
> > > > > Consider PCI_POSSIBLE_ERROR() as an annotation about the meaning of
> > > > > 0xFFFFFFFF and something to grep for.
> > > >
> > > > In this case FFs don't mean an error but a special value, which
> > > > indicates that the eDMA is mapped via the unrolled CSRs space. The
> > > > similar approach has been implemented for the iATU legacy/unroll setup
> > > > auto-detection. So I don't see much reasons to have it grepped, so as
> > > > to have a macro-based parametrization since the special value will
> > > > unluckily change while having the explicit literal utilized gives a
> > > > better understanding of the way the algorithm works.
> >
> > > If 0xFFFFFFFF is the result of a successful PCIe Memory Read,
> >
> > Right. It is.
> >
> > > and not
> > > something synthesized by the host bridge when it handles an
> > > Unsupported Request completion,
> >
> > No it isn't. To be clear 0xFFs don't indicate some PCIe bus/controller
> > malfunction, but they are a result of reading the
> > DMA_CTRL_VIEWPORT_OFF register which doesn't exist. The manual
> > explicitly says: "Note - When register does not exist, value is fixed
> > to 32'hFFFF_FFFF". The register doesn't exist if either eDMA is
> > unavailable or the eDMA CSRs are mapped via the unrolled state.
>
> OK. I don't think that's worded very well in the manual. A register
> that does not exist does not have a value, and attempts to read it
> should fail.
No. The manual explicitly says that this particular CSR
(DMA_CTRL_VIEWPORT_OFF) value is tied to 32'hFFFF_FFFF if the register
doesn't exist. There is no such text mentioned for any other
non-existing CSR.
> If they want to say the register always exists and
> contains 0xFFFFFFFF for versions earlier than X, that would make
> sense. Wouldn't be the first time a manual is ambiguous ;)
They say, that the register doesn't exist if either eDMA isn't
available or it's mapped via the unrolled CSR space. There is no
reference to the IP-core version.
Anyway basically you are right they indeed imply that the register
always exists.
>
> If the device itself, i.e., not the Root Complex, is fabricating this
> 0xFFFFFFFF value, reading it should not cause any AER or other error
> status bits to be set.
>
> If the Root Complex fabricates 0xFFFFFFFF upon receipt of a Completion
> with Unsupported Request status, I would expect bits like Received
> Master Abort to be set in the Root Port's Secondary Status register.
The device CSRs are accessed by means of the controller DBI-interface.
Even though the whole CSRs space do look as the extended PCIe config
space, the DBI-based access isn't tracked by the standard PCIe
capabilities like AER. So in case of the eDMA auto-detection procedure
introduced in this patch (and iATU auto-detection, which is already
available in the DW PCIe driver ) we won't have any bus error status
raised.
-Sergey