PCI: aardvark: Update comment about disabling link training
Subsystems (3)
Pali Rohár
maintainerPATCH 1/1 PCI: aardvark: Update comment about disabling link training
It is not HW bug or workaround for some cards but it is requirement by PCI Express spec. After fundamental reset is needed 100ms delay prior enabling link training. So update comment in code to reflect this requirement.
--- drivers/pci/controller/pci-aardvark.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/pci/controller/pci-aardvark.c +6 -1
@@ -259,7 +259,12 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)if (!pcie->reset_gpio)return ; -/* PERST does not work for some cards when link training is enabled */ +/* +* As required by PCI Express spec a delay for at least 100ms after +* de-asserting PERST# signal is needed before link training is enabled. +* So ensure that link training is disabled prior de-asserting PERST# +* signal to fulfill that PCI Express spec requirement. +*/ reg =advk_readl (pcie,PCIE_CORE_CTRL0_REG ); reg &= ~LINK_TRAINING_EN ;advk_writel (pcie, reg,PCIE_CORE_CTRL0_REG );
Signature · 1 line
2.20.1
Quoted 14 lines · Pali Rohár
It is not HW bug or workaround for some cards but it is requirement by PCI Express spec. After fundamental reset is needed 100ms delay prior enabling link training. So update comment in code to reflect this requirement.
Signed-off-by Pali Rohár <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +0
-0
drivers/pci/controller/pci-aardvark.c static void advk_pcie_issue_perst(struct advk_pcie *pcie) · line 266
Quoted 9 lines
drivers/pci/controller/pci-aardvark.c +5
-1@@ -259,7 +259,12 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)if (!pcie->reset_gpio)return ; -/* PERST does not work for some cards when link training is enabled */ +/* +* As required by PCI Express spec a delay for at least 100ms after +* de-asserting PERST# signal is needed before link training is enabled. +* So ensure that link training is disabled prior de-asserting PERST# +* signal to fulfill that PCI Express spec requirement.
Can you please include the spec citation here? In the PCIe base spec, PERST# is only mentioned in PCIe r5.0, sec 6.6.1, and I don't see the connection there to 100ms between de-assert of PERST# and enabling link training.
Sec 6.1.1 does talk about 100ms before sending config requests (for ports that support <= 5 GT/s), and 100ms after link training completes (for ports that support > 5 GT/s).
Maybe there's more language in a form-factor spec or something?
Quoted 7 lines
drivers/pci/controller/pci-aardvark.c +1
-0+*/ reg =advk_readl (pcie,PCIE_CORE_CTRL0_REG ); reg &= ~LINK_TRAINING_EN ;advk_writel (pcie, reg,PCIE_CORE_CTRL0_REG );Signature · 1 line
2.20.1
Pali Rohár
maintainerQuoted 18 lines · On Thursday 24 September 2020 10:11:06 Bjorn Helgaas wrote:
Quoted 17 lines · Pali Rohár
It is not HW bug or workaround for some cards but it is requirement by PCI Express spec. After fundamental reset is needed 100ms delay prior enabling link training. So update comment in code to reflect this requirement.
Signed-off-by Pali Rohár <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -259,7 +259,12 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)if (!pcie->reset_gpio)return ;
Quoted 11 lines
drivers/pci/controller/pci-aardvark.c static void advk_pcie_issue_perst(struct advk_pcie *pcie) · line 266
Quoted 6 lines
drivers/pci/controller/pci-aardvark.c +5
-1-/* PERST does not work for some cards when link training is enabled */ +/* +* As required by PCI Express spec a delay for at least 100ms after +* de-asserting PERST# signal is needed before link training is enabled. +* So ensure that link training is disabled prior de-asserting PERST# +* signal to fulfill that PCI Express spec requirement. Can you please include the spec citation here? In the PCIe base spec, PERST# is only mentioned in PCIe r5.0, sec 6.6.1, and I don't see the connection there to 100ms between de-assert of PERST# and enabling link training.
Hello! I copied this "comment" from other place in pci-aardvark.c where that timeout 100ms is already applied. Timeout with explanation comment was introduced in following commit:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f4c7d053d7f7
Here are links to discussions about that patch:
https://lore.kernel.org/linux-pci/[email protected]/T/#u
https://lore.kernel.org/linux-pci/[email protected]/T/#u
Quoted 13 lines
Sec 6.1.1 does talk about 100ms before sending config requests (for ports that support <= 5 GT/s), and 100ms after link training completes (for ports that support > 5 GT/s).
Maybe there's more language in a form-factor spec or something?
Quoted 7 lines
drivers/pci/controller/pci-aardvark.c +1
-0+*/ reg =advk_readl (pcie,PCIE_CORE_CTRL0_REG ); reg &= ~LINK_TRAINING_EN ;advk_writel (pcie, reg,PCIE_CORE_CTRL0_REG );Signature · 1 line
2.20.1
Pali Rohár
maintainerQuoted 35 lines · On Thursday 24 September 2020 17:22:32 Pali Rohár wrote:
Quoted 18 lines · On Thursday 24 September 2020 10:11:06 Bjorn Helgaas wrote:
Quoted 17 lines · Pali Rohár
It is not HW bug or workaround for some cards but it is requirement by PCI Express spec. After fundamental reset is needed 100ms delay prior enabling link training. So update comment in code to reflect this requirement.
Signed-off-by Pali Rohár <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -259,7 +259,12 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)if (!pcie->reset_gpio)return ;Quoted 11 lines
drivers/pci/controller/pci-aardvark.c static void advk_pcie_issue_perst(struct advk_pcie *pcie) · line 266
Quoted 6 lines
drivers/pci/controller/pci-aardvark.c +5
-1-/* PERST does not work for some cards when link training is enabled */ +/* +* As required by PCI Express spec a delay for at least 100ms after +* de-asserting PERST# signal is needed before link training is enabled. +* So ensure that link training is disabled prior de-asserting PERST# +* signal to fulfill that PCI Express spec requirement. Can you please include the spec citation here? In the PCIe base spec, PERST# is only mentioned in PCIe r5.0, sec 6.6.1, and I don't see the connection there to 100ms between de-assert of PERST# and enabling link training.
Hello! I copied this "comment" from other place in
pci-aardvark.cwhere that timeout 100ms is already applied. Timeout with explanation comment was introduced in following commit:
Quoted 6 lines
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f4c7d053d7f7
Here are links to discussions about that patch:
https://lore.kernel.org/linux-pci/[email protected]/T/#u
https://lore.kernel.org/linux-pci/[email protected]/T/#u
Bjorn or Lorenzo, do you need something else for this patch? It just updates comment and basically clarify why PERST does not work for some cards when link training is enabled.
Quoted 13 lines
Quoted 13 lines
Sec 6.1.1 does talk about 100ms before sending config requests (for ports that support <= 5 GT/s), and 100ms after link training completes (for ports that support > 5 GT/s).
Maybe there's more language in a form-factor spec or something?
Quoted 7 lines
drivers/pci/controller/pci-aardvark.c +1
-0+*/ reg =advk_readl (pcie,PCIE_CORE_CTRL0_REG ); reg &= ~LINK_TRAINING_EN ;advk_writel (pcie, reg,PCIE_CORE_CTRL0_REG );Signature · 1 line
2.20.1
Pali Rohár
maintainerQuoted 38 lines · On Sunday 11 October 2020 19:21:49 Pali Rohár wrote:
Quoted 37 lines · On Thursday 24 September 2020 17:22:32 Pali Rohár wrote:
Quoted 18 lines · On Thursday 24 September 2020 10:11:06 Bjorn Helgaas wrote:
Quoted 17 lines · Pali Rohár
It is not HW bug or workaround for some cards but it is requirement by PCI Express spec. After fundamental reset is needed 100ms delay prior enabling link training. So update comment in code to reflect this requirement.
Signed-off-by Pali Rohár <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -259,7 +259,12 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)if (!pcie->reset_gpio)return ;Quoted 11 lines
drivers/pci/controller/pci-aardvark.c static void advk_pcie_issue_perst(struct advk_pcie *pcie) · line 266
Quoted 6 lines
drivers/pci/controller/pci-aardvark.c +5
-1-/* PERST does not work for some cards when link training is enabled */ +/* +* As required by PCI Express spec a delay for at least 100ms after +* de-asserting PERST# signal is needed before link training is enabled. +* So ensure that link training is disabled prior de-asserting PERST# +* signal to fulfill that PCI Express spec requirement. Can you please include the spec citation here? In the PCIe base spec, PERST# is only mentioned in PCIe r5.0, sec 6.6.1, and I don't see the connection there to 100ms between de-assert of PERST# and enabling link training.
Hello! I copied this "comment" from other place in
pci-aardvark.cwhere that timeout 100ms is already applied. Timeout with explanation comment was introduced in following commit:https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f4c7d053d7f7
Quoted 8 lines
Quoted 4 lines
Here are links to discussions about that patch:
https://lore.kernel.org/linux-pci/[email protected]/T/#u
https://lore.kernel.org/linux-pci/[email protected]/T/#uBjorn or Lorenzo, do you need something else for this patch? It just updates comment and basically clarify why PERST does not work for some cards when link training is enabled.
PING?
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Sec 6.1.1 does talk about 100ms before sending config requests (for ports that support <= 5 GT/s), and 100ms after link training completes (for ports that support > 5 GT/s).
Maybe there's more language in a form-factor spec or something?
Quoted 7 lines
drivers/pci/controller/pci-aardvark.c +1
-0+*/ reg =advk_readl (pcie,PCIE_CORE_CTRL0_REG ); reg &= ~LINK_TRAINING_EN ;advk_writel (pcie, reg,PCIE_CORE_CTRL0_REG );Signature · 1 line
2.20.1
Quoted 41 lines · Pali Rohár
Quoted 40 lines · On Sunday 11 October 2020 19:21:49 Pali Rohár wrote:
Quoted 39 lines · On Thursday 24 September 2020 17:22:32 Pali Rohár wrote:
Quoted 18 lines · On Thursday 24 September 2020 10:11:06 Bjorn Helgaas wrote:
Quoted 17 lines · Pali Rohár
It is not HW bug or workaround for some cards but it is requirement by PCI Express spec. After fundamental reset is needed 100ms delay prior enabling link training. So update comment in code to reflect this requirement.
Signed-off-by Pali Rohár <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -259,7 +259,12 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)if (!pcie->reset_gpio)return ;Quoted 11 lines
drivers/pci/controller/pci-aardvark.c static void advk_pcie_issue_perst(struct advk_pcie *pcie) · line 266
Quoted 6 lines
drivers/pci/controller/pci-aardvark.c +5
-1-/* PERST does not work for some cards when link training is enabled */ +/* +* As required by PCI Express spec a delay for at least 100ms after +* de-asserting PERST# signal is needed before link training is enabled. +* So ensure that link training is disabled prior de-asserting PERST# +* signal to fulfill that PCI Express spec requirement. Can you please include the spec citation here? In the PCIe base spec, PERST# is only mentioned in PCIe r5.0, sec 6.6.1, and I don't see the connection there to 100ms between de-assert of PERST# and enabling link training.
Hello! I copied this "comment" from other place in
pci-aardvark.cwhere that timeout 100ms is already applied. Timeout with explanation comment was introduced in following commit:https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f4c7d053d7f7
Here are links to discussions about that patch:
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
https://lore.kernel.org/linux-pci/[email protected]/T/#u
https://lore.kernel.org/linux-pci/[email protected]/T/#uBjorn or Lorenzo, do you need something else for this patch? It just updates comment and basically clarify why PERST does not work for some cards when link training is enabled.
PING?
Apologies, I marked it as "changes requested" following Bjorn's reply. Would you mind please adding a link to the relevant PCI specs in the comment ?
I understood you copied the comment, it is worth adding that link to all of them if you don't mind, it can be a preparation patch if you wish.
Thanks,
Lorenzo
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Sec 6.1.1 does talk about 100ms before sending config requests (for ports that support <= 5 GT/s), and 100ms after link training completes (for ports that support > 5 GT/s).
Maybe there's more language in a form-factor spec or something?
Quoted 7 lines
drivers/pci/controller/pci-aardvark.c +1
-0+*/ reg =advk_readl (pcie,PCIE_CORE_CTRL0_REG ); reg &= ~LINK_TRAINING_EN ;advk_writel (pcie, reg,PCIE_CORE_CTRL0_REG );Signature · 1 line
2.20.1
Pali Rohár
maintainerQuoted 49 lines · On Monday 30 November 2020 10:52:00 Lorenzo Pieralisi wrote:
Quoted 48 lines · Pali Rohár
Quoted 46 lines · On Sunday 11 October 2020 19:21:49 Pali Rohár wrote:
Quoted 35 lines · On Thursday 24 September 2020 17:22:32 Pali Rohár wrote:
Quoted 18 lines · On Thursday 24 September 2020 10:11:06 Bjorn Helgaas wrote:
Quoted 17 lines · Pali Rohár
It is not HW bug or workaround for some cards but it is requirement by PCI Express spec. After fundamental reset is needed 100ms delay prior enabling link training. So update comment in code to reflect this requirement.
Signed-off-by Pali Rohár <[email protected]>--- drivers/pci/controller/pci-aardvark.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)drivers/pci/controller/pci-aardvark.c +0
-0@@ -259,7 +259,12 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)if (!pcie->reset_gpio)return ;Quoted 11 lines
drivers/pci/controller/pci-aardvark.c static void advk_pcie_issue_perst(struct advk_pcie *pcie) · line 266
Quoted 6 lines
drivers/pci/controller/pci-aardvark.c +5
-1-/* PERST does not work for some cards when link training is enabled */ +/* +* As required by PCI Express spec a delay for at least 100ms after +* de-asserting PERST# signal is needed before link training is enabled. +* So ensure that link training is disabled prior de-asserting PERST# +* signal to fulfill that PCI Express spec requirement. Can you please include the spec citation here? In the PCIe base spec, PERST# is only mentioned in PCIe r5.0, sec 6.6.1, and I don't see the connection there to 100ms between de-assert of PERST# and enabling link training.
Hello! I copied this "comment" from other place in
pci-aardvark.cwhere that timeout 100ms is already applied. Timeout with explanation comment was introduced in following commit:Quoted 6 lines
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f4c7d053d7f7
Here are links to discussions about that patch:
https://lore.kernel.org/linux-pci/[email protected]/T/#u
https://lore.kernel.org/linux-pci/[email protected]/T/#uBjorn or Lorenzo, do you need something else for this patch? It just updates comment and basically clarify why PERST does not work for some cards when link training is enabled.
Quoted 5 lines
Quoted 1 line
PING?
Apologies, I marked it as "changes requested" following Bjorn's reply. Would you mind please adding a link to the relevant PCI specs in the comment ?
Of course, no problem. I will copy reference to PCI spec as Remi wrote it in linked email, to the comment in source code.
Quoted 19 lines
I understood you copied the comment, it is worth adding that link to all of them if you don't mind, it can be a preparation patch if you wish.
Thanks,
LorenzoQuoted 13 lines
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
Sec 6.1.1 does talk about 100ms before sending config requests (for ports that support <= 5 GT/s), and 100ms after link training completes (for ports that support > 5 GT/s).
Maybe there's more language in a form-factor spec or something?
Quoted 7 lines
drivers/pci/controller/pci-aardvark.c +1
-0+*/ reg =advk_readl (pcie,PCIE_CORE_CTRL0_REG ); reg &= ~LINK_TRAINING_EN ;advk_writel (pcie, reg,PCIE_CORE_CTRL0_REG );Signature · 1 line
2.20.1
Pali Rohár
maintainerPATCH v2 1/1 PCI: aardvark: Update comment about disabling link training
In reply to Pali RohárIt is not HW bug or workaround for some cards but it is requirement by PCI Express spec. After fundamental reset is needed 100ms delay prior enabling link training. So update comment in code to reflect this requirement.
---
Changes in v2:
* Add reference to the PCI Express spec
--- drivers/pci/controller/pci-aardvark.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
drivers/pci/controller/pci-aardvark.c +8 -1
@@ -259,7 +259,14 @@ static void advk_pcie_issue_perst(struct advk_pcie *pcie)if (!pcie->reset_gpio)return ; -/* PERST does not work for some cards when link training is enabled */ +/* +* As required by PCI Express spec (PCI Express Base Specification, REV. +* 4.0 PCI Express, February 19 2014, 6.6.1 Conventional Reset) a delay +* for at least 100ms after de-asserting PERST# signal is needed before +* link training is enabled. So ensure that link training is disabled +* prior de-asserting PERST# signal to fulfill that PCI Express spec +* requirement. +*/ reg =advk_readl (pcie,PCIE_CORE_CTRL0_REG ); reg &= ~LINK_TRAINING_EN ;advk_writel (pcie, reg,PCIE_CORE_CTRL0_REG );
Signature · 1 line
2.20.1
Lorenzo Pieralisi
AppliedQuoted 3 lines · Pali Rohár
It is not HW bug or workaround for some cards but it is requirement by PCI Express spec. After fundamental reset is needed 100ms delay prior enabling link training. So update comment in code to reflect this requirement.
Applied to pci/aardvark, thanks!
[1/1] PCI: aardvark: Update comment about disabling link training
https://git.kernel.org/lpieralisi/pci/c/1d1cd163d0
Thanks,
Lorenzo