Re: [PATCH 3/4] PCI: mediatek-gen3: rely on reset_bulk APIs for phy reset lines

[Lorenzo Bianconi <lorenzo@kernel.org>]

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

> On Fri, Jun 21, 2024 at 04:48:49PM +0200, Lorenzo Bianconi wrote:
> > Use reset_bulk APIs to manage phy reset lines. This is a preliminary
> > patch in order to add Airoha EN7581 pcie support.
> 
> If you have occasion to revise this:
> 
>   s/rely/Rely/ in subject
>   s/phy/PHY/ in subject and commit log
>   s/pcie/PCIe/ in commit log

ack, I will fix them in v2

> 
> > @@ -912,7 +927,13 @@ static int mtk_pcie_setup(struct mtk_gen3_pcie *pcie)
> >  	 * 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_assert(pcie->phy_reset);
> > +	reset_control_bulk_deassert(pcie->soc->phy_resets.num_rsts,
> > +				    pcie->phy_resets);
> > +	usleep_range(5000, 10000);
> > +	reset_control_bulk_assert(pcie->soc->phy_resets.num_rsts,
> > +				  pcie->phy_resets);
> > +	msleep(100);
> 
> Where did these usleep and msleep numbers come from?  They should use
> a #define that we can connect back to a spec.

I think we can get rid of the first usleep_range() since we need to
deassert the line just to avoid unbalance in deassert_count counter since the
reset line is shared (the line is actually already de-assert). I will add a
comment to clarify it.

> 
> These delays should also be mentioned in the commit log because it
> appears unrelated to the conversion to the reset_bulk API.  Actually,
> it would be even better if they were in a separate patch, since it
> looks like a logically separate change.

Regarding the msleep(100), it is not documented in the vendor sdk, I think it
necessary to complete the reset before initialize the pcie-phy. Since it is
required just for EN7581, I guess we can move it in mtk_pcie_en7581_power_up()
(in patch 4/4) before the phy_init(). What do you think?

Regards,
Lorenzo

> 
> >  	reset_control_assert(pcie->mac_reset);
> >  	usleep_range(10, 20);
> 
> Unrelated to this patch, but it would be nice to have an explanation
> of this existing delay, too.
> 
> Bjorn

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