Re: [PATCH 07/10] phy: qualcomm: qmp-pcie: Add support for SM8550 g3x2 and g4x2 PCIEs

[Dmitry Baryshkov]

On 17/01/2023 15:57, Abel Vesa wrote:
> On 23-01-01 22:15:55, Dmitry Baryshkov wrote:
> > On 16/11/2022 14:01, 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>
> > > ---
> > >    drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 354 +++++++++++++++++++++++
> > >    1 file changed, 354 insertions(+)
> > >
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > index 47cccc4b35b2..87c7c20dfc8d 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >
> > [skipped tables]
> >
> > > @@ -1473,6 +1701,8 @@ struct qmp_pcie_offsets {
> > >    struct qmp_phy_cfg_tbls {
> > >    	const struct qmp_phy_init_tbl *serdes;
> > >    	int serdes_num;
> > > +	const struct qmp_phy_init_tbl *ln_shrd_serdes;
> > > +	int ln_shrd_serdes_num;
> > >    	const struct qmp_phy_init_tbl *tx;
> > >    	int tx_num;
> > >    	const struct qmp_phy_init_tbl *rx;
> > > @@ -1510,6 +1740,9 @@ struct qmp_phy_cfg {
> > >    	/* resets to be requested */
> > >    	const char * const *reset_list;
> > >    	int num_resets;
> > > +	/* no CSR resets to be requested */
> > > +	const char * const *nocsr_reset_list;
> > > +	int num_nocsr_resets;
> >
> > Is there any difference between 'no CSR' resets and the plain ones? Can we
> > handle them in a single array instead?
>
> Yes, on power on, only the 'No CSR' are necessary to be deasserted.
>
> So we need to differentiate between 'No CSR' and the rest.

Ack.

> > >    	/* regulators to be requested */
> > >    	const char * const *vreg_list;
> > >    	int num_vregs;

[skipped]

> > > @@ -2502,6 +2847,9 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np
> > >    			return PTR_ERR(qmp->rx2);
> > >    		qmp->pcs_misc = devm_of_iomap(dev, np, 5, NULL);
> > > +
> > > +		if (cfg->has_ln_shrd_serdes_tbl)
> > > +			qmp->ln_shrd_serdes = devm_of_iomap(dev, np, 6, NULL);
> >
> > I think we also need to check the returned value. Also, I think we can drop
> > the conditional check here. we don't have to validate the DT, so if the reg
> > is present in DT, then it's present. If not, it's not required.
>
> Yeah, but I was trying to make sure that if there is a new type of reg
> for some upcomming SoC, we don't take that as ln_shrd. That's why the check
> was added.
>
> I'll add the returned value check though.

This is the parse_dt_legacy() function. I think we can leave it as is 
and support sm8550 only using the new style of parsing.

> > >    	} else {
> > >    		qmp->pcs_misc = devm_of_iomap(dev, np, 3, NULL);
> > >    	}
> > > @@ -2729,6 +3077,12 @@ static const struct of_device_id qmp_pcie_of_match_table[] = {
> > >    	}, {
> > >    		.compatible = "qcom,sm8450-qmp-gen4x2-pcie-phy",
> > >    		.data = &sm8450_qmp_gen4x2_pciephy_cfg,
> > > +	}, {
> > > +		.compatible = "qcom,sm8550-qmp-gen3x2-pcie-phy",
> > > +		.data = &sm8550_qmp_gen3x2_pciephy_cfg,
> > > +	}, {
> > > +		.compatible = "qcom,sm8550-qmp-gen4x2-pcie-phy",
> > > +		.data = &sm8550_qmp_gen4x2_pciephy_cfg,
> > >    	},
> > >    	{ },
> > >    };
> >
> > --
> > With best wishes
> > Dmitry

--
With best wishes
Dmitry