Messages in this thread | ![/](/images/icornerl.gif) | | Date | Wed, 6 Mar 2024 09:50:10 +0100 | Subject | Re: [PATCH v2] PCI: mediatek-gen3: Assert MAC reset only if PHY reset also present | From | AngeloGioacchino Del Regno <> |
| |
Il 01/03/24 03:48, Jianjun Wang (王建军) ha scritto: > Hi Angelo, > > Thanks for your patch. > > On Thu, 2024-02-29 at 10:24 +0100, AngeloGioacchino Del Regno wrote: >> Some SoCs have two PCI-Express controllers: in the case of MT8195, >> one of them is using a dedicated PHY, but the other uses a combo PHY >> that is shared with USB and in that case the PHY cannot be reset >> from the PCIe driver, or USB functionality will be unable to resume. >> >> Resetting the PCIe MAC without also resetting the PHY will result in >> a full system lockup at PCIe resume time and the only option to >> resume operation is to hard reboot the system (with a PMIC cut-off). >> >> To resolve this issue, check if we've got both a PHY and a MAC reset >> and, if not, never assert resets at PM suspend time: in that case, >> the link is still getting powered down as both the clocks and the >> power domains will go down anyway. >> >> Fixes: d537dc125f07 ("PCI: mediatek-gen3: Add system PM support") >> Signed-off-by: AngeloGioacchino Del Regno < >> angelogioacchino.delregno@collabora.com> >> --- >> >> Changes in v2: >> - Rebased over next-20240229 >> >> drivers/pci/controller/pcie-mediatek-gen3.c | 25 ++++++++++++++----- >> -- >> 1 file changed, 17 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c >> b/drivers/pci/controller/pcie-mediatek-gen3.c >> index 975b3024fb08..99b5d7a49be1 100644 >> --- a/drivers/pci/controller/pcie-mediatek-gen3.c >> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c >> @@ -874,17 +874,26 @@ static int mtk_pcie_power_up(struct >> mtk_gen3_pcie *pcie) >> return err; >> } >> >> -static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie) >> +static void mtk_pcie_power_down(struct mtk_gen3_pcie *pcie, bool >> is_suspend) >> { >> + bool suspend_reset_supported = pcie->mac_reset && pcie- >>> phy_reset; >> + >> clk_bulk_disable_unprepare(pcie->num_clks, pcie->clks); >> >> pm_runtime_put_sync(pcie->dev); >> pm_runtime_disable(pcie->dev); >> - reset_control_assert(pcie->mac_reset); >> + >> + /* >> + * Assert MAC reset only if we also got a PHY reset, otherwise >> + * the system will lockup at PM resume time. >> + */ >> + if (is_suspend && suspend_reset_supported) >> + reset_control_assert(pcie->mac_reset); >> >> phy_power_off(pcie->phy); >> phy_exit(pcie->phy); >> - reset_control_assert(pcie->phy_reset); >> + if (is_suspend && suspend_reset_supported) >> + reset_control_assert(pcie->phy_reset); >> } >> >> static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie) >> @@ -920,7 +929,7 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie >> *pcie) >> return 0; >> >> err_setup: >> - mtk_pcie_power_down(pcie); >> + mtk_pcie_power_down(pcie, false); >> >> return err; >> } >> @@ -951,7 +960,7 @@ static int mtk_pcie_probe(struct platform_device >> *pdev) >> err = pci_host_probe(host); >> if (err) { >> mtk_pcie_irq_teardown(pcie); >> - mtk_pcie_power_down(pcie); >> + mtk_pcie_power_down(pcie, false); >> return err; >> } >> >> @@ -969,7 +978,7 @@ static void mtk_pcie_remove(struct >> platform_device *pdev) >> pci_unlock_rescan_remove(); >> >> mtk_pcie_irq_teardown(pcie); >> - mtk_pcie_power_down(pcie); >> + mtk_pcie_power_down(pcie, false); > > Is there any reason not to reset the MAC and PHY when probe fails and > driver removing? Some SoCs may not have MTCMOS to cut off their power, > we need to assert the reset signal to save power in that case. >
Sorry for the late reply - yes, there is a reason.
On platforms needing this quirk, resetting at .remove() time will hang the machine if the module is reinserted later (hence, .probe() called at a later time).
Regards, Angelo
> Thanks. > >> } >> >> static void mtk_pcie_irq_save(struct mtk_gen3_pcie *pcie) >> @@ -1044,7 +1053,7 @@ static int mtk_pcie_suspend_noirq(struct device >> *dev) >> dev_dbg(pcie->dev, "entered L2 states successfully"); >> >> mtk_pcie_irq_save(pcie); >> - mtk_pcie_power_down(pcie); >> + mtk_pcie_power_down(pcie, true); >> >> return 0; >> } >> @@ -1060,7 +1069,7 @@ static int mtk_pcie_resume_noirq(struct device >> *dev) >> >> err = mtk_pcie_startup_port(pcie); >> if (err) { >> - mtk_pcie_power_down(pcie); >> + mtk_pcie_power_down(pcie, false); >> return err; >> } >>
| ![\](/images/icornerr.gif) |