LinuxLists.cc - [PATCH v1 0/6] Add Change to integrate HDMA with dwc ep driver

[permalink] [raw]

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

From: Manivannan Sadhasivam <[email protected]>

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 <[email protected]>
Signed-off-by: Mrinmay Sarkar <[email protected]>
---
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;
+ }
+
+ 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

2024-01-19 13:02:14

[permalink] [raw]

Subject: [PATCH v1 5/6] PCI: qcom-ep: Provide number of read/write channel for HDMA

There is no standard way to auto detect the number of available
read/write channels in a platform. So adding this change to provide
read/write channels count and also provide "EDMA_MF_HDMA_NATIVE"
flag to support HDMA for 8775 platform.

8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for
this platform. Add struct qcom_pcie_ep_cfg as match data. Assign
hdma_supported flag into struct qcom_pcie_ep_cfg and set it true
in cfg_1_34_0.

Signed-off-by: Mrinmay Sarkar <[email protected]>
---
drivers/pci/controller/dwc/pcie-qcom-ep.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
index 45008e0..8d56435 100644
--- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
+++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
@@ -149,6 +149,10 @@ enum qcom_pcie_ep_link_status {
QCOM_PCIE_EP_LINK_DOWN,
};

+struct qcom_pcie_ep_cfg {
+ bool hdma_supported;
+};
+
/**
* struct qcom_pcie_ep - Qualcomm PCIe Endpoint Controller
* @pci: Designware PCIe controller struct
@@ -167,6 +171,7 @@ enum qcom_pcie_ep_link_status {
* @num_clks: PCIe clocks count
* @perst_en: Flag for PERST enable
* @perst_sep_en: Flag for PERST separation enable
+ * @cfg: PCIe EP config struct
* @link_status: PCIe Link status
* @global_irq: Qualcomm PCIe specific Global IRQ
* @perst_irq: PERST# IRQ
@@ -194,6 +199,7 @@ struct qcom_pcie_ep {
u32 perst_en;
u32 perst_sep_en;

+ const struct qcom_pcie_ep_cfg *cfg;
enum qcom_pcie_ep_link_status link_status;
int global_irq;
int perst_irq;
@@ -511,6 +517,10 @@ static void qcom_pcie_perst_assert(struct dw_pcie *pci)
pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
}

+static const struct qcom_pcie_ep_cfg cfg_1_34_0 = {
+ .hdma_supported = true,
+};
+
/* Common DWC controller ops */
static const struct dw_pcie_ops pci_ops = {
.link_up = qcom_pcie_dw_link_up,
@@ -816,6 +826,13 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
pcie_ep->pci.ops = &pci_ops;
pcie_ep->pci.ep.ops = &pci_ep_ops;
pcie_ep->pci.edma.nr_irqs = 1;
+
+ pcie_ep->cfg = of_device_get_match_data(dev);
+ if (pcie_ep->cfg && pcie_ep->cfg->hdma_supported) {
+ pcie_ep->pci.edma.ll_wr_cnt = 1;
+ pcie_ep->pci.edma.ll_rd_cnt = 1;
+ pcie_ep->pci.edma.mf = EDMA_MF_HDMA_NATIVE;
+ }
platform_set_drvdata(pdev, pcie_ep);

ret = qcom_pcie_ep_get_resources(pdev, pcie_ep);
@@ -875,7 +892,7 @@ static void qcom_pcie_ep_remove(struct platform_device *pdev)
}

static const struct of_device_id qcom_pcie_ep_match[] = {
- { .compatible = "qcom,sa8775p-pcie-ep", },
+ { .compatible = "qcom,sa8775p-pcie-ep", .data = &cfg_1_34_0},
{ .compatible = "qcom,sdx55-pcie-ep", },
{ .compatible = "qcom,sm8450-pcie-ep", },
{ }
--
2.7.4

2024-01-19 13:02:24

[permalink] [raw]

Subject: [PATCH v1 4/6] dmaengine: dw-edma: Move HDMA_V0_MAX_NR_CH definition to edma.h

From: Manivannan Sadhasivam <[email protected]>

To maintain uniformity with eDMA, let's move the HDMA max channel
definition to edma.h. While at it, let's also rename it to HDMA_MAX_NR_CH.

Signed-off-by: Manivannan Sadhasivam <[email protected]>
Signed-off-by: Mrinmay Sarkar <[email protected]>
---
drivers/dma/dw-edma/dw-hdma-v0-core.c | 4 ++--
drivers/dma/dw-edma/dw-hdma-v0-regs.h | 3 +--
include/linux/dma/edma.h | 1 +
3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
index 1f4cb7d..819ef1f 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
+++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
@@ -54,7 +54,7 @@ static void dw_hdma_v0_core_off(struct dw_edma *dw)
{
int id;

- for (id = 0; id < HDMA_V0_MAX_NR_CH; id++) {
+ for (id = 0; id < HDMA_MAX_NR_CH; id++) {
SET_BOTH_CH_32(dw, id, int_setup,
HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK);
SET_BOTH_CH_32(dw, id, int_clear,
@@ -70,7 +70,7 @@ static u16 dw_hdma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir)
* available, we set it to maximum channels and let the platform
* set the right number of channels.
*/
- return HDMA_V0_MAX_NR_CH;
+ return HDMA_MAX_NR_CH;
}

static enum dma_status dw_hdma_v0_core_ch_status(struct dw_edma_chan *chan)
diff --git a/drivers/dma/dw-edma/dw-hdma-v0-regs.h b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
index a974abd..cd7eab2 100644
--- a/drivers/dma/dw-edma/dw-hdma-v0-regs.h
+++ b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
@@ -11,7 +11,6 @@

#include <linux/dmaengine.h>

-#define HDMA_V0_MAX_NR_CH 8
#define HDMA_V0_LOCAL_ABORT_INT_EN BIT(6)
#define HDMA_V0_REMOTE_ABORT_INT_EN BIT(5)
#define HDMA_V0_LOCAL_STOP_INT_EN BIT(4)
@@ -92,7 +91,7 @@ struct dw_hdma_v0_ch {
} __packed;

struct dw_hdma_v0_regs {
- struct dw_hdma_v0_ch ch[HDMA_V0_MAX_NR_CH]; /* 0x0000..0x0fa8 */
+ struct dw_hdma_v0_ch ch[HDMA_MAX_NR_CH]; /* 0x0000..0x0fa8 */
} __packed;

struct dw_hdma_v0_lli {
diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
index 550f6a4..2cdf249a 100644
--- a/include/linux/dma/edma.h
+++ b/include/linux/dma/edma.h
@@ -14,6 +14,7 @@

#define EDMA_MAX_WR_CH 8
#define EDMA_MAX_RD_CH 8
+#define HDMA_MAX_NR_CH 8

struct dw_edma;
struct dw_edma_chip;
--
2.7.4

2024-01-19 13:02:37

[permalink] [raw]

Subject: [PATCH v1 6/6] PCI: epf-mhi: Add flag to enable HDMA for SA8775P

SA8775P supports HDMA as DMA engine so adding 'MHI_EPF_USE_DMA'
flag to enable HDMA support.

Signed-off-by: Mrinmay Sarkar <[email protected]>
---
drivers/pci/endpoint/functions/pci-epf-mhi.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
index 2c54d80..570c1d1f 100644
--- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
+++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
@@ -137,6 +137,7 @@ static const struct pci_epf_mhi_ep_info sa8775p_info = {
.epf_flags = PCI_BASE_ADDRESS_MEM_TYPE_32,
.msi_count = 32,
.mru = 0x8000,
+ .flags = MHI_EPF_USE_DMA,
};

struct pci_epf_mhi {
--
2.7.4

2024-01-30 08:54:40

[permalink] [raw]

Subject: Re: [PATCH v1 6/6] PCI: epf-mhi: Add flag to enable HDMA for SA8775P

On Fri, Jan 19, 2024 at 06:30:22PM +0530, Mrinmay Sarkar wrote:
> SA8775P supports HDMA as DMA engine so adding 'MHI_EPF_USE_DMA'

s/adding/add

> flag to enable HDMA support.
>
> Signed-off-by: Mrinmay Sarkar <[email protected]>

With above addressed,

Reviewed-by: Manivannan Sadhasivam <[email protected]>

- Mani

> ---
> drivers/pci/endpoint/functions/pci-epf-mhi.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-mhi.c b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> index 2c54d80..570c1d1f 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-mhi.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-mhi.c
> @@ -137,6 +137,7 @@ static const struct pci_epf_mhi_ep_info sa8775p_info = {
> .epf_flags = PCI_BASE_ADDRESS_MEM_TYPE_32,
> .msi_count = 32,
> .mru = 0x8000,
> + .flags = MHI_EPF_USE_DMA,
> };
>
> struct pci_epf_mhi {
> --
> 2.7.4
>

--
மணிவண்ணன் சதாசிவம்

2024-01-30 09:33:48

[permalink] [raw]

Subject: Re: [PATCH v1 5/6] PCI: qcom-ep: Provide number of read/write channel for HDMA

On Fri, Jan 19, 2024 at 06:30:21PM +0530, Mrinmay Sarkar wrote:
> There is no standard way to auto detect the number of available
> read/write channels in a platform. So adding this change to provide
> read/write channels count and also provide "EDMA_MF_HDMA_NATIVE"
> flag to support HDMA for 8775 platform.
>
> 8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for
> this platform. Add struct qcom_pcie_ep_cfg as match data. Assign
> hdma_supported flag into struct qcom_pcie_ep_cfg and set it true
> in cfg_1_34_0.
>
> Signed-off-by: Mrinmay Sarkar <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-qcom-ep.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> index 45008e0..8d56435 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
> @@ -149,6 +149,10 @@ enum qcom_pcie_ep_link_status {
> QCOM_PCIE_EP_LINK_DOWN,
> };
>

Add kdoc comment please as like the below struct.

> +struct qcom_pcie_ep_cfg {
> + bool hdma_supported;
> +};
> +
> /**
> * struct qcom_pcie_ep - Qualcomm PCIe Endpoint Controller
> * @pci: Designware PCIe controller struct
> @@ -167,6 +171,7 @@ enum qcom_pcie_ep_link_status {
> * @num_clks: PCIe clocks count
> * @perst_en: Flag for PERST enable
> * @perst_sep_en: Flag for PERST separation enable
> + * @cfg: PCIe EP config struct
> * @link_status: PCIe Link status
> * @global_irq: Qualcomm PCIe specific Global IRQ
> * @perst_irq: PERST# IRQ
> @@ -194,6 +199,7 @@ struct qcom_pcie_ep {
> u32 perst_en;
> u32 perst_sep_en;
>
> + const struct qcom_pcie_ep_cfg *cfg;
> enum qcom_pcie_ep_link_status link_status;
> int global_irq;
> int perst_irq;
> @@ -511,6 +517,10 @@ static void qcom_pcie_perst_assert(struct dw_pcie *pci)
> pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
> }
>
> +static const struct qcom_pcie_ep_cfg cfg_1_34_0 = {
> + .hdma_supported = true,
> +};
> +
> /* Common DWC controller ops */
> static const struct dw_pcie_ops pci_ops = {
> .link_up = qcom_pcie_dw_link_up,
> @@ -816,6 +826,13 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
> pcie_ep->pci.ops = &pci_ops;
> pcie_ep->pci.ep.ops = &pci_ep_ops;
> pcie_ep->pci.edma.nr_irqs = 1;
> +
> + pcie_ep->cfg = of_device_get_match_data(dev);

Why do you want to cache "cfg" since it is only used in probe()?

> + if (pcie_ep->cfg && pcie_ep->cfg->hdma_supported) {
> + pcie_ep->pci.edma.ll_wr_cnt = 1;
> + pcie_ep->pci.edma.ll_rd_cnt = 1;

Is the platform really has a single r/w channel?

- Mani

--
மணிவண்ணன் சதாசிவம்

2024-01-30 10:04:35

[permalink] [raw]

Subject: Re: [PATCH v1 0/6] Add Change to integrate HDMA with dwc ep driver

Hi Mrinmay

On Fri, Jan 19, 2024 at 06:30:16PM +0530, Mrinmay Sarkar wrote:
> 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. This patch series is to integrate HDMA with
> dwc ep driver.
>
> Add change to provide a valid base address of the CSRs from the
> platform driver and also provides read/write channels count from
> platform driver since there is no standard way to auto detect the
> number of available read/write channels in a platform and set the
> mapping format in platform driver for HDMA.
>
> This series passes 'struct dw_edma_chip' to irq_vector() as it needs
> to access that particular structure and fix to get the eDMA/HDMA
> max channel count. Also move the HDMA max channel definition to edma.h
> to maintain uniformity with eDMA.

Thanks for the patchset. I'll have a look at it later on this
week or early on the next one. If you wish you can resubmit it by then
with the Dmitry' and Mani' notes fixed.

-Serge(y)

>
> Dependency
> ----------
> Depends on:
> https://lore.kernel.org/dmaengine/[email protected]/
> https://lore.kernel.org/all/[email protected]/
>
> Manivannan Sadhasivam (4):
> dmaengine: dw-edma: Pass 'struct dw_edma_chip' to irq_vector()
> dmaengine: dw-edma: Introduce helpers for getting the eDMA/HDMA max
> channel count
> PCI: dwc: Add HDMA support
> dmaengine: dw-edma: Move HDMA_V0_MAX_NR_CH definition to edma.h
>
> Mrinmay Sarkar (2):
> PCI: qcom-ep: Provide number of read/write channel for HDMA
> PCI: epf-mhi: Add flag to enable HDMA for SA8775P
>
> drivers/dma/dw-edma/dw-edma-core.c | 29 ++++++++++---
> drivers/dma/dw-edma/dw-edma-pcie.c | 4 +-
> drivers/dma/dw-edma/dw-hdma-v0-core.c | 4 +-
> drivers/dma/dw-edma/dw-hdma-v0-regs.h | 3 +-
> drivers/pci/controller/dwc/pcie-designware.c | 63 ++++++++++++++++++++++------
> drivers/pci/controller/dwc/pcie-qcom-ep.c | 19 ++++++++-
> drivers/pci/endpoint/functions/pci-epf-mhi.c | 1 +
> include/linux/dma/edma.h | 18 +++++++-
> 8 files changed, 115 insertions(+), 26 deletions(-)
>
> --
> 2.7.4
>

2024-01-30 13:19:38

[permalink] [raw]

Subject: Re: [PATCH v1 5/6] PCI: qcom-ep: Provide number of read/write channel for HDMA

On 1/30/2024 2:23 PM, Manivannan Sadhasivam wrote:
> On Fri, Jan 19, 2024 at 06:30:21PM +0530, Mrinmay Sarkar wrote:
>> There is no standard way to auto detect the number of available
>> read/write channels in a platform. So adding this change to provide
>> read/write channels count and also provide "EDMA_MF_HDMA_NATIVE"
>> flag to support HDMA for 8775 platform.
>>
>> 8775 has IP version 1.34.0 so intruduce a new cfg(cfg_1_34_0) for
>> this platform. Add struct qcom_pcie_ep_cfg as match data. Assign
>> hdma_supported flag into struct qcom_pcie_ep_cfg and set it true
>> in cfg_1_34_0.
>>
>> Signed-off-by: Mrinmay Sarkar <[email protected]>
>> ---
>> drivers/pci/controller/dwc/pcie-qcom-ep.c | 19 ++++++++++++++++++-
>> 1 file changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> index 45008e0..8d56435 100644
>> --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c
>> @@ -149,6 +149,10 @@ enum qcom_pcie_ep_link_status {
>> QCOM_PCIE_EP_LINK_DOWN,
>> };
>>
> Add kdoc comment please as like the below struct.
>
>> +struct qcom_pcie_ep_cfg {
>> + bool hdma_supported;
>> +};
>> +
>> /**
>> * struct qcom_pcie_ep - Qualcomm PCIe Endpoint Controller
>> * @pci: Designware PCIe controller struct
>> @@ -167,6 +171,7 @@ enum qcom_pcie_ep_link_status {
>> * @num_clks: PCIe clocks count
>> * @perst_en: Flag for PERST enable
>> * @perst_sep_en: Flag for PERST separation enable
>> + * @cfg: PCIe EP config struct
>> * @link_status: PCIe Link status
>> * @global_irq: Qualcomm PCIe specific Global IRQ
>> * @perst_irq: PERST# IRQ
>> @@ -194,6 +199,7 @@ struct qcom_pcie_ep {
>> u32 perst_en;
>> u32 perst_sep_en;
>>
>> + const struct qcom_pcie_ep_cfg *cfg;
>> enum qcom_pcie_ep_link_status link_status;
>> int global_irq;
>> int perst_irq;
>> @@ -511,6 +517,10 @@ static void qcom_pcie_perst_assert(struct dw_pcie *pci)
>> pcie_ep->link_status = QCOM_PCIE_EP_LINK_DISABLED;
>> }
>>
>> +static const struct qcom_pcie_ep_cfg cfg_1_34_0 = {
>> + .hdma_supported = true,
>> +};
>> +
>> /* Common DWC controller ops */
>> static const struct dw_pcie_ops pci_ops = {
>> .link_up = qcom_pcie_dw_link_up,
>> @@ -816,6 +826,13 @@ static int qcom_pcie_ep_probe(struct platform_device *pdev)
>> pcie_ep->pci.ops = &pci_ops;
>> pcie_ep->pci.ep.ops = &pci_ep_ops;
>> pcie_ep->pci.edma.nr_irqs = 1;
>> +
>> + pcie_ep->cfg = of_device_get_match_data(dev);
> Why do you want to cache "cfg" since it is only used in probe()?

Yes Mani, no need to cache "cfg" we can use directly here .

>> + if (pcie_ep->cfg && pcie_ep->cfg->hdma_supported) {
>> + pcie_ep->pci.edma.ll_wr_cnt = 1;
>> + pcie_ep->pci.edma.ll_rd_cnt = 1;
> Is the platform really has a single r/w channel?
the platform has 8 r/w channels. but as per the use case we need to use
single r/w channel.
> - Mani
Thanks,
Mrinmay

2024-02-02 10:59:48

[permalink] [raw]

Subject: Re: [PATCH v1 4/6] dmaengine: dw-edma: Move HDMA_V0_MAX_NR_CH definition to edma.h

Hi Mrinmay

On Fri, Jan 19, 2024 at 06:30:20PM +0530, Mrinmay Sarkar wrote:
> From: Manivannan Sadhasivam <[email protected]>
>
> To maintain uniformity with eDMA, let's move the HDMA max channel
> definition to edma.h. While at it, let's also rename it to HDMA_MAX_NR_CH.

First of all include/linux/dma/edma.h already contains the common DW
EDMA and _HDMA_ settings/entities including the number of channels.
Both of these IP-cores have the same constraints on the max amount of
channels. Moreover the EDMA_MAX_WR_CH/EDMA_MAX_RD_CH macros are
already utilized in the common dw_edma_probe() method. So to speak you
can freely use these macros for HDMA too. Thus this change is
redundant. So is the patches 1/6 and 2/6.

Note currently all the common DW EDMA driver code uses the EDMA_/edma_
prefixes. HDMA_/hdma_ prefixes are utilized in the Native
HDMA-specific module only. So if you wished to provide some IP-core
specific settings utilized in the common part of the driver, then the
best approach would be to provide a change for the entire driver
interface (for instance first convert it to having a neutral prefixes,
like xdma_/etc, and then use the IP-core specific ones). So please
just use the EDMA_MAX_WR_CH and EDMA_MAX_RD_CH macros and drop the
patches 1, 2, and 4.

-Serge(y)

>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> Signed-off-by: Mrinmay Sarkar <[email protected]>
> ---
> drivers/dma/dw-edma/dw-hdma-v0-core.c | 4 ++--
> drivers/dma/dw-edma/dw-hdma-v0-regs.h | 3 +--
> include/linux/dma/edma.h | 1 +
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> index 1f4cb7d..819ef1f 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> @@ -54,7 +54,7 @@ static void dw_hdma_v0_core_off(struct dw_edma *dw)
> {
> int id;
>
> - for (id = 0; id < HDMA_V0_MAX_NR_CH; id++) {
> + for (id = 0; id < HDMA_MAX_NR_CH; id++) {
> SET_BOTH_CH_32(dw, id, int_setup,
> HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK);
> SET_BOTH_CH_32(dw, id, int_clear,
> @@ -70,7 +70,7 @@ static u16 dw_hdma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir)
> * available, we set it to maximum channels and let the platform
> * set the right number of channels.
> */
> - return HDMA_V0_MAX_NR_CH;
> + return HDMA_MAX_NR_CH;
> }
>
> static enum dma_status dw_hdma_v0_core_ch_status(struct dw_edma_chan *chan)
> diff --git a/drivers/dma/dw-edma/dw-hdma-v0-regs.h b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> index a974abd..cd7eab2 100644
> --- a/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> +++ b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> @@ -11,7 +11,6 @@
>
> #include <linux/dmaengine.h>
>
> -#define HDMA_V0_MAX_NR_CH 8
> #define HDMA_V0_LOCAL_ABORT_INT_EN BIT(6)
> #define HDMA_V0_REMOTE_ABORT_INT_EN BIT(5)
> #define HDMA_V0_LOCAL_STOP_INT_EN BIT(4)
> @@ -92,7 +91,7 @@ struct dw_hdma_v0_ch {
> } __packed;
>
> struct dw_hdma_v0_regs {
> - struct dw_hdma_v0_ch ch[HDMA_V0_MAX_NR_CH]; /* 0x0000..0x0fa8 */
> + struct dw_hdma_v0_ch ch[HDMA_MAX_NR_CH]; /* 0x0000..0x0fa8 */
> } __packed;
>
> struct dw_hdma_v0_lli {
> diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> index 550f6a4..2cdf249a 100644
> --- a/include/linux/dma/edma.h
> +++ b/include/linux/dma/edma.h
> @@ -14,6 +14,7 @@
>
> #define EDMA_MAX_WR_CH 8
> #define EDMA_MAX_RD_CH 8
> +#define HDMA_MAX_NR_CH 8
>
> struct dw_edma;
> struct dw_edma_chip;
> --
> 2.7.4
>

2024-02-02 12:39:06

[permalink] [raw]

Subject: Re: [PATCH v1 4/6] dmaengine: dw-edma: Move HDMA_V0_MAX_NR_CH definition to edma.h

On Fri, Feb 02, 2024 at 01:47:37PM +0300, Serge Semin wrote:
> Hi Mrinmay
>
> On Fri, Jan 19, 2024 at 06:30:20PM +0530, Mrinmay Sarkar wrote:
> > From: Manivannan Sadhasivam <[email protected]>
> >
> > To maintain uniformity with eDMA, let's move the HDMA max channel
> > definition to edma.h. While at it, let's also rename it to HDMA_MAX_NR_CH.
>
> First of all include/linux/dma/edma.h already contains the common DW
> EDMA and _HDMA_ settings/entities including the number of channels.
> Both of these IP-cores have the same constraints on the max amount of
> channels. Moreover the EDMA_MAX_WR_CH/EDMA_MAX_RD_CH macros are
> already utilized in the common dw_edma_probe() method. So to speak you
> can freely use these macros for HDMA too. Thus this change is
> redundant. So is the patches 1/6 and 2/6.
>
> Note currently all the common DW EDMA driver code uses the EDMA_/edma_
> prefixes. HDMA_/hdma_ prefixes are utilized in the Native
> HDMA-specific module only. So if you wished to provide some IP-core
> specific settings utilized in the common part of the driver, then the
> best approach would be to provide a change for the entire driver
> interface (for instance first convert it to having a neutral prefixes,
> like xdma_/etc, and then use the IP-core specific ones). So please
> just use the EDMA_MAX_WR_CH and EDMA_MAX_RD_CH macros and drop the
> patches 1, 2, and 4.
>

Hmm. With my limited access to the HDMA IP insights, I was not aware that the
constraints are similar with EDMA. If so, then it makes sense to reuse the same
macros. But I would also add a comment on top of the macros to avoid confusion.

- Mani

> -Serge(y)
>
> >
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > Signed-off-by: Mrinmay Sarkar <[email protected]>
> > ---
> > drivers/dma/dw-edma/dw-hdma-v0-core.c | 4 ++--
> > drivers/dma/dw-edma/dw-hdma-v0-regs.h | 3 +--
> > include/linux/dma/edma.h | 1 +
> > 3 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-hdma-v0-core.c b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > index 1f4cb7d..819ef1f 100644
> > --- a/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > +++ b/drivers/dma/dw-edma/dw-hdma-v0-core.c
> > @@ -54,7 +54,7 @@ static void dw_hdma_v0_core_off(struct dw_edma *dw)
> > {
> > int id;
> >
> > - for (id = 0; id < HDMA_V0_MAX_NR_CH; id++) {
> > + for (id = 0; id < HDMA_MAX_NR_CH; id++) {
> > SET_BOTH_CH_32(dw, id, int_setup,
> > HDMA_V0_STOP_INT_MASK | HDMA_V0_ABORT_INT_MASK);
> > SET_BOTH_CH_32(dw, id, int_clear,
> > @@ -70,7 +70,7 @@ static u16 dw_hdma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir)
> > * available, we set it to maximum channels and let the platform
> > * set the right number of channels.
> > */
> > - return HDMA_V0_MAX_NR_CH;
> > + return HDMA_MAX_NR_CH;
> > }
> >
> > static enum dma_status dw_hdma_v0_core_ch_status(struct dw_edma_chan *chan)
> > diff --git a/drivers/dma/dw-edma/dw-hdma-v0-regs.h b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > index a974abd..cd7eab2 100644
> > --- a/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > +++ b/drivers/dma/dw-edma/dw-hdma-v0-regs.h
> > @@ -11,7 +11,6 @@
> >
> > #include <linux/dmaengine.h>
> >
> > -#define HDMA_V0_MAX_NR_CH 8
> > #define HDMA_V0_LOCAL_ABORT_INT_EN BIT(6)
> > #define HDMA_V0_REMOTE_ABORT_INT_EN BIT(5)
> > #define HDMA_V0_LOCAL_STOP_INT_EN BIT(4)
> > @@ -92,7 +91,7 @@ struct dw_hdma_v0_ch {
> > } __packed;
> >
> > struct dw_hdma_v0_regs {
> > - struct dw_hdma_v0_ch ch[HDMA_V0_MAX_NR_CH]; /* 0x0000..0x0fa8 */
> > + struct dw_hdma_v0_ch ch[HDMA_MAX_NR_CH]; /* 0x0000..0x0fa8 */
> > } __packed;
> >
> > struct dw_hdma_v0_lli {
> > diff --git a/include/linux/dma/edma.h b/include/linux/dma/edma.h
> > index 550f6a4..2cdf249a 100644
> > --- a/include/linux/dma/edma.h
> > +++ b/include/linux/dma/edma.h
> > @@ -14,6 +14,7 @@
> >
> > #define EDMA_MAX_WR_CH 8
> > #define EDMA_MAX_RD_CH 8
> > +#define HDMA_MAX_NR_CH 8
> >
> > struct dw_edma;
> > struct dw_edma_chip;
> > --
> > 2.7.4
> >

--
மணிவண்ணன் சதாசிவம்

2024-02-02 21:57:10

[permalink] [raw]

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

On Fri, Jan 19, 2024 at 06:30:19PM +0530, Mrinmay Sarkar wrote:
> From: Manivannan Sadhasivam <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Mrinmay Sarkar <[email protected]>
> ---
> 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
>

Attachments:

(No filename) (5.61 kB)
0001-PCI-dwc-Fix-eDMA-mapping-info-string.patch (1.42 kB)
0002-PCI-dwc-Split-up-eDMA-parameters-auto-detection-proc.patch (3.08 kB)
0003-PCI-dwc-Convert-eDMA-mapping-detection-to-being-full.patch (2.31 kB)
0004-PCI-dwc-Convert-eDMA-channels-detection-to-being-opt.patch (1.99 kB)
0005-PCI-dwc-Drop-DW_PCIE_CAP_EDMA_UNROLL-flag.patch (3.75 kB)
Download all attachments

2024-02-07 17:07:05

[permalink] [raw]

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

On Sat, Feb 03, 2024 at 12:40:39AM +0300, Serge Semin wrote:
> On Fri, Jan 19, 2024 at 06:30:19PM +0530, Mrinmay Sarkar wrote:
> > From: Manivannan Sadhasivam <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Mrinmay Sarkar <[email protected]>
> > ---
> > 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:
>

Above approach sounds good to me, thanks!

Mrinmay, please integrate Sergey's patches and drop this one. I'll do a review
in v2.

- Mani

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

> From 9b600c17aa56b3949a040055cf82222c48b60bf3 Mon Sep 17 00:00:00 2001
> From: Serge Semin <[email protected]>
> Date: Fri, 2 Feb 2024 18:01:31 +0300
> Subject: [PATCH 1/5] PCI: dwc: Fix eDMA mapping info string
>
> DW PCIe controller can be equipped with the next types of DMA controllers:
> 1. eDMA controller with viewport-based CSRs access (so called legacy),
> 2. eDMA controller with unrolled CSRs mapping,
> 3. HDMA controller compatible with the eDMA unrolled CSRs mapping,
> 4. HDMA controller with native CSRs mapping.
> The later three types imply having the DMA-engine CSRs _unrolled_ mapping.
> Let's fix the info-message printed in the DW PCIe eDMA controller
> detection procedure to comply with that.
>
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 7551e0fea5e9..454ea32ee70b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -1018,7 +1018,7 @@ int dw_pcie_edma_detect(struct dw_pcie *pci)
> }
>
> dev_info(pci->dev, "eDMA: unroll %s, %hu wr, %hu rd\n",
> - pci->edma.mf == EDMA_MF_EDMA_UNROLL ? "T" : "F",
> + pci->edma.mf != EDMA_MF_EDMA_LEGACY ? "T" : "F",
> pci->edma.ll_wr_cnt, pci->edma.ll_rd_cnt);
>
> return 0;
> --
> 2.43.0
>

> From ca70e7dd9b84c9dd01124a13f624441c01f7c09d Mon Sep 17 00:00:00 2001
> From: Serge Semin <[email protected]>
> Date: Fri, 2 Feb 2024 18:18:39 +0300
> Subject: [PATCH 2/5] PCI: dwc: Split up eDMA parameters auto-detection
> procedure
>
> It turns out the DW HDMA controller parameters can't be auto-detected in
> the same way as it's done for DW eDMA: HDMA has only the unrolled CSRs
> mapping and has no way to find out the amount of the channels. For that
> case the best choice would be to have the HDMA controller parameters
> pre-defined by the platform drivers and to convert the implemented
> auto-detection procedure to being optionally executed if no DMA-controller
> parameters specified. As a preparation step before that let's split the
> eDMA auto-detection into three stages:
> 1. initialize DW eDMA data,
> 2. auto-detect the CSRs mapping format,
> 3. auto-detect the amount of channels.
>
> Note this commit doesn't imply the eDMA detection procedure semantics
> change.
>
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 33 +++++++++++++++-----
> 1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 454ea32ee70b..149c7a2a12f2 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -878,7 +878,17 @@ 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 void dw_pcie_edma_init_data(struct dw_pcie *pci)
> +{
> + pci->edma.dev = pci->dev;
> +
> + if (!pci->edma.ops)
> + pci->edma.ops = &dw_pcie_edma_ops;
> +
> + pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> +}
> +
> +static int dw_pcie_edma_find_mf(struct dw_pcie *pci)
> {
> u32 val;
>
> @@ -900,8 +910,6 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
>
> 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;
>
> @@ -910,12 +918,14 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> return -ENODEV;
> }
>
> - pci->edma.dev = pci->dev;
> + return 0;
> +}
>
> - if (!pci->edma.ops)
> - pci->edma.ops = &dw_pcie_edma_ops;
> +static int dw_pcie_edma_find_chan(struct dw_pcie *pci)
> +{
> + u32 val;
>
> - pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
>
> 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);
> @@ -992,8 +1002,15 @@ int dw_pcie_edma_detect(struct dw_pcie *pci)
> {
> int ret;
>
> + dw_pcie_edma_init_data(pci);
> +
> /* Don't fail if no eDMA was found (for the backward compatibility) */
> - ret = dw_pcie_edma_find_chip(pci);
> + ret = dw_pcie_edma_find_mf(pci);
> + if (ret)
> + return 0;
> +
> + /* Don't fail if no valid channels detected (for the backward compatibility) */
> + ret = dw_pcie_edma_find_chan(pci);
> if (ret)
> return 0;
>
> --
> 2.43.0
>

> From f5149d0827d9d8b0ccaaaeb6309b1a86832cdddc Mon Sep 17 00:00:00 2001
> From: Serge Semin <[email protected]>
> Date: Fri, 2 Feb 2024 19:02:35 +0300
> Subject: [PATCH 3/5] PCI: dwc: Convert eDMA mapping detection to being fully
> optional
>
> The DW eDMA CSRs mapping detection procedure doesn't work for the DW HDMA
> controller. Moreover it isn't that easy to distinguish HDMA from eDMA if
> the former controller available in place of the later one. Thus seeing DW
> HDMA controller has the unrolled CSRs mapping only there is no better
> choice but to rely on the HDMA-capable platform drivers having the
> DMA-engine mapping format specified. In order to permit that let's convert
> the eDMA mapping format auto-detection to being fully optional: execute
> the DMA Ctrl-based CSRs mapping auto-detection only if no mapping format
> was specific.
>
> Note the DW_PCIE_CAP_EDMA_UNROLL flag semantics also imply the mapping
> auto-detection optionality. But it doesn't indicate the type of the
> controller. It's merely a fixup for the DW PCIe eDMA controllers which for
> some reason don't support the DMA Ctrl-based CSRs mapping auto-detection
> procedure (see note regarding the Renesas R-Car S4-8's PCIe). So it can't
> be utilized for DW HDMA auto-detection. But after this change is applied
> the flag will get to be redundant and will be subject for removal in one
> of the subsequent commit.
>
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 149c7a2a12f2..2243ffeb95b5 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -892,6 +892,14 @@ static int dw_pcie_edma_find_mf(struct dw_pcie *pci)
> {
> u32 val;
>
> + /*
> + * The platform drivers can pre-define the DMA controller mapping
> + * format especially if the auto-detection procedure doesn't work for
> + * them. In that case the CSRs base must be specified too.
> + */
> + if (pci->edma.mf != EDMA_MF_EDMA_LEGACY)
> + return pci->edma.reg_base ? 0 : -EINVAL;
> +
> /*
> * Indirect eDMA CSRs access has been completely removed since v5.40a
> * thus no space is now reserved for the eDMA channels viewport and
> --
> 2.43.0
>

> From 4f9668ad9e741b501476cd4457cf9ca9013ee6e3 Mon Sep 17 00:00:00 2001
> From: Serge Semin <[email protected]>
> Date: Fri, 2 Feb 2024 20:12:46 +0300
> Subject: [PATCH 4/5] PCI: dwc: Convert eDMA channels detection to being
> optional
>
> DW HDMA controller channels can't be auto-detected. Thus there is no way
> but to rely on the HDMA-capable platform drivers having the number of
> channels specified. In order to permit that convert the
> dw_pcie_edma_find_chan() method to executing the DMA Ctrl CSR-based number
> of channels detection procedure only if no channels amount was specified,
> otherwise just sanity check the specified values.
>
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 16 ++++++++++++----
> 1 file changed, 12 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 2243ffeb95b5..4d53a71ab1b4 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -933,12 +933,20 @@ static int dw_pcie_edma_find_chan(struct dw_pcie *pci)
> {
> u32 val;
>
> - val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> + if (!pci->edma.ll_wr_cnt && !pci->edma.ll_rd_cnt) {
> + if (pci->edma.mf == EDMA_MF_HDMA_NATIVE)
> + return -EINVAL;
> +
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
>
> - 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);
> + 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 */
> + /*
> + * Sanity check the channels count in case if the mapping was
> + * incorrectly detected/specified by the glue-driver.
> + */
> 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;
> --
> 2.43.0
>

> From 9590af6f5114b07e4073083ecde9e563cc920410 Mon Sep 17 00:00:00 2001
> From: Serge Semin <[email protected]>
> Date: Fri, 2 Feb 2024 23:45:00 +0300
> Subject: [PATCH 5/5] PCI: dwc: Drop DW_PCIE_CAP_EDMA_UNROLL flag
>
> That flag was introduced in order to bypass the DW eDMA mapping format
> auto-detection procedure and force the unrolled eDMA CSRs mapping
> procedure. Since the same can be now reached just by pre-defining the
> required mapping format, drop the flag and convert the Renesas R-Car S4-8
> glue-driver to using the new approach.
>
> Signed-off-by: Serge Semin <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 13 +++++--------
> drivers/pci/controller/dwc/pcie-designware.h | 5 ++---
> drivers/pci/controller/dwc/pcie-rcar-gen4.c | 2 +-
> 3 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 4d53a71ab1b4..a49de18c9836 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -895,7 +895,10 @@ static int dw_pcie_edma_find_mf(struct dw_pcie *pci)
> /*
> * The platform drivers can pre-define the DMA controller mapping
> * format especially if the auto-detection procedure doesn't work for
> - * them. In that case the CSRs base must be specified too.
> + * them (e.g. Renesas R-Car S4-8's PCIe controllers for unknown reason
> + * have zeros in the eDMA CTRL register even though the HW-manual
> + * explicitly states there must be FFs if the unrolled mapping is
> + * enabled). In that case the CSRs base must be specified too.
> */
> if (pci->edma.mf != EDMA_MF_EDMA_LEGACY)
> return pci->edma.reg_base ? 0 : -EINVAL;
> @@ -904,14 +907,8 @@ static int dw_pcie_edma_find_mf(struct dw_pcie *pci)
> * Indirect eDMA CSRs access has been completely removed since v5.40a
> * thus no space is now reserved for the eDMA channels viewport and
> * former DMA CTRL register is no longer fixed to FFs.
> - *
> - * Note that Renesas R-Car S4-8's PCIe controllers for unknown reason
> - * have zeros in the eDMA CTRL register even though the HW-manual
> - * explicitly states there must FFs if the unrolled mapping is enabled.
> - * For such cases the low-level drivers are supposed to manually
> - * activate the unrolled mapping to bypass the auto-detection procedure.
> */
> - if (dw_pcie_ver_is_ge(pci, 540A) || dw_pcie_cap_is(pci, EDMA_UNROLL))
> + if (dw_pcie_ver_is_ge(pci, 540A))
> val = 0xFFFFFFFF;
> else
> val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 035df6bc7606..a666190e8b1b 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -54,9 +54,8 @@
>
> /* DWC PCIe controller capabilities */
> #define DW_PCIE_CAP_REQ_RES 0
> -#define DW_PCIE_CAP_EDMA_UNROLL 1
> -#define DW_PCIE_CAP_IATU_UNROLL 2
> -#define DW_PCIE_CAP_CDM_CHECK 3
> +#define DW_PCIE_CAP_IATU_UNROLL 1
> +#define DW_PCIE_CAP_CDM_CHECK 2
>
> #define dw_pcie_cap_is(_pci, _cap) \
> test_bit(DW_PCIE_CAP_ ## _cap, &(_pci)->caps)
> diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> index 3bc45e513b3d..5678d69c413a 100644
> --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> @@ -255,7 +255,7 @@ static struct rcar_gen4_pcie *rcar_gen4_pcie_alloc(struct platform_device *pdev)
> rcar->dw.ops = &dw_pcie_ops;
> rcar->dw.dev = dev;
> rcar->pdev = pdev;
> - dw_pcie_cap_set(&rcar->dw, EDMA_UNROLL);
> + rcar->dw.edma.mf = EDMA_MF_EDMA_UNROLL;
> dw_pcie_cap_set(&rcar->dw, REQ_RES);
> platform_set_drvdata(pdev, rcar);
>
> --
> 2.43.0
>

--
மணிவண்ணன் சதாசிவம்

2024-02-09 17:10:51

by Bjorn Helgaas

[permalink] [raw]

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

2024-02-11 19:38:04

[permalink] [raw]

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

On Fri, Feb 09, 2024 at 11:10:32AM -0600, Bjorn Helgaas wrote:
> On Sat, Feb 03, 2024 at 12:40:39AM +0300, Serge Semin wrote:
> > On Fri, Jan 19, 2024 at 06:30:19PM +0530, Mrinmay Sarkar wrote:
> > > From: Manivannan Sadhasivam <[email protected]>
> > >
> > > 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.
> > ...
>
> > 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:
>
> I am still bound by the opinion of Google's legal team that I cannot
> accept the code changes that were attached here. I think it's fair to
> read the review comments (thank you for those), but I suggest not
> reading the patches that were attached.

Em, the review comment and the resultant patches were my own private
researches and developments. Is Google now blocking even individual
contributors?

In anyway if you are agree with the changes suggested above you can
set to the patches any author you think would be acceptable. My only
concern was to maintain the cleanness of the driver code developed by
me and which is going to be affected in the framework of this series.

-Serge(y)

>
> Bjorn

2024-02-13 07:54:39