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

[Lorenzo Bianconi]

On Nov 15, Manivannan Sadhasivam wrote:
> On Fri, Nov 15, 2024 at 10:15:07AM +0100, Lorenzo Bianconi wrote:
> > > On Sat, Nov 09, 2024 at 10:28:39AM +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()).
> > > > 
> > > 
> > > I don't understand how moving the code (duplicting it also) makes the code more
> > > readable. Could you please explain?
> > 
> > Hi Manivannan,
> > 
> > this has been requested by Bjorn in
> > https://patchwork.kernel.org/project/linux-pci/patch/aca00bd672ee576ad96d279414fc0835ff31f637.1720022580.git.lorenzo@kernel.org/#26110282
> > 
> 
> Ok. The description needs to state the reason i.e., the
> reset_control_bulk_assert() is moved to make it pair with
> reset_control_bulk_deassert() in mtk_pcie_setup() and
> mtk_pcie_en7581_power_up().

ack, I will update the commit log.

> 
> Btw, could you explain why reset_control_bulk_deassert() is present in
> mtk_pcie_setup()?

it is because the clock is shared for Airoha EN7581 and we need to be sure
rstc->deassert_count is not 0 running reset_control_bulk_assert().

https://github.com/torvalds/linux/blob/master/drivers/reset/core.c#L483

Regards,
Lorenzo

> 
> - Mani
> 
> > > 
> > > > Introduce PCIE_MTK_RESET_TIME_US macro for the time needed to
> > > > complete PCIe reset on MediaTek controller.
> > > > 
> > > > Signed-off-by: Lorenzo Bianconi <lorenzo at kernel.org>
> > > > ---
> > > >  drivers/pci/controller/pcie-mediatek-gen3.c | 28 ++++++++++++++++++++--------
> > > >  1 file changed, 20 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > index 8c8c733a145634cdbfefd339f4a692f25a6e24de..1ad93d2407810ba873d9a16da96208b3cc0c3011 100644
> > > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c
> > > > @@ -120,6 +120,9 @@
> > > >  
> > > >  #define MAX_NUM_PHY_RESETS		3
> > > >  
> > > > +/* Time in us needed to complete PCIe reset on MediaTek controller */
> > > 
> > > No need of this comment. Macro name itself is explanatory.
> > 
> > ack, I will fix it.
> > 
> > Regards,
> > Lorenzo
> > 
> > > 
> > > - Mani
> > > 
> > > > +#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
> > > >  
> > > > @@ -867,6 +870,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.
> > > > @@ -941,6 +952,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(PCIE_MTK_RESET_TIME_US, 2 * PCIE_MTK_RESET_TIME_US);
> > > > +
> > > >  	/* 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 +1033,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
> > > > 
> > > 
> > > -- 
> > > மணிவண்ணன் சதாசிவம்
> 
> 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-mediatek/attachments/20241115/f219ee09/attachment.sig>