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...@nxp.com>, but also cleaned up
> quite a bit and extended to support runtime PM.
>
> Signed-off-by: Lucas Stach <l.st...@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
