> 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);
Hi Bjorn,
>
> 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.
The phy reset line was already asserted running reset_control_assert() in
mtk_pcie_setup() and de-asserted running reset_control_deassert() in
mtk_pcie_power_up() before adding EN7581 support. Moreover, EN7581 requires
to run phy_init()/phy_power_on() before de-asserting the phy reset lines.
I guess I can add a comment to make it more clear. Agree?
>
> > + 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()?
Nope, we can replace clk_bulk_enable() with clk_bulk_prepare_enable() and
remove clk_bulk_prepare() in mtk_pcie_en7581_power_up() since we actually
implements just enable callback for EN7581 in clk-en7523.c.
>
> 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().
correct
>
> 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?
I can add a comment in en7581_pci_enable() describing the PERST issue for
EN7581. Please note we have a 250ms delay in en7581_pci_enable() after
configuring REG_PCI_CONTROL register.
https://github.com/torvalds/linux/blob/master/drivers/clk/clk-en7523.c#L396
Regards,
Lorenzo
>
> Bjorn
Attachment:
signature.asc
Description: PGP signature
