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

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> On Mo, 2024-11-18 at 10:03 +0100, Lorenzo Bianconi wrote:
> > On Nov 18, Philipp Zabel wrote:
> > > On Mo, 2024-11-18 at 09:04 +0100, Lorenzo Bianconi wrote:
> > > > In order to make the code more readable, the reset_control_bulk_assert()
> > > > for PHY reset lines is moved to make it pair with
> > > > reset_control_bulk_deassert() in mtk_pcie_power_up() and
> > > > mtk_pcie_en7581_power_up(). The same change is done for
> > > > reset_control_assert() used to assert MAC reset line.
> > > > 
> > > > Introduce PCIE_MTK_RESET_TIME_US macro for the time needed to
> > > > complete PCIe reset on MediaTek controller.
> > > > 
> > > > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > ---
> > > >  drivers/pci/controller/pcie-mediatek-gen3.c | 27 +++++++++++++++++++--------
> > > >  1 file changed, 19 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > index 3cfcb45d31508142d28d338ff213f70de9b4e608..2b80edd4462ad4e9f2a5d192db7f99307113eb8a 100644
> > > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > @@ -125,6 +125,8 @@
> > > >  
> > > >  #define MAX_NUM_PHY_RESETS		3
> > > >  
> > > > +#define PCIE_MTK_RESET_TIME_US		10
> > > > +
> > > >  /* Time in ms needed to complete PCIe reset on EN7581 SoC */
> > > >  #define PCIE_EN7581_RESET_TIME_MS	100
> > > >  
> > > > @@ -912,6 +914,14 @@ 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);
> > > > +
> > > >  	/*
> > > >  	 * Wait for the time needed to complete the bulk assert in
> > > >  	 * mtk_pcie_setup for EN7581 SoC.
> > > 
> > > This comment is not correct anymore.
> > 
> > I agree naming is hard, but I guess we can assume with 'bulk' we refer to both
> > phy and mac reset (similar to what we have in mtk_pcie_power_up()),
> > what do you think?
> 
> My point is that the referenced (bulk) assert isn't in mtk_pcie_setup()
> anymore - it was just moved right above this comment.

ok, thx. I got what you mean now, I will fix it.

> 
> I wonder why that delay is required at all, though. Does the reset
> controller driver return from reset_control_ops::assert before the
> reset line to the PCI controller is asserted?

We discussed it in a previous revision and Bjorn requested to have the required delaies 
in the PCIe driver.

https://patchwork.kernel.org/project/linux-pci/patch/aca00bd672ee576ad96d279414fc0835ff31f637.1720022580.git.lorenzo@kernel.org/#26110282

Btw this the same approach we had in mtk_pcie_setup() before this series (usleep_range(10, 20)).

Regards,
Lorenzo

> 
> regards
> Philipp

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