Re: [PATCH 01/15] phy: qcom-qmp-ufs: Move register settings to qmp_phy_cfg_tables struct

[Manivannan Sadhasivam <mani@kernel.org>]

On Mon, Oct 31, 2022 at 09:50:59PM +0300, Dmitry Baryshkov wrote:
> On 31/10/2022 18:46, Manivannan Sadhasivam wrote:
> > On Sun, Oct 30, 2022 at 12:50:50AM +0300, Dmitry Baryshkov wrote:
> > > On 29/10/2022 17:16, Manivannan Sadhasivam wrote:
> > > > As done for Qcom PCIe PHY driver, let's move the register settings to the
> > > > common qmp_phy_cfg_tables struct. This helps in adding any additional PHY
> > > > settings needed for functionalities like HS-G4 in the future by adding one
> > > > more instance of the qmp_phy_cfg_tables.
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >    drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 223 +++++++++++++-----------
> > > >    1 file changed, 126 insertions(+), 97 deletions(-)
> > > > 
> > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c

[...]

> > > >    static int qmp_ufs_com_init(struct qmp_phy *qphy)
> > > > @@ -933,31 +977,16 @@ static int qmp_ufs_power_on(struct phy *phy)
> > > >    	struct qmp_phy *qphy = phy_get_drvdata(phy);
> > > >    	struct qcom_qmp *qmp = qphy->qmp;
> > > >    	const struct qmp_phy_cfg *cfg = qphy->cfg;
> > > > -	void __iomem *tx = qphy->tx;
> > > > -	void __iomem *rx = qphy->rx;
> > > >    	void __iomem *pcs = qphy->pcs;
> > > >    	void __iomem *status;
> > > >    	unsigned int mask, val, ready;
> > > >    	int ret;
> > > > -	qmp_ufs_serdes_init(qphy);
> > > > -
> > > > -	/* Tx, Rx, and PCS configurations */
> > > > -	qmp_ufs_configure_lane(tx, cfg->regs, cfg->tx_tbl, cfg->tx_tbl_num, 1);
> > > > +	qmp_ufs_serdes_init(qphy, &cfg->tables);
> > > > -	if (cfg->lanes >= 2) {
> > > > -		qmp_ufs_configure_lane(qphy->tx2, cfg->regs,
> > > > -					cfg->tx_tbl, cfg->tx_tbl_num, 2);
> > > > -	}
> > > > -
> > > > -	qmp_ufs_configure_lane(rx, cfg->regs, cfg->rx_tbl, cfg->rx_tbl_num, 1);
> > > > -
> > > > -	if (cfg->lanes >= 2) {
> > > > -		qmp_ufs_configure_lane(qphy->rx2, cfg->regs,
> > > > -					cfg->rx_tbl, cfg->rx_tbl_num, 2);
> > > > -	}
> > > > +	qmp_ufs_lanes_init(qphy, &cfg->tables);
> > > > -	qmp_ufs_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
> > > > +	qmp_ufs_pcs_init(qphy, &cfg->tables);
> > > 
> > > I'd suggest going straight to qmp_ufs_init_registers, which would contain
> > > both serdes, lanes and pcs inits.
> > > 
> > 
> > That adds one more level of indirection which may not be needed here. Moreover,
> > I'm trying to be in sync with other qmp drivers, specifically the pcie one.
> > This helps in working with these drivers.
> 
> Yes, I understand. However I hope that the respective patchset (including
> [1]) will be merged soon. Thus I suggest skipping the step and using the
> same function already.
> 
> [1] https://lore.kernel.org/linux-phy/20221028133603.18470-10-johan+linaro@kernel.org/
> 

Ah, I missed this series. Will use the common function then.

Thanks,
Mani

> > 
> > Thanks,
> > Mani
> > 
> > > >    	ret = reset_control_deassert(qmp->ufs_reset);
> > > >    	if (ret)
> > > 
> > > -- 
> > > With best wishes
> > > Dmitry
> > > 
> > 
> 
> -- 
> With best wishes
> Dmitry
> 

-- 
மணிவண்ணன் சதாசிவம்