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 <manivannan.sadhasivam@xxxxxxxxxx>
> >
> > Hyper DMA (HDMA) is already supported by the dw-edma dmaengine driver.
> > Unlike it's predecessor Embedded DMA (eDMA), HDMA supports only the
> > unrolled mapping format. So the platform drivers need to provide a valid
> > base address of the CSRs. Also, there is no standard way to auto detect
> > the number of available read/write channels in a platform. So the platform
> > drivers has to provide that information as well.
> >
> > For adding HDMA support, the mapping format set by the platform drivers is
> > used to detect whether eDMA or HDMA is being used, since we cannot auto
> > detect it in a sane way.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > Signed-off-by: Mrinmay Sarkar <quic_msarkar@xxxxxxxxxxx>
> > ---
> > drivers/pci/controller/dwc/pcie-designware.c | 55 ++++++++++++++++++++++++----
> > 1 file changed, 47 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 96575b8..07a1f2d 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -880,7 +880,29 @@ static struct dw_edma_plat_ops dw_pcie_edma_ops = {
> > .irq_vector = dw_pcie_edma_irq_vector,
> > };
> >
> > -static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> > +static int dw_pcie_find_hdma(struct dw_pcie *pci)
> > +{
> > + /*
> > + * Since HDMA supports only unrolled mapping, platform drivers need to
> > + * provide a valid base address.
> > + */
> > + if (!pci->edma.reg_base)
> > + return -ENODEV;
> > +
> > + /*
> > + * Since there is no standard way to detect the number of read/write
> > + * HDMA channels, platform drivers are expected to provide the channel
> > + * count. Let's also do a sanity check of them to make sure that the
> > + * counts are within the limit specified by the spec.
> > + */
> > + if (!pci->edma.ll_wr_cnt || pci->edma.ll_wr_cnt > dw_edma_get_max_wr_ch(pci->edma.mf) ||
> > + !pci->edma.ll_rd_cnt || pci->edma.ll_rd_cnt > dw_edma_get_max_rd_ch(pci->edma.mf))
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int dw_pcie_find_edma(struct dw_pcie *pci)
> > {
> > u32 val;
> >
> > @@ -912,13 +934,6 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> > return -ENODEV;
> > }
> >
> > - pci->edma.dev = pci->dev;
> > -
> > - if (!pci->edma.ops)
> > - pci->edma.ops = &dw_pcie_edma_ops;
> > -
> > - pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> > -
> > pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> >
> > @@ -930,6 +945,30 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> > return 0;
> > }
> >
> > +static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> > +{
> > + int ret;
> > +
>
> > + if (pci->edma.mf == EDMA_MF_HDMA_NATIVE) {
> > + ret = dw_pcie_find_hdma(pci);
> > + if (ret)
> > + return ret;
> > + } else {
> > + ret = dw_pcie_find_edma(pci);
> > + if (ret)
> > + return ret;
> > + }
>
> Basically this change defines two versions of the eDMA info
> initialization procedure:
> 1. use pre-defined CSRs mapping format and amount of channels,
> 2. auto-detect CSRs mapping and the amount of channels.
> The second version also supports the optional CSRs mapping format
> detection procedure by means of the DW_PCIE_CAP_EDMA_UNROLL flag
> semantics. Thus should this patch is accepted there will be the
> functionality duplication. I suggest to make things a bit more
> flexible than that. Instead of creating the two types of the
> init-methods selectable based on the mapping format, let's split up
> the already available DW eDMA engine detection procedure into the next
> three stages:
> 1. initialize DW eDMA data,
> 2. auto-detect the CSRs mapping format,
> 3. auto-detect the amount of channels.
> and convert the later two to being optional. They will be skipped in case
> if the mapping format or the amount of channels have been pre-defined
> by the platform drivers. Thus we can keep the eDMA data init procedure
> more linear thus easier to read, drop redundant DW_PCIE_CAP_EDMA_UNROLL flag
> and use the new functionality for the Renesas R-Car S4-8's PCIe
> controller (for which the auto-detection didn't work), for HDMA with compat
> and _native_ CSRs mapping. See the attached patches for details:
>
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 <fancer.lancer@xxxxxxxxx>
> 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 <fancer.lancer@xxxxxxxxx>
> ---
> 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 <fancer.lancer@xxxxxxxxx>
> 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 <fancer.lancer@xxxxxxxxx>
> ---
> 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 <fancer.lancer@xxxxxxxxx>
> 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 <fancer.lancer@xxxxxxxxx>
> ---
> 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 <fancer.lancer@xxxxxxxxx>
> 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 <fancer.lancer@xxxxxxxxx>
> ---
> 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 <fancer.lancer@xxxxxxxxx>
> 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 <fancer.lancer@xxxxxxxxx>
> ---
> 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
>
--
மணிவண்ணன் சதாசிவம்
