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

[Lorenzo Bianconi <lorenzo@kernel.org>]

[-- 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 --]