Re: [PATCH v2 2/3] phy: cdns-dphy: Add band config for dphy tx

[Rahul T R <r-ravikumar@ti.com>]

On 11:16-20220622, Laurent Pinchart wrote:
> Hi Rahul,
> 
> Thank you for the patch.
> 
> On Wed, Jun 22, 2022 at 01:23:39PM +0530, Rahul T R wrote:
> > Add support for band ctrl config for dphy tx.
> > 
> > Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> > ---
> >  drivers/phy/cadence/cdns-dphy.c | 52 ++++++++++++++++++++++++++++++++-
> >  1 file changed, 51 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> > index ba042e39cfaf..ddfa524d8ce7 100644
> > --- a/drivers/phy/cadence/cdns-dphy.c
> > +++ b/drivers/phy/cadence/cdns-dphy.c
> > @@ -4,6 +4,7 @@
> >   */
> >  
> >  #include <linux/bitops.h>
> > +#include <linux/bitfield.h>
> 
> Nitpicking, bitfield goes before bitops :-)
> 
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > @@ -45,6 +46,10 @@
> >  #define DPHY_CMN_OPDIV_FROM_REG		BIT(6)
> >  #define DPHY_CMN_OPDIV(x)		((x) << 7)
> >  
> > +#define DPHY_BAND_CFG			DPHY_PCS(0x0)
> > +#define DPHY_BAND_CFG_LEFT_BAND		GENMASK(4, 0)
> > +#define DPHY_BAND_CFG_RIGHT_BAND	GENMASK(9, 5)
> > +
> >  #define DPHY_PSM_CFG			DPHY_PCS(0x4)
> >  #define DPHY_PSM_CFG_FROM_REG		BIT(0)
> >  #define DPHY_PSM_CLK_DIV(x)		((x) << 1)
> > @@ -92,6 +97,22 @@ struct cdns_dphy {
> >  	struct phy *phy;
> >  };
> >  
> > +struct cdns_dphy_band {
> > +	unsigned int min_rate;
> > +	unsigned int max_rate;
> > +};
> > +
> > +/* Order of bands is important since the index is the band number. */
> > +static struct cdns_dphy_band tx_bands[] = {
> 
> static const
> 
> > +	{80, 100}, {100, 120}, {120, 160}, {160, 200}, {200, 240},
> > +	{240, 320}, {320, 390}, {390, 450}, {450, 510}, {510, 560},
> > +	{560, 640}, {640, 690}, {690, 770}, {770, 870}, {870, 950},
> > +	{950, 1000}, {1000, 1200}, {1200, 1400}, {1400, 1600}, {1600, 1800},
> > +	{1800, 2000}, {2000, 2200}, {2200, 2500}
> 
> The max_rate value of band N is always equal to the min_rate value of
> band N+1. Could we store one only ?
> 
> > +};
> > +
> > +static int num_tx_bands = ARRAY_SIZE(tx_bands);
> 
> You can use ARRAY_SIZE(tx_bands) directly below and drop this.
> 
> > +
> >  static int cdns_dsi_get_dphy_pll_cfg(struct cdns_dphy *dphy,
> >  				     struct cdns_dphy_cfg *cfg,
> >  				     struct phy_configure_opts_mipi_dphy *opts,
> > @@ -232,6 +253,26 @@ static int cdns_dphy_config_from_opts(struct phy *phy,
> >  	return 0;
> >  }
> >  
> > +static int cdns_dphy_tx_get_band_ctrl(unsigned long hs_clk_rate)
> > +{
> > +	unsigned int rate;
> > +	int i;
> > +
> > +	rate = hs_clk_rate / 1000000UL;
> > +
> > +	if (rate < tx_bands[0].min_rate || rate >= tx_bands[num_tx_bands - 1].max_rate)
> > +		return -EOPNOTSUPP;
> > +
> > +	for (i = 0; i < num_tx_bands; i++) {
> > +		if (rate >= tx_bands[i].min_rate && rate < tx_bands[i].max_rate)
> > +			return i;
> > +	}
> > +
> > +	/* Unreachable. */
> > +	WARN(1, "Reached unreachable code.");
> 
> I'd drop the WARN() if it's really unreachable.
>

Hi Laurent,

Thanks for the review!
I have sent a v3, addressing
all the comments

Please review

Regards
Rahul T R

> > +	return -EINVAL;
> > +}
> > +
> >  static int cdns_dphy_validate(struct phy *phy, enum phy_mode mode, int submode,
> >  			      union phy_configure_opts *opts)
> >  {
> > @@ -247,7 +288,8 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> >  {
> >  	struct cdns_dphy *dphy = phy_get_drvdata(phy);
> >  	struct cdns_dphy_cfg cfg = { 0 };
> > -	int ret;
> > +	int ret, band_ctrl;
> > +	unsigned int reg;
> >  
> >  	ret = cdns_dphy_config_from_opts(phy, &opts->mipi_dphy, &cfg);
> >  	if (ret)
> > @@ -276,6 +318,14 @@ static int cdns_dphy_configure(struct phy *phy, union phy_configure_opts *opts)
> >  	 */
> >  	cdns_dphy_set_pll_cfg(dphy, &cfg);
> >  
> > +	band_ctrl = cdns_dphy_tx_get_band_ctrl(opts->mipi_dphy.hs_clk_rate);
> > +	if (band_ctrl < 0)
> > +		return band_ctrl;
> > +
> > +	reg = FIELD_PREP(DPHY_BAND_CFG_LEFT_BAND, band_ctrl) |
> > +	      FIELD_PREP(DPHY_BAND_CFG_RIGHT_BAND, band_ctrl);
> > +	writel(reg, dphy->regs + DPHY_BAND_CFG);
> > +
> >  	return 0;
> >  }
> >  
> 
> -- 
> Regards,
> 
> Laurent Pinchart