Re: [PATCH 3/4] PCI: mediatek-gen3: rely on reset_bulk APIs for phy reset lines

Lorenzo Bianconi
2024-06-21T21:43:21Z

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

> On Fri, Jun 21, 2024 at 04:48:49PM +0200, Lorenzo Bianconi wrote:
> > Use reset_bulk APIs to manage phy reset lines. This is a preliminary
> > patch in order to add Airoha EN7581 pcie support.
>
> If you have occasion to revise this:
>
> s/rely/Rely/ in subject
> s/phy/PHY/ in subject and commit log
> s/pcie/PCIe/ in commit log

ack, I will fix them in v2

>
> > @@ -912,7 +927,13 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
> > * The controller may have been left out of reset by the bootloader
> > * so make sure that we get a clean start by asserting resets here.
> > */
> > - reset_control_assert(pcie->phy_reset);
> > + reset_control_bulk_deassert(pcie->soc->phy_resets.num_rsts,
> > + pcie->phy_resets);
> > + usleep_range(5000, 10000);
> > + reset_control_bulk_assert(pcie->soc->phy_resets.num_rsts,
> > + pcie->phy_resets);
> > + msleep(100);
>
> Where did these usleep and msleep numbers come from? They should use
> a #define that we can connect back to a spec.

I think we can get rid of the first usleep_range() since we need to
deassert the line just to avoid unbalance in deassert_count counter since the
reset line is shared (the line is actually already de-assert). I will add a
comment to clarify it.

>
> These delays should also be mentioned in the commit log because it
> appears unrelated to the conversion to the reset_bulk API. Actually,
> it would be even better if they were in a separate patch, since it
> looks like a logically separate change.

Regarding the msleep(100), it is not documented in the vendor sdk, I think it
necessary to complete the reset before initialize the pcie-phy. Since it is
required just for EN7581, I guess we can move it in mtk_pcie_en7581_power_up()
(in patch 4/4) before the phy_init(). What do you think?

Regards,
Lorenzo

>
> > reset_control_assert(pcie->mac_reset);
> > usleep_range(10, 20);
>
> Unrelated to this patch, but it would be nice to have an explanation
> of this existing delay, too.
>
> Bjorn

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