[v4,09/16] phy: qcom-qmp-pcie: add register init helper

  Generalise the serdes initialisation helper so that it can be used to
initialise all the PHY registers (e.g. serdes, tx, rx, pcs).

Note that this defers the ungating of the PIPE clock somewhat, which is
fine as it isn't needed until starting the PHY.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 37 +++++-------------------
 1 file changed, 8 insertions(+), 29 deletions(-)
  

On 28/10/2022 16:35, Johan Hovold wrote:
> Generalise the serdes initialisation helper so that it can be used to
> initialise all the PHY registers (e.g. serdes, tx, rx, pcs).
> 
> Note that this defers the ungating of the PIPE clock somewhat, which is
> fine as it isn't needed until starting the PHY.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 37 +++++-------------------
>   1 file changed, 8 insertions(+), 29 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
  

On 28-10-22, 15:35, Johan Hovold wrote:
> Generalise the serdes initialisation helper so that it can be used to
> initialise all the PHY registers (e.g. serdes, tx, rx, pcs).
> 
> Note that this defers the ungating of the PIPE clock somewhat, which is
> fine as it isn't needed until starting the PHY.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 37 +++++-------------------
>  1 file changed, 8 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 791ed7ef0eab..f57d10f20277 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -1820,27 +1820,22 @@ static void qmp_pcie_configure(void __iomem *base,
>  	qmp_pcie_configure_lane(base, tbl, num, 0xff);
>  }
>  
> -static void qmp_pcie_serdes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> -{
> -	void __iomem *serdes = qmp->serdes;
> -
> -	if (!tbls)
> -		return;
> -
> -	qmp_pcie_configure(serdes, tbls->serdes, tbls->serdes_num);
> -}
> -
> -static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> +static void qmp_pcie_init_registers(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
>  {
>  	const struct qmp_phy_cfg *cfg = qmp->cfg;
> +	void __iomem *serdes = qmp->serdes;
>  	void __iomem *tx = qmp->tx;
>  	void __iomem *rx = qmp->rx;
>  	void __iomem *tx2 = qmp->tx2;
>  	void __iomem *rx2 = qmp->rx2;
> +	void __iomem *pcs = qmp->pcs;
> +	void __iomem *pcs_misc = qmp->pcs_misc;
>  
>  	if (!tbls)
>  		return;
>  
> +	qmp_pcie_configure(serdes, tbls->serdes, tbls->serdes_num);

We are tbls

> +
>  	qmp_pcie_configure_lane(tx, tbls->tx, tbls->tx_num, 1);
>  	qmp_pcie_configure_lane(rx, tbls->rx, tbls->rx_num, 1);
>  
> @@ -1848,15 +1843,6 @@ static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_t
>  		qmp_pcie_configure_lane(tx2, tbls->tx, tbls->tx_num, 2);
>  		qmp_pcie_configure_lane(rx2, tbls->rx, tbls->rx_num, 2);
>  	}
> -}
> -
> -static void qmp_pcie_pcs_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> -{
> -	void __iomem *pcs = qmp->pcs;
> -	void __iomem *pcs_misc = qmp->pcs_misc;
> -
> -	if (!tbls)
> -		return;
>  
>  	qmp_pcie_configure(pcs, tbls->pcs, tbls->pcs_num);
>  	qmp_pcie_configure(pcs_misc, tbls->pcs_misc, tbls->pcs_misc_num);
> @@ -1932,8 +1918,8 @@ static int qmp_pcie_power_on(struct phy *phy)
>  	else
>  		mode_tables = cfg->tables_ep;
>  
> -	qmp_pcie_serdes_init(qmp, &cfg->tables);
> -	qmp_pcie_serdes_init(qmp, mode_tables);
> +	qmp_pcie_init_registers(qmp, &cfg->tables);
> +	qmp_pcie_init_registers(qmp, mode_tables);

but here tables :(

Lets stick with either please, or if we have differentiation lets make
it real obvious

>  
>  	ret = clk_prepare_enable(qmp->pipe_clk);
>  	if (ret) {
> @@ -1941,13 +1927,6 @@ static int qmp_pcie_power_on(struct phy *phy)
>  		return ret;
>  	}
>  
> -	/* Tx, Rx, and PCS configurations */
> -	qmp_pcie_lanes_init(qmp, &cfg->tables);
> -	qmp_pcie_lanes_init(qmp, mode_tables);
> -
> -	qmp_pcie_pcs_init(qmp, &cfg->tables);
> -	qmp_pcie_pcs_init(qmp, mode_tables);
> -
>  	/* Pull PHY out of reset state */
>  	qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
>  
> -- 
> 2.37.3
  

On Sat, Nov 05, 2022 at 05:38:54PM +0530, Vinod Koul wrote:
> On 28-10-22, 15:35, Johan Hovold wrote:
> > Generalise the serdes initialisation helper so that it can be used to
> > initialise all the PHY registers (e.g. serdes, tx, rx, pcs).
> > 
> > Note that this defers the ungating of the PIPE clock somewhat, which is
> > fine as it isn't needed until starting the PHY.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 37 +++++-------------------
> >  1 file changed, 8 insertions(+), 29 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index 791ed7ef0eab..f57d10f20277 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -1820,27 +1820,22 @@ static void qmp_pcie_configure(void __iomem *base,
> >  	qmp_pcie_configure_lane(base, tbl, num, 0xff);
> >  }
> >  
> > -static void qmp_pcie_serdes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> > -{
> > -	void __iomem *serdes = qmp->serdes;
> > -
> > -	if (!tbls)
> > -		return;
> > -
> > -	qmp_pcie_configure(serdes, tbls->serdes, tbls->serdes_num);
> > -}
> > -
> > -static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> > +static void qmp_pcie_init_registers(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> >  {
> >  	const struct qmp_phy_cfg *cfg = qmp->cfg;
> > +	void __iomem *serdes = qmp->serdes;
> >  	void __iomem *tx = qmp->tx;
> >  	void __iomem *rx = qmp->rx;
> >  	void __iomem *tx2 = qmp->tx2;
> >  	void __iomem *rx2 = qmp->rx2;
> > +	void __iomem *pcs = qmp->pcs;
> > +	void __iomem *pcs_misc = qmp->pcs_misc;
> >  
> >  	if (!tbls)
> >  		return;
> >  
> > +	qmp_pcie_configure(serdes, tbls->serdes, tbls->serdes_num);
> 
> We are tbls

Yeah, it's a separate function.

Note that qmp_pcie_configure_lane() above use 'tbl' too.

> > +
> >  	qmp_pcie_configure_lane(tx, tbls->tx, tbls->tx_num, 1);
> >  	qmp_pcie_configure_lane(rx, tbls->rx, tbls->rx_num, 1);
> >  
> > @@ -1848,15 +1843,6 @@ static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_t
> >  		qmp_pcie_configure_lane(tx2, tbls->tx, tbls->tx_num, 2);
> >  		qmp_pcie_configure_lane(rx2, tbls->rx, tbls->rx_num, 2);
> >  	}
> > -}
> > -
> > -static void qmp_pcie_pcs_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> > -{
> > -	void __iomem *pcs = qmp->pcs;
> > -	void __iomem *pcs_misc = qmp->pcs_misc;
> > -
> > -	if (!tbls)
> > -		return;
> >  
> >  	qmp_pcie_configure(pcs, tbls->pcs, tbls->pcs_num);
> >  	qmp_pcie_configure(pcs_misc, tbls->pcs_misc, tbls->pcs_misc_num);
> > @@ -1932,8 +1918,8 @@ static int qmp_pcie_power_on(struct phy *phy)
> >  	else
> >  		mode_tables = cfg->tables_ep;
> >  
> > -	qmp_pcie_serdes_init(qmp, &cfg->tables);
> > -	qmp_pcie_serdes_init(qmp, mode_tables);
> > +	qmp_pcie_init_registers(qmp, &cfg->tables);
> > +	qmp_pcie_init_registers(qmp, mode_tables);
> 
> but here tables :(
> 
> Lets stick with either please, or if we have differentiation lets make
> it real obvious

It's not uncommon to use shorter local identifiers and a more descriptive
name in structures, but since the driver already used 'tbl' consistently
before the recent addition of the aggregate tables structure, I can
rename also those pointers so that we use 'tbl' and 'tbls' consistently
throughout the driver.

Johan
  

On 05-11-22, 14:17, Johan Hovold wrote:
> On Sat, Nov 05, 2022 at 05:38:54PM +0530, Vinod Koul wrote:
> > On 28-10-22, 15:35, Johan Hovold wrote:

> > > +	qmp_pcie_configure(serdes, tbls->serdes, tbls->serdes_num);
> > 
> > We are tbls
> 
> Yeah, it's a separate function.
> 
> Note that qmp_pcie_configure_lane() above use 'tbl' too.
> 
> > > +
> > >  	qmp_pcie_configure_lane(tx, tbls->tx, tbls->tx_num, 1);
> > >  	qmp_pcie_configure_lane(rx, tbls->rx, tbls->rx_num, 1);
> > >  
> > > @@ -1848,15 +1843,6 @@ static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_t
> > >  		qmp_pcie_configure_lane(tx2, tbls->tx, tbls->tx_num, 2);
> > >  		qmp_pcie_configure_lane(rx2, tbls->rx, tbls->rx_num, 2);
> > >  	}
> > > -}
> > > -
> > > -static void qmp_pcie_pcs_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls)
> > > -{
> > > -	void __iomem *pcs = qmp->pcs;
> > > -	void __iomem *pcs_misc = qmp->pcs_misc;
> > > -
> > > -	if (!tbls)
> > > -		return;
> > >  
> > >  	qmp_pcie_configure(pcs, tbls->pcs, tbls->pcs_num);
> > >  	qmp_pcie_configure(pcs_misc, tbls->pcs_misc, tbls->pcs_misc_num);
> > > @@ -1932,8 +1918,8 @@ static int qmp_pcie_power_on(struct phy *phy)
> > >  	else
> > >  		mode_tables = cfg->tables_ep;
> > >  
> > > -	qmp_pcie_serdes_init(qmp, &cfg->tables);
> > > -	qmp_pcie_serdes_init(qmp, mode_tables);
> > > +	qmp_pcie_init_registers(qmp, &cfg->tables);
> > > +	qmp_pcie_init_registers(qmp, mode_tables);
> > 
> > but here tables :(
> > 
> > Lets stick with either please, or if we have differentiation lets make
> > it real obvious
> 
> It's not uncommon to use shorter local identifiers and a more descriptive
> name in structures, but since the driver already used 'tbl' consistently
> before the recent addition of the aggregate tables structure, I can
> rename also those pointers so that we use 'tbl' and 'tbls' consistently
> throughout the driver.

Thanks that would be great. Lets stick to one convention throughout the
driver