Re: [PATCH 3/3] PCI: mediatek-gen3: Move reset/assert callbacks in .power_up()

Lorenzo Bianconi
2024-11-07T16:19:29Z

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

> On Thu, Nov 07, 2024 at 02:50:55PM +0100, Lorenzo Bianconi wrote:
> > In order to make the code more readable, move phy and mac reset lines
> > assert/de-assert configuration in .power_up callback
> > (mtk_pcie_en7581_power_up/mtk_pcie_power_up).
> >
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> > drivers/pci/controller/pcie-mediatek-gen3.c | 24 ++++++++++++++++--------
> > 1 file changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > index 8c8c733a145634cdbfefd339f4a692f25a6e24de..c0127d0fb4f059b9f9e816360130e183e8f0e990 100644
> > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > @@ -867,6 +867,13 @@ static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie)
> > int err;
> > u32 val;
> >
> > + /*
> > + * 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_bulk_assert(pcie->soc->phy_resets.num_resets,
> > + pcie->phy_resets);
> > + reset_control_assert(pcie->mac_reset);
>
> Add blank line here.

ack, I will fix it.

>
> > /*
> > * Wait for the time needed to complete the bulk assert in
> > * mtk_pcie_setup for EN7581 SoC.
> > @@ -941,6 +948,15 @@ static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie)
> > struct device *dev = pcie->dev;
> > int err;
> >
> > + /*
> > + * 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_bulk_assert(pcie->soc->phy_resets.num_resets,
> > + pcie->phy_resets);
> > + reset_control_assert(pcie->mac_reset);
> > + usleep_range(10, 20);
>
> Unrelated to this patch, but since you're moving it, do you know what
> this delay is for? Can we add a #define and a spec citation for it?

I am not sure about it, this was already there.
@Jianjun Wang: any input on it?

>
> Is there a requirement that the PHY and MAC reset ordering be
> different for EN7581 vs other chips?
>
> EN7581:
>
> assert PHY reset
> assert MAC reset
> power on PHY
> deassert PHY reset
> deassert MAC reset
>
> others:
>
> assert PHY reset
> assert MAC reset
> deassert PHY reset
> power on PHY
> deassert MAC reset
>
> Is there one order that would work for both?

EN7581 requires to run phy_init()/phy_power_on() before deassert PHY reset
lines.

Regards,
Lorenzo

>
> > /* PHY power on and enable pipe clock */
> > err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> > if (err) {
> > @@ -1013,14 +1029,6 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
> > * counter since the bulk is shared.
> > */
> > reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> > - /*
> > - * 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_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets);
> > -
> > - reset_control_assert(pcie->mac_reset);
> > - usleep_range(10, 20);
> >
> > /* Don't touch the hardware registers before power up */
> > err = pcie->soc->power_up(pcie);
> >
> > --
> > 2.47.0
> >

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