PCI: exynos: remove redundant code in exynos_pcie_establish_link

PATCH New Pankaj Dubey 4 participants 10 messages Oct 9, 2017 – Dec 28, 2017 1 message not in the archive

Subsystems (2)

Share & tools

A message that is not in the archive (sent off-list or not yet received): CGME20171009141438epcas2p4ead7e2f2d1af7e0950f2c026e3a9943b@epcas2p4.samsung.com

Pankaj Dubey

author

2017-10-09 14:14 UTC #1 lore raw

PATCH 1/1 PCI: exynos: remove redundant code in exynos_pcie_establish_link

From: Anvesh Salveru <[email protected]>

In exynos_pcie_establish_link if driver is not using generic phy, we are resetting PHY twice, which is redundant, so this patch removes repeated lines of code for PHY reset.

Signed-off-by Anvesh Salveru <[email protected]>

---
 drivers/pci/dwc/pci-exynos.c | 7 -------
 1 file changed, 7 deletions(-)

drivers/pci/dwc/pci-exynos.c +0 -7

index 5596fde..85d2f4b 100644
@@ -423,13 +423,6 @@ static int exynos_pcie_establish_link(struct exynos_pcie *ep)
 		exynos_pcie_deassert_phy_reset(ep);
 		exynos_pcie_power_on_phy(ep);
 		exynos_pcie_init_phy(ep);
-
-		/* pulse for common reset */
-		exynos_pcie_writel(ep->mem_res->block_base, 1,
-					PCIE_PHY_COMMON_RESET);
-		udelay(500);
-		exynos_pcie_writel(ep->mem_res->block_base, 0,
-					PCIE_PHY_COMMON_RESET);
 	}
 
 	/* pulse for common reset */

Signature · 1 line

2.7.4

Pankaj Dubey

author

2017-10-10 13:46 UTC #5 lore raw

PATCH 1/1 PCI: exynos: remove redundant code in exynos_pcie_establish_link

Hi Jingoo,

Quoted 11 lines · Jingoo Han

Quoted 9 lines · Krzysztof Kozlowski
Quoted 4 lines · Pankaj Dubey
From: Anvesh Salveru <[email protected]>
In exynos_pcie_establish_link if driver is not using generic phy, we are resetting PHY twice, which is redundant, so this patch removes
Hi Pankaj,
This lacks the information why it is redundant.
(I resend this mail, because email address of pci list was corrupted.)

Thanks, somehow I typed wrong email id.

Quoted 5 lines

I think so, too.
Did you test this code on some boards with Exynos PCIe? Or did hardware engineers confirm this?
Please add more information on this patch.

I have replied reason behind this patch in reply to Krzysztof, hope I am able to
explain logic behind this change.

I do not have access to Exynos5440 PCIe, and this PHY_COMMON_RESET is not applicable to other Exynos SoC which I have with me, so I can't test this change,
but if you see the change it is an obvious change, before introducing generic phy
support to this driver PHY_COMMON_RESET was programmed only once, then in case platform is not using PHY it suppose to be done only once during linkup.
I am not sure when Jaehoon introduced this patch, he verified this on Exynos5440 or
not. We are just trying to make the logic as it was before without affecting anything.

Thanks,
Pankaj Dubey

Quoted 41 lines

Best regards,
Jingoo Han

Quoted 34 lines

Quoted 3 lines

repeated lines of code for PHY reset.
Signed-off-by Anvesh Salveru <[email protected]>

Your Signed-off-by is needed here.

Best regards,
Krzysztof

drivers/pci/dwc/pci-exynos.c static int exynos_pcie_establish_link(struct

Quoted 9 lines

---
  drivers/pci/dwc/pci-exynos.c | 7 -------
  1 file changed, 7 deletions(-)

drivers/pci/dwc/pci-exynos.c +0 -0

index 5596fde..85d2f4b 100644
@@ -423,13 +423,6 @@ static int exynos_pcie_establish_link(struct

exynos_pcie *ep)

Quoted 16 lines

drivers/pci/dwc/pci-exynos.c +0 -8

                 exynos_pcie_deassert_phy_reset(ep);
                 exynos_pcie_power_on_phy(ep);
                 exynos_pcie_init_phy(ep);
-
-               /* pulse for common reset */
-               exynos_pcie_writel(ep->mem_res->block_base, 1,
-                                       PCIE_PHY_COMMON_RESET);
-               udelay(500);
-               exynos_pcie_writel(ep->mem_res->block_base, 0,
-                                       PCIE_PHY_COMMON_RESET);
         }
 
         /* pulse for common reset */
--

2.7.4

Jingoo Han

2017-12-21 16:31 UTC #6 lore raw

PATCH 1/1 PCI: exynos: remove redundant code in exynos_pcie_establish_link

Quoted 39 lines · Pankaj Dubey

Hi Jingoo,
Quoted 11 lines · Jingoo Han
Quoted 9 lines · Krzysztof Kozlowski
Quoted 4 lines · Pankaj Dubey
From: Anvesh Salveru <[email protected]>
In exynos_pcie_establish_link if driver is not using generic phy, we are resetting PHY twice, which is redundant, so this patch removes
Hi Pankaj,
This lacks the information why it is redundant.
(I resend this mail, because email address of pci list was corrupted.)
Thanks, somehow I typed wrong email id.
Quoted 5 lines
I think so, too.
Did you test this code on some boards with Exynos PCIe? Or did hardware engineers confirm this?
Please add more information on this patch.
I have replied reason behind this patch in reply to Krzysztof, hope I am able to
explain logic behind this change.
I do not have access to Exynos5440 PCIe, and this PHY_COMMON_RESET is not applicable to other Exynos SoC which I have with me, so I can't test this change,
but if you see the change it is an obvious change, before introducing generic phy
support to this driver PHY_COMMON_RESET was programmed only once, then in case platform is not using PHY it suppose to be done only once during linkup.
I am not sure when Jaehoon introduced this patch, he verified this on Exynos5440 or
not. We are just trying to make the logic as it was before without affecting anything.

Quoted 9 lines

Thanks,
Pankaj Dubey
Quoted 7 lines
Best regards,
Jingoo Han
Quoted 4 lines
Quoted 3 lines
repeated lines of code for PHY reset.
Signed-off-by Anvesh Salveru <[email protected]>
Your Signed-off-by is needed here.

Sorry for being late.
I checked that this patch is right.

Can you send this patch again with your Signed-off-by? Also, you can add my Acked-by to your new patch.

Acked-by Jingoo Han <[email protected]>

Best regards,
Jingoo Han

Quoted 35 lines

Quoted 9 lines

Quoted 9 lines
Best regards,
Krzysztof
drivers/pci/dwc/pci-
Quoted 5 lines
---
  drivers/pci/dwc/pci-exynos.c | 7 -------
  1 file changed, 7 deletions(-)
drivers/pci/dwc/pci- renamed from drivers/pci/dwc/pci-exynos.c +0 -0

exynos.c

Quoted 25 lines

Quoted 21 lines

drivers/pci/dwc/pci-exynos.c static int exynos_pcie_establish_link(struct

Quoted 4 lines

index 5596fde..85d2f4b 100644
drivers/pci/dwc/pci-exynos.c +0 -0
@@ -423,13 +423,6 @@ static int exynos_pcie_establish_link(struct

exynos_pcie *ep)

Quoted 16 lines

drivers/pci/dwc/pci-exynos.c +0 -8

                 exynos_pcie_deassert_phy_reset(ep);
                 exynos_pcie_power_on_phy(ep);
                 exynos_pcie_init_phy(ep);
-
-               /* pulse for common reset */
-               exynos_pcie_writel(ep->mem_res->block_base, 1,
-                                       PCIE_PHY_COMMON_RESET);
-               udelay(500);
-               exynos_pcie_writel(ep->mem_res->block_base, 0,
-                                       PCIE_PHY_COMMON_RESET);
         }
 
         /* pulse for common reset */
--

2.7.4

Pankaj Dubey

author

2017-12-28 09:50 UTC #7 lore raw

PATCH 1/1 PCI: exynos: remove redundant code in exynos_pcie_establish_link

Hi Jingoo,

Quoted 47 lines · Jingoo Han

Quoted 46 lines · Pankaj Dubey
Hi Jingoo,
Quoted 11 lines · Jingoo Han
Quoted 9 lines · Krzysztof Kozlowski
Quoted 4 lines · Pankaj Dubey
From: Anvesh Salveru <[email protected]>
In exynos_pcie_establish_link if driver is not using generic phy, we are resetting PHY twice, which is redundant, so this patch removes
Hi Pankaj,
This lacks the information why it is redundant.
(I resend this mail, because email address of pci list was corrupted.)
Thanks, somehow I typed wrong email id.
Quoted 5 lines
I think so, too.
Did you test this code on some boards with Exynos PCIe? Or did hardware engineers confirm this?
Please add more information on this patch.
I have replied reason behind this patch in reply to Krzysztof, hope I am able to
explain logic behind this change.
I do not have access to Exynos5440 PCIe, and this PHY_COMMON_RESET is not applicable to other Exynos SoC which I have with me, so I can't test this change,
but if you see the change it is an obvious change, before introducing generic phy
support to this driver PHY_COMMON_RESET was programmed only once, then in case platform is not using PHY it suppose to be done only once during linkup.
I am not sure when Jaehoon introduced this patch, he verified this on Exynos5440 or
not. We are just trying to make the logic as it was before without affecting anything.
Thanks,
Pankaj Dubey
Quoted 6 lines
Best regards,
Jingoo Han
Quoted 3 lines
Quoted 3 lines
repeated lines of code for PHY reset.
Signed-off-by Anvesh Salveru <[email protected]>

Quoted 8 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
Your Signed-off-by is needed here.
Sorry for being late.
I checked that this patch is right.
Can you send this patch again with your Signed-off-by? Also, you can add my Acked-by to your new patch.
Acked-by Jingoo Han <[email protected]>

Thanks for review and ack.
Will resubmit the patch along with my Signed-off-by and your Acked-by.

Thanks,
Pankaj Dubey

Quoted 40 lines

Best regards,
Jingoo Han

Quoted 33 lines

Quoted 8 lines

Quoted 8 lines
Best regards,
Krzysztof
drivers/pci/dwc/pci-
Quoted 5 lines
---
   drivers/pci/dwc/pci-exynos.c | 7 -------
   1 file changed, 7 deletions(-)
drivers/pci/dwc/pci- renamed from drivers/pci/dwc/pci-exynos.c +0 -0

exynos.c

Quoted 24 lines

Quoted 21 lines

drivers/pci/dwc/pci-exynos.c static int exynos_pcie_establish_link(struct

Quoted 4 lines

index 5596fde..85d2f4b 100644
drivers/pci/dwc/pci-exynos.c +0 -0
@@ -423,13 +423,6 @@ static int exynos_pcie_establish_link(struct

exynos_pcie *ep)

Quoted 16 lines

drivers/pci/dwc/pci-exynos.c +0 -8

                  exynos_pcie_deassert_phy_reset(ep);
                  exynos_pcie_power_on_phy(ep);
                  exynos_pcie_init_phy(ep);
-
-               /* pulse for common reset */
-               exynos_pcie_writel(ep->mem_res->block_base, 1,
-                                       PCIE_PHY_COMMON_RESET);
-               udelay(500);
-               exynos_pcie_writel(ep->mem_res->block_base, 0,
-                                       PCIE_PHY_COMMON_RESET);
          }
 
          /* pulse for common reset */
--

2.7.4

Jaehoon Chung

2017-12-28 09:58 UTC #8 lore raw

PATCH 1/1 PCI: exynos: remove redundant code in exynos_pcie_establish_link

Hi Pankaj,

Quoted 55 lines · Pankaj Dubey

Hi Jingoo,
Quoted 51 lines · Jingoo Han
Quoted 38 lines · Pankaj Dubey
Hi Jingoo,
Quoted 11 lines · Jingoo Han
Quoted 9 lines · Krzysztof Kozlowski
Quoted 4 lines · Pankaj Dubey
From: Anvesh Salveru <[email protected]>
In exynos_pcie_establish_link if driver is not using generic phy, we are resetting PHY twice, which is redundant, so this patch removes
Hi Pankaj,
This lacks the information why it is redundant.
(I resend this mail, because email address of pci list was corrupted.)
Thanks, somehow I typed wrong email id.
Quoted 5 lines
I think so, too.
Did you test this code on some boards with Exynos PCIe? Or did hardware engineers confirm this?
Please add more information on this patch.
I have replied reason behind this patch in reply to Krzysztof, hope I am able to
explain logic behind this change.
I do not have access to Exynos5440 PCIe, and this PHY_COMMON_RESET is not applicable to other Exynos SoC which I have with me, so I can't test this change,
but if you see the change it is an obvious change, before introducing generic phy
support to this driver PHY_COMMON_RESET was programmed only once, then in case platform is not using PHY it suppose to be done only once during linkup.
I am not sure when Jaehoon introduced this patch, he verified this on Exynos5440 or
not. We are just trying to make the logic as it was before without affecting anything.
Quoted 9 lines
Thanks,
Pankaj Dubey
Quoted 7 lines
Best regards,
Jingoo Han
Quoted 4 lines
Quoted 3 lines
repeated lines of code for PHY reset.
Signed-off-by Anvesh Salveru <[email protected]>
Your Signed-off-by is needed here.
Sorry for being late.
I checked that this patch is right.

Quoted 7 lines

Quoted 4 lines
Can you send this patch again with your Signed-off-by? Also, you can add my Acked-by to your new patch.
Acked-by Jingoo Han <[email protected]>
Thanks for review and ack.
Will resubmit the patch along with my Signed-off-by and your Acked-by.

Maybe it will be removed with my patch.

https://patchwork.ozlab.org/patch/853119/

Because of my personal reason, i can't send the email to Bjorn's account..so i removed his account from cc. Sorry.

Best Regards,
Jaehoon Chung

Quoted 47 lines

Thanks,
Pankaj Dubey

Quoted 40 lines

Best regards,
Jingoo Han

Quoted 33 lines

Quoted 8 lines

Quoted 8 lines
Best regards,
Krzysztof
drivers/pci/dwc/pci-
Quoted 5 lines
---
   drivers/pci/dwc/pci-exynos.c | 7 -------
   1 file changed, 7 deletions(-)
drivers/pci/dwc/pci- renamed from drivers/pci/dwc/pci-exynos.c +0 -0

exynos.c

Quoted 24 lines

Quoted 21 lines

drivers/pci/dwc/pci-exynos.c static int exynos_pcie_establish_link(struct

Quoted 4 lines

index 5596fde..85d2f4b 100644
drivers/pci/dwc/pci-exynos.c +0 -0
@@ -423,13 +423,6 @@ static int exynos_pcie_establish_link(struct

exynos_pcie *ep)

Quoted 16 lines

drivers/pci/dwc/pci-exynos.c +0 -8

                  exynos_pcie_deassert_phy_reset(ep);
                  exynos_pcie_power_on_phy(ep);
                  exynos_pcie_init_phy(ep);
-
-               /* pulse for common reset */
-               exynos_pcie_writel(ep->mem_res->block_base, 1,
-                                       PCIE_PHY_COMMON_RESET);
-               udelay(500);
-               exynos_pcie_writel(ep->mem_res->block_base, 0,
-                                       PCIE_PHY_COMMON_RESET);
          }
 
          /* pulse for common reset */
--

2.7.4

Pankaj Dubey

author

2017-12-28 10:29 UTC #9 lore raw

PATCH 1/1 PCI: exynos: remove redundant code in exynos_pcie_establish_link

Hi Jaehoon,

Quoted 62 lines · Jaehoon Chung

Hi Pankaj,
Quoted 59 lines · Pankaj Dubey
Hi Jingoo,
Quoted 55 lines · Jingoo Han
Quoted 38 lines · Pankaj Dubey
Hi Jingoo,
Quoted 11 lines · Jingoo Han
Quoted 9 lines · Krzysztof Kozlowski
Quoted 4 lines · Pankaj Dubey
From: Anvesh Salveru <[email protected]>
In exynos_pcie_establish_link if driver is not using generic phy, we are resetting PHY twice, which is redundant, so this patch removes
Hi Pankaj,
This lacks the information why it is redundant.
(I resend this mail, because email address of pci list was corrupted.)
Thanks, somehow I typed wrong email id.
Quoted 5 lines
I think so, too.
Did you test this code on some boards with Exynos PCIe? Or did hardware engineers confirm this?
Please add more information on this patch.
I have replied reason behind this patch in reply to Krzysztof, hope I am able to
explain logic behind this change.
I do not have access to Exynos5440 PCIe, and this PHY_COMMON_RESET is not applicable to other Exynos SoC which I have with me, so I can't test this change,
but if you see the change it is an obvious change, before introducing generic phy
support to this driver PHY_COMMON_RESET was programmed only once, then in case platform is not using PHY it suppose to be done only once during linkup.
I am not sure when Jaehoon introduced this patch, he verified this on Exynos5440 or
not. We are just trying to make the logic as it was before without affecting anything.
Quoted 9 lines
Thanks,
Pankaj Dubey
Quoted 7 lines
Best regards,
Jingoo Han
Quoted 4 lines
Quoted 3 lines
repeated lines of code for PHY reset.
Signed-off-by Anvesh Salveru <[email protected]>
Your Signed-off-by is needed here.
Sorry for being late.
I checked that this patch is right.
Can you send this patch again with your Signed-off-by? Also, you can add my Acked-by to your new patch.
Acked-by Jingoo Han <[email protected]>

Quoted 8 lines

Quoted 2 lines
Thanks for review and ack.
Will resubmit the patch along with my Signed-off-by and your Acked-by.
Maybe it will be removed with my patch.
https://patchwork.ozlab.org/patch/853119/
Because of my personal reason, i can't send the email to Bjorn's account..so i removed his account from cc. Sorry.

As per submission date, we have submitted it first, so it should be taken in that order, but I I am fine with any of the patch, so I will leave this on Bjorn.

Thanks,
Pankaj Dubey

Quoted 48 lines

Best Regards,
Jaehoon Chung

Quoted 43 lines

Thanks,
Pankaj Dubey

Quoted 38 lines

Best regards,
Jingoo Han

Quoted 32 lines

Quoted 8 lines

Quoted 8 lines
Best regards,
Krzysztof
drivers/pci/dwc/pci-
Quoted 5 lines
---
    drivers/pci/dwc/pci-exynos.c | 7 -------
    1 file changed, 7 deletions(-)
drivers/pci/dwc/pci- renamed from drivers/pci/dwc/pci-exynos.c +0 -0

exynos.c

Quoted 23 lines

Quoted 21 lines

drivers/pci/dwc/pci-exynos.c static int exynos_pcie_establish_link(struct

Quoted 4 lines

index 5596fde..85d2f4b 100644
drivers/pci/dwc/pci-exynos.c +0 -0
@@ -423,13 +423,6 @@ static int exynos_pcie_establish_link(struct

exynos_pcie *ep)

Quoted 16 lines

drivers/pci/dwc/pci-exynos.c +0 -8

                   exynos_pcie_deassert_phy_reset(ep);
                   exynos_pcie_power_on_phy(ep);
                   exynos_pcie_init_phy(ep);
-
-               /* pulse for common reset */
-               exynos_pcie_writel(ep->mem_res->block_base, 1,
-                                       PCIE_PHY_COMMON_RESET);
-               udelay(500);
-               exynos_pcie_writel(ep->mem_res->block_base, 0,
-                                       PCIE_PHY_COMMON_RESET);
           }
 
           /* pulse for common reset */
--

2.7.4

Pankaj Dubey

author

2017-10-10 13:40 UTC #10 lore raw

PATCH 1/1 PCI: exynos: remove redundant code in exynos_pcie_establish_link

In reply to Krzysztof Kozlowski

Quoted 8 lines · Krzysztof Kozlowski

Quoted 4 lines · Pankaj Dubey
From: Anvesh Salveru <[email protected]>
In exynos_pcie_establish_link if driver is not using generic phy, we are resetting PHY twice, which is redundant, so this patch removes
Hi Pankaj,
This lacks the information why it is redundant.

Sure. Ok probably commit message should have been covered more information.

I can see this patch tries to fix redundancy added by patch [1]:

[1] e7cd7ef58e1f "PCI: exynos: Support the PHY generic framework"

Before adding support for the generic PHY, exynos5440 was doing PHY reset via

/* pulse for common reset */ exynos_pcie_writel(ep->mem_res->block_base, 1, PCIE_PHY_COMMON_RESET); udelay(500);
exynos_pcie_writel(ep->mem_res->block_base, 0, PCIE_PHY_COMMON_RESET);

But when Jaehoon introduced generic PHY support, above two lines have been copied in else part (else case covers: if platform does not uses generic PHY),
so eventually we will end-up doing PHY reset twice.

For more clarity copy pasting part of above patch:

------------------------------------
     exynos_pcie_assert_core_reset(ep);
-    exynos_pcie_assert_phy_reset(ep);
-    exynos_pcie_deassert_phy_reset(ep);
-    exynos_pcie_power_on_phy(ep);
-    exynos_pcie_init_phy(ep);
+
+    if (ep->using_phy) {
+        phy_reset(ep->phy);
+
+        exynos_pcie_writel(ep->mem_res->elbi_base, 1, +                PCIE_PWR_RESET);
+
+        phy_power_on(ep->phy);
+        phy_init(ep->phy);
+    } else {
+        exynos_pcie_assert_phy_reset(ep);
+        exynos_pcie_deassert_phy_reset(ep);
+        exynos_pcie_power_on_phy(ep);
+        exynos_pcie_init_phy(ep);
+
+        /* pulse for common reset */
+        exynos_pcie_writel(ep->mem_res->block_base, 1, +                    PCIE_PHY_COMMON_RESET);
+        udelay(500);
+        exynos_pcie_writel(ep->mem_res->block_base, 0, +                    PCIE_PHY_COMMON_RESET);
+    }

     /* pulse for common reset */ exynos_pcie_writel(ep->mem_res->block_base, 1,
PCIE_PHY_COMMON_RESET);
     udelay(500);
     exynos_pcie_writel(ep->mem_res->block_base, 0, PCIE_PHY_COMMON_RESET);
-----------------------------------------------

So if you see, eventually now we will end up doing common reset twice in case
platform not using generic PHY (once in else part and immediately after else).

We have submitted this patch without verifying on actual Exynos5440 board, as currently we do not have access to this board.

But I feel in the original patch this flow has been changed, that too probably without verifying
on Exynos5440 board (Probably as I came across Exynos5433 PCIe patch of Jaehoon where
he mentioned he also do not have access to Exynos5440 board).

So this patch removes that double reset of PHY and makes the code flow as it was originally,
before introduction of generic PHY.

Thanks,
Pankaj Dubey

Quoted 4 lines

Quoted 3 lines
repeated lines of code for PHY reset.
Signed-off-by Anvesh Salveru <[email protected]>
Your Signed-off-by is needed here.

I missed it, will add in v2.

Quoted 31 lines

Best regards,
Krzysztof

Quoted 25 lines

---
  drivers/pci/dwc/pci-exynos.c | 7 -------
  1 file changed, 7 deletions(-)

drivers/pci/dwc/pci-exynos.c +0 -8

index 5596fde..85d2f4b 100644
@@ -423,13 +423,6 @@ static int exynos_pcie_establish_link(struct exynos_pcie *ep)
                 exynos_pcie_deassert_phy_reset(ep);
                 exynos_pcie_power_on_phy(ep);
                 exynos_pcie_init_phy(ep);
-
-               /* pulse for common reset */
-               exynos_pcie_writel(ep->mem_res->block_base, 1,
-                                       PCIE_PHY_COMMON_RESET);
-               udelay(500);
-               exynos_pcie_writel(ep->mem_res->block_base, 0,
-                                       PCIE_PHY_COMMON_RESET);
         }
 
         /* pulse for common reset */
--

2.7.4