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

Bjorn Helgaas
2024-11-07T15:27:08Z

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.

> /*
> * 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?

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?

> /* 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
>