In order to add support for Hyper DMA (HDMA), let's refactor the existing
dw_pcie_edma_find_chip() API by moving the common code to separate
functions.
No functional change.
Suggested-by: Serge Semin <[email protected]>
Signed-off-by: Manivannan Sadhasivam <[email protected]>
---
drivers/pci/controller/dwc/pcie-designware.c | 52 +++++++++++++++++++++-------
1 file changed, 39 insertions(+), 13 deletions(-)
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 250cf7f40b85..193fcd86cf93 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -880,7 +880,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,24 +910,27 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
else
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;
+ /* Set default mapping format here and update it below if needed */
+ pci->edma.mf = EDMA_MF_EDMA_LEGACY;
+ if (val == 0xFFFFFFFF && pci->edma.reg_base)
+ pci->edma.mf = EDMA_MF_EDMA_UNROLL;
+ else if (val != 0xFFFFFFFF)
pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
- } else {
+ else
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_channels(struct dw_pcie *pci)
+{
+ u32 val;
- pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
+ if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
+ val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
+ else
+ 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);
@@ -930,6 +943,19 @@ 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;
+
+ dw_pcie_edma_init_data(pci);
+
+ ret = dw_pcie_edma_find_mf(pci);
+ if (ret)
+ return ret;
+
+ return dw_pcie_edma_find_channels(pci);
+}
+
static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
{
struct platform_device *pdev = to_platform_device(pci->dev);
--
2.25.1
On Mon, Feb 26, 2024 at 05:07:26PM +0530, Manivannan Sadhasivam wrote:
> In order to add support for Hyper DMA (HDMA), let's refactor the existing
> dw_pcie_edma_find_chip() API by moving the common code to separate
> functions.
>
> No functional change.
>
> Suggested-by: Serge Semin <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Siddharth Vadapalli <[email protected]>
Regards,
Siddharth.
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 52 +++++++++++++++++++++-------
> 1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 250cf7f40b85..193fcd86cf93 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -880,7 +880,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,24 +910,27 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> else
> 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;
> + /* Set default mapping format here and update it below if needed */
> + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
>
> + if (val == 0xFFFFFFFF && pci->edma.reg_base)
> + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> + else if (val != 0xFFFFFFFF)
> pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> - } else {
> + else
> 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_channels(struct dw_pcie *pci)
> +{
> + u32 val;
>
> - pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> + else
> + 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);
> @@ -930,6 +943,19 @@ 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;
> +
> + dw_pcie_edma_init_data(pci);
> +
> + ret = dw_pcie_edma_find_mf(pci);
> + if (ret)
> + return ret;
> +
> + return dw_pcie_edma_find_channels(pci);
> +}
> +
> static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
> {
> struct platform_device *pdev = to_platform_device(pci->dev);
>
> --
> 2.25.1
>
>
Hi Manivannan
On Mon, Feb 26, 2024 at 05:07:26PM +0530, Manivannan Sadhasivam wrote:
> In order to add support for Hyper DMA (HDMA), let's refactor the existing
> dw_pcie_edma_find_chip() API by moving the common code to separate
> functions.
>
> No functional change.
>
> Suggested-by: Serge Semin <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 52 +++++++++++++++++++++-------
> 1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 250cf7f40b85..193fcd86cf93 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -880,7 +880,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,24 +910,27 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> else
> 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;
> + /* Set default mapping format here and update it below if needed */
> + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
>
> + if (val == 0xFFFFFFFF && pci->edma.reg_base)
> + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> + else if (val != 0xFFFFFFFF)
> pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> - } else {
> + else
> return -ENODEV;
> - }
Sorry for not posting my opinion about this earlier, but IMO v2 code
was more correct than this one. This version makes the code being not
linear as it was in v2, thus harder to comprehend:
1. Setting up a default value and then overriding it or not makes the
reader to keep in mind the initialized value which is harder than to
just read what is done in the respective branch.
2. Splitting up the case clause with respective inits and the mapping
format setting up also makes it harder to comprehend what's going on.
In the legacy case the reg-base address and the mapping format init are
split up while they should have been done simultaneously only if (val
!= 0xFFFFFFFF).
3. The most of the current devices has the unrolled mapping (available
since v4.9 IP-core), thus having the mf field pre-initialized produces
a redundant store operation for the most of the modern devices.
4. Getting rid from the curly braces isn't something what should be
avoided at any cost and doesn't give any optimization really. It
doesn't cause having less C-lines of the source code and doesn't
improve the code readability.
So to speak, I'd suggest to get back the v2 implementation here.
>
> - pci->edma.dev = pci->dev;
> + return 0;
> +}
>
> - if (!pci->edma.ops)
> - pci->edma.ops = &dw_pcie_edma_ops;
> +static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> +{
> + u32 val;
>
> - pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> + else
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
Just dw_pcie_readl_dma(pci, PCIE_DMA_CTRL)
-Serge(y)
>
> 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 +943,19 @@ 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;
> +
> + dw_pcie_edma_init_data(pci);
> +
> + ret = dw_pcie_edma_find_mf(pci);
> + if (ret)
> + return ret;
> +
> + return dw_pcie_edma_find_channels(pci);
> +}
> +
> static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
> {
> struct platform_device *pdev = to_platform_device(pci->dev);
>
> --
> 2.25.1
>
On Mon, Feb 26, 2024 at 03:45:16PM +0300, Serge Semin wrote:
> Hi Manivannan
>
> On Mon, Feb 26, 2024 at 05:07:26PM +0530, Manivannan Sadhasivam wrote:
> > In order to add support for Hyper DMA (HDMA), let's refactor the existing
> > dw_pcie_edma_find_chip() API by moving the common code to separate
> > functions.
> >
> > No functional change.
> >
> > Suggested-by: Serge Semin <[email protected]>
> > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > ---
> > drivers/pci/controller/dwc/pcie-designware.c | 52 +++++++++++++++++++++-------
> > 1 file changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 250cf7f40b85..193fcd86cf93 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -880,7 +880,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,24 +910,27 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> > else
> > 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;
> > + /* Set default mapping format here and update it below if needed */
> > + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
> >
> > + if (val == 0xFFFFFFFF && pci->edma.reg_base)
> > + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > + else if (val != 0xFFFFFFFF)
> > pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> > - } else {
> > + else
> > return -ENODEV;
> > - }
>
> Sorry for not posting my opinion about this earlier, but IMO v2 code
> was more correct than this one. This version makes the code being not
> linear as it was in v2, thus harder to comprehend:
>
> 1. Setting up a default value and then overriding it or not makes the
> reader to keep in mind the initialized value which is harder than to
> just read what is done in the respective branch.
>
No, I disagree. Whether we set the default value or not, EDMA_MF_EDMA_LEGACY is
indeed the default mapping format (this is one of the reasons why the enums
should start from 1 instead of 0). So initializing it to legacy is not changing
anything, rather making it explicit.
> 2. Splitting up the case clause with respective inits and the mapping
> format setting up also makes it harder to comprehend what's going on.
> In the legacy case the reg-base address and the mapping format init are
> split up while they should have been done simultaneously only if (val
> != 0xFFFFFFFF).
>
Well again, this doesn't matter since the default mapping format is legacy. But
somewhat agree that the two clauses are setting different fields, but even if
the legacy mapping format is set inside the second clause, it still differs from
the first one since we are not setting reg_base.
> 3. The most of the current devices has the unrolled mapping (available
> since v4.9 IP-core), thus having the mf field pre-initialized produces
> a redundant store operation for the most of the modern devices.
>
Ok, this one I agree. We could avoid the extra assignment.
> 4. Getting rid from the curly braces isn't something what should be
> avoided at any cost and doesn't give any optimization really. It
> doesn't cause having less C-lines of the source code and doesn't
> improve the code readability.
>
Yeah, there is no benefit other than a simple view of the code. But for point
(3), I agree to roll back to v2 version.
> So to speak, I'd suggest to get back the v2 implementation here.
>
> >
> > - pci->edma.dev = pci->dev;
> > + return 0;
> > +}
> >
> > - if (!pci->edma.ops)
> > - pci->edma.ops = &dw_pcie_edma_ops;
> > +static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > +{
> > + u32 val;
> >
> > - pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
>
> > + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > + else
> > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
>
> Just dw_pcie_readl_dma(pci, PCIE_DMA_CTRL)
>
'val' is uninitialized. Why should the assignment be skipped?
- Mani
> -Serge(y)
>
> >
> > 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 +943,19 @@ 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;
> > +
> > + dw_pcie_edma_init_data(pci);
> > +
> > + ret = dw_pcie_edma_find_mf(pci);
> > + if (ret)
> > + return ret;
> > +
> > + return dw_pcie_edma_find_channels(pci);
> > +}
> > +
> > static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
> > {
> > struct platform_device *pdev = to_platform_device(pci->dev);
> >
> > --
> > 2.25.1
> >
--
மணிவண்ணன் சதாசிவம்
On Mon, Feb 26, 2024 at 05:07:26PM +0530, Manivannan Sadhasivam wrote:
> In order to add support for Hyper DMA (HDMA), let's refactor the existing
> dw_pcie_edma_find_chip() API by moving the common code to separate
> functions.
>
> No functional change.
>
> Suggested-by: Serge Semin <[email protected]>
> Signed-off-by: Manivannan Sadhasivam <[email protected]>
Reviewed-by: Frank Li <[email protected]>
> ---
> drivers/pci/controller/dwc/pcie-designware.c | 52 +++++++++++++++++++++-------
> 1 file changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 250cf7f40b85..193fcd86cf93 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -880,7 +880,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,24 +910,27 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> else
> 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;
> + /* Set default mapping format here and update it below if needed */
> + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
>
> + if (val == 0xFFFFFFFF && pci->edma.reg_base)
> + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> + else if (val != 0xFFFFFFFF)
> pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> - } else {
> + else
> 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_channels(struct dw_pcie *pci)
> +{
> + u32 val;
>
> - pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> + else
> + 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);
> @@ -930,6 +943,19 @@ 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;
> +
> + dw_pcie_edma_init_data(pci);
> +
> + ret = dw_pcie_edma_find_mf(pci);
> + if (ret)
> + return ret;
> +
> + return dw_pcie_edma_find_channels(pci);
> +}
> +
> static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
> {
> struct platform_device *pdev = to_platform_device(pci->dev);
>
> --
> 2.25.1
>
On Mon, Feb 26, 2024 at 08:57:57PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Feb 26, 2024 at 03:45:16PM +0300, Serge Semin wrote:
> > Hi Manivannan
> >
> > On Mon, Feb 26, 2024 at 05:07:26PM +0530, Manivannan Sadhasivam wrote:
> > > In order to add support for Hyper DMA (HDMA), let's refactor the existing
> > > dw_pcie_edma_find_chip() API by moving the common code to separate
> > > functions.
> > >
> > > No functional change.
> > >
> > > Suggested-by: Serge Semin <[email protected]>
> > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > ---
> > > drivers/pci/controller/dwc/pcie-designware.c | 52 +++++++++++++++++++++-------
> > > 1 file changed, 39 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > index 250cf7f40b85..193fcd86cf93 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > @@ -880,7 +880,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,24 +910,27 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> > > else
> > > 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;
> > > + /* Set default mapping format here and update it below if needed */
> > > + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
> > >
> > > + if (val == 0xFFFFFFFF && pci->edma.reg_base)
> > > + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > > + else if (val != 0xFFFFFFFF)
> > > pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> > > - } else {
> > > + else
> > > return -ENODEV;
> > > - }
> >
> > Sorry for not posting my opinion about this earlier, but IMO v2 code
> > was more correct than this one. This version makes the code being not
> > linear as it was in v2, thus harder to comprehend:
> >
> > 1. Setting up a default value and then overriding it or not makes the
> > reader to keep in mind the initialized value which is harder than to
> > just read what is done in the respective branch.
> >
>
> No, I disagree. Whether we set the default value or not, EDMA_MF_EDMA_LEGACY is
> indeed the default mapping format (this is one of the reasons why the enums
> should start from 1 instead of 0). So initializing it to legacy is not changing
> anything, rather making it explicit.
>
> > 2. Splitting up the case clause with respective inits and the mapping
> > format setting up also makes it harder to comprehend what's going on.
> > In the legacy case the reg-base address and the mapping format init are
> > split up while they should have been done simultaneously only if (val
> > != 0xFFFFFFFF).
> >
>
> Well again, this doesn't matter since the default mapping format is legacy. But
> somewhat agree that the two clauses are setting different fields, but even if
> the legacy mapping format is set inside the second clause, it still differs from
> the first one since we are not setting reg_base.
>
> > 3. The most of the current devices has the unrolled mapping (available
> > since v4.9 IP-core), thus having the mf field pre-initialized produces
> > a redundant store operation for the most of the modern devices.
> >
>
> Ok, this one I agree. We could avoid the extra assignment.
>
> > 4. Getting rid from the curly braces isn't something what should be
> > avoided at any cost and doesn't give any optimization really. It
> > doesn't cause having less C-lines of the source code and doesn't
> > improve the code readability.
> >
>
> Yeah, there is no benefit other than a simple view of the code. But for point
> (3), I agree to roll back to v2 version.
>
> > So to speak, I'd suggest to get back the v2 implementation here.
> >
> > >
> > > - pci->edma.dev = pci->dev;
> > > + return 0;
> > > +}
> > >
> > > - if (!pci->edma.ops)
> > > - pci->edma.ops = &dw_pcie_edma_ops;
> > > +static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > > +{
> > > + u32 val;
> > >
> > > - pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> >
> > > + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > + else
> > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> >
> > Just dw_pcie_readl_dma(pci, PCIE_DMA_CTRL)
> >
>
> 'val' is uninitialized. Why should the assignment be skipped?
The entire
+ if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
+ val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
+ else
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
can be replaced with a single line
+ val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
since in the legacy case (reg_base = PCIE_DMA_VIEWPORT_BASE) and the
reg_base has been initialized by now.
-Serge(y)
>
> - Mani
>
> > -Serge(y)
> >
> > >
> > > 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 +943,19 @@ 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;
> > > +
> > > + dw_pcie_edma_init_data(pci);
> > > +
> > > + ret = dw_pcie_edma_find_mf(pci);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + return dw_pcie_edma_find_channels(pci);
> > > +}
> > > +
> > > static int dw_pcie_edma_irq_verify(struct dw_pcie *pci)
> > > {
> > > struct platform_device *pdev = to_platform_device(pci->dev);
> > >
> > > --
> > > 2.25.1
> > >
>
> --
> மணிவண்ணன் சதாசிவம்
On Tue, Feb 27, 2024 at 12:00:41AM +0300, Serge Semin wrote:
> On Mon, Feb 26, 2024 at 08:57:57PM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Feb 26, 2024 at 03:45:16PM +0300, Serge Semin wrote:
> > > Hi Manivannan
> > >
> > > On Mon, Feb 26, 2024 at 05:07:26PM +0530, Manivannan Sadhasivam wrote:
> > > > In order to add support for Hyper DMA (HDMA), let's refactor the existing
> > > > dw_pcie_edma_find_chip() API by moving the common code to separate
> > > > functions.
> > > >
> > > > No functional change.
> > > >
> > > > Suggested-by: Serge Semin <[email protected]>
> > > > Signed-off-by: Manivannan Sadhasivam <[email protected]>
> > > > ---
> > > > drivers/pci/controller/dwc/pcie-designware.c | 52 +++++++++++++++++++++-------
> > > > 1 file changed, 39 insertions(+), 13 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > index 250cf7f40b85..193fcd86cf93 100644
> > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > @@ -880,7 +880,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,24 +910,27 @@ static int dw_pcie_edma_find_chip(struct dw_pcie *pci)
> > > > else
> > > > 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;
> > > > + /* Set default mapping format here and update it below if needed */
> > > > + pci->edma.mf = EDMA_MF_EDMA_LEGACY;
> > > >
> > > > + if (val == 0xFFFFFFFF && pci->edma.reg_base)
> > > > + pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > > > + else if (val != 0xFFFFFFFF)
> > > > pci->edma.reg_base = pci->dbi_base + PCIE_DMA_VIEWPORT_BASE;
> > > > - } else {
> > > > + else
> > > > return -ENODEV;
> > > > - }
> > >
> > > Sorry for not posting my opinion about this earlier, but IMO v2 code
> > > was more correct than this one. This version makes the code being not
> > > linear as it was in v2, thus harder to comprehend:
> > >
> > > 1. Setting up a default value and then overriding it or not makes the
> > > reader to keep in mind the initialized value which is harder than to
> > > just read what is done in the respective branch.
> > >
> >
> > No, I disagree. Whether we set the default value or not, EDMA_MF_EDMA_LEGACY is
> > indeed the default mapping format (this is one of the reasons why the enums
> > should start from 1 instead of 0). So initializing it to legacy is not changing
> > anything, rather making it explicit.
> >
> > > 2. Splitting up the case clause with respective inits and the mapping
> > > format setting up also makes it harder to comprehend what's going on.
> > > In the legacy case the reg-base address and the mapping format init are
> > > split up while they should have been done simultaneously only if (val
> > > != 0xFFFFFFFF).
> > >
> >
> > Well again, this doesn't matter since the default mapping format is legacy. But
> > somewhat agree that the two clauses are setting different fields, but even if
> > the legacy mapping format is set inside the second clause, it still differs from
> > the first one since we are not setting reg_base.
> >
> > > 3. The most of the current devices has the unrolled mapping (available
> > > since v4.9 IP-core), thus having the mf field pre-initialized produces
> > > a redundant store operation for the most of the modern devices.
> > >
> >
> > Ok, this one I agree. We could avoid the extra assignment.
> >
> > > 4. Getting rid from the curly braces isn't something what should be
> > > avoided at any cost and doesn't give any optimization really. It
> > > doesn't cause having less C-lines of the source code and doesn't
> > > improve the code readability.
> > >
> >
> > Yeah, there is no benefit other than a simple view of the code. But for point
> > (3), I agree to roll back to v2 version.
> >
> > > So to speak, I'd suggest to get back the v2 implementation here.
> > >
> > > >
> > > > - pci->edma.dev = pci->dev;
> > > > + return 0;
> > > > +}
> > > >
> > > > - if (!pci->edma.ops)
> > > > - pci->edma.ops = &dw_pcie_edma_ops;
> > > > +static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > > > +{
> > > > + u32 val;
> > > >
> > > > - pci->edma.flags |= DW_EDMA_CHIP_LOCAL;
> > >
> > > > + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > > + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > > + else
> > > > + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > >
> > > Just dw_pcie_readl_dma(pci, PCIE_DMA_CTRL)
> > >
> >
> > 'val' is uninitialized. Why should the assignment be skipped?
>
> The entire
>
> + if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> + val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> + else
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
>
> can be replaced with a single line
>
> + val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
>
> since in the legacy case (reg_base = PCIE_DMA_VIEWPORT_BASE) and the
> reg_base has been initialized by now.
>
Ah okay, got it!
- Mani
--
மணிவண்ணன் சதாசிவம்