mbox series

[v2,0/3] Add support for DPHY TX on J721E

Message ID 20220622075340.16915-1-r-ravikumar@ti.com
Headers show
Series Add support for DPHY TX on J721E | expand

Message

Rahul T R June 22, 2022, 7:53 a.m. UTC 
Following series of patches adds support for DPHY TX on TI's J721E
SoC. New compatible is added and required cdns dphy ops are implemented.
The series also adds band ctrl configuration required for dphy tx

v2:
-Fix a build error reported by kernel test robot <lkp@intel.com>
 which uses clang compiler. Did not get the error with GNU Toolchain
 9.2-2019.12

Rahul T R (3):
  phy: dt-bindings: cdns,dphy: Add compatible for dphy on j721e
  phy: cdns-dphy: Add band config for dphy tx
  phy: cdns-dphy: Add support for DPHY TX on J721e

 .../devicetree/bindings/phy/cdns,dphy.yaml    |   5 +-
 drivers/phy/cadence/Kconfig                   |  10 ++
 drivers/phy/cadence/cdns-dphy.c               | 114 +++++++++++++++++-
 3 files changed, 126 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart June 22, 2022, 8:16 a.m. UTC | #1 
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.

> +	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;
>  }
>
Rahul T R June 22, 2022, 11:12 a.m. UTC | #2 
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