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
