From: Richard Zhu <hongxing.zhu@nxp.com> Date: 2021-09-08 07:22:24
No function changes, just encapsulate the i.MX PCIe clocks enable
operations into one standalone function
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 82 +++++++++++++++++----------
1 file changed, 51 insertions(+), 31 deletions(-)
@@ -498,33 +500,12 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)}}-ret=clk_prepare_enable(imx6_pcie->pcie_phy);-if(ret){-dev_err(dev,"unable to enable pcie_phy clock\n");-gotoerr_pcie_phy;-}--ret=clk_prepare_enable(imx6_pcie->pcie_bus);+ret=imx6_pcie_clk_enable(imx6_pcie);if(ret){-dev_err(dev,"unable to enable pcie_bus clock\n");-gotoerr_pcie_bus;+dev_err(dev,"unable to enable pcie clocks\n");+gotoerr_clks;}-ret=clk_prepare_enable(imx6_pcie->pcie);-if(ret){-dev_err(dev,"unable to enable pcie clock\n");-gotoerr_pcie;-}--ret=imx6_pcie_enable_ref_clk(imx6_pcie);-if(ret){-dev_err(dev,"unable to enable pcie ref clock\n");-gotoerr_ref_clk;-}--/* allow the clocks to stabilize */-usleep_range(200,500);-/* Some boards don't have PCIe reset GPIO. */if(gpio_is_valid(imx6_pcie->reset_gpio)){gpio_set_value_cansleep(imx6_pcie->reset_gpio,
@@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)usleep_range(1000,10000);}+staticintimx6_pcie_clk_enable(structimx6_pcie*imx6_pcie)+{+structdw_pcie*pci=imx6_pcie->pci;+structdevice*dev=pci->dev;+intret;++ret=clk_prepare_enable(imx6_pcie->pcie_phy);+if(ret){+dev_err(dev,"unable to enable pcie_phy clock\n");+returnret;+}++ret=clk_prepare_enable(imx6_pcie->pcie_bus);+if(ret){+dev_err(dev,"unable to enable pcie_bus clock\n");+gotoerr_pcie_bus;+}++ret=clk_prepare_enable(imx6_pcie->pcie);+if(ret){+dev_err(dev,"unable to enable pcie clock\n");+gotoerr_pcie;+}++ret=imx6_pcie_enable_ref_clk(imx6_pcie);+if(ret){+dev_err(dev,"unable to enable pcie ref clock\n");+gotoerr_ref_clk;+}++/* allow the clocks to stabilize */+usleep_range(200,500);+return0;++err_ref_clk:+clk_disable_unprepare(imx6_pcie->pcie);+err_pcie:+clk_disable_unprepare(imx6_pcie->pcie_bus);+err_pcie_bus:+clk_disable_unprepare(imx6_pcie->pcie_phy);++returnret;++}+staticvoidimx6_pcie_clk_disable(structimx6_pcie*imx6_pcie){clk_disable_unprepare(imx6_pcie->pcie);
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Richard Zhu <hongxing.zhu@nxp.com> Date: 2021-09-08 07:22:30
Refer to the system board signal Quality of PCIe archiecture PHY test
specification. Signal quality tests can be executed with devices in the
polling.compliance state.
To let the device support polling.compliance stat, the clocks and
powers shouldn't be turned off during the compliance tests although
the PHY link might be down.
Add the i.MX PCIe compliance tests mode enable option to keep the and
powers on, and finish the driver probe without error return.
Use the "pcie_cmp_enabled=yes" in kernel command line to enable the
compliance tests mode.
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 41 +++++++++++++++++++++------
1 file changed, 33 insertions(+), 8 deletions(-)
@@ -1187,8 +1204,16 @@ static int imx6_pcie_probe(struct platform_device *pdev)returnret;ret=dw_pcie_host_init(&pci->pp);-if(ret<0)+if(ret<0){+if(imx6_pcie_cmp_enabled){+/* The PCIE clocks and powers wouldn't be turned off */+dev_info(dev,"To do the compliance tests.\n");+ret=0;+}else{+dev_err(dev,"Unable to add pcie port.\n");+}returnret;+}if(pci_msi_enabled()){u8offset=dw_pcie_find_capability(pci,PCI_CAP_ID_MSI);
--
2.25.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Lucas Stach <l.stach@pengutronix.de> Date: 2021-09-08 08:34:38
Hi Richard,
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
Refer to the system board signal Quality of PCIe archiecture PHY test
specification. Signal quality tests can be executed with devices in the
polling.compliance state.
To let the device support polling.compliance stat, the clocks and
powers shouldn't be turned off during the compliance tests although
the PHY link might be down.
Add the i.MX PCIe compliance tests mode enable option to keep the and
powers on, and finish the driver probe without error return.
Use the "pcie_cmp_enabled=yes" in kernel command line to enable the
compliance tests mode.
Adding "random" kernel command line options isn't going to fly. If at
all, this should be a module_param so it gets properly namespaced. Also
this needs a more descriptive name, right now this is abbreviating the
one thing that would tell a user what this is about: compliance
testing.
@@ -1187,8 +1204,16 @@ static int imx6_pcie_probe(struct platform_device *pdev)returnret;ret=dw_pcie_host_init(&pci->pp);-if(ret<0)+if(ret<0){+if(imx6_pcie_cmp_enabled){+/* The PCIE clocks and powers wouldn't be turned off */+dev_info(dev,"To do the compliance tests.\n");
This needs a better message, like "Driver loaded with compliance test
mode enabled" and the message needs to be shown unconditionally, not
only when the host init fails. We don't want to have the user track
down weird issues when the compliance test option was specified by
accident and the link is almost working due to this.
Regards,
Lucas
quoted hunk
+ ret = 0;+ } else {+ dev_err(dev, "Unable to add pcie port.\n");+ } return ret;+ } if (pci_msi_enabled()) { u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
This is a separate fix for the clock enable counts, that isn't
mentioned in the commit message.
It seems like this patch fixes in fact 3 issues: error propagation from
host_init, fix regulator reference handling, fix clk reference
handling. Either split up the patch along those changes (if possible)
or improve the commit message to explain those changes.
Regards,
Lucas
From: Richard Zhu <hongxing.zhu@nxp.com> Date: 2021-09-08 08:46:56
-----Original Message-----
From: Lucas Stach <l.stach@pengutronix.de>
Sent: Wednesday, September 8, 2021 4:34 PM
To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
lorenzo.pieralisi@arm.com
Cc: linux-pci@vger.kernel.org; dl-linux-imx <redacted>;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
kernel@pengutronix.de
Subject: Re: [PATCH 3/3] PCI: imx: add compliance tests mode to enable
measure signal quality
Hi Richard,
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
quoted
Refer to the system board signal Quality of PCIe archiecture PHY test
specification. Signal quality tests can be executed with devices in
the polling.compliance state.
To let the device support polling.compliance stat, the clocks and
powers shouldn't be turned off during the compliance tests although
the PHY link might be down.
Add the i.MX PCIe compliance tests mode enable option to keep the and
powers on, and finish the driver probe without error return.
Use the "pcie_cmp_enabled=yes" in kernel command line to enable the
compliance tests mode.
Adding "random" kernel command line options isn't going to fly. If at all, this
should be a module_param so it gets properly namespaced. Also this needs a
more descriptive name, right now this is abbreviating the one thing that would
tell a user what this is about: compliance testing.
[Richard Zhu] Thanks a lot for your review comments.
Okay, I would use the module_param replace the command line option.
And Would add more compliance tests descriptions(what's compliance used for,
and the simple scope of the tests) later.
@@ -748,10 +749,12 @@ static int imx6_pcie_start_link(struct dw_pcie
*pci)
quoted
* started in Gen2 mode, there is a possibility the devices on the * bus will not be detected at all. This happens with PCIe switches. */- tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);- tmp &= ~PCI_EXP_LNKCAP_SLS;- tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;- dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);+ if (!imx6_pcie_cmp_enabled) {+ tmp = dw_pcie_readl_dbi(pci, offset + PCI_EXP_LNKCAP);+ tmp &= ~PCI_EXP_LNKCAP_SLS;+ tmp |= PCI_EXP_LNKCAP_SLS_2_5GB;+ dw_pcie_writel_dbi(pci, offset + PCI_EXP_LNKCAP, tmp);+ } /* Start LTSSM. */ imx6_pcie_ltssm_enable(dev);
@@ -812,9 +815,12 @@ static int imx6_pcie_start_link(struct dw_pcie
*pci)
quoted
dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG0), dw_pcie_readl_dbi(pci, PCIE_PORT_DEBUG1)); imx6_pcie_reset_phy(imx6_pcie);- imx6_pcie_clk_disable(imx6_pcie);- if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0)- regulator_disable(imx6_pcie->vpcie);+ if (!imx6_pcie_cmp_enabled) {+ imx6_pcie_clk_disable(imx6_pcie);+ if (imx6_pcie->vpcie+ && regulator_is_enabled(imx6_pcie->vpcie) > 0)+ regulator_disable(imx6_pcie->vpcie);+ } return ret; }
imx6_pcie_resume_noirq) };+static int __init imx6_pcie_compliance_test_enable(char *str) {+ if (!strcmp(str, "yes")) {+ pr_info("Enable the i.MX PCIe TX/CLK compliance tests mode.\n");+ imx6_pcie_cmp_enabled = 1;+ }+ return 1;+}++__setup("pcie_cmp_enabled=", imx6_pcie_compliance_test_enable);+ static int imx6_pcie_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev;
@@ -1187,8 +1204,16 @@ static int imx6_pcie_probe(struct
platform_device *pdev)
quoted
return ret; ret = dw_pcie_host_init(&pci->pp);- if (ret < 0)+ if (ret < 0) {+ if (imx6_pcie_cmp_enabled) {+ /* The PCIE clocks and powers wouldn't be turned off */+ dev_info(dev, "To do the compliance tests.\n");
This needs a better message, like "Driver loaded with compliance test mode
enabled" and the message needs to be shown unconditionally, not only when
the host init fails. We don't want to have the user track down weird issues
when the compliance test option was specified by accident and the link is
almost working due to this.
[Richard Zhu] Thanks. Ok, got that, would update the message in next version.
Best Regards
Richard Zhu
Regards,
Lucas
quoted
+ ret = 0;+ } else {+ dev_err(dev, "Unable to add pcie port.\n");+ } return ret;+ } if (pci_msi_enabled()) { u8 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_MSI);
From: Lucas Stach <l.stach@pengutronix.de> Date: 2021-09-08 08:47:54
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
quoted hunk
No function changes, just encapsulate the i.MX PCIe clocks enable
operations into one standalone function
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 82 +++++++++++++++++----------
1 file changed, 51 insertions(+), 31 deletions(-)
I don't think this is strictly needed. Can you just move the placement
of the new imx6_pcie_clk_enable function in the file, such that we can
avoid the forward declaration?
From: Richard Zhu <hongxing.zhu@nxp.com> Date: 2021-09-08 08:59:33
-----Original Message-----
From: Lucas Stach <l.stach@pengutronix.de>
Sent: Wednesday, September 8, 2021 4:42 PM
To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
lorenzo.pieralisi@arm.com
Cc: linux-pci@vger.kernel.org; dl-linux-imx <redacted>;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
kernel@pengutronix.de
Subject: Re: [PATCH 2/3] PCI: imx: add err check to host init and fix regulator
dump
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
quoted
Since there is error return check of the host_init callback, add error
check to imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
function.
Because that i.MX PCIe doesn't support the hot-plug feature. To save
power consumption as much as possible, turn off the clocks and power
supplies when the PCIe PHY link is never came up in probe procedure.
When PCIe link is never came up and vpcie regulator is present, there
would be following dump when try to put the regulator.
Disable this regulator to fix this dump when link is never came up.
[ 2.335880] imx6q-pcie 33800000.pcie: Phy link never came up
[ 2.341642] imx6q-pcie: probe of 33800000.pcie failed with error -110
[ 2.348160] ------------[ cut here ]------------
[ 2.352778] WARNING: CPU: 3 PID: 119 at
Why do we need a separate variable for the error code here? Why not just
use the existing ret?
[Richard Zhu] Thanks for your review.
The "ret" variable might be assigned to one "0" by regulator_disable() in the error exit.
So, I use another one "err" variable to avoid this confliction.
How about do not make the err check of regulator_enable/disable, and just use the "ret" for the error code?
if (imx6_pcie->vpcie && regulator_is_enabled(imx6_pcie->vpcie) > 0) {
ret = regulator_disable(imx6_pcie->vpcie);
if (ret)
dev_err(dev, "failed to disable vpcie regulator: %d\n",
ret);
}
quoted
if (imx6_pcie->vpcie && !regulator_is_enabled(imx6_pcie->vpcie)) { ret = regulator_enable(imx6_pcie->vpcie); if (ret) { dev_err(dev, "failed to enable vpcie regulator: %d\n", ret);- return;+ return ret; } }- ret = imx6_pcie_clk_enable(imx6_pcie);- if (ret) {- dev_err(dev, "unable to enable pcie clocks\n");+ err = imx6_pcie_clk_enable(imx6_pcie);+ if (err) {+ dev_err(dev, "unable to enable pcie clocks: %d\n", err); goto err_clks; }
This is a separate fix for the clock enable counts, that isn't mentioned in the
commit message.
It seems like this patch fixes in fact 3 issues: error propagation from host_init,
fix regulator reference handling, fix clk reference handling. Either split up the
patch along those changes (if possible) or improve the commit message to
explain those changes.
[Richard Zhu] My original purpose of this patch is to turn off the clocks
and power supplies( regulators and so on.) when link is down(for example, no EP device connected).
So, I combine the fix of the clock/regulator used counter balance together.
Would improve the commit message to explain these changes later.
You're right, the error propagation from host_init should be separated to another patch.
Thanks a lot 😊.
BR
Richard
From: Richard Zhu <hongxing.zhu@nxp.com> Date: 2021-09-08 09:02:50
-----Original Message-----
From: Lucas Stach <l.stach@pengutronix.de>
Sent: Wednesday, September 8, 2021 4:48 PM
To: Richard Zhu <hongxing.zhu@nxp.com>; bhelgaas@google.com;
lorenzo.pieralisi@arm.com
Cc: linux-pci@vger.kernel.org; dl-linux-imx <redacted>;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
kernel@pengutronix.de
Subject: Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one
standalone function
Am Mittwoch, dem 08.09.2021 um 14:59 +0800 schrieb Richard Zhu:
quoted
No function changes, just encapsulate the i.MX PCIe clocks enable
operations into one standalone function
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
---
drivers/pci/controller/dwc/pci-imx6.c | 82
+++++++++++++++++----------
1 file changed, 51 insertions(+), 31 deletions(-)
I don't think this is strictly needed. Can you just move the placement of the
new imx6_pcie_clk_enable function in the file, such that we can avoid the
forward declaration?
[Richard Zhu] Thanks for your quickly review.
Okay, would move the placement to avoid he forward declaration.
quoted
static int pcie_phy_poll_ack(struct imx6_pcie *imx6_pcie, bool
exp_val) {
struct dw_pcie *pci = imx6_pcie->pci; @@ -498,33 +500,12 @@ static
void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)
}
}
- ret = clk_prepare_enable(imx6_pcie->pcie_phy);
- if (ret) {
- dev_err(dev, "unable to enable pcie_phy clock\n");
- goto err_pcie_phy;
- }
-
- ret = clk_prepare_enable(imx6_pcie->pcie_bus);
+ ret = imx6_pcie_clk_enable(imx6_pcie);
if (ret) {
- dev_err(dev, "unable to enable pcie_bus clock\n");
- goto err_pcie_bus;
+ dev_err(dev, "unable to enable pcie clocks\n");
+ goto err_clks;
}
- ret = clk_prepare_enable(imx6_pcie->pcie);
- if (ret) {
- dev_err(dev, "unable to enable pcie clock\n");
- goto err_pcie;
- }
-
- ret = imx6_pcie_enable_ref_clk(imx6_pcie);
- if (ret) {
- dev_err(dev, "unable to enable pcie ref clock\n");
- goto err_ref_clk;
- }
-
- /* allow the clocks to stabilize */
- usleep_range(200, 500);
-
/* Some boards don't have PCIe reset GPIO. */
if (gpio_is_valid(imx6_pcie->reset_gpio)) {
gpio_set_value_cansleep(imx6_pcie->reset_gpio,
On Wed, Sep 08, 2021 at 02:59:24PM +0800, Richard Zhu wrote:
No function changes, just encapsulate the i.MX PCIe clocks enable
operations into one standalone function
When you update this,
- it's helpful if you include a cover letter with a multi-patch
series, with the patches being replies to the cover letter, and
- please follow the sentence and formatting conventions for subject
lines and commit logs (driver name should match, capitalize
subject line, end sentences with periods, blank lines between
paragraphs, remove useless information like timestamps from log
messages, indent quoted material like logs by two spaces, etc).
@@ -498,33 +500,12 @@ static void imx6_pcie_deassert_core_reset(struct imx6_pcie *imx6_pcie)}}-ret=clk_prepare_enable(imx6_pcie->pcie_phy);-if(ret){-dev_err(dev,"unable to enable pcie_phy clock\n");-gotoerr_pcie_phy;-}--ret=clk_prepare_enable(imx6_pcie->pcie_bus);+ret=imx6_pcie_clk_enable(imx6_pcie);if(ret){-dev_err(dev,"unable to enable pcie_bus clock\n");-gotoerr_pcie_bus;+dev_err(dev,"unable to enable pcie clocks\n");+gotoerr_clks;}-ret=clk_prepare_enable(imx6_pcie->pcie);-if(ret){-dev_err(dev,"unable to enable pcie clock\n");-gotoerr_pcie;-}--ret=imx6_pcie_enable_ref_clk(imx6_pcie);-if(ret){-dev_err(dev,"unable to enable pcie ref clock\n");-gotoerr_ref_clk;-}--/* allow the clocks to stabilize */-usleep_range(200,500);-/* Some boards don't have PCIe reset GPIO. */if(gpio_is_valid(imx6_pcie->reset_gpio)){gpio_set_value_cansleep(imx6_pcie->reset_gpio,
@@ -914,6 +889,51 @@ static void imx6_pcie_pm_turnoff(struct imx6_pcie *imx6_pcie)usleep_range(1000,10000);}+staticintimx6_pcie_clk_enable(structimx6_pcie*imx6_pcie)+{+structdw_pcie*pci=imx6_pcie->pci;+structdevice*dev=pci->dev;+intret;++ret=clk_prepare_enable(imx6_pcie->pcie_phy);+if(ret){+dev_err(dev,"unable to enable pcie_phy clock\n");+returnret;+}++ret=clk_prepare_enable(imx6_pcie->pcie_bus);+if(ret){+dev_err(dev,"unable to enable pcie_bus clock\n");+gotoerr_pcie_bus;+}++ret=clk_prepare_enable(imx6_pcie->pcie);+if(ret){+dev_err(dev,"unable to enable pcie clock\n");+gotoerr_pcie;+}++ret=imx6_pcie_enable_ref_clk(imx6_pcie);+if(ret){+dev_err(dev,"unable to enable pcie ref clock\n");+gotoerr_ref_clk;+}++/* allow the clocks to stabilize */+usleep_range(200,500);+return0;++err_ref_clk:+clk_disable_unprepare(imx6_pcie->pcie);+err_pcie:+clk_disable_unprepare(imx6_pcie->pcie_bus);+err_pcie_bus:+clk_disable_unprepare(imx6_pcie->pcie_phy);++returnret;++}+staticvoidimx6_pcie_clk_disable(structimx6_pcie*imx6_pcie){clk_disable_unprepare(imx6_pcie->pcie);
From: Richard Zhu <hongxing.zhu@nxp.com> Date: 2021-09-09 02:26:40
-----Original Message-----
From: Bjorn Helgaas <helgaas@kernel.org>
Sent: Wednesday, September 8, 2021 11:12 PM
To: Richard Zhu <hongxing.zhu@nxp.com>
Cc: l.stach@pengutronix.de; bhelgaas@google.com;
lorenzo.pieralisi@arm.com; linux-pci@vger.kernel.org; dl-linux-imx
[off-list ref]; linux-arm-kernel@lists.infradead.org;
linux-kernel@vger.kernel.org; kernel@pengutronix.de
Subject: Re: [PATCH 1/3] PCI: imx: encapsulate the clock enable into one
standalone function
On Wed, Sep 08, 2021 at 02:59:24PM +0800, Richard Zhu wrote:
quoted
No function changes, just encapsulate the i.MX PCIe clocks enable
operations into one standalone function
When you update this,
- it's helpful if you include a cover letter with a multi-patch
series, with the patches being replies to the cover letter, and
- please follow the sentence and formatting conventions for subject
lines and commit logs (driver name should match, capitalize
subject line, end sentences with periods, blank lines between
paragraphs, remove useless information like timestamps from log
messages, indent quoted material like logs by two spaces, etc).
[Richard Zhu] Ok, got that. Thanks for your kindly reminder.
Would use the cover letter, and reformat the subject lines and commit logs later.
Best Regards
Richard Zhu