| Message ID | E1dFi3P-0004lv-2K@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Russell, On 30-05-2017 15:23, Russell King wrote: > Add a CEC driver for the dw-hdmi hardware. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/gpu/drm/bridge/synopsys/Kconfig | 8 + > drivers/gpu/drm/bridge/synopsys/Makefile | 1 + > drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 320 ++++++++++++++++++++++++++ > drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h | 19 ++ > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 +++- > 5 files changed, 387 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > > diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig b/drivers/gpu/drm/bridge/synopsys/Kconfig > index 40d2827a6d19..bd30a0a07272 100644 > --- a/drivers/gpu/drm/bridge/synopsys/Kconfig > +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig > @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO > help > Support the I2S Audio interface which is part of the Synopsys > Designware HDMI block. > + > +config DRM_DW_HDMI_CEC > + tristate "Synopsis Designware CEC interface" > + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT > + select MEDIA_CEC_NOTIFIER > + help > + Support the CE interface which is part of the Synopsis Synopsis -> Synopsys > + Designware HDMI block. > diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile b/drivers/gpu/drm/bridge/synopsys/Makefile > index 17aa7a65b57e..6fe415903668 100644 > --- a/drivers/gpu/drm/bridge/synopsys/Makefile > +++ b/drivers/gpu/drm/bridge/synopsys/Makefile > @@ -3,3 +3,4 @@ > obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o > obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o > obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o > +obj-$(CONFIG_DRM_DW_HDMI_CEC) += dw-hdmi-cec.o > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > new file mode 100644 > index 000000000000..761ef5ae687d > --- /dev/null > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > @@ -0,0 +1,320 @@ > +/* > + * Designware HDMI CEC driver > + * > + * Copyright (C) 2015-2017 Russell King. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/sched.h> > +#include <linux/slab.h> > + > +#include <drm/drm_edid.h> > + > +#include <media/cec.h> > +#include <media/cec-notifier.h> > + > +#include "dw-hdmi-cec.h" > + > +enum { > + HDMI_IH_CEC_STAT0 = 0x0106, > + HDMI_IH_MUTE_CEC_STAT0 = 0x0186, > + > + HDMI_CEC_CTRL = 0x7d00, > + CEC_CTRL_START = BIT(0), > + CEC_CTRL_NORMAL = 1 << 1, > + > + HDMI_CEC_STAT = 0x7d01, > + CEC_STAT_DONE = BIT(0), > + CEC_STAT_EOM = BIT(1), > + CEC_STAT_NACK = BIT(2), > + CEC_STAT_ARBLOST = BIT(3), > + CEC_STAT_ERROR_INIT = BIT(4), > + CEC_STAT_ERROR_FOLL = BIT(5), > + CEC_STAT_WAKEUP = BIT(6), I rather like the format HDMI_{REGISTER_NAME}_{FIELD_NAME} so that it clearly identifies this is a internal driver register and not a CEC core register, but I guess its up to you :) > + > + HDMI_CEC_MASK = 0x7d02, > + HDMI_CEC_POLARITY = 0x7d03, > + HDMI_CEC_INT = 0x7d04, > + HDMI_CEC_ADDR_L = 0x7d05, > + HDMI_CEC_ADDR_H = 0x7d06, > + HDMI_CEC_TX_CNT = 0x7d07, > + HDMI_CEC_RX_CNT = 0x7d08, > + HDMI_CEC_TX_DATA0 = 0x7d10, > + HDMI_CEC_RX_DATA0 = 0x7d20, > + HDMI_CEC_LOCK = 0x7d30, > + HDMI_CEC_WKUPCTRL = 0x7d31, > +}; > + > +struct dw_hdmi_cec { > + struct dw_hdmi *hdmi; > + const struct dw_hdmi_cec_ops *ops; > + u32 addresses; > + struct cec_adapter *adap; > + struct cec_msg rx_msg; > + unsigned int tx_status; > + bool tx_done; > + bool rx_done; > + struct cec_notifier *notify; > + int retries; > + int irq; > +}; > + > +static void dw_hdmi_write(struct dw_hdmi_cec *cec, u8 val, int offset) > +{ > + cec->ops->write(cec->hdmi, val, offset); > +} > + > +static u8 dw_hdmi_read(struct dw_hdmi_cec *cec, int offset) > +{ > + return cec->ops->read(cec->hdmi, offset); > +} > + > +static int dw_hdmi_cec_log_addr(struct cec_adapter *adap, u8 logical_addr) > +{ > + struct dw_hdmi_cec *cec = adap->priv; You can use "cec = cec_get_drvdata(adap)" here, as in the rest of the code. > + u32 addresses; > + > + if (logical_addr == CEC_LOG_ADDR_INVALID) > + addresses = cec->addresses = 0; > + else > + addresses = cec->addresses |= BIT(logical_addr) | BIT(15); > + > + dw_hdmi_write(cec, addresses & 255, HDMI_CEC_ADDR_L); > + dw_hdmi_write(cec, addresses >> 8, HDMI_CEC_ADDR_H); > + > + return 0; > +} > + > +static int dw_hdmi_cec_transmit(struct cec_adapter *adap, u8 attempts, > + u32 signal_free_time, struct cec_msg *msg) > +{ > + struct dw_hdmi_cec *cec = adap->priv; > + unsigned i; > + > + cec->retries = attempts; I think you should check first if CEC engine is in normal operation mode, as a safety measure. > + > + for (i = 0; i < msg->len; i++) > + dw_hdmi_write(cec, msg->msg[i], HDMI_CEC_TX_DATA0 + i); > + > + dw_hdmi_write(cec, msg->len, HDMI_CEC_TX_CNT); > + dw_hdmi_write(cec, CEC_CTRL_NORMAL | CEC_CTRL_START, HDMI_CEC_CTRL); > + > + return 0; > +} > + > +static irqreturn_t dw_hdmi_cec_hardirq(int irq, void *data) > +{ > + struct cec_adapter *adap = data; > + struct dw_hdmi_cec *cec = adap->priv; > + unsigned stat = dw_hdmi_read(cec, HDMI_IH_CEC_STAT0); > + irqreturn_t ret = IRQ_HANDLED; > + > + if (stat == 0) > + return IRQ_NONE; > + > + dw_hdmi_write(cec, stat, HDMI_IH_CEC_STAT0); > + > + if (stat & CEC_STAT_ERROR_INIT) { > + if (cec->retries) { > + unsigned v = dw_hdmi_read(cec, HDMI_CEC_CTRL); > + dw_hdmi_write(cec, v | CEC_CTRL_START, HDMI_CEC_CTRL); > + cec->retries -= 1; > + } else { > + cec->tx_status = CEC_TX_STATUS_MAX_RETRIES; > + cec->tx_done = true; > + ret = IRQ_WAKE_THREAD; > + } > + } else if (stat & CEC_STAT_DONE) { > + cec->tx_status = CEC_TX_STATUS_OK; > + cec->tx_done = true; > + ret = IRQ_WAKE_THREAD; > + } else if (stat & CEC_STAT_NACK) { > + cec->tx_status = CEC_TX_STATUS_NACK; > + cec->tx_done = true; > + ret = IRQ_WAKE_THREAD; > + } > + > + if (stat & CEC_STAT_EOM) { > + unsigned len, i; > + > + len = dw_hdmi_read(cec, HDMI_CEC_RX_CNT); > + if (len > sizeof(cec->rx_msg.msg)) > + len = sizeof(cec->rx_msg.msg); > + > + for (i = 0; i < len; i++) > + cec->rx_msg.msg[i] = > + dw_hdmi_read(cec, HDMI_CEC_RX_DATA0 + i); > + > + dw_hdmi_write(cec, 0, HDMI_CEC_LOCK); > + > + cec->rx_msg.len = len; > + smp_wmb(); > + cec->rx_done = true; > + > + ret = IRQ_WAKE_THREAD; > + } > + > + return ret; > +} > + > +static irqreturn_t dw_hdmi_cec_thread(int irq, void *data) > +{ > + struct cec_adapter *adap = data; > + struct dw_hdmi_cec *cec = adap->priv; > + > + if (cec->tx_done) { > + cec->tx_done = false; > + cec_transmit_done(adap, cec->tx_status, 0, 0, 0, 0); > + } > + if (cec->rx_done) { > + cec->rx_done = false; > + smp_rmb(); > + cec_received_msg(adap, &cec->rx_msg); > + } > + return IRQ_HANDLED; > +} > + > +static int dw_hdmi_cec_enable(struct cec_adapter *adap, bool enable) > +{ > + struct dw_hdmi_cec *cec = adap->priv; > + > + if (!enable) { > + dw_hdmi_write(cec, ~0, HDMI_CEC_MASK); > + dw_hdmi_write(cec, ~0, HDMI_IH_MUTE_CEC_STAT0); > + dw_hdmi_write(cec, 0, HDMI_CEC_POLARITY); > + > + cec->ops->disable(cec->hdmi); > + } else { > + unsigned irqs; > + > + dw_hdmi_write(cec, 0, HDMI_CEC_CTRL); > + dw_hdmi_write(cec, ~0, HDMI_IH_CEC_STAT0); > + dw_hdmi_write(cec, 0, HDMI_CEC_LOCK); > + > + dw_hdmi_cec_log_addr(cec->adap, CEC_LOG_ADDR_INVALID); > + > + cec->ops->enable(cec->hdmi); > + > + irqs = CEC_STAT_ERROR_INIT | CEC_STAT_NACK | CEC_STAT_EOM | > + CEC_STAT_DONE; > + dw_hdmi_write(cec, irqs, HDMI_CEC_POLARITY); > + dw_hdmi_write(cec, ~irqs, HDMI_CEC_MASK); > + dw_hdmi_write(cec, ~irqs, HDMI_IH_MUTE_CEC_STAT0); > + } > + return 0; > +} > + > +static const struct cec_adap_ops dw_hdmi_cec_ops = { > + .adap_enable = dw_hdmi_cec_enable, > + .adap_log_addr = dw_hdmi_cec_log_addr, > + .adap_transmit = dw_hdmi_cec_transmit, > +}; > + > +static void dw_hdmi_cec_del(void *data) > +{ > + struct dw_hdmi_cec *cec = data; > + > + cec_delete_adapter(cec->adap); > +} > + > +static int dw_hdmi_cec_probe(struct platform_device *pdev) > +{ > + struct dw_hdmi_cec_data *data = dev_get_platdata(&pdev->dev); > + struct dw_hdmi_cec *cec; > + int ret; > + > + if (!data) > + return -ENXIO; > + > + /* > + * Our device is just a convenience - we want to link to the real > + * hardware device here, so that userspace can see the association > + * between the HDMI hardware and its associated CEC chardev. > + */ > + cec = devm_kzalloc(&pdev->dev, sizeof(*cec), GFP_KERNEL); > + if (!cec) > + return -ENOMEM; > + > + cec->irq = data->irq; > + cec->ops = data->ops; > + cec->hdmi = data->hdmi; > + > + platform_set_drvdata(pdev, cec); > + > + dw_hdmi_write(cec, 0, HDMI_CEC_TX_CNT); > + dw_hdmi_write(cec, ~0, HDMI_CEC_MASK); > + dw_hdmi_write(cec, ~0, HDMI_IH_MUTE_CEC_STAT0); > + dw_hdmi_write(cec, 0, HDMI_CEC_POLARITY); > + > + cec->adap = cec_allocate_adapter(&dw_hdmi_cec_ops, cec, "dw_hdmi", > + CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT | > + CEC_CAP_RC, CEC_MAX_LOG_ADDRS); > + if (IS_ERR(cec->adap)) > + return PTR_ERR(cec->adap); > + > + /* override the module pointer */ > + cec->adap->owner = THIS_MODULE; > + > + ret = devm_add_action(&pdev->dev, dw_hdmi_cec_del, cec); > + if (ret) { > + cec_delete_adapter(cec->adap); > + return ret; > + } > + > + ret = devm_request_threaded_irq(&pdev->dev, cec->irq, > + dw_hdmi_cec_hardirq, > + dw_hdmi_cec_thread, IRQF_SHARED, > + "dw-hdmi-cec", cec->adap); > + if (ret < 0) > + return ret; > + > + cec->notify = cec_notifier_get(pdev->dev.parent); > + if (!cec->notify) > + return -ENOMEM; > + > + ret = cec_register_adapter(cec->adap, pdev->dev.parent); > + if (ret < 0) { > + cec_notifier_put(cec->notify); > + return ret; > + } > + > + /* > + * CEC documentation says we must not call cec_delete_adapter > + * after a successful call to cec_register_adapter(). > + */ > + devm_remove_action(&pdev->dev, dw_hdmi_cec_del, cec); > + > + cec_register_cec_notifier(cec->adap, cec->notify); > + > + return 0; > +} > + > +static int dw_hdmi_cec_remove(struct platform_device *pdev) > +{ > + struct dw_hdmi_cec *cec = platform_get_drvdata(pdev); > + > + cec_unregister_adapter(cec->adap); > + cec_notifier_put(cec->notify); > + > + return 0; > +} > + > +static struct platform_driver dw_hdmi_cec_driver = { > + .probe = dw_hdmi_cec_probe, > + .remove = dw_hdmi_cec_remove, > + .driver = { > + .name = "dw-hdmi-cec", > + }, > +}; > +module_platform_driver(dw_hdmi_cec_driver); > + > +MODULE_AUTHOR("Russell King <rmk+kernel@armlinux.org.uk>"); > +MODULE_DESCRIPTION("Synopsis Designware HDMI CEC driver for i.MX"); Synopsis -> Synopsys > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS(PLATFORM_MODULE_PREFIX "dw-hdmi-cec"); > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > new file mode 100644 > index 000000000000..cf4dc121a2c4 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > @@ -0,0 +1,19 @@ > +#ifndef DW_HDMI_CEC_H > +#define DW_HDMI_CEC_H > + > +struct dw_hdmi; > + > +struct dw_hdmi_cec_ops { > + void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); > + u8 (*read)(struct dw_hdmi *hdmi, int offset); > + void (*enable)(struct dw_hdmi *hdmi); > + void (*disable)(struct dw_hdmi *hdmi); > +}; > + > +struct dw_hdmi_cec_data { > + struct dw_hdmi *hdmi; > + const struct dw_hdmi_cec_ops *ops; > + int irq; > +}; > + > +#endif > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 58781d4c1034..5a389f1f58d9 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -35,6 +35,7 @@ > > #include "dw-hdmi.h" > #include "dw-hdmi-audio.h" > +#include "dw-hdmi-cec.h" > > #include <media/cec-notifier.h> > > @@ -133,6 +134,7 @@ struct dw_hdmi { > unsigned int version; > > struct platform_device *audio; > + struct platform_device *cec; > struct device *dev; > struct clk *isfr_clk; > struct clk *iahb_clk; > @@ -1766,7 +1768,6 @@ static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi) > hdmi_writeb(hdmi, 0xff, HDMI_AUD_HBR_MASK); > hdmi_writeb(hdmi, 0xff, HDMI_GP_MASK); > hdmi_writeb(hdmi, 0xff, HDMI_A_APIINTMSK); > - hdmi_writeb(hdmi, 0xff, HDMI_CEC_MASK); > hdmi_writeb(hdmi, 0xff, HDMI_I2CM_INT); > hdmi_writeb(hdmi, 0xff, HDMI_I2CM_CTLINT); > > @@ -2223,6 +2224,29 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > return -ENODEV; > } > > +static void dw_hdmi_cec_enable(struct dw_hdmi *hdmi) > +{ > + mutex_lock(&hdmi->mutex); > + hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CECCLK_DISABLE; > + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); > + mutex_unlock(&hdmi->mutex); > +} > + > +static void dw_hdmi_cec_disable(struct dw_hdmi *hdmi) > +{ > + mutex_lock(&hdmi->mutex); > + hdmi->mc_clkdis |= HDMI_MC_CLKDIS_CECCLK_DISABLE; > + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); > + mutex_unlock(&hdmi->mutex); > +} > + > +static const struct dw_hdmi_cec_ops dw_hdmi_cec_ops = { > + .write = hdmi_writeb, > + .read = hdmi_readb, > + .enable = dw_hdmi_cec_enable, > + .disable = dw_hdmi_cec_disable, > +}; > + > static const struct regmap_config hdmi_regmap_8bit_config = { > .reg_bits = 32, > .val_bits = 8, > @@ -2245,6 +2269,7 @@ __dw_hdmi_probe(struct platform_device *pdev, > struct device_node *np = dev->of_node; > struct platform_device_info pdevinfo; > struct device_node *ddc_node; > + struct dw_hdmi_cec_data cec; > struct dw_hdmi *hdmi; > struct resource *iores = NULL; > int irq; > @@ -2445,6 +2470,17 @@ __dw_hdmi_probe(struct platform_device *pdev, > hdmi->audio = platform_device_register_full(&pdevinfo); > } > > + cec.hdmi = hdmi; > + cec.ops = &dw_hdmi_cec_ops; > + cec.irq = irq; > + > + pdevinfo.name = "dw-hdmi-cec"; > + pdevinfo.data = &cec; > + pdevinfo.size_data = sizeof(cec); > + pdevinfo.dma_mask = 0; > + > + hdmi->cec = platform_device_register_full(&pdevinfo); > + > /* Reset HDMI DDC I2C master controller and mute I2CM interrupts */ > if (hdmi->i2c) > dw_hdmi_i2c_init(hdmi); > @@ -2475,6 +2511,8 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) > { > if (hdmi->audio && !IS_ERR(hdmi->audio)) > platform_device_unregister(hdmi->audio); > + if (!IS_ERR(hdmi->cec)) > + platform_device_unregister(hdmi->cec); > > /* Disable all interrupts */ > hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0); Best regards, Jose Miguel Abreu
Hi Russell, First a few top-level questions: 1) What was the reason for using the cec-notifier here? Isn't this tightly integrated into the main dw-hdmi block? For the tda driver it is clearly required, but for tightly coupled HDMI & CEC HW I just create the adapter from the HDMI driver. As a small bonus it avoids adding the cec-notifier code and the control flow is a bit easier to trace. 2) I may have asked this before, apologies if I repeat myself: does this CEC implementation support CEC monitoring (aka snooping)? If it does, then I recommend that it is implemented since it is very useful. 3) Is the CEC still active if there is no hotplug signal? Or is it powered off in that case? Ideally it should still be possible to send CEC messages even if there is no hotplug. This is explicitly allowed by the CEC 2.0 spec to wake up displays that turn off the HPD, but that still have a working CEC controller. If this is not possible, then you need to use the CEC_CAP_NEEDS_HPD capability. See: https://patchwork.linuxtv.org/patch/41478/ This will almost certainly be merged for 4.13 since other CEC drivers need this as well. On 05/30/17 16:23, Russell King wrote: > Add a CEC driver for the dw-hdmi hardware. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/gpu/drm/bridge/synopsys/Kconfig | 8 + > drivers/gpu/drm/bridge/synopsys/Makefile | 1 + > drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 320 ++++++++++++++++++++++++++ > drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h | 19 ++ > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 +++- > 5 files changed, 387 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > > diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig b/drivers/gpu/drm/bridge/synopsys/Kconfig > index 40d2827a6d19..bd30a0a07272 100644 > --- a/drivers/gpu/drm/bridge/synopsys/Kconfig > +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig > @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO > help > Support the I2S Audio interface which is part of the Synopsys > Designware HDMI block. > + > +config DRM_DW_HDMI_CEC > + tristate "Synopsis Designware CEC interface" > + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT > + select MEDIA_CEC_NOTIFIER > + help > + Support the CE interface which is part of the Synopsis > + Designware HDMI block. This will change. Patches to fix the config handling are pending for 4.12. Here you can see the pending patches: https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=drm-cec The patches from 'cec-notifier.h: handle unreachable CONFIG_CEC_CORE' to 'cec: drop MEDIA_CEC_DEBUG' should all be merged in 4.12. That means that this config becomes: + +config DRM_DW_HDMI_CEC + tristate "Synopsis Designware CEC interface" + depends on DRM_DW_HDMI + select CEC_CORE + select CEC_NOTIFIER + help + Support the CE interface which is part of the Synopsis + Designware HDMI block. > diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile b/drivers/gpu/drm/bridge/synopsys/Makefile > index 17aa7a65b57e..6fe415903668 100644 > --- a/drivers/gpu/drm/bridge/synopsys/Makefile > +++ b/drivers/gpu/drm/bridge/synopsys/Makefile > @@ -3,3 +3,4 @@ > obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o > obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o > obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o > +obj-$(CONFIG_DRM_DW_HDMI_CEC) += dw-hdmi-cec.o > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > new file mode 100644 > index 000000000000..761ef5ae687d > --- /dev/null > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > @@ -0,0 +1,320 @@ > +/* > + * Designware HDMI CEC driver > + * > + * Copyright (C) 2015-2017 Russell King. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/sched.h> > +#include <linux/slab.h> > + > +#include <drm/drm_edid.h> > + > +#include <media/cec.h> > +#include <media/cec-notifier.h> > + > +#include "dw-hdmi-cec.h" > + > +enum { > + HDMI_IH_CEC_STAT0 = 0x0106, > + HDMI_IH_MUTE_CEC_STAT0 = 0x0186, > + > + HDMI_CEC_CTRL = 0x7d00, > + CEC_CTRL_START = BIT(0), > + CEC_CTRL_NORMAL = 1 << 1, > + > + HDMI_CEC_STAT = 0x7d01, > + CEC_STAT_DONE = BIT(0), > + CEC_STAT_EOM = BIT(1), > + CEC_STAT_NACK = BIT(2), > + CEC_STAT_ARBLOST = BIT(3), > + CEC_STAT_ERROR_INIT = BIT(4), > + CEC_STAT_ERROR_FOLL = BIT(5), > + CEC_STAT_WAKEUP = BIT(6), > + > + HDMI_CEC_MASK = 0x7d02, > + HDMI_CEC_POLARITY = 0x7d03, > + HDMI_CEC_INT = 0x7d04, > + HDMI_CEC_ADDR_L = 0x7d05, > + HDMI_CEC_ADDR_H = 0x7d06, > + HDMI_CEC_TX_CNT = 0x7d07, > + HDMI_CEC_RX_CNT = 0x7d08, > + HDMI_CEC_TX_DATA0 = 0x7d10, > + HDMI_CEC_RX_DATA0 = 0x7d20, > + HDMI_CEC_LOCK = 0x7d30, > + HDMI_CEC_WKUPCTRL = 0x7d31, > +}; > + > +struct dw_hdmi_cec { > + struct dw_hdmi *hdmi; > + const struct dw_hdmi_cec_ops *ops; > + u32 addresses; > + struct cec_adapter *adap; > + struct cec_msg rx_msg; > + unsigned int tx_status; > + bool tx_done; > + bool rx_done; > + struct cec_notifier *notify; > + int retries; > + int irq; > +}; > + > +static void dw_hdmi_write(struct dw_hdmi_cec *cec, u8 val, int offset) > +{ > + cec->ops->write(cec->hdmi, val, offset); > +} > + > +static u8 dw_hdmi_read(struct dw_hdmi_cec *cec, int offset) > +{ > + return cec->ops->read(cec->hdmi, offset); > +} > + > +static int dw_hdmi_cec_log_addr(struct cec_adapter *adap, u8 logical_addr) > +{ > + struct dw_hdmi_cec *cec = adap->priv; > + u32 addresses; > + > + if (logical_addr == CEC_LOG_ADDR_INVALID) > + addresses = cec->addresses = 0; > + else > + addresses = cec->addresses |= BIT(logical_addr) | BIT(15); > + > + dw_hdmi_write(cec, addresses & 255, HDMI_CEC_ADDR_L); > + dw_hdmi_write(cec, addresses >> 8, HDMI_CEC_ADDR_H); The addresses local variable doesn't really seem needed. > + > + return 0; > +} > + > +static int dw_hdmi_cec_transmit(struct cec_adapter *adap, u8 attempts, > + u32 signal_free_time, struct cec_msg *msg) > +{ > + struct dw_hdmi_cec *cec = adap->priv; > + unsigned i; > + > + cec->retries = attempts; Should probably be 'cec->retries = attempts - 1;' Since 2 attempts == 1 retry. > + > + for (i = 0; i < msg->len; i++) > + dw_hdmi_write(cec, msg->msg[i], HDMI_CEC_TX_DATA0 + i); > + > + dw_hdmi_write(cec, msg->len, HDMI_CEC_TX_CNT); > + dw_hdmi_write(cec, CEC_CTRL_NORMAL | CEC_CTRL_START, HDMI_CEC_CTRL); > + > + return 0; > +} > + > +static irqreturn_t dw_hdmi_cec_hardirq(int irq, void *data) > +{ > + struct cec_adapter *adap = data; > + struct dw_hdmi_cec *cec = adap->priv; > + unsigned stat = dw_hdmi_read(cec, HDMI_IH_CEC_STAT0); > + irqreturn_t ret = IRQ_HANDLED; > + > + if (stat == 0) > + return IRQ_NONE; > + > + dw_hdmi_write(cec, stat, HDMI_IH_CEC_STAT0); > + > + if (stat & CEC_STAT_ERROR_INIT) { > + if (cec->retries) { > + unsigned v = dw_hdmi_read(cec, HDMI_CEC_CTRL); > + dw_hdmi_write(cec, v | CEC_CTRL_START, HDMI_CEC_CTRL); > + cec->retries -= 1; Why handle retries here at all? Just mark this as CEC_TX_STATUS_ERROR and set tx_done to true. The CEC core will just call dw_hdmi_cec_transmit again if it needs to make another attempt. I suggest that the whole retry code is dropped. > + } else { > + cec->tx_status = CEC_TX_STATUS_MAX_RETRIES; > + cec->tx_done = true; > + ret = IRQ_WAKE_THREAD; > + } > + } else if (stat & CEC_STAT_DONE) { > + cec->tx_status = CEC_TX_STATUS_OK; > + cec->tx_done = true; > + ret = IRQ_WAKE_THREAD; > + } else if (stat & CEC_STAT_NACK) { > + cec->tx_status = CEC_TX_STATUS_NACK; > + cec->tx_done = true; > + ret = IRQ_WAKE_THREAD; > + } > + > + if (stat & CEC_STAT_EOM) { > + unsigned len, i; > + > + len = dw_hdmi_read(cec, HDMI_CEC_RX_CNT); > + if (len > sizeof(cec->rx_msg.msg)) > + len = sizeof(cec->rx_msg.msg); > + > + for (i = 0; i < len; i++) > + cec->rx_msg.msg[i] = > + dw_hdmi_read(cec, HDMI_CEC_RX_DATA0 + i); > + > + dw_hdmi_write(cec, 0, HDMI_CEC_LOCK); > + > + cec->rx_msg.len = len; > + smp_wmb(); > + cec->rx_done = true; > + > + ret = IRQ_WAKE_THREAD; > + } > + > + return ret; > +} > + > +static irqreturn_t dw_hdmi_cec_thread(int irq, void *data) > +{ > + struct cec_adapter *adap = data; > + struct dw_hdmi_cec *cec = adap->priv; > + > + if (cec->tx_done) { > + cec->tx_done = false; > + cec_transmit_done(adap, cec->tx_status, 0, 0, 0, 0); I think I will make a patch for a new helper function where you can just call: cec_transmit_attempt_done(adap, cec->tx_status); and that will call cec_transmit_done filling in the other arguments based on the status. Most CEC HW doesn't do retries so only make one attempt. In that case only one of the remaining count arguments is 1 and that's based on the status. Having correct error counts is useful for debugging problems. > + } > + if (cec->rx_done) { > + cec->rx_done = false; > + smp_rmb(); > + cec_received_msg(adap, &cec->rx_msg); > + } > + return IRQ_HANDLED; > +} > + > +static int dw_hdmi_cec_enable(struct cec_adapter *adap, bool enable) > +{ > + struct dw_hdmi_cec *cec = adap->priv; > + > + if (!enable) { > + dw_hdmi_write(cec, ~0, HDMI_CEC_MASK); > + dw_hdmi_write(cec, ~0, HDMI_IH_MUTE_CEC_STAT0); > + dw_hdmi_write(cec, 0, HDMI_CEC_POLARITY); > + > + cec->ops->disable(cec->hdmi); > + } else { > + unsigned irqs; > + > + dw_hdmi_write(cec, 0, HDMI_CEC_CTRL); > + dw_hdmi_write(cec, ~0, HDMI_IH_CEC_STAT0); > + dw_hdmi_write(cec, 0, HDMI_CEC_LOCK); > + > + dw_hdmi_cec_log_addr(cec->adap, CEC_LOG_ADDR_INVALID); > + > + cec->ops->enable(cec->hdmi); > + > + irqs = CEC_STAT_ERROR_INIT | CEC_STAT_NACK | CEC_STAT_EOM | > + CEC_STAT_DONE; > + dw_hdmi_write(cec, irqs, HDMI_CEC_POLARITY); > + dw_hdmi_write(cec, ~irqs, HDMI_CEC_MASK); > + dw_hdmi_write(cec, ~irqs, HDMI_IH_MUTE_CEC_STAT0); > + } > + return 0; > +} > + > +static const struct cec_adap_ops dw_hdmi_cec_ops = { > + .adap_enable = dw_hdmi_cec_enable, > + .adap_log_addr = dw_hdmi_cec_log_addr, > + .adap_transmit = dw_hdmi_cec_transmit, > +}; > + > +static void dw_hdmi_cec_del(void *data) > +{ > + struct dw_hdmi_cec *cec = data; > + > + cec_delete_adapter(cec->adap); > +} > + > +static int dw_hdmi_cec_probe(struct platform_device *pdev) > +{ > + struct dw_hdmi_cec_data *data = dev_get_platdata(&pdev->dev); > + struct dw_hdmi_cec *cec; > + int ret; > + > + if (!data) > + return -ENXIO; > + > + /* > + * Our device is just a convenience - we want to link to the real > + * hardware device here, so that userspace can see the association > + * between the HDMI hardware and its associated CEC chardev. > + */ > + cec = devm_kzalloc(&pdev->dev, sizeof(*cec), GFP_KERNEL); > + if (!cec) > + return -ENOMEM; > + > + cec->irq = data->irq; > + cec->ops = data->ops; > + cec->hdmi = data->hdmi; > + > + platform_set_drvdata(pdev, cec); > + > + dw_hdmi_write(cec, 0, HDMI_CEC_TX_CNT); > + dw_hdmi_write(cec, ~0, HDMI_CEC_MASK); > + dw_hdmi_write(cec, ~0, HDMI_IH_MUTE_CEC_STAT0); > + dw_hdmi_write(cec, 0, HDMI_CEC_POLARITY); > + > + cec->adap = cec_allocate_adapter(&dw_hdmi_cec_ops, cec, "dw_hdmi", > + CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT | > + CEC_CAP_RC, CEC_MAX_LOG_ADDRS); > + if (IS_ERR(cec->adap)) > + return PTR_ERR(cec->adap); > + > + /* override the module pointer */ > + cec->adap->owner = THIS_MODULE; > + > + ret = devm_add_action(&pdev->dev, dw_hdmi_cec_del, cec); > + if (ret) { > + cec_delete_adapter(cec->adap); > + return ret; > + } > + > + ret = devm_request_threaded_irq(&pdev->dev, cec->irq, > + dw_hdmi_cec_hardirq, > + dw_hdmi_cec_thread, IRQF_SHARED, > + "dw-hdmi-cec", cec->adap); > + if (ret < 0) > + return ret; > + > + cec->notify = cec_notifier_get(pdev->dev.parent); > + if (!cec->notify) > + return -ENOMEM; > + > + ret = cec_register_adapter(cec->adap, pdev->dev.parent); > + if (ret < 0) { > + cec_notifier_put(cec->notify); > + return ret; > + } > + > + /* > + * CEC documentation says we must not call cec_delete_adapter > + * after a successful call to cec_register_adapter(). > + */ > + devm_remove_action(&pdev->dev, dw_hdmi_cec_del, cec); > + > + cec_register_cec_notifier(cec->adap, cec->notify); > + > + return 0; > +} > + > +static int dw_hdmi_cec_remove(struct platform_device *pdev) > +{ > + struct dw_hdmi_cec *cec = platform_get_drvdata(pdev); > + > + cec_unregister_adapter(cec->adap); > + cec_notifier_put(cec->notify); > + > + return 0; > +} > + > +static struct platform_driver dw_hdmi_cec_driver = { > + .probe = dw_hdmi_cec_probe, > + .remove = dw_hdmi_cec_remove, > + .driver = { > + .name = "dw-hdmi-cec", > + }, > +}; > +module_platform_driver(dw_hdmi_cec_driver); > + > +MODULE_AUTHOR("Russell King <rmk+kernel@armlinux.org.uk>"); > +MODULE_DESCRIPTION("Synopsis Designware HDMI CEC driver for i.MX"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS(PLATFORM_MODULE_PREFIX "dw-hdmi-cec"); > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > new file mode 100644 > index 000000000000..cf4dc121a2c4 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > @@ -0,0 +1,19 @@ > +#ifndef DW_HDMI_CEC_H > +#define DW_HDMI_CEC_H > + > +struct dw_hdmi; > + > +struct dw_hdmi_cec_ops { > + void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); > + u8 (*read)(struct dw_hdmi *hdmi, int offset); > + void (*enable)(struct dw_hdmi *hdmi); > + void (*disable)(struct dw_hdmi *hdmi); > +}; > + > +struct dw_hdmi_cec_data { > + struct dw_hdmi *hdmi; > + const struct dw_hdmi_cec_ops *ops; > + int irq; > +}; > + > +#endif > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 58781d4c1034..5a389f1f58d9 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -35,6 +35,7 @@ > > #include "dw-hdmi.h" > #include "dw-hdmi-audio.h" > +#include "dw-hdmi-cec.h" > > #include <media/cec-notifier.h> > > @@ -133,6 +134,7 @@ struct dw_hdmi { > unsigned int version; > > struct platform_device *audio; > + struct platform_device *cec; > struct device *dev; > struct clk *isfr_clk; > struct clk *iahb_clk; > @@ -1766,7 +1768,6 @@ static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi) > hdmi_writeb(hdmi, 0xff, HDMI_AUD_HBR_MASK); > hdmi_writeb(hdmi, 0xff, HDMI_GP_MASK); > hdmi_writeb(hdmi, 0xff, HDMI_A_APIINTMSK); > - hdmi_writeb(hdmi, 0xff, HDMI_CEC_MASK); > hdmi_writeb(hdmi, 0xff, HDMI_I2CM_INT); > hdmi_writeb(hdmi, 0xff, HDMI_I2CM_CTLINT); > > @@ -2223,6 +2224,29 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > return -ENODEV; > } > > +static void dw_hdmi_cec_enable(struct dw_hdmi *hdmi) > +{ > + mutex_lock(&hdmi->mutex); > + hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CECCLK_DISABLE; > + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); > + mutex_unlock(&hdmi->mutex); > +} > + > +static void dw_hdmi_cec_disable(struct dw_hdmi *hdmi) > +{ > + mutex_lock(&hdmi->mutex); > + hdmi->mc_clkdis |= HDMI_MC_CLKDIS_CECCLK_DISABLE; > + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); > + mutex_unlock(&hdmi->mutex); > +} > + > +static const struct dw_hdmi_cec_ops dw_hdmi_cec_ops = { > + .write = hdmi_writeb, > + .read = hdmi_readb, > + .enable = dw_hdmi_cec_enable, > + .disable = dw_hdmi_cec_disable, > +}; > + This feels over-engineered and I suspect it is simplified if you just add a cec_adapter pointer to struct dw_hdmi and link dw-hdmi-cec.c together with dw-hdmi.c. In other implementations I have done (see e.g. the recent drm displayport CEC patch series) this works quite well. > static const struct regmap_config hdmi_regmap_8bit_config = { > .reg_bits = 32, > .val_bits = 8, > @@ -2245,6 +2269,7 @@ __dw_hdmi_probe(struct platform_device *pdev, > struct device_node *np = dev->of_node; > struct platform_device_info pdevinfo; > struct device_node *ddc_node; > + struct dw_hdmi_cec_data cec; > struct dw_hdmi *hdmi; > struct resource *iores = NULL; > int irq; > @@ -2445,6 +2470,17 @@ __dw_hdmi_probe(struct platform_device *pdev, > hdmi->audio = platform_device_register_full(&pdevinfo); > } > > + cec.hdmi = hdmi; > + cec.ops = &dw_hdmi_cec_ops; > + cec.irq = irq; > + > + pdevinfo.name = "dw-hdmi-cec"; > + pdevinfo.data = &cec; > + pdevinfo.size_data = sizeof(cec); > + pdevinfo.dma_mask = 0; > + > + hdmi->cec = platform_device_register_full(&pdevinfo); > + > /* Reset HDMI DDC I2C master controller and mute I2CM interrupts */ > if (hdmi->i2c) > dw_hdmi_i2c_init(hdmi); > @@ -2475,6 +2511,8 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) > { > if (hdmi->audio && !IS_ERR(hdmi->audio)) > platform_device_unregister(hdmi->audio); > + if (!IS_ERR(hdmi->cec)) > + platform_device_unregister(hdmi->cec); > > /* Disable all interrupts */ > hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0); > Regards, Hans
On Thu, Jun 01, 2017 at 10:31:10AM +0200, Hans Verkuil wrote: > Hi Russell, > > First a few top-level questions: Hi Hans, > 1) What was the reason for using the cec-notifier here? Isn't this > tightly integrated into the main dw-hdmi block? For the tda driver > it is clearly required, but for tightly coupled HDMI & CEC HW I > just create the adapter from the HDMI driver. As a small bonus it > avoids adding the cec-notifier code and the control flow is a bit > easier to trace. It's to avoid complex dependencies. If it's all built in to the HDMI driver, then the HDMI driver needs to depend on all the media stuff being non-modular. For a video output device, this is sub-optimal, because you want the video output device to work during boot. I feel strongly about this point, especially as we have had many years of users being able to use dw-hdmi without needing CEC enabled. > 2) I may have asked this before, apologies if I repeat myself: does > this CEC implementation support CEC monitoring (aka snooping)? If > it does, then I recommend that it is implemented since it is very > useful. It does not. > 3) Is the CEC still active if there is no hotplug signal? Or is it > powered off in that case? Ideally it should still be possible to > send CEC messages even if there is no hotplug. This is explicitly > allowed by the CEC 2.0 spec to wake up displays that turn off the > HPD, but that still have a working CEC controller. This is not specified in the documentation, so I don't think we know without experimentation. This would be a future enhancement to the patch set. > > diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig b/drivers/gpu/drm/bridge/synopsys/Kconfig > > index 40d2827a6d19..bd30a0a07272 100644 > > --- a/drivers/gpu/drm/bridge/synopsys/Kconfig > > +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig > > @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO > > help > > Support the I2S Audio interface which is part of the Synopsys > > Designware HDMI block. > > + > > +config DRM_DW_HDMI_CEC > > + tristate "Synopsis Designware CEC interface" > > + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT > > + select MEDIA_CEC_NOTIFIER > > + help > > + Support the CE interface which is part of the Synopsis > > + Designware HDMI block. > > This will change. Patches to fix the config handling are pending for 4.12. > > Here you can see the pending patches: > https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=drm-cec > > The patches from 'cec-notifier.h: handle unreachable CONFIG_CEC_CORE' to > 'cec: drop MEDIA_CEC_DEBUG' should all be merged in 4.12. > > That means that this config becomes: > > + > +config DRM_DW_HDMI_CEC > + tristate "Synopsis Designware CEC interface" > + depends on DRM_DW_HDMI > + select CEC_CORE > + select CEC_NOTIFIER > + help > + Support the CE interface which is part of the Synopsis > + Designware HDMI block. You will also need to select MEDIA_SUPPORT to avoid dependency issues. CEC_CORE and CEC_NOTIFIER are both lumped under an "if MEDIA_SUPPORT" block in drivers/media/Kconfig, so they depend on this symbol. Asking Kconfig to select these two symbols without MEDIA_SUPPORT creates an invalid configuration - unless CEC has been moved out from being under MEDIA_SUPPORT. (I haven't looked at your tree yet.) > > +static int dw_hdmi_cec_log_addr(struct cec_adapter *adap, u8 logical_addr) > > +{ > > + struct dw_hdmi_cec *cec = adap->priv; > > + u32 addresses; > > + > > + if (logical_addr == CEC_LOG_ADDR_INVALID) > > + addresses = cec->addresses = 0; > > + else > > + addresses = cec->addresses |= BIT(logical_addr) | BIT(15); > > + > > + dw_hdmi_write(cec, addresses & 255, HDMI_CEC_ADDR_L); > > + dw_hdmi_write(cec, addresses >> 8, HDMI_CEC_ADDR_H); > > The addresses local variable doesn't really seem needed. Ok. > > +static int dw_hdmi_cec_transmit(struct cec_adapter *adap, u8 attempts, > > + u32 signal_free_time, struct cec_msg *msg) > > +{ > > + struct dw_hdmi_cec *cec = adap->priv; > > + unsigned i; > > + > > + cec->retries = attempts; > > Should probably be 'cec->retries = attempts - 1;' > Since 2 attempts == 1 retry. Probably correct, but rather moot given your comment below. > > +static irqreturn_t dw_hdmi_cec_hardirq(int irq, void *data) > > +{ > > + struct cec_adapter *adap = data; > > + struct dw_hdmi_cec *cec = adap->priv; > > + unsigned stat = dw_hdmi_read(cec, HDMI_IH_CEC_STAT0); > > + irqreturn_t ret = IRQ_HANDLED; > > + > > + if (stat == 0) > > + return IRQ_NONE; > > + > > + dw_hdmi_write(cec, stat, HDMI_IH_CEC_STAT0); > > + > > + if (stat & CEC_STAT_ERROR_INIT) { > > + if (cec->retries) { > > + unsigned v = dw_hdmi_read(cec, HDMI_CEC_CTRL); > > + dw_hdmi_write(cec, v | CEC_CTRL_START, HDMI_CEC_CTRL); > > + cec->retries -= 1; > > Why handle retries here at all? Just mark this as CEC_TX_STATUS_ERROR and set > tx_done to true. The CEC core will just call dw_hdmi_cec_transmit again if > it needs to make another attempt. I suggest that the whole retry code is > dropped. Probably comes from a previous iteration of your CEC core which required the driver to do retries, so it can probably be fixed up in the way you describe. However, presumably this needs a different status other than "CEC_TX_STATUS_MAX_RETRIES"? I don't see a way to report this error to the CEC core in a way that it will retry. > > +static irqreturn_t dw_hdmi_cec_thread(int irq, void *data) > > +{ > > + struct cec_adapter *adap = data; > > + struct dw_hdmi_cec *cec = adap->priv; > > + > > + if (cec->tx_done) { > > + cec->tx_done = false; > > + cec_transmit_done(adap, cec->tx_status, 0, 0, 0, 0); > > I think I will make a patch for a new helper function where you can just call: > > cec_transmit_attempt_done(adap, cec->tx_status); > > and that will call cec_transmit_done filling in the other arguments > based on the status. > > Most CEC HW doesn't do retries so only make one attempt. In that case > only one of the remaining count arguments is 1 and that's based on the > status. > > Having correct error counts is useful for debugging problems. It hasn't been clear to me which counts should be non-zero, and it seems like entirely redundant information given that the status argument indicates what happened to the message. > > +static void dw_hdmi_cec_disable(struct dw_hdmi *hdmi) > > +{ > > + mutex_lock(&hdmi->mutex); > > + hdmi->mc_clkdis |= HDMI_MC_CLKDIS_CECCLK_DISABLE; > > + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); > > + mutex_unlock(&hdmi->mutex); > > +} > > + > > +static const struct dw_hdmi_cec_ops dw_hdmi_cec_ops = { > > + .write = hdmi_writeb, > > + .read = hdmi_readb, > > + .enable = dw_hdmi_cec_enable, > > + .disable = dw_hdmi_cec_disable, > > +}; > > + > > This feels over-engineered and I suspect it is simplified if you just > add a cec_adapter pointer to struct dw_hdmi and link dw-hdmi-cec.c > together with dw-hdmi.c. As I mention above, to do so would mean that dw-hdmi has to be modular if you want CEC to be modular, and I don't think that is reasonable for a video output device, which may be the source of kernel boot messages.
On 06/01/17 11:46, Russell King - ARM Linux wrote: > On Thu, Jun 01, 2017 at 10:31:10AM +0200, Hans Verkuil wrote: >> Hi Russell, >> >> First a few top-level questions: > > Hi Hans, > >> 1) What was the reason for using the cec-notifier here? Isn't this >> tightly integrated into the main dw-hdmi block? For the tda driver >> it is clearly required, but for tightly coupled HDMI & CEC HW I >> just create the adapter from the HDMI driver. As a small bonus it >> avoids adding the cec-notifier code and the control flow is a bit >> easier to trace. > > It's to avoid complex dependencies. If it's all built in to the HDMI > driver, then the HDMI driver needs to depend on all the media stuff > being non-modular. For a video output device, this is sub-optimal, > because you want the video output device to work during boot. This is no longer true after my rework of the CEC kernel config. After doing a 'select CEC_CORE' the cec framework will be compiled correctly. It no longer depends on the media subsystem (except if you want to use the remote control passthrough since the RC framework is part of media). Selecting CEC_CORE will only select the cec core code. > > I feel strongly about this point, especially as we have had many years > of users being able to use dw-hdmi without needing CEC enabled. > >> 2) I may have asked this before, apologies if I repeat myself: does >> this CEC implementation support CEC monitoring (aka snooping)? If >> it does, then I recommend that it is implemented since it is very >> useful. > > It does not. > >> 3) Is the CEC still active if there is no hotplug signal? Or is it >> powered off in that case? Ideally it should still be possible to >> send CEC messages even if there is no hotplug. This is explicitly >> allowed by the CEC 2.0 spec to wake up displays that turn off the >> HPD, but that still have a working CEC controller. > > This is not specified in the documentation, so I don't think we know > without experimentation. This would be a future enhancement to the > patch set. > >>> diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig b/drivers/gpu/drm/bridge/synopsys/Kconfig >>> index 40d2827a6d19..bd30a0a07272 100644 >>> --- a/drivers/gpu/drm/bridge/synopsys/Kconfig >>> +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig >>> @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO >>> help >>> Support the I2S Audio interface which is part of the Synopsys >>> Designware HDMI block. >>> + >>> +config DRM_DW_HDMI_CEC >>> + tristate "Synopsis Designware CEC interface" >>> + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT >>> + select MEDIA_CEC_NOTIFIER >>> + help >>> + Support the CE interface which is part of the Synopsis >>> + Designware HDMI block. >> >> This will change. Patches to fix the config handling are pending for 4.12. >> >> Here you can see the pending patches: >> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=drm-cec >> >> The patches from 'cec-notifier.h: handle unreachable CONFIG_CEC_CORE' to >> 'cec: drop MEDIA_CEC_DEBUG' should all be merged in 4.12. >> >> That means that this config becomes: >> >> + >> +config DRM_DW_HDMI_CEC >> + tristate "Synopsis Designware CEC interface" >> + depends on DRM_DW_HDMI >> + select CEC_CORE >> + select CEC_NOTIFIER >> + help >> + Support the CE interface which is part of the Synopsis >> + Designware HDMI block. > > You will also need to select MEDIA_SUPPORT to avoid dependency issues. > CEC_CORE and CEC_NOTIFIER are both lumped under an "if MEDIA_SUPPORT" > block in drivers/media/Kconfig, so they depend on this symbol. No, they've been moved up. They no longer depend on the MEDIA_SUPPORT. So that's been fixed. > Asking > Kconfig to select these two symbols without MEDIA_SUPPORT creates an > invalid configuration - unless CEC has been moved out from being under > MEDIA_SUPPORT. (I haven't looked at your tree yet.) > >>> +static int dw_hdmi_cec_log_addr(struct cec_adapter *adap, u8 logical_addr) >>> +{ >>> + struct dw_hdmi_cec *cec = adap->priv; >>> + u32 addresses; >>> + >>> + if (logical_addr == CEC_LOG_ADDR_INVALID) >>> + addresses = cec->addresses = 0; >>> + else >>> + addresses = cec->addresses |= BIT(logical_addr) | BIT(15); >>> + >>> + dw_hdmi_write(cec, addresses & 255, HDMI_CEC_ADDR_L); >>> + dw_hdmi_write(cec, addresses >> 8, HDMI_CEC_ADDR_H); >> >> The addresses local variable doesn't really seem needed. > > Ok. > >>> +static int dw_hdmi_cec_transmit(struct cec_adapter *adap, u8 attempts, >>> + u32 signal_free_time, struct cec_msg *msg) >>> +{ >>> + struct dw_hdmi_cec *cec = adap->priv; >>> + unsigned i; >>> + >>> + cec->retries = attempts; >> >> Should probably be 'cec->retries = attempts - 1;' >> Since 2 attempts == 1 retry. > > Probably correct, but rather moot given your comment below. > >>> +static irqreturn_t dw_hdmi_cec_hardirq(int irq, void *data) >>> +{ >>> + struct cec_adapter *adap = data; >>> + struct dw_hdmi_cec *cec = adap->priv; >>> + unsigned stat = dw_hdmi_read(cec, HDMI_IH_CEC_STAT0); >>> + irqreturn_t ret = IRQ_HANDLED; >>> + >>> + if (stat == 0) >>> + return IRQ_NONE; >>> + >>> + dw_hdmi_write(cec, stat, HDMI_IH_CEC_STAT0); >>> + >>> + if (stat & CEC_STAT_ERROR_INIT) { >>> + if (cec->retries) { >>> + unsigned v = dw_hdmi_read(cec, HDMI_CEC_CTRL); >>> + dw_hdmi_write(cec, v | CEC_CTRL_START, HDMI_CEC_CTRL); >>> + cec->retries -= 1; >> >> Why handle retries here at all? Just mark this as CEC_TX_STATUS_ERROR and set >> tx_done to true. The CEC core will just call dw_hdmi_cec_transmit again if >> it needs to make another attempt. I suggest that the whole retry code is >> dropped. > > Probably comes from a previous iteration of your CEC core which required > the driver to do retries, so it can probably be fixed up in the way you > describe. > > However, presumably this needs a different status other than > "CEC_TX_STATUS_MAX_RETRIES"? I don't see a way to report this error to > the CEC core in a way that it will retry. If the framework does the retries, then the cec driver will never return the MAX_RETRIES status. Just whether the transmit was OK/NACKed/LOW_DRIVE/ERROR. > >>> +static irqreturn_t dw_hdmi_cec_thread(int irq, void *data) >>> +{ >>> + struct cec_adapter *adap = data; >>> + struct dw_hdmi_cec *cec = adap->priv; >>> + >>> + if (cec->tx_done) { >>> + cec->tx_done = false; >>> + cec_transmit_done(adap, cec->tx_status, 0, 0, 0, 0); >> >> I think I will make a patch for a new helper function where you can just call: >> >> cec_transmit_attempt_done(adap, cec->tx_status); >> >> and that will call cec_transmit_done filling in the other arguments >> based on the status. >> >> Most CEC HW doesn't do retries so only make one attempt. In that case >> only one of the remaining count arguments is 1 and that's based on the >> status. >> >> Having correct error counts is useful for debugging problems. > > It hasn't been clear to me which counts should be non-zero, and it seems > like entirely redundant information given that the status argument > indicates what happened to the message. True for HW that doesn't retry. Expect a patch today/tomorrow. >>> +static void dw_hdmi_cec_disable(struct dw_hdmi *hdmi) >>> +{ >>> + mutex_lock(&hdmi->mutex); >>> + hdmi->mc_clkdis |= HDMI_MC_CLKDIS_CECCLK_DISABLE; >>> + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); >>> + mutex_unlock(&hdmi->mutex); >>> +} >>> + >>> +static const struct dw_hdmi_cec_ops dw_hdmi_cec_ops = { >>> + .write = hdmi_writeb, >>> + .read = hdmi_readb, >>> + .enable = dw_hdmi_cec_enable, >>> + .disable = dw_hdmi_cec_disable, >>> +}; >>> + >> >> This feels over-engineered and I suspect it is simplified if you just >> add a cec_adapter pointer to struct dw_hdmi and link dw-hdmi-cec.c >> together with dw-hdmi.c. > > As I mention above, to do so would mean that dw-hdmi has to be modular > if you want CEC to be modular, and I don't think that is reasonable > for a video output device, which may be the source of kernel boot > messages. > Indeed, but this dependency mess has been fixed, so this is no longer an issue. Regards, Hans
On 05/30/2017 04:23 PM, Russell King wrote: > Add a CEC driver for the dw-hdmi hardware. > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > --- > drivers/gpu/drm/bridge/synopsys/Kconfig | 8 + > drivers/gpu/drm/bridge/synopsys/Makefile | 1 + > drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 320 ++++++++++++++++++++++++++ > drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h | 19 ++ > drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 +++- > 5 files changed, 387 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > > diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig b/drivers/gpu/drm/bridge/synopsys/Kconfig > index 40d2827a6d19..bd30a0a07272 100644 > --- a/drivers/gpu/drm/bridge/synopsys/Kconfig > +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig > @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO > help > Support the I2S Audio interface which is part of the Synopsys > Designware HDMI block. > + > +config DRM_DW_HDMI_CEC > + tristate "Synopsis Designware CEC interface" > + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT > + select MEDIA_CEC_NOTIFIER > + help > + Support the CE interface which is part of the Synopsis > + Designware HDMI block. > diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile b/drivers/gpu/drm/bridge/synopsys/Makefile > index 17aa7a65b57e..6fe415903668 100644 > --- a/drivers/gpu/drm/bridge/synopsys/Makefile > +++ b/drivers/gpu/drm/bridge/synopsys/Makefile > @@ -3,3 +3,4 @@ > obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o > obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o > obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o > +obj-$(CONFIG_DRM_DW_HDMI_CEC) += dw-hdmi-cec.o > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > new file mode 100644 > index 000000000000..761ef5ae687d > --- /dev/null > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > @@ -0,0 +1,320 @@ > +/* > + * Designware HDMI CEC driver > + * > + * Copyright (C) 2015-2017 Russell King. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > +#include <linux/interrupt.h> > +#include <linux/io.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/sched.h> > +#include <linux/slab.h> > + > +#include <drm/drm_edid.h> > + > +#include <media/cec.h> > +#include <media/cec-notifier.h> > + > +#include "dw-hdmi-cec.h" > + > +enum { > + HDMI_IH_CEC_STAT0 = 0x0106, > + HDMI_IH_MUTE_CEC_STAT0 = 0x0186, > + > + HDMI_CEC_CTRL = 0x7d00, > + CEC_CTRL_START = BIT(0), > + CEC_CTRL_NORMAL = 1 << 1, > + > + HDMI_CEC_STAT = 0x7d01, > + CEC_STAT_DONE = BIT(0), > + CEC_STAT_EOM = BIT(1), > + CEC_STAT_NACK = BIT(2), > + CEC_STAT_ARBLOST = BIT(3), > + CEC_STAT_ERROR_INIT = BIT(4), > + CEC_STAT_ERROR_FOLL = BIT(5), > + CEC_STAT_WAKEUP = BIT(6), > + > + HDMI_CEC_MASK = 0x7d02, > + HDMI_CEC_POLARITY = 0x7d03, > + HDMI_CEC_INT = 0x7d04, > + HDMI_CEC_ADDR_L = 0x7d05, > + HDMI_CEC_ADDR_H = 0x7d06, > + HDMI_CEC_TX_CNT = 0x7d07, > + HDMI_CEC_RX_CNT = 0x7d08, > + HDMI_CEC_TX_DATA0 = 0x7d10, > + HDMI_CEC_RX_DATA0 = 0x7d20, > + HDMI_CEC_LOCK = 0x7d30, > + HDMI_CEC_WKUPCTRL = 0x7d31, > +}; > + > +struct dw_hdmi_cec { > + struct dw_hdmi *hdmi; > + const struct dw_hdmi_cec_ops *ops; > + u32 addresses; > + struct cec_adapter *adap; > + struct cec_msg rx_msg; > + unsigned int tx_status; > + bool tx_done; > + bool rx_done; > + struct cec_notifier *notify; > + int retries; > + int irq; > +}; > + > +static void dw_hdmi_write(struct dw_hdmi_cec *cec, u8 val, int offset) > +{ > + cec->ops->write(cec->hdmi, val, offset); > +} > + > +static u8 dw_hdmi_read(struct dw_hdmi_cec *cec, int offset) > +{ > + return cec->ops->read(cec->hdmi, offset); > +} > + > +static int dw_hdmi_cec_log_addr(struct cec_adapter *adap, u8 logical_addr) > +{ > + struct dw_hdmi_cec *cec = adap->priv; > + u32 addresses; > + > + if (logical_addr == CEC_LOG_ADDR_INVALID) > + addresses = cec->addresses = 0; > + else > + addresses = cec->addresses |= BIT(logical_addr) | BIT(15); > + > + dw_hdmi_write(cec, addresses & 255, HDMI_CEC_ADDR_L); > + dw_hdmi_write(cec, addresses >> 8, HDMI_CEC_ADDR_H); > + > + return 0; > +} > + > +static int dw_hdmi_cec_transmit(struct cec_adapter *adap, u8 attempts, > + u32 signal_free_time, struct cec_msg *msg) > +{ > + struct dw_hdmi_cec *cec = adap->priv; > + unsigned i; > + > + cec->retries = attempts; > + > + for (i = 0; i < msg->len; i++) > + dw_hdmi_write(cec, msg->msg[i], HDMI_CEC_TX_DATA0 + i); > + > + dw_hdmi_write(cec, msg->len, HDMI_CEC_TX_CNT); > + dw_hdmi_write(cec, CEC_CTRL_NORMAL | CEC_CTRL_START, HDMI_CEC_CTRL); > + > + return 0; > +} > + > +static irqreturn_t dw_hdmi_cec_hardirq(int irq, void *data) > +{ > + struct cec_adapter *adap = data; > + struct dw_hdmi_cec *cec = adap->priv; > + unsigned stat = dw_hdmi_read(cec, HDMI_IH_CEC_STAT0); > + irqreturn_t ret = IRQ_HANDLED; > + > + if (stat == 0) > + return IRQ_NONE; > + > + dw_hdmi_write(cec, stat, HDMI_IH_CEC_STAT0); > + > + if (stat & CEC_STAT_ERROR_INIT) { > + if (cec->retries) { > + unsigned v = dw_hdmi_read(cec, HDMI_CEC_CTRL); > + dw_hdmi_write(cec, v | CEC_CTRL_START, HDMI_CEC_CTRL); > + cec->retries -= 1; > + } else { > + cec->tx_status = CEC_TX_STATUS_MAX_RETRIES; > + cec->tx_done = true; > + ret = IRQ_WAKE_THREAD; > + } > + } else if (stat & CEC_STAT_DONE) { > + cec->tx_status = CEC_TX_STATUS_OK; > + cec->tx_done = true; > + ret = IRQ_WAKE_THREAD; > + } else if (stat & CEC_STAT_NACK) { > + cec->tx_status = CEC_TX_STATUS_NACK; > + cec->tx_done = true; > + ret = IRQ_WAKE_THREAD; > + } > + > + if (stat & CEC_STAT_EOM) { > + unsigned len, i; > + > + len = dw_hdmi_read(cec, HDMI_CEC_RX_CNT); > + if (len > sizeof(cec->rx_msg.msg)) > + len = sizeof(cec->rx_msg.msg); > + > + for (i = 0; i < len; i++) > + cec->rx_msg.msg[i] = > + dw_hdmi_read(cec, HDMI_CEC_RX_DATA0 + i); > + > + dw_hdmi_write(cec, 0, HDMI_CEC_LOCK); > + > + cec->rx_msg.len = len; > + smp_wmb(); > + cec->rx_done = true; > + > + ret = IRQ_WAKE_THREAD; > + } > + > + return ret; > +} > + > +static irqreturn_t dw_hdmi_cec_thread(int irq, void *data) > +{ > + struct cec_adapter *adap = data; > + struct dw_hdmi_cec *cec = adap->priv; > + > + if (cec->tx_done) { > + cec->tx_done = false; > + cec_transmit_done(adap, cec->tx_status, 0, 0, 0, 0); > + } > + if (cec->rx_done) { > + cec->rx_done = false; > + smp_rmb(); > + cec_received_msg(adap, &cec->rx_msg); > + } > + return IRQ_HANDLED; > +} > + > +static int dw_hdmi_cec_enable(struct cec_adapter *adap, bool enable) > +{ > + struct dw_hdmi_cec *cec = adap->priv; > + > + if (!enable) { > + dw_hdmi_write(cec, ~0, HDMI_CEC_MASK); > + dw_hdmi_write(cec, ~0, HDMI_IH_MUTE_CEC_STAT0); > + dw_hdmi_write(cec, 0, HDMI_CEC_POLARITY); > + > + cec->ops->disable(cec->hdmi); > + } else { > + unsigned irqs; > + > + dw_hdmi_write(cec, 0, HDMI_CEC_CTRL); > + dw_hdmi_write(cec, ~0, HDMI_IH_CEC_STAT0); > + dw_hdmi_write(cec, 0, HDMI_CEC_LOCK); > + > + dw_hdmi_cec_log_addr(cec->adap, CEC_LOG_ADDR_INVALID); > + > + cec->ops->enable(cec->hdmi); > + > + irqs = CEC_STAT_ERROR_INIT | CEC_STAT_NACK | CEC_STAT_EOM | > + CEC_STAT_DONE; > + dw_hdmi_write(cec, irqs, HDMI_CEC_POLARITY); > + dw_hdmi_write(cec, ~irqs, HDMI_CEC_MASK); > + dw_hdmi_write(cec, ~irqs, HDMI_IH_MUTE_CEC_STAT0); > + } > + return 0; > +} > + > +static const struct cec_adap_ops dw_hdmi_cec_ops = { > + .adap_enable = dw_hdmi_cec_enable, > + .adap_log_addr = dw_hdmi_cec_log_addr, > + .adap_transmit = dw_hdmi_cec_transmit, > +}; > + > +static void dw_hdmi_cec_del(void *data) > +{ > + struct dw_hdmi_cec *cec = data; > + > + cec_delete_adapter(cec->adap); > +} > + > +static int dw_hdmi_cec_probe(struct platform_device *pdev) > +{ > + struct dw_hdmi_cec_data *data = dev_get_platdata(&pdev->dev); > + struct dw_hdmi_cec *cec; > + int ret; > + > + if (!data) > + return -ENXIO; > + > + /* > + * Our device is just a convenience - we want to link to the real > + * hardware device here, so that userspace can see the association > + * between the HDMI hardware and its associated CEC chardev. > + */ > + cec = devm_kzalloc(&pdev->dev, sizeof(*cec), GFP_KERNEL); > + if (!cec) > + return -ENOMEM; > + > + cec->irq = data->irq; > + cec->ops = data->ops; > + cec->hdmi = data->hdmi; > + > + platform_set_drvdata(pdev, cec); > + > + dw_hdmi_write(cec, 0, HDMI_CEC_TX_CNT); > + dw_hdmi_write(cec, ~0, HDMI_CEC_MASK); > + dw_hdmi_write(cec, ~0, HDMI_IH_MUTE_CEC_STAT0); > + dw_hdmi_write(cec, 0, HDMI_CEC_POLARITY); > + > + cec->adap = cec_allocate_adapter(&dw_hdmi_cec_ops, cec, "dw_hdmi", > + CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT | > + CEC_CAP_RC, CEC_MAX_LOG_ADDRS); > + if (IS_ERR(cec->adap)) > + return PTR_ERR(cec->adap); > + > + /* override the module pointer */ > + cec->adap->owner = THIS_MODULE; > + > + ret = devm_add_action(&pdev->dev, dw_hdmi_cec_del, cec); > + if (ret) { > + cec_delete_adapter(cec->adap); > + return ret; > + } > + > + ret = devm_request_threaded_irq(&pdev->dev, cec->irq, > + dw_hdmi_cec_hardirq, > + dw_hdmi_cec_thread, IRQF_SHARED, > + "dw-hdmi-cec", cec->adap); > + if (ret < 0) > + return ret; > + > + cec->notify = cec_notifier_get(pdev->dev.parent); > + if (!cec->notify) > + return -ENOMEM; > + > + ret = cec_register_adapter(cec->adap, pdev->dev.parent); > + if (ret < 0) { > + cec_notifier_put(cec->notify); > + return ret; > + } > + > + /* > + * CEC documentation says we must not call cec_delete_adapter > + * after a successful call to cec_register_adapter(). > + */ > + devm_remove_action(&pdev->dev, dw_hdmi_cec_del, cec); > + > + cec_register_cec_notifier(cec->adap, cec->notify); > + > + return 0; > +} > + > +static int dw_hdmi_cec_remove(struct platform_device *pdev) > +{ > + struct dw_hdmi_cec *cec = platform_get_drvdata(pdev); > + > + cec_unregister_adapter(cec->adap); > + cec_notifier_put(cec->notify); > + > + return 0; > +} > + > +static struct platform_driver dw_hdmi_cec_driver = { > + .probe = dw_hdmi_cec_probe, > + .remove = dw_hdmi_cec_remove, > + .driver = { > + .name = "dw-hdmi-cec", > + }, > +}; > +module_platform_driver(dw_hdmi_cec_driver); > + > +MODULE_AUTHOR("Russell King <rmk+kernel@armlinux.org.uk>"); > +MODULE_DESCRIPTION("Synopsis Designware HDMI CEC driver for i.MX"); > +MODULE_LICENSE("GPL"); > +MODULE_ALIAS(PLATFORM_MODULE_PREFIX "dw-hdmi-cec"); > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > new file mode 100644 > index 000000000000..cf4dc121a2c4 > --- /dev/null > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > @@ -0,0 +1,19 @@ > +#ifndef DW_HDMI_CEC_H > +#define DW_HDMI_CEC_H > + > +struct dw_hdmi; > + > +struct dw_hdmi_cec_ops { > + void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); > + u8 (*read)(struct dw_hdmi *hdmi, int offset); > + void (*enable)(struct dw_hdmi *hdmi); > + void (*disable)(struct dw_hdmi *hdmi); > +}; > + > +struct dw_hdmi_cec_data { > + struct dw_hdmi *hdmi; > + const struct dw_hdmi_cec_ops *ops; > + int irq; > +}; > + > +#endif > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > index 58781d4c1034..5a389f1f58d9 100644 > --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c > @@ -35,6 +35,7 @@ > > #include "dw-hdmi.h" > #include "dw-hdmi-audio.h" > +#include "dw-hdmi-cec.h" > > #include <media/cec-notifier.h> > > @@ -133,6 +134,7 @@ struct dw_hdmi { > unsigned int version; > > struct platform_device *audio; > + struct platform_device *cec; > struct device *dev; > struct clk *isfr_clk; > struct clk *iahb_clk; > @@ -1766,7 +1768,6 @@ static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi) > hdmi_writeb(hdmi, 0xff, HDMI_AUD_HBR_MASK); > hdmi_writeb(hdmi, 0xff, HDMI_GP_MASK); > hdmi_writeb(hdmi, 0xff, HDMI_A_APIINTMSK); > - hdmi_writeb(hdmi, 0xff, HDMI_CEC_MASK); > hdmi_writeb(hdmi, 0xff, HDMI_I2CM_INT); > hdmi_writeb(hdmi, 0xff, HDMI_I2CM_CTLINT); > > @@ -2223,6 +2224,29 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) > return -ENODEV; > } > > +static void dw_hdmi_cec_enable(struct dw_hdmi *hdmi) > +{ > + mutex_lock(&hdmi->mutex); > + hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CECCLK_DISABLE; > + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); > + mutex_unlock(&hdmi->mutex); > +} > + > +static void dw_hdmi_cec_disable(struct dw_hdmi *hdmi) > +{ > + mutex_lock(&hdmi->mutex); > + hdmi->mc_clkdis |= HDMI_MC_CLKDIS_CECCLK_DISABLE; > + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); > + mutex_unlock(&hdmi->mutex); > +} > + > +static const struct dw_hdmi_cec_ops dw_hdmi_cec_ops = { > + .write = hdmi_writeb, > + .read = hdmi_readb, > + .enable = dw_hdmi_cec_enable, > + .disable = dw_hdmi_cec_disable, > +}; > + > static const struct regmap_config hdmi_regmap_8bit_config = { > .reg_bits = 32, > .val_bits = 8, > @@ -2245,6 +2269,7 @@ __dw_hdmi_probe(struct platform_device *pdev, > struct device_node *np = dev->of_node; > struct platform_device_info pdevinfo; > struct device_node *ddc_node; > + struct dw_hdmi_cec_data cec; > struct dw_hdmi *hdmi; > struct resource *iores = NULL; > int irq; > @@ -2445,6 +2470,17 @@ __dw_hdmi_probe(struct platform_device *pdev, > hdmi->audio = platform_device_register_full(&pdevinfo); > } > > + cec.hdmi = hdmi; > + cec.ops = &dw_hdmi_cec_ops; > + cec.irq = irq; > + > + pdevinfo.name = "dw-hdmi-cec"; > + pdevinfo.data = &cec; > + pdevinfo.size_data = sizeof(cec); > + pdevinfo.dma_mask = 0; > + > + hdmi->cec = platform_device_register_full(&pdevinfo); Maybe you should check the config bit before enabling CEC : /* CONFIG0_ID field values */ + HDMI_CONFIG0_CEC = 0x2, if (config0 & HDMI_CONFIG0_CEC) { } > + > /* Reset HDMI DDC I2C master controller and mute I2CM interrupts */ > if (hdmi->i2c) > dw_hdmi_i2c_init(hdmi); > @@ -2475,6 +2511,8 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) > { > if (hdmi->audio && !IS_ERR(hdmi->audio)) > platform_device_unregister(hdmi->audio); > + if (!IS_ERR(hdmi->cec)) > + platform_device_unregister(hdmi->cec); > > /* Disable all interrupts */ > hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0); > Apart from that and the s/Synopsis/Synopsys/ typos: Reviewed-by: Neil Armstrong <narmstrong@baylibre.com> Neil
On Thu, Jun 01, 2017 at 01:53:21AM +0100, Jose Abreu wrote: > Hi Russell, > > > On 30-05-2017 15:23, Russell King wrote: > > +static int dw_hdmi_cec_transmit(struct cec_adapter *adap, u8 attempts, > > + u32 signal_free_time, struct cec_msg *msg) > > +{ > > + struct dw_hdmi_cec *cec = adap->priv; > > + unsigned i; > > + > > + cec->retries = attempts; > > I think you should check first if CEC engine is in normal > operation mode, as a safety measure. I'm not sure what you mean there, because the iMX6 manuals don't mention anything about that. The only "modes" it talks about is initiator mode and follower mode. Moreover, there's nothing in what was FSL's driver that hints at that either. Maybe you could provide some further technical information on this point?
On 01-06-2017 23:30, Russell King - ARM Linux wrote: > On Thu, Jun 01, 2017 at 01:53:21AM +0100, Jose Abreu wrote: >> Hi Russell, >> >> >> On 30-05-2017 15:23, Russell King wrote: >>> +static int dw_hdmi_cec_transmit(struct cec_adapter *adap, u8 attempts, >>> + u32 signal_free_time, struct cec_msg *msg) >>> +{ >>> + struct dw_hdmi_cec *cec = adap->priv; >>> + unsigned i; >>> + >>> + cec->retries = attempts; >> I think you should check first if CEC engine is in normal >> operation mode, as a safety measure. > I'm not sure what you mean there, because the iMX6 manuals don't mention > anything about that. The only "modes" it talks about is initiator mode > and follower mode. Moreover, there's nothing in what was FSL's driver > that hints at that either. > > Maybe you could provide some further technical information on this > point? > You should check that CEC is: not in standy, acknowledges broadcast messages, signal free time is 5bit period, and not lost arbitration, which basically means CEC_CTRL must be 0x2 and IH_CEC_STAT0 must not have ARB_LOST set. I do know you set 0x3 at the end of this function but according to all the docs I have you must first set 0x2 before writing message. Best regards, Jose Miguel Abreu
On 06/01/2017 03:47 PM, Neil Armstrong wrote: > On 05/30/2017 04:23 PM, Russell King wrote: >> Add a CEC driver for the dw-hdmi hardware. >> >> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >> --- >> drivers/gpu/drm/bridge/synopsys/Kconfig | 8 + >> drivers/gpu/drm/bridge/synopsys/Makefile | 1 + >> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 320 ++++++++++++++++++++++++++ >> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h | 19 ++ >> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 +++- >> 5 files changed, 387 insertions(+), 1 deletion(-) >> create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >> create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h >> >> diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig b/drivers/gpu/drm/bridge/synopsys/Kconfig >> index 40d2827a6d19..bd30a0a07272 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/Kconfig >> +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig >> @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO >> help >> Support the I2S Audio interface which is part of the Synopsys >> Designware HDMI block. >> + >> +config DRM_DW_HDMI_CEC >> + tristate "Synopsis Designware CEC interface" >> + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT >> + select MEDIA_CEC_NOTIFIER >> + help >> + Support the CE interface which is part of the Synopsis >> + Designware HDMI block. >> diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile b/drivers/gpu/drm/bridge/synopsys/Makefile >> index 17aa7a65b57e..6fe415903668 100644 >> --- a/drivers/gpu/drm/bridge/synopsys/Makefile >> +++ b/drivers/gpu/drm/bridge/synopsys/Makefile >> @@ -3,3 +3,4 @@ >> obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o >> obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o >> obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o >> +obj-$(CONFIG_DRM_DW_HDMI_CEC) += dw-hdmi-cec.o >> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >> new file mode 100644 >> index 000000000000..761ef5ae687d >> --- /dev/null >> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >> @@ -0,0 +1,320 @@ >> +/* >> + * Designware HDMI CEC driver >> + * >> + * Copyright (C) 2015-2017 Russell King. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> +#include <linux/interrupt.h> >> +#include <linux/io.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/sched.h> >> +#include <linux/slab.h> >> + >> +#include <drm/drm_edid.h> >> + >> +#include <media/cec.h> >> +#include <media/cec-notifier.h> >> + >> +#include "dw-hdmi-cec.h" >> + >> +enum { >> + HDMI_IH_CEC_STAT0 = 0x0106, >> + HDMI_IH_MUTE_CEC_STAT0 = 0x0186, >> + >> + HDMI_CEC_CTRL = 0x7d00, >> + CEC_CTRL_START = BIT(0), >> + CEC_CTRL_NORMAL = 1 << 1, >> + >> + HDMI_CEC_STAT = 0x7d01, >> + CEC_STAT_DONE = BIT(0), >> + CEC_STAT_EOM = BIT(1), >> + CEC_STAT_NACK = BIT(2), >> + CEC_STAT_ARBLOST = BIT(3), >> + CEC_STAT_ERROR_INIT = BIT(4), >> + CEC_STAT_ERROR_FOLL = BIT(5), >> + CEC_STAT_WAKEUP = BIT(6), I hadn't realized until Jose Abreu's latest reply, but you need to check the ARBLOST status and set the TX state to CEC_TX_STATUS_ARB_LOST. I think CEC_STAT_ERROR_FOLL might equal CEC_TX_STATUS_LOW_DRIVE, but without documentation I can't be sure. My experience is that this low drive condition tends to be poorly reported by hardware. Either that or poorly documented. This is why I added a CEC_TX_STATUS_ERROR status as a catch-all error status when it is unclear from the hardware/documentation what error occurred. Jose, do you know which status bit is used to report a follower pulling the CEC line low for a long time (approx. 3.6 ms) to signal a CEC error? Regards, Hans
Hi Hans, On 02-06-2017 07:31, Hans Verkuil wrote: > On 06/01/2017 03:47 PM, Neil Armstrong wrote: >> On 05/30/2017 04:23 PM, Russell King wrote: >>> Add a CEC driver for the dw-hdmi hardware. >>> >>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >>> --- >>> drivers/gpu/drm/bridge/synopsys/Kconfig | 8 + >>> drivers/gpu/drm/bridge/synopsys/Makefile | 1 + >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 320 >>> ++++++++++++++++++++++++++ >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h | 19 ++ >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 +++- >>> 5 files changed, 387 insertions(+), 1 deletion(-) >>> create mode 100644 >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >>> create mode 100644 >>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h >>> >>> diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig >>> b/drivers/gpu/drm/bridge/synopsys/Kconfig >>> index 40d2827a6d19..bd30a0a07272 100644 >>> --- a/drivers/gpu/drm/bridge/synopsys/Kconfig >>> +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig >>> @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO >>> help >>> Support the I2S Audio interface which is part of the >>> Synopsys >>> Designware HDMI block. >>> + >>> +config DRM_DW_HDMI_CEC >>> + tristate "Synopsis Designware CEC interface" >>> + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT >>> + select MEDIA_CEC_NOTIFIER >>> + help >>> + Support the CE interface which is part of the Synopsis >>> + Designware HDMI block. >>> diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile >>> b/drivers/gpu/drm/bridge/synopsys/Makefile >>> index 17aa7a65b57e..6fe415903668 100644 >>> --- a/drivers/gpu/drm/bridge/synopsys/Makefile >>> +++ b/drivers/gpu/drm/bridge/synopsys/Makefile >>> @@ -3,3 +3,4 @@ >>> obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o >>> obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o >>> obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o >>> +obj-$(CONFIG_DRM_DW_HDMI_CEC) += dw-hdmi-cec.o >>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >>> new file mode 100644 >>> index 000000000000..761ef5ae687d >>> --- /dev/null >>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >>> @@ -0,0 +1,320 @@ >>> +/* >>> + * Designware HDMI CEC driver >>> + * >>> + * Copyright (C) 2015-2017 Russell King. >>> + * >>> + * This program is free software; you can redistribute it >>> and/or modify >>> + * it under the terms of the GNU General Public License >>> version 2 as >>> + * published by the Free Software Foundation. >>> + */ >>> +#include <linux/interrupt.h> >>> +#include <linux/io.h> >>> +#include <linux/module.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/sched.h> >>> +#include <linux/slab.h> >>> + >>> +#include <drm/drm_edid.h> >>> + >>> +#include <media/cec.h> >>> +#include <media/cec-notifier.h> >>> + >>> +#include "dw-hdmi-cec.h" >>> + >>> +enum { >>> + HDMI_IH_CEC_STAT0 = 0x0106, >>> + HDMI_IH_MUTE_CEC_STAT0 = 0x0186, >>> + >>> + HDMI_CEC_CTRL = 0x7d00, >>> + CEC_CTRL_START = BIT(0), >>> + CEC_CTRL_NORMAL = 1 << 1, >>> + >>> + HDMI_CEC_STAT = 0x7d01, >>> + CEC_STAT_DONE = BIT(0), >>> + CEC_STAT_EOM = BIT(1), >>> + CEC_STAT_NACK = BIT(2), >>> + CEC_STAT_ARBLOST = BIT(3), >>> + CEC_STAT_ERROR_INIT = BIT(4), >>> + CEC_STAT_ERROR_FOLL = BIT(5), >>> + CEC_STAT_WAKEUP = BIT(6), > > I hadn't realized until Jose Abreu's latest reply, but you need > to check the > ARBLOST status and set the TX state to CEC_TX_STATUS_ARB_LOST. > > I think CEC_STAT_ERROR_FOLL might equal > CEC_TX_STATUS_LOW_DRIVE, but without > documentation I can't be sure. > > My experience is that this low drive condition tends to be > poorly reported by > hardware. Either that or poorly documented. This is why I added a > CEC_TX_STATUS_ERROR status as a catch-all error status when it > is unclear from > the hardware/documentation what error occurred. > > Jose, do you know which status bit is used to report a follower > pulling the > CEC line low for a long time (approx. 3.6 ms) to signal a CEC > error? Bit 5 for follower error, bit 4 for initiator error. Best regards, Jose Miguel Abreu > > Regards, > > Hans
On 06/02/17 08:43, Jose Abreu wrote: > Hi Hans, > > > On 02-06-2017 07:31, Hans Verkuil wrote: >> On 06/01/2017 03:47 PM, Neil Armstrong wrote: >>> On 05/30/2017 04:23 PM, Russell King wrote: >>>> Add a CEC driver for the dw-hdmi hardware. >>>> >>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> >>>> --- >>>> drivers/gpu/drm/bridge/synopsys/Kconfig | 8 + >>>> drivers/gpu/drm/bridge/synopsys/Makefile | 1 + >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 320 >>>> ++++++++++++++++++++++++++ >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h | 19 ++ >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 +++- >>>> 5 files changed, 387 insertions(+), 1 deletion(-) >>>> create mode 100644 >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >>>> create mode 100644 >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h >>>> >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig >>>> b/drivers/gpu/drm/bridge/synopsys/Kconfig >>>> index 40d2827a6d19..bd30a0a07272 100644 >>>> --- a/drivers/gpu/drm/bridge/synopsys/Kconfig >>>> +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig >>>> @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO >>>> help >>>> Support the I2S Audio interface which is part of the >>>> Synopsys >>>> Designware HDMI block. >>>> + >>>> +config DRM_DW_HDMI_CEC >>>> + tristate "Synopsis Designware CEC interface" >>>> + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT >>>> + select MEDIA_CEC_NOTIFIER >>>> + help >>>> + Support the CE interface which is part of the Synopsis >>>> + Designware HDMI block. >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile >>>> b/drivers/gpu/drm/bridge/synopsys/Makefile >>>> index 17aa7a65b57e..6fe415903668 100644 >>>> --- a/drivers/gpu/drm/bridge/synopsys/Makefile >>>> +++ b/drivers/gpu/drm/bridge/synopsys/Makefile >>>> @@ -3,3 +3,4 @@ >>>> obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o >>>> obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o >>>> obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o >>>> +obj-$(CONFIG_DRM_DW_HDMI_CEC) += dw-hdmi-cec.o >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >>>> new file mode 100644 >>>> index 000000000000..761ef5ae687d >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c >>>> @@ -0,0 +1,320 @@ >>>> +/* >>>> + * Designware HDMI CEC driver >>>> + * >>>> + * Copyright (C) 2015-2017 Russell King. >>>> + * >>>> + * This program is free software; you can redistribute it >>>> and/or modify >>>> + * it under the terms of the GNU General Public License >>>> version 2 as >>>> + * published by the Free Software Foundation. >>>> + */ >>>> +#include <linux/interrupt.h> >>>> +#include <linux/io.h> >>>> +#include <linux/module.h> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/sched.h> >>>> +#include <linux/slab.h> >>>> + >>>> +#include <drm/drm_edid.h> >>>> + >>>> +#include <media/cec.h> >>>> +#include <media/cec-notifier.h> >>>> + >>>> +#include "dw-hdmi-cec.h" >>>> + >>>> +enum { >>>> + HDMI_IH_CEC_STAT0 = 0x0106, >>>> + HDMI_IH_MUTE_CEC_STAT0 = 0x0186, >>>> + >>>> + HDMI_CEC_CTRL = 0x7d00, >>>> + CEC_CTRL_START = BIT(0), >>>> + CEC_CTRL_NORMAL = 1 << 1, >>>> + >>>> + HDMI_CEC_STAT = 0x7d01, >>>> + CEC_STAT_DONE = BIT(0), >>>> + CEC_STAT_EOM = BIT(1), >>>> + CEC_STAT_NACK = BIT(2), >>>> + CEC_STAT_ARBLOST = BIT(3), >>>> + CEC_STAT_ERROR_INIT = BIT(4), >>>> + CEC_STAT_ERROR_FOLL = BIT(5), >>>> + CEC_STAT_WAKEUP = BIT(6), >> >> I hadn't realized until Jose Abreu's latest reply, but you need >> to check the >> ARBLOST status and set the TX state to CEC_TX_STATUS_ARB_LOST. >> >> I think CEC_STAT_ERROR_FOLL might equal >> CEC_TX_STATUS_LOW_DRIVE, but without >> documentation I can't be sure. >> >> My experience is that this low drive condition tends to be >> poorly reported by >> hardware. Either that or poorly documented. This is why I added a >> CEC_TX_STATUS_ERROR status as a catch-all error status when it >> is unclear from >> the hardware/documentation what error occurred. >> >> Jose, do you know which status bit is used to report a follower >> pulling the >> CEC line low for a long time (approx. 3.6 ms) to signal a CEC >> error? > > Bit 5 for follower error, bit 4 for initiator error. I gathered that from the define names :-) But what does it mean? What sort of error is reported here? I'm guessing here that "follower error" means that one of the remote CEC devices forced a Low Drive condition, where "initiator error" means that our adapter forced a Low Drive condition on the bus. Would that be correct? If so, then the CEC_STAT_ERROR_INIT can be ignored and ERROR_FOLL maps to the LOW_DRIVE status. (Low Drive condition is what is described in section CEC 7.4 "CEC Line Error Handling" of the HDMI 1.4 Specification). Regards, Hans
On Fri, Jun 02, 2017 at 06:02:28AM +0100, Jose Abreu wrote: > You should check that CEC is: not in standy, acknowledges > broadcast messages, signal free time is 5bit period, and not lost > arbitration, which basically means CEC_CTRL must be 0x2 and > IH_CEC_STAT0 must not have ARB_LOST set. If ARB_LOST is set, that will trigger an interrupt, and the interrupt handler will clear the bit. So all the time that the interrupt handler is present, ARB_LOST should be clear whenever we try to send a message. When we enable the CEC interface, we zero the CEC_CTRL register, which takes the controller out of standby, and initialises the command register. Bits 2:1 select the signal free time, and there's no requirement specified to require them to be set to '01' before writing the message - in fact, it's legal for them to be set to other values, particularly when retrying, which is something I've missed. 2-1 Frame Type bit FRAME_TYP 00 Signal Free Time = 3-bit periods. Previous attempt to send frame is unsuccessful. 01 Signal Free Time = 5-bit periods. New initiator wants to send a frame. 10 Signal Free Time = 7-bit periods. Present initiator wants to send another frame immediately after its previous frame. (spec CEC 9.1) 11 Illegal value. If software write this value, hardware will set the value to the default 2'b01. Clearly from that, there are times when we want to transmit a message without a 5-bit signal free time period, particularly when we're retrying or wanting to send another frame, so I don't believe that there's a requirement for the control register to always be set to 0x02. I suspect that where that value is coming from is an application note describing how to send a brand new message each time, and not covering the other cases. It could be that we need to set the frame type before loading the message - that I'll buy, but not that it must always be set to 0x02. Provided that the standby and BC_NACK bits are both cleared should be sufficient.
On Fri, Jun 02, 2017 at 11:06:24AM +0200, Hans Verkuil wrote: > On 06/02/17 08:43, Jose Abreu wrote: > > Hi Hans, > > > > > > On 02-06-2017 07:31, Hans Verkuil wrote: > >> On 06/01/2017 03:47 PM, Neil Armstrong wrote: > >>> On 05/30/2017 04:23 PM, Russell King wrote: > >>>> Add a CEC driver for the dw-hdmi hardware. > >>>> > >>>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> > >>>> --- > >>>> drivers/gpu/drm/bridge/synopsys/Kconfig | 8 + > >>>> drivers/gpu/drm/bridge/synopsys/Makefile | 1 + > >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 320 > >>>> ++++++++++++++++++++++++++ > >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h | 19 ++ > >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 +++- > >>>> 5 files changed, 387 insertions(+), 1 deletion(-) > >>>> create mode 100644 > >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > >>>> create mode 100644 > >>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h > >>>> > >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig > >>>> b/drivers/gpu/drm/bridge/synopsys/Kconfig > >>>> index 40d2827a6d19..bd30a0a07272 100644 > >>>> --- a/drivers/gpu/drm/bridge/synopsys/Kconfig > >>>> +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig > >>>> @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO > >>>> help > >>>> Support the I2S Audio interface which is part of the > >>>> Synopsys > >>>> Designware HDMI block. > >>>> + > >>>> +config DRM_DW_HDMI_CEC > >>>> + tristate "Synopsis Designware CEC interface" > >>>> + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT > >>>> + select MEDIA_CEC_NOTIFIER > >>>> + help > >>>> + Support the CE interface which is part of the Synopsis > >>>> + Designware HDMI block. > >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile > >>>> b/drivers/gpu/drm/bridge/synopsys/Makefile > >>>> index 17aa7a65b57e..6fe415903668 100644 > >>>> --- a/drivers/gpu/drm/bridge/synopsys/Makefile > >>>> +++ b/drivers/gpu/drm/bridge/synopsys/Makefile > >>>> @@ -3,3 +3,4 @@ > >>>> obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o > >>>> obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o > >>>> obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o > >>>> +obj-$(CONFIG_DRM_DW_HDMI_CEC) += dw-hdmi-cec.o > >>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > >>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > >>>> new file mode 100644 > >>>> index 000000000000..761ef5ae687d > >>>> --- /dev/null > >>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c > >>>> @@ -0,0 +1,320 @@ > >>>> +/* > >>>> + * Designware HDMI CEC driver > >>>> + * > >>>> + * Copyright (C) 2015-2017 Russell King. > >>>> + * > >>>> + * This program is free software; you can redistribute it > >>>> and/or modify > >>>> + * it under the terms of the GNU General Public License > >>>> version 2 as > >>>> + * published by the Free Software Foundation. > >>>> + */ > >>>> +#include <linux/interrupt.h> > >>>> +#include <linux/io.h> > >>>> +#include <linux/module.h> > >>>> +#include <linux/platform_device.h> > >>>> +#include <linux/sched.h> > >>>> +#include <linux/slab.h> > >>>> + > >>>> +#include <drm/drm_edid.h> > >>>> + > >>>> +#include <media/cec.h> > >>>> +#include <media/cec-notifier.h> > >>>> + > >>>> +#include "dw-hdmi-cec.h" > >>>> + > >>>> +enum { > >>>> + HDMI_IH_CEC_STAT0 = 0x0106, > >>>> + HDMI_IH_MUTE_CEC_STAT0 = 0x0186, > >>>> + > >>>> + HDMI_CEC_CTRL = 0x7d00, > >>>> + CEC_CTRL_START = BIT(0), > >>>> + CEC_CTRL_NORMAL = 1 << 1, > >>>> + > >>>> + HDMI_CEC_STAT = 0x7d01, > >>>> + CEC_STAT_DONE = BIT(0), > >>>> + CEC_STAT_EOM = BIT(1), > >>>> + CEC_STAT_NACK = BIT(2), > >>>> + CEC_STAT_ARBLOST = BIT(3), > >>>> + CEC_STAT_ERROR_INIT = BIT(4), > >>>> + CEC_STAT_ERROR_FOLL = BIT(5), > >>>> + CEC_STAT_WAKEUP = BIT(6), > >> > >> I hadn't realized until Jose Abreu's latest reply, but you need > >> to check the > >> ARBLOST status and set the TX state to CEC_TX_STATUS_ARB_LOST. > >> > >> I think CEC_STAT_ERROR_FOLL might equal > >> CEC_TX_STATUS_LOW_DRIVE, but without > >> documentation I can't be sure. > >> > >> My experience is that this low drive condition tends to be > >> poorly reported by > >> hardware. Either that or poorly documented. This is why I added a > >> CEC_TX_STATUS_ERROR status as a catch-all error status when it > >> is unclear from > >> the hardware/documentation what error occurred. > >> > >> Jose, do you know which status bit is used to report a follower > >> pulling the > >> CEC line low for a long time (approx. 3.6 ms) to signal a CEC > >> error? > > > > Bit 5 for follower error, bit 4 for initiator error. > > I gathered that from the define names :-) > > But what does it mean? What sort of error is reported here? I think the problem is that no one really knows, the documentation is quite poor: 5 An error is notified by a follower. Abnormal logic data ERROR_FOLL bit error (for follower). 4 An error is detected on cec line (for initiator only). ERROR_INIT It is so vague that you can read anything into that description.
On 06/02/17 11:15, Russell King - ARM Linux wrote: > On Fri, Jun 02, 2017 at 06:02:28AM +0100, Jose Abreu wrote: >> You should check that CEC is: not in standy, acknowledges >> broadcast messages, signal free time is 5bit period, and not lost >> arbitration, which basically means CEC_CTRL must be 0x2 and >> IH_CEC_STAT0 must not have ARB_LOST set. > > If ARB_LOST is set, that will trigger an interrupt, and the interrupt > handler will clear the bit. So all the time that the interrupt handler > is present, ARB_LOST should be clear whenever we try to send a message. > > When we enable the CEC interface, we zero the CEC_CTRL register, which > takes the controller out of standby, and initialises the command > register. > > Bits 2:1 select the signal free time, and there's no requirement > specified to require them to be set to '01' before writing the > message - in fact, it's legal for them to be set to other values, > particularly when retrying, which is something I've missed. > > 2-1 Frame Type bit > FRAME_TYP > 00 Signal Free Time = 3-bit periods. Previous > attempt to send frame is unsuccessful. > 01 Signal Free Time = 5-bit periods. New initiator > wants to send a frame. > 10 Signal Free Time = 7-bit periods. Present > initiator wants to send another frame > immediately after its previous frame. (spec > CEC 9.1) > 11 Illegal value. If software write this value, > hardware will set the value to the default 2'b01. The 'signal_free_time' argument of adap_transmit will have the recommended signal free time. You can test against the CEC_SIGNAL_FREE_TIME_* defines from media/cec.h. You probably saw this already, but just in case you missed this... Regards, Hans
On Fri, Jun 02, 2017 at 11:28:08AM +0200, Hans Verkuil wrote: > The 'signal_free_time' argument of adap_transmit will have the recommended > signal free time. You can test against the CEC_SIGNAL_FREE_TIME_* defines > from media/cec.h. You probably saw this already, but just in case you missed > this... Yes, it's a recent addition to the CEC core, which I've added support for, but it doesn't make too much sense until the retrying stuff is sorted out.
On Thu, Jun 01, 2017 at 10:31:10AM +0200, Hans Verkuil wrote: > This will change. Patches to fix the config handling are pending for 4.12. > > Here you can see the pending patches: > https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=drm-cec > > The patches from 'cec-notifier.h: handle unreachable CONFIG_CEC_CORE' to > 'cec: drop MEDIA_CEC_DEBUG' should all be merged in 4.12. Hi Hans, I'll wait until these have hit mainline before generating another patch series. Do you have any idea when that might happen? Thanks.
On 06/02/17 14:07, Russell King - ARM Linux wrote: > On Thu, Jun 01, 2017 at 10:31:10AM +0200, Hans Verkuil wrote: >> This will change. Patches to fix the config handling are pending for 4.12. >> >> Here you can see the pending patches: >> https://git.linuxtv.org/hverkuil/media_tree.git/log/?h=drm-cec >> >> The patches from 'cec-notifier.h: handle unreachable CONFIG_CEC_CORE' to >> 'cec: drop MEDIA_CEC_DEBUG' should all be merged in 4.12. > > Hi Hans, > > I'll wait until these have hit mainline before generating another > patch series. Do you have any idea when that might happen? rc6? Hard to tell, I know Mauro is very busy so it can be difficult to predict. Regards, Hans
On 06/01/2017 10:31 AM, Hans Verkuil wrote: > Hi Russell, > > First a few top-level questions: > > 1) What was the reason for using the cec-notifier here? Isn't this > tightly integrated into the main dw-hdmi block? For the tda driver > it is clearly required, but for tightly coupled HDMI & CEC HW I > just create the adapter from the HDMI driver. As a small bonus it > avoids adding the cec-notifier code and the control flow is a bit > easier to trace. > > 2) I may have asked this before, apologies if I repeat myself: does > this CEC implementation support CEC monitoring (aka snooping)? If > it does, then I recommend that it is implemented since it is very > useful. > > 3) Is the CEC still active if there is no hotplug signal? Or is it > powered off in that case? Ideally it should still be possible to > send CEC messages even if there is no hotplug. This is explicitly > allowed by the CEC 2.0 spec to wake up displays that turn off the > HPD, but that still have a working CEC controller. > > If this is not possible, then you need to use the CEC_CAP_NEEDS_HPD > capability. See: https://patchwork.linuxtv.org/patch/41478/ > > This will almost certainly be merged for 4.13 since other CEC drivers > need this as well. FYI: I tested your patch series with my cubox-i and CEC doesn't work if there is no HPD. I fiddles around a bit in dw_hdmi.c to prevent it from powering off the HDMI and PHY, but without any luck. It could be a hardware issue on the cubox-i (e.g. a level-shifter that powers off when the HPD goes low, although I don't see anything like that in the schematics), or it can be a driver issue or a Synopsys IP issue. I really can't tell. I added text in my status document (https://hverkuil.home.xs4all.nl/cec-status.txt) at the end on how to test this. Otherwise the CEC support on the cubox-i was working very well. Regards, Hans
diff --git a/drivers/gpu/drm/bridge/synopsys/Kconfig b/drivers/gpu/drm/bridge/synopsys/Kconfig index 40d2827a6d19..bd30a0a07272 100644 --- a/drivers/gpu/drm/bridge/synopsys/Kconfig +++ b/drivers/gpu/drm/bridge/synopsys/Kconfig @@ -21,3 +21,11 @@ config DRM_DW_HDMI_I2S_AUDIO help Support the I2S Audio interface which is part of the Synopsys Designware HDMI block. + +config DRM_DW_HDMI_CEC + tristate "Synopsis Designware CEC interface" + depends on DRM_DW_HDMI && MEDIA_CEC_SUPPORT + select MEDIA_CEC_NOTIFIER + help + Support the CE interface which is part of the Synopsis + Designware HDMI block. diff --git a/drivers/gpu/drm/bridge/synopsys/Makefile b/drivers/gpu/drm/bridge/synopsys/Makefile index 17aa7a65b57e..6fe415903668 100644 --- a/drivers/gpu/drm/bridge/synopsys/Makefile +++ b/drivers/gpu/drm/bridge/synopsys/Makefile @@ -3,3 +3,4 @@ obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o obj-$(CONFIG_DRM_DW_HDMI_I2S_AUDIO) += dw-hdmi-i2s-audio.o +obj-$(CONFIG_DRM_DW_HDMI_CEC) += dw-hdmi-cec.o diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c new file mode 100644 index 000000000000..761ef5ae687d --- /dev/null +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c @@ -0,0 +1,320 @@ +/* + * Designware HDMI CEC driver + * + * Copyright (C) 2015-2017 Russell King. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/sched.h> +#include <linux/slab.h> + +#include <drm/drm_edid.h> + +#include <media/cec.h> +#include <media/cec-notifier.h> + +#include "dw-hdmi-cec.h" + +enum { + HDMI_IH_CEC_STAT0 = 0x0106, + HDMI_IH_MUTE_CEC_STAT0 = 0x0186, + + HDMI_CEC_CTRL = 0x7d00, + CEC_CTRL_START = BIT(0), + CEC_CTRL_NORMAL = 1 << 1, + + HDMI_CEC_STAT = 0x7d01, + CEC_STAT_DONE = BIT(0), + CEC_STAT_EOM = BIT(1), + CEC_STAT_NACK = BIT(2), + CEC_STAT_ARBLOST = BIT(3), + CEC_STAT_ERROR_INIT = BIT(4), + CEC_STAT_ERROR_FOLL = BIT(5), + CEC_STAT_WAKEUP = BIT(6), + + HDMI_CEC_MASK = 0x7d02, + HDMI_CEC_POLARITY = 0x7d03, + HDMI_CEC_INT = 0x7d04, + HDMI_CEC_ADDR_L = 0x7d05, + HDMI_CEC_ADDR_H = 0x7d06, + HDMI_CEC_TX_CNT = 0x7d07, + HDMI_CEC_RX_CNT = 0x7d08, + HDMI_CEC_TX_DATA0 = 0x7d10, + HDMI_CEC_RX_DATA0 = 0x7d20, + HDMI_CEC_LOCK = 0x7d30, + HDMI_CEC_WKUPCTRL = 0x7d31, +}; + +struct dw_hdmi_cec { + struct dw_hdmi *hdmi; + const struct dw_hdmi_cec_ops *ops; + u32 addresses; + struct cec_adapter *adap; + struct cec_msg rx_msg; + unsigned int tx_status; + bool tx_done; + bool rx_done; + struct cec_notifier *notify; + int retries; + int irq; +}; + +static void dw_hdmi_write(struct dw_hdmi_cec *cec, u8 val, int offset) +{ + cec->ops->write(cec->hdmi, val, offset); +} + +static u8 dw_hdmi_read(struct dw_hdmi_cec *cec, int offset) +{ + return cec->ops->read(cec->hdmi, offset); +} + +static int dw_hdmi_cec_log_addr(struct cec_adapter *adap, u8 logical_addr) +{ + struct dw_hdmi_cec *cec = adap->priv; + u32 addresses; + + if (logical_addr == CEC_LOG_ADDR_INVALID) + addresses = cec->addresses = 0; + else + addresses = cec->addresses |= BIT(logical_addr) | BIT(15); + + dw_hdmi_write(cec, addresses & 255, HDMI_CEC_ADDR_L); + dw_hdmi_write(cec, addresses >> 8, HDMI_CEC_ADDR_H); + + return 0; +} + +static int dw_hdmi_cec_transmit(struct cec_adapter *adap, u8 attempts, + u32 signal_free_time, struct cec_msg *msg) +{ + struct dw_hdmi_cec *cec = adap->priv; + unsigned i; + + cec->retries = attempts; + + for (i = 0; i < msg->len; i++) + dw_hdmi_write(cec, msg->msg[i], HDMI_CEC_TX_DATA0 + i); + + dw_hdmi_write(cec, msg->len, HDMI_CEC_TX_CNT); + dw_hdmi_write(cec, CEC_CTRL_NORMAL | CEC_CTRL_START, HDMI_CEC_CTRL); + + return 0; +} + +static irqreturn_t dw_hdmi_cec_hardirq(int irq, void *data) +{ + struct cec_adapter *adap = data; + struct dw_hdmi_cec *cec = adap->priv; + unsigned stat = dw_hdmi_read(cec, HDMI_IH_CEC_STAT0); + irqreturn_t ret = IRQ_HANDLED; + + if (stat == 0) + return IRQ_NONE; + + dw_hdmi_write(cec, stat, HDMI_IH_CEC_STAT0); + + if (stat & CEC_STAT_ERROR_INIT) { + if (cec->retries) { + unsigned v = dw_hdmi_read(cec, HDMI_CEC_CTRL); + dw_hdmi_write(cec, v | CEC_CTRL_START, HDMI_CEC_CTRL); + cec->retries -= 1; + } else { + cec->tx_status = CEC_TX_STATUS_MAX_RETRIES; + cec->tx_done = true; + ret = IRQ_WAKE_THREAD; + } + } else if (stat & CEC_STAT_DONE) { + cec->tx_status = CEC_TX_STATUS_OK; + cec->tx_done = true; + ret = IRQ_WAKE_THREAD; + } else if (stat & CEC_STAT_NACK) { + cec->tx_status = CEC_TX_STATUS_NACK; + cec->tx_done = true; + ret = IRQ_WAKE_THREAD; + } + + if (stat & CEC_STAT_EOM) { + unsigned len, i; + + len = dw_hdmi_read(cec, HDMI_CEC_RX_CNT); + if (len > sizeof(cec->rx_msg.msg)) + len = sizeof(cec->rx_msg.msg); + + for (i = 0; i < len; i++) + cec->rx_msg.msg[i] = + dw_hdmi_read(cec, HDMI_CEC_RX_DATA0 + i); + + dw_hdmi_write(cec, 0, HDMI_CEC_LOCK); + + cec->rx_msg.len = len; + smp_wmb(); + cec->rx_done = true; + + ret = IRQ_WAKE_THREAD; + } + + return ret; +} + +static irqreturn_t dw_hdmi_cec_thread(int irq, void *data) +{ + struct cec_adapter *adap = data; + struct dw_hdmi_cec *cec = adap->priv; + + if (cec->tx_done) { + cec->tx_done = false; + cec_transmit_done(adap, cec->tx_status, 0, 0, 0, 0); + } + if (cec->rx_done) { + cec->rx_done = false; + smp_rmb(); + cec_received_msg(adap, &cec->rx_msg); + } + return IRQ_HANDLED; +} + +static int dw_hdmi_cec_enable(struct cec_adapter *adap, bool enable) +{ + struct dw_hdmi_cec *cec = adap->priv; + + if (!enable) { + dw_hdmi_write(cec, ~0, HDMI_CEC_MASK); + dw_hdmi_write(cec, ~0, HDMI_IH_MUTE_CEC_STAT0); + dw_hdmi_write(cec, 0, HDMI_CEC_POLARITY); + + cec->ops->disable(cec->hdmi); + } else { + unsigned irqs; + + dw_hdmi_write(cec, 0, HDMI_CEC_CTRL); + dw_hdmi_write(cec, ~0, HDMI_IH_CEC_STAT0); + dw_hdmi_write(cec, 0, HDMI_CEC_LOCK); + + dw_hdmi_cec_log_addr(cec->adap, CEC_LOG_ADDR_INVALID); + + cec->ops->enable(cec->hdmi); + + irqs = CEC_STAT_ERROR_INIT | CEC_STAT_NACK | CEC_STAT_EOM | + CEC_STAT_DONE; + dw_hdmi_write(cec, irqs, HDMI_CEC_POLARITY); + dw_hdmi_write(cec, ~irqs, HDMI_CEC_MASK); + dw_hdmi_write(cec, ~irqs, HDMI_IH_MUTE_CEC_STAT0); + } + return 0; +} + +static const struct cec_adap_ops dw_hdmi_cec_ops = { + .adap_enable = dw_hdmi_cec_enable, + .adap_log_addr = dw_hdmi_cec_log_addr, + .adap_transmit = dw_hdmi_cec_transmit, +}; + +static void dw_hdmi_cec_del(void *data) +{ + struct dw_hdmi_cec *cec = data; + + cec_delete_adapter(cec->adap); +} + +static int dw_hdmi_cec_probe(struct platform_device *pdev) +{ + struct dw_hdmi_cec_data *data = dev_get_platdata(&pdev->dev); + struct dw_hdmi_cec *cec; + int ret; + + if (!data) + return -ENXIO; + + /* + * Our device is just a convenience - we want to link to the real + * hardware device here, so that userspace can see the association + * between the HDMI hardware and its associated CEC chardev. + */ + cec = devm_kzalloc(&pdev->dev, sizeof(*cec), GFP_KERNEL); + if (!cec) + return -ENOMEM; + + cec->irq = data->irq; + cec->ops = data->ops; + cec->hdmi = data->hdmi; + + platform_set_drvdata(pdev, cec); + + dw_hdmi_write(cec, 0, HDMI_CEC_TX_CNT); + dw_hdmi_write(cec, ~0, HDMI_CEC_MASK); + dw_hdmi_write(cec, ~0, HDMI_IH_MUTE_CEC_STAT0); + dw_hdmi_write(cec, 0, HDMI_CEC_POLARITY); + + cec->adap = cec_allocate_adapter(&dw_hdmi_cec_ops, cec, "dw_hdmi", + CEC_CAP_LOG_ADDRS | CEC_CAP_TRANSMIT | + CEC_CAP_RC, CEC_MAX_LOG_ADDRS); + if (IS_ERR(cec->adap)) + return PTR_ERR(cec->adap); + + /* override the module pointer */ + cec->adap->owner = THIS_MODULE; + + ret = devm_add_action(&pdev->dev, dw_hdmi_cec_del, cec); + if (ret) { + cec_delete_adapter(cec->adap); + return ret; + } + + ret = devm_request_threaded_irq(&pdev->dev, cec->irq, + dw_hdmi_cec_hardirq, + dw_hdmi_cec_thread, IRQF_SHARED, + "dw-hdmi-cec", cec->adap); + if (ret < 0) + return ret; + + cec->notify = cec_notifier_get(pdev->dev.parent); + if (!cec->notify) + return -ENOMEM; + + ret = cec_register_adapter(cec->adap, pdev->dev.parent); + if (ret < 0) { + cec_notifier_put(cec->notify); + return ret; + } + + /* + * CEC documentation says we must not call cec_delete_adapter + * after a successful call to cec_register_adapter(). + */ + devm_remove_action(&pdev->dev, dw_hdmi_cec_del, cec); + + cec_register_cec_notifier(cec->adap, cec->notify); + + return 0; +} + +static int dw_hdmi_cec_remove(struct platform_device *pdev) +{ + struct dw_hdmi_cec *cec = platform_get_drvdata(pdev); + + cec_unregister_adapter(cec->adap); + cec_notifier_put(cec->notify); + + return 0; +} + +static struct platform_driver dw_hdmi_cec_driver = { + .probe = dw_hdmi_cec_probe, + .remove = dw_hdmi_cec_remove, + .driver = { + .name = "dw-hdmi-cec", + }, +}; +module_platform_driver(dw_hdmi_cec_driver); + +MODULE_AUTHOR("Russell King <rmk+kernel@armlinux.org.uk>"); +MODULE_DESCRIPTION("Synopsis Designware HDMI CEC driver for i.MX"); +MODULE_LICENSE("GPL"); +MODULE_ALIAS(PLATFORM_MODULE_PREFIX "dw-hdmi-cec"); diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h new file mode 100644 index 000000000000..cf4dc121a2c4 --- /dev/null +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h @@ -0,0 +1,19 @@ +#ifndef DW_HDMI_CEC_H +#define DW_HDMI_CEC_H + +struct dw_hdmi; + +struct dw_hdmi_cec_ops { + void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); + u8 (*read)(struct dw_hdmi *hdmi, int offset); + void (*enable)(struct dw_hdmi *hdmi); + void (*disable)(struct dw_hdmi *hdmi); +}; + +struct dw_hdmi_cec_data { + struct dw_hdmi *hdmi; + const struct dw_hdmi_cec_ops *ops; + int irq; +}; + +#endif diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 58781d4c1034..5a389f1f58d9 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -35,6 +35,7 @@ #include "dw-hdmi.h" #include "dw-hdmi-audio.h" +#include "dw-hdmi-cec.h" #include <media/cec-notifier.h> @@ -133,6 +134,7 @@ struct dw_hdmi { unsigned int version; struct platform_device *audio; + struct platform_device *cec; struct device *dev; struct clk *isfr_clk; struct clk *iahb_clk; @@ -1766,7 +1768,6 @@ static void initialize_hdmi_ih_mutes(struct dw_hdmi *hdmi) hdmi_writeb(hdmi, 0xff, HDMI_AUD_HBR_MASK); hdmi_writeb(hdmi, 0xff, HDMI_GP_MASK); hdmi_writeb(hdmi, 0xff, HDMI_A_APIINTMSK); - hdmi_writeb(hdmi, 0xff, HDMI_CEC_MASK); hdmi_writeb(hdmi, 0xff, HDMI_I2CM_INT); hdmi_writeb(hdmi, 0xff, HDMI_I2CM_CTLINT); @@ -2223,6 +2224,29 @@ static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi) return -ENODEV; } +static void dw_hdmi_cec_enable(struct dw_hdmi *hdmi) +{ + mutex_lock(&hdmi->mutex); + hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CECCLK_DISABLE; + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); + mutex_unlock(&hdmi->mutex); +} + +static void dw_hdmi_cec_disable(struct dw_hdmi *hdmi) +{ + mutex_lock(&hdmi->mutex); + hdmi->mc_clkdis |= HDMI_MC_CLKDIS_CECCLK_DISABLE; + hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS); + mutex_unlock(&hdmi->mutex); +} + +static const struct dw_hdmi_cec_ops dw_hdmi_cec_ops = { + .write = hdmi_writeb, + .read = hdmi_readb, + .enable = dw_hdmi_cec_enable, + .disable = dw_hdmi_cec_disable, +}; + static const struct regmap_config hdmi_regmap_8bit_config = { .reg_bits = 32, .val_bits = 8, @@ -2245,6 +2269,7 @@ __dw_hdmi_probe(struct platform_device *pdev, struct device_node *np = dev->of_node; struct platform_device_info pdevinfo; struct device_node *ddc_node; + struct dw_hdmi_cec_data cec; struct dw_hdmi *hdmi; struct resource *iores = NULL; int irq; @@ -2445,6 +2470,17 @@ __dw_hdmi_probe(struct platform_device *pdev, hdmi->audio = platform_device_register_full(&pdevinfo); } + cec.hdmi = hdmi; + cec.ops = &dw_hdmi_cec_ops; + cec.irq = irq; + + pdevinfo.name = "dw-hdmi-cec"; + pdevinfo.data = &cec; + pdevinfo.size_data = sizeof(cec); + pdevinfo.dma_mask = 0; + + hdmi->cec = platform_device_register_full(&pdevinfo); + /* Reset HDMI DDC I2C master controller and mute I2CM interrupts */ if (hdmi->i2c) dw_hdmi_i2c_init(hdmi); @@ -2475,6 +2511,8 @@ static void __dw_hdmi_remove(struct dw_hdmi *hdmi) { if (hdmi->audio && !IS_ERR(hdmi->audio)) platform_device_unregister(hdmi->audio); + if (!IS_ERR(hdmi->cec)) + platform_device_unregister(hdmi->cec); /* Disable all interrupts */ hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
Add a CEC driver for the dw-hdmi hardware. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- drivers/gpu/drm/bridge/synopsys/Kconfig | 8 + drivers/gpu/drm/bridge/synopsys/Makefile | 1 + drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c | 320 ++++++++++++++++++++++++++ drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h | 19 ++ drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 40 +++- 5 files changed, 387 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.c create mode 100644 drivers/gpu/drm/bridge/synopsys/dw-hdmi-cec.h