[v3,08/15] 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 | 51 +++++++-----------------
 1 file changed, 15 insertions(+), 36 deletions(-)
  

On 21/10/2022 14:09, 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 | 51 +++++++-----------------
>   1 file changed, 15 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index dd7e72424fc0..f57d10f20277 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -1820,46 +1820,32 @@ 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 *tables)
> -{
> -	void __iomem *serdes = qmp->serdes;
> -
> -	if (!tables)
> -		return;
> -
> -	qmp_pcie_configure(serdes, tables->serdes, tables->serdes_num);
> -}
> -
> -static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tables)
> +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 (!tables)
> +	if (!tbls)
>   		return;
>   
> -	qmp_pcie_configure_lane(tx, tables->tx, tables->tx_num, 1);
> -	qmp_pcie_configure_lane(rx, tables->rx, tables->rx_num, 1);
> +	qmp_pcie_configure(serdes, tbls->serdes, tbls->serdes_num);
> +
> +	qmp_pcie_configure_lane(tx, tbls->tx, tbls->tx_num, 1);
> +	qmp_pcie_configure_lane(rx, tbls->rx, tbls->rx_num, 1);
>   
>   	if (cfg->lanes >= 2) {
> -		qmp_pcie_configure_lane(tx2, tables->tx, tables->tx_num, 2);
> -		qmp_pcie_configure_lane(rx2, tables->rx, tables->rx_num, 2);
> +		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 *tables)
> -{
> -	void __iomem *pcs = qmp->pcs;
> -	void __iomem *pcs_misc = qmp->pcs_misc;
> -
> -	if (!tables)
> -		return;
>   
> -	qmp_pcie_configure(pcs, tables->pcs, tables->pcs_num);
> -	qmp_pcie_configure(pcs_misc, tables->pcs_misc, tables->pcs_misc_num);
> +	qmp_pcie_configure(pcs, tbls->pcs, tbls->pcs_num);
> +	qmp_pcie_configure(pcs_misc, tbls->pcs_misc, tbls->pcs_misc_num);

As seem above, if nothing else, tables -> tbls rename generates 
unnecessary diff.
Other than that LGTM.

>   }
>   
>   static int qmp_pcie_init(struct phy *phy)
> @@ -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);
>   
>   	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);
>
  

On Fri, Oct 21, 2022 at 02:18:49PM +0300, Dmitry Baryshkov wrote:
> On 21/10/2022 14:09, 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 | 51 +++++++-----------------
> >   1 file changed, 15 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index dd7e72424fc0..f57d10f20277 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -1820,46 +1820,32 @@ 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 *tables)
> > -{
> > -	void __iomem *serdes = qmp->serdes;
> > -
> > -	if (!tables)
> > -		return;
> > -
> > -	qmp_pcie_configure(serdes, tables->serdes, tables->serdes_num);
> > -}
> > -
> > -static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tables)
> > +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 (!tables)
> > +	if (!tbls)
> >   		return;
> >   
> > -	qmp_pcie_configure_lane(tx, tables->tx, tables->tx_num, 1);
> > -	qmp_pcie_configure_lane(rx, tables->rx, tables->rx_num, 1);
> > +	qmp_pcie_configure(serdes, tbls->serdes, tbls->serdes_num);
> > +
> > +	qmp_pcie_configure_lane(tx, tbls->tx, tbls->tx_num, 1);
> > +	qmp_pcie_configure_lane(rx, tbls->rx, tbls->rx_num, 1);
> >   
> >   	if (cfg->lanes >= 2) {
> > -		qmp_pcie_configure_lane(tx2, tables->tx, tables->tx_num, 2);
> > -		qmp_pcie_configure_lane(rx2, tables->rx, tables->rx_num, 2);
> > +		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 *tables)
> > -{
> > -	void __iomem *pcs = qmp->pcs;
> > -	void __iomem *pcs_misc = qmp->pcs_misc;
> > -
> > -	if (!tables)
> > -		return;
> >   
> > -	qmp_pcie_configure(pcs, tables->pcs, tables->pcs_num);
> > -	qmp_pcie_configure(pcs_misc, tables->pcs_misc, tables->pcs_misc_num);
> > +	qmp_pcie_configure(pcs, tbls->pcs, tbls->pcs_num);
> > +	qmp_pcie_configure(pcs_misc, tbls->pcs_misc, tbls->pcs_misc_num);
> 
> As seem above, if nothing else, tables -> tbls rename generates 
> unnecessary diff.

Can you please stop with the bikeshedding. This is not about keeping the
diff small, this is about readability of the new helper function as I
already told you.

And this is a *local* identifier, not some state member that needs a
super descriptive name. And the rest of the driver used "tbl"
consistently until your EP/RC mode patches for that matter.

> Other than that LGTM.

Johan
  

On 21-10-22, 13:32, Johan Hovold wrote:
> On Fri, Oct 21, 2022 at 02:18:49PM +0300, Dmitry Baryshkov wrote:
> > On 21/10/2022 14:09, 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 | 51 +++++++-----------------
> > >   1 file changed, 15 insertions(+), 36 deletions(-)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > index dd7e72424fc0..f57d10f20277 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > @@ -1820,46 +1820,32 @@ 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 *tables)
> > > -{
> > > -	void __iomem *serdes = qmp->serdes;
> > > -
> > > -	if (!tables)
> > > -		return;
> > > -
> > > -	qmp_pcie_configure(serdes, tables->serdes, tables->serdes_num);
> > > -}
> > > -
> > > -static void qmp_pcie_lanes_init(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tables)
> > > +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 (!tables)
> > > +	if (!tbls)
> > >   		return;
> > >   
> > > -	qmp_pcie_configure_lane(tx, tables->tx, tables->tx_num, 1);
> > > -	qmp_pcie_configure_lane(rx, tables->rx, tables->rx_num, 1);
> > > +	qmp_pcie_configure(serdes, tbls->serdes, tbls->serdes_num);
> > > +
> > > +	qmp_pcie_configure_lane(tx, tbls->tx, tbls->tx_num, 1);
> > > +	qmp_pcie_configure_lane(rx, tbls->rx, tbls->rx_num, 1);
> > >   
> > >   	if (cfg->lanes >= 2) {
> > > -		qmp_pcie_configure_lane(tx2, tables->tx, tables->tx_num, 2);
> > > -		qmp_pcie_configure_lane(rx2, tables->rx, tables->rx_num, 2);
> > > +		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 *tables)
> > > -{
> > > -	void __iomem *pcs = qmp->pcs;
> > > -	void __iomem *pcs_misc = qmp->pcs_misc;
> > > -
> > > -	if (!tables)
> > > -		return;
> > >   
> > > -	qmp_pcie_configure(pcs, tables->pcs, tables->pcs_num);
> > > -	qmp_pcie_configure(pcs_misc, tables->pcs_misc, tables->pcs_misc_num);
> > > +	qmp_pcie_configure(pcs, tbls->pcs, tbls->pcs_num);
> > > +	qmp_pcie_configure(pcs_misc, tbls->pcs_misc, tbls->pcs_misc_num);
> > 
> > As seem above, if nothing else, tables -> tbls rename generates 
> > unnecessary diff.
> 
> Can you please stop with the bikeshedding. This is not about keeping the
> diff small, this is about readability of the new helper function as I
> already told you.
> 
> And this is a *local* identifier, not some state member that needs a
> super descriptive name. And the rest of the driver used "tbl"
> consistently until your EP/RC mode patches for that matter.

I would disagree here... You can change tbls/tables but then it does not
help _this_ patch

Right thing would be to change tbls to tables first and then add init
helper... For a reviewer seeing an undocumented change and unnecessary
diff is not right..

Pls split if you would still like the rename
  

On Fri, Oct 28, 2022 at 06:24:38PM +0530, Vinod Koul wrote:
> On 21-10-22, 13:32, Johan Hovold wrote:

> > [...] This is not about keeping the
> > diff small, this is about readability of the new helper function as I
> > already told you.
> > 
> > And this is a *local* identifier, not some state member that needs a
> > super descriptive name. And the rest of the driver used "tbl"
> > consistently until your EP/RC mode patches for that matter.
> 
> I would disagree here... You can change tbls/tables but then it does not
> help _this_ patch
> 
> Right thing would be to change tbls to tables first and then add init
> helper... For a reviewer seeing an undocumented change and unnecessary
> diff is not right..

I still think that it belongs in the patch adding the new helper
because it is essentially only in that new function that the improved
readability due to the shorter identifier matters (the earlier helpers
where per table type).
 
> Pls split if you would still like the rename

But if you prefer I'll split it out in a preparatory patch.

Johan
  

On 28-10-22, 15:07, Johan Hovold wrote:
> On Fri, Oct 28, 2022 at 06:24:38PM +0530, Vinod Koul wrote:
> > On 21-10-22, 13:32, Johan Hovold wrote:
> 
> > > [...] This is not about keeping the
> > > diff small, this is about readability of the new helper function as I
> > > already told you.
> > > 
> > > And this is a *local* identifier, not some state member that needs a
> > > super descriptive name. And the rest of the driver used "tbl"
> > > consistently until your EP/RC mode patches for that matter.
> > 
> > I would disagree here... You can change tbls/tables but then it does not
> > help _this_ patch
> > 
> > Right thing would be to change tbls to tables first and then add init
> > helper... For a reviewer seeing an undocumented change and unnecessary
> > diff is not right..
> 
> I still think that it belongs in the patch adding the new helper
> because it is essentially only in that new function that the improved
> readability due to the shorter identifier matters (the earlier helpers
> where per table type).
>  
> > Pls split if you would still like the rename
> 
> But if you prefer I'll split it out in a preparatory patch.

That would be good thanks