Il 01/03/24 07:42, Siddharth Vadapalli ha scritto:
On Thu, Feb 29, 2024 at 10:24:49AM +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@xxxxxxxxxxxxx>
---
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);
Wouldn't this power off the shared PHY? Or will the PHY driver make this
NO-OP if the PHY is shared, in which case the above two statements could
be combined with the other statements in the:
if (is_suspend && suspend_reset_supported)
condition to get a single block of code that also combines the
reset_control_assert(pcie->phy_reset)
present below.
No, that'd be fine:
static int mtk_phy_power_off(struct phy *phy)
{
struct mtk_phy_instance *instance = phy_get_drvdata(phy);
struct mtk_tphy *tphy = dev_get_drvdata(phy->dev.parent);
if (instance->type == PHY_TYPE_USB2)
u2_phy_instance_power_off(tphy, instance);
else if (instance->type == PHY_TYPE_PCIE)
pcie_phy_instance_power_off(tphy, instance);
return 0;
}
...it's two different PHY instances that we're dealing with, here :-)
Cheers,
Angelo
- reset_control_assert(pcie->phy_reset);
+ if (is_suspend && suspend_reset_supported)
+ reset_control_assert(pcie->phy_reset);
}
...
Regards,
Siddharth.
