mbox series

[v3,0/3] Add support for dphy tx on j721e

Message ID 20220622105311.21415-1-r-ravikumar@ti.com
Headers show
Series Add support for dphy tx on j721e | expand

Message

Rahul T R June 22, 2022, 10: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

v3:
-Fixed the header files order
-Store bands as array of unsigned int, since min[n] = max[n-1]
-Remove unreachable code warning

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               | 102 +++++++++++++++++-
 3 files changed, 114 insertions(+), 3 deletions(-)

Comments

Pratyush Yadav June 23, 2022, 9:38 a.m. UTC | #1 
On 22/06/22 04:23PM, Rahul T R wrote:
> Add support for band ctrl config for dphy tx.
> 
> Signed-off-by: Rahul T R <r-ravikumar@ti.com>

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
Pratyush Yadav June 23, 2022, 9:45 a.m. UTC | #2 
Hi Rahul,

On 22/06/22 04:23PM, Rahul T R wrote:
> Add support new compatible for dphy-tx on j721e
> and implement dphy ops required.
> 
> Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> ---
>  drivers/phy/cadence/Kconfig     | 10 ++++++
>  drivers/phy/cadence/cdns-dphy.c | 62 +++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/phy/cadence/Kconfig b/drivers/phy/cadence/Kconfig
> index 1adde2d99ae7..18024ac6d511 100644
> --- a/drivers/phy/cadence/Kconfig
> +++ b/drivers/phy/cadence/Kconfig
> @@ -22,6 +22,16 @@ config PHY_CADENCE_DPHY
>  	  system. If M is selected, the module will be called
>  	  cdns-dphy.
>  
> +if PHY_CADENCE_DPHY
> +
> +config PHY_CADENCE_DPHY_J721E
> +	depends on ARCH_K3 || COMPILE_TEST
> +	bool "J721E DPHY TX Wiz support"
> +	default y
> +	help
> +	  Support J721E Cadence DPHY TX Wiz configuration.
> +endif
> +
>  config PHY_CADENCE_DPHY_RX
>  	tristate "Cadence D-PHY Rx Support"
>  	depends on HAS_IOMEM && OF
> diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> index 14f951013b4f..a6b7d696f77a 100644
> --- a/drivers/phy/cadence/cdns-dphy.c
> +++ b/drivers/phy/cadence/cdns-dphy.c
> @@ -7,6 +7,7 @@
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/iopoll.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
>  #include <linux/of_device.h>
> @@ -18,6 +19,7 @@
>  
>  #define REG_WAKEUP_TIME_NS		800
>  #define DPHY_PLL_RATE_HZ		108000000
> +#define POLL_TIMEOUT_US			1000
>  
>  /* DPHY registers */
>  #define DPHY_PMA_CMN(reg)		(reg)
> @@ -62,6 +64,18 @@
>  #define DSI_NULL_FRAME_OVERHEAD		6
>  #define DSI_EOT_PKT_SIZE		4
>  
> +#define DPHY_TX_J721E_WIZ_PLL_CTRL	0xF04
> +#define DPHY_TX_J721E_WIZ_STATUS	0xF08
> +#define DPHY_TX_J721E_WIZ_RST_CTRL	0xF0C
> +#define DPHY_TX_J721E_WIZ_PSM_FREQ	0xF10
> +
> +#define DPHY_TX_J721E_WIZ_IPDIV		GENMASK(4, 0)
> +#define DPHY_TX_J721E_WIZ_OPDIV		GENMASK(13, 8)
> +#define DPHY_TX_J721E_WIZ_FBDIV		GENMASK(25, 16)
> +#define DPHY_TX_J721E_WIZ_LANE_RSTB	BIT(31)
> +#define DPHY_TX_WIZ_PLL_LOCK		BIT(31)
> +#define DPHY_TX_WIZ_O_CMN_READY		BIT(31)
> +
>  struct cdns_dphy_cfg {
>  	u8 pll_ipdiv;
>  	u8 pll_opdiv;
> @@ -210,6 +224,43 @@ static void cdns_dphy_ref_set_psm_div(struct cdns_dphy *dphy, u8 div)
>  	       dphy->regs + DPHY_PSM_CFG);
>  }
>  
> +#ifdef CONFIG_PHY_CADENCE_DPHY_J721E

Honestly, I don't think there is much use for using this config here. I 
prefer to have as little ifdefs scattered in code as possible. And I 
don't think the code you add makes much of a difference in terms of 
size.

I have not looked much into the WIZ module but the code looks fine to 
me.

> +static unsigned long cdns_dphy_j721e_get_wakeup_time_ns(struct cdns_dphy *dphy)
> +{
> +	return 1000000;
> +}
> +
> +static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
> +					const struct cdns_dphy_cfg *cfg)
> +{
> +	u32 status;
> +
> +	writel(DPHY_CMN_PWM_HIGH(6) | DPHY_CMN_PWM_LOW(0x101) |
> +	       DPHY_CMN_PWM_DIV(0x8),

Please avoid using magic numbers. Or if you are going to use them, at 
least explain in a comment what they are doing.

> +	       dphy->regs + DPHY_CMN_PWM);
> +
> +	writel((FIELD_PREP(DPHY_TX_J721E_WIZ_IPDIV, cfg->pll_ipdiv) |
> +		FIELD_PREP(DPHY_TX_J721E_WIZ_OPDIV, cfg->pll_opdiv) |
> +		FIELD_PREP(DPHY_TX_J721E_WIZ_FBDIV, cfg->pll_fbdiv)),
> +		dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL);
> +
> +	writel(DPHY_TX_J721E_WIZ_LANE_RSTB,
> +	       dphy->regs + DPHY_TX_J721E_WIZ_RST_CTRL);
> +
> +	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
> +			   (status & DPHY_TX_WIZ_PLL_LOCK), 0, POLL_TIMEOUT_US);
> +
> +	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
> +			   (status & DPHY_TX_WIZ_O_CMN_READY), 0,
> +			   POLL_TIMEOUT_US);
> +}
> +
> +static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
> +{
> +	writel(div, dphy->regs + DPHY_TX_J721E_WIZ_PSM_FREQ);
> +}
> +#endif /* !CONFIG_PHY_CADENCE_DPHY_J721E */
> +
>  /*
>   * This is the reference implementation of DPHY hooks. Specific integration of
>   * this IP may have to re-implement some of them depending on how they decided
> @@ -221,6 +272,14 @@ static const struct cdns_dphy_ops ref_dphy_ops = {
>  	.set_psm_div = cdns_dphy_ref_set_psm_div,
>  };
>  
> +#ifdef CONFIG_PHY_CADENCE_DPHY_J721E
> +static const struct cdns_dphy_ops j721e_dphy_ops = {
> +	.get_wakeup_time_ns = cdns_dphy_j721e_get_wakeup_time_ns,
> +	.set_pll_cfg = cdns_dphy_j721e_set_pll_cfg,
> +	.set_psm_div = cdns_dphy_j721e_set_psm_div,
> +};
> +#endif /* !CONFIG_PHY_CADENCE_DPHY_J721E */
> +
>  static int cdns_dphy_config_from_opts(struct phy *phy,
>  				      struct phy_configure_opts_mipi_dphy *opts,
>  				      struct cdns_dphy_cfg *cfg)
> @@ -408,6 +467,9 @@ static int cdns_dphy_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id cdns_dphy_of_match[] = {
>  	{ .compatible = "cdns,dphy", .data = &ref_dphy_ops },
> +#ifdef CONFIG_PHY_CADENCE_DPHY_J721E
> +	{ .compatible = "ti,j721e-dphy", .data = &j721e_dphy_ops },
> +#endif /* !CONFIG_PHY_CADENCE_DPHY_J721E */
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, cdns_dphy_of_match);
> -- 
> 2.36.1
>
Rahul T R June 23, 2022, 12:58 p.m. UTC | #3 
Hi Pratyush,

On 15:15-20220623, Pratyush Yadav wrote:
> Hi Rahul,
> 
> On 22/06/22 04:23PM, Rahul T R wrote:
> > Add support new compatible for dphy-tx on j721e
> > and implement dphy ops required.
> > 
> > Signed-off-by: Rahul T R <r-ravikumar@ti.com>
> > ---
> >  drivers/phy/cadence/Kconfig     | 10 ++++++
> >  drivers/phy/cadence/cdns-dphy.c | 62 +++++++++++++++++++++++++++++++++
> >  2 files changed, 72 insertions(+)
> > 
> > diff --git a/drivers/phy/cadence/Kconfig b/drivers/phy/cadence/Kconfig
> > index 1adde2d99ae7..18024ac6d511 100644
> > --- a/drivers/phy/cadence/Kconfig
> > +++ b/drivers/phy/cadence/Kconfig
> > @@ -22,6 +22,16 @@ config PHY_CADENCE_DPHY
> >  	  system. If M is selected, the module will be called
> >  	  cdns-dphy.
> >  
> > +if PHY_CADENCE_DPHY
> > +
> > +config PHY_CADENCE_DPHY_J721E
> > +	depends on ARCH_K3 || COMPILE_TEST
> > +	bool "J721E DPHY TX Wiz support"
> > +	default y
> > +	help
> > +	  Support J721E Cadence DPHY TX Wiz configuration.
> > +endif
> > +
> >  config PHY_CADENCE_DPHY_RX
> >  	tristate "Cadence D-PHY Rx Support"
> >  	depends on HAS_IOMEM && OF
> > diff --git a/drivers/phy/cadence/cdns-dphy.c b/drivers/phy/cadence/cdns-dphy.c
> > index 14f951013b4f..a6b7d696f77a 100644
> > --- a/drivers/phy/cadence/cdns-dphy.c
> > +++ b/drivers/phy/cadence/cdns-dphy.c
> > @@ -7,6 +7,7 @@
> >  #include <linux/bitops.h>
> >  #include <linux/clk.h>
> >  #include <linux/io.h>
> > +#include <linux/iopoll.h>
> >  #include <linux/module.h>
> >  #include <linux/of_address.h>
> >  #include <linux/of_device.h>
> > @@ -18,6 +19,7 @@
> >  
> >  #define REG_WAKEUP_TIME_NS		800
> >  #define DPHY_PLL_RATE_HZ		108000000
> > +#define POLL_TIMEOUT_US			1000
> >  
> >  /* DPHY registers */
> >  #define DPHY_PMA_CMN(reg)		(reg)
> > @@ -62,6 +64,18 @@
> >  #define DSI_NULL_FRAME_OVERHEAD		6
> >  #define DSI_EOT_PKT_SIZE		4
> >  
> > +#define DPHY_TX_J721E_WIZ_PLL_CTRL	0xF04
> > +#define DPHY_TX_J721E_WIZ_STATUS	0xF08
> > +#define DPHY_TX_J721E_WIZ_RST_CTRL	0xF0C
> > +#define DPHY_TX_J721E_WIZ_PSM_FREQ	0xF10
> > +
> > +#define DPHY_TX_J721E_WIZ_IPDIV		GENMASK(4, 0)
> > +#define DPHY_TX_J721E_WIZ_OPDIV		GENMASK(13, 8)
> > +#define DPHY_TX_J721E_WIZ_FBDIV		GENMASK(25, 16)
> > +#define DPHY_TX_J721E_WIZ_LANE_RSTB	BIT(31)
> > +#define DPHY_TX_WIZ_PLL_LOCK		BIT(31)
> > +#define DPHY_TX_WIZ_O_CMN_READY		BIT(31)
> > +
> >  struct cdns_dphy_cfg {
> >  	u8 pll_ipdiv;
> >  	u8 pll_opdiv;
> > @@ -210,6 +224,43 @@ static void cdns_dphy_ref_set_psm_div(struct cdns_dphy *dphy, u8 div)
> >  	       dphy->regs + DPHY_PSM_CFG);
> >  }
> >  
> > +#ifdef CONFIG_PHY_CADENCE_DPHY_J721E
> 
> Honestly, I don't think there is much use for using this config here. I 
> prefer to have as little ifdefs scattered in code as possible. And I 
> don't think the code you add makes much of a difference in terms of 
> size.
> 
> I have not looked much into the WIZ module but the code looks fine to 
> me.
> 
> > +static unsigned long cdns_dphy_j721e_get_wakeup_time_ns(struct cdns_dphy *dphy)
> > +{
> > +	return 1000000;
> > +}
> > +
> > +static void cdns_dphy_j721e_set_pll_cfg(struct cdns_dphy *dphy,
> > +					const struct cdns_dphy_cfg *cfg)
> > +{
> > +	u32 status;
> > +
> > +	writel(DPHY_CMN_PWM_HIGH(6) | DPHY_CMN_PWM_LOW(0x101) |
> > +	       DPHY_CMN_PWM_DIV(0x8),
> 
> Please avoid using magic numbers. Or if you are going to use them, at 
> least explain in a comment what they are doing.
> 

Thanks for the review!
I have addressed your review comments
and sent a v4
Please review,

Regards
Rahul T R
> > +	       dphy->regs + DPHY_CMN_PWM);
> > +
> > +	writel((FIELD_PREP(DPHY_TX_J721E_WIZ_IPDIV, cfg->pll_ipdiv) |
> > +		FIELD_PREP(DPHY_TX_J721E_WIZ_OPDIV, cfg->pll_opdiv) |
> > +		FIELD_PREP(DPHY_TX_J721E_WIZ_FBDIV, cfg->pll_fbdiv)),
> > +		dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL);
> > +
> > +	writel(DPHY_TX_J721E_WIZ_LANE_RSTB,
> > +	       dphy->regs + DPHY_TX_J721E_WIZ_RST_CTRL);
> > +
> > +	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_PLL_CTRL, status,
> > +			   (status & DPHY_TX_WIZ_PLL_LOCK), 0, POLL_TIMEOUT_US);
> > +
> > +	readl_poll_timeout(dphy->regs + DPHY_TX_J721E_WIZ_STATUS, status,
> > +			   (status & DPHY_TX_WIZ_O_CMN_READY), 0,
> > +			   POLL_TIMEOUT_US);
> > +}
> > +
> > +static void cdns_dphy_j721e_set_psm_div(struct cdns_dphy *dphy, u8 div)
> > +{
> > +	writel(div, dphy->regs + DPHY_TX_J721E_WIZ_PSM_FREQ);
> > +}
> > +#endif /* !CONFIG_PHY_CADENCE_DPHY_J721E */
> > +
> >  /*
> >   * This is the reference implementation of DPHY hooks. Specific integration of
> >   * this IP may have to re-implement some of them depending on how they decided
> > @@ -221,6 +272,14 @@ static const struct cdns_dphy_ops ref_dphy_ops = {
> >  	.set_psm_div = cdns_dphy_ref_set_psm_div,
> >  };
> >  
> > +#ifdef CONFIG_PHY_CADENCE_DPHY_J721E
> > +static const struct cdns_dphy_ops j721e_dphy_ops = {
> > +	.get_wakeup_time_ns = cdns_dphy_j721e_get_wakeup_time_ns,
> > +	.set_pll_cfg = cdns_dphy_j721e_set_pll_cfg,
> > +	.set_psm_div = cdns_dphy_j721e_set_psm_div,
> > +};
> > +#endif /* !CONFIG_PHY_CADENCE_DPHY_J721E */
> > +
> >  static int cdns_dphy_config_from_opts(struct phy *phy,
> >  				      struct phy_configure_opts_mipi_dphy *opts,
> >  				      struct cdns_dphy_cfg *cfg)
> > @@ -408,6 +467,9 @@ static int cdns_dphy_remove(struct platform_device *pdev)
> >  
> >  static const struct of_device_id cdns_dphy_of_match[] = {
> >  	{ .compatible = "cdns,dphy", .data = &ref_dphy_ops },
> > +#ifdef CONFIG_PHY_CADENCE_DPHY_J721E
> > +	{ .compatible = "ti,j721e-dphy", .data = &j721e_dphy_ops },
> > +#endif /* !CONFIG_PHY_CADENCE_DPHY_J721E */
> >  	{ /* sentinel */ },
> >  };
> >  MODULE_DEVICE_TABLE(of, cdns_dphy_of_match);
> > -- 
> > 2.36.1
> > 
> 
> -- 
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.