Re: [PATCH v4 08/12] phy: qcom-qmp-pcie: Add support for SM8550 g3x2 and g4x2 PCIEs

[Abel Vesa]

On 23-01-23 16:03:48, Johan Hovold wrote:
> On Thu, Jan 19, 2023 at 04:04:49PM +0200, Abel Vesa wrote:
> > Add the SM8550 both g4 and g3 configurations. In addition, there is a
> > new "lane shared" table that needs to be configured for g4, along with
> > the No-CSR list of resets.
> > 
> > Co-developed-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> > Signed-off-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> > 
> > This patchset relies on the following patchset:
> > https://lore.kernel.org/all/20230117224148.1914627-1-abel.vesa@xxxxxxxxxx/
> > 
> > The v3 of this patchset is:
> > https://lore.kernel.org/all/20230118005328.2378792-1-abel.vesa@xxxxxxxxxx/
> > 
> > Changes since v3:
> >  * added Dmitry's R-b tag
> > 
> > Changes since v2:
> >  * none
> > 
> > Changes since v1:
> >  * split all the offsets into separate patches, like Vinod suggested
> > 
> > 
> >  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 365 +++++++++++++++++++++++
> >  1 file changed, 365 insertions(+)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index bffb9e138715..48d179d8d8d6 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c

[...]

> > @@ -2370,6 +2704,14 @@ static int qmp_pcie_power_on(struct phy *phy)
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (qmp->nocsr_resets) {
> > +		ret = reset_control_bulk_deassert(cfg->num_nocsr_resets, qmp->nocsr_resets);
> > +		if (ret) {
> > +			dev_err(qmp->dev, "no-csr reset deassert failed\n");
> > +			goto err_disable_pipe_clk;
> > +		}
> > +	}
> 
> Is this the documented reset sequence? To keep the nocsr reset asserted
> from init() to power_on() and during programming of the PHY registers?
> 
> What if power_on() is never called, etc? (I know we always call
> power_on() after init() currently, but that may change.)
> 
> Could you explain a bit how this reset is supposed work and be used?
> 

The documentation says that the no-CSR reset should be kept asserted until
the clock (PLL) is stable and during configuration. It also says the
no-CSR can be used to reset the PHY without losing the configuration.
It also says pciephy_reset needs to be deasserted before configuration.

So I guess what we need to do here is: deassert the pciephy_reset,
configure the CSR register, then deassert the no-CSR reset.

If power on never gets called, PHY remains in reset, but configured.

> > +
> >  	/* Pull PHY out of reset state */
> >  	qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> >  
> > @@ -2503,6 +2845,21 @@ static int qmp_pcie_reset_init(struct qmp_pcie *qmp)
> >  	if (ret)
> >  		return dev_err_probe(dev, ret, "failed to get resets\n");
> >  
> > +	if (cfg->nocsr_reset_list) {
> > +		qmp->nocsr_resets = devm_kcalloc(dev, cfg->num_nocsr_resets,
> > +				   sizeof(*qmp->nocsr_resets), GFP_KERNEL);
> > +		if (!qmp->nocsr_resets)
> > +			return -ENOMEM;
> > +
> > +		for (i = 0; i < cfg->num_nocsr_resets; i++)
> > +			qmp->nocsr_resets[i].id = cfg->nocsr_reset_list[i];
> > +
> > +		ret = devm_reset_control_bulk_get_exclusive(dev, cfg->num_nocsr_resets,
> > +								qmp->nocsr_resets);
> > +		if (ret)
> > +			return dev_err_probe(dev, ret, "failed to get no CSR resets\n");

[...]