Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 23 Jun 2022 18:28:41 +0530
From:   Rahul T R <r-ravikumar@ti.com>
To:     Pratyush Yadav <p.yadav@ti.com>
CC:     <linux-phy@lists.infradead.org>, <kishon@ti.com>,
        <vkoul@kernel.org>, <robh+dt@kernel.org>,
        <krzysztof.kozlowski+dt@linaro.org>,
        <laurent.pinchart@ideasonboard.com>,
        <tomi.valkeinen@ideasonboard.com>, <linux-kernel@vger.kernel.org>,
        <jpawar@cadence.com>, <sjakhade@cadence.com>, <mparab@cadence.com>,
        <devicetree@vger.kernel.org>, <vigneshr@ti.com>,
        <lee.jones@linaro.org>
Subject: Re: [PATCH v3 3/3] phy: cdns-dphy: Add support for DPHY TX on J721e
Message-ID: <20220623125839.pkrt55ftvvjomzrc@uda0490373>
References: <20220622105311.21415-1-r-ravikumar@ti.com>
 <20220622105311.21415-4-r-ravikumar@ti.com>
 <20220623094523.ccrsmw477a5btgcf@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20220623094523.ccrsmw477a5btgcf@ti.com>
User-Agent: NeoMutt/20171215
Precedence: bulk

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.