[PATCH v2 3/8] imx8mp: power-domain: Add PCIe support

[Marek Vasut]

On 2/26/24 9:04 AM, Sumit Garg wrote:
> Add support for GPCv2 power domains and clock handling for PCIe and
> PCIe PHY.
> 
> Signed-off-by: Sumit Garg <sumit.garg at linaro.org>
> ---
>   drivers/power/domain/imx8mp-hsiomix.c | 101 ++++++++++++++++++++------
>   1 file changed, 78 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/power/domain/imx8mp-hsiomix.c b/drivers/power/domain/imx8mp-hsiomix.c
> index e2d772c5ec78..58cc3f63bb56 100644
> --- a/drivers/power/domain/imx8mp-hsiomix.c
> +++ b/drivers/power/domain/imx8mp-hsiomix.c
> @@ -16,48 +16,73 @@
>   #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)
>   {
>   	struct udevice *dev = power_domain->dev;
>   	struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
> -	struct power_domain *domain;
> +	struct power_domain *domain = NULL;
> +	struct clk *clk = NULL;
> +	u32 gpr_reg0_bits = 0;
>   	int ret;
>   
> -	ret = power_domain_on(&priv->pd_bus);
> -	if (ret)
> -		return ret;
> -
> -	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB) {
> +	switch (power_domain->id) {
> +	case IMX8MP_HSIOBLK_PD_USB:
>   		domain = &priv->pd_usb;
> -	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY1) {
> +		clk = &priv->clk_usb;
> +		gpr_reg0_bits |= USB_CLOCK_MODULE_EN;
> +		break;
> +	case IMX8MP_HSIOBLK_PD_USB_PHY1:
>   		domain = &priv->pd_usb_phy1;
> -	} else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2) {
> +		break;
> +	case IMX8MP_HSIOBLK_PD_USB_PHY2:
>   		domain = &priv->pd_usb_phy2;
> -	} else {
> -		ret = -EINVAL;
> -		goto err_pd;
> +		break;
> +	case IMX8MP_HSIOBLK_PD_PCIE:
> +		domain = &priv->pd_pcie;
> +		clk = &priv->clk_pcie;
> +		gpr_reg0_bits |= PCIE_CLOCK_MODULE_EN;
> +		break;
> +	case IMX8MP_HSIOBLK_PD_PCIE_PHY:
> +		domain = &priv->pd_pcie_phy;
> +		/* Bits to deassert PCIe PHY reset */
> +		gpr_reg0_bits |= PCIE_PHY_APB_RST | PCIE_PHY_INIT_RST;
> +		break;
> +	default:
> +		dev_err(dev, "unknown power domain id: %ld\n",
> +			power_domain->id);
> +		return -EINVAL;
>   	}
>   
> +	ret = power_domain_on(&priv->pd_bus);
> +	if (ret)
> +		return ret;
> +
>   	ret = power_domain_on(domain);
>   	if (ret)
>   		goto err_pd;
>   
> -	ret = clk_enable(&priv->clk_usb);
> -	if (ret)
> -		goto err_clk;
> +	if (clk) {
> +		ret = clk_enable(clk);
> +		if (ret)
> +			goto err_clk;
> +	}
>   
> -	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
> -		setbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
> +	setbits_le32(priv->base + GPR_REG0, gpr_reg0_bits);

Are you sure it is OK to do this unconditionally ?

Why not this:

if (gpr_reg0_bits)
   setbits_le32(priv->base + GPR_REG0, gpr_reg0_bits);

>   	return 0;
>   
> @@ -73,17 +98,31 @@ static int imx8mp_hsiomix_off(struct power_domain *power_domain)
>   	struct udevice *dev = power_domain->dev;
>   	struct imx8mp_hsiomix_priv *priv = dev_get_priv(dev);
>   
> -	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
> +	switch (power_domain->id) {
> +	case IMX8MP_HSIOBLK_PD_USB:
>   		clrbits_le32(priv->base + GPR_REG0, USB_CLOCK_MODULE_EN);
> -
> -	clk_disable(&priv->clk_usb);
> -
> -	if (power_domain->id == IMX8MP_HSIOBLK_PD_USB)
> +		clk_disable(&priv->clk_usb);
>   		power_domain_off(&priv->pd_usb);
> -	else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY1)
> +		break;
> +	case IMX8MP_HSIOBLK_PD_USB_PHY1:
>   		power_domain_off(&priv->pd_usb_phy1);
> -	else if (power_domain->id == IMX8MP_HSIOBLK_PD_USB_PHY2)
> +		break;
> +	case IMX8MP_HSIOBLK_PD_USB_PHY2:
>   		power_domain_off(&priv->pd_usb_phy2);
> +		break;
> +	case IMX8MP_HSIOBLK_PD_PCIE:

Maybe we need some function ... imx8mp_hsiomix_id_to_pd() to avoid this 
duplicate switch statement:

pd = imx8mp_hsiomix_id_to_pd(...)
...
power_domain_off(fd);

> +		clrbits_le32(priv->base + GPR_REG0, PCIE_CLOCK_MODULE_EN);
> +		clk_disable(&priv->clk_pcie);
> +		power_domain_off(&priv->pd_pcie);
> +		break;
> +	case IMX8MP_HSIOBLK_PD_PCIE_PHY:
> +		clrbits_le32(priv->base + GPR_REG0, PCIE_PHY_APB_RST |
> +						    PCIE_PHY_INIT_RST);
> +		power_domain_off(&priv->pd_pcie_phy);

Is it OK to turn off the bus PD after this ?
Please double-check if power_domain_on/off() is refcounted .

> +		break;
> +	default:
> +		break;
> +	}
>   
>   	power_domain_off(&priv->pd_bus);
>   
> @@ -109,6 +148,10 @@ static int imx8mp_hsiomix_probe(struct udevice *dev)
>   	if (ret < 0)
>   		return ret;
>   
> +	ret = clk_get_by_name(dev, "pcie", &priv->clk_pcie);
> +	if (ret < 0)
> +		return ret;
> +
>   	ret = power_domain_get_by_name(dev, &priv->pd_bus, "bus");
>   	if (ret < 0)
>   		return ret;

Some clk_put() seems to be missing for clk_pcie .