Re: [PATCH] PCI: mediatek-gen3: Avoid PCIe resetting for Airoha EN7581 SoC

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Sep 20, 2024 at 10:26:28AM +0200, Lorenzo Bianconi wrote:
> The PCIe controller available on the EN7581 SoC does not support reset
> via the following lines:
> - PCIE_MAC_RSTB
> - PCIE_PHY_RSTB
> - PCIE_BRG_RSTB
> - PCIE_PE_RSTB
> 
> Introduce the reset callback in order to avoid resetting the PCIe port
> for Airoha EN7581 SoC.
> 
> Tested-by: Hui Ma <hui.ma@airoha.com>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
>  drivers/pci/controller/pcie-mediatek-gen3.c | 44 ++++++++++++++++++-----------
>  1 file changed, 28 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> index 5c19abac74e8..9cea67e92d98 100644
> --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> @@ -128,10 +128,12 @@ struct mtk_gen3_pcie;
>  /**
>   * struct mtk_gen3_pcie_pdata - differentiate between host generations
>   * @power_up: pcie power_up callback
> + * @reset: pcie reset callback
>   * @phy_resets: phy reset lines SoC data.
>   */
>  struct mtk_gen3_pcie_pdata {
>  	int (*power_up)(struct mtk_gen3_pcie *pcie);
> +	void (*reset)(struct mtk_gen3_pcie *pcie);
>  	struct {
>  		const char *id[MAX_NUM_PHY_RESETS];
>  		int num_resets;
> @@ -373,6 +375,28 @@ static void mtk_pcie_enable_msi(struct mtk_gen3_pcie *pcie)
>  	writel_relaxed(val, pcie->base + PCIE_INT_ENABLE_REG);
>  }
>  
> +static void mtk_pcie_reset(struct mtk_gen3_pcie *pcie)
> +{
> +	u32 val;
> +
> +	/* Assert all reset signals */
> +	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> +	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> +	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +
> +	/*
> +	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> +	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> +	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> +	 * for the power and clock to become stable.
> +	 */
> +	msleep(100);

I see you're just moving this, but it's a good chance to use
PCIE_T_PVPERL_MS.

> +
> +	/* De-assert reset signals */
> +	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> +	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +}
> +
>  static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  {
>  	struct resource_entry *entry;
> @@ -402,22 +426,9 @@ static int mtk_pcie_startup_port(struct mtk_gen3_pcie *pcie)
>  	val |= PCIE_DISABLE_DVFSRC_VLT_REQ;
>  	writel_relaxed(val, pcie->base + PCIE_MISC_CTRL_REG);
>  
> -	/* Assert all reset signals */
> -	val = readl_relaxed(pcie->base + PCIE_RST_CTRL_REG);
> -	val |= PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB;
> -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> -
> -	/*
> -	 * Described in PCIe CEM specification sections 2.2 (PERST# Signal)
> -	 * and 2.2.1 (Initial Power-Up (G3 to S0)).
> -	 * The deassertion of PERST# should be delayed 100ms (TPVPERL)
> -	 * for the power and clock to become stable.
> -	 */
> -	msleep(100);
> -
> -	/* De-assert reset signals */
> -	val &= ~(PCIE_MAC_RSTB | PCIE_PHY_RSTB | PCIE_BRG_RSTB | PCIE_PE_RSTB);
> -	writel_relaxed(val, pcie->base + PCIE_RST_CTRL_REG);
> +	/* Reset the PCIe port if requested by the hw */

I don't see any real "request" from the hardware.  IIUC, this is more
like "assert reset if this hardware supports it".

> +	if (pcie->soc->reset)
> +		pcie->soc->reset(pcie);
>  
>  	/* Check if the link is up or not */
>  	err = readl_poll_timeout(pcie->base + PCIE_LINK_STATUS_REG, val,
> @@ -1207,6 +1218,7 @@ static const struct dev_pm_ops mtk_pcie_pm_ops = {
>  
>  static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = {
>  	.power_up = mtk_pcie_power_up,
> +	.reset = mtk_pcie_reset,
>  	.phy_resets = {
>  		.id[0] = "phy",
>  		.num_resets = 1,
> 
> ---
> base-commit: f2024903cb387971abdbc6398a430e735a9b394c
> change-id: 20240920-pcie-en7581-rst-fix-8161658c13c4
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo@kernel.org>
>