| Message ID | 20171230210203.24115-1-jernej.skrabec@siol.net |
|---|---|
| Headers | show |
| Series | drm/sun4i: Add A83T HDMI support | expand |
On Sun, Dec 31, 2017 at 5:01 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote: > TCON1 also has M divider, contrary to TCON0. > > Fixes: 05359be1176b ("clk: sunxi-ng: Add driver for A83T CCU") > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> Added "And the mux is only 2 bits wide, instead of 3." to the commit message and applied. ChenYu -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Sat, Dec 30, 2017 at 10:01:53PM +0100, Jernej Skrabec wrote: > For example, A83T have nmp plls which are modelled as nkmp plls. Since k > is not specified, it has offset 0, shift 0 and lowest value 1. This > means that LSB bit is always set to 1, which may change clock rate. > > Fix that by applying k factor only if k width is greater than 0. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> This looks fine... > --- > drivers/clk/sunxi-ng/ccu_nkmp.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/clk/sunxi-ng/ccu_nkmp.c b/drivers/clk/sunxi-ng/ccu_nkmp.c > index e58c95787f94..709f528af2b3 100644 > --- a/drivers/clk/sunxi-ng/ccu_nkmp.c > +++ b/drivers/clk/sunxi-ng/ccu_nkmp.c > @@ -81,7 +81,7 @@ static unsigned long ccu_nkmp_recalc_rate(struct clk_hw *hw, > unsigned long parent_rate) > { > struct ccu_nkmp *nkmp = hw_to_ccu_nkmp(hw); > - unsigned long n, m, k, p; > + unsigned long n, m, k = 1, p; > u32 reg; > > reg = readl(nkmp->common.base + nkmp->common.reg); > @@ -92,11 +92,13 @@ static unsigned long ccu_nkmp_recalc_rate(struct clk_hw *hw, > if (!n) > n++; > > - k = reg >> nkmp->k.shift; > - k &= (1 << nkmp->k.width) - 1; > - k += nkmp->k.offset; > - if (!k) > - k++; > + if (nkmp->k.width) { > + k = reg >> nkmp->k.shift; > + k &= (1 << nkmp->k.width) - 1; > + k += nkmp->k.offset; > + if (!k) > + k++; > + } ... but could you add a comment there to explain why you're using a different construct than the one used for the other factors? Thanks! Maxime
On Sun, Dec 31, 2017 at 5:01 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote: > For example, A83T have nmp plls which are modelled as nkmp plls. Since k > is not specified, it has offset 0, shift 0 and lowest value 1. This > means that LSB bit is always set to 1, which may change clock rate. > > Fix that by applying k factor only if k width is greater than 0. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/clk/sunxi-ng/ccu_nkmp.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/clk/sunxi-ng/ccu_nkmp.c b/drivers/clk/sunxi-ng/ccu_nkmp.c > index e58c95787f94..709f528af2b3 100644 > --- a/drivers/clk/sunxi-ng/ccu_nkmp.c > +++ b/drivers/clk/sunxi-ng/ccu_nkmp.c > @@ -81,7 +81,7 @@ static unsigned long ccu_nkmp_recalc_rate(struct clk_hw *hw, > unsigned long parent_rate) > { > struct ccu_nkmp *nkmp = hw_to_ccu_nkmp(hw); > - unsigned long n, m, k, p; > + unsigned long n, m, k = 1, p; > u32 reg; > > reg = readl(nkmp->common.base + nkmp->common.reg); > @@ -92,11 +92,13 @@ static unsigned long ccu_nkmp_recalc_rate(struct clk_hw *hw, > if (!n) > n++; > > - k = reg >> nkmp->k.shift; > - k &= (1 << nkmp->k.width) - 1; > - k += nkmp->k.offset; > - if (!k) > - k++; > + if (nkmp->k.width) { > + k = reg >> nkmp->k.shift; > + k &= (1 << nkmp->k.width) - 1; > + k += nkmp->k.offset; > + if (!k) > + k++; > + } The conditional shouldn't be necessary. With nkmp->k.width = 0, you'd simply get k & 0, which is 0, which then gets bumped up to 1, unless k.offset > 1, which would be a bug. > > m = reg >> nkmp->m.shift; > m &= (1 << nkmp->m.width) - 1; > @@ -153,12 +155,15 @@ static int ccu_nkmp_set_rate(struct clk_hw *hw, unsigned long rate, > > reg = readl(nkmp->common.base + nkmp->common.reg); > reg &= ~GENMASK(nkmp->n.width + nkmp->n.shift - 1, nkmp->n.shift); > - reg &= ~GENMASK(nkmp->k.width + nkmp->k.shift - 1, nkmp->k.shift); > + if (nkmp->k.width) > + reg &= ~GENMASK(nkmp->k.width + nkmp->k.shift - 1, > + nkmp->k.shift); > reg &= ~GENMASK(nkmp->m.width + nkmp->m.shift - 1, nkmp->m.shift); > reg &= ~GENMASK(nkmp->p.width + nkmp->p.shift - 1, nkmp->p.shift); > > reg |= (_nkmp.n - nkmp->n.offset) << nkmp->n.shift; > - reg |= (_nkmp.k - nkmp->k.offset) << nkmp->k.shift; > + if (nkmp->k.width) > + reg |= (_nkmp.k - nkmp->k.offset) << nkmp->k.shift; I think a better way would be reg |= ((_nkmp.k - nkmp->k.offset) << nkmp->k.shift) & GENMASK(nkmp->k.width + nkmp->k.shift - 1, nkmp->k.shift); And do this for all the factors, not just k. This pattern is what regmap_update_bits does, which seems much safer. I wonder what GENMASK() with a negative value would do though... ChenYu > reg |= (_nkmp.m - nkmp->m.offset) << nkmp->m.shift; > reg |= ilog2(_nkmp.p) << nkmp->p.shift; > > -- > 2.15.1 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Sat, Dec 30, 2017 at 10:01:59PM +0100, Jernej Skrabec wrote: > This TCON doesn't have channel 0, so quirk has_channel_0 is added in the > process. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> Ideally, this should be split in two patches, one to add the new flag, and one to add the support for the A83t TCON-TV. Maxime
On Sat, Dec 30, 2017 at 10:02:00PM +0100, Jernej Skrabec wrote: > It supports 1 VI and 1 UI plane and HW scaling on both planes. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> Acked-by: Maxime Ripard <maxime.ripard@free-electrons.com> Maxime
Hi, Dne četrtek, 04. januar 2018 ob 15:45:18 CET je Chen-Yu Tsai napisal(a): > On Sun, Dec 31, 2017 at 5:01 AM, Jernej Skrabec <jernej.skrabec@siol.net> wrote: > > For example, A83T have nmp plls which are modelled as nkmp plls. Since k > > is not specified, it has offset 0, shift 0 and lowest value 1. This > > means that LSB bit is always set to 1, which may change clock rate. > > > > Fix that by applying k factor only if k width is greater than 0. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/clk/sunxi-ng/ccu_nkmp.c | 21 +++++++++++++-------- > > 1 file changed, 13 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/clk/sunxi-ng/ccu_nkmp.c > > b/drivers/clk/sunxi-ng/ccu_nkmp.c index e58c95787f94..709f528af2b3 100644 > > --- a/drivers/clk/sunxi-ng/ccu_nkmp.c > > +++ b/drivers/clk/sunxi-ng/ccu_nkmp.c > > @@ -81,7 +81,7 @@ static unsigned long ccu_nkmp_recalc_rate(struct clk_hw > > *hw,> > > unsigned long parent_rate) > > > > { > > > > struct ccu_nkmp *nkmp = hw_to_ccu_nkmp(hw); > > > > - unsigned long n, m, k, p; > > + unsigned long n, m, k = 1, p; > > > > u32 reg; > > > > reg = readl(nkmp->common.base + nkmp->common.reg); > > > > @@ -92,11 +92,13 @@ static unsigned long ccu_nkmp_recalc_rate(struct > > clk_hw *hw,> > > if (!n) > > > > n++; > > > > - k = reg >> nkmp->k.shift; > > - k &= (1 << nkmp->k.width) - 1; > > - k += nkmp->k.offset; > > - if (!k) > > - k++; > > + if (nkmp->k.width) { > > + k = reg >> nkmp->k.shift; > > + k &= (1 << nkmp->k.width) - 1; > > + k += nkmp->k.offset; > > + if (!k) > > + k++; > > + } > > The conditional shouldn't be necessary. With nkmp->k.width = 0, > you'd simply get k & 0, which is 0, which then gets bumped up to 1, > unless k.offset > 1, which would be a bug. > > > m = reg >> nkmp->m.shift; > > m &= (1 << nkmp->m.width) - 1; > > > > @@ -153,12 +155,15 @@ static int ccu_nkmp_set_rate(struct clk_hw *hw, > > unsigned long rate,> > > reg = readl(nkmp->common.base + nkmp->common.reg); > > reg &= ~GENMASK(nkmp->n.width + nkmp->n.shift - 1, nkmp->n.shift); > > > > - reg &= ~GENMASK(nkmp->k.width + nkmp->k.shift - 1, nkmp->k.shift); > > + if (nkmp->k.width) > > + reg &= ~GENMASK(nkmp->k.width + nkmp->k.shift - 1, > > + nkmp->k.shift); > > > > reg &= ~GENMASK(nkmp->m.width + nkmp->m.shift - 1, nkmp->m.shift); > > reg &= ~GENMASK(nkmp->p.width + nkmp->p.shift - 1, nkmp->p.shift); > > > > reg |= (_nkmp.n - nkmp->n.offset) << nkmp->n.shift; > > > > - reg |= (_nkmp.k - nkmp->k.offset) << nkmp->k.shift; > > + if (nkmp->k.width) > > + reg |= (_nkmp.k - nkmp->k.offset) << nkmp->k.shift; > > I think a better way would be > > reg |= ((_nkmp.k - nkmp->k.offset) << nkmp->k.shift) & > GENMASK(nkmp->k.width + nkmp->k.shift - 1, nkmp->k.shift); > > And do this for all the factors, not just k. This pattern is what > regmap_update_bits does, which seems much safer. I wonder what > GENMASK() with a negative value would do though... You're right, GENMASK(-1, 0) equals 0 (calculated by hand, not tested). This seems much more elegant solution. Semi-related question: All nmp PLLs have much wider N range than real nkmp PLLs. This causes integer overflow when using nkmp formula from datasheet. Usually, N is 1-256 for nmp PLLs, which means that for very high N factors, it overflows. This also causes issue that M factor is never higher than 1. I was wondering, if patch would be acceptable which would change this formula: RATE = (24MHz * N * K) / (M * P) to this: RATE ((24MHz / M) * N * K) / P I checked all M factors and are all in 1-4 or 1-2 range, which means it wouldn't have any impact for real nkmp PLLs when parent is 24 MHz clock which is probably always. What do you think? I discovered that when I tried to set A83T PLL_VIDEO to 346.5 MHz which is possible only when above formula is changed. Best regards, Jernej > > ChenYu > > > reg |= (_nkmp.m - nkmp->m.offset) << nkmp->m.shift; > > reg |= ilog2(_nkmp.p) << nkmp->p.shift; > > > > -- > > 2.15.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 5, 2018 at 3:28 AM, Jernej Škrabec <jernej.skrabec@siol.net> wrote: > Hi, > > Dne četrtek, 04. januar 2018 ob 15:45:18 CET je Chen-Yu Tsai napisal(a): >> On Sun, Dec 31, 2017 at 5:01 AM, Jernej Skrabec <jernej.skrabec@siol.net> > wrote: >> > For example, A83T have nmp plls which are modelled as nkmp plls. Since k >> > is not specified, it has offset 0, shift 0 and lowest value 1. This >> > means that LSB bit is always set to 1, which may change clock rate. >> > >> > Fix that by applying k factor only if k width is greater than 0. >> > >> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> >> > --- >> > >> > drivers/clk/sunxi-ng/ccu_nkmp.c | 21 +++++++++++++-------- >> > 1 file changed, 13 insertions(+), 8 deletions(-) >> > >> > diff --git a/drivers/clk/sunxi-ng/ccu_nkmp.c >> > b/drivers/clk/sunxi-ng/ccu_nkmp.c index e58c95787f94..709f528af2b3 100644 >> > --- a/drivers/clk/sunxi-ng/ccu_nkmp.c >> > +++ b/drivers/clk/sunxi-ng/ccu_nkmp.c >> > @@ -81,7 +81,7 @@ static unsigned long ccu_nkmp_recalc_rate(struct clk_hw >> > *hw,> >> > unsigned long parent_rate) >> > >> > { >> > >> > struct ccu_nkmp *nkmp = hw_to_ccu_nkmp(hw); >> > >> > - unsigned long n, m, k, p; >> > + unsigned long n, m, k = 1, p; >> > >> > u32 reg; >> > >> > reg = readl(nkmp->common.base + nkmp->common.reg); >> > >> > @@ -92,11 +92,13 @@ static unsigned long ccu_nkmp_recalc_rate(struct >> > clk_hw *hw,> >> > if (!n) >> > >> > n++; >> > >> > - k = reg >> nkmp->k.shift; >> > - k &= (1 << nkmp->k.width) - 1; >> > - k += nkmp->k.offset; >> > - if (!k) >> > - k++; >> > + if (nkmp->k.width) { >> > + k = reg >> nkmp->k.shift; >> > + k &= (1 << nkmp->k.width) - 1; >> > + k += nkmp->k.offset; >> > + if (!k) >> > + k++; >> > + } >> >> The conditional shouldn't be necessary. With nkmp->k.width = 0, >> you'd simply get k & 0, which is 0, which then gets bumped up to 1, >> unless k.offset > 1, which would be a bug. >> >> > m = reg >> nkmp->m.shift; >> > m &= (1 << nkmp->m.width) - 1; >> > >> > @@ -153,12 +155,15 @@ static int ccu_nkmp_set_rate(struct clk_hw *hw, >> > unsigned long rate,> >> > reg = readl(nkmp->common.base + nkmp->common.reg); >> > reg &= ~GENMASK(nkmp->n.width + nkmp->n.shift - 1, nkmp->n.shift); >> > >> > - reg &= ~GENMASK(nkmp->k.width + nkmp->k.shift - 1, nkmp->k.shift); >> > + if (nkmp->k.width) >> > + reg &= ~GENMASK(nkmp->k.width + nkmp->k.shift - 1, >> > + nkmp->k.shift); >> > >> > reg &= ~GENMASK(nkmp->m.width + nkmp->m.shift - 1, nkmp->m.shift); >> > reg &= ~GENMASK(nkmp->p.width + nkmp->p.shift - 1, nkmp->p.shift); >> > >> > reg |= (_nkmp.n - nkmp->n.offset) << nkmp->n.shift; >> > >> > - reg |= (_nkmp.k - nkmp->k.offset) << nkmp->k.shift; >> > + if (nkmp->k.width) >> > + reg |= (_nkmp.k - nkmp->k.offset) << nkmp->k.shift; >> >> I think a better way would be >> >> reg |= ((_nkmp.k - nkmp->k.offset) << nkmp->k.shift) & >> GENMASK(nkmp->k.width + nkmp->k.shift - 1, nkmp->k.shift); >> >> And do this for all the factors, not just k. This pattern is what >> regmap_update_bits does, which seems much safer. I wonder what >> GENMASK() with a negative value would do though... > > You're right, GENMASK(-1, 0) equals 0 (calculated by hand, not tested). This > seems much more elegant solution. > > Semi-related question: All nmp PLLs have much wider N range than real nkmp > PLLs. This causes integer overflow when using nkmp formula from datasheet. > Usually, N is 1-256 for nmp PLLs, which means that for very high N factors, it > overflows. This also causes issue that M factor is never higher than 1. Sounds like we can't use u8 for storing the factors. At least the intermediate values we use to calculate the rates. > > I was wondering, if patch would be acceptable which would change this formula: > > RATE = (24MHz * N * K) / (M * P) > > to this: > > RATE ((24MHz / M) * N * K) / P > > I checked all M factors and are all in 1-4 or 1-2 range, which means it > wouldn't have any impact for real nkmp PLLs when parent is 24 MHz clock which > is probably always. > > What do you think? I think this is acceptable. M is normally the pre-divider, so this actually fits how the hardware works, including possible rounding errors. ChenYu > I discovered that when I tried to set A83T PLL_VIDEO to 346.5 MHz which is > possible only when above formula is changed. > > Best regards, > Jernej > >> >> ChenYu >> >> > reg |= (_nkmp.m - nkmp->m.offset) << nkmp->m.shift; >> > reg |= ilog2(_nkmp.p) << nkmp->p.shift; >> > >> > -- >> > 2.15.1 > > > > > -- > You received this message because you are subscribed to the Google Groups "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/31/2017 02:31 AM, Jernej Skrabec wrote: > Parts of PHY code could be useful also for custom PHYs. For example, > Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY > with few additional memory mapped registers, so most of the Synopsys PHY > related code could be reused. > > It turns out that even completely custom HDMI PHYs, such as the one > found in Allwinner H3, can reuse some of those functions. This would > suggest that (some?) functions exported in this commit are actually part > of generic PHY interface and not really specific to Synopsys PHYs. > > Export useful PHY functions. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 45 ++++++++++++++++++++++--------- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 2 ++ > include/drm/bridge/dw_hdmi.h | 10 +++++++ > 3 files changed, 44 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 7ca14d7325b5..67467d0b683a 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -1037,19 +1037,21 @@ static void dw_hdmi_phy_enable_svsret(struct dw_hdmi *hdmi, u8 enable) > HDMI_PHY_CONF0_SVSRET_MASK); > } > > -static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable) > +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable) > { > hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0, > HDMI_PHY_CONF0_GEN2_PDDQ_OFFSET, > HDMI_PHY_CONF0_GEN2_PDDQ_MASK); > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_pddq); > > -static void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable) > +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable) > { > hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0, > HDMI_PHY_CONF0_GEN2_TXPWRON_OFFSET, > HDMI_PHY_CONF0_GEN2_TXPWRON_MASK); > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_txpwron); > > static void dw_hdmi_phy_sel_data_en_pol(struct dw_hdmi *hdmi, u8 enable) > { > @@ -1065,6 +1067,23 @@ static void dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable) > HDMI_PHY_CONF0_SELDIPIF_MASK); > } > > +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable) > +{ > + hdmi_mask_writeb(hdmi, enable, HDMI_MC_PHYRSTZ, > + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET, > + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK); > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_reset); > + > +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi) > +{ > + hdmi_phy_test_clear(hdmi, 1); > + hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > + HDMI_PHY_I2CM_SLAVE_ADDR); > + hdmi_phy_test_clear(hdmi, 0); > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_set_slave_addr); Should this be called dw_hdmi_phy_gen2_set_slave_addr? Looks good otherwise. Same for patches 3 and 4 in this series. Thanks, Archit > + > static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > { > const struct dw_hdmi_phy_data *phy = hdmi->phy.data; > @@ -1204,15 +1223,12 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi) > dw_hdmi_phy_enable_svsret(hdmi, 1); > > /* PHY reset. The reset signal is active high on Gen2 PHYs. */ > - hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_PHYRSTZ, HDMI_MC_PHYRSTZ); > - hdmi_writeb(hdmi, 0, HDMI_MC_PHYRSTZ); > + dw_hdmi_phy_gen2_reset(hdmi, 1); > + dw_hdmi_phy_gen2_reset(hdmi, 0); > > hdmi_writeb(hdmi, HDMI_MC_HEACPHY_RST_ASSERT, HDMI_MC_HEACPHY_RST); > > - hdmi_phy_test_clear(hdmi, 1); > - hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > - HDMI_PHY_I2CM_SLAVE_ADDR); > - hdmi_phy_test_clear(hdmi, 0); > + dw_hdmi_phy_set_slave_addr(hdmi); > > /* Write to the PHY as configured by the platform */ > if (pdata->configure_phy) > @@ -1251,15 +1267,16 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi, void *data) > dw_hdmi_phy_power_off(hdmi); > } > > -static enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > - void *data) > +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > + void *data) > { > return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ? > connector_status_connected : connector_status_disconnected; > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_read_hpd); > > -static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > - bool force, bool disabled, bool rxsense) > +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > + bool force, bool disabled, bool rxsense) > { > u8 old_mask = hdmi->phy_mask; > > @@ -1271,8 +1288,9 @@ static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > if (old_mask != hdmi->phy_mask) > hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_update_hpd); > > -static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > { > /* > * Configure the PHY RX SENSE and HPD interrupts polarities and clear > @@ -1291,6 +1309,7 @@ static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | HDMI_IH_PHY_STAT0_RX_SENSE), > HDMI_IH_MUTE_PHY_STAT0); > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_setup_hpd); > > static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = { > .init = dw_hdmi_phy_init, > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > index 9d90eb9c46e5..fd150430d0b3 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > @@ -950,6 +950,8 @@ enum { > > /* MC_PHYRSTZ field values */ > HDMI_MC_PHYRSTZ_PHYRSTZ = 0x01, > + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET = 0x00, > + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK = 0x01, > > /* MC_HEACPHY_RST field values */ > HDMI_MC_HEACPHY_RST_ASSERT = 0x1, > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index 182f83283e24..f5cca4362154 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -159,5 +159,15 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi); > /* PHY configuration */ > void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data, > unsigned char addr); > +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > + void *data); > +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > + bool force, bool disabled, bool rxsense); > +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); > + > +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable); > +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable); > +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable); > +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi); > > #endif /* __IMX_HDMI_H__ */ >
Hi Jernej, Thank you for the patch. On Saturday, 30 December 2017 23:01:55 EET Jernej Skrabec wrote: > Allwinner SoCs have dw hdmi controller v1.32a which exhibits same > magenta line issue as i.MX6Q and i.MX6DL. Enable workaround for it. > > Tests show that one iteration is enough. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> This does not break R-Car DU, so Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > a38db40ce990..7ca14d7325b5 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -1634,9 +1634,10 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi > *hdmi) * then write one of the FC registers several times. > * > * The number of iterations matters and depends on the HDMI TX revision > - * (and possibly on the platform). So far only i.MX6Q (v1.30a) and > - * i.MX6DL (v1.31a) have been identified as needing the workaround, with > - * 4 and 1 iterations respectively. > + * (and possibly on the platform). So far i.MX6Q (v1.30a), i.MX6DL > + * (v1.31a) and multiple Allwinner SoCs (v1.32a) have been identified > + * as needing the workaround, with 4 iterations for v1.30a and 1 > + * iteration for others. > */ > > switch (hdmi->version) { > @@ -1644,6 +1645,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi > *hdmi) count = 4; > break; > case 0x131a: > + case 0x132a: > count = 1; > break; > default:
Hi Jernej, Thank you for the patch. On Saturday, 30 December 2017 23:01:56 EET Jernej Skrabec wrote: > Parts of PHY code could be useful also for custom PHYs. For example, > Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY > with few additional memory mapped registers, so most of the Synopsys PHY > related code could be reused. > > It turns out that even completely custom HDMI PHYs, such as the one > found in Allwinner H3, can reuse some of those functions. This would > suggest that (some?) functions exported in this commit are actually part > of generic PHY interface and not really specific to Synopsys PHYs. That's correct, those functions control the interface between the HDMI controller and the PHY. They're not specific to Synopsys PHYs, but they're specific to the PHY interface as designed by Synopsys. > Export useful PHY functions. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 45 ++++++++++++++++++++------- > drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 2 ++ > include/drm/bridge/dw_hdmi.h | 10 +++++++ > 3 files changed, 44 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > 7ca14d7325b5..67467d0b683a 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -1037,19 +1037,21 @@ static void dw_hdmi_phy_enable_svsret(struct dw_hdmi > *hdmi, u8 enable) HDMI_PHY_CONF0_SVSRET_MASK); > } > > -static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable) > +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable) > { > hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0, > HDMI_PHY_CONF0_GEN2_PDDQ_OFFSET, > HDMI_PHY_CONF0_GEN2_PDDQ_MASK); > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_pddq); > > -static void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable) > +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable) > { > hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0, > HDMI_PHY_CONF0_GEN2_TXPWRON_OFFSET, > HDMI_PHY_CONF0_GEN2_TXPWRON_MASK); > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_txpwron); > > static void dw_hdmi_phy_sel_data_en_pol(struct dw_hdmi *hdmi, u8 enable) > { > @@ -1065,6 +1067,23 @@ static void dw_hdmi_phy_sel_interface_control(struct > dw_hdmi *hdmi, u8 enable) HDMI_PHY_CONF0_SELDIPIF_MASK); > } > > +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable) > +{ > + hdmi_mask_writeb(hdmi, enable, HDMI_MC_PHYRSTZ, > + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET, > + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK); > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_reset); > + > +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi) > +{ > + hdmi_phy_test_clear(hdmi, 1); > + hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > + HDMI_PHY_I2CM_SLAVE_ADDR); > + hdmi_phy_test_clear(hdmi, 0); > +} > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_set_slave_addr); Should the I2C address be passed as an argument ? > static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > { > const struct dw_hdmi_phy_data *phy = hdmi->phy.data; > @@ -1204,15 +1223,12 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi) > dw_hdmi_phy_enable_svsret(hdmi, 1); > > /* PHY reset. The reset signal is active high on Gen2 PHYs. */ > - hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_PHYRSTZ, HDMI_MC_PHYRSTZ); > - hdmi_writeb(hdmi, 0, HDMI_MC_PHYRSTZ); > + dw_hdmi_phy_gen2_reset(hdmi, 1); > + dw_hdmi_phy_gen2_reset(hdmi, 0); > > hdmi_writeb(hdmi, HDMI_MC_HEACPHY_RST_ASSERT, HDMI_MC_HEACPHY_RST); > > - hdmi_phy_test_clear(hdmi, 1); > - hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > - HDMI_PHY_I2CM_SLAVE_ADDR); > - hdmi_phy_test_clear(hdmi, 0); > + dw_hdmi_phy_set_slave_addr(hdmi); > > /* Write to the PHY as configured by the platform */ > if (pdata->configure_phy) > @@ -1251,15 +1267,16 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > *hdmi, void *data) dw_hdmi_phy_power_off(hdmi); > } > > -static enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > - void *data) > +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > + void *data) > { > return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ? > connector_status_connected : connector_status_disconnected; > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_read_hpd); > > -static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > - bool force, bool disabled, bool rxsense) > +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > + bool force, bool disabled, bool rxsense) > { > u8 old_mask = hdmi->phy_mask; > > @@ -1271,8 +1288,9 @@ static void dw_hdmi_phy_update_hpd(struct dw_hdmi > *hdmi, void *data, if (old_mask != hdmi->phy_mask) > hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_update_hpd); > > -static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > { > /* > * Configure the PHY RX SENSE and HPD interrupts polarities and clear > @@ -1291,6 +1309,7 @@ static void dw_hdmi_phy_setup_hpd(struct dw_hdmi > *hdmi, void *data) hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | > HDMI_IH_PHY_STAT0_RX_SENSE), HDMI_IH_MUTE_PHY_STAT0); > } > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_setup_hpd); > > static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = { > .init = dw_hdmi_phy_init, > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h index > 9d90eb9c46e5..fd150430d0b3 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > @@ -950,6 +950,8 @@ enum { > > /* MC_PHYRSTZ field values */ > HDMI_MC_PHYRSTZ_PHYRSTZ = 0x01, > + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET = 0x00, > + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK = 0x01, > > /* MC_HEACPHY_RST field values */ > HDMI_MC_HEACPHY_RST_ASSERT = 0x1, > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > index 182f83283e24..f5cca4362154 100644 > --- a/include/drm/bridge/dw_hdmi.h > +++ b/include/drm/bridge/dw_hdmi.h > @@ -159,5 +159,15 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi); > /* PHY configuration */ > void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data, > unsigned char addr); > +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > + void *data); > +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > + bool force, bool disabled, bool rxsense); > +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); > + > +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable); > +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable); > +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable); > +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi); > > #endif /* __IMX_HDMI_H__ */
Hi, I think this question is for Jose. On 09/01/2018 13:56, Laurent Pinchart wrote: > Hi Jernej, > > Thank you for the patch. > > On Saturday, 30 December 2017 23:01:55 EET Jernej Skrabec wrote: >> Allwinner SoCs have dw hdmi controller v1.32a which exhibits same >> magenta line issue as i.MX6Q and i.MX6DL. Enable workaround for it. We observe the same issue on Amlogic SoCs with the dw hdmi controller v2.01a. Rockchip seems to also use count=1 for 0x200a, 0x201a and 0x211a in https://github.com/rockchip-linux/kernel/commit/cafa8ebd5f4df41425d6f2f61633d5bc64f20e65 Changelog is : The issue can be worked around by issuing a TMDS software reset and then write one of the FC registers several times. After tested, the number of iterations of RK3399/RK3328(v2.11a), RK3368(v2.01a), RK3288(v2.00a) is one. Can you confirm it is necessary ? Neil >> >> Tests show that one iteration is enough. >> >> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > This does not break R-Car DU, so > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> --- >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index >> a38db40ce990..7ca14d7325b5 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c >> @@ -1634,9 +1634,10 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi >> *hdmi) * then write one of the FC registers several times. >> * >> * The number of iterations matters and depends on the HDMI TX revision >> - * (and possibly on the platform). So far only i.MX6Q (v1.30a) and >> - * i.MX6DL (v1.31a) have been identified as needing the workaround, with >> - * 4 and 1 iterations respectively. >> + * (and possibly on the platform). So far i.MX6Q (v1.30a), i.MX6DL >> + * (v1.31a) and multiple Allwinner SoCs (v1.32a) have been identified >> + * as needing the workaround, with 4 iterations for v1.30a and 1 >> + * iteration for others. >> */ >> >> switch (hdmi->version) { >> @@ -1644,6 +1645,7 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi >> *hdmi) count = 4; >> break; >> case 0x131a: >> + case 0x132a: >> count = 1; >> break; >> default: > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chen-Yu, Dne ponedeljek, 08. januar 2018 ob 10:19:47 CET je Chen-Yu Tsai napisal(a): > On Fri, Jan 5, 2018 at 3:28 AM, Jernej Škrabec <jernej.skrabec@siol.net> wrote: > > Hi, > > > > Dne četrtek, 04. januar 2018 ob 15:45:18 CET je Chen-Yu Tsai napisal(a): > >> On Sun, Dec 31, 2017 at 5:01 AM, Jernej Skrabec <jernej.skrabec@siol.net> > > > > wrote: > >> > For example, A83T have nmp plls which are modelled as nkmp plls. Since > >> > k > >> > is not specified, it has offset 0, shift 0 and lowest value 1. This > >> > means that LSB bit is always set to 1, which may change clock rate. > >> > > >> > Fix that by applying k factor only if k width is greater than 0. > >> > > >> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > >> > --- > >> > > >> > drivers/clk/sunxi-ng/ccu_nkmp.c | 21 +++++++++++++-------- > >> > 1 file changed, 13 insertions(+), 8 deletions(-) > >> > > >> > diff --git a/drivers/clk/sunxi-ng/ccu_nkmp.c > >> > b/drivers/clk/sunxi-ng/ccu_nkmp.c index e58c95787f94..709f528af2b3 > >> > 100644 > >> > --- a/drivers/clk/sunxi-ng/ccu_nkmp.c > >> > +++ b/drivers/clk/sunxi-ng/ccu_nkmp.c > >> > @@ -81,7 +81,7 @@ static unsigned long ccu_nkmp_recalc_rate(struct > >> > clk_hw > >> > *hw,> > >> > > >> > unsigned long parent_rate) > >> > > >> > { > >> > > >> > struct ccu_nkmp *nkmp = hw_to_ccu_nkmp(hw); > >> > > >> > - unsigned long n, m, k, p; > >> > + unsigned long n, m, k = 1, p; > >> > > >> > u32 reg; > >> > > >> > reg = readl(nkmp->common.base + nkmp->common.reg); > >> > > >> > @@ -92,11 +92,13 @@ static unsigned long ccu_nkmp_recalc_rate(struct > >> > clk_hw *hw,> > >> > > >> > if (!n) > >> > > >> > n++; > >> > > >> > - k = reg >> nkmp->k.shift; > >> > - k &= (1 << nkmp->k.width) - 1; > >> > - k += nkmp->k.offset; > >> > - if (!k) > >> > - k++; > >> > + if (nkmp->k.width) { > >> > + k = reg >> nkmp->k.shift; > >> > + k &= (1 << nkmp->k.width) - 1; > >> > + k += nkmp->k.offset; > >> > + if (!k) > >> > + k++; > >> > + } > >> > >> The conditional shouldn't be necessary. With nkmp->k.width = 0, > >> you'd simply get k & 0, which is 0, which then gets bumped up to 1, > >> unless k.offset > 1, which would be a bug. > >> > >> > m = reg >> nkmp->m.shift; > >> > m &= (1 << nkmp->m.width) - 1; > >> > > >> > @@ -153,12 +155,15 @@ static int ccu_nkmp_set_rate(struct clk_hw *hw, > >> > unsigned long rate,> > >> > > >> > reg = readl(nkmp->common.base + nkmp->common.reg); > >> > reg &= ~GENMASK(nkmp->n.width + nkmp->n.shift - 1, > >> > nkmp->n.shift); > >> > > >> > - reg &= ~GENMASK(nkmp->k.width + nkmp->k.shift - 1, > >> > nkmp->k.shift); > >> > + if (nkmp->k.width) > >> > + reg &= ~GENMASK(nkmp->k.width + nkmp->k.shift - 1, > >> > + nkmp->k.shift); > >> > > >> > reg &= ~GENMASK(nkmp->m.width + nkmp->m.shift - 1, > >> > nkmp->m.shift); > >> > reg &= ~GENMASK(nkmp->p.width + nkmp->p.shift - 1, > >> > nkmp->p.shift); > >> > > >> > reg |= (_nkmp.n - nkmp->n.offset) << nkmp->n.shift; > >> > > >> > - reg |= (_nkmp.k - nkmp->k.offset) << nkmp->k.shift; > >> > + if (nkmp->k.width) > >> > + reg |= (_nkmp.k - nkmp->k.offset) << nkmp->k.shift; > >> > >> I think a better way would be > >> > >> reg |= ((_nkmp.k - nkmp->k.offset) << nkmp->k.shift) & > >> > >> GENMASK(nkmp->k.width + nkmp->k.shift - 1, nkmp->k.shift); > >> > >> And do this for all the factors, not just k. This pattern is what > >> regmap_update_bits does, which seems much safer. I wonder what > >> GENMASK() with a negative value would do though... > > > > You're right, GENMASK(-1, 0) equals 0 (calculated by hand, not tested). > > This seems much more elegant solution. > > > > Semi-related question: All nmp PLLs have much wider N range than real nkmp > > PLLs. This causes integer overflow when using nkmp formula from datasheet. > > Usually, N is 1-256 for nmp PLLs, which means that for very high N > > factors, it overflows. This also causes issue that M factor is never > > higher than 1. > Sounds like we can't use u8 for storing the factors. At least the > intermediate values we use to calculate the rates. Only issue with u8 could be max field in struct ccu_mult_internal for K factor. But since it's not used, there is no issue. All intermediate variables in ccu_nkmp are wider. > > > I was wondering, if patch would be acceptable which would change this > > formula: > > > > RATE = (24MHz * N * K) / (M * P) > > > > to this: > > > > RATE ((24MHz / M) * N * K) / P > > > > I checked all M factors and are all in 1-4 or 1-2 range, which means it > > wouldn't have any impact for real nkmp PLLs when parent is 24 MHz clock > > which is probably always. > > > > What do you think? > > I think this is acceptable. M is normally the pre-divider, so this > actually fits how the hardware works, including possible rounding > errors. Ok, I'll add a patch for that in v2. Best regards, Jernej > > ChenYu > > > I discovered that when I tried to set A83T PLL_VIDEO to 346.5 MHz which is > > possible only when above formula is changed. > > > > Best regards, > > Jernej > > > >> ChenYu > >> > >> > reg |= (_nkmp.m - nkmp->m.offset) << nkmp->m.shift; > >> > reg |= ilog2(_nkmp.p) << nkmp->p.shift; > >> > > >> > -- > >> > 2.15.1 > > > > -- > > You received this message because you are subscribed to the Google Groups > > "linux-sunxi" group. To unsubscribe from this group and stop receiving > > emails from it, send an email to > > linux-sunxi+unsubscribe@googlegroups.com. For more options, visit > > https://groups.google.com/d/optout. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Archit, Dne torek, 09. januar 2018 ob 11:43:08 CET je Archit Taneja napisal(a): > On 12/31/2017 02:31 AM, Jernej Skrabec wrote: > > Parts of PHY code could be useful also for custom PHYs. For example, > > Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY > > with few additional memory mapped registers, so most of the Synopsys PHY > > related code could be reused. > > > > It turns out that even completely custom HDMI PHYs, such as the one > > found in Allwinner H3, can reuse some of those functions. This would > > suggest that (some?) functions exported in this commit are actually part > > of generic PHY interface and not really specific to Synopsys PHYs. > > > > Export useful PHY functions. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 45 > > ++++++++++++++++++++++--------- > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 2 ++ > > include/drm/bridge/dw_hdmi.h | 10 +++++++ > > 3 files changed, 44 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > > 7ca14d7325b5..67467d0b683a 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > @@ -1037,19 +1037,21 @@ static void dw_hdmi_phy_enable_svsret(struct > > dw_hdmi *hdmi, u8 enable)> > > HDMI_PHY_CONF0_SVSRET_MASK); > > > > } > > > > -static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable) > > +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable) > > > > { > > > > hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0, > > > > HDMI_PHY_CONF0_GEN2_PDDQ_OFFSET, > > HDMI_PHY_CONF0_GEN2_PDDQ_MASK); > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_pddq); > > > > -static void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable) > > +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable) > > > > { > > > > hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0, > > > > HDMI_PHY_CONF0_GEN2_TXPWRON_OFFSET, > > HDMI_PHY_CONF0_GEN2_TXPWRON_MASK); > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_txpwron); > > > > static void dw_hdmi_phy_sel_data_en_pol(struct dw_hdmi *hdmi, u8 enable) > > { > > > > @@ -1065,6 +1067,23 @@ static void > > dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable)> > > HDMI_PHY_CONF0_SELDIPIF_MASK); > > > > } > > > > +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable) > > +{ > > + hdmi_mask_writeb(hdmi, enable, HDMI_MC_PHYRSTZ, > > + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET, > > + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK); > > +} > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_reset); > > + > > +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi) > > +{ > > + hdmi_phy_test_clear(hdmi, 1); > > + hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > > + HDMI_PHY_I2CM_SLAVE_ADDR); > > + hdmi_phy_test_clear(hdmi, 0); > > +} > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_set_slave_addr); > > Should this be called dw_hdmi_phy_gen2_set_slave_addr? Probably. I will rename it in v2 to be consistent with other phy functions. Best regards, Jernej > > Looks good otherwise. Same for patches 3 and 4 in this series. > > Thanks, > Archit > > > + > > > > static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > > { > > > > const struct dw_hdmi_phy_data *phy = hdmi->phy.data; > > > > @@ -1204,15 +1223,12 @@ static int hdmi_phy_configure(struct dw_hdmi > > *hdmi) > > > > dw_hdmi_phy_enable_svsret(hdmi, 1); > > > > /* PHY reset. The reset signal is active high on Gen2 PHYs. */ > > > > - hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_PHYRSTZ, HDMI_MC_PHYRSTZ); > > - hdmi_writeb(hdmi, 0, HDMI_MC_PHYRSTZ); > > + dw_hdmi_phy_gen2_reset(hdmi, 1); > > + dw_hdmi_phy_gen2_reset(hdmi, 0); > > > > hdmi_writeb(hdmi, HDMI_MC_HEACPHY_RST_ASSERT, HDMI_MC_HEACPHY_RST); > > > > - hdmi_phy_test_clear(hdmi, 1); > > - hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > > - HDMI_PHY_I2CM_SLAVE_ADDR); > > - hdmi_phy_test_clear(hdmi, 0); > > + dw_hdmi_phy_set_slave_addr(hdmi); > > > > /* Write to the PHY as configured by the platform */ > > if (pdata->configure_phy) > > > > @@ -1251,15 +1267,16 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > > *hdmi, void *data)> > > dw_hdmi_phy_power_off(hdmi); > > > > } > > > > -static enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi > > *hdmi, - void *data) > > +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > > + void *data) > > > > { > > > > return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ? > > > > connector_status_connected : connector_status_disconnected; > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_read_hpd); > > > > -static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > > - bool force, bool disabled, bool rxsense) > > +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > > + bool force, bool disabled, bool rxsense) > > > > { > > > > u8 old_mask = hdmi->phy_mask; > > > > @@ -1271,8 +1288,9 @@ static void dw_hdmi_phy_update_hpd(struct dw_hdmi > > *hdmi, void *data,> > > if (old_mask != hdmi->phy_mask) > > > > hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_update_hpd); > > > > -static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > > +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > > > > { > > > > /* > > > > * Configure the PHY RX SENSE and HPD interrupts polarities and clear > > > > @@ -1291,6 +1309,7 @@ static void dw_hdmi_phy_setup_hpd(struct dw_hdmi > > *hdmi, void *data)> > > hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | > > HDMI_IH_PHY_STAT0_RX_SENSE), > > > > HDMI_IH_MUTE_PHY_STAT0); > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_setup_hpd); > > > > static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = { > > > > .init = dw_hdmi_phy_init, > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h index > > 9d90eb9c46e5..fd150430d0b3 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > > @@ -950,6 +950,8 @@ enum { > > > > /* MC_PHYRSTZ field values */ > > > > HDMI_MC_PHYRSTZ_PHYRSTZ = 0x01, > > > > + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET = 0x00, > > + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK = 0x01, > > > > /* MC_HEACPHY_RST field values */ > > > > HDMI_MC_HEACPHY_RST_ASSERT = 0x1, > > > > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > > index 182f83283e24..f5cca4362154 100644 > > --- a/include/drm/bridge/dw_hdmi.h > > +++ b/include/drm/bridge/dw_hdmi.h > > @@ -159,5 +159,15 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi); > > > > /* PHY configuration */ > > void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data, > > > > unsigned char addr); > > > > +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > > + void *data); > > +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > > + bool force, bool disabled, bool rxsense); > > +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); > > + > > +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable); > > +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable); > > +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable); > > +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi); > > > > #endif /* __IMX_HDMI_H__ */ > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, Dne torek, 09. januar 2018 ob 14:30:22 CET je Laurent Pinchart napisal(a): > Hi Jernej, > > Thank you for the patch. > > On Saturday, 30 December 2017 23:01:56 EET Jernej Skrabec wrote: > > Parts of PHY code could be useful also for custom PHYs. For example, > > Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY > > with few additional memory mapped registers, so most of the Synopsys PHY > > related code could be reused. > > > > It turns out that even completely custom HDMI PHYs, such as the one > > found in Allwinner H3, can reuse some of those functions. This would > > suggest that (some?) functions exported in this commit are actually part > > of generic PHY interface and not really specific to Synopsys PHYs. > > That's correct, those functions control the interface between the HDMI > controller and the PHY. They're not specific to Synopsys PHYs, but they're > specific to the PHY interface as designed by Synopsys. Ok, I'll update commit message. > > > Export useful PHY functions. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 45 > > ++++++++++++++++++++------- > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 2 ++ > > include/drm/bridge/dw_hdmi.h | 10 +++++++ > > 3 files changed, 44 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > > 7ca14d7325b5..67467d0b683a 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > @@ -1037,19 +1037,21 @@ static void dw_hdmi_phy_enable_svsret(struct > > dw_hdmi *hdmi, u8 enable) HDMI_PHY_CONF0_SVSRET_MASK); > > > > } > > > > -static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable) > > +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable) > > > > { > > > > hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0, > > > > HDMI_PHY_CONF0_GEN2_PDDQ_OFFSET, > > HDMI_PHY_CONF0_GEN2_PDDQ_MASK); > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_pddq); > > > > -static void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable) > > +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable) > > > > { > > > > hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0, > > > > HDMI_PHY_CONF0_GEN2_TXPWRON_OFFSET, > > HDMI_PHY_CONF0_GEN2_TXPWRON_MASK); > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_txpwron); > > > > static void dw_hdmi_phy_sel_data_en_pol(struct dw_hdmi *hdmi, u8 enable) > > { > > > > @@ -1065,6 +1067,23 @@ static void > > dw_hdmi_phy_sel_interface_control(struct > > dw_hdmi *hdmi, u8 enable) HDMI_PHY_CONF0_SELDIPIF_MASK); > > > > } > > > > +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable) > > +{ > > + hdmi_mask_writeb(hdmi, enable, HDMI_MC_PHYRSTZ, > > + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET, > > + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK); > > +} > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_reset); > > + > > +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi) > > +{ > > + hdmi_phy_test_clear(hdmi, 1); > > + hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > > + HDMI_PHY_I2CM_SLAVE_ADDR); > > + hdmi_phy_test_clear(hdmi, 0); > > +} > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_set_slave_addr); > > Should the I2C address be passed as an argument ? Yes, I already planned to do that for v2. Best regards, Jernej > > > static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > > { > > > > const struct dw_hdmi_phy_data *phy = hdmi->phy.data; > > > > @@ -1204,15 +1223,12 @@ static int hdmi_phy_configure(struct dw_hdmi > > *hdmi) > > > > dw_hdmi_phy_enable_svsret(hdmi, 1); > > > > /* PHY reset. The reset signal is active high on Gen2 PHYs. */ > > > > - hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_PHYRSTZ, HDMI_MC_PHYRSTZ); > > - hdmi_writeb(hdmi, 0, HDMI_MC_PHYRSTZ); > > + dw_hdmi_phy_gen2_reset(hdmi, 1); > > + dw_hdmi_phy_gen2_reset(hdmi, 0); > > > > hdmi_writeb(hdmi, HDMI_MC_HEACPHY_RST_ASSERT, HDMI_MC_HEACPHY_RST); > > > > - hdmi_phy_test_clear(hdmi, 1); > > - hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > > - HDMI_PHY_I2CM_SLAVE_ADDR); > > - hdmi_phy_test_clear(hdmi, 0); > > + dw_hdmi_phy_set_slave_addr(hdmi); > > > > /* Write to the PHY as configured by the platform */ > > if (pdata->configure_phy) > > > > @@ -1251,15 +1267,16 @@ static void dw_hdmi_phy_disable(struct dw_hdmi > > *hdmi, void *data) dw_hdmi_phy_power_off(hdmi); > > > > } > > > > -static enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi > > *hdmi, - void *data) > > +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > > + void *data) > > > > { > > > > return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ? > > > > connector_status_connected : connector_status_disconnected; > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_read_hpd); > > > > -static void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > > - bool force, bool disabled, bool rxsense) > > +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > > + bool force, bool disabled, bool rxsense) > > > > { > > > > u8 old_mask = hdmi->phy_mask; > > > > @@ -1271,8 +1288,9 @@ static void dw_hdmi_phy_update_hpd(struct dw_hdmi > > *hdmi, void *data, if (old_mask != hdmi->phy_mask) > > > > hdmi_writeb(hdmi, hdmi->phy_mask, HDMI_PHY_MASK0); > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_update_hpd); > > > > -static void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > > +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data) > > > > { > > > > /* > > > > * Configure the PHY RX SENSE and HPD interrupts polarities and clear > > > > @@ -1291,6 +1309,7 @@ static void dw_hdmi_phy_setup_hpd(struct dw_hdmi > > *hdmi, void *data) hdmi_writeb(hdmi, ~(HDMI_IH_PHY_STAT0_HPD | > > HDMI_IH_PHY_STAT0_RX_SENSE), HDMI_IH_MUTE_PHY_STAT0); > > > > } > > > > +EXPORT_SYMBOL_GPL(dw_hdmi_phy_setup_hpd); > > > > static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = { > > > > .init = dw_hdmi_phy_init, > > > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > > b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h index > > 9d90eb9c46e5..fd150430d0b3 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h > > @@ -950,6 +950,8 @@ enum { > > > > /* MC_PHYRSTZ field values */ > > > > HDMI_MC_PHYRSTZ_PHYRSTZ = 0x01, > > > > + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET = 0x00, > > + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK = 0x01, > > > > /* MC_HEACPHY_RST field values */ > > > > HDMI_MC_HEACPHY_RST_ASSERT = 0x1, > > > > diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h > > index 182f83283e24..f5cca4362154 100644 > > --- a/include/drm/bridge/dw_hdmi.h > > +++ b/include/drm/bridge/dw_hdmi.h > > @@ -159,5 +159,15 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi); > > > > /* PHY configuration */ > > void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data, > > > > unsigned char addr); > > > > +enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi, > > + void *data); > > +void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data, > > + bool force, bool disabled, bool rxsense); > > +void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data); > > + > > +void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable); > > +void dw_hdmi_phy_gen2_txpwron(struct dw_hdmi *hdmi, u8 enable); > > +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable); > > +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi); > > > > #endif /* __IMX_HDMI_H__ */ > > -- > Regards, > > Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Tuesday, 9 January 2018 17:58:46 EET Jernej Škrabec wrote: > Dne torek, 09. januar 2018 ob 11:43:08 CET je Archit Taneja napisal(a): > > On 12/31/2017 02:31 AM, Jernej Skrabec wrote: > >> Parts of PHY code could be useful also for custom PHYs. For example, > >> Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY > >> with few additional memory mapped registers, so most of the Synopsys PHY > >> related code could be reused. > >> > >> It turns out that even completely custom HDMI PHYs, such as the one > >> found in Allwinner H3, can reuse some of those functions. This would > >> suggest that (some?) functions exported in this commit are actually part > >> of generic PHY interface and not really specific to Synopsys PHYs. > >> > >> Export useful PHY functions. > >> > >> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > >> --- > >> > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 45 +++++++++++++++++------- > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 2 ++ > >> include/drm/bridge/dw_hdmi.h | 10 +++++++ > >> 3 files changed, 44 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > >> 7ca14d7325b5..67467d0b683a 100644 > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c [snip] > >> @@ -1065,6 +1067,23 @@ static void > >> dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable) > >> HDMI_PHY_CONF0_SELDIPIF_MASK); > >> } > >> > >> +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable) > >> +{ > >> + hdmi_mask_writeb(hdmi, enable, HDMI_MC_PHYRSTZ, > >> + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET, > >> + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK); > >> +} > >> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_reset); I don't remember the details, is the reset signal Gen2-specific ? How about asserting and deasserting the reset signal in the same call instead of having to call this function twice ? > >> +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi) > >> +{ > >> + hdmi_phy_test_clear(hdmi, 1); > >> + hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > >> + HDMI_PHY_I2CM_SLAVE_ADDR); > >> + hdmi_phy_test_clear(hdmi, 0); > >> +} > >> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_set_slave_addr); > > > > Should this be called dw_hdmi_phy_gen2_set_slave_addr? > > Probably. I will rename it in v2 to be consistent with other phy functions. The I2C write function is called dw_hdmi_phy_i2c_write(). If we want to be conosistent we should either rename this one to dw_hdmi_phy_i2c_set_addr() or rename them both to dw_hdmi_phy_gen2_i2c_write() and dw_hdmi_phy_gen2_i2c_set_addr(). I think I'd prefer the former, and we could even drop gen2 from dw_hdmi_phy_gen2_pddq() and dw_hdmi_phy_gen2_txpwron() if desired. > > Looks good otherwise. Same for patches 3 and 4 in this series. > > > >> + > >> static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > >> { > >> const struct dw_hdmi_phy_data *phy = hdmi->phy.data; [snip]
Hi, Dne torek, 09. januar 2018 ob 17:08:55 CET je Laurent Pinchart napisal(a): > Hello, > > On Tuesday, 9 January 2018 17:58:46 EET Jernej Škrabec wrote: > > Dne torek, 09. januar 2018 ob 11:43:08 CET je Archit Taneja napisal(a): > > > On 12/31/2017 02:31 AM, Jernej Skrabec wrote: > > >> Parts of PHY code could be useful also for custom PHYs. For example, > > >> Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY > > >> with few additional memory mapped registers, so most of the Synopsys > > >> PHY > > >> related code could be reused. > > >> > > >> It turns out that even completely custom HDMI PHYs, such as the one > > >> found in Allwinner H3, can reuse some of those functions. This would > > >> suggest that (some?) functions exported in this commit are actually > > >> part > > >> of generic PHY interface and not really specific to Synopsys PHYs. > > >> > > >> Export useful PHY functions. > > >> > > >> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > >> --- > > >> > > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 45 +++++++++++++++++------- > > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 2 ++ > > >> include/drm/bridge/dw_hdmi.h | 10 +++++++ > > >> 3 files changed, 44 insertions(+), 13 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > > >> 7ca14d7325b5..67467d0b683a 100644 > > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > [snip] > > > >> @@ -1065,6 +1067,23 @@ static void > > >> dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable) > > >> > > >> HDMI_PHY_CONF0_SELDIPIF_MASK); > > >> > > >> } > > >> > > >> +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable) > > >> +{ > > >> + hdmi_mask_writeb(hdmi, enable, HDMI_MC_PHYRSTZ, > > >> + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET, > > >> + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK); > > >> +} > > >> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_reset); > > I don't remember the details, is the reset signal Gen2-specific ? I don't know, since there is not much documentation. Should I remove "gen2" from the name? > > How about asserting and deasserting the reset signal in the same call > instead of having to call this function twice ? A83T BSP driver clears txpwron and sets pddq between asserting and deasserting reset. I'll test if it works with your proposal. > > > >> +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi) > > >> +{ > > >> + hdmi_phy_test_clear(hdmi, 1); > > >> + hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > > >> + HDMI_PHY_I2CM_SLAVE_ADDR); > > >> + hdmi_phy_test_clear(hdmi, 0); > > >> +} > > >> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_set_slave_addr); > > > > > > Should this be called dw_hdmi_phy_gen2_set_slave_addr? > > > > Probably. I will rename it in v2 to be consistent with other phy > > functions. > > The I2C write function is called dw_hdmi_phy_i2c_write(). If we want to be > conosistent we should either rename this one to dw_hdmi_phy_i2c_set_addr() > or rename them both to dw_hdmi_phy_gen2_i2c_write() and > dw_hdmi_phy_gen2_i2c_set_addr(). I think I'd prefer the former, and we could > even drop gen2 from dw_hdmi_phy_gen2_pddq() and dw_hdmi_phy_gen2_txpwron() > if desired. Ok, then I'll name it dw_hdmi_phy_i2c_set_addr(). I'll leave other names as they are. Best regards, Jernej > > > > Looks good otherwise. Same for patches 3 and 4 in this series. > > > > > >> + > > >> > > >> static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > > >> { > > >> > > >> const struct dw_hdmi_phy_data *phy = hdmi->phy.data; > > [snip] > > -- > Regards, > > Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, Dne torek, 09. januar 2018 ob 17:08:55 CET je Laurent Pinchart napisal(a): > Hello, > > On Tuesday, 9 January 2018 17:58:46 EET Jernej Škrabec wrote: > > Dne torek, 09. januar 2018 ob 11:43:08 CET je Archit Taneja napisal(a): > > > On 12/31/2017 02:31 AM, Jernej Skrabec wrote: > > >> Parts of PHY code could be useful also for custom PHYs. For example, > > >> Allwinner A83T has custom PHY which is probably Synopsys gen2 PHY > > >> with few additional memory mapped registers, so most of the Synopsys > > >> PHY > > >> related code could be reused. > > >> > > >> It turns out that even completely custom HDMI PHYs, such as the one > > >> found in Allwinner H3, can reuse some of those functions. This would > > >> suggest that (some?) functions exported in this commit are actually > > >> part > > >> of generic PHY interface and not really specific to Synopsys PHYs. > > >> > > >> Export useful PHY functions. > > >> > > >> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > >> --- > > >> > > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 45 +++++++++++++++++------- > > >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.h | 2 ++ > > >> include/drm/bridge/dw_hdmi.h | 10 +++++++ > > >> 3 files changed, 44 insertions(+), 13 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > >> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index > > >> 7ca14d7325b5..67467d0b683a 100644 > > >> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > > [snip] > > > >> @@ -1065,6 +1067,23 @@ static void > > >> dw_hdmi_phy_sel_interface_control(struct dw_hdmi *hdmi, u8 enable) > > >> > > >> HDMI_PHY_CONF0_SELDIPIF_MASK); > > >> > > >> } > > >> > > >> +void dw_hdmi_phy_gen2_reset(struct dw_hdmi *hdmi, u8 enable) > > >> +{ > > >> + hdmi_mask_writeb(hdmi, enable, HDMI_MC_PHYRSTZ, > > >> + HDMI_MC_PHYRSTZ_PHYRSTZ_OFFSET, > > >> + HDMI_MC_PHYRSTZ_PHYRSTZ_MASK); > > >> +} > > >> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_gen2_reset); > > I don't remember the details, is the reset signal Gen2-specific ? According to this comment: /* PHY reset. The reset signal is active high on Gen2 PHYs. */ I would guess that it is not specific to Gen2, otherwise the comment wouldn't be needed. I will remove "gen2" from the name. > > How about asserting and deasserting the reset signal in the same call > instead of having to call this function twice ? It works on A83T if reset signal is asserted and deasserted immediately. I will change function according to your proposal. Best regards, Jernej > > > >> +void dw_hdmi_phy_set_slave_addr(struct dw_hdmi *hdmi) > > >> +{ > > >> + hdmi_phy_test_clear(hdmi, 1); > > >> + hdmi_writeb(hdmi, HDMI_PHY_I2CM_SLAVE_ADDR_PHY_GEN2, > > >> + HDMI_PHY_I2CM_SLAVE_ADDR); > > >> + hdmi_phy_test_clear(hdmi, 0); > > >> +} > > >> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_set_slave_addr); > > > > > > Should this be called dw_hdmi_phy_gen2_set_slave_addr? > > > > Probably. I will rename it in v2 to be consistent with other phy > > functions. > > The I2C write function is called dw_hdmi_phy_i2c_write(). If we want to be > conosistent we should either rename this one to dw_hdmi_phy_i2c_set_addr() > or rename them both to dw_hdmi_phy_gen2_i2c_write() and > dw_hdmi_phy_gen2_i2c_set_addr(). I think I'd prefer the former, and we could > even drop gen2 from dw_hdmi_phy_gen2_pddq() and dw_hdmi_phy_gen2_txpwron() > if desired. > > > > Looks good otherwise. Same for patches 3 and 4 in this series. > > > > > >> + > > >> > > >> static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi) > > >> { > > >> > > >> const struct dw_hdmi_phy_data *phy = hdmi->phy.data; > > [snip] > > -- > Regards, > > Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html