Re: [PATCH v0 07/10] phy: freescale: add Samsung HDMI PHY

Hi Lucas,

On Mi, 2022-04-06 at 18:01 +0200, Lucas Stach wrote:

This adds the driver for the Samsung HDMI PHY found on the i.MX8MP SoC.

Heavily based on the PHY implementation in the downstream kernel written by Sandor Yu Sandor.yu@nxp.com, but also cleaned up quite a bit and extended to support runtime PM.

Signed-off-by: Lucas Stach l.stach@pengutronix.de

FIXME: The PHY configuration could be cleaned up further, it currently has a lot of register writes that are same across all supported modes.

Agreed.

[...]

---

drivers/phy/freescale/Kconfig | 7 +  drivers/phy/freescale/Makefile | 1 +  drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 1145 ++++++++++++++++++  3 files changed, 1153 insertions(+)  create mode 100644 drivers/phy/freescale/phy-fsl-samsung-hdmi.c

diff --git a/drivers/phy/freescale/Kconfig b/drivers/phy/freescale/Kconfig index f9c54cd02036..f80c92eb7c55 100644 --- a/drivers/phy/freescale/Kconfig +++ b/drivers/phy/freescale/Kconfig @@ -26,6 +26,13 @@ config PHY_FSL_IMX8M_PCIE   Enable this to add support for the PCIE PHY as found on   i.MX8M family of SOCs.   +config PHY_FSL_SAMSUNG_HDMI_PHY

• tristate "Samsung HDMI PHY support"
• depends on OF && HAS_IOMEM
• select GENERIC_PHY

Why select GENERIC_PHY when all the driver does is register a clock?

[...]

+struct fsl_samsung_hdmi_phy {

• struct device *dev;
• void __iomem *regs;
• struct clk *apbclk;
• struct clk *refclk;

refclk isn't really used beyond phy_clk_register, it doesn't have to be stored in struct fsl_samsung_hdmi_phy.

• /* clk provider */
• struct clk_hw hw;
• const struct phy_config *cur_cfg;

+};

+static inline struct fsl_samsung_hdmi_phy * +to_fsl_samsung_hdmi_phy(struct clk_hw *hw) +{

• return container_of(hw, struct fsl_samsung_hdmi_phy, hw);

+}

+static void fsl_samsung_hdmi_phy_configure(struct fsl_samsung_hdmi_phy *phy,

• 			  const struct phy_config *cfg)
  

+{

• int i;
• /* HDMI PHY init */
• writeb(REG33_FIX_DA, phy->regs + PHY_REG_33);
• for (i = 0; i < PHY_PLL_REGS_NUM; i++)

• writeb(cfg->regs[i], phy->regs + i * 4);
  

• writeb(REG33_FIX_DA | REG33_MODE_SET_DONE , phy->regs + PHY_REG_33);

+}

+static int phy_clk_prepare(struct clk_hw *hw) +{

• struct fsl_samsung_hdmi_phy *phy = to_fsl_samsung_hdmi_phy(hw);
• int ret = 0;
• u8 val;
• return 0;

I'd say remove this line or pyh_clk_prepare().

• ret = readb_poll_timeout(phy->regs + PHY_REG_34, val,

• 		 val & REG34_PLL_LOCK,
  

• 		 20, 20000);
  

• if (ret)

• dev_err(phy->dev, "PLL failed to lock\n");
  

• return ret;

+}

+static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,

• 			 unsigned long parent_rate)
  

+{

• struct fsl_samsung_hdmi_phy *phy = to_fsl_samsung_hdmi_phy(hw);
• if (!phy->cur_cfg)

• return 0;
  

• return phy->cur_cfg->clk_rate;

+}

+static long phy_clk_round_rate(struct clk_hw *hw,

• 	       unsigned long rate, unsigned long *parent_rate)
  

+{

• const struct phy_config *phy_cfg = phy_pll_cfg;
• for (; phy_cfg->clk_rate != 0; phy_cfg++)

• if (phy_cfg->clk_rate == rate)
  

* @round_rate: Given a target rate as input, returns the closest rate actually * supported by the clock. The parent rate is an input/output * parameter.

This should round, not -EINVAL on unsupported rates.

• 	break;
  

• if (phy_cfg->clk_rate == 0)

• return -EINVAL;
  

• return phy_cfg->clk_rate;

+}

+static int phy_clk_set_rate(struct clk_hw *hw,

• 	    unsigned long rate, unsigned long parent_rate)
  

+{

• struct fsl_samsung_hdmi_phy *phy = to_fsl_samsung_hdmi_phy(hw);
• const struct phy_config *phy_cfg = phy_pll_cfg;
• int ret = 0;

Unnecessary initialization.

• u8 val;
• for (; phy_cfg->clk_rate != 0; phy_cfg++)

• if (phy_cfg->clk_rate == rate)
  

• 	break;
  

• if (phy_cfg->clk_rate == 0)

• return -EINVAL;
  

• phy->cur_cfg = phy_cfg;
• fsl_samsung_hdmi_phy_configure(phy, phy_cfg);
• ret = readb_poll_timeout(phy->regs + PHY_REG_34, val,

• 		 val & REG34_PLL_LOCK,
  

• 		 50, 20000);
  

• if (ret)

• dev_err(phy->dev, "PLL failed to lock\n");
  

• return ret;

+}

+static const struct clk_ops phy_clk_ops = {

• .prepare = phy_clk_prepare,
• .recalc_rate = phy_clk_recalc_rate,
• .round_rate = phy_clk_round_rate,
• .set_rate = phy_clk_set_rate,

+};

+static int phy_clk_register(struct fsl_samsung_hdmi_phy *phy) +{

• struct device *dev = phy->dev;
• struct device_node *np = dev->of_node;
• struct clk_init_data init;
• const char *parent_name;
• struct clk *phyclk;
• int ret;
• parent_name = __clk_get_name(phy->refclk);
• init.parent_names = &parent_name;
• init.num_parents = 1;
• init.flags = 0;
• init.name = "hdmi_pclk";
• init.ops = &phy_clk_ops;
• phy->hw.init = &init;
• phyclk = devm_clk_register(dev, &phy->hw);
• if (IS_ERR(phyclk))

• return dev_err_probe(dev, PTR_ERR(phyclk),
  

• 		     "failed to register clock\n");
  

• ret = of_clk_add_provider(np, of_clk_src_simple_get, phyclk);
• if (ret)

• return dev_err_probe(dev, ret,
  

• 		     "failed to register clock provider\n");
  

• return 0;

+}

+static int fsl_samsung_hdmi_phy_probe(struct platform_device *pdev) +{

• struct fsl_samsung_hdmi_phy *phy;
• int ret;
• phy = devm_kzalloc(&pdev->dev, sizeof(*phy), GFP_KERNEL);
• if (!phy)

• return -ENOMEM;
  

• platform_set_drvdata(pdev, phy);
• phy->dev = &pdev->dev;
• phy->regs = devm_platform_ioremap_resource(pdev, 0);
• if (IS_ERR(phy->regs))

• return PTR_ERR(phy->regs);
  

• phy->apbclk = devm_clk_get(phy->dev, "apb");
• if (IS_ERR(phy->apbclk))

• return dev_err_probe(phy->dev, PTR_ERR(phy->apbclk),
  

• 		     "failed to get apb clk\n");
  

• phy->refclk = devm_clk_get(phy->dev, "ref");
• if (IS_ERR(phy->refclk))

• return dev_err_probe(phy->dev, PTR_ERR(phy->refclk),
  

• 		     "failed to get ref clk\n");
  

• ret = clk_prepare_enable(phy->apbclk);
• if (ret) {

• dev_err(phy->dev, "failed to enable apbclk\n");
  

• return ret;
  

• }
• pm_runtime_get_noresume(phy->dev);
• pm_runtime_set_active(phy->dev);
• pm_runtime_enable(phy->dev);
• ret = phy_clk_register(phy);
• if (ret) {

• dev_err(&pdev->dev, "register clk failed\n");
  

• goto register_clk_failed;
  

• }
• pm_runtime_put(phy->dev);
• return 0;

+register_clk_failed:

• clk_disable_unprepare(phy->apbclk);
• return ret;

+}

+static int fsl_samsung_hdmi_phy_remove(struct platform_device *pdev) +{

• of_clk_del_provider(pdev->dev.of_node);
• return 0;

+}

+#ifdef CONFIG_PM +static int fsl_samsung_hdmi_phy_suspend(struct device *dev) +{

• struct fsl_samsung_hdmi_phy *phy = dev_get_drvdata(dev);
• clk_disable_unprepare(phy->apbclk);
• return 0;

+}

+static int fsl_samsung_hdmi_phy_resume(struct device *dev) +{

• struct fsl_samsung_hdmi_phy *phy = dev_get_drvdata(dev);
• int ret;
• ret = clk_prepare_enable(phy->apbclk);
• if (ret) {

• dev_err(phy->dev, "failed to enable apbclk\n");
  

• return ret;
  

• }
• if (phy->cur_cfg)

• fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
  

Not checking PLL lock during resume?

regards Philipp