Re: [PATCH v4 4/4] PCI: mediatek-gen3: Add Airoha EN7581 support

[Bjorn Helgaas <helgaas@kernel.org>]

On Wed, Jul 03, 2024 at 06:12:44PM +0200, Lorenzo Bianconi wrote:
> Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3
> PCIe controller driver.
> ...

> +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> +{
> +	struct device *dev = pcie->dev;
> +	int err;
> +	u32 val;
> +
> +	/*
> +	 * Wait for the time needed to complete the bulk assert in
> +	 * mtk_pcie_setup for EN7581 SoC.
> +	 */
> +	mdelay(PCIE_EN7581_RESET_TIME_MS);

It looks wrong to me to do the assert and deassert in different
places:

  mtk_pcie_setup
    reset_control_bulk_assert(pcie->phy_resets)        <--
    mtk_pcie_en7581_power_up
      mdelay(PCIE_EN7581_RESET_TIME_MS)
      reset_control_bulk_deassert(pcie->phy_resets)    <--
      mdelay(PCIE_EN7581_RESET_TIME_MS)

That makes the code hard to understand.

> +	err = phy_init(pcie->phy);
> +	if (err) {
> +		dev_err(dev, "failed to initialize PHY\n");
> +		return err;
> +	}
> +
> +	err = phy_power_on(pcie->phy);
> +	if (err) {
> +		dev_err(dev, "failed to power on PHY\n");
> +		goto err_phy_on;
> +	}
> +
> +	err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> +	if (err) {
> +		dev_err(dev, "failed to deassert PHYs\n");
> +		goto err_phy_deassert;
> +	}
> +
> +	/*
> +	 * Wait for the time needed to complete the bulk de-assert above.
> +	 * This time is specific for EN7581 SoC.
> +	 */
> +	mdelay(PCIE_EN7581_RESET_TIME_MS);
> +
> +	pm_runtime_enable(dev);
> +	pm_runtime_get_sync(dev);
> +

> +	err = clk_bulk_prepare(pcie->num_clks, pcie->clks);
> +	if (err) {
> +		dev_err(dev, "failed to prepare clock\n");
> +		goto err_clk_prepare;
> +	}
> +
> +	val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) |
> +	      FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) |
> +	      FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) |
> +	      FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41);
> +	writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG);
> +
> +	val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT |
> +	      FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) |
> +	      FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) |
> +	      FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf);
> +	writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG);

Why is this equalization stuff in the middle between
clk_bulk_prepare() and clk_bulk_enable()?  Is the split an actual
requirement, or could we use clk_bulk_prepare_enable() here, like we
do in mtk_pcie_power_up()?

If the split is required, a comment about why would be helpful.

> +	err = clk_bulk_enable(pcie->num_clks, pcie->clks);
> +	if (err) {
> +		dev_err(dev, "failed to prepare clock\n");
> +		goto err_clk_enable;
> +	}

Per https://lore.kernel.org/r/ZypgYOn7dcYIoW4i@lore-desk,
REG_PCI_CONTROL is asserted/deasserted here by en7581_pci_enable().

Is this where PERST# is asserted?  If so, a comment to that effect
would be helpful.  Where is PERST# deasserted?  Where are the required
delays before deassert done?

Bjorn