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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6387 bytes --]

> On Wed, Nov 06, 2024 at 11:40:28PM +0100, Lorenzo Bianconi wrote:
> > > 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.
> > 
> > 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?
> 
> I assume the first deassert(phy_resets) in mtk_pcie_setup() is not
> paired with anything in this driver.

correct

> 
> I think it would be better to pair the other assert/deasserts in the
> same functions like the below.  Then it's easy to see the matching.

ack, I will post a fix for it

> 
> While looking at this, I noticed that we assert(mac_reset) in
> mtk_pcie_setup(), but it's never deasserted for EN7581.

ack, I will post a fix for it

> 
>   mtk_pcie_setup
>     reset_control_bulk_deassert(phy_resets)
>     mtk_pcie_en7581_power_up
>       reset_control_bulk_assert(phy_resets)  # move here
>       reset_control_assert(mac_reset)        # move here
>       mdelay(PCIE_EN7581_RESET_TIME_MS)
>       phy_init
>       phy_power_on
>       reset_control_deassert(mac_reset)      # add; seems missing?
>       reset_control_bulk_deassert(phy_resets)
>       mdelay(PCIE_EN7581_RESET_TIME_MS)
> 
>   mtk_pcie_setup
>     reset_control_bulk_deassert(phy_resets)
>     mtk_pcie_power_up
>       reset_control_bulk_assert(phy_resets)  # move here
>       reset_control_assert(mac_reset)        # move here
>       reset_control_bulk_deassert(phy_resets)
>       phy_init
>       phy_power_on
>       reset_control_deassert(mac_reset)
> 
> > > > +	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
> 
> Does that 250ms delay correspond to a PCIe mandatory delay, e.g.,
> something like PCIE_T_PVPERL_MS?  I think it would be nice to have the
> required PCI delays in this driver if possible so it's easy to verify
> that they are all covered.

IIRC I just used the delay value used in the vendor sdk. I do not have a strong
opinion about it but I guess if we move it in the pcie-mediatek-gen3 driver, we
will need to add it in each driver where this clock is used. What do you think?

Regards,
Lorenzo

> 
> Bjorn

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]