Hans Verkuil <hverk...@xs4all.nl> writes: > From: Hans Verkuil <hans.verk...@cisco.com> > > This patch adds support to VC4 for CEC. > > To prevent the firmware from eating the CEC interrupts you need to add this to > your config.txt: > > mask_gpu_interrupt1=0x100 > > Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
This looks pretty great. Just a couple of little comments.
> ---
> drivers/gpu/drm/vc4/Kconfig | 8 ++
> drivers/gpu/drm/vc4/vc4_hdmi.c | 203
> ++++++++++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/vc4/vc4_regs.h | 5 +
> 3 files changed, 211 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/Kconfig b/drivers/gpu/drm/vc4/Kconfig
> index 4361bdcfd28a..fdae18aeab4f 100644
> --- a/drivers/gpu/drm/vc4/Kconfig
> +++ b/drivers/gpu/drm/vc4/Kconfig
> @@ -19,3 +19,11 @@ config DRM_VC4
> This driver requires that "avoid_warnings=2" be present in
> the config.txt for the firmware, to keep it from smashing
> our display setup.
> +
> +config DRM_VC4_HDMI_CEC
> + bool "Broadcom VC4 HDMI CEC Support"
> + depends on DRM_VC4
> + select CEC_CORE
> + help
> + Choose this option if you have a Broadcom VC4 GPU
> + and want to use CEC.
Do we need a Kconfig for this? Couldn't we just #ifdef on CEC_CORE
instead?
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index b0521e6cc281..14e2ece5db94 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> +static int vc4_hdmi_cec_adap_enable(struct cec_adapter *adap, bool enable)
> +{
> + struct vc4_dev *vc4 = cec_get_drvdata(adap);
> + u32 hsm_clock = clk_get_rate(vc4->hdmi->hsm_clock);
> + u32 cntrl1 = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
> + u32 divclk = cntrl1 & VC4_HDMI_CEC_DIV_CLK_CNT_MASK;
We should probably be setting the divider to a value of our choice,
rather than relying on whatever default value is there.
(Bonus points if we were to do this using a common clk divider, so the
rate shows up in /debug/clk/clk_summary, but I won't require that)
> + /* clock period in microseconds */
> + u32 usecs = 1000000 / (hsm_clock / divclk);
> + u32 val = HDMI_READ(VC4_HDMI_CEC_CNTRL_5);
> +
> + val &= ~(VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET |
> + VC4_HDMI_CEC_CNT_TO_4700_US_MASK |
> + VC4_HDMI_CEC_CNT_TO_4500_US_MASK);
> + val |= ((4700 / usecs) << VC4_HDMI_CEC_CNT_TO_4700_US_SHIFT) |
> + ((4500 / usecs) << VC4_HDMI_CEC_CNT_TO_4500_US_SHIFT);
> +
> + if (enable) {
> + cntrl1 &= VC4_HDMI_CEC_DIV_CLK_CNT_MASK |
> + VC4_HDMI_CEC_ADDR_MASK;
> +
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
> + VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val);
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_2,
> + ((1500 / usecs) << VC4_HDMI_CEC_CNT_TO_1500_US_SHIFT) |
> + ((1300 / usecs) << VC4_HDMI_CEC_CNT_TO_1300_US_SHIFT) |
> + ((800 / usecs) << VC4_HDMI_CEC_CNT_TO_800_US_SHIFT) |
> + ((600 / usecs) << VC4_HDMI_CEC_CNT_TO_600_US_SHIFT) |
> + ((400 / usecs) << VC4_HDMI_CEC_CNT_TO_400_US_SHIFT));
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_3,
> + ((2750 / usecs) << VC4_HDMI_CEC_CNT_TO_2750_US_SHIFT) |
> + ((2400 / usecs) << VC4_HDMI_CEC_CNT_TO_2400_US_SHIFT) |
> + ((2050 / usecs) << VC4_HDMI_CEC_CNT_TO_2050_US_SHIFT) |
> + ((1700 / usecs) << VC4_HDMI_CEC_CNT_TO_1700_US_SHIFT));
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_4,
> + ((4300 / usecs) << VC4_HDMI_CEC_CNT_TO_4300_US_SHIFT) |
> + ((3900 / usecs) << VC4_HDMI_CEC_CNT_TO_3900_US_SHIFT) |
> + ((3600 / usecs) << VC4_HDMI_CEC_CNT_TO_3600_US_SHIFT) |
> + ((3500 / usecs) << VC4_HDMI_CEC_CNT_TO_3500_US_SHIFT));
> +
> + HDMI_WRITE(VC4_HDMI_CPU_MASK_CLEAR, VC4_HDMI_CPU_CEC);
> + } else {
> + HDMI_WRITE(VC4_HDMI_CPU_MASK_SET, VC4_HDMI_CPU_CEC);
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_5, val |
> + VC4_HDMI_CEC_TX_SW_RESET | VC4_HDMI_CEC_RX_SW_RESET);
> + }
> + return 0;
> +}
> +static int vc4_hdmi_cec_adap_transmit(struct cec_adapter *adap, u8 attempts,
> + u32 signal_free_time, struct cec_msg *msg)
> +{
> + struct vc4_dev *vc4 = cec_get_drvdata(adap);
> + u32 val;
> + unsigned int i;
> +
> + for (i = 0; i < msg->len; i += 4)
> + HDMI_WRITE(VC4_HDMI_CEC_TX_DATA_1 + i,
> + (msg->msg[i]) |
> + (msg->msg[i + 1] << 8) |
> + (msg->msg[i + 2] << 16) |
> + (msg->msg[i + 3] << 24));
> +
> + val = HDMI_READ(VC4_HDMI_CEC_CNTRL_1);
> + val &= ~VC4_HDMI_CEC_START_XMIT_BEGIN;
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
> + val &= ~VC4_HDMI_CEC_MESSAGE_LENGTH_MASK;
> + val |= (msg->len - 1) << VC4_HDMI_CEC_MESSAGE_LENGTH_SHIFT;
> + val |= VC4_HDMI_CEC_START_XMIT_BEGIN;
It doesn't look to me like len should have 1 subtracted from it. The
field has 4 bits for our up-to-16-byte length, and the firmware seems to
be setting it to the same value as a memcpy for the message data uses.
> +
> + HDMI_WRITE(VC4_HDMI_CEC_CNTRL_1, val);
> + return 0;
> +}
> +
> +static const struct cec_adap_ops vc4_hdmi_cec_adap_ops = {
> + .adap_enable = vc4_hdmi_cec_adap_enable,
> + .adap_log_addr = vc4_hdmi_cec_adap_log_addr,
> + .adap_transmit = vc4_hdmi_cec_adap_transmit,
> +};
> +#endif
> diff --git a/drivers/gpu/drm/vc4/vc4_regs.h b/drivers/gpu/drm/vc4/vc4_regs.h
> index b18cc20ee185..55677bd50f66 100644
> --- a/drivers/gpu/drm/vc4/vc4_regs.h
> +++ b/drivers/gpu/drm/vc4/vc4_regs.h
> @@ -595,6 +595,7 @@
> # define VC4_HDMI_CEC_ADDR_MASK VC4_MASK(15, 12)
> # define VC4_HDMI_CEC_ADDR_SHIFT 12
> /* Divides off of HSM clock to generate CEC bit clock. */
> +/* With the current defaults the CEC bit clock is 40 kHz = 25 usec */
> # define VC4_HDMI_CEC_DIV_CLK_CNT_MASK VC4_MASK(11, 0)
> # define VC4_HDMI_CEC_DIV_CLK_CNT_SHIFT 0
>
> @@ -670,6 +671,10 @@
> # define VC4_HDMI_CPU_CEC BIT(6)
> # define VC4_HDMI_CPU_HOTPLUG BIT(0)
>
> +#define VC4_HDMI_CPU_MASK_STATUS 0x34c
> +#define VC4_HDMI_CPU_MASK_SET 0x350
> +#define VC4_HDMI_CPU_MASK_CLEAR 0x354
> +
> #define VC4_HDMI_GCP(x) (0x400 + ((x) * 0x4))
> #define VC4_HDMI_RAM_PACKET(x) (0x400 + ((x) * 0x24))
> #define VC4_HDMI_PACKET_STRIDE 0x24
> --
> 2.11.0
Maybe squash these changes into the previous patch? Or we could squash
the previous patch into this one and just tack my signed-off-by on
yours. Either way's fine with me.
signature.asc
Description: PGP signature
