Messages in this thread | | | Subject | Re: [PATCH v2 12/26] drm/sun4i: Add support for multiple DW HDMI PHY clock parents | From | Sergey Suloev <> | Date | Fri, 18 May 2018 18:09:40 +0300 |
| |
Hi, guys,
On 05/18/2018 05:46 PM, Jernej Škrabec wrote: > Hi, > > Dne petek, 18. maj 2018 ob 12:01:16 CEST je Maxime Ripard napisal(a): >> On Fri, May 18, 2018 at 03:15:22PM +0530, Jagan Teki wrote: >>> From: Jernej Skrabec <jernej.skrabec@siol.net> >>> >>> Some SoCs with DW HDMI have multiple possible clock parents, like A64 >>> and R40. >>> >>> Expand HDMI PHY clock driver to support second clock parent. >>> >>> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>> --- >>> Changes for v2: >>> - new patch >>> >>> drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 9 ++- >>> drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 33 ++++++++--- >>> drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 89 >>> ++++++++++++++++++++++-------- 3 files changed, 96 insertions(+), 35 >>> deletions(-) >>> >>> diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h >>> b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 79154f0f674a..303189d6602c >>> 100644 >>> --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h >>> +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h >>> @@ -98,7 +98,8 @@ >>> >>> #define SUN8I_HDMI_PHY_PLL_CFG1_LDO2_EN BIT(29) >>> #define SUN8I_HDMI_PHY_PLL_CFG1_LDO1_EN BIT(28) >>> #define SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 BIT(27) >>> >>> -#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL BIT(26) >>> +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK BIT(26) >>> +#define SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT 26 >>> >>> #define SUN8I_HDMI_PHY_PLL_CFG1_PLLEN BIT(25) >>> #define SUN8I_HDMI_PHY_PLL_CFG1_LDO_VSET(x) ((x) << 22) >>> #define SUN8I_HDMI_PHY_PLL_CFG1_UNKNOWN(x) ((x) << 20) >>> >>> @@ -146,7 +147,7 @@ >>> >>> struct sun8i_hdmi_phy; >>> >>> struct sun8i_hdmi_phy_variant { >>> >>> - bool has_phy_clk; >>> + int phy_clk_num; >>> >>> void (*phy_init)(struct sun8i_hdmi_phy *phy); >>> void (*phy_disable)(struct dw_hdmi *hdmi, >>> >>> struct sun8i_hdmi_phy *phy); >>> >>> @@ -160,6 +161,7 @@ struct sun8i_hdmi_phy { >>> >>> struct clk *clk_mod; >>> struct clk *clk_phy; >>> struct clk *clk_pll0; >>> >>> + struct clk *clk_pll1; >>> >>> unsigned int rcal; >>> struct regmap *regs; >>> struct reset_control *rst_phy; >>> >>> @@ -188,6 +190,7 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi >>> *hdmi); >>> >>> void sun8i_hdmi_phy_init(struct sun8i_hdmi_phy *phy); >>> const struct dw_hdmi_phy_ops *sun8i_hdmi_phy_get_ops(void); >>> >>> -int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev); >>> +int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev, >>> + int clk_num); >>> >>> #endif /* _SUN8I_DW_HDMI_H_ */ >>> >>> diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c >>> b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 5a52fc489a9d..0eadf087fc46 >>> 100644 >>> --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c >>> +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c >>> @@ -183,7 +183,13 @@ static int sun8i_hdmi_phy_config_h3(struct dw_hdmi >>> *hdmi,> >>> regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG, >>> >>> SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK, 0); >>> >>> - regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, pll_cfg1_init); >>> + /* >>> + * NOTE: We have to be careful not to overwrite PHY parent >>> + * clock selection bit and clock divider. >>> + */ >>> + regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, >>> + (u32)~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, >>> + pll_cfg1_init); >>> >>> regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, >>> >>> (u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK, >>> pll_cfg2_init); >>> >>> @@ -232,7 +238,7 @@ static int sun8i_hdmi_phy_config(struct dw_hdmi *hdmi, >>> void *data,> >>> regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_DBG_CTRL_REG, >>> >>> SUN8I_HDMI_PHY_DBG_CTRL_POL_MASK, val); >>> >>> - if (phy->variant->has_phy_clk) >>> + if (phy->variant->phy_clk_num) >>> >>> clk_set_rate(phy->clk_phy, mode->crtc_clock * 1000); >>> >>> return phy->variant->phy_config(hdmi, phy, mode->crtc_clock * 1000); >>> >>> @@ -393,7 +399,7 @@ static const struct sun8i_hdmi_phy_variant >>> sun8i_a83t_hdmi_phy = {> >>> }; >>> >>> static const struct sun8i_hdmi_phy_variant sun8i_h3_hdmi_phy = { >>> >>> - .has_phy_clk = true, >>> + .phy_clk_num = 1, >>> >>> .phy_init = &sun8i_hdmi_phy_init_h3, >>> .phy_disable = &sun8i_hdmi_phy_disable_h3, >>> .phy_config = &sun8i_hdmi_phy_config_h3, >>> >>> @@ -464,7 +470,7 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, >>> struct device_node *node)> >>> goto err_put_clk_bus; >>> >>> } >>> >>> - if (phy->variant->has_phy_clk) { >>> + if (phy->variant->phy_clk_num) { >>> >>> phy->clk_pll0 = of_clk_get_by_name(node, "pll-0"); >>> if (IS_ERR(phy->clk_pll0)) { >>> >>> dev_err(dev, "Could not get pll-0 clock\n"); >>> >>> @@ -472,7 +478,16 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, >>> struct device_node *node)> >>> goto err_put_clk_mod; >>> >>> } >>> >>> - ret = sun8i_phy_clk_create(phy, dev); >>> + if (phy->variant->phy_clk_num) { >>> + phy->clk_pll1 = of_clk_get_by_name(node, "pll-1"); >>> + if (IS_ERR(phy->clk_pll1)) { >>> + dev_err(dev, "Could not get pll-1 clock\n"); >>> + ret = PTR_ERR(phy->clk_pll1); >>> + goto err_put_clk_mod; >>> + } >>> + } >>> + >> You have a bug here. If phy_clk_num == 1, you'll still try to lookup >> pll-1. > This is actually WIP patch taken from my github. This issue was fixed already > locally on disk. I thought Jagan will not use it until SRAM C patches land. > >> And this is a bit sloppy, since if phy_clk_num == 3, you won't try to >> lookup pll-2 either. > It is highly unlikely this will be higher than 2, at least for this HDMI PHY, > since it has only 1 bit reserved for parent selection. But since I have to fix > it, I'll add ">= 2" > >>> + ret = sun8i_phy_clk_create(phy, dev, phy->variant->phy_clk_num); >>> >>> if (ret) { >>> >>> dev_err(dev, "Couldn't create the PHY clock\n"); >>> goto err_put_clk_pll0; >>> >>> @@ -515,8 +530,8 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, >>> struct device_node *node)> >>> err_put_rst_phy: >>> reset_control_put(phy->rst_phy); >>> >>> err_put_clk_pll0: >>> - if (phy->variant->has_phy_clk) >>> - clk_put(phy->clk_pll0); >>> + clk_put(phy->clk_pll0); >>> + clk_put(phy->clk_pll1); >>> >>> err_put_clk_mod: >>> clk_put(phy->clk_mod); >>> >>> err_put_clk_bus: >>> @@ -536,8 +551,8 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi) >>> >>> reset_control_put(phy->rst_phy); >>> >>> - if (phy->variant->has_phy_clk) >>> - clk_put(phy->clk_pll0); >>> + clk_put(phy->clk_pll0); >>> + clk_put(phy->clk_pll1); >>> >>> clk_put(phy->clk_mod); >>> clk_put(phy->clk_bus); >>> >>> } >>> >>> diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c >>> b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c index >>> faea449812f8..85b12fc96dbc 100644 >>> --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c >>> +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c >>> @@ -22,29 +22,36 @@ static int sun8i_phy_clk_determine_rate(struct clk_hw >>> *hw,> >>> { >>> >>> unsigned long rate = req->rate; >>> unsigned long best_rate = 0; >>> >>> - struct clk_hw *parent; >>> + struct clk_hw *best_parent = NULL; >>> + struct clk_hw *parent = NULL; >>> >>> int best_div = 1; >>> >>> - int i; >>> + int i, p; >>> >>> - parent = clk_hw_get_parent(hw); >>> - >>> - for (i = 1; i <= 16; i++) { >>> - unsigned long ideal = rate * i; >>> - unsigned long rounded; >>> - >>> - rounded = clk_hw_round_rate(parent, ideal); >>> - >>> - if (rounded == ideal) { >>> - best_rate = rounded; >>> - best_div = i; >>> - break; >>> - } >>> + for (p = 0; p < clk_hw_get_num_parents(hw); p++) { >>> + parent = clk_hw_get_parent_by_index(hw, p); >>> + if (!parent) >>> + continue; >>> >>> - if (!best_rate || >>> - abs(rate - rounded / i) < >>> - abs(rate - best_rate / best_div)) { >>> - best_rate = rounded; >>> - best_div = i; >>> + for (i = 1; i <= 16; i++) { >>> + unsigned long ideal = rate * i; >>> + unsigned long rounded; >>> + >>> + rounded = clk_hw_round_rate(parent, ideal); >>> + >>> + if (rounded == ideal) { >>> + best_rate = rounded; >>> + best_div = i; >>> + best_parent = parent; >>> + break; >>> + } >>> + >>> + if (!best_rate || >>> + abs(rate - rounded / i) < >>> + abs(rate - best_rate / best_div)) { >>> + best_rate = rounded; >>> + best_div = i; >>> + best_parent = parent; >>> + } >>> >>> } >>> >>> } >>> >>> @@ -95,22 +102,58 @@ static int sun8i_phy_clk_set_rate(struct clk_hw *hw, >>> unsigned long rate,> >>> return 0; >>> >>> } >>> >>> +static u8 sun8i_phy_clk_get_parent(struct clk_hw *hw) >>> +{ >>> + struct sun8i_phy_clk *priv = hw_to_phy_clk(hw); >>> + u32 reg; >>> + >>> + regmap_read(priv->phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, ®); >>> + reg = (reg & SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK) >> >>> + SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT; >>> + >>> + return reg; >>> +} >>> + >>> +static int sun8i_phy_clk_set_parent(struct clk_hw *hw, u8 index) >>> +{ >>> + struct sun8i_phy_clk *priv = hw_to_phy_clk(hw); >>> + >>> + if (index > 1) >>> + return -EINVAL; >>> + >>> + regmap_update_bits(priv->phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, >>> + SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, >>> + index << SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_SHIFT); >>> + >>> + return 0; >>> +} >>> + >> The DT bindings changes and the clk changes should be part of separate >> patches. > By DT bindings changes you mean code which reads DT and not DT documentation, > right? > > Ok, I'll split it. > > BTW, I'll resend fixed version of this patch for my R40 HDMI series, since > there is nothing to hold it back, unlike for this. > > Best regards, > Jernej > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
you have been talking about SRAM patches, required for A64 DE2, for about a half a year. May I ask you to explain in a couple of words why they are so important ? I am really curious because I have DE2 already working on my A64 without those magic patches..
Thanks, Sergey
| |