* [PATCH v2 0/2] drm/sun4i: dw-hdmi: Improve CEC support
@ 2019-04-01 19:15 Jernej Skrabec
2019-04-01 19:15 ` [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Add an option to suppress loading CEC driver Jernej Skrabec
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Jernej Skrabec @ 2019-04-01 19:15 UTC (permalink / raw)
To: maxime.ripard
Cc: a.hajda, Laurent.pinchart, airlied, daniel, wens, narmstrong,
dri-devel, linux-kernel, linux-arm-kernel
It turns out that additional logic between HDMI CEC controller and
pins on PHY on some Allwinner SoCs prevents proper communication.
It might be possible to fix it, but it's much easier and less error
prone to just directly drive pins using software implementation of
CEC protocol.
Let me know what do you think.
Best regards,
Jernej
Changes from v1:
- renamed is_cec_unusable to disable_cec
- added review-by tag
Jernej Skrabec (2):
drm/bridge/synopsys: dw-hdmi: Add an option to suppress loading CEC
driver
drm/sun4i: dw-hdmi: Bit bang CEC on some SoCs
drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +-
drivers/gpu/drm/sun4i/Kconfig | 10 +++
drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 11 +++
drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 83 ++++++++++++++++++++++-
include/drm/bridge/dw_hdmi.h | 2 +
5 files changed, 105 insertions(+), 3 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Add an option to suppress loading CEC driver 2019-04-01 19:15 [PATCH v2 0/2] drm/sun4i: dw-hdmi: Improve CEC support Jernej Skrabec @ 2019-04-01 19:15 ` Jernej Skrabec 2019-04-01 19:15 ` [PATCH v2 2/2] drm/sun4i: dw-hdmi: Bit bang CEC on some SoCs Jernej Skrabec 2019-04-12 10:15 ` [PATCH v2 0/2] drm/sun4i: dw-hdmi: Improve CEC support Hans Verkuil 2 siblings, 0 replies; 5+ messages in thread From: Jernej Skrabec @ 2019-04-01 19:15 UTC (permalink / raw) To: maxime.ripard Cc: a.hajda, Laurent.pinchart, airlied, daniel, wens, narmstrong, dri-devel, linux-kernel, linux-arm-kernel DW HDMI controller on some Allwinner SoCs has support for CEC, but due to additional logic put between CEC controller and pins, it doesn't work correctly, at least not with a lot of instrusive changes. Fortunately, it's still possible to bitbang protocol. For such cases, add a platform option to suppress loading CEC driver. If DW HDMI CEC driver would be loaded, it wouldn't work anyway and would only cause a confusion with multiple /dev entries. Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- include/drm/bridge/dw_hdmi.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index db761329a1e3..d2b0aa8d964c 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2660,7 +2660,7 @@ __dw_hdmi_probe(struct platform_device *pdev, hdmi->audio = platform_device_register_full(&pdevinfo); } - if (config0 & HDMI_CONFIG0_CEC) { + if (!plat_data->disable_cec && (config0 & HDMI_CONFIG0_CEC)) { cec.hdmi = hdmi; cec.ops = &dw_hdmi_cec_ops; cec.irq = irq; diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index 66e70770cce5..c03bea3a1a36 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -144,6 +144,8 @@ struct dw_hdmi_plat_data { int (*configure_phy)(struct dw_hdmi *hdmi, const struct dw_hdmi_plat_data *pdata, unsigned long mpixelclock); + + unsigned int disable_cec : 1; }; struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev, -- 2.21.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] drm/sun4i: dw-hdmi: Bit bang CEC on some SoCs 2019-04-01 19:15 [PATCH v2 0/2] drm/sun4i: dw-hdmi: Improve CEC support Jernej Skrabec 2019-04-01 19:15 ` [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Add an option to suppress loading CEC driver Jernej Skrabec @ 2019-04-01 19:15 ` Jernej Skrabec 2019-04-12 10:24 ` Hans Verkuil 2019-04-12 10:15 ` [PATCH v2 0/2] drm/sun4i: dw-hdmi: Improve CEC support Hans Verkuil 2 siblings, 1 reply; 5+ messages in thread From: Jernej Skrabec @ 2019-04-01 19:15 UTC (permalink / raw) To: maxime.ripard Cc: a.hajda, Laurent.pinchart, airlied, daniel, wens, narmstrong, dri-devel, linux-kernel, linux-arm-kernel All DW HDMI controllers used by Allwinner SoCs include CEC controller. However, due to additional logic put between CEC controller and pins, CEC communication doesn't work well on some of them. Based on observations, it seems that only outgoing messages are properly transmitted. It's possible that it would still work correctly if pins are switched between input and output mode manually in right moment. But that's very error prone. It's better and easier just to bit bang protocol. Enable bit banging just for controller and phy combination found in H3 and other 40nm SoCs. Other combinations work (H6) or the status is unknown (A83T). Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> --- drivers/gpu/drm/sun4i/Kconfig | 10 ++++ drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 11 ++++ drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 83 +++++++++++++++++++++++++- 3 files changed, 102 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig index 1dbbc3a1b763..7149c72e44c8 100644 --- a/drivers/gpu/drm/sun4i/Kconfig +++ b/drivers/gpu/drm/sun4i/Kconfig @@ -60,6 +60,16 @@ config DRM_SUN8I_DW_HDMI DesignWare HDMI controller with custom HDMI PHY. If M is selected the module will be called sun8i_dw_hdmi. +config DRM_SUN8I_DW_HDMI_CEC + bool "Allwinner DesignWare HDMI CEC Support for 40nm SoCs" + depends on DRM_SUN8I_DW_HDMI + select CEC_CORE + select CEC_PIN + help + Choose this option if you have an 40nm Allwinner SoC with + the DesignWare HDMI controller with custom HDMI PHY and + you want to use CEC. + config DRM_SUN8I_MIXER tristate "Support for Allwinner Display Engine 2.0 Mixer" default MACH_SUN8I diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h index 720c5aa8adc1..49ca001923e3 100644 --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h @@ -12,6 +12,7 @@ #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/reset.h> +#include <media/cec-pin.h> #define SUN8I_HDMI_PHY_DBG_CTRL_REG 0x0000 #define SUN8I_HDMI_PHY_DBG_CTRL_PX_LOCK BIT(0) @@ -144,6 +145,13 @@ #define SUN8I_HDMI_PHY_ANA_STS_RCAL_MASK GENMASK(5, 0) #define SUN8I_HDMI_PHY_CEC_REG 0x003c +#define SUN8I_HDMI_PHY_CEC_PIN_CTRL BIT(7) +/* + * Documentation says that this bit is output enable. However, + * it seems that this bit is actually output disable. + */ +#define SUN8I_HDMI_PHY_CEC_OUT_DIS BIT(2) +#define SUN8I_HDMI_PHY_CEC_IN_DATA BIT(1) struct sun8i_hdmi_phy; @@ -151,6 +159,7 @@ struct sun8i_hdmi_phy_variant { bool has_phy_clk; bool has_second_pll; unsigned int is_custom_phy : 1; + unsigned int bit_bang_cec : 1; const struct dw_hdmi_curr_ctrl *cur_ctr; const struct dw_hdmi_mpll_config *mpll_cfg; const struct dw_hdmi_phy_config *phy_cfg; @@ -163,6 +172,8 @@ struct sun8i_hdmi_phy_variant { }; struct sun8i_hdmi_phy { + struct cec_adapter *cec_adapter; + struct cec_notifier *cec_notifier; struct clk *clk_bus; struct clk *clk_mod; struct clk *clk_phy; diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c index 66ea3a902e36..8fd6bf91714e 100644 --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c @@ -503,8 +503,9 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy) regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, 0); - /* set HW control of CEC pins */ - regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, 0); + /* manual control of CEC pins */ + regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, + SUN8I_HDMI_PHY_CEC_PIN_CTRL); /* read calibration data */ regmap_read(phy->regs, SUN8I_HDMI_PHY_ANA_STS_REG, &val); @@ -530,8 +531,49 @@ void sun8i_hdmi_phy_set_ops(struct sun8i_hdmi_phy *phy, plat_data->cur_ctr = variant->cur_ctr; plat_data->phy_config = variant->phy_cfg; } + plat_data->disable_cec = phy->variant->bit_bang_cec; } +#ifdef CONFIG_DRM_SUN8I_DW_HDMI_CEC +static bool sun8i_hdmi_phy_cec_pin_read(struct cec_adapter *adap) +{ + struct sun8i_hdmi_phy *phy = cec_get_drvdata(adap); + unsigned int val; + + regmap_read(phy->regs, SUN8I_HDMI_PHY_CEC_REG, &val); + + return val & SUN8I_HDMI_PHY_CEC_IN_DATA; +} + +static void sun8i_hdmi_phy_cec_pin_low(struct cec_adapter *adap) +{ + struct sun8i_hdmi_phy *phy = cec_get_drvdata(adap); + + /* Start driving the CEC pin low */ + regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, + SUN8I_HDMI_PHY_CEC_PIN_CTRL); +} + +static void sun8i_hdmi_phy_cec_pin_high(struct cec_adapter *adap) +{ + struct sun8i_hdmi_phy *phy = cec_get_drvdata(adap); + + /* + * Stop driving the CEC pin, the pull up will take over + * unless another CEC device is driving the pin low. + */ + regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, + SUN8I_HDMI_PHY_CEC_PIN_CTRL | + SUN8I_HDMI_PHY_CEC_OUT_DIS); +} + +static const struct cec_pin_ops sun8i_hdmi_phy_cec_pin_ops = { + .read = sun8i_hdmi_phy_cec_pin_read, + .low = sun8i_hdmi_phy_cec_pin_low, + .high = sun8i_hdmi_phy_cec_pin_high, +}; +#endif + static struct regmap_config sun8i_hdmi_phy_regmap_config = { .reg_bits = 32, .val_bits = 32, @@ -548,6 +590,7 @@ static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = { }; static const struct sun8i_hdmi_phy_variant sun8i_h3_hdmi_phy = { + .bit_bang_cec = true, .has_phy_clk = true, .is_custom_phy = true, .phy_init = &sun8i_hdmi_phy_init_h3, @@ -556,6 +599,7 @@ static const struct sun8i_hdmi_phy_variant sun8i_h3_hdmi_phy = { }; static const struct sun8i_hdmi_phy_variant sun8i_r40_hdmi_phy = { + .bit_bang_cec = true, .has_phy_clk = true, .has_second_pll = true, .is_custom_phy = true, @@ -565,6 +609,7 @@ static const struct sun8i_hdmi_phy_variant sun8i_r40_hdmi_phy = { }; static const struct sun8i_hdmi_phy_variant sun50i_a64_hdmi_phy = { + .bit_bang_cec = true, .has_phy_clk = true, .is_custom_phy = true, .phy_init = &sun8i_hdmi_phy_init_h3, @@ -708,10 +753,40 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node) goto err_disable_clk_bus; } +#ifdef CONFIG_DRM_SUN8I_DW_HDMI_CEC + if (phy->variant->bit_bang_cec) { + phy->cec_notifier = cec_notifier_get(dev); + if (!phy->cec_notifier) { + ret = -ENOMEM; + goto err_disable_clk_mod; + } + + phy->cec_adapter = + cec_pin_allocate_adapter(&sun8i_hdmi_phy_cec_pin_ops, + phy, "sun8i-cec", + CEC_CAP_DEFAULTS); + ret = PTR_ERR_OR_ZERO(phy->cec_adapter); + if (ret < 0) + goto err_put_cec_notifier; + + ret = cec_register_adapter(phy->cec_adapter, dev); + if (ret < 0) + goto err_delete_cec_adapter; + + cec_register_cec_notifier(phy->cec_adapter, phy->cec_notifier); + } +#endif + hdmi->phy = phy; return 0; +err_delete_cec_adapter: + cec_delete_adapter(phy->cec_adapter); +err_put_cec_notifier: + cec_notifier_put(phy->cec_notifier); +err_disable_clk_mod: + clk_disable_unprepare(phy->clk_mod); err_disable_clk_bus: clk_disable_unprepare(phy->clk_bus); err_deassert_rst_phy: @@ -736,6 +811,10 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi) { struct sun8i_hdmi_phy *phy = hdmi->phy; + cec_unregister_adapter(phy->cec_adapter); + if (phy->cec_notifier) + cec_notifier_put(phy->cec_notifier); + clk_disable_unprepare(phy->clk_mod); clk_disable_unprepare(phy->clk_bus); clk_disable_unprepare(phy->clk_phy); -- 2.21.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] drm/sun4i: dw-hdmi: Bit bang CEC on some SoCs 2019-04-01 19:15 ` [PATCH v2 2/2] drm/sun4i: dw-hdmi: Bit bang CEC on some SoCs Jernej Skrabec @ 2019-04-12 10:24 ` Hans Verkuil 0 siblings, 0 replies; 5+ messages in thread From: Hans Verkuil @ 2019-04-12 10:24 UTC (permalink / raw) To: Jernej Skrabec, maxime.ripard Cc: narmstrong, airlied, linux-kernel, dri-devel, wens, Laurent.pinchart, linux-arm-kernel On 4/1/19 9:15 PM, Jernej Skrabec wrote: > All DW HDMI controllers used by Allwinner SoCs include CEC controller. > However, due to additional logic put between CEC controller and pins, > CEC communication doesn't work well on some of them. > > Based on observations, it seems that only outgoing messages are > properly transmitted. It's possible that it would still work correctly > if pins are switched between input and output mode manually in right > moment. But that's very error prone. It's better and easier just to bit > bang protocol. > > Enable bit banging just for controller and phy combination found in H3 > and other 40nm SoCs. Other combinations work (H6) or the status is > unknown (A83T). > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/gpu/drm/sun4i/Kconfig | 10 ++++ > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 11 ++++ > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 83 +++++++++++++++++++++++++- > 3 files changed, 102 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/Kconfig b/drivers/gpu/drm/sun4i/Kconfig > index 1dbbc3a1b763..7149c72e44c8 100644 > --- a/drivers/gpu/drm/sun4i/Kconfig > +++ b/drivers/gpu/drm/sun4i/Kconfig > @@ -60,6 +60,16 @@ config DRM_SUN8I_DW_HDMI > DesignWare HDMI controller with custom HDMI PHY. If M is > selected the module will be called sun8i_dw_hdmi. > > +config DRM_SUN8I_DW_HDMI_CEC > + bool "Allwinner DesignWare HDMI CEC Support for 40nm SoCs" I'm not sure mentioning "DesignWare" here is right. The CEC support (as I understand it) bypasses the DW entirely and is really a sunxi specific implementation. > + depends on DRM_SUN8I_DW_HDMI > + select CEC_CORE > + select CEC_PIN > + help > + Choose this option if you have an 40nm Allwinner SoC with > + the DesignWare HDMI controller with custom HDMI PHY and > + you want to use CEC. > + > config DRM_SUN8I_MIXER > tristate "Support for Allwinner Display Engine 2.0 Mixer" > default MACH_SUN8I > diff --git a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > index 720c5aa8adc1..49ca001923e3 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > +++ b/drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h > @@ -12,6 +12,7 @@ > #include <linux/regmap.h> > #include <linux/regulator/consumer.h> > #include <linux/reset.h> > +#include <media/cec-pin.h> > > #define SUN8I_HDMI_PHY_DBG_CTRL_REG 0x0000 > #define SUN8I_HDMI_PHY_DBG_CTRL_PX_LOCK BIT(0) > @@ -144,6 +145,13 @@ > #define SUN8I_HDMI_PHY_ANA_STS_RCAL_MASK GENMASK(5, 0) > > #define SUN8I_HDMI_PHY_CEC_REG 0x003c > +#define SUN8I_HDMI_PHY_CEC_PIN_CTRL BIT(7) > +/* > + * Documentation says that this bit is output enable. However, > + * it seems that this bit is actually output disable. > + */ > +#define SUN8I_HDMI_PHY_CEC_OUT_DIS BIT(2) > +#define SUN8I_HDMI_PHY_CEC_IN_DATA BIT(1) > > struct sun8i_hdmi_phy; > > @@ -151,6 +159,7 @@ struct sun8i_hdmi_phy_variant { > bool has_phy_clk; > bool has_second_pll; > unsigned int is_custom_phy : 1; > + unsigned int bit_bang_cec : 1; > const struct dw_hdmi_curr_ctrl *cur_ctr; > const struct dw_hdmi_mpll_config *mpll_cfg; > const struct dw_hdmi_phy_config *phy_cfg; > @@ -163,6 +172,8 @@ struct sun8i_hdmi_phy_variant { > }; > > struct sun8i_hdmi_phy { > + struct cec_adapter *cec_adapter; > + struct cec_notifier *cec_notifier; > struct clk *clk_bus; > struct clk *clk_mod; > struct clk *clk_phy; > diff --git a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > index 66ea3a902e36..8fd6bf91714e 100644 > --- a/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > +++ b/drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c > @@ -503,8 +503,9 @@ static void sun8i_hdmi_phy_init_h3(struct sun8i_hdmi_phy *phy) > regmap_update_bits(phy->regs, SUN8I_HDMI_PHY_PLL_CFG1_REG, > SUN8I_HDMI_PHY_PLL_CFG1_CKIN_SEL_MSK, 0); > > - /* set HW control of CEC pins */ > - regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, 0); > + /* manual control of CEC pins */ > + regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, > + SUN8I_HDMI_PHY_CEC_PIN_CTRL); > > /* read calibration data */ > regmap_read(phy->regs, SUN8I_HDMI_PHY_ANA_STS_REG, &val); > @@ -530,8 +531,49 @@ void sun8i_hdmi_phy_set_ops(struct sun8i_hdmi_phy *phy, > plat_data->cur_ctr = variant->cur_ctr; > plat_data->phy_config = variant->phy_cfg; > } > + plat_data->disable_cec = phy->variant->bit_bang_cec; > } > > +#ifdef CONFIG_DRM_SUN8I_DW_HDMI_CEC > +static bool sun8i_hdmi_phy_cec_pin_read(struct cec_adapter *adap) > +{ > + struct sun8i_hdmi_phy *phy = cec_get_drvdata(adap); > + unsigned int val; > + > + regmap_read(phy->regs, SUN8I_HDMI_PHY_CEC_REG, &val); > + > + return val & SUN8I_HDMI_PHY_CEC_IN_DATA; > +} > + > +static void sun8i_hdmi_phy_cec_pin_low(struct cec_adapter *adap) > +{ > + struct sun8i_hdmi_phy *phy = cec_get_drvdata(adap); > + > + /* Start driving the CEC pin low */ > + regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, > + SUN8I_HDMI_PHY_CEC_PIN_CTRL); > +} > + > +static void sun8i_hdmi_phy_cec_pin_high(struct cec_adapter *adap) > +{ > + struct sun8i_hdmi_phy *phy = cec_get_drvdata(adap); > + > + /* > + * Stop driving the CEC pin, the pull up will take over > + * unless another CEC device is driving the pin low. > + */ > + regmap_write(phy->regs, SUN8I_HDMI_PHY_CEC_REG, > + SUN8I_HDMI_PHY_CEC_PIN_CTRL | > + SUN8I_HDMI_PHY_CEC_OUT_DIS); > +} > + > +static const struct cec_pin_ops sun8i_hdmi_phy_cec_pin_ops = { > + .read = sun8i_hdmi_phy_cec_pin_read, > + .low = sun8i_hdmi_phy_cec_pin_low, > + .high = sun8i_hdmi_phy_cec_pin_high, > +}; > +#endif > + > static struct regmap_config sun8i_hdmi_phy_regmap_config = { > .reg_bits = 32, > .val_bits = 32, > @@ -548,6 +590,7 @@ static const struct sun8i_hdmi_phy_variant sun8i_a83t_hdmi_phy = { > }; > > static const struct sun8i_hdmi_phy_variant sun8i_h3_hdmi_phy = { > + .bit_bang_cec = true, > .has_phy_clk = true, > .is_custom_phy = true, > .phy_init = &sun8i_hdmi_phy_init_h3, > @@ -556,6 +599,7 @@ static const struct sun8i_hdmi_phy_variant sun8i_h3_hdmi_phy = { > }; > > static const struct sun8i_hdmi_phy_variant sun8i_r40_hdmi_phy = { > + .bit_bang_cec = true, > .has_phy_clk = true, > .has_second_pll = true, > .is_custom_phy = true, > @@ -565,6 +609,7 @@ static const struct sun8i_hdmi_phy_variant sun8i_r40_hdmi_phy = { > }; > > static const struct sun8i_hdmi_phy_variant sun50i_a64_hdmi_phy = { > + .bit_bang_cec = true, > .has_phy_clk = true, > .is_custom_phy = true, > .phy_init = &sun8i_hdmi_phy_init_h3, > @@ -708,10 +753,40 @@ int sun8i_hdmi_phy_probe(struct sun8i_dw_hdmi *hdmi, struct device_node *node) > goto err_disable_clk_bus; > } > > +#ifdef CONFIG_DRM_SUN8I_DW_HDMI_CEC > + if (phy->variant->bit_bang_cec) { > + phy->cec_notifier = cec_notifier_get(dev); > + if (!phy->cec_notifier) { > + ret = -ENOMEM; > + goto err_disable_clk_mod; > + } > + > + phy->cec_adapter = > + cec_pin_allocate_adapter(&sun8i_hdmi_phy_cec_pin_ops, > + phy, "sun8i-cec", > + CEC_CAP_DEFAULTS); Please note that while this cec-pin implementation is quite accurate, you can make it miss messages. Specifically when transmitting or receiving broadcast messages since there is no proper acknowledge mechanism to verify that there were no timing errors due to a missed deadline of the high-res timer. That said, when I tested this originally on the Allwinner A10, it was very hard to make it fail. But it remains a second-choice implementation. But it does have an advantage in that it allows you to enable CEC error injection and test all sorts of nasty scenarios :-) I do think you should document somewhere why you had to choose this option for CEC instead of the DW solution. It's not obvious. Regards, Hans > + ret = PTR_ERR_OR_ZERO(phy->cec_adapter); > + if (ret < 0) > + goto err_put_cec_notifier; > + > + ret = cec_register_adapter(phy->cec_adapter, dev); > + if (ret < 0) > + goto err_delete_cec_adapter; > + > + cec_register_cec_notifier(phy->cec_adapter, phy->cec_notifier); > + } > +#endif > + > hdmi->phy = phy; > > return 0; > > +err_delete_cec_adapter: > + cec_delete_adapter(phy->cec_adapter); > +err_put_cec_notifier: > + cec_notifier_put(phy->cec_notifier); > +err_disable_clk_mod: > + clk_disable_unprepare(phy->clk_mod); > err_disable_clk_bus: > clk_disable_unprepare(phy->clk_bus); > err_deassert_rst_phy: > @@ -736,6 +811,10 @@ void sun8i_hdmi_phy_remove(struct sun8i_dw_hdmi *hdmi) > { > struct sun8i_hdmi_phy *phy = hdmi->phy; > > + cec_unregister_adapter(phy->cec_adapter); > + if (phy->cec_notifier) > + cec_notifier_put(phy->cec_notifier); > + > clk_disable_unprepare(phy->clk_mod); > clk_disable_unprepare(phy->clk_bus); > clk_disable_unprepare(phy->clk_phy); > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 0/2] drm/sun4i: dw-hdmi: Improve CEC support 2019-04-01 19:15 [PATCH v2 0/2] drm/sun4i: dw-hdmi: Improve CEC support Jernej Skrabec 2019-04-01 19:15 ` [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Add an option to suppress loading CEC driver Jernej Skrabec 2019-04-01 19:15 ` [PATCH v2 2/2] drm/sun4i: dw-hdmi: Bit bang CEC on some SoCs Jernej Skrabec @ 2019-04-12 10:15 ` Hans Verkuil 2 siblings, 0 replies; 5+ messages in thread From: Hans Verkuil @ 2019-04-12 10:15 UTC (permalink / raw) To: Jernej Skrabec, maxime.ripard Cc: narmstrong, airlied, linux-kernel, dri-devel, wens, Laurent.pinchart, linux-arm-kernel Hi Jernej, On 4/1/19 9:15 PM, Jernej Skrabec wrote: > It turns out that additional logic between HDMI CEC controller and > pins on PHY on some Allwinner SoCs prevents proper communication. > It might be possible to fix it, but it's much easier and less error > prone to just directly drive pins using software implementation of > CEC protocol. 1) Please CC linux-media as well, I had almost missed this series. 2) Is this a SoC limitation or a board limitation? 3) Is the bit-banging support sunxi specific? Or is it still using some DW CEC functionality? It appears to be sunxi specific. 4) I had an old patch series: https://www.spinics.net/lists/dri-devel/msg170232.html Rob Herring disagreed with that, but the discussion fizzled out at the end. I still think doing this in the device tree is the right approach since this is very much hardware related. If the DW CEC implementation doesn't work for some reason (CEC pins not hooked up, or for other reasons non-functional), then it should be disabled in the device tree. So then adding a new SUN8I option to enable bitbanging CEC support seems logical. Regards, Hans > > Let me know what do you think. > > Best regards, > Jernej > > Changes from v1: > - renamed is_cec_unusable to disable_cec > - added review-by tag > > Jernej Skrabec (2): > drm/bridge/synopsys: dw-hdmi: Add an option to suppress loading CEC > driver > drm/sun4i: dw-hdmi: Bit bang CEC on some SoCs > > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 2 +- > drivers/gpu/drm/sun4i/Kconfig | 10 +++ > drivers/gpu/drm/sun4i/sun8i_dw_hdmi.h | 11 +++ > drivers/gpu/drm/sun4i/sun8i_hdmi_phy.c | 83 ++++++++++++++++++++++- > include/drm/bridge/dw_hdmi.h | 2 + > 5 files changed, 105 insertions(+), 3 deletions(-) > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-04-12 10:24 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-04-01 19:15 [PATCH v2 0/2] drm/sun4i: dw-hdmi: Improve CEC support Jernej Skrabec 2019-04-01 19:15 ` [PATCH v2 1/2] drm/bridge/synopsys: dw-hdmi: Add an option to suppress loading CEC driver Jernej Skrabec 2019-04-01 19:15 ` [PATCH v2 2/2] drm/sun4i: dw-hdmi: Bit bang CEC on some SoCs Jernej Skrabec 2019-04-12 10:24 ` Hans Verkuil 2019-04-12 10:15 ` [PATCH v2 0/2] drm/sun4i: dw-hdmi: Improve CEC support Hans Verkuil
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).