On Tue, 20 Feb 2024 at 21:02, Marek Vasut marex@denx.de wrote:
On 2/20/24 14:10, Sumit Garg wrote:
Pre-requisite to enable PCIe support on iMX8MP SoC.
This commit message is useless, write a proper one.
How about the following?
Add support for GPCv2 power domains and clock handling for PCIe and PCIe PHY. It is required to enable PCIe support on the iMX8MP SoC.
Signed-off-by: Sumit Garg sumit.garg@linaro.org
drivers/power/domain/imx8mp-hsiomix.c | 50 +++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-)
diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c index e2d772c5ec7..62145e0261b 100644 --- a/drivers/power/domain/imx8mp-hsiomix.c +++ b/drivers/power/domain/imx8mp-hsiomix.c @@ -16,14 +16,19 @@ #define GPR_REG0 0x0 #define PCIE_CLOCK_MODULE_EN BIT(0) #define USB_CLOCK_MODULE_EN BIT(1) +#define PCIE_PHY_APB_RST BIT(4) +#define PCIE_PHY_INIT_RST BIT(5)
struct imx8mp_hsiomix_priv { void __iomem *base; struct clk clk_usb;
struct clk clk_pcie; struct power_domain pd_bus; struct power_domain pd_usb;struct power_domain pd_pcie; struct power_domain pd_usb_phy1; struct power_domain pd_usb_phy2;struct power_domain pd_pcie_phy;};
static int imx8mp_hsiomix_on(struct power_domain *power_domain)
@@ -43,6 +48,10 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain) domain = &priv->pd_usb_phy1; } else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) { domain = &priv->pd_usb_phy2;
} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE) {domain = &priv->pd_pcie;} else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY) {domain = &priv->pd_pcie_phy; } else { ret = -EINVAL; goto err_pd;@@ -54,14 +63,25 @@ static int imx8mp_hsiomix_on(struct power_domain *power_domain)
ret = clk_enable(&priv->clk_usb); if (ret)
goto err_clk;
goto err_clk_usb;ret = clk_enable(&priv->clk_pcie);if (ret)goto err_clk_pcie;Does this mean that when USB power domains get enabled, PCIe clock are also enabled ? Why ?
What if the PCIe clock enable fails, do USB clock remain enabled ?
Let me gate them behind corresponding power domain IDs.
if (power_domain->id == IMX8MP_HSIOBLK_PD_USB) setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE)setbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);else if (power_domain->id == IMX8MP_HSIOBLK_PD_PCIE_PHY)setbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |PCIE_PHY_INIT_RST);Shouldn't the reset bits be cleared here ?
Although I can't find their reference in the TRM, as per Linux commit [1], setting the reset bit is actually deassertion of PCIe PHY reset. You can think of it like an active low signal.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...
-Sumit