Re: [PATCH v3 3/3] phy: cdns-dphy: Add support for DPHY TX on J721e

[Pratyush Yadav <p.yadav@ti.com>]

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
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.