| Message ID | 20180519183127.2718-13-jernej.skrabec@siol.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jernej,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on drm/drm-next]
[also build test WARNING on v4.17-rc6 next-20180517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jernej-Skrabec/Add-support-for-R40-HDMI-pipeline/20180521-131839
base: git://people.freedesktop.org/~airlied/linux.git drm-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm64
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h:12:0,
from drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c:9:
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c: In function 'sun8i_hdmi_phy_config_h3':
>> drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c:191:7: warning: large integer implicitly truncated to unsigned type [-Woverflow]
~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK,
^
include/linux/regmap.h:76:36: note: in definition of macro 'regmap_update_bits'
regmap_update_bits_base(map, reg, mask, val, NULL, false, false)
^~~~
vim +191 drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c
8
> 9 #include "sun8i_dw_hdmi.h"
10
11 /*
12 * Address can be actually any value. Here is set to same value as
13 * it is set in BSP driver.
14 */
15 #define I2C_ADDR 0x69
16
17 static int sun8i_hdmi_phy_config_a83t(struct dw_hdmi *hdmi,
18 struct sun8i_hdmi_phy *phy,
19 unsigned int clk_rate)
20 {
21 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_REXT_CTRL_REG,
22 SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN,
23 SUN8I_HDMI_PHY_REXT_CTRL_REXT_EN);
24
25 /* power down */
26 dw_hdmi_phy_gen2_txpwron(hdmi, 0);
27 dw_hdmi_phy_gen2_pddq(hdmi, 1);
28
29 dw_hdmi_phy_reset(hdmi);
30
31 dw_hdmi_phy_gen2_pddq(hdmi, 0);
32
33 dw_hdmi_phy_i2c_set_addr(hdmi, I2C_ADDR);
34
35 /*
36 * Values are taken from BSP HDMI driver. Although AW didn't
37 * release any documentation, explanation of this values can
38 * be found in i.MX 6Dual/6Quad Reference Manual.
39 */
40 if (clk_rate <= 27000000) {
41 dw_hdmi_phy_i2c_write(hdmi, 0x01e0, 0x06);
42 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x15);
43 dw_hdmi_phy_i2c_write(hdmi, 0x08da, 0x10);
44 dw_hdmi_phy_i2c_write(hdmi, 0x0007, 0x19);
45 dw_hdmi_phy_i2c_write(hdmi, 0x0318, 0x0e);
46 dw_hdmi_phy_i2c_write(hdmi, 0x8009, 0x09);
47 } else if (clk_rate <= 74250000) {
48 dw_hdmi_phy_i2c_write(hdmi, 0x0540, 0x06);
49 dw_hdmi_phy_i2c_write(hdmi, 0x0005, 0x15);
50 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x10);
51 dw_hdmi_phy_i2c_write(hdmi, 0x0007, 0x19);
52 dw_hdmi_phy_i2c_write(hdmi, 0x02b5, 0x0e);
53 dw_hdmi_phy_i2c_write(hdmi, 0x8009, 0x09);
54 } else if (clk_rate <= 148500000) {
55 dw_hdmi_phy_i2c_write(hdmi, 0x04a0, 0x06);
56 dw_hdmi_phy_i2c_write(hdmi, 0x000a, 0x15);
57 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x10);
58 dw_hdmi_phy_i2c_write(hdmi, 0x0002, 0x19);
59 dw_hdmi_phy_i2c_write(hdmi, 0x0021, 0x0e);
60 dw_hdmi_phy_i2c_write(hdmi, 0x8029, 0x09);
61 } else {
62 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x06);
63 dw_hdmi_phy_i2c_write(hdmi, 0x000f, 0x15);
64 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x10);
65 dw_hdmi_phy_i2c_write(hdmi, 0x0002, 0x19);
66 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x0e);
67 dw_hdmi_phy_i2c_write(hdmi, 0x802b, 0x09);
68 }
69
70 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x1e);
71 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x13);
72 dw_hdmi_phy_i2c_write(hdmi, 0x0000, 0x17);
73
74 dw_hdmi_phy_gen2_txpwron(hdmi, 1);
75
76 return 0;
77 }
78
79 static int sun8i_hdmi_phy_config_h3(struct dw_hdmi *hdmi,
80 struct sun8i_hdmi_phy *phy,
81 unsigned int clk_rate)
82 {
83 u32 pll_cfg1_init;
84 u32 pll_cfg2_init;
85 u32 ana_cfg1_end;
86 u32 ana_cfg2_init;
87 u32 ana_cfg3_init;
88 u32 b_offset = 0;
89 u32 val;
90
91 /* bandwidth / frequency independent settings */
92
93 pll_cfg1_init = SUN8I_HDMI_PHY_PLL_CFG1_LDO2_EN |
94 SUN8I_HDMI_PHY_PLL_CFG1_LDO1_EN |
95 SUN8I_HDMI_PHY_PLL_CFG1_LDO_VSET(7) |
96 SUN8I_HDMI_PHY_PLL_CFG1_UNKNOWN(1) |
97 SUN8I_HDMI_PHY_PLL_CFG1_PLLDBEN |
98 SUN8I_HDMI_PHY_PLL_CFG1_CS |
99 SUN8I_HDMI_PHY_PLL_CFG1_CP_S(2) |
100 SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(63) |
101 SUN8I_HDMI_PHY_PLL_CFG1_BWS;
102
103 pll_cfg2_init = SUN8I_HDMI_PHY_PLL_CFG2_SV_H |
104 SUN8I_HDMI_PHY_PLL_CFG2_VCOGAIN_EN |
105 SUN8I_HDMI_PHY_PLL_CFG2_SDIV2;
106
107 ana_cfg1_end = SUN8I_HDMI_PHY_ANA_CFG1_REG_SVBH(1) |
108 SUN8I_HDMI_PHY_ANA_CFG1_AMP_OPT |
109 SUN8I_HDMI_PHY_ANA_CFG1_EMP_OPT |
110 SUN8I_HDMI_PHY_ANA_CFG1_AMPCK_OPT |
111 SUN8I_HDMI_PHY_ANA_CFG1_EMPCK_OPT |
112 SUN8I_HDMI_PHY_ANA_CFG1_ENRCAL |
113 SUN8I_HDMI_PHY_ANA_CFG1_ENCALOG |
114 SUN8I_HDMI_PHY_ANA_CFG1_REG_SCKTMDS |
115 SUN8I_HDMI_PHY_ANA_CFG1_TMDSCLK_EN |
116 SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK |
117 SUN8I_HDMI_PHY_ANA_CFG1_TXEN_ALL |
118 SUN8I_HDMI_PHY_ANA_CFG1_BIASEN_TMDSCLK |
119 SUN8I_HDMI_PHY_ANA_CFG1_BIASEN_TMDS2 |
120 SUN8I_HDMI_PHY_ANA_CFG1_BIASEN_TMDS1 |
121 SUN8I_HDMI_PHY_ANA_CFG1_BIASEN_TMDS0 |
122 SUN8I_HDMI_PHY_ANA_CFG1_ENP2S_TMDS2 |
123 SUN8I_HDMI_PHY_ANA_CFG1_ENP2S_TMDS1 |
124 SUN8I_HDMI_PHY_ANA_CFG1_ENP2S_TMDS0 |
125 SUN8I_HDMI_PHY_ANA_CFG1_CKEN |
126 SUN8I_HDMI_PHY_ANA_CFG1_LDOEN |
127 SUN8I_HDMI_PHY_ANA_CFG1_ENVBS |
128 SUN8I_HDMI_PHY_ANA_CFG1_ENBI;
129
130 ana_cfg2_init = SUN8I_HDMI_PHY_ANA_CFG2_M_EN |
131 SUN8I_HDMI_PHY_ANA_CFG2_REG_DENCK |
132 SUN8I_HDMI_PHY_ANA_CFG2_REG_DEN |
133 SUN8I_HDMI_PHY_ANA_CFG2_REG_CKSS(1) |
134 SUN8I_HDMI_PHY_ANA_CFG2_REG_CSMPS(1);
135
136 ana_cfg3_init = SUN8I_HDMI_PHY_ANA_CFG3_REG_WIRE(0x3e0) |
137 SUN8I_HDMI_PHY_ANA_CFG3_SDAEN |
138 SUN8I_HDMI_PHY_ANA_CFG3_SCLEN;
139
140 /* bandwidth / frequency dependent settings */
141 if (clk_rate <= 27000000) {
142 pll_cfg1_init |= SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 |
143 SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(32);
144 pll_cfg2_init |= SUN8I_HDMI_PHY_PLL_CFG2_VCO_S(4) |
145 SUN8I_HDMI_PHY_PLL_CFG2_S(4);
146 ana_cfg1_end |= SUN8I_HDMI_PHY_ANA_CFG1_REG_CALSW;
147 ana_cfg2_init |= SUN8I_HDMI_PHY_ANA_CFG2_REG_SLV(4) |
148 SUN8I_HDMI_PHY_ANA_CFG2_REG_RESDI(phy->rcal);
149 ana_cfg3_init |= SUN8I_HDMI_PHY_ANA_CFG3_REG_AMPCK(3) |
150 SUN8I_HDMI_PHY_ANA_CFG3_REG_AMP(5);
151 } else if (clk_rate <= 74250000) {
152 pll_cfg1_init |= SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 |
153 SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(32);
154 pll_cfg2_init |= SUN8I_HDMI_PHY_PLL_CFG2_VCO_S(4) |
155 SUN8I_HDMI_PHY_PLL_CFG2_S(5);
156 ana_cfg1_end |= SUN8I_HDMI_PHY_ANA_CFG1_REG_CALSW;
157 ana_cfg2_init |= SUN8I_HDMI_PHY_ANA_CFG2_REG_SLV(4) |
158 SUN8I_HDMI_PHY_ANA_CFG2_REG_RESDI(phy->rcal);
159 ana_cfg3_init |= SUN8I_HDMI_PHY_ANA_CFG3_REG_AMPCK(5) |
160 SUN8I_HDMI_PHY_ANA_CFG3_REG_AMP(7);
161 } else if (clk_rate <= 148500000) {
162 pll_cfg1_init |= SUN8I_HDMI_PHY_PLL_CFG1_HV_IS_33 |
163 SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(32);
164 pll_cfg2_init |= SUN8I_HDMI_PHY_PLL_CFG2_VCO_S(4) |
165 SUN8I_HDMI_PHY_PLL_CFG2_S(6);
166 ana_cfg2_init |= SUN8I_HDMI_PHY_ANA_CFG2_REG_BIGSWCK |
167 SUN8I_HDMI_PHY_ANA_CFG2_REG_BIGSW |
168 SUN8I_HDMI_PHY_ANA_CFG2_REG_SLV(2);
169 ana_cfg3_init |= SUN8I_HDMI_PHY_ANA_CFG3_REG_AMPCK(7) |
170 SUN8I_HDMI_PHY_ANA_CFG3_REG_AMP(9);
171 } else {
172 b_offset = 2;
173 pll_cfg1_init |= SUN8I_HDMI_PHY_PLL_CFG1_CNT_INT(63);
174 pll_cfg2_init |= SUN8I_HDMI_PHY_PLL_CFG2_VCO_S(6) |
175 SUN8I_HDMI_PHY_PLL_CFG2_S(7);
176 ana_cfg2_init |= SUN8I_HDMI_PHY_ANA_CFG2_REG_BIGSWCK |
177 SUN8I_HDMI_PHY_ANA_CFG2_REG_BIGSW |
178 SUN8I_HDMI_PHY_ANA_CFG2_REG_SLV(4);
179 ana_cfg3_init |= SUN8I_HDMI_PHY_ANA_CFG3_REG_AMPCK(9) |
180 SUN8I_HDMI_PHY_ANA_CFG3_REG_AMP(13);
181 }
182
183 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG,
184 SUN8I_HDMI_PHY_ANA_CFG1_TXEN_MASK, 0);
185
186 /*
187 * NOTE: We have to be careful not to overwrite PHY parent
188 * clock selection bit and clock divider.
189 */
190 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG,
> 191 ~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK,
192 pll_cfg1_init);
193 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG,
194 (u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK,
195 pll_cfg2_init);
196 usleep_range(10000, 15000);
197 regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG3_REG,
198 SUN8I_HDMI_PHY_PLL_CFG3_SOUT_DIV2);
199 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG,
200 SUN8I_HDMI_PHY_PLL_CFG1_PLLEN,
201 SUN8I_HDMI_PHY_PLL_CFG1_PLLEN);
202 msleep(100);
203
204 /* get B value */
205 regmap_read(phy->regs, SUN8I_HDMI_PHY_ANA_STS_REG, &val);
206 val = (val & SUN8I_HDMI_PHY_ANA_STS_B_OUT_MSK) >>
207 SUN8I_HDMI_PHY_ANA_STS_B_OUT_SHIFT;
208 val = min(val + b_offset, (u32)0x3f);
209
210 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG,
211 SUN8I_HDMI_PHY_PLL_CFG1_REG_OD1 |
212 SUN8I_HDMI_PHY_PLL_CFG1_REG_OD,
213 SUN8I_HDMI_PHY_PLL_CFG1_REG_OD1 |
214 SUN8I_HDMI_PHY_PLL_CFG1_REG_OD);
215 regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG,
216 SUN8I_HDMI_PHY_PLL_CFG1_B_IN_MSK,
217 val << SUN8I_HDMI_PHY_PLL_CFG1_B_IN_SHIFT);
218 msleep(100);
219 regmap_write(phy->regs, SUN8I_HDMI_PHY_ANA_CFG1_REG, ana_cfg1_end);
220 regmap_write(phy->regs, SUN8I_HDMI_PHY_ANA_CFG2_REG, ana_cfg2_init);
221 regmap_write(phy->regs, SUN8I_HDMI_PHY_ANA_CFG3_REG, ana_cfg3_init);
222
223 return 0;
224 }
225
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Sat, May 19, 2018 at 08:31:24PM +0200, Jernej Skrabec wrote: > Expand HDMI PHY clock driver to support second clock parent. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 6 +- > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 29 ++++++- > drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++++++++++++++------ > 3 files changed, 98 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > index 801a17222762..aadbe0a10b0c 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) > @@ -190,6 +191,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, > + bool second_parent); > > #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 deba47ed69d8..7a911f0a3ae3 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, > + ~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, > + pll_cfg1_init); It feels like it belongs in a separate patch. > regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, > (u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK, > pll_cfg2_init); > @@ -352,6 +358,10 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy) > SUN8I_HDMI_PHY_ANA_CFG3_SCLEN | > SUN8I_HDMI_PHY_ANA_CFG3_SDAEN); > > + /* reset PLL clock configuration */ > + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0); > + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 0); > + Ditto, > + > + /* > + * Even though HDMI PHY clock doesn't have enable/disable > + * handlers, we have to enable it. Otherwise it could happen > + * that parent PLL is not enabled by clock framework in a > + * highly unlikely event when parent PLL is used solely for > + * HDMI PHY clock. > + */ > + clk_prepare_enable(phy->clk_phy); The implementation of the clock doesn't really matter in our API usage. If we're using a clock, we have to call clk_prepare_enable. That's documented everywhere, and mentionning how the clock is implemented is an abstraction leakage and it's irrelevant. I'd simply remove the comment here. And it should be in a separate patch as well. Maxime
Hi, Dne ponedeljek, 21. maj 2018 ob 10:12:53 CEST je Maxime Ripard napisal(a): > On Sat, May 19, 2018 at 08:31:24PM +0200, Jernej Skrabec wrote: > > Expand HDMI PHY clock driver to support second clock parent. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 6 +- > > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 29 ++++++- > > drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++++++++++++++------ > > 3 files changed, 98 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 801a17222762..aadbe0a10b0c > > 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) > > > > @@ -190,6 +191,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, > > + bool second_parent); > > > > #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 deba47ed69d8..7a911f0a3ae3 > > 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, > > + ~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, > > + pll_cfg1_init); > > It feels like it belongs in a separate patch. Why? clk_set_rate() on HDMI PHY clock is called before this function, so SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL bit is already set to correct value. Original code doesn't take this into account and sets it to 0 here in all cases, which obviously is not right. If you insist on splitting this in new patch, it has to be applied before clock changes. It would also need to include "reset PLL clock configuration" chunk (next change in this patch), otherwise other SoCs with only one parent could get 1 there, which is obviously wrong. Note that I didn't really tested if default value of this bit is 0 or 1, but I wouldn't really like to depend on default values. > > > regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, > > > > (u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK, > > pll_cfg2_init); > > > > @@ -352,6 +358,10 @@ static void sun8i_hdmi_phy_init_h3(struct > > sun8i_hdmi_phy *phy)> > > SUN8I_HDMI_PHY_ANA_CFG3_SCLEN | > > SUN8I_HDMI_PHY_ANA_CFG3_SDAEN); > > > > + /* reset PLL clock configuration */ > > + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0); > > + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 0); > > + > > Ditto, > > > + > > + /* > > + * Even though HDMI PHY clock doesn't have enable/disable > > + * handlers, we have to enable it. Otherwise it could happen > > + * that parent PLL is not enabled by clock framework in a > > + * highly unlikely event when parent PLL is used solely for > > + * HDMI PHY clock. > > + */ > > + clk_prepare_enable(phy->clk_phy); > > The implementation of the clock doesn't really matter in our API > usage. If we're using a clock, we have to call > clk_prepare_enable. That's documented everywhere, and mentionning how > the clock is implemented is an abstraction leakage and it's > irrelevant. I'd simply remove the comment here. > > And it should be in a separate patch as well. OK. Should I add Fixes tag, since current code obviously is not by the specs? It could still get in 4.17... Best regards, Jernej
On Mon, May 21, 2018 at 05:02:23PM +0200, Jernej Škrabec wrote: > Hi, > > Dne ponedeljek, 21. maj 2018 ob 10:12:53 CEST je Maxime Ripard napisal(a): > > On Sat, May 19, 2018 at 08:31:24PM +0200, Jernej Skrabec wrote: > > > Expand HDMI PHY clock driver to support second clock parent. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > --- > > > > > > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 6 +- > > > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 29 ++++++- > > > drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++++++++++++++------ > > > 3 files changed, 98 insertions(+), 27 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > > > b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 801a17222762..aadbe0a10b0c > > > 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) > > > > > > @@ -190,6 +191,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, > > > + bool second_parent); > > > > > > #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 deba47ed69d8..7a911f0a3ae3 > > > 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, > > > + ~SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, > > > + pll_cfg1_init); > > > > It feels like it belongs in a separate patch. > > Why? clk_set_rate() on HDMI PHY clock is called before this function, so > SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL bit is already set to correct value. Original > code doesn't take this into account and sets it to 0 here in all cases, which > obviously is not right. > > If you insist on splitting this in new patch, it has to be applied before > clock changes. It would also need to include "reset PLL clock configuration" > chunk (next change in this patch), otherwise other SoCs with only one parent > could get 1 there, which is obviously wrong. Note that I didn't really tested > if default value of this bit is 0 or 1, but I wouldn't really like to depend > on default values. I don't have a strong feeling about this, but to me, the fact that you don't want to overwrite the muxing bit because the clock might use it is sensible in itself, disregarding the fact that the clock *will* use it. > > > > > > regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, > > > > > > (u32)~SUN8I_HDMI_PHY_PLL_CFG2_PREDIV_MSK, > > > pll_cfg2_init); > > > > > > @@ -352,6 +358,10 @@ static void sun8i_hdmi_phy_init_h3(struct > > > sun8i_hdmi_phy *phy)> > > > SUN8I_HDMI_PHY_ANA_CFG3_SCLEN | > > > SUN8I_HDMI_PHY_ANA_CFG3_SDAEN); > > > > > > + /* reset PLL clock configuration */ > > > + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0); > > > + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 0); > > > + > > > > Ditto, > > > > > + > > > + /* > > > + * Even though HDMI PHY clock doesn't have enable/disable > > > + * handlers, we have to enable it. Otherwise it could happen > > > + * that parent PLL is not enabled by clock framework in a > > > + * highly unlikely event when parent PLL is used solely for > > > + * HDMI PHY clock. > > > + */ > > > + clk_prepare_enable(phy->clk_phy); > > > > The implementation of the clock doesn't really matter in our API > > usage. If we're using a clock, we have to call > > clk_prepare_enable. That's documented everywhere, and mentionning how > > the clock is implemented is an abstraction leakage and it's > > irrelevant. I'd simply remove the comment here. > > > > And it should be in a separate patch as well. > > OK. Should I add Fixes tag, since current code obviously is not by the specs? > It could still get in 4.17... Yep! Maxime
diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 801a17222762..aadbe0a10b0c 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) @@ -190,6 +191,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, + bool second_parent); #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 deba47ed69d8..7a911f0a3ae3 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, + ~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); @@ -352,6 +358,10 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy) SUN8I_HDMI_PHY_ANA_CFG3_SCLEN | SUN8I_HDMI_PHY_ANA_CFG3_SDAEN); + /* reset PLL clock configuration */ + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, 0); + regmap_write(phy->regs, SUN8I_HDMI_PHY_PLL_CFG2_REG, 0); + /* set HW control of CEC pins */ regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, 0); @@ -481,18 +491,28 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node) } } - ret = sun8i_phy_clk_create(phy, dev); + ret = sun8i_phy_clk_create(phy, dev, + phy->variant->has_second_pll); if (ret) { dev_err(dev, "Couldn't create the PHY clock\n"); goto err_put_clk_pll1; } + + /* + * Even though HDMI PHY clock doesn't have enable/disable + * handlers, we have to enable it. Otherwise it could happen + * that parent PLL is not enabled by clock framework in a + * highly unlikely event when parent PLL is used solely for + * HDMI PHY clock. + */ + clk_prepare_enable(phy->clk_phy); } phy->rst_phy = of_reset_control_get_shared(node, "phy"); if (IS_ERR(phy->rst_phy)) { dev_err(dev, "Could not get phy reset control\n"); ret = PTR_ERR(phy->rst_phy); - goto err_put_clk_pll1; + goto err_disable_clk_phy; } ret = reset_control_deassert(phy->rst_phy); @@ -523,6 +543,8 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node) reset_control_assert(phy->rst_phy); err_put_rst_phy: reset_control_put(phy->rst_phy); +err_disable_clk_phy: + clk_disable_unprepare(phy->clk_phy); err_put_clk_pll1: clk_put(phy->clk_pll1); err_put_clk_pll0: @@ -541,6 +563,7 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi) clk_disable_unprepare(phy->clk_mod); clk_disable_unprepare(phy->clk_bus); + clk_disable_unprepare(phy->clk_phy); reset_control_assert(phy->rst_phy); diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c index faea449812f8..a4d31fe3abff 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c @@ -22,35 +22,45 @@ static int sun8i_phy_clk_determine_rate(struct clk_hw *hw, { unsigned long rate = req->rate; unsigned long best_rate = 0; + struct clk_hw *best_parent = NULL; struct clk_hw *parent; 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); + for (p = 0; p < clk_hw_get_num_parents(hw); p++) { + parent = clk_hw_get_parent_by_index(hw, p); + if (!parent) + continue; - if (rounded == ideal) { - best_rate = rounded; - best_div = i; - break; + 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; + } } - if (!best_rate || - abs(rate - rounded / i) < - abs(rate - best_rate / best_div)) { - best_rate = rounded; - best_div = i; - } + if (best_rate / best_div == rate) + break; } req->rate = best_rate / best_div; req->best_parent_rate = best_rate; - req->best_parent_hw = parent; + req->best_parent_hw = best_parent; return 0; } @@ -95,22 +105,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; +} + static const struct clk_ops sun8i_phy_clk_ops = { .determine_rate = sun8i_phy_clk_determine_rate, .recalc_rate = sun8i_phy_clk_recalc_rate, .set_rate = sun8i_phy_clk_set_rate, + + .get_parent = sun8i_phy_clk_get_parent, + .set_parent = sun8i_phy_clk_set_parent, }; -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, + bool second_parent) { struct clk_init_data init; struct sun8i_phy_clk *priv; - const char *parents[1]; + const char *parents[2]; parents[0] = __clk_get_name(phy->clk_pll0); if (!parents[0]) return -ENODEV; + if (second_parent) { + parents[1] = __clk_get_name(phy->clk_pll1); + if (!parents[1]) + return -ENODEV; + } + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); if (!priv) return -ENOMEM; @@ -118,7 +164,7 @@ int sun8i_phy_clk_create(struct sun8i_hdmi_phy *phy, struct device *dev) init.name = "hdmi-phy-clk"; init.ops = &sun8i_phy_clk_ops; init.parent_names = parents; - init.num_parents = 1; + init.num_parents = second_parent ? 2 : 1; init.flags = CLK_SET_RATE_PARENT; priv->phy = phy;
Expand HDMI PHY clock driver to support second clock parent. Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 6 +- drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 29 ++++++- drivers/gpu/drm/sun4i/sun8i_hdmi_phy_clk.c | 90 ++++++++++++++++------ 3 files changed, 98 insertions(+), 27 deletions(-)