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@xxxxxx>
> > ---
> > 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.
